feat(activity-feed-v2): sync sidebar timestamps with video time format#4636
Open
kduncanhsu wants to merge 1 commit into
Open
feat(activity-feed-v2): sync sidebar timestamps with video time format#4636kduncanhsu wants to merge 1 commit into
kduncanhsu wants to merge 1 commit into
Conversation
|
Duncan Hsu seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
Contributor
WalkthroughAdds video-aware timestamp formatting to the activity feed. New ChangesVideo Timestamp Formatting
Sequence Diagram(s)sequenceDiagram
participant ActivityFeedV2
participant FeedItemRow
participant useTimeFormat
participant bp-media-container
participant formatByTimeFormat
ActivityFeedV2->>FeedItemRow: isVideo={file.extension in FILE_EXTENSIONS.video}
FeedItemRow->>useTimeFormat: useTimeFormat(isVideo)
useTimeFormat->>bp-media-container: read data-time-format, data-fps
bp-media-container-->>useTimeFormat: timeFormat, fps
useTimeFormat-->>FeedItemRow: { timeFormat, fps }
FeedItemRow->>formatByTimeFormat: formatByTimeFormat(annotationTimestampMs, timeFormat, fps)
formatByTimeFormat-->>FeedItemRow: formatted timestamp string
FeedItemRow-->>ActivityFeedV2: renders ThreadedAnnotation with formatted timestamp
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Open
3 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/utils/__tests__/timestamp.test.js (1)
93-155: ⚡ Quick winAdd fractional-FPS coverage for timecode/frame conversions.
Current cases are integer-FPS only. Since
data-fpsis parsed as a number upstream, add at least one29.97case to lock expected behavior and prevent player/sidebar drift regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/__tests__/timestamp.test.js` around lines 93 - 155, The test suite for convertMillisecondsToTimecode and convertMillisecondsToFrames lacks coverage for fractional FPS values (such as 29.97), which can be passed as numbers from upstream data-fps parsing. Add at least one test case within each describe block that tests a fractional FPS value like 29.97 to ensure the conversion functions handle non-integer frame rates correctly and prevent player/sidebar drift regressions. Include test cases with typical fractional FPS values (29.97, 23.976) and verify that the timecode and frame calculations remain accurate.src/elements/content-sidebar/activity-feed-v2/__tests__/FeedItemRow.test.tsx (1)
60-64: ⚡ Quick winReset
mockTimeFormatinbeforeEachto avoid cross-test state leakage.
mockTimeFormatis mutated in multiple tests but never reset. Add a deterministic reset inbeforeEachso tests remain order-independent.Suggested fix
beforeEach(() => { lastThreadedAnnotationProps = {}; lastTaskProps = {}; lastVersionProps = {}; + mockTimeFormat.timeFormat = 'standard'; + mockTimeFormat.fps = 24; mockedSerializeEditorContent.mockReturnValue({ hasMention: false, text: 'serialized-text' }); });Also applies to: 213-218
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/elements/content-sidebar/activity-feed-v2/__tests__/FeedItemRow.test.tsx` around lines 60 - 64, The mockTimeFormat object is being mutated across multiple tests but never reset between test runs, causing state leakage and making test execution order-dependent. Add a beforeEach block that resets mockTimeFormat back to its initial values (timeFormat: 'standard', fps: 24) before each test runs to ensure consistent and isolated test behavior. This ensures the mock object starts with known values for every test regardless of what previous tests may have modified.src/elements/content-sidebar/activity-feed-v2/useTimeFormat.ts (1)
50-55: 💤 Low valueConsider validating the attribute value before casting.
The type assertion
as TimeFormaton line 51 bypasses TypeScript's type checking. If the DOM attribute contains an invalid value like"invalid", the cast will succeed but the runtime value won't match theTimeFormatunion. WhileformatByTimeFormathas a default case that safely handles this, explicit validation would be more type-safe:♻️ Suggested type-safe validation
const readAttributes = (container: Element): void => { - const format = (container.getAttribute('data-time-format') as TimeFormat) || 'standard'; + const formatAttr = container.getAttribute('data-time-format'); + const format: TimeFormat = + formatAttr === 'timecode' || formatAttr === 'frames' || formatAttr === 'standard' + ? formatAttr + : 'standard'; const fpsAttr = Number(container.getAttribute('data-fps')); setTimeFormat(format); setFps(fpsAttr > 0 ? fpsAttr : DEFAULT_FPS); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/elements/content-sidebar/activity-feed-v2/useTimeFormat.ts` around lines 50 - 55, In the readAttributes function, the getAttribute call for 'data-time-format' uses a direct type assertion with `as TimeFormat` which bypasses TypeScript type checking. Replace this unsafe cast by validating that the retrieved attribute value is actually a valid TimeFormat value before using it. Create a type guard or validation check that confirms the attribute value matches one of the valid TimeFormat union values, and only apply the type assertion after validation passes. This ensures type safety at runtime rather than relying solely on the default case handling in downstream code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/timestamp.ts`:
- Around line 73-77: The frames calculation on line 76 uses Math.round(fps)
while the hours, minutes, and seconds calculations on lines 73-75 use the raw
fps value, creating an inconsistency that produces incorrect timecode output for
fractional frame rates like 29.97. Replace Math.round(fps) with fps in the
frames calculation to ensure all time components use the same FPS basis
throughout the timecode conversion logic.
---
Nitpick comments:
In
`@src/elements/content-sidebar/activity-feed-v2/__tests__/FeedItemRow.test.tsx`:
- Around line 60-64: The mockTimeFormat object is being mutated across multiple
tests but never reset between test runs, causing state leakage and making test
execution order-dependent. Add a beforeEach block that resets mockTimeFormat
back to its initial values (timeFormat: 'standard', fps: 24) before each test
runs to ensure consistent and isolated test behavior. This ensures the mock
object starts with known values for every test regardless of what previous tests
may have modified.
In `@src/elements/content-sidebar/activity-feed-v2/useTimeFormat.ts`:
- Around line 50-55: In the readAttributes function, the getAttribute call for
'data-time-format' uses a direct type assertion with `as TimeFormat` which
bypasses TypeScript type checking. Replace this unsafe cast by validating that
the retrieved attribute value is actually a valid TimeFormat value before using
it. Create a type guard or validation check that confirms the attribute value
matches one of the valid TimeFormat union values, and only apply the type
assertion after validation passes. This ensures type safety at runtime rather
than relying solely on the default case handling in downstream code.
In `@src/utils/__tests__/timestamp.test.js`:
- Around line 93-155: The test suite for convertMillisecondsToTimecode and
convertMillisecondsToFrames lacks coverage for fractional FPS values (such as
29.97), which can be passed as numbers from upstream data-fps parsing. Add at
least one test case within each describe block that tests a fractional FPS value
like 29.97 to ensure the conversion functions handle non-integer frame rates
correctly and prevent player/sidebar drift regressions. Include test cases with
typical fractional FPS values (29.97, 23.976) and verify that the timecode and
frame calculations remain accurate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: af5d97fb-998e-4283-8020-8511d4ce4996
📒 Files selected for processing (9)
src/elements/content-sidebar/activity-feed-v2/ActivityFeedV2.tsxsrc/elements/content-sidebar/activity-feed-v2/FeedItemRow.tsxsrc/elements/content-sidebar/activity-feed-v2/__tests__/FeedItemRow.test.tsxsrc/elements/content-sidebar/activity-feed-v2/__tests__/useTimeFormat.test.tsxsrc/elements/content-sidebar/activity-feed-v2/__tests__/useVideoTimestamp.test.tsxsrc/elements/content-sidebar/activity-feed-v2/useTimeFormat.tssrc/elements/content-sidebar/activity-feed-v2/useVideoTimestamp.tssrc/utils/__tests__/timestamp.test.jssrc/utils/timestamp.ts
Comment on lines
+73
to
+77
| const hours = Math.floor(totalFrames / (fps * 3600)); | ||
| const minutes = Math.floor((totalFrames % (fps * 3600)) / (fps * 60)); | ||
| const secs = Math.floor((totalFrames % (fps * 60)) / fps); | ||
| const frames = totalFrames % Math.round(fps); | ||
|
|
Contributor
There was a problem hiding this comment.
Inconsistent FPS basis causes incorrect timecode for fractional frame rates.
Line 73-75 decomposes time using raw fps, but Line 76 computes frames using Math.round(fps). With fractional FPS (e.g., 29.97 from data-fps), this produces drift and mismatched HH:MM:SS:FF output relative to the player format.
Suggested fix
const convertMillisecondsToTimecode = (timestampInMilliseconds: number, fps: number): string => {
const seconds = timestampInMilliseconds && timestampInMilliseconds > 0 ? timestampInMilliseconds / 1000 : 0;
const val = Number.isFinite(seconds) ? seconds : 0;
- const totalFrames = Math.floor(val * fps);
+ const frameBase = Number.isFinite(fps) && fps > 0 ? Math.round(fps) : 24;
+ const totalFrames = Math.floor(val * frameBase);
- const hours = Math.floor(totalFrames / (fps * 3600));
- const minutes = Math.floor((totalFrames % (fps * 3600)) / (fps * 60));
- const secs = Math.floor((totalFrames % (fps * 60)) / fps);
- const frames = totalFrames % Math.round(fps);
+ const hours = Math.floor(totalFrames / (frameBase * 3600));
+ const minutes = Math.floor((totalFrames % (frameBase * 3600)) / (frameBase * 60));
+ const secs = Math.floor((totalFrames % (frameBase * 60)) / frameBase);
+ const frames = totalFrames % frameBase;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/timestamp.ts` around lines 73 - 77, The frames calculation on line
76 uses Math.round(fps) while the hours, minutes, and seconds calculations on
lines 73-75 use the raw fps value, creating an inconsistency that produces
incorrect timecode output for fractional frame rates like 29.97. Replace
Math.round(fps) with fps in the frames calculation to ensure all time components
use the same FPS basis throughout the timecode conversion logic.
Sidebar comment timestamps (both the editor checkbox and posted comment badges) now respect the time format selected in the video player controls (standard, timecode, or frame numbers). Communication uses data attributes on .bp-media-container observed via MutationObserver. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
kduncanhsu
force-pushed
the
kduncanhsu/sync-timestamp-to-comments
branch
from
23e2093 to
c4307d0
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/timestamp.ts`:
- Around line 86-91: The convertMillisecondsToFrames function currently
validates the timestampInMilliseconds parameter but does not validate the fps
parameter. Add validation to check if fps is a valid positive number (not zero,
negative, NaN, or Infinity) and return 0 if the fps value is invalid. This will
prevent the calculation on line 90 from producing NaN, Infinity, or other
incorrect frame counts when fps is provided with invalid values.
- Around line 68-84: The convertMillisecondsToTimecode function does not
validate the fps parameter, which can lead to incorrect timecode calculations if
fps is zero, negative, NaN, or Infinity. Add validation at the beginning of the
function to ensure fps is a valid positive finite number, and return a default
timecode (such as "00:00:00:00") if it is not. This validation will also resolve
the Math.round inconsistency where fps is rounded only in the frames calculation
but not in the hours, minutes, and seconds calculations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c675e84b-5bad-45cd-8a15-a43d28d20e37
📒 Files selected for processing (9)
src/elements/content-sidebar/activity-feed-v2/ActivityFeedV2.tsxsrc/elements/content-sidebar/activity-feed-v2/FeedItemRow.tsxsrc/elements/content-sidebar/activity-feed-v2/__tests__/FeedItemRow.test.tsxsrc/elements/content-sidebar/activity-feed-v2/__tests__/useTimeFormat.test.tsxsrc/elements/content-sidebar/activity-feed-v2/__tests__/useVideoTimestamp.test.tsxsrc/elements/content-sidebar/activity-feed-v2/useTimeFormat.tssrc/elements/content-sidebar/activity-feed-v2/useVideoTimestamp.tssrc/utils/__tests__/timestamp.test.jssrc/utils/timestamp.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- src/elements/content-sidebar/activity-feed-v2/ActivityFeedV2.tsx
- src/elements/content-sidebar/activity-feed-v2/useVideoTimestamp.ts
- src/utils/tests/timestamp.test.js
- src/elements/content-sidebar/activity-feed-v2/useTimeFormat.ts
- src/elements/content-sidebar/activity-feed-v2/tests/useVideoTimestamp.test.tsx
- src/elements/content-sidebar/activity-feed-v2/tests/FeedItemRow.test.tsx
- src/elements/content-sidebar/activity-feed-v2/FeedItemRow.tsx
Comment on lines
+68
to
+84
| const convertMillisecondsToTimecode = (timestampInMilliseconds: number, fps: number): string => { | ||
| const seconds = timestampInMilliseconds && timestampInMilliseconds > 0 ? timestampInMilliseconds / 1000 : 0; | ||
| const val = Number.isFinite(seconds) ? seconds : 0; | ||
| const totalFrames = Math.floor(val * fps); | ||
|
|
||
| const hours = Math.floor(totalFrames / (fps * 3600)); | ||
| const minutes = Math.floor((totalFrames % (fps * 3600)) / (fps * 60)); | ||
| const secs = Math.floor((totalFrames % (fps * 60)) / fps); | ||
| const frames = totalFrames % Math.round(fps); | ||
|
|
||
| const hh = hours.toString().padStart(2, '0'); | ||
| const mm = minutes.toString().padStart(2, '0'); | ||
| const ss = secs.toString().padStart(2, '0'); | ||
| const ff = frames.toString().padStart(2, '0'); | ||
|
|
||
| return `${hh}:${mm}:${ss}:${ff}`; | ||
| }; |
Contributor
There was a problem hiding this comment.
Validate fps parameter to prevent incorrect timecode output.
Both fps parameter validation and the Math.round inconsistency flagged below will cause incorrect timecode calculations. If fps is zero, negative, NaN, or Infinity, division operations on lines 73-75 will produce Infinity, NaN, or incorrect results.
🛡️ Proposed fix to add fps validation
const convertMillisecondsToTimecode = (timestampInMilliseconds: number, fps: number): string => {
const seconds = timestampInMilliseconds && timestampInMilliseconds > 0 ? timestampInMilliseconds / 1000 : 0;
const val = Number.isFinite(seconds) ? seconds : 0;
+ const validFps = Number.isFinite(fps) && fps > 0 ? fps : 24;
- const totalFrames = Math.floor(val * fps);
+ const totalFrames = Math.floor(val * validFps);
- const hours = Math.floor(totalFrames / (fps * 3600));
- const minutes = Math.floor((totalFrames % (fps * 3600)) / (fps * 60));
- const secs = Math.floor((totalFrames % (fps * 60)) / fps);
- const frames = totalFrames % Math.round(fps);
+ const hours = Math.floor(totalFrames / (validFps * 3600));
+ const minutes = Math.floor((totalFrames % (validFps * 3600)) / (validFps * 60));
+ const secs = Math.floor((totalFrames % (validFps * 60)) / validFps);
+ const frames = totalFrames % validFps;Note: This fix also resolves the Math.round(fps) inconsistency flagged separately.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const convertMillisecondsToTimecode = (timestampInMilliseconds: number, fps: number): string => { | |
| const seconds = timestampInMilliseconds && timestampInMilliseconds > 0 ? timestampInMilliseconds / 1000 : 0; | |
| const val = Number.isFinite(seconds) ? seconds : 0; | |
| const totalFrames = Math.floor(val * fps); | |
| const hours = Math.floor(totalFrames / (fps * 3600)); | |
| const minutes = Math.floor((totalFrames % (fps * 3600)) / (fps * 60)); | |
| const secs = Math.floor((totalFrames % (fps * 60)) / fps); | |
| const frames = totalFrames % Math.round(fps); | |
| const hh = hours.toString().padStart(2, '0'); | |
| const mm = minutes.toString().padStart(2, '0'); | |
| const ss = secs.toString().padStart(2, '0'); | |
| const ff = frames.toString().padStart(2, '0'); | |
| return `${hh}:${mm}:${ss}:${ff}`; | |
| }; | |
| const convertMillisecondsToTimecode = (timestampInMilliseconds: number, fps: number): string => { | |
| const seconds = timestampInMilliseconds && timestampInMilliseconds > 0 ? timestampInMilliseconds / 1000 : 0; | |
| const val = Number.isFinite(seconds) ? seconds : 0; | |
| const validFps = Number.isFinite(fps) && fps > 0 ? fps : 24; | |
| const totalFrames = Math.floor(val * validFps); | |
| const hours = Math.floor(totalFrames / (validFps * 3600)); | |
| const minutes = Math.floor((totalFrames % (validFps * 3600)) / (validFps * 60)); | |
| const secs = Math.floor((totalFrames % (validFps * 60)) / validFps); | |
| const frames = totalFrames % validFps; | |
| const hh = hours.toString().padStart(2, '0'); | |
| const mm = minutes.toString().padStart(2, '0'); | |
| const ss = secs.toString().padStart(2, '0'); | |
| const ff = frames.toString().padStart(2, '0'); | |
| return `${hh}:${mm}:${ss}:${ff}`; | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/timestamp.ts` around lines 68 - 84, The
convertMillisecondsToTimecode function does not validate the fps parameter,
which can lead to incorrect timecode calculations if fps is zero, negative, NaN,
or Infinity. Add validation at the beginning of the function to ensure fps is a
valid positive finite number, and return a default timecode (such as
"00:00:00:00") if it is not. This validation will also resolve the Math.round
inconsistency where fps is rounded only in the frames calculation but not in the
hours, minutes, and seconds calculations.
Comment on lines
+86
to
+91
| const convertMillisecondsToFrames = (timestampInMilliseconds: number, fps: number): number => { | ||
| if (!timestampInMilliseconds || timestampInMilliseconds < 0) { | ||
| return 0; | ||
| } | ||
| return Math.floor((timestampInMilliseconds / 1000) * fps); | ||
| }; |
Contributor
There was a problem hiding this comment.
Validate fps parameter to prevent incorrect frame counts.
If fps is zero, negative, NaN, or Infinity, line 90 will produce incorrect frame counts (NaN, Infinity, or negative values).
🛡️ Proposed fix to add fps validation
const convertMillisecondsToFrames = (timestampInMilliseconds: number, fps: number): number => {
if (!timestampInMilliseconds || timestampInMilliseconds < 0) {
return 0;
}
- return Math.floor((timestampInMilliseconds / 1000) * fps);
+ const validFps = Number.isFinite(fps) && fps > 0 ? fps : 24;
+ return Math.floor((timestampInMilliseconds / 1000) * validFps);
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const convertMillisecondsToFrames = (timestampInMilliseconds: number, fps: number): number => { | |
| if (!timestampInMilliseconds || timestampInMilliseconds < 0) { | |
| return 0; | |
| } | |
| return Math.floor((timestampInMilliseconds / 1000) * fps); | |
| }; | |
| const convertMillisecondsToFrames = (timestampInMilliseconds: number, fps: number): number => { | |
| if (!timestampInMilliseconds || timestampInMilliseconds < 0) { | |
| return 0; | |
| } | |
| const validFps = Number.isFinite(fps) && fps > 0 ? fps : 24; | |
| return Math.floor((timestampInMilliseconds / 1000) * validFps); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/timestamp.ts` around lines 86 - 91, The convertMillisecondsToFrames
function currently validates the timestampInMilliseconds parameter but does not
validate the fps parameter. Add validation to check if fps is a valid positive
number (not zero, negative, NaN, or Infinity) and return 0 if the fps value is
invalid. This will prevent the calculation on line 90 from producing NaN,
Infinity, or other incorrect frame counts when fps is provided with invalid
values.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
useTimeFormathook that observesdata-time-formatanddata-fpsattributes on.bp-media-containervia MutationObserverconvertMillisecondsToTimecodeandconvertMillisecondsToFramesto timestamp utilsTest plan
useTimeFormathook tests (11 tests) — initial read, reactive updates, late container, fallbacks, disable resetuseVideoTimestampformat integration tests (4 tests) — timecode/frames formatting, live format switchFeedItemRowbadge format tests (6 tests) — comment/annotation badges update per formatconvertMillisecondsToTimecodeandconvertMillisecondsToFramesSummary by CodeRabbit
Release Notes