Speed up video while holding spacebar#3390
Conversation
📝 WalkthroughWalkthroughAdds hold-to-speed playback via SPACE (temporary speed-up while held, restore on keyup), hardens player/container access, expands shortcuts API to handle keyup/state reset, integrates a keyboard-shortcuts overlay and settings menu item, plus styles and constants for the new UI and wiring. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Window as Browser Window
participant Shortcuts as VideoJS_Shortcuts
participant Player as VideoPlayer
User->>Window: keydown (SPACE)
Window->>Shortcuts: keydown handler -> handleKeyDown
Shortcuts->>Shortcuts: mark spacePressed, start hold timer (HOLD_SPEED_DELAY_MS)
alt short press (release before delay)
Shortcuts->>Player: togglePlay(containerRef, forcePlay?)
else hold exceeds delay
Shortcuts->>Shortcuts: set holding=true, save lastSpeed
Shortcuts->>Player: changePlaybackSpeed(shouldSpeedUp=true, newRate=FAST_SPEED)
end
User->>Window: keyup (SPACE)
Window->>Shortcuts: keyup handler -> handleKeyUp
Shortcuts->>Shortcuts: clear hold state, pendingToggle
Shortcuts->>Player: changePlaybackSpeed(shouldSpeedUp=false, newRate=lastSpeed)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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
🤖 Fix all issues with AI Agents
In @ui/component/viewers/videoViewer/internal/videojs-shortcuts.jsx:
- Around line 13-15: The module-level mutable variables holding, spacePressed,
and lastSpeed cause cross-instance leakage; move them into the component scope
of VideoJsShortcuts (e.g., as local let/const or useRef inside the
VideoJsShortcuts function) or initialize/reset them when the player
mounts/unmounts (cleanup in the player init/unload handlers); update any handler
closures to reference the new scoped variables (or passed refs from VideoJs.jsx)
so each player instance has its own independent state.
- Around line 152-157: handleKeyUp currently forwards every keyup to
handleSingleKeyActions causing non-spacebar shortcuts to run twice; change
handleKeyUp to only invoke handleSingleKeyActions for spacebar (and 'k')
releases (check e.key/e.code for Space/Spacebar and 'k') and stop calling
handleSingleKeyActions for other keys, and then remove/undo any keyup-specific
branches inside handleSingleKeyActions so that all other shortcuts (F, M, L, J,
arrows, number keys) are handled only on keydown.
🧹 Nitpick comments (2)
ui/component/viewers/videoViewer/internal/videojs-shortcuts.jsx (2)
103-108: Fragile state management when saving/restoring speed.When called to restore speed (on keyup), line 105 overwrites
lastSpeedwith the current fast speed. This works incidentally because the next hold re-saves the correct speed, but it's fragile and could cause bugs if the logic is extended.Consider only saving
lastSpeedwhen entering fast mode, not when restoring:🔎 Proposed fix
if (newRate !== -1) { rate = newRate; - lastSpeed = player.playbackRate(); + // Only save when speeding up, not when restoring + if (newRate === FAST_SPEED) { + lastSpeed = player.playbackRate(); + } } else { rate = player.playbackRate(); }
11-11: Consider making this configurable in a follow-up.Per the linked issue, allowing users to adjust the temporary speed is a desired enhancement. The current hardcoded value is acceptable for the initial implementation.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ui/component/viewers/videoViewer/internal/videojs-shortcuts.jsxui/component/viewers/videoViewer/internal/videojs.jsx
🧰 Additional context used
🧬 Code graph analysis (2)
ui/component/viewers/videoViewer/internal/videojs-shortcuts.jsx (1)
ui/component/viewers/videoViewer/internal/videojs.jsx (3)
containerRef(204-204)playerRef(203-203)player(598-598)
ui/component/viewers/videoViewer/internal/videojs.jsx (1)
ui/component/viewers/videoViewer/internal/videojs-shortcuts.jsx (3)
createKeyUpShortcutsHandler(270-274)createVideoScrollShortcutsHandler(275-279)createVolumePanelScrollShortcutsHandler(280-284)
🪛 Biome (2.1.2)
ui/component/viewers/videoViewer/internal/videojs-shortcuts.jsx
[error] 97-97: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 152-152: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 270-270: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 270-270: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 271-271: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
🔇 Additional comments (6)
ui/component/viewers/videoViewer/internal/videojs.jsx (2)
211-211: LGTM!The
keyUpHandlerRefand handler wiring follows the established pattern forkeyDownHandler. Event listener registration and ref storage are correctly implemented.Also applies to: 448-459
669-669: LGTM!Proper cleanup of the keyup event listener in the useEffect teardown, matching the keydown cleanup pattern.
ui/component/viewers/videoViewer/internal/videojs-shortcuts.jsx (4)
91-95: LGTM!The
forcePlayparameter correctly enables forcing playback when transitioning to hold state, and the added null guard oncontainerRefimproves robustness.
171-191: Hold-to-speed-up logic is well-structured.The state machine correctly differentiates between initial press (toggle play), held state (speed up), and release (restore speed). The key repeat detection approach is pragmatic.
270-274: LGTM!The handler factory follows the established pattern of the other shortcut handlers.
Also applies to: 288-288
1-1: Static analysis warnings are false positives.The Biome errors about "TypeScript only syntax" can be ignored - this file uses Flow type annotations (indicated by
// @flow), which is the type system used in this project.
ui/component/viewers/videoViewer/internal/videojs-shortcuts.jsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ui/component/viewers/videoViewer/internal/videojs-shortcuts.jsx (2)
108-132: Refactor: Avoid overwriting lastSpeed when restoring.The function overwrites
lastSpeedon line 116 even when restoring the saved speed, creating a confusing circular dependency:
- Speed up (line 187):
lastSpeedcorrectly saves the current rate (e.g., 1.0), then sets to 2x.- Restore (line 160): passes
lastSpeed(1.0) asnewRate, but line 116 immediately overwriteslastSpeedwith the current rate (2.0).This works because
lastSpeedis reset on the next speed-up, but it's semantically incorrect and fragile if speed changes occur between hold events.🔎 Cleaner approach: Don't save when restoring
Option 1: Add a boolean flag to skip saving lastSpeed when restoring:
-function changePlaybackSpeed(shouldSpeedUp: boolean, playerRef, newRate = -1) { +function changePlaybackSpeed(shouldSpeedUp: boolean, playerRef, newRate = -1, saveLastSpeed = true) { const player = playerRef.current; if (!player) return; const isSpeedUp = shouldSpeedUp; let rate; if (newRate !== -1) { rate = newRate; - lastSpeed = player.playbackRate(); + if (saveLastSpeed) { + lastSpeed = player.playbackRate(); + } } else { rate = player.playbackRate(); } // ... rest of function }Then call with
changePlaybackSpeed(true, playerRef, lastSpeed, false)when restoring.Option 2: Directly set playback rate in handleKeyUp without calling changePlaybackSpeed:
if (holding) { - changePlaybackSpeed(true, playerRef, lastSpeed); + const player = playerRef.current; + if (player) { + player.playbackRate(lastSpeed); + OVERLAY.showPlaybackRateOverlay(player, lastSpeed, false); + player.userActive(true); + } }Minor: Silent failure if FAST_SPEED not in VIDEO_PLAYBACK_RATES.
If
FAST_SPEED(or anynewRatevalue) is not found inVIDEO_PLAYBACK_RATESarray (line 121),rateIndexwill be -1 and the condition on line 122 fails, causing silent failure with no user feedback.
179-191: Optional: Hold detection relies on OS keyboard repeat timing.The current implementation detects a "hold" when the OS sends a repeat keydown event (typically after ~500ms initial delay). This works but has drawbacks:
- OS-dependent: Repeat timing varies by platform and user settings.
- Delay: Users may perceive 500ms as sluggish for a "hold to speed up" feature.
- Non-standard: Most apps use explicit timers for long-press detection.
🔎 Alternative: Use setTimeout for predictable long-press detection
+ let holdTimer = null; + if (e.keyCode === KEYCODES.SPACEBAR || e.keyCode === KEYCODES.K) { e.preventDefault(); if (!spacePressed) { togglePlay(containerRef); + // Start timer to detect long press + holdTimer = setTimeout(() => { + if (spacePressed && !holding) { + holding = true; + togglePlay(containerRef, true); + changePlaybackSpeed(true, playerRef, FAST_SPEED); + } + }, 300); // 300ms feels more responsive than waiting for OS repeat - } else { - if (!holding) { - holding = true; - togglePlay(containerRef, true); - changePlaybackSpeed(true, playerRef, FAST_SPEED); - } } spacePressed = true; }And in handleKeyUp:
if (e.keyCode === KEYCODES.SPACEBAR || e.keyCode === KEYCODES.K) { e.preventDefault(); + if (holdTimer) { + clearTimeout(holdTimer); + holdTimer = null; + } if (holding) { changePlaybackSpeed(true, playerRef, lastSpeed); } spacePressed = false; holding = false; }This provides consistent ~300ms detection across all platforms.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ui/component/viewers/videoViewer/internal/videojs-shortcuts.jsx
🧰 Additional context used
🧬 Code graph analysis (1)
ui/component/viewers/videoViewer/internal/videojs-shortcuts.jsx (1)
ui/component/viewers/videoViewer/internal/videojs.jsx (4)
videoNode(434-434)containerRef(204-204)playerRef(203-203)player(598-598)
🪛 Biome (2.1.2)
ui/component/viewers/videoViewer/internal/videojs-shortcuts.jsx
[error] 108-108: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 152-152: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 270-270: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 270-270: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 271-271: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
🔇 Additional comments (5)
ui/component/viewers/videoViewer/internal/videojs-shortcuts.jsx (5)
11-11: Good default; consider making configurable as noted.The 2x speed is a reasonable default for the hold-to-speed feature. As your comment suggests, making this user-configurable would be a nice enhancement for a follow-up PR.
152-165: Excellent fix: Keyup now only handles spacebar/K.This correctly addresses the previous critical issue where all shortcuts were firing twice (on both keydown and keyup). Now only the spacebar/K hold-to-speed functionality is processed on keyup, while other shortcuts (M, F, L, J, arrows, number keys) remain keydown-only.
270-274: LGTM: Keyup handler follows existing patterns.The new
createKeyUpShortcutsHandlerfollows the same currying pattern as the existing keydown handler and is correctly exported in the public API.Also applies to: 288-288
87-91: LGTM: togglePlay correctly handles forcePlay parameter.The new
forcePlayparameter properly ensures the video plays when needed (line 186 during hold-to-speed). The logic correctly handles all cases, including callingplay()when already playing (which is a harmless no-op).
104-106: State isolation is correctly implemented.The variables
holding,spacePressed, andlastSpeedare properly scoped within theVideoJsShorcutsfunction, which is instantiated once perVideoJscomponent instance (one per player). Each player maintains independent state, and event listeners are correctly cleaned up on unmount. No further action needed.
|
Thanks for the PR and addressing some of the comments from coderabbit! Gonna give it a review and whirl soon! |
tzarebczan
force-pushed
the
speed-up-space
branch
from
74fca56 to
1ce14d8
Compare
|
Thought this was a good opportunity to display all the keyboard shortcuts as a shortcut (shift ?) and in the settings cog. I'm pushing up these changes to our staging environment at salt.odysee.tv to play around with . I also improved that short pause that would happen when holding the spacebar, should be smooth now. Let us know what you think / if you find any issues! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
ui/component/viewers/videoViewer/internal/keyboard-shortcuts-overlay.scss (1)
148-152: Minor: Simplify redundant padding value.
padding: 12px 12pxcan be simplified topadding: 12px.♻️ Suggested fix
.vjs-shortcuts-card { max-height: calc(100% - 16px); - padding: 12px 12px; + padding: 12px; width: calc(100% - 24px); }ui/component/viewers/videoViewer/internal/plugins/videojs-settings-menu/menuItems/KeyboardShortcutsMenuItem.js (1)
30-40: LGTM with optional simplification.The defensive checks are appropriate. Consider using optional chaining for cleaner access:
♻️ Optional: Use optional chaining
handleClick() { const player = this.player_; - if (player && typeof player.toggleKeyboardShortcutsOverlay === 'function') { - player.toggleKeyboardShortcutsOverlay(true); - } + player?.toggleKeyboardShortcutsOverlay?.(true); - const menuButton = this.menu && this.menu.options_ && this.menu.options_.menuButton; - if (menuButton && typeof menuButton.hideMenu === 'function') { - menuButton.hideMenu(); - } + this.menu?.options_?.menuButton?.hideMenu?.(); }ui/component/viewers/videoViewer/internal/keyboard-shortcuts-overlay.js (1)
129-139: Consider adding Escape key to close the overlay.Pressing Escape is a common and expected UX pattern for closing dialogs/modals. Currently, users can only close the overlay by clicking the close button or clicking outside the card.
♻️ Suggested implementation
if (toggleButton) { toggleButton.addEventListener('click', () => setExpanded(!isExpanded)); } overlayEl.addEventListener('click', (event) => { if (event.target === overlayEl) close(); }); + overlayEl.addEventListener('keydown', (event) => { + if (event.key === 'Escape' || event.keyCode === 27) { + close(); + } + }); + player.on(VJS_EVENTS.PLAYER_CLOSED, () => close());
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
ui/component/viewers/videoViewer/internal/keyboard-shortcuts-overlay.jsui/component/viewers/videoViewer/internal/keyboard-shortcuts-overlay.scssui/component/viewers/videoViewer/internal/plugins/videojs-settings-menu/menuItems/KeyboardShortcutsMenuItem.jsui/component/viewers/videoViewer/internal/plugins/videojs-settings-menu/plugin.jsui/component/viewers/videoViewer/internal/videojs-shortcuts.jsxui/component/viewers/videoViewer/internal/videojs.jsxui/constants/keycodes.jsui/constants/player.js
🧰 Additional context used
🧬 Code graph analysis (3)
ui/component/viewers/videoViewer/internal/plugins/videojs-settings-menu/plugin.js (1)
ui/constants/player.js (2)
VJS_COMP(22-27)VJS_COMP(22-27)
ui/component/viewers/videoViewer/internal/keyboard-shortcuts-overlay.js (2)
ui/component/viewers/videoViewer/internal/videojs-shortcuts.jsx (12)
player(24-24)player(35-35)player(44-44)player(69-69)player(80-80)player(114-114)player(147-147)player(164-164)player(174-174)player(182-182)player(279-279)player(297-297)ui/constants/player.js (2)
VJS_EVENTS(30-36)VJS_EVENTS(30-36)
ui/component/viewers/videoViewer/internal/videojs.jsx (2)
ui/component/viewers/videoViewer/internal/keyboard-shortcuts-overlay.js (1)
ensureKeyboardShortcutsOverlay(69-155)ui/component/viewers/videoViewer/internal/videojs-shortcuts.jsx (4)
createKeyUpShortcutsHandler(320-324)createKeyStateResetHandler(325-330)createVideoScrollShortcutsHandler(331-335)createVolumePanelScrollShortcutsHandler(336-340)
🪛 Biome (2.1.2)
ui/component/viewers/videoViewer/internal/videojs.jsx
[error] 85-85: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 476-476: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 476-476: expected => but instead found .
Remove .
(parse)
[error] 695-695: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 695-695: expected => but instead found .
Remove .
(parse)
ui/component/viewers/videoViewer/internal/videojs-shortcuts.jsx
[error] 100-106: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 113-113: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 181-181: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 200-200: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 320-320: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 320-320: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 321-321: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 325-325: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 326-326: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
🔇 Additional comments (17)
ui/constants/keycodes.js (1)
43-43: LGTM!The SLASH keycode constant (191) is correctly added and follows the established pattern in this file.
ui/constants/player.js (1)
25-25: LGTM!The new constant follows the existing naming convention and integrates well with the other Video.js component identifiers.
ui/component/viewers/videoViewer/internal/plugins/videojs-settings-menu/plugin.js (1)
17-23: LGTM!The keyboard shortcuts menu item is cleanly integrated into the display order using the centralized constant from
VJS_COMP.ui/component/viewers/videoViewer/internal/videojs-shortcuts.jsx (5)
107-111: Good fix: State variables moved to function scope.Moving
holding,spacePressed,lastSpeed, and addingholdTimeoutId/pendingToggleinside theVideoJsShorcutsfunction addresses the previously reported cross-instance state leakage issue.
181-198: Good fix: keyup handler now scoped to spacebar/K only.This correctly addresses the previous issue where all shortcuts would fire on both keydown and keyup. Now only the hold-to-speed logic processes keyup events.
219-241: Well-designed hold-to-speed-up logic.The implementation correctly handles all edge cases:
- Tap to toggle play/pause
- Hold to speed up after 300ms delay
- Proper state management for pending toggles vs. hold actions
The delay mechanism using
setTimeoutwithpendingToggleensures taps still function as expected while allowing holds to trigger speed-up.
1-1: Static analysis false positive: Flow annotations are intentional.The Biome warnings about "TypeScript only syntax" are false positives. This file uses Flow type annotations (as indicated by
// @flow), which is the type system used in this codebase. No changes needed.
325-330: Handler correctly wired to bothblurandvisibilitychangeevents.The
createKeyStateResetHandleris properly attached at lines 475–476 in videojs.jsx with correct event listener cleanup (lines 694–695). The conditional logic correctly skips reset only when visibilitychange fires as the document becomes visible, ensuring stale hold state is cleared on blur and when visibility is lost.ui/component/viewers/videoViewer/internal/videojs.jsx (9)
6-6: LGTM! Clean imports for the keyboard shortcuts overlay feature.The new imports are well-organized and follow the existing patterns in the file.
Also applies to: 20-20
83-90: LGTM! Type definitions correctly reflect the keyboard shortcuts overlay API.The Flow type annotations accurately describe the overlay interface from
keyboard-shortcuts-overlay.js. The static analysis error on line 85 is a false positive—Biome is misinterpreting valid Flow syntax as JavaScript.
221-222: LGTM! Refs follow the established cleanup pattern.The new refs are consistent with the existing
keyDownHandlerRefpattern and are properly used for event listener cleanup.
233-237: LGTM! Well-defended toggle function.The defensive checks ensure safe invocation even if the overlay hasn't been initialized yet, making this wrapper robust against timing issues.
240-253: LGTM! Keyboard shortcuts initialization properly extended.The new handler factories and
toggleKeyboardShortcutsOverlaycallback are cleanly integrated into the existing initialization pattern.
460-460: LGTM! Overlay initialization properly placed.Calling
ensureKeyboardShortcutsOverlayafter the player is ready ensures the overlay is available for both new and reused player instances. The function's idempotent and defensive nature makes this safe.
469-476: LGTM! Handler setup follows the established pattern.The new handlers are created and wired consistently with the existing
keyDownHandlerapproach. UsingkeyStateResetHandlerfor bothblurandvisibilitychangeevents ensures hold-state is properly reset when the window loses focus or becomes hidden.
482-483: LGTM! Handler refs properly stored for cleanup.Storing the handlers in refs enables proper cleanup in the effect's return function.
693-695: LGTM! Event listener cleanup is complete and correct.All three new event listeners (
keyup,blur,visibilitychange) are properly removed in the cleanup function. The static analysis errors on lines 476 and 695 are false positives—the(document: any)syntax is valid Flow type casting, not TypeScript.
|
Looks good to me, one nitpick i have is the font-size of the shortcuts descriptions (its a kinda small) and the whole compact logic is a bit redundant(?) let me know about that. The following commit i'm fixing forceState not being passed with toggleKeyboardShortcutsOverlay and using that to close the shortcuts menu with ESC. |
|
Thanks for adding the Esc option. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @ui/component/viewers/videoViewer/internal/keyboard-shortcuts-overlay.scss:
- Around line 161-163: Remove the orphaned duplicate CSS rule for
.vjs-shortcuts-actions (the standalone block setting justify-self: end) that
exists outside the .video-js scope; keep the correctly scoped declaration inside
.video-js (the existing rule around .video-js .vjs-shortcuts-actions) and delete
the out-of-scope block to avoid unintended global styling.
🧹 Nitpick comments (1)
ui/component/viewers/videoViewer/internal/keyboard-shortcuts-overlay.js (1)
127-128: Consider consolidating the dual API surface.Both
player.keyboardShortcutsOverlay(object with methods) andplayer.toggleKeyboardShortcutsOverlay(direct function reference) are exposed. While this provides flexibility, it introduces redundancy. Consider whether both are necessary or if consumers could useplayer.keyboardShortcutsOverlay.toggleexclusively.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ui/component/viewers/videoViewer/internal/keyboard-shortcuts-overlay.jsui/component/viewers/videoViewer/internal/keyboard-shortcuts-overlay.scssui/component/viewers/videoViewer/internal/videojs.jsx
🧰 Additional context used
🧬 Code graph analysis (2)
ui/component/viewers/videoViewer/internal/keyboard-shortcuts-overlay.js (1)
ui/constants/player.js (2)
VJS_EVENTS(30-36)VJS_EVENTS(30-36)
ui/component/viewers/videoViewer/internal/videojs.jsx (2)
ui/component/viewers/videoViewer/internal/keyboard-shortcuts-overlay.js (1)
ensureKeyboardShortcutsOverlay(68-131)ui/component/viewers/videoViewer/internal/videojs-shortcuts.jsx (4)
createKeyUpShortcutsHandler(321-325)createKeyStateResetHandler(326-331)createVideoScrollShortcutsHandler(332-336)createVolumePanelScrollShortcutsHandler(337-341)
🪛 Biome (2.1.2)
ui/component/viewers/videoViewer/internal/videojs.jsx
[error] 85-85: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 232-232: optional parameters are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 232-232: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 475-475: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 475-475: expected => but instead found .
Remove .
(parse)
[error] 694-694: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 694-694: expected => but instead found .
Remove .
(parse)
🔇 Additional comments (10)
ui/component/viewers/videoViewer/internal/keyboard-shortcuts-overlay.js (2)
68-82: Defensive null-checks look good.The early returns for missing
playerorplayer.el()prevent errors. The idempotency check (returning existing overlay if already initialized) is also appropriate.
114-116: Verify backdrop click behavior.The click handler closes the overlay when clicking directly on
overlayEl(the backdrop). Ensure that clicks on child elements (e.g., the card content) don't bubble up and inadvertently close the overlay. The current implementation should work correctly sinceevent.target === overlayElchecks for exact target match, but manual testing is recommended.ui/component/viewers/videoViewer/internal/keyboard-shortcuts-overlay.scss (1)
1-160: Styling structure looks clean and responsive.The overlay uses flexbox and grid layouts appropriately, includes proper z-index layering, and provides responsive adjustments via media queries. Accessibility is supported through ARIA attributes (managed in the JS file).
ui/component/viewers/videoViewer/internal/videojs.jsx (7)
6-6: LGTM on the new imports.The SCSS import and
ensureKeyboardShortcutsOverlayfunction import are properly placed and integrate cleanly with existing imports.Also applies to: 20-20
83-89: Player type extensions are well-defined.The optional properties for
keyboardShortcutsOverlayandtoggleKeyboardShortcutsOverlayare correctly typed and align with the API exposed byensureKeyboardShortcutsOverlay.
232-236: Good defensive programming for toggle function.Checking that
toggleKeyboardShortcutsOverlayexists before invoking it prevents runtime errors if the overlay hasn't been initialized.
459-459: Overlay initialization properly sequenced.Calling
ensureKeyboardShortcutsOverlay(vjsPlayer)after the player is assigned toplayerRef.currentensures the overlay is attached to a fully initialized player instance.
468-475: Proper handler creation and registration.The keyUp and keyStateReset handlers are created, registered with appropriate event listeners (keyup, blur, visibilitychange), and stored in refs for cleanup. This ensures hold-state is reset when the window loses focus or visibility changes.
Also applies to: 481-482
692-694: Cleanup properly removes all new event listeners.The three event listeners (keyup, blur, visibilitychange) are removed in the cleanup function, preventing memory leaks.
1-770: Ignore static analysis false positives.The Biome errors flagged in the static analysis hints are false positives. This file uses Flow type annotations (indicated by
// @flowat line 1), not TypeScript. The reported "TypeScript only syntax" errors and semicolon warnings do not apply to Flow and can be safely ignored. Consider configuring Biome to recognize Flow syntax or disable it for Flow files.
ui/component/viewers/videoViewer/internal/keyboard-shortcuts-overlay.scss
Outdated
Show resolved
Hide resolved
Updates the keyboard shortcuts menu item to be hidden on mobile devices. This improves the user experience by preventing the display of irrelevant options on touch-based devices.
Removes the "Show all/Show fewer" toggle button from the keyboard shortcuts overlay for a cleaner and more streamlined user experience. The overlay now displays all shortcuts by default, improving discoverability and reducing user interaction.
Removes the `vjs-shortcuts-actions` style rule as it's no longer needed, simplifying the stylesheet and potentially improving performance.
tzarebczan
force-pushed
the
speed-up-space
branch
from
8fa9514 to
67cbe52
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @ui/component/viewers/videoViewer/internal/videojs-shortcuts.jsx:
- Around line 113-137: The changePlaybackSpeed function currently overwrites
lastSpeed whenever newRate !== -1, which can clobber the saved rate; update
changePlaybackSpeed (and/or restorePlaybackRate) so lastSpeed is only set when
initiating an explicit hold-to-speed change (e.g., set lastSpeed to
player.playbackRate() only if lastSpeed is unset or when a dedicated "startHold"
flag is passed) and do not overwrite lastSpeed on subsequent calls with newRate;
alternatively, save the current playbackRate into lastSpeed immediately before
calling changePlaybackSpeed(FAST_SPEED) so restorePlaybackRate can rely on an
immutable saved value.
🧹 Nitpick comments (1)
ui/component/viewers/videoViewer/internal/videojs-shortcuts.jsx (1)
11-12: Consider making FAST_SPEED configurable.The hardcoded value of 2x aligns with the current implementation, but as noted in the comment and PR objectives, allowing users to customize this value would enhance the feature.
Do you want me to open an issue to track making the temporary speed-up value user-configurable?
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
ui/component/viewers/videoViewer/internal/keyboard-shortcuts-overlay.jsui/component/viewers/videoViewer/internal/keyboard-shortcuts-overlay.scssui/component/viewers/videoViewer/internal/plugins/videojs-settings-menu/menuItems/KeyboardShortcutsMenuItem.jsui/component/viewers/videoViewer/internal/plugins/videojs-settings-menu/plugin.jsui/component/viewers/videoViewer/internal/videojs-shortcuts.jsxui/component/viewers/videoViewer/internal/videojs.jsxui/constants/keycodes.jsui/constants/player.js
🚧 Files skipped from review as they are similar to previous changes (3)
- ui/constants/player.js
- ui/component/viewers/videoViewer/internal/plugins/videojs-settings-menu/plugin.js
- ui/component/viewers/videoViewer/internal/keyboard-shortcuts-overlay.js
🧰 Additional context used
🧬 Code graph analysis (2)
ui/component/viewers/videoViewer/internal/videojs-shortcuts.jsx (2)
ui/component/viewers/videoViewer/internal/videojs.jsx (5)
videoNode(452-452)containerRef(213-213)playerRef(212-212)player(621-621)toggleKeyboardShortcutsOverlay(232-236)ui/constants/player.js (2)
VIDEO_PLAYBACK_RATES(11-11)VIDEO_PLAYBACK_RATES(11-11)
ui/component/viewers/videoViewer/internal/videojs.jsx (3)
ui/component/viewers/videoViewer/view.jsx (1)
playerRef(200-200)ui/component/viewers/videoViewer/internal/keyboard-shortcuts-overlay.js (1)
ensureKeyboardShortcutsOverlay(68-131)ui/component/viewers/videoViewer/internal/videojs-shortcuts.jsx (4)
createKeyUpShortcutsHandler(321-325)createKeyStateResetHandler(326-331)createVideoScrollShortcutsHandler(332-336)createVolumePanelScrollShortcutsHandler(337-341)
🪛 Biome (2.1.2)
ui/component/viewers/videoViewer/internal/videojs-shortcuts.jsx
[error] 100-106: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 113-113: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 181-181: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 200-200: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 321-321: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 321-321: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 322-322: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 326-326: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 327-327: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
ui/component/viewers/videoViewer/internal/videojs.jsx
[error] 85-85: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 232-232: optional parameters are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 232-232: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 475-475: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 475-475: expected => but instead found .
Remove .
(parse)
[error] 694-694: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 694-694: expected => but instead found .
Remove .
(parse)
🔇 Additional comments (12)
ui/constants/keycodes.js (1)
43-43: LGTM!Clean addition of the SLASH keycode constant to support the new keyboard shortcuts overlay toggle functionality.
ui/component/viewers/videoViewer/internal/keyboard-shortcuts-overlay.scss (1)
1-160: LGTM!Well-structured SCSS for the keyboard shortcuts overlay with:
- Proper positioning and z-index management
- Clean responsive design for mobile devices
- Modern CSS layout techniques (grid, flexbox)
- Appropriate visual hierarchy
ui/component/viewers/videoViewer/internal/plugins/videojs-settings-menu/menuItems/KeyboardShortcutsMenuItem.js (1)
1-54: LGTM!Well-implemented menu item component with:
- Proper defensive checks for player and menu button availability
- Appropriate platform-specific visibility (hidden on mobile)
- Clean integration with Video.js component system
ui/component/viewers/videoViewer/internal/videojs.jsx (3)
6-6: Clean integration of keyboard shortcuts overlay.The imports are properly structured and the SCSS is loaded alongside the overlay initialization logic.
Also applies to: 20-20
220-221: Excellent handler lifecycle management.The implementation properly:
- Creates and stores handler refs for cleanup
- Defensively checks player availability before invoking overlay methods
- Registers all necessary event listeners (keydown, keyup, blur, visibilitychange)
- Initializes the overlay after the player is ready
Also applies to: 232-252, 459-484
691-694: Proper cleanup of event listeners.The cleanup correctly removes all newly added event listeners (keyup, blur, visibilitychange), preventing memory leaks.
ui/component/viewers/videoViewer/internal/videojs-shortcuts.jsx (6)
88-92: LGTM!The refactored
togglePlaywithforcePlayparameter correctly handles the logic for the hold-to-speed-up feature. The element lookup is properly guarded.
139-161: Well-structured state management helpers.The helper functions (
clearHoldTimeout,restorePlaybackRate,resetHoldState) provide clean lifecycle management for the hold-to-speed-up feature and handle window blur/tab visibility changes appropriately.
181-198: LGTM!The
handleKeyUpimplementation correctly handles key release for both tap-to-toggle and hold-to-speed-up scenarios. The state machine properly:
- Clears the pending timeout
- Restores speed if held
- Toggles play if tapped quickly
- Resets all state flags
202-208: Clean keyboard shortcuts overlay integration.The Shift+Slash toggle and Escape close functionality are properly integrated with appropriate defensive checks for the optional
toggleKeyboardShortcutsOverlayprop.Also applies to: 243-243
321-331: LGTM!The new handler factory functions follow the established pattern and are properly exported for consumption by the VideoJs component.
Also applies to: 343-349
219-241: Remove the null safety concern—togglePlay safely guards against null videoNode.The early return at line 176 ensures
videoNodeexists whenhandleSingleKeyActionsis called. While line 226 re-queries the DOM (and could theoretically return null),togglePlayitself has an internal null-safety guard at line 89 (if (!videoNode) return;) that prevents any incorrect behavior. The code is safe as written.Likely an incorrect or invalid review comment.
| function changePlaybackSpeed(shouldSpeedUp: boolean, playerRef, newRate = -1) { | ||
| const player = playerRef.current; | ||
| if (!player) return; | ||
| const isSpeedUp = shouldSpeedUp; | ||
| let rate; | ||
|
|
||
| if (newRate !== -1) { | ||
| rate = newRate; | ||
| lastSpeed = player.playbackRate(); | ||
| } else { | ||
| rate = player.playbackRate(); | ||
| } | ||
|
|
||
| let rateIndex = VIDEO_PLAYBACK_RATES.findIndex((x) => x === rate); | ||
| if (rateIndex >= 0) { | ||
| if (newRate === -1) { | ||
| rateIndex = isSpeedUp ? Math.min(rateIndex + 1, VIDEO_PLAYBACK_RATES.length - 1) : Math.max(rateIndex - 1, 0); | ||
| } | ||
| const nextRate = VIDEO_PLAYBACK_RATES[rateIndex]; | ||
|
|
||
| OVERLAY.showPlaybackRateOverlay(player, nextRate, isSpeedUp); | ||
| player.userActive(true); | ||
| player.playbackRate(nextRate); | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential state management issue in changePlaybackSpeed.
When newRate !== -1, line 121 unconditionally overwrites lastSpeed with the current playback rate. This works currently because restorePlaybackRate is only called once per hold cycle (the holding flag prevents multiple calls). However, this creates a fragile pattern where calling restorePlaybackRate multiple times would restore to the wrong speed.
Consider separating the logic:
- When setting a new explicit rate for hold-to-speed-up, save the current rate separately
- When restoring, don't overwrite the saved rate
♻️ Suggested refactor to make state management more robust
function changePlaybackSpeed(shouldSpeedUp: boolean, playerRef, newRate = -1) {
const player = playerRef.current;
if (!player) return;
const isSpeedUp = shouldSpeedUp;
let rate;
if (newRate !== -1) {
rate = newRate;
- lastSpeed = player.playbackRate();
} else {
+ lastSpeed = player.playbackRate();
rate = player.playbackRate();
}
let rateIndex = VIDEO_PLAYBACK_RATES.findIndex((x) => x === rate);
if (rateIndex >= 0) {
if (newRate === -1) {
rateIndex = isSpeedUp ? Math.min(rateIndex + 1, VIDEO_PLAYBACK_RATES.length - 1) : Math.max(rateIndex - 1, 0);
}
const nextRate = VIDEO_PLAYBACK_RATES[rateIndex];
OVERLAY.showPlaybackRateOverlay(player, nextRate, isSpeedUp);
player.userActive(true);
player.playbackRate(nextRate);
}
}Alternatively, save lastSpeed explicitly before calling changePlaybackSpeed with FAST_SPEED in line 239.
📝 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.
| function changePlaybackSpeed(shouldSpeedUp: boolean, playerRef, newRate = -1) { | |
| const player = playerRef.current; | |
| if (!player) return; | |
| const isSpeedUp = shouldSpeedUp; | |
| let rate; | |
| if (newRate !== -1) { | |
| rate = newRate; | |
| lastSpeed = player.playbackRate(); | |
| } else { | |
| rate = player.playbackRate(); | |
| } | |
| let rateIndex = VIDEO_PLAYBACK_RATES.findIndex((x) => x === rate); | |
| if (rateIndex >= 0) { | |
| if (newRate === -1) { | |
| rateIndex = isSpeedUp ? Math.min(rateIndex + 1, VIDEO_PLAYBACK_RATES.length - 1) : Math.max(rateIndex - 1, 0); | |
| } | |
| const nextRate = VIDEO_PLAYBACK_RATES[rateIndex]; | |
| OVERLAY.showPlaybackRateOverlay(player, nextRate, isSpeedUp); | |
| player.userActive(true); | |
| player.playbackRate(nextRate); | |
| } | |
| } | |
| function changePlaybackSpeed(shouldSpeedUp: boolean, playerRef, newRate = -1) { | |
| const player = playerRef.current; | |
| if (!player) return; | |
| const isSpeedUp = shouldSpeedUp; | |
| let rate; | |
| if (newRate !== -1) { | |
| rate = newRate; | |
| } else { | |
| lastSpeed = player.playbackRate(); | |
| rate = player.playbackRate(); | |
| } | |
| let rateIndex = VIDEO_PLAYBACK_RATES.findIndex((x) => x === rate); | |
| if (rateIndex >= 0) { | |
| if (newRate === -1) { | |
| rateIndex = isSpeedUp ? Math.min(rateIndex + 1, VIDEO_PLAYBACK_RATES.length - 1) : Math.max(rateIndex - 1, 0); | |
| } | |
| const nextRate = VIDEO_PLAYBACK_RATES[rateIndex]; | |
| OVERLAY.showPlaybackRateOverlay(player, nextRate, isSpeedUp); | |
| player.userActive(true); | |
| player.playbackRate(nextRate); | |
| } | |
| } |
🧰 Tools
🪛 Biome (2.1.2)
[error] 113-113: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
🤖 Prompt for AI Agents
In @ui/component/viewers/videoViewer/internal/videojs-shortcuts.jsx around lines
113 - 137, The changePlaybackSpeed function currently overwrites lastSpeed
whenever newRate !== -1, which can clobber the saved rate; update
changePlaybackSpeed (and/or restorePlaybackRate) so lastSpeed is only set when
initiating an explicit hold-to-speed change (e.g., set lastSpeed to
player.playbackRate() only if lastSpeed is unset or when a dedicated "startHold"
flag is passed) and do not overwrite lastSpeed on subsequent calls with newRate;
alternatively, save the current playbackRate into lastSpeed immediately before
calling changePlaybackSpeed(FAST_SPEED) so restorePlaybackRate can rely on an
immutable saved value.
2 participants
Closes #3388
What is the current behavior?
Only way to speed up video playback is using shift + comma/dot or via settings, there is no way to temporarily speed it up to skip over a section quickly
What is the new behavior?
This PR adds the mentioned functionality when holding the spacebar or 'k'
PR Checklist
Toggle...
What kind of change does this PR introduce?
Please check all that apply to this PR using "x":
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.