Repository navigation
feat(studio): the key resolver accepts key combinations - #6965
miguel-heygen wants to merge 1 commit into
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 · 3 blockers · 3 important · 0 nits · head 252d9ef4
Three checks fail at this head because of the diff, and the body doesn't explain them:
- blocker, preflight
oxfmt --check:studioCombo.ts,studioCombo.test.tsandstudioShortcuts.tsaren't formatted. The body saysoxfmt --check .was clean. - blocker, Comments ratchet:
studioShortcuts.tscomment share goes from 2/207 to 3/206 because of the new doc comment onmatchesKey(↓ inline). - blocker, PR captures: the body has no
## Before/## After. It also can't use## No visible change, because the diff is over 20 lines.
The fail-without-fix line doesn't apply here: this is a feature PR, not a fix PR. Important findings are inline.
Scope
Read all 3 files end to end, plus every caller of getStudioKey/keyHint/setStudioKeys on this head. No prior reviews. #6966 is stacked on this branch and was not reviewed here.
| `Studio keys: using defaults, "${action}" is not a known action with one character`, | ||
| ); | ||
| for (const [action, text] of Object.entries(overrides)) { | ||
| const combo = typeof text === "string" ? parseCombo(text) : null; |
There was a problem hiding this comment.
important: after this change setStudioKeys (public via src/index.ts) accepts combos, but every handler still compares the raw string. Examples: SnapToolbar.tsx:56 (e.key === getStudioKey("snap")), TimelineToolbar.tsx:216, useKeyframeKeyboard.ts:16, playbackShortcuts.ts:92, usePlaybackKeyboard.ts. So if a host sets { fullscreen: "mod+shift+f" } or { mute: "Space" }, the hint shows ⌘⇧F and nothing warns, yet that shortcut stops working entirely; the old f is gone too. Before this change, that map was refused and the defaults stayed. If this merges to main ahead of #6966, main and any release cut in between ship that state. Either land the two PRs together, or keep the single-character guard here and lift it in #6966.
| 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): this comment line is the one that raises the file's comment share and fails the Comments check. The name matchesKey already says what this line says, so cut it.
| } | ||
|
|
||
| export function matchCombo(e: KeyboardEvent, c: Combo): boolean { | ||
| if (e.shiftKey !== c.shift || e.altKey !== c.alt) return false; |
There was a problem hiding this comment.
important: characters that need Shift to type can never match. On a US layout + and ? arrive with shiftKey: true, but parseCombo("cmd++") / parseCombo("?") give shift: false, so this check returns false. The reverse also fails: shift+1 arrives as e.key === "!", and charMatches only falls back to e.code when Alt is held. The test pins that cmd++ parses, but nothing tests that it matches. Either ignore Shift for non-letter characters (compare e.key), or refuse those inputs in parseCombo, and add a match test for +.
| } | ||
|
|
||
| export function formatCombo(c: Combo): string { | ||
| const mods = `${c.ctrl ? "⌃" : ""}${c.alt ? "⌥" : ""}${c.mod || c.meta ? "⌘" : ""}${c.shift ? "⇧" : ""}`; |
There was a problem hiding this comment.
important: the body says hints render ⌘⇧Z on Apple platforms and Ctrl+Shift+Z elsewhere, but formatCombo has no platform branch. A Windows or Linux user who gets mod+k sees ⌘K. The test runs in happy-dom, which is not a Mac platform, and still expects ⌘⇧Z, so it pins the Mac-only output. utils/studioHelpers.ts:124 already has an isMac check you could reuse. Otherwise, fix the claim in the body.
Edit accuracy: accurate 271 (base branch 271), smooth 97 of thoseThe gate passes. Quarantined, measured but not gated (0) |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| /** "mod+shift+z", "Space", "b": mod is Cmd or Ctrl; Ctrl+Alt is refused because Windows sends AltGr as Ctrl+Alt. */ | ||
| export function parseCombo(text: string): Combo | null { | ||
| const match = /^((?:[A-Za-z]+\+)*)(.+)$/.exec(text); |
|
Folded into #6966: combos are only safe to accept once the handlers route them, so they ship together. |
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Closed, superseded PR review. Reviewed at 252d9ef4da7bc43e23d6a75cbd42fa3da845b518; this PR is closed and its exact head is an ancestor of merged #6966 at f6f2ee1f04acc1a5d6dce68ae17190508778f0db.
Genesis already covered the unconverted handlers, shifted symbols and platform-hint claim at this head. Those concerns were addressed in the combined #6966 implementation: handlers use matchesKey, symbols match the Shift required to type them while explicit Shift+symbol is refused, and its body discloses the platform-neutral ⌘ display. I am not reopening the obsolete standalone increment or issuing another approval on it. Follow-up correctness findings on the combined implementation are recorded on #6966.
Validation: git merge-base --is-ancestor 252d9ef4… f6f2ee1f… succeeds; the exact #6965 resolver/combo tests pass (15 passed) under an isolated Happy DOM Vitest config. The standard Studio config initially cannot load the unbuilt workspace exports on this worktree, so this is resolver coverage, not a full Studio test claim.
Need: yes, host-controlled combos · Size: ok as part of #6966 · Reuse: existing key registry · Simpler: combined landing avoids a temporarily unsupported combo map · Structure: fine
Risk: medium — standalone parser acceptance must agree with all event handlers.
Verdict: COMMENT
Reasoning: Closed and superseded; the folded implementation was reviewed on #6966, with concrete follow-ups there.
— hf-review
What
The Studio key resolver accepts key combinations, not only single characters:
"mod+shift+z","Space","ArrowLeft","alt+shift+d".modmeans Cmd or Ctrl, as the editor shortcuts already do. Single characters keep working exactly as before, and with no override nothing changes.Why
Hosts that embed Studio want every shortcut remappable, including the ones that need a modifier. This is the resolver half; the next PR in the stack moves the editor and playback keys onto it.
How
studioCombo.tsparses, matches, formats and clash-keys a combo. Shortcut hints show it as⌘⇧Zon Apple platforms andCtrl+Shift+Zelsewhere.setStudioKeysvalidates combos the same way it validated characters: an unknown action, an unparsable value, or two actions on one key refuses the whole map and warns.Test plan
studioCombo.test.ts: parse, match (modas Cmd or Ctrl), format, Ctrl+Alt refused, clash keys.studioShortcutstests unchanged.What I measured
On a Linux box:
studioCombo.test.tsand the existingstudioShortcutstests pass;oxfmt --check .,oxlint,fallow audit --base origin/mainandtsc --noEmitare clean.What I did NOT exercise
No handler uses combos yet (next PR). Non-Latin and dead-key layouts. No visual change: this PR renders nothing new.