Skip to content

fix(app): a tab moved to a new pane leaves its old pane's strip in the same turn, so focus follows it at once - #31920

Merged
lawrencecchen merged 4 commits into
feat-cmux-nextfrom
fix/dogfood-port-moved-tab-focus
Nov 2, 2026
Merged

lawrencecchen merged 4 commits into
feat-cmux-nextfrom
fix/dogfood-port-moved-tab-focus

Conversation

@lawrencecchen

@lawrencecchen lawrencecchen commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Ports b760527 from cp/dogfood-v2-2-12-opt (dogfood-only fixes audit, row 10).

When a tab moved to a new pane, the workspace's layout apply showed it in its new pane and sent the focus topology while the old pane's strip still listed it. That strip changed only when the old pane's own strip observation ran, a main-actor job later. The first topology had the tab in both panes, so focus did not follow it in that turn; keys typed in that gap reached the window instead of the moved terminal.

On feat, Home's dock no longer shows this: feat's OutboundTab (#28127) keeps a tab that the dock makes out of the dock's strip until it has moved, and the dogfood regression test HomeDockNewTerminalFocusTests passes on feat 3 of 3 (fleet step fd5c7591a5259c2e6904a199). Move Tab to New Split (and other moves of a tab the pane already shows) still has the gap: the new MoveTabToNewSplitFocusTests is red on feat.

Now the model changes in one turn, under one topology (Chrome's rule: input sees the model as of its turn):

  • The workspace's apply brings every pane strip whose record's tabs changed since its last apply up to the store before it sends its one topology (syncStaleStrips). O(tabs of the workspace's panes), only on a layout change.
  • A pane whose own strip observation runs first applies the workspace's layout first (applyLayoutIfChanged, a no-op when the layout did not change), so the other order also moves the tab in one turn.

No key hold and no wait.

App lines: StaticURL (a Foundation-only enum of fixed URLs) moves from CmuxNextApp to CmuxNextAppCore with no change in behavior, so CmuxNextApp does not grow (app_lines ratchet).

Testing

  • MoveTabToNewSplitFocusTests (new): one workspace, two terminal tabs in one pane, tab.moveToNewSplit of the second (origin user). The first topology that lists the moved tab in a new pane must no longer list it in the old pane; focus must follow it in that reduction; focus never resolves to an empty pane.
    • Red on feat: test-only commit 4d5a30d, fleet step 24f91a9c4ae78fd84ea0fb0d (cmux9s-Mac-mini): 5 of 5 runs fail at the same two expectations (the old pane still lists the moved tab; focus stays on the old pane). One of the 5 runs also hit the suite's 3-minute limit after the expectations.
    • Green at e47f4f3, fleet step ff1ad1628fc1be4064596120 (aws-m4pro-1): 5 of 5 pass, and HomeDockNewTerminalFocusTests, HomeDockOpensTests, PaidPlanUpgradeTests, CloudCreateWithoutAskingTests (the StaticURL users) pass.
  • HomeDockNewTerminalFocusTests (the dogfood test, ported as is) passes on feat before the fix (step fd5c7591a5259c2e6904a199, 3 of 3) and with it.
  • Compile checks (CmuxNext, app, cmux-tui): 97b2f3597e57fb786fd0972f passed at the head.

Observed / Not run

Observed: the red and green fleet steps above; the pre-push gate (no new violations, no arch-ratchet rise).
Not run: a live tagged build with typed keys right after Move Tab to New Split; a drag of a tab to a pane edge (it goes through the same layout and strip observation paths, read only).

Changelog

Fixed: After Move Tab to New Split, keys typed at once go to the moved terminal.

Proof

No capture: the change is in the same-turn focus order, not in what a frame shows.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need.


Summary by cubic

Fixes the gap where a tab moved to a new pane stayed listed in its old pane's strip until that pane's strip observation ran later, so the focus topology had the tab in both panes and keys typed in that turn reached the window instead of the moved terminal.

Now the model changes in one turn, under one topology: the workspace's layout apply syncs every stale pane strip before sending its one topology (syncStaleStrips), and a pane whose own strip observation runs first applies the workspace's layout first (applyLayoutIfChanged, a no-op when unchanged).

  • StaticURL moves from CmuxNextApp to CmuxNextAppCore, so CmuxNextApp does not grow (app_lines ratchet).
  • Adds MoveTabToNewSplitFocusTests (red on the base before the fix) and ports the dogfood regression test HomeDockNewTerminalFocusTests.

Written for commit e47f4f3. Summary will update on new commits.

View guided diff Turn on auto-fix

…rn it shows beside the dock

Ports the red test 16e0b97 of b760527 from cp/dogfood-v2-2-12-opt (dogfood v2.1: the first
keys typed into a new terminal asked for from Home's dock were lost while the dock's strip still listed
the moved tab). Run on feat first: feat's OutboundTab hold (#28127) keeps a tab Home's dock makes out
of the dock's strip until it has moved, which may already cover this path.
… shows there

The dogfood red test of b760527 (HomeDockNewTerminalFocusTests) passes on feat 3 of 3 (fleet step
fd5c7591a5259c2e6904a199): feat's OutboundTab keeps a tab that Home's dock makes out of the dock's strip.
Move Tab to New Split moves a tab that its pane already shows, with no such hold. This test checks that
path: the first topology that shows the moved tab in its new pane no longer lists it in its old pane,
focus follows it in that reduction, and focus never resolves to an empty pane.
…e same turn, so focus follows it at once

Ports b760527 from cp/dogfood-v2-2-12-opt (dogfood-only fixes audit, row 10).

A workspace's layout apply showed a moved tab in its new pane and sent the focus topology with the old pane's
strip as it was, which still listed the tab until that pane's own strip observation ran in a later main-actor
job. Focus waited for the tab to leave the old pane and did not follow it; keys typed then reached the window.
On feat, Home's dock no longer shows this (OutboundTab, #28127, keeps a tab the dock makes out of its strip;
the dogfood test HomeDockNewTerminalFocusTests passes on feat), but Move Tab to New Split does:
MoveTabToNewSplitFocusTests is red on feat (fleet step 24f91a9c4ae78fd84ea0fb0d).

The model now changes in one turn, under one topology (Chrome's rule: input sees the model as of its turn):
- The workspace's apply brings every pane strip whose record's tabs changed since its last apply up to the
  store before it sends its one topology (`syncStaleStrips`). O(tabs of the workspace's panes), only on a
  layout change.
- A pane whose own strip observation runs first applies the workspace's layout first (`applyLayoutIfChanged`,
  a no-op when the layout did not change), so the other order also moves the tab in one turn.

App lines: StaticURL (a Foundation-only enum of fixed URLs) moves from CmuxNextApp to CmuxNextAppCore, so
CmuxNextApp does not grow (app_lines ratchet).
@lawrencecchen lawrencecchen added the risk:normal Risk tier (docs/ci/risk-tiers.md) label Nov 2, 2026
@github-actions

github-actions Bot commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

cmux status (one comment, edited in place; each line is updated by the job that owns it)

Review e47f4f3: HQ feed card · gallery before/after runs · checks

App of this head (app.zip, kept 1 day)

gh workflow run reload-build.yml -R manaflow-ai/cmux --ref feat-cmux-next -f tag=pr31920 -f ref=fix/dogfood-port-moved-tab-focus -f platform=macos -f backend_mode=local

Shipped in:

@blacksmith-sh

blacksmith-sh Bot commented Nov 2, 2026

Copy link
Copy Markdown

Found 2 test failures on Blacksmith runners:

Failures

Test View Logs
test_every_compile_job_names_its_warm_sources (main.ThisTree.test_every_compile_job
_names_its_warm_sources)/
test_every_compile_job_names_its_warm_sources (main.ThisTree.test_every_compile_job
_names_its_warm_sources)
View Logs
test_the_repository_highlights_validate (main.DigestTests.test_the_repository_highl
ights_validate)/
test_the_repository_highlights_validate (main.DigestTests.test_the_repository_highl
ights_validate)
View Logs

Fix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need.

@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Review (independent review subagent): SHIP at e47f4f3

Read only. I did not compile or run tests. I read the diff against merge-base b5060b4, the dogfood original b760527, and the code paths around it. The head merges cleanly with the current origin/feat-cmux-next (the only later change to these files is #31837's one-line tab(_:) lookup).

Correctness: no blocking findings

  • Re-entrancy (pane observation -> workspace apply -> syncStaleStrips -> the same pane's sync -> then the observation's own apply). This terminates and is safe. PaneController.apply never calls applyLayoutIfChanged. Only the observation loop calls it (PaneController.swift:151), so nothing recurses. Inside the main apply branch, isApplying is true, so a synced pane's sendTopologyIfChanged -> setNeedsTopology is a no-op and the apply sends one topology. In the topologyMoved branch (WorkspaceContentController.swift:272), isApplying is false. The synced panes queue a pending topology, and sendTopology -> topologySent(by:) drops it. A pane that the apply adopts (adopt -> observe cancels the running task) or tears down (teardown cancels it) stops at the Task.isCancelled guard. Its new or replacement observation has already applied synchronously.
  • applyLayoutIfChanged against the workspace's own observation. On feat, apply has no cause parameter, so this port correctly drops the dogfood original's changeCauses?.take(). The call uses the same non-force path and the same LayoutMapping.map(workspace) that the workspace observation uses. If the store tree did not change, appliedTree.update returns false, and the call does nothing except a handle compare. If it did change, the call applies what the observation would apply, only earlier in the same burst. The later observation is then a no-op. The apply-count and topology-count budgets do not change (PaneCloseApplyTests still expects 0/0, because showPaneClose does not fire a strip observation).
  • Off-screen, parked and suspended panes. Only park() makes a workspace off screen, and park() also suspends every pane, so a suspended pane has no observation. The off-screen branch of apply (deferredApply and repairIfEmpty) therefore cannot run from a pane. stripIsStale excludes suspended panes. That keeps ParkedWorkspacePaneWorkTests at one strip map on unpark: comeOnScreen applies while the panes are still suspended, and then resume maps each pane once. A parked companion that draws (multiView) still applies, and sendTopology is guarded by !isParked, the same as before.
  • OutboundTab and NewTabSuccession. stripIsStale compares the record's tab ids with the record's tab ids at the last apply (PaneController.swift:318), not with strip items. A hidden outbound tab therefore makes the strip stale once, when the record changes, and never again. It cannot loop or sync on every apply. The sync goes through the normal apply(snapshot()), so the OutboundTab holds and NewTabSuccession ordering still apply. If NewTabSuccession's order runs twice in one turn, it has no harmful side effects: the second call sees no new arrival, and pendingSelectSurface was already consumed.
  • Regression test. MoveTabToNewSplitFocusTests captures the first topology that lists the moved tab outside its source pane. It expects that topology to no longer list the tab in the source pane, and expects focus to follow the tab in that reduction. On the base the strip lags by one job, so the test fails for the right reason, and the red step in the PR body shows exactly those two expectations failing. HomeDockNewTerminalFocusTests passes on feat without the fix, as the body says, so it is extra coverage, not the red/green test for this change.
  • StaticURL move. CmuxNextAppCore uses InternalImportsByDefault, so public import Foundation is required for public var url: URL. All five CmuxNextApp users and both test users now import CmuxNextAppCore, and the app test target depends on it. CmuxNextAccounts/PaidPlanUpgrade.swift mentions StaticURL only in a doc comment. The type has cases and an instance url, so it does not break the namespace convention. Behavior does not change.

Non-blocking (follow-ups or nits)

  1. Cost per strip change (PaneController.swift:151, WorkspaceContentController.swift:255). Every strip-structure change of every pane now maps the whole workspace (LayoutMapping.map, a screens compare, adoptHandles), also when the layout did not change. That includes tab create, close, reorder and group changes, but not selection or title changes, which go through tabItems. The cost is O(panes in the workspace), not O(session). It does not run per key or per tab switch. But a scripted burst that changes tabs in K panes in one daemon batch now does K workspace maps in place of one. This is acceptable for this fix. A store revision check before the map would make it O(1) when nothing in the layout changed.
  2. Redundant second apply on the pane-first order (PaneController.swift:151-153). When applyLayoutIfChanged syncs this pane through syncStaleStrips, the loop then applies compose(structure) again. That structure was computed before the sync. In practice it is identical (same turn) and the strip diffs are no-ops, but every tab item is mapped twice (snapshot() refreshes all items). The loop could skip its own apply when the inner sync already ran.
  3. Old pane's selection now presents before the topology moves focus. Inside the workspace apply, the source pane's presentSelection still sees isFocusedInWorkspace == true. If its new selection is a cold live terminal, it opens holdUntilShown. focusDidSettle on the moved terminal flushes that hold, so this is not a lost key. The pane-first order on the base already had this.
  4. Unpark with stale strips is still two topologies (WorkspaceContentController.swift:108-113). This is out of scope. Because suspended panes are excluded, a tab moved while the workspace was parked still gives one topology with stale strips at unpark (comeOnScreen runs before setDrawsNothing(false)), and resume then sends a second. The base has the same behavior.
  5. Stale doc reference (CmuxNextAppCore/StaticURL.swift:5). The comment cites "StaticURLTests", but no such test exists in the repo. The move carried this over from the base.

Verdict: SHIP. Not run: compile, tests and a live tagged build. This review only read the code.

@lawrencecchen lawrencecchen added the ready-to-land Reviewed at this head; next-autoland merges it into feat-cmux-next when its only reds are base reds label Nov 2, 2026
@lawrencecchen
lawrencecchen merged commit 00090e6 into feat-cmux-next Nov 2, 2026
219 of 238 checks passed
@lawrencecchen
lawrencecchen deleted the fix/dogfood-port-moved-tab-focus branch November 2, 2026 07:11

This branch was previously deployed

1 inactive deployment
ci-routing-agent — e47f4f3a Deployed Nov 2, 2026 by lawrencecchen via namespace autofix #12045
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-land Reviewed at this head; next-autoland merges it into feat-cmux-next when its only reds are base reds risk:normal Risk tier (docs/ci/risk-tiers.md)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant