Repository navigation
fix(keys): a key that missed the blocking dialog goes to its keyboard, never the terminal under it - #31849
Conversation
…d (red) Journey paste-confirm-keys (dogfood build 6c761f2926a5): with the paste confirm in the overlay panel and the key sent to the shell window (app not active, debug.key), Return went to the terminal under the dialog and the dialog stayed open. On feat only Escape has a missed-modal route (KeyRouter.cancelsBlockingDialog); Return, Space and typed text go on to the window's first responder, the terminal the dialog blocks. A Return there can run a command. BlockingDialogMissedKeyTests fails on this commit (Return and typed keys pass on to the view under the dialog) and passes with the fix. Ports the test of a37e55c from cp/dogfood-v2-2-12-opt.
…, never the view under it Root cause: a cmux dialog's overlay has the keyboard only while its panel is key. When the key reaches the window under it instead (the app was not active as the dialog opened, the person came back to the window, or automation sent the key to the window), KeyRouter.cancelsBlockingDialog answers Escape only. Return, Space and typed text go on to the window's first responder: the terminal the dialog blocks. A Return there can run a command. KeyRouter.answersBlockingDialog runs first in interceptKeyDown, and CmuxDialogCenter.answersMissedKey (CmuxNextDesign) decides: a key that is not a chord or Escape, in a window or tab a cmux dialog blocks (CmuxDialogCenter.dialogBlockingKeys, as for Escape), goes to that dialog's keyboard (CmuxDialogKeys: Return presses the default, Tab moves focus) and is consumed. Command and Control chords keep their routes; a key for the dialog's own controls (its field in the window's top layer) and a key in another pane or the sidebar list are untouched. Cost on every key-down while no dialog shows: one empty dictionary read (CmuxDialogCenter.showsDialog). CmuxNextApp lines do not rise (the logic is in CmuxNextDesign; two doc comments of KeyRouter+Sheets.swift are shorter). Regression test: BlockingDialogMissedKeyTests (red on the parent commit). Ports 5a87aa8 from cp/dogfood-v2-2-12-opt.
|
cmux status (one comment, edited in place; each line is updated by the job that owns it) Review App of this head (app.zip, kept 1 day)
Shipped in:
|
|
Found 2 test failures on Blacksmith runners: Failures
|
|
Review (independent review subagent): FIX at e152a4a Blocking findingA dialog on a hidden (parked) tab takes every key typed into a Chromium page in the same pane. A Return then presses that hidden dialog's default button. Steps:
What happens:
Escape had the same reach before this PR (8e9a096). The PR widens it from "Escape cancels a hidden dialog" to "all typing in the page is lost, and Return confirms". Suggested fix (root cause, which also fixes the Escape route): in Answers to the review questions
Repo rules
|
…ked in (red) Review of #31849: a pane keeps its hidden tab's view in the window (parking). dialogBlockingKeys matches a tab dialog inside the focused area even when that tab view is hidden, so with a page window of the same pane holding the keyboard, every typed key (and Return: Allow of a hidden paste confirm) went to the hidden dialog.
…ed in Review of #31849: a tab dialog blocks keys only while its tab view is shown (dialogBlockingKeys skips a view with a hidden ancestor). Before, with a page window of the same pane holding the keyboard, every typed key (and Return: Allow of a hidden paste confirm) went to the hidden dialog. This also keeps a parked tab's dialog from taking the shown tab's Escape. Regression test: BlockingDialogMissedKeyTests.aHiddenTabsDialogBlocksNoKeyOfThePaneItIsParkedIn (red on the parent commit).
Bugbot needs on-demand usage enabledBugbot uses usage-based billing for this team and requires on-demand usage to be enabled. A team admin can enable on-demand usage in the Cursor dashboard. |
|
Review (independent delta review subagent): SHIP at d739af0 Scope: the delta e152a4a..d739af0 (3a02f9c red test, d739af0 fix), read against the full PR diff and the code it touches. I did not build or run tests. I used the fleet red and green results in the PR body. 1. The repair (
|
…sions, dialogs, page-first chords), Rust only (#34052) * docs(focus-core): the agent cursor visibility rules are called through the typed C ABI The module comment still said the app asks over the JSON operation. Since #33313 the Swift resolver calls cmux_agent_cursor_visibility; the JSON operation stays and its FFI test replays the vectors. * test(input-router): the router's scenarios and model-based fuzz, red on today's two key rules (R-INPUT S1a) The one input router (handoff/rewrites/R-INPUT/DESIGN.md, stage S1a) in cmux-focus-core::input, with its tests: a scenario for each key defect class R-INPUT ends (P0 1193, REOPEN-F4/F5, #32633, close-then-type, INVERSE-KEY, a click before the first paint, the cross-window key, keys for Home after a new terminal), and a seeded model-based fuzz against a plain reference model, with seven mutants it must catch. This commit is the red: its router keeps TODAY's two rules of the app's key paths, so the tests fail here: - AppKit's dispatch decides where a key that runs no binding goes (the first responder of the key window), as the key window's first responder does now; - a held key runs again through the router when it leaves its hold, and goes where the focus is then (the CreationInputCoordinator / NewTabInputCoordinator replay through NSApplication.sendEvent). The next commit removes both: a key's sink is decided in the key's own call from the window's focus, and a waiting key leaves its lane only to that sink. The topology's Tab gets the host terminal id (`terminal`), so a creation's stand-in tab and the daemon's tab are one terminal sink. The C topology carries none yet (the FFI decode sets None; the app sends it from S3). Run: cmux-ci run --class isolated --script scripts/ci/cmux-tui-rust-check.sh --os linux --arg test --arg key_router:: --ref <this sha> * fix(input-router): a key's sink is fixed in the key's own call; a waiting key leaves its lane only to that sink (R-INPUT S1a) Removes today's two rules from the router (see the red commit): - the sink of a key that runs no binding comes from the window's focus at key time: the tab the focus is on its way to (a creation's), else the focused target. AppKit's dispatch only chooses HOW the key reaches that sink (its responder when it reaches it, else the sink's own path), never which sink; - a key that waits stays in its sink's lane until that sink can take it, and leaves only to it, in typing order; when the sink ends the key is dropped with the reason. No re-aim, no replay through the router, no timer. Rust only, no caller yet: the app links it from stage S3 (shadow), and the behavior stages flip one runtime switch each (DESIGN.md section 5). * fix(input-router): the router's send-or-queue step is route_to, not to_sink (clippy wrong_self_convention) A to_* method that takes &mut self breaks clippy's naming rule (green step 759965538c90: fmt and the 25 tests passed, clippy -D warnings failed on this name only). A pure rename; no behavior change. * test(input-router): key sessions, dialogs, page-first chords and the Escape rule, red on today's rules (R-INPUT S1b) Stage S1b of the one input router (handoff/rewrites/R-INPUT/DESIGN.md sections 4.4, 4.7 and 4.8), Rust only, no caller yet: - key sessions (link hints the person started, window hints, the shortcut recorder): at most one runs (I5), and a key it takes reaches no sink (I4); - blocking dialogs (a window's or one tab's): a key typed under a dialog is the dialog's, a key typed before it waits for the answer with its own target; - page-first chords: a chord a page may own goes to the page first, and its action runs once, by the key's own id, only when the page leaves it unhandled; - Escape in a field the person moved into from content gives the keyboard back to that content (#33049). The tests: 20 scenarios (#32537 review rounds, #31849, #32291/#32409, #32849, #33049, with their STATUS items), the reference model and the fuzz extended with sessions, dialogs and page reports, and four more mutants the fuzz must catch (a hint letter to the page, two sessions, an action run twice, a key under a dialog to its target). This commit is the red: the router keeps TODAY's rules (no key session in the router, a dialog gets keys only through AppKit, a bound chord runs at once, Escape stays in the field), so the tests fail here. The next commit adds the rules. * fix(input-router): sessions take their keys, dialogs block their targets, page-first chords wait for the page's report, Escape returns from a field (R-INPUT S1b) Adds the four rules to the router's one decision per key (see the red commit): - a running key session takes its window's hint keys (letters, Backspace, Escape; the recorder: every key) in the key's own call; any other key, a key of another window, a focus move off its target, its window closing or a newer session ends it, and that key then routes as usual. At most one runs; link hints start only on the page the window's keys go to; - a dialog that blocks a key's target takes the key (a bound chord still runs), and a target a dialog blocks takes no key: keys typed before the dialog wait for the answer and go on in order, or are dropped when the answer closes their tab; - a chord a page may own (Find, a page-editing chord, a content action) goes to the focused page first when the page takes keys now; its action runs once, aimed at the key's tab, when the page reports that key unhandled. A report of a newer key forgets the older offers (a page reports in key order); - a bare Escape in a sidebar or text field the person moved into from content gives the keyboard back to that pane, unless the field uses Escape itself. Rust only, no caller yet: the app links it from stage S3 (shadow), and S6/S7 flip input.router.pageKeys and input.router.overlays (DESIGN.md section 5). * test(input-router): sink lifetimes, creation replies, actions that wait, the key's own target for its binding (R-INPUT S1 review), red Scenarios for the findings of the first independent review of PR #34052 and for the fuzz failure of the S1b green step (fleet step 1803a1b30c2e, seed 39, I7: a key reached Host("T2") after the ledger saw T2 end): - a terminal that ended takes no key while its tab still shows, and takes keys again only when a window shows it after none did (seed 39); - Reopen Closed brings a tab back under its old id: the new page takes keys after its own first paint; a creation that names an ended tab again makes it a new sink (finding 1); - a creation that fails after its stand-in showed still ends its terminal: waiting and later keys are dropped with the reason (finding 2); - a Paste before a page's first paint waits for it and keeps its place; a Paste under a dialog runs at once (finding 3); - a Ghostty keybind typed for a new tab runs its action (finding 8); - the 1193 scenario checks the tab each Cmd-D runs on (finding 9); - a key-down that reached nothing has no repeat or release anywhere (finding 10); - an address bar expected by surface takes the keys typed for it (finding 11); - seed 39's shrunk case: a key for the open palette waits for it and always ends (delivered, or dropped when the palette closes). This commit is the red: the router is unchanged, so these tests fail here (the palette and 1193 checks pass: they pin behavior that is right today). * fix(input-router): an ended sink takes keys only as a new sink, creations stay known until their reply, input actions wait like keys, the binding rules read the key's own target (R-INPUT S1 review) Fixes the findings of the first independent review of PR #34052 and the fuzz failure of the S1b green step (fleet step 1803a1b30c2e, seed 39): - Sink lifetimes (finding 1, seed 39): a tab's sink that ended takes no key while its tab still shows, and takes keys again only as a new sink: a window shows it again after none did (Reopen Closed reuses the tab id), or a creation names it again. An ended sink that no window shows and no held key names is forgotten. The router says so with the new SinkEnded and SinkNew effects, and the ledger follows them, so its I7 check holds across a reused id. A window's field, overlay or dialog never ends that way. - Creation replies (finding 2): a creation stays known until its reply (new event Created, or CreationFailed), so a failure after the stand-in showed still ends its terminal. Creation carries the tab's kind. - Input actions (finding 3): an action that edits an input (Paste) waits in that input's lane while the input cannot take input, like a key, and runs when a key could go; under a dialog it edits the dialog's input at once. - Lane limit (finding 7, R9 R-L1): LaneLimit::for_memory derives the bound from physical memory (1/4096 of it, at 1 KiB a waiting key: 4096 keys on 16 GiB); the platform reads hw.memsize at start. - Binding rules (finding 8): the tier and Ghostty rules read the key's own target (the tab the focus is on its way to), the same state as its sink; a page gets a chord first only when the key is aimed at that focused page. - A press that reached nothing (finding 10): no target, dropped, or its sink became a new one: its repeats and release are consumed. - Expected targets (finding 11): a chrome target only on a browser tab, as the focus reducer keeps it; a creation known by its surface keeps its tab. - The ledger keeps only what can still change (finding 6), and names its two protocol checks OwnCall and IdOrder. - Tests: the fuzz runs 20,000 cases of 200 events on every core, with a lane limit of 8, new actions (creation replies after the show, Reopen Closed with the same id, an end while the tab shows, chrome targets, surface keys), ids that never repeat by chance, and a finale the checker adds itself (the shrinker can no longer cut it off, which made seed 39's shrunk case look like a waiting key). It compares the action and tab of every run (finding 9). Two more mutants (I7: a key to an ended sink; I8: an unbounded lane), and the mutant test checks that the real router passes each case first. A counting allocator checks 0 allocations per key, release and bound chord. The router splits into lane, lifetime and binding modules. * fix(input-router): the fuzz finale's window list uses a match guard (clippy collapsible_match) Green step 05d9b051358a at 5ddd5d8 passed fmt and all 56 key_router:: tests (the 20,000-case fuzz, the 0-allocation test, the 13 mutants) and failed clippy -D warnings on one collapsible_match in the test target (fuzz.rs:814). No behavior change. * test(input-router): exits that stay ended, creations done before their tab shows, offers per page with a visible end, bare keys under a dialog (R-INPUT S1 second review), red Scenarios for the findings of the second independent review of PR #34052 (receipt #34052 (comment)): - an exited terminal takes no key after a workspace switch, a tab move or a window reopen, until a restart names it again; an exited terminal whose tab closes is a new tab when reopened (finding 2); - a creation done before its tab shows keeps the keys typed for its terminal (finding 1); - a page that does not report unhandled keys (WebKit) gets no offer: its chords run at once (finding 3); - the offers waiting for one page are bounded by the lane limit and the oldest past the bound closes visibly; a page that closes closes its offers; a handled report closes the offer and the page's older ones (finding 4, R9 R-L1/R-L5); - one page's report keeps the offers of another page (finding 5); - a bare bound key under a dialog goes to the dialog (finding 6); - a Paste for an exited terminal is dropped like a key (finding 7); - copy mode is the focused terminal's, not a new terminal's; the release of an offered key reaches its page; a Paste under a dialog names no tab (nits). The API gets what the tests name: GoneReason::Exited, the PageHandled event, the OfferClosed effect with OfferEnd, and KeyRecord.page_reports_unhandled. The ledger checks I4 on key-downs (a key-down typed while a session runs is the session's unless the session ended first in the call) and accepts an offer's close. The reference model and the fuzz state the new rules; the generator adds replies before the show, exits, restarts, workspace switches and handled reports. This commit is the red: the router is unchanged, so these tests fail here. * fix(input-router): an exited terminal stays ended until a restart, a done creation keeps its keys until its tab shows, offers go only to pages that report and end once per page (R-INPUT S1 second review) Fixes the findings of the second independent review of PR #34052: - Exits (finding 2): GoneReason::Exited is an end whose tab stays (a terminal's process ended). The sink stays ended wherever its tab shows (a workspace switch, a tab move, a window reopen) until a creation names it again (a restart) or its tab closes. Only a closed or failed sink becomes new when a window shows it again after none did (Reopen Closed). - Creations (finding 1): a done creation is kept until a window shows its tab, so a reply that comes before the store's delta never sends the keys for the new terminal to the old target. One reply a creation: a failure after "done" changes nothing. - Offers (findings 3-5, R9 R-L1/R-L5): only a page that reports the keys it leaves unhandled (KeyRecord.page_reports_unhandled, the app's KeyFacts.pageReportsUnhandledKeys) gets a chord first, so a WebKit page's chords run at once as today. Each offer ends exactly once: its action runs, or OfferClosed says why not (Handled, PageGone, Overflow). A page's answer (PageUnhandled, or the new PageHandled) closes only that page's older offers. The offers waiting for one page are bounded by the lane limit (from physical memory), and the oldest past it closes visibly. - A bare key under a blocking dialog is the dialog's, bound or not; a Command or Control chord still runs (finding 6). - An input action for an ended input is dropped with its reason, like a key (finding 7); under a dialog it names no tab. - Copy mode is the focused terminal's, never a new terminal's; the release of a key-down a page got first goes to that page after a repeat that ran; readiness after SinkNew is documented (nits). * test(input-router): a newer key's report closes the older offers as handled, visibly (R-INPUT S1 second review) Green step 2dfb26f0dc2c at f7edc24: fmt and clippy -D warnings on all targets passed, and 67 of 68 key_router:: tests passed (the 20,000-case fuzz, the 13 mutants, the 0-allocation test and the four regression tests of the second review). The one failure was this S1b scenario: it asserted the old rule, where a newer key's report made the older offers vanish with no sign. DESIGN.md 4.7 (v1.3) and R9 R-L5 say every offer ends exactly once, visibly: the older offer closes with OfferClosed { Handled }, and its action never runs. The router is right; the test now asserts that rule. * test(input-router): a failed restart keeps an exited terminal ended; a blocking dialog takes bare keys before a session (R-INPUT S1 third review), red The third independent review of PR #34052 (at 679b1d1, receipt #34052 (comment)): - major: "Restart Shell Here" on an exited terminal's tab is a creation of the same terminal. When it fails, the dead tab stays, but the router records the end as CreationFailed, which follows the closed-tab rule: after a workspace switch, a tab move or a window reopen the terminal is new again, and keys go to a shell that does not run. Scenario a_failed_restart_of_an_exited_terminal_keeps_it_ended_wherever_it_shows. The fuzz: a restart's failure keeps its tab (World::restart marks its reply), so later seeds reach this path. - minor: a running key session took bare keys typed under a blocking dialog; today the dialog gets them first. Scenario a_blocking_dialog_takes_bare_keys_before_a_running_session. This commit is the red: the router is unchanged. * fix(input-router): a failed restart leaves an exited terminal exited; a blocking dialog takes bare keys before a key session (R-INPUT S1 third review) Fixes the two findings of the third independent review of PR #34052: - A creation remembers that it restarts an exited sink. When it fails, the dead tab stays, so the sink ends as Exited again, not CreationFailed: it stays ended wherever its tab shows (a workspace switch, a tab move, a window reopen) until another restart works. - A blocking dialog over the key's target takes a key with no Command or Control before any key session and any binding (dialog_first, DESIGN.md 4.4 step 3, v1.4): today's order, and Chrome's (a page's alert blocks its hint script). The session keeps running and takes its keys again after the answer. A key of another window still ends a running session first. The ledger's I4 check accepts a dialog's key while a session runs. The reference model states both rules. DESIGN.md section 8 lists the behavior changes of S1. * test(input-router): the ledger requires the offers of a page to end in key order (R-INPUT S1), red At dce7c8c the ledger accepted an offer's OfferClosed but did not require it: an older offer that vanished with no end (the expectation that 679b1d1 replaced) passed the ledger. DESIGN.md 4.7 says a page answers its keys in key order and every offer ends exactly once, so the offers to one page must end in key order (I3). This test feeds the ledger both streams directly, with no router and no model: the silent one must break I3 at the newer key's run, and the router's order (the older offer closed as handled, then the newer key's run) must pass. This commit is the red: the ledger is unchanged. * fix(input-router): the ledger requires the offers of a page to end in key order (R-INPUT S1) The ledger keeps each open offer's page, and an offer's end (its action's run, or its OfferClosed) while an older offer to the same page is still open breaks I3. A page answers its keys in key order (DESIGN.md 4.7), so an older offer that vanished with no end can no longer pass: the check that the PR body claimed now exists. Every end the router emits is in that order (an answer closes the page's older offers before it runs or closes its own key; a page's end and an overflow close the oldest first). * fix(input-router): the ledger order test passes its one-effect slice with slice::from_ref (clippy cloned_ref_to_slice_refs) Green step fd523cb817e1 at 75ed18a passed fmt and 71 of 71 key_router:: tests, and failed clippy -D warnings on this one line of the new ledger test (sessions.rs:634). No behavior change.
![Fix with [code]smith](https://pr-comments-assets.blacksmith.sh/codesmith/fix-with-codesmith-light-3x.png)
Summary
A cmux dialog (for example the multi-line paste confirm) has the keyboard only while its overlay panel is key. When a key reaches the window under it instead (the app was not active as the dialog opened, the person clicked back into the window, or automation sent the key to the window), feat answers only Escape (
KeyRouter.cancelsBlockingDialog). Return, Space and typed text go on to the window's first responder: the terminal the dialog blocks. A Return there can run a command.After this change,
KeyRouter.interceptKeyDownfirst asksKeyRouter.answersBlockingDialog;CmuxDialogCenter.answersMissedKey(CmuxNextDesign) decides. A key that is not a Command/Control chord or Escape, in a window or tab that a cmux dialog blocks (dialogBlockingKeys, the same rule as for Escape), goes to that dialog's keyboard (CmuxDialogKeys: Return presses the default button, Tab moves focus) and is consumed. Chords keep their routes (Cmd-W, Cmd-Q); a key for the dialog's own controls, a key in another pane and a key in the sidebar list are untouched. Cost on every key-down while no dialog shows: one read of an empty dictionary (showsDialog). CmuxNextApp lines do not rise.Ports 5a87aa8 (and its red test a37e55c) from cp/dogfood-v2-2-12-opt. Feat's KeyRouter has no normal-mode fast path, so the call goes first in
interceptKeyDown, before the new-tab and creation key holds, as on the dogfood line. Row 3 of the dogfood-only fixes audit (handoff/dogfood-only-fixes.md).Testing
Review repair (review FIX, comment 6569985783): a dialog on a hidden (parked) tab no longer blocks the keys of the shown tab in the same pane.
dialogBlockingKeysnow skips a tab view with a hidden ancestor; this also fixes the older Escape route. TestaHiddenTabsDialogBlocksNoKeyOfThePaneItIsParkedIn: red at 3a02f9c (step 39e1b79d14b548c0583e24a6 failed: the parked tab's dialog blocked the shown tab's keys), green at head d739af0 (step ec02f21dd4e5e61e821c878d: BlockingDialogMissedKeyTests 3 of 3 runs, CloseThenTypeKeysTests, DialogFadeTests; compile passed).First round:
Focused command (fleet, Mac):
package-test-lane.sh suite Packages/macOS/CmuxNext BlockingDialogMissedKeyTests.Observed / Not run
Observed: the fleet steps above. Not run: the dogfood journey paste-confirm-keys on a feat build; a live check in a tagged build.
Changelog
Fixed: A Return or typed text that reaches a window while a cmux dialog blocks it goes to the dialog, never to the terminal under it.
Proof
No capture: no visual change; the key route is checked by the test.