fix(menubar): restore launch layout and stop move/storm loops - #889
Merged
Conversation
github-actions
Bot
added
enhancement
Contributor
|
Labels: Warning This PR looks large (131 files, 18533 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 | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
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
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 `@Shared/Bridging/Bridging.swift`:
- Around line 751-758: Update the host-display selection around the compactMap
closure to reject intersections whose area is zero, not just null rectangles.
Compute the intersection area and only return a display candidate when it is
strictly positive, so edge-touching displays are excluded while the existing
maximum-area selection and nil fallback remain unchanged.
🪄 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: 4112ff47-13f3-4313-b567-a620e42df563
📒 Files selected for processing (5)
Shared/Bridging/Bridging.swiftThaw/MenuBar/MenuBarItems/LayoutSolver.swiftThawTests/MenuBar/ControlItem/ControlItemDefaultsSeedingTests.swiftThawTests/MenuBar/Layout/PlanUnmanagedPlacementTests.swiftThawTests/MenuBar/Layout/PrunedSectionOrderTests.swift
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Shared/Bridging/Bridging.swift (1)
804-812: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject zero-area refreshed-bound intersections.
CGRect.intersection(unionBounds)can return a non-null empty rect whenscreenBoundsonly touchesunionBoundsat an edge. The current.isNullcheck then keeps that stale crop; fall back tounionBoundswhen the intersection width or height is zero.🤖 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 `@Shared/Bridging/Bridging.swift` around lines 804 - 812, Update the effectiveBounds closure around screenBounds.intersection(unionBounds) to treat intersections with zero width or zero height as invalid, not only intersections where isNull is true. When usedRefreshedTopology is enabled and the intersection has no positive area, keep the existing warning and return unionBounds; otherwise preserve the current screenBounds path.
🤖 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 751-758: The display selection near resolve currently accepts
partial intersections, allowing effectiveBounds to extend beyond the selected
display. After computing effectiveBounds and selecting the display, validate
that effectiveBounds is contained within display.frame; if not, bypass the SCK
configuration path and use the existing display-independent fallback that
supports cross-display windows.
---
Outside diff comments:
In `@Shared/Bridging/Bridging.swift`:
- Around line 804-812: Update the effectiveBounds closure around
screenBounds.intersection(unionBounds) to treat intersections with zero width or
zero height as invalid, not only intersections where isNull is true. When
usedRefreshedTopology is enabled and the intersection has no positive area, keep
the existing warning and return unionBounds; otherwise preserve the current
screenBounds path.
🪄 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: c07dac8c-ae8a-4adf-a1c2-9cc454faec48
📒 Files selected for processing (1)
Shared/Bridging/Bridging.swift
This was
linked to
issues
Aug 6, 2026
github-actions
Bot
added
appearance
3 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
ThawTests/Settings/Models/ProfileDecodingDefaultsTests.swift (1)
470-470: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExercise
moveCursorToRevealedItemin the full round trip.The source model keeps this new Boolean at its default
false, and the destination does not compare it. The test can pass if profile encoding or decoding omits the field.Set
source.moveCursorToRevealedItem = trueand assert that the destination equals the source after decode and apply.🤖 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/Settings/Models/ProfileDecodingDefaultsTests.swift` at line 470, Update the full round-trip test around the source profile setup to set moveCursorToRevealedItem to true, then ensure the decoded-and-applied destination is asserted equal to source so this field is exercised and validated.Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swift (2)
8347-8354: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winThis early return skips
recordBulkApplyOutcome, so a partial arrangement can be persisted.
didAttemptHCtrlistrueat this point, and the H_ctrl move may have failed, incrementingunenactedMoveCountat Line 8277. The LCS pass never runs. The bar is left in whatever arrangement the aborted apply reached.The branch returns without calling
recordBulkApplyOutcome(unenactedMoveCount:).unfinishedMoveBatchObservedAtstays unset,shouldPersistSavedOrderpasses on the next cache cycle, andsaveSectionOrderwrites the partial arrangement over the layout this apply was restoring. That is the#900drift this PR set out to stop.The sibling early return at Lines 8533-8541 handles the identical situation: it increments
unenactedMoveCountand records the outcome before returning. Apply the same treatment here.🐛 Proposed fix
} else { MenuBarItemManager.diagLog.warning( "Profile layout: could not reclassify sections after moving H_ctrl" ) + // Abandoning here leaves the divider wherever the H_ctrl move + // put it and skips every move the LCS pass would have made, + // so the arrangement is not an order of record. + unenactedMoveCount += 1 + recordBulkApplyOutcome(unenactedMoveCount: unenactedMoveCount) clearProfileState(source: source, items: postMoveItems) scheduleDeferredCacheRefresh() return }🤖 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 8347 - 8354, Update the early-return branch after the failed H_ctrl reclassification to call recordBulkApplyOutcome(unenactedMoveCount:) before clearing state, scheduling the cache refresh, and returning. Match the sibling early-return handling so the failed or partial apply records its outcome and prevents persistence of the partial arrangement.
4949-5038: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThe stale check reintroduces the coordinate-space mixing that
itemHasCorrectPositionwas rewritten to avoid.The doc comment at Lines 4392-4395 states that
getCurrentBounds(for:)answers from two different sources: the windowID path reports parked offscreen coordinates, and the tag-matching fallback reports from the on-screen list. Which path runs depends on timing.
plannedTargetBounds(Line 4953) andcurrentTargetBounds(Line 5023) both go throughgetCurrentBounds(for:). If one resolves through the windowID path and the other through the on-screen fallback, the measured delta can exceed a display width for a target that never moved. The move then aborts withstaleDestinationinstead of retrying.Read both operands from one live snapshot, the same way
itemHasCorrectPositiondoes, so the comparison cannot straddle two coordinate sources.🤖 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 4949 - 5038, Update the stale-destination logic in the move retry loop to obtain the planned and current target bounds from a single live snapshot using the same unified coordinate source as itemHasCorrectPosition. Remove the separate getCurrentBounds(for:) reads for plannedTargetBounds and currentTargetBounds, then compare both positions from that snapshot before throwing EventError.staleDestination.
🤖 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 9054-9062: Refactor applySavedLayout to accept an options struct
that bundles the defaulted context and policy flags, including
previousDisplayID, currentDisplayID, previousCCGenericWindowIDs,
bypassMoveCooldown, and resolvedIdentitiesOnly. Update every call site to
construct or pass this options value while preserving existing defaults and
behavior; keep the core item, window, and controlItems parameters separate.
- Around line 9671-9681: Fix the garbled explanatory comment immediately above
the failureLedger.isUnderBackoff check so it is a complete, grammatically
correct sentence describing how the ledger bounds repeated failed ejections and
profile-layout moves; leave the executable logic unchanged.
- Around line 8984-9028: Align the save-withhold duration in
unfinishedMoveBatchBlocksSave with the automatic dispatch cooldown in
automaticBulkApplyPermitted so the withhold remains active until a retry can
run. Prefer deriving both defaults from one shared duration constant, preserving
the existing retry and cooldown behavior.
- Around line 4408-4433: Filter system status item clones from the items
snapshot before sorting and performing adjacency checks in the
landing-confirmation logic. Reuse the same clone-exclusion predicate or
established filtering behavior used by cacheItemsRegardless and
applyProfileLayout, while preserving the existing exact-window-then-tag matching
and destination adjacency checks.
- Around line 3267-3288: Update the early resolved-only apply in
cacheItemsRegardless so it cannot await the active startupSettlingTask through
applySavedLayout and applyProfileLayout. Either bypass
waitForStartupSettlingToEnd for this resolved-only operation or launch the apply
asynchronously without awaiting it from the settling task, while preserving the
didAttemptEarlySavedLayoutApply behavior only when dispatch succeeds.
In `@Thaw/Resources/Localizable.xcstrings`:
- Around line 51616-51619: Update the Dutch localization value in the nl
stringUnit to replace “ernaartoe” with “ernaast”, preserving the rest of the
translation unchanged.
In `@Thaw/Settings/Models/DisplaySettingsManager.swift`:
- Around line 699-732: Batch the scoped updates in setUseThawBarForAlwaysHidden
and toggleUseThawBarForAlwaysHidden by building a modified configurations
dictionary during each display loop instead of calling updateConfiguration per
display. Assign the resulting dictionary to configurations once after processing
all eligible non-IceBar displays, preserving each display’s existing
configuration and the current scope filtering.
In `@Thaw/Utilities/SettingsURIHandler.swift`:
- Around line 1004-1008: The URI configuration lookup currently falls back to
defaultConfiguration instead of the active global template. Update
getDisplayConfiguration(forUUID:) and getDisplayInfo(screen:uuid:) to decode
globalDisplayConfiguration and use it as the fallback, keeping explicit display
configurations unchanged.
---
Outside diff comments:
In `@Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swift`:
- Around line 8347-8354: Update the early-return branch after the failed H_ctrl
reclassification to call recordBulkApplyOutcome(unenactedMoveCount:) before
clearing state, scheduling the cache refresh, and returning. Match the sibling
early-return handling so the failed or partial apply records its outcome and
prevents persistence of the partial arrangement.
- Around line 4949-5038: Update the stale-destination logic in the move retry
loop to obtain the planned and current target bounds from a single live snapshot
using the same unified coordinate source as itemHasCorrectPosition. Remove the
separate getCurrentBounds(for:) reads for plannedTargetBounds and
currentTargetBounds, then compare both positions from that snapshot before
throwing EventError.staleDestination.
In `@ThawTests/Settings/Models/ProfileDecodingDefaultsTests.swift`:
- Line 470: Update the full round-trip test around the source profile setup to
set moveCursorToRevealedItem to true, then ensure the decoded-and-applied
destination is asserted equal to source so this field is exercised and
validated.
🪄 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: 794f266b-c154-41fd-878d-7474cc82f853
📒 Files selected for processing (50)
MenuBarItemService/SourcePIDCache.swiftShared/Bridging/Bridging.swiftThaw/MenuBar/Appearance/Configurations/MenuBarAppearanceConfigurationV2.swiftThaw/MenuBar/Appearance/MenuBarAppearanceEditor/MenuBarAppearanceEditor.swiftThaw/MenuBar/Appearance/MenuBarAppearanceManager.swiftThaw/MenuBar/Appearance/MenuBarOverlayPanel.swiftThaw/MenuBar/ControlItem/ControlItem.swiftThaw/MenuBar/IceBar/IceBar.swiftThaw/MenuBar/LayoutBar/LayoutBarContainer.swiftThaw/MenuBar/MenuBarItems/AXItemActivator.swiftThaw/MenuBar/MenuBarItems/LayoutSolver.swiftThaw/MenuBar/MenuBarItems/MenuBarItemManager.swiftThaw/MenuBar/MenuBarManager.swiftThaw/MenuBar/MenuBarSection.swiftThaw/MenuBar/MenuBarSectionLayout.swiftThaw/MenuBar/Search/MenuBarSearchPanel.swiftThaw/Resources/Localizable.xcstringsThaw/Settings/Models/AdvancedSettings.swiftThaw/Settings/Models/DisplayIceBarConfiguration.swiftThaw/Settings/Models/DisplaySettingsManager.swiftThaw/Settings/Models/Profile.swiftThaw/Settings/Models/ProfileManager.swiftThaw/Settings/Search/SearchIndex.swiftThaw/Settings/SettingsPanes/AdvancedSettingsPane.swiftThaw/Settings/SettingsPanes/DisplaySettingsPane.swiftThaw/Utilities/Defaults.swiftThaw/Utilities/MouseHelpers.swiftThaw/Utilities/SettingsURIHandler.swiftThawTests/MenuBar/Appearance/MenuBarAppearanceConfigurationTests.swiftThawTests/MenuBar/ControlItem/ControlItemDefaultsSeedingTests.swiftThawTests/MenuBar/Items/AutomaticBulkApplyGateTests.swiftThawTests/MenuBar/Items/EarlySavedLayoutRestrictionTests.swiftThawTests/MenuBar/Items/ShouldPersistSavedOrderTests.swiftThawTests/MenuBar/Items/StaleDestinationGateTests.swiftThawTests/MenuBar/Items/UnfinishedMoveBatchGateTests.swiftThawTests/MenuBar/Layout/LayoutBarContainerTests.swiftThawTests/MenuBar/Layout/PlanLeftmostMoveTests.swiftThawTests/MenuBar/Section/ThawBarSectionRoutingTests.swiftThawTests/Settings/Models/AdvancedSettingsSnapshotTests.swiftThawTests/Settings/Models/AdvancedSettingsTests.swiftThawTests/Settings/Models/DisplayIceBarConfigurationTests.swiftThawTests/Settings/Models/DisplaySettingsManagerGlobalFallbackTests.swiftThawTests/Settings/Models/DisplaySettingsManagerLookupTests.swiftThawTests/Settings/Models/DisplaySettingsManagerURINotificationTests.swiftThawTests/Settings/Models/ProfileDecodingDefaultsTests.swiftThawTests/Settings/URI/SettingsURIHandlerApplyTests.swiftThawTests/Settings/URI/SettingsURIHandlerCoverageTests.swiftThawTests/Settings/URI/SettingsURIHandlerTailTests.swiftThawTests/Utilities/MouseHelpersCursorPointTests.swiftdocs/URI_SCHEMES.md
Folded from 3 commits; full history preserved on archive/fix-menubar-reliability-v1. Original messages: fix(menubar): bound recovery and restore auto-rehide Signed-off-by: René Jiménez <diazdesandi@proton.me> fix(menubar): stop the move-failure storm after a refused batch Signed-off-by: René Jiménez <diazdesandi@proton.me> fix(menubar): harden layout recovery and notch overflow 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>
Folded from 3 commits; full history preserved on archive/fix-menubar-reliability-v1. Original messages: fix(menubar): address review findings for #923/#927 reliability fixes - 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> fix(menubar): stop cursor flicker, Thaw Bar vanish, and rehide issues - 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> fix(menubar): exclude provisional items from notch overflow budget 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>
Folded from 2 commits; full history preserved on archive/fix-menubar-reliability-v1. Original messages: ci: add MacOSCompatibilityWarning.swift to swiftlint inputs The static_analysis check regenerates the xcfilelist and diffs it against the committed copy, which was not regenerated when the file was added. Signed-off-by: René Jiménez <diazdesandi@proton.me> fix(defaults): revert test-build values of hidden diagnostic flags bulkApplyIdleThresholdMs, enforceConcealedSectionOrder, and postMoveEventsToWindowOwner shipped with test-build overrides (300, false, true) that contradicted their documented contracts (0, true, false) — the DefaultsKeyTests comments still said "TEST BUILD". The docs win: the idle gate stays off, concealed-section order stays part of what restore means, and window-owner event routing stays opt-in. Also route the postsMoveEventsToWindowOwner reader through DefaultValue like its siblings so the constant is load-bearing. Signed-off-by: René Jiménez <diazdesandi@proton.me>
The activation-notification rewrite dropped guards the merge-base applied to every strategy, reintroducing the #924 bug class: - .focusedApp hid synchronously with no mouse-position check, no settle delay, and no reveal grace period, so clicking a third-party item collapsed the section under the cursor. It now shares the .smart path (mouse guards, delay-or-remaining-grace, strategy re-check), differing only in a shorter settle delay and skipping the open-menu scan. - .timed lost its fire-time isMouseInsideActiveArea() re-check when EventMonitor became Task.sleep; the interval still isn't gated, but the hide now defers while the cursor is over the bar or Thaw Bar, matching .smart. smartRehideDelay(since:) becomes rehideDelay(for:since:) and the policy tests cover both strategies. Signed-off-by: René Jiménez <diazdesandi@proton.me>
…nd reset Folded from 2 commits; full history preserved on archive/fix-menubar-reliability-v1. Original messages: fix(menubar): apply unmanaged placements in the pruned index space Placement was planned against the ledger-pruned saved order, but applyUnmanagedPlacementsToDesired received the unpruned dict, so a saved index computed with ghosts removed was scanned against a sequence that still contained them. With retired entries ahead of a returning item's slot, the anchor scan started early and could seat the item left of the neighbor it should follow — the drift the ledger exists to stop. Both sites now share one pruned order. Signed-off-by: René Jiménez <diazdesandi@proton.me> fix(menubar): forget stale-identifier verdicts on layout reset StaleIdentifierLedger.removeAll() documents that a reset starts from no verdicts, but neither reset path called it: the Settings reset cleared every other piece of layout state and the --reset-layout CLI key list omitted the ledger keys, so pre-reset retirements kept pruning identifiers out of a supposedly clean layout. prepareLayoutStateForReset() now clears the ledger, and the CLI list gains the ledger keys plus the new-items placement keys the Settings reset already cleared, removing the divergence between the two. Signed-off-by: René Jiménez <diazdesandi@proton.me>
The survival set used plain `itemOrder ?? savedSectionOrder`, ignoring resolvedItemOrder's empty-means-absent rule: a present-but-empty itemOrder (a mistimed capture, or imported JSON) filtered itemSectionMap down to [:] and persisted it, permanently shadowing the savedSectionOrder-derived fallback. The repair now falls back to savedSectionOrder for an empty order, and compares map keys in canonical form (storing them canonicalized) so a pre-canonical entry survives when its item does. Signed-off-by: René Jiménez <diazdesandi@proton.me>
The pass-2 floor recorded where the previous rightOf item landed as an index, but a later leftOf insertion at or before that index (the clamped-anchor case, where every slot resolves to the same section start) shifts the placed item right without updating the record. The next rightOf item's floor was then one slot low and it inserted ahead of its predecessor, reversing the group order the mechanism exists to preserve. The floor now records the placed uid and looks up its current position at use time. Signed-off-by: René Jiménez <diazdesandi@proton.me>
Folded from 2 commits; full history preserved on archive/fix-menubar-reliability-v1. Original messages: perf(menubar): correlate AX identity once per refused drag A refused unresolved-CC-placeholder drag paid the bounded 500 ms AXIdentityCatalog snapshot twice back-to-back on the main thread: once in the alias attempt and again in logMoveRefusal when the alias came back nil — up to ~1 s of hitch before the alert appeared. The alias attempt now returns the identity it correlated and the refusal log consumes it instead of re-running the snapshot. Signed-off-by: René Jiménez <diazdesandi@proton.me> perf(menubar): evaluate interfaceState once per rehide poll check rehideTemporarilyShownItems read the computed interfaceState twice per shown context on its 1-second poll — once for the showing test and again for the unknown filter — and each evaluation's terminal case enumerates every on-screen window. Snapshot the state once per context and answer both questions from it. Signed-off-by: René Jiménez <diazdesandi@proton.me>
…y breaker Folded from 2 commits; full history preserved on archive/fix-menubar-reliability-v1. Original messages: fix(settings): register screen-parameters observer before task startup DisplaySettingsManager registered its NotificationCenter observer inside the observation Task, so a screen-parameters notification posted between configureObservers() returning and the task's first run was silently missed. debouncedNotificationTask — which ProfileManager already uses for this same notification — registers the observer synchronously before returning, closing the window. Signed-off-by: René Jiménez <diazdesandi@proton.me> fix(menubar): enforce bulk-apply circuit breaker inside applyProfileLayout automaticBulkApplyPermitted was checked only at the dispatch sites, so any future automatic caller of applyProfileLayout(automatic:) would bypass the breaker unknowingly. Every automatic apply funnels through applyProfileLayout, so the guard now also lives there, alongside the automaticArrangementEnabled escape hatch and before any profile state is armed. Signed-off-by: René Jiménez <diazdesandi@proton.me>
…wift-algorithms Folded from 3 commits; full history preserved on archive/fix-menubar-reliability-v1. Original messages: style: use the required file header in MacOSCompatibilityWarning The swiftlint file_header pattern requires both copyright lines in every file; this was the one violation left in the target. Signed-off-by: René Jiménez <diazdesandi@proton.me> refactor(menubar): consolidate copy-paste blocks flagged in review Three dedups, no behavior change: - The bulk-apply circuit breaker's four guard-log-return call sites collapse into isAutomaticBulkApplyPermitted(caller:quietly:); the notch rebalance keeps its debug-level refusal via quietly. - applyProfileLayout's six abandon exits (increment unenacted count, record outcome, clear profile state, schedule cache refresh) share one nested abandonApply(reason:items:). - The MenuBarItemManager.* defaults keys get one home, MenuBarItemManager.LayoutStateKey, referenced by the persistence sites, ProfileManager's capture path, and --reset-layout; the CLI list's remaining literals now come from Defaults.Key rawValues. Signed-off-by: René Jiménez <diazdesandi@proton.me> refactor(menubar): use swift-algorithms for pairwise deltas and dedup targetIsRetreating's zip-with-dropFirst becomes adjacentPairs(), and windowIDsNeedingSourceResolution's manual seen-set compactMap becomes filter/map/uniqued(). Both files already import Algorithms; behavior is unchanged and locked by the existing suites. Signed-off-by: René Jiménez <diazdesandi@proton.me>
A control-item lookup failure leaves the window-ID snapshot uncommitted so the change detector re-fires — right for a transient race, wrong for a failure that is not going away. #933's process ran a full recache every 3-second poll for 27 hours (49k failures, 292k window-list enumerations, a 272 MB log), and on build 49 each third failure also rebuilt the status items, whose scene churn leaks CA fence Mach ports on macOS 26 (~0.5 MiB owned-unmapped each, 47 GiB total). The episode latch already bounds the rebuilds; this bounds the retry cadence: below the rebuild threshold retries stay immediate, past it the wait doubles per failure and caps at 60 s, so recovery stays automatic without the churn. Event-driven recaches bypass the detector and are unaffected. Signed-off-by: René Jiménez <diazdesandi@proton.me>
Sonar S107 flagged TemporarilyShownItemContext's 8-parameter init. The tag and PID of the fallback neighbor only travel together — the show-time resolver already returns them as one tuple and the sole consumer requires both — so the context now stores that pair as a single optional, dropping the init to 7 parameters and the half-present state the two flat optionals allowed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: René Jiménez <diazdesandi@proton.me>
The $state sink ran updateStatusItem() on every assignment, and updateControlItemState reassigns an unchanged state on each HID display-change check — each rewrite of the button's image, title, and length commits a status-item scene update, and on macOS 26 every scene commit costs Core Animation fence ports (#933's leak class). removeDuplicates() keeps the rewrite for real transitions; fresh subscriptions after recreateStatusItem() still fire because @published replays the current value to new subscribers, and recreateStatusItem() also calls updateStatusItem() directly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: René Jiménez <diazdesandi@proton.me>
diazdesandi
force-pushed
the
fix/menubar-reliability
branch
from
August 14, 2026 16:53
8b18d88 to
3d1854b
Compare
Member
Author
|
Updated the description to catch issues this branch already fixes that were not linked. Newly closed here
Reviewed, not closed
Already-linked closures (#885, #881, #900, #815, #890, #895, #899, #794, #886, #897, #248, #903, #751, #769, #904, #905, #887) are unchanged. |
The unfinished-move-batch latch expired after 30 seconds, so a partial arrangement a failed apply left behind became persistable once enough time passed — exactly the drift #900 describes. Time cannot make a partial result authoritative: the latch now holds until a clean apply clears it through recordBulkApplyOutcome or an explicit user move clears it through recordExternalMoveOperation. Layout-editor drops report whether the item verifiably reached its section, and only a verified placement (or a suppressed-alert arrival) counts as that explicit choice. A user move also clears the pending divergence reading and the bulk-apply breaker streak: a hand-chosen arrangement is fresh evidence, and the breaker re-trips after two failures if the bar still refuses moves. Includes swiftformat over the touched files. Signed-off-by: René Jiménez <diazdesandi@proton.me>
#887 names icon order and spacing as what profiles forget; the preview showed order but not spacing. A new ProfilePreviewModel.spacingRows assembles the global offset first, then per-display overrides sorted by resolved display name (stable regardless of dictionary order), skipping zero offsets since they change nothing. The popover renders it as one caption line and the settings pane supplies display names from the known-display cache. Signed-off-by: René Jiménez <diazdesandi@proton.me>
The multi-display heading #886 added showed the bare display name; without context it read as decoration. "Active display: <name>" says what the bars below are describing. Includes swiftformat over the touched file. Signed-off-by: René Jiménez <diazdesandi@proton.me>
The PR checklist promises a swiftformat pass; 15 files touched by this branch still failed --lint (modifier order, wrap rules, and trailing commas). Formatting only — no code change. Signed-off-by: René Jiménez <diazdesandi@proton.me>
The catalog picks up the keys headless builds never extracted — the Active display label, the spacing-preview formats, the layout-reset destinations, and the macOS 27 warning — so Crowdin can see them. MARKETING_VERSION drops the +test.1 build metadata: this branch is the rc.3 release line now, not a side test build. Signed-off-by: René Jiménez <diazdesandi@proton.me>
|
diazdesandi
added a commit
that referenced
this pull request
Aug 15, 2026
Two defects found while writing the rc.3 release notes, both created by work that landed after the fix that should have covered it. Reliability gate defaults. The test build handed to reporters on #854, #905, #923, #924 and #927 was cut from 8e08508, which carried: postMoveEventsToWindowOwner false -> true bulkApplyIdleThresholdMs 0 -> 300 enforceConcealedSectionOrder true -> false d37e238 reverted all three about 4.5 hours after that build was cut, so every field report on the reliability work describes behaviour that development would not have shipped. Adopt the tested configuration as the shipping default and move the doc comments and DefaultsKeyTests with it, since both asserted the reverted values. enforceConcealedSectionOrder is the inverted one: false is the shorter batch. It restores membership of the concealed sections and skips the ordering moves, which cost a cursor hijack each to land an item thousands of points off-screen where Thaw Bar renders from cache. automaticArrangementEnabled stays true. Its enabled state is what lets Thaw arrange on its own initiative, so the test build deliberately left it alone rather than mask every other fix behind a bar that no longer restores its layout. Advanced reset coverage. #910 closed the hole where resetAdvanced() omitted six persisted AdvancedSettings booleans. automaticArrangementEnabled and moveCursorToRevealedItem landed five days later in #889 and reopened it: both are persisted, both have Toggles in Settings, both are reachable from the URI scheme, and neither was restored by resetting Advanced or by resetAllSettingsToDefaults. moveCursorToRevealedItem sits on the Advanced pane itself, so its own reset button left it alone. automaticArrangementEnabled is the one with consequences: switched off it stands down the late-arrival re-sort and the saved-layout restore, so a user who turned it off while debugging and then reset to defaults kept a Thaw that would not restore layouts, with no setting visibly out of place to explain it. The #910 regression test now covers both, and its doc comment records that any boolean added to AdvancedSettings belongs in the reset and in the test. The three hidden diagnostic gates are deliberately excluded from the reset: they have no Settings UI, no URI case, and are reachable only by defaults write, so a reset is not meant to reach them. Signed-off-by: René Jiménez <diazdesandi@proton.me>
diazdesandi
added a commit
that referenced
this pull request
Aug 15, 2026
Covers the 21 commits since 2.0.0-rc.2.1: the #889 reliability track (cold-start restore, move planning and storm bounds, control-item pairing and identity, persist gates), the settings and IceBar fixes from #910 through #930, the #933 recache backoff, and the CI moves. Also documents the two fixes made while writing these notes: the reliability gate defaults and the Advanced reset coverage. 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), stops a mis-paired visible control item from collapsing the hidden section (restart dump, notch-left icons, layout-editor drags), 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
Closes: #923
Closes: #924
Closes: #927
Closes: #933
Closes: #935
Closes: #907
Related / needs field verification: #788, #634, #791, #868
Duplicate / same class as #927 (already closed independently): #849
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, #923, #924, #927)
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).hiddenSectionHasRoompermanently read-only. A true collapse ([Bug] Hidden section collapses to zero width after docking to an external main display with a notched secondary #868) still refuses: parked off-display items are the tell, not a live empty section.bundleId:bundleId) are treated as a failed observation, not a new bar. Caching them was minting a second identifier set and scheduling late-arrival re-sorts that captured the cursor ([Bug] Mouse disappearing and also moving to top right of display #881, [Bug] All Items from hidden section move to visible section after restart #927).com.stonerl.Thaw:(e.g.com.stonerl.Thaw:WiFi) are dropped rather than planned as unenactable moves ([Bug] All Items from hidden section move to visible section after restart #927).Storms, capture, and leak (#900, #935, #933)
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). Settings reset and the CLI now both clear the stale-identifier ledger.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.PermissionsView.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/SectionOrderDigestTestsControlItemDefaultsSeedingTests/ControlItemRecoveryTestsStaleDestinationGateTests/UnfinishedMoveBatchGateTests/EarlySavedLayoutRestrictionTests/AutomaticBulkApplyGateTestsParkedDividerTests/SectionGeometryGateTestsUnresolvedPlaceholderAliasTestsThawBarSectionRoutingTestsKnown 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).NSSceneStatusItemitself.Other information
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,
--reset-layout, control-item identity (#923/#924/#927), screen-recording notch budget (#935), and recache backoff (#933).Commit groups (headlines):
com.stonerl.Thaw:-namespaced foreign items; skip temporarily shown items in divergence--reset-layout(including ledger keys)