Repository navigation
fix(focus): after a relaunch the saved pane of the shown workspace gets the keyboard, and Home does not take it (QA v2.2.2 bug 1) - #31852
Conversation
…ses its saved pane (QA v2.2.2 bug 1, red) Ports the Rust half of 92f8d64 from cp/dogfood-v2-2-12-opt. The launch window shows the home workspace (Home's page first, the pane beside it with a terminal and the selected browser tab) before it adopts the saved window record. The record's focused pane then comes as restoredPane for the shown workspace, and cmux-focus-core drops it: the first pane (Home) keeps the focus and the typed keys go to Home's Chief message box. Expected red on the base: resolved stays page(home).
…record focuses the saved pane (QA v2.2.2 bug 1, red) Ports the Swift test of 92f8d64 from cp/dogfood-v2-2-12-opt, through the real relaunch path (WindowState.adopt) instead of a bare send. RestoredFocusTests: the launch window shows the home workspace (Home's page, the pane beside it with a terminal and the selected browser tab), then adopts the saved record whose focused pane is the browser pane. Expected red on the base: resolved stays page(home-pane). It stays red until CCmuxAppFFI is repinned to a release with the cmux-focus-core fix: the app links the reducer only through that pin.
…focuses its saved pane (QA v2.2.2 bug 1) Ports bde3363 from cp/dogfood-v2-2-12-opt (the dogfood line later moved the rule into the app's FocusCoordinator, 77c9594, only because its pinned FFI could not take a reducer change; on feat the rule belongs to cmux-focus-core). After Cmd-Q with a browser tab focused in the pane beside Home and a relaunch, typed keys went to Home's Chief message box. The launch window showed the home workspace before WindowManager adopted the saved window record; WindowState.adopt then sent the record's focused pane as restoredPane for the shown workspace, and the rule only remembered a restore for a workspace the window does not show. The first pane (Home) kept the focus. Now restoredPane for the shown workspace makes the saved pane the focus (target content, the responder re-applied) while no user intent happened in the window (generation 0). A view that took the keyboard by itself (a programmatic report: Home's box, AppKit's own pick) does not keep it; a person's click or key does. The recorded Swift corpus has 4 restores for the shown workspace; all come after an intent (generation 4 or 9), so no recorded step changes. The app links this rule only through the CCmuxAppFFI pin: the Swift RestoredFocusTests go green with the repin.
…l keeps it in Home's pane (QA v2.2.2 bug 1, red) Ports 4033999 from cp/dogfood-v2-2-12-opt. Home's message box took the keyboard from a restored browser page: a Chromium page that has the keys leaves the cmux window itself first responder, so Home read 'nothing has the keyboard'. FocusState.keepsKeyboard(inContentOf:) is the model's answer Home must ask. Red on the base: it does not exist (the test target does not compile).
… the focus model keeps it in Home's pane (QA v2.2.2 bug 1) Ports 47d6a3f and its follow-up c93f03a from cp/dogfood-v2-2-12-opt. Home took the keyboard from a restored browser page: when it shows a conversation nobody opened (TopHomePageView.show), it gave the message box the keyboard whenever the window's first responder was the window or nil, now or when the box is installed later (MessagesLabHomeView.finishOpenKeyboard). A Chromium page that has the keys keeps them in its own page window and leaves the cmux window itself first responder, so that read 'nothing has the keyboard', and the focus model then took the programmatic report as a move to Home's pane. Fix: the focus model answers. NSWindow.keyboardIsFree(for:) is true only when no view of the window holds the keyboard and, for a view in a pane the focus model lists, the model keeps the keyboard in that pane's content (FocusState.keepsKeyboard(inContentOf:)). Outside every pane, or in a pane the model does not list yet (a first launch before the first topology), the first-responder rule alone decides, as before. Home's unasked path asks it when the box takes the keyboard (now, or at its install: takeKeyboardOnOpen(while:)). A conversation the person opens (a click, Return, a switch key, an action) takes the keyboard as before. Test: RestoredFocusTests.homeTakesTheKeyboardUnaskedOnlyWhileTheModelKeepsItInHomesPane.
|
Review (independent review subagent): SHIP at 43d699e Read-only review of What I checked
Findings (none blocking)
CI on this head (classified)
|
…rkspace's restore off; doc the new restoredPane rule Review of #31852: the 'Home took it' case reported the pane that already had the focus, a no-op, so it did not prove that a programmatic report is no person's choice. The new case moves the target (the saved pane's address bar, programmatic) before the record comes; the restore still puts the keyboard on the page. FocusEvent.restoredPane (Swift) and FocusEvent::RestoredPane (Rust) now say what the rule does for the shown workspace.
|
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)
red: cmux-next checks (god files, concurrency, crash safety, l10n); NEW: green on the base e34fecc; do not merge until green; see lane-rules |
|
Found 6 test failures on Blacksmith runners: Failures
|
…ood-port-restored-pane
|
Review (independent delta review subagent): SHIP at a1ef7df Scope: the delta since the SHIP at 43d699e (56aa6b2, plus the merge a1ef7df of feat-cmux-next 947bb5e). Read-only review with git; nothing compiled or run. New test case (
Docs: accurate.
Merge a1ef7df: clean.
Not run: no Rust or Swift test was run for this review. The red/green evidence for the new case must come from the fleet run named in the PR. |
…ns in the cmux-tui workspace) The cases of focus_rules.rs (cmux-layout-reducer-ffi, its own Cargo workspace) that cover QA v2.2.2 bug 1, in the crate that owns the rule, so the fleet Rust test step (cmux-tui-rust-check.sh test) runs them. Red on the base (the rule dropped restoredPane for the shown workspace), green with e4a5c7a.
|
Review (independent delta review subagent): SHIP at 77e15d9 Scope: delta a1ef7df..77e15d9 (one new file, Correct
Nits (do not block)
Duplication: The cases are copied on purpose from |
|
Review (next-review patch-id; carried): SHIP at 20ab387 The independent SHIP at 77e15d9 carries to this head: the PR's own change ( |
|
Split per lead decision (STATUS 5388, two-step FFI move). Step A, the Rust reducer change alone: #32055 (branch fix/dogfood-port-restored-pane-rust). This PR (#31852) is now step B: the Swift half (Home's unasked keyboard gate, 47d6a3f) and the 4 RestoredFocusTests. It lands after #32055 lands and the bot's CCmuxAppFFI repin is on feat; I then merge feat here (the Rust commits become no-ops) and run the Mac suites. No ready-to-land until then. |
|
Review (next-review patch-id; carried): SHIP at bf6bcba The independent SHIP at 77e15d9 carries to this head: the PR's own change ( |
…focuses its saved pane (QA v2.2.2 bug 1, step A) (#32055) * test(focus): a record for the workspace the window already shows focuses its saved pane (QA v2.2.2 bug 1, red) Ports the Rust half of 92f8d64 from cp/dogfood-v2-2-12-opt. The launch window shows the home workspace (Home's page first, the pane beside it with a terminal and the selected browser tab) before it adopts the saved window record. The record's focused pane then comes as restoredPane for the shown workspace, and cmux-focus-core drops it: the first pane (Home) keeps the focus and the typed keys go to Home's Chief message box. Expected red on the base: resolved stays page(home). (cherry picked from commit a52678b) * test(focus): cmux-focus-core's own test of the restored-pane rule (runs in the cmux-tui workspace) The cases of focus_rules.rs (cmux-layout-reducer-ffi, its own Cargo workspace) that cover QA v2.2.2 bug 1, in the crate that owns the rule, so the fleet Rust test step (cmux-tui-rust-check.sh test) runs them. Red on the base (the rule dropped restoredPane for the shown workspace), green with e4a5c7a. (cherry picked from commit 77e15d9) * fix(focus-core): a record for the workspace the window already shows focuses its saved pane (QA v2.2.2 bug 1) Ports bde3363 from cp/dogfood-v2-2-12-opt (the dogfood line later moved the rule into the app's FocusCoordinator, 77c9594, only because its pinned FFI could not take a reducer change; on feat the rule belongs to cmux-focus-core). After Cmd-Q with a browser tab focused in the pane beside Home and a relaunch, typed keys went to Home's Chief message box. The launch window showed the home workspace before WindowManager adopted the saved window record; WindowState.adopt then sent the record's focused pane as restoredPane for the shown workspace, and the rule only remembered a restore for a workspace the window does not show. The first pane (Home) kept the focus. Now restoredPane for the shown workspace makes the saved pane the focus (target content, the responder re-applied) while no user intent happened in the window (generation 0). A view that took the keyboard by itself (a programmatic report: Home's box, AppKit's own pick) does not keep it; a person's click or key does. The recorded Swift corpus has 4 restores for the shown workspace; all come after an intent (generation 4 or 9), so no recorded step changes. The app links this rule only through the CCmuxAppFFI pin: the Swift RestoredFocusTests go green with the repin. (cherry picked from commit e4a5c7a) * test(focus-core): a programmatic target move does not keep a shown workspace's restore off; doc the new restoredPane rule The Rust part of 56aa6b2 on fix/dogfood-port-restored-pane (#31852): the focus_rules.rs case and the event.rs doc comment. The matching Swift doc comment (FocusEvent.swift) lands with the Swift half after the repin.
|
For awareness: this PR conflicts with feat-cmux-next, so it can't land until it's rebased or base is merged in. Its current review verdict stays as is; a hand-resolved merge gets a fresh review at the new head. |
![Fix with [code]smith](https://pr-comments-assets.blacksmith.sh/codesmith/fix-with-codesmith-light-3x.png)
Summary
After Cmd-Q with a browser tab focused in the pane beside Home and a relaunch, the keys a person types go into Home's Chief message box (text for an agent), not to the browser page that had the keyboard (QA v2.2.2 bug 1).
Ports 77c9594 and 47d6a3f from cp/dogfood-v2-2-12-opt (with their companions 92f8d64, bde3363, 4033999 and c93f03a). On the dogfood line the restore rule ended in the app's FocusCoordinator only because that build could not take a reducer change; on feat the rule belongs to cmux-focus-core (plans/cmux-next/layer-ownership.md), so this PR puts it there.
Two causes, two fixes:
reducer.rs,RestoredPane). The launch window shows the home workspace before it adopts the saved window record.WindowState.adoptthen sends the record's focused pane asrestoredPanefor the workspace the window already shows, and the rule dropped it (it only remembered a restore for a workspace the window does not show). Now, for the shown workspace, the saved pane becomes the focus (target content, the responder re-applied) while no user intent happened in the window (generation 0). A view that took the keyboard by itself (a programmatic report: Home's box, AppKit's own pick) does not keep it; a person's click or key does. The recorded Swift focus corpus has 4 restores for the shown workspace, all after an intent (generation 4 or 9), so no recorded step changes.TopHomePageView.showof a conversation nobody opened, and the message box's later install inMessagesLabHomeView.finishOpenKeyboard) read "the window itself is first responder" as "nothing has the keyboard". A Chromium page that has the keys leaves the cmux window itself first responder. Now the unasked path also asks the focus model: for a view in a pane the model lists, the model must keep the keyboard in that pane's content (FocusState.keepsKeyboard(inContentOf:),allowsUnaskedKeyboard(inContentOf:)). A conversation the person opens (a click, Return, a switch key, an action) takes the keyboard as before.To keep
CmuxNextAppfrom growing (arch ratchetapp_lines),KeyRouter.mayHavePrimaryInput(_:)moves toFocusState.Resolved.mayHavePrimaryInputin CmuxNextFocus (same rule, two callers).Repin: BLOCKED (do not land yet)
The app links cmux-focus-core only through the prebuilt CCmuxAppFFI pin. The R3b rule says the repin goes in this same PR, but STATUS 5013 (rust-migration) found that a PR cannot repin itself today: app-ffi-release.yml publishes only from feat-cmux-next, and the
ffi-releaseenvironment admits only that ref. Decision 5013 waits for cmux-ad. Until a pre-merge release exists, this PR has no ready-to-land.Check the app FFI pinis red on this PR because of that (the reducer changed with no repin). Two Swift tests stay red on this head until the repin:theSavedPaneOfTheShownWorkspaceGetsTheKeyboardandaViewThatTookTheKeyboardByItselfDoesNotKeepTheRestore. The repin PR #31684 (REOPEN-F4) also changes the pin line; this PR does not touch it.Testing
Swift (
RestoredFocusTests, through the real relaunch pathWindowState.adopt; fleet Mac lane, compile-checks passed first: 1b269db218a7d8e055324007 at the red ref, 8defc5b7b0aeaf74e15df608 at 56aa6b2):resolvedstayspage(home-pane), nomoveResponder):theSavedPaneOfTheShownWorkspaceGetsTheKeyboard,aViewThatTookTheKeyboardByItselfDoesNotKeepTheRestore. 2 guard tests passed (thePersonsChoiceBeforeTheRecordStays,aRecordBeforeTheWorkspaceShowsStillLandsOnItsPane), as they do on the base rule too.homeTakesTheKeyboardUnaskedOnlyWhileTheModelKeepsItInHomesPaneand the 2 guard tests pass; the same 2 restore tests fail the same way, as expected with the old pin.HomeTypeToFocusTests(9) andHomeOpenFocusesComposerTests(2) pass: the Home keyboard paths a person uses still work.Rust (
cmux-layout-reducer-ffi/tests/focus_rules.rs:a_record_for_the_shown_workspace_focuses_its_saved_pane,a_record_before_the_workspace_shows_still_lands_on_its_pane). The crate is its own Cargo workspace, so the fleetcmux-tui-rust-check.sh teststeps (572f40404ba2, bbfa6a835f69) and thetest (linux)dispatch jobs do not reach it; their green says nothing. Thelint (linux)job runs it, but on every dispatch so far (runs 41394713812, 41394716337, 41397167791, 41397170316) a base clippy red stopped the job before that step (#31462'ssuspicious_open_options, fixed by #31697; then three clippy errors incmux-tui-coreon feat 947bb5e). The red and green go through a Testbox run ofcargo test --test focus_rulesin that crate; results are added here.Observed / Not run
restoredPanesteps for the shown workspace, all at generation 4 or 9, so no recorded step changes (a script overtests/focus/corpus.json, repeated by the reviewer).TopHomePageView.focusPrimaryInput(unasked:),MessagesLabHomeViewopenKeyboardAllowed). The new test checks only the focus-model predicate it asks; a revert of the gate keeps every test green. A gate test needs a realWindowControllerwith a Home tab beside a browser page that has the keys; not written.Rust red/green (cmux-focus-core, fleet, Linux)
cmux-tui-rust-check.sh test restored_panerunscmux-tui/crates/cmux-focus-core/tests/restored_pane.rs(3 of the 5 restored-pane cases of focus_rules.rs, in the cmux-tui workspace; commit 77e15d9; the addressBar case stays only in focus_rules.rs, which the fleet step does not run):restored_pane_for_the_shown_workspace_focuses_its_saved_panepanics at restored_pane.rs:41 (resolved stays Home's page); the guardrestored_pane_never_overrides_the_persons_choicepasses.Changelog
Fixed: After Cmd-Q with a browser tab focused beside Home and a relaunch, typed keys go to the browser page again, not into Home's Chief message box.
Proof
No capture: a focus-routing fix with no visual change; the proof is the red/green tests above.