Fix workspace color picker context menu regression - #4648
austinywang wants to merge 4 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR centralizes context-menu visibility into a lifecycle, adds a hover-tracker callback to report AppKit menu tracking, returns the full frozen presentation snapshot for matched tab IDs, and replaces ad-hoc onAppear/onDisappear freezing with begin/end orchestration that conditionally clears frozen state and flushes deferred invalidation. ChangesContext-Menu Lifecycle and Presentation Freezing
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 1 warning, 1 inconclusive)
✅ Passed checks (13 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@Sources/Sidebar/SidebarWorkspaceSnapshotRefreshPolicy.swift`:
- Around line 115-121: In contextMenuDidDisappear(), avoid clearing the hover
suppression while AppKit tracking remains authoritative: only set
contextMenuTrackingSuppressesCloseButton = false and call
applyDeferredPointerHovering() when contextMenuLifecycle != .appKitTracking so
the suppression remains active until contextMenuTrackingDidEnd() clears it;
update the logic in the contextMenuDidDisappear() method to check
contextMenuLifecycle (and keep the existing contextMenuLifecycle = .inactive
assignment behavior) before releasing suppression.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5359a749-9a99-4a51-82f7-46b5b311d31b
📒 Files selected for processing (4)
Sources/ContentView.swiftSources/Sidebar/SidebarWorkspaceRowHoverTracker.swiftSources/Sidebar/SidebarWorkspaceSnapshotRefreshPolicy.swiftcmuxTests/SidebarWorkspaceSnapshotRefreshPolicyTests.swift
Greptile SummaryFixes the workspace color picker context menu regression by fully freezing
Confidence Score: 5/5Safe to merge; the lifecycle state machine changes are well-guarded and regression coverage is thorough. The logic for the full presentation freeze, the dual-entrypoint idempotency guards, and the contextMenuLifecycle enum transitions are all correct and directly covered by the new tests. No functional regressions introduced. No files require special attention; the @State vs @StateObject re-render behavior difference in ContentView.swift is worth awareness but does not affect correctness. Important Files Changed
Sequence DiagramsequenceDiagram
participant AppKit as NSMenu (AppKit)
participant Tracker as SidebarWorkspaceRowHoverTracker
participant TabItemView
participant LifecycleState as SidebarWorkspaceRowInteractionState
participant FreezeState as frozenPresentation (Binding)
AppKit->>Tracker: NSMenu.didBeginTrackingNotification
Tracker->>LifecycleState: contextMenuTrackingDidBegin() → .appKitTracking
Tracker->>TabItemView: onContextMenuTrackingChanged(true)
TabItemView->>FreezeState: "beginContextMenuPresentationFreeze() → freeze = livePresentation"
Note over TabItemView: SwiftUI .onAppear (may be late or skipped)
TabItemView->>LifecycleState: contextMenuDidAppear() → stays .appKitTracking (guarded)
TabItemView->>FreezeState: beginContextMenuPresentationFreeze() → guard: already frozen, no-op
Note over TabItemView: Notification store publish (unread/preview update)
TabItemView->>TabItemView: resolved() returns frozen → contextMenu content unchanged
Note over TabItemView: SwiftUI .onDisappear (may be early or skipped)
TabItemView->>LifecycleState: contextMenuDidDisappear() → guard: .appKitTracking, no-op
Note over TabItemView: endContextMenuPresentationFreeze() NOT called (contextMenuVisible still true)
AppKit->>Tracker: NSMenu.didEndTrackingNotification
Tracker->>LifecycleState: contextMenuTrackingDidEnd() → .inactive
Tracker->>TabItemView: onContextMenuTrackingChanged(false)
TabItemView->>FreezeState: "endContextMenuPresentationFreeze() → frozen = nil, flush deferred"
Reviews (3): Last reviewed commit: "refactor: derive context menu freeze sta..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| mutating func contextMenuDidDisappear() { | ||
| contextMenuVisible = false | ||
| if contextMenuLifecycle != .appKitTracking { | ||
| contextMenuLifecycle = .inactive | ||
| } | ||
| contextMenuTrackingSuppressesCloseButton = false | ||
| applyDeferredPointerHovering() | ||
| } |
There was a problem hiding this comment.
contextMenuDidDisappear unconditionally clears contextMenuTrackingSuppressesCloseButton and calls applyDeferredPointerHovering() even when lifecycle is .appKitTracking. If SwiftUI fires .onDisappear while AppKit is still tracking — the exact rebuild scenario this PR is guarding against, which can still occur if the freeze fails to hold — the close button suppression is prematurely dropped and any pending deferred hover is drained. The lifecycle check already gates contextMenuLifecycle, but the same guard is not applied to the two side-effects below it.
| mutating func contextMenuDidDisappear() { | |
| contextMenuVisible = false | |
| if contextMenuLifecycle != .appKitTracking { | |
| contextMenuLifecycle = .inactive | |
| } | |
| contextMenuTrackingSuppressesCloseButton = false | |
| applyDeferredPointerHovering() | |
| } | |
| mutating func contextMenuDidDisappear() { | |
| guard contextMenuLifecycle != .appKitTracking else { return } | |
| contextMenuLifecycle = .inactive | |
| contextMenuTrackingSuppressesCloseButton = false | |
| applyDeferredPointerHovering() | |
| } |
There was a problem hiding this comment.
Fixed in 362343c with the same lifecycle guard, plus regression coverage for SwiftUI disappearance during active AppKit tracking.
— Claude Code
| private final class SidebarTabItemContextMenuState: ObservableObject { | ||
| var presentationFreezeActive = false | ||
| var hasDeferredWorkspaceObservationInvalidation = false | ||
| var pendingWorkspaceSnapshot: SidebarWorkspaceSnapshotBuilder.Snapshot? | ||
| } |
There was a problem hiding this comment.
SidebarTabItemContextMenuState is a mutable final class: ObservableObject without @MainActor. All its call sites are on the main actor (SwiftUI lifecycle callbacks and AppKit queue: .main notifications), but the absence of an explicit @MainActor annotation leaves that isolation implicit. In Swift 6 strict-concurrency mode, storing or accessing this class from a @MainActor-bound view without the annotation can produce isolation diagnostics, and the new presentationFreezeActive property deepens the pattern.
| private final class SidebarTabItemContextMenuState: ObservableObject { | |
| var presentationFreezeActive = false | |
| var hasDeferredWorkspaceObservationInvalidation = false | |
| var pendingWorkspaceSnapshot: SidebarWorkspaceSnapshotBuilder.Snapshot? | |
| } | |
| @MainActor | |
| private final class SidebarTabItemContextMenuState: ObservableObject { | |
| var presentationFreezeActive = false | |
| var hasDeferredWorkspaceObservationInvalidation = false | |
| var pendingWorkspaceSnapshot: SidebarWorkspaceSnapshotBuilder.Snapshot? | |
| } |
Rule Used: Flag new or materially worsened Swift 6 actor isol... (source)
There was a problem hiding this comment.
Fixed in 362343c by marking SidebarTabItemContextMenuState as @mainactor; its SwiftUI/AppKit menu lifecycle accesses are main-actor UI state.
— Claude Code
There was a problem hiding this comment.
Updated in 6de0bbe: instead of keeping this as an @mainactor ObservableObject, I removed the object entirely and made the non-published context-menu storage a plain @State value. The freeze-active bit is now derived from the frozen presentation binding.
— Claude Code
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 `@Sources/ContentView.swift`:
- Around line 14033-14040: The guard in endContextMenuPresentationFreeze() is
too permissive and can clear a shared frozenPresentation for another row; update
the logic so you only clear frozenPresentation and call
flushDeferredWorkspaceObservationInvalidation() when the frozenPresentation
actually belongs to the current row (i.e. frozenPresentation?.tabId == tab.id)
AND the deferred invalidation flag applies to this same row
(contextMenuState.hasDeferredWorkspaceObservationInvalidation must be tied to
this row), rather than using OR; in practice change the condition so both checks
must be true (or add an explicit comparison between frozenPresentation's owner
identifier and this row's contextMenuState identifier) before nil-ing
frozenPresentation and flushing.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 442d3c3b-b8cd-45b6-9680-6adfcb141d72
📒 Files selected for processing (1)
Sources/ContentView.swift
| private func endContextMenuPresentationFreeze() { | ||
| guard frozenPresentation?.tabId == tab.id || | ||
| contextMenuState.hasDeferredWorkspaceObservationInvalidation | ||
| else { | ||
| return | ||
| } | ||
| frozenPresentation = nil | ||
| flushDeferredWorkspaceObservationInvalidation() |
There was a problem hiding this comment.
Don't clear another row's frozen snapshot during late teardown.
If row A still has deferred invalidation and row B has already started a new context menu, this guard still passes for A and Line 14039 clears the shared frozenPresentation for B. That reopens the same cross-row rebuild/dismissal race this PR is trying to close.
Suggested fix
private func endContextMenuPresentationFreeze() {
- guard frozenPresentation?.tabId == tab.id ||
+ let ownsFrozenPresentation = frozenPresentation?.tabId == tab.id
+ guard ownsFrozenPresentation ||
contextMenuState.hasDeferredWorkspaceObservationInvalidation
else {
return
}
- frozenPresentation = nil
+ if ownsFrozenPresentation {
+ frozenPresentation = nil
+ }
flushDeferredWorkspaceObservationInvalidation()
}🤖 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 `@Sources/ContentView.swift` around lines 14033 - 14040, The guard in
endContextMenuPresentationFreeze() is too permissive and can clear a shared
frozenPresentation for another row; update the logic so you only clear
frozenPresentation and call flushDeferredWorkspaceObservationInvalidation() when
the frozenPresentation actually belongs to the current row (i.e.
frozenPresentation?.tabId == tab.id) AND the deferred invalidation flag applies
to this same row (contextMenuState.hasDeferredWorkspaceObservationInvalidation
must be tied to this row), rather than using OR; in practice change the
condition so both checks must be true (or add an explicit comparison between
frozenPresentation's owner identifier and this row's contextMenuState
identifier) before nil-ing frozenPresentation and flushing.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6de0bbe. Configure here.
| @Binding var frozenPresentation: SidebarTabItemPresentationSnapshot? | ||
| @State private var workspaceSnapshotStorage: SidebarWorkspaceSnapshotBuilder.Snapshot? | ||
| @StateObject private var contextMenuState = SidebarTabItemContextMenuState() | ||
| @State private var contextMenuState = SidebarTabItemContextMenuState() |
There was a problem hiding this comment.
@State mutations trigger body re-evaluations during menu freeze
Medium Severity
Changing contextMenuState from @StateObject (class with non-@Published properties) to @State (struct) means mutations to pendingWorkspaceSnapshot now trigger SwiftUI body re-evaluations during the freeze. Each debounced workspace observation arriving while the menu is open mutates @State, causing the body—including .contextMenu { workspaceContextMenu }—to be re-evaluated. With the prior @StateObject approach, these internal bookkeeping mutations were invisible to SwiftUI's update cycle. Since this entire PR exists to prevent .contextMenu rebuilds mid-submenu, the new @State re-evaluations work against that guarantee, risking the same submenu-dismissal regression on macOS versions where SwiftUI may diff and rebuild open context-menu content.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 6de0bbe. Configure here.
This branch was successfully deployed
1 participant
Fixes #4646.
Regression context: #2560 was fixed by #2566 on 2026-04-07 by freezing
TabItemViewcontext-menu presentation and deferring workspace observation updates while the menu is open.Summary
TabItemViewequality and rebuild.contextMenumid-submenu.NSMenutracking and SwiftUI.contextMenucontent as the same active phase. This covers the macOS 26 case where SwiftUI.onAppearmay be late or skipped, and the inverse.onDisappear-skipped case.Root Cause
#4084 made frozen sidebar presentations keep unread count and latest notification text live during a context-menu freeze. Those fields participate in
TabItemView ==, so notification-store publishes could still re-evaluate the row and rebuild.contextMenuwhile the Workspace Color submenu was open. Separately, the deferral gate only trusted SwiftUI.contextMenu.onAppear, while the row already had a more reliable AppKit menu-tracking signal.Regression Coverage
Two-commit red/green structure:
e4f809c0dchanges/adds failing runtime policy tests for frozen presentation stability and AppKit menu-tracking visibility.515b8e45cimplements the fix.Verification
git diff --checkNot run locally per repo/task policy: local tests/builds and
./scripts/reload.sh.Cloud Mac video: unavailable.
cloud-mac preflightis currently blocked locally by unauthenticatedmacfleet, andgh auth statusreports invalid keyring tokens. I still pushed the branch successfully over the repo remote, but cloud VM provisioning could not be started from this checkout.Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Touching sidebar row/menu interaction state and snapshot freezing logic could introduce subtle UI regressions (hover/close button visibility or deferred snapshot flushing), though changes are localized and backed by new tests.
Overview
Fixes a sidebar workspace context-menu regression by fully freezing
SidebarTabItemPresentationSnapshotwhile a row menu is active, preventing live unread/notification updates from rebuildingTabItemView(and its.contextMenu) mid-interaction.Unifies menu visibility handling by replacing the simple boolean with a SwiftUI + AppKit context-menu lifecycle in
SidebarWorkspaceRowInteractionState, and starts/ends the freeze based on both SwiftUI appear/disappear and AppKitNSMenutracking callbacks (via a newonContextMenuTrackingChangedhook inSidebarWorkspaceRowHoverTracker). Adds/updates tests to cover the new freeze semantics and menu lifecycle edge cases.Reviewed by Cursor Bugbot for commit 6de0bbe. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes the workspace color picker context menu regression by fully freezing the sidebar row while any menu is active and deriving visibility from a unified SwiftUI/AppKit lifecycle. Stops unread/preview changes from rebuilding the menu mid-submenu and keeps hover suppression until AppKit tracking ends.
Bug Fixes
SidebarTabItemPresentationSnapshotduring an active menu to stabilize unread count, preview text, and shortcut hints.NSMenutracking and SwiftUI; ignore SwiftUI disappear while tracking and flush deferred workspace updates only after tracking ends;SidebarWorkspaceRowHoverTrackernow reports tracking changes to drive the freeze.Refactors
SidebarWorkspaceRowInteractionStateto unify visibility and close-button suppression across edge cases.Written for commit 6de0bbe. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
Bug Fixes
Tests