fix: reduce IceBar click jitter with fast-path temporarilyShow#338
fix: reduce IceBar click jitter with fast-path temporarilyShow#338CamilleGuillory wants to merge 1 commit intostonerl:developmentfrom
Conversation
When clicking hidden items from the IceBar, the Cmd+drag move() function retried up to 8 times with exponentially increasing timeouts (~1.8s of visible icon jitter), even though the first attempt always repositioned the item close enough. Post-move delays (250ms settle wait, item re-fetch, 25ms extra sleep) added further latency before the click. Changes: - Add maxMoveAttempts parameter to move() (default 8, no behavior change for existing callers) - Add fastPath parameter to temporarilyShow() that limits move to 1 attempt and skips post-move delays - IceBar click handlers now use fastPath: true for both left and right clicks
📝 WalkthroughWalkthroughThe changes introduce a "fast path" mode for menu bar item visibility handling. The Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swift`:
- Around line 1953-1954: The parameter maxMoveAttempts can be <= 0 and will
crash the loop "for n in 1 ... maxAttempts"; fix this by validating or clamping
it before the range is built (e.g., compute let attempts = max(1,
maxMoveAttempts) or guard maxMoveAttempts > 0 else { return } ), then use that
safe value in the loop ("for n in 1 ... attempts"). Update the initializer/
method signature handling the watchdogTimeout/maxMoveAttempts parameters
(referencing maxMoveAttempts and the loop that uses maxAttempts) to ensure no
runtime trap occurs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ee5887f9-7241-4a6d-8756-60c17f8f8ef9
📒 Files selected for processing (2)
Thaw/MenuBar/IceBar/IceBar.swiftThaw/MenuBar/MenuBarItems/MenuBarItemManager.swift
| watchdogTimeout: DispatchTimeInterval? = nil, | ||
| maxMoveAttempts: Int = 8 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read-only verification:
# 1) Locate all call sites passing maxMoveAttempts.
# 2) Confirm move() still uses a closed range over maxAttempts.
# 3) Confirm there is (or isn't) a guard/clamp for non-positive values.
rg -nP --type=swift '\bmaxMoveAttempts\s*:'
rg -nP --type=swift 'for\s+\w+\s+in\s+1\s*\.\.\.\s*maxAttempts'
rg -nP --type=swift 'maxMoveAttempts\s*<\s*1|max\(\s*1\s*,\s*maxMoveAttempts\s*\)'Repository: stonerl/Thaw
Length of output: 515
🏁 Script executed:
# Check the file size first
wc -l Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swiftRepository: stonerl/Thaw
Length of output: 111
🏁 Script executed:
# Read the move() function around lines 1953-2020 to see the full signature and guard logic
sed -n '1945,2020p' Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swift | cat -nRepository: stonerl/Thaw
Length of output: 3598
🏁 Script executed:
# Also read around the second loop at line 2188 to understand context
sed -n '2180,2200p' Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swift | cat -nRepository: stonerl/Thaw
Length of output: 1180
Guard maxMoveAttempts to prevent a runtime crash.
At line 2014, for n in 1 ... maxAttempts will trap if maxMoveAttempts <= 0. The parameter accepts any integer without validation, so a caller could pass an invalid value. Add a guard or clamp before building the range.
🛠️ Proposed fix
func move(
item: MenuBarItem,
to destination: MoveDestination,
on displayID: CGDirectDisplayID? = nil,
skipInputPause: Bool = false,
watchdogTimeout: DispatchTimeInterval? = nil,
maxMoveAttempts: Int = 8
) async throws {
+ let maxAttempts = max(1, maxMoveAttempts)
+ if maxMoveAttempts < 1 {
+ MenuBarItemManager.diagLog.warning("move(): received invalid maxMoveAttempts=\(maxMoveAttempts), clamping to 1")
+ }
+
guard item.isMovable else {
throw EventError.itemNotMovable(item)
}
@@
- let maxAttempts = maxMoveAttempts
for n in 1 ... maxAttempts {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Thaw/MenuBar/MenuBarItems/MenuBarItemManager.swift` around lines 1953 - 1954,
The parameter maxMoveAttempts can be <= 0 and will crash the loop "for n in 1
... maxAttempts"; fix this by validating or clamping it before the range is
built (e.g., compute let attempts = max(1, maxMoveAttempts) or guard
maxMoveAttempts > 0 else { return } ), then use that safe value in the loop
("for n in 1 ... attempts"). Update the initializer/ method signature handling
the watchdogTimeout/maxMoveAttempts parameters (referencing maxMoveAttempts and
the loop that uses maxAttempts) to ensure no runtime trap occurs.
1 participant
Problem
When clicking hidden items from the IceBar, the icon visibly jitters/shifts in the menu bar for ~1.8 seconds before the menu opens. This is caused by:
move()retrying 8 times with exponentially increasing timeouts, even though the first attempt always repositions the item close enough (position verification fails due to slight drift after mouseUp)Fix
Add a
fastPathmode for IceBar clicks that:move()to 1 attempt (via newmaxMoveAttemptsparameter, default 8 for existing callers)This reduces the visible jitter from ~1.8s to a brief flash (~100ms).
Changes
MenuBarItemManager.swift: AddmaxMoveAttemptsparameter tomove()andfastPathparameter totemporarilyShow()IceBar.swift: PassfastPath: truefor both left and right click handlersTesting
Tested on macOS Tahoe with 30+ menu bar items across two displays. Hidden items clicked from the IceBar now open their menus with minimal visible jitter. No regression for non-IceBar move operations (layout, rehide, etc.) which continue using the default 8-attempt path.
Summary by CodeRabbit