mirror of
https://github.com/getpaseo/paseo.git
synced 2026-10-28 04:03:05 +00:00
paseo-bot[bot]andIvan Malison
d6861f81e5
### Linked issue Closes #4821 ### Type of change - [x] Bug fix ### Reasoning A key sequence recorded in Settings, for example `Ctrl+K` then `J`, silently did nothing whenever the app re-rendered between the two keys. Resizing the window, toggling the sidebar, or an agent turn starting or finishing is enough, so a rebound sequence looks broken at random and the second key lands in the composer as text. The global `keydown`/`keyup`/`blur`/`visibilitychange` listeners, and on desktop the `browser-shortcut-input` subscription, were registered by an effect that depended on eighteen render-scoped values. One of them, `useActiveWorkspaceSelection()`, returns a new object every render, so the effect tore down and re-subscribed on every render of `AppContainer`. Its cleanup resets the in-progress sequence, so the second key was resolved from step 0, matched nothing, and fell through as ordinary input. The key handlers now read the latest render through `useStableEvent`, so the listener effect re-subscribes only when shortcuts are enabled or disabled. Clearing held badge modifiers when the workspace-index jump binding changes moved to its own effect keyed on that modifier. The cleanup still resets the sequence, which is now correct, because it only runs when shortcuts genuinely go away. ### Goals - A recorded key sequence completes across a root re-render between its keys. - The global shortcut listeners are registered once while shortcuts are enabled, instead of on every render. - A regression spec that fails on the current `main` and passes with this change. ### Non-goals - `useActiveWorkspaceSelection()` still returns a new object per render. This PR stops the shortcut listeners from depending on that identity, it does not change the hook. - A sequence whose second step also carries a modifier (`Ctrl+K` `Ctrl+J`) is still cancelled when the modifier is re-pressed, because the bare `Ctrl` keydown does not match the next step. That is a separate defect and is not fixed here. - The Electron `browser-shortcut-input` path is unchanged in shape and was not exercised in a desktop build. ### QA New spec `packages/app/e2e/browser/keyboard-shortcut-sequence.spec.ts`. It seeds `Ctrl+K J` as the "Toggle command center" binding, opens a workspace, and presses the sequence twice: once with nothing in between, once with a viewport resize settled over two frames between the keys. Before, on `main`, run twice: ``` ✘ multi-key shortcut sequences › a sequence completes when the window resizes between its keys expect(getByTestId('command-center-panel')).toBeVisible() failed ``` The control assertion in the same test passed both times, so the sequence itself works and only the re-render breaks it. Instrumenting the hook locally showed the mechanism directly: ``` key Control -> chord step 0 key K -> chord step 1 effect cleanup at chord step 1 <- the resize re-render effect setup; changed deps: activeWorkspaceSelection key j -> chord step 0, match null ``` After, the same spec passes, and `activeWorkspaceSelection` was the only dependency that changed on those renders. Also run locally on this branch: - `packages/app` unit suite: 603 files, 5319 tests, all passing. - Playwright `browser` project, keyboard and command-center specs: `keyboard-shortcut-sequence`, `empty-pane-keyboard`, `changes-shortcut-placement`, `sidebar-workspace-pin-shortcut`, `workspace-archive-shortcut`, `command-center-agent-controls`. 17 passed. - `npm run typecheck --workspace=@getpaseo/app`, `oxlint`, and `oxfmt --check` on the changed files: clean. Web only, in Chromium on Linux. Keyboard shortcuts are off on mobile, and the desktop build was not exercised. ### Supersedes - [PR #4834 — fix(app): keep multi-key shortcuts alive across root re-renders](https://github.com/getpaseo/paseo/pull/4834) — same fix for the same bug. I diagnosed #4821 independently, reached the same cause, and built on that commit, so its author is credited with a `Co-authored-by` trailer. This PR replaces its spec with one that drives the app through the keyboard only, without patching `window.addEventListener` to count registrations. ### Checklist - [x] One focused change - [x] `npm run typecheck` passes - [x] `npm run lint` passes - [x] `npm run format` passes - [x] QA evidence - [x] Tests added or updated where it made sense Co-authored-by: Ivan Malison <IvanMalison@gmail.com>