Repository navigation
feat(studio): every editor and player shortcut is remappable - #6966
Conversation
![heygengenesis[bot]](https://avatars.githubusercontent.com/in/2913576?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: REQUEST CHANGES · 2 blockers · 2 important · 0 nits · head a634fdba
This now folds in the closed #6965. It fixes the three things I raised there: handlers now go through matchesKey, Shift with a symbol is refused, and the body states that ⌘ prints on every platform.
- blocker,
Commentsratchet fails at this head:captions/keyboard.tsgoes from 3/21 to 4/22 comment lines because of the new doc comment onresolveCaptionNudge(↓ inline).CIwas still running when I reviewed. - important (playback ignore list still uses fixed keys):
playbackShortcuts.ts:80still uses the hard-codedPLAYBACK_FRAME_STEP_CODES(ArrowLeft/ArrowRight) to stop frame-stepping while caption segments are selected. Suppose a host putsframeBackandnudgeLeftboth on,, whichfindClashallows because that pair is shared by design. In caption edit mode with segments selected,,then frame-steps and nudges the caption. That double handling is exactly what the guard exists to prevent. Make the guard testmatchesKey(e, "frameBack"|"frameForward"|…Ten)instead.
The other blocker and important are inline.
Scope
Read end to end: studioCombo.ts, studioShortcuts.ts, appHotkeysDispatch.ts, useAppHotkeys.ts, usePlaybackKeyboard.ts, playbackShortcuts.ts, linkShortcuts.ts, domEditNudge.ts, captions/keyboard.ts, useTimelineKeyboardActor.ts. Not read: the remaining one- to three-line hint call sites and the test files. Not run locally: there are no node_modules on this box, so the dead-path finding comes from reading the code.
| togglePlay(); | ||
| return; | ||
| } | ||
| if (e.defaultPrevented || ignoresPlaybackKey(e)) return; |
There was a problem hiding this comment.
blocker: a playback action remapped to any combo with Cmd, Ctrl or Alt can never fire. ignoresPlaybackKey calls shouldIgnorePlaybackShortcutEvent, and that returns true straight away for any metaKey || ctrlKey || altKey (playbackShortcuts.ts:76). So setStudioKeys({ playPause: "mod+p" }) or { mute: "alt+m" } is accepted, the panel shows ⌘P, and the key does nothing; the old key stops working too. That breaks the PR's own claim that every shortcut is remappable, modifier combos included. The same goes for all 16 PLAYBACK_ACTIONS. Either drop the modifier early-out and rely on matchesKey, which is already exact on modifiers, or refuse modifier combos for playback actions in resolve. Add an override test with a modifier through the real handler; the current override test uses bare p, which slips past this check.
| if (isFrameStep && canvasNudgeKeysClaimed() && resolveCanvasNudgeDelta(e)) return; | ||
| if (e.repeat && action !== "playPause" && !isFrameStep) return; | ||
| const shuttleOrStep = (direction: -1 | 1) => | ||
| pressedKeysRef.current.has(keyOf("pause")) |
There was a problem hiding this comment.
important: this compares keyOf("pause") (the combo's key) with pressedKeysRef, which stores e.key.toLowerCase(). With pause on a named key the two never agree. For example, with { pause: "Space", playPause: "p" }, keyOf gives "Space" while the ref holds " ". So holding the pause key plus J or L shuttles instead of stepping one frame. Track held actions with matchesKey on keydown and keyup instead of raw e.key.
| type CaptionNudgeKeyEvent = Parameters<typeof resolveCanvasNudgeDelta>[0]; | ||
|
|
||
| type CaptionNudgeKeyEvent = Pick<KeyboardEvent, "altKey" | "ctrlKey" | "metaKey" | "key">; | ||
| /** The nudge delta for this key, or null when it is not a nudge or the target owns its keys. */ |
There was a problem hiding this comment.
blocker (Comments ratchet): this added line raises the file's comment share from 3/21 to 4/22, which fails the Comments check at this head. Cut it; the name and the | null return already say this.
Edit accuracy: accurate 271 (base branch 271), smooth 120 of thoseThe gate passes. Quarantined, measured but not gated (0) Unstable (1)
|
| hintKey(keys[action]); | ||
| formatCombo(parseCombo(keys[action])!); | ||
|
|
||
| /** True when the event is the combo the host (or the default) set for this action. */ |
There was a problem hiding this comment.
blocker (Comments ratchet, new at this head): with the captions/keyboard.ts comment gone, the ratchet now fails on this file: 3/266 against 2/207 at the base. This doc comment is the added line. Cut it.
…d held stop key follow the map
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Post-merge advisory review (this PR is already merged into main). Findings are follow-ups, not gating. Reviewed at f6f2ee1f04acc1a5d6dce68ae17190508778f0db.
Genesis's prior modifier-combo early-out, fixed caption-arrow guard, and named held-pause findings are fixed at this head. The resolver now drives those real playback paths, and #6965's 252d9ef4 is an ancestor of this head.
Important follow-ups:
packages/studio/src/components/editor/InspectorHeaderActions.tsx:58andPropertyPanelFlatHeader.tsx:59:useStudioKeys()runs only when the Ungroup button exists. Rendering either header first without that button and then with it throwsRendered more hooks than during the previous render. The classic inspector header is retained while selecting an ordinary element and then a group (PropertyPanel.tsx:304; itsStudioRightPanels.tsx:202caller is unkeyed). The flat panel is selection-keyed, which mitigates ordinary selection swaps there, but its header has the same conditional-hook defect. Callconst keys = useStudioKeys()unconditionally at each component's top level and usekeyHint("ungroup", keys)in JSX. Add same-root ordinary/group transitions, rather than separate initial renders.packages/studio/src/hooks/appHotkeysDispatch.ts:244: a remappedresetKeyframesis still reachable only inside thedeleteSelected/deleteSelectedAltbranch.setStudioKeys({resetKeyframes:"q"})is accepted, yet Q never callsonResetKeyframes; Backspace can instead proceed to clip deletion once it no longer matches Reset. Resolve Reset independently while preserving selected-keyframe and automation ownership for shared defaults. Test the independent reset override and ensure its old key cannot reset.
Evidence: exact-head focused Vitest invocation of studioCombo, studioShortcuts, useAppHotkeys, usePlaybackKeyboard, useTimelinePlayer, caption keyboard, domEditNudge, linkShortcuts, and PropertyPanelFlatHeader: 157 passed. Three independent regression controls fail at this head: two header transitions throw, and the reset override calls its callback zero times. Moving the two hooks to component top level and adding an independent reset branch makes the same three controls pass; temporary source changes were restored. bun run build passes. No live host, real Windows AltGr/non-Latin input, or complete interactive editor session was exercised.
Audited: resolver/parser, playback and app dispatch, caption/canvas guards, focused-widget pickup/navigation boundary, header hint lifecycle, and integration callers. Trusting: visual screenshots and reported 8268-test suite; not rerun here.
Need: yes, host-controlled shortcuts · Size: ok, one shared key contract · Reuse: existing resolver and ownership guards · Simpler: keep event-time resolution; call hooks unconditionally and separate Reset dispatch · Structure: fine apart from the two concrete correctness gaps above
Risk: medium — keyboard mapping crosses editor, playback, focused widgets and React lifecycle.
Verdict: COMMENT (post-merge: With fixes)
Reasoning: The prior review blockers are resolved, but two independently reproduced regressions need follow-up fixes before widening host adoption.
— hf-review
What
Every shortcut Studio's editor and player answer is now an action in the key resolver, so a host can remap each one with
setStudioKeys, including the ones with modifiers, Space and the arrow keys. The key a handler answers and the key the shortcut panel, tooltips and clip menu show change together. With no override nothing changes.Shortcuts can be key combinations (
"mod+shift+z","Space","ArrowLeft");modmeans Cmd or Ctrl. Ctrl+Alt is refused because Windows sends AltGr as Ctrl+Alt, and Shift with a symbol key is refused because it depends on the layout.Why
Hosts that embed Studio want users to change any shortcut, not just the single letters. The playback and editing keys were literals in about twenty files.
How
cancelaction for the Escape that cancels a gesture.usePlaybackKeyboard.modstays Cmd or Ctrl either way, as today.Not remappable, on purpose
These are the keyboard contract of a focused widget rather than editor shortcuts; changing them would break keyboard and screen-reader use:
Test plan
usePlaybackKeyboard.test.tsanduseAppHotkeys.test.tscover every default through the real handlers, an override answering with the old key silent, and hints following the override.What I measured
On a Linux box at the pushed head:
oxfmt --check,oxlint,fallow audit --base origin/mainandtsc --noEmitclean; the full studio suite passes (722 files, 8268 tests). New tests:studioCombo.test.ts(parse, match withmodas Cmd or Ctrl, symbols, refused combos), every default and an override through the real handlers inusePlaybackKeyboard.test.tsanduseAppHotkeys.test.ts. Mutating a handler back to a literal fails the override tests.What I did NOT exercise
Host adoption (no host feeds the resolver here). Non-Latin and dead-key layouts. Real Windows AltGr input.
Before and after
A Studio dev server on a Linux box, headless Chrome, the repo's
timeline-trim-snapfixture as the openproject. Before is
main. After is this branch with one host call made from the page:The same script drove both sides:
SpaceP⌘Z⌘UThe panel prints
⌘formodon every platform, as it did before this PR;modstill answers Cmd or Ctrl,so Ctrl+U is what undoes on Linux.
Before (main)
Shortcuts panel: Space plays, ⌘Z undoes.
Space plays: the clock reads 00:01 and the transport shows Pause.
After (this branch, play/pause on P and undo on mod+u)
The panel follows the override.
Space no longer plays: the clock stays at 00:00 and the transport still shows Play.
P plays.