Repository navigation
Add pane controls and single-terminal attach to cmux TUI - #8710
Conversation
|
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 TUI adds ChangesTUI surface attach
Action catalog and compact sidebar
Shortcut-aware menus and overlays
Focused sidebar rendering
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 24 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
Greptile SummaryThis PR adds pane maximize/restore controls, a compact sidebar mode, scoped right-click context menus with localized shortcut labels, a scrollable shortcut-help modal, unified scrollbar rendering, and a
Confidence Score: 5/5Safe to merge. The changes are large but well-structured, with comprehensive test coverage. Every new code path has matching tests, and changes are isolated behind clear feature flags. The surface-only attach lifecycle is carefully sequenced to avoid event loss. The browser mouse-release reserve slot, the ReentrantMutex stdout guard, and the SurfaceSessionScope path-tracking mutation all address real edge cases with targeted fixes. No correctness issues were found in the changed paths. Files Needing Attention: No files require special attention. The most complex new logic — SurfaceSessionScope.accepts_tree_delta and BrowserInputDispatcher's reliable_mouse_release — are both covered by tests that exercise the specific edge cases they address. Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI as cmux-tui attach --surface
participant RS as RemoteSession
participant SRV as cmux-tui server
participant APP as App (surface-only)
participant BUS as MuxEventBroadcaster
CLI->>RS: connect_for_surface_attach (no subscribe yet)
RS->>SRV: identify + set-client-info
CLI->>RS: refresh_tree()
RS->>SRV: list-tree request
SRV-->>RS: tree snapshot
CLI->>RS: resolve_surface(reference)
Note over CLI: validates PTY kind and capability
CLI->>RS: scope_events_to_surface(surface_id)
RS->>BUS: prime_local_subscription (All filter)
RS->>SRV: "subscribe { surface: id }"
Note over RS: events accumulate in primed mailbox
CLI->>RS: refresh_tree() [surface still alive?]
CLI->>APP: "run_tui(session, surface_only=id)"
APP->>RS: subscribe() returns primed mailbox
loop event loop
SRV-->>RS: surface events (filtered)
RS->>BUS: emit(MuxEvent)
BUS->>APP: forward via accepts_mux_event
APP->>APP: render target surface full-screen
end
SRV-->>RS: surface-exited
RS->>APP: SurfaceExited(id)
APP->>APP: "quit = true, exit"
Reviews (22): Last reviewed commit: "fix(tui): bound browser release and atta..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/src/app.rs (1)
7613-7636: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSurface-only context menu should not offer pane/tab creation actions.
In
open_context_menu, onlyTogglePaneZoomis gated byself.surface_only.is_none(), whileNewPaneSmart,NewTab,NewBrowserTab,SplitRight, andSplitDownremain available. Surface-only mode always renders onlyNode::Leaf(screen.active_pane), so activating these actions creates panes/panes that this client cannot display or manage. Remove them in surface-only mode alongsideTogglePaneZoom.🤖 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 7613 - 7636, Update open_context_menu to filter pane/tab creation actions from the context-menu groups when self.surface_only is set, including NewPaneSmart, NewTab, NewBrowserTab, SplitRight, and SplitDown. Preserve the existing menu behavior otherwise, and keep TogglePaneZoom gated by self.surface_only.is_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/app.rs`:
- Around line 7613-7636: Update open_context_menu to filter pane/tab creation
actions from the context-menu groups when self.surface_only is set, including
NewPaneSmart, NewTab, NewBrowserTab, SplitRight, and SplitDown. Preserve the
existing menu behavior otherwise, and keep TogglePaneZoom gated by
self.surface_only.is_none().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7805af13-7ffa-4a19-bf5a-56c1647f3b0a
📒 Files selected for processing (15)
cmux-tui/README.mdcmux-tui/crates/cmux-tui/src/app.rscmux-tui/crates/cmux-tui/src/config.rscmux-tui/crates/cmux-tui/src/localization.rscmux-tui/crates/cmux-tui/src/main.rscmux-tui/crates/cmux-tui/src/session/tree.rscmux-tui/crates/cmux-tui/src/ui/mod.rscmux-tui/crates/cmux-tui/src/ui/overlay.rscmux-tui/crates/cmux-tui/src/ui/sidebar.rscmux-tui/crates/cmux-tui/tests/cli.rscmux-tui/docs/configuration.mdcmux-tui/docs/getting-started.mdcmux-tui/docs/keyboard.mdcmux-tui/docs/mouse.mdcmux-tui/spec/cli.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 (5)
cmux-tui/crates/cmux-tui/src/app.rs (1)
7657-7657: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Ctrl-b zstill mutates shared zoom state in--surfacemode, unlike the equivalent menu item.
open_context_menudeliberately omitsTogglePaneZoomwhenself.surface_only.is_some()because zoom has no visible effect in the single-pane view (sync_layoutalways rendersscreen.active_paneas aLeaf, ignoringzoomed_pane). ButAction::ZoomPaneinrun_action_for_paneisn't gated the same way, soCtrl-b zstill callsself.session.zoom_pane(pane)and silently changeszoomed_paneon the shared tree — invisible to this client but visible to any other client attached to the same session without--surface.🐛 Proposed fix: gate the keyboard action the same way the menu already is
- Action::ZoomPane => self.session.zoom_pane(pane), + Action::ZoomPane => { + if !self.is_surface_only() { + self.session.zoom_pane(pane); + } + }Also applies to: 10209-10217
🤖 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` at line 7657, Gate the Action::ZoomPane branch in run_action_for_pane with the same surface_only check used by open_context_menu, so --surface mode does not call self.session.zoom_pane(pane) or mutate shared zoom state; preserve the existing zoom behavior for non-surface sessions.cmux-tui/docs/configuration.md (1)
45-45: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftAdd locale-specific documentation for the new settings and shortcuts.
These user-facing Markdown additions are English-only, while
cmux-tui/crates/cmux-tui/src/localization.rsestablishes English and Japanese support. Add the corresponding Japanese documentation/source and keep the examples synchronized across supported locales.As per coding guidelines, user-facing Markdown must use locale-specific sources and update every supported locale.
Also applies to: 229-229, 248-248, 286-286, 338-338, 347-347
🤖 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/docs/configuration.md` at line 45, Add Japanese locale-specific documentation for the new settings and shortcuts, including the sidebar.compact_width entry and all related examples at the referenced sections. Keep the English and Japanese content synchronized, following the locale sources defined by localization.rs and covering every supported locale.Source: Coding guidelines
cmux-tui/crates/cmux-tui/src/ui/sidebar.rs (2)
389-395: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftKeep new UI and documentation copy locale-complete.
The workspace rail hard-codes
shellandscreenswhile adjacent copy useslocalization::catalog(), so Japanese users receive English fragments. The updated shortcut and mouse documentation is also English-only despite this PR’s English/Japanese localization scope.
cmux-tui/crates/cmux-tui/src/ui/sidebar.rs#L389-L395: add catalog-backed fallback and count formatting for the workspace subtitle.cmux-tui/docs/keyboard.md#L9-L10: update the corresponding localized documentation source for the shortcut-help behavior.cmux-tui/docs/mouse.md#L37-L41: update the corresponding localized documentation source for context-menu behavior.As per coding guidelines, “web, metadata, API, markdown, changelog, and user-facing data must use locale-specific sources and update every supported locale.”
🤖 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/sidebar.rs` around lines 389 - 395, Make the workspace subtitle locale-complete in cmux-tui/crates/cmux-tui/src/ui/sidebar.rs lines 389-395 by using localization::catalog() for the fallback title and screen-count formatting instead of hard-coded “shell” and “screens”. Update cmux-tui/docs/keyboard.md lines 9-10 with the corresponding localized shortcut-help documentation, and update cmux-tui/docs/mouse.md lines 37-41 with localized context-menu documentation for every supported locale.Source: Coding guidelines
172-228: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftBound rail rendering to the viewport and index managed machines.
draw_machinesrenders all N machines and linearly searches M managed records for each one (O(N×M)) despite a scroll viewport;draw_workspacessimilarly formats all W+R rows before discarding offscreen entries. Provider inventories and workspace histories are not fixed-size, so this runs on every redraw.
cmux-tui/crates/cmux-tui/src/ui/sidebar.rs#L172-L228: build aMachineKeylookup once per machine-state update and iterate only the viewport-visible machine indices.cmux-tui/crates/cmux-tui/src/ui/sidebar.rs#L379-L441: derive visible workspace/recoverable ranges from viewport offsets and emit rows/hits only for those ranges.As per coding guidelines, “Avoid repeated full scans, sorting, filtering, or per-item nested scans over scalable collections in production code.”
🤖 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/sidebar.rs` around lines 172 - 228, The draw_machines section in cmux-tui/crates/cmux-tui/src/ui/sidebar.rs:172-228 must build a MachineKey lookup once per machine-state update and iterate only machine indices visible in the viewport, replacing the per-machine managed_machines scan. The draw_workspaces section in cmux-tui/crates/cmux-tui/src/ui/sidebar.rs:379-441 must derive visible workspace and recoverable ranges from viewport offsets and emit rows and hit targets only for those ranges; preserve existing rendering and selection behavior for visible entries.Source: Coding guidelines
cmux-tui/crates/cmux-tui/src/localization.rs (1)
213-216: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSanitize technical details in user-facing error messages.
These strings expose implementation terms such as provider lifecycle/workspace updates, catalog updates, and managed-workspace authority bindings. Replace them with product-level wording and concrete retry, reconnect, or alternate-action guidance; retain technical details in logs or telemetry.
As per coding guidelines, user-facing errors must omit implementation details and provide one or two safe next actions.
Also applies to: 247-252, 307-310, 341-346
🤖 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 213 - 216, Update the user-facing localization strings around managed workspace errors, including managed_workspace_unsupported, managed_workspace_machine_inactive, managed_workspace_unavailable, and managed_workspace_operation_not_allowed, plus the referenced ranges, to remove provider lifecycle, catalog, update, and authority-binding terminology. Replace it with product-level wording and one or two concrete safe actions such as retrying, reconnecting, or choosing an available menu action; keep technical details only in logs or telemetry.Source: Coding guidelines
🤖 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`:
- Line 7657: Gate the Action::ZoomPane branch in run_action_for_pane with the
same surface_only check used by open_context_menu, so --surface mode does not
call self.session.zoom_pane(pane) or mutate shared zoom state; preserve the
existing zoom behavior for non-surface sessions.
In `@cmux-tui/crates/cmux-tui/src/localization.rs`:
- Around line 213-216: Update the user-facing localization strings around
managed workspace errors, including managed_workspace_unsupported,
managed_workspace_machine_inactive, managed_workspace_unavailable, and
managed_workspace_operation_not_allowed, plus the referenced ranges, to remove
provider lifecycle, catalog, update, and authority-binding terminology. Replace
it with product-level wording and one or two concrete safe actions such as
retrying, reconnecting, or choosing an available menu action; keep technical
details only in logs or telemetry.
In `@cmux-tui/crates/cmux-tui/src/ui/sidebar.rs`:
- Around line 389-395: Make the workspace subtitle locale-complete in
cmux-tui/crates/cmux-tui/src/ui/sidebar.rs lines 389-395 by using
localization::catalog() for the fallback title and screen-count formatting
instead of hard-coded “shell” and “screens”. Update cmux-tui/docs/keyboard.md
lines 9-10 with the corresponding localized shortcut-help documentation, and
update cmux-tui/docs/mouse.md lines 37-41 with localized context-menu
documentation for every supported locale.
- Around line 172-228: The draw_machines section in
cmux-tui/crates/cmux-tui/src/ui/sidebar.rs:172-228 must build a MachineKey
lookup once per machine-state update and iterate only machine indices visible in
the viewport, replacing the per-machine managed_machines scan. The
draw_workspaces section in cmux-tui/crates/cmux-tui/src/ui/sidebar.rs:379-441
must derive visible workspace and recoverable ranges from viewport offsets and
emit rows and hit targets only for those ranges; preserve existing rendering and
selection behavior for visible entries.
In `@cmux-tui/docs/configuration.md`:
- Line 45: Add Japanese locale-specific documentation for the new settings and
shortcuts, including the sidebar.compact_width entry and all related examples at
the referenced sections. Keep the English and Japanese content synchronized,
following the locale sources defined by localization.rs and covering every
supported locale.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 63e48b44-76f7-4b25-afcb-4d67bcfa57c1
📒 Files selected for processing (15)
cmux-tui/README.mdcmux-tui/crates/cmux-tui/src/app.rscmux-tui/crates/cmux-tui/src/config.rscmux-tui/crates/cmux-tui/src/localization.rscmux-tui/crates/cmux-tui/src/main.rscmux-tui/crates/cmux-tui/src/ui/mod.rscmux-tui/crates/cmux-tui/src/ui/overlay.rscmux-tui/crates/cmux-tui/src/ui/rail.rscmux-tui/crates/cmux-tui/src/ui/sidebar.rscmux-tui/crates/cmux-tui/tests/cli.rscmux-tui/docs/configuration.mdcmux-tui/docs/getting-started.mdcmux-tui/docs/keyboard.mdcmux-tui/docs/mouse.mdcmux-tui/spec/cli.md
There was a problem hiding this comment.
Actionable comments posted: 1
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/ui/overlay.rs (1)
253-275: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCap reserved shortcut width before truncating the label.
The label reserves the uncapped
shortcut.width(), while rendering caps it to the available row width. A long configured shortcut can therefore reduce the label budget to zero even though the displayed shortcut is truncated. Compute one capped shortcut width and use it for both calculations.🤖 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/overlay.rs` around lines 253 - 275, In the menu-row rendering logic, compute the shortcut width once with the available-width cap before calling label truncation. Reuse that capped value in both the label’s saturating width calculation and the shortcut rendering/positioning path, replacing the separate uncapped calculation in the `MenuItem` handling block.
🤖 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/docs/keyboard.md`:
- Around line 9-10: Localize the new shortcut-modal instructions in
cmux-tui/docs/keyboard.md lines 9-10 and the context-menu and shortcut-modal
instructions in cmux-tui/docs/mouse.md lines 37-41 by adding locale-specific
documentation sources or catalog entries and updating every supported locale,
including Japanese. Preserve the documented controls and behavior across all
translations.
---
Outside diff comments:
In `@cmux-tui/crates/cmux-tui/src/ui/overlay.rs`:
- Around line 253-275: In the menu-row rendering logic, compute the shortcut
width once with the available-width cap before calling label truncation. Reuse
that capped value in both the label’s saturating width calculation and the
shortcut rendering/positioning path, replacing the separate uncapped calculation
in the `MenuItem` handling block.
🪄 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: 5767593a-c756-4024-9241-b1ae3733cb98
📒 Files selected for processing (9)
cmux-tui/crates/cmux-tui/src/app.rscmux-tui/crates/cmux-tui/src/config.rscmux-tui/crates/cmux-tui/src/localization.rscmux-tui/crates/cmux-tui/src/main.rscmux-tui/crates/cmux-tui/src/ui/mod.rscmux-tui/crates/cmux-tui/src/ui/overlay.rscmux-tui/docs/configuration.mdcmux-tui/docs/keyboard.mdcmux-tui/docs/mouse.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/src/app.rs (1)
8295-8331: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win"Send prefix" silently no-ops on an unfocused browser pane.
forward_key_to_pane(called byAction::SendPrefixviarun_action_for_pane, and reachable from the newMenuAction::SendPrefix(pane)context-menu item) only forwards toforward_pty_key_to_surfacewhensurface.kind() == SurfaceKind::Pty.forward_keyhandles the active-pane case for both PTY and Browser (viaforward_browser_key), but there is no browser-aware equivalent for an explicit non-activepane. Right-clicking a background pane whose active tab is a browser tab and choosing "Send prefix" therefore does nothing, with no status feedback.🛡️ Proposed fix
fn forward_key_to_pane(&mut self, key: &KeyEvent, pane: Option<PaneId>) { if pane == self.active_pane() { self.forward_key(key); return; } if !self.session_available() { self.status_message = Some(localization::catalog().sidebar.no_active_session.to_string()); return; } let Some(surface_id) = pane.and_then(|pane| self.tree.pane(pane)).and_then(|pane| pane.active_surface()) else { return; }; let Some(surface) = self.session.surface(surface_id) else { return }; - if surface.kind() == SurfaceKind::Pty { - self.forward_pty_key_to_surface(key, surface_id, surface); - } + if surface.kind() == SurfaceKind::Pty { + self.forward_pty_key_to_surface(key, surface_id, surface); + } else { + self.status_message = + Some("Send prefix is not supported for browser panes".to_string()); + } }🤖 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 8295 - 8331, Update forward_key_to_pane so an explicitly targeted non-active pane with an active SurfaceKind::Browser is handled through the browser key-forwarding path, matching forward_key behavior; retain the existing PTY forwarding path and avoid silently dropping SendPrefix for browser panes.cmux-tui/crates/cmux-tui/src/main.rs (1)
697-710: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject
--surfacewhen dynamic providers are requested, or require--machine-provider-commandif the surface should remain attach-only.
validate_provider_process_argsonly checksargs.attach; a provider invocation likecmux-tui --surface s:abc --machine-provider /tmp/cmux.sockis currently accepted, butresolve_provider_launchreturnsSome(...)andmaincallsrun_provider_machine_client(...)instead ofrun_attach(...). That drops the requested single-surface filter and shows the full TUI instead.🤖 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/main.rs` around lines 697 - 710, Update validate_provider_process_args and the provider dispatch around resolve_provider_launch so --surface cannot be combined with a dynamic provider request; alternatively require --machine-provider-command for surface attach-only behavior. Ensure surface invocations continue through run_attach and never call run_provider_machine_client, preserving the requested single-surface filter.
🤖 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 8295-8331: Update forward_key_to_pane so an explicitly targeted
non-active pane with an active SurfaceKind::Browser is handled through the
browser key-forwarding path, matching forward_key behavior; retain the existing
PTY forwarding path and avoid silently dropping SendPrefix for browser panes.
In `@cmux-tui/crates/cmux-tui/src/main.rs`:
- Around line 697-710: Update validate_provider_process_args and the provider
dispatch around resolve_provider_launch so --surface cannot be combined with a
dynamic provider request; alternatively require --machine-provider-command for
surface attach-only behavior. Ensure surface invocations continue through
run_attach and never call run_provider_machine_client, preserving the requested
single-surface filter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f4a31e94-2be9-4f90-81c8-ec3f732cc3bd
📒 Files selected for processing (9)
cmux-tui/crates/cmux-tui/src/app.rscmux-tui/crates/cmux-tui/src/localization.rscmux-tui/crates/cmux-tui/src/main.rscmux-tui/crates/cmux-tui/src/ui/mod.rscmux-tui/crates/cmux-tui/src/ui/overlay.rscmux-tui/crates/cmux-tui/src/ui/scrollbar.rscmux-tui/crates/cmux-tui/src/ui/sidebar.rscmux-tui/docs/keyboard.mdcmux-tui/docs/mouse.md
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. |
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/docs/configuration.md (1)
207-207: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winLocalize these documentation changes across supported locales.
configuration.mdandmouse.mdcontain end-user copy and new reference-table text, but the matching localized docs are only present for the root README,README.ja.md, and the web frontend READMEs. Add translated versions of the new configuration, shortcut, scrollbar, resizing, and context-menu content for each supported locale before merging.🤖 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/docs/configuration.md` at line 207, Localize the new end-user documentation across all supported locales: translate the added configuration, shortcut, scrollbar, resizing, and context-menu content in cmux-tui/docs/configuration.md (line 207) and cmux-tui/docs/mouse.md (line 7), while preserving the existing localized documentation structure and terminology.Source: Coding guidelines
cmux-tui/crates/cmux-tui/src/app.rs (1)
7657-7699: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Action::CloseTabignores the explicitpaneparameter, unlikeAction::ClosePane.In
run_action_for_pane,Action::ClosePanecorrectly derives the target from thepaneargument, butAction::CloseTabreadsself.active_surface()instead. This contradicts the function's own doc comment ("Execute an action against an explicit pane. Context menus use this shared path because right-clicking does not change keyboard focus.") and the analogous handling immediately below it. Currently every call path reachingCloseTabhere happens to passpane == self.active_pane()(menu'sMenuAction::CloseTabbypasses this function entirely), so there's no live bug today, but it's a latent trap ifCloseTabis ever routed through this shared path with a non-focused pane (e.g. a future context-menu refactor).🐛 Proposed fix to honor the explicit pane
Action::CloseTab => { // Close the active tab; the pane collapses with its last // tab, so this is also "close pane" for single-tab panes. - if let Some(surface) = self.active_surface() { + if let Some(surface) = + pane.and_then(|pane| self.tree.pane(pane)).and_then(|pane| pane.active_surface()) + { self.render_states.remove(&surface); self.session.close_surface(surface); } }🤖 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 7657 - 7699, Update the Action::CloseTab branch in run_action_for_pane to target the explicitly provided pane rather than self.active_surface(). Resolve the pane’s surface using the same pane-to-surface relationship used by the surrounding pane actions, then remove its render state and close that surface; preserve the existing no-op behavior when no pane is provided.
🤖 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 7657-7699: Update the Action::CloseTab branch in
run_action_for_pane to target the explicitly provided pane rather than
self.active_surface(). Resolve the pane’s surface using the same pane-to-surface
relationship used by the surrounding pane actions, then remove its render state
and close that surface; preserve the existing no-op behavior when no pane is
provided.
In `@cmux-tui/docs/configuration.md`:
- Line 207: Localize the new end-user documentation across all supported
locales: translate the added configuration, shortcut, scrollbar, resizing, and
context-menu content in cmux-tui/docs/configuration.md (line 207) and
cmux-tui/docs/mouse.md (line 7), while preserving the existing localized
documentation structure and terminology.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9ab3121a-b72a-404b-98a1-00fc9355f83d
📒 Files selected for processing (6)
cmux-tui/crates/cmux-tui/src/app.rscmux-tui/crates/cmux-tui/src/config.rscmux-tui/crates/cmux-tui/src/main.rscmux-tui/docs/configuration.mdcmux-tui/docs/keyboard.mdcmux-tui/docs/mouse.md
…ntrols # Conflicts: # 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/pane.rs
7b28338 to
232c55c
Compare
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. |
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. |
1 participant
Centralizes executable TUI actions in one English/Japanese catalog used by config parsing, shortcut labels, the prefix guide, shortcut modal, context menus, and a future command palette.
Scopes right-click menus by region. Pane menus include New pane with
Alt-n, pane controls, IDs, and shortcut help. Sidebar menus include show/hide, compact/full, focus, and shortcut help. Screen menus contain screen actions and shortcut help. The files/workspaces view toggle stays keyboard-only.Adds pane maximize/restore,
Ctrl-b Ctrl-bprefix forwarding, screen and workspace navigation, compact sidebar mode, andcmux-tui attach --surface <id>for a chrome-free single-terminal client.The shortcut modal now uses neutral shortcut text, an always-visible neutral scrollbar, and a localized
[Esc close]button. The workspace sidebar has its own always-visible scrollbar one column inside the resize divider. Both scrollbars share row-viewport geometry and support wheel, track-click, and thumb-drag input.Validated with formatting, Clippy with warnings denied, the full locked Rust workspace test suite (including 447 cmux-tui unit tests and 9 CLI integration tests), an exact debug build, and live track-click, thumb-drag, and close-button interaction on the PR binary.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Adds pane controls, localized menus with shortcut labels, a scrollable shortcut modal, unified scrollbars, and a chrome-free single-terminal attach. Event filtering now isolates single-surface attach clients, preserves scoped browser input state, follows targets after tab moves, guards single-surface topology; fixes scoped attach lifecycle and explicit zoom intent, prevents a rare render deadlock, renders control characters as blanks, bounds attach actions, reserves capacity for reliable browser mouse releases, and exposes cell pixel size as a production API.
New Features
Ctrl-b ?opens a centered, scrollable list with a visible[Esc close]button;Ctrl-b Ctrl-bsends a literal prefix; routing prioritizes Close Tab and honors explicit pane targets; overlay drags finish cleanly and auto-dismiss on very small terminals.Ctrl-b m; width viasidebar.compact_width. Dragging the divider exits compact mode and applies a full-width override.cmux-tui attach --surface <id>accepts decimal or short ids; uses the full grid without chrome; isolates the client with a capability-gated, surface-scoped subscription that refreshes when the tab moves; exits when the terminal closes; bounds available actions; shows a clear message and falls back when filtered subscriptions are unavailable.Migration
sidebar.compact_width; rebind viakeys.toggle-sidebar-compact.--surfaceonly withattach:cmux-tui attach --surface <id>.attach --surfacestill accepts exact six-character short ids and rejects ambiguous digit-only values.Written for commit 54b76a2. Summary will update on new commits.
Summary by CodeRabbit
attach --surface <id>for surface-only PTY rendering (single terminal, full host grid) that exits when the target surface closes.Ctrl-b ?) with improved shortcut label rendering and overlay dismissal controls.sidebar.compact_width, plus related toggle/focus and expanded context-menu actions.--surface, shortcut modal behavior/labels, compact sizing, and scrollbar interaction.