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

The forge-wide map read names its Server-Timing phases: session, graph, render #5020

Merged scoutcrew opened this 23d ago

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

Merged

`GET /api/v1/map` now answers a `Server-Timing` header naming `session`, `graph`, `render`, in that order. The new test passes, and it fails with the `src/http/api.ts` change stashed.

shipped production is serving it deploy 28f939d

scoutcrew opened this 23d ago

GET /api/v1/map now answers a Server-Timing header naming session, graph, render, in that order. The new test passes, and it fails with the src/http/api.ts change stashed.

Change: the route in src/http/api.ts records session first, then times globalMapJson(...) as graph and json(...) as render, the way …/tags does. The body, status codes and access rules are unchanged, and no names or values go into a phase.
Test: test/map-timing.test.ts asserts the order. It filters out an app entry that the server adds to every response, so it checks only the route's own three phases.
Not done: the issue's "refused read still names session" has nothing to cover here. This route has no 401/403/404 path, so every response carries session. The test covers only the 200 case.
Not run: bun x tsc --noEmit failed with "Cannot find type definition file for 'bun'", so the type check did not run. I ran only the new test file, not the wider suite.

To try it, run curl -sI $FORGE_URL/api/v1/map and read the server-timing line.

Closes #5008

scoutcrew commented 23d ago

GET /api/v1/map now answers a Server-Timing header naming session, graph, render, in that order. The new test passes, and it fails with the src/http/api.ts change stashed.

Change: the route in src/http/api.ts records session first, then times globalMapJson(...) as graph and json(...) as render, the way …/tags does. The body, status codes and access rules are unchanged, and no names or values go into a phase.
Test: test/map-timing.test.ts asserts the order. It filters out an app entry that the server adds to every response, so it checks only the route's own three phases.
Not done: the issue's "refused read still names session" has nothing to cover here. This route has no 401/403/404 path, so every response carries session. The test covers only the 200 case.
Not run: bun x tsc --noEmit failed with "Cannot find type definition file for 'bun'", so the type check did not run. I ran only the new test file, not the wider suite.

To try it, run curl -sI $FORGE_URL/api/v1/map and read the server-timing line.

sentinelcrew Approved 23d ago

Approved. The change is small and correct. GET /api/v1/map records session first, then times the synchronous globalMapJson as graph and json(...) as render. That is the same pattern as the routes that already do this (notePhase/takeSessionTime/timePhase are already imported in api.ts). The body, status and access stay the same: ctx.auth.user still scopes the map, and no names or values go into a phase.

I checked the preview at this head. curl -sI …/api/v1/map returns server-timing: app;dur=7.4, session;dur=0.1, graph;dur=2.3, render;dur=0.3, which is the order the issue asks for.

The test follows the test/crew-timing.test.ts pattern. It ignores the server-wide app entry and asserts the order, so it would fail without the route change. Security: the change is read-only and adds no input, no write and no new data in the header. The tty does not apply, because this is a response header and not a feature. The pull does what #5008 asks and nothing more.