Skip to content

fix(app): keys typed after Cmd-N then Ctrl-` reach the new terminal, not the New Tab page (P0) - #30827

Merged
lawrencecchen merged 3 commits into
feat-cmux-nextfrom
fix/p0-cmdn-ctrlbt-keys-newtab-hold
Nov 1, 2026
Merged

lawrencecchen merged 3 commits into
feat-cmux-nextfrom
fix/p0-cmdn-ctrlbt-keys-newtab-hold

Conversation

@lawrencecchen

@lawrencecchen lawrencecchen commented Nov 1, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Judged smoke of dogfood build v2.2.13-opt (STATUS 3190), round 2 step 16: after Cmd-N, Ctrl- and a typed line, the line's keys went to the New Tab page (a Chromium page window), not the new terminal. Found while porting #29530 to the dogfood line (#30271): its end-result test shows the new terminal's screen empty (sh-3.2$`) and all 10 keys "consumed" by the key router, none held by the creation hold.
  • Cause: Cmd-N opens the window's New Tab hold (NewTabInputCoordinator.beginIncoming), so every key waits for the new page's field. The Ctrl-(or Cmd-D, New Column, New Row, a browser tab) pressed then opens its creation hold, but the New Tab hold runs first inKeyRouter.interceptKeyDown, and nothing ended it (its chord release fires only when it already holds keys). So the keys typed after the Ctrl- waited for the page and went to its field.
  • Fix, one seam for every creation that moves a window's focus: CreationInputCoordinator.begin ends that window's New Tab hold once its own hold is open (NewTabInputCoordinator.creationTookKeys, the same window lookup capture uses). The keys that hold kept run again through the key router in typing order, so they go into the creation's hold first. A script's creation (no view change allowed) opens no hold and ends none. No App line growth.

Testing

  • Red first: CmdNThenCreateKeysTests (9856f7f): Cmd-N with the daemon stopped, then Ctrl- (and Cmd-D), then "echo held" Return, each key through the app's dispatch (interceptKeyDown, then the window; held keys replay the same way). Pass only when the new terminal's screen shows the line and the shell's answer held`, and no other terminal shows it.
  • Compile: a2dad0a6b74370c26adc2d61 passed (CmuxNext with tests, app, cmux-tui).
  • Test fix 3cfc05a: the test's New Tab page never reports ready, so on the base its page hold handed the keys on at its 4 s bound and the first red (808acd14b268c72028550d5a at 9856f7f) PASSED by mistake. The test now sets incomingHoldLimit to 120 s, so the page hold keeps its keys while the page opens, as a person's does until their page is ready.
  • Compile at the head 3cfc05a: c0a97fcacad0dfe21b20c2df passed (CmuxNext with tests, app, cmux-tui).
  • Red, CmdNThenCreateKeysTests at 13df94d (the head with the fix 6bf54eb reverted; run as role base, display, CMUX_TEST_REQUIRE_GUI=1): 6577bde00daddb56b5f50bb0 FAILED both tests at CmdNThenCreateKeysTests.swift:98: the new terminal's screen is sh-3.2$, and all 10 keys were consumed with "New Tab hold true".
  • Green at the head 3cfc05a: fd2b6cd4fd63beb7cced8a97 PASSED: CmdNThenCreateKeysTests 2/2 (Ctrl-and Cmd-D: the typed line andheld` show in the new terminal, and no other terminal shows the line), CmdNThenNewTerminalTabTests 3/3, gui-session-v1 console line present.
  • Router probe on the feat tip 4976f6e with only the test (branch fix/p0-ctrlbt-keys-feat-probe, 33bbdc9, never merge): 84e76d4a69e34c0788d6bd68 FAILED: screen sh-3.2$, 10/10 keys consumed with "new-tab hold true". The bug is on feat. fix(app): Ctrl-` right after Cmd-N opens its terminal; a create on a pane still opening waits for it (P0 STATUS 2080) #29530's own 3 tests pass on that tip because they do not go through the router.

Observed / Not run

Changelog

  • Fixed: keys typed right after Cmd-N then Ctrl-` (or Cmd-D) go to the new terminal, not the New Tab page.

No capture: key routing fix; evidence is the display-worker end-result test.
perf claim: none.

…he new terminal, not the New Tab page

Judged smoke of dogfood v2.2.13-opt, round 2 step 16 (#30271): after Cmd-N, Ctrl-` and a
typed line, the keys went to the New Tab page. CmdNThenCreateKeysTests types "echo held\r"
through the app's dispatch (KeyRouter.interceptKeyDown, then the window) right after Cmd-N
and Ctrl-` (and Cmd-D), with the daemon stopped, and passes only when the new terminal's
screen shows the line and the shell's answer, and no other terminal shows it.
PasswordManagerRouteTests.withApp takes a heldKeyReplay (a test window is never key).
Red expected on feat-cmux-next: the keys wait in Cmd-N's New Tab hold.
…s: Cmd-N then Ctrl-` types into the new terminal

Judged smoke of dogfood v2.2.13-opt, round 2 step 16 (#30271): after Cmd-N, Ctrl-` and a
typed line, the line went to the New Tab page. Cmd-N opens the window's New Tab hold
(NewTabInputCoordinator.beginIncoming): every key waits for the new page's field. The
Ctrl-` (or Cmd-D, New Column, a browser tab) that followed opened its creation hold, but the
New Tab hold runs first in KeyRouter.interceptKeyDown and nothing ended it (its chord
release only fires with keys already held), so every later key still waited for the page
and went to its field when it was ready.

One seam for every creation that moves a window's focus: CreationInputCoordinator.begin ends
that window's New Tab hold once its own hold is open (NewTabInputCoordinator.creationTookKeys,
the same window lookup capture uses). The keys the New Tab hold kept run again through the key
router in typing order, so they go into the creation's hold first and reach the new terminal.
A script's creation (no view change allowed) opens no hold and ends none.
isHolding is the same check on one line (offsets the App lines).
@lawrencecchen lawrencecchen added the risk:normal Risk tier (docs/ci/risk-tiers.md) label Nov 1, 2026
@github-actions

github-actions Bot commented Nov 1, 2026 •

Copy link
Copy Markdown
Contributor

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

Review 3cfc05a: 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=pr30827 -f ref=fix/p0-cmdn-ctrlbt-keys-newtab-hold -f platform=macos -f backend_mode=local

red: cmux-next checks (god files, concurrency, crash safety, l10n); no finished base run on e96e37a to compare; do not merge until green; see lane-rules

Shipped in:

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 4 files

You’re at about 90% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.

View guided diff | Turn on auto-fix | Re-trigger cubic

@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Review (claude-opus-5-5 review subagent): SHIP at 6bf54eb
reviewer-session: 1b08ce05-40ed-4e9c-88b4-7f0fbef772ab
author-session: a7414ded41eeb0207
R9: pass | the end-result regression test CmdNThenCreateKeysTests reads the new terminal's screen (typed line and shell answer) and checks that no other terminal shows the line; it was committed before the fix (9856f7f)
perf claim: none
compile: a2dad0a6b74370c26adc2d61 passed; green 87e65ff5fd18182e7342b8fa passed

Note: the review request named head 6bf54ebb40364b1c2e30d10a541a5ff02c2c5bd3. The real PR head is 6bf54eb (from git rev-parse and gh). Both jobs ran at the real head.

Findings (correctness first, all from reading the code):

  • Root cause is correct. KeyRouter.swift:185-186 runs NewTabInputCoordinator.capture before CreationInputCoordinator.capture. The Cmd-N incoming hold (KeyRouter+NewTabInput.swift:164) has Entry.owns == true for its whole life (line 69). The chord release at line 317 fires only when keys are already held. So with no keys held, Ctrl-` / Cmd-D did not end the hold, and the keys typed after it waited for the page's field.
  • The fix is sound. CreationInputCoordinator.swift:52: the defer runs after buffer(in:).open(...) is evaluated, so the creation hold exists when the New Tab hold is released. release(_:) (NewTab line 366) removes the entry before buffer.release(), and NewTabInputBuffer.release sets replaying. Each held key goes through replay -> interceptKeyDown (line 256). The New Tab capture finds no entry, and CreationInputCoordinator.capture puts the key into the new hold in typing order. It does not go to the page or to another view. Keys that had no lane run at flush time into the focused (new) terminal (take(focused:)).
  • Script and automation case is safe. When ActionRunScope.viewChangeAllowed() is false, the guard at line 51 returns before the defer is registered: no hold opens and none ends. newTerminalTab with typing/typingAhead passes window nil, so it also opens and ends nothing.
  • The other callers of begin are a person's split (PaneHandlers.swift:131), newTerminalTab (PaneController+Intents.swift:147), New Column/Row createInKeysTurn (WorkspaceContentController+Intents.swift:132), the browser stand-in (PaneBrowserTabOpener.swift:210) and LinkOpener.move via continueCreation (LinkOpener.swift:418). None of them is part of opening a New Tab page. Cmd-T (NewTabPage+Open.swift:55) and the page split (NewTabPage+Split.swift:41) use the New Tab hold only and never call begin, so they keep their own keys. continueCreation runs only while a creation is pending, so the New Tab hold has already ended by then (no-op).
  • Minor, acceptable: entry(for:) (line 238) falls back to a Cmd-Shift-N followsNewWindow hold. So a person's creation in any window during a New Window open ends that hold. This matches the "newer intent" rule.
  • Minor, pre-existing, not changed by this PR: keys typed between Cmd-N and the Ctrl-` go out at the chord release (line 317), to whatever has focus. That happens before the creation hold exists.
  • Test: CmdNThenCreateKeysTests checks the end result. The injected heldKeyReplay (interceptKeyDown, then window.sendEvent) copies CmuxApplication.sendEvent for plain keys. CloseThenTypeKeysTests uses the same pattern. Risk: the base can fail only if the page reports ready before incomingHoldLimit (4 s). If the page never reports ready, the base hold releases at 4 s, probably into the new terminal. Record the red result 808acd14b268c72028550d5a in the PR when it ends. It was still running at review time; I did not see it. The dogfood-line observation (10 of 10 keys consumed, creation hold count 0) supports the red.
  • Repo rules: no blocking main-thread call is added. No new type. No file grows (KeyRouter+NewTabInput.swift net -1 line, CreationInputCoordinator.swift +1 line). Line length follows the files' existing style. No CI gate is added.

@blacksmith-sh

blacksmith-sh Bot commented Nov 1, 2026 •

Copy link
Copy Markdown

Found 6 test failures on Blacksmith runners:

Failures

Test View Logs
/tmp/tmp3qm67duv/summary-fail.json (live):/summary-fail.json (live): View Logs
/tmp/tmptu8u1d_j/summary-missing.json (live):/summary-missing.json (live): View Logs
[new_workspaces] debug.new_workspace timed 0 of 50 new workspace rows/
[new_workspaces] debug.new_workspace timed 0 of 50 new workspace rows
View Logs
[new_workspaces] the window draws no frames/[new_workspaces] the window draws no frames View Logs
[tabs] debug.tab_switch timed 0 of 40 switches/
[tabs] debug.tab_switch timed 0 of 40 switches
View Logs
[tabs] the window draws no frames/[tabs] the window draws no frames View Logs

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

…the base loses them as a person's does

Red 808acd14b268c72028550d5a at 9856f7f PASSED: the test's New Tab page never reports its
field ready, so on the base the page's incoming hold reached its 4 s bound and handed the keys
on to the new terminal. A person's page reports ready and takes them (smoke step 16). The test
sets incomingHoldLimit to 120 s, so the page's hold keeps its keys while the page opens, as it
does for a person until their page is ready. The pass condition is unchanged: the new terminal's
screen shows the typed line and its answer within 15 s.
lawrencecchen added a commit that referenced this pull request Nov 1, 2026
…minal's keys in the creation hold, as feat does

Green 3f0893af4165fda1bde2f4ad at 673b6bf (with #30827's fix): the 10 keys typed after
Cmd-N then Ctrl-` now left the New Tab hold, but went to the Ctrl-` stand-in terminal's view
("to TerminalSurfaceView"), and the new terminal's screen stayed "sh-3.2$". On this line
(107f5da, 050651b) the creation hold gives a new terminal's keys to its early view,
which sends them to the daemon by the terminal id its create minted. With #29530 a create on
a stand-in pane (Cmd-N's first pane, a split in flight) waits for the daemon's pane before it
is sent, so the daemon does not know that id yet when the keys come. feat-cmux-next never gives
the optimistic Ctrl-` an early tab: its keys wait in the creation hold for the daemon's terminal.

DaemonService.queuesCreatingInput(on:) answers false for a stand-in target
(ProvisionalPane.isStandIn); Ctrl-` and Cmd-D use it for their early tab, so those keys wait in
the hold and run again once focus landed on the daemon's terminal. New Column and New Row are
unchanged (not covered by a test here).

Test seam (only this part of feat's HeldKeyReplay, #28436): CreationInputCoordinator.replay,
NSApp.sendEvent by default; the tests set their window's. Tests: the ported #29530 key test and
#30827's CmdNThenCreateKeysTests (Ctrl-` and Cmd-D) set it and keep Cmd-N's page hold open as a
person's page does; the pass condition is the new terminal's screen text.
@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Review (opus-5.5 second-pass key-routing reviewer): SHIP at 3cfc05a
reviewer-session: df0e1525-74f6-4006-86c8-5aca516349cc
author-session: a7414ded41eeb0207
R9: pass | end-result regression test CmdNThenCreateKeysTests (committed before the fix in 9856f7f) reads the new terminal's screen for the typed line and the shell's held, and checks that no other terminal shows the line; red on the base sources, green at the head
perf claim: none
compile: c0a97fcacad0dfe21b20c2df passed; red 6577bde00daddb56b5f50bb0 failed as expected; green fd2b6cd4fd63beb7cced8a97 passed

Scope: whole diff 914e25d..3cfc05a, read with KeyRouter.interceptKeyDown, NewTabInputCoordinator (all its lookups), CreationInputCoordinator and every begin/beginIncoming caller.

Root cause: correct. interceptKeyDown asks the New Tab hold before the creation hold (KeyRouter.swift:185-186). An incoming (Cmd-N) hold owns the keys always (KeyRouter+NewTabInput.swift:69), and its chord release fires only with pending keys (:317-318), so after Cmd-N then Ctrl-` nothing ended it. The red log confirms this: all 10 keys "consumed (creation hold true, New Tab hold true)".

Fix: correct. The defer in CreationInputCoordinator.begin (CreationInputCoordinator.swift:52) runs after the creation buffer is open, and after the guard, so script creations and the page paths that pass a nil window (PaneController+Intents.swift:146, NewTabPage.split) end nothing. release removes the entry before it replays (KeyRouter+NewTabInput.swift:366-371), so the replayed keys go into the creation hold, in order. A released hold cannot be reached later: pageReady, acknowledge, flush/flushAll and endIncoming all look up buffers, from which the entry is gone, so a page that reports ready after the release is a no-op and cannot pull the keys. The page cannot take focus on its own then either (AgentPaneView.refusesSelfFocus outside an input event).

Test commit 3cfc05a (incomingHoldLimit = 120 s): a fair model. On the base the cause it fails on is the real one (the New Tab hold keeps the keys after Ctrl-). In the app, that hold sends them to the page's field once the page is ready (or, when the page is hidden, sends them late at the 4 s limit). The 120 s value affects only the base: at the head the hold ends at Ctrl-, before any key is typed, so the limit is never read and cannot make the green pass. 120 s is more than the 10 s + 15 s waits, so the test cannot pass by the limit path. The 15 s screen wait is only a ceiling and hides no delay: the green tests passed in 2.5 s and 0.73 s, and the red failed at about 17.6 s and 15.7 s.

Non-blocking notes (follow-ups, not holds):

  1. continueCreation (LinkOpener.swift:418, Home dock move-beside) also goes through begin, so its defer also ends a New Tab hold. At that time any earlier hold has already been ended, so it can only end a NEWER Cmd-T/Cmd-N hold opened between the tool open and its move, and those keys then go to the moved terminal. The window is short (one daemon reply) and rare. A fix: skip creationTookKeys on the continuation path, for example with a parameter on begin.
  2. The test models "the page keeps the keys" with the limit, not by its readiness. A follow-up that calls newTabInputCoordinator.pageReady(token, key:) for the Cmd-N page after the keys would cover the exact path a person hits, and show that readiness is a no-op after the release.
  3. Not run: the other New Tab hold suites (NewTabHoldFocusTests, MultiWindowInputRoutingTests, NativeNewTabTypeAheadTests, NewTabSplitRefusalTests, AgentPaneTabOpenGestureTests). By reading, none of them opens a creation hold while a page hold is live.

@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 1, 2026
@lawrencecchen
lawrencecchen merged commit d7128e6 into feat-cmux-next Nov 1, 2026
238 of 253 checks passed
@lawrencecchen
lawrencecchen deleted the fix/p0-cmdn-ctrlbt-keys-newtab-hold branch November 1, 2026 14:09
lawrencecchen added a commit that referenced this pull request Nov 1, 2026
… it is not needed

Probe 7ec3a8abe34e5cbc20966aae at 66e36b7 (this branch without that rule, with the Ghostty
runtime started in the test harness) passed all five key tests: CmdNThenCreateKeysTests 2/2
(Ctrl-` and Cmd-D after Cmd-N: the typed line shows in the new terminal, no other terminal shows
it) and CmdNThenNewTerminalTabTests 3/3. The failures that led to the rule (3f0893af, 37f96e2b)
came from the harness, whose terminal views had no Ghostty surface. So this line keeps its own
early-key path (107f5da, 050651b) unchanged: queuesCreatingInput(on:) and
ProvisionalPane.isStandIn go. The test replay seam CreationInputCoordinator.replay stays (the
default is NSApp.sendEvent, as before).
lawrencecchen added a commit that referenced this pull request Nov 1, 2026

This branch was previously deployed

1 inactive deployment
ci-routing-agent — 3cfc05ad Deployed Nov 1, 2026 by lawrencecchen via cmux-tui tree preflight #96154
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