Repository navigation
Fix 100% CPU from ContentView publisher feedback loop (fixes #3020) - #3028
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds a MainActor Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as "ContentView / UI"
participant TabMgr as "TabManager"
participant Observer as "SelectedWorkspaceDirectoryObserver"
participant Workspace as "Workspace (currentDirectory)"
participant SessionStore as "SessionIndexStore"
rect rgba(200,200,255,0.5)
UI->>TabMgr: wire subscription onAppear (selectedTabId)
TabMgr->>Observer: publish selectedTabId
end
rect rgba(200,255,200,0.5)
Observer->>Workspace: resolve selected workspace
Workspace-->>Observer: stream currentDirectory
Observer->>UI: increment directoryChangeGeneration
end
rect rgba(255,200,200,0.5)
UI->>Observer: onChange(directoryChangeGeneration)
UI->>SessionStore: call syncFileExplorerDirectory()
SessionStore->>SessionStore: setCurrentDirectoryIfChanged(next)
SessionStore-->>UI: publish only if value changed
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 fixes a 100% CPU feedback loop (issue #3020) caused by two compounding problems:
Confidence Score: 4/5The core logic fix is sound, but the test file has a compile error that must be resolved before merging. One P1 finding: the test target will not build due to a missing
Important Files Changed
Sequence DiagramsequenceDiagram
participant TM as TabManager
participant Obs as SelectedWorkspaceDirectoryObserver (@StateObject)
participant CV as ContentView.body
participant SIS as SessionIndexStore
Note over CV: body re-evaluation (stable @StateObject, no subscription teardown)
CV->>Obs: wire(tabManager:) [idempotent guard]
TM-->>Obs: $selectedTabId publishes
Obs->>Obs: removeDuplicates(by workspace id)
Obs->>Obs: switchToLatest to workspace.$currentDirectory
Obs->>Obs: removeDuplicates(by workspace id + dir)
Obs->>Obs: directoryChangeGeneration += 1
CV->>CV: onChange(directoryChangeGeneration) fires
CV->>SIS: syncFileExplorerDirectory()
SIS->>SIS: setCurrentDirectoryIfChanged(dir)
SIS-->>SIS: currentDirectory published only if changed
|
![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: 1
🧹 Nitpick comments (2)
Sources/SessionIndexStore.swift (1)
294-297: Route direct writes throughsetCurrentDirectoryIfChanged(_:)for consistency.
RightSidebarPanelView.swiftbypasses the guard at two sites: line 50 (inside the mode-switch conditional inmodeBar) and line 76 (in.onAppearofcontentForMode). Both writesessionIndexStore.currentDirectory = …directly, allowing equal-value assignments to fire unnecessary@Publishedupdates.ContentView.swiftalready adopts this pattern uniformly across four call sites; migrating these two writes would alignRightSidebarPanelViewwith the established approach and prevent regressions from similar feedback loops.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/SessionIndexStore.swift` around lines 294 - 297, RightSidebarPanelView currently writes sessionIndexStore.currentDirectory directly in two places (inside the mode-switch conditional in modeBar and in the .onAppear of contentForMode); change those direct assignments to call sessionIndexStore.setCurrentDirectoryIfChanged(_:) instead so the guard-based check prevents redundant `@Published` updates—replace both occurrences of sessionIndexStore.currentDirectory = … with a call to setCurrentDirectoryIfChanged(next) and remove the direct writes to keep behavior consistent with ContentView's existing pattern.Sources/ContentView.swift (1)
1905-1910: RedundantTask {@mainactor… }inside a@MainActor-isolated sink.The observer is
@MainActor, and.receive(on: DispatchQueue.main)already delivers the sink on the main queue, so wrappingdirectoryChangeGeneration &+= 1inTask {@mainactor[weak self] in … }just adds an extra async hop that can reorder increments relative to the tab-switch side effects inTabManager.selectedTabId.didSet(which itself usesDispatchQueue.main.async). A direct assignment keeps the update tightly ordered with the source event and is still safe w.r.t. the "no body mutation" rule because this closure runs from a Combine dispatch, not frombody.♻️ Proposed simplification
- .receive(on: DispatchQueue.main) - .sink { [weak self] _ in - Task { `@MainActor` [weak self] in - self?.directoryChangeGeneration &+= 1 - } - } + .receive(on: DispatchQueue.main) + .sink { [weak self] _ in + self?.directoryChangeGeneration &+= 1 + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 1905 - 1910, The sink closure currently wraps self?.directoryChangeGeneration &+= 1 inside an extra Task { `@MainActor` … } which is redundant and can reorder updates; remove the Task wrapper so the Combine subscriber delivered via .receive(on: DispatchQueue.main) directly performs self?.directoryChangeGeneration &+= 1. Locate the Combine chain using .receive(on: DispatchQueue.main) and the .sink that references directoryChangeGeneration and replace the async Task block with a direct increment, ensuring ordering with TabManager.selectedTabId.didSet (which uses DispatchQueue.main.async) remains consistent.
🤖 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 1890-1911: The current compactMap drops nil selectedTabId values
so the pipeline never emits a "no-selection" event; change the chain so
tabManager.$selectedTabId is mapped (not compactMapped) into an inner
AnyPublisher that emits a sentinel/no-selection tuple when tabId is nil (instead
of returning nil) so switchToLatest gets detached and the downstream
removeDuplicates/sink sees the deselection; update the intermediate publisher
types and the removeDuplicates comparison to handle the optional/no-selection
tuple and keep incrementing directoryChangeGeneration in the existing sink
branch (symbols to change: tabManager.$selectedTabId mapping, the closure that
currently returns nil, the inner publisher returned for
workspace.$currentDirectory, switchToLatest usage, and the removeDuplicates
comparison that currently compares previous.0/next.0 and previous.1/next.1).
---
Nitpick comments:
In `@Sources/ContentView.swift`:
- Around line 1905-1910: The sink closure currently wraps
self?.directoryChangeGeneration &+= 1 inside an extra Task { `@MainActor` … }
which is redundant and can reorder updates; remove the Task wrapper so the
Combine subscriber delivered via .receive(on: DispatchQueue.main) directly
performs self?.directoryChangeGeneration &+= 1. Locate the Combine chain using
.receive(on: DispatchQueue.main) and the .sink that references
directoryChangeGeneration and replace the async Task block with a direct
increment, ensuring ordering with TabManager.selectedTabId.didSet (which uses
DispatchQueue.main.async) remains consistent.
In `@Sources/SessionIndexStore.swift`:
- Around line 294-297: RightSidebarPanelView currently writes
sessionIndexStore.currentDirectory directly in two places (inside the
mode-switch conditional in modeBar and in the .onAppear of contentForMode);
change those direct assignments to call
sessionIndexStore.setCurrentDirectoryIfChanged(_:) instead so the guard-based
check prevents redundant `@Published` updates—replace both occurrences of
sessionIndexStore.currentDirectory = … with a call to
setCurrentDirectoryIfChanged(next) and remove the direct writes to keep behavior
consistent with ContentView's existing pattern.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7ed888be-8a3e-4afc-89c2-5c7db09512fb
📒 Files selected for processing (3)
Sources/ContentView.swiftSources/SessionIndexStore.swiftcmuxTests/SessionIndexViewTests.swift
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/ContentView.swift">
<violation number="1" location="Sources/ContentView.swift:1891">
P2: `compactMap` drops `nil` `selectedTabId` values, so tab deselection never triggers `syncFileExplorerDirectory()`. The nil-clearing branch at line 3108 (`setCurrentDirectoryIfChanged(nil)`) becomes unreachable through this path, leaving `sessionIndexStore.currentDirectory` stale when no tab is selected. Use `.map` instead of `.compactMap` and handle the `nil` workspace case by emitting a sentinel value (e.g., `Just((nil, nil))`) so `switchToLatest` delivers a deselection event.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
Addressed the remaining Greptile note in c346693 by adding |
…-ai#3020) (manaflow-ai#3028) * Add regression test for session directory publishing * Fix ContentView directory sync feedback loop * Address directory observer review comments * Fix SessionIndexView test harness section initializer
This branch was successfully deployed
2 participants
Fixes #3020.
Summary
SessionIndexStore.currentDirectorywrites behindsetCurrentDirectoryIfChanged(_:)so equal values do not fire@PublishedfromsyncFileExplorerDirectory().ContentView.bodyinto a stable@StateObjectobserver, so body re-evaluation no longer recreates the subscription and resetsremoveDuplicates().Verification
origin(https://github.com/manaflow-ai/cmux.git)../scripts/reload.sh --tag issue-3020-content-view-cpu-loop --launch.cmux DEV issue-3020-content-view-cpu-loopprocess sampled at 14.4% CPU, then 11.8% CPU five seconds later, down from the reported 100% loop.Tests were not run locally per repo policy.
Note
Medium Risk
Refactors SwiftUI/Combine state propagation for workspace directory changes; behavior changes around when
currentDirectorypublishes could affect sidebar/session filtering if edge cases are missed.Overview
Fixes a runaway SwiftUI/Combine update loop that could peg CPU when syncing the file explorer/session index scope to the selected workspace directory.
Moves the selected-workspace/
currentDirectoryCombine chain out ofContentView.bodyinto a stable@StateObject(SelectedWorkspaceDirectoryObserver) and triggerssyncFileExplorerDirectory()via a generation counter to avoid re-subscribing on every body re-evaluation.Adds
SessionIndexStore.setCurrentDirectoryIfChanged(_:)and switches call sites (includingRightSidebarPanelView) to avoid equal-value@Publishedemissions, with a new Combine regression test ensuring duplicate sets don’t publish.Reviewed by Cursor Bugbot for commit c346693. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes a 100% CPU spin caused by a Combine feedback loop when syncing the file explorer directory to the selected workspace (fixes #3020). Keeps the subscription stable and blocks equal-value publishes to stop cascading updates.
SelectedWorkspaceDirectoryObserveras a@StateObjectand switched toonChangeof a generation counter to keep the subscription stable.SessionIndexStore.currentDirectorywithsetCurrentDirectoryIfChanged(_:)and applied it inContentViewandRightSidebarPanelView.SessionIndexViewtest harness section initializer to includeentries.Written for commit c346693. Summary will update on new commits.
Summary by CodeRabbit
Refactor
Tests