Repository navigation
Fix settings appearance dispatch_once reentrancy - #4415
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:
📝 WalkthroughWalkthroughRemove appearance-terminal-theme synchronization from CmuxSettingsFileStore init/reload: drop appearanceEnvironment plumbing, remove synchronous appearance application during store init, simplify managed-default side-effects to defaultsKey-only, add a UserDefaults appearance observer, update startup tests, and set SWIFT_BACKTRACE in CI and the Xcode scheme. ChangesDeferred Appearance Synchronization
AppearanceSettings UserDefaults observer
App lifecycle & right-sidebar
CI workflow
🎯 4 (Complex) | ⏱️ ~45 minutes
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
Confidence Score: 5/5Safe to merge. The architectural change is well-reasoned, the ownership boundary is clearly documented, and the new regression tests directly assert the fixed invariant. The reentrancy fix is clean: the settings file store now only writes to UserDefaults for appearance keys, and all live application is deferred to the post-launch observer. All changed code paths are covered by new and updated tests that assert the desired no-application behaviour during init and reload. Sources/AppearanceSettings.swift — the new AppearanceSettingsUserDefaultsObserver class would benefit from a @mainactor annotation to make its main-thread contract explicit to the Swift 6 type checker. Important Files Changed
Sequence DiagramsequenceDiagram
participant App as cmuxApp / AppDelegate
participant Store as CmuxSettingsFileStore
participant UD as UserDefaults
participant Obs as AppearanceSettingsUserDefaultsObserver
participant Ghostty as Ghostty reloadConfiguration
Note over App,Ghostty: Old (broken) path — reentrancy during dispatch_once
App->>Store: init (dispatch_once)
Store->>UD: "write appearanceModeKey = "dark""
Store->>Ghostty: synchronizeTerminalThemeWithAppearance() 💥 reentrancy
Note over App,Ghostty: New (fixed) path
App->>Store: init (dispatch_once)
Store->>UD: "write appearanceModeKey = "dark""
Note over Store: managedDefaultSideEffects returns empty for appearanceModeKey — no Ghostty call
App->>Obs: shared.startObserving() (in didFinishLaunching)
Obs->>UD: "read current value → lastObservedRawValue = "dark""
Note over Obs: Future UserDefaults changes observed
UD-->>Obs: didChangeNotification (value changes to "light")
Obs->>Obs: applyIfChanged() — value changed
Obs->>Ghostty: applyStoredMode → synchronizeTerminalTheme ✅ safe post-init
Reviews (5): Last reviewed commit: "fix: decouple settings appearance replay..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private func managedDefaultSideEffects(for defaultsKey: String) -> ManagedDefaultBatchSideEffects { | ||
| guard defaultsKey != AppearanceSettings.appearanceModeKey else { | ||
| return ManagedDefaultBatchSideEffects() | ||
| } | ||
| var sideEffects = ManagedDefaultBatchSideEffects() | ||
| sideEffects.append( | ||
| defaultsKey: defaultsKey, | ||
| source: source, | ||
| synchronizeAppearanceTerminalTheme: synchronizeAppearanceTerminalTheme | ||
| ) | ||
| sideEffects.append(defaultsKey: defaultsKey) | ||
| return sideEffects | ||
| } |
There was a problem hiding this comment.
The guard now returns an empty ManagedDefaultBatchSideEffects for appearanceModeKey on every call path — not only the dispatch_once-constrained init. This means a live settings-file reload (store.reload() from the file watcher) will write the new appearance to UserDefaults but will never fire any side effect through this store, including after the store has fully initialized. The tests deliberately assert that appliedAppearanceNames.isEmpty and synchronizedAppearanceNames.isEmpty even after a post-init store.reload(), confirming this is intentional. For the visual appearance to update when the user edits the settings file at runtime, the "app appearance owner" mentioned in the PR description must observe UserDefaults.didChangeNotification (or KVO) on AppearanceSettings.appearanceModeKey and apply the change independently. That ownership path is not shown in this PR — consider a comment here or a companion test that exercises the full round-trip through the appearance owner to make the contract explicit and guard against future divergence.
Rule Used: Flag Swift fixes that patch symptoms while leaving... (source)
There was a problem hiding this comment.
Addressed by documenting the owner boundary in Sources/KeyboardShortcutSettingsFileStore.swift: cmuxApp owns live appearance application through launch bootstrap and its @AppStorage observer; the settings store only imports the default to avoid Ghostty reentry during singleton initialization.
— Claude Code
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
be6468b to
81a028d
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
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 (1)
cmuxTests/KeyboardShortcutSettingsFileStoreStartupTests.swift (1)
204-281: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd a restore-path regression for removing
app.appearance.These tests cover initial import and value changes, but the production change also altered the restore branch to skip appearance side effects. A case that starts managed, removes
app.appearancefrom the file, and asserts the backup/default is restored without live apply would lock down the other half of this contract.Also applies to: 283-353, 502-579
🤖 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 `@cmuxTests/KeyboardShortcutSettingsFileStoreStartupTests.swift` around lines 204 - 281, Add a test case that starts with a managed appearance in the settings file, then removes the "app.appearance" key and asserts the stored default/backup is restored and no live-apply side effects occur: in testManagedAppearanceReplayUpdatesDefaultWithoutLiveAppearanceApplication (and the other similar tests at the ranges mentioned) after the initial write of "appearance": "dark" and the checks, write a new settings file that omits app.appearance (e.g., an empty JSON object or no "app" key), call store.reload(), then assert defaults.string(forKey: AppearanceSettings.appearanceModeKey) has been cleared/restored to the pre-import value (nil or the original backup) and that appliedAppearanceNames and synchronizedAppearanceNames remain empty; use the existing KeyboardShortcutSettingsFileStore instance and applyDeferredManagedDefaultSideEffects() same as the other assertions so the restore branch that skips appearance side effects is covered.
🤖 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 `@cmuxTests/KeyboardShortcutSettingsFileStoreStartupTests.swift`:
- Around line 204-281: Add a test case that starts with a managed appearance in
the settings file, then removes the "app.appearance" key and asserts the stored
default/backup is restored and no live-apply side effects occur: in
testManagedAppearanceReplayUpdatesDefaultWithoutLiveAppearanceApplication (and
the other similar tests at the ranges mentioned) after the initial write of
"appearance": "dark" and the checks, write a new settings file that omits
app.appearance (e.g., an empty JSON object or no "app" key), call
store.reload(), then assert defaults.string(forKey:
AppearanceSettings.appearanceModeKey) has been cleared/restored to the
pre-import value (nil or the original backup) and that appliedAppearanceNames
and synchronizedAppearanceNames remain empty; use the existing
KeyboardShortcutSettingsFileStore instance and
applyDeferredManagedDefaultSideEffects() same as the other assertions so the
restore branch that skips appearance side effects is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 49f9bf1c-1bad-495c-bce0-6c1fc4453147
📒 Files selected for processing (2)
Sources/KeyboardShortcutSettingsFileStore.swiftcmuxTests/KeyboardShortcutSettingsFileStoreStartupTests.swift
81a028d to
3f96d03
Compare
3f96d03 to
4207b5e
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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 4207b5e. Configure here.
4207b5e to
70bcbda
Compare
This branch was successfully deployed
1 participant
Fixes #4412.
Summary
Testing
git diff --checkxcodebuild, or./scripts/reload.sh.Notes
Need help on this PR? Tag
@codesmithwith what you need.Note
Medium Risk
Changes ownership of appearance application by moving it out of the settings-file store and into an app-lifecycle
UserDefaultsobserver, which could affect when/if UI and terminal theme updates occur. Risk is mitigated by new regression tests covering startup and reload reentrancy paths.Overview
Prevents startup reentrancy by stopping
CmuxSettingsFileStorefrom applying live appearance / terminal-theme side effects when importing managedappearanceMode; it now only persists theUserDefaultsvalue and skips appearance-related side effects.Adds an app-lifecycle
AppearanceSettingsUserDefaultsObserverstarted inAppDelegateto apply live appearance changes when the stored mode actually changes, plus tests ensuring managed appearance imports/replays never trigger live appearance or Ghostty reload paths during init/reload. Also adjusts remote right-sidebar focus to preserve the current sidebarmode, and setsSWIFT_BACKTRACE=interactive=no,color=noin CI and the unit-test scheme to avoid interactive crash prompts.Reviewed by Cursor Bugbot for commit 70bcbda. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Prevents re-entrant appearance updates by moving live appearance/terminal-theme application out of the settings file store and into the app via a
UserDefaultsobserver. Fixes #4412.Bug Fixes
UserDefaults.AppearanceSettingsUserDefaultsObserverand start it at launch; the app now applies live appearance when the stored mode changes.appearanceEnvironmentand sync flags;reload(...)no longer coordinates appearance, and the live env comes fromAppearanceSettings.modetofocusRightSidebarInActiveMainWindow(...).Dependencies
SWIFT_BACKTRACE=interactive=no,color=noin CI and the unit-test scheme to avoid interactive prompts on Swift crashes.Written for commit 70bcbda. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
Refactor
New Features
Tests
Bug Fix
Chores