Skip to content

feat(web): add New Loop keyboard shortcut#50

Open
Mingholy wants to merge 2 commits into
mainfrom
feat/ft-4-new-loop-shortcut
Open

feat(web): add New Loop keyboard shortcut#50
Mingholy wants to merge 2 commits into
mainfrom
feat/ft-4-new-loop-shortcut

Conversation

@Mingholy

@Mingholy Mingholy commented Jun 9, 2026

Copy link
Copy Markdown
Owner

Summary

  • Add a global Cmd/Ctrl+Shift+L shortcut to open the New Loop dialog.
  • Ignore the shortcut while focus is inside editable targets.
  • Surface the shortcut in the New Loop button title/tooltip and cover it with e2e coverage.

Test plan

  • cd web && bunx tsc -b
  • bun run build
  • git diff --check
  • Targeted Playwright e2e coverage for the New Loop shortcut was run during implementation.
  • Lint not run: root, web, and server package manifests do not define a lint script.

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 Loop title/description is create new loop (Cmd/Ctrl+Shift+L).

@Mingholy Mingholy left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Dispatches from a focused <input> with ctrlKey -- correctly expects the dialog NOT to open.
  2. Dispatches on window with metaKey and key: "x" / code: "KeyL" -- tests the event.code fallback 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant