Repository navigation
Fix 100% CPU render loop when an extension sidebar is selected (#5970) - #5972
kevinsslin wants to merge 1 commit into
Conversation
|
@kevinsslin is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughContentView replaces two pairs of ChangesTask-based extension-sidebar observation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 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 |
|
@codex review |
@kevinsslin I can't start this review because your workspace has reached its free monthly review limit. cubic has reviewed 241,260 of the 240,000 allowed lines of code this month. Reviews resume on 1 July 2026 (in 19 days). Paid plans include much higher monthly review limits. Upgrade now to resume reviews. To help optimise your usage, you can tune cubic to get the most out of your usage limits:
|
|
To use Codex here, create a Codex account and connect to github. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get PR details and changed files
gh pr view 5972 --repo manaflow-ai/cmux
echo "---FILES---"
gh pr diff 5972 --repo manaflow-ai/cmuxLength of output: 11625
Review of PR
|
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Greptile SummaryFixes the 100% CPU render loop (#5970) by replacing four inline
Confidence Score: 5/5Safe to merge — the change is a well-scoped subscription-lifecycle fix that eliminates the render loop without touching any publisher logic or downstream state contracts. The four modified subscription sites are straightforwardly converted from inline Combine No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant SwiftUI as SwiftUI Runtime
participant Task as .task(id: tabIds)
participant Pub as Observation Publisher
participant Refresh as refreshExtensionSidebarSnapshot()
Note over SwiftUI,Refresh: BEFORE (CPU loop)
SwiftUI->>Pub: body pass → new publisher created
Pub-->>SwiftUI: .onReceive subscribes
Pub->>Refresh: replays current value on subscribe
Refresh->>SwiftUI: "extensionSidebarUpdateToken &+= 1"
SwiftUI->>Pub: body re-invalidated → new publisher created
Pub-->>SwiftUI: .onReceive re-subscribes
Note right of SwiftUI: loop at ~100% CPU
Note over SwiftUI,Refresh: AFTER (this PR)
SwiftUI->>Task: view appears / tabIds changes
Task->>Pub: publisher(renderContext).values
Pub->>Refresh: replays current value once on subscribe
Refresh->>SwiftUI: "extensionSidebarUpdateToken &+= 1"
Note right of Task: task is NOT restarted (tabIds unchanged)
SwiftUI->>SwiftUI: body re-evaluates, task id unchanged
Pub->>Refresh: only fires on real workspace state change
Note over Task,Pub: tabIds changes → task cancelled and restarted
Reviews (5): Last reviewed commit: "Fix 100% CPU render loop when an extensi..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private func updateExtensionSidebarObservation(renderContext: WorkspaceListRenderContext) { | ||
| guard extensionSidebarObservedTabIds != renderContext.tabIds else { return } | ||
| extensionSidebarObservedTabIds = renderContext.tabIds | ||
| extensionSidebarImmediateObservation = extensionSidebarImmediateObservationPublisher(renderContext: renderContext) | ||
| .receive(on: RunLoop.main) | ||
| .eraseToAnyPublisher() | ||
| extensionSidebarDebouncedObservation = extensionSidebarDebouncedObservationPublisher(renderContext: renderContext) | ||
| .receive(on: RunLoop.main) | ||
| .debounce(for: Self.extensionSidebarObservationCoalesceInterval, scheduler: RunLoop.main) | ||
| .eraseToAnyPublisher() | ||
| } |
There was a problem hiding this comment.
@State is a symptom patch, not the correct SwiftUI fix
Storing AnyPublisher<Void, Never> in @State and guarding rebuilds with a parallel extensionSidebarObservedTabIds shadow is the kind of new flag/cache/side-channel that cmux-swift-architectural-rethink calls out: the bad pattern (onReceive on a per-body-pass Combine publisher) is still present and representable; the guard just breaks the visible feedback loop. The SwiftUI-idiomatic replacement is .task(id: renderContext.tabIds) { for await _ in someAsyncSequence { refreshExtensionSidebarSnapshot() } } or a single .task(id:) that wraps both publishers via AsyncPublisher. A task modifier is torn down and restarted by SwiftUI whenever its id changes, so no shadow state, no guard comparison, and no manual publisher caching are needed — the runtime owns the subscription lifecycle.
Rule Used: Flag Swift fixes that patch symptoms while leaving... (source)
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!
| @State private var extensionSidebarObservedTabIds: [UUID] = [] | ||
| @State private var extensionSidebarImmediateObservation: AnyPublisher<Void, Never> = Empty().eraseToAnyPublisher() | ||
| @State private var extensionSidebarDebouncedObservation: AnyPublisher<Void, Never> = Empty().eraseToAnyPublisher() |
There was a problem hiding this comment.
@State — should be Swift Concurrency
extensionSidebarImmediateObservation and extensionSidebarDebouncedObservation are new AnyPublisher<Void, Never> values held in SwiftUI @State. Per cmux-swift-concurrency-modernization, new Combine app state in cmux-owned Swift should be replaced with Swift concurrency primitives. Exposing the workspace observation sources as AsyncSequence (e.g. wrapping the existing Publishers.MergeMany via values) and driving refresh from a .task(id:) modifier would eliminate the need for these two new @State fields, the shadow extensionSidebarObservedTabIds tracker, and the manual guard in updateExtensionSidebarObservation altogether.
Rule Used: Flag new legacy async patterns in cmux-owned Swift... (source)
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!
Greptile SummaryThis PR fixes a 100% CPU render loop triggered by selecting a bundled extension-sidebar preset. The loop was caused by
Confidence Score: 4/5Safe to merge; the fix correctly breaks the infinite re-subscription loop with no functional regressions. The core logic is sound — caching the publishers in @State and rebuilding only on tabIds changes correctly prevents the per-render resubscription cycle. The only notable issue is that extensionSidebarObservedTabIds is a shadow copy of tabIds whose guard is redundant with .onChange's own change-detection; dropping it would simplify the invariant and reduce the surface of future sync bugs, but it does not break anything today. Sources/ContentView.swift — specifically the updateExtensionSidebarObservation method and its guard condition. Important Files Changed
Sequence DiagramsequenceDiagram
participant SW as SwiftUI Runtime
participant V as VerticalTabsSidebar.body
participant OC as .onChange(tabIds)
participant UP as updateExtensionSidebarObservation
participant OR as .onReceive(publisher)
participant RS as refreshExtensionSidebarSnapshot()
Note over SW,RS: Before fix - infinite loop
SW->>V: body evaluation
V->>OR: inline publisher creation, new subscription
OR->>RS: replay on subscribe
RS->>SW: bump extensionSidebarUpdateToken
SW->>V: re-evaluate body (loop)
Note over SW,RS: After fix - stable publishers
SW->>V: body evaluation (initial)
V->>OC: .onChange fires (initial: true)
OC->>UP: updateExtensionSidebarObservation
UP->>UP: guard tabIds changed, build publishers once
UP-->>V: "@State publishers set"
SW->>V: body re-evaluates
V->>OR: .onReceive subscribes to same stable publisher
OR->>RS: replay on genuine re-subscribe
RS->>SW: bump extensionSidebarUpdateToken
SW->>V: body re-evaluates
V->>OC: .onChange sees same tabIds, no-op
Note over OR,RS: no more replays
Reviews (2): Last reviewed commit: "Fix 100% CPU render loop when an extensi..." | Re-trigger Greptile |
| private func updateExtensionSidebarObservation(renderContext: WorkspaceListRenderContext) { | ||
| guard extensionSidebarObservedTabIds != renderContext.tabIds else { return } | ||
| extensionSidebarObservedTabIds = renderContext.tabIds | ||
| extensionSidebarImmediateObservation = extensionSidebarImmediateObservationPublisher(renderContext: renderContext) | ||
| .receive(on: RunLoop.main) | ||
| .eraseToAnyPublisher() | ||
| extensionSidebarDebouncedObservation = extensionSidebarDebouncedObservationPublisher(renderContext: renderContext) | ||
| .receive(on: RunLoop.main) | ||
| .debounce(for: Self.extensionSidebarObservationCoalesceInterval, scheduler: RunLoop.main) | ||
| .eraseToAnyPublisher() | ||
| } |
There was a problem hiding this comment.
tabIds state doubles the source of truth for workspace membership. extensionSidebarObservedTabIds is a copy of renderContext.tabIds whose only purpose is to gate publisher rebuilds — it's a side channel that must stay in sync with renderContext. If renderContext.tabIds ever changes without going through .onChange on this ZStack (e.g., if the modifier is accidentally removed or the ZStack is conditionally excluded), the publishers silently become stale. Relying on the fact that .onChange fires only on change removes the need for the separate extensionSidebarObservedTabIds @State variable entirely.
Rule Used: Flag Swift fixes that patch symptoms while leaving... (source)
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!
Greptile SummaryFixes a 100% CPU SwiftUI render loop triggered when an extension sidebar preset was selected. The root cause was that
Confidence Score: 4/5The change correctly eliminates the render loop and is safe to merge; the two comments are clean-up suggestions that do not affect correctness. The fix is targeted and mechanically sound: moving publisher construction out of Sources/ContentView.swift — specifically the Important Files Changed
Sequence DiagramsequenceDiagram
participant SwiftUI as SwiftUI Runtime
participant Body as VerticalTabsSidebar.body
participant State as @State publishers
participant onReceive as .onReceive
participant Pub as AnyPublisher (Merged @Published)
participant Token as extensionSidebarUpdateToken
Note over SwiftUI,Token: BEFORE fix - unbounded loop
SwiftUI->>Body: evaluate body
Body->>onReceive: inline factory() new AnyPublisher
onReceive->>Pub: subscribe (new instance every pass)
Pub-->>onReceive: replay current value (on-subscribe)
onReceive->>Token: refreshExtensionSidebarSnapshot() token++
Token-->>SwiftUI: "@State change - invalidate body"
SwiftUI->>Body: evaluate body (loop)
Note over SwiftUI,Token: AFTER fix - bounded, stable
SwiftUI->>Body: evaluate body
Body->>State: read cached extensionSidebarImmediateObservation
State-->>onReceive: same AnyPublisher instance (stable)
Note over onReceive: no re-subscription - no replay - no token bump
Note over SwiftUI,State: On tabIds change only
SwiftUI->>Body: tabIds changed - .onChange fires
Body->>State: updateExtensionSidebarObservation() new AnyPublisher
State-->>onReceive: new instance - re-subscribe once
Pub-->>onReceive: replay current value
onReceive->>Token: token++ (one body re-render)
Token-->>SwiftUI: "@State change"
SwiftUI->>Body: evaluate body
Body->>State: same publisher instances (guard prevented rebuild)
Note over onReceive: no re-subscription - loop stops
Reviews (3): Last reviewed commit: "Fix 100% CPU render loop when an extensi..." | Re-trigger Greptile |
| private func updateExtensionSidebarObservation(renderContext: WorkspaceListRenderContext) { | ||
| guard extensionSidebarObservedTabIds != renderContext.tabIds else { return } | ||
| extensionSidebarObservedTabIds = renderContext.tabIds | ||
| extensionSidebarImmediateObservation = extensionSidebarImmediateObservationPublisher(renderContext: renderContext) | ||
| .receive(on: RunLoop.main) | ||
| .eraseToAnyPublisher() | ||
| extensionSidebarDebouncedObservation = extensionSidebarDebouncedObservationPublisher(renderContext: renderContext) | ||
| .receive(on: RunLoop.main) | ||
| .debounce(for: Self.extensionSidebarObservationCoalesceInterval, scheduler: RunLoop.main) | ||
| .eraseToAnyPublisher() | ||
| } |
There was a problem hiding this comment.
@State-held AnyPublisher is new Combine view state
The three new @State vars (extensionSidebarObservedTabIds, extensionSidebarImmediateObservation, extensionSidebarDebouncedObservation) introduce explicit Combine state management into the view — the publisher instances are owned and lifetime-managed by the view's @State bag rather than by a dedicated object. While the fix is correct, the pattern is non-idiomatic and runs against the repo's push away from new ObservableObject/Combine-owned view state. A @StateObject (or @Observable-conforming) coordinator that initialises its own merged publisher from tabIds and exposes a single updateToken-style change signal would own the publisher lifetime, avoid the extensionSidebarObservedTabIds mirror variable entirely, and make the re-subscription contract visible at the type level rather than in a body-level onChange closure. Worth a follow-up to bring this pattern into the @Observable shape the rest of the sidebar state uses, even if the immediate fix is safe.
Rule Used: Flag new legacy async patterns in cmux-owned Swift... (source)
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!
| .onChange(of: renderContext.tabIds, initial: true) { _, _ in | ||
| updateExtensionSidebarObservation(renderContext: renderContext) | ||
| } |
There was a problem hiding this comment.
renderContext captured in onChange closure can diverge from emitted tabIds
The closure discards the newValue parameter and calls updateExtensionSidebarObservation(renderContext: renderContext) using a copy of renderContext captured from the surrounding body scope. In the common case this is fine, but if a second body pass (for an unrelated state change) produces a new renderContext before this onChange action fires, the publishers are rebuilt from a snapshot that already matches extensionSidebarObservedTabIds. The guard on line 11624 (extensionSidebarObservedTabIds != renderContext.tabIds) would compare the captured (possibly stale) renderContext.tabIds against the stored one, potentially passing when it should not or skipping when it should. Using the emitted newValue of the onChange parameter (i.e., { _, newTabIds in ... }) for the guard comparison, and ensuring renderContext.tabs is sourced from the live tabManager at call time, would make this immune to the snapshot-staleness risk.
Rule Used: Flag Swift fixes that patch symptoms while leaving... (source)
…low-ai#5970) VerticalTabsSidebar observed workspace state with `.onReceive` on a Combine publisher rebuilt inline on every body pass, so SwiftUI re-subscribed on every render. Each publisher is a CombineLatest of @published fields that replays its current value on subscription; that replay bumped the @State extensionSidebarUpdateToken, re-invalidated the body, and re-subscribed again: an unbounded re-render loop that pinned the main thread at ~100% CPU whenever a non-default extension sidebar (e.g. the bundled "Project Worktrees" preset) was selected. The selection persists, so the app re-entered the loop on every launch. Observe via `.task(id: renderContext.tabIds)` instead, bridging the existing observation publishers through `.values`. SwiftUI now owns the subscription lifecycle: it restarts only when the observed workspace set changes and is torn down on disappear, so a subscription is established once per genuine change rather than every frame. No new Combine view state and no shadow tab-id tracker. The publishers are unchanged, so their on-subscribe replay (relied on by a late-mounting sidebar row, covered by WorkspaceSidebarObservationTests) still works.
4f4bbf0 to
73da164
Compare
|
Thanks for the review. Agreed on the Reworked to the suggested Swift Concurrency shape: two
This also addresses the Verified locally on a from-source build at the affected commit: with (The two failing |
|
@coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
gh pr diff 5972 --repo manaflow-ai/cmuxLength of output: 4427
|
|
Reviewed this and reproduced the CPU issue before/after. The CPU fix is real: with I think there is still one blocker before merge: the new Suggested fix: add an explicit bounded refresh trigger for the unread projection consumed by hosted extension snapshots, without reintroducing body-level Combine resubscription. |
|
CI follow-up: Failure: Since this PR only changes |
3 participants
Summary
Fixes #5970: selecting a bundled extension-sidebar preset (e.g. "Project Worktrees") pinned the main thread at ~100% CPU in an unbounded SwiftUI re-render loop, and because the selection persists in
cmuxExtensionSidebar.providerId, the app re-entered the loop on every launch.Root cause.
VerticalTabsSidebarobserved workspace state with.onReceiveon a publisher built inline on everybodypass:That factory returns a fresh
AnyPublishereach render, so SwiftUI re-subscribed every render. Each publisher is aCombineLatestof@Publishedworkspace fields, which replays its current value on every subscription. The replay calledrefreshExtensionSidebarSnapshot()-> bumped the@StateextensionSidebarUpdateToken-> re-invalidatedbody-> re-subscribed -> replayed again. (The default workspaces sidebar doesn't wire a recreated-per-render publisher into a body-level@Statebump, so it was unaffected.)Fix. Observe via
.task(id: renderContext.tabIds), bridging the existing observation publishers through.values. SwiftUI owns the subscription lifecycle: the task restarts only when the observed workspace set changes and is torn down on disappear, so a subscription is established once per genuine change instead of every frame. No new Combine view state, no shadow tab-id tracker. The publishers themselves are unchanged, so their on-subscribe replay (relied on by a late-mounting sidebar row, covered byWorkspaceSidebarObservationTests) still fires when a subscription is genuinely established.Testing
cmuxExtensionSidebar.providerId = com.example.cmux.sidebar.project-worktreeswith a workspace open -> sustained ~100% CPU.sampleshows a continuousGraphHost.flushTransactions->VerticalTabsSidebar.body->CmuxExtensionSidebarSelection.descriptor(for:).WorkspaceSidebarObservationTests(the publishers' emit-on-subscribe contract) is unaffected since the publishers are untouched.Notes
@State-publisher-caching approach to.task(id:)per reviewer feedback (cmux-swift-concurrency-modernization/cmux-swift-architectural-rethink): no new Combine@State, and the runtime owns the subscription lifecycle.Checklist
WorkspaceSidebarObservationTests) remain valid; the change is in view-subscription lifecycle, not the publishersSummary by CodeRabbit