Repository navigation
P0 port: keys typed after Cmd-N then Ctrl-` reach the new terminal (#30827, #29530 keys part) - #30271
Merged
lawrencecchen merged 10 commits intoNov 1, 2026
Merged
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 15 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Packages/macOS/CmuxNext/Tests/CmuxNextDaemonTests/CmdNStandInTargetTests.swift">
<violation number="1" location="Packages/macOS/CmuxNext/Tests/CmuxNextDaemonTests/CmdNStandInTargetTests.swift:59">
P2: These five yields do not establish that `daemonTarget` entered its stand-in wait, so this regression test can pass without exercising the race. Park until `store.standInWaits` contains the wait and assert that state (and apply the same change to the identical loop in the failure test).</violation>
</file>
<file name="Packages/macOS/CmuxNext/Sources/CmuxNextDaemonService/NewTerminalTabCommand.swift">
<violation number="1" location="Packages/macOS/CmuxNext/Sources/CmuxNextDaemonService/NewTerminalTabCommand.swift:62">
P2: The `target` closure now suspends, so a cancelled wait throws CancellationError. The `send` closure maps every non-PaneNotReady/PendingPaneNotCreated failure to NewTerminalTabFailure, so the person sees 'the daemon refused the new tab' even though no request was sent. Handle the cancellation explicitly, or make the message reflect that nothing was sent.</violation>
</file>
<file name="Packages/macOS/CmuxNext/Tests/CmuxNextAppTests/PaneVerbOnPendingPaneTests.swift">
<violation number="1" location="Packages/macOS/CmuxNext/Tests/CmuxNextAppTests/PaneVerbOnPendingPaneTests.swift:114">
P2: This assertion does not verify that the new tab landed in the pending pane's daemon pane; a tab opened in another pane would still satisfy the global `tabsBefore + 2` count. Assert that the pending pane itself gained the tab, and apply the same target-specific check to the repeated assertion below.</violation>
</file>
<file name="Packages/macOS/CmuxNext/Sources/CmuxNextDaemonService/NewRowCommand.swift">
<violation number="1" location="Packages/macOS/CmuxNext/Sources/CmuxNextDaemonService/NewRowCommand.swift:36">
P3: `daemonTarget` can now throw `PendingPaneNotCreated`, but this command still converts it to `PaneSplitFailure`, falsely reporting a daemon refusal. Preserve and rethrow `PendingPaneNotCreated` like `PaneSplitCommand` so a failed parent create reports that the target pane was not created.</violation>
</file>
<file name="Packages/macOS/CmuxNext/Sources/CmuxNextDaemonService/NewColumnCommand.swift">
<violation number="1" location="Packages/macOS/CmuxNext/Sources/CmuxNextDaemonService/NewColumnCommand.swift:48">
P3: `daemonTarget` can now throw `PendingPaneNotCreated`, but this command still converts it to `PaneSplitFailure`, falsely reporting a daemon refusal. Preserve and rethrow `PendingPaneNotCreated` like `PaneSplitCommand` so a failed parent create reports that the target pane was not created.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| let send = Task { @MainActor in | ||
| do { outcome.pane = try await store.daemonTarget(workspace.pane) } catch { outcome.error = error } | ||
| } | ||
| for _ in 0..<5 { await Task.yield() } |
There was a problem hiding this comment.
P2: These five yields do not establish that daemonTarget entered its stand-in wait, so this regression test can pass without exercising the race. Park until store.standInWaits contains the wait and assert that state (and apply the same change to the identical loop in the failure test).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At Packages/macOS/CmuxNext/Tests/CmuxNextDaemonTests/CmdNStandInTargetTests.swift, line 59:
<comment>These five yields do not establish that `daemonTarget` entered its stand-in wait, so this regression test can pass without exercising the race. Park until `store.standInWaits` contains the wait and assert that state (and apply the same change to the identical loop in the failure test).</comment>
<file context>
@@ -0,0 +1,170 @@
+ let send = Task { @MainActor in
+ do { outcome.pane = try await store.daemonTarget(workspace.pane) } catch { outcome.error = error }
+ }
+ for _ in 0..<5 { await Task.yield() }
+ #expect(outcome.error == nil, "the create on Cmd-N's stand-in was refused: \(String(describing: outcome.error))")
+ #expect(outcome.pane == nil, "the create named a pane before the daemon had one: \(String(describing: outcome.pane))")
</file context>
Suggested change
| for _ in 0..<5 { await Task.yield() } | |
| for _ in 0..<100 where store.standInWaits.isEmpty { await Task.yield() } | |
| #expect(store.standInWaits.count == 1, "daemonTarget did not park while Cmd-N was opening") |
| // The target at send time (as Cmd-D, New Column and New Row): a Ctrl-` pressed on a stand-in whose create is in | ||
| // flight (a split's, Cmd-N's first pane) waits for it and names the daemon's pane that took it over, never the | ||
| // stand-in (``DaemonStore/daemonTarget(_:)``). P0 STATUS 2080: refused there before, no terminal. | ||
| let target: @MainActor @Sendable () async throws -> PaneID = { try await daemon.store.daemonTarget(pane) } |
There was a problem hiding this comment.
P2: The target closure now suspends, so a cancelled wait throws CancellationError. The send closure maps every non-PaneNotReady/PendingPaneNotCreated failure to NewTerminalTabFailure, so the person sees 'the daemon refused the new tab' even though no request was sent. Handle the cancellation explicitly, or make the message reflect that nothing was sent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At Packages/macOS/CmuxNext/Sources/CmuxNextDaemonService/NewTerminalTabCommand.swift, line 62:
<comment>The `target` closure now suspends, so a cancelled wait throws CancellationError. The `send` closure maps every non-PaneNotReady/PendingPaneNotCreated failure to NewTerminalTabFailure, so the person sees 'the daemon refused the new tab' even though no request was sent. Handle the cancellation explicitly, or make the message reflect that nothing was sent.</comment>
<file context>
@@ -55,9 +56,10 @@ public struct NewTerminalTabCommand: Sendable {
+ // The target at send time (as Cmd-D, New Column and New Row): a Ctrl-` pressed on a stand-in whose create is in
+ // flight (a split's, Cmd-N's first pane) waits for it and names the daemon's pane that took it over, never the
+ // stand-in (``DaemonStore/daemonTarget(_:)``). P0 STATUS 2080: refused there before, no terminal.
+ let target: @MainActor @Sendable () async throws -> PaneID = { try await daemon.store.daemonTarget(pane) }
let send = daemon.intendNowReporting("new-tab", intent, transaction: transaction) { connection in
var sent = sent
</file context>
| // The split confirms, and the typed tab opens in its daemon pane: the split's tab plus this one. | ||
| try await PinnedPageTests.until({ | ||
| content.panes.values.allSatisfy { $0.pane.handle.rawValue < ProvisionalPane.firstHandle } | ||
| && content.panes.values.reduce(0) { $0 + $1.pane.tabs.count } == tabsBefore + 2 |
There was a problem hiding this comment.
P2: This assertion does not verify that the new tab landed in the pending pane's daemon pane; a tab opened in another pane would still satisfy the global tabsBefore + 2 count. Assert that the pending pane itself gained the tab, and apply the same target-specific check to the repeated assertion below.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At Packages/macOS/CmuxNext/Tests/CmuxNextAppTests/PaneVerbOnPendingPaneTests.swift, line 114:
<comment>This assertion does not verify that the new tab landed in the pending pane's daemon pane; a tab opened in another pane would still satisfy the global `tabsBefore + 2` count. Assert that the pending pane itself gained the tab, and apply the same target-specific check to the repeated assertion below.</comment>
<file context>
@@ -109,23 +108,22 @@ struct PaneVerbOnPendingPaneTests {
+ // The split confirms, and the typed tab opens in its daemon pane: the split's tab plus this one.
+ try await PinnedPageTests.until({
+ content.panes.values.allSatisfy { $0.pane.handle.rawValue < ProvisionalPane.firstHandle }
+ && content.panes.values.reduce(0) { $0 + $1.pane.tabs.count } == tabsBefore + 2
+ }, report: {
+ "the typed new tab did not open: \(content.panes.values.reduce(0) { $0 + $1.pane.tabs.count }) tabs, \(tabsBefore) before the split; HUD \(services.refusalHUD.message ?? "none")"
</file context>
| let intent = Intent.splitPane(pane: pane, direction: .down, ratio: 0.5, provisional: provisional) | ||
| // The target at send time (see ``NewColumnCommand``): a stand-in has become the daemon's pane. | ||
| let target: @MainActor @Sendable () throws -> PaneID = { try daemon.store.daemonTarget(pane) } | ||
| let target: @MainActor @Sendable () async throws -> PaneID = { try await daemon.store.daemonTarget(pane) } |
There was a problem hiding this comment.
P3: daemonTarget can now throw PendingPaneNotCreated, but this command still converts it to PaneSplitFailure, falsely reporting a daemon refusal. Preserve and rethrow PendingPaneNotCreated like PaneSplitCommand so a failed parent create reports that the target pane was not created.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At Packages/macOS/CmuxNext/Sources/CmuxNextDaemonService/NewRowCommand.swift, line 36:
<comment>`daemonTarget` can now throw `PendingPaneNotCreated`, but this command still converts it to `PaneSplitFailure`, falsely reporting a daemon refusal. Preserve and rethrow `PendingPaneNotCreated` like `PaneSplitCommand` so a failed parent create reports that the target pane was not created.</comment>
<file context>
@@ -33,7 +33,7 @@ public struct NewRowCommand: Sendable {
let intent = Intent.splitPane(pane: pane, direction: .down, ratio: 0.5, provisional: provisional)
// The target at send time (see ``NewColumnCommand``): a stand-in has become the daemon's pane.
- let target: @MainActor @Sendable () throws -> PaneID = { try daemon.store.daemonTarget(pane) }
+ let target: @MainActor @Sendable () async throws -> PaneID = { try await daemon.store.daemonTarget(pane) }
let send = daemon.intendNowReporting("new-row", intent, transaction: transaction) { connection in
try await RowCommands(connection).newRow(below: try await target(), height: height, options: sent)
</file context>
| // daemon's pane by then (``DaemonStore/daemonHandle(showing:)``; creates of one workspace go | ||
| // out in order, each once the store holds the one before). | ||
| let target: @MainActor @Sendable () throws -> PaneID = { try daemon.store.daemonTarget(pane) } | ||
| let target: @MainActor @Sendable () async throws -> PaneID = { try await daemon.store.daemonTarget(pane) } |
There was a problem hiding this comment.
P3: daemonTarget can now throw PendingPaneNotCreated, but this command still converts it to PaneSplitFailure, falsely reporting a daemon refusal. Preserve and rethrow PendingPaneNotCreated like PaneSplitCommand so a failed parent create reports that the target pane was not created.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At Packages/macOS/CmuxNext/Sources/CmuxNextDaemonService/NewColumnCommand.swift, line 48:
<comment>`daemonTarget` can now throw `PendingPaneNotCreated`, but this command still converts it to `PaneSplitFailure`, falsely reporting a daemon refusal. Preserve and rethrow `PendingPaneNotCreated` like `PaneSplitCommand` so a failed parent create reports that the target pane was not created.</comment>
<file context>
@@ -45,7 +45,7 @@ public struct NewColumnCommand: Sendable {
// daemon's pane by then (``DaemonStore/daemonHandle(showing:)``; creates of one workspace go
// out in order, each once the store holds the one before).
- let target: @MainActor @Sendable () throws -> PaneID = { try daemon.store.daemonTarget(pane) }
+ let target: @MainActor @Sendable () async throws -> PaneID = { try await daemon.store.daemonTarget(pane) }
let send = daemon.intendNowReporting("new-pane-right", intent, transaction: transaction) { connection in
try await connection.newColumn(rightOf: try await target(), width: width, options: sent)
</file context>
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. |
Contributor
Author
|
Review (opus-review-30271): SHIP at 024eb02 Findings: none that block. What I checked (read-only, against #29530's squash 82a70ea and the dogfood base a516f14):
Notes (not blocking):
|
…ys (held, or sent to the Ctrl-` terminal at once) Green 06886ca01b09669e601a82d7 at 024eb02: CmdNThenNewTerminalTabTests:171 held 0 of 10, while the same test's checks after it passed (no failure, the hold ended .focusLanded, the focused tab is the new terminal). On this line (107f5da, 050651b, EarlyTerminalKeysTests) a key typed while a new terminal's early view has the keyboard is not held: CreationInputCoordinator.capture returns false and AppKit sends the key to that view, which sends it by the minted terminal id (or by the attachment once the daemon does not know the id). feat's coordinator holds it instead. The assertion now counts a key as the new terminal's when it is held, or when it is not dropped and the window's first responder is the Ctrl-` stand-in terminal (earlyDeliveryState: takesKeysEarly true). Any other key fails the test with its path (dropped flag, first responder, hold state). Test-only; product code unchanged.
…hows the typed line (end-result oracle) Lead 08:57Z: the pass condition is what the person sees, not an internal path. Each key of "echo held\r" goes through the app's dispatch (KeyRouter.interceptKeyDown, then the window for a key it does not consume, as CmuxApplication.sendEvent does). After the create settles, the new terminal's screen (read-screen) must show `echo held` and the shell's answer `held` (all ten keys, in order, Return included), within 30 s, and no other terminal's screen may show `echo held`. How each key went (consumed or to which responder, the hold count) is in the failure text only. Known limit: a key the hold keeps is replayed through NSApp.sendEvent, which a test (never the key window) does not deliver; such a key would fail this test and show as "consumed" in its paths. Test-only; product code unchanged.
Contributor
Author
|
Review (opus-review-30271-test): SHIP at 2afdba7 Findings: none that block. What I checked (read-only, whole diff a516f14..2afdba7, with extra care on 024eb02..2afdba7):
Notes (not blocking):
|
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Packages/macOS/CmuxNext/Tests/CmuxNextAppTests/CmdNThenNewTerminalTabTests.swift">
<violation number="1" location="Packages/macOS/CmuxNext/Tests/CmuxNextAppTests/CmdNThenNewTerminalTabTests.swift:211">
P2: This assertion treats a failed screen read as an empty screen, so the test can pass without checking whether another terminal received the command. Propagate the request error so the test fails instead of silently accepting an unverifiable result.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| // New Tab page's composer had none of them either once the new terminal shows all ten). | ||
| for other in store.panesByHandle.values.flatMap(\.tabs) where other.kind == .pty && other.surface != terminal | ||
| && !ProvisionalTab.isProvisional(surface: other.surface) { | ||
| let text = (try? await connection.request(ReadScreenRequest(surface: other.surface), timeout: .seconds(2)).text) ?? "" |
There was a problem hiding this comment.
P2: This assertion treats a failed screen read as an empty screen, so the test can pass without checking whether another terminal received the command. Propagate the request error so the test fails instead of silently accepting an unverifiable result.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At Packages/macOS/CmuxNext/Tests/CmuxNextAppTests/CmdNThenNewTerminalTabTests.swift, line 211:
<comment>This assertion treats a failed screen read as an empty screen, so the test can pass without checking whether another terminal received the command. Propagate the request error so the test fails instead of silently accepting an unverifiable result.</comment>
<file context>
@@ -199,6 +190,27 @@ struct CmdNThenNewTerminalTabTests {
+ // New Tab page's composer had none of them either once the new terminal shows all ten).
+ for other in store.panesByHandle.values.flatMap(\.tabs) where other.kind == .pty && other.surface != terminal
+ && !ProvisionalTab.isProvisional(surface: other.surface) {
+ let text = (try? await connection.request(ReadScreenRequest(surface: other.surface), timeout: .seconds(2)).text) ?? ""
+ #expect(!text.contains("echo held"), "terminal \(other.surface) also got the typed line: \(text.debugDescription)")
+ }
</file context>
Suggested change
| let text = (try? await connection.request(ReadScreenRequest(surface: other.surface), timeout: .seconds(2)).text) ?? "" | |
| let text = try await connection.request(ReadScreenRequest(surface: other.surface), timeout: .seconds(2)).text |
…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). (cherry picked from commit 6bf54eb)
…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.
There was a problem hiding this comment.
2 issues found across 7 files (changes from recent commits).
You’re at about 94% 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.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Packages/macOS/CmuxNext/Sources/CmuxNextApp/Focus/CreationInputCoordinator.swift">
<violation number="1" location="Packages/macOS/CmuxNext/Sources/CmuxNextApp/Focus/CreationInputCoordinator.swift:367">
P3: The new default for `replay` calls `NSApp.sendEvent` directly and ignores the `Sender` that `HeldKeyReplay` hands in. That breaks the seam's documented rule that a held key is sent only through the `Sender`. Current behavior is the same, but the send step is now outside the seam. Make the seam optional and fall back to `send.application(event)` so production keeps going through the `Sender`.</violation>
</file>
<file name="Packages/macOS/CmuxNext/Sources/CmuxNextDaemonService/DaemonService+Endpoint.swift">
<violation number="1" location="Packages/macOS/CmuxNext/Sources/CmuxNextDaemonService/DaemonService+Endpoint.swift:41">
P1: New Column and New Row still bypass this target-aware check: `createInKeysTurn` uses `daemon.queuesCreatingInput` without the source pane. Pass that handle through and call `queuesCreatingInput(on:)`, otherwise keys typed while a stand-in target is resolving are sent before the daemon knows the terminal.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| /// flight) waits for the daemon's pane before it is sent (`DaemonStore.daemonTarget`, #29530), so the daemon does not know | ||
| /// its terminal id yet and refuses keys sent by it: those keys wait in the creation hold for the daemon's terminal, as | ||
| /// on feat-cmux-next (#30271: after Cmd-N then Ctrl-`, the line typed into the stand-in never reached the terminal). | ||
| public func queuesCreatingInput(on target: PaneID) -> Bool { queuesCreatingInput && !ProvisionalPane.isStandIn(target) } |
There was a problem hiding this comment.
P1: New Column and New Row still bypass this target-aware check: createInKeysTurn uses daemon.queuesCreatingInput without the source pane. Pass that handle through and call queuesCreatingInput(on:), otherwise keys typed while a stand-in target is resolving are sent before the daemon knows the terminal.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At Packages/macOS/CmuxNext/Sources/CmuxNextDaemonService/DaemonService+Endpoint.swift, line 41:
<comment>New Column and New Row still bypass this target-aware check: `createInKeysTurn` uses `daemon.queuesCreatingInput` without the source pane. Pass that handle through and call `queuesCreatingInput(on:)`, otherwise keys typed while a stand-in target is resolving are sent before the daemon knows the terminal.</comment>
<file context>
@@ -34,6 +34,12 @@ extension DaemonService {
+ /// flight) waits for the daemon's pane before it is sent (`DaemonStore.daemonTarget`, #29530), so the daemon does not know
+ /// its terminal id yet and refuses keys sent by it: those keys wait in the creation hold for the daemon's terminal, as
+ /// on feat-cmux-next (#30271: after Cmd-N then Ctrl-`, the line typed into the stand-in never reached the terminal).
+ public func queuesCreatingInput(on target: PaneID) -> Bool { queuesCreatingInput && !ProvisionalPane.isStandIn(target) }
+
/// The input of a view whose terminal `terminalID` is still being created goes to this daemon
</file context>
| replayedCount += 1 | ||
| router.heldKeys.replay(event, heldBy: .creation, keyWindow: window, throughApplication: true) { $0.application(event) } | ||
| (replaying, replayedCount) = (true, replayedCount + 1) | ||
| router.heldKeys.replay(event, heldBy: .creation, keyWindow: window, throughApplication: true) { [replay] _ in replay(event, window, router) } |
There was a problem hiding this comment.
P3: The new default for replay calls NSApp.sendEvent directly and ignores the Sender that HeldKeyReplay hands in. That breaks the seam's documented rule that a held key is sent only through the Sender. Current behavior is the same, but the send step is now outside the seam. Make the seam optional and fall back to send.application(event) so production keeps going through the Sender.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At Packages/macOS/CmuxNext/Sources/CmuxNextApp/Focus/CreationInputCoordinator.swift, line 367:
<comment>The new default for `replay` calls `NSApp.sendEvent` directly and ignores the `Sender` that `HeldKeyReplay` hands in. That breaks the seam's documented rule that a held key is sent only through the `Sender`. Current behavior is the same, but the send step is now outside the seam. Make the seam optional and fall back to `send.application(event)` so production keeps going through the `Sender`.</comment>
<file context>
@@ -362,9 +363,8 @@ final class CreationInputCoordinator {
- replayedCount += 1
- router.heldKeys.replay(event, heldBy: .creation, keyWindow: window, throughApplication: true) { $0.application(event) }
+ (replaying, replayedCount) = (true, replayedCount + 1)
+ router.heldKeys.replay(event, heldBy: .creation, keyWindow: window, throughApplication: true) { [replay] _ in replay(event, window, router) }
replaying = false
}
</file context>
lawrencecchen
added a commit
that referenced
this pull request
Nov 1, 2026
…not the New Tab page (P0) (#30827) * test(app): keys typed after Cmd-N then Ctrl-` (or Cmd-D) must reach the 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. * fix(app): a creation pressed while a New Tab page opens takes the keys: 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). * test(app): Cmd-N's page hold keeps the keys while the page opens, so 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.
Contributor
Author
|
Status at head 28bcb1b (a7414ded41eeb0207): not ready, do not merge.
|
…nt, and a control line typed after the hold (#30271 diagnostics) Green 37f96e2b9465316565e74dc5 at 28bcb1b: the creation hold kept all 10 keys and ended .focusLanded, but the new terminal showed none of them. On failure the test now records, for each held key at its replay: whether the key router consumed it, the window's first responder and whether it is the focused pane's shown terminal view, the pane, its shown tab, the view's stand-in gate and the terminal id its input goes by; and a control: "echo ctl" typed through the same dispatch after the hold, with the screen it gives. The pass rule is unchanged.
…then records it (compile f615173a)
…/p0-ctrlbt-after-cmdn-dogfood # Conflicts: # Packages/macOS/CmuxNext/Tests/CmuxNextAppTests/CmdNThenNewTerminalTabTests.swift
lawrencecchen
changed the title
…rminal views can type Diagnostic 12f65c19335cf44d44de0317 at 537cb91: every replayed key went to the new terminal's own view (first responder, focus on its tab), and a control line typed after the hold did not show either. PasswordManagerRouteTests.withApp starts no Ghostty runtime on this line, so a terminal view has no surface and TerminalSurfaceView.keyDown sends nothing to the PTY. TestHosts.liveDaemonApp (NewTerminalOnPendingSplitTests, which types the same way) calls GhosttyRuntime.startedForTests(); CmdNThenCreateKeysTests and the #29530 key test now do too. Test-only. The earlier screen-oracle failures of these tests on this line (37f96e2b, 3f0893af) did not test the product.
…h Cmd-N key test (lead 17:3xZ) On feat-cmux-next the Ghostty runtime starts on its first use (product code); on this line the app starts it at launch, so PasswordManagerRouteTests.withApp, the harness these tests share, now starts it the way this line's TestHosts.liveDaemonApp does (GhosttyRuntime.startedForTests). The per-test calls of b3ac9cf go. Test-only.
… 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).
Contributor
Author
|
Review (opus-review-30271-net): SHIP at 16891d1 Findings: none in the code that block. What I checked (read-only, net diff
Notes (not blocking the code; do them before
|
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.
Summary
cp/dogfood-v2-2-12-opt. The opening part landed as P0 port: Ctrl-` right after Cmd-N opens its terminal (#29530, opening part) #31034 (merge 1f0153f). Net change against that tip:CreationInputCoordinator.beginends the window's New Tab page hold (NewTabInputCoordinator.creationTookKeys) once its own hold is open. The keys that hold kept replay, in typing order, into the creation hold. Before, after Cmd-N then Ctrl-` (or Cmd-D), the keys typed next waited for the New Tab page and went to its field: smoke round 2 step 16's typed line.CreationInputCoordinator.replay(feat'sHeldKeyReplay, P0: a line typed after a quick double Cmd-D can miss the new pane (rare race, after #28331) #28436). The default isNSApp.sendEvent, as before, so product behavior is unchanged.CmdNThenCreateKeysTests(fix(app): keys typed after Cmd-N then Ctrl-` reach the new terminal, not the New Tab page (P0) #30827's, for Ctrl-and Cmd-D; it records a diagnostic only when it fails) and #29530'skeysTypedWhileCtrlBacktickWaitsForCmdNAreHeldForTheNewTerminal`. Each requires the focused tab to be a new terminal, its screen to show the typed line and the shell's answer, and no other terminal to show the line.PasswordManagerRouteTests.withAppcallsGhosttyRuntime.startedForTests(). On this line the app starts the Ghostty runtime at launch; on feat it starts on first use. Without the call, the harness's terminal views have no surface and type nothing (diagnostic 12f65c19335cf44d44de0317). That made the earlier screen-oracle failures on this line (37f96e2b, 3f0893af) harness failures, not product failures.key itself held, empty trace): NOT proven fixed. These tests callnewSurfacedirectly; they do not press a Ctrl-key. Which hold kept that key (the New Tab hold or the creation hold, which holds chords while one of its creations is pending) is not known. pa-d's next judged rounds record it.Testing
CmdNThenCreateKeysTestsfailed 2/2 on the screen check (sh-3.2$; every key "New Tab hold true"). 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 key test passed there: it does not discriminate this fix.Observed / Not run
withAppsuites at the head (the harness now starts the runtime in all of them, as the app does); a real Ctrl-` key press (smoke rounds); New Column and New Row after Cmd-N.Changelog
No capture: key routing; the evidence is the display-worker screen oracle.
perf claim: none.