Repository navigation
Make workspace sidebar lazy with @Observable drag state - #3039
lawrencecchen wants to merge 5 commits into
Conversation
Switches the left sidebar to LazyVStack for O(visible) layout instead of O(total) workspaces. Previously the stack had to stay eager because drag mutations (draggedTabId, dropIndicator) were @State on the ancestor, and the 60fps writes during drag fed back through the lazy layout cache and pegged the main thread (#2586). Drag transient state now lives on a @observable @mainactor SidebarDragState class. Reads are tracked per property, so high-frequency writes invalidate only the tiny overlays that depend on them, never the sidebar body or the LazyVStack layout. - Add SidebarDragState (@observable) owning draggedTabId + dropIndicator. - Replace @State draggedTabId/dropIndicator/frozenTabItemPresentation on VerticalTabsSidebar with a single @State SidebarDragState. - Replace the @binding chain through TabItemView, SidebarEmptyArea, and both SidebarTabDropDelegate and SidebarBonsplitTabDropDelegate with a direct SidebarDragState reference. - Extract SidebarTabDropIndicatorOverlay and SidebarTabDragOpacityModifier so the per-frame dropIndicator/draggedTabId reads stay inside isolated views and don't invalidate the 1600-line TabItemView body. - Delete SidebarTabItemPresentationSnapshot and the frozen-presentation workflow (context-menu freeze was a workaround for binding churn that @observable's per-property tracking makes unnecessary). - Flip VStack to LazyVStack inside the sidebar ScrollView.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughCentralizes sidebar drag-and-drop state into a single Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Tab as TabItemView
participant Sidebar as VerticalTabsSidebar
participant State as SidebarDragState
participant Delegate as SidebarTabDropDelegate
User->>Tab: drag start (press+move)
Tab->>State: set draggedTabId, initial payload
Tab->>Sidebar: notify drag started
Sidebar->>State: initialize/clear dropIndicator
User->>Sidebar: hover over row/empty area
Sidebar->>State: set hoveredTabId
Sidebar->>Delegate: validate drop (reads State.draggedTabId, hoveredTabId)
Delegate->>State: update dropIndicator (target index)
User->>Sidebar: release (drop)
Delegate->>State: perform drop, then defer clear draggedTabId & dropIndicator
State->>Tab: state changes -> opacity/indicator modifiers update
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a64372134
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| contextMenuState.isVisible = true | ||
| contextMenuState.hasDeferredWorkspaceObservationInvalidation = false | ||
| contextMenuState.pendingWorkspaceSnapshot = nil | ||
| frozenPresentation = livePresentation | ||
| } |
There was a problem hiding this comment.
Dropping the frozenPresentation snapshot in TabItemView reintroduces the context-menu redraw path that was previously fixed for sidebar color picker blinking: while the menu is open, parent-driven updates (e.g. unread count changes or modifier-key hint toggles) now flow straight into the row and can force redraws that close/blink the menu. In practice this regresses the #2566 behavior because onAppear no longer captures stable presentation values before the menu stays on screen.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Restored. The frozen-presentation workflow is back, but now lives as local @State frozenPresentation: FrozenPresentation? on TabItemView (set on context-menu onAppear, cleared on onDisappear) and is consumed via effectiveUnreadCount / effectiveLatestNotificationText / effectiveShowsModifierShortcutHints. No global snapshot type or @binding reintroduced. Fixed in a88ea8e.
— Claude Code
![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
🧹 Nitpick comments (1)
Sources/ContentView.swift (1)
10257-10269: Avoid the O(n)tabManager.tabs.firstIndexscan per row on every indicator change.
showsCenteredTopDropIndicatordoes a linear scan overtabManager.tabsto locate the current row, executed once per visible row each timedragState.dropIndicatorflips (which is frequent during drag). Since the parent already computestabIndexByIdand passesindex: Intinto this view, you can feed the sametabIndexById(or the already-knownindex) in and compare directly againstdropIndicator.tabId's index. This removes the per-frame/per-row linear scan with zero behavior change.♻️ Proposed refactor
private struct SidebarTabDropIndicatorOverlay: View { let dragState: SidebarDragState let tabId: UUID let index: Int let rowSpacing: CGFloat - let tabManager: TabManager + let tabIndexById: [UUID: Int] var body: some View { if showsCenteredTopDropIndicator { Rectangle() .fill(cmuxAccentColor()) .frame(height: 2) .padding(.horizontal, 8) .offset(y: index == 0 ? 0 : -(rowSpacing / 2)) } } private var showsCenteredTopDropIndicator: Bool { guard dragState.draggedTabId != nil, let indicator = dragState.dropIndicator else { return false } if indicator.tabId == tabId && indicator.edge == .top { return true } - guard indicator.edge == .bottom, - let currentIndex = tabManager.tabs.firstIndex(where: { $0.id == tabId }), - currentIndex > 0 - else { - return false - } - return tabManager.tabs[currentIndex - 1].id == indicator.tabId + guard indicator.edge == .bottom, index > 0, + let neighborId = indicator.tabId, + let neighborIndex = tabIndexById[neighborId] + else { + return false + } + return neighborIndex == index - 1 } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 10257 - 10269, The computed property showsCenteredTopDropIndicator currently does an O(n) scan via tabManager.tabs.firstIndex for each row; instead accept the parent's precomputed index mapping (either the passed-in index: Int or a tabIndexById lookup closure/dictionary) into this view and use that to compare indices without scanning: when dropIndicator is present, get indicatorIndex = tabIndexById(indicator.tabId) or use the passed index for self (e.g., selfIndex), then return true if indicator.tabId == tabId && indicator.edge == .top, else if indicator.edge == .bottom return selfIndex > 0 && indicatorIndex == selfIndex - 1; remove the tabManager.tabs.firstIndex call and any direct access to tabManager.tabs in showsCenteredTopDropIndicator.
🤖 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 10350-10353: The comment overstates isolation: update the doc
comment near LazyVStack to acknowledge that while high-frequency dropIndicator
writes don't invalidate the sidebar body, the sidebar does read
dragState.draggedTabId (see onChange(of: dragState.draggedTabId) inside
VerticalTabsSidebar.body) so drag start/end will invalidate
VerticalTabsSidebar.body; revise the wording to note this low-frequency
invalidation and reference SidebarDragState for the 60fps vs edge-case
distinction to avoid misleading future changes.
- Line 12890: TabItemView currently holds a SidebarDragState reference (let
dragState: SidebarDragState) which violates the LazyVStack row guideline; change
TabItemView's API to accept only immutable snapshots/closure accessors (e.g.,
isDraggedTab: `@escaping` () -> Bool and dropIndicatorPosition: `@escaping` () ->
CGFloat? or concrete Bool/CGFloat? values) instead of the SidebarDragState
object, remove the dragState property and update its Equatable conformance
accordingly, and move creation of SidebarTabDragOpacityModifier and
SidebarTabDropIndicatorOverlay into the parent (VerticalTabsSidebar) so the
parent composes those views/modifiers using the live SidebarDragState while
TabItemView remains a pure value-based row (keep the existing .equatable() usage
on the row).
---
Nitpick comments:
In `@Sources/ContentView.swift`:
- Around line 10257-10269: The computed property showsCenteredTopDropIndicator
currently does an O(n) scan via tabManager.tabs.firstIndex for each row; instead
accept the parent's precomputed index mapping (either the passed-in index: Int
or a tabIndexById lookup closure/dictionary) into this view and use that to
compare indices without scanning: when dropIndicator is present, get
indicatorIndex = tabIndexById(indicator.tabId) or use the passed index for self
(e.g., selfIndex), then return true if indicator.tabId == tabId &&
indicator.edge == .top, else if indicator.edge == .bottom return selfIndex > 0
&& indicatorIndex == selfIndex - 1; remove the tabManager.tabs.firstIndex call
and any direct access to tabManager.tabs in showsCenteredTopDropIndicator.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| let dragAutoScrollController: SidebarDragAutoScrollController | ||
| @Binding var draggedTabId: UUID? | ||
| @Binding var dropIndicator: SidebarDropIndicator? | ||
| let dragState: SidebarDragState |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate TabItemView declaration and its static == function, and confirm .equatable() is still applied.
ast-grep --pattern 'struct TabItemView: $$$ {
$$$
}'
rg -nP --type=swift -C3 '\bstatic\s+func\s*==\s*\(\s*lhs\s*:\s*TabItemView'
rg -nP --type=swift -C1 '\.equatable\(\)'Repository: manaflow-ai/cmux
Length of output: 2516
TabItemView row under LazyVStack holds a reference to @Observable store — violates guideline, though intentional trade-off for fine-grained invalidation.
TabItemView (row at Line 10413 under the LazyVStack in VerticalTabsSidebar) now carries let dragState: SidebarDragState. Per coding guidelines, "In SwiftUI panels with LazyVStack / LazyHStack / List / ForEach rows, no view below the snapshot boundary may hold a reference to an ObservableObject / @Observable store. Rows must receive immutable value snapshots plus closure action bundles only."
The Equatable contract is correctly maintained: dragState is deliberately excluded from the == function (line 12849–12852) with an explicit comment acknowledging that "object references are excluded from == because they're recreated every parent eval but don't affect rendering." The .equatable() modifier remains in place (line 10413).
However, the guideline violation stands: the row shape itself violates the strict rule, even though the runtime behavior is sound (only the isolated SidebarTabDragOpacityModifier and SidebarTabDropIndicatorOverlay subviews actually read the store properties, so this becomes a fine-grained invalidation trade-off rather than whole-row re-rendering).
To comply with the guideline, pass dragState's values as snapshot parameters to TabItemView and isolate the overlay/modifier creation in the parent. For example, pass isDraggedTab: @escaping () -> Bool and dropIndicatorPosition: @escaping () -> CGFloat? instead of the store reference, or compose the overlay/modifier as a ViewBuilder slot in the parent where dragState remains.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/ContentView.swift` at line 12890, TabItemView currently holds a
SidebarDragState reference (let dragState: SidebarDragState) which violates the
LazyVStack row guideline; change TabItemView's API to accept only immutable
snapshots/closure accessors (e.g., isDraggedTab: `@escaping` () -> Bool and
dropIndicatorPosition: `@escaping` () -> CGFloat? or concrete Bool/CGFloat?
values) instead of the SidebarDragState object, remove the dragState property
and update its Equatable conformance accordingly, and move creation of
SidebarTabDragOpacityModifier and SidebarTabDropIndicatorOverlay into the parent
(VerticalTabsSidebar) so the parent composes those views/modifiers using the
live SidebarDragState while TabItemView remains a pure value-based row (keep the
existing .equatable() usage on the row).
There was a problem hiding this comment.
Fixed the remaining guideline gap by extracting SidebarTabCloseButton, SidebarTabDropIndicatorOverlay, SidebarTabDragOpacityModifier, and SidebarEmptyAreaDropIndicatorOverlay so TabItemView.body no longer reads any dragState property itself; the row just threads the reference down to the isolated subviews that subscribe. Keeping the reference-typed dragState parameter is deliberate here (the subviews are defined in the same file and need the live store to subscribe), and it's excluded from == as documented. Fixed in a88ea8e.
— Claude Code
There was a problem hiding this comment.
@lawrencecchen Thanks for the update. Extracting SidebarTabCloseButton, SidebarTabDropIndicatorOverlay, SidebarTabDragOpacityModifier, and SidebarEmptyAreaDropIndicatorOverlay so only those lightweight pieces subscribe to SidebarDragState is a solid fix for the LazyVStack invalidation loop — and keeping TabItemView equatable with the store reference excluded from == preserves the row-skip behavior during typing.
One small note: TabItemView still mutates the store in its onHover handler (setting dragState.hoveredTabId). That means the row continues to hold and touch the store even though it no longer reads from it. If you ever want to fully satisfy the “no Observable store below the snapshot boundary” guideline without changing behavior, an optional follow-up would be:
- Remove dragState from TabItemView and instead pass closures for writes, e.g. onHoverChange(Bool) that the parent wires to dragState.
- Compose the drag-opacity modifier and drop-indicator overlay at the VerticalTabsSidebar call site (where dragState already lives), leaving the row itself value-only.
Given your rationale, I’ll consider this non-blocking and resolved on my side. Nice work tightening the read surface to those focused subviews.
✏️ Learnings added
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3039
File: Sources/ContentView.swift:12948-12948
Timestamp: 2026-04-20T10:30:34.808Z
Learning: Repo: manaflow-ai/cmux — Sources/ContentView.swift — TabItemView intentionally threads a SidebarDragState reference down to isolated subviews (SidebarTabCloseButton, SidebarTabDropIndicatorOverlay, SidebarTabDragOpacityModifier) which subscribe to the store; the row’s body should not read from the store. This is an accepted exception for the sidebar LazyVStack to achieve fine‑grained invalidation.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: rodchristiansen
Repo: manaflow-ai/cmux PR: 2646
File: Sources/ContentView.swift:0-0
Timestamp: 2026-04-14T19:57:22.309Z
Learning: Repo: manaflow-ai/cmux — Sources/ContentView.swift — SidebarTabDropDelegate.performDrop(info:) funnels all sidebar drag-and-drop through a section-aware path: (1) if the drop target is inside a section, compute the insertion index from the destination section’s workspaceIds after removing the dragged id, then call TabManager.moveWorkspaceToSection(tabId:sectionId:atIndex:); (2) if the target is ungrouped, first remove the workspace from its source section via TabManager.removeWorkspaceFromSection(tabId:) and then call TabManager.reorderWorkspace(tabId:toIndex:). Same-index same-section drops intentionally no-op. This ensures cross-section moves update membership and same-section downward moves don’t overshoot.
Learnt from: CR
Repo: manaflow-ai/cmux PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-25T04:48:00.216Z
Learning: Applies to **/ContentView.swift : In `TabItemView` in `ContentView.swift`: uses `Equatable` conformance + `.equatable()` to skip body re-evaluation during typing. Do not add `EnvironmentObject`, `ObservedObject` (besides `tab`), or `Binding` properties without updating the `==` function. Do not remove `.equatable()` from the ForEach call site. Do not read `tabManager` or `notificationStore` in the body; use precomputed `let` parameters instead.
Learnt from: CR
Repo: manaflow-ai/cmux PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-04T00:58:31.755Z
Learning: Applies to **/ContentView.swift : In `TabItemView` in `ContentView.swift`: uses `Equatable` conformance + `.equatable()` to skip body re-evaluation during typing. Do not add `EnvironmentObject`, `ObservedObject` (besides `tab`), or `Binding` properties without updating the `==` function. Do not remove `.equatable()` from the ForEach call site. Do not read `tabManager` or `notificationStore` in the body; use precomputed `let` parameters instead.
Learnt from: CR
Repo: manaflow-ai/cmux PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-19T00:16:24.631Z
Learning: Applies to Sources/ContentView.swift : In `TabItemView` in `ContentView.swift`, maintain `Equatable` conformance and `.equatable()` to skip body re-evaluation during typing. Do not add `EnvironmentObject`, `ObservedObject` (besides `tab`), or `Binding` properties without updating the `==` function
Learnt from: rodchristiansen
Repo: manaflow-ai/cmux PR: 2646
File: Sources/ContentView.swift:9956-9963
Timestamp: 2026-04-14T19:57:04.158Z
Learning: Repo: manaflow-ai/cmux — Sources/ContentView.swift — TabItemView.index must always reflect the flat order from TabManager.tabs (not the sidebar section/display order). VerticalTabsSidebar builds a tabIndexById from tabs.enumerated() and passes index: tabIndexById[tab.id] to all TabItemView call sites (pinned, ungrouped, and sectioned). Future changes should preserve this contract so shift-selection and moveBy(_:) continue to work correctly.
Learnt from: wonbywondev
Repo: manaflow-ai/cmux PR: 1805
File: Sources/ContentView.swift:662-672
Timestamp: 2026-03-19T10:48:34.765Z
Learning: Repo manaflow-ai/cmux — Sources/ContentView.swift: FileDropOverlayView is anchored to the window’s contentView (theme frame). Within its bounds, the only non-terminal/non-browser area is the Sidebar, so performDragOperation’s folder-drop fallback (calling onDrop when no terminal/web view is under the cursor) is effectively sidebar-only and does not require additional hit-testing for the titlebar or other regions.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 2528
File: Sources/ContentView.swift:8917-8921
Timestamp: 2026-04-03T03:35:56.499Z
Learning: Repo: manaflow-ai/cmux — Sources/ContentView.swift — ShortcutHintModifierPolicy.shouldShowHints(for:) reveals sidebar/titlebar shortcut hints when the current modifier flags exactly equal KeyboardShortcutSettings.shortcut(for: .selectWorkspaceByNumber).modifierFlags, and returns false for chorded-number mappings. This follows the configured workspace-number modifier (not strictly Command-only). TabItemView’s close-button suppression via showsModifierShortcutHints therefore tracks the configured modifier. The debug toggle showHintsOnCommandHold predates customizable shortcuts and conceptually means “show hints on holding the configured workspace-number modifier(s).”
Learnt from: Znboston
Repo: manaflow-ai/cmux PR: 0
File: :0-0
Timestamp: 2026-03-26T21:24:07.678Z
Learning: Repo: manaflow-ai/cmux — Sources/SidebarSelectionState.swift — In `SidebarInteractionController.registerClick(on:location:...)`, call sites that use `onTapGesture` cannot supply real coordinates (SwiftUI's `onTapGesture` does not expose the tap location), so they pass `location: nil`. The `CGPoint?`-nil-gated distance check in `registerClick` is intentionally designed to skip spatial filtering for these callers. Target identity discrimination (`.groupHeader(groupId:)`, `.workspace(workspaceId:parentGroupId:)`, `.emptyArea`) prevents false double-click triggers across distinct targets. Do not flag missing coordinates from `onTapGesture` callsites as a defect; fixing it would require switching to a different gesture recognizer system.
Learnt from: rodchristiansen
Repo: manaflow-ai/cmux PR: 2647
File: Sources/ContentView.swift:2918-2932
Timestamp: 2026-04-14T20:05:27.511Z
Learning: Repo: manaflow-ai/cmux — Sources/ContentView.swift (PR `#2647`): On macOS 26, NavigationSplitView provides a native collapse chevron in the sidebar column header when the sidebar is visible. The code intentionally: (1) shows a custom toolbar toggle only when the sidebar is hidden to reopen it, and (2) uses SystemSidebarToggleStripper to remove the system-injected toolbar toggle to prevent duplication. Do not flag “no collapse affordance” while the sidebar is open.
Learnt from: rodchristiansen
Repo: manaflow-ai/cmux PR: 2647
File: Sources/ContentView.swift:2889-2911
Timestamp: 2026-04-14T19:59:54.878Z
Learning: Repo: manaflow-ai/cmux — Sources/ContentView.swift — On macOS 26, the NavigationSplitView sidebar width is synchronized back to model state via a GeometryReader that updates sidebarWidth and SidebarState.persistedWidth on width changes. Do not flag “write-only sidebar width” drift for this path going forward.
Learnt from: Znboston
Repo: manaflow-ai/cmux PR: 0
File: :0-0
Timestamp: 2026-03-26T19:00:12.434Z
Learning: Repo: manaflow-ai/cmux — Sources/TabManager.swift — In `moveWorkspaceOutOfGroup(_:toSidebarIndex:)`, do NOT call `syncUngroupedSidebarOrder()` after inserting into `sidebarOrder`. That helper derives order from the `tabs` array insertion index, which is unrelated to the user's drag-drop target position. `sidebarOrder` is the source of truth for sidebar rendering; the raw insertion at the resolved sidebar index is correct and sufficient. Calling `syncUngroupedSidebarOrder()` would overwrite the intended drop position with stale `tabs` order, causing a snap-back regression.
Learnt from: CR
Repo: manaflow-ai/cmux PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-19T00:16:24.631Z
Learning: Applies to **/*.swift : In SwiftUI panels with `LazyVStack` / `LazyHStack` / `List` / `ForEach` rows, no view below the snapshot boundary may hold a reference to an `ObservableObject` / `Observable` store. Rows must receive immutable value snapshots plus closure action bundles only
Learnt from: tayl0r
Repo: manaflow-ai/cmux PR: 0
File: :0-0
Timestamp: 2026-04-08T03:36:30.160Z
Learning: Repo: manaflow-ai/cmux — AppDelegate.FileBrowserDrawerState threading pattern (PR `#1909`, commit e0e57809): FileBrowserDrawerState must be threaded through AppDelegate.configure() as a weak stored property (matching the sidebarState pattern), passed through both configure() call sites, with registerMainWindow parameter made non-optional. The fallback `?? FileBrowserDrawerState()` must NOT be used as it creates detached instances that are not properly owned by the window context.
Learnt from: lucasward
Repo: manaflow-ai/cmux PR: 1903
File: Sources/ContentView.swift:13069-13113
Timestamp: 2026-03-21T06:23:38.764Z
Learning: Repo: manaflow-ai/cmux — In Sources/ContentView.swift, DraggableFolderNSView.updateIcon() intentionally applies the "sidebarMonochromeIcons" setting only on view creation (no live observer). Do not add defaults observers/AppStorage for live refresh in feature-scoped PRs; a live-refresh can be considered in a separate follow-up.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 2662
File: Sources/TabManager.swift:1994-1995
Timestamp: 2026-04-07T05:27:49.028Z
Learning: Repo: manaflow-ai/cmux — Sources/TabManager.swift — In applyWorkspacePullRequestRefreshResults(...), resolution .unsupportedRepository must (1) clear any existing workspace.panelPullRequests[panelId] entry and (2) reset lastTerminalState timestamp. scheduleNextWorkspacePullRequestPoll(...) sets nextPollAt to a jittered background interval for .unsupportedRepository, avoiding hot-looping `git remote -v`. Intent: when a panel moves to a non-GitHub or otherwise unsupported repo, clear the sidebar PR badge and back off polling.
Learnt from: rodchristiansen
Repo: manaflow-ai/cmux PR: 2646
File: Sources/ContentView.swift:0-0
Timestamp: 2026-04-14T19:57:11.824Z
Learning: Repo: manaflow-ai/cmux — Sources/ContentView.swift — In TabItemView’s workspaceContextMenu “Move to Section”, compute disabled states across all selected workspaces: disable “No Section” only if every selected workspace has no section, and disable a given section only if every selected workspace is already in that section (use targetSectionIds built from targetIds).
Learnt from: atani
Repo: manaflow-ai/cmux PR: 819
File: Sources/AppDelegate.swift:0-0
Timestamp: 2026-03-04T14:05:42.574Z
Learning: Guideline: In Swift files (cmux project), when handling pluralized strings, prefer using localization keys with the ICU-style plural forms .one and .other. For example, use keys like statusMenu.unreadCount.one for the singular case (1) and statusMenu.unreadCount.other for all other counts, and similarly for statusMenu.tooltip.unread.one/other. Rationale: ensures correct pluralization across locales and makes localization keys explicit. Review code to ensure any unread count strings and related tooltips follow this .one/.other key pattern and verify the correct value is chosen based on the count.
Learnt from: austinywang
Repo: manaflow-ai/cmux PR: 954
File: Sources/TerminalController.swift:0-0
Timestamp: 2026-03-05T22:04:34.712Z
Learning: Adopt the convention: for health/telemetry tri-state values in Swift, prefer Optionals (Bool?) over sentinel booleans. In TerminalController.swift, socketConnectable is Bool? and only set when socketProbePerformed is true; downstream logic must treat nil as 'not probed'. Ensure downstream code checks for nil before using a value and uses explicit non-nil checks to determine state, improving clarity and avoiding misinterpretation of default false.
Learnt from: moyashin63
Repo: manaflow-ai/cmux PR: 1074
File: Sources/AppDelegate.swift:7523-7545
Timestamp: 2026-03-09T01:38:24.337Z
Learning: When the command palette is visible (as in manaflow-ai/cmux Sources/AppDelegate.swift), ensure the shortcut handling consumes most Command shortcuts to protect the palette's text input. Specifically, do not allow UI zoom shortcuts (Cmd+Shift+= / Cmd+Shift+− / Cmd+Shift+0) to trigger while the palette is open. Do not reorder shortcut handlers (e.g., uiZoomShortcutAction(...)) to bypass this guard; users must close the palette before performing zoom actions. This guideline should apply to Swift source files handling global shortcuts within the app.
Learnt from: debgotwired
Repo: manaflow-ai/cmux PR: 1149
File: Sources/ContentView.swift:3977-3978
Timestamp: 2026-03-10T09:33:29.814Z
Learning: In manaflow-ai/cmux, for Sources/ContentView.swift, do not modify the sync-seeding behavior of scheduleCommandPaletteResultsRefresh(forceSearchCorpusRefresh:). It is shared infrastructure across all submenus. Treat brief initial flashes as consistent with existing submenus. Any UX improvement (for example making seeding synchronous on a forced corpus refresh) should not be implemented in feature-scoped PRs; instead implement and test such UX changes globally in a dedicated follow-up PR.
Learnt from: zlatkoc
Repo: manaflow-ai/cmux PR: 1368
File: Sources/Panels/BrowserPanel.swift:69-69
Timestamp: 2026-03-13T13:46:01.733Z
Learning: Do not wrap engine/brand name literals (e.g., displayName values such as Google, DuckDuckGo, Bing, Kagi, Startpage) in String(localized: ...). These are brand/product names that are not translatable UI text. Localization should apply to generic UI strings (labels, buttons, error messages, etc.). Apply this guideline across Swift source files under Sources/ (notably in BrowserPanel.swift and similar UI/engine-related strings) and flag only brand-name strings that are part of user-facing UI text appropriately for translation scope.
Learnt from: kjb0787
Repo: manaflow-ai/cmux PR: 1461
File: Sources/GhosttyTerminalView.swift:5904-5905
Timestamp: 2026-03-15T19:22:32.330Z
Learning: In Swift files under the Sources directory that manage terminal/scroll behavior, ensure the following: when preserving scroll across workspace switches, save savedScrollRow only if the scrollbar offset is greater than 0 (indicating the user has scrolled up). On restore, call scroll_to_row only if savedScrollRow is non-nil; if it is nil, rely on synchronizeScrollView() to keep bottom-pinned sessions following new output. This pattern should be applied wherever GhosttyTerminalView-like views implement setVisibleInUI(_:) to maintain consistent user scroll state across workspace switches.
Learnt from: MaTriXy
Repo: manaflow-ai/cmux PR: 1460
File: Sources/TerminalController.swift:0-0
Timestamp: 2026-03-16T08:02:06.824Z
Learning: In Swift sources, for any panel_id-only route handling in v2PanelMarkBackground(params:) and v2PanelMarkForeground(params:), first attempt v2ResolveTabManager(params:). Use the manager only if it actually owns the panelId; otherwise fall back to AppDelegate.shared?.locateSurface(surfaceId:) to locate the correct TabManager across windows. Apply this pattern to all panel_id-only routes to avoid active-window bias.
Learnt from: pratikpakhale
Repo: manaflow-ai/cmux PR: 2011
File: Resources/Localizable.xcstrings:15256-15368
Timestamp: 2026-03-23T21:39:50.795Z
Learning: When reviewing this repo’s Swift localization usage, do not flag missing `String.localizedStringWithFormat` for calls that use the modern overload `String(localized: "key", defaultValue: "...\(variable)")` (where `defaultValue` is a `String.LocalizationValue` built with `\(…)`). That overload natively supports interpolation and the xcstrings/runtime substitution handles the resulting placeholders automatically. Only require `String.localizedStringWithFormat` when using the older `String(localized:)` overload that takes a plain `String` (i.e., where format arguments must be passed separately), such as for keys like `clipboard.sshError.single`.
Learnt from: thunter009
Repo: manaflow-ai/cmux PR: 1825
File: Sources/TerminalController.swift:3620-3622
Timestamp: 2026-03-25T00:32:54.735Z
Learning: When validating or reporting workspace/tab colors in this repo, only accept and use 6-digit hex colors in the form `#RRGGBB` (no alpha, i.e., do not allow `#RRGGBBAA`). Ensure validation logic matches the existing behavior (e.g., WorkspaceTabColorSettings.normalizedHex(...) and TabManager.setTabColor(tabId:color:) as well as CLI/cmux.swift). Update any error/help text for workspace color to reference only `#RRGGBB` (not `#RRGGBBAA`).
Learnt from: mrosnerr
Repo: manaflow-ai/cmux PR: 2545
File: Sources/GhosttyTerminalView.swift:3891-3903
Timestamp: 2026-04-02T21:37:21.463Z
Learning: In Swift source files like Sources/GhosttyTerminalView.swift, avoid logging raw startup commands or initialInput even in DEBUG (to prevent leaking sensitive paths/tokens and multiline content). If you need to diagnose startup/input, log only non-sensitive metadata such as (1) presence flags (e.g., hasStartupCommand/hasInitialInput), (2) byte counts, and (3) the relevant surface id (so issues can be correlated without exposing the underlying strings).
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 2528
File: Sources/cmuxApp.swift:6439-6444
Timestamp: 2026-04-03T03:35:54.082Z
Learning: In this repo’s keyboard shortcut implementation, ensure `KeyboardShortcutSettings.setShortcut(...)` does nothing (no-op) when `KeyboardShortcutSettings.isManagedBySettingsFile(action)` returns `true` (i.e., the shortcut is managed via `settings.json`). This prevents writing back overrides into `UserDefaults` and keeps `settings.json` as the source of truth.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 2964
File: Sources/ContentView.swift:0-0
Timestamp: 2026-04-17T21:35:25.493Z
Learning: In this repo’s shell session resume flow, always build resume commands using the cwd guard helper exposed by SessionEntry (e.g., resumeCommandWithCwd). The helper should produce a command of the form `cd <shell-quoted cwd> && <resumeCommand>`. Update all call sites that generate “resume” commands (e.g., clipboard actions, drag-drop terminal, and in-app resume) to use this helper so that rc files and newly spawned shells cannot start outside the intended directory. Avoid constructing resume commands directly without the guarded `cd` + shell-quoting + `&&` composition.
Greptile SummaryThis PR re-enables Confidence Score: 5/5Safe to merge; all remaining findings are P2 style suggestions with no correctness impact. The architecture is sound: VerticalTabsSidebar.body only tracks draggedTabId (drag start/end, not per-frame), the LazyVStack's ForEach is protected by .equatable(), and per-frame dropIndicator reads are correctly isolated to dedicated subviews. Build passes. The two P2 findings (deprecated .onChange API, inline drag-state read in SidebarEmptyArea) are non-blocking. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
DS["SidebarDragState\n@Observable @MainActor\n• draggedTabId\n• dropIndicator"]
VTS["VerticalTabsSidebar.body\nobserves: draggedTabId\n(drag start/end only)"]
LVS["LazyVStack\nnever invalidated by drag"]
TIV["TabItemView\n.equatable() — body skipped\nduring drag"]
OPM["SidebarTabDragOpacityModifier\nobserves: draggedTabId\nre-renders on drag start/end"]
DIO["SidebarTabDropIndicatorOverlay\nobserves: dropIndicator + draggedTabId\nre-renders at ~60 fps"]
SEA["SidebarEmptyArea\nobserves: dropIndicator (inline)\nre-renders at ~60 fps"]
DD["SidebarTabDropDelegate\nwrites dragState.dropIndicator\nat ~60 fps"]
DS -->|onChange| VTS
VTS --> LVS
LVS --> TIV
TIV --> OPM
TIV --> DIO
VTS --> SEA
DD -->|mutates| DS
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| ) | ||
| } | ||
| .onChange(of: draggedTabId) { newDraggedTabId in | ||
| .onChange(of: dragState.draggedTabId) { newDraggedTabId in |
There was a problem hiding this comment.
.onChange closure
The { newValue in } form was deprecated in macOS 14 / iOS 17. The new two-argument { oldValue, newValue in } signature is required by the current SDK and avoids a compiler warning in Xcode 16 (Swift 6 mode).
| .onChange(of: dragState.draggedTabId) { newDraggedTabId in | |
| .onChange(of: dragState.draggedTabId) { _, newDraggedTabId in |
There was a problem hiding this comment.
Fixed: switched to the two-argument form .onChange(of: dragState.draggedTabId) { _, newDraggedTabId in ... }. Fixed in a88ea8e.
— Claude Code
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Under LazyVStack, per-row `@State isHovering` could get stuck at true when a row scrolled out during an in-flight hover event, leaving multiple rows showing their close (X) button simultaneously. Move hover to the shared SidebarDragState.hoveredTabId so at most one row is hovered by construction. Row still derives `isHovering` locally as a computed read against dragState, so behavior (close button opacity, accessibility) is identical when only one row is hovered.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 224ea30703
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| private var isHovering: Bool { | ||
| dragState.hoveredTabId == tab.id |
There was a problem hiding this comment.
Each TabItemView now reads dragState.hoveredTabId via isHovering, so every hover transition invalidates every visible row in the LazyVStack instead of just the row being entered/exited. This is a regression from the previous per-row @State behavior and can create O(visible-rows) recomputation while the mouse moves across the sidebar, which is exactly the invalidation pattern warned about in /workspace/cmux/AGENTS.md ("Snapshot boundary for list subtrees"). In large workspaces this can hurt sidebar responsiveness and reintroduce lazy-layout thrash.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: extracted SidebarTabCloseButton which reads dragState.hoveredTabId inside its own body, so hover transitions now invalidate only that tiny subview (one per visible row) rather than the full ~1600-line TabItemView body. TabItemView.body no longer reads dragState.hoveredTabId at all. Fixed in a88ea8e.
— Claude Code
- 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).
Codex adversarial review flagged: a row hovered when it scrolls offscreen under LazyVStack never receives .onHover(false), so dragState.hoveredTabId stays pointing at its tab id. When the row scrolls back in, its SidebarTabCloseButton renders as hovered even though the pointer is elsewhere. Add .onDisappear on the row that nils hoveredTabId if it matches tab.id.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Sources/ContentView.swift (1)
10262-10274: Consider memoizing the previous-row lookup to avoid O(n) scans per frame on every visible row during drag.
showsCenteredTopDropIndicatorcallstabManager.tabs.firstIndex(where:)each time it evaluates. Since every visibleSidebarTabDropIndicatorOverlayreadsdragState.dropIndicator(which mutates at ~60fps during drag), each frame doesO(N)linear search per visible row →O(N·V)per indicator update, whereVis the number of visible rows. For typical sidebars this is negligible, but if users hit the reported large-workspace-count scenarios from issue#2586this undoes some of the laziness win.One option: precompute
previousTabIdinVerticalTabsSidebar(you already havetabIndexById) and pass it in as alet previousTabId: UUID?, turning the check into an==comparison. Non-blocking; flagging for awareness since perf under many workspaces is the explicit motivation of this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 10262 - 10274, showsCenteredTopDropIndicator currently does an O(n) search via tabManager.tabs.firstIndex(where:) each evaluation; precompute the previous tab id and pass it in so the method can use an O(1) comparison. Add a let previousTabId: UUID? in VerticalTabsSidebar (using existing tabIndexById or a one-time lookup) and forward it to SidebarTabDropIndicatorOverlay, then change showsCenteredTopDropIndicator to compare indicator.tabId == previousTabId (and keep the existing checks for indicator.edge == .top/.bottom and tabId equality) to avoid per-frame linear scans during drag.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Sources/ContentView.swift`:
- Around line 10262-10274: showsCenteredTopDropIndicator currently does an O(n)
search via tabManager.tabs.firstIndex(where:) each evaluation; precompute the
previous tab id and pass it in so the method can use an O(1) comparison. Add a
let previousTabId: UUID? in VerticalTabsSidebar (using existing tabIndexById or
a one-time lookup) and forward it to SidebarTabDropIndicatorOverlay, then change
showsCenteredTopDropIndicator to compare indicator.tabId == previousTabId (and
keep the existing checks for indicator.edge == .top/.bottom and tabId equality)
to avoid per-frame linear scans during drag.
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
2 participants
Summary
VStacktoLazyVStackso layout is O(visible), not O(total) workspaces.draggedTabId,dropIndicator) off@Statebindings onto a single@Observable @MainActor SidebarDragState, so 60fps writes during drag invalidate only the tiny overlays that read them, not the sidebar body or theLazyVStackcache.SidebarTabItemPresentationSnapshot+ frozen-presentation workflow;@Observableper-property tracking makes the context-menu freeze workaround unnecessary.Why not just swap
VStack→LazyVStack?That's what regressed #2586:
SidebarTabDropDelegatemutatesdropIndicatorat ~60fps during drag; withdropIndicatorheld as@State+ passed as@Binding, every write invalidated the sidebar body and thrashedLazySubviewPlacements.placeSubviews, pegging the main thread and deadlocking CLI commands. The real invariant we needed was: pointer state mutated during drag must not invalidate any ancestor of the lazy list.@Observable's per-keypath tracking enforces that by construction — only views that directly readdragState.dropIndicatorinvalidate.To keep that guarantee, the per-frame reads are isolated in dedicated views:
SidebarTabDropIndicatorOverlay— readsdragState.dropIndicatoranddragState.draggedTabId, renders the indicator rectangle.SidebarTabDragOpacityModifier— readsdragState.draggedTabId, applies drag opacity.The 1600-line
TabItemViewbody no longer reads drag transient state directly, so mid-drag writes don't invalidate it even though it's inside theLazyVStack.What changed
SidebarDragState— new@Observable @MainActorclass owningdraggedTabId+dropIndicator.VerticalTabsSidebar— three@Statefields (draggedTabId,dropIndicator,frozenTabItemPresentation) collapsed into one@State dragState = SidebarDragState().VStack→LazyVStack. The comment at the old line 10297 ("Workspaces are bounded, so prefer a non-lazy stack here") is gone.TabItemView— drops three@Bindings (draggedTabId,dropIndicator,frozenPresentation) and onelet livePresentation; gains onelet dragState: SidebarDragState. Context-menuonAppear/onDisappearno longer set/clear a frozen snapshot.SidebarEmptyArea— drops two@Bindings, gains onelet dragState.SidebarTabDropDelegate— drops two@Bindings, gains onelet dragState. Write sites becomedragState.draggedTabId = …/dragState.dropIndicator = ….SidebarTabItemPresentationSnapshot— deleted (was only used by the frozen workflow).Testing
xcodebuild -scheme cmux -configuration Debug -destination 'platform=macOS' -derivedDataPath /tmp/cmux-sidebar-lazy build— succeeds.sb-lazyproduced for manual verification of drag-reorder, multi-select (cmd/shift click), context menu, notification pills, and mid-drag CPU behavior. No E2E test added — XCUITest coverage for sidebar drag/CPU is brittle and the real verification signal (3s CPU sample under drag, per the Nightly freezes: sidebar LazyVStack layout loop pegs main thread at 100% CPU, deadlocks CLI #2586 methodology) is manual.Related
LazyVStacknow that the mechanism is fixed at the ownership level.VStackand dedupeddropIndicatorwrites.Summary by cubic
Switches the workspace sidebar to
LazyVStackand moves drag/hover state into an@ObservableSidebarDragStateso per-frame updates only re-render tiny overlays. Restores smooth scrolling, dragging, hover behavior, and keeps context‑menu visuals stable.Refactors
VStackwithLazyVStackinVerticalTabsSidebar.@MainActor@ObservableSidebarDragState(draggedTabId,dropIndicator,hoveredTabId); removed the globalSidebarTabItemPresentationSnapshotand re‑implemented a local per‑row frozen presentation (@State) while a context menu is open.dragStatethroughTabItemView,SidebarEmptyArea, andSidebarTabDropDelegate, removing previous@Bindingchains.SidebarTabDropIndicatorOverlay,SidebarEmptyAreaDropIndicatorOverlay,SidebarTabDragOpacityModifier, andSidebarTabCloseButtonto isolate high‑frequency reads from the main view body..onChangeto the macOS 14+ two‑argument API.Bug Fixes
dragState.hoveredTabIdand clearing it ononDisappearwhen rows are recycled byLazyVStack.Written for commit 6574946. Summary will update on new commits.
Summary by CodeRabbit