fix: keep menu bar spacing global#552
Closed
CamilleGuillory wants to merge 2 commits intostonerl:developmentfrom
Closed
fix: keep menu bar spacing global#552CamilleGuillory wants to merge 2 commits intostonerl:developmentfrom
CamilleGuillory wants to merge 2 commits intostonerl:developmentfrom
Conversation
Restore menu bar item spacing to General settings because macOS applies NSStatusItemSpacing as a single host-wide preference, not as a per-display value. Co-authored-by: Cursor <[email protected]>
Contributor
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR relocates menu bar item spacing from per-display ChangesGlobal Menu Bar Item Spacing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
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 unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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
🧹 Nitpick comments (2)
Thaw/Settings/Models/ProfileManager.swift (1)
236-236: ⚡ Quick winAlign direct
spacingManager.offsetassignment with the clamping applied by the$itemSpacingOffsetsink.The
$itemSpacingOffsetCombine sink inGeneralSettingsguards withmax(-16, min(16, offset))before assigningspacingManager.offset. Line 236 setsspacingManager.offsetdirectly without applying the same guard. WhileGeneralSettingsSnapshotclamps during decoding, the two code paths are now inconsistent — if an unclamped value ever reaches this point,applyOffset()would propagate the out-of-range integer to the system.🛡️ Proposed fix
- appState.spacingManager.offset = Int(profile.generalSettings.itemSpacingOffset.rounded()) + appState.spacingManager.offset = Int(max(-16, min(16, profile.generalSettings.itemSpacingOffset)).rounded())🤖 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/ProfileManager.swift` at line 236, The assignment to appState.spacingManager.offset uses profile.generalSettings.itemSpacingOffset directly and can bypass the clamping logic used by the $itemSpacingOffset sink; change this to clamp the value the same way (e.g., compute let clamped = max(-16, min(16, profile.generalSettings.itemSpacingOffset)) and assign appState.spacingManager.offset = Int(clamped.rounded())) or call the existing applyOffset() path that enforces the clamp so spacingManager.offset cannot receive out-of-range values; update references to spacingManager.offset, profile.generalSettings.itemSpacingOffset, $itemSpacingOffset sink, applyOffset(), and GeneralSettingsSnapshot accordingly.Thaw/Settings/Models/Profile.swift (1)
41-80:GeneralSettingsSnapshotchanges are well-structured.The clamping-at-init approach correctly covers both the construct-from-settings path (
capture(from:)) and the decode path (init(from:)→self.init(...)), whiledecodeIfPresent+?? Defaults.DefaultValue.itemSpacingOffsetprovides clean backward-compatible deserialization for profiles that predate this field.One low-priority note: the clamp bounds (
-16,16) on line 127 are duplicated in theIceSliderrangein: -16 ... 16inGeneralSettingsPane.swift. Extracting them into a shared constant inDefaultswould prevent silent drift if either is changed independently.Also applies to: 126-170
🤖 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/Profile.swift` around lines 41 - 80, The -16/16 bounds for item spacing are duplicated between the clampItemSpacingOffset logic (used in Profile.init / clampItemSpacingOffset) and the IceSlider range (in: -16 ... 16) in GeneralSettingsPane.swift; extract them into a single shared constant on Defaults (for example Defaults.DefaultValue.itemSpacingOffsetMin and itemSpacingOffsetMax or a Range/ClosedRange like Defaults.DefaultValue.itemSpacingOffsetRange) and replace the hardcoded -16/16 uses in clampItemSpacingOffset and the IceSlider range with that constant to keep the bounds consistent.
🤖 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/GeneralSettings.swift`:
- Line 117: When loading itemSpacingOffset in loadInitialState via
Defaults.ifPresent(key: .itemSpacingOffset, assign: &itemSpacingOffset), clamp
the value to the allowed range before assigning so in-memory state matches the
persisted/effective value (use the same clamp logic used in the
$itemSpacingOffset sink and spacingManager.offset); additionally, for robustness
consider writing the clamped value back to Defaults inside the
$itemSpacingOffset sink (or keep removeDuplicates() to avoid feedback storms) so
programmatic mid-session sets are normalized — apply the same change to the
other similar load sites noted (lines ~181-192).
In `@Thaw/Settings/SettingsPanes/GeneralSettingsPane.swift`:
- Around line 286-298: The commit to the model (settings.itemSpacingOffset) is
done before the async apply and on error leaves the model equal to
tempItemSpacingOffset so Apply/Reset become disabled; in
applyTempItemSpacingOffset() call appState.spacingManager.applyOffset() first
(or if you must set the model prior, then on catch reset
settings.itemSpacingOffset back to the previous value stored before assignment)
and only clear isApplyingItemSpacingOffset after success/rollback so the
Apply/Reset buttons remain enabled after a failed apply; reference
settings.itemSpacingOffset, tempItemSpacingOffset, applyTempItemSpacingOffset(),
appState.spacingManager.applyOffset(), and isApplyingItemSpacingOffset when
making the change.
---
Nitpick comments:
In `@Thaw/Settings/Models/Profile.swift`:
- Around line 41-80: The -16/16 bounds for item spacing are duplicated between
the clampItemSpacingOffset logic (used in Profile.init / clampItemSpacingOffset)
and the IceSlider range (in: -16 ... 16) in GeneralSettingsPane.swift; extract
them into a single shared constant on Defaults (for example
Defaults.DefaultValue.itemSpacingOffsetMin and itemSpacingOffsetMax or a
Range/ClosedRange like Defaults.DefaultValue.itemSpacingOffsetRange) and replace
the hardcoded -16/16 uses in clampItemSpacingOffset and the IceSlider range with
that constant to keep the bounds consistent.
In `@Thaw/Settings/Models/ProfileManager.swift`:
- Line 236: The assignment to appState.spacingManager.offset uses
profile.generalSettings.itemSpacingOffset directly and can bypass the clamping
logic used by the $itemSpacingOffset sink; change this to clamp the value the
same way (e.g., compute let clamped = max(-16, min(16,
profile.generalSettings.itemSpacingOffset)) and assign
appState.spacingManager.offset = Int(clamped.rounded())) or call the existing
applyOffset() path that enforces the clamp so spacingManager.offset cannot
receive out-of-range values; update references to spacingManager.offset,
profile.generalSettings.itemSpacingOffset, $itemSpacingOffset sink,
applyOffset(), and GeneralSettingsSnapshot accordingly.
🪄 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: e7f5666f-ba9b-45da-90a6-e37b55b29178
📒 Files selected for processing (14)
Thaw/MenuBar/IceBar/IceBar.swiftThaw/MenuBar/MenuBarItems/MenuBarItemManager.swiftThaw/MenuBar/Spacing/MenuBarItemSpacingManager.swiftThaw/Settings/Models/DisplayIceBarConfiguration.swiftThaw/Settings/Models/DisplaySettingsManager.swiftThaw/Settings/Models/GeneralSettings.swiftThaw/Settings/Models/Profile.swiftThaw/Settings/Models/ProfileManager.swiftThaw/Settings/Models/SettingsResetter.swiftThaw/Settings/SettingsPanes/DisplaySettingsPane.swiftThaw/Settings/SettingsPanes/GeneralSettingsPane.swiftThaw/Utilities/Defaults.swiftThawTests/DisplayIceBarConfigurationTests.swiftThawTests/GeneralSettingsSnapshotTests.swift
💤 Files with no reviewable changes (1)
- Thaw/Settings/SettingsPanes/DisplaySettingsPane.swift
Clamp restored spacing values and roll back the draft when applying spacing fails so users can retry without extra slider changes. Co-authored-by: Cursor <[email protected]>
Owner
|
Closing this one for now, since it is not what we want as discussed in #551 |
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.
Summary
NSStatusItemSpacing/NSStatusItemSelectionPaddingbehavior.Fixes #551
Test plan
ReadLintson touched Swift filesgit diff --checkswiftformat(not installed in this environment)xcodebuild test -project Thaw.xcodeproj -scheme Thaw -derivedDataPath Build/ -destination 'platform=macOS' -enableCodeCoverage YES CODE_SIGN_IDENTITY="" CODE_SIGNING_REQUIRED=NO CODE_SIGNING_ALLOWED=NObecause the active developer directory is Command Line Tools, not full XcodeMade with Cursor
Summary by CodeRabbit
New Features
Removed Features
Tests