Prototype shortcuts settings layouts - #204
Conversation
Co-Authored-By: Oz <oz-agent@warp.dev>
|
@Mamdouh66 is attempting to deploy a commit to the bholmesdev's projects Team on Vercel. A member of the Team first needs to authorize it. |
Co-Authored-By: Oz <oz-agent@warp.dev>
|
Addressed the critical review findings in
Verification:
|
|
@bholmesdev wdyt? |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
Dev-only throwaway prototype for #194: three switchable Shortcuts settings layouts (sidebar / grouped cards / table) over a 33-command mock registry, with in-memory search, recording, conflict feedback, disable/reset. Production Settings path stays behind import.meta.env.DEV; SettingsDialog gains an optional width override.
Scope matches the PR intent — no persistence, menu wiring, or hotkey runtime. Fine as a layout-direction spike.
Concerns
- ArrowLeft/Right variant cycling is a window-level listener while Settings is open; easy to trigger accidentally and it rewrites the URL.
- Grouped-cards search empty state is inconsistent with the other two layouts.
useCallbackfights the desktop React Compiler guidance (apps/desktop/AGENTS.md).- Optional: confirm the unconditional
ShortcutsSettingsPrototypeimport is dropped from prod bundles (Vite usually DCE's the DEV branch; worth a quick check).
None of these block a prototype merge.
Verdict
Found: 0 critical, 0 important, 3 suggestions
Approve with nits
Reviewed by a Warp Factory agent.
…tcuts-tab # Conflicts: # apps/desktop/src/App.tsx
There was a problem hiding this comment.
Overview
Dev-only throwaway prototype for #194: three Shortcuts settings layouts (sidebar / grouped cards / table), mock 33-command registry, in-memory record/validate/reset, wired through Settings via import.meta.env.DEV. Prod settings path stays the stacked sections; SettingsDialog just gains an optional className.
Concerns
- No blocking correctness/security issues for a DEV-gated prototype.
resetAllrestores pure defaults and drops the seeded demo rows (app.new-filecustomized,app.chat-about-notedisabled), so “Reset all” no longer matches first-load demo state.- PR text mentions Left/Right arrow keys to cycle variants; only the floating ←/→ buttons are wired (fine if copy meant those controls).
- Prototype root is a fragment under Settings’
divide-ywrapper, so the fixed switcher is a second child and can leave a stray divider.
Verdict
Found: 0 critical, 0 important, 3 suggestions
Approve with nits
Reviewed by a Warp Factory agent.
| const resetAll = () => { | ||
| setBindings(initialBindings()); | ||
| setErrors({}); | ||
| setRecordingId(null); | ||
| }; |
There was a problem hiding this comment.
💡 [SUGGESTION] resetAll uses pure initialBindings(), but mount seeds app.new-file / app.chat-about-note for demo. Extract one demoBindings() used by both so Reset all matches first paint.
| const currentIndex = variants.findIndex(({ id }) => id === current); | ||
| const cycle = (direction: -1 | 1) => { | ||
| const nextIndex = | ||
| (currentIndex + direction + variants.length) % variants.length; | ||
| onChange(variants[nextIndex].id); | ||
| }; |
There was a problem hiding this comment.
💡 [SUGGESTION] PR claims Left/Right arrow keys cycle variants; only buttons call cycle. If keyboard cycling is desired, add a window keydown handler here when !recording (ignore when focus is in an input).
| <> | ||
| {variant === "sidebar" ? ( | ||
| <SidebarVariant {...props} /> | ||
| ) : variant === "tabs" ? ( | ||
| <TabsVariant {...props} /> | ||
| ) : ( | ||
| <TableVariant {...props} /> | ||
| )} | ||
| <PrototypeSwitcher | ||
| current={variant} | ||
| onChange={chooseVariant} | ||
| recording={state.recordingId !== null} | ||
| /> | ||
| </> |
There was a problem hiding this comment.
💡 [SUGGESTION] Fragment makes the fixed switcher a second child of Settings’ divide-y container (possible stray divider). Render the active variant as the only dialog child and portal/mount the switcher outside that wrapper.
|
@bholmesdev do u think we should continue here or not? |
1 participant
Description
Related to #194.
This is a deliberately throwaway, dev-only UI prototype for choosing the direction of the Shortcuts settings experience. It adds three switchable layouts inside the existing Settings dialog:
Each variant projects the same 33-command mock registry and supports in-memory search, shortcut recording, duplicate/reserved-key feedback, disable, reset, and reset-all states. Use the floating switcher, Left/Right arrow keys, or open
?variant=sidebar,?variant=tabs, or?variant=tablein the renderer URL.This PR intentionally does not choose TanStack Hotkeys, persist bindings, update Electron menus, or wire shortcuts to runtime commands. Those decisions belong in the follow-up implementation after a layout is selected and the command-registry work is available.
Type of Change
Testing
Manual Testing Details:
pnpm checkpnpm build:desktoppnpm buildChecklist
Prototype limitations