fix: improve cte review presentation
This commit is contained in:
@@ -169,30 +169,30 @@ Expected: PASS.
|
||||
- `SqlViewer({ blocks })` shows the layout toggle only when `blocks.length > 1`.
|
||||
- CTE plan data shape remains unchanged.
|
||||
|
||||
- [ ] **Step 1: Write failing semantic/layout tests**
|
||||
- [x] **Step 1: Write failing semantic/layout tests**
|
||||
|
||||
Assert long filter data is rendered in distinct elements labeled Column, Operator, and Value;
|
||||
assert card sections expose stable headings. Assert a one-block SQL viewer has no Horizontal or
|
||||
Vertical controls while a two-block viewer retains both.
|
||||
|
||||
- [ ] **Step 2: Run tests and verify RED**
|
||||
- [x] **Step 2: Run tests and verify RED**
|
||||
|
||||
Run: `cd frontend && npx vitest run src/viewers/CtePlanViewer.test.tsx src/viewers/SqlViewer.test.tsx src/viewers/CteResultViewer.test.tsx`
|
||||
|
||||
Expected: structured filter labels are absent and the one-block toggle is present.
|
||||
|
||||
- [ ] **Step 3: Implement responsive CTE cards**
|
||||
- [x] **Step 3: Implement responsive CTE cards**
|
||||
|
||||
Use a bordered header grid, prose blocks with `leading-relaxed`, metadata rows with fixed labels,
|
||||
`break-words`/`font-mono` for physical identifiers, and a responsive filter grid such as
|
||||
`grid-cols-1 sm:grid-cols-[minmax(0,1fr)_auto_minmax(0,1fr)]`. Keep output columns wrapping.
|
||||
|
||||
- [ ] **Step 4: Hide the single-block toggle**
|
||||
- [x] **Step 4: Hide the single-block toggle**
|
||||
|
||||
Wrap the layout control in `{blocks.length > 1 && (...)}` without changing multi-block state or
|
||||
rendering.
|
||||
|
||||
- [ ] **Step 5: Run targeted tests and typecheck**
|
||||
- [x] **Step 5: Run targeted tests and typecheck**
|
||||
|
||||
Run: `cd frontend && npx vitest run src/viewers/CtePlanViewer.test.tsx src/viewers/SqlViewer.test.tsx src/viewers/CteResultViewer.test.tsx && npx tsc -b`
|
||||
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
import { render, screen } from "@testing-library/react";
|
||||
import { render, screen, within } from "@testing-library/react";
|
||||
import { CtePlanViewer } from "./CtePlanViewer";
|
||||
import type { CtePlanV2 } from "./artifactV2";
|
||||
|
||||
@@ -56,6 +56,18 @@ test("renders one card per CTE with index badge, purpose, and filter value", ()
|
||||
expect(screen.getByText(/TRUE/)).toBeInTheDocument();
|
||||
});
|
||||
|
||||
test("renders each filter as aligned column, operator, and value fields", () => {
|
||||
render(<CtePlanViewer plan={plan} />);
|
||||
|
||||
const filter = screen.getByRole("group", { name: "Filter 1" });
|
||||
expect(within(filter).getByText("Column")).toBeInTheDocument();
|
||||
expect(within(filter).getByText("Operator")).toBeInTheDocument();
|
||||
expect(within(filter).getByText("Value")).toBeInTheDocument();
|
||||
expect(within(filter).getByText("pazienti.stato")).toBeInTheDocument();
|
||||
expect(within(filter).getByText("IS")).toBeInTheDocument();
|
||||
expect(within(filter).getByText("TRUE")).toBeInTheDocument();
|
||||
});
|
||||
|
||||
test("shows 'no dependencies' for a CTE with an empty depends_on", () => {
|
||||
render(<CtePlanViewer plan={plan} />);
|
||||
expect(screen.getByText(/no dependencies/i)).toBeInTheDocument();
|
||||
|
||||
@@ -4,19 +4,55 @@ import { Badge } from "../components/ui/badge";
|
||||
import type { CtePlanCte, CtePlanFilter, CtePlanV2 } from "./artifactV2";
|
||||
|
||||
const CHIP = "rounded-md bg-muted px-1.5 py-0.5 font-mono text-xs text-foreground/90";
|
||||
const FIELD_LABEL = "text-[0.65rem] font-semibold uppercase tracking-[0.08em] text-muted-foreground";
|
||||
|
||||
function Chip({ children }: { children: ReactNode }) {
|
||||
return <span className={CHIP}>{children}</span>;
|
||||
}
|
||||
|
||||
function FilterChip({ filter }: { filter: CtePlanFilter }) {
|
||||
function FilterCard({ filter, index }: { filter: CtePlanFilter; index: number }) {
|
||||
return (
|
||||
<div className="flex flex-col gap-0.5">
|
||||
<span className={CHIP}>
|
||||
{filter.column} {filter.op} {filter.value}
|
||||
</span>
|
||||
{filter.description && <p className="text-xs text-muted-foreground">{filter.description}</p>}
|
||||
{filter.rationale && <p className="text-xs text-muted-foreground">{filter.rationale}</p>}
|
||||
<div
|
||||
role="group"
|
||||
aria-label={`Filter ${index}`}
|
||||
className="space-y-3 rounded-lg border border-border/60 bg-muted/20 p-3"
|
||||
>
|
||||
<div className="grid min-w-0 grid-cols-1 gap-3 sm:grid-cols-[minmax(0,1fr)_minmax(5rem,auto)_minmax(0,1fr)]">
|
||||
<div className="min-w-0 space-y-1">
|
||||
<p className={FIELD_LABEL}>Column</p>
|
||||
<code className="block [overflow-wrap:anywhere] font-mono text-xs leading-relaxed text-foreground">
|
||||
{filter.column}
|
||||
</code>
|
||||
</div>
|
||||
<div className="min-w-0 space-y-1 sm:text-center">
|
||||
<p className={FIELD_LABEL}>Operator</p>
|
||||
<code className="block [overflow-wrap:anywhere] font-mono text-xs font-semibold leading-relaxed text-primary">
|
||||
{filter.op}
|
||||
</code>
|
||||
</div>
|
||||
<div className="min-w-0 space-y-1">
|
||||
<p className={FIELD_LABEL}>Value</p>
|
||||
<code className="block whitespace-pre-wrap [overflow-wrap:anywhere] font-mono text-xs leading-relaxed text-foreground">
|
||||
{filter.value}
|
||||
</code>
|
||||
</div>
|
||||
</div>
|
||||
{(filter.description || filter.rationale) && (
|
||||
<div className="grid gap-x-4 gap-y-2 border-t border-border/50 pt-3 sm:grid-cols-[5.5rem_minmax(0,1fr)]">
|
||||
{filter.description && (
|
||||
<>
|
||||
<span className={FIELD_LABEL}>Description</span>
|
||||
<p className="text-xs leading-relaxed text-muted-foreground">{filter.description}</p>
|
||||
</>
|
||||
)}
|
||||
{filter.rationale && (
|
||||
<>
|
||||
<span className={FIELD_LABEL}>Rationale</span>
|
||||
<p className="text-xs leading-relaxed text-muted-foreground">{filter.rationale}</p>
|
||||
</>
|
||||
)}
|
||||
</div>
|
||||
)}
|
||||
</div>
|
||||
);
|
||||
}
|
||||
@@ -25,84 +61,97 @@ function CteCard({ cte, total }: { cte: CtePlanCte; total: number }) {
|
||||
const dependsOn = cte.depends_on ?? [];
|
||||
|
||||
return (
|
||||
<Card>
|
||||
<CardHeader>
|
||||
<div className="flex items-center gap-2">
|
||||
<Card className="overflow-hidden">
|
||||
<CardHeader className="gap-3 border-b border-border/60 bg-muted/15 pb-4">
|
||||
<div className="grid min-w-0 grid-cols-[auto_minmax(0,1fr)] items-center gap-2.5">
|
||||
<Badge variant="outline" className="font-mono">
|
||||
CTE {cte.index}/{total}
|
||||
</Badge>
|
||||
<CardTitle className="font-mono text-sm">{cte.name}</CardTitle>
|
||||
<CardTitle className="min-w-0 [overflow-wrap:anywhere] font-mono text-sm leading-relaxed">
|
||||
{cte.name}
|
||||
</CardTitle>
|
||||
</div>
|
||||
{cte.purpose && <p className="text-sm text-muted-foreground">{cte.purpose}</p>}
|
||||
{cte.purpose && (
|
||||
<p className="text-sm leading-relaxed text-muted-foreground">{cte.purpose}</p>
|
||||
)}
|
||||
</CardHeader>
|
||||
<CardContent className="flex flex-col gap-3">
|
||||
<div>
|
||||
<h4 className="thot-label mb-1">Depends on</h4>
|
||||
{dependsOn.length ? (
|
||||
<div className="flex flex-wrap gap-1.5">
|
||||
{dependsOn.map((d) => (
|
||||
<Chip key={d}>{d}</Chip>
|
||||
))}
|
||||
</div>
|
||||
) : (
|
||||
<p className="text-xs text-muted-foreground">no dependencies</p>
|
||||
<CardContent className="flex flex-col gap-5 pt-1">
|
||||
<div className="grid gap-4 sm:grid-cols-2">
|
||||
<section>
|
||||
<h4 className="thot-label mb-2">Depends on</h4>
|
||||
{dependsOn.length ? (
|
||||
<div className="flex flex-wrap gap-1.5">
|
||||
{dependsOn.map((dependency) => (
|
||||
<Chip key={dependency}>{dependency}</Chip>
|
||||
))}
|
||||
</div>
|
||||
) : (
|
||||
<p className="text-xs text-muted-foreground">no dependencies</p>
|
||||
)}
|
||||
</section>
|
||||
|
||||
{cte.keys && cte.keys.length > 0 && (
|
||||
<section>
|
||||
<h4 className="thot-label mb-2">Keys</h4>
|
||||
<div className="flex flex-wrap gap-1.5">
|
||||
{cte.keys.map((key) => (
|
||||
<Chip key={key}>{key}</Chip>
|
||||
))}
|
||||
</div>
|
||||
</section>
|
||||
)}
|
||||
</div>
|
||||
|
||||
{cte.tables && cte.tables.length > 0 && (
|
||||
<div>
|
||||
<h4 className="thot-label mb-1">Tables</h4>
|
||||
<div className="flex flex-col gap-1">
|
||||
{cte.tables.map((t) => (
|
||||
<div key={t.name}>
|
||||
<span className="font-mono text-sm">{t.name}</span>
|
||||
{t.description && (
|
||||
<p className="text-xs text-muted-foreground">{t.description}</p>
|
||||
<section>
|
||||
<h4 className="thot-label mb-2">Tables</h4>
|
||||
<div className="divide-y divide-border/50 rounded-lg border border-border/60">
|
||||
{cte.tables.map((table) => (
|
||||
<div
|
||||
key={table.name}
|
||||
className="grid min-w-0 gap-1 px-3 py-2.5 sm:grid-cols-[minmax(10rem,0.8fr)_minmax(0,1.2fr)] sm:gap-4"
|
||||
>
|
||||
<code className="[overflow-wrap:anywhere] font-mono text-xs leading-relaxed text-foreground">
|
||||
{table.name}
|
||||
</code>
|
||||
{table.description && (
|
||||
<p className="text-xs leading-relaxed text-muted-foreground">
|
||||
{table.description}
|
||||
</p>
|
||||
)}
|
||||
</div>
|
||||
))}
|
||||
</div>
|
||||
</div>
|
||||
)}
|
||||
|
||||
{cte.keys && cte.keys.length > 0 && (
|
||||
<div>
|
||||
<h4 className="thot-label mb-1">Keys</h4>
|
||||
<div className="flex flex-wrap gap-1.5">
|
||||
{cte.keys.map((k) => (
|
||||
<Chip key={k}>{k}</Chip>
|
||||
))}
|
||||
</div>
|
||||
</div>
|
||||
</section>
|
||||
)}
|
||||
|
||||
{cte.filters && cte.filters.length > 0 && (
|
||||
<div>
|
||||
<h4 className="thot-label mb-1">Filters</h4>
|
||||
<div className="flex flex-col gap-2">
|
||||
<section>
|
||||
<h4 className="thot-label mb-2">Filters</h4>
|
||||
<div className="flex flex-col gap-3">
|
||||
{cte.filters.map((f, i) => (
|
||||
<FilterChip key={i} filter={f} />
|
||||
<FilterCard key={`${f.column}-${i}`} filter={f} index={i + 1} />
|
||||
))}
|
||||
</div>
|
||||
</div>
|
||||
</section>
|
||||
)}
|
||||
|
||||
{cte.output_columns && cte.output_columns.length > 0 && (
|
||||
<div>
|
||||
<h4 className="thot-label mb-1">Output columns</h4>
|
||||
<section>
|
||||
<h4 className="thot-label mb-2">Output columns</h4>
|
||||
<div className="flex flex-wrap gap-1.5">
|
||||
{cte.output_columns.map((c) => (
|
||||
<Chip key={c}>{c}</Chip>
|
||||
{cte.output_columns.map((column) => (
|
||||
<Chip key={column}>{column}</Chip>
|
||||
))}
|
||||
</div>
|
||||
</div>
|
||||
</section>
|
||||
)}
|
||||
|
||||
{cte.rationale && (
|
||||
<div>
|
||||
<h4 className="thot-label mb-1">Rationale</h4>
|
||||
<p className="text-sm text-foreground/90">{cte.rationale}</p>
|
||||
</div>
|
||||
<section className="rounded-lg border-l-2 border-primary/40 bg-primary/5 px-3.5 py-3">
|
||||
<h4 className="thot-label mb-1.5">Rationale</h4>
|
||||
<p className="text-sm leading-relaxed text-foreground/90">{cte.rationale}</p>
|
||||
</section>
|
||||
)}
|
||||
</CardContent>
|
||||
</Card>
|
||||
@@ -112,8 +161,8 @@ function CteCard({ cte, total }: { cte: CtePlanCte; total: number }) {
|
||||
export function CtePlanViewer({ plan }: { plan: CtePlanV2 }) {
|
||||
return (
|
||||
<div className="flex flex-col gap-4">
|
||||
{plan.question && <p className="text-sm text-muted-foreground">{plan.question}</p>}
|
||||
{plan.strategy && <p className="thot-prose">{plan.strategy}</p>}
|
||||
{plan.question && <p className="text-sm leading-relaxed text-muted-foreground">{plan.question}</p>}
|
||||
{plan.strategy && <p className="thot-prose leading-relaxed">{plan.strategy}</p>}
|
||||
|
||||
<div className="flex flex-wrap items-center gap-1.5 font-mono text-xs text-muted-foreground">
|
||||
{plan.ctes.map((c, i) => (
|
||||
|
||||
@@ -45,6 +45,7 @@ test("passes sql to SqlViewer with mapped test status", async () => {
|
||||
render(<CteResultViewer result={okResult} />);
|
||||
await screen.findByTestId("hl");
|
||||
expect(screen.getByText(/passed/i)).toBeInTheDocument();
|
||||
expect(screen.queryByRole("button", { name: /horizontal|vertical/i })).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
test("error status maps to failed test status and destructive badge", async () => {
|
||||
|
||||
@@ -84,3 +84,11 @@ test("(c) layout toggle flips its label between Orizzontale and Verticale", asyn
|
||||
screen.queryByRole("button", { name: /horizontal/i })
|
||||
).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
test("a single SQL block hides the multi-block layout toggle", async () => {
|
||||
render(<SqlViewer blocks={[blocks[0]]} />);
|
||||
await waitFor(() => expect(screen.getByTestId("hl")).toBeInTheDocument());
|
||||
|
||||
expect(screen.queryByRole("button", { name: /horizontal/i })).not.toBeInTheDocument();
|
||||
expect(screen.queryByRole("button", { name: /vertical/i })).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
@@ -89,16 +89,18 @@ export function SqlViewer({ blocks }: { blocks: SqlBlock[] }) {
|
||||
|
||||
return (
|
||||
<div className="sql-viewer">
|
||||
<div className="flex gap-2 mb-2">
|
||||
<button
|
||||
className="text-xs px-2 py-1 rounded-md border border-border/70 shadow-xs transition-colors hover:bg-accent"
|
||||
onClick={() =>
|
||||
setLayout((l) => (l === "vertical" ? "horizontal" : "vertical"))
|
||||
}
|
||||
>
|
||||
{layout === "vertical" ? "Horizontal" : "Vertical"}
|
||||
</button>
|
||||
</div>
|
||||
{blocks.length > 1 && (
|
||||
<div className="flex gap-2 mb-2">
|
||||
<button
|
||||
className="text-xs px-2 py-1 rounded-md border border-border/70 shadow-xs transition-colors hover:bg-accent"
|
||||
onClick={() =>
|
||||
setLayout((l) => (l === "vertical" ? "horizontal" : "vertical"))
|
||||
}
|
||||
>
|
||||
{layout === "vertical" ? "Horizontal" : "Vertical"}
|
||||
</button>
|
||||
</div>
|
||||
)}
|
||||
|
||||
<div
|
||||
className={
|
||||
|
||||
Reference in New Issue
Block a user