Repository navigation
perf(sidebar): throttle immediate observation publisher to coalesce agent title bursts - #6807
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:
📝 WalkthroughWalkthroughAdds a 50ms sidebar coalescing interval, a custom Combine operator, applies it to workspace and merged sidebar observation streams, and adds tests plus project wiring. ChangesSidebar Observation Coalescing
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (3 errors)
✅ Passed checks (22 passed)
✨ 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 sustained main-thread CPU regression caused by agent (Codex) title rewrites driving a full
Confidence Score: 5/5Safe to merge — the custom operator is well-encapsulated, main-thread-only as documented, and all edge cases (overdue callbacks, cancellation, cross-workspace aggregate coalescing) are covered by deterministic unit tests that were red on the throttle approach and green on this one. The operator state machine has been traced through multiple interleaving scenarios (replay, leading edge, overdue window, concurrent workspace bursts) and behaves correctly in all of them. The four new tests — including a virtual-scheduler test that exercises the overdue-callback supersession path — directly cover the contracts the PR changes. The extension-sidebar fan-out gap called out in a previous review comment is closed by the new merged helper. No existing invariants are weakened. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Upstream @Published value arrives] --> B{hasReceivedReplay?}
B -- "No" --> C[Forward synchronously, no window opened]
B -- "Yes" --> D{windowStart set AND now < windowStart+50ms?}
D -- "No" --> E[Forward synchronously, windowStart = now, pendingValue = nil]
D -- "Yes" --> F[pendingValue = input]
F --> G{trailingScheduled?}
G -- No --> H[Schedule emitTrailing at deadline]
G -- Yes --> I[No-op]
H --> J([RunLoop fires after ~50ms])
J --> K{isCancelled or pendingValue nil?}
K -- Yes --> L[Return - stale callback]
K -- No --> M{now < windowStart+50ms?}
M -- Yes --> N[Reschedule at new deadline]
M -- No --> O[Emit pendingValue, windowStart = now]
%%{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"}}}%%
flowchart TD
A[Upstream @Published value arrives] --> B{hasReceivedReplay?}
B -- "No" --> C[Forward synchronously, no window opened]
B -- "Yes" --> D{windowStart set AND now < windowStart+50ms?}
D -- "No" --> E[Forward synchronously, windowStart = now, pendingValue = nil]
D -- "Yes" --> F[pendingValue = input]
F --> G{trailingScheduled?}
G -- No --> H[Schedule emitTrailing at deadline]
G -- Yes --> I[No-op]
H --> J([RunLoop fires after ~50ms])
J --> K{isCancelled or pendingValue nil?}
K -- Yes --> L[Return - stale callback]
K -- No --> M{now < windowStart+50ms?}
M -- Yes --> N[Reschedule at new deadline]
M -- No --> O[Emit pendingValue, windowStart = now]
Reviews (8): Last reviewed commit: "Make the overdue-trailing regression tes..." | Re-trigger Greptile |
Agents (e.g. Codex) rewrite a workspace title every turn. The immediate sidebar observation publisher had removeDuplicates() but no burst coalescing, and removeDuplicates() cannot collapse distinct titles. Every rewrite therefore drove a full makeWorkspaceSnapshot() rebuild (git/PR/directory lookups) in each downstream consumer, for every workspace, every turn. With many open workspaces this is a sustained main-thread CPU spike. The publisher now fans out to two consumers on main: the per-row TabItemView subscription and the MergeMany extension-sidebar aggregate. Placing the throttle in the publisher coalesces both. A 50ms leading-edge throttle (latest: true) keeps the first change in a burst instant (user pin/color/title edits still feel immediate) while collapsing the rest to at most one emission per window. Mirrors the existing 40ms debounce on the slower sidebarObservationPublisher. Verified on a tagged build: 60 unique title changes in 0.5s coalesced to 2 snapshot refreshes; 3 edits spaced 250ms apart stayed at 3 (instant single-edit feedback preserved). Refs #4127 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
83a2894 to
3b24768
Compare
![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: 3b2476851f
ℹ️ 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".
| .throttle( | ||
| for: Self.sidebarImmediateObservationCoalesceInterval, | ||
| scheduler: RunLoop.main, | ||
| latest: true | ||
| ) |
There was a problem hiding this comment.
Because this publisher intentionally emits the current @Published values as soon as a sidebar row subscribes, this throttle starts its 50 ms window with that initial snapshot. If the caller then changes an immediate field right after subscribing, as WorkspaceUnitTests does by subscribing and calling manager.setTabColor, the real customColor/title/pin change is no longer delivered immediately but is deferred to the trailing timer, breaking the immediate-invalidation contract and the existing synchronous expectations. Consider excluding the initial current-state emission from the throttle window or coalescing only the burst source that needs throttling.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e68322b, and this was a sharper catch than it first looked: Combine's throttle schedules every emission onto the scheduler, so not just the post-subscribe change but the initial replay itself was deferred to the next run-loop turn, breaking the synchronous contract asserted by sidebarImmediateObservationPublisherEmitsForLateTitleSubscriber and the setTabColor tests. Replaced with coalesceLatest, a custom operator with a synchronous leading edge: the replay forwards synchronously without opening a coalesce window, the first change after idle forwards synchronously, and only the burst tail defers (latest value, once per 50ms window). Contract tests added in 36df8c1 fail on the throttle revision.
…dge contract The replay a late subscriber receives, and the first change after idle, must arrive in the same run-loop turn; only a burst tail may coalesce. These fail on the current throttle-based head: Combine's throttle schedules every emission onto the scheduler, so nothing is synchronous. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…, per workspace and across the extension aggregate Replace Combine's throttle with coalesceLatest, a custom operator whose leading edge is synchronous: throttle schedules every emission onto the scheduler, so the @published replay and the first change after idle were deferred to the next run-loop turn, breaking the immediate-invalidation contract the tests in the previous commit assert. coalesceLatest forwards the replay and any post-idle change in the same run-loop turn and defers only the tail of a burst, emitting the latest value once per 50ms window. Also coalesce across the extension-sidebar MergeMany aggregate: per- workspace coalescing caps each stream, but N workspaces bursting concurrently still re-rendered the whole extension sidebar once per workspace per window. Refs #4127 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit e68322b. Configure here.
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmuxTests/WorkspaceSidebarObservationTests.swift`:
- Around line 139-198: The tests in WorkspaceSidebarObservationTests still rely
on RunLoop.main.run(until:) to trigger the trailing emission, which makes the
assertions depend on real time. Update
sidebarImmediateObservationPublisherCoalescesTitleBursts and
coalesceLatestKeepsLeadingEdgeSynchronousAndEmitsLatestTrailing to use a
controllable test scheduler/manual clock when calling
coalesceLatest(for:scheduler:), then advance time explicitly to verify the
leading and trailing emissions deterministically.
In `@Sources/WorkspaceSidebarObservation.swift`:
- Around line 227-308: The trailing emission logic in CoalesceLatestInner can
emit stale values out of order when a new window starts before the previous
scheduled closure fires. Update receive(_:) and
scheduleTrailingEmission/emitTrailing so that starting a fresh window clears any
pendingValue and invalidates the prior scheduled emission (for example by using
a generation/token or resetting trailingScheduled when windowStart advances).
Ensure the old scheduled callback cannot flush an earlier value after a newer
synchronous send from the same CoalesceLatestInner instance.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4b69e3ea-a2cc-40f0-b001-157862cc4c6c
📒 Files selected for processing (3)
Sources/ContentView.swiftSources/WorkspaceSidebarObservation.swiftcmuxTests/WorkspaceSidebarObservationTests.swift
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues 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="cmuxTests/WorkspaceSidebarObservationTests.swift">
<violation number="1" location="cmuxTests/WorkspaceSidebarObservationTests.swift:159">
P2: This test relies on `RunLoop.main.run(until:)` with a wall-clock delay to let the 50ms trailing emission fire. This makes the assertion timing-dependent — on a slow CI machine or under load the trailing emission might not fire within the pumped interval, or timing jitter could cause flakiness. Since `coalesceLatest` already accepts a generic `Scheduler`, consider driving these tests with a deterministic test scheduler (or `ImmediateScheduler` + manual time advancement) so assertions don't depend on real elapsed time.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…tion Keeps ContentView.swift inside the Swift file length budget and puts the aggregate coalescing next to the operator and interval it uses. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 00ae6fdd37
ℹ️ 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".
| } else { | ||
| windowStart = now | ||
| _ = downstream.receive(input) |
There was a problem hiding this comment.
When the main run loop is busy past the scheduled deadline, the trailing closure may not have run yet; a subsequent input reaches this else path and opens a new window while leaving the old pendingValue and trailingScheduled intact. The delayed old timer can then emit an older pending state after the newer leading value, or emit the new window's pending value immediately instead of at its own deadline, which breaks the latest-only coalescing behavior during long synchronous title/update batches.
Useful? React with 👍 / 👎.
…sion supersedes it Codex autoreview P2: if the trailing callback is delayed past its deadline (main run-loop stall) and a newer value arrives first, the new value took the leading branch but left pendingValue set, so the late callback emitted the stale value out of order. The leading branch now clears the superseded pending value, and an overdue callback firing inside a newer window reschedules to that window's deadline instead of emitting early. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
cmux policy: major added types get their own TypeName.swift file. Wires the new file into the pbxproj alongside WorkspaceSidebarObservation.swift. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
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/CoalesceLatestPublisher.swift">
<violation number="1" location="Sources/CoalesceLatestPublisher.swift:83">
P2: `receive(subscription:)` unconditionally requests `.unlimited` from upstream after calling `downstream.receive(subscription:)`. If the downstream synchronously cancels during that call, the upstream subscription is cancelled and `isCancelled` is set, yet the method still requests demand on the now-cancelled subscription. Adding a `guard !isCancelled else { return }` before `subscription.request(.unlimited)` respects the cancellation and avoids driving unnecessary upstream work.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| self.scheduler = scheduler | ||
| } | ||
|
|
||
| func receive(subscription: Subscription) { |
There was a problem hiding this comment.
P2: receive(subscription:) unconditionally requests .unlimited from upstream after calling downstream.receive(subscription:). If the downstream synchronously cancels during that call, the upstream subscription is cancelled and isCancelled is set, yet the method still requests demand on the now-cancelled subscription. Adding a guard !isCancelled else { return } before subscription.request(.unlimited) respects the cancellation and avoids driving unnecessary upstream work.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/CoalesceLatestPublisher.swift, line 83:
<comment>`receive(subscription:)` unconditionally requests `.unlimited` from upstream after calling `downstream.receive(subscription:)`. If the downstream synchronously cancels during that call, the upstream subscription is cancelled and `isCancelled` is set, yet the method still requests demand on the now-cancelled subscription. Adding a `guard !isCancelled else { return }` before `subscription.request(.unlimited)` respects the cancellation and avoids driving unnecessary upstream work.</comment>
<file context>
@@ -0,0 +1,158 @@
+ self.scheduler = scheduler
+ }
+
+ func receive(subscription: Subscription) {
+ upstreamSubscription = subscription
+ downstream.receive(subscription: self)
</file context>
…l scheduler The test determinism gate rejects sleep-then-assert. coalesceLatest is generic over Scheduler, so drive the stall interleaving exactly: advance now past the deadline without running the scheduled callback, assert the supersede, then run the overdue callback and assert no stale emission. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…gent title bursts (manaflow-ai#6807) * perf(sidebar): leading-edge throttle on immediate observation publisher Agents (e.g. Codex) rewrite a workspace title every turn. The immediate sidebar observation publisher had removeDuplicates() but no burst coalescing, and removeDuplicates() cannot collapse distinct titles. Every rewrite therefore drove a full makeWorkspaceSnapshot() rebuild (git/PR/directory lookups) in each downstream consumer, for every workspace, every turn. With many open workspaces this is a sustained main-thread CPU spike. The publisher now fans out to two consumers on main: the per-row TabItemView subscription and the MergeMany extension-sidebar aggregate. Placing the throttle in the publisher coalesces both. A 50ms leading-edge throttle (latest: true) keeps the first change in a burst instant (user pin/color/title edits still feel immediate) while collapsing the rest to at most one emission per window. Mirrors the existing 40ms debounce on the slower sidebarObservationPublisher. Verified on a tagged build: 60 unique title changes in 0.5s coalesced to 2 snapshot refreshes; 3 edits spaced 250ms apart stayed at 3 (instant single-edit feedback preserved). Refs manaflow-ai#4127 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(sidebar): assert the immediate publisher's synchronous leading-edge contract The replay a late subscriber receives, and the first change after idle, must arrive in the same run-loop turn; only a burst tail may coalesce. These fail on the current throttle-based head: Combine's throttle schedules every emission onto the scheduler, so nothing is synchronous. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * perf(sidebar): synchronous-leading coalesce for immediate observation, per workspace and across the extension aggregate Replace Combine's throttle with coalesceLatest, a custom operator whose leading edge is synchronous: throttle schedules every emission onto the scheduler, so the @published replay and the first change after idle were deferred to the next run-loop turn, breaking the immediate-invalidation contract the tests in the previous commit assert. coalesceLatest forwards the replay and any post-idle change in the same run-loop turn and defers only the tail of a burst, emitting the latest value once per 50ms window. Also coalesce across the extension-sidebar MergeMany aggregate: per- workspace coalescing caps each stream, but N workspaces bursting concurrently still re-rendered the whole extension sidebar once per workspace per window. Refs manaflow-ai#4127 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * sidebar: move merged immediate aggregate into WorkspaceSidebarObservation Keeps ContentView.swift inside the Swift file length budget and puts the aggregate coalescing next to the operator and interval it uses. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * coalesceLatest: drop stale pending value when an overdue leading emission supersedes it Codex autoreview P2: if the trailing callback is delayed past its deadline (main run-loop stall) and a newer value arrives first, the new value took the leading branch but left pendingValue set, so the late callback emitted the stale value out of order. The leading branch now clears the superseded pending value, and an overdue callback firing inside a newer window reschedules to that window's deadline instead of emitting early. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Move coalesceLatest operator into Sources/CoalesceLatestPublisher.swift cmux policy: major added types get their own TypeName.swift file. Wires the new file into the pbxproj alongside WorkspaceSidebarObservation.swift. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Document why CoalesceLatestPublisher and its Inner share one file Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Make the overdue-trailing regression test deterministic with a virtual scheduler The test determinism gate rejects sleep-then-assert. coalesceLatest is generic over Scheduler, so drive the stall interleaving exactly: advance now past the deadline without running the scheduled callback, assert the supersede, then run the overdue callback and assert no stale emission. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> (cherry picked from commit 57070ba)
Backports upstream manaflow-ai#6807 (coalesce observation bursts), manaflow-ai#7117 (sidebar scroll render storm), manaflow-ai#7221 (lazy-layout contract scale gate) plus narrow fork bridges. Fixes the 100%-CPU sidebar livelock hit on 0.64.170.
This branch was successfully deployed
1 participant
What
Coalesces agent title bursts in the immediate sidebar observation stream while keeping the leading edge synchronous, and adds cross-workspace coalescing to the extension-sidebar aggregate.
Why
Agents (e.g. Codex) rewrite a workspace title every turn.
removeDuplicates()cannot collapse distinct titles, so each rewrite drove a fullrefreshWorkspaceSnapshot()rebuild in every subscriber, for every workspace, every turn. With many open workspaces this is a sustained main-thread CPU cost (#4127).The first revision used Combine's
throttle. Codex review caught that this breaks the immediate contract:throttleschedules every emission onto the scheduler, including the first, so the@Publishedcurrent-state replay and the first change after idle both slipped to the next run-loop turn. Three existing unit tests assert that contract synchronously (sidebarImmediateObservationPublisherEmitsForLateTitleSubscriberand the twosetTabColortests inSidebarSelectedWorkspaceColorTests); all three fail underthrottle. CI showed green only because the unit shards did not execute them.How
coalesceLatest, a small custom operator inWorkspaceSidebarObservation.swift, per subscription:@Publishedreplay) forwards synchronously and does not open a coalesce window, so a change made right after subscribing is still synchronous.Applied in the per-workspace immediate publisher and in the extension-sidebar
MergeManyaggregate (ContentView.swift): per-workspace coalescing caps each stream, but N workspaces bursting concurrently still re-rendered the whole extension sidebar once per workspace per window; the aggregate coalesce settles a cross-workspace burst into one re-render per window.Two-commit structure: the first commit adds the contract tests, the second adds the operator and rewiring.
Verification
publishCount 0 > 0); on head, all 8WorkspaceSidebarObservationTestsand all 13SidebarSelectedWorkspaceColorTestspass.sbthr) via the debug socket, countingsidebar.row.invalidate source=immediatefrom the visible row's own subscription: 60 distinct renames spaced ~68ms apart produce 60 synchronous invalidations (leading edge preserved for spaced changes); 60 concurrent renames landing within ~1.3s coalesce to 9 invalidations (vs 60 without coalescing), and the sidebar settles on the final title.Principled, not hacky: the operator makes the intended semantics (synchronous leading edge, deferred burst tail) explicit instead of approximating them with
throttle, and it is deterministically testable. Residual risk: the operator ignores downstream demand and is main-thread only; both are documented and match its only use (sink-driven observation streams onRunLoop.main).Refs #4127
🤖 Generated with Claude Code
Note
Medium Risk
Touches main-thread sidebar invalidation timing for all workspace rows and the extension sidebar; incorrect coalescing could delay or reorder UI updates, though behavior is heavily tested and scoped to sink-style observation streams.
Overview
Adds a custom Combine operator
coalesceLatestthat forwards the first value and the first change after idle synchronously, then collapses rapid follow-ups into one 50ms trailing emission—unlikethrottle, which defers every emission and broke the sidebar’s immediate contract.Per-workspace immediate sidebar observation (
makeSidebarImmediateObservationPublisher) and the extension sidebar aggregate (Workspace.mergedImmediateObservationPublisher, wired fromContentView) both use it so agent title rewrites no longer trigger a full snapshot rebuild per subscriber per turn. The operator handles overdue scheduled callbacks so stale pending values cannot emit out of order after a stalled run loop.Unit tests cover synchronous first change, title burst coalescing, operator semantics, and overdue-trailing edge cases (with a virtual scheduler).
Reviewed by Cursor Bugbot for commit 9de6d4a. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Tests