Fix repeated cmux themes set reloads - #4359
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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:
📝 WalkthroughWalkthroughCLI threads socketPath/explicitPassword through themes commands and attempts a socket "reload_config" RPC (with optional auth) before falling back to distributed notification. Appearance store accepts an injected apply hook and skips terminal-theme synchronization during the initial reload. Tests and socket-request instrumentation added. ChangesSocket-based theme reload for successive theme changes
Appearance startup gating and injection
Sequence DiagramsequenceDiagram
participant CLI as cmux CLI
participant runThemesSet as runThemesSet
participant reloadSupport as reloadThemesIfPossible
participant socketHelper as requestThemeReloadOverSocket
participant Socket as UnixSocket
participant App as Running App
CLI->>runThemesSet: themes set <name>
runThemesSet->>runThemesSet: write theme to config
runThemesSet->>reloadSupport: socketPath, explicitPassword
reloadSupport->>socketHelper: socketPath, explicitPassword
socketHelper->>Socket: connect
alt socket authenticates
Socket->>Socket: authenticate
end
Socket->>App: "reload_config"
App-->>Socket: success
Socket-->>socketHelper: success
socketHelper-->>reloadSupport: true
reloadSupport-->>runThemesSet: ThemeReloadStatus(requested: bundleId)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (4 errors, 1 warning, 1 inconclusive)
✅ Passed checks (11 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 |
…s-set-state-dependent # Conflicts: # cmuxTests/CMUXCLIErrorOutputRegressionTests.swift
Greptile SummaryThis PR fixes repeated and flickery cmux theme-change behavior by routing
Confidence Score: 5/5Safe to merge; the theme reload pipeline is well-covered by new behavior-level regression tests and the changed paths are all guarded by the new reentrancy checks. All issues found are style and architecture observations — the debounce using asyncAfter, dead parameters in the synchronization decision helper, and the depth-counter approach to reentrancy. None introduce incorrect behavior on the changed paths, and the regression test suite directly exercises the flicker path, repeated theme writes, signal-cancel handling, and stale-bundle-ID targeting. Sources/AppDelegate.swift and Sources/GhosttyTerminalView.swift contain the new debounce and reentrancy-guard logic; a follow-on task to migrate these to structured Swift concurrency would reduce the maintenance surface. Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI as cmux CLI
participant DN as DistributedNotificationCenter
participant AD as AppDelegate
participant GA as GhosttyApp
participant S as GhosttySurface
CLI->>CLI: writeManagedThemeOverride()
CLI->>DN: "post(cmuxThemesReload, phase=final, bundleId, socketPath)"
alt legacy / preview reload
DN-->>AD: "handleThemesReloadNotification(phase=preview)"
AD->>AD: debounce 180ms asyncAfter + DispatchWorkItem
AD->>GA: "reloadConfiguration(source=preview)"
else final reload
DN-->>AD: "handleThemesReloadNotification(phase=final)"
AD->>AD: cancel pending workItem immediately
AD->>GA: "reloadConfiguration(source=final)"
end
GA->>GA: "guard reloadConfigurationDepth == 0"
GA->>GA: runtimeColorSchemeForConfigLoad() stable scheme for single-theme
GA->>GA: synchronizeGhosttyRuntimeColorScheme(loadScheme)
GA->>GA: updateDefaultBackground effectiveTerminalColorSchemePreference
GA->>GA: synchronizeGhosttyRuntimeColorScheme(resolvedScheme)
GA->>GA: ghostty_app_update_config()
GA->>AD: scheduleSurfaceRefreshAfterConfigurationReload(preferredColorScheme)
AD->>S: reapplySurfaceColorSchemeAfterGhosttyConfigReload(preferred)
AD->>S: reloadSurfaceConfiguration(soft, source)
AD->>S: refreshHostBackgroundAfterGhosttyConfigReload()
Reviews (22): Last reviewed commit: "fix: preserve right sidebar remembered m..." | 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.
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 `@CLI/CMUXCLI`+ThemeSupport.swift:
- Around line 331-343: The code currently calls client.close() both via the
defer { client.close() } placed immediately after try client.connect() and again
inside the catch block, risking a double-close; remove the explicit
client.close() from the catch branch and rely on the defer to always close the
client after connect; keep the defer where it is and leave error handling in the
catch (return false) as-is, referencing client.connect(), defer { client.close()
}, authenticateClientIfNeeded(...), client.send(command:), and the catch block
to locate the change.
🪄 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: 04622fd3-c05f-4f79-b3d0-5a8253950f71
📒 Files selected for processing (4)
CLI/CMUXCLI+ThemeSupport.swiftCLI/CMUXCLI+Themes.swiftCLI/cmux.swiftcmuxTests/CMUXCLIErrorOutputRegressionTests.swift
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 4 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
44c2c5f to
935c7c9
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
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 `@CLI/CMUXCLI`+Themes.swift:
- Line 134: Replace error.localizedDescription with String(describing: error)
when constructing the CLIError in the interactive theme picker throw site;
update the throw CLIError(message: "Failed to launch interactive theme picker:
\(error.localizedDescription)") to use String(describing: error) so the CLIError
includes the full diagnostic for non-LocalizedError types (reference the throw
site that constructs CLIError in CMUXCLI+Themes.swift).
🪄 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: eb815632-5f46-437e-b667-6f92bbaebeaf
📒 Files selected for processing (3)
CLI/CMUXCLI+Themes.swiftSources/AppDelegate.swiftcmuxTests/CMUXCLIErrorOutputRegressionTests.swift
…s-set-state-dependent # Conflicts: # Sources/KeyboardShortcutSettingsFileStore.swift # cmuxTests/KeyboardShortcutSettingsFileStoreStartupTests.swift
Addressed in 415c1a0 by switching the CLI error formatting to String(describing: error). CodeRabbit's latest status for the head commit is passing.
|
Updated with the working fix in two commits:
What changed:
Dogfood:
Tests:
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes 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 452745b. Configure here.
|
▎ Thanks for the flicker fix and the regression tests — solid work. |
This branch was successfully deployed
2 participants
Summary
cmux themes set,cmux themes clear, and the bare interactivecmux themespicker now request a targeted cmux theme reload withphase=finalafter writing the managed Ghostty config.socket.reload_configfor theme writes, avoiding the extra dark/light load pass that caused3024 Dayto flicker.3024 Day, cmux preserves the resolved terminal color scheme during the reload load phase instead of briefly switching through app appearance.Root Cause
The previous reload path mixed two different mechanisms: cmux theme-specific distributed reloads and generic Ghostty
reload_configsocket reloads. For3024 Day, the generic reload could make the load phase resolve against app appearance before the terminal theme scheme settled, so the surface pulsed between dark and light.A later guard also skipped runtime color-scheme synchronization when the requested light/dark scheme matched the already-applied scheme. That avoided one reentrant crash path, but it also meant repeated theme changes could stop repainting after the first change. The guard now skips only active reentrant setter calls.
Verification
./scripts/reload.sh --tag fix-day3024-flicker.CMUX_TAG=fix-day3024-flicker scripts/cmux-debug-cli.sh --json themes set '3024 Night'andCMUX_TAG=fix-day3024-flicker scripts/cmux-debug-cli.sh --json themes set '3024 Day'.3024 Night/3024 Daychanges still apply after the first change.3024 Daywhile already light path logsdistributed.cmux.themes.final, usesload scheme=light, and does not emitsocket.reload_configoralready_applied.Note
Medium Risk
Touches theme reload plumbing across the CLI and app runtime color-scheme/config reload paths; mistakes could cause missed reloads, flicker, or incorrect appearance updates, but changes are localized and heavily covered by new regression tests.
Overview
Theme reload flow is reworked to stop repeated/flickering reloads. The CLI now posts a distributed reload notification after
themes set/clearand after the interactive picker exits, includingsocketPathandphase=final, and chooses the target bundle id from the socket name (tagged debug/nightly/staging supported).The app now interprets theme reload phases and stabilizes color-scheme updates.
AppDelegatefilters reload notifications by bundle id, debounces legacy/preview reloads but applies final immediately (cancelling pending previews), and refreshes surfaces by reapplying the surface color scheme plus passing apreferredColorSchemethrough config reload paths to avoid transient dark/light flips on single-theme values.UI/appearance readability is improved. Adds contrast-based foreground color helpers and uses them for sidebar selected/workspace rows, titlebar controls, and panel foregrounds;
WindowAppearanceSnapshotnow derives a chrome/sidebars content scheme from the composited terminal background and can add a subtle sidebar contrast overlay when backdrops are unified.Safety + coverage. Prevents socket-path overrides when an inherited
CMUX_BUNDLE_IDconflicts with the current bundle, and adds extensive regression tests for theme reload notifications/phases, interactive picker behavior, runtime scheme decisions, contrast helpers, and process timeout handling.Reviewed by Cursor Bugbot for commit aac8005. Bugbot is set up for automated code reviews on this repo. Configure here.