Strand-themed video.js skin#99
Conversation
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
WalkthroughThis pull request introduces a new ChangesStrand Video Player Component
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsThese MCP integrations need to be re-authenticated in the Integrations settings: Sentry 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 |
|
The CodeRabbit comment above is a 'review in progress' status notification — no actionable feedback yet. Will address concrete review items once CodeRabbit posts them. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new custom video player component, StrandPlayer, styled with Strand design tokens. It replaces the native video controls on the Watch page and updates the native-player library to allow custom fullscreen targets, ensuring that custom controls remain visible during fullscreen playback. Feedback focuses on improving the responsiveness of keyboard seeking by updating the seek input logic, removing redundant event listeners on the seek bar that trigger on non-navigation keys, and consolidating time formatting logic into a shared utility to avoid code duplication.
| const onSeekInput = (e: React.ChangeEvent<HTMLInputElement>): void => { | ||
| const v = Number.parseFloat(e.target.value); | ||
| setCurrent(v); | ||
| }; |
There was a problem hiding this comment.
Keyboard seeking (using arrow keys on the range input) currently feels unresponsive because the video only seeks on onKeyUp. Additionally, the timeupdate listener will cause the slider to jump back to the old position while a key is held down because scrubbing is false.
Updating onSeekInput to seek immediately when not dragging (i.e., keyboard interaction) provides a much smoother experience.
const onSeekInput = (e: React.ChangeEvent<HTMLInputElement>): void => {
const v = Number.parseFloat(e.target.value);
setCurrent(v);
if (!scrubbing) {
seekTo(v);
}
};
| function fmt(total: number): string { | ||
| if (!Number.isFinite(total) || total < 0) return '0:00'; | ||
| const t = Math.floor(total); | ||
| const h = Math.floor(t / 3600); | ||
| const m = Math.floor((t % 3600) / 60); | ||
| const s = t % 60; | ||
| const mm = String(m).padStart(h > 0 ? 2 : 1, '0'); | ||
| const ss = String(s).padStart(2, '0'); | ||
| return h > 0 ? `${h}:${mm}:${ss}` : `${mm}:${ss}`; | ||
| } |
There was a problem hiding this comment.
The time formatting logic in fmt is identical to formatHms in Watch.tsx. To improve maintainability and adhere to DRY principles, this should be moved to a shared utility file (e.g., src/frontend/lib/format.ts).
| onChange={onSeekInput} | ||
| onMouseUp={onSeekCommit} | ||
| onTouchEnd={onSeekCommit} | ||
| onKeyUp={onSeekCommit} |
There was a problem hiding this comment.
The onKeyUp listener on the range input is problematic as it triggers a seek on every key release, including non-navigation keys like Tab or Shift. With the suggested improvement to onSeekInput, this listener is no longer necessary for keyboard seeking.
| onKeyUp={onSeekCommit} | |
| onMouseUp={onSeekCommit} | |
| onTouchEnd={onSeekCommit} |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12a22a4e2a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| onMouseDown={() => setScrubbing(true)} | ||
| onTouchStart={() => setScrubbing(true)} | ||
| onChange={onSeekInput} | ||
| onMouseUp={onSeekCommit} | ||
| onTouchEnd={onSeekCommit} |
There was a problem hiding this comment.
scrubbing is set to true on press, but it is only cleared in the input’s own onMouseUp/onTouchEnd handlers. If the user drags the seek thumb and releases outside the range element (or the interaction is canceled), those handlers may not fire, leaving scrubbing stuck true. In that state, onTime stops syncing playback time (if (!scrubbing)), so the displayed progress/time can freeze for the rest of playback until another successful seek interaction resets it.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/frontend/components/StrandPlayer.tsx`:
- Around line 66-73: The component fails to reset timeline state when the video
source changes: update the event handling so that on loadstart and emptied you
clear current and buffered (e.g., setCurrent(0) and setBuffered(0)) to avoid
showing the previous video's progress, and then keep onMeta() (and onTime()) to
re-sync duration/current/buffered once the new source metadata/timeupdates
arrive; modify or add handlers tied to the same video element reference (the el
used in onTime and onMeta) to reset state on 'loadstart' and 'emptied' and
ensure onMeta() also sets current/buffered when appropriate.
- Around line 145-151: The scrub state is only being cleared by input-local end
handlers so releasing the pointer outside the range leaves scrubbing true and
blocks timeupdate; update the interaction to use pointer events and a global
release handler: on pointerdown (attach in the same component where you
currently set scrubbing true, e.g., the range input handlers around onSeekCommit
/ the corresponding start handler) call setScrubbing(true) and call
e.currentTarget.setPointerCapture(e.pointerId) if available, and add a one-time
window.addEventListener('pointerup', ...) (or document) to call
setScrubbing(false) and release capture; also ensure onSeekCommit still calls
seekTo and setScrubbing(false) for final commits. Apply the same change to the
other range handlers referenced (the block around lines 198-203) so scrubbing is
reliably cleared when the pointer is released anywhere.
In `@src/frontend/styles/strand.css`:
- Around line 614-619: The CSS rules that hide controls when playing rely on
:hover/:focus-within and break touch/coarse-pointer devices; scope the hiding to
only hover-capable devices by wrapping the selectors
.strand-player.is-playing:not(:hover):not(:focus-within) and its
.strand-player__controls rule inside a media query for hover-capable,
fine-pointer devices (e.g., media query using (hover: hover) and (pointer:
fine)), and likewise scope the cursor: none rule the same way so controls remain
reachable on touch devices while preserving the current behavior for mouse
users.
🪄 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: 92d10f13-d209-4a55-b8f8-e747461e4547
📒 Files selected for processing (4)
src/frontend/components/StrandPlayer.tsxsrc/frontend/lib/native-player.tssrc/frontend/pages/Watch.tsxsrc/frontend/styles/strand.css
| const onTime = (): void => { | ||
| if (!scrubbing) setCurrent(el.currentTime); | ||
| const b = el.buffered; | ||
| if (b.length > 0) setBuffered(b.end(b.length - 1)); | ||
| }; | ||
| const onMeta = (): void => { | ||
| setDuration(Number.isFinite(el.duration) ? el.duration : 0); | ||
| }; |
There was a problem hiding this comment.
Reset timeline state when the source changes.
onMeta() only refreshes duration, so when Watch swaps the source on the same <video> element this component can keep rendering the previous video's time/progress until the next timeupdate. Clear current/buffered on loadstart/emptied and re-sync them in onMeta().
One way to re-sync the UI state
const onMeta = (): void => {
+ setCurrent(Number.isFinite(el.currentTime) ? el.currentTime : 0);
setDuration(Number.isFinite(el.duration) ? el.duration : 0);
+ const b = el.buffered;
+ setBuffered(b.length > 0 ? b.end(b.length - 1) : 0);
+ };
+ const onLoadStart = (): void => {
+ setCurrent(0);
+ setDuration(0);
+ setBuffered(0);
};
const onVol = (): void => {
setMuted(el.muted);
setVolume(el.volume);
};
@@
+ el.addEventListener('loadstart', onLoadStart);
el.addEventListener('loadedmetadata', onMeta);
el.addEventListener('durationchange', onMeta);
el.addEventListener('volumechange', onVol);
@@
+ el.removeEventListener('loadstart', onLoadStart);
el.removeEventListener('loadedmetadata', onMeta);
el.removeEventListener('durationchange', onMeta);
el.removeEventListener('volumechange', onVol);Also applies to: 87-88
🤖 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/frontend/components/StrandPlayer.tsx` around lines 66 - 73, The component
fails to reset timeline state when the video source changes: update the event
handling so that on loadstart and emptied you clear current and buffered (e.g.,
setCurrent(0) and setBuffered(0)) to avoid showing the previous video's
progress, and then keep onMeta() (and onTime()) to re-sync
duration/current/buffered once the new source metadata/timeupdates arrive;
modify or add handlers tied to the same video element reference (the el used in
onTime and onMeta) to reset state on 'loadstart' and 'emptied' and ensure
onMeta() also sets current/buffered when appropriate.
| const onSeekCommit = ( | ||
| e: React.SyntheticEvent<HTMLInputElement>, | ||
| ): void => { | ||
| const v = Number.parseFloat(e.currentTarget.value); | ||
| seekTo(v); | ||
| setScrubbing(false); | ||
| }; |
There was a problem hiding this comment.
Don't rely on element-local mouse/touch end events to finish scrubbing.
scrubbing is turned on at drag start, but it's only turned off by end handlers attached to the range itself. If the pointer is released outside the input, those handlers won't run and timeupdate stays ignored, so the scrubber/time display can freeze out of sync.
Use pointer events so the control keeps the interaction until release
- onMouseDown={() => setScrubbing(true)}
- onTouchStart={() => setScrubbing(true)}
+ onPointerDown={(event) => {
+ event.currentTarget.setPointerCapture(event.pointerId);
+ setScrubbing(true);
+ }}
onChange={onSeekInput}
- onMouseUp={onSeekCommit}
- onTouchEnd={onSeekCommit}
+ onPointerUp={onSeekCommit}
+ onPointerCancel={() => setScrubbing(false)}
+ onBlur={onSeekCommit}
onKeyUp={onSeekCommit}Also applies to: 198-203
🤖 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/frontend/components/StrandPlayer.tsx` around lines 145 - 151, The scrub
state is only being cleared by input-local end handlers so releasing the pointer
outside the range leaves scrubbing true and blocks timeupdate; update the
interaction to use pointer events and a global release handler: on pointerdown
(attach in the same component where you currently set scrubbing true, e.g., the
range input handlers around onSeekCommit / the corresponding start handler) call
setScrubbing(true) and call e.currentTarget.setPointerCapture(e.pointerId) if
available, and add a one-time window.addEventListener('pointerup', ...) (or
document) to call setScrubbing(false) and release capture; also ensure
onSeekCommit still calls seekTo and setScrubbing(false) for final commits. Apply
the same change to the other range handlers referenced (the block around lines
198-203) so scrubbing is reliably cleared when the pointer is released anywhere.
| .strand-player.is-playing:not(:hover):not(:focus-within) .strand-player__controls { | ||
| opacity: 0; | ||
| } | ||
| .strand-player.is-playing:not(:hover):not(:focus-within) { | ||
| cursor: none; | ||
| } |
There was a problem hiding this comment.
Keep the controls reachable on touch devices.
This hide rule depends entirely on :hover/:focus-within. On touch or coarse-pointer devices there is no real hover state, so once playback starts the controls stay transparent while the overlay still sits above the hit target. That makes seek, volume, and fullscreen effectively unavailable during playback.
Suggested CSS guard for non-hover devices
.strand-player.is-playing:not(:hover):not(:focus-within) .strand-player__controls {
opacity: 0;
}
.strand-player.is-playing:not(:hover):not(:focus-within) {
cursor: none;
}
+
+@media (hover: none), (pointer: coarse) {
+ .strand-player.is-playing .strand-player__controls {
+ opacity: 1;
+ }
+
+ .strand-player.is-playing {
+ cursor: auto;
+ }
+}🤖 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/frontend/styles/strand.css` around lines 614 - 619, The CSS rules that
hide controls when playing rely on :hover/:focus-within and break
touch/coarse-pointer devices; scope the hiding to only hover-capable devices by
wrapping the selectors .strand-player.is-playing:not(:hover):not(:focus-within)
and its .strand-player__controls rule inside a media query for hover-capable,
fine-pointer devices (e.g., media query using (hover: hover) and (pointer:
fine)), and likewise scope the cursor: none rule the same way so controls remain
reachable on touch devices while preserving the current behavior for mouse
users.
There was a problem hiding this comment.
Code Review
This pull request introduces a custom video player skin, StrandPlayer, which includes custom controls for playback, volume, and seeking, along with associated CSS styles. The createNativePlayer utility was updated to support a fullscreenTarget, allowing the custom UI to remain visible in fullscreen mode, and the Watch page was refactored to use this new component. Review feedback suggests extracting the time formatting logic into a shared utility to avoid duplication and addresses a UI flickering issue when seeking via keyboard by managing the scrubbing state on key down events.
| function fmt(total: number): string { | ||
| if (!Number.isFinite(total) || total < 0) return '0:00'; | ||
| const t = Math.floor(total); | ||
| const h = Math.floor(t / 3600); | ||
| const m = Math.floor((t % 3600) / 60); | ||
| const s = t % 60; | ||
| const mm = String(m).padStart(h > 0 ? 2 : 1, '0'); | ||
| const ss = String(s).padStart(2, '0'); | ||
| return h > 0 ? `${h}:${mm}:${ss}` : `${mm}:${ss}`; | ||
| } |
There was a problem hiding this comment.
The time formatting logic in fmt is identical to formatHms in src/frontend/pages/Watch.tsx. To improve maintainability and avoid duplication, consider extracting this logic into a shared utility function (e.g., in a new src/frontend/lib/time.ts or similar).
| <input | ||
| className="strand-player__seek" | ||
| type="range" | ||
| min={0} | ||
| max={duration > 0 ? duration : 0} | ||
| step="0.01" | ||
| value={current} | ||
| onMouseDown={() => setScrubbing(true)} | ||
| onTouchStart={() => setScrubbing(true)} | ||
| onChange={onSeekInput} | ||
| onMouseUp={onSeekCommit} | ||
| onTouchEnd={onSeekCommit} | ||
| onKeyUp={onSeekCommit} | ||
| aria-label="Seek" | ||
| /> |
There was a problem hiding this comment.
When using keyboard navigation (e.g., arrow keys) to seek, the scrubber will flicker or jump back to the current playhead position. This happens because scrubbing is only set to true on mouse/touch events, allowing the timeupdate listener to overwrite the current state with the video's old position before the seek is committed on onKeyUp. Adding onKeyDown to set scrubbing to true resolves this.
<input
className="strand-player__seek"
type="range"
min={0}
max={duration > 0 ? duration : 0}
step="0.01"
value={current}
onMouseDown={() => setScrubbing(true)}
onTouchStart={() => setScrubbing(true)}
onKeyDown={() => setScrubbing(true)}
onChange={onSeekInput}
onMouseUp={onSeekCommit}
onTouchEnd={onSeekCommit}
onKeyUp={onSeekCommit}
aria-label="Seek"
/>
Replaces the native UA controls bar on the Watch page with a custom overlay that matches the Strand design system: pill scrubber with buffered + played fills, play/pause + mute + fullscreen buttons, volume slider, monochrome OKLCH palette, auto-hide on idle. Extends the NativePlayer adapter with volume() / setVolume() and bufferedAhead() so the controls can render without reaching into the underlying <video>. The keyboard-shortcut layer (j/k/l, space, ←/→, f, m, 0–9) already lives on the Watch page and continues to work alongside the new overlay.
aloewright
force-pushed
the
conductor/alo-141-strand-themed-video-js-skin
branch
from
12a22a4 to
f85ced8
Compare
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
Closes ALO-141.
Summary
/watch/:idwith a Strand-themed custom overlay (PlayerControls) layered over the<video>element.NativePlayeradapter withvolume()/setVolume()(with clamp + auto-unmute) andbufferedAhead()so the controls render without poking at the underlying element. Also broadens the event surface (durationchange,playing,progress,volumechange,waiting,canplay).Files
src/frontend/components/PlayerControls.tsx— new overlay componentsrc/frontend/components/PlayerControls.test.ts— tests for the time-format helpersrc/frontend/lib/native-player.ts/.test.ts— new methods + coveragesrc/frontend/styles/strand.css— new.player,.player__bar,.player__scrubber*,.player__btn,.player__volume*styles using Strand tokenssrc/frontend/pages/Watch.tsx— dropsel.controls = true, mounts<PlayerControls />over the<video>Test plan
npm test— 498 passing (45 files)npm run lint— 0 warnings, AI Gateway guard cleannpm run type-check— cleannpm run build— builds; Watch chunk ~32.6 KB raw / ~9 KB gz🤖 Generated with Claude Code