Repository navigation
Conversation
|
Someone is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces per-row SwiftUI .contextMenu with an AppKit-backed WorkspaceContextMenuOverlay (NSViewRepresentable), adds WorkspaceContextMenuAction, updates TabItemView inputs/Equatable, snapshots frozen presentation during menu lifecycle, precomputes Finder-directory caches, and implements an imperative handleMenuAction(_:) with remote-workspace helpers. ChangesWorkspace Context Menu Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (5 errors, 1 warning)
✅ Passed checks (11 passed)
✨ 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 |
Contributor
Greptile SummaryReplaces the sidebar workspace right-click handler from SwiftUI
Confidence Score: 5/5Safe to merge — the architectural swap is well-contained, actor isolation is correct throughout, and the passthrough hitTest preserves all existing drag/selection/hover/scroll interactions. The NSMenu lifecycle is cleanly isolated from SwiftUI's render cycle: buildMenu() is called once per right-click from menu(for:event:), the coordinator is @mainactor matching AppKit's main-thread menu dispatch, and all precomputed state (tabCount, eligibleTargetIds, hasCustomColor, etc.) was verified to match what the old ViewBuilder computed. The tabCount parameter maps to renderContext.workspaceCount which equals tabs.count, so Close Below / Above / Others enabled-state logic is equivalent to the original. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant U as User
participant MHV as MenuHostView (NSView)
participant C as Coordinator (NSMenuDelegate)
participant SM as SwiftUI TabItemView
participant Store as tabManager / stores
U->>MHV: rightMouseDown / ctrl+leftMouseDown
MHV->>MHV: hitTest() → return self (claim event)
MHV->>C: menu(for event:)
C->>C: buildMenu() — snapshot overlay props at click time
C-->>MHV: "NSMenu (prebuilt, autoenablesItems=false)"
MHV->>U: NSMenu displayed (AppKit-owned, outside SwiftUI render cycle)
Note over SM: SwiftUI body re-evaluations paused during menu display
U->>C: Select menu item (e.g. Close Workspace)
C->>C: handleMenuItemAction(_:) → overlay.onAction(.close)
C->>SM: handleMenuAction(.close) via onAction closure
SM->>Store: closeTabs(targetIds, allowPinned:)
C->>SM: menuWillOpen → onMenuWillOpen()
SM->>SM: rowInteractionState.contextMenuDidAppear()
C->>SM: menuDidClose → onMenuDidClose()
SM->>SM: rowInteractionState.contextMenuDidDisappear()
SM->>SM: flushDeferredWorkspaceObservationInvalidation()
Reviews (8): Last reviewed commit: "fix: rewrite sidebar workspace context m..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 14005-14029: TabItemView.body is currently reading tabManager and
notificationStore and passing tabManager into WorkspaceContextMenuOverlay which
can cause stale menu state; precompute all mutable/derived values above the row
(e.g., let precomputedTabCount = tabManager.tabs.count, let
precomputedHasLatestNotifications = hasLatestNotifications(in:
contextMenuWorkspaceIds), let canMarkWorkspaceRead =
notificationStore.canMarkWorkspaceRead(forTabIds: contextMenuWorkspaceIds), let
canMarkWorkspaceUnread = notificationStore.canMarkWorkspaceUnread(forTabIds:
contextMenuWorkspaceIds)) and then pass those snapshot values (and any needed
action closures that capture tabManager lazily) into WorkspaceContextMenuOverlay
instead of the tabManager or direct notificationStore reads; remove the direct
tabManager reference from the overlay initializer and replace with explicit
value parameters and small closures for mutations so TabItemView.body performs
no store reads.
In `@Sources/Sidebar/WorkspaceContextMenuOverlay.swift`:
- Line 37: Remove the TabManager reference from WorkspaceContextMenuOverlay and
instead accept an immutable referenceWindowId input from the parent; replace
uses of the tabManager property (specifically the computation of
referenceWindowId) with this new immutable property, update the view
initializer/signature to take referenceWindowId, and propagate that value into
any row/drop-gap subviews so they no longer capture an ObservableObject; also
find the other occurrences in this file where TabManager is used (the spots
noted around the later row implementations) and refactor them the same way so
rows only receive immutable data plus action closures.
🪄 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: ffc1f31b-9937-4e11-9618-5c1c53a2e7c0
📒 Files selected for processing (3)
Sources/ContentView.swiftSources/Sidebar/WorkspaceContextMenuOverlay.swiftcmux.xcodeproj/project.pbxproj
nanami-he
force-pushed
the
feature/nsmenu-sidebar-context-menu
branch
from
May 23, 2026 04:40
2518293 to
ce4fc47
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
Sources/ContentView.swift (1)
10801-10805: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftFinish the row-store extraction.
Precomputing these menu flags is the right direction, but
TabItemViewis still being fed livetabManager/notificationStorereferences, so the equatable row still sits on the forbidden store-holding path under the lazy sidebar. Please finish this by passing only immutable snapshots plus action closures intoTabItemView, and keep the store-owned imperative handlers above the row boundary.As per coding guidelines, "
TabItemViewinContentView.swift... Do not readtabManagerornotificationStorein the body; use precomputedletparameters instead" and "no view below that boundary may hold a reference to anObservableObject/@Observablestore. Rows and drop-gaps receive immutable value snapshots plus closure action bundles only."Also applies to: 10836-10840
🤖 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 10801 - 10805, TabItemView is still reading live tabManager/notificationStore inside its body; finish the row-store extraction by computing immutable snapshots (e.g. tab snapshot including id, title, unread count, hasLatestNotification) and the precomputed flags (precomputedTabCount, precomputedHasLatestNotifications, canMarkWorkspaceRead, canMarkWorkspaceUnread, referenceWindowId, contextMenuWorkspaceIds) above the row and pass only those value snapshots plus action closures into TabItemView; remove any direct references to tabManager or notificationStore from TabItemView and keep all imperative store calls (mark read/unread, open/close/move tab, fetch latest) in the parent scope where you build the closures so rows and drop-gaps hold no ObservableObject/store references.
🤖 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 14174-14184: The context-menu close cases call closeTabs,
closeOtherTabs, closeTabsBelow, and closeTabsAbove without an explicit
CloseTabConfirmationTrigger, which allows the default/forbidden trigger to be
used; update each case (.close, .closeOthers, .closeBelow, .closeAbove) to pass
an explicit trigger (e.g. CloseTabConfirmationTrigger.contextMenu) into the
calls to closeTabs(targetIds, allowPinned:), closeOtherTabs(_:),
closeTabsBelow(tabId:), and closeTabsAbove(tabId:), and ensure downstream APIs
(Workspace.markExplicitClose(surfaceId:trigger:) and
TabManager.closeWorkspaceFromCloseTabGesture(_:trigger:)) are invoked with that
trigger rather than relying on default parameter values.
In `@Sources/Sidebar/WorkspaceContextMenuOverlay.swift`:
- Around line 36-37: The context menu currently checks overlay.tab.customColor
(in WorkspaceContextMenuOverlay) which uses the clicked row model; change the
overlay to accept an immutable selection-level snapshot (e.g.,
selectedWorkspaces or selectionSnapshot) passed from the parent that owns the
LazyVStack/ForEach, then replace uses of overlay.tab.customColor (lines
~200–205) with a computed check over that snapshot (e.g.,
selectionSnapshot.contains { $0.customColor != nil }) so the "Clear Color" menu
item is driven by the selection state rather than the clicked row; ensure the
overlay API does not capture any ObservableObject/store and only uses value-type
snapshots and action closures.
- Line 96: In WorkspaceContextMenuOverlay replace every hardcoded NSMenu(title:
"...") with a localized string using String(localized: "key.name", defaultValue:
"English text"); e.g. change NSMenu(title: "Workspace Context Menu") to
NSMenu(title: String(localized: "workspace.context_menu.title", defaultValue:
"Workspace Context Menu")) and do the same for the other two NSMenu initializers
(use distinct keys such as "workspace.submenu1.title" and
"workspace.submenu2.title"), then add those keys and default English values to
Resources/Localizable.xcstrings and provide translations.
---
Duplicate comments:
In `@Sources/ContentView.swift`:
- Around line 10801-10805: TabItemView is still reading live
tabManager/notificationStore inside its body; finish the row-store extraction by
computing immutable snapshots (e.g. tab snapshot including id, title, unread
count, hasLatestNotification) and the precomputed flags (precomputedTabCount,
precomputedHasLatestNotifications, canMarkWorkspaceRead, canMarkWorkspaceUnread,
referenceWindowId, contextMenuWorkspaceIds) above the row and pass only those
value snapshots plus action closures into TabItemView; remove any direct
references to tabManager or notificationStore from TabItemView and keep all
imperative store calls (mark read/unread, open/close/move tab, fetch latest) in
the parent scope where you build the closures so rows and drop-gaps hold no
ObservableObject/store references.
🪄 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: 0329053d-27d6-433e-81b5-719d117f4075
📒 Files selected for processing (3)
Sources/ContentView.swiftSources/Sidebar/WorkspaceContextMenuOverlay.swiftcmux.xcodeproj/project.pbxproj
nanami-he
force-pushed
the
feature/nsmenu-sidebar-context-menu
branch
from
May 23, 2026 05:05
ce4fc47 to
f14eb47
Compare
nanami-he
commented
May 23, 2026
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@Resources/Localizable.xcstrings`:
- Line 38310: The new localization key "contextMenu.title" only has entries for
"en" and "ja"; update the Resources/Localizable.xcstrings catalog so that
"contextMenu.title" includes a localization object for every locale already
present in that catalog (add the missing locale keys with translated strings or
approved placeholder translations), ensuring each locale's "stringUnit" has
"state" and "value" populated; modify the same "contextMenu.title" entry to add
those locale entries rather than creating a partial subset so the catalog
remains complete per the localization guidelines.
In `@Sources/ContentView.swift`:
- Around line 14060-14062: The TabItemView is mutating shared state via
handleMenuAction(_:) inside the row; move the switch that performs
tabManager-backed mutations out of TabItemView's body and into the parent that
builds the LazyVStack/.equatable() rows. Replace onAction: {
handleMenuAction(action) } with a narrow action bundle (e.g., closures like
onRename(_:), onClose(id:), onMoveUp(id:)) or a single delegated closure that
only forwards an enum action to the parent; implement the actual switch and
calls to tabManager (or other Observable stores) in the parent scope and pass
only immutable snapshot data plus these small closures into TabItemView/overlay
so the row remains snapshot-only.
In `@Sources/Sidebar/WorkspaceContextMenuOverlay.swift`:
- Around line 257-260: The "Move to Top" menu item is enabled whenever
overlay.contextMenuWorkspaceIds is non-empty, which allows a no-op when the
top-most selected workspace is already first; change the enablement logic in the
block that creates moveToTopItem (and references
WorkspaceContextMenuAction.moveToTop and overlay.contextMenuWorkspaceIds) to
compute the minimum position among the selected/target workspaces (map
overlay.contextMenuWorkspaceIds to their indices in the current workspace
ordering — e.g., via the same data source used to render rows or a helper that
maps workspace ID -> index) and set moveToTopItem.isEnabled = true only if that
minimum index > 0; for single-click context use the clicked row index as the
selection’s min index so multi-select moves as a block and the menu is disabled
when the block is already at the top.
- Line 17: The enum case copySshError(error: String) is forwarding raw SSH error
text into the menu action payload — stop passing raw upstream messages; change
the action to carry either no user-facing string or an opaque identifier (e.g.,
copySshError(token: String) or copySshError) and resolve/sanitize/localize the
clipboard text in the action handler instead (use a new sanitizer like
sanitizeSSHMessage(_:) or lookup by token and produce a redacted/localized
string), and update the clipboard helper call sites to accept the
sanitized/localized text rather than the original raw error.
🪄 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: c450fa41-5cd4-47fc-815a-04d80a64953a
📒 Files selected for processing (4)
Resources/Localizable.xcstringsSources/ContentView.swiftSources/Sidebar/WorkspaceContextMenuOverlay.swiftcmux.xcodeproj/project.pbxproj
nanami-he
force-pushed
the
feature/nsmenu-sidebar-context-menu
branch
from
May 23, 2026 08:39
f14eb47 to
e3f586e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
Sources/Sidebar/WorkspaceContextMenuOverlay.swift (2)
257-260:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win“Move to Top” enablement still allows a multi-select no-op.
This remains enabled based on the clicked row index, not the selected block’s minimum index, so top-anchored multi-selection can still show an enabled no-op action.
🤖 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/Sidebar/WorkspaceContextMenuOverlay.swift` around lines 257 - 260, The "Move to Top" menu item is being enabled using the clicked row index (overlay.index) which allows a no-op for multi-selection; update the enablement logic for moveToTopItem to compute the minimum index among the selected block's workspace IDs (overlay.contextMenuWorkspaceIds -> map to their indexes) and set moveToTopItem.isEnabled = (minSelectedIndex > 0) && !overlay.contextMenuWorkspaceIds.isEmpty so the action is disabled when the topmost selected item is already at index 0; adjust the code around moveToTopItem, overlay.index, overlay.contextMenuWorkspaceIds, and WorkspaceContextMenuAction.moveToTop accordingly.
17-17:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop forwarding raw SSH error text in the action payload.
copySshError(error: String)still pushes raw upstream text into the menu contract and downstream clipboard path. Keep the action opaque (no raw message) and resolve a sanitized/localized string in the handler layer instead.♻️ Proposed fix
enum WorkspaceContextMenuAction { @@ - case copySshError(error: String) + case copySshError @@ - if let sshError = overlay.copyableSidebarSSHError { + if overlay.copyableSidebarSSHError != nil { let copySshItem = NSMenuItem(title: String(localized: "contextMenu.copySshError", defaultValue: "Copy SSH Error"), action: `#selector`(handleMenuItemAction(_:)), keyEquivalent: "") copySshItem.target = self - copySshItem.representedObject = WorkspaceContextMenuAction.copySshError(error: sshError) + copySshItem.representedObject = WorkspaceContextMenuAction.copySshError menu.addItem(copySshItem) }As per coding guidelines: “user-facing errors, alerts, command output, API error bodies, or recovery copy must not expose … raw upstream messages …”.
Also applies to: 233-236
🤖 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/Sidebar/WorkspaceContextMenuOverlay.swift` at line 17, The action case copySshError(error: String) currently forwards raw upstream SSH text; change the action to be opaque (e.g., copySshError with no associated String) and remove any code that places the raw error into the menu contract or clipboard payload; update all call sites that dispatch copySshError to dispatch the new no-payload variant; in the action handler/clipboard writer (the reducer/handler that previously received the String) resolve a sanitized/localized user-facing message there (sanitize/lookup a localized message, not the raw error) and write that sanitized text to the clipboard; repeat the same change for the other occurrences mentioned (the other copySshError usages around the referenced block).
🤖 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 10801-10808: Precompute the Finder URL(s) from
workspaceFinderDirectoryCache.url(for:) alongside the other context-menu
snapshots (e.g., add a precomputedFinderDirectoryURL or a mapping keyed by
workspace id where you already compute precomputedTabCount,
precomputedHasLatestNotifications, etc.), pass that snapshot into TabItemView as
an immutable value, and include it in TabItemView’s equality comparison (the
TabItemView == implementation) so Show in Finder enablement won’t go stale;
update TabItemView.body to use the injected precomputedFinderDirectoryURL
instead of reading workspaceFinderDirectoryCache.url(for:) directly. Ensure the
same change is applied at the other indicated spots (around the other ranges
referencing workspaceFinderDirectoryCache.url(for:)).
---
Duplicate comments:
In `@Sources/Sidebar/WorkspaceContextMenuOverlay.swift`:
- Around line 257-260: The "Move to Top" menu item is being enabled using the
clicked row index (overlay.index) which allows a no-op for multi-selection;
update the enablement logic for moveToTopItem to compute the minimum index among
the selected block's workspace IDs (overlay.contextMenuWorkspaceIds -> map to
their indexes) and set moveToTopItem.isEnabled = (minSelectedIndex > 0) &&
!overlay.contextMenuWorkspaceIds.isEmpty so the action is disabled when the
topmost selected item is already at index 0; adjust the code around
moveToTopItem, overlay.index, overlay.contextMenuWorkspaceIds, and
WorkspaceContextMenuAction.moveToTop accordingly.
- Line 17: The action case copySshError(error: String) currently forwards raw
upstream SSH text; change the action to be opaque (e.g., copySshError with no
associated String) and remove any code that places the raw error into the menu
contract or clipboard payload; update all call sites that dispatch copySshError
to dispatch the new no-payload variant; in the action handler/clipboard writer
(the reducer/handler that previously received the String) resolve a
sanitized/localized user-facing message there (sanitize/lookup a localized
message, not the raw error) and write that sanitized text to the clipboard;
repeat the same change for the other occurrences mentioned (the other
copySshError usages around the referenced block).
🪄 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: d2447680-1d2b-48fd-bf2f-fe8c90359895
📒 Files selected for processing (4)
Resources/Localizable.xcstringsSources/ContentView.swiftSources/Sidebar/WorkspaceContextMenuOverlay.swiftcmux.xcodeproj/project.pbxproj
nanami-he
force-pushed
the
feature/nsmenu-sidebar-context-menu
branch
from
May 23, 2026 14:39
e3f586e to
0cb0e64
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 13324-13331: The TabItemView Equatable implementation is missing
the new `@Binding` frozenPresentation, causing SwiftUI to reuse stale rows; update
the equality checks (the == function(s) or Equatable conformance for
TabItemView) to include a comparison of lhs.frozenPresentation ==
rhs.frozenPresentation (and likewise add frozenPresentation to any other
TabItemView equality comparison site referenced in the diff), so the row
equality reflects changes to the frozenPresentation binding and SwiftUI will
rebuild the view when it changes.
In `@Sources/Sidebar/WorkspaceContextMenuOverlay.swift`:
- Around line 36-37: WorkspaceContextMenuOverlay currently captures an
ObservableObject Tab (alias Workspace) which breaks the "rows must be
value-only" rule; change the overlay to store an immutable Bool snapshot instead
(replace stored property let tab: Tab with let isPinned: Bool) and update any
logic inside WorkspaceContextMenuOverlay that reads overlay.tab.isPinned to use
isPinned; also update the call site(s) that construct
WorkspaceContextMenuOverlay to pass isPinned: tab.isPinned (not the Tab object)
and remove any remaining references to Tab within the overlay.
🪄 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: 38386449-a4ca-458b-ac1f-c6f6a56c1287
📒 Files selected for processing (4)
Resources/Localizable.xcstringsSources/ContentView.swiftSources/Sidebar/WorkspaceContextMenuOverlay.swiftcmux.xcodeproj/project.pbxproj
nanami-he
force-pushed
the
feature/nsmenu-sidebar-context-menu
branch
from
May 23, 2026 14:57
0cb0e64 to
0e5810c
Compare
Author
|
@lawrencecchen Could you take a look when you have a chance? This fixes the sidebar second-level context menu flicker by moving the menu out of SwiftUI Local verification:
The only remaining red checks appear to be Vercel team authorization. |
Author
|
@lawrencecchen gentle follow-up here. I want to flag that #2003 is still a user-visible bug: the second-level sidebar context menus, especially Workspace Color, can dismiss before the cursor reaches an item while terminal output is active. This PR moves the menu out of SwiftUI Current status from my side: CodeRabbit/Greptile are green on the latest head, local verification passed, and the remaining red checks appear to be Vercel team authorization. |
…w-ai#2003) Move the workspace context menu off the SwiftUI render tree to prevent submenu flickering and disappearing when the sidebar body is invalidated at a high frequency. Align with latest main: added workspace grouping and copy link support.
nanami-he
force-pushed
the
feature/nsmenu-sidebar-context-menu
branch
from
June 3, 2026 08:19
b6db8a3 to
863fad9
Compare
Author
|
Force-pushed an updated version of this PR on top of the latest This is still the fix for the workspace color submenu dismissal bug tracked in #2003 and #4646 (regression of #2560), but now aligned with current Verified locally: the workspace color submenu no longer flickers or dismisses before the cursor reaches a swatch while terminal output is active. |
Author
|
@austinywang gentle follow-up here. This PR has been force-updated on top of the latest From the current merge box, it looks blocked by maintainer-side items rather than new code changes:
If there are still concerns with the updated |
teamleaderleo
added
bug
This branch has not been deployed
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
2 participants
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.
Summary
Replaces SwiftUI
.contextMenu/ nestedMenufor the sidebar workspace right-click menu with an AppKitNSMenuhosted by a transparentNSViewRepresentableoverlay. Closes #2003.Why the previous patches didn't fully fix it
Earlier fixes (
9d4220b17,cdcb16a34,b0e6b35fd) each shaved off a specific invalidation source but couldn't address the architecture: SwiftUI tears down any expandedMenuwhenever the host view body re-evaluates. The sidebar body re-evaluates at 1–4 Hz under normal terminal output, so the color submenu — which has the most items and the longest hover distance — gets dismissed before the cursor reaches a color.Detailed source analysis with SwiftUI's
_printChanges()+ a publish-frequency watcher identified 5 invalidation sources in the ContentView body:titlebarText: @Statewrites (≈1 Hz).onReceive(.ghosttyDidSetTitle)— emit invalidates host body even with an empty closure (undocumented Apple behavior;NotificationCenter.addObserverdirectly avoids it)fileExplorerStore: @StateObjectinternal@Published(≈1.5 Hz)4–5.
notificationStore: @EnvironmentObjectre-renders on every readPatching all of those still leaves a ≈1 Hz floor from
\PaneState.tabs(@Observablemacro). The repository's existingSnapshot boundary for list subtreesrule (CLAUDE.md, motivated by #2586) mitigates row churn but not parent body churn — and parent body churn is what kills the SwiftUI submenu. The fix is to move the menu off the SwiftUI render tree entirely.Approach
New file
Sources/Sidebar/WorkspaceContextMenuOverlay.swiftWorkspaceContextMenuOverlay: NSViewRepresentable(@MainActor)MenuHostView: NSViewwithhitTest(_:)that claims the event only on.rightMouseDownorcontrol + .leftMouseDown. All other events (drag, left-click, hover) pass through to the SwiftUI hierarchy underneath, so drag-to-reorder, selection, and hover highlight are untouched.Coordinator: NSObject, NSMenuDelegate(@MainActor) builds theNSMenuper right-click from a Workspace snapshot. The@MainActorisolation lets the delegate readAppDelegate.sharedand workspace properties directly without cross-actor hops.WorkspaceContextMenuActionenum covers full parity with the old SwiftUI menu: pin/unpin, rename, remove custom name, edit/clear description, reconnect/disconnect SSH, scrollbar toggle, color palette (16 entries) + custom color + clear, move-to-window targets pulled dynamically viawindowMoveTargets(referenceWindowId:), close current/others/above/below, mark read/unread, clear latest notification, copy UUID, show in Finder.coloredCircleImage(color:)(NSImage) sinceNSMenuItemdoesn't accept SwiftUI views.Sources/ContentView.swift— replaces the.contextMenu { ... Menu("Workspace Color") { ... } ... }chain with.overlay(WorkspaceContextMenuOverlay(...)). Wires menu open/close into the existingrowInteractionState.contextMenuDidAppear()/contextMenuDidDisappear()so the sidebar's anti-jitter pause logic still triggers.handleMenuAction(_:)dispatches the action enum to the existing functions previously called by the SwiftUI menu items.cmux.xcodeproj/project.pbxproj— registers the new file inPBXBuildFile,PBXFileReference,PBXGroup, andPBXSourcesBuildPhase.Testing
./scripts/reload.sh --tag nsmenu-sidebar-context-menu— Debug build completes cleanly (reload succeeded in 16s), no concurrency warnings.hitTestpass-through verified).Not added: an automated regression test. Driving NSMenu interaction under high-rate publish in XCTest would need a non-trivial UI harness. Happy to add one if a preferred test seam exists — guidance from maintainers would help.
Closes #2003.
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Summary by cubic
Rewrites the sidebar workspace context menu to an AppKit
NSMenuvia a transparentNSViewRepresentableoverlay so submenus don’t close during SwiftUI re-renders. Adds workspace grouping and “Copy Workspace Link” actions; shortcuts are preserved, items use precomputed state, and drag/selection/hover/scroll behavior is unchanged.Bug Fixes
Refactors
WorkspaceContextMenuOverlayand aWorkspaceContextMenuActionenum; builds anNSMenuper click and only captures right-/control-clicks. Enum now includes workspace grouping (new group, move to group, remove from group) and “Copy Workspace Link”..contextMenuwith.overlay(...), routing actions throughhandleMenuAction(_);menuWillOpen/DidClosedrive the existing hooks.cmux.xcodeproj.Written for commit 863fad9. Summary will update on new commits.
Summary by CodeRabbit
New Features
Performance
Bug Fixes
Localization