Custom sidebar: instant toggle remount and live resize repaint - #5864
Merged
Merged
Conversation
Park the warm RenderWorkerClient across unmounts (pool of one) instead of shutting the worker down, so reopening the sidebar adopts the cached remote context synchronously. Republish rootView on geometry changes in the worker: without a display link in the never-ordered window, a frame change alone never repainted until the next 1s scene tick. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
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:
📝 WalkthroughWalkthroughRenderWorkerClient ownership moved to window/root-level state and passed as a Binding into RemoteCustomSidebarHost; RemoteCustomSidebarHost uses a .task(id: sourceKey) lifecycle; RenderWorkerCoordinator republished the SwiftUI rootView on resize; tests poll for context reset and recovery. ChangesClient lifecycle ownership refactoring
Sequence Diagram(s)sequenceDiagram
participant ComponentA
participant ComponentB
ComponentA->>ComponentB: observable interaction
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (3 errors, 1 warning, 1 inconclusive)
✅ Passed checks (16 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 two sidebar latency issues by lifting
Confidence Score: 5/5Safe to merge after dogfood approval; the two latency fixes are well-scoped and the worker lifecycle is correctly handled across all paths (toggle, file switch, window close, window dealloc while hidden). The binding-based ownership transfer is structurally clean: all assignments in the task body are synchronous before the only suspension point, so no partial-shutdown race is possible on cancellation. The displayedState mirror is kept in sync with every rootView assignment in refresh(), and the nil→.missing fallback on the first geometry replay before any scene is processed is harmless because the scene arrives in the same pipe burst and overwrites it before the host adopts the context. The single finding is a test-only nit about not checking task cancellation after a sleep in the polling helper. No files require special attention; the test helper in RenderWorkerClientTests.swift has a minor cancellation-handling gap but it is test-only scaffolding. Important Files Changed
Sequence DiagramsequenceDiagram
participant CV as ContentView (@State client)
participant Host as RemoteCustomSidebarHost (@Binding client)
participant Client as RenderWorkerClient (actor)
participant Worker as Render Worker (subprocess)
participant Coord as RenderWorkerCoordinator (@MainActor)
Note over CV,Host: First sidebar mount (client is nil)
Host->>Host: task(id: sourceKey) fires
Host->>Client: reexecingCurrentBinary(sourceKey:)
CV-->>CV: "@State sidebarRenderWorkerClient set"
Note over Client,Worker: Lazy worker spawn on first send
Host->>Client: updateScene / resize
Client->>Worker: launch() — replay lastGeometry + lastScene
Worker->>Coord: handle(.resize) → apply(geometry:)
Coord->>Coord: "hosting.rootView = currentContent(state: displayedState)"
Coord->>Coord: pump()
Worker->>Coord: handle(.scene) → refresh()
Coord->>Coord: "displayedState = displayState"
Coord->>Coord: "hosting.rootView = currentContent(state:)"
Coord->>Coord: pump()
Worker-->>Client: .context(contextId)
Client-->>Host: subscribe() stream → .context
Note over Host,CV: Sidebar hidden (toggle off)
Host->>Host: "task cancelled — client stays in @Binding"
Note over Client,Worker: Worker remains warm
Note over Host,CV: Sidebar shown again (toggle on)
Host->>Host: task(id: sourceKey) fires
Host->>Host: "guard client?.sourceKey == sourceKey → return (no-op)"
Host->>Client: subscribe() — gets cached contextId immediately
Note over CV,Worker: Window close
CV->>Client: shutdown() via window-close reaper
Client->>Worker: terminate()
Reviews (5): Last reviewed commit: "Remove render worker recovery test race" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Replace the global RenderWorkerClientPool with per-window ownership: the client lives as @State on ContentView (the per-window root) and threads down to RemoteCustomSidebarHost as a binding. Still created lazily on the first custom-sidebar mount; survives sidebar toggles and provider switches; reaped by the surface's window-close observer, with worker pipe-EOF exit as the backstop when the window deallocates while hidden. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![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
`@Packages/CmuxSidebarInterpreterService/Sources/CmuxSidebarRemoteRender/RemoteCustomSidebarHost.swift`:
- Around line 24-25: The cached RenderWorkerClient binding in
RemoteCustomSidebarHost (the `@Binding` private var client: RenderWorkerClient?)
is being reused across provider/fileURL changes and can serve stale snapshots;
update the logic to scope or key the cached client by the sidebar source (e.g.,
fileURL or provider identifier) or explicitly reset/invalidate the cached render
when the provider or fileURL changes: add a source key (or compare current
provider id/fileURL) and recreate or nil-out client and any cached remote
snapshot when that key differs (apply the same fix to other cached usages
referenced in the comment such as the other client bindings/usages), ensuring
any authoritative read replaces the cached state per
cache-substitution-correctness.md.
In `@Sources/ContentView.swift`:
- Around line 1086-1092: The PR text claiming sidebarRenderWorkerClient will
"linger until app exit" is inaccurate: clarify that RemoteSidebarSurfaceView
(not ContentView) wires window-close shutdown by observing
NSWindow.willCloseNotification in viewDidMoveToWindow and calling Task { await
client.shutdown() }, and when the sidebar is unmounted teardown() only removes
that observer (relying on pipe EOF on window dealloc to stop the worker); update
the PR objective/description to reflect this actual mechanism (referencing
sidebarRenderWorkerClient, RemoteSidebarSurfaceView, viewDidMoveToWindow,
teardown(), and shutdown()) unless you have a reproducible case where the window
fails to deallocate on close.
🪄 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: 589acdb3-e6d6-4f1a-8b3d-b66897ef9f8b
📒 Files selected for processing (2)
Packages/CmuxSidebarInterpreterService/Sources/CmuxSidebarRemoteRender/RemoteCustomSidebarHost.swiftSources/ContentView.swift
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.
Fixes two latency issues in the out-of-process custom sidebar, found while dogfooding the interpreter work.
Toggle blank (~1s).
RemoteCustomSidebarHostshut the render worker down on every unmount and respawned it on remount, paying process spawn + first interpret + render before anything appeared. The client is now owned by the window:@StateonContentView(per-window root), threaded down to the host as aBinding. It is still created lazily on the first custom-sidebar mount (never eagerly per init, per the #5372/#5382 re-land lesson), survives sidebar toggles and provider switches, and a remount adopts the cached remote context synchronously, so the sidebar reappears instantly. No shared/global state.Resize lag (1-2s). The worker's geometry handler resized the offscreen window and hosting view but never republished
rootView. With no display link in the never-ordered window, a frame change alone never re-rendered the SwiftUI content, so resizes showed stretched stale pixels until the next 1s scene tick repainted.apply(geometry:)now republishes the cached render (no re-interpretation) and pumps, following the coordinator's existing "mutate rootView, force layout, commit" rendering rule.Worker lifecycle: the surface's window-close reaper shuts the worker down when the sidebar is mounted at close; if the window deallocates while the sidebar is hidden, the worker exits on pipe EOF (
RunSidebarRenderWorkerEOF path).Dogfooding on tag
snappy; not for merge until dogfood approval.🤖 Generated with Claude Code
Summary by CodeRabbit
Refactor
Tests