fix(test): stop a swallowed Escape sinking the shortcuts sheet suite - #4744
Merged
Conversation
`gotoApp` opened the command palette to prove the Workspace global-shortcut handler was attached, then closed it with a single Escape and asserted `toBeHidden()`. Closing is setup here — all three tests in this file are about the shortcuts sheet, and nothing covers the palette's own Escape behavior — so a one-shot press made an incidental step a failure mode for the whole suite. The press can be swallowed rather than merely late: `useFocusTrap` attaches its Escape listener and moves focus in the same post-paint effect, and the handler no-ops unless its trap is topmost. A key that lands a beat early is dropped, not queued, so `toBeHidden()` then polls a palette that will never close. That is why it failed outright instead of flaking to a pass — main run 32303914004 held the dialog visible through the whole 5s window. Wait for the trap's observable half (focus inside the dialog, the same guard right-chat-panel.spec.ts uses) and then press Escape until the palette actually closes, mirroring the open poll directly above. Both loops stop on the first press that takes effect, so neither sends a stray key. Verified: 22 passes across `--repeat-each=6 --workers=1` locally. Note this hardens the helper against a dropped key; it does not attempt to fix why the key is dropped, which is not reproducible on an idle machine.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the
Frontend buildfailure onmainatf2fcd625f(run 32303914004) —1 failed, 1 flaky, 3 skipped, 280 passed.What failed
The failure is inside the
gotoAppsetup helper, not in any assertion the three tests actually make.Not a regression from the dependency bumps
main@f2fcd625fand the PR head that preceded it (51a5dc914, #4739) have byte-identical trees — same tree SHAb274a2308534a4a4f1f3eaf5fb4c431f34e0ccc0,git diffbetween them is empty. That PR run was282 passed / 0 flaky / 0 failed. Same code, different outcome, so nothing in the nanoid/postcss/sharp/serde_with work caused this.Why a retry could not save it
useFocusTrapattaches its Escape listener and moves focus into the dialog in the same post-paint effect, andonKeyreturns early unless its trap is topmost:So a press that lands a beat early is dropped, not queued.
toBeHidden()then polls a palette that will never close — which is exactly the shape of the log above: 13 consecutive resolutions tovisibleacross the full 5 s window, a dialog that is persistently open rather than slow to disappear. That is why this failed outright instead of flaking to a pass on retry.The change
Closing the palette is incidental here: the helper opens it only to prove the Workspace global-shortcut handler is live, and nothing in the suite covers the palette's own Escape behavior (
grep "Command palette" tests/matches this helper and nothing else). A one-shot press made that incidental step a failure mode for all three tests.right-chat-panel.spec.ts:189already uses before its own Escape, so it matches existing convention rather than inventing one.No source changes;
tests/keyboard-shortcuts.spec.tsonly, +27/−2.Validation
pnpm exec playwright test tests/keyboard-shortcuts.spec.ts --project=desktop --repeat-each=6 --workers=1→ 22 passedpnpm lint,pnpm typecheck→ passWhat this does not claim
This hardens the helper against a dropped key; it does not fix why the key is dropped. I could not reproduce the gap locally — an instrumented probe over 8 serial runs found focus already inside the dialog at the moment visibility became observable every time (
focusedAtVisible=1, 3–5 ms), so on an idle machine the window is effectively closed. The listener-vs-paint ordering above is a real property of the code and the log matches it, but the exact trigger on a loaded runner is inferred, not demonstrated. A topmost-trap conflict from another overlay would produce the identical symptom and is not ruled out.Since the swallowed press is in setup rather than in a behavior under test, hardening it is the right fix either way — but if the palette's Escape path deserves its own coverage, that is worth a separate issue.
Tracked as
cave-i1c. The second flaky test in that run,right-chat-panel.spec.ts:176(also flaky in #4741), is untouched here and tracked separately ascave-ued— it passed on retry both times and has not been root-caused.