fix(menubar): suppress restore during startup settling period to prevent icon parade#244
Draft
7a6163 wants to merge 3 commits intostonerl:mainfrom
Draft
fix(menubar): suppress restore during startup settling period to prevent icon parade#2447a6163 wants to merge 3 commits intostonerl:mainfrom
7a6163 wants to merge 3 commits intostonerl:mainfrom
Conversation
…ent icon parade Introduce a startup settling period (isInStartupSettling) that blocks all restore operations and saveSectionOrder() calls for a fixed window after performSetup() completes: - 30 s when system uptime < 60 s (login item / auto-launch boot) - 5 s otherwise (manual restart, app-update-triggered restart) During the settling period every didLaunchApplicationNotification still triggers a cache cycle so the UI stays accurate, but neither restoreItemsToSavedSections nor restoreSavedItemOrder runs. This prevents the "icon parade" (10–15 s of continuous icon shuffling) caused by each app's launch notification triggering its own restore cycle during login, where 20+ apps load over ~30 s and cascade into each other. When the period expires, one final cacheItemsRegardless(skipRecentMoveCheck: true) enforces the user's saved layout against whatever macOS placed items. skipRecentMoveCheck: true prevents the final restore being silently blocked by the 5 s move cooldown if relocateNewLeftmostItems stamped a timestamp near the end of the settling window. saveSectionOrder() is also suppressed during settling to protect the UserDefaults-loaded reference order from being overwritten by transient physical positions before restore has had a chance to run. Additionally raise the move-operation cooldown in both restoreItemsToSaved- Sections and restoreSavedItemOrder from 2 s to 5 s, giving more separation between consecutive automatic restores triggered by rapid app restarts (e.g. BetterTouchTool update checks).
…entry Add startupSettlingTask: Task<Void, Never>? to track the in-flight startup settling Task. Before starting a new settling period, any previous Task is cancelled via startupSettlingTask?.cancel(). The cancelled Task handles CancellationError explicitly in a do/catch block and returns immediately without touching isInStartupSettling or any other shared state — the new performSetup() call owns those for the new period. This prevents multiple concurrent settling tasks from accumulating if performSetup() is called more than once (e.g. display reconnect or a permission re-grant triggers a second setup pass). All reads and writes of isInStartupSettling remain on @mainactor: the property is declared on the @MainActor-isolated MenuBarItemManager class and the Task closure is annotated @mainactor. The 30 s / 5 s thresholds and skipRecentMoveCheck: true behaviour are unchanged.
… layout reset Two follow-up fixes from code review of the startup settling period: **Preserve settling deadline on performSetup() re-entry (HIGH)** Replace the `Duration`-based `Task.sleep(for:)` with a deadline-based `Task.sleep(until:clock:.continuous)` anchored to a stored `settlingDeadline: ContinuousClock.Instant?`. On re-entry, the new deadline is `max(existingDeadline, newDeadline)`, so a second performSetup() call that fires after systemUptime crosses the 60 s boundary can no longer silently truncate the original 30-second login settling window to 5 seconds. On normal completion, `settlingDeadline` is nil'd so subsequent sessions start fresh. **End settling immediately on user-initiated layout reset (MEDIUM)** `resetLayoutToFreshState()` now cancels `startupSettlingTask` and clears both `isInStartupSettling` and `settlingDeadline` at the top of the function. A user-initiated reset is authoritative: the post-reset cache must be free to run `saveSectionOrder()` and restore logic without being blocked by a settling period that was started before the reset.
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
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.
Problem
Addresses the "icon parade" reported in #224 — icons continuously shuffling for 10–15 seconds. The root cause is that
restoreItemsToSavedSectionsandrestoreSavedItemOrderfire on everydidLaunchApplicationNotification(1 s debounce). During login, 20+ apps load over ~30 seconds, each triggering its own restore cycle that conflicts with the next, producing cascading icon moves.A secondary trigger is app update checks (e.g. BetterTouchTool), which briefly remove and re-add a menu bar icon, causing the same back-to-back restore cascade outside of login.
Solution
Startup settling period
Introduce
isInStartupSettling, a flag that blocks restore operations andsaveSectionOrder()calls for a fixed window afterperformSetup()completes:ProcessInfo.processInfo.systemUptime < 60(login item / auto-launch boot)During the period, every
didLaunchApplicationNotificationstill triggers a cache cycle so the UI stays accurate, but neitherrestoreItemsToSavedSectionsnorrestoreSavedItemOrderruns.saveSectionOrder()is also suppressed during settling to protect the UserDefaults-loaded reference order from being overwritten by transient physical positions before restore has run.When the period expires, one final
cacheItemsRegardless(skipRecentMoveCheck: true)enforces the user's saved layout against whatever macOS placed items.skipRecentMoveCheck: trueprevents the restore from being silently skipped by the 5 s move cooldown in caserelocateNewLeftmostItemsstamped a timestamp near the end of the window.Re-entrancy safety
startupSettlingTask: Task<Void, Never>?tracks the in-flight task. A subsequentperformSetup()call cancels the previous task before starting a new one. The cancelled task usesdo/catchonTask.sleepto exit cleanly without touching shared state.A
settlingDeadline: ContinuousClock.Instant?is stored so that re-entry computesmax(existingDeadline, newDeadline), preventing a second call from truncating the original 30 s login window ifsystemUptimehas crossed 60 s by then.Layout reset interaction
resetLayoutToFreshState()cancels the settling task and clears the flag immediately — a user-initiated reset is authoritative and must not be blocked by an in-flight settling period.Move cooldown bump
Both
restoreItemsToSavedSectionsandrestoreSavedItemOrderraise their move-operation cooldown from 2 s to 5 s, providing more separation between consecutive automatic restores when apps restart in quick succession.Changes
Single file:
Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swift(+89 / -5)Test plan
Fixes #224