feat(content-sidebar): timeline markers for timestamped comments and annotations#4647
feat(content-sidebar): timeline markers for timestamped comments and annotations#4647mrscobbler wants to merge 1 commit into
Conversation
…annotations Activity sidebar derives video timeline markers from filteredItems (comments with timestamp markup, annotations with frame target) and pushes them to the box-content-preview viewer via a window CustomEvent. Marker clicks bubble back through AnnotatorContext, where the existing handleAnnotationSelect flow renders the overlay and seeks the video. handleAnnotationSelect now seeks frame annotations on the current file version first so the playhead moves before sidebar scroll and overlay render. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
WalkthroughThis PR introduces timeline marker support across the annotator context infrastructure. New ChangesTimeline Marker Feature
Sequence Diagram(s)sequenceDiagram
participant Viewer as BP Viewer
participant Window as window (CustomEvents)
participant withAnnotations as withAnnotations HOC
participant ActivityFeedV2 as ActivityFeedV2
participant ActivitySidebar as ActivitySidebar
rect rgba(100, 149, 237, 0.5)
Note over ActivityFeedV2,withAnnotations: Marker push path
ActivityFeedV2->>withAnnotations: setTimelineMarkers(markers) via context
withAnnotations->>withAnnotations: cache lastTimelineMarkers
withAnnotations->>Window: dispatch bp:timeline_markers_update
Window->>Viewer: CustomEvent(markers)
end
rect rgba(144, 238, 144, 0.5)
Note over Viewer,withAnnotations: Viewer ready replay
Viewer->>Window: dispatch bp:timeline_markers_ready
Window->>withAnnotations: handleTimelineMarkersReady
withAnnotations->>Window: dispatch bp:timeline_markers_update (replay)
end
rect rgba(255, 165, 0, 0.5)
Note over Viewer,ActivitySidebar: Marker click path
Viewer->>Window: dispatch bp:timeline_marker_click
Window->>withAnnotations: unwrap CustomEvent.detail
withAnnotations->>ActivitySidebar: handleTimelineMarkerClick(payload)
ActivitySidebar->>ActivitySidebar: seekVideoToMs(timestampMs)
ActivitySidebar->>ActivitySidebar: navigate to annotation/comment
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Biome (2.5.0)src/elements/content-sidebar/ActivitySidebar.jsFile contains syntax errors that prevent linting: Line 15: 'import { type x ident }' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 51: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 59: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 74: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 75: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 76: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 77: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 78: 'import type' are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.; Line 79: ' ... [truncated 25060 characters] ... ken. Did you mean 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: 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/elements/content-sidebar/activity-feed-v2/ActivityFeedV2.tsx`:
- Around line 270-276: The useEffect hook that calls setTimelineMarkers
publishes timeline markers but does not clear them when the component unmounts,
causing stale markers to remain cached on the video scrubber. Add a cleanup
function (return statement) to the useEffect hook that clears the timeline
markers by calling setTimelineMarkers with an empty array or null value when the
component unmounts, ensuring the scrubber displays current markers only.
In `@src/elements/content-sidebar/ActivitySidebar.js`:
- Around line 254-284: The handleSelectActivityClick method falls through to
comment navigation when type is annotation but no matching feedItem is found,
causing annotation IDs to be incorrectly routed as comment IDs. After the
annotationItem check within the type === 'annotation' block, add logic to handle
the case when annotationItem is null or feedItems is not yet loaded, such as
returning early or using annotation-specific navigation instead of allowing
execution to continue to the comment navigation branch that uses
getActiveCommentPath and FeedEntryType.COMMENTS.
🪄 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: 3e3ddfcc-6528-42ad-b920-257759f6a72d
📒 Files selected for processing (9)
src/elements/common/annotator-context/__tests__/withAnnotations.test.tsxsrc/elements/common/annotator-context/types.js.flowsrc/elements/common/annotator-context/types.tssrc/elements/common/annotator-context/withAnnotations.js.flowsrc/elements/common/annotator-context/withAnnotations.tsxsrc/elements/common/annotator-context/withAnnotatorContext.js.flowsrc/elements/common/annotator-context/withAnnotatorContext.tsxsrc/elements/content-sidebar/ActivitySidebar.jssrc/elements/content-sidebar/activity-feed-v2/ActivityFeedV2.tsx
| const { setTimelineMarkers } = React.useContext(AnnotatorContext); | ||
|
|
||
| React.useEffect(() => { | ||
| if (setTimelineMarkers) { | ||
| setTimelineMarkers(timelineMarkers); | ||
| } | ||
| }, [setTimelineMarkers, timelineMarkers]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clear timeline markers on unmount to avoid stale scrubber markers.
The effect publishes markers but never clears them. When ActivityFeedV2 unmounts (e.g. switching away from the activity tab), the previously published markers remain cached by the withAnnotations bridge and stay rendered on the video scrubber.
🧹 Proposed cleanup
React.useEffect(() => {
if (setTimelineMarkers) {
setTimelineMarkers(timelineMarkers);
}
+ return () => {
+ setTimelineMarkers?.([]);
+ };
}, [setTimelineMarkers, timelineMarkers]);📝 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.
| const { setTimelineMarkers } = React.useContext(AnnotatorContext); | |
| React.useEffect(() => { | |
| if (setTimelineMarkers) { | |
| setTimelineMarkers(timelineMarkers); | |
| } | |
| }, [setTimelineMarkers, timelineMarkers]); | |
| const { setTimelineMarkers } = React.useContext(AnnotatorContext); | |
| React.useEffect(() => { | |
| if (setTimelineMarkers) { | |
| setTimelineMarkers(timelineMarkers); | |
| } | |
| return () => { | |
| setTimelineMarkers?.([]); | |
| }; | |
| }, [setTimelineMarkers, timelineMarkers]); |
🤖 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 270 - 276, The useEffect hook that calls setTimelineMarkers publishes
timeline markers but does not clear them when the component unmounts, causing
stale markers to remain cached on the video scrubber. Add a cleanup function
(return statement) to the useEffect hook that clears the timeline markers by
calling setTimelineMarkers with an empty array or null value when the component
unmounts, ensuring the scrubber displays current markers only.
| if (type === 'annotation') { | ||
| const { feedItems } = this.state; | ||
| const annotationItem = feedItems | ||
| ? feedItems.find(item => item.id === id && item.type === FEED_ITEM_TYPE_ANNOTATION) | ||
| : null; | ||
| if (annotationItem) { | ||
| // Reuse the badge-click flow so the annotator's active-annotation | ||
| // state, URL push, overlay rendering, and seek all stay consistent. | ||
| this.handleAnnotationSelect(annotationItem); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| // Comment markers carry a timestamp directly; seek immediately so the | ||
| // playhead moves before the sidebar scroll catches up. | ||
| if (typeof timestampMs === 'number') { | ||
| seekVideoToMs(timestampMs); | ||
| } | ||
|
|
||
| const { history, internalSidebarNavigationHandler, routerDisabled } = this.props; | ||
|
|
||
| if (routerDisabled && internalSidebarNavigationHandler) { | ||
| internalSidebarNavigationHandler({ | ||
| sidebar: ViewType.ACTIVITY, | ||
| activeFeedEntryId: id, | ||
| activeFeedEntryType: FeedEntryType.COMMENTS, | ||
| }); | ||
| } else { | ||
| history.push(this.getActiveCommentPath(id)); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Annotation markers fall through to comment navigation when the feed item isn't found.
If type === 'annotation' but no matching entry exists in this.state.feedItems (e.g. not yet loaded, or filtered out), execution falls through to the comment branch and navigates with activeFeedEntryType: FeedEntryType.COMMENTS / getActiveCommentPath(id) for an annotation id, producing incorrect routing. Consider returning early (or using the annotation path) for unresolved annotation markers instead of treating them as comments.
🤖 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/ActivitySidebar.js` around lines 254 - 284, The
handleSelectActivityClick method falls through to comment navigation when type
is annotation but no matching feedItem is found, causing annotation IDs to be
incorrectly routed as comment IDs. After the annotationItem check within the
type === 'annotation' block, add logic to handle the case when annotationItem is
null or feedItems is not yet loaded, such as returning early or using
annotation-specific navigation instead of allowing execution to continue to the
comment navigation branch that uses getActiveCommentPath and
FeedEntryType.COMMENTS.
1 participant
Summary
filteredItems(comments with timestamp markup, annotations with frame target) and pushes them to the box-content-preview viewer via a window CustomEventAnnotatorContext; the existinghandleAnnotationSelectflow renders the overlay and seeks the video for annotations, comment markers seek + push URLhandleAnnotationSelectnow seeks frame annotations on the current file version first so the playhead moves before sidebar scroll and overlay renderwithAnnotationsTest plan
videoPlayerV2.enabled,activityFeed.threadedRepliesV2, andactivityFeed.timestampedCommentsall on🤖 Generated with Claude Code
Summary by CodeRabbit