feat: make keyboard shortcuts configurable - #31
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 752e1746e9
ℹ️ 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".
| export function parseKeyEvent(event: KeyboardEvent): string | null { | ||
| if (MODIFIER_CODES.has(event.code)) return null; | ||
| const parts: string[] = []; | ||
| if (event.ctrlKey || event.metaKey) parts.push("$mod"); |
There was a problem hiding this comment.
On Windows, metaKey represents the Windows key, while $mod resolves to Ctrl, so recording Win+G saves and displays Ctrl+G and the combination the user actually pressed will not trigger the shortcut. Record Meta separately from Ctrl on non-macOS platforms rather than collapsing both flags into $mod.
AGENTS.md reference: AGENTS.md:L9-L10
Useful? React with 👍 / 👎.
| resetKeybinding: (id) => { | ||
| set((s) => { | ||
| const { [id]: _removed, ...rest } = s.keybindingOverrides; | ||
| return { keybindingOverrides: rest }; | ||
| }); |
There was a problem hiding this comment.
If a user assigns Import chat's default combination to New chat, Import chat is unbound; clicking Import chat's reset button then removes that empty override and restores its default without checking that New chat still owns it. Both actions consequently advertise the same key, while bindings() retains only one handler for that map entry, so one shortcut silently stops working. Resetting an individual binding needs the same collision handling as assigning it.
Useful? React with 👍 / 👎.
|
|
||
| cycleThinking: withProject(() => void cycleThinkingLevel()), | ||
| }), | ||
| }, keybindingOverrides), |
There was a problem hiding this comment.
When Stop turn is rebound to a non-Escape combination and focus is in an HTMLInputElement, the existing guard at line 443 still suppresses the action because it was written specifically to let Escape clear or close fields. For example, rebinding Stop turn to Ctrl+X and pressing it in sidebar search edits the field instead of aborting the run; gate that exception on the pressed/effective Escape binding.
Useful? React with 👍 / 👎.
| const openTerminal = useAppStore((s) => s.openTerminal); | ||
| const editorId = useAppStore((s) => s.preferences.preferredEditorId); | ||
| const searchFocusRequest = useAppStore((s) => s.searchFocusRequest); | ||
| const keybindingOverrides = useAppStore((s) => s.keybindingOverrides); |
There was a problem hiding this comment.
After New chat is rebound or displaced by a collision, the nested ChatList still renders hintFor("newChat") without overrides at line 410, so an empty project instructs the user to press the default key even though the global handler listens elsewhere. Have ChatList subscribe to the binding override when rendering this copy.
AGENTS.md reference: AGENTS.md:L125-L125
Useful? React with 👍 / 👎.
| <button | ||
| type="button" | ||
| onClick={() => setRecordingId(recording ? null : shortcut.id)} | ||
| aria-label={`Change shortcut for ${shortcut.label}`} |
There was a problem hiding this comment.
For screen-reader users, this aria-label replaces the accessible name derived from the <Kbd> children, so every recorder is announced only as “Change shortcut for …” and its assigned, unassigned, or recording state is unavailable. Include the current binding and recording state in the accessible name or description so the editable shortcut list remains usable without sight.
AGENTS.md reference: AGENTS.md:L138-L138
Useful? React with 👍 / 👎.
1 participant
Summary
keybindingOverrides), separate from Pi's settings.Test plan
bun test src/renderer/lib/shortcuts.test.ts src/renderer/lib/shortcutBindings.test.ts— registry, override, conflict, and keystroke-parsing behaviorbun test src/shared/rpc-schema.test.ts— newkeybindingOverridesfield defaults and corrupt-value handlingbun run typecheck— no new errors (one pre-existing, unrelatedelectron.vite.config.tserror from a vite/rolldown version mismatch)