fix(session-ui): skip redundant diffs when grouping patch files - #46768
Merged
Conversation
Defer per-file view normalization until a group needs it, derive chained group counts from the parsed hunks instead of a second line diff, reuse the cached original view for one-file groups, and count distinct changed paths directly for the patch error subtitle.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new benchmark static server has a confirmed path-escape issue due to path.join with an absolute pathname, and there is also an ineffective test assertion that should be fixed/removed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR optimizes patch grouping/rendering in session-ui to avoid redundant normalization and diff passes when collapsing or chaining patch tool outputs, and adds a manual browser benchmark harness to measure the impact.
Changes:
- Make per-file
ApplyPatchFile.viewlazy/memoized and reuse normalized views for single-file chained groups. - Compute
+/-counts for chained complete groups from parsed hunk metadata rather than re-runningdiffLines. - Avoid re-running grouping logic in the patch tool error subtitle by counting distinct changed paths directly.
- Add a dedicated Playwright/Vite-based manual benchmark fixture under
packages/app/e2e/performance/patch-groups.
File summaries
| File | Description |
|---|---|
| packages/session-ui/src/tools/tool-renderer.tsx | Avoids calling patchFileGroups just to compute a patch file-count subtitle. |
| packages/session-ui/src/components/apply-patch-file.ts | Lazies view normalization and derives chained counts from parsed hunks to reduce redundant work. |
| packages/session-ui/src/components/apply-patch-file.test.ts | Adds coverage for chained/complete patch composition and count derivation behavior. |
| packages/app/e2e/performance/patch-groups/vite.config.ts | Adds a benchmark build config with optional instrumentation/baseline loading. |
| packages/app/e2e/performance/patch-groups/serve.ts | Adds a small Bun static server for the benchmark build output. |
| packages/app/e2e/performance/patch-groups/README.md | Documents how to run the manual benchmark and interpret outputs. |
| packages/app/e2e/performance/patch-groups/playwright.config.ts | Adds a dedicated Playwright config for the benchmark runner. |
| packages/app/e2e/performance/patch-groups/patch-groups.bench.ts | Implements the benchmark scenarios and measurements collection. |
| packages/app/e2e/performance/patch-groups/index.html | Adds the benchmark HTML entrypoint. |
| packages/app/e2e/performance/patch-groups/fixture.tsx | Adds the benchmark fixture mounting production components and exposing grouping metrics. |
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+8
to
+11
| async fetch(request) { | ||
| const pathname = new URL(request.url).pathname | ||
| const file = Bun.file(path.join(directory, pathname === "/" ? "index.html" : pathname)) | ||
| return (await file.exists()) ? new Response(file) : new Response("Not found", { status: 404 }) |
Comment on lines
34
to
+38
| additions: value.additions, | ||
| deletions: value.deletions, | ||
| view: normalize(value), | ||
| get view() { | ||
| return (view ??= normalize(value)) | ||
| }, |
| expect(groups[0]!.views).toHaveLength(1) | ||
| expect(text(groups[0]!.views[0]!, "deletions")).toBe(before) | ||
| expect(text(groups[0]!.views[0]!, "additions")).toBe(after) | ||
| expect(groups[0]!.views).toBe(groups[0]!.views) |
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.
patchFileGroupsruns for every edit/patch tool group in the transcript, including collapsed headers. It parsed each file's patch eagerly, then for complete/chained groups ran a seconddiffLines(before, after)only to get+/-counts, then normalized the composed contents a third time.toolErrorSubtitlealso re-ran the whole grouping just to count paths.ApplyPatchFile.viewa lazy memoized getter so per-file normalization only runs for groups that show individual views (partial/disconnected patches).hunk.additionLines/hunk.deletionLines) instead of a separatediffLinespass.flowchart LR subgraph Before a1[patchFile x N<br/>normalize each] --> a2[group by path] --> a3[diffLines before/after] --> a4[normalize composed] --> a5[views + counts] end subgraph After b1[patchFile x N<br/>view deferred] --> b2[group by path] --> b3[normalize once<br/>or reuse cached view] --> b4[counts from hunks] --> b5[views + counts] endBenchmark
packages/app/e2e/performance/patch-groupsmounts the productionCurrentFileToolGroup/ToolDisplay/Filecomponents in a production Vite build (Chromium via Playwright, 1366x768, fresh context per sample, fonts-ready gate). Each sample: cold collapsed mount through layout, unmount, warm remount, then expandedit.tsuntil the file renderer'sonRenderedfires. Payloads are real Core tool sources with deterministic identifier renames.ToolDisplaypatch tool, 1 fileBaseline =
335e4ca56fproduction modules (same harness), candidate = this branch. n = 20 serial samples per run, order B1 → C1 → B2 → C2, values are median / p95 ms. Machine drift is visible across runs (comparepartial, which this change does not touch), so read adjacent columns.Warm remount (collapsed)
Cold mount (collapsed)
patchFileGroupsalone (warm, collapsed)Mechanism (separate build-only instrumented bundle, calls during cold mount / warm remount)
diffLinesnormalizepatchFileGroupsExpansion (click → file renderer
onRendered) is dominated by the file renderer and highlighting, which this PR does not touch. Run-to-run variance of the same bundle (chained: C1 271.9 vs C2 223.9; baselines 240.5 / 247.2) exceeds any between-build difference, so no change is claimed there.Rendering parity
Collapsed screenshots (including
+/-counts) are byte-identical between builds. Expanded four-file case, before / after (only the fixture's own timing readout differs):Limitations: browser component benchmark on one Windows machine; not a desktop process memory measurement. The harness is manual (not part of normal test discovery) and enforces completion, not thresholds.