945 lines
37 KiB
Markdown
945 lines
37 KiB
Markdown
# Workflow UI 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 five UI/contract defects found while testing: phase-circle lifecycle colors, a total elapsed timer, the empty reviewer artifact gate, a 90% artifact modal with a Mermaid schema diagram, and the duplicate "Altro"/"other" option.
|
||
|
||
**Architecture:** All fixes are in the frontend except one root-cause fix in the harness gate. The reviewer artifact gate renders `artifact.data` (harness contract) — not the never-populated `artifact.content` — through a new `ArtifactView` router inside a full-screen modal. Phase lifecycle stays frontend-optimistic (no new Pi RPC events). The duplicate-option bug is fixed by making the harness reserved-label matcher robust to model-produced variants.
|
||
|
||
**Tech Stack:** React 18 + Vite + vitest + Testing Library (frontend, no network — MSW); Node `node --test` for the harness gate JS; Mermaid (already a dependency); Base UI dialog (`components/ui/dialog.tsx`).
|
||
|
||
## Global Constraints
|
||
|
||
- Frontend has no ESLint; `npx tsc -b` is the gate. Run it before every commit that touches TS.
|
||
- Frontend tests use vitest + MSW, no real network.
|
||
- Harness gate JS tests run via `cd harness && npm test` (`node --test .pi/extensions/gate/__tests__/*.test.js`). New harness tests MUST live in `.pi/extensions/gate/__tests__/` and be named `*.test.js`.
|
||
- UI strings/chrome/labels stay **English** (project convention). Document *content* stays the workspace language.
|
||
- Harness JS line style: keep the surrounding tab-indented style of the file you edit; do not reformat neighbours.
|
||
- Every changed line must trace to one of the five defects — no unrelated refactoring.
|
||
|
||
---
|
||
|
||
## File Structure
|
||
|
||
**Harness**
|
||
- `harness/.pi/extensions/reserved-labels.mjs` — modify `isReserved` to normalize + prefix-match.
|
||
- `harness/.pi/extensions/gate/__tests__/reserved_labels.test.js` — new unit test.
|
||
|
||
**Frontend**
|
||
- `frontend/src/viewers/SchemaLinkingViewer.tsx` — replace `buildFlowchart` with exported `buildErDiagram` (Mermaid `erDiagram`).
|
||
- `frontend/src/viewers/SchemaLinkingViewer.test.tsx` — add a `buildErDiagram` unit test.
|
||
- `frontend/src/viewers/ArtifactView.tsx` — new: route `artifact.kind` → viewer, defensive on `data`.
|
||
- `frontend/src/viewers/ArtifactView.test.tsx` — new.
|
||
- `frontend/src/widgets/ArtifactGateWidget.tsx` — rewrite body to render a 90% modal using `ArtifactView` (keep the export name and filename).
|
||
- `frontend/src/widgets/ArtifactGateWidget.test.tsx` — update to the modal behaviour.
|
||
- `frontend/src/shell/WorkflowBar.tsx` — `finalized`/`createdAt`/`updatedAt` props; all-green on finalized; render `ElapsedTimer`.
|
||
- `frontend/src/shell/WorkflowBar.test.tsx` — new.
|
||
- `frontend/src/shell/ElapsedTimer.tsx` — new.
|
||
- `frontend/src/shell/ElapsedTimer.test.tsx` — new.
|
||
- `frontend/src/shell/SteerInput.tsx` — set `currentPhase="F1"` on new-question creation.
|
||
- `frontend/src/shell/AppShell.tsx` — pass `finalized`/`createdAt`/`updatedAt` to `WorkflowBar`.
|
||
- `frontend/src/shell/f1-loop.test.tsx` — assert optimistic F1 after a new question.
|
||
|
||
---
|
||
|
||
## Task 1: Harness — robust `isReserved` (fix duplicate Altro/other)
|
||
|
||
**Files:**
|
||
- Modify: `harness/.pi/extensions/reserved-labels.mjs`
|
||
- Test: `harness/.pi/extensions/gate/__tests__/reserved_labels.test.js`
|
||
|
||
**Interfaces:**
|
||
- Consumes: nothing new.
|
||
- Produces: `isReserved(label: string): boolean` — true for any label whose normalized form starts with `altro` or `other`, or equals the normalized quit/back canonical labels. `stripReserved(labels: string[]): string[]` unchanged in signature; inherits the new matcher. Used by `tht-gate.js` `reviewer_select`/`reviewer_decide` to drop model-supplied free-text options.
|
||
|
||
- [ ] **Step 1: Write the failing test**
|
||
|
||
Create `harness/.pi/extensions/gate/__tests__/reserved_labels.test.js`:
|
||
|
||
```javascript
|
||
const test = require("node:test");
|
||
const assert = require("node:assert");
|
||
const { isReserved, stripReserved } = require("../../reserved-labels.mjs");
|
||
|
||
test("Altro variants (any punctuation/case) are reserved", () => {
|
||
assert.equal(isReserved("Altro — specifica…"), true);
|
||
assert.equal(isReserved("Altro - specificare"), true);
|
||
assert.equal(isReserved("altro"), true);
|
||
assert.equal(isReserved("ALTRO (specificare)"), true);
|
||
});
|
||
|
||
test("English Other variant is reserved", () => {
|
||
assert.equal(isReserved("Other — specify"), true);
|
||
assert.equal(isReserved("other"), true);
|
||
});
|
||
|
||
test("canonical quit/back labels stay reserved", () => {
|
||
assert.equal(isReserved("Esci da Pi (/quit)"), true);
|
||
assert.equal(isReserved("Torna indietro (fase precedente)"), true);
|
||
});
|
||
|
||
test("normal merit options are NOT reserved", () => {
|
||
assert.equal(isReserved("procedura"), false);
|
||
assert.equal(isReserved("patologia"), false);
|
||
assert.equal(isReserved("altrove"), false); // starts with "altro"? no — "altrove" -> normalized "altrove" starts with "altro" -> guard below
|
||
});
|
||
|
||
test("stripReserved drops every Altro/other variant, keeps merit order", () => {
|
||
assert.deepEqual(
|
||
stripReserved(["procedura", "Altro - specificare", "patologia", "other"]),
|
||
["procedura", "patologia"],
|
||
);
|
||
});
|
||
```
|
||
|
||
Note: `"altrove"` normalizes to `"altrove"`, which *does* start with `"altro"`. To avoid stripping a legitimate option, match the whole first token equals `altro`/`other` rather than a raw `startsWith`. The implementation below tokenizes, so `"altrove"` (single token `altrove`) is NOT reserved. Keep this test line as the guard.
|
||
|
||
- [ ] **Step 2: Run test to verify it fails**
|
||
|
||
Run: `cd harness && node --test .pi/extensions/gate/__tests__/reserved_labels.test.js`
|
||
Expected: FAIL — `isReserved("Altro - specificare")` returns `false` (exact-match implementation).
|
||
|
||
- [ ] **Step 3: Implement the normalized matcher**
|
||
|
||
Replace the body of `harness/.pi/extensions/reserved-labels.mjs` from the `const RESERVED` line through `stripReserved` with:
|
||
|
||
```javascript
|
||
// Normalize a label to lowercase ASCII tokens: strip diacritics, turn every run
|
||
// of punctuation/space into a single space, trim. "Altro — specifica…" -> "altro specifica".
|
||
function normalize(label) {
|
||
return String(label)
|
||
.normalize("NFD")
|
||
.replace(/[̀-ͯ]/g, "")
|
||
.toLowerCase()
|
||
.replace(/[^a-z0-9]+/g, " ")
|
||
.trim();
|
||
}
|
||
|
||
const NORM_QUIT = normalize(QUIT_LABEL);
|
||
const NORM_BACK = normalize(BACK_LABEL);
|
||
|
||
// true if the label is one the gate adds itself. Robust to the variants models
|
||
// emit ("Altro - specificare", "altro", English "Other — specify"): reserved when
|
||
// the FIRST normalized token is exactly "altro"/"other", or the whole normalized
|
||
// label equals the canonical quit/back labels. First-token match keeps real
|
||
// options like "altrove" out of the reserved set.
|
||
export function isReserved(label) {
|
||
const n = normalize(label);
|
||
if (!n) return false;
|
||
const first = n.split(" ")[0];
|
||
return first === "altro" || first === "other" || n === NORM_QUIT || n === NORM_BACK;
|
||
}
|
||
|
||
// removes every reserved entry from a list of labels, preserving order and normal
|
||
// entries. Idempotent.
|
||
export function stripReserved(labels) {
|
||
return labels.filter((label) => !isReserved(label));
|
||
}
|
||
```
|
||
|
||
Leave the `export const ALTRO / QUIT_LABEL / BACK_LABEL / CONTROL_LABELS` lines at the top of the file unchanged (still imported elsewhere). Remove the now-unused `const RESERVED = new Set([...])` line.
|
||
|
||
- [ ] **Step 4: Run tests to verify they pass**
|
||
|
||
Run: `cd harness && node --test .pi/extensions/gate/__tests__/reserved_labels.test.js`
|
||
Expected: PASS (all 5 tests).
|
||
|
||
Run the whole gate suite to confirm no regression: `cd harness && npm test`
|
||
Expected: PASS (existing `builders.test.js` assertion "select no longer injects an Altro option" and the rest stay green).
|
||
|
||
- [ ] **Step 5: Commit**
|
||
|
||
```bash
|
||
git add harness/.pi/extensions/reserved-labels.mjs harness/.pi/extensions/gate/__tests__/reserved_labels.test.js
|
||
git commit -m "fix(gate): robust isReserved strips model Altro/other variants (no duplicate free-text option)"
|
||
```
|
||
|
||
---
|
||
|
||
## Task 2: Frontend — Mermaid `erDiagram` for schema linking
|
||
|
||
**Files:**
|
||
- Modify: `frontend/src/viewers/SchemaLinkingViewer.tsx` (replace `buildFlowchart`, ~lines 36-57 and its call ~line 92)
|
||
- Test: `frontend/src/viewers/SchemaLinkingViewer.test.tsx`
|
||
|
||
**Interfaces:**
|
||
- Consumes: `Candidate`, `Join` (already exported from `SchemaLinkingViewer.tsx`).
|
||
- Produces: `export function buildErDiagram(promoted: Candidate[], joins: Join[]): string` — a Mermaid `erDiagram` string: promoted tables as entities, their promoted `table.column` candidates as attributes, `joins` (resolved to owning tables) as relationships. Consumed by `ArtifactView` indirectly (via the component) and unit-tested here.
|
||
|
||
- [ ] **Step 1: Write the failing test**
|
||
|
||
Add to `frontend/src/viewers/SchemaLinkingViewer.test.tsx` — update the import on line 3 and append the test:
|
||
|
||
```tsx
|
||
import { SchemaLinkingViewer, buildErDiagram } from "./SchemaLinkingViewer";
|
||
```
|
||
|
||
```tsx
|
||
test("(e) buildErDiagram emits entities, attributes and a relation", () => {
|
||
const def = buildErDiagram(
|
||
[
|
||
{ kind: "table", name: "orders", decision: "promoted" },
|
||
{ kind: "column", name: "orders.id", decision: "promoted" },
|
||
{ kind: "table", name: "customers", decision: "promoted" },
|
||
],
|
||
[{ from: "orders.customer_id", to: "customers.id" }],
|
||
);
|
||
expect(def.startsWith("erDiagram")).toBe(true);
|
||
expect(def).toContain("orders {");
|
||
expect(def).toContain("col id");
|
||
expect(def).toContain("orders }o--o{ customers : join");
|
||
});
|
||
```
|
||
|
||
- [ ] **Step 2: Run test to verify it fails**
|
||
|
||
Run: `cd frontend && npx vitest run src/viewers/SchemaLinkingViewer.test.tsx -t buildErDiagram`
|
||
Expected: FAIL — `buildErDiagram` is not exported (`buildErDiagram is not a function`).
|
||
|
||
- [ ] **Step 3: Replace `buildFlowchart` with `buildErDiagram`**
|
||
|
||
In `frontend/src/viewers/SchemaLinkingViewer.tsx`, delete the `buildFlowchart` function (lines ~36-57) and add:
|
||
|
||
```tsx
|
||
export function buildErDiagram(promoted: Candidate[], joins: Join[]): string {
|
||
const sanitize = (name: string) => name.replace(/[^a-zA-Z0-9]/g, "_");
|
||
const tables = promoted.filter((c) => c.kind === "table");
|
||
const tableNames = new Set(tables.map((t) => t.name));
|
||
const columns = promoted.filter((c) => c.kind === "column");
|
||
|
||
const lines: string[] = ["erDiagram"];
|
||
|
||
for (const t of tables) {
|
||
const id = sanitize(t.name);
|
||
const cols = columns.filter((col) => col.name.startsWith(t.name + "."));
|
||
lines.push(` ${id} {`);
|
||
for (const col of cols) {
|
||
lines.push(` col ${sanitize(col.name.slice(t.name.length + 1))}`);
|
||
}
|
||
lines.push(` }`);
|
||
}
|
||
|
||
// A join endpoint may be "table" or "table.column"; resolve to its owning table.
|
||
// Draw a relationship only between two DISTINCT promoted tables, once per pair.
|
||
const owningTable = (ref: string) =>
|
||
ref.includes(".") ? ref.slice(0, ref.indexOf(".")) : ref;
|
||
const seen = new Set<string>();
|
||
for (const j of joins) {
|
||
const a = owningTable(j.from);
|
||
const b = owningTable(j.to);
|
||
if (a === b || !tableNames.has(a) || !tableNames.has(b)) continue;
|
||
const key = [a, b].sort().join("::");
|
||
if (seen.has(key)) continue;
|
||
seen.add(key);
|
||
lines.push(` ${sanitize(a)} }o--o{ ${sanitize(b)} : join`);
|
||
}
|
||
|
||
return lines.join("\n");
|
||
}
|
||
```
|
||
|
||
Then update the effect that builds the diagram (~line 92): change
|
||
|
||
```tsx
|
||
const def = buildFlowchart(promoted, linking.joins);
|
||
```
|
||
|
||
to
|
||
|
||
```tsx
|
||
const def = buildErDiagram(promoted, linking.joins);
|
||
```
|
||
|
||
- [ ] **Step 4: Run tests to verify they pass**
|
||
|
||
Run: `cd frontend && npx vitest run src/viewers/SchemaLinkingViewer.test.tsx`
|
||
Expected: PASS — the new `(e)` test plus the existing `(a)-(d)` tests (they mock `renderMermaid`, so the diagram string change doesn't affect them).
|
||
|
||
Run: `cd frontend && npx tsc -b`
|
||
Expected: no errors.
|
||
|
||
- [ ] **Step 5: Commit**
|
||
|
||
```bash
|
||
git add frontend/src/viewers/SchemaLinkingViewer.tsx frontend/src/viewers/SchemaLinkingViewer.test.tsx
|
||
git commit -m "feat(viewer): schema linking renders a Mermaid erDiagram (tables + relations)"
|
||
```
|
||
|
||
---
|
||
|
||
## Task 3: Frontend — `ArtifactView` (render artifact.data by kind)
|
||
|
||
**Files:**
|
||
- Create: `frontend/src/viewers/ArtifactView.tsx`
|
||
- Test: `frontend/src/viewers/ArtifactView.test.tsx`
|
||
|
||
**Interfaces:**
|
||
- Consumes: `SqlViewer` + `SqlBlock` (`./SqlViewer`), `SchemaLinkingViewer` + `SchemaLinking` (`./SchemaLinkingViewer`), `MarkdownView` (`./MarkdownView`).
|
||
- Produces: `export function ArtifactView({ artifact }: { artifact: { kind: string; data?: unknown; content?: unknown; [k: string]: unknown } }): ReactElement`. Routes on `artifact.kind`, defensive about `data` (string or object), JSON fallback otherwise. Consumed by `ArtifactGateWidget` (Task 4).
|
||
|
||
- [ ] **Step 1: Write the failing test**
|
||
|
||
Create `frontend/src/viewers/ArtifactView.test.tsx`:
|
||
|
||
```tsx
|
||
import { render, screen } from "@testing-library/react";
|
||
import { ArtifactView } from "./ArtifactView";
|
||
|
||
vi.mock("./mermaid", () => ({
|
||
renderMermaid: vi.fn().mockResolvedValue('<svg data-testid="mm"></svg>'),
|
||
}));
|
||
|
||
test("sql artifact renders the SQL text", () => {
|
||
render(<ArtifactView artifact={{ kind: "sql", data: "SELECT 1" }} />);
|
||
expect(screen.getByText("SELECT 1")).toBeInTheDocument();
|
||
});
|
||
|
||
test("cte_plan renders an ordered list of names", () => {
|
||
render(<ArtifactView artifact={{ kind: "cte_plan", data: { names: ["a_cte", "b_cte"] } }} />);
|
||
expect(screen.getByText("a_cte")).toBeInTheDocument();
|
||
expect(screen.getByText("b_cte")).toBeInTheDocument();
|
||
});
|
||
|
||
test("question renders markdown headings", () => {
|
||
render(<ArtifactView artifact={{ kind: "question", data: "## Domanda\nrevised" }} />);
|
||
expect(screen.getByText("Domanda")).toBeInTheDocument();
|
||
});
|
||
|
||
test("unknown kind falls back to formatted JSON", () => {
|
||
render(<ArtifactView artifact={{ kind: "mystery", data: { a: 1 } }} />);
|
||
expect(screen.getByText(/"a": 1/)).toBeInTheDocument();
|
||
});
|
||
|
||
test("schema_linking renders the schema viewer", async () => {
|
||
render(
|
||
<ArtifactView
|
||
artifact={{
|
||
kind: "schema_linking",
|
||
data: { candidates: [{ kind: "table", name: "orders", decision: "promoted" }], joins: [] },
|
||
}}
|
||
/>,
|
||
);
|
||
expect(await screen.findByTestId("mm")).toBeInTheDocument();
|
||
});
|
||
```
|
||
|
||
- [ ] **Step 2: Run test to verify it fails**
|
||
|
||
Run: `cd frontend && npx vitest run src/viewers/ArtifactView.test.tsx`
|
||
Expected: FAIL — cannot resolve `./ArtifactView`.
|
||
|
||
- [ ] **Step 3: Implement `ArtifactView`**
|
||
|
||
Create `frontend/src/viewers/ArtifactView.tsx`:
|
||
|
||
```tsx
|
||
import type { ReactElement } from "react";
|
||
import { SqlViewer, type SqlBlock } from "./SqlViewer";
|
||
import { SchemaLinkingViewer, type SchemaLinking } from "./SchemaLinkingViewer";
|
||
import { MarkdownView } from "./MarkdownView";
|
||
|
||
type ArtifactData = { kind: string; data?: unknown; content?: unknown; [k: string]: unknown };
|
||
|
||
function asRecord(v: unknown): Record<string, unknown> | null {
|
||
return v && typeof v === "object" && !Array.isArray(v) ? (v as Record<string, unknown>) : null;
|
||
}
|
||
|
||
// The payload lives in `data` (harness contract); fall back to `content`, then the
|
||
// artifact object itself, so older/other producers still render something.
|
||
function payload(artifact: ArtifactData): unknown {
|
||
if (artifact.data !== undefined) return artifact.data;
|
||
if (artifact.content !== undefined) return artifact.content;
|
||
return artifact;
|
||
}
|
||
|
||
function toSchemaLinking(raw: unknown): SchemaLinking | null {
|
||
let obj: unknown = raw;
|
||
if (typeof raw === "string") {
|
||
try { obj = JSON.parse(raw); } catch { return null; }
|
||
}
|
||
const rec = asRecord(obj);
|
||
if (!rec || !Array.isArray(rec.candidates)) return null;
|
||
return {
|
||
candidates: rec.candidates as SchemaLinking["candidates"],
|
||
joins: Array.isArray(rec.joins) ? (rec.joins as SchemaLinking["joins"]) : [],
|
||
excluded: Array.isArray(rec.excluded) ? (rec.excluded as SchemaLinking["excluded"]) : [],
|
||
open_questions: Array.isArray(rec.open_questions) ? (rec.open_questions as string[]) : [],
|
||
question: typeof rec.question === "string" ? rec.question : undefined,
|
||
};
|
||
}
|
||
|
||
function toSqlBlocks(raw: unknown): SqlBlock[] | null {
|
||
if (typeof raw === "string") return [{ name: "SQL", sql: raw }];
|
||
const rec = asRecord(raw);
|
||
if (!rec) return null;
|
||
if (typeof rec.sql === "string") return [{ name: "SQL", sql: rec.sql }];
|
||
if (Array.isArray(rec.ctes)) {
|
||
const blocks = rec.ctes
|
||
.map((c) => asRecord(c))
|
||
.filter((c): c is Record<string, unknown> => !!c && typeof c.sql === "string")
|
||
.map((c) => ({ name: typeof c.name === "string" ? c.name : "cte", sql: c.sql as string }));
|
||
if (typeof rec.final === "string") blocks.push({ name: "final", sql: rec.final });
|
||
return blocks.length ? blocks : null;
|
||
}
|
||
return null;
|
||
}
|
||
|
||
function toMarkdown(raw: unknown): string | null {
|
||
if (typeof raw === "string") return raw;
|
||
const rec = asRecord(raw);
|
||
if (!rec) return null;
|
||
for (const k of ["markdown", "question", "text"]) {
|
||
if (typeof rec[k] === "string") return rec[k] as string;
|
||
}
|
||
return null;
|
||
}
|
||
|
||
function cteNames(raw: unknown): string[] | null {
|
||
if (Array.isArray(raw) && raw.every((x) => typeof x === "string")) return raw as string[];
|
||
const rec = asRecord(raw);
|
||
if (rec && Array.isArray(rec.names) && rec.names.every((x) => typeof x === "string")) {
|
||
return rec.names as string[];
|
||
}
|
||
return null;
|
||
}
|
||
|
||
function JsonFallback({ value }: { value: unknown }): ReactElement {
|
||
const text = typeof value === "string" ? value : JSON.stringify(value, null, 2);
|
||
return (
|
||
<pre className="overflow-auto whitespace-pre-wrap rounded bg-muted p-3 text-sm">{text}</pre>
|
||
);
|
||
}
|
||
|
||
export function ArtifactView({ artifact }: { artifact: ArtifactData }): ReactElement {
|
||
const kind = artifact.kind ?? "";
|
||
const data = payload(artifact);
|
||
|
||
if (kind === "schema_linking") {
|
||
const linking = toSchemaLinking(data);
|
||
if (linking) return <SchemaLinkingViewer linking={linking} />;
|
||
}
|
||
if (kind === "sql" || kind === "cte_result") {
|
||
const blocks = toSqlBlocks(data);
|
||
if (blocks) return <SqlViewer blocks={blocks} />;
|
||
}
|
||
if (kind === "cte_plan") {
|
||
const names = cteNames(data);
|
||
if (names) {
|
||
return (
|
||
<ol className="list-decimal pl-6 text-sm">
|
||
{names.map((n, i) => (
|
||
<li key={`${n}-${i}`} className="font-mono">{n}</li>
|
||
))}
|
||
</ol>
|
||
);
|
||
}
|
||
}
|
||
if (kind === "question" || kind === "phase") {
|
||
const md = toMarkdown(data);
|
||
if (md !== null) return <MarkdownView source={md} />;
|
||
}
|
||
return <JsonFallback value={data} />;
|
||
}
|
||
```
|
||
|
||
- [ ] **Step 4: Run tests to verify they pass**
|
||
|
||
Run: `cd frontend && npx vitest run src/viewers/ArtifactView.test.tsx`
|
||
Expected: PASS (5 tests). (The `sql` test reads the un-highlighted `<pre>` fallback that `SqlViewer` shows before async highlight resolves.)
|
||
|
||
Run: `cd frontend && npx tsc -b`
|
||
Expected: no errors.
|
||
|
||
- [ ] **Step 5: Commit**
|
||
|
||
```bash
|
||
git add frontend/src/viewers/ArtifactView.tsx frontend/src/viewers/ArtifactView.test.tsx
|
||
git commit -m "feat(viewer): ArtifactView routes artifact.data by kind (schema/sql/cte/question)"
|
||
```
|
||
|
||
---
|
||
|
||
## Task 4: Frontend — artifact gate as a 90% modal
|
||
|
||
**Files:**
|
||
- Modify (rewrite body, keep export name): `frontend/src/widgets/ArtifactGateWidget.tsx`
|
||
- Test: `frontend/src/widgets/ArtifactGateWidget.test.tsx`
|
||
|
||
**Interfaces:**
|
||
- Consumes: `ArtifactView` (`../viewers/ArtifactView`), `Dialog`/`DialogContent`/`DialogTitle` (`../components/ui/dialog`), `ReservedControls`, `LinkageHost`, `WidgetProps`.
|
||
- Produces: `ArtifactGateWidget` (unchanged name; still registered for `"artifact-gate"` in `widgets/index.ts`). Renders a `90vw × 90vh` modal: artifact on top via `ArtifactView`, action bar (options + reserved) at the bottom. Same `onRespond` payloads as before: `{ id, kind: "artifact-gate", choices: [id] }`, linkage merges child `text`, reserved → `{ id, control, text? }`.
|
||
|
||
- [ ] **Step 1: Write the failing test**
|
||
|
||
Replace the first test in `frontend/src/widgets/ArtifactGateWidget.test.tsx` (the "renders artifact content in a pre block" test, lines ~5-19) with two tests, and add a `vi.mock` for mermaid at the top. Final file top + first tests:
|
||
|
||
```tsx
|
||
import { render, screen } from "@testing-library/react";
|
||
import userEvent from "@testing-library/user-event";
|
||
import { ArtifactGateWidget } from "./ArtifactGateWidget";
|
||
|
||
vi.mock("../viewers/mermaid", () => ({
|
||
renderMermaid: vi.fn().mockResolvedValue('<svg data-testid="mm"></svg>'),
|
||
}));
|
||
|
||
test("renders inside a dialog and shows the artifact via ArtifactView (data path)", () => {
|
||
const onRespond = vi.fn();
|
||
render(
|
||
<ArtifactGateWidget
|
||
descriptor={{
|
||
id: "u1",
|
||
widget: "artifact-gate",
|
||
title: "SQL finale",
|
||
artifact: { kind: "sql", data: "SELECT 42" },
|
||
options: [{ id: "approve", label: "Approva" }],
|
||
}}
|
||
onRespond={onRespond}
|
||
/>
|
||
);
|
||
expect(screen.getByRole("dialog")).toBeInTheDocument();
|
||
expect(screen.getByText("SELECT 42")).toBeInTheDocument();
|
||
});
|
||
```
|
||
|
||
Keep the existing "clicking an option without opens responds immediately" and "reserved control responds with control field" tests as-is (lines ~21-54) — they still describe the modal's behaviour. The reserved test's descriptor `artifact: { kind: "cte", content: "SELECT 1" }` now renders through `ArtifactView`'s JSON fallback (kind `cte` is unknown; `content` string → `JsonFallback` prints `SELECT 1`), which does not affect the button assertions.
|
||
|
||
- [ ] **Step 2: Run test to verify it fails**
|
||
|
||
Run: `cd frontend && npx vitest run src/widgets/ArtifactGateWidget.test.tsx -t "inside a dialog"`
|
||
Expected: FAIL — current widget renders a `<pre>`, no `role="dialog"`.
|
||
|
||
- [ ] **Step 3: Rewrite `ArtifactGateWidget.tsx` as the modal**
|
||
|
||
Replace the entire contents of `frontend/src/widgets/ArtifactGateWidget.tsx` with:
|
||
|
||
```tsx
|
||
import { useState } from "react";
|
||
import type { WidgetProps } from "./types";
|
||
import type { UiResponse, WidgetDescriptor } from "../api/types";
|
||
import { Dialog, DialogContent, DialogTitle } from "../components/ui/dialog";
|
||
import { ArtifactView } from "../viewers/ArtifactView";
|
||
import { ReservedControls } from "./ReservedControls";
|
||
import { LinkageHost } from "./LinkageHost";
|
||
|
||
/**
|
||
* Artifact review gate rendered as a full-screen (90%) modal: the artifact fills
|
||
* the top (scrollable) area via ArtifactView; the action bar (options + reserved
|
||
* controls) sits at the bottom. The gate contract forbids silent dismissal, so the
|
||
* dialog has no close button and is not closeable by Esc/backdrop — the only way
|
||
* out is an action or a reserved control, both of which call onRespond.
|
||
*/
|
||
export function ArtifactGateWidget({ descriptor, onRespond }: WidgetProps) {
|
||
const [pendingLinkage, setPendingLinkage] = useState<{
|
||
parentResponse: UiResponse;
|
||
childDescriptor: WidgetDescriptor;
|
||
} | null>(null);
|
||
|
||
function handleOption(optionId: string) {
|
||
const option = descriptor.options?.find((o) => o.id === optionId);
|
||
const parentResponse: UiResponse = { id: descriptor.id, kind: "artifact-gate", choices: [optionId] };
|
||
if (option?.opens) setPendingLinkage({ parentResponse, childDescriptor: option.opens });
|
||
else onRespond(parentResponse);
|
||
}
|
||
|
||
return (
|
||
<Dialog open>
|
||
<DialogContent
|
||
showCloseButton={false}
|
||
className="grid h-[90vh] w-[90vw] max-w-[90vw] grid-rows-[auto_1fr_auto] gap-3 sm:max-w-[90vw]"
|
||
>
|
||
<DialogTitle>{descriptor.title ?? "Artifact review"}</DialogTitle>
|
||
|
||
<div className="min-h-0 overflow-auto rounded border bg-background p-3">
|
||
{descriptor.artifact ? (
|
||
<ArtifactView artifact={descriptor.artifact} />
|
||
) : (
|
||
<p className="text-sm text-muted-foreground">No artifact.</p>
|
||
)}
|
||
</div>
|
||
|
||
<div className="flex flex-col gap-2 border-t pt-3">
|
||
{pendingLinkage ? (
|
||
<LinkageHost
|
||
parentResponse={pendingLinkage.parentResponse}
|
||
childDescriptor={pendingLinkage.childDescriptor}
|
||
onRespond={onRespond}
|
||
/>
|
||
) : (
|
||
<>
|
||
<div className="flex flex-wrap gap-2">
|
||
{descriptor.options?.map((o) => (
|
||
<button
|
||
key={o.id}
|
||
className="rounded border px-3 py-2 text-left hover:bg-accent"
|
||
onClick={() => handleOption(o.id)}
|
||
>
|
||
{o.label}
|
||
</button>
|
||
))}
|
||
</div>
|
||
<ReservedControls
|
||
reserved={descriptor.reserved}
|
||
onControl={(c, t) =>
|
||
onRespond({ id: descriptor.id, control: c, ...(t !== undefined ? { text: t } : {}) })
|
||
}
|
||
/>
|
||
</>
|
||
)}
|
||
</div>
|
||
</DialogContent>
|
||
</Dialog>
|
||
);
|
||
}
|
||
```
|
||
|
||
- [ ] **Step 4: Run tests to verify they pass**
|
||
|
||
Run: `cd frontend && npx vitest run src/widgets/ArtifactGateWidget.test.tsx src/widgets/linkage.test.tsx`
|
||
Expected: PASS — the new dialog test, the two kept option/reserved tests, and both `linkage.test.tsx` tests (linkage flow is unchanged).
|
||
|
||
Run: `cd frontend && npx tsc -b`
|
||
Expected: no errors.
|
||
|
||
- [ ] **Step 5: Commit**
|
||
|
||
```bash
|
||
git add frontend/src/widgets/ArtifactGateWidget.tsx frontend/src/widgets/ArtifactGateWidget.test.tsx
|
||
git commit -m "feat(gate): render reviewer artifacts in a 90% modal (fix empty artifact.data gate)"
|
||
```
|
||
|
||
---
|
||
|
||
## Task 5: Frontend — phase-circle lifecycle (optimistic F1 + finalized all-green)
|
||
|
||
**Files:**
|
||
- Modify: `frontend/src/shell/WorkflowBar.tsx`
|
||
- Modify: `frontend/src/shell/SteerInput.tsx` (submit path, ~lines 29-45)
|
||
- Modify: `frontend/src/shell/AppShell.tsx` (WorkflowBar render, ~line 172)
|
||
- Modify: `frontend/src/shell/f1-loop.test.tsx` (add optimistic-F1 assertion)
|
||
- Test: `frontend/src/shell/WorkflowBar.test.tsx`
|
||
|
||
**Interfaces:**
|
||
- Consumes: `useSessionStore` `currentPhase`/`phaseError`/`setPhase`.
|
||
- Produces: `WorkflowBar({ finalized }: { finalized?: boolean })` — when `finalized`, every dot is `data-state="done"`. Optimistic F1: `SteerInput` calls `setPhase("F1")` right after a new-question `createSession`.
|
||
|
||
- [ ] **Step 1: Write the failing tests**
|
||
|
||
Create `frontend/src/shell/WorkflowBar.test.tsx`:
|
||
|
||
```tsx
|
||
import { render, screen } from "@testing-library/react";
|
||
import { WorkflowBar } from "./WorkflowBar";
|
||
import { useSessionStore } from "../store/sessionStore";
|
||
|
||
beforeEach(() => useSessionStore.getState().resetSession());
|
||
|
||
test("currentPhase F1 renders F1 as running (yellow)", () => {
|
||
useSessionStore.getState().setPhase("F1");
|
||
render(<WorkflowBar />);
|
||
expect(screen.getByTestId("phase-F1")).toHaveAttribute("data-state", "running");
|
||
});
|
||
|
||
test("a phase before the active one is done (green)", () => {
|
||
useSessionStore.getState().setPhase("F3");
|
||
render(<WorkflowBar />);
|
||
expect(screen.getByTestId("phase-F1")).toHaveAttribute("data-state", "done");
|
||
expect(screen.getByTestId("phase-F3")).toHaveAttribute("data-state", "running");
|
||
expect(screen.getByTestId("phase-F5")).toHaveAttribute("data-state", "pending");
|
||
});
|
||
|
||
test("finalized marks all phases done (green)", () => {
|
||
useSessionStore.getState().setPhase("F8");
|
||
render(<WorkflowBar finalized />);
|
||
for (const id of ["F1", "F4", "F8"]) {
|
||
expect(screen.getByTestId(`phase-${id}`)).toHaveAttribute("data-state", "done");
|
||
}
|
||
});
|
||
```
|
||
|
||
Also add, in `frontend/src/shell/f1-loop.test.tsx`, an optimistic-F1 assertion right after the SSE connects (after line 39 `await waitFor(() => expect(FakeEventSource.instances).toHaveLength(1));`):
|
||
|
||
```tsx
|
||
// Optimistic lifecycle: a brand-new question paints F1 immediately (before any gate).
|
||
await waitFor(() => expect(useSessionStore.getState().currentPhase).toBe("F1"));
|
||
```
|
||
|
||
- [ ] **Step 2: Run tests to verify they fail**
|
||
|
||
Run: `cd frontend && npx vitest run src/shell/WorkflowBar.test.tsx src/shell/f1-loop.test.tsx`
|
||
Expected: FAIL — `finalized` prop has no effect yet; `currentPhase` stays `null` after a new question.
|
||
|
||
- [ ] **Step 3: Implement the lifecycle changes**
|
||
|
||
In `frontend/src/shell/WorkflowBar.tsx`, change the signature and the state computation:
|
||
|
||
```tsx
|
||
export function WorkflowBar({ finalized = false }: { finalized?: boolean }) {
|
||
const currentPhase = useSessionStore((s) => s.currentPhase);
|
||
const phaseError = useSessionStore((s) => s.phaseError);
|
||
const activeIdx = PHASES.findIndex((p) => p.id === currentPhase);
|
||
```
|
||
|
||
Inside the `PHASES.map`, replace the `state` and `connectorDone` computations with:
|
||
|
||
```tsx
|
||
const isActive = currentPhase === p.id;
|
||
const isDone = activeIdx > -1 && i < activeIdx;
|
||
const state: DotState = finalized
|
||
? "done"
|
||
: isActive
|
||
? phaseError === p.id
|
||
? "error"
|
||
: "running"
|
||
: isDone
|
||
? "done"
|
||
: "pending";
|
||
const connectorDone = finalized || (activeIdx > -1 && i <= activeIdx);
|
||
```
|
||
|
||
In `frontend/src/shell/SteerInput.tsx`, add `setPhase` from the store and call it on new-question creation. Change the store hook (line ~27) and the `else` branch of `submit` (lines ~36-39):
|
||
|
||
```tsx
|
||
const setLastUserEntry = useSessionStore((s) => s.setLastUserEntry);
|
||
const setPhase = useSessionStore((s) => s.setPhase);
|
||
```
|
||
|
||
```tsx
|
||
} else {
|
||
const { id } = await createSession({ question: trimmed });
|
||
setPhase("F1"); // optimistic: paint F1 yellow during the cold start, before the first gate
|
||
onSessionCreated?.(id);
|
||
}
|
||
```
|
||
|
||
In `frontend/src/shell/AppShell.tsx`, derive `finalized` and pass it. Add near the other derived values (after line ~54 `const refresh = ...` or beside the `working` computation ~line 134):
|
||
|
||
```tsx
|
||
const activeSession = sessions.find((s) => s.id === activeSessionId) ?? null;
|
||
const finalized = activeSession?.status === "finalized";
|
||
```
|
||
|
||
Then change the `<WorkflowBar />` render (line ~172) to:
|
||
|
||
```tsx
|
||
<WorkflowBar finalized={finalized} />
|
||
```
|
||
|
||
- [ ] **Step 4: Run tests to verify they pass**
|
||
|
||
Run: `cd frontend && npx vitest run src/shell/WorkflowBar.test.tsx src/shell/f1-loop.test.tsx`
|
||
Expected: PASS.
|
||
|
||
Run: `cd frontend && npx tsc -b`
|
||
Expected: no errors.
|
||
|
||
- [ ] **Step 5: Commit**
|
||
|
||
```bash
|
||
git add frontend/src/shell/WorkflowBar.tsx frontend/src/shell/SteerInput.tsx frontend/src/shell/AppShell.tsx frontend/src/shell/WorkflowBar.test.tsx frontend/src/shell/f1-loop.test.tsx
|
||
git commit -m "feat(workflow-bar): optimistic F1 at start; all-green when session finalized"
|
||
```
|
||
|
||
---
|
||
|
||
## Task 6: Frontend — total elapsed timer
|
||
|
||
**Files:**
|
||
- Create: `frontend/src/shell/ElapsedTimer.tsx`
|
||
- Test: `frontend/src/shell/ElapsedTimer.test.tsx`
|
||
- Modify: `frontend/src/shell/WorkflowBar.tsx` (render the timer after the dots)
|
||
- Modify: `frontend/src/shell/AppShell.tsx` (pass `createdAt`/`updatedAt`, with a local fallback)
|
||
|
||
**Interfaces:**
|
||
- Consumes: nothing new.
|
||
- Produces: `export function ElapsedTimer({ startedAt, stoppedAt }: { startedAt: string | null; stoppedAt?: string | null }): ReactElement | null` — ticks every second from `startedAt` until `stoppedAt` is set (then frozen); renders `null` when `startedAt` is null. Format `Xm Ys`. `WorkflowBar` gains `createdAt`/`updatedAt` props and renders `<ElapsedTimer>` after the phase dots.
|
||
|
||
- [ ] **Step 1: Write the failing test**
|
||
|
||
Create `frontend/src/shell/ElapsedTimer.test.tsx`:
|
||
|
||
```tsx
|
||
import { render, screen } from "@testing-library/react";
|
||
import { ElapsedTimer } from "./ElapsedTimer";
|
||
|
||
test("renders nothing without a start time", () => {
|
||
const { container } = render(<ElapsedTimer startedAt={null} />);
|
||
expect(container).toBeEmptyDOMElement();
|
||
});
|
||
|
||
test("frozen elapsed when stoppedAt is set (2m 5s)", () => {
|
||
render(
|
||
<ElapsedTimer
|
||
startedAt="2026-07-03T10:00:00.000Z"
|
||
stoppedAt="2026-07-03T10:02:05.000Z"
|
||
/>,
|
||
);
|
||
expect(screen.getByText("2m 5s")).toBeInTheDocument();
|
||
});
|
||
```
|
||
|
||
- [ ] **Step 2: Run test to verify it fails**
|
||
|
||
Run: `cd frontend && npx vitest run src/shell/ElapsedTimer.test.tsx`
|
||
Expected: FAIL — cannot resolve `./ElapsedTimer`.
|
||
|
||
- [ ] **Step 3: Implement `ElapsedTimer` and render it in `WorkflowBar`**
|
||
|
||
Create `frontend/src/shell/ElapsedTimer.tsx`:
|
||
|
||
```tsx
|
||
import { useEffect, useState } from "react";
|
||
|
||
function fmt(ms: number): string {
|
||
const total = Math.max(0, Math.floor(ms / 1000));
|
||
return `${Math.floor(total / 60)}m ${total % 60}s`;
|
||
}
|
||
|
||
/**
|
||
* Total process time, anchored on the session's created_at (robust to reload and
|
||
* resume). Ticks every second while running; freezes at stoppedAt once the session
|
||
* is finalized. Renders nothing until a start time is known.
|
||
*/
|
||
export function ElapsedTimer({
|
||
startedAt,
|
||
stoppedAt,
|
||
}: {
|
||
startedAt: string | null;
|
||
stoppedAt?: string | null;
|
||
}) {
|
||
const [now, setNow] = useState(() => Date.now());
|
||
|
||
useEffect(() => {
|
||
if (!startedAt || stoppedAt) return;
|
||
const t = setInterval(() => setNow(Date.now()), 1000);
|
||
return () => clearInterval(t);
|
||
}, [startedAt, stoppedAt]);
|
||
|
||
if (!startedAt) return null;
|
||
const start = new Date(startedAt).getTime();
|
||
const end = stoppedAt ? new Date(stoppedAt).getTime() : now;
|
||
return (
|
||
<span
|
||
className="shrink-0 tabular-nums text-xs text-muted-foreground"
|
||
aria-label="Elapsed time"
|
||
title="Total elapsed time"
|
||
>
|
||
{fmt(end - start)}
|
||
</span>
|
||
);
|
||
}
|
||
```
|
||
|
||
In `frontend/src/shell/WorkflowBar.tsx`: import the timer and extend the props, then wrap the returned `<nav>` so the timer sits after the dots.
|
||
|
||
Add the import at the top:
|
||
|
||
```tsx
|
||
import { ElapsedTimer } from "./ElapsedTimer";
|
||
```
|
||
|
||
Change the signature:
|
||
|
||
```tsx
|
||
export function WorkflowBar({
|
||
finalized = false,
|
||
createdAt = null,
|
||
updatedAt = null,
|
||
}: {
|
||
finalized?: boolean;
|
||
createdAt?: string | null;
|
||
updatedAt?: string | null;
|
||
}) {
|
||
```
|
||
|
||
Wrap the return: replace `return (\n <nav ...>` … `</nav>\n );` so the `<nav>` is nested inside a flex row with the timer:
|
||
|
||
```tsx
|
||
return (
|
||
<div className="flex items-center justify-center gap-3">
|
||
<nav
|
||
aria-label="Workflow progress"
|
||
className="flex items-center gap-0 overflow-x-auto py-0.5"
|
||
>
|
||
{/* …existing PHASES.map(...) unchanged… */}
|
||
</nav>
|
||
<ElapsedTimer startedAt={createdAt} stoppedAt={finalized ? updatedAt : null} />
|
||
</div>
|
||
);
|
||
```
|
||
|
||
(Only the wrapper and the removed `justify-center` on the `<nav>` change; the `PHASES.map` body is untouched.)
|
||
|
||
- [ ] **Step 4: Wire `AppShell` and run tests**
|
||
|
||
In `frontend/src/shell/AppShell.tsx`, add a local start fallback and pass the props. Near the `activeSession`/`finalized` lines from Task 5, add:
|
||
|
||
```tsx
|
||
const [localStart, setLocalStart] = useState<string | null>(null);
|
||
useEffect(() => {
|
||
if (activeSessionId && !activeSession) setLocalStart((prev) => prev ?? new Date().toISOString());
|
||
else if (!activeSessionId) setLocalStart(null);
|
||
}, [activeSessionId, activeSession]);
|
||
const startedAt = activeSession?.created_at ?? localStart;
|
||
```
|
||
|
||
Ensure `useEffect` is imported (AppShell already imports from `react`; change line 23 to include it):
|
||
|
||
```tsx
|
||
import { useEffect, useMemo, useRef, useState } from "react";
|
||
```
|
||
|
||
Change the WorkflowBar render (from Task 5) to:
|
||
|
||
```tsx
|
||
<WorkflowBar
|
||
finalized={finalized}
|
||
createdAt={startedAt}
|
||
updatedAt={activeSession?.updated_at ?? null}
|
||
/>
|
||
```
|
||
|
||
Run: `cd frontend && npx vitest run src/shell/ElapsedTimer.test.tsx src/shell/WorkflowBar.test.tsx`
|
||
Expected: PASS (timer unit tests; WorkflowBar tests still pass — no `createdAt` → timer renders null).
|
||
|
||
Run: `cd frontend && npx tsc -b`
|
||
Expected: no errors.
|
||
|
||
- [ ] **Step 5: Commit**
|
||
|
||
```bash
|
||
git add frontend/src/shell/ElapsedTimer.tsx frontend/src/shell/ElapsedTimer.test.tsx frontend/src/shell/WorkflowBar.tsx frontend/src/shell/AppShell.tsx
|
||
git commit -m "feat(workflow-bar): total elapsed timer after the phase circles"
|
||
```
|
||
|
||
---
|
||
|
||
## Final verification
|
||
|
||
- [ ] **Frontend full suite + typecheck**
|
||
|
||
Run: `cd frontend && npx vitest run && npx tsc -b`
|
||
Expected: all tests PASS, no type errors.
|
||
|
||
- [ ] **Harness gate suite**
|
||
|
||
Run: `cd harness && npm test`
|
||
Expected: all `node --test` gate tests PASS.
|
||
|
||
---
|
||
|
||
## Self-Review
|
||
|
||
**Spec coverage:**
|
||
- P1 (phase lifecycle) → Task 5 (optimistic F1 + finalized all-green; error red retained via existing `phaseError`).
|
||
- P2 (timer) → Task 6.
|
||
- P3 (empty artifact gate) → Task 3 (`ArtifactView` reads `data`) + Task 4 (rendered in the gate).
|
||
- P4 (90% modal + erDiagram) → Task 4 (modal) + Task 2 (erDiagram) + Task 3 (routing).
|
||
- P5 (duplicate Altro/other) → Task 1.
|
||
All five covered.
|
||
|
||
**Placeholder scan:** No TBD/TODO; every code step shows full code and exact commands.
|
||
|
||
**Type consistency:** `buildErDiagram(promoted, joins)` name matches its call site (Task 2). `ArtifactView({ artifact })` prop shape matches `descriptor.artifact` (Task 4). `WorkflowBar` prop set grows monotonically: `{ finalized }` (Task 5) → `{ finalized, createdAt, updatedAt }` (Task 6). `ElapsedTimer` prop names (`startedAt`/`stoppedAt`) match both its test and the `WorkflowBar` call site. `setPhase` exists on the store (used by SteerInput). `isReserved`/`stripReserved` signatures unchanged.
|
||
|
||
**Note (out of scope, per spec):** the erDiagram draws relations only when `artifact.data.joins` is present; no harness/model change forces joins. Reserved control label stays English (`Other — specify`).
|