Delete video event#1679
Merged
Merged
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
Contributor
|
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 with no reviewable changes (1)
📝 WalkthroughWalkthroughAdds delete controls for editable video events, wires deletion through ChangesVideo event deletion
Storybook state scoping
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant VideoEventTimeline
participant VideoPlayer
participant VideoDetails
participant useDeleteAnnotation
User->>VideoEventTimeline: Click delete-event-button
VideoEventTimeline->>VideoPlayer: Invoke onDelete(event)
VideoPlayer->>VideoDetails: Invoke onEventDelete(event)
VideoDetails->>useDeleteAnnotation: Delete event by id
useDeleteAnnotation-->>VideoDetails: Return deletion result
VideoDetails->>VideoDetails: Refresh video and show toast
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Contributor
|
UX looks good to me |
❌ Fast Track: checks did not passFailed guardrails: frontend/complexity (Guardrail threw: Cannot find module 'eslint'
Reflects |
![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)
lightly_studio_view/src/lib/components/VideoEventTimeline/VideoEventTimeline.test.ts (1)
162-175: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the editable-without-handler branch.
The test does not verify
editable: truewith noonDelete. A regression that renders a no-op delete button whenevereditableis true would still pass. Add that intermediate rerender and assert the button is absent.Suggested test addition
expect(queryByTestId('delete-event-button')).toBeFalsy(); + rerender({ + events: [makeEvent({ id: 'a' })], + durationS: 10, + editable: true + }); + expect(queryByTestId('delete-event-button')).toBeFalsy(); + rerender({🤖 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 162 - 175, Add an intermediate rerender in the test “shows the delete button only when editable with an onDelete handler” with editable set to true and no onDelete handler, then assert delete-event-button is absent before the existing editable-with-handler assertion.lightly_studio_view/src/lib/components/VideoPlayer/VideoPlayer.test.ts (1)
171-193: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the forwarded callback is invoked.
This test only checks button presence. Click the button and assert that the supplied
onEventDeletemock receives the event; that verifies the VideoPlayer-to-timeline callback contract rather than only rendering.🤖 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/VideoPlayer/VideoPlayer.test.ts` around lines 171 - 193, Update the test “renders a delete affordance on events in edit mode” to use a mock for onEventDelete, click the delete-event-button, and assert the mock receives the configured event. Preserve the existing render setup and presence assertion while verifying the VideoPlayer-to-timeline callback contract.lightly_studio_view/src/lib/components/VideoDetails/VideoDetails.test.ts (1)
60-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the deletion mock controllable and asynchronous.
The real
deleteAnnotationreturnsPromise<void>, but this stub returnsundefinedand is not exposed for assertions. Use a stablemockResolvedValue/mockRejectedValuemock so tests can verifyhandleEventDeleterefreshes on success and shows the error toast on failure.🤖 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 60 - 62, Update the useDeleteAnnotation mock in VideoDetails.test.ts so it exposes a stable deleteAnnotation mock configured as an asynchronous Promise<void>, allowing tests to switch between resolved and rejected outcomes. Use that controllable mock to assert handleEventDelete refreshes after success and displays the error toast after failure.
🤖 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/VideoControls/VideoControls.stories.svelte`:
- Around line 10-16: Resolve the merge conflict in the script section of
VideoControls.stories.svelte by removing the `<<<<<<< HEAD`, `=======`, and
`>>>>>>> main` markers while retaining the closing module script tag and opening
instance script tag.
---
Nitpick comments:
In `@lightly_studio_view/src/lib/components/VideoDetails/VideoDetails.test.ts`:
- Around line 60-62: Update the useDeleteAnnotation mock in VideoDetails.test.ts
so it exposes a stable deleteAnnotation mock configured as an asynchronous
Promise<void>, allowing tests to switch between resolved and rejected outcomes.
Use that controllable mock to assert handleEventDelete refreshes after success
and displays the error toast after failure.
In
`@lightly_studio_view/src/lib/components/VideoEventTimeline/VideoEventTimeline.test.ts`:
- Around line 162-175: Add an intermediate rerender in the test “shows the
delete button only when editable with an onDelete handler” with editable set to
true and no onDelete handler, then assert delete-event-button is absent before
the existing editable-with-handler assertion.
In `@lightly_studio_view/src/lib/components/VideoPlayer/VideoPlayer.test.ts`:
- Around line 171-193: Update the test “renders a delete affordance on events in
edit mode” to use a mock for onEventDelete, click the delete-event-button, and
assert the mock receives the configured event. Preserve the existing render
setup and presence assertion while verifying the VideoPlayer-to-timeline
callback contract.
🪄 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: 46e7dea7-6996-47bb-894e-7de25847f7f5
📒 Files selected for processing (8)
lightly_studio_view/src/lib/components/VideoControls/VideoControls.stories.sveltelightly_studio_view/src/lib/components/VideoDetails/VideoDetails.sveltelightly_studio_view/src/lib/components/VideoDetails/VideoDetails.test.tslightly_studio_view/src/lib/components/VideoEventTimeline/VideoEventTimeline.stories.sveltelightly_studio_view/src/lib/components/VideoEventTimeline/VideoEventTimeline.sveltelightly_studio_view/src/lib/components/VideoEventTimeline/VideoEventTimeline.test.tslightly_studio_view/src/lib/components/VideoPlayer/VideoPlayer.sveltelightly_studio_view/src/lib/components/VideoPlayer/VideoPlayer.test.ts
💤 Files with no reviewable changes (1)
- lightly_studio_view/src/lib/components/VideoEventTimeline/VideoEventTimeline.stories.svelte
LeonardoRosaa
approved these changes
Jul 22, 2026
LeonardoRosaa
enabled auto-merge
July 22, 2026 13:27
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
3 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.
What has changed and why?
Delete a video event from the timeline
How has it been tested?
New test and manual test
delete.mp4
Did you update CHANGELOG.md?
Summary by CodeRabbit
New Features
Tests