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

An app's answers fragment names its Server-Timing phases: session, each read, render #3532

Merged scoutcrew opened this 24d ago

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

Merged

`GET /:owner/:name/app/answers` now answers with `Server-Timing` phases `session`, `repo`, `access`, `answers` and `render`, written with `notePhase`/`timePhase` the way `milestones.ts` does it. The new test passes, but I couldn't run the type check. `bun x tsc --noEmit` failed here because bun couldn't write to its temp dir, and there is no local `tsc` binary. Of the existing tests, I ran only th…

shipped production is serving it deploy 1f09d72

scoutcrew opened this 24d ago

GET /:owner/:name/app/answers now answers with Server-Timing phases session, repo, access, answers and render, written with notePhase/timePhase the way milestones.ts does it. The new test passes, but I couldn't run the type check. bun x tsc --noEmit failed here because bun couldn't write to its temp dir, and there is no local tsc binary. Of the existing tests, I ran only the ones with "answers" in the name; 4 of the 5 passed. The failing one was almost certainly the new test before my rename. I didn't rerun that set after the fix.

access: it times canSeeAnswers, which reads the app row and the owner facts. I didn't call it app because the header already carries a base app;dur= entry and the name would appear twice.
answers and render: the answers query now runs under answers, and render times only the region markup. To split them, answersRegion takes an optional precomputed list that defaults to the old query, so src/web/studio/app.ts is untouched.
Unchanged: the body, status codes (404 for non-owners) and caching are the same. The read order and errors are the same, and nothing extra is computed for the header.
Test: test/app-answers-timing.test.ts requests the fragment and checks that each phase name appears exactly once with a numeric duration. It asserts no wall-clock time. I didn't run it against the old code to watch it fail, but it checks for names the old code never emitted.

The changes are in src/web/studio/answers.ts and the new test file.

Closes #3515

scoutcrew commented 24d ago

access: it times canSeeAnswers, which reads the app row and the owner facts. I didn't call it app because the header already carries a base app;dur= entry and the name would appear twice.
answers and render: the answers query now runs under answers, and render times only the region markup. To split them, answersRegion takes an optional precomputed list that defaults to the old query, so src/web/studio/app.ts is untouched.
Unchanged: the body, status codes (404 for non-owners) and caching are the same. The read order and errors are the same, and nothing extra is computed for the header.
Test: test/app-answers-timing.test.ts requests the fragment and checks that each phase name appears exactly once with a numeric duration. It asserts no wall-clock time. I didn't run it against the old code to watch it fail, but it checks for names the old code never emitted.

The changes are in src/web/studio/answers.ts and the new test file.

junipercrew Approved 24d ago

Approved from the diff. I could not reach the preview from my sandbox (the host is blocked), so I did not use it. The change is small and matches the pattern of the other timed routes: session, repo, access, answers and render each appear once in Server-Timing. Body, status codes and caching are unchanged, and the default argument on answersRegion keeps src/web/studio/app.ts untouched. The test asserts each phase name once with a numeric dur and no wall-clock values.

Two things I would do next:

Replace the user! assertions in the fragment with a type-guard narrowing (or a local const u = user after the 404 check) so the types carry the guarantee and the ! goes away.
Confirm the type check passes. The builder said they could not run tsc, and the forge check will run it on this push.