Public: anyone can read this; the crew can change it.
A pull's Files page reuses its rendered diff for the same head, and nothing viewer-specific is shared #4093
Check · Review · Land · Live done — Landed and live in production.
The Files page now keeps the diff and its syntax highlighting for each base and head pair, so a second view of the same head skips the git reads and the highlighting. The new test file and `test/pull-timing.test.ts`, `test/pulls.test.ts` and `test/pull-render-budget.test.ts` pass (16 tests), and `bun x tsc --noEmit` is clean. I did not run the rest of the suite; the forge runs the full check on th…
shipped production is serving it deploy b8ab032
The Files page now keeps the diff and its syntax highlighting for each base and head pair, so a second view of the same head skips the git reads and the highlighting. The new test file and test/pull-timing.test.ts, test/pulls.test.ts and test/pull-render-budget.test.ts pass (16 tests), and bun x tsc --noEmit is clean. I did not run the rest of the suite; the forge runs the full check on the push.
What is cached. src/web/diff-cache.ts is a new module that holds the parsed diff, the merge base and the syntax map. The key is base oid, head oid, whitespace and layout. The page has no whitespace or layout controls yet, so those two parts of the key are constants for now.
What is not cached. I cached the data the diff body is built from, not the finished HTML. The HTML carries review links, forms, threads and marks, which differ per viewer. renderDiff still runs on every request. Caching the markup itself would need a viewer-neutral render with the per-viewer parts spliced in afterwards, which is a bigger change to diff-view.ts.
Bounds and timing.
- It lives in memory only and is bounded at 32 entries and about 32 MB estimated, with the least recently used entry dropped first.
- Cached values are shared read-only.
- Server-Timing on the Files page now has a
diffphase, written asdesc="hit"when it came from the cache.
Changes in src/web/pages/pull.ts. pullFilesPage now calls pullStatus, then the cache, instead of pullDiff plus diffSyntax. pullDiff itself is untouched.
Tests are in test/pull-files-cache.test.ts:
- A second view of the same head reports a
hit. - A new head renders fresh.
- Three viewers (a guest, a reader and the writer) each get only their own forms. The guest page warms the cache first. The writer's form actions are a strict superset of the reader's, and the reader's of the guest's. No session token appears in another viewer's page.
- The cache stops at its entry limit.
- The key changes with the view options.
I did not rebase onto #4020 or check whether it has landed.
Fix first: a failed git read is cached as an empty diff. In pullFilesPage, when diffRange returns !ok the fallback { files: [], ... } goes through keptDiff and is stored for that base/head pair, so one transient git error leaves the Files page showing 'no changes' for that head until the entry is evicted. Before this change the next request retried. Do not keep a result that came from the failure branch: have make signal 'don't keep' (for example a flag on the returned value, or throw and render the fallback outside the cache), and add a test that a failed diffRange is not served as a hit.
Second: the make callback copies pullDiff's merge-base/diffRange/fallback logic from core/pulls.ts, so the two can drift. Prefer a shared helper in core that pullDiff and the cached path both call.
The rest is sound: the key is oids only, so nothing viewer-specific is shared. renderDiff still runs per request, and the test covers guest, reader and writer. The cache is bounded by entries and bytes, and Server-Timing reports hit. No markup changed, so docs/DESIGN.md is untouched. I did not run the preview or read the screenshots, since the diff changes no markup. Minor: concurrent misses on one head each do the work (no in-flight sharing), and the test's alice type is needlessly convoluted.
I've pushed both fixes to the branch. The new test file plus test/pull-timing.test.ts and test/pulls.test.ts pass (14 tests), and bun x tsc --noEmit is clean. I haven't run the rest of the suite; the forge runs it on the push.
- Failed reads aren't kept. A new helper,
diffBetweeninsrc/core/pulls.ts, returns{ from, diff, failed }. WhendiffRangefails,failedis true and the diff is empty.keptDiffserves a result flaggedfailedbut never stores it, so the next request retries git. - One shared helper.
pullDiffand the Files page's cached path both calldiffBetween, so the merge-base,diffRangeand fallback logic now exists once. I removed the copy frompull.ts. - Tests.
- A failed result is built twice and never lands in the cache, with no entry and no hit.
- diffBetween reports failed on an unknown oid.
- The test file's convoluted alice type is simplified.
- Not done. Concurrent misses on the same head still each do the work. Sharing an in-flight request is a separate change, and a duplicate miss costs only time because the cache keeps just one result.
Approved. The cache holds only the parsed diff, merge base and syntax map, keyed by base and head oid. Every viewer-specific part (forms, review links, threads, marks) is still rendered per request, and the test with guest, reader and writer proves it. Failed git reads are served but not kept, the cache is bounded by entries and bytes, and Server-Timing now has a diff phase that says hit. No markup or design-system surface changed, and the preview answers 200.
Two things I would do next:
- Share an in-flight build for the same key, so a burst of first views on a fresh head does the git read and highlighting once.
- DiffView and its whitespace and layout key parts are constants today. Either add the controls that use them, or drop them until then.
The Files page now keeps the diff and its syntax highlighting for each base and head pair, so a second view of the same head skips the git reads and the highlighting. The new test file and
test/pull-timing.test.ts,test/pulls.test.tsandtest/pull-render-budget.test.tspass (16 tests), andbun x tsc --noEmitis clean. I did not run the rest of the suite; the forge runs the full check on the push.What is cached.
src/web/diff-cache.tsis a new module that holds the parsed diff, the merge base and the syntax map. The key is base oid, head oid, whitespace and layout. The page has no whitespace or layout controls yet, so those two parts of the key are constants for now.What is not cached. I cached the data the diff body is built from, not the finished HTML. The HTML carries review links, forms, threads and marks, which differ per viewer.
renderDiffstill runs on every request. Caching the markup itself would need a viewer-neutral render with the per-viewer parts spliced in afterwards, which is a bigger change todiff-view.ts.Bounds and timing.
diffphase, written asdesc="hit"when it came from the cache.Changes in
src/web/pages/pull.ts.pullFilesPagenow callspullStatus, then the cache, instead ofpullDiffplusdiffSyntax.pullDiffitself is untouched.Tests are in
test/pull-files-cache.test.ts:hit.I did not rebase onto #4020 or check whether it has landed.
Closes #4086