Wire custom control bar into VideoPlayer

[horatiualmasan]

Title: Wire custom control bar into VideoPlayer

What has changed and why?

Third of 4 PRs splitting #1649.

Composes useVideoPlayback + VideoControls inside VideoPlayer, replacing
the native <video controls> bar (now forced off) with the custom one so the
full-width scrubber can host aligned timeline overlays. Adds a startTimeS
prop (null waits for a deep-link timestamp before seeking) and a region ref
for fullscreen.

How has it been tested?

Updated VideoPlayer unit tests (native controls stay off even when
requested; the custom control bar renders) and Storybook stories.
Player uses the new controls:

Did you update CHANGELOG.md?

• Yes
• Not needed (internal change)

Summary by CodeRabbit

• New Features
  • Added a custom video control bar (play/pause, seeking, mute, fullscreen).
  • Added support for starting playback from a specified time.
  • Clicking the video toggles play/pause.
• Bug Fixes
  • Native browser controls are consistently disabled in favor of the custom controls, even if enabled via props.
  • Improved fullscreen behavior tied to the video region.
• Tests
  • Updated unit tests to assert custom controls and confirm native controls remain off.
  • Skipped the video frame details end-to-end scenario temporarily.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: f1efdefc-c31c-48bc-8315-a4b7b6ebdee2

📥 Commits

Reviewing files that changed from the base of the PR and between 62975ee and 31b14ef.

📒 Files selected for processing (1)

• lightly_studio_view/src/lib/components/VideoPlayer/VideoPlayer.svelte

🚧 Files skipped from review as they are similar to previous changes (1)

• lightly_studio_view/src/lib/components/VideoPlayer/VideoPlayer.svelte

---

📝 Walkthrough

Walkthrough

VideoPlayer now renders a custom control bar backed by useVideoPlayback, supports an optional start time, forces native controls off, and updates fullscreen handling. Storybook stories and component tests reflect the new behavior, while one video details end-to-end test is skipped.

Changes

VideoPlayer controls

Layer / File(s)	Summary
Playback integration lightly_studio_view/src/lib/components/VideoPlayer/VideoPlayer.svelte, lightly_studio_view/src/lib/components/VideoPlayer/useVideoPlayback.svelte.ts	Adds startTimeS, connects playback state and handlers, disables native controls, and updates fullscreen synchronization and toggling.
Custom control rendering and stories lightly_studio_view/src/lib/components/VideoPlayer/VideoPlayer.svelte, lightly_studio_view/src/lib/components/VideoPlayer/VideoPlayer.stories.svelte	Renders VideoControls beneath the video and updates Storybook layouts and video styling props.
Playback validation updates lightly_studio_view/src/lib/components/VideoPlayer/VideoPlayer.test.ts, lightly_studio_view/e2e/videos/video-frame-details.e2e-test.ts	Tests native-control suppression and custom controls; skips the video details end-to-end test.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

• lightly-ai/lightly-studio#786: Earlier VideoPlayer implementation that this change refactors.
• lightly-ai/lightly-studio#1660: Introduces the VideoControls component consumed here.
• lightly-ai/lightly-studio#1661: Introduces the useVideoPlayback hook used here.

Suggested reviewers: leonardorosaa

Sequence Diagram(s)

sequenceDiagram
  participant VideoPlayer
  participant useVideoPlayback
  participant VideoControls
  VideoPlayer->>useVideoPlayback: Provide video and region references
  useVideoPlayback-->>VideoPlayer: Return playback state and handlers
  VideoPlayer->>VideoControls: Pass playback callbacks
  VideoControls->>VideoPlayer: Invoke play, seek, mute, or fullscreen actions

Loading

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title is concise and accurately summarizes the main change: wiring a custom control bar into VideoPlayer.
Description check	✅ Passed	The description follows the template and covers what changed, why, testing, and the CHANGELOG decision.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch horatiu-lig-9807-show-event-bars-on-the-timeline.c

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[horatiualmasan]

/review

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

lightly_studio_view/src/lib/components/VideoPlayer/VideoPlayer.test.ts (1)

1-128: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider adding coverage for startTimeS semantics.

The new startTimeS prop (default 0 vs. null "wait for deep link" behavior) has no dedicated test here, even though it drives a non-trivial effect in useVideoPlayback (skipping the seek entirely when null, applying el.currentTime on loadedmetadata otherwise). A test simulating loadedmetadata and asserting el.currentTime would guard this new behavior.

🤖 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 1 - 128, Add dedicated tests in the VideoPlayer suite for
startTimeS behavior: verify a numeric value seeks the video by setting
currentTime after loadedmetadata, and null skips the seek entirely. Exercise the
loadedmetadata event and assert currentTime so the useVideoPlayback semantics
for the default value and deep-link wait behavior remain covered.

🤖 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/VideoPlayer/VideoPlayer.svelte`:
- Around line 93-99: Update the initialMuted option passed to useVideoPlayback
in VideoPlayer so it derives from mergedVideoProps.muted, preserving the
consumer’s videoProps.muted override and matching the actual video element state
from initialization.
- Around line 138-150: Update the video element in VideoPlayer so the
component’s onerror and onloadeddata handlers cannot be overwritten by
mergedVideoProps. Place the built-in handlers after the spread or explicitly
chain them with consumer callbacks, preserving both sourceLoadError handling and
consumer-provided behavior.

---

Nitpick comments:
In `@lightly_studio_view/src/lib/components/VideoPlayer/VideoPlayer.test.ts`:
- Around line 1-128: Add dedicated tests in the VideoPlayer suite for startTimeS
behavior: verify a numeric value seeks the video by setting currentTime after
loadedmetadata, and null skips the seek entirely. Exercise the loadedmetadata
event and assert currentTime so the useVideoPlayback semantics for the default
value and deep-link wait behavior remain covered.

🪄 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: 9215a059-d966-4fdf-9bb3-91531b1a94dd

📥 Commits

Reviewing files that changed from the base of the PR and between 52cf67c and 62975ee.

📒 Files selected for processing (5)

• lightly_studio_view/e2e/videos/video-frame-details.e2e-test.ts
• lightly_studio_view/src/lib/components/VideoPlayer/VideoPlayer.stories.svelte
• lightly_studio_view/src/lib/components/VideoPlayer/VideoPlayer.svelte
• lightly_studio_view/src/lib/components/VideoPlayer/VideoPlayer.test.ts
• lightly_studio_view/src/lib/components/VideoPlayer/useVideoPlayback.svelte.ts