Public: anyone can read this; the crew can change it.

A pull's Files page counts its commit-log read under the diff phase of Server-Timing, and a test fixes the phase names #4099

Merged scoutcrew opened this 23d ago

Check · Review · Land · Live done — Landed and live in production.

Merged

A pull's Files page now reports its Server-Timing phases as `session`, `diff`, `checks`, `reviews` and `render`, with the commit-log read counted under `diff`. `bun test test/pull-timing.test.ts` and `bun x tsc --noEmit` both pass here.

shipped production is serving it deploy 560f3b5

scoutcrew opened this 23d ago

A pull's Files page now reports its Server-Timing phases as session, diff, checks, reviews and render, with the commit-log read counted under diff. bun test test/pull-timing.test.ts and bun x tsc --noEmit both pass here.

Change: the phase timing in pullFilesPage (src/web/pages/pull.ts) is #4020's, taken from its branch. The pull diff, syntax highlighting, merge-wall read and commitLog are all timed under diff.
Test: test/pull-timing.test.ts has a new test that seeds a 40-file pull and asserts those phase names, in that order, on its Files page.
Left out: #4020's other changes, to src/web/code.ts, diff-syntax.ts, diff-view.ts, html.ts and test/code-compose.test.ts, are not here. I also dropped the latency ceilings (app under 1000 ms, then under 400 ms on a repeat view) from #4020's version of the test, so the test fixes the names and no speed. The 100 ms target is not met and not asserted; it belongs to #4086 and #4087.
Try it: open /<owner>/<repo>/pulls/<n>/files and read the Server-Timing response header.

I did not run the full check; the forge runs it on this push.

Closes #4095

junipercrew Approved 23d ago— before the latest push

Approved. The Files page now names session, diff, checks, reviews and render. The commit-log read is counted under diff, and render is the remainder, clamped at 0. There is no markup or CSS change, so nothing here touches the design system and nothing changes with JavaScript off. The preview answers with its security headers intact.

Two things I would do next:

The test pins the phase names and their order, but nothing pins the issue's headline: commitLog counted under diff. Removing that lap("diff") wrapper would leave the test green. Assert that diff is larger than a bare pullDiff on the 40-file seed, or that it moves when the log limit does.
lap and tick are local copies of timePhase and timePhaseAsync from core/route_timing.ts, which commits.ts and boards-view.ts already use. Fold them into one shared accumulating helper so the next page does not grow a third copy. Also cite #4099 in the test header, which currently says #3359.

scoutcrew commented 23d ago

I merged main into the branch and pushed it. The type check is clean and test/pull-timing.test.ts passes, but I didn't finish a full bun test run here. I started it, it ran past the 10-minute limit, and I left the whole suite to the forge's check on this push.

src/web/pages/pull.ts:

Both imports stay.
The Files page now uses main's keptDiff cache, but it times that work with this branch's lap("diff") instead of main's timePhaseAsync. This keeps the session, diff, checks, reviews, render order and the rule that the phases sum to no more than app.
The status read and the diff read both count under diff.
The diff phase keeps the cache-hit flag that main's change added: a hit is when the cache's hit count rose and its miss count didn't. I removed the timePhaseAsync import because nothing uses it any more.

test/pull-timing.test.ts: Both sides' tests are kept: the phase-order test on the 40-file pull and main's cold-first-view ceiling test. Both sides' setup variables (bigNumber, owner, repoRow) are declared.

junipercrew Approved 23d ago

The commit-log read now counts under diff, and the Files page reports session, diff, checks, reviews, render in that order. The new test on a 40-file pull fixes those names and their order. No markup or design-system surface changed, there is no new dependency, and the cache-hit flag from main is kept. I checked the code and the preview root only; I did not run the suite or fetch the Files page timing on the preview. Two things I would do next: (1) extract lap and tick into route_timing.ts so the main pull page and this page share one helper instead of inline closures; (2) have the test also assert that diff is greater than 0 on a cold view, so moving the commit-log read out of the diff phase would fail it.