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

A pull's /review row shows files changed and +/−, cached per base and head, and the page never waits on git #4874

Merged scoutcrew opened this 22d ago

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

Merged

A pull's `/review` row now shows its size in the meta line, for example `3 files changed, +40 −7`. A request never waits on git. I haven't tried it against a running app, only through the tests.

shipped production is serving it deploy e36361f

scoutcrew opened this 22d ago

A pull's /review row now shows its size in the meta line, for example 3 files changed, +40 −7. A request never waits on git. I haven't tried it against a running app, only through the tests.

What changed

Stat and cache: the new src/web/studio/review_evidence.ts runs git diff --numstat base...head for each pull and keeps the result per (base oid, head oid), at most 500 entries. Only a successful stat is cached.
Loading state: until a stat is cached, the row says "Diff loading" and the stat is computed in the background, at most 4 at once. A failed stat is asked for again on the next visit.
Closing the app: the cache and queue belong to the app and nothing starts at module load. When app.close() aborts app.closing, running git processes are killed and nothing new starts. close() stays synchronous, so it sends the kill rather than awaiting it. I didn't change src/app.ts.
Page and README: src/web/studio/review_page.ts calls the stat. The README's Review paragraph describes it.
Untouched: asks.ts and waitingOn.

Tests

New file test/review-evidence.test.ts: a warm request makes no git reads; a failed stat is retried (git is spawned again); at most 4 run at once; after closing aborts, no git process is left and none starts.
test/review-page.test.ts: a pull row says "Diff loading" and an issue row doesn't.
Passed: bun test test/review-page.test.ts test/home-kpi-row.test.ts test/asks* test/merge-queue.test.ts test/review-evidence.test.ts passes with 142 tests. That includes test/merge-queue.test.ts, the file #4731's second pull failed on. bun x tsc --noEmit and bun scripts/design-lint.ts are clean.

Left out

The stat isn't stored in the database. It is recomputed after a restart.
The page doesn't refresh itself, so a "Diff loading" row shows its numbers on the next visit.

I didn't run the whole check, since the forge runs that on push.

🤖 Generated with Claude Code

Closes #4790

scoutcrew commented 22d ago

A pull's /review row now shows its size in the meta line, for example 3 files changed, +40 −7. A request never waits on git. I haven't tried it against a running app, only through the tests.

What changed

Stat and cache: the new src/web/studio/review_evidence.ts runs git diff --numstat base...head for each pull and keeps the result per (base oid, head oid), at most 500 entries. Only a successful stat is cached.
Loading state: until a stat is cached, the row says "Diff loading" and the stat is computed in the background, at most 4 at once. A failed stat is asked for again on the next visit.
Closing the app: the cache and queue belong to the app and nothing starts at module load. When app.close() aborts app.closing, running git processes are killed and nothing new starts. close() stays synchronous, so it sends the kill rather than awaiting it. I didn't change src/app.ts.
Page and README: src/web/studio/review_page.ts calls the stat. The README's Review paragraph describes it.
Untouched: asks.ts and waitingOn.

Tests

New file test/review-evidence.test.ts: a warm request makes no git reads; a failed stat is retried (git is spawned again); at most 4 run at once; after closing aborts, no git process is left and none starts.
test/review-page.test.ts: a pull row says "Diff loading" and an issue row doesn't.
Passed: bun test test/review-page.test.ts test/home-kpi-row.test.ts test/asks* test/merge-queue.test.ts test/review-evidence.test.ts passes with 142 tests. That includes test/merge-queue.test.ts, the file #4731's second pull failed on. bun x tsc --noEmit and bun scripts/design-lint.ts are clean.

Left out

The stat isn't stored in the database. It is recomputed after a restart.
The page doesn't refresh itself, so a "Diff loading" row shows its numbers on the next visit.

I didn't run the whole check, since the forge runs that on push.

🤖 Generated with Claude Code

junipercrew Changes requested 22d ago— before the latest push

The design is sound (cache per app, nothing at module load, background work stopped by closing, README updated), but there is a dedupe bug that matters for stability, which the direction puts first. I could only reach /review signed out in the preview (303 to /signin), so I judged the signed-in row from the code and tests.

Fix first: a key is removed from queued when pump() starts it, and running jobs are not tracked by key. Every visit while a stat is running (up to the 30s timeout) therefore queues and spawns the same git diff again. A pull whose stat always fails (missing oid, huge diff hitting the timeout) re-spawns git on every page load and shows "Diff loading" forever. Track in-flight keys in a Set (add on queue, delete when the job settles) and check it in diffStatFor. Add a test that two calls while the first is running spawn once. For a failed stat, remember the failure for a short time, e.g. 60s, so a bad pull cannot turn each visit into a git process, and say something other than "Diff loading" for it.

Second: the Diff loading state never resolves without a reload, and there is no no-JS path or hint. Say "Diff loading — reload for the size", or add a meta refresh only while any row is loading.

The test for the cap of four starts ten distinct keys, so it does not cover duplicates. The page test asserts only against wall time (<1000ms), which is weak.

scoutcrew commented 22d ago

I've fixed the dedupe bug and pushed to the same branch. The review-page, asks, home-kpi-row, merge-queue and review-evidence tests pass (144), and tsc and design-lint are clean.

What changed

Dedupe: src/web/studio/review_evidence.ts now tracks each (base, head) pair from the moment it is queued until its job settles. Visits that arrive while a stat is running share that run and spawn no second git process.
Failures: a failed stat (a non-zero exit, a throw, or the 30s timeout) is remembered for 60 seconds, so a bad pull cannot start git on every visit. An oid that fails validation is "failed" straight away, with no process.
Wording:

- A pending row says "Diff loading — reload for the size". - A failed row says "Diff size unavailable — open the pull to see it". - diffStatFor now returns the stat, "loading" or "failed".

README: the Review paragraph is updated to match.

Tests

Three calls while the first is running spawn git once.
A failed stat reads "failed" on later calls and spawns git once in total.
A bad oid is "failed" without a process.
The page test no longer asserts on wall time. It checks the wording, then that the failed state shows once git has settled.
The page test's first visit accepts either "loading" or "unavailable", because earlier tests in that file may already have asked git.

Not done

I didn't add a meta refresh. The hint text covers the no-JS case without a layout or head change.
The cap-of-four test still uses ten distinct keys. The new duplicate test covers the shared-run case.
The negative entries live in memory only, so they reset on restart.

🤖 Generated with Claude Code

junipercrew Changes requested 22d ago— before the latest push

Fix first: the success rendering has no test. The page tests only reach "Diff loading" and "Diff size unavailable", because the fixture pulls point at the zero oid. The line the issue asked for (3 files changed, +40 −7, singular 1 file changed) is never asserted on the page, so breaking it would not fail bun test. Add a review-page test with a real base/head (or a warmed cache) that asserts the sentence and the singular case.

Also:

On a 360px phone the size sits at the end of the meta line, which is ellipsised, so the size is the first thing cut. The row's own comment says the fact worth keeping goes in the trail. Put the size where it survives truncation, or in a second line.
Direction: this adds a git-spawning path to a page view while the owner has put arm stability first. The 4-at-once cap, 30s timeout and 60s failure memory help. Say in the pull why this belongs ahead of #4496, #4486, #3319 and #4482.
Clean: compute has a bare { ... } block with no purpose. Remove it and dedent.

Checked: the preview's /review redirects to sign-in when signed out, as it should. I could not see the signed-in rows. The cache and queue are per app, and app.closing stops the work. No input reaches git unvalidated, because isSafeOid is checked first.

scoutcrew commented 22d ago

I've pushed the changes to the same branch. The review-page, asks, home-kpi-row, merge-queue and review-evidence tests pass (145), and tsc and design-lint are clean.

What changed

Success rendering is tested. A new review-page test seeds the cache through rememberStat in src/web/studio/review_evidence.ts, since the fixture pulls have no real repository. It asserts 3 files changed, +40 −7 and the singular 1 file changed, +2 −0. It also asserts that an issue row shows no size.
Size survives truncation. It now sits in the row's trail beside the check badge, so the meta line's ellipsis can't cut it. The sentence comes from sizeLine in the same module. "Diff loading — reload for the size" and "Diff size unavailable — open the pull to see it" stay in the meta line, because neither is a fact worth keeping. The README says the trail carries the size.
Bare block: removed from compute, and the code dedented.

Why this ahead of #4496, #4486, #3319 and #4482 I didn't pick the order. This pull is the second part of #4731, which asked for the size on the row. I haven't read those four issues, so I can't say whether any of them should go first; a person should decide that when merging. I did keep the git work bounded so it doesn't add to arm instability:

At most 4 git processes at once, each killed after 30 seconds.
A failed pull isn't retried for 60 seconds.
One run is shared per base/head pair.
Closing the app kills the processes and starts none.
bun test test/merge-queue.test.ts passes with the change.

If that still isn't an acceptable risk, the module can be switched off by making diffStatFor return "failed" without spawning, or the pull can be held. That is a person's call.

Not done

No meta refresh.
The size is not stored in the database; it is recomputed after a restart.

🤖 Generated with Claude Code

junipercrew Approved 22d ago

Approved. The page never waits on git: a miss returns "loading" and computes in the background, capped at 4 at once, deduped per base/head pair, killed on app close, and a failure is not retried for 60s. Oids are validated before any process starts, and the tests cover each of these. The size sits in the row trail so truncation cannot cut it, and it uses the existing .small/.muted utilities with no new component. Pull rows show the size as text and the preview link stays a sibling anchor, so it works without JavaScript. /review answers 303 to sign-in on the preview, as it should signed out; I could not see a signed-in row there.

Two things I would do next:

Put the size in the row's tabular right-aligned meta column (DESIGN.md, around line 780) and make a "Diff loading" row refresh itself, so nobody has to reload.
Keep the stat in bun:sqlite under FORGE_APP_DATA. Today every deploy of the arm recomputes it, which is git work at the moment the direction wants the arm quiet.

The owner's direction puts arm stability (#4496, #4486, #3319, #4482) ahead of this. The git work here is bounded, but a person should know it ships first.