Repository navigation
Fix cmux-tui startup config latency - #9738
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:
📝 WalkthroughWalkthroughGhostty defaults now load directly from readable configuration files. The loader supports recursive includes, cycle prevention, overlay ordering, conditional themes, and resource theme lookup. Unix tests verify startup behavior without external resolver execution. ChangesGhostty configuration loading
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Startup
participant GhosttyConfig
participant ThemeFile
Startup->>GhosttyConfig: Load first readable config file
GhosttyConfig->>GhosttyConfig: Parse includes and select conditional theme
GhosttyConfig->>ThemeFile: Read selected theme
ThemeFile-->>Startup: Return foreground, background, and palette values
🚥 Pre-merge checks | ✅ 24 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (24 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmux-tui/crates/cmux-tui/src/config.rs`:
- Around line 3224-3227: Update the test setup around the environment-variable
saves and the load call to also save, remove, and restore CMUX_TUI_CONFIG
alongside CMUX_MUX_CONFIG and the other process-global variables, ensuring load
uses the temporary Ghostty configuration path.
🪄 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: 57817b3b-8e44-40d3-99e4-c5763a4946f4
📒 Files selected for processing (1)
cmux-tui/crates/cmux-tui/src/config.rs
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/config.rs (2)
2779-2810: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSelect the conditional Ghostty theme based on system appearance.
selected_conditional_ghostty_themeaccepts bothlightanddarkentries but always returnslight, so conditional themes are resolved incorrectly for dark-mode users. Use an authoritative system appearance source to select the matching entry, and fail closed when that source is unavailable. Update the Unix resolver test to cover both appearance values.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmux-tui/crates/cmux-tui/src/config.rs` around lines 2779 - 2810, Update selected_conditional_ghostty_theme to query the authoritative system appearance source and return the matching light or dark theme, rather than always returning light; return None when appearance detection is unavailable. Extend the Unix resolver test to cover both light- and dark-mode selections.Source: Coding guidelines
2633-2636: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winUse Ghostty’s resolved config for startup defaults.
ghostty_config_paths()is only a candidate list, butghostty_defaults()parses the first readable candidate file and stops. Ghostty can layer several config files and handlesconfig-fileincludes with later values winning, or it can choose a different value through dynamic theme selection. Use Ghostty’s resolved application defaults as the source of truth and apply the file-only fallback only when the resolver is unavailable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmux-tui/crates/cmux-tui/src/config.rs` around lines 2633 - 2636, Update the startup defaults logic surrounding ghostty_defaults() to use Ghostty’s resolved application defaults as the primary source, preserving layered config-file includes, later-value precedence, and dynamic theme selection. Only invoke the existing ghostty_config_paths()/parse_ghostty_defaults fallback when the resolver is unavailable.
🤖 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/config.rs`:
- Around line 2779-2810: Update selected_conditional_ghostty_theme to query the
authoritative system appearance source and return the matching light or dark
theme, rather than always returning light; return None when appearance detection
is unavailable. Extend the Unix resolver test to cover both light- and dark-mode
selections.
- Around line 2633-2636: Update the startup defaults logic surrounding
ghostty_defaults() to use Ghostty’s resolved application defaults as the primary
source, preserving layered config-file includes, later-value precedence, and
dynamic theme selection. Only invoke the existing
ghostty_config_paths()/parse_ghostty_defaults fallback when the resolver is
unavailable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 80f114af-338e-42b4-ae97-a4cd1cd2e85a
📒 Files selected for processing (1)
cmux-tui/crates/cmux-tui/src/config.rs
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/src/config.rs`:
- Around line 2713-2721: In the configuration-loading flow around
parse_ghostty_config_text and parse_ghostty_config_file, defer resolving the
root conditional theme until all included files have updated the effective
theme_mode, so an included window-theme = dark selects the dark root theme.
Preserve default overlays and add the regression test
load_applies_ghostty_config_file_after_root_and_respects_dark_theme_mode
covering a root conditional theme with an included mode.
- Around line 2633-2635: Update the Ghostty defaults loading around
parse_ghostty_defaults_from_path to discover Ghostty’s current ordered
config.ghostty candidates, including the com.mitchellh.幽灵ghostty location,
rather than only legacy config paths. Replace find_map’s first-match behavior
with ordered parsing and overlay of every readable candidate, preserving
later-file precedence before applying application defaults.
🪄 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: 5e3452a7-db94-4386-928c-5fd8f10b44f5
📒 Files selected for processing (1)
cmux-tui/crates/cmux-tui/src/config.rs
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmux-tui/crates/cmux-tui/src/config.rs (2)
2882-2889: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftProcess
config-filein order, not after the full file.
config-fileis parsed intoconfig_filesand applied after all lines in the current file, so settings after the directive cannot override included values. Ghostty processes included config at the directive’s position.Apply each include before parsing the remaining lines of the current file so later root keys still win.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmux-tui/crates/cmux-tui/src/config.rs` around lines 2882 - 2889, Update the config parsing flow around the config-file match and its surrounding loop so each GhosttyConfigFile include is parsed and applied immediately at the directive’s position, rather than queued in config_files for later application. Preserve input order so keys encountered after the include in the current file override included settings.
2874-2906: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDrive conditional theme selection from system appearance, not
window-theme.
theme = light:X,dark:Yselects the terminal content color palette by system appearance.window-themecontrols window chrome only, with values such aslightanddarkalready distinct from theme branch selection. This code writeswindow-themeinto the mode thatload_ghostty_themeuses for thelight:/dark:branches, sowindow-theme = lightcan load the dark terminal palette on a dark appearance. Keepwindow-themefor chrome resolution and use the detected system appearance for conditional palette selection.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmux-tui/crates/cmux-tui/src/config.rs` around lines 2874 - 2906, Update the theme-loading flow in the parsed Ghostty configuration so load_ghostty_theme selects light:/dark: branches using the detected system appearance, not theme_mode populated by parse_ghostty_window_theme. Preserve theme_mode for window chrome resolution and continue parsing window-theme independently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmux-tui/crates/cmux-tui/src/config.rs`:
- Around line 3685-3729: Add a regression test alongside ghostty configuration
parsing tests that creates two config files which include each other via
config-file, then invokes parse_ghostty_defaults_from_path or the relevant
parse_ghostty_config_file entry point. Assert parsing returns successfully and
that representative keys from both files are applied exactly once, covering the
loaded-set cycle guard without changing production logic.
- Around line 2762-2792: Document at the call site where
GhosttyThemeMode::detect is consumed that its result is a non-authoritative,
cosmetic hint used only to select terminal colors. Keep the detection logic
unchanged and place the clarification adjacent to the invocation so future
callers do not treat it as a correctness-critical decision.
---
Outside diff comments:
In `@cmux-tui/crates/cmux-tui/src/config.rs`:
- Around line 2882-2889: Update the config parsing flow around the config-file
match and its surrounding loop so each GhosttyConfigFile include is parsed and
applied immediately at the directive’s position, rather than queued in
config_files for later application. Preserve input order so keys encountered
after the include in the current file override included settings.
- Around line 2874-2906: Update the theme-loading flow in the parsed Ghostty
configuration so load_ghostty_theme selects light:/dark: branches using the
detected system appearance, not theme_mode populated by
parse_ghostty_window_theme. Preserve theme_mode for window chrome resolution and
continue parsing window-theme independently.
🪄 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: e1a77785-2c08-42e9-ae8a-c893d1eea588
📒 Files selected for processing (1)
cmux-tui/crates/cmux-tui/src/config.rs
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
0ab474f to
643c688
Compare
6fa249d to
ad52218
Compare
ad52218 to
cbc0df0
Compare
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 f04599d. Configure here.
Summary
cmux-tuistartup no longer runsghostty +show-configfromconfig::load. It reads the user Ghostty config file directly, parses the color/cursor defaults we already support, and keeps the resolver parser path covered by tests without invoking an external process on startup.The first commit adds a regression test that fails on main because
load()executes the configuredGHOSTTY_BIN. The second commit removes the startup subprocess path and makes that test pass.Incident Notes
Benchmark harness: isolated
UV_CACHE_DIR,UV_TOOL_DIR,UV_PYTHON_INSTALL_DIR,npm_config_cache, isolatedXDG_RUNTIME_DIR, isolated cmux state roots, PTY first-draw detection. Baseline was taken before editing on main. Current-head package validation used local debug-size packages built from this branch, so cold package numbers include package-manager install work but are not release artifact size forecasts.Baseline public package samples on main:
uvx cmux --helpcoldnpx cmux --helpcolduvx cmux --helpwarmnpx cmux --helpwarmuvx cmuxwarm first drawnpx cmuxwarm first drawCurrent-head local package samples:
uvx --from <wheel> cmux --helpcoldnpx --package file:<pkg> cmux --helpcolduvx --from <wheel> cmux --helpwarmnpx --package file:<pkg> cmux --helpwarmuvx --from <wheel> cmuxcold first drawnpx --package file:<pkg> cmuxcold first drawuvx --from <wheel> cmuxwarm first drawnpx --package file:<pkg> cmuxwarm first drawDecomposition:
uvxcold help was 2.4 s median, baseline registrynpxcold help was 0.9 s median. Local debug packages are not size-comparable to release packages, but they keep package-manager install work in the measured path.uvxlocal help is 52 ms median versus 33 ms direct console script; current warmnpxlocal help is 237 ms median versus 37 ms direct node shim.--helpstays around 7 ms median, with one scheduler outlier at 991 ms.config::load, before terminal recovery completed.uvx, 299 ms for localnpx, and 40 ms direct.Tests
CARGO_TARGET_DIR=/tmp/cmux-tui-startup-latency-target rustup run 1.94.0 cargo test -p cmux-tui ghostty -- --nocaptureCARGO_TARGET_DIR=/tmp/cmux-tui-startup-latency-target rustup run 1.94.0 cargo test -p cmux-tui load_uses_file_ghostty_defaults_without_invoking_external_resolver -- --nocaptureCARGO_TARGET_DIR=/tmp/cmux-tui-startup-latency-target rustup run 1.94.0 cargo build -p cmux-tui --bin cmux-tuiNeed help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Speed up
cmux-tuistartup by resolving Ghostty defaults with an in-binary, self-reexec helper instead of callingghostty +show-config. Warm launch is ~90 ms viauvx, ~300 ms vianpx, and ~40 ms direct.Bug Fixes
__ghostty-config-defaultshelper with a ~250 ms budget, bounded stdout drains, bounded descendant scan, and process‑group kill/reap; prints fg/bg/cursor/blink/palette.GHOSTTY_BIN); on helper miss, fall back to bounded file parsing; on timeout, skip fallback to keep startup bounded.~and optional?, deferred theme resolution, preserve resource themes, explicit overrides beat themes, first resolvable theme wins;cursor-style = block_hollow;window-themesupportslight/dark/system/auto; select light/dark from system appearance on macOS and Linux/BSD; skip probes whenwindow-themeis fixed.Dependencies
wait-timeoutto bound the helper process and reads.Written for commit b423d56. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Refactor
Note
Medium Risk
Large change to startup config resolution with subprocess timeouts, process-group kills, and desktop appearance probes; wrong defaults or hung children could affect every launch, but behavior is heavily tested and bounded.
Overview
Startup no longer shells out to
ghostty +show-config(orGHOSTTY_BIN) duringconfig::load. Terminal colors and cursor defaults come from a self-reexec path__ghostty-config-defaultsthat parses Ghostty config files inside the same binary, with a ~250 ms parse budget and ~500 ms parent wait viawait-timeout.The helper serializes resolved fg/bg/cursor/palette lines on stdout; the main process prefers that result, falls back to the same bounded file parser on failure, and on helper timeout skips file fallback so startup stays bounded. Process cleanup uses isolated process groups, capped stdout reads, optional
psdescendant scan, and scrubs provider secret env vars before any desktop theme probes.File parsing now matches more Ghostty behavior:
config-fileincludes (depth/file/byte limits), deferred theme resolution, first successful theme, light/dark conditional themes from system appearance (macOS / Linux desktop probes with short deadlines),cursor-style = block_hollow, and explicit overrides winning over themes.mainhandles the helper argv early; CLI tests assert no external resolver at startup and correct helper output.Reviewed by Cursor Bugbot for commit b423d56. Bugbot is set up for automated code reviews on this repo. Configure here.