Repository navigation
Speed up sidebar pin state lookup - #6233
azooz2003-bit wants to merge 10 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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:
📝 WalkthroughWalkthrough
ChangesPin-state lookup refactor
Terminal session snapshot VT export behavior
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 20 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (20 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 speeds up the sidebar context-menu pin-state lookup by reusing the already-indexed
Confidence Score: 4/5Safe to merge after addressing the scrollback cache eviction issue in Workspace.swift; the pin-state and terminal-read changes are solid. The Sources/Workspace.swift — the Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant SB as VerticalTabsSidebar (row render)
participant RC as VerticalTabsSidebarRenderContext
participant WAD as WorkspaceActionDispatcher
participant TM as TabManager
participant WS as Workspace
SB->>RC: workspaceById (pre-indexed)
SB->>WAD: pinState(workspacesById:, target:)
Note over WAD: O(1) dict lookup, no tabs scan
Note over TM: On session snapshot
TM->>TM: build claimScrollbackCapture() closure
TM->>WS: sessionSnapshot(claimScrollbackCapture:, usesScrollbackCaptureBudget: true)
WS->>WS: claimScrollbackCapture() → Bool
alt budget available
WS->>WS: readTerminalTextForSessionSnapshot()
WS->>WS: terminalSnapshotScrollback(allowFallbackScrollback: true)
else budget denied
WS->>WS: terminalSnapshotScrollback(allowFallbackScrollback: false)
Note over WS: restoredTerminalScrollbackByPanelId entry removed
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant SB as VerticalTabsSidebar (row render)
participant RC as VerticalTabsSidebarRenderContext
participant WAD as WorkspaceActionDispatcher
participant TM as TabManager
participant WS as Workspace
SB->>RC: workspaceById (pre-indexed)
SB->>WAD: pinState(workspacesById:, target:)
Note over WAD: O(1) dict lookup, no tabs scan
Note over TM: On session snapshot
TM->>TM: build claimScrollbackCapture() closure
TM->>WS: sessionSnapshot(claimScrollbackCapture:, usesScrollbackCaptureBudget: true)
WS->>WS: claimScrollbackCapture() → Bool
alt budget available
WS->>WS: readTerminalTextForSessionSnapshot()
WS->>WS: terminalSnapshotScrollback(allowFallbackScrollback: true)
else budget denied
WS->>WS: terminalSnapshotScrollback(allowFallbackScrollback: false)
Note over WS: restoredTerminalScrollbackByPanelId entry removed
end
Reviews (7): Last reviewed commit: "Budget restored scrollback fallback" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @MainActor | ||
| private static func liveWorkspaceIds( | ||
| in tabManager: TabManager, | ||
| static func liveWorkspaceIds( | ||
| workspacesById: [UUID: Workspace], | ||
| from workspaceIds: [UUID] | ||
| ) -> [UUID] { |
There was a problem hiding this comment.
liveWorkspaceIds was private before this PR. No external caller was added — the tests exercise it only indirectly through pinState(workspacesById:) — so removing private needlessly widens the internal API surface of WorkspaceActionDispatcher.
| @MainActor | |
| private static func liveWorkspaceIds( | |
| in tabManager: TabManager, | |
| static func liveWorkspaceIds( | |
| workspacesById: [UUID: Workspace], | |
| from workspaceIds: [UUID] | |
| ) -> [UUID] { | |
| @MainActor | |
| private static func liveWorkspaceIds( | |
| workspacesById: [UUID: Workspace], | |
| from workspaceIds: [UUID] | |
| ) -> [UUID] { |
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!
| capturedScrollback: capturedScrollback, | ||
| includeScrollback: includeScrollback, | ||
| allowFallbackScrollback: shouldPersistScrollback || allowDebugFallbackScrollback || hasRestoredScrollbackFallback | ||
| allowFallbackScrollback: shouldCaptureScrollback || allowDebugFallbackScrollback || (!usesScrollbackCaptureBudget && hasRestoredScrollbackFallback) |
There was a problem hiding this comment.
When usesScrollbackCaptureBudget = true and a panel's claimScrollbackCapture() returns false, shouldCaptureScrollback is false, so allowFallbackScrollback collapses to false || false || (false && hasRestoredScrollbackFallback) = false. terminalSnapshotScrollback then receives capturedScrollback: nil and allowFallbackScrollback: false, causing resolvedSnapshotTerminalScrollback to return nil, which triggers restoredTerminalScrollbackByPanelId.removeValue(forKey: panelId).
This permanently wipes the in-memory scrollback cache entry for every budget-denied panel on each session snapshot. Workspaces ranked beyond the 8 background slots will lose all previously-persisted scrollback after the first background save, even though the intent of the budget is only to limit live terminal reads, not to evict data already in hand.
Restoring hasRestoredScrollbackFallback unconditionally preserves the existing stored value without incurring any terminal read:
allowFallbackScrollback: shouldCaptureScrollback || allowDebugFallbackScrollback || hasRestoredScrollbackFallback
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
3 participants
Summary
VerticalTabsSidebarRenderContext.workspaceByIdwhen computing sidebar row context-menu pin state.Benchmark
28401.14 ms, new pre-indexed shape49.26 ms.Testing
git diff --checkxcodebuild -project cmux.xcodeproj -scheme cmux-unit -configuration Debug -destination 'platform=macOS' -derivedDataPath /tmp/cmux-sclrow build./scripts/reload.sh --tag sclrow(cloud reload script was unavailable in this worktree)sclrow, moved the window toLG HDR 4K, built a 73-workspace sidebar fixture before workspace creation started timing out, verifiedworkspace-action pinandworkspace-action unpinonworkspace:1, and captureddebug.window.screenshotat/var/folders/xw/j2s0lpvj16b4y5_5hsfcphb00000gn/T/cmux-screenshots/sclrow-sidebar_2026-06-16T04-50-00Z_D824AFAB.png.Follow-up Evidence
workspace.createtook5016.92 msthen20745.89 ms, andlist-workspacestimed out while logs showed repeated git probe, PR refresh, mobile observer, and sidebar invalidation work.cgWindowNotFound, so the PR proves the shared pin action and rendered sidebar, while the exact context-menu click remains manual dogfood.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Speeds up sidebar context‑menu pin state with a pre‑indexed workspace map, cutting 2000 row renders from 28,401 ms to 49 ms. Also speeds up and bounds session scrollback by reading terminal text in‑process, skipping VT export/Base64, prioritizing the selected workspace with per‑window capture limits, and budgeting restored‑scrollback fallback.
Refactors
WorkspaceActionDispatcher.pinState(workspacesById:target)and updated the sidebar to passrenderContext.workspaceById;pinState(in:tabManager:)wraps the indexed path and keeps the live lookup helper private.encodeBase64toTerminalController.terminalTextPayload; addedreadTerminalTextForSessionSnapshotand switchedWorkspaceto this path. Migrated dispatcher tests to Swift Testing with coverage for indexed parity and stale/duplicate targets.Performance
lineLimitwhen set.Written for commit bd325d0. Summary will update on new commits.
Summary by CodeRabbit