Edit video events in the timeline#1677
Conversation
…ions' into horatiu-lig-9805-import-activitynet-style-event-annotations-3
…ent-annotations-3
…ent-annotations-3
Previously, filtering and counting video annotations only considered annotations attached to a video's frames. Videos can also carry annotations directly (e.g. ActivityNet-style event/classification labels on the whole video). This makes both the video filter and the annotation counter consider these direct video annotations in addition to frame annotations.
…ions-3' into horatiu-lig-9806-filter-videos-by-event-metadata
Mirrors a <video> element's playback state into reactive fields and exposes intent callbacks (seek, play/pause, mute, fullscreen) for a custom control bar. Listening via native events keeps it composable with handlers on the element. Covered by a harness-driven unit test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Compose useVideoPlayback + VideoControls, forcing native <video controls> off so the full-width scrubber can host aligned timeline overlays. Adds a startTimeS prop (null waits for a deep-link timestamp) and a region ref for fullscreen. Updates tests and stories accordingly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Hand the deep-linked frame's timestamp to VideoPlayer via startTimeS instead of seeking after load, and remount on frame-number change with a composite #key. Align the annotation overlay to the <video> box and fix min-h-0 layout so the player fills its card. Adds a VideoDetails test covering the frame-load branch and deep-link handoff. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Full-width scrubber + transport buttons that own no playback state and call back on user intent. Includes helpers (formatTime, clampPercent, timeFromClientX) and unit tests. Exported from the components barrel. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Interactive Playground plus paused / near-end states, on a dark backdrop matching how the bar overlays a video. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Mirrors a <video> element's playback state into reactive fields and exposes intent callbacks (seek, play/pause, mute, fullscreen) for a custom control bar. Listening via native events keeps it composable with handlers on the element. Covered by a harness-driven unit test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Compose useVideoPlayback + VideoControls, forcing native <video controls> off so the full-width scrubber can host aligned timeline overlays. Adds a startTimeS prop (null waits for a deep-link timestamp) and a region ref for fullscreen. Updates tests and stories accordingly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Hand the deep-linked frame's timestamp to VideoPlayer via startTimeS instead of seeking after load, and remount on frame-number change with a composite #key. Align the annotation overlay to the <video> box and fix min-h-0 layout so the player fills its card. Adds a VideoDetails test covering the frame-load branch and deep-link handoff. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…github.com:lightly-ai/lightly-studio into horatiu-lig-9807-show-event-bars-on-the-timeline.d
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ts-model' into horatiu-lig-9807-show-event-bars-on-the-timeline-3-events-timeline
…ts-timeline' into horatiu-lig-9807-show-event-bars-on-the-timeline-3-events-wiring
…ts-wiring' of github.com:lightly-ai/lightly-studio into horatiu-lig-9807-show-event-bars-on-the-timeline-3-events-wiring
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAnnotation updates now carry temporal spans through the API, service, and resolver layers. Video event timeline edges can be resized interactively, with optimistic rendering and persistence through the annotation mutation flow. ChangesTemporal span annotation editing
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant VideoEventTimeline
participant VideoPlayer
participant VideoDetails
participant AnnotationAPI
User->>VideoEventTimeline: Drag event edge
VideoEventTimeline->>VideoPlayer: Emit resized event span
VideoPlayer->>VideoDetails: Forward resize callback
VideoDetails->>AnnotationAPI: Submit start/end timestamps
AnnotationAPI-->>VideoDetails: Return update result
VideoDetails->>VideoEventTimeline: Refresh timeline data
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
❌ Fast Track: checks did not passFailed guardrails: frontend/complexity (Guardrail threw: Cannot find module 'eslint'
Reflects |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
lightly_studio_view/src/lib/components/VideoDetails/VideoDetails.test.ts (1)
46-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test exercises
handleEventResizeitself.This diff only adds the mock needed for the component to mount; there's no test asserting
updateAnnotationsis called with the expectedAnnotationUpdateInputpayload, or thatonVideoUpdatefires after resize. Worth a follow-up test, especially once error handling is added tohandleEventResize.🤖 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 `@lightly_studio_view/src/lib/components/VideoDetails/VideoDetails.test.ts` around lines 46 - 50, Extend the VideoDetails tests to exercise handleEventResize, verifying updateAnnotations receives the expected AnnotationUpdateInput payload and onVideoUpdate is called after a successful resize. Reuse the existing useUpdateAnnotationsMutation mock and cover the success path without changing unrelated component behavior.lightly_studio_view/src/lib/components/VideoEventTimeline/VideoEventTimeline.test.ts (1)
94-128: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding coverage for the 'end' edge and the min-duration clamp.
clampSpanhas a symmetric 'end' branch and aMIN_EVENT_DURATION_Sclamp that aren't exercised by any test here — worth a follow-up test given how easy it is to regress clamping math.🤖 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 `@lightly_studio_view/src/lib/components/VideoEventTimeline/VideoEventTimeline.test.ts` around lines 94 - 128, Add tests alongside the existing edge-drag test for dragging the end handle and for enforcing the MIN_EVENT_DURATION_S minimum when an edge crosses the opposite boundary. Reuse the existing VideoEventTimeline setup and geometry mocks, and assert both onResize’s resulting start/end values and onSeek’s final playback position.
🤖 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 `@lightly_studio_view/src/lib/components/VideoDetails/VideoDetails.svelte`:
- Around line 46-61: Update handleEventResize to catch rejected
updateAnnotations calls and surface the persistence error through the
component’s existing user-facing error notification mechanism, while retaining
the finally block’s onVideoUpdate refresh. Ensure the fire-and-forget
VideoEventTimeline onResize callback does not produce an unhandled rejection.
In
`@lightly_studio_view/src/lib/components/VideoEventTimeline/VideoEventTimeline.svelte`:
- Around line 207-225: Make the edge handles in the editable branch
keyboard-operable by adding a keydown handler alongside the existing pointer
handlers and implementing arrow-key adjustments for the corresponding start or
end edge. Reuse the existing resize/update logic where possible, preserve
aria-valuenow updates and duration/span bounds, and ignore unrelated keys.
In
`@lightly_studio/src/lightly_studio/services/annotations_service/update_annotation.py`:
- Around line 60-66: Update the temporal-span handling in the annotation update
flow to raise a ValueError when exactly one of annotation_update.start_time_s or
annotation_update.end_time_s is provided. Continue calling
annotations_service.update_temporal_span only when both bounds are present, and
preserve existing handling for updates without either bound.
---
Nitpick comments:
In `@lightly_studio_view/src/lib/components/VideoDetails/VideoDetails.test.ts`:
- Around line 46-50: Extend the VideoDetails tests to exercise
handleEventResize, verifying updateAnnotations receives the expected
AnnotationUpdateInput payload and onVideoUpdate is called after a successful
resize. Reuse the existing useUpdateAnnotationsMutation mock and cover the
success path without changing unrelated component behavior.
In
`@lightly_studio_view/src/lib/components/VideoEventTimeline/VideoEventTimeline.test.ts`:
- Around line 94-128: Add tests alongside the existing edge-drag test for
dragging the end handle and for enforcing the MIN_EVENT_DURATION_S minimum when
an edge crosses the opposite boundary. Reuse the existing VideoEventTimeline
setup and geometry mocks, and assert both onResize’s resulting start/end values
and onSeek’s final playback position.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4e1ddc7e-be7c-4c8f-848a-6ab14d13ca28
📒 Files selected for processing (12)
lightly_studio/src/lightly_studio/api/routes/api/annotation.pylightly_studio/src/lightly_studio/resolvers/annotation_resolver/__init__.pylightly_studio/src/lightly_studio/resolvers/annotation_resolver/update_temporal_span.pylightly_studio/src/lightly_studio/services/annotations_service/__init__.pylightly_studio/src/lightly_studio/services/annotations_service/update_annotation.pylightly_studio/src/lightly_studio/services/annotations_service/update_temporal_span.pylightly_studio/tests/resolvers/annotations/test_update_temporal_span.pylightly_studio_view/src/lib/components/VideoDetails/VideoDetails.sveltelightly_studio_view/src/lib/components/VideoDetails/VideoDetails.test.tslightly_studio_view/src/lib/components/VideoEventTimeline/VideoEventTimeline.sveltelightly_studio_view/src/lib/components/VideoEventTimeline/VideoEventTimeline.test.tslightly_studio_view/src/lib/components/VideoPlayer/VideoPlayer.svelte
What has changed and why?
Update for editing the video events.
How has it been tested?
new tests, manual test
edit_events.mp4
Did you update CHANGELOG.md?
Summary by CodeRabbit
VideoPlayer/VideoDetailswiring).