Fix theme override path for channel builds - #4484
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:
📝 WalkthroughWalkthroughTheme configuration path discovery and override handling are refactored to be bundle-identifier aware. A new ChangesBundle-identifier-aware theme configuration resolution
Sequence DiagramsequenceDiagram
participant User as User/Picker
participant ThemesCmd as runThemes
participant Support as CMUXCLI+ThemeSupport
participant Resolver as CmuxGhosttyConfigPathResolver
participant FS as FileSystem
participant Notif as DistributedNotificationCenter
User->>ThemesCmd: cmux themes set <theme>
ThemesCmd->>ThemesCmd: compute targetBundleIdentifier (e.g., com.cmuxterm.app.nightly)
ThemesCmd->>Support: writeManagedThemeOverride(theme, target)
Support->>Resolver: editableConfigURL(currentBundleIdentifier: target)
Resolver-->>Support: ~/Library/.../<target>/config.ghostty
Support->>FS: write override file
Support-->>ThemesCmd: ok / config_path
ThemesCmd->>Support: reloadThemesIfPossible(socket, target)
Support->>Notif: post distributed notification with bundleIdentifier=target
Notif-->>User: running app receives reload
User->>Resolver: loadConfigURLs(currentBundleIdentifier: target)
Resolver->>FS: check for existing config files
FS-->>Resolver: config found / not found
Resolver-->>User: resolved config URLs
User->>FS: read override
FS-->>User: theme applied
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (15 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 fixes a channel-build theme mismatch where
Confidence Score: 5/5Safe to merge. The fix is narrow and well-targeted: one resolution point, propagated consistently, with a cross-checked regression test. The change resolves the bundle-ID split between write target and reload target by threading a single targetBundleIdentifier through every consumer. The refactor is a pure code move with an identical body. CI changes address known flakiness without removing meaningful coverage. The new regression test validates the write path against the app's own config resolution, closing the loop on the original bug report. No new concurrency primitives, no new user-facing state, no schema or migration changes — all call sites are updated and the old dead wrapper is gone. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI as cmux CLI
participant TS as CMUXCLI+Themes.swift
participant TSup as CMUXCLI+ThemeSupport.swift
participant Res as CmuxGhosttyConfigPathResolver
participant FS as FileSystem
participant App as Channel App (e.g. Nightly)
CLI->>TS: runThemes(socketPath, ...)
TS->>TSup: themeTargetBundleIdentifier(socketPath)
TSup-->>TS: com.cmuxterm.app.nightly
note over TS: targetBundleIdentifier resolved ONCE
TS->>TSup: writeManagedThemeOverride(rawThemeValue, targetBundleIdentifier)
TSup->>Res: editableConfigURL(currentBundleIdentifier: com.cmuxterm.app.nightly)
Res-->>TSup: .../Application Support/com.cmuxterm.app.nightly/config.ghostty
TSup->>FS: write config.ghostty
TS->>TSup: reloadThemesIfPossible(socketPath, targetBundleIdentifier)
TSup->>App: DistributedNotification(bundleIdentifier: com.cmuxterm.app.nightly)
App->>FS: read .../com.cmuxterm.app.nightly/config.ghostty
App-->>CLI: surface.config.reload
Reviews (14): Last reviewed commit: "ci: poll xcodebuild process for test tim..." | 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 `@cmuxTests/CMUXCLIErrorOutputRegressionTests.swift`:
- Around line 349-355: Remove the redundant assertion that checks containment:
delete the XCTAssertTrue(appReadablePaths.contains(configPath)) line because the
prior XCTAssertEqual(appReadablePaths, [expectedConfigURL.path]) already
guarantees equality (and configPath == expectedConfigURL.path). Keep the
existing call to GhosttyApp.cmuxAppSupportConfigURLs(...) and the XCTAssertEqual
assertion; no other logic changes are needed.
🪄 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: b34a3403-19d8-4554-81e1-999a4a6872e0
📒 Files selected for processing (5)
CLI/CMUXCLI+ThemeSupport.swiftCLI/CMUXCLI+Themes.swiftSources/CmuxApplicationSupportDirectories.swiftSources/Settings/ConfigSource.swiftcmuxTests/CMUXCLIErrorOutputRegressionTests.swift
💤 Files with no reviewable changes (1)
- Sources/Settings/ConfigSource.swift
Resolved by 9f1c56b (removed the redundant assertion) and the inline thread is marked resolved.
|
Review follow-up:
All actionable review threads are resolved. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?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 `@cmux.xcodeproj/xcshareddata/xcschemes/cmux.xcscheme`:
- Line 20: Restore the default SWIFT_BACKTRACE value in the EnvironmentVariable
element named SWIFT_BACKTRACE by appending ",warnings=suppressed" to the value
string so it reads
interactive=no,timeout=0s,symbolicate=off,color=no,warnings=suppressed; update
the EnvironmentVariable with key="SWIFT_BACKTRACE" to include that suffix to
match the documented default-scheme backtrace config.
🪄 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: 93d655ac-c30a-408b-b374-fcf406d25b42
📒 Files selected for processing (4)
.github/workflows/ci.ymlcmux.xcodeproj/xcshareddata/xcschemes/cmux-ci.xcschemecmux.xcodeproj/xcshareddata/xcschemes/cmux-unit.xcschemecmux.xcodeproj/xcshareddata/xcschemes/cmux.xcscheme
Dismissed as false positive: warnings=suppressed is not a valid SWIFT_BACKTRACE key, and re-adding it would reintroduce stderr noise. The valid noninteractive settings remain in place.
![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 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
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 417f786. Configure here.
This branch was successfully deployed
2 participants
Summary
Fixes #4477.
Confirmed the bug model: the themes CLI derived the reload target from the active cmux socket/bundle, but its managed override path was still rooted at the release bundle id (
com.cmuxterm.app). That meant channel builds such as Nightly/Staging could post a reload notification to the running channel app while writing the override into a different bundle's Application Support directory.This implements Option 1 from the issue: derive the theme write target, picker config path, picker bundle id, and reload notification bundle id from the same active target bundle id. Channel builds now write
config.ghosttyunder their own bundle Application Support directory instead of silently retheming Release.Test-first history
Commit 1 adds the failing regression only:
CMUXCLIErrorOutputRegressionTests.testThemesSetNightlyOverridePathIsReadableByNightlyAppConfigResolutioncmux themes setwith a Nightly-equivalent bundle id and asserts the CLI'sconfig_pathis the same channel-local config path returned byGhosttyApp.cmuxAppSupportConfigURLs(currentBundleIdentifier: "com.cmuxterm.app.nightly").Commit 2 applies the fix.
Verification
Local focused regression on the rebased branch:
./scripts/test-unit.sh -only-testing:cmuxTests/CMUXCLIErrorOutputRegressionTests/testThemesSetNightlyOverridePathIsReadableByNightlyAppConfigResolution testAdjacent path/reload coverage also passed before the final rebase:
./scripts/test-unit.sh \ -only-testing:cmuxTests/CMUXCLIErrorOutputRegressionTests/testThemesSetReloadsRunningAppAfterEveryThemeWrite \ -only-testing:cmuxTests/CMUXCLIErrorOutputRegressionTests/testThemesSetTargetsResolvedTaggedSocketWhenBundleEnvironmentIsStale \ -only-testing:cmuxTests/GhosttyConfigPathResolverTests/testCmuxAppSupportConfigURLsUseNightlyConfigWhenPresent \ -only-testing:cmuxTests/GhosttyConfigPathResolverTests/testCmuxAppSupportConfigURLsUseReleaseConfigForDebugBundleWithoutCurrentConfig \ -only-testing:cmuxTests/GhosttyConfigPathResolverTests/testCmuxAppSupportConfigURLsUseReleaseConfigForNightlyWithoutCurrentConfig \ testTagged dev build/repro command used:
Before, the issue repro produced a release-bundle
config_pathwhile reloading a Nightly bundle. After this patch, the tagged dev app produced matching write/reload targets:{ "config_path": "/Users/austinwang/Library/Application Support/com.cmuxterm.app.debug.issue.4477.cmux.themes.nightly.bundle.mismatch/config.ghostty", "reload_target_bundle_id": "com.cmuxterm.app.debug.issue.4477.cmux.themes.nightly.bundle.mismatch", "ok": true }The running dev app then logged that it loaded the same channel-local config and refreshed the surface:
Release is unaffected: the release bundle id source remains
com.cmuxterm.app, so release-equivalent targets still write/read the release Application Support directory. Debug is unaffected: the existing debug fallback remains covered bytestCmuxAppSupportConfigURLsUseReleaseConfigForDebugBundleWithoutCurrentConfig, while a debug channel override now writes to the active debug bundle directory and is read directly once present.Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Touches CLI theme config read/write paths and reload targeting, which could affect where user theme overrides are persisted across app variants. Also adjusts CI test execution/timeout behavior, which may hide flakes if misconfigured.
Overview
Fixes channel-build theme overrides by deriving a single
targetBundleIdentifierfrom the active socket and using it consistently for theme config discovery, managed override write/clear location, interactive picker env, and distributed reload notifications.Centralizes Ghostty config path resolution into
CmuxGhosttyConfigPathResolver(moved out ofConfigSource.swift), adds a regression ensuring Nightly writes to/returns the channel-localApplication Support/<bundle>/config.ghostty, and refactors a markdown local-image test to avoid WebKit window/app-host flakiness.CI/test stability is improved by switching
SWIFT_BACKTRACEto non-interactive settings, skipping a set of flaky app-host tests, and adding a watchdog timeout that terminates stuckxcodebuildruns.Reviewed by Cursor Bugbot for commit a1e870e. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes channel-build theme overrides by resolving a single target bundle id and using it for discovery, writes/clears, picker, and reloads. Nightly/Staging/Debug now read and write
config.ghosttyin their own Application Support directory.Bug Fixes
targetBundleIdentifierfor config search, managed override write/clear, picker env, and reload;themesnow reads/writes the channel-localconfig.ghostty.WKURLSchemeTaskspy.Refactors
CmuxGhosttyConfigPathResolver(moved toSources/CmuxApplicationSupportDirectories.swift); remove stale helper and passthemeTargetBundleIdentifierthrough the CLI.SWIFT_BACKTRACEnon-interactive, quarantine flaky app-host tests, and pollxcodebuildwith a timeout watchdog (configurable viaCMUX_UNIT_TEST_TIMEOUT_SECONDS).Written for commit a1e870e. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
Refactor
Tests
Chores