Repository navigation
Decouple terminal resources from layout views - #9903
Closed
lawrencecchen wants to merge 71 commits into
Closed
lawrencecchen wants to merge 71 commits into
lawrencecchen wants to merge 71 commits into
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:
📝 WalkthroughWalkthroughTerminal PTY state now lives in persistent ChangesTerminal resource separation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant Mux
participant TerminalResource
participant PtySurface
Client->>Server: attach or resize terminal
Server->>Mux: resolve terminal resource and viewer
Mux->>TerminalResource: obtain runtime identity and compatibility surface
TerminalResource->>PtySurface: project updates to active placements
Mux-->>Server: return sizing or attachment result
Server-->>Client: publish terminal state
Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 1 warning, 1 inconclusive)
✅ Passed checks (21 passed)
✨ Finishing Touches 💡 1📝 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
lawrencecchen
force-pushed
the
feat-pty-resource-graph
branch
2 times, most recently
from
August 10, 2026 06:01
c1ce4b0 to
cf9aa86
Compare
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
cmux-tui/crates/cmux-tui-core/src/mux.rs (4)
8982-9006: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winNon-terminal surfaces no longer receive a redraw event after a default-color change.
unique_surface_runtimesreturns one compatibility surface per terminal resource plus every non-terminal surface (Line 14284). All three loops applyset_default_colorsto every returned surface, including browser and sidebar-plugin surfaces. The emit is now guarded bysurface.terminal_event_target(), which isNonefor a non-terminal surface. Those surfaces change their default colors and never emitSurfaceOutput, so the frontend does not repaint them until unrelated output arrives.Emit a placement-scoped output event when the surface has no terminal event target.
🐛 Proposed fix for all three loops
for surface in surfaces { surface.set_default_colors(colors); - if let Some(target) = surface.terminal_event_target() { - self.emit_terminal_output(target); - } + match surface.terminal_event_target() { + Some(target) => self.emit_terminal_output(target), + None => self.emit(MuxEvent::SurfaceOutput(surface.id)), + } }Also applies to: 9057-9066
🤖 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 8982 - 9006, Update all three default-color propagation loops, including seed_default_colors_if_no_durable_override, to emit a placement-scoped output event when surface.terminal_event_target() returns None, while preserving the existing terminal-target emission path. Ensure terminal, browser, and sidebar-plugin surfaces receive the appropriate redraw notification after set_default_colors.
14130-14163: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftMissing host-terminal-ID index forces full catalog scans with a per-entry
compatibility_surface()allocation.Stateindexes terminals byTerminalPublicIdand by runtimeSurfaceId, but not by the durable hostterminal_id. Every host lookup therefore walksstate.terminal_catalogand materializes anArc<Surface>per entry. The change adds that per-entry allocation to scans that previously only read the stored surface. Add aHashMap<String, TerminalPublicId>host index toState, maintain it next toterminal_catalog_by_runtime, and replace all three scans with a single lookup.
cmux-tui/crates/cmux-tui-core/src/mux.rs#L14130-L14163: replace the duplicate-host scan inregister_terminal_runtime_checkedwith one index lookup; this scan makes restoring N terminals O(N²).cmux-tui/crates/cmux-tui-core/src/mux.rs#L7957-L7970: replace thecatalog_terminal_by_hostscan with one index lookup, and drop the per-candidatereserved_in_process_terminalslock.cmux-tui/crates/cmux-tui-core/src/mux.rs#L6772-L6779: replace the legacy catalog fallback inclose_terminal_with_mutationwith the same index lookup.As per coding guidelines: "Avoid repeated full scans, sorting, filtering, or per-item nested scans over scalable collections in production code... When reporting a violation, identify the file and line, collection, estimated complexity, expected scale, and smallest source-of-truth fix."
🤖 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 14130 - 14163, In cmux-tui/crates/cmux-tui-core/src/mux.rs#L14130-L14163, add and maintain a State host-terminal-ID index (HashMap<String, TerminalPublicId>) alongside terminal_catalog_by_runtime, then replace the duplicate-host scan in register_terminal_runtime_checked with a single index lookup. In cmux-tui/crates/cmux-tui-core/src/mux.rs#L7957-L7970, update catalog_terminal_by_host to use the same index and remove the per-candidate reserved_in_process_terminals lock; in cmux-tui/crates/cmux-tui-core/src/mux.rs#L6772-L6779, replace the legacy catalog fallback in close_terminal_with_mutation with that index lookup.Source: Coding guidelines
9118-9128: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not fail the resize after the PTY has already been resized.
Line 9109 applies the resize and Line 9122 then returns
Errwhenterminal_event_target()isNone. Callers treat thatErras a failed resize and roll back their sizing bookkeeping (Line 5856 and Line 5911), while the PTY keeps the new geometry. The recorded client size and the real PTY size then disagree.Resolve the event target before the resize, or degrade to skipping the event instead of returning an error.
The error text also names an internal concept. As per coding guidelines: "Do not expose internal provider names, provider-specific flags, templates, snapshots, manifests, environment variable names, database or migration details" and "User-facing errors should state what happened in product terms".
🐛 Proposed fix
let (cols, rows) = surface.size(); - self.emit_terminal_resized( - surface.terminal_event_target().context("PTY omitted its event target")?, - cols, - rows, - None, - ); + if let Some(target) = surface.terminal_event_target() { + self.emit_terminal_resized(target, cols, rows, None); + } else { + self.emit(MuxEvent::SurfaceResized { + surface: surface.id, + cols, + rows, + reservation_id: None, + }); + } Ok((true, None))🤖 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 9118 - 9128, Update the resize flow around the PTY resize operation and terminal_event_target() so a missing event target cannot return an error after the PTY has been resized. Resolve the target before resizing, or skip emit_terminal_resized when unavailable while still returning successful resize status; replace the internal “PTY omitted its event target” message with product-facing wording if an error remains.Source: Coding guidelines
212-216: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winUse terminal runtime IDs consistently for Kitty budget entries.
reserve_kitty_image_surfacecurrently acceptsSurfaceId::RuntimeId, but callers passnext_terminal_runtime_id(&mux, id), whose generated runtime IDs may not reuse existing terminal runtime identities.kitty_image_limits_for_reconnectandreconcile_reconnected_kitty_image_surfaceboth load budget entries by the currentterminal.runtime_id(), so reconnect can miss an existing reservation and the weak reference will not bind againstentry.surface. Keep the established terminal runtime ID as the budget key, or align the reconnect/unregister lookup path to the reservation key.🤖 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 212 - 216, Use the established terminal runtime ID consistently for Kitty image budget reservations and lookups. Update reserve_kitty_image_surface and its callers to key entries with terminal.runtime_id(), or otherwise make kitty_image_limits_for_reconnect and reconcile_reconnected_kitty_image_surface use the exact reservation key; ensure reconnect finds the existing entry and binds its weak surface reference.
🤖 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 8745-8798: Add a concise comment at the wrapped reporter closure
in set_cell_pixel_size_reporting stating that callers must not invoke the
reporter while holding the mux state lock, since cell_pixel_report_targets
acquires self.state. Do not change the reporter behavior.
- Around line 5486-5489: Update the in-process terminal branch in
spawn_surface_with to replace the expect on surface.terminal_runtime_id() with
an error return when no runtime identity exists. Before returning the error,
preserve the existing terminal-exit persistence and surface-kill cleanup used by
other spawn failures, and only insert into reserved_in_process_terminals after
obtaining a valid runtime ID.
In `@cmux-tui/crates/cmux-tui-core/src/surface.rs`:
- Around line 2648-2650: Use the existing local event_target binding as the
authoritative value in both PtyTerminalRuntime constructors: at
cmux-tui/crates/cmux-tui-core/src/surface.rs#L2648-L2650 and `#L3592-L3594`, move
event_target into the runtime field instead of re-deriving it, preserving the
existing callback clone usage and eliminating the unnecessary clone.
- Around line 1927-1929: Change next_terminal_runtime_id to return
Option<SurfaceId> and remove the placement fallback, yielding None when
Weak<Mux> cannot upgrade. Update every call site to propagate failure with ?,
including the terminal spawn path, so spawning aborts when the mux is
unavailable and runtime IDs never reuse placement IDs.
---
Outside diff comments:
In `@cmux-tui/crates/cmux-tui-core/src/mux.rs`:
- Around line 8982-9006: Update all three default-color propagation loops,
including seed_default_colors_if_no_durable_override, to emit a placement-scoped
output event when surface.terminal_event_target() returns None, while preserving
the existing terminal-target emission path. Ensure terminal, browser, and
sidebar-plugin surfaces receive the appropriate redraw notification after
set_default_colors.
- Around line 14130-14163: In
cmux-tui/crates/cmux-tui-core/src/mux.rs#L14130-L14163, add and maintain a State
host-terminal-ID index (HashMap<String, TerminalPublicId>) alongside
terminal_catalog_by_runtime, then replace the duplicate-host scan in
register_terminal_runtime_checked with a single index lookup. In
cmux-tui/crates/cmux-tui-core/src/mux.rs#L7957-L7970, update
catalog_terminal_by_host to use the same index and remove the per-candidate
reserved_in_process_terminals lock; in
cmux-tui/crates/cmux-tui-core/src/mux.rs#L6772-L6779, replace the legacy catalog
fallback in close_terminal_with_mutation with that index lookup.
- Around line 9118-9128: Update the resize flow around the PTY resize operation
and terminal_event_target() so a missing event target cannot return an error
after the PTY has been resized. Resolve the target before resizing, or skip
emit_terminal_resized when unavailable while still returning successful resize
status; replace the internal “PTY omitted its event target” message with
product-facing wording if an error remains.
- Around line 212-216: Use the established terminal runtime ID consistently for
Kitty image budget reservations and lookups. Update reserve_kitty_image_surface
and its callers to key entries with terminal.runtime_id(), or otherwise make
kitty_image_limits_for_reconnect and reconcile_reconnected_kitty_image_surface
use the exact reservation key; ensure reconnect finds the existing entry and
binds its weak surface reference.
🪄 Autofix
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: 14d561c8-b7c3-4c6b-acf0-9901b218913e
📒 Files selected for processing (12)
cmux-tui/crates/cmux-tui-core/src/lib.rscmux-tui/crates/cmux-tui-core/src/model.rscmux-tui/crates/cmux-tui-core/src/mux.rscmux-tui/crates/cmux-tui-core/src/mux/public_projections.rscmux-tui/crates/cmux-tui-core/src/mux/resource_content.rscmux-tui/crates/cmux-tui-core/src/mux/resource_topology.rscmux-tui/crates/cmux-tui-core/src/resource_api.rscmux-tui/crates/cmux-tui-core/src/resource_router.rscmux-tui/crates/cmux-tui-core/src/resource_router/content.rscmux-tui/crates/cmux-tui-core/src/resource_selector.rscmux-tui/crates/cmux-tui-core/src/server.rscmux-tui/crates/cmux-tui-core/src/surface.rs
lawrencecchen
force-pushed
the
feat-pty-resource-graph
branch
from
August 10, 2026 06:21
cf9aa86 to
c5d393b
Compare
lawrencecchen
force-pushed
the
feat-pty-resource-graph
branch
2 times, most recently
from
August 10, 2026 07:09
d38e044 to
c4d6222
Compare
lawrencecchen
force-pushed
the
feat-pty-resource-graph
branch
from
August 10, 2026 07:14
c4d6222 to
c2d36b0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 8892-8916: Update project_cell_pixel_update to acquire self.state
once before processing all resizes and failures, then resolve each runtime or
failure surface’s report targets using that shared guard. Refactor or reuse the
lookup logic so cell_pixel_report_targets does not reacquire the lock per entry,
while preserving the existing projection and failure handling behavior.
- Around line 6300-6347: Move the shared viewer filtering and report-order
sorting into a ClientSizingState method, such as
terminal_client_viewers(runtime, client), preserving the existing ordering.
Update Mux::terminal_client_viewers to lock and delegate to that method, and
replace the duplicated latest computation in
set_terminal_client_size_participation with the delegated result’s last viewer
while retaining the existing lock ownership.
🪄 Autofix
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: 30466f6c-9e2e-478c-b85b-0ab9390804c8
📒 Files selected for processing (5)
cmux-tui/crates/cmux-tui-core/src/lib.rscmux-tui/crates/cmux-tui-core/src/mux.rscmux-tui/crates/cmux-tui-core/src/resource_selector.rscmux-tui/crates/cmux-tui-core/src/server.rscmux-tui/crates/cmux-tui/src/app.rs
lawrencecchen
force-pushed
the
feat-pty-resource-graph
branch
7 times, most recently
from
August 10, 2026 08:59
2245a6d to
8bcab24
Compare
lawrencecchen
force-pushed
the
feat-pty-resource-graph
branch
from
August 10, 2026 09:16
8bcab24 to
a80be0d
Compare
This comment has been minimized.
This comment has been minimized.
# Conflicts: # cmux-tui/crates/cmux-tui-core/src/server.rs
This was referenced Aug 27, 2026
lawrencecchen
added
the
stale-revisit
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
df06bf8168432f8eab35b14a0f9dc093fe74a86d, SHA-256ad6d8ed674745f774b5c1174246d52ecf488e15acb4eda3ee7169889403e2f2b. One terminal was projected into two workspaces, retained output appeared in both views, the resource accepted output with zero views, reprojection restored that output, and explicit close removed the resource.Migration seam
Legacy terminal-only code can request an unregistered Surface adapter. The adapter has no layout identity and does not own terminal lifetime. A later cleanup can move those method signatures to TerminalResource without changing this ownership model.
Note
High Risk
This is a large refactor of mux core state, terminal lifecycle, client sizing, Kitty budgets, and hosted exit/reconnect paths—areas where subtle regressions affect every terminal session and multiview layout.
Overview
Terminal ownership is split from layout views.
TerminalResourcenow holds the PTY, emulator state, and process lifetime; eachSurfaceis only a tab/view placement with its own viewport. Theterminal_catalogstoresArc<TerminalResource>instead of catalog surfaces, and closing the last tab detaches the view while the terminal can remain alive with zero projections.Runtime behavior routes through stable terminal identity. Terminal output, title, resize, scroll, and bell events use
TerminalEventTargetand fan out to every materialized view of that resource.emit_terminal_view_snapshotreplays retained state when a new view appears after output already arrived. Layout lookups usesurface(); sizing and host I/O useviewer_target_surface()so runtime IDs are not confused with placement IDs.Shared infrastructure is keyed by terminal runtime, not one selected tab. Kitty graphics budgets, cell-pixel updates, and geometry authority (
terminal_runtime_by_viewer,set_terminal_client_size_participation) operate on the resource and report or resize across all views. Exit and host reconnect paths accept callbacks while the resource is still pending publication (TerminalHostCallbackTarget::PendingvsLive), andterminal_resource_exitedcan finish cleanup without any live view.Legacy callers still get an unregistered
compatibility_surface()adapter from the catalog; agents, notifications, and close/detach semantics are updated so zero-view terminals remain addressable by public terminal ID or runtime ID until explicit close.Reviewed by Cursor Bugbot for commit df06bf8. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes