Repository navigation
fix(cmux-tui): a terminal tab aimed at a pinned dock that takes no tabs opens in a new column right of it (FC-1 follow-up) - #30886
lawrencecchen wants to merge 23 commits into
Conversation
…pen in a new column right of it (FC-1 follow-up) cmux-tui refused every terminal tab in a pinned column with allow_tabs off (dock-column-pinned), so every creator that does not go through TabLifecycle.newTerminal failed in Home's dock before its allow_tabs: config commands, login openers, Duplicate Tab, Tab Group's New Tab, agent handlers, Agent Teams, the CLI and MCP. Rust: new-tab and tab.create_terminal into the dock, alone and beside a column. App: PaneController.newTerminalTab on Home's closed dock. Red on the base.
…bs opens in a new column right of it (FC-1 follow-up) cmux-tui refused every terminal tab in a pinned column with allow_tabs off (dock-column-pinned). Home's dock is pinned before it takes tabs, so the first terminal a script, the CLI, MCP, a config command, a login opener, Duplicate Tab, Tab Group's New Tab, an agent handler or Agent Teams made there failed. Every terminal tab commits through commit_resource_topology_operation (JSON-lines new-tab, tab.create_terminal, the frontend creation); there a tab.create_terminal whose pane is in such a column becomes one pane.split right of the dock with a viewport width (the lone-dock neighbor width beside a lone dock, as a link Home opens, else the default column width) and keeps the focus where it was unless the caller asked. No client needs its own guard. The two wire tests that asserted the old refusal for terminal tabs now assert that none enters the dock. Regression tests: pinned_columns_tests a_terminal_tab_aimed_at_a_pinned_dock_that_takes_no_tabs_opens_in_a_new_column_right_of_it and HomeClosedDockTerminalCreatorsTests (red at the commit before).
…s_column (clippy unused_qualifications)
GitHub Actions
Inactive
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 5 files
You’re at about 92% 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="cmux-tui/crates/cmux-tui-core/src/mux/resource_topology/topology_operation.rs">
<violation number="1" location="cmux-tui/crates/cmux-tui-core/src/mux/resource_topology/topology_operation.rs:553">
P1: Workspace- or screen-scoped terminal creation still fails when its active pane is the closed pinned dock. Resolve the effective active pane for selectors without an explicit `pane` before deciding whether to convert this operation to `pane.split`, so those API forms receive the same FC-1 behavior.</violation>
</file>
<file name="Packages/macOS/CmuxNext/Tests/CmuxNextAppTests/HomeClosedDockTerminalCreatorsTests.swift">
<violation number="1" location="Packages/macOS/CmuxNext/Tests/CmuxNextAppTests/HomeClosedDockTerminalCreatorsTests.swift:42">
P2: This assertion does not prove that the terminal landed in a new column to the dock’s right. A same-column split would pass all current checks; assert the column count and that the dock and terminal occupy distinct, ordered columns.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| /// The new column's width when `selectors` name a pane in a pinned column that takes no | ||
| /// tabs ([`terminal_column_beside_closed_dock`]), else `None`. | ||
| fn terminal_column_beside_closed_dock(&self, selectors: &ResourceSelectors) -> Option<f32> { | ||
| selectors.pane.as_ref()?; |
There was a problem hiding this comment.
P1: Workspace- or screen-scoped terminal creation still fails when its active pane is the closed pinned dock. Resolve the effective active pane for selectors without an explicit pane before deciding whether to convert this operation to pane.split, so those API forms receive the same FC-1 behavior.
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 cmux-tui/crates/cmux-tui-core/src/mux/resource_topology/topology_operation.rs, line 553:
<comment>Workspace- or screen-scoped terminal creation still fails when its active pane is the closed pinned dock. Resolve the effective active pane for selectors without an explicit `pane` before deciding whether to convert this operation to `pane.split`, so those API forms receive the same FC-1 behavior.</comment>
<file context>
@@ -530,6 +547,17 @@ impl Mux {
+ /// The new column's width when `selectors` name a pane in a pinned column that takes no
+ /// tabs ([`terminal_column_beside_closed_dock`]), else `None`.
+ fn terminal_column_beside_closed_dock(&self, selectors: &ResourceSelectors) -> Option<f32> {
+ selectors.pane.as_ref()?;
+ let pane = self.resolve_resource_path(ResourceTarget::Pane, selectors).ok()?.pane?;
+ self.with_state(|state| {
</file context>
| #expect(failures.isEmpty, "the terminal made in Home's closed dock failed: \(failures)") | ||
| try await PasswordManagerRouteTests.until({ | ||
| let panes = HomeSplitKeysTests.panes(home) | ||
| return panes.count == 2 && panes.first === dock && dock.tabs.count == 1 |
There was a problem hiding this comment.
P2: This assertion does not prove that the terminal landed in a new column to the dock’s right. A same-column split would pass all current checks; assert the column count and that the dock and terminal occupy distinct, ordered columns.
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/HomeClosedDockTerminalCreatorsTests.swift, line 42:
<comment>This assertion does not prove that the terminal landed in a new column to the dock’s right. A same-column split would pass all current checks; assert the column count and that the dock and terminal occupy distinct, ordered columns.</comment>
<file context>
@@ -0,0 +1,49 @@
+ #expect(failures.isEmpty, "the terminal made in Home's closed dock failed: \(failures)")
+ try await PasswordManagerRouteTests.until({
+ let panes = HomeSplitKeysTests.panes(home)
+ return panes.count == 2 && panes.first === dock && dock.tabs.count == 1
+ && panes.last?.tabs.contains { $0.kind == .pty && !ProvisionalTab.isProvisional(surface: $0.surface) } == true
+ }, report: {
</file context>
This comment has been minimized.
This comment has been minimized.
|
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)
web and size checks: green |
|
Review (subagent, correctness first): FIX at 4e49f38 I read the code only. I did not build or run anything on the review machine. Defects
Suggested fix (removes all three)Decide the reroute where the creation's intent is built, not at the commit entry:
Then:
Checked and correct
Evidence
perf claim: none |
…replay a retry, and cover pane.run and the receipted creation (review of #30886) The review found that the commit-point reroute misses the receipted create-surface-with-receipt path (the TUI, the SDKs) and pane.run, drops a tab.create_terminal name, and turns a keyed retry into a creation_conflict (the receipt stored pane.split, the fingerprint tab.create_terminal). The wire test now covers all four. Red at this commit (the old fix).
…intent, under the locks (review of #30886) The reroute moves from commit_resource_topology_operation (which the receipted creation path and pane.run bypass, and which changed the stored operation) into resource_topology_effect_intent: for a tab.create_terminal or pane.run whose resolved pane is in a pinned column that takes no tabs, the intent records beside_closed_dock {viewport_width} with pane and split reservations, under the state lock. effect_add_terminal_tab then opens the terminal with effect_add_pane (right, that width, focus false) and keeps its name and exit policy (PaneAddOptions gains both). The operation and its fingerprint stay the caller's, so a keyed retry replays; every creator gets it: JSON-lines new-tab, tab.create_terminal, create-surface-with-receipt and pane.run. The browser test's doc comment is back above its test.
GitHub Actions
Inactive
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
You’re at about 93% 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="cmux-tui/crates/cmux-tui-core/src/server/pinned_columns_tests.rs">
<violation number="1" location="cmux-tui/crates/cmux-tui-core/src/server/pinned_columns_tests.rs:467">
P2: The new `pane.run` and receipted-creation checks do not verify the required placement. Capture each returned surface/pane and assert its column is `Some(1)`; otherwise a regression that creates both terminals in another column can still satisfy the final `columns.len() == 5` assertion.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| "receipt": "fc1-receipted-new-tab", "pane": panes[0], | ||
| })); | ||
| assert_eq!(wire.tabs(panes[0]), vec![home], "the dock keeps only its pinned tab: {receipted}"); | ||
| let (columns, _) = columns_and_column_of(&wire, beside); |
There was a problem hiding this comment.
P2: The new pane.run and receipted-creation checks do not verify the required placement. Capture each returned surface/pane and assert its column is Some(1); otherwise a regression that creates both terminals in another column can still satisfy the final columns.len() == 5 assertion.
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 cmux-tui/crates/cmux-tui-core/src/server/pinned_columns_tests.rs, line 467:
<comment>The new `pane.run` and receipted-creation checks do not verify the required placement. Capture each returned surface/pane and assert its column is `Some(1)`; otherwise a regression that creates both terminals in another column can still satisfy the final `columns.len() == 5` assertion.</comment>
<file context>
@@ -432,27 +427,60 @@ fn a_terminal_tab_aimed_at_a_pinned_dock_that_takes_no_tabs_opens_in_a_new_colum
+ "receipt": "fc1-receipted-new-tab", "pane": panes[0],
+ }));
+ assert_eq!(wire.tabs(panes[0]), vec![home], "the dock keeps only its pinned tab: {receipted}");
+ let (columns, _) = columns_and_column_of(&wire, beside);
+ assert_eq!(columns.len(), 5, "each made its own column: {columns:?}");
}
</file context>
…neTerminalSpawn (clippy too_many_arguments)
GitHub Actions
Error
…ipts-v1 before its receipted creation, as the TUI and SDK clients do
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
|
Review (codex gpt-6-astra high, next-review): SHIP at 895a703 SHIP at 895a703 Visual verdict: no images; Leo tries the build.
Follow-ups:
|
|
Review (next-review patch-id; carried): SHIP at bb9f876 The independent SHIP at 895a703 carries to this head: the PR's own change ( |
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 (next-review patch-id; carried): SHIP at 370ba16 The independent SHIP at 895a703 carries to this head: the PR's own change ( |
|
Review (next-review patch-id; carried): SHIP at 27fa49c The independent SHIP at 895a703 carries to this head: the PR's own change ( |
|
Review (next-review patch-id; carried): SHIP at 245ef96 The independent SHIP at 895a703 carries to this head: the PR's own change ( |
|
Review (next-review patch-id; carried): SHIP at 55dd725 The independent SHIP at 895a703 carries to this head: the PR's own change ( |
This comment has been minimized.
This comment has been minimized.
|
Review (next-review patch-id; carried): SHIP at c6dd7a9 The independent SHIP at 895a703 carries to this head: the PR's own change ( |
|
Review (next-review patch-id; carried): SHIP at 6c45be6 The independent SHIP at 895a703 carries to this head: the PR's own change ( |
|
Review (next-review patch-id; carried): SHIP at 25fa715 The independent SHIP at 895a703 carries to this head: the PR's own change ( |
Summary
cmux-tui now opens a terminal tab aimed at a pinned dock that takes no tabs in a new column right of the dock. Before, it refused the tab with
dock-column-pinned. The rule lives in one daemon commit point, so every terminal creator gets it: the app's own creators, the CLI, MCP and scripts.FC-1 (fuzz catalog). Home's dock is pinned in its dock move, and it takes other tabs only after a later
column.update {allow_tabs: true}. A person can also close it to tabs. Whileallow_tabsis off, cmux-tui refused every terminal tab in it. #30676 fixes this forTabLifecycle.newTerminal(New Terminal Tab from a script, the CLI or MCP). The independent review of #30676 found that many other creators still failed in the dock: config commands (ConfigCommandRunner), the account and provider login openers, Duplicate Tab (TabHandlers:80), the page openers, Tab Group's New Tab, agent handlers and Agent Teams (rawconnection.newTab).Fix (per cmux-shared-behavior: one shared point, not a guard per caller). Every terminal tab commits through
Mux::commit_resource_topology_operation(cmux-tui/crates/cmux-tui-core/src/mux/resource_topology/topology_operation.rs): the JSON-linesnew-tab,tab.create_terminal, and the frontend's creation. There, atab.create_terminalwhose pane is in a pinned column that takes no tabs becomes onepane.splitto the right with a viewport width, which makes a new column beside the dock:LONE_DOCK_NEIGHBOR_WIDTH, the same width as a link Home opens there (the daemon's browser rule for a lone dock).DEFAULT_VIEWPORT_PANE_WIDTH.New helpers in
pinned_columns.rs:pane_in_pinned_column_taking_no_tabs(now also used byensure_pane_takes_tabs) andterminal_column_beside_closed_dock.Behavior change: two wire tests asserted the old refusal for terminal tabs (
pinned_column_takes_no_new_tab_until_allow_tabs_is_on,a_browser_tab_made_in_a_lone_pinned_dock_opens_in_a_new_column_beside_it). They now assert that no tab enters the dock. Browser and conversation tabs are unchanged: in a dock with a column beside it they are still refused, and from a lone dock the daemon already reroutes them.Related: #30503 (landed), #30676, #30826.
Owner note: this touches the cmux-tui daemon area of PL-daemon (session cmux-b6;
plans/cmux-next/OWNERS.md). Only the creation route changes; no request-speed path changes.Changelog
Testing
Regression tests (red at 34883fd, tests only):
server::pinned_columns_tests::a_terminal_tab_aimed_at_a_pinned_dock_that_takes_no_tabs_opens_in_a_new_column_right_of_it. It sendsnew-tabinto a lone dock (expects a new column right of the dock at the lone-dock width), thentab.create_terminalinto the dock with a column beside it (expects a third column, right of the dock). The dock keeps only its pinned tab.HomeClosedDockTerminalCreatorsTests. It callsPaneController.newTerminalTab(the config-command and login-opener path) on Home's closed dock, and expects no failure and a real terminal in a column right of the dock.Fleet:
{"cmd":"new-tab","pane":3} failed: {"error":"dock-column-pinned: the pinned column takes no other tab (allow_tabs is off)" ...}.pinned_columns_testspassed.HomeClosedDockTerminalCreatorsTests, plusHomeDockOpensTestsin the green): running when this was written; results are added here.unused_qualificationsattopology_operation.rs:601, fixed at 4e49f38.Base clippy reds (not this PR's)
crates/cmux-tui-cloud-conversations/src/environments.rs:89"this boolean expression can be simplified": from Cloud: add repository picker and recovery UI #30357. Its revert Revert "Cloud: add repository picker and recovery UI" #30791 (ready-to-land) removes it.crates/acpmux/src/prompt_provenance.rs:584"this boolean expression can be simplified": from acpmux: add authenticated Cloud trigger chat delivery #30526. The panic agent's base red: panic ratchet and clippy, #30526's unreachable!, index reads and lints, #30574's redundant clone #30853 fixes it.crates/cmux-tui/src/link/start_chat_cli.rs:282"redundant clone": from Cloud start-chat: a personal send from a repository folder gets the platform's set-up workspace machine #30574. The panic agent's base red: panic ratchet and clippy, #30526's unreachable!, index reads and lints, #30574's redundant clone #30853 fixes it.cmux-tui-core/src/mux/resource_topology/topology_operation.rs:601as a base red (from ebce631). On this branch that line was this PR's own new code, and 4e49f38 fixed it.Observed / Not run
prepush-gate.pywith no new violations.pinned_columns_tests), a first-5s bench, a dogfood build. I read, but did not run, the frontendnew-tabcreation path throughcmd_frontend.rs; it commits through the same function.No capture: the visible result is a terminal in a new column right of Home, as for a person's new terminal from Home.
perf claim: none