fix(menubar): exclude parked items from the display-spread gate - #930
Merged
diazdesandi merged 2 commits intoAug 10, 2026
Merged
Conversation
`LayoutSolver.itemsSpanMultipleDisplays` was documented as ignoring parked hidden items on the grounds that the control item shoves them "thousands of points to the left", onto no display at all. That only holds when no screen occupies negative x. On an arrangement with a display positioned to the left of the main one, hidden-section items park at coordinates that fall squarely inside that display's frame, so the predicate matched two distinct screens and reported a spread on a completely settled layout. Both gates that consult the predicate then fired continuously. The persist gate in the cache-update path skipped `saveSectionOrder` on every cycle: a field log covering a single day recorded 1088 skips and zero successful writes, leaving `MenuBarItemManager.savedSectionOrder` on disk as three empty arrays. With the saved order frozen, any item the user moved into always-hidden was absent from `desiredAH`, so `partitionUnmanagedUIDs` kept it, `planUnmanagedPlacement` classified it as a new item, and every window-ID change dragged it back to the new-items section. Quitting any app was enough to trigger it. Callers now exclude parked items before consulting the predicate rather than relying on them landing on no display. The persist path passes visible-section centers only; `applySavedLayout` and `rebalanceNotchOverflowIfNeeded` filter to items at or right of the hidden divider's `minX`. A genuine relocation still splits those items across screens, so the protection against applying or persisting mid-migration geometry is unchanged. Two related repairs travel with it. `applySavedLayout` regains the display-spread guard that `d2824fa4` dropped while leaving the persist-side guard in place; that asymmetry is what made the stale layout visible, because applies kept running against an order that could no longer be saved. And `rebalanceNotchOverflowIfNeeded` now derives its screen frames from `CGDisplayBounds` rather than `NSScreen.frame`, so the rectangles share the top-left origin space of the item bounds they are tested against instead of mixing it with AppKit's bottom-left origin. The persist-side skip is logged at warning rather than debug, matching the sibling branches around it. A run of these means the saved layout has stopped tracking the user's arrangement entirely, and the symptom that surfaces gives no hint that persistence is the cause. Signed-off-by: Amir Zarrinkafsh <3339418+nightah@users.noreply.github.com>
github-actions
Bot
added
fix
Contributor
|
Labels: All convention checks passed. ✅ cc @nightah |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughDisplay-spread detection now excludes parked hidden and always-hidden items from layout persistence, saved-layout application, and notch-overflow rebalancing. Tests cover parked coordinates on left displays and unparked cross-display placement. ChangesDisplay spread filtering
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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: 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/MenuBar/MenuBarItems/MenuBarItemManager.swift`:
- Around line 9024-9030: Update the caller and the method containing the
`unparkedItems` calculation to receive the hidden divider position from the
retained control-item data, such as `controlItems.hidden.bounds.minX` or the
`ControlItemPair` itself. Use that passed value to always filter items before
calling `LayoutSolver.itemsSpanMultipleDisplays`, including the
`cacheItemsRegardless` path where `.hiddenControlItem` has already been removed
from `items`.
🪄 Autofix
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9b407a6e-dbfc-4927-90a6-dc279adc6741
📒 Files selected for processing (3)
Thaw/MenuBar/MenuBarItems/LayoutSolver.swiftThaw/MenuBar/MenuBarItems/MenuBarItemManager.swiftThawTests/MenuBar/Items/DisplaySpreadGateTests.swift
nightah
force-pushed
the
fix/exclude-parked-items-from-display-spread-gate
branch
2 times, most recently
from
August 10, 2026 23:35
959b049 to
8673dd7
Compare
`rebalanceNotchOverflowIfNeeded` looked its hidden divider up by searching the item array it was handed for `.hiddenControlItem`. Its only caller is `cacheItemsRegardless`, which builds its `ControlItemPair` with an inout array, and that initializer removes the hidden and always-hidden control items from the array it is given. The caller then passes that same stripped array down, so the lookup always found nothing, the optional binding never fired, and the display-spread gate fell back to its unfiltered branch and kept receiving parked items. The pair is now passed in from the caller and the filter runs unconditionally against `controlItems.hidden.bounds.minX`. The function no longer re-derives its own `ControlItemPair` from that stripped array either. The copy it built for the purpose was used nowhere else, and the `guard` around it could not succeed once the control items had been removed upstream. The doc comment on `lastNotchRebalanceTimestamp` drops its DocC backticks while being updated for the new signature, per the project convention that backticks in Swift comments are reserved for GitHub-rendered surfaces. Signed-off-by: Amir Zarrinkafsh <3339418+nightah@users.noreply.github.com>
nightah
force-pushed
the
fix/exclude-parked-items-from-display-spread-gate
branch
from
August 10, 2026 23:36
8673dd7 to
430ab7c
Compare
diazdesandi
approved these changes
Aug 10, 2026
|
diazdesandi
deleted the
fix/exclude-parked-items-from-display-spread-gate
branch
August 10, 2026 23:42
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
LayoutSolver.itemsSpanMultipleDisplaysassumed parked hidden items land on no display, which is false whenever a screen sits to the left of the main one. The predicate then reported a spread on a settled layout, permanently, and the persist gate it feeds stoppedsaveSectionOrderfrom ever running. Callers now exclude parked items before consulting it.Scope: This PR changes one focused thing (bug fix or feature) plus minimal plumbing. Larger refactors need prior agreement in the issue.
Linked issue (required)
Closes: N/A
Reported directly against
2.0.0-rc.2.1with diagnostic logs rather than through the tracker. Not a duplicate of #927, which reports a similar symptom on a single monitor, where this predicate short-circuits atguard screenFrames.count > 1.PR Type
Area
Does this PR introduce a breaking change?
What is the new behavior?
itemsSpanMultipleDisplayswas documented as ignoring parked hidden items because the control item shoves them "thousands of points to the left", onto no display. That assumption only holds when no screen occupies negative x. On the reporter's arrangement (main atx=03440 wide, a second display atx=3440, and a third atx=-2560) the hidden section parks atx ≈ -2400to-2570, which is inside the third display's frame. The predicate matched two distinct screens and returnedtrueon a completely settled layout, for as long as that display stayed connected.Both consumers then fired continuously. The persist gate skipped
saveSectionOrderon every cache cycle: one field log covering a single day recorded 1088 skips and zero successful writes, andMenuBarItemManager.savedSectionOrderon disk was three empty arrays. Everything the user saw came from the active display profile's frozen snapshot, becausearmProfileStateoverwrites the in-memory copy at startup whileProfileManager.captureCurrentLayoutreads the empty persisted one.With the saved order unable to advance, any item moved into always-hidden stayed absent from
desiredAH, sopartitionUnmanagedUIDskept it andplanUnmanagedPlacementclassified it as a new item bound for the new-items section. Quitting any app changed the window-ID set, which dispatched a bulk apply, which dragged the item back to visible. From the reporter's log:The log corroborates the geometry independently: the hidden section captured cleanly at those coordinates while the always-hidden items at
x ≈ -7660producedcaptureWindowsImageSCK: no display intersects. Hidden items were on a real display; always-hidden ones were not.What changed
Callers exclude parked items rather than relying on them falling outside every screen frame. The persist path passes visible-section centers only;
applySavedLayoutandrebalanceNotchOverflowIfNeededfilter to items at or right of the hidden divider'sminX. A real relocation still splits those items across screens, so deferring on mid-migration geometry is unchanged.Two related repairs travel with it:
applySavedLayoutregains the display-spread guard thatd2824fa4dropped while leaving the persist-side guard in place. Both halves landed together in4c04159e; the asymmetry is what made this visible rather than merely inert, because applies kept running against an order that could no longer be saved.rebalanceNotchOverflowIfNeededderives its screen frames fromCGDisplayBoundsinstead ofNSScreen.frame, so the rectangles share the top-left origin space of the item bounds they are tested against rather than mixing it with AppKit's bottom-left origin.The persist-side skip is logged at
warningrather thandebug, matching the sibling branches around it. A run of these means the saved layout has stopped tracking the user's arrangement, and the symptom that surfaces gives no hint that persistence is the cause.Regression range
4c04159e(2026-06-24) introduced the predicate and both gates.2.0.0-rc.1is dated 2026-06-16, so this regressed inside the rc.1 to rc.2 window and shipped in2.0.0-rc.2and2.0.0-rc.2.1.PR Checklist
swiftformat .to keep the code style consistent.xcodebuild test …orswift test --package-path MenuBarModel.developmentbranch.Package.resolved, Actions pins, etc.),dependency-scais green — or anyosv-scanner.tomlsuppression includes bothreasonandignoreUntil(see SECURITY.md § Dependency SCA policy).Test commands run:
xcodebuild test -project Thaw.xcodeproj -scheme Thaw -destination 'platform=macOS' -only-testing:ThawTests/DisplaySpreadGateTestsxcodebuild test -project Thaw.xcodeproj -scheme Thaw -destination 'platform=macOS'(full suite: 1993 tests in 257 suites)swiftformatwas run against the touched files only.swiftlintis clean on all three touched files.Verified on the reporting machine
Debug build run against the three-display arrangement that reproduces the bug:
span multiple displaysskipsSaved section order:writesplanUnmanagedPlacementfor the affected itemMenuBarItemManager.savedSectionOrderwent from three empty arrays to a populated dict with the affected item present inalwaysHidden, which is the condition that stopspartitionUnmanagedUIDsfrom picking it up at all.Other information
The predicate keeps its signature and its behaviour of ignoring centers that match no screen. That fallback is still useful, but its doc comment now states plainly that it is not the filter and that callers own the exclusion, so the next caller does not re-derive the same assumption.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by CodeRabbit
Bug Fixes
Documentation
Tests