feat(activity-feed-v2): sync sidebar timestamps with video time format#4636
Merged
mergify[bot] merged 3 commits intoJun 18, 2026
Merged
Conversation
|
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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: 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 reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
Merged
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
kduncanhsu
force-pushed
the
kduncanhsu/sync-timestamp-to-comments
branch
from
June 16, 2026 23:38
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
jackiejou
previously approved these changes
Jun 17, 2026
jfox-box
reviewed
Jun 17, 2026
jfox-box
reviewed
Jun 17, 2026
jfox-box
reviewed
Jun 17, 2026
jfox-box
previously approved these changes
Jun 17, 2026
kduncanhsu
force-pushed
the
kduncanhsu/sync-timestamp-to-comments
branch
from
June 17, 2026 23:56
1ba5ccc to
a42a9e5
Compare
jackiejou
approved these changes
Jun 18, 2026
kduncanhsu
force-pushed
the
kduncanhsu/sync-timestamp-to-comments
branch
from
June 18, 2026 00:08
a42a9e5 to
813f456
Compare
kduncanhsu
force-pushed
the
kduncanhsu/sync-timestamp-to-comments
branch
from
June 18, 2026 00:22
813f456 to
bfc56c0
Compare
kduncanhsu
force-pushed
the
kduncanhsu/sync-timestamp-to-comments
branch
from
June 18, 2026 00:24
bfc56c0 to
ffd7ec5
Compare
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.
kduncanhsu
force-pushed
the
kduncanhsu/sync-timestamp-to-comments
branch
from
June 18, 2026 00:43
ffd7ec5 to
84e2f4c
Compare
Contributor
Merge Queue Status
This pull request spent 11 seconds in the queue, including 1 second running CI. Required conditions to merge
|
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
4 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
New Features
Tests