Skip to content

Add horizontally scrolling columns to cmux-tui - #8850

Merged
lawrencecchen merged 78 commits into
mainfrom
feat-cmux-tui-horizontal-scroll
Jul 28, 2026
Merged

lawrencecchen merged 78 commits into
mainfrom
feat-cmux-tui-horizontal-scroll

Conversation

@lawrencecchen

@lawrencecchen lawrencecchen commented Jul 24, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Keep ordinary startup, tiled layout, protocol fields, and whole-screen Alt-n behavior unchanged until a horizontal column is created.
  • Make Ctrl-b g insert a stable terminal column immediately after the focused column at two thirds of the viewport width.
  • Apply Zellij automatic layout with Alt-n only inside the focused horizontal column.
  • Resize the focused column with Alt-= and Alt--, or drag a visible column edge. Keep splits inside each column independently resizable.
  • Show a continuous horizontal status-bar track only on overflow. Animate focus and track movement over 180 ms, with {"viewport":{"animation":false}} for immediate movement.
  • Render each pane's tab bar on the logical canvas before clipping so tab chrome cannot become unusably half-visible.
  • Add screen-local structural undo on Ctrl-b U. Resize samples coalesce, non-destructive actions restore immediately, and pane-creation undo requires exact CONFIRM.
  • Fence destructive undo with the previewed layout revision, cap history at 32 in-memory entries, and clear history after direct pane closure because a closed process cannot be recreated.
  • Add undo-layout behind layout-undo-v1 and keep set-viewport-pane-width behind viewport-column-resize-v1.
  • Fail closed on stale confirmations, unsupported servers, invalid widths, renderer panics, poisoned render locks, host-terminal disconnects, and terminal setup/input/render/thread errors.
  • Localize new TUI status and confirmation text in English and Japanese.

Verification

  • cargo +1.97.1 fmt --all -- --check
  • ZIG=/opt/homebrew/Cellar/zig/0.15.2_1/bin/zig cargo +1.97.1 clippy --workspace --all-targets -- -D warnings
  • ZIG=/opt/homebrew/Cellar/zig/0.15.2_1/bin/zig cargo +1.97.1 test --workspace -- --test-threads=1
  • ZIG=/opt/homebrew/Cellar/zig/0.15.2_1/bin/zig cargo +1.97.1 build -p cmux-tui
  • Live PTY preflight of the exact PR binary in ephemeral session niri8856: unchanged startup, Ctrl-b g, animated reveal, per-column Alt-n, keyboard resize, pointer drag, immediate resize undo, confirmed pane-creation undo, ordinary-layout collapse, clean detach, and session cleanup.
  • Integration coverage closes a real host PTY, requires the frontend to exit within five seconds, and verifies the session server still answers ping.

Summary by CodeRabbit

  • New Features
    • Added horizontally scrollable “viewport panes” (Ctrl-b g) with a two-thirds-width right-append behavior for focused horizontal columns, plus animated horizontal navigation, a clickable/dragable horizontal scrollbar, and horizontal mouse-wheel panning.
    • Added configurable viewport animation (viewport.animation) and horizontal-column viewport-width resizing, plus structural layout undo via Ctrl-b U with confirmation when closing panes.
    • Added CLI/protocol support for new-pane-right, set-viewport-pane-width, and undo-layout (with capability gating).
  • Bug Fixes
    • Improved clipped-pane rendering and hit-detection (tabs, terminal input, mouse/selection) and sanitized control characters in displayed text.
  • Documentation
    • Updated keyboard, mouse, config, CLI, concepts, and protocol docs to reflect the new viewport/undo behavior and settings.

@coderabbitai

coderabbitai Bot commented Jul 24, 2026 •

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds viewport-width pane creation through Ctrl-b g, extended split-tree geometry, horizontally clipped rendering, animated scrollbar navigation, layout undo, protocol and CLI support, configuration, tests, and documentation.

Changes

Viewport pane layout

Layer / File(s) Summary
Viewport layout, columns, and undo
cmux-tui/crates/cmux-tui-core/src/layout.rs, model.rs, mux.rs
Adds viewport split metadata, virtual-width layout traversal, column projection, pane creation, viewport resizing, structural undo, topology cleanup, and core tests.
Viewport protocol and command integration
cmux-tui/crates/cmux-tui-core/src/server.rs, cmux-tui/crates/cmux-tui/src/session/*, cli.rs, tests/cli.rs
Adds capability-gated viewport commands, serializes and parses viewport metadata, wires local and remote sessions, validates CLI input, and exercises end-to-end behavior.
Viewport clipping and rendering
cmux-tui/crates/cmux-tui/src/app.rs, ui/*
Adds logical clipped pane geometry, cropped terminal and graphic rendering, animated viewport state, scrollbar geometry, edge-aware pane drawing, and rendering tests.
Viewport interaction and input mapping
cmux-tui/crates/cmux-tui/src/app.rs
Handles scrollbar clicks and drags, horizontal wheel movement, logical PTY and browser coordinates, selection mapping, resizing, focus navigation, and interaction tests.
Viewport configuration and documentation
cmux-tui/crates/cmux-tui/src/config.rs, main.rs, localization.rs, README.md, docs/*, spec/*
Adds viewport animation configuration, default actions, help text, localization, user documentation, and protocol and CLI contracts.

Estimated code review effort: 5 (Critical) | ~100 minutes

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant App
  participant Session
  participant Mux
  participant LayoutEngine
  participant Renderer
  User->>App: Press Ctrl-b g
  App->>Session: Create right viewport pane
  Session->>Mux: new_pane_right
  Mux->>LayoutEngine: Store viewport split and compute virtual width
  LayoutEngine-->>Mux: Viewport pane geometry
  Mux-->>Session: Created surface
  Session-->>App: Update screen state
  App->>LayoutEngine: Compute clipped viewport layout
  LayoutEngine-->>App: Logical pane placements
  App->>Renderer: Draw cropped panes and scrollbar
  Renderer-->>User: Render viewport and horizontal track
Loading

Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (2 errors, 2 warnings)

Check name Status Explanation Resolution
Cmux Algorithmic Complexity ❌ Error viewport_column_rects scans panes and linearly searches columns per pane, making resize-drag O(n^2) on user-grown panes/columns. Index columns by owner with a HashMap or build a one-pass grouped list so each pane doesn't rescan the current columns.
Cmux User-Facing Error Privacy ❌ Error New CLI capability probing forwards identify/server error strings to users, which can leak raw upstream/internal details instead of a sanitized message. Replace passthrough identify/server error text with generic cmux-facing messages and keep details in logs/telemetry only.
Description check ⚠️ Warning It covers Summary and Testing, but omits the required Demo Video, Review Trigger, and Checklist sections from the template. Add the missing sections, especially a demo video link for the UI change, the review trigger block, and the checklist items.
Docstring Coverage ⚠️ Warning Docstring coverage is 53.09% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (21 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: adding horizontally scrolling columns to cmux-tui.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Cmux Swift Actor Isolation ✅ Passed No Swift files changed relative to origin/main; this PR is Rust/docs-only, so Swift actor-isolation rules aren’t implicated.
Cmux Swift Blocking Runtime ✅ Passed No Swift files changed; the only sleep calls found are in Rust UI/retry code, so the Swift blocking-runtime rule is not applicable.
Cmux Browser Automation Off-Main ✅ Passed No browser.* command or routing changes appear in the PR diff; the edits are layout/viewport and docs only, so the off-main WebKit rule isn’t implicated.
Cmux Expensive Synchronous Load ✅ Passed PR touches only Rust/docs/Python/YAML files; no Swift diffs or main-actor/agent-history load paths are present, so the Swift sync-load rule is inapplicable.
Cmux Cache Substitution Correctness ✅ Passed PR changes are Rust/docs only, so the Swift/TypeScript/JavaScript cache-substitution rule is not applicable.
Cmux No Hacky Sleeps ✅ Passed Only new fixed delay is a 0.05s poll in dist/scripts/test_linux_packages.py, which is test-only scaffolding; no production runtime sleep/poll was added.
Cmux Swift Concurrency ✅ Passed No Swift files were modified in the PR; all changed files are Rust, docs, scripts, or config, so the Swift concurrency rule is not implicated.
Cmux Swift @Concurrent ✅ Passed PR diff touches no .swift files, so the Swift concurrent-annotation rule is not applicable.
Cmux Swift Package Boundaries ✅ Passed PASS: the PR diff shows no Swift files or SwiftPM boundaries to inspect, so the Swift package-boundaries rule is not applicable.
Cmux Swiftpm Lockfiles ✅ Passed No SwiftPM manifests, Package.resolved, .gitignore, or Xcode project files changed; the only workflow edit is Rust package build logic, not SwiftPM dependency resolution.
Cmux Swift Logging ✅ Passed No Swift files were changed; the diff only touches Rust, docs, scripts, and workflow files, so the Swift logging rule is not applicable.
Cmux Full Internationalization ✅ Passed PASS: The touched catalog is the only locale source, and both ENGLISH and JAPANESE entries include the new sidebar strings; no other supported locales surfaced.
Cmux Swiftui State Layout ✅ Passed Diff touches only Rust/docs/spec/build files; no SwiftUI files or @Observable/@Published/GeometryReader patterns were introduced.
Cmux Architecture Rethink ✅ Passed No Swift files were changed; the PR only touches Rust cmux-tui sources/docs/workflows, so the Swift architectural rule doesn’t apply.
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed No Swift files changed in the PR; only Rust/docs/scripts were modified, so the auxiliary-window shortcut rule is not applicable.
Cmux Source Artifacts ✅ Passed All changed paths are intentional source/docs/workflow/script files; no logs, temp dirs, caches, or build outputs were added.
Cmux No Test Or Debug Seam In Production Source ✅ Passed PASS: The PR diff touches only Rust/docs/workflow files; no Swift production Sources/** files were modified, so no test/debug seam could be introduced.
Cmux No Ambient Global State ✅ Passed No Swift files were changed, and the touched Rust files add instance-owned methods/structs, not new global singletons or ambient mutable state.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat-cmux-tui-horizontal-scroll

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@greptile-apps

greptile-apps Bot commented Jul 24, 2026 •

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR introduces horizontally scrollable viewport columns to cmux-tui: Ctrl-b g inserts a new terminal column at two-thirds viewport width, Alt-=/Alt-- and pointer drag resize it, Ctrl-b U undoes the last structural layout change (with confirmation when panes would close), and a horizontal status-bar scrollbar appears on overflow with optional 180 ms animation.

  • Viewport column model (model.rs, mux.rs, layout.rs): LayoutColumn, ScreenLayoutSnapshot, and a 32-entry VecDeque<LayoutUndoEntry> per screen back the new split tree; pane creation and resize changes are recorded, with resize samples coalesced by owner+transaction key; undo history is cleared on direct pane closure.
  • Rendering (app.rs, ui/mod.rs, ui/scrollbar.rs): copy_buffer_row_cropped clips pane content to the logical canvas before writing to the frame, sanitize_render_buffer scrubs control bytes after each draw, and new horizontal thumb geometry helpers handle the status-bar scrollbar with u128 intermediate arithmetic to avoid u16 overflow.
  • CLI and protocol (cli.rs, server.rs, session/mod.rs): new-pane-right, set-viewport-pane-width, and undo-layout commands are added behind viewport-splits-v1, viewport-column-resize-v1, and layout-undo-v1 capability gates; typed error_code fields in responses enable client-side localization; all new user-facing text is translated into English and Japanese.

Confidence Score: 5/5

The change is safe to merge; the only finding is a cosmetic issue in one CLI success message.

The viewport column and undo machinery is well-structured: capability gating protects older servers, the revision-fencing prevents stale confirmations from closing newer panes, and the 32-entry history cap bounds memory growth. Error paths are localized for both CLI and TUI surfaces, the renderer panic is caught and localized, and the horizontal scrollbar math is covered by unit tests including u16-boundary cases. The sole finding — the layout_undo_applied success message embedding an internal screen ID — is cosmetic and does not affect correctness or security.

Files Needing Attention: No files require special attention; the cosmetic CLI message issue is in localization.rs.

Important Files Changed

Filename Overview
cmux-tui/crates/cmux-tui/src/localization.rs Adds LayoutMessages and RuntimeMessages with complete English and Japanese translations for all new viewport/undo UI text; the layout_undo_applied success message embeds an internal screen ID that is not user-actionable.
cmux-tui/crates/cmux-tui-core/src/mux.rs Adds new_pane_right, undo_layout, set_viewport_pane_width, and related layout undo machinery; several bare anyhow::bail! paths in undo_layout produce plain errors that fall through the typed error handler and surface as raw strings (covered in previous review threads).
cmux-tui/crates/cmux-tui/src/app.rs Large addition of viewport column management, horizontal scrollbar, layout undo confirmation flow, PaneViewportClip rendering, and catch_unwind renderer panic recovery; logic looks correct and error paths are well-localized.
cmux-tui/crates/cmux-tui/src/cli.rs Adds new-pane-right, set-viewport-pane-width, and undo-layout CLI verbs with localized flag validation, capability checks, and a localized_response_error_for dispatcher that maps typed error codes to user-friendly messages.
cmux-tui/crates/cmux-tui-core/src/model.rs Adds LayoutColumn, ScreenLayoutSnapshot, LayoutUndoEntry, and supporting types for the 32-entry capped in-memory undo history with coalescing resize support.
cmux-tui/crates/cmux-tui/src/ui/scrollbar.rs Adds horizontal_thumb_geometry, horizontal_offset_at, and horizontal_drag_offset for the new horizontal viewport scrollbar; math is correct and covered by tests including u16-overflow edge cases.
cmux-tui/crates/cmux-tui/src/session/mod.rs Wires up undo_layout, set_viewport_pane_width, and new_pane_right for both local mux and remote sessions with proper capability checks and normalize_remote_* error converters that map typed error codes to LayoutUndoError/ViewportWidthError.
cmux-tui/crates/cmux-tui-core/src/server.rs Adds NewPaneRight, SetViewportPaneWidth, and UndoLayout command handlers with capability advertisement and typed error_code in the response envelope for client-side localization.
cmux-tui/crates/cmux-tui/src/ui/mod.rs Adds copy_buffer_row_cropped for clipped pane rendering, sanitize_render_buffer to scrub control bytes before frame diffing, ReusableRowBuffer scratch allocation, and a horizontal scrollbar track in the status bar.

Sequence Diagram

sequenceDiagram
    participant User
    participant App/CLI
    participant OrderedSession
    participant Session/Mux
    participant Server

    Note over User,Server: Ctrl-b g / new-pane-right
    User->>App/CLI: Ctrl-b g (or CLI new-pane-right)
    App/CLI->>OrderedSession: new_pane_right(pane, width, size)
    OrderedSession->>Session/Mux: session.new_pane_right(pane, width, size)
    Session/Mux->>Server: insert_layout_column_after + spawn PTY
    Server-->>Session/Mux: SurfaceCreated(surface_id)
    Session/Mux-->>OrderedSession: Ok(surface)
    OrderedSession-->>App/CLI: SessionCompletionAction::SurfaceCreated
    App/CLI-->>User: Viewport column appears (animated)

    Note over User,Server: Ctrl-b U / undo-layout (pane-creating action)
    User->>App/CLI: Ctrl-b U
    App/CLI->>OrderedSession: undo_layout(pane, None, false)
    OrderedSession->>Session/Mux: session.undo_layout(pane, None, false)
    Session/Mux-->>OrderedSession: "ConfirmationRequired{revision, closes_panes}"
    OrderedSession-->>App/CLI: "LayoutUndoConfirmation{pane, revision}"
    App/CLI-->>User: Type CONFIRM to close pane(s)
    User->>App/CLI: CONFIRM
    App/CLI->>OrderedSession: undo_layout(pane, Some(revision), true)
    OrderedSession->>Session/Mux: session.undo_layout(pane, Some(revision), true)
    Session/Mux-->>OrderedSession: "Undone{screen, revision}"
    App/CLI-->>User: Layout restored
Loading

Reviews (38): Last reviewed commit: "fix(cmux-tui): synchronize SDK versions ..." | Re-trigger Greptile

@coderabbitai coderabbitai 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.

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/src/app.rs (1)

6482-6495: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Gate SplitRight for scrolling mode; it can produce partial-height "columns".

split_size_hint (Lines 6487-6495) adapts the size hint for scrolling-mode SplitRight, but nothing prevents the action itself (run_action's Action::SplitRight, plus MenuAction::SplitRight) from running when the target pane is one row of a multi-row (Down-split) column — a fully supported scrolling-mode layout per this same PR's "vertical splits within columns" design.

In that case the new sibling gets a different rect.x (per walk_scrolling in cmux-tui-core/src/layout.rs) but only the partial height of that one row, so horizontal_columns() buckets it into its own column that is shorter than the screen. Scrolling to that column leaves the remaining height blank. new_pane_smart was already special-cased for scrolling mode (always using content_area, bypassing this path entirely) — SplitRight/the % keybinding and the pane context-menu split action were not.

Minimal fix: mirror the existing resize_split/resize_focused_split gating pattern and either no-op SplitRight (with a status message) or redirect it through the same new_pane_smart-style full-height path whenever self.config.layout.mode == PaneLayoutMode::Scrolling.

🐛 Suggested minimal gating (mirrors existing resize_split pattern)
             Action::SplitRight => {
                 if let Some(pane) = pane {
+                    if self.config.layout.mode == PaneLayoutMode::Scrolling {
+                        self.status_message =
+                            Some("Right splits are unavailable in scrolling layout; use Alt-n or `%` on a full-height pane".to_string());
+                    } else {
                     self.split_pane(pane, SplitDir::Right)?;
+                    }
                 }
             }

Also applies to: 6524-6550, 7729-7731, 10082-10086, 10133-10138

🤖 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/src/app.rs` around lines 6482 - 6495, Gate all
scrolling-mode SplitRight entry points so they cannot create a partial-height
sibling: update run_action’s Action::SplitRight handling and the
MenuAction::SplitRight path (including the related keybinding/context-menu
routes) to mirror the existing resize_split/resize_focused_split gating. Either
no-op with a status message or route through the full-height
new_pane_smart-style path when layout mode is PaneLayoutMode::Scrolling, while
preserving current behavior for other layout modes.
🤖 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/src/app.rs`:
- Around line 6482-6495: Gate all scrolling-mode SplitRight entry points so they
cannot create a partial-height sibling: update run_action’s Action::SplitRight
handling and the MenuAction::SplitRight path (including the related
keybinding/context-menu routes) to mirror the existing
resize_split/resize_focused_split gating. Either no-op with a status message or
route through the full-height new_pane_smart-style path when layout mode is
PaneLayoutMode::Scrolling, while preserving current behavior for other layout
modes.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 76903692-9933-4200-9cc5-4c3a80b0f2cb

📥 Commits

Reviewing files that changed from the base of the PR and between 4dc00ae and eb4c18c.

📒 Files selected for processing (12)
  • cmux-tui/README.md
  • cmux-tui/crates/cmux-tui-core/src/layout.rs
  • cmux-tui/crates/cmux-tui-core/src/lib.rs
  • cmux-tui/crates/cmux-tui/src/app.rs
  • cmux-tui/crates/cmux-tui/src/config.rs
  • cmux-tui/crates/cmux-tui/src/main.rs
  • cmux-tui/crates/cmux-tui/src/ui/mod.rs
  • cmux-tui/crates/cmux-tui/src/ui/pane.rs
  • cmux-tui/crates/cmux-tui/src/ui/scrollbar.rs
  • cmux-tui/docs/configuration.md
  • cmux-tui/docs/keyboard.md
  • cmux-tui/docs/mouse.md

@cursor

cursor Bot commented Jul 24, 2026

Copy link
Copy Markdown

Bugbot is paused — on-demand spend limit reached

Bugbot 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 lawrencecchen changed the title Add horizontal scrolling layout to cmux-tui Add command-driven horizontal viewport panes to cmux-tui Jul 24, 2026

@coderabbitai coderabbitai 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.

Actionable comments posted: 13

🤖 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/layout.rs`:
- Around line 531-545: Extract the shared viewport column geometry calculation
used by walk_viewport and the split-layout branch around a_rect, b_rect, and
split_area into a helper. Reuse it at both call sites for the fraction clamp and
width calculation so b_rect.x and the rendered viewport column remain identical,
preserving matches_boundary divider resolution.

In `@cmux-tui/crates/cmux-tui-core/src/mux.rs`:
- Around line 6204-6214: Prevent viewport layout probes from saturating u16
coordinates in both affected sites: cmux-tui/crates/cmux-tui-core/src/mux.rs
lines 6204-6214 and 6226-6236. Keep the 10,000-cell probe for tiled layout, but
use a bounded viewport probe width (such as 1,000) or otherwise scale it so
layout_screen_with_viewport cannot exceed u16::MAX; apply the same change
consistently to pane_neighbor and pane_focus_neighbor/focus_direction paths.

In `@cmux-tui/crates/cmux-tui-core/src/server.rs`:
- Around line 2119-2121: Extend the ApplyLayout request contract and handler to
accept and forward exported viewport_splits metadata to mux.apply_layout,
preserving viewport semantics during export/apply round trips. Add a round-trip
test covering export-layout followed by ApplyLayout, or explicitly document that
viewport metadata is intentionally not re-applicable.

In `@cmux-tui/crates/cmux-tui/src/app.rs`:
- Around line 10370-10399: The viewport-aware split lookup and viewport-split
exclusion are duplicated in resize_focused_split and resize_split. Extract this
logic into a shared exact_split_for_edge helper near the related resize methods,
then update both call sites to use it while preserving the existing active-pane,
area, pane, edge, and smallest-target selection behavior.
- Around line 10758-10765: Update the horizontal-scroll handling in
handle_horizontal_scroll so visible scrollbars do not consume wheel events
across the entire content_area. Apply the viewport scroll and early return only
when the pointer is on the horizontal scrollbar track or the pane content is
actually horizontally clipped; otherwise allow execution to reach
forward_pty_mouse_at.

In `@cmux-tui/crates/cmux-tui/src/cli.rs`:
- Around line 227-232: Update the help text for the new-pane-right VerbSpec to
state that the pane defaults to two-thirds width, while preserving the existing
optional --width behavior.

In `@cmux-tui/crates/cmux-tui/src/session/tree.rs`:
- Around line 352-363: Update the viewport_splits parsing in the remote snapshot
parser to retain entries only when width is finite and within the protocol range
0.1..=1.0. Continue ignoring entries with missing or invalid split/width fields,
so rejected entries are omitted and the existing fallback split ratio is used.

In `@cmux-tui/crates/cmux-tui/src/ui/graphics.rs`:
- Around line 17-18: Update the crop parameters used by place_image_cropped() to
replace source_x_px and source_width_px with a single optional horizontal crop
value such as source_crop_px: Option<(u32, u32)>. Build the `,x=...,w=...`
fragment only from that tuple, ensuring an offset cannot be supplied without its
corresponding width and preserving uncropped behavior when it is None.

In `@cmux-tui/crates/cmux-tui/src/ui/scrollbar.rs`:
- Around line 37-54: Update horizontal_offset_at and
horizontal_track_positions_map_to_offsets to map the position through the
thumb’s travel range rather than the full track, treating position as the thumb
centre; use horizontal_thumb_geometry’s thumb width and preserve clamping and
zero-range behavior. Ensure clicks and Drag::HorizontalScrollbar round-trip with
rendered geometry, and add the requested round-trip assertion covering
representative offsets.

In `@cmux-tui/docs/concepts.md`:
- Around line 39-42: Use the hyphenated compound modifier “two-thirds”
consistently in cmux-tui/docs/concepts.md lines 39-42 and
cmux-tui/docs/configuration.md lines 191-198, replacing each occurrence of “two
thirds” without changing the surrounding documentation.

In `@cmux-tui/README.md`:
- Around line 38-39: Update the README sentence describing the appended terminal
width to hyphenate “two-thirds,” so it reads “two-thirds of the viewport width.”

In `@cmux-tui/spec/commands.md`:
- Line 1152: Update the new-pane error-handling flow associated with
mux.new_pane_right so callers receive the stable user-facing error “pane
creation failed” instead of raw PTY or spawn details. Log the underlying failure
internally using the existing server/session logging mechanism, and update the
documented contract to describe the sanitized error.
- Line 1109: Update the command mappings in commands.md: remove --width
<fraction> from new-pane and add it to new-pane-right, matching the request
parameters and the existing CLI specification. Preserve the other new-pane and
new-pane-right flags unchanged.
🪄 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: 8618c6e4-9267-4154-aaad-e0d7fb2596fb

📥 Commits

Reviewing files that changed from the base of the PR and between eb4c18c and 528cc75.

📒 Files selected for processing (27)
  • cmux-tui/README.md
  • cmux-tui/crates/cmux-tui-core/src/layout.rs
  • cmux-tui/crates/cmux-tui-core/src/lib.rs
  • cmux-tui/crates/cmux-tui-core/src/model.rs
  • cmux-tui/crates/cmux-tui-core/src/mux.rs
  • cmux-tui/crates/cmux-tui-core/src/server.rs
  • cmux-tui/crates/cmux-tui/src/app.rs
  • cmux-tui/crates/cmux-tui/src/cli.rs
  • cmux-tui/crates/cmux-tui/src/config.rs
  • cmux-tui/crates/cmux-tui/src/main.rs
  • cmux-tui/crates/cmux-tui/src/session/mod.rs
  • cmux-tui/crates/cmux-tui/src/session/tree.rs
  • cmux-tui/crates/cmux-tui/src/ui/graphics.rs
  • cmux-tui/crates/cmux-tui/src/ui/graphics_writer.rs
  • cmux-tui/crates/cmux-tui/src/ui/mod.rs
  • cmux-tui/crates/cmux-tui/src/ui/pane.rs
  • cmux-tui/crates/cmux-tui/src/ui/scrollbar.rs
  • cmux-tui/crates/cmux-tui/src/ui/terminal_grid.rs
  • cmux-tui/crates/cmux-tui/tests/cli.rs
  • cmux-tui/docs/concepts.md
  • cmux-tui/docs/configuration.md
  • cmux-tui/docs/keyboard.md
  • cmux-tui/docs/mouse.md
  • cmux-tui/docs/protocol.md
  • cmux-tui/spec/cli.md
  • cmux-tui/spec/commands.md
  • cmux-tui/spec/transports.md

Comment thread cmux-tui/crates/cmux-tui-core/src/layout.rs Outdated
Comment thread cmux-tui/crates/cmux-tui-core/src/mux.rs Outdated
Comment thread cmux-tui/crates/cmux-tui-core/src/server.rs Outdated
Comment thread cmux-tui/crates/cmux-tui/src/app.rs Outdated
Comment thread cmux-tui/crates/cmux-tui/src/app.rs Outdated
Comment thread cmux-tui/crates/cmux-tui/src/ui/scrollbar.rs
Comment thread cmux-tui/docs/concepts.md
Comment thread cmux-tui/README.md Outdated
Comment thread cmux-tui/spec/commands.md Outdated
Comment thread cmux-tui/spec/commands.md Outdated
@lawrencecchen lawrencecchen changed the title Add command-driven horizontal viewport panes to cmux-tui Add horizontally scrolling columns to cmux-tui Jul 26, 2026
Comment thread cmux-tui/crates/cmux-tui/src/app.rs Outdated

@coderabbitai coderabbitai 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.

Actionable comments posted: 8

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 (1)

2130-2153: 🗄️ Data Integrity & Integration | 🟠 Major

Viewport metadata still isn't round-trippable through apply-layout.

export_layout_json now emits viewport_splits/viewport_base_width, but Command::ApplyLayout still only accepts layout: LayoutRequest, and its handler still forwards just that layout to mux.apply_layout. Re-applying an exported layout therefore silently drops viewport column state. This was flagged on a previous commit and remains unresolved.

Also applies to: 168-178, 3179-3189

🤖 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 2130 - 2153, Extend
Command::ApplyLayout and its request parsing to accept viewport_splits and
viewport_base_width alongside LayoutRequest. Update the ApplyLayout handler to
forward these fields through mux.apply_layout, preserving them when reapplying
JSON produced by export_layout_json, including empty or absent metadata
behavior.
cmux-tui/crates/cmux-tui/src/app.rs (1)

5717-5757: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win

viewport_states grows without bound as screens are closed.

sync_viewport_motion/set_viewport_target insert a ViewportMotion per ScreenId but nothing removes entries when a screen is closed; the map only clears on reset_session_presentation. In a long-lived session with churning screens this retains one motion per historical screen, and viewport_animation_active/tick_viewport_animation iterate the whole map every idle tick. Prune against the live screen set when the tree is replaced.

♻️ Suggested pruning hook (in `replace_tree`/`sync_layout`)
+        let live_screens = self
+            .tree
+            .workspaces
+            .iter()
+            .flat_map(|workspace| workspace.screens.iter())
+            .map(|screen| screen.id)
+            .collect::<HashSet<_>>();
+        self.viewport_states.retain(|screen, _| live_screens.contains(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/src/app.rs` around lines 5717 - 5757, Prune stale
entries from viewport_states whenever the screen tree is replaced or
synchronized, using the live ScreenId set from the current tree. Update the
replace_tree/sync_layout flow rather than sync_viewport_motion or
set_viewport_target, and retain motions only for screens still present so
animation iteration cannot grow with closed screens.

Source: Coding guidelines

♻️ Duplicate comments (5)
cmux-tui/spec/commands.md (2)

1107-1116: 🎯 Functional Correctness | 🟡 Minor

--width documented on new-pane, which doesn't accept it.

new-pane's Params table has no width field, and its CLI allowed list in cli.rs doesn't include "width" either — passing --width to new-pane will be rejected with "unexpected --width". This exact misplacement (move --width to new-pane-right only) was already flagged in a previous review round; this diff re-adds it to new-pane's flags row instead of removing it.

✏️ Proposed fix
-| Flags | `--pane <id> [--width <fraction>] [--cols <n> --rows <n>]` |
+| Flags | `--pane <id> [--cols <n> --rows <n>]` |

(applies to the new-pane row at line 1112 only; the new-pane-right row at line 1163 is already correct)

🤖 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/spec/commands.md` around lines 1107 - 1116, Update the `new-pane`
CLI mapping row to remove the unsupported `--width <fraction>` flag, leaving
only its accepted pane, column, and row options. Keep the existing
`new-pane-right` mapping unchanged, since it is the command that supports
`--width`.

1149-1156: 🔒 Security & Privacy | 🟠 Major

new-pane-right still documents a raw spawn/PTY error string.

Unlike new-pane, which was updated to return a sanitized pane creation failed message with "raw runtime details are logged internally only", new-pane-right's Errors table still lists "spawn or PTY error string" verbatim. This was flagged as a major security/privacy concern in a previous review round and remains unaddressed. As per coding guidelines, user-facing/API error bodies must not expose raw upstream errors and should state what happened in product terms.

🤖 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/spec/commands.md` around lines 1149 - 1156, The new-pane-right
Errors documentation still exposes raw spawn/PTY errors; update its error
contract to describe the sanitized “pane creation failed” response and state
that runtime details are logged internally only, matching new-pane. Retain the
other documented conditions unchanged.

Source: Coding guidelines

cmux-tui/crates/cmux-tui/src/cli.rs (1)

230-235: 📐 Maintainability & Code Quality | 🟡 Minor

Help text still says "two-thirds-width" despite the optional --width override.

new-pane-right accepts --width, so describing it as always two-thirds width is misleading. This was flagged on a previous commit and the text is unchanged.

✏️ Proposed fix
-        help: "Create a two-thirds-width viewport pane to the right.",
+        help: "Create a viewport pane to the right (default width: two-thirds).",
🤖 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/src/cli.rs` around lines 230 - 235, Update the help
text for the new-pane-right VerbSpec to describe the default right-pane sizing
without implying it is always two-thirds width, while retaining that the
optional width override is supported.
cmux-tui/crates/cmux-tui/src/session/tree.rs (1)

346-376: 🎯 Functional Correctness | 🟡 Minor

Viewport widths from remote snapshots still aren't validated.

viewport_splits and viewport_base_width are parsed straight from as_f64() as f32 with no check against the protocol's documented 0.1..=1.0 range (or finiteness). A malformed/malicious remote response could inject NaN/Infinity or out-of-range widths into layout math. This was flagged on a previous commit for viewport_splits; the fix was never applied, and viewport_base_width has the same gap.

🛡️ Proposed fix
+        viewport_base_width: value
+            .get("viewport_base_width")
+            .and_then(Value::as_f64)
+            .map(|width| width as f32)
+            .filter(|width| (0.1..=1.0).contains(width)),
         viewport_splits: value
             .get("viewport_splits")
             .and_then(Value::as_array)
             .map(|splits| {
                 splits
                     .iter()
                     .filter_map(|value| {
-                        Some((value.get("split")?.as_u64()?, value.get("width")?.as_f64()? as f32))
+                        let split = value.get("split")?.as_u64()?;
+                        let width = value.get("width")?.as_f64()?;
+                        (0.1..=1.0).contains(&width).then_some((split, width as f32))
                     })
                     .collect()
             })
             .unwrap_or_default(),
🤖 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/src/session/tree.rs` around lines 346 - 376,
Validate viewport widths in parse_screen before storing them: require finite
values within the protocol’s inclusive 0.1..=1.0 range for both
viewport_base_width and each viewport_splits width, rejecting invalid entries or
the snapshot consistently with existing parsing behavior. Apply the same
validation after as_f64 and before f64-to-f32 conversion, using the
viewport_base_width and viewport_splits parsing paths.
cmux-tui/crates/cmux-tui-core/src/mux.rs (1)

6379-6389: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

The 10 000-cell probe can still saturate u16 for viewport layouts.

Both neighbor helpers keep Rect { width: 10_000, height: 10_000 } for layout_screen_with_viewport. Unlike tiling, viewport columns append round(10_000 * fraction) cells each (minimum 1 000 at MIN_VIEWPORT_PANE_WIDTH), so from roughly the 9th appended column walk_viewport's saturating_add clamps every further column to 65 535 and adjacency is computed from overlapping rects. A previous review flagged this and it was marked addressed, but the probe width is unchanged here — please confirm the fix landed elsewhere or bound the viewport probe (for example width: 1_000) while keeping 10 000 for the tiled path.

#!/bin/bash
# Look for any bounding of the viewport probe width introduced elsewhere.
rg -nP -C4 '10_000|65_535|probe' --type=rust cmux-tui/crates/cmux-tui-core/src/mux.rs
rg -nP -C3 'saturating_add|virtual_width' --type=rust cmux-tui/crates/cmux-tui-core/src/layout.rs

Also applies to: 6402-6412

🤖 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 6379 - 6389, Update
the neighbor-helper probe dimensions used by layout_screen_with_viewport so
viewport layouts use a bounded width (for example 1,000) rather than 10,000,
preventing u16 saturation during appended-column calculation. Keep the
10,000-cell probe for the tiled layout path and preserve the existing height and
adjacency behavior.
🤖 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/mux.rs`:
- Around line 5106-5142: Avoid cloning layout snapshots for screens that do not
own the target pane: in split_with_viewport_width at
cmux-tui/crates/cmux-tui-core/src/mux.rs:5106-5142, continue when
!screen.root.contains(target) before calling screen.layout_snapshot(); in
set_ratio at cmux-tui/crates/cmux-tui-core/src/mux.rs:6229-6250, confirm the
screen owns pane before taking the snapshot. Preserve the existing mutation
behavior for matching screens.
- Around line 5232-5276: Extract the duplicated auto-layout rebuilding logic
from the column and non-column branches into a helper accepting &mut Node, &mut
Option<Vec<PaneId>>, and the new PaneId. Have the helper prune missing panes,
append the new pane, rebuild the default layout, and update the stored
auto-layout list; call it with the appropriate column or screen fields before
preserving projection synchronization for column layouts.
- Around line 6441-6462: Update the columns branch in swap_panes to detect when
pane and target are the same before mutating columns or setting changed. Return
false for this self-swap, matching the non-column Node::swap_leaves behavior, so
no projection sync, layout reset, undo entry, or change events are produced.
- Around line 6195-6204: Update the layout-columns branch around
expand_stack_pane to track whether either expansion call returns true, and call
sync_layout_column_projection only when at least one stack actually expanded.
Preserve the existing expansion calls and non-column behavior.

In `@cmux-tui/crates/cmux-tui/src/app.rs`:
- Around line 1886-1900: Introduce typed variants or a structured undo-error
type in cmux-tui-core’s Mux::undo_layout for unavailable and stale-revision
outcomes, preserving those distinctions through session.undo_layout and any
remote/error-wrapping paths. Update the enqueue_with_completion “undo layout”
handler to match the typed error/result variants rather than comparing
error.to_string() or using contains, mapping unavailable to
LayoutUndoUnavailable and stale revisions to LayoutUndoStale when confirm_close
is enabled.

In `@cmux-tui/crates/cmux-tui/src/localization.rs`:
- Line 198: Update the English and Japanese layout_undo_stale messages in
cmux-tui/crates/cmux-tui/src/localization.rs at lines 198-198 and 281-281 to
include a concrete “try undo again” recovery action and its Japanese equivalent,
then revise the localization assertions at lines 368-383 to match both updated
messages.

In `@cmux-tui/crates/cmux-tui/src/ui/pane.rs`:
- Around line 370-374: Clamp the raw-buffer copy in the pane rendering function
around visible_width and frame_buf so the loop never indexes beyond the frame’s
right edge. Account for bar.x plus the requested width, while preserving the
existing source_x/full_width clipping and early rejection for bars starting
outside the screen; ensure each (bar.x + dx, bar.y) coordinate is within frame
bounds.

In `@cmux-tui/spec/bindings.md`:
- Line 15: Update the Version check row in bindings.md to also require
viewport-splits-v1 for new-pane-right and viewport-column-resize-v1 for
set-viewport-pane-width, alongside the existing layout-undo-v1 requirement.

---

Outside diff comments:
In `@cmux-tui/crates/cmux-tui-core/src/server.rs`:
- Around line 2130-2153: Extend Command::ApplyLayout and its request parsing to
accept viewport_splits and viewport_base_width alongside LayoutRequest. Update
the ApplyLayout handler to forward these fields through mux.apply_layout,
preserving them when reapplying JSON produced by export_layout_json, including
empty or absent metadata behavior.

In `@cmux-tui/crates/cmux-tui/src/app.rs`:
- Around line 5717-5757: Prune stale entries from viewport_states whenever the
screen tree is replaced or synchronized, using the live ScreenId set from the
current tree. Update the replace_tree/sync_layout flow rather than
sync_viewport_motion or set_viewport_target, and retain motions only for screens
still present so animation iteration cannot grow with closed screens.

---

Duplicate comments:
In `@cmux-tui/crates/cmux-tui-core/src/mux.rs`:
- Around line 6379-6389: Update the neighbor-helper probe dimensions used by
layout_screen_with_viewport so viewport layouts use a bounded width (for example
1,000) rather than 10,000, preventing u16 saturation during appended-column
calculation. Keep the 10,000-cell probe for the tiled layout path and preserve
the existing height and adjacency behavior.

In `@cmux-tui/crates/cmux-tui/src/cli.rs`:
- Around line 230-235: Update the help text for the new-pane-right VerbSpec to
describe the default right-pane sizing without implying it is always two-thirds
width, while retaining that the optional width override is supported.

In `@cmux-tui/crates/cmux-tui/src/session/tree.rs`:
- Around line 346-376: Validate viewport widths in parse_screen before storing
them: require finite values within the protocol’s inclusive 0.1..=1.0 range for
both viewport_base_width and each viewport_splits width, rejecting invalid
entries or the snapshot consistently with existing parsing behavior. Apply the
same validation after as_f64 and before f64-to-f32 conversion, using the
viewport_base_width and viewport_splits parsing paths.

In `@cmux-tui/spec/commands.md`:
- Around line 1107-1116: Update the `new-pane` CLI mapping row to remove the
unsupported `--width <fraction>` flag, leaving only its accepted pane, column,
and row options. Keep the existing `new-pane-right` mapping unchanged, since it
is the command that supports `--width`.
- Around line 1149-1156: The new-pane-right Errors documentation still exposes
raw spawn/PTY errors; update its error contract to describe the sanitized “pane
creation failed” response and state that runtime details are logged internally
only, matching new-pane. Retain the other documented conditions unchanged.
🪄 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: fea47a26-e520-4a86-9673-87d946260a16

📥 Commits

Reviewing files that changed from the base of the PR and between 528cc75 and 254b462.

📒 Files selected for processing (29)
  • cmux-tui/README.md
  • cmux-tui/crates/cmux-tui-core/src/layout.rs
  • cmux-tui/crates/cmux-tui-core/src/lib.rs
  • cmux-tui/crates/cmux-tui-core/src/model.rs
  • cmux-tui/crates/cmux-tui-core/src/mux.rs
  • cmux-tui/crates/cmux-tui-core/src/server.rs
  • cmux-tui/crates/cmux-tui/src/app.rs
  • cmux-tui/crates/cmux-tui/src/cli.rs
  • cmux-tui/crates/cmux-tui/src/config.rs
  • cmux-tui/crates/cmux-tui/src/localization.rs
  • cmux-tui/crates/cmux-tui/src/main.rs
  • cmux-tui/crates/cmux-tui/src/session/mod.rs
  • cmux-tui/crates/cmux-tui/src/session/tree.rs
  • cmux-tui/crates/cmux-tui/src/ui/graphics_writer.rs
  • cmux-tui/crates/cmux-tui/src/ui/mod.rs
  • cmux-tui/crates/cmux-tui/src/ui/pane.rs
  • cmux-tui/crates/cmux-tui/src/ui/terminal_grid.rs
  • cmux-tui/crates/cmux-tui/tests/cli.rs
  • cmux-tui/crates/cmux-tui/tests/terminal_host_recovery.rs
  • cmux-tui/docs/concepts.md
  • cmux-tui/docs/configuration.md
  • cmux-tui/docs/keyboard.md
  • cmux-tui/docs/mouse.md
  • cmux-tui/docs/protocol.md
  • cmux-tui/spec/README.md
  • cmux-tui/spec/bindings.md
  • cmux-tui/spec/cli.md
  • cmux-tui/spec/commands.md
  • cmux-tui/spec/transports.md

Comment thread cmux-tui/crates/cmux-tui-core/src/mux.rs
Comment thread cmux-tui/crates/cmux-tui-core/src/mux.rs
Comment thread cmux-tui/crates/cmux-tui-core/src/mux.rs
Comment thread cmux-tui/crates/cmux-tui-core/src/mux.rs
Comment thread cmux-tui/crates/cmux-tui/src/app.rs Outdated
Comment thread cmux-tui/crates/cmux-tui/src/localization.rs
Comment thread cmux-tui/crates/cmux-tui/src/ui/pane.rs Outdated
Comment thread cmux-tui/spec/bindings.md Outdated
…tal-scroll

# Conflicts:
#	cmux-tui/README.md
#	cmux-tui/crates/cmux-tui-core/src/server.rs
#	cmux-tui/crates/cmux-tui/src/app.rs
#	cmux-tui/crates/cmux-tui/src/config.rs
#	cmux-tui/crates/cmux-tui/src/main.rs
#	cmux-tui/crates/cmux-tui/src/ui/graphics_writer.rs
#	cmux-tui/crates/cmux-tui/src/ui/mod.rs
#	cmux-tui/crates/cmux-tui/src/ui/terminal_grid.rs
#	cmux-tui/crates/cmux-tui/tests/cli.rs
#	cmux-tui/docs/configuration.md
#	cmux-tui/docs/keyboard.md
#	cmux-tui/docs/protocol.md
#	cmux-tui/spec/README.md
#	cmux-tui/spec/commands.md
#	cmux-tui/spec/transports.md

@coderabbitai coderabbitai 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.

Actionable comments posted: 3

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/src/config.rs (1)

1474-1482: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win

Index bindings once before resolving the catalog.

resolved_shortcuts() scans self.bindings once per catalog action: O(65 × B), where B is config-controlled binding count. Build HashMap<Action, Vec<String>> in one pass, then emit entries in catalog order.

🤖 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/src/config.rs` around lines 1474 - 1482, Update
resolved_shortcuts to build a HashMap<Action, Vec<String>> from self.bindings in
a single pass, then resolve catalog entries from that index in
action_definitions() order. Preserve filtering of actions without shortcuts and
the existing return type/order.

Source: Coding guidelines

cmux-tui/crates/cmux-tui/src/localization.rs (1)

49-58: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win

Do not echo raw CLI input or provider flags in errors.

These helpers interpolate arbitrary arguments directly into terminal-facing messages, including --cloud-port. Use product-level diagnostics with a safe recovery action instead; retain exact values only in sanitized logs.

As per coding guidelines, user-facing errors must not expose provider-specific flags or raw payloads and must provide concrete next actions.

🤖 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/src/localization.rs` around lines 49 - 58, Update
argument_needs_value_message, invalid_cloud_port_message, and
unknown_argument_message so terminal-facing errors use product-level diagnostics
and concrete safe recovery actions rather than interpolating raw CLI arguments,
values, or provider flags. Remove direct placeholder substitution from these
helpers; preserve exact inputs only through appropriately sanitized logging
paths.

Source: Coding guidelines

♻️ Duplicate comments (4)
cmux-tui/crates/cmux-tui-core/src/mux.rs (1)

5375-5423: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicate auto-layout rebuild logic (unaddressed from prior review).

The layout-columns branch (5384-5404) and the non-column branch (5406-5422) differ only in whether the owner is a LayoutColumn or the Screen; the prune/append/rebuild/store sequence is otherwise identical and can drift.

🤖 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 5375 - 5423, Extract
the shared pane auto-layout prune, append, rebuild, and store sequence from the
layout-columns and non-column branches into a reusable helper, parameterized by
the owning layout container and its pane IDs. Update both branches around
layout_column_for_pane_mut and screen to call that helper, preserving the
existing column projection synchronization only for the column path.
cmux-tui/crates/cmux-tui/src/cli.rs (1)

232-236: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Still says "two-thirds-width" even though --width overrides it.

This was already flagged on a prior commit and remains unfixed: the verb accepts --width, so the help text should say the two-thirds width is only the default.

🐛 Proposed fix
-        help: "Create a two-thirds-width viewport pane to the right.",
+        help: "Create a viewport pane to the right (default width: two-thirds).",
🤖 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/src/cli.rs` around lines 232 - 236, Update the help
text for the new-pane-right command in the command definition containing
build_new_pane_right to clarify that two-thirds width is the default and can be
overridden by --width; leave the command behavior unchanged.
cmux-tui/crates/cmux-tui/src/ui/scrollbar.rs (1)

191-209: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Thumb click/drag mapping still doesn't match rendered thumb geometry.

Unaddressed from a prior review: horizontal_offset_at maps position linearly over the full track (track_width - 1), but horizontal_thumb_geometry places the thumb within the travel range (track_width - thumb_width). The two spaces diverge whenever thumb_width > 1, so clicking on the rendered thumb (or dragging Drag::HorizontalScrollbar) will not track the cursor and will saturate before reaching the track end.

🐛 Proposed fix (from prior review)
 pub(crate) fn horizontal_offset_at(
     content_width: u16,
     viewport_width: u16,
     track_width: u16,
     position: u16,
 ) -> Option<u16> {
     if content_width == 0 || viewport_width == 0 || track_width == 0 {
         return None;
     }
     let maximum = content_width.saturating_sub(viewport_width);
-    if maximum == 0 || track_width == 1 {
+    let (_, thumb_width) =
+        horizontal_thumb_geometry(content_width, viewport_width, 0, track_width);
+    let travel = track_width.saturating_sub(thumb_width);
+    if maximum == 0 || travel == 0 {
         return Some(0);
     }
-    let position = position.min(track_width - 1) as u32;
-    let offset = (position * u32::from(maximum) + u32::from(track_width - 1) / 2)
-        / u32::from(track_width - 1);
+    let position = u32::from(position.min(track_width - 1))
+        .saturating_sub(u32::from(thumb_width) / 2)
+        .min(u32::from(travel));
+    let offset =
+        (position * u32::from(maximum) + u32::from(travel) / 2) / u32::from(travel);
     Some(offset as u16)
 }

The existing test horizontal_track_positions_map_to_offsets will need updated expected values, and a round-trip assertion against horizontal_thumb_geometry's output would lock the invariant in.

🤖 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/src/ui/scrollbar.rs` around lines 191 - 209, Update
horizontal_offset_at to map positions across the thumb’s travel range, using the
same thumb width and track geometry produced by horizontal_thumb_geometry rather
than the full track width. Keep clamping and zero-content behavior intact,
update horizontal_track_positions_map_to_offsets expected values, and add a
round-trip assertion verifying geometry-derived thumb positions map back
consistently.
cmux-tui/crates/cmux-tui/src/ui/pane.rs (1)

377-381: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Tab-bar frame copy can still panic if the pane rect overruns the frame edge.

Unaddressed from a prior review: visible_width is clamped against the logical bar's remaining width (full_width - source_x) but never against the frame's remaining width (frame_buf.area.width - bar.x), and there's no bar.y >= frame_buf.area.height guard at this copy site. Unlike the set_stringn/Widget paths this replaced, frame_buf[(x, y)] does not clip — it panics on out-of-bounds coordinates. The early-return guard at lines 218-227 only checks that bar.x/bar.y themselves are in range, not bar.x + bar.width.

🛡️ Proposed fix (from prior review)
-    let visible_width = bar.width.min(full_width.saturating_sub(source_x));
     let frame_buf = frame.buffer_mut();
+    let visible_width = bar
+        .width
+        .min(full_width.saturating_sub(source_x))
+        .min(frame_buf.area.width.saturating_sub(bar.x));
+    if bar.y >= frame_buf.area.height {
+        return;
+    }
     for dx in 0..visible_width {
         frame_buf[(bar.x + dx, bar.y)] = logical[(source_x + dx, 0)].clone();
     }

This is worth re-raising now specifically: the new fractional-width viewport columns and live resizing make an off-by-one pane rect more plausible than before.

🤖 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/src/ui/pane.rs` around lines 377 - 381, Update the
tab-bar frame copy around visible_width and the frame_buf indexing to guard
bar.y against frame_buf.area.height and clamp the copy width to the frame’s
remaining horizontal space (frame_buf.area.width.saturating_sub(bar.x)) as well
as the logical bar width. Preserve the existing logical source_x bounds while
ensuring every (bar.x + dx, bar.y) index is in range.
🤖 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/src/app.rs`:
- Around line 6259-6273: Prune stale entries from viewport_states whenever the
live screen tree is reconciled, using the existing sync_layout or replace_tree
flow. Retain ViewportMotion values only for screens currently present in the
tree, removing entries for closed screens so viewport_animation_active and
tick_viewport_animation scan a bounded map without adding repeated per-event
full-tree scans.

In `@cmux-tui/docs/configuration.md`:
- Line 200: Update the documentation sentence describing the insertion position
to use the hyphenated phrase “two-thirds of the current viewport width.”

In `@cmux-tui/spec/cli.md`:
- Around line 113-114: Escape the pipe separator in the direction alternatives
for the split and set-ratio command rows so each remains a five-cell Markdown
table row; preserve the existing right/down option text.

---

Outside diff comments:
In `@cmux-tui/crates/cmux-tui/src/config.rs`:
- Around line 1474-1482: Update resolved_shortcuts to build a HashMap<Action,
Vec<String>> from self.bindings in a single pass, then resolve catalog entries
from that index in action_definitions() order. Preserve filtering of actions
without shortcuts and the existing return type/order.

In `@cmux-tui/crates/cmux-tui/src/localization.rs`:
- Around line 49-58: Update argument_needs_value_message,
invalid_cloud_port_message, and unknown_argument_message so terminal-facing
errors use product-level diagnostics and concrete safe recovery actions rather
than interpolating raw CLI arguments, values, or provider flags. Remove direct
placeholder substitution from these helpers; preserve exact inputs only through
appropriately sanitized logging paths.

---

Duplicate comments:
In `@cmux-tui/crates/cmux-tui-core/src/mux.rs`:
- Around line 5375-5423: Extract the shared pane auto-layout prune, append,
rebuild, and store sequence from the layout-columns and non-column branches into
a reusable helper, parameterized by the owning layout container and its pane
IDs. Update both branches around layout_column_for_pane_mut and screen to call
that helper, preserving the existing column projection synchronization only for
the column path.

In `@cmux-tui/crates/cmux-tui/src/cli.rs`:
- Around line 232-236: Update the help text for the new-pane-right command in
the command definition containing build_new_pane_right to clarify that
two-thirds width is the default and can be overridden by --width; leave the
command behavior unchanged.

In `@cmux-tui/crates/cmux-tui/src/ui/pane.rs`:
- Around line 377-381: Update the tab-bar frame copy around visible_width and
the frame_buf indexing to guard bar.y against frame_buf.area.height and clamp
the copy width to the frame’s remaining horizontal space
(frame_buf.area.width.saturating_sub(bar.x)) as well as the logical bar width.
Preserve the existing logical source_x bounds while ensuring every (bar.x + dx,
bar.y) index is in range.

In `@cmux-tui/crates/cmux-tui/src/ui/scrollbar.rs`:
- Around line 191-209: Update horizontal_offset_at to map positions across the
thumb’s travel range, using the same thumb width and track geometry produced by
horizontal_thumb_geometry rather than the full track width. Keep clamping and
zero-content behavior intact, update horizontal_track_positions_map_to_offsets
expected values, and add a round-trip assertion verifying geometry-derived thumb
positions map back consistently.
🪄 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: 6d0aa175-cb1a-4371-a2ac-108360e32618

📥 Commits

Reviewing files that changed from the base of the PR and between 254b462 and f016c3c.

📒 Files selected for processing (25)
  • cmux-tui/README.md
  • cmux-tui/crates/cmux-tui-core/src/mux.rs
  • cmux-tui/crates/cmux-tui-core/src/server.rs
  • cmux-tui/crates/cmux-tui/src/app.rs
  • cmux-tui/crates/cmux-tui/src/cli.rs
  • cmux-tui/crates/cmux-tui/src/config.rs
  • cmux-tui/crates/cmux-tui/src/localization.rs
  • cmux-tui/crates/cmux-tui/src/main.rs
  • cmux-tui/crates/cmux-tui/src/session/mod.rs
  • cmux-tui/crates/cmux-tui/src/session/tree.rs
  • cmux-tui/crates/cmux-tui/src/ui/graphics_writer.rs
  • cmux-tui/crates/cmux-tui/src/ui/mod.rs
  • cmux-tui/crates/cmux-tui/src/ui/pane.rs
  • cmux-tui/crates/cmux-tui/src/ui/scrollbar.rs
  • cmux-tui/crates/cmux-tui/src/ui/terminal_grid.rs
  • cmux-tui/crates/cmux-tui/tests/cli.rs
  • cmux-tui/docs/configuration.md
  • cmux-tui/docs/keyboard.md
  • cmux-tui/docs/mouse.md
  • cmux-tui/docs/protocol.md
  • cmux-tui/spec/README.md
  • cmux-tui/spec/bindings.md
  • cmux-tui/spec/cli.md
  • cmux-tui/spec/commands.md
  • cmux-tui/spec/transports.md

Comment thread cmux-tui/crates/cmux-tui/src/app.rs
Comment thread cmux-tui/docs/configuration.md Outdated
Comment thread cmux-tui/spec/cli.md Outdated
…tal-scroll

# Conflicts:
#	cmux-tui/crates/cmux-tui/src/app.rs

@coderabbitai coderabbitai 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.

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/src/localization.rs (1)

759-773: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Cover every new provider-action mapping in both locales.

This test checks only one action label and one field mapping through catalog(), so it can miss regressions in the other mappings or Japanese catalog. Assert all action IDs and port-field mappings against both ENGLISH and JAPANESE, preferably with a small table-driven test.

🤖 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/src/localization.rs` around lines 759 - 773, Expand
workspace_port_provider_actions_use_localized_labels into a table-driven test
covering every provider-action label and port-field mapping, including the
expected values from both ENGLISH and JAPANESE catalogs. Invoke the mapping
helpers on each locale directly rather than only through catalog(), while
preserving the assertion that an unknown action returns None.
🤖 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/src/localization.rs`:
- Around line 759-773: Expand
workspace_port_provider_actions_use_localized_labels into a table-driven test
covering every provider-action label and port-field mapping, including the
expected values from both ENGLISH and JAPANESE catalogs. Invoke the mapping
helpers on each locale directly rather than only through catalog(), while
preserving the assertion that an unknown action returns None.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: cf7087c2-a546-4a94-bd9c-ce4a1bbf2ae4

📥 Commits

Reviewing files that changed from the base of the PR and between f016c3c and 41466df.

📒 Files selected for processing (2)
  • cmux-tui/crates/cmux-tui/src/app.rs
  • cmux-tui/crates/cmux-tui/src/localization.rs

Comment thread cmux-tui/crates/cmux-tui/src/app.rs
@cursor

cursor Bot commented Jul 27, 2026

Copy link
Copy Markdown

Bugbot is paused — on-demand spend limit reached

Bugbot 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

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Another round soon, please!

Reviewed commit: f4e0d37013

ℹ️ 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".

@lawrencecchen

Copy link
Copy Markdown
Contributor Author

@codex review

@lawrencecchen

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 27, 2026 •

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e1196f5e75

ℹ️ 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".

Comment thread cmux-tui/bindings/typescript/src/client.ts
@lawrencecchen
lawrencecchen merged commit e50bba3 into main Jul 28, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant