Repository navigation
Fix sidebar lag regression since v0.64.16 (#6612): cut per-row font-modifier + pin-state work - #6613
Merged
Merged
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
ChangesPinResolutionContext and TabItemView font magnification
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (21 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 SummaryThis PR fixes a ~13× main-thread regression in the sidebar introduced by #6554's
Confidence Score: 5/5Safe to merge — all three changes are purely additive optimizations that reduce SwiftUI view-graph node count and per-render allocations without altering observable behavior. The font change is behavior-preserving by construction: magnifiedFont resolves the exact same Font as CmuxFontModifier. PinResolutionContext refactor correctly reuses the already-built workspaceById dict, keeps liveWorkspaceIds(in tabManager:) live for performPinAction, and adds a defensive guard. Tooltip deferral is a pure allocation optimization. No files require special attention. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant VTS as VerticalTabsSidebar
participant PRC as PinResolutionContext
participant WAD as WorkspaceActionDispatcher
participant TIV as TabItemView (per row)
Note over VTS: Single render pass (N workspaces)
VTS->>VTS: tabs.map(\.id) → tabIds + workspaceById
VTS->>PRC: init(workspacesById, liveWorkspaceIds) — once
loop For each workspace row
VTS->>WAD: pinState(in: context, target:)
WAD->>PRC: context.liveWorkspaceIds, context.workspacesById
WAD-->>VTS: PinState
VTS->>TIV: TabItemView(tab, ...)
TIV->>TIV: "magnifiedFont() — one @Env read vs 20 DynamicBodies"
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 VTS as VerticalTabsSidebar
participant PRC as PinResolutionContext
participant WAD as WorkspaceActionDispatcher
participant TIV as TabItemView (per row)
Note over VTS: Single render pass (N workspaces)
VTS->>VTS: tabs.map(\.id) → tabIds + workspaceById
VTS->>PRC: init(workspacesById, liveWorkspaceIds) — once
loop For each workspace row
VTS->>WAD: pinState(in: context, target:)
WAD->>PRC: context.liveWorkspaceIds, context.workspacesById
WAD-->>VTS: PinState
VTS->>TIV: TabItemView(tab, ...)
TIV->>TIV: "magnifiedFont() — one @Env read vs 20 DynamicBodies"
end
Reviews (9): Last reviewed commit: "Merge origin/main into issue-6612-sideba..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+118
to
+121
| XCTAssertEqual( | ||
| WorkspaceActionDispatcher.pinState(in: context, target: .single(second.id))?.pinned, | ||
| false | ||
| ) |
Contributor
There was a problem hiding this comment.
The test is named …ReadsLivePinState, but the assertion being tested is actually a snapshot-vs-live split: pinState(in: context) reads the anchor workspace's isPinned through the live reference, so after manager.setPinned(second, true) the anchor's isPinned is true, and pinned = !isPinned = false. That assertion is correct, but the test name implies the context tracks live pin state — it does, but only because Workspace is a class. A comment like // context holds a live reference: isPinned is now true, so pinned = !isPinned = false directly on this assertion would make the intent immediately clear to future maintainers, since a reader who sees ?.pinned == false right after setPinned(_, pinned: true) will find the expectation surprising without that framing.
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!
Contributor
Author
There was a problem hiding this comment.
Addressed in d9b98f7 — added a comment documenting that PinResolutionContext captures Workspace by reference and that pinState returns the toggle (pinned = !isPinned), so pinned == false right after setPinned(_, pinned: true) is expected.
— Claude Code
Time Profiler A/B on the live 105-workspace app (idle agent-status churn, 35s) shows main-thread SwiftUI render/layout (NSHostingView.layout -> ViewGraphRootValueUpdater.render -> AG::Graph::UpdateStack::update) rose ~13x vs v0.64.16 (0.11s -> 1.47s), dominated by DynamicBody.updateValue / ForEachChild.updateValue re-evaluating the workspace-row ForEach. pinState was only ~25ms (~1%). The largest NEW per-row cost since v0.64.16 is the global font magnification feature (#6554): every sidebar row applied ~20 `.cmuxFont(...)` modifiers, each a custom @Environment-reading `CmuxFontModifier` ViewModifier (a DynamicBody + environment attribute). With 100+ workspaces continuously re-rendering rows under agent churn, that is ~20*N redundant per-label modifier bodies the sidebar must re-evaluate on every render pass. Read the magnification percent once per row via the existing `@Environment(\.cmuxGlobalFontMagnificationPercent)` and apply a primitive `.font(...)` resolved with `GlobalFontMagnification.scaledSize(_:percent:)` — identical math to `CmuxFontModifier`, so magnification still works, but the ~20 custom per-label modifier bodies per row become primitive font modifiers plus a single environment read. Deterministic per-row redundant-work count (N = workspace count): custom font-modifier bodies drop from 20*N to 0; magnification environment reads drop from 20*N to N. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
austinywang
changed the title
Addresses the cmux Aziz test-framework policy: new and touched non-UI tests should use Swift Testing (XCTest is reserved for cmuxUITests). Behavior-identical assertions; XCTUnwrap -> #require, XCTAssert* -> #expect. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Resolve ContentView.swift conflict: #6619 removed the high-memory pane warning UI (the `if hasMemoryWarning` sidebar triangle); my branch had only converted that block's `.cmuxFont` to `.font(magnifiedFont(...))`. Took origin/main's deletion — the row's other font conversions, the pinResolutionContext hoist, and lazy media tooltips are preserved. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address Greptile P2: the assertion checks `pinned == false` right after `setPinned(second, pinned: true)`, which is surprising without framing. Document that PinResolutionContext captures Workspace by reference and that pinState returns the toggle (!isPinned). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The migration to Swift Testing dropped `import XCTest`, which had transitively provided Foundation's `UUID`. `import Testing` does not re-export Foundation, so the suite failed to compile with "Cannot find 'UUID' in scope" (CI: app-host unit tests shards 1/4 and 4/4). Add an explicit `import Foundation`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…rd reshuffle The Swift Testing migration + added test changed the suite's weight (4->5), which is fed into the deterministic weight-balanced partition in scripts/ci/cmux_unit_test_shard.py. A 1-unit total-weight change (1268->1269) reshuffles ~62 suites across the 4 app-host shards, concentrating a cluster of order/parallelism-sensitive GUI/WebView suites (MarkdownMermaidZoomTests, TerminalSearchOverlayMouseReleaseTests, BrowserWebContentProcessTests, FileExplorerStoreTests, WorkspaceTerminalFocusRecoverySwiftTests, ...) into shard 1/4, where they fail under co-scheduling. Those same suites are 12/12 green on main, where the weight-1268 partition spreads them out. Reverting this test file to origin/main restores weight 1268 and a byte-identical shard partition (verified locally: shard 1 membership == main). The new PinResolutionContext / pinState(in:context:) seam stays covered transitively — the existing pinState(in:manager:) tests now delegate through it. Declining the Aziz "use Swift Testing" P2 here: CI-partition stability outweighs the style preference, and the migration also dropped the Foundation import (UUID). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
CI note: reverted the test-file change (shard-partition stability)The earlier red
Fix: reverted This means I'm declining the "migrate to Swift Testing" suggestion on this suite for now: the migration + extra test destabilized the shard partition (and dropped the Foundation/ |
Integrate #6625 (revert sidebar row-height layout feedback), #6617/#6621, and #6624. ContentView conflict: #6625 removed the render-context/row `sidebarWidth` threading while this branch added `pinResolutionContext` — took #6625's sidebarWidth removal and kept pinResolutionContext. Font fix (`magnifiedFont`/@Environment), lazy tooltips, and #6625's rowHeight probe all preserved. swift-file-length-budget.tsv regenerated via --write-budget (only ContentView's entry changes, +40 lines). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Integrate #6609 (Codex sidebar status lifecycle) and #6627 (sidebar tab selection highlight timing). ContentView auto-merged (no overlap with the font fix / pinResolutionContext). swift-file-length-budget.tsv regenerated via --write-budget. Shard partition verified identical to origin/main (weight 1267). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This branch was successfully 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
1 participant
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
Closes #6612
Fixes the sidebar lag regression that landed after
v0.64.16(5321becb6) with 100+ workspaces under continuous agent-status churn.This PR makes three reductions to the per-row work the sidebar performs on every render pass:
.cmuxFont(...)modifiers — each a custom@Environment-readingCmuxFontModifierViewModifierintroduced by the global font-magnification feature (Add global font magnification #6554). The row now reads the magnification percent once via the existing@Environment(\.cmuxGlobalFontMagnificationPercent)and applies a primitive.font(...)resolved withGlobalFontMagnification.scaledSize(_:percent:)(identical math toCmuxFontModifier, so magnification is unchanged).WorkspaceActionDispatcher.PinResolutionContextonce perVerticalTabsSidebarrender pass and reuse it for every row's context-menu pin state (was a full workspace dictionary + live-id set rebuilt per row).String(localized:)allocations every render.Diagnosis (Time Profiler, live 105-workspace app)
I profiled two already-running builds with the same 105-workspace session under idle agent-status churn, 35s each, attaching
xctraceTime Profiler:v0.64.16nightly (regressed)NSHostingView.layout→ViewGraphRootValueUpdater.render→AG::Graph::UpdateStack::update)The regressed build spends ~13× more main-thread time in SwiftUI view-graph render/layout of the sidebar over a comparable idle-churn window. App-level breakdown of the regressed main thread:
DynamicBody.updateValue0.33s + closures 0.31s,ForEachChild.updateValue0.22s + closures 0.30s — i.e. SwiftUI re-evaluating/re-diffing the workspace-rowForEach.VerticalTabsSidebar.workspaceRow0.082s,TabItemView.body0.048s, andWorkspaceActionDispatcher.pinStateonly ~0.025s (~1%).Attribution. The
workspaceRowsForEachre-evaluates per-row subtrees whenever a workspace's state changes (this O(N)-per-tick architecture is the same asv0.64.16). What changed afterv0.64.16is the per-row subtree weight:v0.64.16rows used primitive.font(...)(the row's.font(count was 21); the current rows replaced all of them with custom.cmuxFont(...)modifiers (#6554), each adding aModifiedContent+ an environment-read attribute + aModifierBodyAccessor/DynamicBody. With 100+ workspaces re-rendering rows continuously, that multiplied theDynamicBody/environment node count the sidebar must re-evaluate. (#6517's media glyphs are conditional and inert for non-browser workspaces; #6558's memory-warning read is gated and polled every 4s — neither is the dominant per-tick cost.)The original
pinStaterebuild this PR also fixes is a real per-row inefficiency, but the profile shows it was only ~1% of main-thread cost — the per-row font-modifier bloat is the dominant regressing path, so this PR targets that directly.Before/After redundant-work count (N = workspace count)
Per sidebar render pass, per row:
20·N→0(primitive.font(...))20·N→N(one read per row)Ndictionaries +Nsets →0extra dictionaries, one shared live-id set per passString(localized:):3·N→0for rows without active mediaSince
v0.64.16already used primitive.font(...)on these rows, change (1) restores the row's font-node shape to the baseline while preserving the magnification feature, directly reversing the dominant per-row structural regression.Tests / Verification
PinResolutionContext/pinState(in:context:)seam is covered transitively by the existingWorkspaceActionDispatcherTests(thepinState(in:manager:)path now delegates through the new context overload). The test file is kept atorigin/mainon purpose — migrating it to Swift Testing + adding a test changed the deterministic CI shard weight (1268→1269) and reshuffled the 4 app-host shards, concentrating unrelated flaky GUI/WebView suites into shard 1/4 (see the CI note comment). Reverting it restores the byte-identical, proven-green partition.magnifiedFont(...)resolves the exact sameFontasCmuxFontModifier(Font.system(size: GlobalFontMagnification.scaledSize(base, percent:), weight:, design:)+ optional.monospacedDigit()).git diff --check;.github/swift-file-length-budget.tsvrefreshed viascripts/swift_file_length_budget.py --write-budget.Localization
No user-facing strings added or changed. The media tooltip strings were already localized; this only moves their evaluation into the branch where the glyph renders. Magnification behavior is unchanged.
Follow-up (not in this PR)
A further reduction is possible by scoping the row's
sidebarUnreadreads so a single workspace's unread/notification change updates only its own row instead of re-evaluating the wholeworkspaceRowsForEach(the O(N)-per-tick re-eval is pre-existing, not av0.64.16regression). That is a larger, separately-testable change and is intentionally out of scope here.Summary by CodeRabbit