Repository navigation
mux: pluggable sidebar — host external TUI plugins in the sidebar - #7695
Conversation
sidebar.plugin in mux.json runs an external TUI program in a server-owned PTY sized to the sidebar; output renders through the shared ghostty-vt grid path (new ui/terminal_grid.rs factored from the pane renderer). Env contract: CMUX_MUX_SOCKET + CMUX_SIDEBAR=1. prefix S focuses the sidebar (keys forward verbatim; prefix returns focus). Plugin survives hide/show, restarts with backoff on exit, dies with the server. Attach clients render it via the new sidebar-plugin command. spec/plugins.md documents the contract; smoke-tui.py drives a trivial echo plugin end to end incl. reload-config back to builtin. Reference plugin: https://github.com/manaflow-ai/cmux-sidebar-fzf
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR adds a sidebar plugin feature to mux: a configurable external process spawned in a PTY and rendered in the sidebar. It adds mux-core types/lifecycle logic with retry backoff, a control-socket command, TUI config parsing, focus/input wiring, a shared terminal-grid renderer, protocol bump to v7, and spec/docs/tests updates. ChangesSidebar plugin feature
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant Session
participant Mux
participant PluginSurface
User->>App: press prefix+S (focus-sidebar)
App->>App: toggle_sidebar_focus
App->>Session: sidebar_plugin(size, relaunch)
Session->>Mux: ensure_sidebar_plugin(cols, rows, relaunch)
Mux->>PluginSurface: spawn or resize
PluginSurface-->>Mux: status
Mux-->>Session: SidebarPluginStatus
Session-->>App: SidebarPluginSurface
App->>PluginSurface: forward_sidebar_key / paste_sidebar
App->>App: draw_plugin via terminal_grid
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (23 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 support for running external TUI plugins inside the mux sidebar. The main changes are:
Confidence Score: 4/5These issues should be fixed before merging.
mux/crates/mux-tui/src/app.rs, mux/crates/mux-tui/src/ui/sidebar.rs, mux/crates/mux-tui/src/config.rs Important Files Changed
Reviews (4): Last reviewed commit: "mux: judge fixes — protocol stays 6, cli..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
| } | ||
|
|
||
| fn sync_sidebar_plugin(&mut self, relaunch: bool) { |
There was a problem hiding this comment.
When an attached TUI connects to a server that has sidebar.plugin configured, this local config check returns before sending the new sidebar-plugin request unless the attach client has the same config file. The server-owned plugin surface is never discovered, so attach clients keep rendering the built-in sidebar instead of the configured plugin.
| pub fn draw(app: &mut App, frame: &mut Frame) { | ||
| if app.config.sidebar.plugin.is_some() { | ||
| draw_plugin(app, frame); | ||
| return; |
There was a problem hiding this comment.
This chooses the plugin renderer from the attach client's local config, but the plugin process is configured and hosted by the server. A remote client without a matching local sidebar.plugin entry always calls draw_builtin(), so even a valid server-returned plugin surface cannot be displayed.
| let command = plugin | ||
| .command | ||
| .unwrap_or_default() | ||
| .into_iter() | ||
| .filter(|arg| !arg.is_empty()) | ||
| .collect::<Vec<_>>(); |
There was a problem hiding this comment.
A plugin command can validly include an empty argument, such as an empty query, delimiter, or positional value. This filter removes those argv entries before spawning the process, so the plugin receives a different command line than the user configured.
| let command = plugin | |
| .command | |
| .unwrap_or_default() | |
| .into_iter() | |
| .filter(|arg| !arg.is_empty()) | |
| .collect::<Vec<_>>(); | |
| let command = plugin.command.unwrap_or_default(); |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@mux/crates/mux-core/src/mux.rs`:
- Around line 357-383: spawn_sidebar_plugin_surface duplicates the id creation,
test/non-test Surface::spawn dispatch, and state insertion logic already handled
by spawn_surface_with. Extract the shared spawn bookkeeping into a helper such
as finish_spawn on mux::Mux that takes the SurfaceId and SurfaceOptions,
performs the cfg(test) Surface::spawn_for_test vs Surface::spawn selection,
inserts into state.surfaces, and returns the Arc<Surface>; then have
spawn_sidebar_plugin_surface only build its overridden opts and delegate to that
helper.
- Around line 612-677: ensure_sidebar_plugin is racing because it drops
sidebar_plugin before spawn_sidebar_plugin_surface, allowing multiple callers to
start duplicate sidebar processes. Keep the sidebar_plugin slot exclusively
claimed under the same lock during startup, or introduce a spawning sentinel in
the runtime state so only one thread can spawn at a time. Update
ensure_sidebar_plugin and the SidebarPluginRuntime flow to serialize startup,
then set surface/last_error/retry_at only after the single spawn completes.
In `@mux/crates/mux-tui/src/app.rs`:
- Line 879: `sync_layout()` is re-querying the remote sidebar plugin on every
Draw because it always calls `sync_sidebar_plugin(false)`, which can cause
repeated blocking requests. Update the `App::sync_sidebar_plugin` flow to cache
the last plugin status and honor `sidebar_plugin_retry_after_ms` so `request()`
is skipped until the retry window expires, geometry changes, relaunch is
requested, or the plugin exits. Use the existing `sync_layout`,
`sync_sidebar_plugin`, and `sidebar_plugin_retry_after_ms` symbols to wire the
backoff logic into the draw path.
In `@mux/crates/mux-tui/src/ui/sidebar.rs`:
- Around line 66-67: The sidebar rendering path in sidebar.rs is returning too
early when app.sidebar_plugin_surface is set but app.session.surface(surface_id)
is missing, which prevents the fallback error message from being shown. Update
the sidebar rendering logic around the surface lookup to avoid the let-else
early return and instead fall through to the existing unavailable/error-message
branch, using nested if-let or equivalent control flow so the sidebar can
display the configured “plugin unavailable” message.
🪄 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
Run ID: 6bf9471d-475c-436f-9dfa-71f3ed161776
📒 Files selected for processing (17)
mux/crates/mux-core/src/lib.rsmux/crates/mux-core/src/mux.rsmux/crates/mux-core/src/server.rsmux/crates/mux-tui/src/app.rsmux/crates/mux-tui/src/config.rsmux/crates/mux-tui/src/main.rsmux/crates/mux-tui/src/session/mod.rsmux/crates/mux-tui/src/session/remote.rsmux/crates/mux-tui/src/ui/mod.rsmux/crates/mux-tui/src/ui/pane.rsmux/crates/mux-tui/src/ui/sidebar.rsmux/crates/mux-tui/src/ui/terminal_grid.rsmux/crates/mux-tui/tests/cli.rsmux/scripts/smoke-tui.pymux/spec/README.mdmux/spec/commands.mdmux/spec/plugins.md
| fn spawn_sidebar_plugin_surface( | ||
| self: &Arc<Self>, | ||
| options: &SidebarPluginOptions, | ||
| size: (u16, u16), | ||
| ) -> anyhow::Result<Arc<Surface>> { | ||
| if options.command.is_empty() { | ||
| anyhow::bail!("sidebar plugin command is empty"); | ||
| } | ||
| let id = self.next_id(); | ||
| let mut opts = self.surface_options.lock().unwrap().clone(); | ||
| opts.command = Some(options.command.clone()); | ||
| opts.cwd = options.cwd.clone(); | ||
| opts.cols = size.0.max(1); | ||
| opts.rows = size.1.max(1); | ||
| opts.extra_env.push(("CMUX_SIDEBAR".to_string(), "1".to_string())); | ||
| #[cfg(test)] | ||
| let surface = if self.test_surface_runtime { | ||
| Surface::spawn_for_test(id, opts, Arc::downgrade(self))? | ||
| } else { | ||
| Surface::spawn(id, opts, Arc::downgrade(self))? | ||
| }; | ||
| #[cfg(not(test))] | ||
| let surface = Surface::spawn(id, opts, Arc::downgrade(self))?; | ||
| self.state.lock().unwrap().surfaces.insert(id, surface.clone()); | ||
| Ok(surface) | ||
| } | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Duplicated surface-spawn boilerplate.
spawn_sidebar_plugin_surface repeats spawn_surface_with's id-generation / #[cfg(test)] spawn dispatch / state-insert block almost verbatim — only the opts field overrides differ. Consider extracting a shared fn finish_spawn(self: &Arc<Self>, id: SurfaceId, opts: SurfaceOptions) -> anyhow::Result<Arc<Surface>> helper that both callers build opts for and delegate to, so future changes to spawn bookkeeping (e.g. additional insert-time side effects) don't need to be kept in sync across two near-identical copies.
♻️ Sketch of a shared helper
+ fn finish_spawn(self: &Arc<Self>, id: u64, opts: SurfaceOptions) -> anyhow::Result<Arc<Surface>> {
+ #[cfg(test)]
+ let surface = if self.test_surface_runtime {
+ Surface::spawn_for_test(id, opts, Arc::downgrade(self))?
+ } else {
+ Surface::spawn(id, opts, Arc::downgrade(self))?
+ };
+ #[cfg(not(test))]
+ let surface = Surface::spawn(id, opts, Arc::downgrade(self))?;
+ self.state.lock().unwrap().surfaces.insert(id, surface.clone());
+ Ok(surface)
+ }
+
fn spawn_sidebar_plugin_surface(
self: &Arc<Self>,
options: &SidebarPluginOptions,
size: (u16, u16),
) -> anyhow::Result<Arc<Surface>> {
if options.command.is_empty() {
anyhow::bail!("sidebar plugin command is empty");
}
let id = self.next_id();
let mut opts = self.surface_options.lock().unwrap().clone();
opts.command = Some(options.command.clone());
opts.cwd = options.cwd.clone();
opts.cols = size.0.max(1);
opts.rows = size.1.max(1);
opts.extra_env.push(("CMUX_SIDEBAR".to_string(), "1".to_string()));
- #[cfg(test)]
- let surface = if self.test_surface_runtime {
- Surface::spawn_for_test(id, opts, Arc::downgrade(self))?
- } else {
- Surface::spawn(id, opts, Arc::downgrade(self))?
- };
- #[cfg(not(test))]
- let surface = Surface::spawn(id, opts, Arc::downgrade(self))?;
- self.state.lock().unwrap().surfaces.insert(id, surface.clone());
- Ok(surface)
+ self.finish_spawn(id, opts)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn spawn_sidebar_plugin_surface( | |
| self: &Arc<Self>, | |
| options: &SidebarPluginOptions, | |
| size: (u16, u16), | |
| ) -> anyhow::Result<Arc<Surface>> { | |
| if options.command.is_empty() { | |
| anyhow::bail!("sidebar plugin command is empty"); | |
| } | |
| let id = self.next_id(); | |
| let mut opts = self.surface_options.lock().unwrap().clone(); | |
| opts.command = Some(options.command.clone()); | |
| opts.cwd = options.cwd.clone(); | |
| opts.cols = size.0.max(1); | |
| opts.rows = size.1.max(1); | |
| opts.extra_env.push(("CMUX_SIDEBAR".to_string(), "1".to_string())); | |
| #[cfg(test)] | |
| let surface = if self.test_surface_runtime { | |
| Surface::spawn_for_test(id, opts, Arc::downgrade(self))? | |
| } else { | |
| Surface::spawn(id, opts, Arc::downgrade(self))? | |
| }; | |
| #[cfg(not(test))] | |
| let surface = Surface::spawn(id, opts, Arc::downgrade(self))?; | |
| self.state.lock().unwrap().surfaces.insert(id, surface.clone()); | |
| Ok(surface) | |
| } | |
| fn finish_spawn(self: &Arc<Self>, id: u64, opts: SurfaceOptions) -> anyhow::Result<Arc<Surface>> { | |
| #[cfg(test)] | |
| let surface = if self.test_surface_runtime { | |
| Surface::spawn_for_test(id, opts, Arc::downgrade(self))? | |
| } else { | |
| Surface::spawn(id, opts, Arc::downgrade(self))? | |
| }; | |
| #[cfg(not(test))] | |
| let surface = Surface::spawn(id, opts, Arc::downgrade(self))?; | |
| self.state.lock().unwrap().surfaces.insert(id, surface.clone()); | |
| Ok(surface) | |
| } | |
| fn spawn_sidebar_plugin_surface( | |
| self: &Arc<Self>, | |
| options: &SidebarPluginOptions, | |
| size: (u16, u16), | |
| ) -> anyhow::Result<Arc<Surface>> { | |
| if options.command.is_empty() { | |
| anyhow::bail!("sidebar plugin command is empty"); | |
| } | |
| let id = self.next_id(); | |
| let mut opts = self.surface_options.lock().unwrap().clone(); | |
| opts.command = Some(options.command.clone()); | |
| opts.cwd = options.cwd.clone(); | |
| opts.cols = size.0.max(1); | |
| opts.rows = size.1.max(1); | |
| opts.extra_env.push(("CMUX_SIDEBAR".to_string(), "1".to_string())); | |
| self.finish_spawn(id, opts) | |
| } |
🤖 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 `@mux/crates/mux-core/src/mux.rs` around lines 357 - 383,
spawn_sidebar_plugin_surface duplicates the id creation, test/non-test
Surface::spawn dispatch, and state insertion logic already handled by
spawn_surface_with. Extract the shared spawn bookkeeping into a helper such as
finish_spawn on mux::Mux that takes the SurfaceId and SurfaceOptions, performs
the cfg(test) Surface::spawn_for_test vs Surface::spawn selection, inserts into
state.surfaces, and returns the Arc<Surface>; then have
spawn_sidebar_plugin_surface only build its overridden opts and delegate to that
helper.
| pub fn ensure_sidebar_plugin( | ||
| self: &Arc<Self>, | ||
| cols: u16, | ||
| rows: u16, | ||
| relaunch: bool, | ||
| ) -> SidebarPluginStatus { | ||
| let now = Instant::now(); | ||
| let size = (cols.max(1), rows.max(1)); | ||
| let spawn_options = { | ||
| let mut runtime = self.sidebar_plugin.lock().unwrap(); | ||
| let Some(options) = runtime.options.clone() else { | ||
| return SidebarPluginStatus { surface: None, error: None, retry_after: None }; | ||
| }; | ||
| if let Some(surface_id) = runtime.surface { | ||
| if let Some(surface) = self.surface(surface_id).filter(|surface| !surface.is_dead()) | ||
| { | ||
| drop(runtime); | ||
| let _ = self.resize_surface(surface_id, size.0, size.1); | ||
| drop(surface); | ||
| return SidebarPluginStatus { | ||
| surface: Some(surface_id), | ||
| error: None, | ||
| retry_after: None, | ||
| }; | ||
| } | ||
| runtime.surface = None; | ||
| } | ||
| if let Some(error) = runtime.last_error.clone() { | ||
| let retry_after = runtime.retry_at.and_then(|retry_at| { | ||
| (retry_at > now).then_some(retry_at.saturating_duration_since(now)) | ||
| }); | ||
| if !relaunch || retry_after.is_some() { | ||
| return SidebarPluginStatus { surface: None, error: Some(error), retry_after }; | ||
| } | ||
| } | ||
| options | ||
| }; | ||
| match self.spawn_sidebar_plugin_surface(&spawn_options, size) { | ||
| Ok(surface) => { | ||
| let surface_id = surface.id; | ||
| { | ||
| let mut runtime = self.sidebar_plugin.lock().unwrap(); | ||
| runtime.surface = Some(surface_id); | ||
| runtime.last_error = None; | ||
| runtime.failures = 0; | ||
| runtime.retry_at = None; | ||
| } | ||
| self.reap_if_dead(&surface); | ||
| SidebarPluginStatus { surface: Some(surface_id), error: None, retry_after: None } | ||
| } | ||
| Err(err) => { | ||
| let mut runtime = self.sidebar_plugin.lock().unwrap(); | ||
| runtime.surface = None; | ||
| runtime.failures = runtime.failures.saturating_add(1); | ||
| let delay = sidebar_retry_delay(runtime.failures); | ||
| let message = format!("sidebar plugin failed to start: {err}"); | ||
| runtime.last_error = Some(message.clone()); | ||
| runtime.retry_at = Some(now + delay); | ||
| SidebarPluginStatus { | ||
| surface: None, | ||
| error: Some(message), | ||
| retry_after: Some(delay), | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm each socket connection is handled on its own thread (so
# concurrent SidebarPlugin commands from different clients are plausible).
rg -n 'thread::Builder|incoming\(\)|accept\(\)' mux/crates/mux-core/src/server.rs -A3Repository: manaflow-ai/cmux
Length of output: 1575
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant source first, then inspect the sidebar-plugin call path.
ast-grep outline mux/crates/mux-core/src/mux.rs --view expanded | sed -n '1,220p'
printf '\n--- ensure_sidebar_plugin references ---\n'
rg -n 'ensure_sidebar_plugin\(' mux/crates/mux-core/src -A3 -B3
printf '\n--- sidebar plugin command path ---\n'
rg -n 'sidebar-plugin|SidebarPlugin|ensure_sidebar_plugin' mux/crates/mux-core/src/server.rs mux/crates/mux-core/src -A4 -B4Repository: manaflow-ai/cmux
Length of output: 25539
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the sidebar plugin spawn path and any state registration/cleanup it triggers.
sed -n '307,390p' mux/crates/mux-core/src/mux.rs
printf '\n--- surface insertion/cleanup helpers ---\n'
sed -n '1338,1370p' mux/crates/mux-core/src/mux.rs
sed -n '1813,1848p' mux/crates/mux-core/src/mux.rsRepository: manaflow-ai/cmux
Length of output: 5710
Serialize sidebar plugin startup mux/crates/mux-core/src/mux.rs:620-660 — ensure_sidebar_plugin releases sidebar_plugin before spawning, so two connection threads can both see no live surface and start separate sidebar processes. The later surface id wins and the other stays in state.surfaces without a cleanup path. Keep the slot claimed under the same lock (or add a “spawning” sentinel) until spawn completes.
🤖 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 `@mux/crates/mux-core/src/mux.rs` around lines 612 - 677, ensure_sidebar_plugin
is racing because it drops sidebar_plugin before spawn_sidebar_plugin_surface,
allowing multiple callers to start duplicate sidebar processes. Keep the
sidebar_plugin slot exclusively claimed under the same lock during startup, or
introduce a spawning sentinel in the runtime state so only one thread can spawn
at a time. Update ensure_sidebar_plugin and the SidebarPluginRuntime flow to
serialize startup, then set surface/last_error/retry_at only after the single
spawn completes.
| height: height.saturating_sub(1), // status bar | ||
| }; | ||
| self.content_area = area; | ||
| self.sync_sidebar_plugin(false); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm sync_layout runs on every Draw and sync_sidebar_plugin has no request gating.
rg -nP -C3 'fn sync_layout' mux/crates/mux-tui/src/app.rs
rg -nP -C2 'sync_sidebar_plugin|RenderAction::Draw' mux/crates/mux-tui/src/app.rs
# Contrast with tree() caching in remote session.
rg -nP -C3 'tree_stale' mux/crates/mux-tui/src/session/remote.rsRepository: manaflow-ai/cmux
Length of output: 17654
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the sidebar sync path and the remote implementation it calls.
sed -n '936,980p' mux/crates/mux-tui/src/app.rs
printf '\n---\n'
rg -n -C3 'fn sidebar_plugin|sidebar_plugin_surface|request\(' mux/crates/mux-tui/src/session/remote.rs mux/crates/mux-tui/src/session.rs mux/crates/mux-tui/src -g '!**/target/**'Repository: manaflow-ai/cmux
Length of output: 38720
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the remote request implementation and whether retry_after_ms is used to avoid re-polling.
sed -n '404,436p' mux/crates/mux-tui/src/session/remote.rs
printf '\n---\n'
rg -n -C2 'sidebar_plugin_retry_after_ms|retry_after_ms' mux/crates/mux-tui/src/app.rs mux/crates/mux-tui/src/session/mod.rs mux/crates/mux-tui/src/session/remote.rsRepository: manaflow-ai/cmux
Length of output: 5140
Throttle remote sidebar-plugin lookups on draw
sync_layout() calls sync_sidebar_plugin(false) on every Draw, and the remote sidebar_plugin path issues a blocking request() each time. sidebar_plugin_retry_after_ms is captured but never used to back off, so a missing or slow sidebar plugin can turn redraws into repeated socket round-trips. Cache the last status and skip re-polling until geometry changes, relaunch is requested, the plugin exits, or the retry window expires.
🤖 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 `@mux/crates/mux-tui/src/app.rs` at line 879, `sync_layout()` is re-querying
the remote sidebar plugin on every Draw because it always calls
`sync_sidebar_plugin(false)`, which can cause repeated blocking requests. Update
the `App::sync_sidebar_plugin` flow to cache the last plugin status and honor
`sidebar_plugin_retry_after_ms` so `request()` is skipped until the retry window
expires, geometry changes, relaunch is requested, or the plugin exits. Use the
existing `sync_layout`, `sync_sidebar_plugin`, and
`sidebar_plugin_retry_after_ms` symbols to wire the backoff logic into the draw
path.
| if let Some(surface_id) = app.sidebar_plugin_surface { | ||
| let Some(surface) = app.session.surface(surface_id) else { return }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Missing surface should fall through to error message, not return early.
When sidebar_plugin_surface is set but the surface doesn't exist (e.g., during plugin restart backoff), the let-else at line 67 returns early, skipping the error message fallback at lines 87-99. The user sees stale content from the previous plugin frame instead of a meaningful "sidebar plugin unavailable" or configured error message.
🔧 Proposed fix: restructure as nested if-let to fall through
if let Some(surface_id) = app.sidebar_plugin_surface {
- let Some(surface) = app.session.surface(surface_id) else { return };
- surface.take_dirty();
- let theme = app.config.theme;
- let rs = app
- .render_states
- .entry(surface_id)
- .or_insert_with(|| ghostty_vt::RenderState::new().expect("render state alloc"));
- if surface.snapshot(rs).is_ok() {
- rs.set_clean();
- let _ =
- super::terminal_grid::draw_render_state(frame, content, rs, &theme, |_, _| false);
- {
- let buf = frame.buffer_mut();
- for y in 0..height {
- buf[(border_x, y)].set_symbol("│").set_style(border_style);
- }
+ if let Some(surface) = app.session.surface(surface_id) {
+ surface.take_dirty();
+ let theme = app.config.theme;
+ let rs = app
+ .render_states
+ .entry(surface_id)
+ .or_insert_with(|| ghostty_vt::RenderState::new().expect("render state alloc"));
+ if surface.snapshot(rs).is_ok() {
+ rs.set_clean();
+ let _ =
+ super::terminal_grid::draw_render_state(frame, content, rs, &theme, |_, _| false);
+ {
+ let buf = frame.buffer_mut();
+ for y in 0..height {
+ buf[(border_x, y)].set_symbol("│").set_style(border_style);
+ }
+ }
+ return;
}
- return;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if let Some(surface_id) = app.sidebar_plugin_surface { | |
| let Some(surface) = app.session.surface(surface_id) else { return }; | |
| if let Some(surface_id) = app.sidebar_plugin_surface { | |
| if let Some(surface) = app.session.surface(surface_id) { | |
| surface.take_dirty(); | |
| let theme = app.config.theme; | |
| let rs = app | |
| .render_states | |
| .entry(surface_id) | |
| .or_insert_with(|| ghostty_vt::RenderState::new().expect("render state alloc")); | |
| if surface.snapshot(rs).is_ok() { | |
| rs.set_clean(); | |
| let _ = | |
| super::terminal_grid::draw_render_state(frame, content, rs, &theme, |_, _| false); | |
| { | |
| let buf = frame.buffer_mut(); | |
| for y in 0..height { | |
| buf[(border_x, y)].set_symbol("│").set_style(border_style); | |
| } | |
| } | |
| return; | |
| } | |
| } | |
| } |
🤖 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 `@mux/crates/mux-tui/src/ui/sidebar.rs` around lines 66 - 67, The sidebar
rendering path in sidebar.rs is returning too early when
app.sidebar_plugin_surface is set but app.session.surface(surface_id) is
missing, which prevents the fallback error message from being shown. Update the
sidebar rendering logic around the surface lookup to avoid the let-else early
return and instead fall through to the existing unavailable/error-message
branch, using nested if-let or equivalent control flow so the sidebar can
display the configured “plugin unavailable” message.
Every additive command batch (wait-for/run/notify verbs, WM ops, server-control) kept protocol 6 per spec convention; the bump broke the conformance fixtures that pin protocol 6 (bindings-e2e).
| } | ||
|
|
||
| fn sync_sidebar_plugin(&mut self, relaunch: bool) { | ||
| if self.config.sidebar.plugin.is_none() || self.sidebar_width < 3 || !self.sidebar_visible { |
There was a problem hiding this comment.
This local config check still stops remote plugin discovery before the client asks the server for its sidebar surface. When an attached client has no local sidebar.plugin entry but the server does, this branch clears sidebar_plugin_surface and returns before sending sidebar-plugin. The sidebar then stays on the built-in workspace view even though the server-owned plugin is configured and available.
| .command | ||
| .unwrap_or_default() | ||
| .into_iter() | ||
| .filter(|arg| !arg.is_empty()) |
There was a problem hiding this comment.
This still removes valid empty string arguments from sidebar.plugin.command before the command reaches the spawn path. A configured command like [/path/plugin, "", --flag] is a valid argv shape, and the later surface spawn code only checks whether the whole vector is empty. Dropping the empty positional argument shifts the remaining arguments, so the plugin can receive a different command line than the user configured.
…ened smoke, docs Revert protocol to 6 everywhere (additive command; bump broke the published SDKs' version checks and the conformance fixtures). Clicking outside the sidebar plugin returns keyboard focus to the panes. Smoke plugin now awk-prefixes forwarded input (only real PTY forwarding can produce the asserted string) and verifies prefix-S returns focus to the pane. configuration.md/keyboard.md document sidebar.plugin.* and focus-sidebar; spec notes sidebar-plugin has no CLI verb (client-internal).
| fn sync_sidebar_plugin(&mut self, relaunch: bool) { | ||
| if self.config.sidebar.plugin.is_none() || self.sidebar_width < 3 || !self.sidebar_visible { | ||
| self.sidebar_plugin_surface = None; | ||
| self.sidebar_plugin_error = None; | ||
| self.sidebar_plugin_retry_after_ms = None; | ||
| self.sidebar_focused = false; | ||
| return; | ||
| } | ||
| let rect = self.sidebar_plugin_rect(); | ||
| if rect.width == 0 || rect.height == 0 { |
There was a problem hiding this comment.
This branch still uses the attach client's local mux.json to decide whether to ask the server for the sidebar plugin. When a remote client connects without a local sidebar.plugin entry, but the mux server has one configured, this returns early, clears sidebar_plugin_surface, and never sends the sidebar-plugin request. The attached client keeps showing the built-in sidebar instead of the server-owned plugin.
| pub fn draw(app: &mut App, frame: &mut Frame) { | ||
| if app.config.sidebar.plugin.is_some() { | ||
| draw_plugin(app, frame); | ||
| return; | ||
| } | ||
| draw_builtin(app, frame); |
There was a problem hiding this comment.
This render choice is still based on the attach client's local config, but the plugin surface comes from the server. A remote client without a local sidebar.plugin entry always calls draw_builtin(), so a valid server-returned plugin surface is never rendered. Keying this path off the discovered plugin surface or server status would keep remote attach clients from falling back to the built-in sidebar incorrectly.
| let command = plugin | ||
| .command | ||
| .unwrap_or_default() | ||
| .into_iter() | ||
| .filter(|arg| !arg.is_empty()) | ||
| .collect::<Vec<_>>(); |
There was a problem hiding this comment.
This still removes valid empty string arguments from sidebar.plugin.command. A configured command like ["/path/plugin", "", "--flag"] is a valid argv shape, but this rewrites it to ["/path/plugin", "--flag"] before spawn_sidebar_plugin_surface passes the vector to the child process. That shifts positional arguments and can make the plugin receive a different command line than the user configured.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 43fcbf8. Configure here.
| .and_then(serde_json::Value::as_str) | ||
| .map(str::to_string), | ||
| retry_after_ms: data.get("retry_after_ms").and_then(serde_json::Value::as_u64), | ||
| } |
There was a problem hiding this comment.
Stale sidebar id after attach
Medium Severity
On a remote session, when sidebar-plugin returns a surface id but attach-surface fails, the client still stores that id. The plugin drawer then exits early because no mirror exists, so the sidebar stays blank even when the RPC included an error.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 43fcbf8. Configure here.
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 (3)
mux/crates/mux-tui/src/app.rs (3)
1870-1908: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
pasteandpaste_sidebarduplicate the bracketed-paste encoding.The bracketed-paste-mode check and byte assembly (
\x1b[200~...\x1b[201~) is copy-pasted verbatim betweenpasteandpaste_sidebar. Same pattern asforward_key/forward_sidebar_keyabove — extract a sharedwrite_pasted_text(surface: &SurfaceHandle, text: &str)helper so the two entrypoints can't silently diverge.♻️ Proposed extraction
+ fn write_pasted_text(surface: &SurfaceHandle, text: &str) { + let Some(bracketed) = surface.with_terminal(|t| t.mode(2004, false)) else { + return; + }; + if bracketed { + let mut bytes = Vec::with_capacity(text.len() + 12); + bytes.extend_from_slice(b"\x1b[200~"); + bytes.extend_from_slice(text.as_bytes()); + bytes.extend_from_slice(b"\x1b[201~"); + surface.write_bytes(&bytes); + } else { + surface.write_bytes(text.as_bytes()); + } + } + fn paste_sidebar(&mut self, text: &str) { let Some(surface) = self.sidebar_surface_handle() else { return }; - let Some(bracketed) = surface.with_terminal(|t| t.mode(2004, false)) else { - return; - }; - if bracketed { - let mut bytes = Vec::with_capacity(text.len() + 12); - bytes.extend_from_slice(b"\x1b[200~"); - bytes.extend_from_slice(text.as_bytes()); - bytes.extend_from_slice(b"\x1b[201~"); - surface.write_bytes(&bytes); - } else { - surface.write_bytes(text.as_bytes()); - } + Self::write_pasted_text(&surface, text); }🤖 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 `@mux/crates/mux-tui/src/app.rs` around lines 1870 - 1908, `paste` and `paste_sidebar` duplicate the same bracketed-paste handling logic, so refactor the shared `mode(2004, false)` check and `\x1b[200~...\x1b[201~` byte assembly into a common helper such as `write_pasted_text` that accepts a surface handle and text. Update both `paste` and `paste_sidebar` to call the helper, matching the existing `forward_key`/`forward_sidebar_key` pattern so the two paths stay in sync.Source: Coding guidelines
1343-1375: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
handle_prefixed's FocusSidebar special-case is convoluted.Clearing
sidebar_focusedfor every action and then guarding withwas_sidebar_focused && action == Action::FocusSidebarto suppress the laterrun_actioncall works, but it's easy to break with future edits (any new early-return inserted between the clear and the guard would silently change behavior). HandlingAction::FocusSidebarfirst, before the generic clear, removes the need forwas_sidebar_focusedentirely.♻️ Proposed simplification
let Some(action) = self.config.keys.action_for(&key) else { if self.sidebar_focused { self.sidebar_focused = false; } return Ok(RenderAction::Draw); // unknown prefix command: swallow, redraw indicator }; - let was_sidebar_focused = self.sidebar_focused; + if action == Action::FocusSidebar { + self.toggle_sidebar_focus(); + return Ok(RenderAction::Draw); + } if self.sidebar_focused { self.sidebar_focused = false; } - if was_sidebar_focused && action == Action::FocusSidebar { - return Ok(RenderAction::Draw); - } if browser_only_action(action)🤖 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 `@mux/crates/mux-tui/src/app.rs` around lines 1343 - 1375, The `handle_prefixed` logic around `Action::FocusSidebar` is overly fragile because it clears `sidebar_focused` generically and then relies on `was_sidebar_focused` to suppress `run_action`. Update `handle_prefixed` in `app.rs` to handle `Action::FocusSidebar` as a dedicated early case before the blanket sidebar state reset, so the focused-sidebar toggle is explicit and `was_sidebar_focused` is no longer needed. Keep the existing prefix forwarding and browser-only action behavior unchanged.
1784-1821: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
forward_keyandforward_sidebar_keyduplicate the encode/write body.Everything from
self.encode_buf.clear();throughsurface.write_bytes(&self.encode_buf);is identical between the two functions, differing only in howsurfaceis obtained (active_surface_handle()vssidebar_surface_handle()). Extract a shared helper taking the resolvedSurfaceHandleto avoid future divergence between the two paths. As per coding guidelines, "implement one shared action/model path and verify every entrypoint that should invoke it... Do not patch one surface while leaving the others with duplicated logic."♻️ Proposed extraction
+ fn encode_and_write(&mut self, key: &KeyEvent, surface: &SurfaceHandle) { + let Some(input) = keys::key_input_from(key) else { return }; + self.encode_buf.clear(); + let _ = surface.scroll_to_bottom(); + let Some(encoded) = surface.with_terminal(|term| { + self.encoder.sync_from_terminal(term); + self.encoder.encode(&input, &mut self.encode_buf) + }) else { + return; + }; + if encoded.is_ok() && !self.encode_buf.is_empty() { + surface.write_bytes(&self.encode_buf); + } + } + fn forward_key(&mut self, key: &KeyEvent) { if self .active_surface_handle() .is_some_and(|surface| surface.kind() == SurfaceKind::Browser) { self.forward_browser_key(key); return; } - let Some(input) = keys::key_input_from(key) else { return }; let Some(surface) = self.active_surface_handle() else { return }; - self.encode_buf.clear(); - let _ = surface.scroll_to_bottom(); - let Some(encoded) = surface.with_terminal(|term| { - self.encoder.sync_from_terminal(term); - self.encoder.encode(&input, &mut self.encode_buf) - }) else { - return; - }; - if encoded.is_ok() && !self.encode_buf.is_empty() { - surface.write_bytes(&self.encode_buf); - } + self.encode_and_write(key, &surface); } fn forward_sidebar_key(&mut self, key: &KeyEvent) { - let Some(input) = keys::key_input_from(key) else { return }; let Some(surface) = self.sidebar_surface_handle() else { return }; - self.encode_buf.clear(); - let _ = surface.scroll_to_bottom(); - let Some(encoded) = surface.with_terminal(|term| { - self.encoder.sync_from_terminal(term); - self.encoder.encode(&input, &mut self.encode_buf) - }) else { - return; - }; - if encoded.is_ok() && !self.encode_buf.is_empty() { - surface.write_bytes(&self.encode_buf); - } + self.encode_and_write(key, &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 `@mux/crates/mux-tui/src/app.rs` around lines 1784 - 1821, The key forwarding logic in forward_key and forward_sidebar_key is duplicated after the surface lookup, so extract the shared encode/write flow into a helper that accepts a resolved SurfaceHandle. Keep forward_key and forward_sidebar_key focused on selecting the correct surface (active_surface_handle vs sidebar_surface_handle) and then delegate the common self.encode_buf.clear(), scroll_to_bottom, with_terminal, encoder.encode, and write_bytes sequence to the shared helper to prevent future divergence.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 `@mux/crates/mux-tui/src/app.rs`:
- Around line 1870-1908: `paste` and `paste_sidebar` duplicate the same
bracketed-paste handling logic, so refactor the shared `mode(2004, false)` check
and `\x1b[200~...\x1b[201~` byte assembly into a common helper such as
`write_pasted_text` that accepts a surface handle and text. Update both `paste`
and `paste_sidebar` to call the helper, matching the existing
`forward_key`/`forward_sidebar_key` pattern so the two paths stay in sync.
- Around line 1343-1375: The `handle_prefixed` logic around
`Action::FocusSidebar` is overly fragile because it clears `sidebar_focused`
generically and then relies on `was_sidebar_focused` to suppress `run_action`.
Update `handle_prefixed` in `app.rs` to handle `Action::FocusSidebar` as a
dedicated early case before the blanket sidebar state reset, so the
focused-sidebar toggle is explicit and `was_sidebar_focused` is no longer
needed. Keep the existing prefix forwarding and browser-only action behavior
unchanged.
- Around line 1784-1821: The key forwarding logic in forward_key and
forward_sidebar_key is duplicated after the surface lookup, so extract the
shared encode/write flow into a helper that accepts a resolved SurfaceHandle.
Keep forward_key and forward_sidebar_key focused on selecting the correct
surface (active_surface_handle vs sidebar_surface_handle) and then delegate the
common self.encode_buf.clear(), scroll_to_bottom, with_terminal, encoder.encode,
and write_bytes sequence to the shared helper to prevent future divergence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 748a6fbb-9382-48e3-9f18-5c742b81c766
📒 Files selected for processing (9)
mux/crates/mux-core/src/server.rsmux/crates/mux-tui/src/app.rsmux/crates/mux-tui/src/session/remote.rsmux/docs/configuration.mdmux/docs/keyboard.mdmux/scripts/smoke-tui.pymux/spec/README.mdmux/spec/cli.mdmux/spec/commands.md
This branch was successfully deployed
1 participant
Implements the mux side of the sidebar-plugin contract (reference plugin: https://github.com/manaflow-ai/cmux-sidebar-fzf, already live-tested against the control socket).
A sidebar plugin is an ordinary TUI program: mux runs it in a PTY sized to the sidebar and renders it via the existing ghostty-vt pipeline (shared ui/terminal_grid.rs, factored from the pane renderer — no second VT path). Env contract: CMUX_MUX_SOCKET + CMUX_SIDEBAR=1; resize is plain SIGWINCH. prefix S focuses the sidebar; keys forward verbatim, the global prefix escapes back. Plugins survive sidebar hide/show, restart with bounded backoff on exit, and die with the server. Attach clients get the plugin surface via a new additive protocol command. Built-in sidebar is untouched when no plugin is configured.
spec/plugins.md documents the contract incl. the cmux-plugin.toml manifest for the follow-up plugin-manager round. smoke-tui.py now boots a trivial echo plugin, focuses it, types, asserts the echo renders, then reload-configs back to the built-in sidebar.
Local compile fully blocked (host ghostty zig issue) — CI is the compile gate.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Spawns and manages an extra server-owned PTY with new input-focus routing and attach-client behavior; impact is mostly scoped to sessions that enable
sidebar.plugin, but failures or focus bugs could affect the whole TUI frame.Overview
Adds an optional sidebar plugin path: when
sidebar.pluginis set inmux.json, the mux server runs that command in a dedicated PTY (withCMUX_SIDEBAR=1and normal resize), instead of the built-in workspace list.Mux core gains
configure_sidebar_plugin/ensure_sidebar_pluginwith exponential backoff on start/exit failures; plugin surfaces are kept outside the pane tree and reaped specially on exit. The control socket exposes an additivesidebar-plugincommand (protocol remains v6) returning surface id, error, andretry_after_ms.TUI syncs plugin size each frame, draws it through new shared
ui/terminal_grid(VT painting factored out of pane rendering), and addsfocus-sidebar(prefix S) plus click-to-focus with keys/paste forwarded to the plugin while focused; prefix still escapes back to panes. Attached clients callsidebar-pluginover RPC and attach the returned surface.Config reload applies plugin changes; smoke tests cover plugin render, focus/echo, and fallback to the built-in sidebar.
spec/plugins.mddocuments the contract.Reviewed by Cursor Bugbot for commit 43fcbf8. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Adds a pluggable sidebar that runs an external TUI in a server-owned PTY and renders it via a shared VT grid. Introduces an additive
sidebar-pluginsocket command (protocol stays v6); clicking outside the plugin now returns focus to panes.New Features
CMUX_MUX_SOCKETandCMUX_SIDEBAR=1; survives hide/show; restarts with bounded backoff; exits on shutdown or config change.prefix Sor click; keys and paste forward verbatim while focused;prefixescapes; click outside unfocuses; drawing uses sharedui/terminal_grid.sidebar.plugininmux.jsonwithcommandand optionalcwd;reload-configapplies; when unset, the built‑in sidebar is used.sidebar-plugincommand returns{surface,error,retry_after_ms}; clients render viaattach-surface; docs/spec/tests updated (no CLI verb); protocol remains v6.Migration
sidebar.plugin.commandin~/.config/cmux/mux.json(e.g.["/path/to/plugin"]), thenreload-configor restart the server.sidebar-pluginto obtain the server-owned surface before attaching.Written for commit 43fcbf8. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation