From daceda35559166b4c12546526abb46d7d67ba1d5 Mon Sep 17 00:00:00 2001 From: mptyl Date: Thu, 2 Jul 2026 17:16:57 +0200 Subject: [PATCH] docs(plan): reviewer gate UX fixes implementation plan; align spec Part 4 to ctx.ui.notify Co-Authored-By: Claude Opus 4.8 --- .../2026-07-02-reviewer-gate-ux-fixes.md | 575 ++++++++++++++++++ ...026-07-02-reviewer-gate-ux-fixes-design.md | 6 +- 2 files changed, 578 insertions(+), 3 deletions(-) create mode 100644 docs/superpowers/plans/2026-07-02-reviewer-gate-ux-fixes.md diff --git a/docs/superpowers/plans/2026-07-02-reviewer-gate-ux-fixes.md b/docs/superpowers/plans/2026-07-02-reviewer-gate-ux-fixes.md new file mode 100644 index 00000000..2012c880 --- /dev/null +++ b/docs/superpowers/plans/2026-07-02-reviewer-gate-ux-fixes.md @@ -0,0 +1,575 @@ +# Reviewer gate UX fixes — Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Fix four reviewer-gate UX defects — the multiselect lacks Back/Exit/Other, the phase gate renders no forward button (and "Other" silently approves), "Altro — specifica" never captures text, and the empty memory phase shows a pointless empty checklist. + +**Architecture:** Four surgical changes across the frontend widgets and the Pi gate. Keep logic in pure, unit-testable functions (the repo's pattern: `builders.js` and `resolveSelectOutcome` are unit-tested; the tool `execute()` glue that shells `tht` is L2/live). Add a pure `resolveConfirmOutcome` (twin of `resolveSelectOutcome`) and a pure `shouldSkipEmptyDecide` predicate so the new gate behavior is testable in node. + +**Tech Stack:** React 18 + Vite + vitest + @testing-library/react (frontend); Node.js gate extension tested with `node:test` + a fake Pi runtime (harness); `tht` Python CLI unchanged. + +## Global Constraints + +- **Design source of truth:** [docs/superpowers/specs/2026-07-02-reviewer-gate-ux-fixes-design.md](../specs/2026-07-02-reviewer-gate-ux-fixes-design.md). +- **No new auto-advance phases.** `phase.py` `_AUTO_ADVANCE_PHASES` stays `{2,6}`; do NOT touch `phase.py` or `workflow.yaml`. F1/F3/F5/F7 keep their `reviewer_confirm kind:"phase"` gate. +- **Reserved wire contract:** after this work the only reserved responses on the wire are `control:"back"`, `control:"exit"`, and `control:"freetext"` (with `text`). The bare `control:"other"` must no longer be emitted by any widget. +- **UI chrome (React button labels) is English** (`"Go back"`, `"Exit"`, `"Other — specify"`). **Gate notifications / `textResult` strings are Italian** (the workspace `psd` language), matching existing gate strings (e.g. `"Uso: /torna …"`, the `reLoop` notice). The reviewer-facing forward button label is **"Conferma e prosegui"** (decided). +- **Match each file's existing indentation:** `tht-gate.js` uses TABS; `gate/builders.js` and the frontend use 2-space. No reformatting of untouched lines. +- **Typecheck the frontend** after any `.tsx` change: `cd frontend && npx tsc -b`. No ESLint on any layer. +- **Test commands:** frontend `cd frontend && npx vitest run `; gate `node --test harness/.pi/extensions/gate/__tests__/`. + +--- + +## File structure + +**Frontend (`frontend/src/widgets/`)** +- `MultiselectWidget.tsx` — render `` (Task 1). +- `ReservedControls.tsx` — "Other — specify" reveals a textarea and emits `control:"freetext"` + text; `onControl` gains an optional `text` arg (Task 2). +- `SelectWidget.tsx`, `ArtifactGateWidget.tsx` — update the `onControl` call site to forward `text` (Task 2). +- `ReservedControls.test.tsx` (new), `MultiselectWidget.test.tsx` (extend) — tests. + +**Harness gate (`harness/.pi/extensions/`)** +- `gate/builders.js` — `buildArtifactGate` derives `options` from `action.kind` (Task 3). +- `tht-gate.js` — add exported pure `resolveConfirmOutcome` + rewrite `reviewer_confirm` guard to require an explicit approve and treat freetext as actionable (Task 4); add exported pure `shouldSkipEmptyDecide` + short-circuit the empty memory decide (Task 5). +- `gate/__tests__/builders.test.js` (extend), `gate/__tests__/gate_confirm_outcome.test.js` (new), `gate/__tests__/gate_decide_empty.test.js` (new) — tests. + +**Skill (`harness/.pi/skills/tht-sessione/`)** +- `SKILL.md` — document the empty-memory short-circuit in Phase 2 (Task 6). + +--- + +## Task 1: Multiselect gets Back/Exit/Other controls + +**Files:** +- Modify: `frontend/src/widgets/MultiselectWidget.tsx` +- Test: `frontend/src/widgets/MultiselectWidget.test.tsx` + +**Interfaces:** +- Consumes: `ReservedControls` from `./ReservedControls` (existing; signature `{ reserved?: string[]; onControl: (c: string) => void }` at this point — Task 2 widens it). +- Produces: nothing new; the multiselect now emits `{ id, control }` for reserved controls, matching `SelectWidget`. + +- [ ] **Step 1: Write the failing test** — append to `frontend/src/widgets/MultiselectWidget.test.tsx`: + +```tsx +test("renders reserved controls and emits control on click", async () => { + const onRespond = vi.fn(); + render( + + ); + await userEvent.click(screen.getByRole("button", { name: /go back/i })); + expect(onRespond).toHaveBeenCalledWith({ id: "u1", control: "back" }); + await userEvent.click(screen.getByRole("button", { name: /^exit$/i })); + expect(onRespond).toHaveBeenCalledWith({ id: "u1", control: "exit" }); +}); +``` + +- [ ] **Step 2: Run the test to verify it fails** + +Run: `cd frontend && npx vitest run src/widgets/MultiselectWidget.test.tsx -t "renders reserved controls"` +Expected: FAIL — no button named "Go back" (ReservedControls not rendered). + +- [ ] **Step 3: Implement — render `` in `MultiselectWidget.tsx`.** Add the import at the top (after line 2): + +```tsx +import { ReservedControls } from "./ReservedControls"; +``` + +Then insert the control block immediately AFTER the closing `` of the Confirm button (currently line 56), before the closing ``: + +```tsx + onRespond({ id: descriptor.id, control: c })} + /> +``` + +- [ ] **Step 4: Run the test to verify it passes** + +Run: `cd frontend && npx vitest run src/widgets/MultiselectWidget.test.tsx` +Expected: PASS (all existing tests + the new one). + +- [ ] **Step 5: Typecheck** + +Run: `cd frontend && npx tsc -b` +Expected: no errors. + +- [ ] **Step 6: Commit** + +```bash +git add frontend/src/widgets/MultiselectWidget.tsx frontend/src/widgets/MultiselectWidget.test.tsx +git commit -m "fix(frontend): render Back/Exit/Other controls on the multiselect widget" +``` + +--- + +## Task 2: "Other — specify" opens a text field and emits `control:"freetext"` + +**Files:** +- Modify: `frontend/src/widgets/ReservedControls.tsx` +- Modify: `frontend/src/widgets/SelectWidget.tsx` (line 16 call site) +- Modify: `frontend/src/widgets/ArtifactGateWidget.tsx` (lines 54-57 call site) +- Modify: `frontend/src/widgets/MultiselectWidget.tsx` (the call site added in Task 1) +- Test: `frontend/src/widgets/ReservedControls.test.tsx` (new) + +**Interfaces:** +- Produces: `ReservedControls` `onControl` signature becomes `(control: string, text?: string) => void`. On "other", the component collects text and calls `onControl("freetext", text)` — it never emits the bare `"other"` control. `back`/`exit` fire immediately with no text. +- Consumes (all three widgets): the call site becomes + `onControl={(c, t) => onRespond({ id: descriptor.id, control: c, ...(t !== undefined ? { text: t } : {}) })}`. + +- [ ] **Step 1: Write the failing test** — create `frontend/src/widgets/ReservedControls.test.tsx`: + +```tsx +import { render, screen } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { ReservedControls } from "./ReservedControls"; + +test("back and exit fire immediately with no text", async () => { + const onControl = vi.fn(); + render(); + await userEvent.click(screen.getByRole("button", { name: /go back/i })); + expect(onControl).toHaveBeenCalledWith("back"); + await userEvent.click(screen.getByRole("button", { name: /^exit$/i })); + expect(onControl).toHaveBeenCalledWith("exit"); +}); + +test("other reveals a textarea and emits freetext with the typed text", async () => { + const onControl = vi.fn(); + render(); + await userEvent.click(screen.getByRole("button", { name: /other — specify/i })); + // clicking Other does NOT emit a control yet — it reveals the input + expect(onControl).not.toHaveBeenCalled(); + await userEvent.type(screen.getByRole("textbox"), "usa la tabella X"); + await userEvent.click(screen.getByRole("button", { name: /send/i })); + expect(onControl).toHaveBeenCalledWith("freetext", "usa la tabella X"); +}); +``` + +- [ ] **Step 2: Run the test to verify it fails** + +Run: `cd frontend && npx vitest run src/widgets/ReservedControls.test.tsx` +Expected: FAIL — clicking "Other — specify" currently calls `onControl("other")` immediately; no textbox appears. + +- [ ] **Step 3: Implement — rewrite `frontend/src/widgets/ReservedControls.tsx`:** + +```tsx +import { useState } from "react"; + +const LABELS: Record = { back: "Go back", exit: "Exit", other: "Other — specify" }; + +export function ReservedControls({ + reserved, + onControl, +}: { + reserved?: string[]; + onControl: (c: string, text?: string) => void; +}) { + const [otherOpen, setOtherOpen] = useState(false); + const [text, setText] = useState(""); + if (!reserved?.length) return null; + return ( +
+
+ {reserved.map((c) => ( + + ))} +
+ {otherOpen && ( +
+