feat(activity-feed-v2): wire video timestamp toggle for main composer#4620
Open
jackiejou wants to merge 3 commits into
Open
feat(activity-feed-v2): wire video timestamp toggle for main composer#4620jackiejou wants to merge 3 commits into
jackiejou wants to merge 3 commits into
Conversation
Adds useVideoTimestamp hook implementing the spec state machine and wires it through ActivityFeedV2 to the published Editor videoTimestamp prop. Gated by features.activityFeed.timestampedComments.enabled and file extension membership in FILE_EXTENSIONS.video.
- Drop debug console.log statements left in for local devpod testing - Move isTimestampedCommentsEnabled to alphabetical position in props - Gate allowVideoTimestamps on fileVersionId so the toggle is hidden when the file version is not yet loaded; eliminates the silent strip-on-post - Reset hook state when transitioning from enabled to disabled so file switches do not leak pressed state or captured ms - Make onPressedChange a no-op when disabled - Re-attach pause/seek/loadstart/loadeddata listeners when the video element is replaced; element swaps were previously invisible to loadstart and leaked listeners on the orphan - Add loadstart->loadeddata guard so mid-load currentTime jitter does not clobber the just-reset value - Replace getTimestampMs callback with a timestampMs number field; removes the useMemo rebuild in the consumer - Trim file header and field-level JSDoc on the hook to match the convention of sibling files - Replace the vacuous playing-event test with a non-subscribed event sanity test - Add tests for: press-while-disabled, fileVersionId missing, loadstart/loadeddata mid-load capture skip, enabled->disabled reset
Contributor
WalkthroughAdds timestamped video comment support to ActivityFeedV2: new types and transformer parsing, a DOM-driven useVideoTimestamp hook with seek helper and tests, editor wiring that conditionally prefixes posts with timestamp/version markup, UI badge seeking, and feature-flag wiring from ActivitySidebar. ChangesTimestamped Video Comments in Activity Feed
Sequence DiagramsequenceDiagram
participant User
participant Editor as ActivityFeed.Editor
participant Hook as useVideoTimestamp
participant Feed as ActivityFeedV2
participant API
User->>Editor: Toggle timestamp (onPressedChange)
Editor->>Hook: set isPressed=true
Hook->>Hook: capture video currentTime (ms)
User->>Editor: Submit comment
Editor->>Feed: onPost(serializedText)
Feed->>Feed: check isTimestampedCommentsEnabled && file is video && file_version.id
Feed->>Feed: prefix text with #[timestamp:ms,versionId:id] when selected
Feed->>API: post comment with (possibly) prefixed text
API-->>Feed: comment posted
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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: 2
🧹 Nitpick comments (2)
src/elements/content-sidebar/activity-feed-v2/__tests__/useVideoTimestamp.test.tsx (1)
205-240: ⚡ Quick winAdd a regression test for video-element replacement re-binding.
This suite verifies load/reset behavior on a single
<video>instance, but it doesn’t assert theMutationObserverlistener migration path when the preview replaces the<video>node. That path is core to file-switch behavior and should be locked by a focused test.Suggested test shape
+ test('should re-bind listeners when the video element is replaced', async () => { + const container = document.createElement('div'); + container.className = 'bp-media-container'; + const first = createVideoElement(1); + container.appendChild(first); + document.body.appendChild(container); + try { + render(<TestHarness enabled />); + act(() => screen.getByText('press').click()); + + const second = createVideoElement(0); + container.replaceChildren(second); + await act(async () => {}); // flush MutationObserver callback + + Object.defineProperty(second, 'currentTime', { configurable: true, value: 9, writable: true }); + act(() => second.dispatchEvent(new Event('pause'))); + expect(screen.getByTestId('timestamp').textContent).toBe('0:09'); + } finally { + container.remove(); + } + });🤖 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__/useVideoTimestamp.test.tsx` around lines 205 - 240, Add a focused regression test in the same file that verifies the MutationObserver re-binds when the preview replaces the <video> node: create an initial video via createVideoElement and mount it with mountVideoInDom, render TestHarness, then simulate replacing the video DOM node with a new video element (preserving the same container) and assert that the library (useVideoTimestamp / the timestamp test id) continues to receive events from the new node (dispatch play/pause/seek/loadstart/loadeddata on the new element and assert timestamp updates match expected values). Ensure the test covers the re-binding path by replacing the node after initial setup and before firing events that should be observed, and clean up with the existing cleanup function.src/elements/content-sidebar/activity-feed-v2/__tests__/ActivityFeedV2.test.tsx (1)
470-607: ⚡ Quick winAdd a regression test for reply-composer exclusion.
This block only asserts the main
ActivityFeed.Editorpath. The PR contract also says reply composers must never receivevideoTimestamp, so it would be worth locking that down here; otherwise a future prop-forward inFeedItemRowcan regress without tripping this suite.🤖 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__/ActivityFeedV2.test.tsx` around lines 470 - 607, Add a regression test that ensures reply composers never receive the videoTimestamp prop: extend the existing "video timestamp" suite (using mountVideo and lastEditorProps) to render ActivityFeedV2 with a feedItems array that includes a reply entry (so FeedItemRow will mount a reply composer) and assert that the reply editor's props.videoTimestamp is undefined; this mirrors the existing main-editor assertions but specifically targets the reply-path to prevent future prop-forward regressions in FeedItemRow / reply composer rendering.
🤖 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/elements/content-sidebar/activity-feed-v2/ActivityFeedV2.tsx`:
- Around line 286-307: The code injects fileVersionId verbatim into timestamp
markup, but extractTimestampMarkup expects numeric versionId, so
validate/sanitize fileVersionId before enabling or inserting timestamps: in the
block around fileVersionId, allowVideoTimestamps, and handleCommentPost, verify
fileVersionId matches /^\d+$/ (or coerce to a safe numeric string) and only set
allowVideoTimestamps true when that check passes; then use the sanitized numeric
version id (e.g., sanitizedVersionId) when composing the timestamp token with
timestampMs and isTimestampPressed in handleCommentPost, otherwise omit the
timestamp directive.
In `@src/elements/content-sidebar/activity-feed-v2/useVideoTimestamp.ts`:
- Around line 50-77: The issue is that isPressedRef.current is not updated
together with the state changes, so handlers like handlePauseOrSeek can see
stale ref values; update isPressedRef.current synchronously wherever you call
setIsPressed or reset state: in the enabled reset effect (when !enabled) set
isPressedRef.current = false alongside setIsPressed(false) and
setTimestampMs(0), and inside onPressedChange set isPressedRef.current = true
immediately when setting setIsPressed(true) and set isPressedRef.current = false
immediately when you setIsPressed(false) (before early returns). This keeps
isPressedRef in sync with the visible toggle and prevents race conditions in
handlePauseOrSeek.
---
Nitpick comments:
In
`@src/elements/content-sidebar/activity-feed-v2/__tests__/ActivityFeedV2.test.tsx`:
- Around line 470-607: Add a regression test that ensures reply composers never
receive the videoTimestamp prop: extend the existing "video timestamp" suite
(using mountVideo and lastEditorProps) to render ActivityFeedV2 with a feedItems
array that includes a reply entry (so FeedItemRow will mount a reply composer)
and assert that the reply editor's props.videoTimestamp is undefined; this
mirrors the existing main-editor assertions but specifically targets the
reply-path to prevent future prop-forward regressions in FeedItemRow / reply
composer rendering.
In
`@src/elements/content-sidebar/activity-feed-v2/__tests__/useVideoTimestamp.test.tsx`:
- Around line 205-240: Add a focused regression test in the same file that
verifies the MutationObserver re-binds when the preview replaces the <video>
node: create an initial video via createVideoElement and mount it with
mountVideoInDom, render TestHarness, then simulate replacing the video DOM node
with a new video element (preserving the same container) and assert that the
library (useVideoTimestamp / the timestamp test id) continues to receive events
from the new node (dispatch play/pause/seek/loadstart/loadeddata on the new
element and assert timestamp updates match expected values). Ensure the test
covers the re-binding path by replacing the node after initial setup and before
firing events that should be observed, and clean up with the existing cleanup
function.
🪄 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: bb96a94a-9675-4f35-b5cd-3b508eb0b5ce
📒 Files selected for processing (6)
src/elements/content-sidebar/ActivitySidebar.jssrc/elements/content-sidebar/activity-feed-v2/ActivityFeedV2.tsxsrc/elements/content-sidebar/activity-feed-v2/__tests__/ActivityFeedV2.test.tsxsrc/elements/content-sidebar/activity-feed-v2/__tests__/useVideoTimestamp.test.tsxsrc/elements/content-sidebar/activity-feed-v2/types.tssrc/elements/content-sidebar/activity-feed-v2/useVideoTimestamp.ts
Comment on lines
+286
to
+307
| const fileVersionId = file?.file_version?.id; | ||
| const allowVideoTimestamps = isVideo && isTimestampedCommentsEnabled && Boolean(fileVersionId); | ||
|
|
||
| const { | ||
| formattedTimestamp, | ||
| isPressed: isTimestampPressed, | ||
| onPressedChange, | ||
| timestampMs, | ||
| } = useVideoTimestamp(allowVideoTimestamps); | ||
|
|
||
| const editorVideoTimestamp = allowVideoTimestamps | ||
| ? { formattedTimestamp, isPressed: isTimestampPressed, onPressedChange } | ||
| : undefined; | ||
|
|
||
| const handleCommentPost = React.useCallback( | ||
| async (content: unknown) => { | ||
| if (!onCommentCreate) return; | ||
| const serialized = serializeEditorContent(content); | ||
| if (!serialized || !serialized.text) return; | ||
| const text = | ||
| allowVideoTimestamps && isTimestampPressed && fileVersionId | ||
| ? `#[timestamp:${timestampMs},versionId:${fileVersionId}] ${serialized.text}` |
Contributor
There was a problem hiding this comment.
Validate fileVersionId against the timestamp markup contract.
extractTimestampMarkup only recognizes versionId:\d+, but this path enables timestamping for any truthy file.file_version.id and injects it verbatim into the comment text. If an alphanumeric id reaches this prop, the renderer will not strip the directive and users will see raw #[timestamp:...,versionId:...] text in the comment body.
Suggested fix
- const fileVersionId = file?.file_version?.id;
- const allowVideoTimestamps = isVideo && isTimestampedCommentsEnabled && Boolean(fileVersionId);
+ const fileVersionId = file?.file_version?.id;
+ const supportedFileVersionId =
+ typeof fileVersionId === 'string' && /^\d+$/.test(fileVersionId) ? fileVersionId : undefined;
+ const allowVideoTimestamps = isVideo && isTimestampedCommentsEnabled && Boolean(supportedFileVersionId);
@@
- allowVideoTimestamps && isTimestampPressed && fileVersionId
- ? `#[timestamp:${timestampMs},versionId:${fileVersionId}] ${serialized.text}`
+ allowVideoTimestamps && isTimestampPressed
+ ? `#[timestamp:${timestampMs},versionId:${supportedFileVersionId}] ${serialized.text}`
: serialized.text;
@@
- [allowVideoTimestamps, filteredItems, fileVersionId, isTimestampPressed, onCommentCreate, timestampMs],
+ [allowVideoTimestamps, filteredItems, isTimestampPressed, onCommentCreate, supportedFileVersionId, timestampMs],📝 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 fileVersionId = file?.file_version?.id; | |
| const allowVideoTimestamps = isVideo && isTimestampedCommentsEnabled && Boolean(fileVersionId); | |
| const { | |
| formattedTimestamp, | |
| isPressed: isTimestampPressed, | |
| onPressedChange, | |
| timestampMs, | |
| } = useVideoTimestamp(allowVideoTimestamps); | |
| const editorVideoTimestamp = allowVideoTimestamps | |
| ? { formattedTimestamp, isPressed: isTimestampPressed, onPressedChange } | |
| : undefined; | |
| const handleCommentPost = React.useCallback( | |
| async (content: unknown) => { | |
| if (!onCommentCreate) return; | |
| const serialized = serializeEditorContent(content); | |
| if (!serialized || !serialized.text) return; | |
| const text = | |
| allowVideoTimestamps && isTimestampPressed && fileVersionId | |
| ? `#[timestamp:${timestampMs},versionId:${fileVersionId}] ${serialized.text}` | |
| const fileVersionId = file?.file_version?.id; | |
| const supportedFileVersionId = | |
| typeof fileVersionId === 'string' && /^\d+$/.test(fileVersionId) ? fileVersionId : undefined; | |
| const allowVideoTimestamps = isVideo && isTimestampedCommentsEnabled && Boolean(supportedFileVersionId); | |
| const { | |
| formattedTimestamp, | |
| isPressed: isTimestampPressed, | |
| onPressedChange, | |
| timestampMs, | |
| } = useVideoTimestamp(allowVideoTimestamps); | |
| const editorVideoTimestamp = allowVideoTimestamps | |
| ? { formattedTimestamp, isPressed: isTimestampPressed, onPressedChange } | |
| : undefined; | |
| const handleCommentPost = React.useCallback( | |
| async (content: unknown) => { | |
| if (!onCommentCreate) return; | |
| const serialized = serializeEditorContent(content); | |
| if (!serialized || !serialized.text) return; | |
| const text = | |
| allowVideoTimestamps && isTimestampPressed | |
| ? `#[timestamp:${timestampMs},versionId:${supportedFileVersionId}] ${serialized.text}` | |
| : serialized.text; | |
| }, | |
| [allowVideoTimestamps, filteredItems, isTimestampPressed, onCommentCreate, supportedFileVersionId, timestampMs], | |
| ); |
🤖 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/ActivityFeedV2.tsx` around
lines 286 - 307, The code injects fileVersionId verbatim into timestamp markup,
but extractTimestampMarkup expects numeric versionId, so validate/sanitize
fileVersionId before enabling or inserting timestamps: in the block around
fileVersionId, allowVideoTimestamps, and handleCommentPost, verify fileVersionId
matches /^\d+$/ (or coerce to a safe numeric string) and only set
allowVideoTimestamps true when that check passes; then use the sanitized numeric
version id (e.g., sanitizedVersionId) when composing the timestamp token with
timestampMs and isTimestampPressed in handleCommentPost, otherwise omit the
timestamp directive.
… review - Wire onAnnotationBadgeClick on comment rows to a new seekVideoToMs helper so clicking a comment timestamp badge seeks the video and pauses, matching v1 behavior. Plumbs the raw ms through extractTimestampMarkup -> transformFeedItem as annotationTimestampMs. - Sync isPressedRef synchronously with setIsPressed in onPressedChange and the disable-effect so a pause or seek firing in the same tick as an unpress no longer captures a stale value.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/elements/content-sidebar/activity-feed-v2/FeedItemRow.tsx (1)
132-133: 💤 Low valueConsider memoizing the badge click handler with useCallback.
The handler is recreated on every render. While this follows the existing pattern in this file (other handlers like
handleDelete,handleStatusChange,handleEditare also inline), wrapping it inuseCallbackwould prevent unnecessary recreation whentimestampMshasn't changed.♻️ Optional optimization
+ const handleBadgeClick = React.useCallback( + timestampMs !== undefined ? () => seekVideoToMs(timestampMs) : undefined, + [timestampMs] + ); - const timestampMs = item.annotationTimestampMs; - const handleBadgeClick = timestampMs !== undefined ? () => seekVideoToMs(timestampMs) : undefined;Note: This optimization is minor since the item prop changes would trigger re-renders anyway. The current approach is acceptable and consistent with the rest of the file.
🤖 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/FeedItemRow.tsx` around lines 132 - 133, The inline badge click handler is recreated on every render; wrap its creation in React.useCallback to memoize it so it only changes when item.annotationTimestampMs or seekVideoToMs change. Replace the inline assignment for handleBadgeClick with a useCallback that returns () => seekVideoToMs(timestampMs) when timestampMs !== undefined and undefined otherwise, referencing the existing symbols handleBadgeClick, seekVideoToMs, and item.annotationTimestampMs to locate the change.
🤖 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.
Nitpick comments:
In `@src/elements/content-sidebar/activity-feed-v2/FeedItemRow.tsx`:
- Around line 132-133: The inline badge click handler is recreated on every
render; wrap its creation in React.useCallback to memoize it so it only changes
when item.annotationTimestampMs or seekVideoToMs change. Replace the inline
assignment for handleBadgeClick with a useCallback that returns () =>
seekVideoToMs(timestampMs) when timestampMs !== undefined and undefined
otherwise, referencing the existing symbols handleBadgeClick, seekVideoToMs, and
item.annotationTimestampMs to locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 53551d22-7e16-465d-bcfd-2aee5a344e32
📒 Files selected for processing (7)
src/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__/transformers.test.tssrc/elements/content-sidebar/activity-feed-v2/__tests__/useVideoTimestamp.test.tsxsrc/elements/content-sidebar/activity-feed-v2/transformers.tssrc/elements/content-sidebar/activity-feed-v2/types.tssrc/elements/content-sidebar/activity-feed-v2/useVideoTimestamp.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/elements/content-sidebar/activity-feed-v2/types.ts
- src/elements/content-sidebar/activity-feed-v2/useVideoTimestamp.ts
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
1 participant
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
useVideoTimestamphook that tracks the playback time of the preview video and drives the editor timestamp toggle for the v2 activity feed.videoTimestampthroughActivityFeedV2to the publishedActivityFeed.Editoronly for video files when the host enablesfeatures.activityFeed.timestampedCommentsAND afile_version.idis loaded.#[timestamp:<ms>,versionId:<v>]to the editor text on post when the toggle is pressed, matching the v1 markup format read bytransformers.ts.Behavior (per spec)
Test plan
yarn test --watchAll=false --testPathPattern="useVideoTimestamp|activity-feed-v2"-> 208/208 passingyarn test --watchAll=false --testPathPattern="content-sidebar/__tests__/ActivitySidebar"-> 167/167 passingyarn lint:tscleanSummary by CodeRabbit