feat(editor): support keyboard shortcuts for timeline start/end navigation (fixes #2120) - #2292
shuoYun114 wants to merge 3 commits into
Conversation
| if (editorState.playing) { | ||
| await commands.stopPlayback(); | ||
| setEditorState("playing", false); | ||
| } | ||
| setEditorState("playbackTime", 0); | ||
| setEditorState("previewTime", null); | ||
| if (!handoffPlaybackPending()) { | ||
| await commands.seekTo(0); | ||
| } |
There was a problem hiding this comment.
During a preparing-playback handoff, active native playback is represented by the handoff state rather than editorState.playing. These handlers therefore skip stopPlayback(), then skip seekTo() because a handoff is pending, without sending the target through requestHandoffPlayback(). The native player continues from its old position and overwrites the local time update, so all four shortcuts are ineffective in this state. Route these seeks through the existing handoff-aware path used by the previous and next controls.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/routes/editor/Player.tsx
Line: 367-375
Comment:
**Shortcuts Fail During Handoff**
During a preparing-playback handoff, active native playback is represented by the handoff state rather than `editorState.playing`. These handlers therefore skip `stopPlayback()`, then skip `seekTo()` because a handoff is pending, without sending the target through `requestHandoffPlayback()`. The native player continues from its old position and overwrites the local time update, so all four shortcuts are ineffective in this state. Route these seeks through the existing handoff-aware path used by the previous and next controls.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| }, | ||
| }, | ||
| { | ||
| combo: "ArrowUp", |
There was a problem hiding this comment.
The new document-level ArrowUp and ArrowDown shortcuts also run when an editor overlay is selected. Existing canvas and text overlay handlers use the same events to nudge the selected element without stopping propagation, so one keypress now both moves the element and seeks the playhead to the start or end. Suppress the timeline shortcut while an overlay owns the arrow keys, or otherwise make these actions mutually exclusive.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/routes/editor/Player.tsx
Line: 365
Comment:
**Arrow Shortcuts Trigger Twice**
The new document-level ArrowUp and ArrowDown shortcuts also run when an editor overlay is selected. Existing canvas and text overlay handlers use the same events to nudge the selected element without stopping propagation, so one keypress now both moves the element and seeks the playhead to the start or end. Suppress the timeline shortcut while an overlay owns the arrow keys, or otherwise make these actions mutually exclusive.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| return; | ||
| } | ||
|
|
||
| if (event.key === "ArrowUp" || event.key === "Home") { | ||
| event.preventDefault(); | ||
| playback.seek(0); | ||
| return; | ||
| } | ||
|
|
||
| if (event.key === "ArrowDown" || event.key === "End") { | ||
| event.preventDefault(); | ||
| playback.seek(playback.getDuration()); | ||
| return; | ||
| } |
There was a problem hiding this comment.
These parent-level ArrowUp and ArrowDown branches also receive events from the focusable TimelineRail, which is exposed as an ARIA slider. A focused slider should increment with ArrowUp and decrement with ArrowDown, but this handler instead jumps to the absolute start or end and prevents the default event. This prevents keyboard users from operating the slider with its expected controls.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/app/s/[videoId]/_components/timeline/TimelineView.tsx
Line: 425-438
Comment:
**Slider Keys Are Hijacked**
These parent-level ArrowUp and ArrowDown branches also receive events from the focusable `TimelineRail`, which is exposed as an ARIA slider. A focused slider should increment with ArrowUp and decrement with ArrowDown, but this handler instead jumps to the absolute start or end and prevents the default event. This prevents keyboard users from operating the slider with its expected controls.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| function createTimelineKeyHandler(playback: { | ||
| seek: (time: number) => void; | ||
| getCurrentTime: () => number; | ||
| getDuration: () => number; | ||
| getPlaying: () => boolean; | ||
| play: () => void; | ||
| pause: () => void; | ||
| }): KeyHandler { | ||
| const KEYBOARD_SEEK_STEP = 5; |
There was a problem hiding this comment.
Tests Duplicate Production Logic
This test reimplements the keyboard handler instead of invoking production code, and its copy already differs by using tag-name checks where production uses instanceof and by hardcoding the seek-step constant. The tests can therefore remain green if the actual listener wiring or guards regress. Extract and import the production handler, or exercise TimelineView through a DOM-capable component test.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/__tests__/unit/timeline-keyboard.test.ts
Line: 9-17
Comment:
**Tests Duplicate Production Logic**
This test reimplements the keyboard handler instead of invoking production code, and its copy already differs by using tag-name checks where production uses `instanceof` and by hardcoding the seek-step constant. The tests can therefore remain green if the actual listener wiring or guards regress. Extract and import the production handler, or exercise `TimelineView` through a DOM-capable component test.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| @@ -0,0 +1,34 @@ | |||
| import { describe, expect, it } from "vitest"; | |||
There was a problem hiding this comment.
Test Filename Violates Convention
The new useEditorShortcuts.test.ts filename violates the repository directive that TypeScript files use kebab-case names. Rename it to use-editor-shortcuts.test.ts; this repository requirement must be satisfied before merging.
Context Used: CLAUDE.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/routes/editor/useEditorShortcuts.test.ts
Line: 1
Comment:
**Test Filename Violates Convention**
The new `useEditorShortcuts.test.ts` filename violates the repository directive that TypeScript files use kebab-case names. Rename it to `use-editor-shortcuts.test.ts`; this repository requirement must be satisfied before merging.
**Context Used:** CLAUDE.md ([source](https://github.com/capsoftware/cap/blob/main/CLAUDE.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
… and focus guards
…udge, and slider a11y
|
Thank you for the thorough review! All feedback items have been resolved in commit
|
Summary
Resolves #2120 (internal ref: CAP-785).
Adds dedicated keyboard shortcuts for fast timeline navigation to the start and end of the timeline across both Desktop and Web editor surfaces, matching the industry standard workflow (DaVinci Resolve / Premiere / FCPX):
ArrowUporHomeArrowDownorEndImplementation Details
Desktop App (
apps/desktop):ArrowUp,Home,ArrowDown, andEndbindings insidePlayer.tsx'suseEditorShortcutshook.previewTimehover ghost and dispatchescommands.seekTo.normalizeCombofromuseEditorShortcuts.tsto allow unit testing of keyboard combo normalization.apps/desktop/src/routes/editor/useEditorShortcuts.test.ts.Web Player (
apps/web):ArrowUp/Home(seek to 0) andArrowDown/End(seek to duration) inTimelineView.tsx'shandleKeyDown.event.preventDefault()to prevent outer viewport vertical scrolling.target?.isContentEditableguard alongside existingHTMLInputElement/HTMLTextAreaElementchecks to avoid accidental timeline jumps when editing text.apps/web/__tests__/unit/timeline-keyboard.test.ts.Verification & Compliance
biome.json).AGENTS.md.The PR is not yet safe to merge because desktop boundary shortcuts can silently fail during playback handoff and simultaneously trigger existing overlay nudge actions.
Findings
Fix with agent prompt
Summary
Reviews (1) · Last reviewed commit: "feat(editor): support keyboard shortcuts..."