Repository navigation
Add canonical Browser layout relocation and fix resize replay - #9235
lawrencecchen wants to merge 25 commits into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds canonical layout columns, activation-aware creation and relocation APIs, placement metadata, and independent client selection. It updates protocol projections, specifications, frontend guidance, and tests. It also validates replay lengths in resized hosted frames and supports ordered splits. ChangesCanonical layout placement and activation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (24 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmux-tui/crates/cmux-tui-core/src/surface.rs`:
- Around line 3992-3995: Add regression cases alongside the existing valid VT
replay fixture in the relevant surface test: assert rejection for truncated
length-prefixed data, payloads with trailing bytes, and replay lengths exceeding
VT_REPLAY_MAX_BYTES. Reuse the existing replay parsing/validation entry point
and preserve the current valid-payload assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 77b9e525-ff6c-4e9b-91ae-4732319cd770
📒 Files selected for processing (1)
cmux-tui/crates/cmux-tui-core/src/surface.rs
lawrencecchen
changed the title
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmux-tui/crates/cmux-tui-core/src/server.rs (2)
3468-3518: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winNew Tab
urlfield is undocumented in the canonical schema.pane_jsonin server.rs now serializes aurlfield for every tab, but theTabschema in commands.md (the canonical schema referenced by protocol.md and frontends.md) does not list it.
cmux-tui/crates/cmux-tui-core/src/server.rs#L3468-L3518: this is the producer change; no action needed here beyond confirming the field is intentional and stable.cmux-tui/spec/commands.md#L107-L131: addurl:string|nullto theTabobject schema so frontends relying on this canonical reference know the field exists.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmux-tui/crates/cmux-tui-core/src/server.rs` around lines 3468 - 3518, Document the existing tab url field in cmux-tui/spec/commands.md at lines 107-131 by adding url:string|null to the canonical Tab schema; cmux-tui/crates/cmux-tui-core/src/server.rs lines 3468-3518 requires no direct change because pane_json already emits the intentional stable field.
3468-3518: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winNew
urlTab field is not reflected in the documentedTabschema.Line 3494 adds
"url": surface.and_then(|s| s.browser_url())to every tab entry. commands.md'sTabobject schema does not list aurlfield. See the consolidated comment for the required documentation update.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmux-tui/crates/cmux-tui-core/src/server.rs` around lines 3468 - 3518, The tab JSON produced by pane_json now includes a url field, so update the documented Tab object schema in commands.md to declare this field with its nullable browser URL type and semantics. Keep the documentation aligned with the existing browser-related tab fields and the runtime output from pane_json.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmux-tui/crates/cmux-tui-core/src/model.rs`:
- Around line 532-546: Add a `Node::Stack` case to
`ordered_split_places_a_new_leaf_on_the_requested_side` that invokes
`split_leaf_ordered` on a stack target and verifies the resulting split places
the new leaf on the requested side while preserving the stack node on the other
side.
In `@cmux-tui/crates/cmux-tui-core/src/mux.rs`:
- Around line 8396-8414: Update move_pane_to_new_column_with_activation so the
relocated column always applies the requested width, including when
detach_pane_layout_for_relocation returns an existing column; preserve the
existing width only when no requested override is applicable. Extend the
existing test around the 0.7-width case to assert that the resulting column
width is 0.7.
- Around line 5563-5591: Extract the duplicated viewport-width bounds check into
a free helper named validate_viewport_width returning Result<(),
ViewportWidthError>. Replace the inline validation in
new_pane_right_with_activation, new_browser_pane_right_with_activation,
move_tab_to_new_column_with_activation, move_pane_to_new_column_with_activation,
and set_viewport_pane_width_inner with calls to this helper, preserving the
existing error propagation and behavior.
- Around line 8886-8902: Update both root-removal branches in the surrounding
detach function so a failed remove_leaf does not leave Node::Leaf(0) installed.
In the layout-column branch and the screen-root branch, only assign the returned
root after removal succeeds, or restore the original root before returning None;
preserve the existing successful removal behavior and cleanup.
- Around line 4897-4901: Update the target selection logic around target_pane to
fail when an explicitly requested pane is no longer owned by workspace wi,
rather than falling back to active_screen_ref().active_pane. Preserve the
active-pane fallback only when no target_pane was requested, and return the
existing invalid-target/error result for a missing or moved requested pane.
- Around line 8325-8352: In the relocation block, validate the target pane and
its destination column before calling detach_pane_layout_for_relocation, using
the same layout branch later used for insertion. After this prevalidation, treat
a failed split_leaf_ordered result as an invariant violation rather than
returning a recoverable “target pane disappeared” error, so detachment cannot
leave the source unreachable; preserve the existing successful layout and zoom
updates.
In `@cmux-tui/crates/cmux-tui-core/src/server.rs`:
- Around line 5295-5318: The NewPaneRight and Split command handlers misreport a
supplied URL with kind "pty" as an invalid pane kind. Update both match flows to
handle this combination explicitly and return a user-facing error stating that
url is only valid with kind "browser", including a concrete correction such as
removing url or changing kind; preserve the existing PTY and browser creation
behavior for valid inputs.
- Around line 4969-4991: Enforce the documented pane requirement in the no-pane
branch of Command::NewBrowserTab: reject activate:false before calling
mux.new_browser_tab, analogous to the existing index.is_some() validation.
Preserve the current default activating behavior when activate is omitted or
true, and keep the pane-provided path unchanged.
In `@cmux-tui/spec/commands.md`:
- Around line 2622-2647: Escape or otherwise rewrite the raw pipe characters in
the dir parameter descriptions for move-tab-to-split and move-pane-to-split so
each Markdown table row retains exactly three columns. Follow the existing file
convention of describing the alternatives without an unescaped pipe, while
preserving the accepted right/down values.
---
Outside diff comments:
In `@cmux-tui/crates/cmux-tui-core/src/server.rs`:
- Around line 3468-3518: Document the existing tab url field in
cmux-tui/spec/commands.md at lines 107-131 by adding url:string|null to the
canonical Tab schema; cmux-tui/crates/cmux-tui-core/src/server.rs lines
3468-3518 requires no direct change because pane_json already emits the
intentional stable field.
- Around line 3468-3518: The tab JSON produced by pane_json now includes a url
field, so update the documented Tab object schema in commands.md to declare this
field with its nullable browser URL type and semantics. Keep the documentation
aligned with the existing browser-related tab fields and the runtime output from
pane_json.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: db66d518-811c-4b20-b29e-2b6d4f7886b4
📒 Files selected for processing (6)
cmux-tui/crates/cmux-tui-core/src/model.rscmux-tui/crates/cmux-tui-core/src/mux.rscmux-tui/crates/cmux-tui-core/src/server.rscmux-tui/docs/protocol.mdcmux-tui/spec/commands.mdcmux-tui/spec/frontends.md
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmux-tui/crates/cmux-tui-core/src/server.rs (1)
5462-5489: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
move-tabreports success even when the underlying move fails.
mux.move_tab_with_activationreturns abool(moved or not), but the return value is discarded at Line 5471. If the targetpanedisappears between the earliervalidcheck and the actual move (a real race, sincevalidis checked without holding the workspace lock, while the move itself re-validates under a lifecycle lock),move_tab_with_activationreturnsfalseand nothing moves. The handler still looks upstate.pane_of(surface)afterward and returns that as a successful placement, so the response reports"ok":truewith the tab's original pane instead of an error, even though the requested move topanenever happened.Distinguishing this from the legitimate same-position no-op (which also returns
falsebut leaves the surface already at the requestedpane) is possible by comparing the resolved placement's pane against the requestedpanerather than by checking the raw boolean.🐛 Proposed fix
mux.move_tab_with_activation(surface, pane, index, activate.unwrap_or(true)); let placement = mux .with_state(|state| { let pane = state.pane_of(surface)?; let (workspace_index, screen_index) = state.screen_of(pane)?; Some(( pane, state.workspaces[workspace_index].screens[screen_index].id, state.workspaces[workspace_index].id, )) }) .ok_or_else(|| anyhow::anyhow!("moved surface has no placement"))?; + if placement.0 != pane { + anyhow::bail!("unknown surface/pane"); + } Ok(json!({🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmux-tui/crates/cmux-tui-core/src/server.rs` around lines 5462 - 5489, Update the Command::MoveTab handler to verify the resolved placement after mux.move_tab_with_activation against the requested pane. Return an error when placement.0 differs from pane, while preserving success for same-position no-ops where the surface is already in the requested pane; do not rely solely on the discarded boolean result.
♻️ Duplicate comments (1)
cmux-tui/crates/cmux-tui-core/src/mux.rs (1)
5588-5608: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated viewport-width bounds check into one helper.
new_pane_right_with_activationandnew_browser_pane_right_with_activationboth inline the same!width.is_finite() || !(MIN_VIEWPORT_PANE_WIDTH..=MAX_VIEWPORT_PANE_WIDTH).contains(&width)check. The same check also appears inset_viewport_pane_width_inner(Line 7086),move_tab_to_new_column_with_activation(Line 8167), andmove_pane_to_new_column_with_activation(Line 8404). Five copies of one bound rule will diverge over time.This finding is unchanged from the prior review round.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmux-tui/crates/cmux-tui-core/src/mux.rs` around lines 5588 - 5608, Extract the repeated viewport-width validation into a shared helper near the existing viewport-width logic, returning the established ViewportWidthError for non-finite or out-of-range values. Replace the inline checks in new_pane_right_with_activation, new_browser_pane_right_with_activation, set_viewport_pane_width_inner, move_tab_to_new_column_with_activation, and move_pane_to_new_column_with_activation with calls to that helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@cmux-tui/crates/cmux-tui-core/src/server.rs`:
- Around line 5462-5489: Update the Command::MoveTab handler to verify the
resolved placement after mux.move_tab_with_activation against the requested
pane. Return an error when placement.0 differs from pane, while preserving
success for same-position no-ops where the surface is already in the requested
pane; do not rely solely on the discarded boolean result.
---
Duplicate comments:
In `@cmux-tui/crates/cmux-tui-core/src/mux.rs`:
- Around line 5588-5608: Extract the repeated viewport-width validation into a
shared helper near the existing viewport-width logic, returning the established
ViewportWidthError for non-finite or out-of-range values. Replace the inline
checks in new_pane_right_with_activation,
new_browser_pane_right_with_activation, set_viewport_pane_width_inner,
move_tab_to_new_column_with_activation, and
move_pane_to_new_column_with_activation with calls to that helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0ddf38b2-73ed-42f1-840b-e0375301b5fe
📒 Files selected for processing (5)
cmux-tui/crates/cmux-tui-core/src/mux.rscmux-tui/crates/cmux-tui-core/src/server.rscmux-tui/docs/protocol.mdcmux-tui/spec/commands.mdcmux-tui/spec/frontends.md
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmux-tui/crates/cmux-tui-core/src/mux.rs (2)
4969-4994: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winGate
stamp_pane_focuswithactivatewhen materializing the first screen.In this branch of
create_terminal_in_workspace_impl, when the target workspace has no active pane (first screen materialization),stamp_pane_focus(self, &mut state, pane_id)at line 4973 runs unconditionally. This ignores theactivateparameter.Compare this to
new_screen_with_cwd(lines 4472-4480), where the identical call is correctly gated:if activate { stamp_pane_focus(self, &mut state, pane_id); }. Here, anactivate:falserequest still steals the daemon's globalactive_workspace/active_paneand stamps focus/focused_at, even thoughattached.2(selection_resync) is later set tofalsefor this branch, hiding the state mutation from the emitted event while still corrupting the underlying selection state.A detached frontend creating a terminal with
activate:falsein a previously empty workspace will unexpectedly steal the owner TUI's active workspace/pane selection, contradicting this PR's stated goal ("Allows detached frontends to create workspaces without activating them").🐛 Proposed fix
} else { let (pane_id, pane) = self.make_pane(surface.id); let screen_id = self.next_id(); state.insert_pane(pane); - stamp_pane_focus(self, &mut state, pane_id); + if activate { + stamp_pane_focus(self, &mut state, pane_id); + } state.workspaces[wi].screens.push(Screen {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmux-tui/crates/cmux-tui-core/src/mux.rs` around lines 4969 - 4994, In the first-screen materialization branch of create_terminal_in_workspace_impl, gate stamp_pane_focus(self, &mut state, pane_id) on the activate parameter, matching new_screen_with_cwd. Preserve the existing pane and screen creation flow while ensuring activate:false does not modify global workspace or pane focus state.
8052-8058: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSelect the projection target with
activate:true.
project_terminal_to_workspace_at_in_statemoves the terminal into the destination, but thenrestore_focus_identity(previous_focus)restores the original workspace/screen/pane because onlyfocus.surfacebecomesNone. Foractivate:true, update active focus to the destination, for example by stamping the destination pane or applying the same active-workspace/screen/pane target logic used by the other activation paths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmux-tui/crates/cmux-tui-core/src/mux.rs` around lines 8052 - 8058, Update the activate branch in project_terminal_to_workspace_at_in_state so preserved_focus targets the destination workspace, screen, and pane rather than only clearing focus.surface. Reuse the existing destination-focus or activation logic used by other activate paths, then pass that adjusted focus to restore_focus_identity; preserve the current behavior when activate is false.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@cmux-tui/crates/cmux-tui-core/src/mux.rs`:
- Around line 4969-4994: In the first-screen materialization branch of
create_terminal_in_workspace_impl, gate stamp_pane_focus(self, &mut state,
pane_id) on the activate parameter, matching new_screen_with_cwd. Preserve the
existing pane and screen creation flow while ensuring activate:false does not
modify global workspace or pane focus state.
- Around line 8052-8058: Update the activate branch in
project_terminal_to_workspace_at_in_state so preserved_focus targets the
destination workspace, screen, and pane rather than only clearing focus.surface.
Reuse the existing destination-focus or activation logic used by other activate
paths, then pass that adjusted focus to restore_focus_identity; preserve the
current behavior when activate is false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 46f6f01d-7d64-4020-9a89-b7fe7367ebf5
📒 Files selected for processing (3)
cmux-tui/crates/cmux-tui-core/src/mux.rscmux-tui/docs/protocol.mdcmux-tui/spec/frontends.md
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f46686b896
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…st-resize-replay # Conflicts: # cmux-tui/crates/cmux-tui-core/src/mux.rs # cmux-tui/crates/cmux-tui-core/src/server.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3604b88df1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
lawrencecchen
added
the
stale-revisit
2 participants
Summary
This is the cmux half of the canonical cmux-browser integration:
Resizedframes ascols:u16 + rows:u16 + replay_len:u32 + replay, rejecting truncated,oversized, and trailing data
mixed browser/PTY tabs, browser URLs, and stable backend IDs
new columns
activate:falseearlier Screen is removed or a terminal host is adopted
canonical-layout-columns-v1,canonical-layout-relocation-v1, andindependent-client-selection-v1The resize decoder fixes attached TUIs showing a stray leading
dand losingthe prompt's final space after a browser-driven resize. The old decoder fed
the replay-length prefix into Ghostty as terminal output.
State contract
cmux owns durable shared structure: workspaces, screens, niri columns and
widths, split ratios, stack membership, panes, surface identity/kind, tab
order, browser URLs, and PTY state.
Each frontend owns presentation and selection: active workspace/screen/pane/
tab, stack expansion, viewport/scroll, terminal grid size, and keyboard focus.
Non-focus structural operations preserve the complete owner-TUI focus tuple.
Closing or reordering an unrelated sibling preserves that tuple by stable ID;
only deleting the selected object may choose a fallback.
Exact revisions
1239ef8a97328c6fb9a0fe16647746bfe031cf0dcmux-tui/tree:f3504ca71c96c08f589a7c0104944d28e4c2ffe7main:541fe7f0c7d2dfa641de9c1fe1ad44f922083d1babcf5697d4fcd05e29a83ccfc090d6e234952849Regression proof
The resize regression and fix remain split:
bf714a2930makes the consumer test use the real length-prefixed payloadand fails before the decoder fix.
de8ac6fef7fixes decoding and makes it pass.26651d19f9covers truncated, trailing, and oversized frames.Canonical relocation tests assert exact IDs and placement results while the
owner's
{workspace, screen, pane, surface}focus tuple stays unchanged.Dedicated tests cover detached workspace creation and deleting an earlier
Screen without changing that tuple.
Testing
cargo fmt --all -- --check: passedcargo clippy -p cmux-tui-core --all-targets --locked -- -D warnings:passed with Zig 0.16
selection tests passed; 539/543 core tests passed
passed immediately in isolation, and the remaining two reproduce from the
pre-feature checkout whose
browser.rsis byte-identicalThe final packaged Browser/TUI screenshot and interaction matrix is tracked in
cmux-browser#35.
Review trigger
Checklist
mainis integrated