Repository navigation
Clean stale release theme overrides for channel builds - #4502
austinywang wants to merge 28 commits into
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:
📝 WalkthroughWalkthroughAdds a helper that removes stale release-managed theme override blocks when the active bundle identifier differs from the configured theme override bundle; the helper is invoked at the start of ChangesStale Theme Override Cleanup
Sequence Diagram(s)sequenceDiagram
participant CMUXCLI
participant removeStaleReleaseManagedThemeOverrideIfNeeded
participant ReleaseConfigFile
CMUXCLI->>removeStaleReleaseManagedThemeOverrideIfNeeded: invoke before write/clear
removeStaleReleaseManagedThemeOverrideIfNeeded->>ReleaseConfigFile: load release config (cmuxThemeOverrideBundleIdentifier)
removeStaleReleaseManagedThemeOverrideIfNeeded->>ReleaseConfigFile: strip managed themes block
removeStaleReleaseManagedThemeOverrideIfNeeded->>ReleaseConfigFile: delete file if empty or rewrite normalized contents
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 16 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (16 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 fixes stale
Confidence Score: 5/5Safe to merge — all cleanup paths are guarded and idempotent, error messages are properly sanitized and localized, and the surface color-scheme pre-apply correctly branches on main vs. background thread. The stale-block removal is idempotent and guarded by bundle-identifier checks at every entry point; no cleanup runs for the release bundle itself or bundles outside the allowed-fallback set. The localized error for cleanup failures sanitizes all NSError details. Thread handling in prepareTerminalSurfacesForConfigurationReload uses MainActor.assumeIsolated on the main thread and async dispatch off it, matching the pattern of the sibling scheduleSurfaceRefreshAfterConfigurationReload helper. Regression tests cover set, partial set, clear, picker, failure abort, and resolver edge cases at the process level. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI as cmux CLI
participant Resolver as CmuxGhosttyConfigPathResolver
participant FS as Filesystem
participant App as GhosttyApp
Note over CLI: themes set / clear / bare picker
CLI->>Resolver: removeStaleReleaseManagedThemeOverrideBeforeFallbackIfNeeded
Resolver->>FS: configURLs(nightly) → empty?
alt nightly has own config
Resolver-->>CLI: return (skip cleanup)
else nightly falls back to release
Resolver->>FS: read release config.ghostty
Resolver->>FS: strip managed block (or delete if empty)
Resolver-->>CLI: done
end
CLI->>FS: write managed theme override (nightly config)
Note over App: startup / reload
App->>Resolver: loadConfigURLs(nightly, [appSupportDirs])
alt nightly config exists
Resolver-->>App: [nightly config URL]
else fallback to release
Resolver->>FS: try? removeStaleReleaseManagedThemeOverrideIfNeeded
Resolver-->>App: [release config URL] (cleaned)
end
App->>App: loadResolvedCmuxThemePairOverrideIfNeeded
App->>App: prepareTerminalSurfacesForConfigurationReload
App->>App: ghostty_app_update_config
Reviews (21): Last reviewed commit: "Use multi-root cmux config loading in ap..." | 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}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?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 290-298: The catch-all in
removeStaleReleaseManagedThemeOverrideIfNeeded currently throws a generic
CLIError and discards the underlying error; update the generic catch to include
the original error details by constructing the CLIError message with
String(describing: error) (e.g., CLIError(message: "Unable to clean stale cmux
theme override: \(String(describing: error))")), leaving the existing catch-let
CLIError path unchanged and keeping the call to
removeStaleReleaseManagedThemeOverride() as-is.
🪄 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: dd094446-6e3c-4e5c-a78c-a09a655b7065
📒 Files selected for processing (1)
CLI/CMUXCLI+ThemeSupport.swift
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 296-300: The cleanup failure currently injects the raw underlying
error into the user-facing CLIError (see CLIError usage and the localized key
"cli.themes.error.cleanupFailed" and the local variable format), which can leak
internal filesystem/bundle details; change the thrown CLIError to only contain a
sanitized, non‑sensitive message (e.g., the localized string without
interpolating String(describing: error)). If you need diagnostic details, send
the raw error to a debug/internal logger (not to the CLIError) using the
existing logging facility instead of exposing it in the thrown error.
In `@Resources/Localizable.xcstrings`:
- Line 19: The localization entry for the key cli.themes.error.cleanupFailed in
Resources/Localizable.xcstrings contains copied English text for many
non-English locales; replace each non-English "Unable to clean stale cmux theme
override: %@" value with proper translations for that locale (or mark the locale
for proper translation workflow) so no locale slot contains copied English,
TODO, placeholder or machine markers; update the corresponding "stringUnit" ->
"value" for each affected locale (e.g., ja, fr, de, es, ko, zh-Hans, etc.)
ensuring the format specifier "%@" is preserved.
- Line 19: The localization entry cli.themes.error.cleanupFailed currently
includes an interpolated %@ which can leak internal details; remove the %@ from
the user-facing string so it becomes a fixed localized message (e.g., "Unable to
clean stale cmux theme override.") and ensure the calling code logs the raw
error details only to debug/diagnostic logs (not the user-facing message).
Update the localization key cli.themes.error.cleanupFailed to omit interpolation
and modify the cleanup code that emits this message to call
processLogger.debug/processLogger.errorWithDetails (or equivalent) with the raw
error while showing the fixed localized string to users.
🪄 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: 4b1c2e6a-7ccd-46dc-9f8a-56a62fe46938
📒 Files selected for processing (2)
CLI/CMUXCLI+ThemeSupport.swiftResources/Localizable.xcstrings
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Greptile SummaryThis PR fixes a regression where a stale
Confidence Score: 4/5Safe to merge once the two unlocalized CLIError messages are routed through String(localized:defaultValue:) with catalog entries. The stale-block cleanup logic is well-structured, idempotent, and thoroughly tested. The two new guard-exit CLIErrors — "Unable to resolve Application Support directory" — are raw English literals that bypass the localization system, surfacing verbatim in all locales, whereas cleanupFailed in the same PR was correctly localized across all 20 catalog locales. CLI/CMUXCLI+ThemeSupport.swift and CLI/CMUXCLI+Themes.swift — both contain the new unlocalized error messages that need catalog entries added to Resources/Localizable.xcstrings. Important Files Changed
Reviews (14): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
Greptile SummaryThis PR fixes stale release-channel
Confidence Score: 5/5Safe to merge; the bug fix is well-targeted, the cleanup logic is idempotent and covered by extensive process-level and unit regressions, and the previous deadlock surface in the reload path has been correctly replaced. The stale-block cleanup is idempotent, error-contained, and covered by four new process-level regressions plus three new unit tests that exercise strip, delete, leave-alone, abort-on-failure, and the interactive-picker path. The app-side silent swallow is intentional best-effort. The only finding is a pair of guard-failure error strings that follow an existing pre-PR convention and are in an essentially unreachable codepath on a working macOS system. No files require special attention; the two new non-localized guard-failure messages in Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[cmux themes set/clear/picker] --> B{Non-release\nbundle?}
B -- No --> E[Write/clear nightly config]
B -- Yes --> C{Nightly config\nalready exists?}
C -- Yes --> E
C -- No --> D[removeStaleReleaseManagedThemeOverrideBeforeFallbackIfNeeded]
D --> D1{Cleanup\nsucceeded?}
D1 -- No --> F[Abort: cli.themes.error.cleanupFailed]
D1 -- Yes --> E
E --> G{themes clear?}
G -- Yes --> H[clearManagedThemeOverride\nalso calls removeStaleRelease]
G -- No --> I[writeManagedThemeOverride]
J[App: loadDefaultConfigFiles] --> K{Non-release\nbundle?}
K -- No --> M[Load release config as-is]
K -- Yes --> L[removeStaleRelease best-effort\nstrip or delete release block]
L --> M
M --> N[loadResolvedCmuxThemePairOverrideIfNeeded]
N --> O[ghostty_app_update_config]
O --> P[prepareTerminalSurfacesForConfigurationReload]
Reviews (16): Last reviewed commit: "Wire resolved scheme for paired cmux con..." | Re-trigger Greptile |
| private func removeStaleReleaseManagedThemeOverride(activeBundleIdentifier: String?) throws { | ||
| guard let activeBundleIdentifier = (activeBundleIdentifier ?? currentCmuxAppBundleIdentifier())? | ||
| .trimmingCharacters(in: .whitespacesAndNewlines), | ||
| !activeBundleIdentifier.isEmpty, | ||
| activeBundleIdentifier != Self.cmuxThemeOverrideBundleIdentifier else { | ||
| return | ||
| } | ||
|
|
||
| guard let appSupport = FileManager.default.urls(for: .applicationSupportDirectory, in: .userDomainMask).first else { | ||
| throw CLIError(message: "Unable to resolve Application Support directory") | ||
| } |
There was a problem hiding this comment.
"Unable to resolve Application Support directory" is thrown as a CLIError directly, which causes it to be re-thrown unchanged in removeStaleReleaseManagedThemeOverrideIfNeeded (via the catch let error as CLIError { throw error } branch), completely bypassing the localized cli.themes.error.cleanupFailed wrapper. The raw string reaches the user un-localized and exposes an internal macOS directory name as an implementation detail.
| private func removeStaleReleaseManagedThemeOverride(activeBundleIdentifier: String?) throws { | |
| guard let activeBundleIdentifier = (activeBundleIdentifier ?? currentCmuxAppBundleIdentifier())? | |
| .trimmingCharacters(in: .whitespacesAndNewlines), | |
| !activeBundleIdentifier.isEmpty, | |
| activeBundleIdentifier != Self.cmuxThemeOverrideBundleIdentifier else { | |
| return | |
| } | |
| guard let appSupport = FileManager.default.urls(for: .applicationSupportDirectory, in: .userDomainMask).first else { | |
| throw CLIError(message: "Unable to resolve Application Support directory") | |
| } | |
| private func removeStaleReleaseManagedThemeOverride(activeBundleIdentifier: String?) throws { | |
| guard let activeBundleIdentifier = (activeBundleIdentifier ?? currentCmuxAppBundleIdentifier())? | |
| .trimmingCharacters(in: .whitespacesAndNewlines), | |
| !activeBundleIdentifier.isEmpty, | |
| activeBundleIdentifier != Self.cmuxThemeOverrideBundleIdentifier else { | |
| return | |
| } | |
| guard let appSupport = FileManager.default.urls(for: .applicationSupportDirectory, in: .userDomainMask).first else { | |
| throw CLIError(message: String( | |
| localized: "cli.themes.error.cleanupFailed", | |
| defaultValue: "Unable to clean stale cmux theme override." | |
| )) | |
| } |
Rule Used: Flag production user-facing text that is not fully... (source)
There was a problem hiding this comment.
Fixed by routing the Application Support resolution failure in the stale-theme cleanup path through the existing localized cleanup failure message instead of surfacing the raw internal string.
— Claude Code
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 747a3bd. Configure here.
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
3 participants
Fixes #4501
Summary
themes clearcom.cmuxterm.app/config.ghosttywhen non-release channel builds runcmux themes setorcmux themes clearTesting
Note
Medium Risk
Changes theme/config file writes and Ghostty reload color-scheme logic across CLI and app; mistakes could affect channel users' themes or terminal appearance, though behavior is heavily regression-tested.
Overview
Channel builds (nightly/staging/debug) no longer inherit stale
# cmux themesblocks from the release app-support config when they have no channel config of their own. Config resolution now scans multiple Application Support roots, strips only the managed theme block (or deletes the file if nothing else remains) before release fallback, and CLIthemesruns that cleanup on entry, after clear, and surfacescli.themes.error.cleanupFailedwhen cleanup cannot complete.Ghostty reload treats true light/dark theme pairs as appearance-driven: resolves the active theme at load, locks terminal color scheme during cmux theme reloads, and reapplies surface schemes before refresh to reduce flashes. CI bumps
actions/checkoutto v6.0.2 in the perf activation workflow.Reviewed by Cursor Bugbot for commit 1c13f5a. Bugbot is set up for automated code reviews on this repo. Configure here.
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Summary by cubic
Cleans stale release-managed “# cmux themes” blocks for channel builds (nightly/staging/debug) and resolves light/dark theme pairs by appearance while keeping the terminal scheme stable. Fixes #4501 to stop channel themes being shadowed and reduce flicker on startup and reloads.
Bug Fixes
config.ghosttyand legacyconfig); only when a channel build would fall back to release; abort with a localized, sanitized error on failure.light:...,dark:...pairs at startup and reload by appearance; inline the resolved theme; lock terminal surface color scheme during reload to preserve the runtime scheme; for cleared-theme reloads, use the requested appearance.Dependencies
actions/checkout@v6.0.2.Written for commit 1c13f5a. Summary will update on new commits. Review in cubic
Summary by CodeRabbit