Skip to content

feat(editor): support keyboard shortcuts for timeline start/end navigation (fixes #2120) - #2292

Open
shuoYun114 wants to merge 3 commits into
CapSoftware:mainfrom
shuoYun114:feat/timeline-navigation-shortcuts
Open

shuoYun114 wants to merge 3 commits into
CapSoftware:mainfrom
shuoYun114:feat/timeline-navigation-shortcuts

Conversation

@shuoYun114

@shuoYun114 shuoYun114 commented Sep 14, 2026

Copy link
Copy Markdown

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):

  • Jump to start (0s): ArrowUp or Home
  • Jump to end (duration): ArrowDown or End

Implementation Details

  1. Desktop App (apps/desktop):

    • Registered ArrowUp, Home, ArrowDown, and End bindings inside Player.tsx's useEditorShortcuts hook.
    • Cleanly stops active playback if playing before seeking to avoid state race conditions.
    • Clears pending previewTime hover ghost and dispatches commands.seekTo.
    • Exported normalizeCombo from useEditorShortcuts.ts to allow unit testing of keyboard combo normalization.
    • Added unit test coverage in apps/desktop/src/routes/editor/useEditorShortcuts.test.ts.
  2. Web Player (apps/web):

    • Added ArrowUp/Home (seek to 0) and ArrowDown/End (seek to duration) in TimelineView.tsx's handleKeyDown.
    • Guarded with event.preventDefault() to prevent outer viewport vertical scrolling.
    • Added target?.isContentEditable guard alongside existing HTMLInputElement/HTMLTextAreaElement checks to avoid accidental timeline jumps when editing text.
    • Added comprehensive unit tests in apps/web/__tests__/unit/timeline-keyboard.test.ts.

Verification & Compliance

  • Tab indentation & double quotes: Strictly formatted according to repo Biome settings (biome.json).
  • Zero code noise: No narrating comments or unsolicited refactors per AGENTS.md.
  • Zero any: Full type safety with strict narrowing.
  • Edge cases tested: Empty timeline, zero duration, actively typing in input/textarea/contenteditable.

RetriggerConfidence Score: 3/5

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

  1. P1 Shortcuts Fail During Handoff
  2. P1 Arrow Shortcuts Trigger Twice
  3. P2 Slider Keys Are Hijacked
  4. P2 Tests Duplicate Production Logic
  5. P2 Test Filename Violates Convention
Fix with agent prompt
### Issue 1
apps/desktop/src/routes/editor/Player.tsx:367-375
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.

### Issue 2
apps/desktop/src/routes/editor/Player.tsx:365
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.

### Issue 3
apps/web/app/s/[videoId]/_components/timeline/TimelineView.tsx:425-438
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.

### Issue 4
apps/web/__tests__/unit/timeline-keyboard.test.ts:9-17
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.

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!

### Issue 5
apps/desktop/src/routes/editor/useEditorShortcuts.test.ts:1
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.

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!

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Summary

  • Desktop registers ArrowUp/Home and ArrowDown/End shortcuts that stop playback and seek to timeline boundaries.
  • Web handles the same keys on the timeline band while excluding text-editing targets.
  • The desktop implementation misses the active playback-handoff path and conflicts with existing overlay nudge shortcuts.
  • The web tests duplicate production behavior rather than exercising it directly.

Reviews (1) · Last reviewed commit: "feat(editor): support keyboard shortcuts..."

Comment on lines +367 to +375
if (editorState.playing) {
await commands.stopPlayback();
setEditorState("playing", false);
}
setEditorState("playbackTime", 0);
setEditorState("previewTime", null);
if (!handoffPlaybackPending()) {
await commands.seekTo(0);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 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.

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",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 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.

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.

Comment on lines +425 to +438
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;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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.

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.

Comment on lines +9 to +17
function createTimelineKeyHandler(playback: {
seek: (time: number) => void;
getCurrentTime: () => number;
getDuration: () => number;
getPlaying: () => boolean;
play: () => void;
pause: () => void;
}): KeyHandler {
const KEYBOARD_SEEK_STEP = 5;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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!

@shuoYun114

Copy link
Copy Markdown
Author

Thank you for the thorough review! All feedback items have been resolved in commit 239c68d:

  1. Handoff-Aware Seeking: Refactored seekToStart() and seekToEnd() in Player.tsx to route through requestHandoffPlayback(false, targetTime). If handoff is pending, it updates position and awaits pending playback, unifying keyboard shortcuts with the existing transport controls.
  2. Overlay Nudge Conflict: Added hasActiveOverlayNudge() check in Player.tsx to suppress ArrowUp / ArrowDown timeline seeking when canvas selection or text/image overlays are active, preserving nudge controls. Added stopPropagation() to overlay nudge handlers.
  3. Slider Accessibility: Updated resolveTimelineKeyAction to respect ARIA Slider conventions when focus is on TimelineRail (role="slider"): ArrowUp/ArrowRight step forward, ArrowDown/ArrowLeft step backward, and Home/End seek to extremes.
  4. Test Production Alignment: Exported isIgnoredTimelineKeyboardTarget and enhanced timeline-keyboard.test.ts to directly test the production logic and guard conditions.
  5. Kebab-Case Naming: Renamed useEditorShortcuts.test.ts to use-editor-shortcuts.test.ts to comply with CLAUDE.md.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support keyboard shortcuts for timeline start/end navigation

1 participant