fix(ui): prepare diffs off the render thread#31309
Open
Hona wants to merge 1 commit into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR moves expensive diff parsing/preparation off the UI render thread by introducing a dedicated diff-preparation Web Worker + client-side resource wrapper, and updates multiple UI surfaces (session review, turn diffs, edit/apply-patch tools, timeline) to render only already-prepared diff state.
Changes:
- Added a diff preparation worker/client with request/response protocol, caching, and Solid resource helpers.
- Refactored diff-rendering call sites to depend on
createPreparedDiff()output instead of running diff algorithms during reactive rendering. - Added tests to enforce parser isolation and a Playwright smoke/perf test that validates renderer responsiveness on a pathological diff.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ui/src/diff/worker.ts | Worker entrypoint that prepares diffs and posts results back to the main thread. |
| packages/ui/src/diff/resource.ts | Solid helpers to snapshot diff inputs and fetch prepared diffs (via worker). |
| packages/ui/src/diff/render-purity.test.ts | Test to ensure browser diff parsers aren’t imported by render components. |
| packages/ui/src/diff/protocol.ts | Types for worker request/response messages. |
| packages/ui/src/diff/client.ts | Lazily-instantiated worker client wrapper. |
| packages/ui/src/diff/client.test.ts | Unit tests for diff preparation dedupe, snapshotting, and failure handling. |
| packages/ui/src/diff/client-core.ts | Core diff preparation client: caching/dedup + worker message lifecycle. |
| packages/ui/src/components/session-turn.tsx | Session turn diff rendering updated to use prepared diffs. |
| packages/ui/src/components/session-review.tsx | Session review updated to prepare diffs lazily and render only prepared state. |
| packages/ui/src/components/session-diff.ts | Extracted prepareDiff result shape and removed patch diff cache from render path. |
| packages/ui/src/components/message-part.tsx | Edit/apply-patch tool diffs updated to render prepared diffs instead of parsing during render. |
| packages/ui/src/components/apply-patch-file.ts | Apply-patch file model now carries a DiffSource instead of a precomputed view diff. |
| packages/ui/src/components/apply-patch-file.test.ts | Tests updated to assert metadata adaptation without preparing diffs. |
| packages/ui/package.json | Export map extended to expose diff/* entrypoints. |
| packages/app/src/pages/session/message-timeline.tsx | Timeline diff view updated to use prepared diffs. |
| packages/app/e2e/utils/mock-server.ts | Mock server enhanced to serve custom /vcs/diff payloads for tests. |
| packages/app/e2e/smoke/session-review-performance.spec.ts | New Playwright perf test measuring frame gaps while preparing a pathological review diff. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+75
to
+79
| function sourceKey(source: DiffSource) { | ||
| if (source.patch !== undefined) return `${source.file.length}:${source.file}p${source.patch.length}:${source.patch}` | ||
| const before = source.before ?? "" | ||
| const after = source.after ?? "" | ||
| return `${source.file.length}:${source.file}c${before.length}:${before}${after.length}:${after}` |
Comment on lines
+4
to
+12
| self.addEventListener("message", (event: MessageEvent<DiffWorkerRequest>) => { | ||
| try { | ||
| self.postMessage({ id: event.data.id, result: prepareDiff(event.data.source) } satisfies DiffWorkerResponse) | ||
| } catch (error) { | ||
| self.postMessage({ | ||
| id: event.data.id, | ||
| error: error instanceof Error ? error.message : String(error), | ||
| } satisfies DiffWorkerResponse) | ||
| } |
Comment on lines
+1
to
+15
| import { createMemo, createResource, type Accessor } from "solid-js" | ||
| import type { DiffSource } from "../components/session-diff" | ||
| import { prepareDiff } from "./client" | ||
| import { snapshotSource } from "./client-core" | ||
|
|
||
| export function createPreparedDiff(source: Accessor<DiffSource | undefined>) { | ||
| const snapshot = createDiffSource(source) | ||
| if (typeof Worker === "undefined") return [() => undefined] as const | ||
| return createResource(snapshot, (value) => | ||
| prepareDiff(value).catch((error) => { | ||
| console.error("failed to prepare diff", error) | ||
| return undefined | ||
| }), | ||
| ) | ||
| } |
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
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.
TLDR;
Problem
Two Windows user debug exports repeatedly sampled the renderer inside this synchronous path:
Session Review eagerly parsed every file while building reactive component state, before expansion or Pierre virtualization. A sufficiently difficult line diff could therefore block the renderer for seconds while messages and parts continued streaming.
Changelog
Fixed
Improved
Performance
A Playwright smoke test reproduces the user stack with a full-context replacement containing 1,536 unique old lines and 1,536 unique new lines. It opens the real Review panel, expands the file, samples
requestAnimationFrame, and waits for the actual Pierre viewer.Five isolated Chromium runs per implementation:
upstream/devmedianBaseline maximum frame gaps:
This PR:
The committed 200 ms responsiveness budget fails 5/5 baseline measurements and passes 5/5 worker measurements. A final repeated worker run measured
66.6, 66.6, 66.6, 66.7, 66.7 ms.Verification