Repository navigation
Conversation
After commit 70bcbda (PR manaflow-ai#4415) decoupled live appearance from the settings-file store side-effects, `AppearanceSettingsUserDefaultsObserver` became the single owner of live appearance. But `startObserving()` only primes `lastObservedRawValue` and waits for change events — when the launch begins with the stored mode already matching what's persisted in UserDefaults (the steady state after the first import of `app.appearance` from `cmux.json`), no change event ever fires and the first NSWindow materializes with the wrong appearance. This test arranges that exact scenario: pre-seed `appearanceMode = dark` in UserDefaults, call `startObserving()`, assert that the dark appearance was applied to the application. It fails on current HEAD (no apply happens) and will pass once `startObserving()` performs a one-shot apply of the persisted value. Two-commit structure per CLAUDE.md regression-test policy: this commit adds the failing test; the next commit applies the fix.
The observer added in manaflow-ai#4415 only fires on UserDefaults *changes*. After the first launch that imports `app.appearance` from `cmux.json`, the persisted value and the imported value are equal, so no change event fires at startObserving time and `NSApplication.shared.appearance` is never updated by the observer. The early `Self.applyAppearance(_, duringLaunch: true)` in `cmuxApp.init()` runs before `NSApp` is fully initialized so the value can fail to propagate to the first NSWindow created by the SwiftUI `WindowGroup`, leaving the main window (and any `NSVisualEffectView`-backed sidebar) stuck in the system default appearance. Apply the stored mode once during `startObserving()`, after the defaults observer is registered. This runs from `applicationDidFinishLaunching` — well after FileStore initialization, so it does not re-enter the Ghostty reload guard that PR manaflow-ai#4415 was fixing. Subsequent calls to `startObserving()` skip the re-apply (only the baseline is refreshed) to avoid spurious work if the observer is ever re-attached. Fixes the regression where: - `cmux.json` contains `"app": { "appearance": "dark" }` - `defaults read com.cmuxterm.app appearanceMode` is `dark` - Settings UI → Appearance → App Appearance shows Dark - But main NSWindow and sidebar render Light until the user manually toggles App Appearance in Settings to force a change event. Regression test added in the previous commit.
|
@ma-pony is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
To use Codex here, create a Codex account and connect to github. |
📝 WalkthroughWalkthroughThis PR fixes a regression where the main window renders Light on launch despite persisted appearance being Dark. The observer now applies the stored mode immediately when registered from ChangesAppearance Mode Launch-Time Application
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 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 |
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
|
✅ Actions performedReview triggered.
|
@ma-pony I have started the AI code review. It will take a few minutes to complete. |
|
To use Codex here, create a Codex account and connect to github. |
Greptile SummaryFixes the steady-state launch appearance bug (#4527) where
Confidence Score: 3/5The fix correctly resolves the steady-state dark-mode launch gap, but The new
Important Files Changed
Sequence DiagramsequenceDiagram
participant App as cmuxApp.init()
participant DL as applicationDidFinishLaunching
participant Obs as AppearanceSettingsUserDefaultsObserver
participant UD as UserDefaults
participant NSApp as NSApplication.shared
App->>NSApp: applyAppearance(duringLaunch:true)
Note over App,NSApp: Before NSApp fully initialized — may not stick to first NSWindow
DL->>Obs: startObserving()
Obs->>UD: currentRawValue() → "dark"
Obs->>Obs: register NotificationCenter observer (queue: .main)
Note over Obs: NEW (this PR)
Obs->>Obs: applyStoredMode("dark", source)
Obs->>NSApp: setApplicationAppearance(.darkAqua)
Obs->>Obs: "lastObservedRawValue = "dark""
Note over UD,NSApp: Later — user changes mode in Settings
UD-->>Obs: UserDefaults.didChangeNotification (on .main queue)
Obs->>Obs: applyIfChanged()
Obs->>UD: currentRawValue()
Obs->>NSApp: setApplicationAppearance(…)
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| defaultsObserver = environment.addDefaultsObserver { [weak self] in | ||
| self?.applyIfChanged() | ||
| } | ||
| // After commit 70bcbda2 (PR #4415) the settings-file store no | ||
| // longer applies appearance as a side-effect during launch — | ||
| // that responsibility was delegated to this observer to avoid | ||
| // re-entering Ghostty while the file store initializes. The | ||
| // observer's `applyIfChanged` only fires on *changes* to | ||
| // `appearanceMode`, so in the steady-state case (UserDefaults | ||
| // already has the imported value), no change event ever fires | ||
| // and the launch-time apply is missed. | ||
| // | ||
| // The early `Self.applyAppearance(_, duringLaunch: true)` in | ||
| // `cmuxApp.init()` does set `NSApplication.shared.appearance` | ||
| // but runs before `NSApp` is fully initialized, so the value | ||
| // can fail to stick to the first `NSWindow`. Re-apply once | ||
| // here, from `applicationDidFinishLaunching`, to guarantee | ||
| // the stored mode reaches the main WindowGroup window. | ||
| let appliedMode = environment.applyStoredMode(initialRawValue, self.source) | ||
| lastObservedRawValue = appliedMode.rawValue |
There was a problem hiding this comment.
After this fix there are two independent sites that apply appearance at launch: the cmuxApp.init() early path (with duringLaunch: true, which resolves .system to an explicit NSAppearance) and the new startObserving() path (with duringLaunch: false, which sends nil for .system). The two calls have different duringLaunch semantics and different windows in the app lifecycle, but neither call site names the invariant that makes the other one complementary. For .system mode the startObserving() apply silently overwrites the NSApplication.shared.appearance value that the init path carefully computed, without a comment explaining that this is intentional. A future maintainer removing either path — or changing the duringLaunch logic — would not have a clear signal that both sites must stay in sync.
Rule Used: Flag Swift fixes that patch symptoms while leaving... (source)
Greptile SummaryThis PR fixes a launch-time appearance gap: when
Confidence Score: 3/5The fix correctly addresses the missed initial apply for Both Sources/AppearanceSettings.swift — the new Important Files Changed
Sequence DiagramsequenceDiagram
participant App as cmuxApp.init()
participant AD as AppDelegate.applicationDidFinishLaunching
participant Obs as AppearanceSettingsUserDefaultsObserver
participant AS as AppearanceSettings
participant NSApp as NSApplication.shared
App->>AS: applyLiveMode(mode, duringLaunch: true, syncTheme: false)
AS->>NSApp: "appearance = systemNSAppearance() [system] or darkAqua/aqua [dark/light]"
Note over NSApp: NSApp may not be fully initialized — apply can fail to stick
AD->>Obs: startObserving()
Obs->>AS: applyStoredMode(initialRawValue, source, duringLaunch: false)
AS->>NSApp: "appearance = nil [system] or darkAqua/aqua [dark/light]"
Note over NSApp: Authoritative apply — NSApp fully initialized
Obs->>Obs: "defaultsObserver = addDefaultsObserver"
Obs->>Obs: "lastObservedRawValue = appliedMode.rawValue"
Note over Obs: Subsequent UserDefaults changes
Obs-->>AS: applyIfChanged() → applyStoredMode(rawValue, source)
AS->>NSApp: appearance updated
Reviews (2): Last reviewed commit: "fix: apply stored appearance when observ..." | Re-trigger Greptile |
| // After commit 70bcbda2 (PR #4415) the settings-file store no | ||
| // longer applies appearance as a side-effect during launch — | ||
| // that responsibility was delegated to this observer to avoid | ||
| // re-entering Ghostty while the file store initializes. The | ||
| // observer's `applyIfChanged` only fires on *changes* to | ||
| // `appearanceMode`, so in the steady-state case (UserDefaults | ||
| // already has the imported value), no change event ever fires | ||
| // and the launch-time apply is missed. | ||
| // | ||
| // The early `Self.applyAppearance(_, duringLaunch: true)` in | ||
| // `cmuxApp.init()` does set `NSApplication.shared.appearance` | ||
| // but runs before `NSApp` is fully initialized, so the value | ||
| // can fail to stick to the first `NSWindow`. Re-apply once | ||
| // here, from `applicationDidFinishLaunching`, to guarantee | ||
| // the stored mode reaches the main WindowGroup window. | ||
| let appliedMode = environment.applyStoredMode(initialRawValue, self.source) | ||
| lastObservedRawValue = appliedMode.rawValue |
There was a problem hiding this comment.
duringLaunch semantics
startObserving() now performs a one-shot applyStoredMode(duringLaunch: false) at applicationDidFinishLaunching, but cmuxApp.init() still calls Self.applyAppearance(startupAppearance, duringLaunch: true) unconditionally first. The comment here acknowledges the init() path "can fail to stick," but it remains active, so both paths run at every cold launch in sequence. They behave differently for .system mode: the init() path resolves systemNSAppearance() and sets a concrete NSAppearance, while this new path calls applicationAppearance(for: .system, duringLaunch: false) which returns nil, resetting NSApp.appearance back to the OS default. Setting nil is functionally correct for system mode at applicationDidFinishLaunching, but having two sequential launch-time apply paths — one explicitly described as unreliable and one intended to be authoritative — leaves the invariant ("appearance is applied exactly once at the right lifecycle point") unowned. A future caller who removes or conditions the init() path will not know the startObserving() apply is load-bearing, and vice versa. The cmuxApp.init() early apply should either be removed or clearly documented as intentional pre-NSApp-init scaffolding that is superseded by this call.
Rule Used: Flag Swift fixes that patch symptoms while leaving... (source)
![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 `@Sources/AppearanceSettings.swift`:
- Around line 287-293: The bug is that startObserving() updates
lastObservedRawValue when defaultsObserver != nil, which can cancel a queued
UserDefaults.didChangeNotification and prevent applyIfChanged() from running;
fix by removing the assignment to lastObservedRawValue inside the guard branch
so that when defaultsObserver is already set (i.e., already observing) the
method simply returns without changing the baseline; update the code around
startObserving(), defaultsObserver, lastObservedRawValue,
environment.currentRawValue(), and applyIfChanged() accordingly so the queued
notification still triggers an actual change check.
🪄 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: 93fcb6a4-766a-44d4-8761-86bd664b9c58
📒 Files selected for processing (2)
Sources/AppearanceSettings.swiftcmuxTests/AppearanceSettingsTests.swift
| let initialRawValue = environment.currentRawValue() | ||
| guard defaultsObserver == nil else { | ||
| // Already observing — keep the change-detection baseline | ||
| // accurate without re-applying. | ||
| lastObservedRawValue = initialRawValue | ||
| return | ||
| } |
There was a problem hiding this comment.
Avoid updating lastObservedRawValue when defaultsObserver != nil—it can suppress a pending appearance change.
In Sources/AppearanceSettings.swift, startObserving() captures initialRawValue and, when already observing, sets lastObservedRawValue to that new baseline before returning. If appearanceMode changes and a UserDefaults.didChangeNotification is already queued on .main, calling startObserving() again before the queued applyIfChanged() runs can turn the queued update into a no-op, leaving the app on the old appearance despite defaults having changed.
Suggested fix
- let initialRawValue = environment.currentRawValue()
guard defaultsObserver == nil else {
- // Already observing — keep the change-detection baseline
- // accurate without re-applying.
- lastObservedRawValue = initialRawValue
return
}
+ let initialRawValue = environment.currentRawValue()🤖 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 `@Sources/AppearanceSettings.swift` around lines 287 - 293, The bug is that
startObserving() updates lastObservedRawValue when defaultsObserver != nil,
which can cancel a queued UserDefaults.didChangeNotification and prevent
applyIfChanged() from running; fix by removing the assignment to
lastObservedRawValue inside the guard branch so that when defaultsObserver is
already set (i.e., already observing) the method simply returns without changing
the baseline; update the code around startObserving(), defaultsObserver,
lastObservedRawValue, environment.currentRawValue(), and applyIfChanged()
accordingly so the queued notification still triggers an actual change check.
Greptile SummaryThis PR fixes a launch-time appearance regression introduced by PR #4415:
Confidence Score: 4/5Safe to merge for the targeted dark/light launch regression; the system mode path now sets NSApp.appearance to nil at observation start which is correct but untested. The fix is small and well-commented. The two findings are a secondary launch-time apply path that duplicates cmuxApp.init() with different duringLaunch semantics, and a missing test case for system mode at startObserving() time. Neither blocks the intended fix from working correctly for the reported bug. Sources/AppearanceSettings.swift — the interaction between the cmuxApp.init() apply (duringLaunch: true) and the new startObserving() apply (duringLaunch: false) for .system mode is worth a second look. Important Files Changed
Sequence DiagramsequenceDiagram
participant App as cmuxApp.init()
participant DFL as applicationDidFinishLaunching
participant Obs as AppearanceSettingsUserDefaultsObserver
participant UD as UserDefaults
participant NSApp as NSApplication
App->>NSApp: applyAppearance(duringLaunch: true)
Note over NSApp: may not stick to first NSWindow
DFL->>Obs: startObserving()
Obs->>UD: addObserver (fires on changes only)
Obs->>UD: "currentRawValue() -> dark"
Obs->>NSApp: applyStoredMode(dark, source)
Note over NSApp: duringLaunch: false -> darkAqua
Note over Obs: lastObservedRawValue = dark
UD-->>Obs: didChangeNotification (user toggles)
Obs->>Obs: applyIfChanged()
Obs->>UD: "currentRawValue() -> new value"
Obs->>NSApp: applyStoredMode(newValue, source)
|
| func testStartObservingAppliesStoredAppearanceImmediately() { | ||
| let suiteName = "AppearanceSettingsTests.StartObservingInitialApply.\(UUID().uuidString)" | ||
| guard let defaults = UserDefaults(suiteName: suiteName) else { | ||
| XCTFail("Failed to create isolated UserDefaults suite") | ||
| return | ||
| } | ||
| defer { defaults.removePersistentDomain(forName: suiteName) } | ||
|
|
||
| var appliedAppearanceName: NSAppearance.Name? | ||
| var synchronizedAppearanceName: NSAppearance.Name? | ||
| var synchronizedSource: String? | ||
| let liveEnvironment = AppearanceSettings.LiveApplyEnvironment( | ||
| setApplicationAppearance: { appearance in | ||
| appliedAppearanceName = appearance?.bestMatch(from: [.darkAqua, .aqua]) | ||
| }, | ||
| synchronizeTerminalThemeWithAppearance: { appearance, source in | ||
| synchronizedAppearanceName = appearance?.bestMatch(from: [.darkAqua, .aqua]) | ||
| synchronizedSource = source | ||
| }, | ||
| systemAppearance: { | ||
| XCTFail("Dark mode should not resolve system appearance") | ||
| return nil | ||
| } | ||
| ) | ||
| let observer = AppearanceSettingsUserDefaultsObserver( | ||
| environment: .init( | ||
| addDefaultsObserver: { _ in NSObject() }, | ||
| removeObserver: { _ in }, | ||
| currentRawValue: { | ||
| defaults.string(forKey: AppearanceSettings.appearanceModeKey) | ||
| }, | ||
| applyStoredMode: { rawValue, source in | ||
| AppearanceSettings.applyStoredMode( | ||
| rawValue: rawValue, | ||
| defaults: defaults, | ||
| source: source, | ||
| environment: liveEnvironment | ||
| ) | ||
| } | ||
| ), | ||
| source: "test.startObservingInitialApply" | ||
| ) | ||
|
|
||
| defaults.set(AppearanceMode.dark.rawValue, forKey: AppearanceSettings.appearanceModeKey) | ||
| observer.startObserving() | ||
|
|
||
| XCTAssertEqual( | ||
| appliedAppearanceName, | ||
| .darkAqua, | ||
| "startObserving must apply the persisted appearance mode immediately so the main WindowGroup window picks it up on launch" | ||
| ) | ||
| XCTAssertEqual(synchronizedAppearanceName, .darkAqua) | ||
| XCTAssertEqual(synchronizedSource, "test.startObservingInitialApply") | ||
| } |
There was a problem hiding this comment.
.system mode behavior at startObserving() time
The regression test only seeds .dark and asserts .darkAqua. For .system mode, applyStoredMode(..., duringLaunch: false) sets NSApp.appearance = nil (the applicationAppearance function returns nil for .system when duringLaunch == false). This is different from the cmuxApp.init() call which uses duringLaunch: true and explicitly calls environment.systemAppearance(). A test pre-seeding .system should assert that appliedAppearanceName == nil and that systemAppearance is not called — confirming this nil-set behavior is intentional, and that the XCTFail guard in the existing systemAppearance stub would correctly catch an accidental regression where startObserving inadvertently calls through to system-appearance resolution.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
teamleaderleo
added
S3: minor
This branch has not been deployed
2 participants
Fixes #4527.
Summary
AppearanceSettingsUserDefaultsObserver.startObserving()now performs a one-shot apply of the persistedappearanceModeafter registering the defaults observer. Subsequent calls tostartObserving()only refresh the baseline (no re-apply), preserving idempotence if the observer is ever re-attached.70bcbda2) delegated live appearance to this observer but the observer only fires on UserDefaults changes. In the steady-state case — whereappearanceModeis already"dark"in UserDefaults at observe-start time — no change event ever fires, soNSApplication.shared.appearanceis never updated by the observer. The earlierSelf.applyAppearance(_, duringLaunch: true)incmuxApp.init()runs beforeNSAppis fully initialized and can fail to propagate to the firstNSWindowmaterialized by theWindowGroup, leaving the main window and anyNSVisualEffectView-backed sidebar stuck in the system default appearance. The fix runs fromapplicationDidFinishLaunching, well after FileStore initialization, so it does not re-enter the Ghostty reload guard Fix settings appearance dispatch_once reentrancy #4415 was fixing.Testing
testStartObservingAppliesStoredAppearanceImmediatelyincmuxTests/AppearanceSettingsTests.swiftthat pre-seedsappearanceMode = dark, callsstartObserving()against an isolatedUserDefaultssuite, and asserts the.darkAquaappearance was applied via the injectedLiveApplyEnvironment.app.appearance: "dark",defaults appearanceMode = dark, Settings UI shows Dark, main window renders Light); the manual workaround in the issue (Settings Light → Dark toggle) matches the code path that this fix executes once at launch.Demo Video
Bug is "Light instead of Dark on first launch after upgrade with
app.appearance: darkset"; visible diff is identical to the issue's screenshots/state. Happy to record one if the maintainers want.Review Trigger (Copy/Paste as PR comment)
Checklist
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Summary by cubic
Apply the stored app appearance as soon as the defaults observer starts so the first window uses the correct theme on launch. Fixes cases where Dark is set but the app opens in Light until the user toggles settings.
appearanceModeinAppearanceSettingsUserDefaultsObserver.startObserving()after registering the observer.startObserving()calls only refresh the baseline (no re-apply).testStartObservingAppliesStoredAppearanceImmediatelyto confirm Dark/Light is applied at launch.Written for commit 88c4402. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
Bug Fixes
Tests