Skip to content

Fix workspace color picker context menu blinking #2566

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
austinywang merged 4 commits into from
Apr 7, 2026
Merged

Fix workspace color picker context menu blinking #2566

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ed3c36f
Fix workspace color picker menu redraw
austinywang Apr 3, 2026
62d1189
Update clipboard callback for current GhosttyKit
austinywang Apr 7, 2026
3175f5e
Freeze tab row invalidations while workspace color menu is open
austinywang Apr 7, 2026
0837d1a
Limit workspace menu freeze to background row fields
austinywang Apr 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 81 additions & 15 deletions Sources/ContentView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9887,6 +9887,13 @@ private final class SidebarTabItemSettingsStore: ObservableObject {
}
}

private struct SidebarTabItemPresentationSnapshot: Equatable {
let tabId: UUID
let unreadCount: Int
let latestNotificationText: String?
let showsModifierShortcutHints: Bool
}

struct VerticalTabsSidebar: View {
@ObservedObject var updateViewModel: UpdateViewModel
let onSendFeedback: () -> Void
Expand All @@ -9902,6 +9909,7 @@ struct VerticalTabsSidebar: View {
@ObservedObject private var keyboardShortcutSettingsObserver = KeyboardShortcutSettingsObserver.shared
@State private var draggedTabId: UUID?
@State private var dropIndicator: SidebarDropIndicator?
@State private var frozenTabItemPresentation: SidebarTabItemPresentationSnapshot?
@AppStorage(WorkspacePresentationModeSettings.modeKey)
private var workspacePresentationMode = WorkspacePresentationModeSettings.defaultMode.rawValue

Expand Down Expand Up @@ -9967,6 +9975,26 @@ struct VerticalTabsSidebar: View {
let allRemoteContextMenuTargetsDisconnected = usesSelectedContextMenuTargets
? allSelectedRemoteContextMenuTargetsDisconnected
: (tab.isRemoteWorkspace && tab.remoteConnectionState == .disconnected)
let liveUnreadCount = notificationStore.unreadCount(forTabId: tab.id)
let liveLatestNotificationText: String? = {
guard showsSidebarNotificationMessage,
let notification = notificationStore.latestNotification(forTabId: tab.id) else {
return nil
}
let text = notification.body.isEmpty ? notification.title : notification.body
let trimmed = text.trimmingCharacters(in: .whitespacesAndNewlines)
return trimmed.isEmpty ? nil : trimmed
}()
let liveShowsModifierShortcutHints = modifierKeyMonitor.isModifierPressed
let livePresentation = SidebarTabItemPresentationSnapshot(
tabId: tab.id,
unreadCount: liveUnreadCount,
latestNotificationText: liveLatestNotificationText,
showsModifierShortcutHints: liveShowsModifierShortcutHints
)
let frozenPresentation = frozenTabItemPresentation?.tabId == tab.id
? frozenTabItemPresentation
: nil
TabItemView(
tabManager: tabManager,
notificationStore: notificationStore,
Expand All @@ -9980,29 +10008,23 @@ struct VerticalTabsSidebar: View {
workspaceShortcutModifierSymbol: workspaceNumberShortcut.numberedDigitHintPrefix,
canCloseWorkspace: canCloseWorkspace,
accessibilityWorkspaceCount: workspaceCount,
unreadCount: notificationStore.unreadCount(forTabId: tab.id),
latestNotificationText: {
guard showsSidebarNotificationMessage,
let notification = notificationStore.latestNotification(forTabId: tab.id) else {
return nil
}
let text = notification.body.isEmpty ? notification.title : notification.body
let trimmed = text.trimmingCharacters(in: .whitespacesAndNewlines)
return trimmed.isEmpty ? nil : trimmed
}(),
unreadCount: frozenPresentation?.unreadCount ?? liveUnreadCount,
latestNotificationText: frozenPresentation?.latestNotificationText ?? liveLatestNotificationText,
rowSpacing: tabRowSpacing,
setSelectionToTabs: { selection = .tabs },
selectedTabIds: $selectedTabIds,
lastSidebarSelectionIndex: $lastSidebarSelectionIndex,
showsModifierShortcutHints: modifierKeyMonitor.isModifierPressed,
showsModifierShortcutHints: frozenPresentation?.showsModifierShortcutHints ?? liveShowsModifierShortcutHints,
dragAutoScrollController: dragAutoScrollController,
draggedTabId: $draggedTabId,
dropIndicator: $dropIndicator,
contextMenuWorkspaceIds: contextMenuWorkspaceIds,
remoteContextMenuWorkspaceIds: remoteContextMenuWorkspaceIds,
allRemoteContextMenuTargetsConnecting: allRemoteContextMenuTargetsConnecting,
allRemoteContextMenuTargetsDisconnected: allRemoteContextMenuTargetsDisconnected,
settings: tabItemSettings
settings: tabItemSettings,
livePresentation: livePresentation,
frozenPresentation: $frozenTabItemPresentation
)
.equatable()
}
Expand Down Expand Up @@ -10103,6 +10125,11 @@ struct VerticalTabsSidebar: View {
dragAutoScrollController.stop()
dropIndicator = nil
}
.onChange(of: tabs.map(\.id)) { tabIds in
guard let frozenTabItemPresentation,
!tabIds.contains(frozenTabItemPresentation.tabId) else { return }
self.frozenTabItemPresentation = nil
}
Comment on lines +10149 to +10153

@coderabbitai coderabbitai Bot Apr 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

.onReceive(NotificationCenter.default.publisher(for: SidebarDragLifecycleNotification.requestClear)) { notification in
guard draggedTabId != nil else { return }
let reason = SidebarDragLifecycleNotification.reason(from: notification)
Expand Down Expand Up @@ -12377,6 +12404,11 @@ enum SidebarTrailingAccessoryWidthPolicy {
// and bridge only sidebar-visible workspace changes into local state.
// Do NOT add @EnvironmentObject or new @Binding without updating ==.
// Do NOT remove .equatable() from the ForEach call site in VerticalTabsSidebar.
private final class SidebarTabItemContextMenuState: ObservableObject {
var isVisible = false
var hasDeferredWorkspaceObservationInvalidation = false
}

private struct TabItemView: View, Equatable {
private static let workspaceObservationCoalesceInterval: RunLoop.SchedulerTimeType.Stride = .milliseconds(40)

Expand Down Expand Up @@ -12429,7 +12461,10 @@ private struct TabItemView: View, Equatable {
let allRemoteContextMenuTargetsConnecting: Bool
let allRemoteContextMenuTargetsDisconnected: Bool
let settings: SidebarTabItemSettingsSnapshot
let livePresentation: SidebarTabItemPresentationSnapshot
@Binding var frozenPresentation: SidebarTabItemPresentationSnapshot?
@State private var workspaceObservationGeneration: UInt64 = 0
@StateObject private var contextMenuState = SidebarTabItemContextMenuState()
@State private var isHovering = false
@State private var rowHeight: CGFloat = 1

Expand Down Expand Up @@ -13027,7 +13062,7 @@ private struct TabItemView: View, Equatable {
"desc=\"\(debugCommandPaletteTextPreview(description))\""
)
#endif
workspaceObservationGeneration &+= 1
scheduleWorkspaceObservationInvalidation()
}
.onReceive(
tab.sidebarObservationPublisher
Expand All @@ -13047,7 +13082,7 @@ private struct TabItemView: View, Equatable {
"desc=\"\(debugCommandPaletteTextPreview(description))\""
)
#endif
workspaceObservationGeneration &+= 1
scheduleWorkspaceObservationInvalidation()
}
.onDrag {
#if DEBUG
Expand Down Expand Up @@ -13078,6 +13113,7 @@ private struct TabItemView: View, Equatable {
updateSelection()
}
.onHover { hovering in
guard !contextMenuState.isVisible else { return }
isHovering = hovering
}
.accessibilityElement(children: .combine)
Expand All @@ -13089,7 +13125,37 @@ private struct TabItemView: View, Equatable {
.accessibilityAction(named: Text(moveDownActionText)) {
moveBy(1)
}
.contextMenu { workspaceContextMenu }
.contextMenu {
workspaceContextMenu
.onAppear {
contextMenuState.isVisible = true
contextMenuState.hasDeferredWorkspaceObservationInvalidation = false
frozenPresentation = livePresentation
}
.onDisappear {
contextMenuState.isVisible = false
frozenPresentation = nil
if isHovering {
isHovering = false
}
flushDeferredWorkspaceObservationInvalidation()
}
}
Comment on lines +13128 to +13143

@greptile-apps greptile-apps Bot Apr 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Potential stuck 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.

Comment on lines +13128 to +13143

@greptile-apps greptile-apps Bot Apr 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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!

}

private func scheduleWorkspaceObservationInvalidation() {
// Keep the context menu stable while background workspace telemetry keeps arriving.
if contextMenuState.isVisible {
contextMenuState.hasDeferredWorkspaceObservationInvalidation = true
return
}
workspaceObservationGeneration &+= 1
}

private func flushDeferredWorkspaceObservationInvalidation() {
guard contextMenuState.hasDeferredWorkspaceObservationInvalidation else { return }
contextMenuState.hasDeferredWorkspaceObservationInvalidation = false
workspaceObservationGeneration &+= 1
}

private func contextMenuLabel(multi: String, single: String, isMulti: Bool) -> String {
Expand Down
Loading