fix(profiles): prompt before applying display spacing#621
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR adds a draft/confirm/error flow for menu-bar item spacing changes, seeds per-display spacing from system NSStatusItemSpacing on first load, exposes the active menu-bar display UUID, and provides a ProfileManager API to apply spacing offsets across all profiles. ChangesSpacing Confirmation Flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Thaw/Settings/Models/DisplaySettingsManager.swift (1)
107-118:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGate the seed migration on missing persisted data, not on
configurations.isEmpty.Line 116 currently treats both a decode failure and a deliberately persisted empty
{}as “first launch”. That lets this migration rerun later and can overwrite existing per-display settings with freshly seeded values. Only callseedConfigurationsFromSystemSpacing()when.displayIceBarConfigurationsis absent.Suggested fix
private func loadInitialState() { - if let data = Defaults.data(forKey: .displayIceBarConfigurations) { + let persistedConfigurationsData = Defaults.data(forKey: .displayIceBarConfigurations) + if let data = persistedConfigurationsData { do { configurations = try decoder.decode([String: DisplayIceBarConfiguration].self, from: data) diagLog.info("Loaded per-display configurations for \(configurations.count) display(s)") } catch { diagLog.error("Failed to decode per-display configurations: \(error)") } } - if configurations.isEmpty { + if persistedConfigurationsData == nil { seedConfigurationsFromSystemSpacing() } if let data = Defaults.data(forKey: .knownDisplays) {🤖 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 `@Thaw/Settings/Models/DisplaySettingsManager.swift` around lines 107 - 118, The code currently calls seedConfigurationsFromSystemSpacing() when configurations.isEmpty, which treats a persisted empty dictionary as missing and can overwrite real user settings; change loadInitialState() to call seedConfigurationsFromSystemSpacing() only when Defaults.data(forKey: .displayIceBarConfigurations) is nil (i.e., no persisted value). Specifically, in loadInitialState() use the presence/absence of Defaults.data(forKey: .displayIceBarConfigurations) to decide whether to seed (call seedConfigurationsFromSystemSpacing()) and keep the existing decode/empty handling for configurations without triggering the migration when an empty persisted value exists.
🤖 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 `@Thaw/Settings/Models/ProfileManager.swift`:
- Around line 676-684: Load and build the full batch of updated Profile objects
first (in updateAllProfilesItemSpacingOffset: iterate profiles and call
loadProfile(id:) and modify displayConfigurations with withItemSpacingOffset
into a temporary [Profile] array), so any load error aborts before writing; then
persist the changes: save each modified profile using a save method that does
NOT update the manifest (avoid calling saveProfileAndUpdateManifest in the
per-profile loop), and once all profile writes succeed, update/save the manifest
exactly once (call the existing manifest-update routine or a single
saveProfileAndUpdateManifest-like call for the manifest step).
In `@Thaw/Settings/SettingsPanes/DisplaySettingsPane.swift`:
- Around line 314-347: Before mutating live state with
commitSpacing(displayID:pending.displayID, offset:pending.offset), snapshot the
current offset (e.g., let previousOffset = currentConfiguration.offset or read
from the same source commitSpacing writes to) and then call commitSpacing only
after the profile write succeeds; alternatively keep the existing order but
restore the snapshot in each catch: in the catch blocks for
appState.profileManager.updateProfile(...) and
updateAllProfilesItemSpacingOffset(...) set the live config back to
previousOffset (via commitSpacing or the same internal updater) and then set
errorMessage/showingError as before. Ensure you reference commitSpacing,
pending.displayID, pending.offset, appState.profileManager.updateProfile, and
updateAllProfilesItemSpacingOffset when locating the places to snapshot and
restore.
---
Outside diff comments:
In `@Thaw/Settings/Models/DisplaySettingsManager.swift`:
- Around line 107-118: The code currently calls
seedConfigurationsFromSystemSpacing() when configurations.isEmpty, which treats
a persisted empty dictionary as missing and can overwrite real user settings;
change loadInitialState() to call seedConfigurationsFromSystemSpacing() only
when Defaults.data(forKey: .displayIceBarConfigurations) is nil (i.e., no
persisted value). Specifically, in loadInitialState() use the presence/absence
of Defaults.data(forKey: .displayIceBarConfigurations) to decide whether to seed
(call seedConfigurationsFromSystemSpacing()) and keep the existing decode/empty
handling for configurations without triggering the migration when an empty
persisted value exists.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9160d0b3-f7c3-46d1-ad52-d63b9ce1d2e7
📒 Files selected for processing (4)
Thaw/Resources/Localizable.xcstringsThaw/Settings/Models/DisplaySettingsManager.swiftThaw/Settings/Models/ProfileManager.swiftThaw/Settings/SettingsPanes/DisplaySettingsPane.swift
Clicking Apply on Displays > "Menu Bar Item Spacing" while a profile is active triggered a relaunch wave for menu bar apps, after which `DisplaySettingsManager.applyActiveDisplaySpacing` called `ProfileManager.reapplyActiveProfile()`. The reapplied profile's stored `displayConfigurations` still held the previous offset, so the just-applied spacing was silently reverted, then re-applied by a second relaunch wave once the value flipped back. Reported in #619. The Apply and inline ↺ reset buttons now route through a single confirmation gate in `DisplaySettingsPane`. When the change targets the active menu bar display, the alert names the relaunch wave; when a profile is active, the alert lets the user pick `Update Active Profile` (saves into the active profile via `updateProfile(scope:.configurationOnly)`) or `Update All Profiles` (writes the spacing into every profile via the new `ProfileManager.updateAllProfilesItemSpacingOffset(displayUUID:offset:)` helper). The profile file(s) are written synchronously before the Combine sink dispatches `applyActiveDisplaySpacing`, so the subsequent `reapplyActiveProfile` loads the new spacing instead of the old one and the value sticks. Cases without an active profile and a non-active display still apply immediately, preserving the prior fast path. Cancelling the alert snaps the slider back to the saved value. New strings are added to `Localizable.xcstrings` for Crowdin pickup. `DisplaySettingsManager.loadInitialState` also now adopts externally configured system spacing on first launch. When no per-display configurations exist yet and the user has set `NSStatusItemSpacing` outside Thaw (e.g. via a `defaults write` in Terminal), the manager seeds each connected display's `itemSpacingOffset` from that on-disk value and persists the seed inline. Previously Thaw read its own default offset of 0, computed target = 16, and fired a relaunch wave on every startup that overwrote the user's manual setting. Addresses the recurring restart loops in #602 and #598. Fixes #598, #602, #619 Signed-off-by: Amir Zarrinkafsh <3339418+nightah@users.noreply.github.com>
nightah
force-pushed
the
fix/display-spacing-apply-saves
branch
from
3182f42 to
488ed67
Compare
|
Contributor
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 `@Thaw/Settings/SettingsPanes/DisplaySettingsPane.swift`:
- Around line 314-331: The current handler uses
appState.profileManager.updateProfile(id:scope:.configurationOnly) which
persists the entire profile and can unintentionally save unrelated live changes;
replace that call with a targeted updater that only patches
displayConfigurations[pending.displayID].itemSpacingOffset (e.g. add a new
method like updateProfileItemSpacingOffset(id: UUID, displayUUID: String,
offset: Double) on ProfileManager and call it instead of updateProfile(...)).
Keep the existing snapshot/rollback logic: capture previousOffset via
displaySettings.configuration(forUUID: pending.displayID).itemSpacingOffset,
call commitSpacing(displayID:pending.displayID, offset:pending.offset), then try
the new targeted update; on error restore the in-memory spacing by calling
commitSpacing(displayID:pending.displayID, offset: previousOffset) and
rethrow/log as before.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 278bc5e6-c229-4c0a-9005-aea9e3189f2e
📒 Files selected for processing (4)
Thaw/Resources/Localizable.xcstringsThaw/Settings/Models/DisplaySettingsManager.swiftThaw/Settings/Models/ProfileManager.swiftThaw/Settings/SettingsPanes/DisplaySettingsPane.swift
✅ Files skipped from review due to trivial changes (1)
- Thaw/Resources/Localizable.xcstrings
This was referenced May 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Two related fixes to the Displays > Menu Bar Item Spacing flow that together stop Thaw from breaking pre-existing user setups:
↺reset) buttons that doubles as a destructive-action warning for the relaunch wave and as the persistence channel that writes the new spacing into the active profile (or every profile) before the reapply pass fires.NSStatusItemSpacing(set viadefaults writein Terminal) is adopted as the implicit baseline instead of overwritten by Thaw's default of 0.PR Type
Does this PR introduce a breaking change?
What is the current behavior?
Issue Number: #598, #602, #619
With a profile active, dragging the Menu Bar Item Spacing slider for the active display and clicking Apply triggered a relaunch wave for all menu bar apps. Once the wave settled,
DisplaySettingsManager.applyActiveDisplaySpacingcalledProfileManager.reapplyActiveProfile(), which re-applied the active profile's storeddisplayConfigurations. Those still held the previous offset, so the just-applied value was silently overwritten, and a second relaunch wave fired to apply the reverted value. The Reset (↺) button next to Apply had the same problem because it also calledupdateConfigurationimmediately. The user's documented workaround in #619 was to export the profile, delete it, change the setting, reimport, and re-save.Separately, users who had set
NSStatusItemSpacing(and/orNSStatusItemSelectionPadding) externally viadefaults writesaw Thaw fire a relaunch wave on every startup.applyActiveDisplaySpacingread the per-display default offset of 0, computed target = 16, found the on-disk value at e.g. 6, and rewrote it to 16, restarting every menu bar app in the process. The same wave fired again on screen-parameter changes (sleep/wake, monitor connect/disconnect, Sidecar handshake), giving the recurring app-restart symptom reported in #602 and #598.What is the new behavior?
Confirmation gate in
DisplaySettingsPane. The Apply button and the inline↺reset button now route through a singlerequestSpacingApplyhelper. When the change targets the active menu bar display, an alert names the relaunch wave; when a profile is active, the alert offersUpdate Active ProfileandUpdate All Profilesalongside Cancel.Update Active ProfilecallsdisplaySettings.updateConfigurationand thenprofileManager.updateProfile(scope: .configurationOnly)synchronously, so the profile file is on disk before the Combine sink dispatchesapplyActiveDisplaySpacing. The subsequentreapplyActiveProfileloads the new spacing instead of the old one.Update All Profilescalls a newProfileManager.updateAllProfilesItemSpacingOffset(displayUUID:offset:)helper that walks every profile and writes the new offset into itsdisplayConfigurations[displayUUID]viawithItemSpacingOffset, inserting adefaultConfigurationentry where the profile has no prior entry for that display.DisplaySettingsManager.activeMenuBarDisplayUUIDaccessor wrapsBridging.getActiveMenuBarDisplayUUID()so the pane can decide whether a change targets the active display.First-launch seed in
DisplaySettingsManager.loadInitialState. When no per-display configurations have been persisted yet andNSStatusItemSpacingdiffers from the macOS default of 16, the manager reads the on-disk value, computesoffset = onDisk - 16, and seeds every connected display'sitemSpacingOffsetwith that offset. The seeded dictionary is written toDefaults.displayIceBarConfigurationsinline (the Combine persistence sink is not wired yet at that point), so adoption is one-shot. The next timeapplyActiveDisplaySpacingruns, its target matches on-disk and the no-op guard skips the relaunch wave.NSStatusItemSelectionPaddingis not consulted because Thaw drives both keys from a single offset; users whose padding diverges from spacing see one normalising relaunch on first launch and none thereafter.New strings (
Apply spacing change?, two profile-aware message variants, the relaunch-only message,Update Active Profile,Update All Profiles) are added toLocalizable.xcstringswith empty localizations for Crowdin pickup.PR Checklist
Other information
The confirmation-gate fix relies on a timing invariant: the Combine sink that drives
applyActiveDisplaySpacingis.receive(on: DispatchQueue.main).sink { ... }, so it runs on the next main-runloop turn. The confirmation action callsupdateConfigurationand thenupdateProfile/updateAllProfilesItemSpacingOffsetsynchronously on the main thread before yielding, which guarantees the profile file(s) are written before the sink fires andreapplyActiveProfilereads them back.The seeding fix persists inline rather than relying on the standard Combine persistence sink, because the sink's
.dropFirst()would discard the seeded emission since it is set up afterloadInitialState. Without inline persistence the seed would re-run on every launch and never make it intoDefaults.displayIceBarConfigurations.Fixes #598, #602, #619
Summary by CodeRabbit
New Features
Bug Fixes