fix(menubar): restore launch layout and stop move/storm loops - #889
Open
diazdesandi wants to merge 90 commits into
Open
fix(menubar): restore launch layout and stop move/storm loops#889diazdesandi wants to merge 90 commits into
diazdesandi wants to merge 90 commits into
Conversation
Signed-off-by: René Jiménez <diazdesandi@proton.me>
Cherry-pick of a1bb846 from feat/macos-27-experimental, so the 26/27 shared codebase does not diverge on image handling. Introduce `AlphaChannelView` to encapsulate alpha channel access, buffer validation, and pixel/row transparency checks. This centralizes bounds validation, preventing potential out-of-bounds reads during image processing, and simplifies the `TransparencyContext` and `isTransparent(fastPath:)` implementations. Signed-off-by: René Jiménez <diazdesandi@proton.me>
Signed-off-by: René Jiménez <diazdesandi@proton.me>
Signed-off-by: René Jiménez <diazdesandi@proton.me>
…ult off) Signed-off-by: René Jiménez <diazdesandi@proton.me>
Signed-off-by: René Jiménez <diazdesandi@proton.me>
github-actions
Bot
added
enhancement
Contributor
|
Labels: Warning This PR looks large (130 files, 17945 lines changed). Consider splitting if possible (see CONTRIBUTING: aim ≤20 files / ≤500 LOC). cc @diazdesandi |
Contributor
|
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:
📝 WalkthroughWalkthroughThis PR refactors menu-bar layout restoration and move execution, adds saved-order safeguards, introduces Thaw Bar and cursor settings, separates appearance borders, hardens capture utilities, and adds regression coverage. ChangesLayout reliability
Settings, routing, and appearance
Capture and utility hardening
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant MenuBarItemManager
participant LayoutSolver
participant MenuBarItem
participant Accessibility
participant SavedOrder
MenuBarItemManager->>SavedOrder: load and prune saved section order
MenuBarItemManager->>LayoutSolver: build LCS move sequence
LayoutSolver->>MenuBarItem: resolve canonical item identity
MenuBarItemManager->>Accessibility: execute and verify move
Accessibility-->>MenuBarItemManager: landing or displacement outcome
MenuBarItemManager->>SavedOrder: persist only after move-batch gates pass
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 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: 7
🤖 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 7907-7911: Renumber the sub-phase comments around the control-item
movement logic so their labels match execution order: the block currently marked
“Sub-phase 0” at the later location executes first and should be “Sub-phase 1,”
while the earlier block currently marked “Sub-phase 1” should be “Sub-phase 0.”
- Around line 4484-4494: The landing check in postMoveEvents duplicates move()’s
verification and performs an unnecessary Window Server query while
eventSemaphore is held. Remove the itemHasCorrectPosition call from
postMoveEvents, return or otherwise propagate the move outcome to move(), and
use move()’s single position check to supply the landed versus
displacedWithoutLanding result to nextMoveOperationTimeout.
- Line 8069: Recompute the section classification after the H_ctrl move in the
surrounding MenuBarItemManager flow, updating currentVisibleSet,
currentHiddenSet, and currentAHSet from the post-move sectionByWindowID state.
Recalculate crossSectionMoves and totalSectionMismatch from this fresh
classification, then use those values in the always-hidden gate guarding the
ahCtrlUID block so per-item fallback runs only when post-move mismatches remain.
- Around line 4346-4350: Update the notch override branch around
notchMouseDownPoint so it only applies when useGestureGeometry is false; in
gesture mode, preserve pressPoint and the matching postGestureDragSteps
interpolation path. Keep the existing legacy offscreen notch behavior unchanged.
In `@Thaw/Utilities/Defaults.swift`:
- Around line 278-283: Remove all remaining references to
useLCSSortingOnNotchedDisplays from docs/URI_SCHEMES.md and the ThawCtl
ContentView export list, ensuring URI inputs and CLI exports no longer advertise
the deleted setting. If the URI handler still accepts the raw legacy key, add a
one-shot migration mapping it before removal; otherwise remove its handling
consistently.
In `@ThawTests/Fixtures/LayoutStormLog.swift`:
- Around line 92-97: Change sectionMap from a computed static var into a stored
static let initialized once from currentVisible and currentHidden, preserving
the existing visible/hidden mapping.
In `@ThawTests/MenuBar/Items/SectionOrderDigestTests.swift`:
- Line 100: Update the `after` assignment in the section-order digest test to
materialize `before.reversed()` as an `Array` before concatenating the
additional item, preserving the existing reversed ordering and appended value.
🪄 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: 3cfc950d-e2af-4a89-970a-b5649992f4c5
📒 Files selected for processing (22)
Thaw/MenuBar/MenuBarItems/LayoutSolver.swiftThaw/MenuBar/MenuBarItems/MenuBarItemManager.swiftThaw/Resources/Localizable.xcstringsThaw/Settings/Models/AdvancedSettings.swiftThaw/Settings/Models/Profile.swiftThaw/Settings/Search/SearchIndex.swiftThaw/Settings/SettingsPanes/AdvancedSettingsPane.swiftThaw/Settings/SettingsPanes/MenuBarLayoutSettingsPane.swiftThaw/Utilities/Defaults.swiftThaw/Utilities/Extensions.swiftThaw/Utilities/SettingsURIHandler.swiftThawTests/Fixtures/LayoutStormLog.swiftThawTests/MenuBar/Items/MoveOperationTimeoutTests.swiftThawTests/MenuBar/Items/SectionOrderDigestTests.swiftThawTests/MenuBar/Layout/LayoutStormReplayTests.swiftThawTests/MenuBar/Layout/PlanFullSortSequenceTests.swiftThawTests/Settings/Models/AdvancedSettingsSnapshotTests.swiftThawTests/Settings/Models/AdvancedSettingsTests.swiftThawTests/Settings/Models/ProfileDecodingDefaultsTests.swiftThawTests/Settings/Search/SearchIndexTests.swiftThawTests/Settings/URI/SettingsURIHandlerCoverageTests.swiftThawTests/Support/ProfileTestFixtures.swift
💤 Files with no reviewable changes (12)
- ThawTests/Settings/URI/SettingsURIHandlerCoverageTests.swift
- Thaw/Utilities/SettingsURIHandler.swift
- ThawTests/MenuBar/Layout/PlanFullSortSequenceTests.swift
- ThawTests/Settings/Search/SearchIndexTests.swift
- Thaw/MenuBar/MenuBarItems/LayoutSolver.swift
- ThawTests/Support/ProfileTestFixtures.swift
- ThawTests/Settings/Models/ProfileDecodingDefaultsTests.swift
- ThawTests/Settings/Models/AdvancedSettingsTests.swift
- Thaw/Resources/Localizable.xcstrings
- Thaw/Settings/Models/Profile.swift
- ThawTests/Settings/Models/AdvancedSettingsSnapshotTests.swift
- Thaw/Settings/SettingsPanes/MenuBarLayoutSettingsPane.swift
3 tasks
LyricsX titles its menu bar item with the lyric line currently on screen, and identity is namespace:title. Every song change — every line — minted a fresh uniqueIdentifier, so the item read as a brand-new arrival and was filed under the user's new-items section. Dragging the lyrics into visible could not stick, because the item that was moved stopped existing at the next line.
The volatile-title machinery already existed for iStat Menus, which has the same problem in a different shape, but it did not cover this. Two reasons:
1. The owner allowlist held exactly one bundle identifier.
2. canonicalMetricTitle collapses digits, and a lyric has none. Adding LyricsX to the existing list would have changed nothing —"I walked through the door" canonicalizes to itself.
So this adds a second title shape rather than a second entry. A metric title has a stable skeleton worth keeping ("CPU #" and "Network #" still tell two iStat items apart); a lyric has none, every character is the volatile part.
For such an owner the whole title collapses to a constant, leaving the instance index as the only distinguishing part. That is sufficient while the owner contributes one item, and is why this stays an allowlist rather than becoming a heuristic.
Signed-off-by: René Jiménez <diazdesandi@proton.me>
Signed-off-by: René Jiménez <diazdesandi@proton.me>
Signed-off-by: René Jiménez <diazdesandi@proton.me>
…ows (#794) Signed-off-by: René Jiménez <diazdesandi@proton.me>
Signed-off-by: René Jiménez <diazdesandi@proton.me>
…890) Signed-off-by: René Jiménez <diazdesandi@proton.me>
Signed-off-by: René Jiménez <diazdesandi@proton.me>
Signed-off-by: VailElla <63608800+VailElla@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Thaw/MenuBar/MenuBarItems/MenuBarItemTag.swift (1)
258-273: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse canonical identity during saved-position lookup.
This function makes two LyricsX titles one persistent identity.
LayoutSolver.savedPositionByBaseIDstill compares rawnamespace:titlevalues. DuringplanUnmanagedPlacement, a current lyric misses its saved lyric entry and falls through tonewItemDefault. A profile apply can then move LyricsX into the configured new-items section.After raw exact matching fails, compare canonical identifiers before the base-ID fallback. Preserve the instance index. Add a profile-placement regression test.
🤖 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/MenuBar/MenuBarItems/MenuBarItemTag.swift` around lines 258 - 273, Update LayoutSolver.savedPositionByBaseID and its planUnmanagedPlacement lookup so that, after raw namespace:title matching fails, it compares canonicalPersistentIdentifier values before applying the base-ID fallback, preserving any instance index. Add a profile-placement regression test verifying a canonicalized LyricsX identity reuses its saved position instead of newItemDefault.
🤖 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 `@Shared/Bridging/Bridging.swift`:
- Around line 774-781: Update the recovery branch in captureWindowsImageSCK so a
successful shareableContentIncludingOffscreen refresh replaces the cached
snapshot, re-resolves the requested windows, and recomputes unionBounds from
fresh before creating the SCContentFilter and source rect. Re-evaluate
hostDisplay(in:) against the refreshed windows, and retain a caller-provided
screenBounds only when its coordinate contract remains valid for the refreshed
topology.
In `@Thaw/MenuBar/MenuBarItems/LayoutSolver.swift`:
- Around line 1209-1223: Update the saved-order repair flow around the
section-wise mapValues filter so canonical identifiers are deduplicated across
the complete saved order rather than resetting seenCanonical per section.
Establish deterministic section precedence, retain each canonical identifier
only in its preferred section before planLeftmostMove builds its lookup, and add
a regression test covering the same canonical LyricsX or iStat identifier
appearing in different sections.
In `@ThawTests/MenuBar/ControlItem/ControlItemDefaultsSeedingTests.swift`:
- Around line 21-76: Update ControlItemDefaultsSeedingTests to isolate and
restore shared Defaults.store state: mark the suite serialized, and ensure each
test that mutates the hidden or visible preferred position preserves its
original value and restores it with defer (or use a scratch defaults store).
Keep the existing assertions and seeding behavior unchanged.
---
Outside diff comments:
In `@Thaw/MenuBar/MenuBarItems/MenuBarItemTag.swift`:
- Around line 258-273: Update LayoutSolver.savedPositionByBaseID and its
planUnmanagedPlacement lookup so that, after raw namespace:title matching fails,
it compares canonicalPersistentIdentifier values before applying the base-ID
fallback, preserving any instance index. Add a profile-placement regression test
verifying a canonicalized LyricsX identity reuses its saved position instead of
newItemDefault.
🪄 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: 31aea040-96d0-4200-befa-f9a6563ee369
⛔ Files ignored due to path filters (1)
Thaw.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolvedis excluded by!**/Package.resolved
📒 Files selected for processing (14)
Shared/Bridging/Bridging.swiftThaw.xcodeproj/project.pbxprojThaw/MenuBar/ControlItem/ControlItem.swiftThaw/MenuBar/MenuBarItems/LayoutSolver.swiftThaw/MenuBar/MenuBarItems/MenuBarItemManager.swiftThaw/MenuBar/MenuBarItems/MenuBarItemTag.swiftThaw/MenuBar/Spacing/MenuBarItemSpacingManager.swiftThaw/Resources/Info.plistThaw/Settings/SettingsPanes/MenuBarLayoutSettingsPane.swiftThaw/Utilities/HookRunner.swiftThawTests/MenuBar/ControlItem/ControlItemDefaultsSeedingTests.swiftThawTests/MenuBar/Items/MenuBarItemTagCanonicalizationTests.swiftThawTests/MenuBar/Layout/PrunedSectionOrderTests.swiftThawTests/Utilities/ShareableContentCacheTests.swift
Contributor
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 (2)
ThawTests/MenuBar/Items/SectionOrderDigestTests.swift (1)
27-27: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winMaterialize the reversed collection before calling
orderDigest.
MenuBarItemManager.orderDigestaccepts[String], butitems.reversed()has typeReversedCollection<[String]>. Swift does not implicitly convert this view to an array, so the test does not compile.Proposed fix
- != MenuBarItemManager.orderDigest(items.reversed()) + != MenuBarItemManager.orderDigest(Array(items.reversed()))Verify the declaration and call with:
#!/bin/bash set -euo pipefail rg -n -C 3 \ 'static nonisolated func orderDigest|orderDigest\(items\.reversed\(\)\)' \ Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swift \ ThawTests/MenuBar/Items/SectionOrderDigestTests.swift🤖 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 `@ThawTests/MenuBar/Items/SectionOrderDigestTests.swift` at line 27, Materialize the reversed collection before passing it to MenuBarItemManager.orderDigest in the SectionOrderDigestTests assertion, converting items.reversed() to the required [String] array while preserving the existing comparison.Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swift (1)
4476-4484: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSend gesture drag events before waiting for displacement.
When
useGestureGeometryis true, Line 4471 waits for an origin change before Line 4477 sends the firstmouseDraggedevent. A normal drag owner moves only aftermouseDragged. The wait can time out, and the fallback sendsmouseUpwithout sending any drag event.Send
postGestureDragStepsimmediately aftermouseDown. Then wait for the origin change. Keep the existing wait order for legacy press-at-destination moves.Proposed fix
try await scrombleEvent( mouseDown, item: item, timeout: timeout ) + if useGestureGeometry { + try await postGestureDragSteps( + item: item, + source: source, + from: pressPoint, + to: targetPoints.end, + warpAlongPath: warpIsOnScreen + ) + } itemOrigin = try await waitForMoveEventResponse( from: item, initialOrigin: itemOrigin, timeout: timeout ) - if useGestureGeometry { - try await postGestureDragSteps( - item: item, - source: source, - from: pressPoint, - to: targetPoints.end, - warpAlongPath: warpIsOnScreen - ) - }🤖 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/MenuBar/MenuBarItems/MenuBarItemManager.swift` around lines 4476 - 4484, Reorder the useGestureGeometry drag flow so postGestureDragSteps is invoked immediately after mouseDown, before waiting for the origin change. Then retain the displacement wait and existing fallback behavior, while preserving the current wait-before-drag order for legacy press-at-destination moves.
🤖 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 `@Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swift`:
- Around line 4476-4484: Reorder the useGestureGeometry drag flow so
postGestureDragSteps is invoked immediately after mouseDown, before waiting for
the origin change. Then retain the displacement wait and existing fallback
behavior, while preserving the current wait-before-drag order for legacy
press-at-destination moves.
In `@ThawTests/MenuBar/Items/SectionOrderDigestTests.swift`:
- Line 27: Materialize the reversed collection before passing it to
MenuBarItemManager.orderDigest in the SectionOrderDigestTests assertion,
converting items.reversed() to the required [String] array while preserving the
existing comparison.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 356ffa20-40d2-4269-880b-885c8f169c99
📒 Files selected for processing (5)
Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swiftThawCtl/Sources/ThawCtl/ContentView.swiftThawTests/Fixtures/LayoutStormLog.swiftThawTests/MenuBar/Items/SectionOrderDigestTests.swiftdocs/URI_SCHEMES.md
💤 Files with no reviewable changes (1)
- docs/URI_SCHEMES.md
Signed-off-by: René Jiménez <diazdesandi@proton.me>
REVERT BEFORE MERGE. This exists so a build handed to the reporters on #854, #905, #923, #924 and #927 exercises the fixes without asking them to run `defaults write` first — three testers, three chances to forget, and a "still broken" report that means nothing. postMoveEventsToWindowOwner false → true bulkApplyIdleThresholdMs 0 → 300 enforceConcealedSectionOrder true → false automaticArrangementEnabled is deliberately left alone. Its enabled state *stops* automatic arranging, so turning it on would disable the late-arrival re-sort and the saved-layout restore — the behaviour the testers need to exercise — and mask every other fix while reading as "Thaw stopped restoring my layout". Each flag keeps its shipping default documented on the Defaults.Key case; only DefaultValue moves. Reverting this commit restores the shipping posture exactly. Signed-off-by: Rene <diazdesandi@proton.me>
github-actions
Bot
added
onboarding
Bumps the swift group with 3 updates: [github.com/sparkle-project/sparkle](https://github.com/sparkle-project/Sparkle), [github.com/swiftlang/swift-subprocess](https://github.com/swiftlang/swift-subprocess) and [github.com/apple/swift-system](https://github.com/apple/swift-system). Updates `github.com/sparkle-project/sparkle` from 2.9.4 to 2.9.5 - [Release notes](https://github.com/sparkle-project/Sparkle/releases) - [Changelog](https://github.com/sparkle-project/Sparkle/blob/2.x/CHANGELOG) - [Commits](sparkle-project/Sparkle@2.9.4...79bc9e8) Updates `github.com/swiftlang/swift-subprocess` from 0.5.0 to 1.0.0 - [Release notes](https://github.com/swiftlang/swift-subprocess/releases) - [Commits](swiftlang/swift-subprocess@0.5.0...b3937ab) Updates `github.com/apple/swift-system` from 1.7.5 to 1.8.0 - [Release notes](https://github.com/apple/swift-system/releases) - [Commits](apple/swift-system@1.7.5...704705c) --- updated-dependencies: - dependency-name: github.com/sparkle-project/sparkle dependency-version: 2.9.5 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: swift - dependency-name: github.com/swiftlang/swift-subprocess dependency-version: 1.0.0 dependency-type: direct:production update-type: version-update:semver-major dependency-group: swift - dependency-name: github.com/apple/swift-system dependency-version: 1.8.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: swift ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
This was referenced Aug 10, 2026
A saved position is an index into a per-section array, so an identifier that no longer names anything on the bar does not merely sit there harmlessly: it pushes every entry behind it one slot right. An item whose app is reinstalled, renamed, or re-identified comes back one place off from where the user left it, and does so on every apply from then on. StaleIdentifierLedger counts, per canonical identifier, how many consecutive applies planned for it without a live item answering. Past a threshold the entry is retired and dropped before the section-order lookup rather than after, so the indices behind it close up. Two guards keep the count honest. A pass whose unmatched fraction is above a ceiling is discarded rather than recorded, because that is a degraded reading of the bar and not evidence about any one item. And the counts carry the build they were earned under, so improving identity resolution does not leave items retired on the strength of misses the old behaviour caused. Sampling happens at the one point in the apply that holds both halves at once, past every early return, so an apply that never looked at the bar cannot count as evidence that an item is gone. Signed-off-by: René Jiménez <diazdesandi@proton.me>
Two classes of persisted entry can never name a live item again, and both were reaching the planner on every apply. A source-PID resolution that hands a foreign window Thaw's own PID writes the item under our namespace: #927's reporter carried `com.stonerl.Thaw:WiFi` for an item Control Center owns. Only the control items and the spacers belong there, so anything else is dropped. The order of that check matters more than the drop does. Building the set of titles claimed by a real owner now skips these entries, because counting one as a real owner is what made the provisional-duplicate rule delete the *genuine* Control Center twin — which is why that reporter had lost `com.apple.controlcenter:WiFi` and kept the poisoned copy, and why the live WiFi and Bluetooth items were planned as unmanaged on all fourteen passes in their log. WindowServer's `System Status Item Clone` windows are the second class. The cache already refuses them, but layouts captured before that gate existed hold one entry per clone; the same reporter carried six under a single owner. Pruning alone does not reach the profiles. The saved section order has always been repaired at load and written straight back, so a widened rule takes effect on the next launch, but profiles were only ever pruned on the way out through `resolvedItemOrder`. The damage stayed in the file and seeded the in-memory saved order again at every startup, and a profile is also the one copy a user can re-apply by hand. `repairPersistedLayouts` rewrites them, filtering `itemSectionMap` by the same verdict because `resolvedItemSectionMap` hands that map back verbatim when present. It runs once per build rather than once ever, so a later build that recognizes more can revisit files an earlier one called clean, and it leaves anything it cannot decode untouched. Signed-off-by: René Jiménez <diazdesandi@proton.me>
…t gate A user who drags every hidden item into visible leaves the two dividers correctly adjacent, because nothing is between them. hiddenSectionHasRoom read that as the #795 collapse and refused to persist — and the saved order it refused to write is the same order whose non-zero hidden count made it refuse. The gate's own effect preserved its trigger, so Thaw went permanently read-only on that bar: no save, and no apply either, since applySavedLayout consults the same answer. Reinstalling did not help, because the frozen order is on disk. #924's log shows the warning firing from the tick hidden reached zero through every pass after it. The live count cannot break the tie on its own. A collapse reads as zero live hidden items too — that misclassification is the fault — so releasing on an empty live section alone would hand #868 straight back. What separates the two is where the items went. A collapse leaves them parked thousands of points off every display while the reading calls them visible, a position no genuinely visible item can hold; an emptied section leaves every visible item on the bar. So an empty live section is trusted only when nothing out there is parked. Membership is decided by geometry rather than by asking the cache, for two reasons. The apply path is handed a bar and runs before any recache, so it has no cache to consult. And restricting the parked check to items at or right of the hidden divider keeps a closed always-hidden section full of legitimately parked items from reading as a fault, which is the state the reporter spent most of their session in. Off-display is the tell, not left-of-divider: in #868's geometry the misread items sit just right of the collapsed divider while being nowhere near a screen. Signed-off-by: René Jiménez <diazdesandi@proton.me>
kCGWindowName degrades bar-wide. In #881's 12:38 log the live hidden section came back as com.apphousekitchen.aldente-pro:com.apphousekitchen.aldente-pro com.rogueamoeba.soundsource:com.rogueamoeba.soundsource leits.MeetingBar:leits.MeetingBar com.stonerl.Thaw:com.stonerl.Thaw:1 …and seven more two minutes after the same items had read normally, and #927's reporter carried 23 of the same shape on an unrelated machine with a disjoint set of apps. Caching that reading is what makes the damage self-sustaining. Every item looks new, so the whole bar is persisted under a second set of identifiers, and every later flip between the two spellings presents a bar's worth of late arrivals to lateArrivingProfileIdentifiers, which schedules a re-sort, which posts moves and captures the cursor. #881's reporter rode that loop to a streak of nine consecutive bulk applies ending with unenacted moves. Treat it the way an empty reading is already treated — a failed observation rather than the bar changing — and hold the last known good cache. Two signals, either sufficient: - An item in our own namespace titled with our own bundle identifier. We title our items Thaw.ControlItem.*, so this can only be a degraded read. It is also the signal with consequences of its own, since MenuBarItem recognizes control items by that prefix and a degraded reading therefore arrives with the dividers missing — which is what the same logs report. - Half or more of the reading self-titled, for bars of at least four. One app whose window really is named after its own bundle identifier reaches half of a two- or three-item bar alone. It cannot reach half of four. Only once there is a cache to fall back to; a first launch has nothing better and behaves as before. A partial degradation that trips neither signal still reaches the cache, and prunedSectionOrder clears those entries at the next load rather than letting them accumulate. Signed-off-by: René Jiménez <diazdesandi@proton.me>
The live guard added in the previous commit keeps new self-titled identities out of the cache, but every layout written by an earlier build still holds them: 21 in #881's saved order, 23 in #927's profile. Those entries match nothing on the bar, so each apply plans moves that cannot be enacted — #927's reporter logged 53 fallbacks over 52 minutes. Drop them in prunedSectionOrder, which repairPersistedLayouts() already runs over the profile JSON and MenuBarItemManager over the saved order. Both halves are canonicalized before comparing, because pruning runs after canonicalizedSectionOrder, which rewrites a nested helper's namespace and carries its title through verbatim. A degraded Little Snitch entry arrives as at.obdev.littlesnitch:at.obdev.littlesnitch.agent whose halves are no longer literally equal. Self-titled entries are excluded from titlesWithRealOwner for the same reason misattributed ones already are. A degraded com.microsoft.OneDrive:com.microsoft.OneDrive is not evidence that anything owns that title, and letting it count would license the #788 rule to delete the genuine Control Center twin beside it — the failure that cost #927's reporter their real WiFi item. Only exact equality counts, so a title that continues past the bundle identifier keeps its entry. An app whose window name genuinely is its own bundle identifier does lose its saved entry and becomes unmanaged, which is the one user-visible cost of this rule. Signed-off-by: René Jiménez <diazdesandi@proton.me>
Showing a hidden item moves it into the visible section, so on the next cache cycle currentLayoutDivergesFromSaved finds it sitting somewhere other than where savedSectionOrder records it and reports divergence. That is the second of applySavedLayout's two triggers, so a bulk apply is dispatched to drag the item home — while its menu is open. The only thing between that dispatch and the user's bar is isAnyMenuBarItemMenuOpen, and #924's logs show the probe bailing at "no candidate menu windows on screen" on 120 of 173 and 428 of 428 calls, with two successful menu detections across the whole session. A false negative there is the menu closing the instant it opens. The item's position is not drift. Thaw put it there deliberately and the rehide is what brings it back; this pass has no business racing it. So the divergence rule now skips items with a live TemporarilyShownItemContext. The exemption is keyed on tagIdentifier rather than the tag, because the move that reveals the item can change its window ID and the two must still match afterwards. Scoped to the trigger, not the plan. applyProfileLayout still sees these items when it computes the batch, which is deliberate: dropping them from its input would hide them from partitionUnmanagedUIDs and let the planner anchor neighbours against a phantom, the same hazard the isSystemClone filter guards. Nor does this help when the apply is armed by windowIDsChanged instead — an unrelated app quitting during an open menu still reaches the probe. That is the general unreliability of the probe rather than this regression. The rule moves to a pure static so both of its exemptions can be tested. The notch-overflow carve-out was already there but derived inline from appState and NSScreen, which is why it had no coverage; it now arrives as a set from the caller and keeps its existing behaviour, pinned by two tests. The caller still decides that the feature is on and the display is notched before passing anything. Refs: #924 Signed-off-by: René Jiménez <diazdesandi@proton.me>
REVERT ALONGSIDE 99d174b. 99d174b flipped three defaults so testers exercise the fixes without running defaults write first, but left the version alone. Every DMG built off this branch therefore reports 2.0.0-rc.2.1, the same string as the release that already shipped over Sparkle with those flags off. Neither the reporter nor the log header can say which posture produced a given report. That ambiguity already cost a round. A reporter on run 31418776871 was asked to turn postMoveEventsToWindowOwner off to bisect it; menus still closed, and the flag turned out to also gate isMovableAddressingWindowOwner, so the test both failed to isolate the change and stopped unresolved Control Center items from being shown at all. Attribution has to come from the build, not from asking. 2.0.0-rc.3+test.1 (50). DiagnosticLogger reads CFBundleShortVersionString into every log header, so a log now identifies its own posture. The build number moves too because CFBundleVersion is what Sparkle compares, and 49 is taken by the real rc.2.1. Signed-off-by: René Jiménez <diazdesandi@proton.me>
Signed-off-by: René Jiménez <diazdesandi@proton.me>
Activating an item from the search panel showed it, opened its menu, and then tore both down two to three seconds later. The rehide check was reaching a negative reading by two separate routes, and either one alone is enough to lose the menu. The first is PID breadth. An item's window and the menu that window opens do not always belong to the same process; on macOS 26 Control Center hosts the status item while the app draws the menu. Two sites — the fallback interface capture and appHasVisiblePopup — matched candidates against a single PID, and for an item whose sourcePID never resolved the `?? ownerPID` collapse made that Control Center's, so the app's open menu could never be found. ClickReactionVerifier already matched against both PIDs, so the sites disagreed with each other. Carry the pair on the context as interfacePIDs and match both everywhere. The second is that "not found" was read as "closed". isShowingInterface returned a Bool, so an observation that the menu is gone and an admission that it was never identified were the same answer, and the rehide acted on both. Split them into InterfaceState.showing/absent/unknown and spend up to four further checks on unknown before rehiding anyway. An item that lingers is a far cheaper failure than a menu that closes under the user, and the bound keeps an unidentifiable interface from parking the item in the visible section forever. Extract windowIsOpenInterface as a pure static so the PID and window level rules are testable. Its tests pin the level trade in both directions: a menu-bar-sized status window is the item itself and must not count, while floating and modal levels — which the previous liberal "above normal" match accepted — are ordinary app windows that kept the reading positive indefinitely. Refs #924 Signed-off-by: René Jiménez <diazdesandi@proton.me>
#881's 08:41 log dragged the same six items 69 times in four minutes, each move a synthetic Cmd-drag that takes the cursor. Two faults compound: one supplies a trigger that never stops, the other supplies work that never converges. rebalanceNotchOverflowIfNeeded handed off to a profile apply at the top of the method, before the cooldown and before the budget was ever computed. Every cache tick therefore armed a full apply, and every apply recaches, which runs the pass again. The bar in that log never once overflowed — the pass returned before reaching computeNotchOverflowBudget and not a single ejection was planned across 73 handoffs. Move the handoff below the planner and gate it on the planner having found real overflow, and stamp the rebalance timestamp on the way out so the deferral is rate-limited like the ejection path. Reading the planner for emptiness is sound under the pass's all-unmanaged inputs because the tiers are a priority order over one budget: they decide which items are chosen, not whether any are, which the two new cases pin from both directions. applyProfileLayout meanwhile proceeded with a nil always-hidden divider. Without that boundary findSection collapses always-hidden into hidden, so Phase 1 read all nine always-hidden items as misplaced and planned eight cross-section moves. The moves landed — the mover finds the divider by tag even when ControlItemPair could not — and changed nothing, so the next pass planned the same set. The divider went unresolved in 552 of 578 cycles, and 69 of 75 Phase 1 passes ran on ahCtrlUID=nil. saveSectionOrder already refuses to persist this reading (#849); refusing to move on it is the same judgement one step earlier, on the same predicate. Also drops a clickPID binding left unused when the interface lookup moved to the context's PID set. Refs #881 Signed-off-by: René Jiménez <diazdesandi@proton.me>
Activating an item from the search panel showed it, opened its menu, and then took both down inside a second. f5c114f went after the same report and moved it the wrong way: it was two to three seconds before. That commit widened the interface capture to both of an item's PIDs without widening what counts as an interface. The fallback took the first new on-screen window owned by either one, and the pair includes the window's owner — Control Center, for every item it hosts, which opens short-lived windows of its own around a click. Latching onto one is worse than latching onto nothing: a tracked window short-circuits interfaceState, skipping both the grace period and the unknown budget, so the moment it goes the reading is a confident absent and the rehide drags the item home under the open menu. The same false negative f5c114f set out to remove, arriving sooner and with more conviction. A candidate now has to look like an interface — a menu-level window first, then any window too tall to be a status item, which is how interfaceState already recognizes the popovers and non-standard-level menus that never reach pop-up level. Nothing qualifying tracks nothing, leaving the reading unknown where it belongs. ClickReactionVerifier's window goes through the same test: it answers whether the owner reacted at all and settles for any new window of the owner's when no menu-level one appeared, which is sound evidence of a reaction and a poor guess at the menu. The second fault is that none of the retry intervals were real. The KVO publisher's default options include .initial, so re-subscribing replays the current frontmost app, and runRehideTimer re-subscribes on every call — including from the retry paths it schedules. Every "look again in three seconds" was looking again in two hundred milliseconds, and the four unknown checks documented as roughly twelve seconds of grace were spending eight hundred. Drop the replay so only a real app switch fires, and make the post-show timer a one-second poll rather than the fifteen- second ceiling: with the replay gone nothing else re-arms a check, and dismissing a menu with Escape changes no frontmost app, so the ceiling would have parked the item out on the bar for fifteen seconds every time. Refs #924 Signed-off-by: René Jiménez <diazdesandi@proton.me>
An item cached without a source process has a provisional identity: its namespace falls back to the owner of its window — Control Center, for everything it hosts — and its display name to "Menu Bar Item". The first AX scan after login routinely lands there, because other apps' accessibility trees are still warming up in the seconds it runs. Nothing ever took it back. cacheItemsIfNeeded recaches on a change in the set of window IDs, and a late-resolving item's window does not go anywhere, so the bad reading survived the session and only a relaunch cleared it. SourcePIDNegativeCachePolicy was built for exactly this case — short deadlines on the first failures so a warmer scan can land — and its own reasoning names the half it cannot fix from the service side: "the app stops requesting once settled". This is the app not stopping. When the window set is unchanged, ask the service about the items the cache is holding without a source, and recache if any of them now resolve. Read from the cache rather than by differencing window IDs against the resolved-PID map: those are the items actually on display under a provisional identity, and the set can only shrink as they resolve, so a probe cannot talk the cache into recaching what it just cached. Control items are excluded for the reason enumeration excludes them from resolution to begin with — their AX children are disabled dividers, so the request is a guaranteed miss that can start a full scan of every running app. The probe costs one round trip per tick while anything is unresolved and nothing once everything has resolved; the TTL ladder is what bounds how often a request behind it becomes a real scan. The search panel's name memo needed the other half. It is keyed by tag and cleared on close and on rename, but the whole point of a late resolution is that the row is already on screen. Clear it when the item cache changes, which is the only event that can change what a name resolves to; keystrokes, the cost the memo exists to absorb, do not reach it. Signed-off-by: René Jiménez <diazdesandi@proton.me>
…t opened Only third-party items, and not in rc.2.1: useAXClickDelivery shipped there defaulted off behind a Settings toggle, and this branch turned it on and removed the toggle. What it turned on reads a blocked accessibility call as a failed one. AXShowMenu on a status item opens the menu and then blocks. The menu runs a modal tracking loop, the app cannot answer the accessibility message until it closes, and the activator sets a 250 ms messaging timeout — so the call throws on precisely the items where it worked. #924's 15:27:18 log has the whole thing in three hundred milliseconds: AXShowMenu goes out, the on-screen window count climbs from 14 to 15 while it is blocked, the call comes back actionFailed, and a synthetic click lands 24 ms later. The menu the user asked for appeared and was gone inside the same second. Both escalations from a thrown action are another activation of the same item, and an item whose menu is open toggles it shut: AXPress closes what AXShowMenu opened, and the fallback click closes it again. So an action that throws is now checked before escalating past it — ClickReactionVerifier already knows how to see an owner reacting, and after a call that blocked for its whole timeout the answer needs no waiting, hence reactionSoFar. The same check guards the caller's fallback, for the errors raised before the activator gets to its own. Apple's own items were unaffected because they answer the message. The rule is extracted as performFirstEffectiveAction and pinned from both sides: a threw-but-worked action must not escalate, and an inert one still must. actionFailed now means the item was genuinely left alone, which is what makes the fallback click safe. Refs #924 Signed-off-by: René Jiménez <diazdesandi@proton.me>
Signed-off-by: René Jiménez <diazdesandi@proton.me>
Signed-off-by: René Jiménez <diazdesandi@proton.me>
|
Signed-off-by: René Jiménez <diazdesandi@proton.me>
Signed-off-by: René Jiménez <diazdesandi@proton.me>
Signed-off-by: René Jiménez <diazdesandi@proton.me>
- Use Defaults.bool(forKey:) instead of manual cast in MenuBarItem - Add [weak self] to smart-rehide Task in MenuBarManager - Mark StaleIdentifierLedger.currentBuildVersion nonisolated - Gate ±1 control-item drop bias to zero-width dividers only - Add comment explaining CGDisplayBounds coordinate-space switch - Add parked-divider recovery episode rearm/below-threshold/custom tests - Add wide control-item destination gets no bias test - Correct cooldown-bypass test docstring and document hard-cap invariant Signed-off-by: René Jiménez <diazdesandi@proton.me>
- Rehide: use 30s cursor watchdog and isBulkApplyInProgress gate around rehideTemporarilyShownItems so the 1s default watchdog cannot force-show the cursor mid-batch and per-item hide/show oscillation is suppressed (#899). - Ice Bar: hover-hide now uses rehideInterval instead of the 0.2s showOnHoverDelay when the Thaw Bar is the active presentation, so the panel stays open for the user-configured timeout instead of snapping shut before the user can click an icon. - Focus rehide: add diagnostic comment in the else branch noting that autoRehide must be on for any strategy to fire. Signed-off-by: René Jiménez <diazdesandi@proton.me>
The screen recording indicator appears as an unresolved com.apple.controlcenter:Item-0 with a nil sourcePID. It was counted as an unmanaged item in the notch overflow footprint, which made the rebalance see 98pt of unmanaged space and attempt to eject 20 visible items — all failing with cannotComplete because the recording item can't be moved. Each cache tick re-triggered the loop. Now items with hasProvisionalIdentity (unresolved Control Center generic slots) are excluded from the budget alongside the existing transient tags (AudioVideoModule, FaceTime, ScreenCaptureUI, GameMode), so the rebalance computes unmanagedCount=0 and never fires while the recording indicator is the only unmanaged item (#935). Signed-off-by: René Jiménez <diazdesandi@proton.me>
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
4 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
Menu bar layout reliability: cold-start restore that actually runs, early apply for already-resolved identities, and hard bounds on automatic re-applies so a failed or parked-divider move cannot storm the cursor or rewrite the saved order. The same branch removes the full-sort rearrange, fixes divider seeding/pairing for show-hide (including collapsed sections), and adds field-repair tools (
--reset-layout, clearer move-refusal diagnostics, profile layout preview).Scope: One reliability track across launch restore, move planning, control-item pairing, and the related layout/settings surfaces that the field logs required. Not a drive-by refactor of unrelated areas.
Linked issue (required)
PR Metadata fails without a
Closes:line in this exact form (keep it on its own line):Replace
N/Awith#<issue_number>(e.g.Closes: #123) when this PR fixes/implements a specific issue.Closes: #885
Closes: #881
Closes: #900
Closes: #815
Closes: #890
Closes: #895
Closes: #899
Closes: #794
Closes: #886
Closes: #897
Closes: #248
Closes: #903
Closes: #751
Closes: #769
Closes: #904
Closes: #905
Closes: #887
Related / needs field verification: #788, #634, #791
PR Type
Describe what this change does (not the linked issue’s request kind). Bug reports use the
BugIssue type; bug fixes useFixon PRs.If you tick Feature or Refactor and touch more than ~20 files, please mention why this can’t be split.
Area
Product surfaces (optional when the change is not about the app UI). Path-based labeling also applies.
Use PR Type for what changed (
CI/CD,Documentation,Other/ chore, etc.).Use
opsfor where when it is repo operations: CI, release, GitHub hygiene, scripts, lint/sonar config — not a product surface.menubar
icebar
layout
appearance
settings
onboarding
permissions
profiles
hotkeys
updates
ops
Does this PR introduce a breaking change?
What is the new behavior?
Cold start / settling (#881, #900)
savedSectionOrder.Order at any time (#885, #881)
Show / hide and control items (#890, #895, #899, #897)
preflightSetupno longer re-stamps the hidden divider to1on every launch / recreate.cannotComplete([Bug] Third-party item refused as immovable with no diagnostic, while macOS reports it identically to movable items #905).Profiles / repair (#904, #887, #788, #815)
Thaw --reset-layoutclears persisted order / pinning / relocation bookkeeping and re-seeds divider positions without starting the app (for repairing a wrecked bar before launch).Also on this branch
0([Bug] The profile does not respect the saved spacing setting. #903).AlphaChannelViewcentralizes transparency-scan alpha access.swift-subprocess1.0.0 replaces the 0.5 pin and drops the env trampoline.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:
LayoutStormReplayTestsMoveOperationTimeoutTests/SectionOrderDigestTestsControlItemDefaultsSeedingTestsStaleDestinationGateTests/UnfinishedMoveBatchGateTests/EarlySavedLayoutRestrictionTests/AutomaticBulkApplyGateTestsParkedDividerTestsUnresolvedPlaceholderAliasTestsThawBarSectionRoutingTestsKnown limitations / follow-ups
--reset-layoutentry-point wiring is compile-covered; the reset logic is tested against a throwaway defaults suite (not the livecom.stonerl.Thawdomain).Other information
~50 commits on
fix/menubar-reliability. Started as full-sort removal + timeout hardening; grew through launch restore, parked-divider / unfinished-batch storm bounds, control-item pairing/seeding, capture topology, profile prune/preview, and--reset-layout.Commit groups (headlines):
--reset-layout