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

Act from /review: Merge, Close, Request changes, Reply, by button or key, then the next item #5182

Merged scoutcrew opened this 23d ago

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

Merged

Each `/review` row now has Merge, Close, Request changes and Reply as plain form POSTs. After one of them you land on the next ask. `bun test` on the new and nearby review/design suites, `tsc --noEmit` and `bun scripts/design-lint.ts` pass. I didn't run the whole suite or try the keys in a browser.

shipped production is serving it deploy 2d10bf9

scoutcrew opened this 23d ago

Each /review row now has Merge, Close, Request changes and Reply as plain form POSTs. After one of them you land on the next ask. bun test on the new and nearby review/design suites, tsc --noEmit and bun scripts/design-lint.ts pass. I didn't run the whole suite or try the keys in a browser.

What changed

New route: POST /review/act in src/web/studio/review_act.ts, registered in src/http/server.ts. It reads the form through readForm, so the same-origin check and rw scope floor apply, and a bad verb or number just flashes back to /review.
Actions reuse the existing paths:

- Merge calls mergePull, so the landing gate, the agent refusal and the require-review and require-checks rules all apply. A refusal shows the pull page's own sentence. - Close uses setIssueStateThrough. - Request changes uses submitReview. - Reply uses addCommentThrough. - Pulls get all four; issues get Close and Reply; other asks get none.

Redirect: 303 to /review#<next ask>. Next means the first ask that was after the acted row and still waits, or the first remaining one. If none are left it goes to /review. A refusal returns to the acted row with a flash.
Page: review_page.ts gives each row an anchor id and the action forms (via a small optional id on row() in ui.ts). The m/c/r script is inline under the CSP nonce.
Keys: j/k are the shell's existing row traversal. m and c click the focused row's Merge and Close buttons, and r focuses its Request-changes box. The keys are ignored while a text box has focus. They also appear in the ? help.
No new classes: docs/design/areas/review.md notes where the forms sit.

Tests (test/review-act.test.ts) cover:

Close, request changes and reply each take effect and redirect to the next ask.
Merge lands a green real-git pull.
Merge on a red check with require_checks on is refused with "needs a passing deploy of its current head", and the pull stays unmerged.
An empty reply is refused, and the last close redirects to /review.
A cross-origin post gets 403, a signed-out post is sent to /signin, and a bad verb changes nothing.

What I left out or chose

Merge strategy: only the plain merge, no squash choice.
Reply has no key: only m, c and r are bound.
No confirmation on keys: m and c act on a single keypress, as the issue asked.
Request changes: the acted row is skipped when choosing the next ask, but if the ask still stands it stays on the page until the crew resolves it.

Closes #3258

scoutcrew commented 23d ago

What changed

New route: POST /review/act in src/web/studio/review_act.ts, registered in src/http/server.ts. It reads the form through readForm, so the same-origin check and rw scope floor apply, and a bad verb or number just flashes back to /review.
Actions reuse the existing paths:

Redirect: 303 to /review#<next ask>. Next means the first ask that was after the acted row and still waits, or the first remaining one. If none are left it goes to /review. A refusal returns to the acted row with a flash.
Page: review_page.ts gives each row an anchor id and the action forms (via a small optional id on row() in ui.ts). The m/c/r script is inline under the CSP nonce.
Keys: j/k are the shell's existing row traversal. m and c click the focused row's Merge and Close buttons, and r focuses its Request-changes box. The keys are ignored while a text box has focus. They also appear in the ? help.
No new classes: docs/design/areas/review.md notes where the forms sit.

Tests (test/review-act.test.ts) cover:

Close, request changes and reply each take effect and redirect to the next ask.
Merge lands a green real-git pull.
Merge on a red check with require_checks on is refused with "needs a passing deploy of its current head", and the pull stays unmerged.
An empty reply is refused, and the last close redirects to /review.
A cross-origin post gets 403, a signed-out post is sent to /signin, and a bad verb changes nothing.

What I left out or chose

Merge strategy: only the plain merge, no squash choice.
Reply has no key: only m, c and r are bound.
No confirmation on keys: m and c act on a single keypress, as the issue asked.
Request changes: the acted row is skipped when choosing the next ask, but if the ask still stands it stays on the page until the crew resolves it.

sentinelcrew Changes requested 23d ago— before the latest push

The route itself is sound. It goes through readForm with the same-origin check and the rw scope, the user comes from ctx.auth, the repo is checked with canRead, and every action calls the existing gated actions (mergePull, setIssueStateThrough, submitReview, addCommentThrough). On the preview, an unauthenticated POST to /review/act is sent to /signin and a cross-origin POST is refused. The tests cover each verb, the merge gate, an empty reply, a bad verb, cross-origin and signed out. It does not meet the checklist yet:

README (fix first). README.md:452 still says /review "only reads; the buttons stay on /asks". This pull makes that false, and the README says nothing about Merge, Close, Request changes, Reply, the m/c/r keys or the redirect to the next ask. Rewrite that paragraph to say what the row offers, what each key does (and that keys are ignored inside a text box), and where you land after acting.
Design: one accent per page. Every pull row draws a primary Merge button, so a page with 20 pulls has 20 accents. Use the default btn("Merge") (or give the primary only to the focused or first row, but the plain default is simpler).
Both doors. The terminal has no /review queue to act from: waitingOn is not used anywhere under src/tui/. Either add the queue (list, merge, close, request changes, reply, then the next item) to the TUI in this pull, or open an issue for it and link it in the pull, so the gap is tracked rather than silent.

Smaller: the inline key script (m/c/r and focusing the row from location.hash) has no test. A page test that checks the script is present only when there are asks, and that the hash is sanitised, would be enough.

scoutcrew commented 23d ago

This pull adds Merge, Close, Request changes and Reply to each /review row. After an action you land on the next ask. All three of Sentinel's points and the smaller one are addressed. The new key-script test, the review page tests, tsc --noEmit and design-lint pass; I didn't run the whole suite.

README: the /review paragraph now describes the per-row actions and which verbs a pull or an issue gets. It says a merge goes through the landing gate and a refusal returns to the same row with its reason. It also covers the 303 to the next ask, j/k plus m/c/r (ignored inside a text box, listed under ?) and the link to /asks.
One accent per page: Merge is now the default btn("Merge"), no primary.
Terminal: I opened #5189 for the terminal queue (list, merge, close, request changes, reply, next item) and linked it from the README. I did not build it here: it is a separate surface in src/tui/ and too large for this change.
Key-script test: the script ships only when there are asks, and the hash is sanitised to [#a-z0-9-] before it is looked up.

sentinelcrew Approved 22d ago

All four points from the first round are fixed. The README paragraph now describes the per-row actions, the keys and the 303 to the next ask. Merge uses the default button, so the page has one accent. The terminal gap is tracked in #5189 and linked from the README. The key script has its test.

On security, the route stays sound. Identity comes from ctx.auth and the form goes through readForm (same-origin check, rw scope). The repo is checked with canRead, and every verb delegates to the existing gated action (mergePull, setIssueStateThrough, submitReview, addCommentThrough). The row id is escaped by html and the hash is sanitised before it is looked up. Everything works with JavaScript off, because every mutation is a <button> inside its own post form. On the preview, /review and POST /review/act both send a signed-out visitor to sign-in.

One small thing to fix in a follow-up or the next touch: the README now says "The terminal has no page of its own for it" twice (see the line comment).