From cbee9b10307de965d77a7d505cf17920c7cf2f48 Mon Sep 17 00:00:00 2001 From: Ed Rocha Date: Mon, 2 Nov 2026 13:51:41 -0500 Subject: [PATCH 1/6] board: ask the reviewer about one finding from the review sheet Every finding on the review sheet gets "ask a question". The board types /board:review-finding-question "" into the review's own pane and brings Flock to it. There the reviewer explores with a read-only helper, reports a verdict, and asks for the outcome with a mattstack:wrap-up form. On a revise it rewrites the finding and re-raises the gate; the human finishes the review on the board. - POST /review/finding-question (finding-question.ts), which presses Escape only over the gate's own question form. - The board:review-finding-question skill. - The open sheet follows a re-raised gate onto its successor: /data.json carries gateReplacements and the decision queue swaps the old gate in place. - A revised card shows "Revised in the review pane" with the original, derived by comparing the new gate with the one it replaced. - Ticks and comment edits carry over to the re-raised gate through the kv table, so the board schema is unchanged. - Mockups B48 and B48b in docs/apps/design/board/board.pen. Rebuilt on main as one commit; the branch's earlier 43-commit history included a first design it later removed. Co-Authored-By: Claude Opus 5.5 --- apps/board/docs/api.md | 29 +- .../review-finding-question/SKILL.md | 124 +++++ .../skills/review-finding-question/SKILL.md | 127 +++++ .../review-finding-question/compiled.json | 8 + .../src/__tests__/finding-question.test.ts | 377 ++++++++++++++ apps/board/src/__tests__/gates-cache.test.ts | 125 +++++ apps/board/src/__tests__/herdr.test.ts | 13 + .../board/src/__tests__/question-form.test.ts | 56 ++ .../board/src/__tests__/review-launch.test.ts | 30 ++ .../src/__tests__/review-revision.test.ts | 171 ++++++ .../src/__tests__/review-sheet-draft.test.ts | 107 ++++ apps/board/src/client/board/Board.tsx | 35 +- .../src/client/board/DecisionQueueModal.tsx | 8 +- .../src/client/board/ReviewDraftCard.tsx | 211 +++++++- .../src/client/board/ReviewGateSheet.tsx | 146 +++++- .../board/__tests__/decision-queue.test.ts | 65 +++ .../queue-superseded-refresh-dom.test.tsx | 172 ++++++ .../__tests__/review-question-dom.test.tsx | 491 ++++++++++++++++++ .../board/__tests__/sheet-draft-dom.test.tsx | 410 +++++++++++++++ .../board/__tests__/sheet-draft.test.ts | 294 +++++++++++ apps/board/src/client/board/decision-queue.ts | 74 ++- apps/board/src/client/board/icons.tsx | 12 + apps/board/src/client/board/launch-flow.ts | 25 + apps/board/src/client/board/sheet-draft.ts | 261 ++++++++++ apps/board/src/client/types.ts | 8 + apps/board/src/finding-question.ts | 221 ++++++++ apps/board/src/gates/cache.ts | 78 ++- apps/board/src/herdr.ts | 21 + apps/board/src/question-form.ts | 34 ++ apps/board/src/review-launch.ts | 8 + apps/board/src/review-revision.ts | 99 ++++ apps/board/src/review-sheet-draft.ts | 83 +++ apps/board/src/server.ts | 127 +++++ apps/board/src/sheet-draft-shape.ts | 87 ++++ apps/board/src/style.css | 106 +++- .../2026-10-24-board-finding-questions.md | 468 +++++++++++++++++ ...26-10-23-board-finding-questions-design.md | 321 ++++++++++++ ...26-10-27-review-finding-question-design.md | 99 ++++ 38 files changed, 5078 insertions(+), 53 deletions(-) create mode 100644 apps/board/skills-src/review-finding-question/SKILL.md create mode 100644 apps/board/skills/review-finding-question/SKILL.md create mode 100644 apps/board/skills/review-finding-question/compiled.json create mode 100644 apps/board/src/__tests__/finding-question.test.ts create mode 100644 apps/board/src/__tests__/question-form.test.ts create mode 100644 apps/board/src/__tests__/review-revision.test.ts create mode 100644 apps/board/src/__tests__/review-sheet-draft.test.ts create mode 100644 apps/board/src/client/board/__tests__/queue-superseded-refresh-dom.test.tsx create mode 100644 apps/board/src/client/board/__tests__/review-question-dom.test.tsx create mode 100644 apps/board/src/client/board/__tests__/sheet-draft-dom.test.tsx create mode 100644 apps/board/src/client/board/__tests__/sheet-draft.test.ts create mode 100644 apps/board/src/client/board/sheet-draft.ts create mode 100644 apps/board/src/finding-question.ts create mode 100644 apps/board/src/question-form.ts create mode 100644 apps/board/src/review-revision.ts create mode 100644 apps/board/src/review-sheet-draft.ts create mode 100644 apps/board/src/sheet-draft-shape.ts create mode 100644 docs/superpowers/plans/2026-10-24-board-finding-questions.md create mode 100644 docs/superpowers/specs/2026-10-23-board-finding-questions-design.md create mode 100644 docs/superpowers/specs/2026-10-27-review-finding-question-design.md diff --git a/apps/board/docs/api.md b/apps/board/docs/api.md index 4f749fee24..7f31b46a0b 100644 --- a/apps/board/docs/api.md +++ b/apps/board/docs/api.md @@ -33,12 +33,13 @@ board is not peering at all. ## Launching agents (local-only) -| endpoint | body | -| --------------- | ----------------------------------------------------------------------------------- | -| `POST /review` | `{ mrUrl, iid }`. `400` when `reviewCwd` is unset | -| `POST /respond` | same shape, for your own MR | -| `POST /doctor` | same shape, to repair mechanical breakage | -| `POST /drafts` | approve or dismiss a held doctor draft. The only path from a draft to a GitLab note | +| endpoint | body | +| ------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `POST /review` | `{ mrUrl, iid }`. `400` when `reviewCwd` is unset | +| `POST /respond` | same shape, for your own MR | +| `POST /doctor` | same shape, to repair mechanical breakage | +| `POST /drafts` | approve or dismiss a held doctor draft. The only path from a draft to a GitLab note | +| `POST /review/finding-question` | `{ mrUrl, iid, gateId, findingId, question, tabId? }`. Types `/board:review-finding-question ""` into the review agent's own pane (a pane at its gate form gets one Escape, then the line again) and focuses it. `400` for a finding id outside `[A-Za-z0-9._:-]` or an empty question or one over 2000 characters; `409` when the gate is not the MR's open review gate, the gate does not list the finding, the review has no pane on file, or the pane will not take the line | A launched agent's lifecycle status never arrives over HTTP. The status CLIs publish it on the rt daemon's event bus (`board/agent-status/`) and the @@ -46,13 +47,15 @@ board consumes the bus journal by cursor; see `docs/agent-actions.md`. ## Mutating (local-only) -| endpoint | body | -| ---------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `POST /settings` | flip a member's hidden state | -| `POST /roster` | add or drop a teammate | -| `POST /tabs` | replace the tab list | -| `POST /draft` | flip one of your own MRs between draft and ready, by rewriting the title prefix | -| `POST /api/settings/*` | the settings-kit routes behind the config modal. Reads are as public as `/data.json`; writes are local-only, and a successful write live-reloads the board's config | +| endpoint | body | +| ---------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `POST /settings` | flip a member's hidden state | +| `POST /roster` | add or drop a teammate | +| `POST /tabs` | replace the tab list | +| `POST /draft` | flip one of your own MRs between draft and ready, by rewriting the title prefix | +| `GET /review/sheet-draft?mr=` | the review sheet's unsent ticks and edits for one MR, or `null`. Kept so a review gate the review agent re-raises after revising a finding restores them | +| `PUT /review/sheet-draft` | `{ mrUrl, draft: { ticked, known, texts, outcome?, notes } }`: replace that MR's saved ticks and edits. A new review run, a verdict on the review gate and the MR leaving the board drop it | +| `POST /api/settings/*` | the settings-kit routes behind the config modal. Reads are as public as `/data.json`; writes are local-only, and a successful write live-reloads the board's config | ## Slack (local-only, and `400` without a Slack token) diff --git a/apps/board/skills-src/review-finding-question/SKILL.md b/apps/board/skills-src/review-finding-question/SKILL.md new file mode 100644 index 0000000000..11929903f0 --- /dev/null +++ b/apps/board/skills-src/review-finding-question/SKILL.md @@ -0,0 +1,124 @@ +--- +name: board:review-finding-question +description: >- + Use when the human asks about one finding of the review this session wrote, + invoked in a board review pane as "/board:review-finding-question + """ while the session holds at its review-post gate. + Not for any other session. +disable-model-invocation: true +--- + +# Review finding question + +The human is deciding on your review gate and has a question about one +finding: is it real, is it overstated, what does the data say. You answer +with evidence, then they choose to revise the finding or leave it. They +finish the review on the board, never in this pane. + +## Arguments + +` ""`, commas between them optional. The +gate is the review-post gate this session opened; the finding is an `id` in +your report json (the `.json` sibling of your `--report`). Your `--state` +and `--status-bin` are the ones this session was launched with. + +This skill runs only inside the board review session that wrote that +report. If this session has no `/board:review` launch, or the finding id is +not in the report json, say so in one line and stop. + +## The rule for the whole conversation + +Until the human says to revise, you change nothing: no edit to the report, +no gate opened or answered, no post to the MR, no commit, no database +write. Databases only through rt's sdm verbs (the `rt:sdm-connect` skill), +only `*-read-only` resources, QA unless the human names prod in this +conversation. Posting and the verdict are the human's, on the board. + +## 1. Restate, then explore through a helper + +Restate in two lines: the finding (tier, title, `file:line`) and the +question. Then dispatch one read-only explorer with the Agent tool +(`model: sonnet`, `effort: medium`) and wait for it. Its brief is: + +- the finding, verbatim from the report json, and the MR url; +- the human's question, verbatim; +- what it may use: the code (`mr_view`, `gitlab_get`, or a worktree + through the `rt:worktree` skill, disposed of before it returns), logs, + New Relic, and QA databases through rt's sdm verbs on `*-read-only` + resources; prod only when the human named it; +- that it changes nothing anywhere and returns: what it checked (each query + or search, verbatim), what it found, what it could not check, and its + verdict. + +Each follow-up question that needs new digging gets its own explorer with +the same brief and the new question. A question you can answer from what +you already hold, answer directly. + +## 2. Report in this shape, then ask for the outcome as a form + +``` +Verdict: holds | narrower than written | broader than written | does not hold +Evidence: +- → (one line each, queries verbatim) +Not checked: +Proposed change: +``` + +**REQUIRED SUB-SKILL:** Use mattstack:wrap-up to ask for the outcome. The +form has one Decisions question, "What should happen to ?", +with exactly these options, the one matching your verdict first and +labelled `(Recommended)`: + +- **Revise and re-raise the gate**: apply the proposed change. +- **Leave it as is**: the finding stands. +- **Keep asking**: the human has more to ask. + +Recommend "Revise" when the proposed change is not "none", otherwise +"Leave it as is". Act on the answer: §3, §4, or wait for the next +question. A typed reply instead of a pick is a follow-up question: answer +it, report again in the same shape, and ask again. + +## 3. Revise (only on the human's go-ahead) + +1. Show the revised finding next to the original: tier, anchor, body, + comment. Keep its `id` and every field you were not asked to change. + For "drop", say so. Then ask with mattstack:wrap-up: one Decisions + question, "Apply this revision to ?", options **Apply it + (Recommended)**, **Change the wording**, **Cancel**. Apply only on + "Apply it"; "Change the wording" takes their edit and asks again; + "Cancel" goes to §4. +2. Edit the finding in the report json and the report markdown, keeping + them in step (tier counts and summary lines that name it included). +3. If a background wait loop is running on this gate, stop it with + TaskStop (load it with ToolSearch when it is deferred) before anything + else touches the gate. +4. Rebuild the gate's open file exactly as this session built the first + one (the same scripts, extras and arguments), then run the same + `open-gate.sh review-post `. The new gate + supersedes the old one. +5. Start one background ` gate wait --max-ms 90000` + loop on the new gate and end the turn holding at it. Do not show a pane + form: the human finishes on the board. +6. Tell the human: the finding is revised, the gate is re-raised, finish + the review on the board. + +A `closed (superseded)` result for the old gate id later is your own +rebuild: ignore it and keep holding at the new gate. If the rebuild or the +open fails, say what failed, restore nothing by hand, and leave the old +gate's state as the wrapper's gate step would. + +## 4. Leave it as is + +Change nothing. Tell the human the finding stands and to finish on the +board. If this gate's wait loop is no longer running, start one, then end +the turn holding at the gate. + +## Red flags + +| Thought | Do instead | +|---|---| +| "I'll just grep and query myself" | Dispatch the explorer; your context holds the review. | +| "Prod read-only would settle it" | Offer it; run it only when the human names prod. | +| "They want it gone, I'll answer the gate as comment" | Revise, re-raise, send them to the board. | +| "The old gate closed, the run is over" | That close is your rebuild. Hold at the new gate. | +| "I'll list the options in text, it's quicker" | The outcome is a mattstack:wrap-up form; the pick is the record. | diff --git a/apps/board/skills/review-finding-question/SKILL.md b/apps/board/skills/review-finding-question/SKILL.md new file mode 100644 index 0000000000..e53ab032ac --- /dev/null +++ b/apps/board/skills/review-finding-question/SKILL.md @@ -0,0 +1,127 @@ +--- +name: board:review-finding-question +description: >- + Use when the human asks about one finding of the review this session wrote, + invoked in a board review pane as "/board:review-finding-question + """ while the session holds at its review-post gate. + Not for any other session. +disable-model-invocation: true +--- + + + + +# Review finding question + +The human is deciding on your review gate and has a question about one +finding: is it real, is it overstated, what does the data say. You answer +with evidence, then they choose to revise the finding or leave it. They +finish the review on the board, never in this pane. + +## Arguments + +` ""`, commas between them optional. The +gate is the review-post gate this session opened; the finding is an `id` in +your report json (the `.json` sibling of your `--report`). Your `--state` +and `--status-bin` are the ones this session was launched with. + +This skill runs only inside the board review session that wrote that +report. If this session has no `/board:review` launch, or the finding id is +not in the report json, say so in one line and stop. + +## The rule for the whole conversation + +Until the human says to revise, you change nothing: no edit to the report, +no gate opened or answered, no post to the MR, no commit, no database +write. Databases only through rt's sdm verbs (the `rt:sdm-connect` skill), +only `*-read-only` resources, QA unless the human names prod in this +conversation. Posting and the verdict are the human's, on the board. + +## 1. Restate, then explore through a helper + +Restate in two lines: the finding (tier, title, `file:line`) and the +question. Then dispatch one read-only explorer with the Agent tool +(`model: sonnet`, `effort: medium`) and wait for it. Its brief is: + +- the finding, verbatim from the report json, and the MR url; +- the human's question, verbatim; +- what it may use: the code (`mr_view`, `gitlab_get`, or a worktree + through the `rt:worktree` skill, disposed of before it returns), logs, + New Relic, and QA databases through rt's sdm verbs on `*-read-only` + resources; prod only when the human named it; +- that it changes nothing anywhere and returns: what it checked (each query + or search, verbatim), what it found, what it could not check, and its + verdict. + +Each follow-up question that needs new digging gets its own explorer with +the same brief and the new question. A question you can answer from what +you already hold, answer directly. + +## 2. Report in this shape, then ask for the outcome as a form + +``` +Verdict: holds | narrower than written | broader than written | does not hold +Evidence: +- → (one line each, queries verbatim) +Not checked: +Proposed change: +``` + +**REQUIRED SUB-SKILL:** Use mattstack:wrap-up to ask for the outcome. The +form has one Decisions question, "What should happen to ?", +with exactly these options, the one matching your verdict first and +labelled `(Recommended)`: + +- **Revise and re-raise the gate**: apply the proposed change. +- **Leave it as is**: the finding stands. +- **Keep asking**: the human has more to ask. + +Recommend "Revise" when the proposed change is not "none", otherwise +"Leave it as is". Act on the answer: §3, §4, or wait for the next +question. A typed reply instead of a pick is a follow-up question: answer +it, report again in the same shape, and ask again. + +## 3. Revise (only on the human's go-ahead) + +1. Show the revised finding next to the original: tier, anchor, body, + comment. Keep its `id` and every field you were not asked to change. + For "drop", say so. Then ask with mattstack:wrap-up: one Decisions + question, "Apply this revision to ?", options **Apply it + (Recommended)**, **Change the wording**, **Cancel**. Apply only on + "Apply it"; "Change the wording" takes their edit and asks again; + "Cancel" goes to §4. +2. Edit the finding in the report json and the report markdown, keeping + them in step (tier counts and summary lines that name it included). +3. If a background wait loop is running on this gate, stop it with + TaskStop (load it with ToolSearch when it is deferred) before anything + else touches the gate. +4. Rebuild the gate's open file exactly as this session built the first + one (the same scripts, extras and arguments), then run the same + `open-gate.sh review-post `. The new gate + supersedes the old one. +5. Start one background ` gate wait --max-ms 90000` + loop on the new gate and end the turn holding at it. Do not show a pane + form: the human finishes on the board. +6. Tell the human: the finding is revised, the gate is re-raised, finish + the review on the board. + +A `closed (superseded)` result for the old gate id later is your own +rebuild: ignore it and keep holding at the new gate. If the rebuild or the +open fails, say what failed, restore nothing by hand, and leave the old +gate's state as the wrapper's gate step would. + +## 4. Leave it as is + +Change nothing. Tell the human the finding stands and to finish on the +board. If this gate's wait loop is no longer running, start one, then end +the turn holding at the gate. + +## Red flags + +| Thought | Do instead | +|---|---| +| "I'll just grep and query myself" | Dispatch the explorer; your context holds the review. | +| "Prod read-only would settle it" | Offer it; run it only when the human names prod. | +| "They want it gone, I'll answer the gate as comment" | Revise, re-raise, send them to the board. | +| "The old gate closed, the run is over" | That close is your rebuild. Hold at the new gate. | +| "I'll list the options in text, it's quicker" | The outcome is a mattstack:wrap-up form; the pick is the record. | diff --git a/apps/board/skills/review-finding-question/compiled.json b/apps/board/skills/review-finding-question/compiled.json new file mode 100644 index 0000000000..c1055692a3 --- /dev/null +++ b/apps/board/skills/review-finding-question/compiled.json @@ -0,0 +1,8 @@ +{ + "id": "skills:board:review-finding-question", + "dir": "skills/review-finding-question", + "covers": [ + "skills:board:review-finding-question" + ], + "keys": {} +} diff --git a/apps/board/src/__tests__/finding-question.test.ts b/apps/board/src/__tests__/finding-question.test.ts new file mode 100644 index 0000000000..5e19c79689 --- /dev/null +++ b/apps/board/src/__tests__/finding-question.test.ts @@ -0,0 +1,377 @@ +import { readFileSync } from 'node:fs'; +import { join } from 'node:path'; +import { describe, expect, test } from 'bun:test'; + +import type { + GateRow as FacilityGateRow, + PaneSendResult, +} from '@mattstack/rt-client'; +import { + ESCAPE_SETTLE_MS, + findingQuestionText, + handleFindingQuestion, + NO_REVIEW_PANE, + PANE_AT_OTHER_PROMPT, + quoteQuestion, + type FindingQuestionIo, +} from '../finding-question.ts'; + +const MR = 'https://gitlab.example.com/acme/widgets/-/merge_requests/31'; + +// Screens captured from real panes, shared with the daemon's detector tests. +const daemonFixture = (name: string) => + readFileSync( + join( + import.meta.dir, + '..', + '..', + '..', + '..', + 'lib', + 'daemon', + '__tests__', + 'fixtures', + name + ), + 'utf8' + ); +const FORM_SCREEN = daemonFixture('question-form-single.txt'); +const PERMISSION_SCREEN = [ + '⏺ Bash(rm -rf build)', + '', + ' Do you want to proceed?', + ' ❯ 1. Yes', + ' 2. No, and tell Claude what to do differently (esc)', + '', +].join('\n'); + +function gateRow(over: Partial = {}): FacilityGateRow { + return { + id: 'g-review', + subject: `mr:${MR}`, + kind: 'review-post', + questions: [ + { + id: 'findings-1', + label: 'Post which findings?', + multi: true, + options: [ + { value: 'f1', label: 'one' }, + { value: 'f2', label: 'two' }, + ], + }, + { + id: 'outcome', + label: 'Verdict', + multi: false, + options: ['approve', 'comment'], + }, + ], + meta: null, + status: 'open', + answer: null, + openedAt: 1, + parkedAt: null, + closedAt: null, + closedReason: null, + agent: null, + pane: null, + nudge: null, + delivery: null, + released: false, + supersededBy: null, + owner: null, + escalatedAt: null, + consumedAt: null, + ...over, + } as FacilityGateRow; +} + +const sent = ( + delivered: PaneSendResult['delivered'], + reason?: string, + paneId = 'w1:p1' +): { ok: true; data: PaneSendResult } => ({ + ok: true, + data: { paneId, delivered, ...(reason ? { reason } : {}) }, +}); + +/** A recording io: `replies` answers each paneSend in turn. */ +function io( + over: Partial = {}, + replies: Array>> = [ + sent('accepted'), + ] +) { + const calls: string[] = []; + const texts: string[] = []; + const sessions: Array = []; + const focused: Array<{ paneId: string; tabId?: string }> = []; + const value: FindingQuestionIo = { + reviewGate: () => gateRow(), + reviewPane: () => ({ paneId: 'w1:p1', tabId: 'w1:t1' }), + paneSend: async ({ paneId, text, selfSession }) => { + calls.push(`send:${paneId}`); + texts.push(text); + sessions.push(selfSession); + return replies.shift() ?? sent('refused', 'no more replies'); + }, + readScreen: async paneId => { + calls.push(`read:${paneId}`); + return FORM_SCREEN; + }, + sendEscape: async paneId => { + calls.push(`escape:${paneId}`); + }, + sleep: async ms => { + calls.push(`sleep:${ms}`); + }, + focusPane: async pane => { + calls.push('focus'); + focused.push(pane); + }, + ...over, + }; + return { io: value, calls, texts, sessions, focused }; +} + +const body = (over: Record = {}) => ({ + mrUrl: MR, + iid: 31, + gateId: 'g-review', + findingId: 'f2', + question: 'Is this really Important?', + ...over, +}); + +describe('quoting the question', () => { + test('collapses every run of whitespace, newlines included, to one space', () => { + expect(quoteQuestion(' why\n\n is this\tcritical? ')).toBe( + '"why is this critical?"' + ); + }); + + test('escapes double quotes', () => { + expect(quoteQuestion('does "retry" mean the loop?')).toBe( + '"does \\"retry\\" mean the loop?"' + ); + }); + + test('escapes backslashes before quotes, so a trailing one cannot eat the closing quote', () => { + expect(quoteQuestion('is C:\\temp\\')).toBe('"is C:\\\\temp\\\\"'); + expect(quoteQuestion('a \\"b\\" c')).toBe('"a \\\\\\"b\\\\\\" c"'); + }); + + test('the pane line names the skill, the gate and the finding', () => { + expect(findingQuestionText('g-review', 'f2', 'a\nb "c"')).toBe( + '/board:review-finding-question g-review f2 "a b \\"c\\""' + ); + }); +}); + +describe('handleFindingQuestion refusals', () => { + test.each([ + ['a finding id with a space', { findingId: 'f 2' }], + ['a finding id with a quote', { findingId: 'f"2' }], + ['an empty question', { question: ' \n ' }], + ['a question over 2000 characters', { question: 'x'.repeat(2001) }], + ['a body missing the gate', { gateId: undefined }], + ])('400 for %s', async (_name, over) => { + const { io: deps, calls } = io(); + const res = await handleFindingQuestion(body(over), deps); + expect(res.status).toBe(400); + expect(calls).toEqual([]); + }); + + test("409 when the gate is not this MR's open review gate", async () => { + for (const reviewGate of [ + () => undefined, + () => gateRow({ id: 'g-other' }), + () => gateRow({ status: 'parked' }), + ]) { + const { io: deps, calls } = io({ reviewGate }); + const res = await handleFindingQuestion(body(), deps); + expect(res.status).toBe(409); + expect(calls).toEqual([]); + } + }); + + test("409 when the gate's findings do not list the finding", async () => { + const { io: deps, calls } = io(); + const res = await handleFindingQuestion(body({ findingId: 'f9' }), deps); + expect(res.status).toBe(409); + expect(calls).toEqual([]); + }); + + test('409 with the resume hint when the review has no pane on file', async () => { + const { io: deps, calls } = io({ reviewPane: () => ({ tabId: 'w1:t1' }) }); + const res = await handleFindingQuestion(body(), deps); + expect(res).toEqual({ status: 409, body: NO_REVIEW_PANE }); + expect(NO_REVIEW_PANE).toBe( + "the review pane isn't running; resume the review from its row first" + ); + expect(calls).toEqual([]); + }); + + test('409 carrying the reason when the pane refuses for anything else', async () => { + const { io: deps, calls } = io({}, [sent('refused', 'pane is gone')]); + const res = await handleFindingQuestion(body(), deps); + expect(res.status).toBe(409); + expect(res.body).toContain('pane is gone'); + expect(calls).toEqual(['send:w1:p1']); + }); + + test('409 when rt cannot deliver at all', async () => { + const { io: deps } = io({}, [{ ok: false, error: 'daemon unreachable' }]); + const res = await handleFindingQuestion(body(), deps); + expect(res.status).toBe(409); + expect(res.body).toContain('daemon unreachable'); + }); +}); + +describe('handleFindingQuestion delivery', () => { + test('types the line into the review pane and focuses it', async () => { + const { io: deps, calls, texts, focused } = io(); + const res = await handleFindingQuestion( + body({ question: 'Is "this"\nreally Important?' }), + deps + ); + expect(res).toEqual({ status: 200, body: { ok: true } }); + expect(texts).toEqual([ + '/board:review-finding-question g-review f2 "Is \\"this\\" really Important?"', + ]); + expect(calls).toEqual(['send:w1:p1', 'focus']); + expect(focused).toEqual([{ paneId: 'w1:p1', tabId: 'w1:t1' }]); + }); + + test('a queued delivery is success too', async () => { + const { io: deps } = io({}, [sent('queued')]); + expect((await handleFindingQuestion(body(), deps)).status).toBe(200); + }); + + test("the body's tabId wins over the review state's", async () => { + const { io: deps, focused } = io(); + await handleFindingQuestion(body({ tabId: 'w2:t9' }), deps); + expect(focused).toEqual([{ paneId: 'w1:p1', tabId: 'w2:t9' }]); + }); + + test('a pane at the gate form gets one Escape, a pause, then the line again', async () => { + const { + io: deps, + calls, + texts, + } = io({}, [sent('refused', 'at a prompt'), sent('accepted')]); + const res = await handleFindingQuestion(body(), deps); + expect(res.status).toBe(200); + expect(calls).toEqual([ + 'send:w1:p1', + 'read:w1:p1', + 'escape:w1:p1', + `sleep:${ESCAPE_SETTLE_MS}`, + 'send:w1:p1', + 'focus', + ]); + expect(texts[0]).toBe(texts[1]); + expect(ESCAPE_SETTLE_MS).toBe(600); + }); + + test('a pane blocked on any other prompt gets no Escape and a 409', async () => { + const { io: deps, calls } = io( + { readScreen: async () => PERMISSION_SCREEN }, + [sent('refused', 'at a prompt')] + ); + const res = await handleFindingQuestion(body(), deps); + expect(res).toEqual({ status: 409, body: PANE_AT_OTHER_PROMPT }); + expect(PANE_AT_OTHER_PROMPT).toBe( + 'the review pane is waiting on a prompt; answer it there first' + ); + expect(calls).toEqual(['send:w1:p1']); + }); + + test('a blocked pane whose screen cannot be read gets no Escape either', async () => { + const errors: string[] = []; + const { io: deps, calls } = io( + { + readScreen: async () => { + throw new Error('herdr down'); + }, + logError: m => errors.push(m), + }, + [sent('refused', 'at a prompt')] + ); + const res = await handleFindingQuestion(body(), deps); + expect(res).toEqual({ status: 409, body: PANE_AT_OTHER_PROMPT }); + expect(calls).toEqual(['send:w1:p1']); + expect(errors[0]).toContain('herdr down'); + }); + + test('only one retry: a pane still at a prompt is a 409', async () => { + const { io: deps, calls } = io({}, [ + sent('refused', 'at a prompt'), + sent('refused', 'at a prompt'), + ]); + const res = await handleFindingQuestion(body(), deps); + expect(res.status).toBe(409); + expect(res.body).toContain('at a prompt'); + expect(calls.filter(c => c.startsWith('send:')).length).toBe(2); + expect(calls).not.toContain('focus'); + }); + + test("the send names the review's session, and the pane it resolved gets the Escape and the focus", async () => { + const { + io: deps, + calls, + sessions, + focused, + } = io( + { + reviewPane: () => ({ + paneId: 'w1:p1', + tabId: 'w1:t1', + sessionId: 'sess-1', + }), + }, + [ + sent('refused', 'at a prompt', 'w3:p7'), + sent('accepted', undefined, 'w3:p7'), + ] + ); + const res = await handleFindingQuestion(body(), deps); + expect(res.status).toBe(200); + expect(sessions).toEqual(['sess-1', 'sess-1']); + expect(calls).toEqual([ + 'send:w1:p1', + 'read:w3:p7', + 'escape:w3:p7', + `sleep:${ESCAPE_SETTLE_MS}`, + 'send:w3:p7', + 'focus', + ]); + expect(focused).toEqual([{ paneId: 'w3:p7', tabId: 'w1:t1' }]); + }); + + test('no session on file sends by pane id alone', async () => { + const { io: deps, sessions } = io(); + await handleFindingQuestion(body(), deps); + expect(sessions).toEqual([undefined]); + }); + + test('a pane that is gone is the resume hint', async () => { + const { io: deps, calls } = io({}, [sent('refused', 'not a claude pane')]); + const res = await handleFindingQuestion(body(), deps); + expect(res).toEqual({ status: 409, body: NO_REVIEW_PANE }); + expect(calls).toEqual(['send:w1:p1']); + }); + + test('a focus that fails does not lose the delivered question', async () => { + const { io: deps } = io({ + focusPane: async () => { + throw new Error('herdr down'); + }, + }); + expect(await handleFindingQuestion(body(), deps)).toEqual({ + status: 200, + body: { ok: true }, + }); + }); +}); diff --git a/apps/board/src/__tests__/gates-cache.test.ts b/apps/board/src/__tests__/gates-cache.test.ts index 732ffa938a..83296f5d41 100644 --- a/apps/board/src/__tests__/gates-cache.test.ts +++ b/apps/board/src/__tests__/gates-cache.test.ts @@ -1227,3 +1227,128 @@ describe('attachGates owner', () => { expect(mr!.gates[0]!.owner).toBe('human'); }); }); + +describe('GateCache.supersededBy', () => { + const opened = (id: string) => ({ + topic: `gate/opened/${id}`, + payload: { + id, + subject: SUBJECT_A, + kind: 'review-post', + questions: [{ id: 'q1', label: 'Ship it?', options: ['yes', 'no'] }], + }, + }); + + test("the supersede rule's frames keep the old gate as the new one's predecessor", () => { + const cache = new GateCache(); + cache.applyEvent(opened('g-old')); + cache.applyEvent(opened('g-new')); + cache.applyEvent({ + topic: 'gate/closed/g-old', + payload: { + id: 'g-old', + subject: SUBJECT_A, + kind: 'review-post', + reason: 'superseded', + supersededBy: 'g-new', + }, + }); + expect(cache.get(SUBJECT_A, 'review-post')?.id).toBe('g-new'); + expect(cache.rows().map(r => r.id)).toEqual(['g-new']); + const prior = cache.supersededBy('g-new'); + expect(prior?.id).toBe('g-old'); + expect(prior?.status).toBe('closed'); + expect(prior?.closedReason).toBe('superseded'); + }); + + test('a replaced gate that was never closed as superseded is no predecessor', () => { + const cache = new GateCache(); + cache.applyEvent(opened('g-old')); + cache.applyEvent(opened('g-new')); + expect(cache.supersededBy('g-new')).toBeUndefined(); + }); + + test('a reconcile listing both rounds, newest first, keeps the predecessor', () => { + const cache = new GateCache(); + cache.reconcile([ + row({ id: 'g-new', openedAt: 2000 }), + row({ + id: 'g-old', + openedAt: 1000, + status: 'closed', + closedReason: 'superseded', + supersededBy: 'g-new', + }), + ]); + expect(cache.get(SUBJECT_A, 'review-post')?.id).toBe('g-new'); + expect(cache.supersededBy('g-new')?.id).toBe('g-old'); + }); + + test('an older round that names another gate is dropped', () => { + const cache = new GateCache(); + cache.reconcile([ + row({ id: 'g-new', openedAt: 2000 }), + row({ + id: 'g-older', + openedAt: 500, + status: 'closed', + closedReason: 'superseded', + supersededBy: 'g-old', + }), + ]); + expect(cache.supersededBy('g-new')).toBeUndefined(); + expect(cache.supersededBy('g-old')).toBeUndefined(); + }); +}); + +describe('GateCache.replacements', () => { + const opened = (id: string) => ({ + topic: `gate/opened/${id}`, + payload: { + id, + subject: SUBJECT_A, + kind: 'review-post', + questions: [{ id: 'q1', label: 'Ship it?', options: ['yes', 'no'] }], + }, + }); + + test('maps each superseded gate to the gate that replaced it', () => { + const cache = new GateCache(); + cache.reconcile([ + row({ id: 'g-new', openedAt: 2000 }), + row({ + id: 'g-old', + openedAt: 1000, + status: 'closed', + closedReason: 'superseded', + supersededBy: 'g-new', + }), + row({ id: 'p-new', kind: 'respond-plan', openedAt: 2000 }), + row({ + id: 'p-old', + kind: 'respond-plan', + openedAt: 1000, + status: 'closed', + closedReason: 'superseded', + supersededBy: 'p-new', + }), + ]); + expect(cache.replacements()).toEqual({ + 'g-old': 'g-new', + 'p-old': 'p-new', + }); + }); + + test('a replaced gate that is not closed with a successor is left out', () => { + const cache = new GateCache(); + cache.applyEvent(opened('g-old')); + cache.applyEvent(opened('g-new')); + expect(cache.replacements()).toEqual({}); + }); + + test('is empty for a cache that replaced nothing', () => { + const cache = new GateCache(); + cache.applyRow(row()); + expect(cache.replacements()).toEqual({}); + }); +}); diff --git a/apps/board/src/__tests__/herdr.test.ts b/apps/board/src/__tests__/herdr.test.ts index fb3cbf1602..6fd0aea382 100644 --- a/apps/board/src/__tests__/herdr.test.ts +++ b/apps/board/src/__tests__/herdr.test.ts @@ -22,6 +22,7 @@ import { parseLaunchNote, parseTabCreate, parseWorkspaceCreate, + readPaneScreen, reopenPrompt, respondPrompt, reviewPrompt, @@ -1444,3 +1445,15 @@ describe('sendPaneText', () => { ]); }); }); + +describe('readPaneScreen', () => { + test('reads the visible screen with `pane read --source visible`', async () => { + const calls: string[][] = []; + const runner: HerdrRunner = async args => { + calls.push(args); + return 'screen text'; + }; + expect(await readPaneScreen('w1:p1', runner)).toBe('screen text'); + expect(calls).toEqual([['pane', 'read', 'w1:p1', '--source', 'visible']]); + }); +}); diff --git a/apps/board/src/__tests__/question-form.test.ts b/apps/board/src/__tests__/question-form.test.ts new file mode 100644 index 0000000000..f66096d98e --- /dev/null +++ b/apps/board/src/__tests__/question-form.test.ts @@ -0,0 +1,56 @@ +import { readFileSync } from 'node:fs'; +import { join } from 'node:path'; +import { describe, expect, test } from 'bun:test'; + +import { hasQuestionForm as daemonHasQuestionForm } from '../../../../lib/daemon/question-form.ts'; +import { hasQuestionForm } from '../question-form.ts'; + +// The board's copy of the daemon's AskUserQuestion detector must agree with +// it on every screen: the daemon's captured fixtures and the shapes its own +// tests build from them. +const FIXTURES = join( + import.meta.dir, + '..', + '..', + '..', + '..', + 'lib', + 'daemon', + '__tests__', + 'fixtures' +); +const fixture = (name: string) => readFileSync(join(FIXTURES, name), 'utf8'); + +const single = fixture('question-form-single.txt'); +const screens: Array<[string, string, boolean]> = [ + ['a single-question form', single, true], + ['a multi-question form', fixture('question-form-multi.txt'), true], + ['an inline-rendered form', fixture('question-form-inline.txt'), true], + ['an idle prompt', fixture('pane-footer-1-shell.txt'), false], + [ + 'a form the session moved past', + `${single}\n⏺ User answered Claude's questions:\n`, + false, + ], + [ + 'the vim-mode footer', + single.replace( + 'Enter to select · ↑/↓ to navigate · Esc to cancel', + '-- INSERT -- · Enter to select · ↑/↓ to navigate' + ), + true, + ], + [ + 'transcript text quoting the footer', + '⏺ quoting:\n Enter to select · ↑/↓ to navigate · Esc to cancel\n❯ \n', + false, + ], + ['an empty screen', '', false], +]; + +describe('hasQuestionForm (port of lib/daemon/question-form.ts)', () => { + test.each(screens)('%s', (_name, screen, form) => { + expect(hasQuestionForm(screen)).toBe(form); + expect(daemonHasQuestionForm(screen)).toBe(form); + }); +}); diff --git a/apps/board/src/__tests__/review-launch.test.ts b/apps/board/src/__tests__/review-launch.test.ts index 866ab82b1b..2e5eb237a3 100644 --- a/apps/board/src/__tests__/review-launch.test.ts +++ b/apps/board/src/__tests__/review-launch.test.ts @@ -19,6 +19,11 @@ import { type RespondAskCtx, type RespondAskIo, } from '../review-launch.ts'; +import { + deleteSheetDraft, + readSheetDraft, + writeSheetDraft, +} from '../review-sheet-draft.ts'; import { readReviewStates, reviewFilePath, @@ -100,6 +105,7 @@ function makeIo(over: Partial = {}): ReReviewIo { writeReviewState: (path, patch, now) => writeReviewState(path, patch, now, db), readReviewStates: () => readReviewStates(db), + deleteSheetDraft: mrUrl => deleteSheetDraft(mrUrl, db), ...over, }; } @@ -995,3 +1001,27 @@ describe('launchReReview: a skill that does not resolve opens no pane', () => { } ); }); + +describe('launchReReview: the review sheet draft', () => { + test.each([ + ['agentId on file (resume)', { agentId: 'agent-old' }], + ['sessionId only (legacy resume)', { sessionId: 'sess-old' }], + ['nothing on file (fresh launch)', {}], + ])('a re-review launch drops the MR draft: %s', async (_name, prior) => { + writeReviewState( + reviewFilePath(URL_A), + { mrUrl: URL_A, iid: IID, status: 'done', ...prior }, + Date.now(), + db + ); + writeSheetDraft( + URL_A, + { ticked: ['f1'], known: ['f1'], texts: {}, notes: {} }, + db + ); + + await launchReReview(URL_A, IID, CTX, makeIo(), noSkillPath); + + expect(readSheetDraft(URL_A, db)).toBeUndefined(); + }); +}); diff --git a/apps/board/src/__tests__/review-revision.test.ts b/apps/board/src/__tests__/review-revision.test.ts new file mode 100644 index 0000000000..7d7b3bdb5a --- /dev/null +++ b/apps/board/src/__tests__/review-revision.test.ts @@ -0,0 +1,171 @@ +import { describe, expect, test } from 'bun:test'; + +import type { GateRow as FacilityGateRow } from '@mattstack/rt-client'; +import type { FindingEntry } from '../client/board/gate-ctx.ts'; +import { GateCache } from '../gates/cache.ts'; +import { + findingChanged, + gateFindings, + reviewRevisionFor, +} from '../review-revision.ts'; + +const MR = 'https://gitlab.example.com/acme/widgets/-/merge_requests/31'; +const SUBJECT = `mr:${MR}`; + +const F1: FindingEntry = { + id: 'f1', + severity: 'important', + title: 'Retry loop never backs off', + body: 'The loop retries at once.', + file: 'lib/retry.ts:12', + comment: 'Could this back off?', +}; +const F2: FindingEntry = { + id: 'f2', + severity: 'minor', + title: 'Stale doc comment', + body: 'The comment names the old flag.', + file: 'lib/flags.ts:4', + comment: 'Nit: the comment is stale.', +}; + +/** A review-post facility row whose findings@1 joins its options. */ +function reviewRow( + id: string, + findings: FindingEntry[], + over: Partial = {} +): FacilityGateRow { + const counts = { critical: 0, important: 0, minor: 0 }; + for (const f of findings) counts[f.severity]++; + return { + id, + subject: SUBJECT, + kind: 'review-post', + questions: [ + { + id: 'findings-1', + label: 'Post which findings?', + multi: true, + context: JSON.stringify({ 'gate-ctx': 'findings@1', findings }), + options: findings.map(f => ({ value: f.id, label: f.title })), + }, + { + id: 'outcome', + label: 'Verdict', + multi: false, + options: [ + { value: 'approve', label: 'approve' }, + { value: 'comment', label: 'comment' }, + ], + }, + ], + context: JSON.stringify({ + 'gate-ctx': 'review@1', + readiness: 'with-fixes', + summary: 'Two findings.', + findings: counts, + }), + meta: null, + status: 'open', + answer: null, + openedAt: 2000, + parkedAt: null, + closedAt: null, + closedReason: null, + agent: null, + pane: null, + nudge: null, + delivery: null, + released: false, + supersededBy: null, + owner: null, + escalatedAt: null, + consumedAt: null, + ...over, + } as FacilityGateRow; +} + +const superseded = (id: string, findings: FindingEntry[], by: string) => + reviewRow(id, findings, { + openedAt: 1000, + status: 'closed', + closedReason: 'superseded', + supersededBy: by, + }); + +function cacheWith(...rows: FacilityGateRow[]): GateCache { + const cache = new GateCache(); + cache.reconcile(rows); + return cache; +} + +describe('gateFindings', () => { + test("reads a review sheet's findings by id", () => { + expect([...gateFindings(reviewRow('g', [F1, F2]))!.keys()]).toEqual([ + 'f1', + 'f2', + ]); + }); + + test('a row the sheet cannot read has none', () => { + expect(gateFindings(reviewRow('g', [F1], { context: null }))).toBeNull(); + }); +}); + +describe('findingChanged', () => { + test.each([ + ['severity', { severity: 'minor' as const }], + ['anchor', { file: 'lib/retry.ts:14' }], + ['title', { title: 'Retry loop spins' }], + ['body', { body: 'Reworded.' }], + ])('a different %s is a revision', (_name, change) => { + expect(findingChanged(F1, { ...F1, ...change })).toBe(true); + }); + + test('a changed comment alone is not', () => { + expect(findingChanged(F1, { ...F1, comment: 'Other draft.' })).toBe(false); + }); +}); + +describe('reviewRevisionFor', () => { + test("names each finding the open gate changed, with the superseded gate's original", () => { + const revisedF1 = { ...F1, severity: 'minor' as const, body: 'Narrower.' }; + const cache = cacheWith( + reviewRow('g-new', [revisedF1, F2]), + superseded('g-old', [F1, F2], 'g-new') + ); + expect(reviewRevisionFor(cache, MR)).toEqual({ + gateId: 'g-new', + revised: { f1: { original: F1 } }, + }); + }); + + test('a dropped finding has no card, so it is left out', () => { + const cache = cacheWith( + reviewRow('g-new', [F1]), + superseded('g-old', [F1, F2], 'g-new') + ); + expect(reviewRevisionFor(cache, MR)).toBeUndefined(); + }); + + test('no predecessor in the cache (a board restart) means no revision', () => { + const cache = cacheWith(reviewRow('g-new', [F1, F2])); + expect(reviewRevisionFor(cache, MR)).toBeUndefined(); + }); + + test('an answered gate is not on screen to mark', () => { + const cache = cacheWith( + reviewRow('g-new', [{ ...F1, title: 'New' }], { status: 'answered' }), + superseded('g-old', [F1], 'g-new') + ); + expect(reviewRevisionFor(cache, MR)).toBeUndefined(); + }); + + test('a re-raise that changed nothing has no revision', () => { + const cache = cacheWith( + reviewRow('g-new', [F1, F2]), + superseded('g-old', [F1, F2], 'g-new') + ); + expect(reviewRevisionFor(cache, MR)).toBeUndefined(); + }); +}); diff --git a/apps/board/src/__tests__/review-sheet-draft.test.ts b/apps/board/src/__tests__/review-sheet-draft.test.ts new file mode 100644 index 0000000000..8a54ec982c --- /dev/null +++ b/apps/board/src/__tests__/review-sheet-draft.test.ts @@ -0,0 +1,107 @@ +import { mkdtempSync, rmSync } from 'fs'; +import { tmpdir } from 'os'; +import { join } from 'path'; +import type { Database } from 'bun:sqlite'; +import { afterEach, beforeEach, describe, expect, test } from 'bun:test'; + +import { + dropDraftOnReviewVerdict, + pruneSheetDrafts, + readSheetDraft, + SHEET_DRAFT_NS, + writeSheetDraft, +} from '../review-sheet-draft.ts'; +import { openStateDb } from '../state/db.ts'; +import { setKvValue } from '../state/kv-blob.ts'; + +const MR_A = 'https://gitlab.example.com/acme/widgets/-/merge_requests/31'; +const MR_B = 'https://gitlab.example.com/acme/widgets/-/merge_requests/32'; +const DRAFT = { + ticked: ['f1'], + known: ['f1', 'f2'], + texts: { 'comment:f1': 'my words' }, + outcome: 'comment', + notes: {}, +}; + +let dir: string; +let db: Database; +beforeEach(() => { + dir = mkdtempSync(join(tmpdir(), 'sheet-draft-')); + db = openStateDb(join(dir, 'state.db')); +}); +afterEach(() => { + db.close(); + rmSync(dir, { recursive: true, force: true }); +}); + +const answered = ( + answers: Record, + over: Record = {} +) => ({ + topic: 'gate/answered/g1', + payload: { + id: 'g1', + subject: `mr:${MR_A}`, + kind: 'review-post', + answers, + by: 'board', + ...over, + }, +}); + +describe('the review sheet draft store', () => { + test('lives in the kv table, with no table or migration of its own', () => { + const tables = db + .query("SELECT name FROM sqlite_master WHERE type = 'table'") + .all() as { name: string }[]; + expect(tables.some(t => /sheet/i.test(t.name))).toBe(false); + writeSheetDraft(MR_A, DRAFT, db); + expect(readSheetDraft(MR_A, db)).toEqual(DRAFT); + const row = db + .query('SELECT v FROM kv WHERE ns = ? AND k = ?') + .get(SHEET_DRAFT_NS, MR_A) as { v: string } | null; + expect(JSON.parse(row!.v)).toEqual(DRAFT); + }); + + test('a stored value of the wrong shape reads as no draft', () => { + setKvValue(SHEET_DRAFT_NS, MR_A, { ticked: 'f1' }, db); + expect(readSheetDraft(MR_A, db)).toBeUndefined(); + }); + + test('an MR that left the board takes its draft with it', () => { + writeSheetDraft(MR_A, DRAFT, db); + writeSheetDraft(MR_B, DRAFT, db); + pruneSheetDrafts(new Set([MR_B]), db); + expect(readSheetDraft(MR_A, db)).toBeUndefined(); + expect(readSheetDraft(MR_B, db)).toEqual(DRAFT); + }); +}); + +describe('dropDraftOnReviewVerdict', () => { + test('a verdict on the review gate drops the draft', () => { + writeSheetDraft(MR_A, DRAFT, db); + expect(dropDraftOnReviewVerdict(answered({ outcome: 'approve' }), db)).toBe( + true + ); + expect(readSheetDraft(MR_A, db)).toBeUndefined(); + }); + + test.each([ + ['iterate', 'iterate'], + ['hold', 'hold'], + ['iterate with a note', { value: 'iterate', note: 'one more pass' }], + ])('an answer whose next is %s keeps it', (_name, next) => { + writeSheetDraft(MR_A, DRAFT, db); + expect(dropDraftOnReviewVerdict(answered({ next }), db)).toBe(false); + expect(readSheetDraft(MR_A, db)).toEqual(DRAFT); + }); + + test('another gate kind, another MR or another frame leaves it alone', () => { + writeSheetDraft(MR_A, DRAFT, db); + dropDraftOnReviewVerdict(answered({}, { kind: 'respond-post' }), db); + dropDraftOnReviewVerdict(answered({}, { subject: `mr:${MR_B}` }), db); + dropDraftOnReviewVerdict({ ...answered({}), topic: 'gate/opened/g1' }, db); + expect(readSheetDraft(MR_A, db)).toEqual(DRAFT); + }); +}); diff --git a/apps/board/src/client/board/Board.tsx b/apps/board/src/client/board/Board.tsx index 59b8fc2ce4..9d5274bf2f 100644 --- a/apps/board/src/client/board/Board.tsx +++ b/apps/board/src/client/board/Board.tsx @@ -128,6 +128,7 @@ import { useOptimisticLifecycle, } from './hooks.ts'; import { assignMemberLooks } from './invadr-colors.ts'; +import { FINDING_QUESTION, findingQuestionFields } from './launch-flow.ts'; import { MemberInvadr, MemberLooksProvider } from './MemberInvadr.tsx'; import { mrRef, onGitHub } from './MrLinks.tsx'; import { NEED_LABEL, NEED_ORDER, needOf } from './needs-me.ts'; @@ -676,6 +677,32 @@ export function Board() { failureMessage: resumeRespondFailureMessage, }); + const questionAction = useLaunchAction({ + axis: null, + ...FINDING_QUESTION, + optimistic: optimisticLifecycle, + addToast, + startToast, + reload: load, + }); + // Resolves whether the review pane took the question, so the sheet can + // say so under the finding. + const askQuestion = useCallback( + async ( + mr: BoardMRWithReview, + gateId: string, + findingId: string, + question: string + ) => { + const result = await questionAction( + mr, + findingQuestionFields(gateId, findingId, question, mr.review?.tabId) + ); + return Boolean(result?.ok); + }, + [questionAction] + ); + // Each flow's payload lives here once: the row menu, the bulk menu, the // status line's verbs and the decision queue all launch through it. const launch = useCallback( @@ -1476,7 +1503,12 @@ export function Board() { if (gate.status === 'answered') ids.add(gate.gateId); return ids; }, [data]); - const queue = useDecisionQueue(queueEntries, answeredGateIds); + const gateReplacements = data?.gateReplacements; + const queue = useDecisionQueue( + queueEntries, + answeredGateIds, + gateReplacements + ); const activeGateId = queue.active?.gate.gateId ?? null; const retireActiveGate = () => { if (activeGateId === null) return; @@ -2234,6 +2266,7 @@ export function Board() { onContinue={retireActiveGate} onLostChange={lost => queue.hold(lost ? activeGateId : null)} self={seat} + onAskQuestion={askQuestion} readOnly={gateReadOnlyReason( queue.active.gate, queue.active.mr, diff --git a/apps/board/src/client/board/DecisionQueueModal.tsx b/apps/board/src/client/board/DecisionQueueModal.tsx index b8357c2a41..385feb4266 100644 --- a/apps/board/src/client/board/DecisionQueueModal.tsx +++ b/apps/board/src/client/board/DecisionQueueModal.tsx @@ -19,7 +19,7 @@ import { mrRef } from './MrLinks.tsx'; import { postPicks } from './respond-post.ts'; import { RespondSheetBody } from './RespondSheet.tsx'; import { isReviewSheetGate, paneContext } from './review-gate.ts'; -import { ReviewGateSheet } from './ReviewGateSheet.tsx'; +import { ReviewGateSheet, type AskQuestion } from './ReviewGateSheet.tsx'; import { paneReason, PaneSheetBody, StageSheetBody } from './StageSheet.tsx'; /** One pip per queued gate. */ @@ -68,6 +68,7 @@ function DecisionQueueModal({ onLostChange, readOnly: seatReadOnly, self, + onAskQuestion, }: { gate: GateRow & { executor?: ExecutorState }; /** Absent for a non-MR gate (queueExtras) -- the strip and face below @@ -98,6 +99,9 @@ function DecisionQueueModal({ readOnly?: string | null; /** The seat's username, for the avatar beside "you" in a sheet. */ self?: string | null; + /** Types a question about one review finding into the review pane, + through the board's launch flow; resolves whether the pane took it. */ + onAskQuestion?: AskQuestion; }) { const paneGone = gate.executor === 'gone'; const context = gateContext(gate); @@ -217,6 +221,8 @@ function DecisionQueueModal({ +
+ {text} + +
+ {open && ( +
+

{original.title}

+ {original.body} +
+ )} + + ); +} + +/** One finding's question to the review pane: `idle` offers "ask a + question", `open` is the area being written, `sending` is the request in + flight, `asked` is a question the pane took. `blocked` is set while + another finding's question is sending: one question at a time. + `unavailable` is why no question can be asked at all (the review pane is + gone): the link stays, disabled, with the reason as its tooltip. */ +export interface FindingAsk { + state: 'idle' | 'open' | 'sending' | 'asked'; + blocked: boolean; + unavailable?: string; + onOpen: () => void; + onCancel: () => void; + onSend: (question: string) => void; + /** Brings the review pane forward; absent when the sheet cannot. */ + onFocusPane?: () => void; +} + +export const ASK_PLACEHOLDER = 'What do you want to know about this finding?'; + +/** A finding's "ask a question" link, at the end of the card head. */ +export function AskLink({ title, ask }: { title: string; ask: FindingAsk }) { + return ( + + ); +} + +/** The area under the card head where the question is written and sent to + the review pane, then where the sheet says the pane took it. */ +function AskArea({ title, ask }: { title: string; ask: FindingAsk }) { + const [text, setText] = useState(''); + // A question the pane took is done with; asking again starts empty. + useEffect(() => { + if (ask.state === 'asked') setText(''); + }, [ask.state]); + if (ask.state === 'asked') + return ( +
+ + asked in the review pane + + {ask.onFocusPane && ( + + )} +
+ ); + const sending = ask.state === 'sending'; + const empty = text.trim() === ''; + return ( +
+
+