Fix workspace color picker context menu blinking - #2566
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds per-tab frozen presentation snapshots and context-menu state to freeze UI and defer workspace-observation invalidation while a tab's context menu is open; clears stale frozen snapshots when tab identities change and prevents hover updates during menu visibility. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Sidebar as VerticalTabsSidebar
participant TabView as TabItemView
participant Telemetry as WorkspaceTelemetry
rect rgba(200,200,255,0.5)
User->>Sidebar: right-click tab
Sidebar->>TabView: show context menu (set visible)
end
rect rgba(200,255,200,0.5)
TabView->>TabView: frozenPresentation = livePresentation
TabView->>Telemetry: defer invalidation (hasDeferred = true)
TabView-->>Sidebar: suppress hover updates while visible
end
rect rgba(255,200,200,0.5)
User->>TabView: close context menu
TabView->>Telemetry: flush deferred invalidation (increment once)
TabView->>TabView: clear frozenPresentation, restore hover behavior
Sidebar->>TabView: resume live updates
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Greptile SummaryThis PR fixes the workspace color submenu blinking (issue #2560) by deferring
Confidence Score: 3/5Safe to merge for the happy path; carries a low-probability stuck-state risk if onDisappear is skipped on abnormal context menu dismissal. The core fix is logically sound and addresses the root cause clearly. The GhosttyTerminalView change is a trivial API alignment. The main uncertainty is the reliability of onAppear/onDisappear inside a .contextMenu { } block on macOS across all dismissal paths — if onDisappear is ever skipped, contextMenuVisible stays true and the sidebar row stops updating entirely for the lifetime of that view instance. Sources/ContentView.swift — specifically the onAppear/onDisappear lifecycle contract inside .contextMenu { } and the stuck-state scenario if onDisappear doesn't fire. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["onReceive fires\n(immediate or debounced publisher)"] --> B["scheduleWorkspaceObservationInvalidation()"]
B --> C{contextMenuVisible?}
C -- Yes --> D["hasDeferredWorkspaceObservationInvalidation = true\n(return, skip increment)"]
C -- No --> E["workspaceObservationGeneration &+= 1\n→ SwiftUI re-evaluates body"]
F["User right-clicks row"] --> G[".onAppear fires"]
G --> H["contextMenuVisible = true"]
I["User dismisses menu"] --> J[".onDisappear fires"]
J --> K["contextMenuVisible = false"]
K --> L["flushDeferredWorkspaceObservationInvalidation()"]
L --> M{hasDeferredWorkspaceObservationInvalidation?}
M -- Yes --> N["workspaceObservationGeneration &+= 1\n→ one coalesced body re-eval"]
M -- No --> O["no-op"]
P["⚠️ Abnormal dismissal\nonDisappear skipped"] --> Q["contextMenuVisible stuck = true\nSidebar row never refreshes"]
Reviews (1): Last reviewed commit: "Update clipboard callback for current Gh..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| .contextMenu { | ||
| workspaceContextMenu | ||
| .onAppear { | ||
| contextMenuVisible = true | ||
| } | ||
| .onDisappear { | ||
| contextMenuVisible = false | ||
| flushDeferredWorkspaceObservationInvalidation() | ||
| } | ||
| } |
There was a problem hiding this comment.
contextMenuVisible state
contextMenuVisible is set to true in .onAppear and only reset to false in .onDisappear. On macOS, SwiftUI context menus are backed by NSMenu, and .onDisappear on context menu content views may not fire in certain edge cases — for example, when the window is closed, the app is force-quit, or the menu is dismissed via an unusual path. If .onDisappear doesn't fire, contextMenuVisible remains true indefinitely.
In that stuck state, every subsequent call to scheduleWorkspaceObservationInvalidation() defers the update and sets hasDeferredWorkspaceObservationInvalidation = true, but flushDeferredWorkspaceObservationInvalidation() is never called (since that only happens in .onDisappear). The sidebar row would never refresh again for the lifetime of the TabItemView instance.
Consider adding a safety reset — for example, observing a SwiftUI scenePhase change — or defending against an abnormally long contextMenuVisible = true window with a timeout-based flush.
| .contextMenu { | ||
| workspaceContextMenu | ||
| .onAppear { | ||
| contextMenuVisible = true | ||
| } | ||
| .onDisappear { | ||
| contextMenuVisible = false | ||
| flushDeferredWorkspaceObservationInvalidation() | ||
| } | ||
| } |
There was a problem hiding this comment.
onAppear/onDisappear on multi-item context menu content
workspaceContextMenu is a @ViewBuilder that returns a heterogeneous TupleView (multiple Buttons, Menu, Divider, etc.). Attaching .onAppear/.onDisappear to a composite view inside .contextMenu { } on macOS relies on SwiftUI treating the overall content block as a single lifecycle-trackable node.
In practice this works on recent macOS releases, but it may be worth wrapping the content in an explicit Group { } to clarify intent and give SwiftUI a definitive single root:
.contextMenu {
Group {
workspaceContextMenu
}
.onAppear {
contextMenuVisible = true
}
.onDisappear {
contextMenuVisible = false
flushDeferredWorkspaceObservationInvalidation()
}
}This makes the attachment point unambiguous across SwiftUI versions.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
edd733f to
3175f5e
Compare
|
This review could not be run because your cubic account has exceeded the monthly review limit. If you need help restoring access, please contact contact@cubic.dev. |
![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 `@Sources/ContentView.swift`:
- Around line 10149-10153: The current onChange only clears
frozenTabItemPresentation when its tabId disappears; extend this to also clear
the frozen presentation (and any frozen snapshot) when any frozen target
workspace ID no longer exists: inside the .onChange(of: tabs.map(\.id)) handler,
compute the current set of workspace IDs (including contextMenuWorkspaceIds and
remoteContextMenuWorkspaceIds), then check frozenTabItemPresentation's target
IDs (or the property that holds the frozen target workspace IDs) and set
self.frozenTabItemPresentation = nil if any of those target IDs are missing from
the current set; ensure you reference frozenTabItemPresentation,
contextMenuWorkspaceIds and remoteContextMenuWorkspaceIds when making this check
so stale plural actions are cleared.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| .onChange(of: tabs.map(\.id)) { tabIds in | ||
| guard let frozenTabItemPresentation, | ||
| !tabIds.contains(frozenTabItemPresentation.tabId) else { return } | ||
| self.frozenTabItemPresentation = nil | ||
| } |
There was a problem hiding this comment.
Also clear the frozen snapshot when any frozen target disappears.
Right now this only checks the anchor tabId. If the menu was opened on a multi-selection and one of contextMenuWorkspaceIds / remoteContextMenuWorkspaceIds is removed while the menu is open, the frozen menu can keep stale target IDs and stale plural actions until dismiss.
Suggested fix
- .onChange(of: tabs.map(\.id)) { tabIds in
- guard let frozenTabItemPresentation,
- !tabIds.contains(frozenTabItemPresentation.tabId) else { return }
- self.frozenTabItemPresentation = nil
+ .onChange(of: tabs.map(\.id)) { tabIds in
+ guard let frozenTabItemPresentation else { return }
+ let existingIds = Set(tabIds)
+ let frozenTargetIds = Set(
+ [frozenTabItemPresentation.tabId]
+ + frozenTabItemPresentation.contextMenuWorkspaceIds
+ + frozenTabItemPresentation.remoteContextMenuWorkspaceIds
+ )
+ guard frozenTargetIds.isSubset(of: existingIds) else {
+ self.frozenTabItemPresentation = nil
+ return
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/ContentView.swift` around lines 10149 - 10153, The current onChange
only clears frozenTabItemPresentation when its tabId disappears; extend this to
also clear the frozen presentation (and any frozen snapshot) when any frozen
target workspace ID no longer exists: inside the .onChange(of: tabs.map(\.id))
handler, compute the current set of workspace IDs (including
contextMenuWorkspaceIds and remoteContextMenuWorkspaceIds), then check
frozenTabItemPresentation's target IDs (or the property that holds the frozen
target workspace IDs) and set self.frozenTabItemPresentation = nil if any of
those target IDs are missing from the current set; ensure you reference
frozenTabItemPresentation, contextMenuWorkspaceIds and
remoteContextMenuWorkspaceIds when making this check so stale plural actions are
cleared.
- Extract SidebarTabCloseButton so hover reads no longer invalidate the full TabItemView body on every sidebar row (Codex P1). - Restore context-menu frozen-presentation locally on the row using @State instead of the deleted global snapshot, preserving the stable visuals while the menu is open (Codex P2; preserves #2566 fix). - Extract SidebarEmptyAreaDropIndicatorOverlay so SidebarEmptyArea body does not re-evaluate at ~60fps during drag (Greptile P2). - Update .onChange signature to the two-argument form required by the macOS 14+ SDK (Greptile P2). - Tighten the LazyVStack doc comment to note that draggedTabId is also observed by the sidebar body via onChange (CodeRabbit minor).
This branch was successfully deployed
1 participant
Closes #2560.
Summary
TabItemViewworkspace observation invalidations while its context menu is on screen so the workspace color submenu stays stableread_clipboard_cbinGhosttyTerminalViewto match the current importedGhosttyKitcallback signature so the branch builds cleanly in the current repo stateRoot Cause
TabItemViewwas subscribing to debounced workspace updates and incrementingworkspaceObservationGenerationeven while the context menu was open. Those updates caused SwiftUI to re-evaluate the row body and rebuild.contextMenu, which dismissed and re-presented the workspace color submenu repeatedly.Verification
./scripts/reload.sh --tag color-picker-blink --launch./scripts/reload.sh --tag color-picker-blinkNotes
GhosttyTerminalViewcallback adjustment is not part of the color-picker behavior change itself; it is included here because it was required to keep the tagged debug build green in the current branch state.Summary by cubic
Fixes blinking in the workspace color picker by freezing only background row fields (unread count, latest notification text, modifier-hint state) and deferring updates while the context menu is open. Also updates the
read_clipboard_cbto the latestGhosttyKitAPI.Written for commit 0837d1a. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Note
Medium Risk
Changes SwiftUI invalidation and state handling for sidebar tab rows while a context menu is open; incorrect state coordination could leave sidebar badges/hover state stale or miss an update until the menu closes.
Overview
Prevents the workspace/tab context menu (including the color picker submenu) from blinking by freezing each tab row’s presentation (unread badge, latest notification text, shortcut hint visibility) while the menu is visible.
Adds context-menu-aware invalidation in
TabItemView: workspace observation updates are deferred during menu display and a single coalesced redraw is flushed on dismissal, with hover updates suppressed while the menu is open. Also clears any frozen presentation if the underlying tab disappears.Reviewed by Cursor Bugbot for commit 0837d1a. Bugbot is set up for automated code reviews on this repo. Configure here.