feat(web): add New Loop keyboard shortcut#50
Conversation
Mingholy
left a comment
There was a problem hiding this comment.
Code Review: PR #50
Summary
Adds a global Cmd/Ctrl+Shift+L keyboard shortcut to open the New Loop dialog, with proper guards for editable targets (input, textarea, select, contentEditable). Updates the button tooltip to surface the shortcut. Includes e2e test coverage for both the happy path and the editable-target bypass.
Implementation Quality
Architecture is clean. A single useEffect in Shell registers a window-level keydown listener, gated behind shareMode and loggedIn. The shortcut choice (Cmd/Ctrl+Shift+L) is reasonable -- unlikely to collide with browser or OS defaults.
Good cross-platform handling. Checking both event.key.toLowerCase() === "l" and event.code === "KeyL" ensures the shortcut works on non-Latin keyboard layouts where key might differ but the physical key position is the same. The metaKey || ctrlKey check covers macOS vs Windows/Linux.
Editable target guard is solid. Covers input, textarea, select, and contentEditable elements. The instanceof HTMLElement check at the top prevents issues with non-element targets (e.g., events dispatched directly on window).
Cleanup is correct. The useEffect return function removes the listener.
Dependency array is appropriate. ws.setNewLoopDialogOpen is wrapped in useCallback(_, []) in the state module (state.ts:175-182), so it is referentially stable -- no spurious re-registrations.
Code Quality
Test design. The e2e test has two well-structured scenarios:
- Dispatches from a focused
<input>withctrlKey-- correctly expects the dialog NOT to open. - Dispatches on
windowwithmetaKeyandkey: "x"/code: "KeyL"-- tests theevent.codefallback path and confirms the dialog opens.
This covers the two most important edges (editable bypass + non-Latin layout).
Tooltip update ("create new loop (Cmd/Ctrl+Shift+L)") -- practical and discoverable.
One minor observation (not blocking): isEditableTarget is defined inline inside the effect callback. For a one-off helper this is perfectly fine; just noting that if more shortcuts are added later, extracting it to a shared utility would avoid duplication.
Security
No security concerns. The shortcut opens the same dialog that is already accessible via the button click. The shareMode and !loggedIn guards ensure unauthenticated users cannot trigger it.
Verdict
LGTM. This is a well-scoped, cleanly implemented feature with good test coverage and no issues. The cross-platform keyboard handling and editable-target guards are done right.
Summary
Test plan
cd web && bunx tsc -bbun run buildgit diff --checkweb, andserverpackage manifests do not define alintscript.The shortcut affects keyboard behavior and the button tooltip only, with no standalone visual change to screenshot. After state was verified via the browser accessibility snapshot: the
+ New Looptitle/description iscreate new loop (Cmd/Ctrl+Shift+L).