fix(shell): stop keys pressed inside a shell surface from reaching page-level listeners

[Chris0Jeky]

Summary

PR #2635 guarded the page behind a modal with a capture-phase window listener in AppShell.vue, but it has to stand aside when the keydown target is inside the surface — it runs ahead of every handler the surface owns, so stopping there would break typing and arrow navigation. That carve-out was the leak this issue records: keys pressed with focus inside the surface ran the surface's handlers and then kept bubbling to page-level window listeners. On a Legacy board, Tab into the open help dialog and press f or n and the board's useKeyboardShortcuts listener toggled the filter panel and pulled focus into the add-card composer. Same class with Ctrl+Shift+C opening CaptureModal over a board and Tab off the textarea onto a button.

This adds the other half of the same guard: a document-level bubble-phase listener, kept beside the capture-phase one.

Why document bubble, and why it needs only one file. That phase is the single seam between the surface and the page. All four surfaces bind their own keys on their own elements (ShellCommandPalette / PaperCommandPalette @keydown.down/up/enter/escape, CaptureModal @keydown, PaperShortcutsOverlay's backdrop), so those handlers have already run by the time the event reaches document; every page-level listener binds on window, one hop further out. Stopping there cannot take a key away from the surface that owns it — which is why no surface component needed changing and the whole mechanism lands in AppShell.vue. The two guards now share one per-event surface scan, so the pair costs no more than the capture half did alone (#1968's laziness is preserved, and its no-scan-while-typing spec still passes untouched).

Listener audit

Every page-level keydown listener outside tests. The guard silences page-level window listeners under any modal, so all of them were checked. Grep: addEventListener filtered to key events across src, excluding src/tests.

Listener	Where	Phase	Handles	Must fire under a modal?	How the guard treats it
handleKeydown	AppShell.vue	window, capture	Shell ledger keys (?, mod+k, mod+shift+c, h/t/b/i/r, g chord) plus the #2635 guard	Yes — it is the shell owner	Untouched. Capture on window is the first thing to run; it now shares its surface scan with the new guard.
handleEscapeKeydown	useEscapeStack.ts	window, capture	Escape to the top registered handler	Yes	Untouched, twice over: capture runs before document bubble, and the guard never stops Escape.
handleKeyDown	useKeyboardShortcuts.ts	window, bubble	Board keymap — f, n, j, k, l, Enter, arrows, and Escape to BoardView.closeOpenUi	No, except Escape	Silenced while a surface is active — this is the defect. Escape still reaches closeOpenUi.
handleGlobalKeydown	PaperShortcutsOverlay.vue	window, bubble	Escape only, to close the overlay	Yes (Escape)	Unaffected: the guard never stops Escape, and the handler ignores every other key.
onCaptureShortcut	PaperHomeView.vue	window, bubble	mod+; to focus the quick-capture row	No — it would pull focus out of the modal	Silenced while a surface is active, except from a text-entry target (see Not verified).
handleGlobalKeydown	PaperInboxView.vue	window, bubble	mod+; to toggle capture variant	No	Same as above.
handleKeyDown	useShortcutContext.ts	document, bubble	Contextual shortcuts for the active context	No	Beyond the six the claim listed. Not reached in production — useContextualShortcuts has no caller outside its own spec — and stopPropagation would not stop it anyway (same node, same phase). Recorded, not changed.
handleKeyDown	useReviewKeymap.ts, via PaperReviewView.vue	window, bubble	Review decisions (apply / reject / request-edit / defer / provenance / preview)	No	Also beyond the claim's six, and registered indirectly: the composable's target option defaults to window. Its enabled predicate already gates its own dialogs (#1818, GH-1969) but not the shell surfaces, so the guard adds that. Audited read-only — PaperReviewView is leased to the #2214 residual pass; nothing there was changed and no spec was added to its spec files.

Not page-level, so out of range by construction: ApplyToBoardDialog.vue and BatchExecuteDialog.vue bind keydown on their own dialog elements, and LegacyReviewView.vue uses a template @keydown — all of which run before document. BatchExecuteDialog's window listener is keyup, a different event. LegacyReviewView.vue carries no window keydown listener.

Changes

• AppShell.vue — new guardPageListenersFromSurfaceKeys, registered on document in the bubble phase in onMounted and removed in onUnmounted. Two carve-outs, matching the capture half's reasoning: Escape is never stopped, and text-entry targets are left alone (useKeyboardShortcuts ignores them anyway, and the early-out is what keeps [Frontend][UX] Documented bare-letter shortcuts (H/T/B/I/R, G T, board C/R) have no handler, and Settings has no Keyboard page #1968's promise that an ordinary keystroke in a field never pays for the surface scan).
• AppShell.vue — the per-event surface scan is lifted out of handleKeydown into a shared keyboardOwningSurfacesFor(event) memo so the two guards scan once between them, not twice. The answer is pinned to the event rather than re-read on the bubble: a surface handler may have closed its own surface on the way up, and the key still belonged to the surface that was open when it was pressed.

Test plan

Specs dispatch from a node inside the surface with bubbles: true. A window-dispatched event is AT_TARGET, where capture and bubble listeners both run whatever propagation says, and every assertion below would have passed against the unguarded source.

Red-first, against the unmodified source (4 of the 6 new cases failed; the other 2 are regression guards that already held):

• keeps board keys off the board when focus is inside the help dialog — AssertionError: expected 1 to be +0 (boardProbe.filterToggles)
• keeps the bare-letter navigation set and the g-chord off page listeners from inside a surface — AssertionError: expected [ 'h', 't', 'b', 'i', 'r', 'g' ] to deeply equal []
• keeps board keys off the board with focus inside a capture modal over the board — AssertionError: expected 1 to be +0
• keeps board keys off the board from a focused option inside the command palette — AssertionError: expected 1 to be +0

Already green before the fix, kept as regression guards: still lets Escape out of a surface to the page close paths and leaves each surface its own keys with focus inside it (? closes the help dialog from a control inside it; arrows move the palette selection; mod+k closes the palette from its own input).

Commands, each with the file count the summary reported:

• npx vitest --run --maxWorkers=2 src/tests/components/AppShell.spec.ts — Test Files 1 passed (1), 66 tests. Includes the untouched [Frontend][UX] Documented bare-letter shortcuts (H/T/B/I/R, G T, board C/R) have no handler, and Settings has no Keyboard page #1968 spec does not scan the DOM for modal surfaces on an ordinary keystroke in a field.
• npx vitest --run --maxWorkers=2 over the shell surfaces and keyboard composables — AppShell.spec.ts, AppShell.paperVariant.spec.ts, ShellKeyboardHelp.spec.ts, ShellCommandPalette.spec.ts, CaptureModal.spec.ts, paper/PaperCommandPalette.spec.ts, paper/PaperShortcutsOverlay.spec.ts, useKeyboardShortcuts.spec.ts, useEscapeStack.spec.ts, useEscapeToClose.spec.ts, useShortcutContext.spec.ts — 11 named, Test Files 11 passed (11), 189 tests.
• npx vitest --run --maxWorkers=2 over the boards and Paper views that own page-level listeners — BoardView.spec.ts, BoardView.keyboardRouting.spec.ts, BoardView.coverage.spec.ts, paper/PaperBoardView.spec.ts, paper/PaperBoardCard.spec.ts, paper/PaperHomeView.spec.ts, paper/PaperHomeView.escape.spec.ts, paper/PaperHomeCaptureRecovery.spec.ts, paper/PaperInboxView.spec.ts — 9 named, Test Files 9 passed (9), 228 tests.
• npx vitest --run --maxWorkers=2 over the review keymap seam the audit turned up — review/ApplyToBoardDialog.spec.ts, useReviewKeymap.spec.ts, paper/review/ReviewKeymap.spec.ts — 3 named, Test Files 3 passed (3), 68 tests.
• npm run typecheck (vue-tsc -b) — clean.
• npx eslint src/components/shell/AppShell.vue src/tests/components/AppShell.spec.ts — clean.
• git diff --check — clean.
• E2E against the config's own web server, with the four Llm__Gemini__* variables unset: npx playwright test tests/e2e/keyboard-navigation.spec.ts tests/e2e/workspace-help.spec.ts --reporter=line — 6 passed (33.1s).

Not verified

• No browser check of the real focus path by hand; the leak and the fix are proven at component level plus the two E2E specs above.
• The guard does not cover a page-level listener that acts on a modifier combo while focus is in a modal's text field — PaperHomeView / PaperInboxView mod+; typed inside a modal textarea still reaches them. That is outside this issue's acceptance, which names f, n, the bare-letter set and the g-chord (all non-text-entry), and closing it would mean scanning on every keystroke typed anywhere, breaking [Frontend][UX] Documented bare-letter shortcuts (H/T/B/I/R, G T, board C/R) have no handler, and Settings has no Keyboard page #1968's guarantee. Recorded here rather than fixed.
• No full frontend suite or backend run: the change is one frontend component and its spec.

Boundaries

• Files changed: AppShell.vue and AppShell.spec.ts only — both owned by this claim. No surface component needed changing, for the reason given above.
• No change to the shortcut ledger rows or the notation guard (Keystroke ledger documents shortcuts that don't exist, omits ones that do, and there are three divergent shortcut systems #2007 stays a ruling).
• PaperReviewView.vue / LegacyReviewView.vue and their specs are leased to the Review queue refresh residuals: deep-link re-auth, poll restart after recovery, stale indicator (from PR #2208) #2214 residual pass: audited read-only and reported in the table, nothing changed there, no spec added to their files.
• No doc edits.

Refs #2636

---

Round 2 (head 8da74c1b5)

Fresh-context review returned SHIP with no logic defect. One MEDIUM and three LOWs, all comment truth on a safety seam plus one coverage gap, fixed in a single commit — no mechanism change, so no E2E rerun.

MEDIUM — the guard docblock stated a false premise. It justified the guard as "all four surfaces bind their own keys on their own elements". Two things were wrong. PaperShortcutsOverlay — a help twin, aria-modal, data-shell-surface="keyboard-help" — binds its Escape handler on window in the bubble phase, one hop outside the document guard; it survives on the Escape carve-out, not on that premise. And the guard's reach is not four surfaces: it fires for every dialog[open], [role="alertdialog"] or [aria-modal="true"] in the app, which a grep puts at 16 components today (CardModal, TdDialog and the review dialogs built on it, ProvenanceDrawer, PaperBoardDialogShell, the four board modals, WorkspaceSetupModal, MfaChallengeModal, both palettes, both help twins, CaptureModal).

The docblock now states the real invariant: a surface keeps a key only if it handles that key at or below document — an element-level handler anywhere from the target up to and including document — or if the key is Escape. Anything a surface binds on window is outside this guard and, unless it is Escape, gets silenced. That makes a window-level non-Escape handler belonging to a modal a forbidden shape here, and item 4 pins it.

LOW — false lifetime claim on the memo reset. The unmount comment ("nothing should outlive the shell holding a reference to a detached surface") described a module-level hazard. scannedEvent/scannedSurfaces live in the per-instance setup() closure and die with the instance. The reset is kept but now reads as belt-and-braces, and the declaration says the memo is per-instance so two mounted shells never share one.

LOW — unverified mechanism in a spec comment. The comment asserted "the selection re-render replaces the input element". Rather than swap one unverified claim for another, I re-measured with a throwaway probe: after the ArrowDown the captured node reports isConnected === false and no longer matches the selector, so the element the test holds really is detached under this mount (jsdom, Teleport stubbed). Why Vue drops it is not pinned down, so the comment now states the measurement and the consequence rather than a mechanism — and adds why the re-query cannot mask a defect: a key dispatched on a detached node never enters the tree, so no listener runs and no state changes, meaning the palette would stay open and the exists() assertion would go red.

LOW — coverage gap. All six from-inside specs ran with the Paper theme off. Added one Paper-skin spec (mockPaperTheme.isOn = true) on PaperShortcutsOverlay, the one surface whose Escape lives on window: focusing its close button, a bare f and n reach neither a window-bubble probe nor the board keymap, and Escape still closes the overlay. Red-first was not required (the mechanism is skin-independent) but I checked it anyway by disabling the document listener: red with expected [ 'f', 'n' ] to deeply equal [], then restored.

Also folded in: this suite now stubs versionApi the way AppShell.paperVariant.spec.ts already does. The Paper sidebar reads the product version on mount, so the new spec was logging a real ECONNREFUSED on every run; that noise is now zero.

Proof at 8da74c1b5 — npx vitest --run --maxWorkers=2 src/tests/components/AppShell.spec.ts src/tests/components/AppShell.paperVariant.spec.ts src/tests/components/paper/PaperShortcutsOverlay.spec.ts → 3 named, Test Files 3 passed (3), 94 tests (AppShell alone is now 67, up one). npm run typecheck clean; npx eslint on both changed files clean; git diff --check clean. Still the same two files changed overall, and no doc edits.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

[Chris0Jeky]

Review record (alpha product-trust lane; one fresh-context read-only reviewer subagent since Codex credits are exhausted, SC-9; round 1 at head 8e0fe2efb, inputs: the clean worktree at the head and the merge-base diff, 2 files).

Verdict at round 1: SHIP; no CRITICAL or HIGH, and the reviewer could not construct a path from the changed lines to a surface key dying or a page key that must fire and does not. Triage:

• MEDIUM, fixed in round 2 as comments plus one spec: the guard's docblock claims safety because "all four surfaces bind their own keys on their own elements"; PaperShortcutsOverlay (a help twin) binds its Escape on window bubble, outside the document guard, and survives only through the Escape carve-out, and the guard's reach is every aria-modal, dialog[open] and alertdialog in the app, not four surfaces. No live defect (every surface in the tree handles its keys at element level, re-derived by the reviewer's grep); the defect is the false invariant a future contributor would read. Round 2 states the real invariant and pins the Paper help overlay from inside.
• LOW, fixed in round 2: the unmount reset's comment describes a module-level lifetime hazard for a per-instance memo.
• LOW, fixed in round 2: a spec comment names the wrong mechanism for the palette input re-query (the assertion itself cannot mask a defect).
• LOW, fixed in round 2: the six from-inside specs run with the Paper theme off; one Paper-skin spec from inside the help overlay is added.
• LOW, recorded, correct as stated by the PR: mod+; pressed in a modal's text field still reaches the Paper Home and Inbox listeners (text-entry targets are skipped by design, and useKeyboardShortcuts and useReviewKeymap already ignore text targets); outside the issue's acceptance; tracked on #2636 as the residual.
• LOW, the lane's: docs/STATUS.md line 268 records "a key pressed with focus inside the surface still bubbles to page-level listeners" as a known residual; this PR closes it, so the eighteenth STATUS block retires that clause.

Clean lenses corroborated by the reviewer against the tree at the head: document bubble runs before window bubble and stopPropagation there leaves other document listeners untouched; the page-listener audit re-derived independently matches the PR's eight; every surface's own key handling is element-level (both palettes, CaptureModal, TdDialog's trap, CardModal's Tab trap, ProvenanceDrawer, PaperBoardDialogShell, the batch dialogs, the four board modals); no preventDefault, so Tab, Space, Enter, typing and scrolling keys keep their defaults; the Escape carve-out is by key, not target; the shell's own ? and mod+k bindings run at window capture with stopImmediatePropagation and never meet the bubble guard; the #1968 zero-scan-while-typing pin holds; the memo is keyed by event identity, so only a re-dispatched event object could read a stale answer; pinning the surface answer to the event is right in both directions; the new specs dispatch with bubbles: true from a real focused node and assert on the real board keymap plus a window-bubble probe, with page.stop() before every assertion; the p provenance key was suspected and refuted.

Unverified by the read-only reviewer, run by the worker at the head (AppShell spec 66; shell surfaces and keyboard composables 11 files / 189; page-listener views 9 files / 228; review keymap seam 3 files / 68; typecheck, eslint, diff-check; keyboard-navigation and workspace-help E2E 6 passed locally) and by CI's frontend unit job. Round count: 2 (comments and one spec, no logic, so no further pass is owed). Merge gate remaining: the fix head's CI green and the three-minute age.

[Chris0Jeky]

Round 2 at 8da74c1b5 (one commit, the same two files, no mechanism change): the guard's docblock now states the real invariant (a surface keeps a key only if it handles it at or below document, or the key is Escape), names PaperShortcutsOverlay as the one surface that fails the premise and survives on the carve-out, declares a window-level non-Escape handler on a modal a forbidden shape, and puts the guard's reach at 16 components (measured), not four; the memo reset is relabelled belt-and-braces for a per-instance memo; the palette-input spec comment states what was measured (the captured node is detached after the selection re-render under this mount, mechanism not pinned) and why the re-query cannot mask a defect; a Paper-skin spec from inside the help overlay pins f and n reaching neither a window-bubble probe nor the board keymap while Escape still closes it (red with the document listener disabled: expected [ 'f', 'n' ] to deeply equal []); the new Paper mount's version read is stubbed the way the Paper-variant spec already does. Three named specs 3 files / 94 tests, typecheck, eslint and git diff --check clean. Comments and one unit spec only, so no further review pass is owed. Merge gate: CI green at 8da74c1b5 plus the three-minute age.