feat(activity-feed-v2): emit timestamped comment markers to video player#4651
feat(activity-feed-v2): emit timestamped comment markers to video player#4651kduncanhsu wants to merge 5 commits into
Conversation
|
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:
WalkthroughActivity sidebar routing now passes an optional viewer getter into ActivityFeedV2. The feed emits video comment markers from filtered items, listens for marker selection, and scrolls matching activity entries into view. Tests cover marker payloads, filtering, and listener cleanup. ChangesActivity feed viewer markers
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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/elements/content-sidebar/activity-feed-v2/__tests__/ActivityFeedV2.test.tsx (1)
1141-1145: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that cleanup removes the same callback instance.
This test still passes if
removeListenergets a different function than the one registered inaddListener. Capture the listener passed at registration time and assert cleanup removes that exact reference, so leaks in this path are caught.🤖 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 1141 - 1145, The cleanup test for the commentmarkerselect listener is too weak because it only checks that removeListener is called with some function, not the exact one that was registered. Update the ActivityFeedV2 test to capture the callback passed to mockViewer.addListener during renderComponentWithMarkers and assert unmount calls mockViewer.removeListener with that same function reference. Use the existing addListener/removeListener behavior in ActivityFeedV2 to verify the registered listener is the one cleaned up.
🤖 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 350-360: Clear the viewer’s marker state during teardown because
the current cleanup in ActivityFeedV2 only removes the commentmarkerselect
listener and leaves the last commentmarkers payload behind. Update the marker
lifecycle around viewer.emit('commentmarkers', markers) and the returned cleanup
so unmounting or stopping the feed also emits an empty marker set (or equivalent
reset) before detaching the listener. Keep marker emission separate from the
listener setup in handleMarkerSelect so normal filter updates don’t briefly
clear and re-add markers.
---
Nitpick comments:
In
`@src/elements/content-sidebar/activity-feed-v2/__tests__/ActivityFeedV2.test.tsx`:
- Around line 1141-1145: The cleanup test for the commentmarkerselect listener
is too weak because it only checks that removeListener is called with some
function, not the exact one that was registered. Update the ActivityFeedV2 test
to capture the callback passed to mockViewer.addListener during
renderComponentWithMarkers and assert unmount calls mockViewer.removeListener
with that same function reference. Use the existing addListener/removeListener
behavior in ActivityFeedV2 to verify the registered listener is the one cleaned
up.
🪄 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: 68221e7c-15c9-4207-a9a4-cb2b9741a893
📒 Files selected for processing (5)
src/elements/content-sidebar/ActivitySidebar.jssrc/elements/content-sidebar/SidebarPanels.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/types.ts
| React.useEffect(() => { | ||
| if (!getViewer || !isVideo) return undefined; | ||
| const viewer = getViewer(); | ||
| if (!viewer) return undefined; | ||
|
|
||
| const markers: Array<{ | ||
| avatarUrl?: string; | ||
| colorIndex?: number; | ||
| id: string; | ||
| initial?: string; | ||
| time: number; | ||
| type: 'annotation' | 'comment'; | ||
| }> = []; | ||
| for (const item of filteredItems) { | ||
| if (item.type === 'comment' && item.annotationTimestampMs != null) { | ||
| const author = item.messages[0]?.author; | ||
| markers.push({ | ||
| avatarUrl: author?.avatarUrl ?? undefined, | ||
| colorIndex: author?.id ?? 0, | ||
| id: item.id, | ||
| initial: author?.name?.[0] ?? undefined, | ||
| time: item.annotationTimestampMs / 1000, | ||
| type: 'comment', | ||
| }); | ||
| } else if (item.type === 'annotation') { | ||
| const loc = item.annotation?.target?.location; | ||
| if (loc?.type === 'frame' && loc.value != null) { | ||
| const author = item.messages[0]?.author; | ||
| markers.push({ | ||
| avatarUrl: author?.avatarUrl ?? undefined, | ||
| colorIndex: author?.id ?? 0, | ||
| id: item.id, | ||
| initial: author?.name?.[0] ?? undefined, | ||
| time: loc.value / 1000, | ||
| type: 'annotation', | ||
| }); | ||
| } | ||
| } | ||
| } | ||
| viewer.emit('commentmarkers', markers); | ||
|
|
||
| const handleMarkerSelect = ({ id }: { id: string }) => { | ||
| requestAnimationFrame(() => { | ||
| const el = document.querySelector(`[data-activity-id="${CSS.escape(id)}"]`); | ||
| if (el) { | ||
| el.scrollIntoView({ behavior: 'smooth', block: 'start' }); | ||
| } | ||
| }); | ||
| }; | ||
| viewer.addListener('commentmarkerselect', handleMarkerSelect); | ||
| return () => { | ||
| viewer.removeListener('commentmarkerselect', handleMarkerSelect); | ||
| }; | ||
| }, [filteredItems, getViewer, isVideo]); |
There was a problem hiding this comment.
🟡 useEffect deps include filteredItems directly. filteredItems is rebuilt every render that touches the comment/task/annotation state (filter selection, pagination, mutation, even a re-mount in <ContentSidebar>), and array identity changes on every recompute. Result: the markers array is rebuilt, the listener is removed + re-added, and the viewer re-emits commentmarkers on every parent render — even when no timestamped item changed.
Two cheaper fixes:
- Memoize the marker array on a stable signature (only the fields you actually emit):
useMemokeyed onJSON.stringify(filteredItems.map(i => [i.id, i.annotationTimestampMs, i.annotation?.target?.location]))would. - Split the listener-attach from the emit: the listener only needs
getViewer + isVideo; the emit only needs the markers. TwouseEffects with appropriately tight deps.
Not load-bearing — emit is microseconds — but the listener-detach-and-reattach on every parent render is the sharper edge.
There was a problem hiding this comment.
just noticed a bug with this change where comments stop scrolling correctly when a marker is selected.
Reason:
Splitting the useEffect into two separate effects didn't work is because getViewer() returns null on initial mount (viewer hasn't loaded yet), and the listener effect with [getViewer, isVideo] deps never re-runs because getViewer is a stable function reference — the effect doesn't know that getViewer()'s return value changed. So marker select events were never being received. The single effect works because filteredItems changes when feed items arrive (after the viewer is loaded), forcing the effect to re-run at a point where getViewer() returns the actual viewer.
I have since reverted this change, could I get a re-review?
kduncanhsu
force-pushed
the
kduncanhsu/annotations-in-scrubber-bar-plumbing
branch
from
0cbfbee to
3539750
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/elements/content-sidebar/activity-feed-v2/ActivityFeedV2.tsx (1)
359-374: 🚀 Performance & Scalability | 🔵 TrivialConfirm
getVieweridentity stability to prevent effect churnThe effect at lines 359–374 depends on
getViewer. Tracing the component tree inSidebarPanelsandActivitySidebarshowsgetVieweris passed down as a prop (e.g., line 1429 inActivitySidebar) rather than defined inline. If the upstream parent recreates thegetViewerfunction on every render (e.g., inline() => this.viewer.getViewer()), this effect will tear down and re-attach the listener on every update. The cleanup function emitscommentmarkers: []during this churn, potentially causing scrubber marker flicker. Consider moving the effect inside a child wrapper that receives a stableuseCallbackfactory oruseMemo-izedgetViewer.🤖 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 359 - 374, The ActivityFeedV2 effect that registers the `commentmarkerselect` listener is churn-prone because it depends on `getViewer`, which may be recreated by upstream props from `ActivitySidebar`/`SidebarPanels`. Make `getViewer` stable before it reaches `ActivityFeedV2` (for example by wrapping the factory in `useCallback` or memoizing the prop) so the `React.useEffect` in `ActivityFeedV2` only reattaches when the actual viewer changes. Keep the listener setup/cleanup in sync with the stable viewer identity to avoid repeated `commentmarkers` clears and marker flicker.
🤖 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/ActivityFeedV2.tsx`:
- Around line 359-374: The ActivityFeedV2 effect that registers the
`commentmarkerselect` listener is churn-prone because it depends on `getViewer`,
which may be recreated by upstream props from `ActivitySidebar`/`SidebarPanels`.
Make `getViewer` stable before it reaches `ActivityFeedV2` (for example by
wrapping the factory in `useCallback` or memoizing the prop) so the
`React.useEffect` in `ActivityFeedV2` only reattaches when the actual viewer
changes. Keep the listener setup/cleanup in sync with the stable viewer identity
to avoid repeated `commentmarkers` clears and marker flicker.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8f500df1-0788-4f60-bfb4-ab2deee7bfae
📒 Files selected for processing (3)
src/elements/content-sidebar/activity-feed-v2/ActivityFeedV2.tsxsrc/elements/content-sidebar/activity-feed-v2/__tests__/ActivityFeedV2.test.tsxsrc/elements/content-sidebar/activity-feed-v2/types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/elements/content-sidebar/activity-feed-v2/tests/ActivityFeedV2.test.tsx
kduncanhsu
force-pushed
the
kduncanhsu/annotations-in-scrubber-bar-plumbing
branch
from
3401384 to
928ca74
Compare
4 participants
Summary
ActivityFeedV2now extracts timestamped comments and frame annotations fromfilteredItemsand emits them to the video viewer via thecommentmarkerseventcommentmarkerselectevents from the viewer to scroll the sidebar to the selected item when a scrubber marker is clickedgetViewerprop fromSidebarPanels→ActivitySidebar→ActivityFeedV2Modified files
ActivityFeedV2.tsx— AddeduseEffectto build markers array and emit to viewer, registers/cleans upcommentmarkerselectlistenertypes.ts— AddedgetViewerprop toActivityFeedV2PropsActivitySidebar.js— AddedgetViewerto props type, passes it toActivityFeedV2SidebarPanels.js— PassesgetViewertoLoadableActivitySidebarActivityFeedV2.test.tsx— 10 new tests for marker emission, listener lifecycle, and filteringDepends on
box-content-previewPR that adds thecommentmarkersevent listener andcommentmarkerselectevent emissionTest plan
Summary by CodeRabbit