Studio: add a keyboard shortcuts page and make the existing chords rebindable - #8948
Conversation
…bindable Settings gains a Shortcuts tab listing every shortcut, with search, a recorder to change a chord, and controls to unassign or restore a default. The four chords that already existed were hardcoded across four files. They now resolve through one registry, so an edit applies without a reload.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
… registry order loadInitialTab checked a hand-written list that the new tab was missing from, so picking it and reloading fell back to General. The list is now the source of the SettingsTab union, so the two cannot drift. A chord claimed by two actions is consumed by whichever window listener runs first, which followed mount order and so varied by route. Registry order owns it instead, and the tab names the row that loses.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78a57c6bd4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // runner, which does not do bundler-style resolution. | ||
| } from "../lib/keyboard-shortcuts.ts"; | ||
|
|
||
| const STORAGE_KEY = "unsloth_keyboard_shortcuts"; |
There was a problem hiding this comment.
When a user customizes a shortcut and then selects Reset all local preferences in General settings, resetAllPrefs() removes only the keys listed in PREFS_KEYS, which does not include this new storage key. After the forced reload, loadOverrides() restores the customized shortcuts, so the reset-all action leaves this newly introduced local preference unchanged; add the shortcut storage key to that reset list.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and fixed. Without it a reset stranded the user on chords they had just discarded. PREFS_KEYS now includes KEYBOARD_SHORTCUTS_STORAGE_KEY and the handler does removeItem plus a reload, so the in-memory zustand store is discarded too rather than writing the old overrides straight back.
…uts tab in every locale
Before / after evidenceTwo isolated installs, BEFORE at this PR's own merge base
The rail gains exactly one entry, Shortcuts, in the slot between Read the pair honestly: the two halves show different tab bodies - BEFORE sits on General, AFTER on Shortcuts - because BEFORE has no Shortcuts tab to open at all. That asymmetry is the finding, not a missed click. The rail is the half to compare, not the body. A CI gate this PR currently fails
A genuine bug fixed alongside it
Frontend suite Item-by-item triage of the three Codex comments is in the threads above, and a fuller simulation report follows separately. |
Review summaryVerdict: useful, merge after a rebase. Three review items triaged, 231 assertions x 3 browser engines all green, one CI failure genuinely PR-caused and fixed ( Item verdicts
The first two look live only because GitHub reports an item as not-outdated when its anchor line moves rather than vanishes: the review posted at Before / after, measured on both treesThree chords lived in three separate hand-rolled 7 improvements. 4 regressions, and this is the one thing I want you to decide. Switching from 2 new behaviours. Simulation, real engines231 assertions x 3 engines, 0 failures. Edge is Chromium, so three engines, not four. The harness drives the real compiled
Real OS-level keystrokes via Old installs (all PASS x3): absent key, empty / corrupt / Does it break anythingThe diff is 100% Two findings the bots missed, both below the bar so reported not fixed
(Also: CI
The contract test greps Blocker: the PR is Three things before merge: the rebase (mandatory), the test fix (already pushed), and your decision on the non-QWERTY regression. |
# Conflicts: # studio/frontend/src/features/settings/settings-dialog.tsx
|
Rebase blocker cleared in Gates after the merge: typecheck 0, That one failure is main's, not this branch's, and it is worth knowing about because |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dce9cd4e39
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| mod: mac ? event.metaKey : event.ctrlKey, | ||
| ctrl: mac ? event.ctrlKey : false, |
There was a problem hiding this comment.
On Windows or Linux, recording a chord that includes the Meta/Super key silently omits that modifier: for example, Meta+Alt+K is persisted and displayed as Alt+K. The recorded key event itself then cannot match because matchesBinding explicitly rejects Meta on non-macOS, while pressing Alt+K alone unexpectedly triggers the action. Reject non-macOS events with metaKey set, or represent Meta explicitly rather than saving a different chord.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and fixed in 91793c4. Confirmed in the source: off macOS bindingFromEvent writes mod from ctrlKey and ctrl false, so event.metaKey is dropped on the floor, and matchesBinding separately rejects any non-mac event carrying Meta. Super+Alt+K therefore persisted and displayed as Alt+K, which is worse than a no-op in both directions at once: the action is assigned to a chord the user never chose and fires on Alt+K pressed alone, while the chord they actually pressed can never match.
Recording nothing is the same answer the recorder already gives while only modifiers are held, so it just keeps listening rather than saving something wrong. Representing Meta explicitly was the other option you offered, but that would need matchesBinding to accept it too, and a Super chord is not reliably deliverable to a web view on either Windows or most Linux desktops, so the shortcut would look bound and never fire.
New test covers Super+Alt+K, bare Super+K and Ctrl+Super+K off macOS, and pins that the same chord on macOS still records as Mod+Alt+KeyK. Mutation-checked: removing the guard fails it. Full suite 3777 passed with one failure, which is tests/queued-model-capabilities.test.ts, red on main today for an unrelated extensionless import that #9192 fixes.
for more information, see https://pre-commit.ci
There is nowhere to put Meta on Windows or Linux: matchesBinding rejects an event carrying it, and the recorder wrote mod from ctrlKey alone, so Super+Alt+K persisted and displayed as Alt+K. That assigns the action to a chord the user did not choose, fires it on Alt+K pressed by itself, and leaves the chord they actually pressed unable to match anything. Returning null is the answer the recorder already gives while only modifiers are held, so it simply keeps listening.
|
@codex review |
|
@codex review |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a5d18b594
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| event.preventDefault(); | ||
| event.stopPropagation(); | ||
| if ( | ||
| event.code === "Escape" && |
There was a problem hiding this comment.
When a user tries to assign bare Escape, this branch always treats it as cancellation and returns before bindingFromEvent can save it. That conflicts with isAcceptableBinding, which explicitly accepts Escape as a standalone binding, leaving no UI path to create an otherwise supported shortcut; use a different cancellation gesture or otherwise provide a way to distinguish recording Escape from cancelling.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
No shortcut ships with bare Escape and every chord that includes a modifier (Mod+Escape included) still records, so nothing a user has is unreachable. Escape is the recorder's only way out while it is swallowing every keydown in the capture phase, and spending it on a binding would leave a row stuck in recording mode. isAcceptableBinding stays permissive so a stored or imported Escape binding keeps working.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a5d18b594
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else if (otherModHeld) { | ||
| return false; | ||
| } | ||
| return event.shiftKey === binding.shift && event.altKey === binding.alt; |
There was a problem hiding this comment.
On Windows/Linux layouts that use AltGr, browsers expose character entry as Ctrl+Alt (the existing handling in features/chat/utils/composer-send-guard.ts lines 47-52 confirms both representations occur), so this modifier comparison treats ordinary AltGr typing as a custom Mod+Alt+<key> shortcut. Because actions such as newChat, toggleSidebar, and openSettings remain active in text fields, assigning one of those chords can make typing an AltGr character navigate away, toggle UI, or open Settings; check getModifierState("AltGraph") (with the existing fallback heuristic) before matching or recording these chords.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
No shipped default is a Mod+Alt chord, so nothing here collides with AltGr typing out of the box; reaching this needs the user to first assign a Ctrl+Alt chord that their own layout also uses to type a character. Leaving the modifier comparison exact rather than special-casing a chord nobody is bound to.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 115772f874
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| useShortcut("searchChats", () => useChatSearchStore.getState().open(), { | ||
| skipInTextFields: true, | ||
| }); |
There was a problem hiding this comment.
When the user rebinds or clears searchChats, this handler immediately follows the override, but the search button tooltip in components/app-sidebar.tsx still unconditionally displays ⌘K or Ctrl+K at line 2907. The primary visible shortcut hint therefore instructs users to press a chord that may no longer work; render that hint from the resolved searchChats binding and hide it when unassigned.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in cae0cdd. The search tooltip now renders useShortcutLabel("searchChats") and drops the key cap entirely when the action is unassigned.
| useShortcut( | ||
| "openSettings", | ||
| () => useSettingsDialogStore.getState().openDialog(), | ||
| { enabled: !isAuthFlowRoute }, | ||
| ); |
There was a problem hiding this comment.
When the user rebinds or clears openSettings, this hook uses the new value, while the Settings item in components/app-sidebar.tsx still displays the hard-coded ⌘, at line 3797. The menu consequently advertises a stale shortcut after any customization; derive the displayed value from the resolved openSettings binding and omit it when the action is unassigned.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in cae0cdd. The Settings menu row now renders useShortcutLabel("openSettings") and omits the shortcut when it is cleared, which also stops it showing the Mac comma chord on Windows.
The search tooltip and the Settings menu row hard-coded the shipped chords, so after a rebind or a clear in the shortcuts tab they kept telling the user to press a chord that no longer runs anything. Both now read the resolved binding through useShortcutLabel and drop the hint when the action is unassigned.
|
@codex review |
for more information, see https://pre-commit.ci
2 participants
The problem
Studio has four keyboard shortcuts and no way to see or change any of them. They are hardcoded in four separate files, so the only way to learn one is to read the source:
Mod+Shift+Oapp/routes/__root.tsxMod+,app/routes/__root.tsxMod+Kfeatures/chat/components/chat-search-dialog.tsxMod+Bcomponents/ui/sidebar.tsxEach one re-implements the same
metaKey || ctrlKeytest against its ownwindowlistener.The change
Settings gains a Shortcuts tab: every action in one searchable list, a pencil to record a new chord, a trash to unassign, an undo arrow to restore a shipped default, and a reset-all.
The four chords above now resolve through a single registry, so what the page shows is what actually fires, and an edit applies without a reload. A fifth action,
Mod+/, opens the page itself.Four new modules under
features/settings:lib/keyboard-shortcuts.ts, the action registry plus binding parse, match and formatstores/keyboard-shortcuts-store.ts, persistence and conflict detectionhooks/use-shortcut.ts, the hook call sites usetabs/keyboard-shortcuts-tab.tsx, the pageDecisions worth flagging
Bindings store
KeyboardEvent.code, notkey.codeis the physical key, so a chord recorded on one layout still fires on another, and it does not change under Shift or Option.Modserializes once and resolves to Cmd on macOS and Ctrl elsewhere, which is what all four handlers already did by hand.Only deltas are persisted. localStorage holds overrides, never the full table, so a default we change in a later release still reaches everyone who did not touch that row. A stored
nullis a deliberate "unassigned", distinct from an absent key meaning "use the default". Ids from an older build are dropped on load, so a removed action cannot resurrect.Off-platform modifiers do not count. On macOS a bare Ctrl does not satisfy a
Modbinding, and on Windows and Linux the Meta key does not either. Ctrl stays separately bindable on macOS.A bare letter is refused by the recorder, since it would swallow ordinary typing. Function keys and Escape stand alone.
Conflicts are surfaced, not blocked. Two actions on one chord both fire, so the page flags both rows rather than silently rejecting the edit.
The recorder listens in the capture phase, so the chord being recorded does not also trigger the shortcut it is replacing, and Escape cancels recording instead of closing the dialog.
Behaviour preserved
Mod+Kstill stands down while a text field has focus, which the hand-rolled handler did explicitly and the composer relies on. That is now theskipInTextFieldsoption rather than four lines repeated per call site.components/ui/sidebar.tsximports the hook by path rather than through thefeatures/settingsbarrel: the barrel pulls inSettingsDialog, which would close an import cycle.Testing
New
tests/keyboard-shortcuts.test.ts, 14 cases: serialize and parse round-trip, junk and modifier-only values rejected, every shipped default parses, exact modifier matching, the macOS and non-macOSModsplit in both directions, the recorder ignoring a lone modifier, bare-letter refusal, per-platform label rendering, override and clear resolution, and conflict detection including cleared rows.Two of these caught a real problem while I was writing them:
matchesBindingandbindingFromEventread the platform from a global, which made them untestable and would have hidden a mac/non-mac modifier bug. Both now take the platform as an argument and default to the detected one.Full frontend suite passes at 2716, typecheck is clean,
i18n:checkpasses, and the production build succeeds.Not covered
Only English strings are added. The locale overlays are partial by design and parity passes without them.
The list is the five actions above. Anything else worth binding is a registry entry plus one
useShortcutcall.