diff --git a/.agents/skills/frontend/information-architecture/SKILL.md b/.agents/skills/frontend/information-architecture/SKILL.md index 823f2574..cc14d62b 100644 --- a/.agents/skills/frontend/information-architecture/SKILL.md +++ b/.agents/skills/frontend/information-architecture/SKILL.md @@ -62,7 +62,15 @@ Rules derived from the 2026-08 audit (§6): - **Single route definition site**: `frontend/app/src/routes.tsx`. No routes defined elsewhere. - **`ui-core` stays router-free.** Screens receive navigation as callback props (`routes.tsx:113-121` pattern). Never import a router in `ui-core`. -- **Back-links are declared** in the routes parent map (`PARENT` in `routes.tsx`) and must point to the contextual parent: the gallery's back is the Batches tab. **A tab has no back-link** — its way out is the tab bar, and one inside a panel would be a second, contradictory answer to "where am I". That is why `DatasetScreen` takes `onBack` as optional and the tab mount passes none. **A rail destination has no back-link either**, for the same reason and with the rail in the tab bar's place: `InferenceScreen` takes no `onBack`, and `PARENT.inference` exists as the address other screens send people *to* (#424's D6 panel is the first) rather than as a parent anything returns from. +- **Navigation renders the whole ancestor chain, not one level.** Every destination in it is declared in the routes parent map (`PARENT` in `routes.tsx`); the labels come from the screen, because a project's name is behind a query `ui-core` makes and `routes.tsx` does not fetch. The chains, and the current page is never in its own — it is the `

`: + + | route | chain | + | --- | --- | + | `/projects/:id` | `Projects` | + | `/projects/:id/ingest` | `Projects / ` | + | `/projects/:id/batches/:batchId` | `Projects / / Batches` | + + **A tab in the query string is a level**, which is what the batch route's third crumb is: its ancestor is `PARENT.batches` (`/projects/:id?tab=batches`), not the project's default section — landing on Schema after leaving a batch is landing somewhere you were not. Below `lg` the same chain collapses to `← `; one component, two presentations. **A tab has no chain of its own** — its way out is the tab bar, and one inside a panel would be a second, contradictory answer to "where am I". That is why `DatasetScreen` takes no navigation prop at all: it had a vestigial optional `onBack` that no mount passed after the move to a tab, and it is gone. **A rail destination has none either**, for the same reason with the rail in the tab bar's place: `InferenceScreen` takes no `onBack`, and `PARENT.inference` exists as the address other screens send people *to* (#424's D6 panel is the first) rather than as an ancestor anything returns from. **The annotator is the one sub-view with no chain**: its 44px bar keeps the ghost `ArrowLeft` meaning *up* to the batch, because the bar's left zone already truncates to hold its navigation cluster centred. - Tab state lives in `?tab=` with `replace: true`; unknown values fall back to `overview` silently. ## Process rule diff --git a/DESIGN.md b/DESIGN.md index f440c4f7..1fa46003 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -348,29 +348,55 @@ and without knowing the URL scheme. Before **#199** five of the six sub-views of nothing at all and the sixth offered history, so this section exists to keep the rule from being rediscovered one screen at a time. -- **Every sub-view declares a parent, and the back affordance goes there - structurally.** `navigate(-1)` is not a parent: it means the gallery when you - clicked a tile, nothing at all on a fresh tab, and one asset at a time after - walking forward through a job. The destination has to be the same however the page - was reached — clicked through, pasted, reloaded, or walked forward from a sibling. - The parents live in one `PARENT` table in `app/src/routes.tsx`, because a parent is - a fact about the route table and `ui-core` deliberately has no router. -- **The affordance names its destination.** "Back" alone is a promise about history; +- **Every sub-view declares its ancestor chain, and it is rendered in full.** + `navigate(-1)` is not an ancestor: it means the gallery when you clicked a tile, + nothing at all on a fresh tab, and one asset at a time after walking forward + through a job. Every destination has to be the same however the page was reached — + clicked through, pasted, reloaded, or walked forward from a sibling. The + destinations live in one `PARENT` table in `app/src/routes.tsx`, because a + destination is a fact about the route table and `ui-core` deliberately has no + router. + + A single-level control is not enough, and the gallery is why. Its control read + `← road-signs` — the *project's* name — while landing on the project's **Batches + tab**. Both halves were right: the tab is where somebody leaving a batch belongs, + and naming the project is the most one level can say. Only the chain says both, + and it reads `Projects / road-signs / Batches`. +- **Ancestors only. The current page is the `

`, never a crumb**, which is also + why no crumb carries `aria-current`. A breadcrumb repeating the heading beneath it + spends a line telling somebody what they are already reading. +- **A tab in the query string is a level.** #171 put tabs in the URL because + somebody links to one and returns to it — that makes it somewhere you were, so it + is somewhere you can be sent back to. It is what the gallery's third crumb *is*. +- **The affordance names its destinations.** "Back" alone is a promise about history; "Projects", or a project's own name, is a promise about structure — the one the control can keep. A name that has not loaded yet falls back to the noun (`parentLabel`) rather than to nothing, so the control does not change width under - a cursor that is already aiming at it. -- **Placement follows the pane.** On a padded page it is `patterns/BackLink.tsx` - directly above the page header: meta-size, muted, a 14px `ArrowLeft`, pulled left - by the gutter (`-ml-1`) so its text aligns with the `

` beneath it. On the - full-bleed editor it is the first control in the 44px top bar, as a 36px ghost icon - button — the shape that bar is already built from. -- **A screen takes it as an optional callback, never a route.** The same rule every + a cursor that is already aiming at it. Each crumb truncates with its full label in + `title`, and the row never wraps to a second line at any width. +- **Placement follows the pane.** On a padded page it is `patterns/Breadcrumb.tsx` + directly above the page header: meta-size, muted, `/` separators and no arrow, + pulled left by the gutter (`-ml-1`) so the first crumb aligns with the `

` + beneath it. On the full-bleed editor there is **no chain** — the way out is the + first control in the 44px top bar, a 36px ghost `ArrowLeft` meaning *up*, because + that bar's left zone is a `minmax(0, 1fr)` track already truncating to hold the + navigation cluster on the bar's geometric centre, and crumbs there would be paid + for out of the frame's identity readout. +- **Below `lg` the same chain collapses to `← `**, which is the + shape the single-level control had. One component, one items array, one set of + destinations, two presentations — and the collapse is CSS on one DOM node per + crumb, never a second list, so nothing is read twice by a screen reader and the + two presentations have nowhere to drift apart. +- **A screen takes it as optional callbacks, never a route.** The same rule every forward edge follows: `ui-core` may not import a router, so a host that has nowhere - to send anybody renders no control rather than a dead one. + to send anybody renders no control rather than a dead one. A screen omits a level + it has no callback for rather than rendering dead text, which is what makes an + empty chain mean *nothing to offer*. The host spells every URL; the screen supplies + every label, because a project's name is behind a query `ui-core` makes and + `routes.tsx` does not fetch. - **The rail is for top-level destinations only.** Per-screen return navigation never lives on it — that is what lets it name where it goes, and what keeps the rail the - five things `## Layout` gives it. A rail destination therefore has no back-link of + five things `## Layout` gives it. A rail destination therefore has no breadcrumb of its own, for the reason a tab has none: the rail *is* its way out, and a second answer to "where am I" inside the pane would contradict it. - **The browser's Back button stays correct, and is never the only way out.** Nothing @@ -545,7 +571,7 @@ what #207–#213 build against. Four lines and two buttons, in this order: -1. The back affordance (`← Projects`), per **Navigation rules**. +1. The breadcrumb (`Projects`, this page's whole chain), per **Navigation rules**. 2. The project name, at the page-title role. 3. The description **if there is one**. If there is not, render *nothing* — the string "No description." spends a line telling somebody about a field rather than about their diff --git a/frontend/app/e2e/navigation.spec.ts b/frontend/app/e2e/navigation.spec.ts index 472d29b1..059ec1b3 100644 --- a/frontend/app/e2e/navigation.spec.ts +++ b/frontend/app/e2e/navigation.spec.ts @@ -1,5 +1,5 @@ /** - * The way back out of every sub-view. + * Where you are, and the way back out of every sub-view. * * ## Every scenario navigates by URL, and that is the whole method * @@ -10,12 +10,18 @@ * * So each one does `page.goto` straight to the sub-view, signs in there, and * presses the control. With an empty history there is nowhere for `navigate(-1)` - * to go, so only a **structural** parent can satisfy these. + * to go, so only a **structural** ancestor can satisfy these. * - * The parents themselves are `routes.tsx`'s `PARENT` table, and `DESIGN.md`'s + * The destinations themselves are `routes.tsx`'s `PARENT` table, and `DESIGN.md`'s * **Navigation rules** is the prose. `ui-core`'s `navigation.test.tsx` holds the - * other half — that each screen draws the control and calls back — which a - * component test can see and a URL cannot. + * half a component test can see — that each screen draws its chain and calls back. + * + * ## Two claims live here and nowhere else + * + * That a crumb reaches the right **URL**, which `ui-core` cannot know because it + * imports no router; and **which crumbs are visible below `lg`**, because the + * collapse is a media query and both presentations are in the DOM either way. A + * jsdom assertion about the second would pass whatever the CSS said. */ import { expect, test, type Page } from "@playwright/test"; @@ -174,7 +180,11 @@ async function openCold(page: Page, url: string): Promise { await page.getByTestId("token-submit").click(); } -/** Every sub-view and its parent, as data. One testid on every padded screen. */ +/** + * Every sub-view and its **immediate** parent, as data — the crumb the collapsed + * presentation keeps, and the one every scenario below presses. One testid on + * every padded screen. + */ const SUBVIEWS = [ { name: "the project", @@ -197,8 +207,8 @@ const SUBVIEWS = [ parent: new RegExp(`/projects/${PROJECT}\\?tab=batches$`), }, // **The dataset is not here any more, and its absence is the change.** It was a - // route with a back-link; it is a project *tab* now, so its way out is the tab - // bar and the back-link on that page belongs to the project. Its old URL still + // route with its own way out; it is a project *tab* now, so its way out is the + // tab bar and the crumbs above it belong to the project. Its old URL still // works — see the redirect scenario below. ] as const; @@ -207,7 +217,7 @@ for (const view of SUBVIEWS) { await openCold(page, view.url); await expect(page.getByTestId(view.ready)).toBeVisible(); - await page.getByTestId("back-link").click(); + await page.getByTestId("breadcrumb-parent").click(); await expect(page).toHaveURL(view.parent); }); } @@ -269,14 +279,82 @@ test("the way out names the project it goes to", async ({ page }) => { // "Back" alone is a promise about history. Naming the destination is a promise // about structure, which is the one the control can keep. await openCold(page, `/projects/${PROJECT}/ingest`); - await expect(page.getByTestId("back-link")).toContainText("road-signs"); + await expect(page.getByTestId("breadcrumb-parent")).toContainText("road-signs"); }); test("the project's own way out names the list, not a project", async ({ page }) => { // One level up from a project is `Projects`, and it is the one sub-view whose // parent has a fixed name rather than one that has to load. await openCold(page, `/projects/${PROJECT}`); - await expect(page.getByTestId("back-link")).toContainText("Projects"); + await expect(page.getByTestId("breadcrumb-parent")).toContainText("Projects"); +}); + +/** + * The gallery's whole chain, and the disagreement it settles. + * + * The single-level control here read `← road-signs` — the *project's* name — while + * landing on the project's **Batches tab**. Both halves were right on their own; + * only the chain says both, and this is the scenario that holds it to that. + */ +test("the batch gallery names its whole chain, and every crumb goes where it says", async ({ + page, +}) => { + await openCold(page, `/projects/${PROJECT}/batches/${BATCH}`); + await expect(page.getByTestId("gallery")).toBeVisible(); + + const crumbs = page.getByTestId("breadcrumb").getByRole("button"); + await expect(crumbs).toHaveText(["Projects", "road-signs", "Batches"]); + + // The middle crumb: the project itself, which is not where the immediate parent + // goes and never had a control of its own. + await crumbs.nth(1).click(); + await expect(page).toHaveURL(new RegExp(`/projects/${PROJECT}$`)); + + await page.goBack(); + await expect(page.getByTestId("gallery")).toBeVisible(); + + // The root, two levels up, which the single-level control could not reach at all. + await page.getByTestId("breadcrumb").getByRole("button").first().click(); + await expect(page).toHaveURL(/\/projects$/); +}); + +test("the Batches crumb lands with the Batches tab actually selected", async ({ page }) => { + // A `?tab=` in the URL and a tab bar showing something else is the failure this + // asserts against — the redirect-that-moved-only-the-URL shape. A tab in the + // query string is a place, which is what makes it a legitimate crumb level. + await openCold(page, `/projects/${PROJECT}/batches/${BATCH}`); + await page.getByTestId("breadcrumb-parent").click(); + + await expect(page).toHaveURL(new RegExp(`/projects/${PROJECT}\\?tab=batches$`)); + await expect(page.getByTestId("tab-batches")).toHaveAttribute("aria-selected", "true"); +}); + +test("below lg the chain collapses to the immediate parent, on one line", async ({ page }) => { + // The claim jsdom structurally cannot make: every crumb is in the DOM at every + // width, and which ones are *shown* is a media query. A component test asserting + // this would pass whatever the CSS said. + await openCold(page, `/projects/${PROJECT}/batches/${BATCH}`); + await expect(page.getByTestId("gallery")).toBeVisible(); + + const row = page.getByTestId("breadcrumb"); + const tall = await row.boundingBox(); + + await page.setViewportSize({ width: 900, height: 800 }); + + // `getByRole` reads the **accessibility tree**, which a `display: none` crumb is + // not in — so this is the whole claim in one assertion: below `lg` a screen + // reader is offered the immediate parent and nothing above it. + await expect(row.getByRole("button")).toHaveText(["Batches"]); + + // And `locator` reads the **DOM**, which every crumb is still in. The two + // together are what says the collapse is one list presented twice rather than + // two lists — a duplicated chain would count six here and read twice aloud. + await expect(row.locator("button")).toHaveCount(3); + + // Still one line. The row never wraps at any width, which is the other half of + // "collapses" — a chain that merely reflowed would be taller here. + const short = await row.boundingBox(); + expect(short?.height).toBe(tall?.height); }); diff --git a/frontend/app/e2e/shell.spec.ts b/frontend/app/e2e/shell.spec.ts index b03627ad..d8a119cb 100644 --- a/frontend/app/e2e/shell.spec.ts +++ b/frontend/app/e2e/shell.spec.ts @@ -198,9 +198,9 @@ test("the Inference entry goes to the section, and is current once you are on it await expect(page).toHaveURL(/\/inference$/); await expect(page.getByTestId("rail-inference")).toHaveAttribute("aria-current", "page"); await expect(page.getByTestId("rail-projects")).not.toHaveAttribute("aria-current", "page"); - // A rail destination has no back-link: the rail is its way out, and a second + // A rail destination has no breadcrumb: the rail is its way out, and a second // answer to "where am I" inside the pane would contradict it. - await expect(page.getByTestId("back-link")).toHaveCount(0); + await expect(page.getByTestId("breadcrumb")).toHaveCount(0); }); test("navigation is real links, and the active one is the one you are on", async ({ page }) => { diff --git a/frontend/app/src/routes.tsx b/frontend/app/src/routes.tsx index 25881b73..f2589ebb 100644 --- a/frontend/app/src/routes.tsx +++ b/frontend/app/src/routes.tsx @@ -173,21 +173,29 @@ function Home(): JSX.Element { } /** - * Every sub-view's parent, in one place. + * Every ancestor of every sub-view, in one place. * - * A back affordance navigates to its **declared parent**, never `navigate(-1)`: + * A breadcrumb crumb navigates to its **declared ancestor**, never `navigate(-1)`: * the destination has to be the same whether the page was reached by clicking * through, by pasting a URL, by reloading, or by walking forward from a sibling. * History cannot promise that, and on a fresh tab it leaves the application * entirely. * - * The parents live here rather than in the screens because a parent is a fact - * about the *route table*, and `ui-core` deliberately does not have one — the note - * on `Projects` above is the same rule from the other side. `DESIGN.md`'s + * The ancestors live here rather than in the screens because a destination is a + * fact about the *route table*, and `ui-core` deliberately does not have one — the + * note on `Projects` above is the same rule from the other side. `DESIGN.md`'s * **Navigation rules** is the prose half of this table. * - * The gallery's parent carries `?tab=batches`, because landing on the project's - * default Schema tab after leaving a batch is landing somewhere you were not. + * **This table is walked by hand rather than transitively, and the reason is + * labels.** A crumb needs a name as well as a URL, and a project's name is behind + * a query `ui-core` makes — this file holds ids and does not fetch, by the rule at + * the top of it. So the split is: the host spells every URL, the screen supplies + * every label and composes the chain from the callbacks it was handed. What could + * silently drift is a URL, and a URL still has exactly one spelling. + * + * The gallery's chain ends at `?tab=batches`, because landing on the project's + * default Schema tab after leaving a batch is landing somewhere you were not — a + * tab in the query string (#171) is a place, so it is a level. */ const PARENT = { //: A rail destination, like `inference` below, so nothing declares it as a @@ -276,6 +284,10 @@ function Gallery(): JSX.Element { projectId={projectId} batchId={batchId} onBack={() => void navigate(PARENT.batches(projectId))} + // The two levels above the Batches tab. The gallery is the product's + // deepest padded page, so it is the one whose chain is three long. + onOpenProject={() => void navigate(PARENT.project(projectId))} + onOpenProjects={() => void navigate(PARENT.projects)} onOpenAsset={(asset) => { if (asset.job_id === null || asset.job_id === undefined) return; void navigate(`/jobs/${asset.job_id}?asset=${asset.id}`); @@ -384,6 +396,7 @@ function Ingest(): JSX.Element { void navigate(PARENT.project(projectId))} + onOpenProjects={() => void navigate(PARENT.projects)} onOpenBatch={(batchId) => void navigate(`/projects/${projectId}/batches/${batchId}`)} // The foreshadowing banner's link: the schema section is a `?tab=` // on the project page, and spelling that URL is this file's job. diff --git a/frontend/ui-core/src/index.ts b/frontend/ui-core/src/index.ts index 66bfedb7..20a6eba8 100644 --- a/frontend/ui-core/src/index.ts +++ b/frontend/ui-core/src/index.ts @@ -286,8 +286,12 @@ export { writeRailCollapsed, } from "./data/railState.js"; -// The way out of a sub-view. Structural, never `navigate(-1)`. -export { BackLink, type BackLinkProps } from "./patterns/BackLink.js"; +// Where you are, as the whole ancestor chain. Structural, never `navigate(-1)`. +export { + Breadcrumb, + type BreadcrumbItem, + type BreadcrumbProps, +} from "./patterns/Breadcrumb.js"; export { parentLabel } from "./patterns/parentLabel.js"; // The floating tool palette. Reports the derived tool; never stores one. diff --git a/frontend/ui-core/src/patterns/BackLink.tsx b/frontend/ui-core/src/patterns/BackLink.tsx deleted file mode 100644 index 762639c8..00000000 --- a/frontend/ui-core/src/patterns/BackLink.tsx +++ /dev/null @@ -1,55 +0,0 @@ -/** - * The way out of a sub-view — one control, one rule, every screen. - * - * ## Why it is structural and never `navigate(-1)` - * - * VisionSet is an application, not a website. A person who walks into a sub-view - * has to be able to walk back out of it *from the screen*, and the place they land - * has to be the same place whether they clicked through, pasted a URL, reloaded, or - * walked forward from a sibling. History cannot promise that: on a fresh tab it - * leaves the app, and after walking forward through several assets it walks back - * through them one at a time. - * - * `routes.tsx` already made this argument once, about the annotation page's *grid* - * button — "it has to mean that whether the annotator was reached by clicking a - * tile, by pasting a URL, or by walking forward from another asset". This is the - * same argument applied to the direction nobody had claimed. - * - * ## The label names the parent, and that is the point - * - * "Back" alone is a promise about history. Naming the destination — the project, - * the batch, Projects — is a promise about *structure*, and it is the one the - * component can keep. A screen that knows its parent's name passes it; one whose - * name has not loaded yet passes the noun, so the control never blinks between two - * widths while a query settles. - * - * ## It is not on the app rail - * - * The rail is top-level destinations (`DESIGN.md`, and the thin-app rule). A back - * affordance is per screen, so it lives with the screen — which is also what lets - * it name where it goes. - */ - -import { ArrowLeft } from "lucide-react"; -import type { JSX } from "react"; - -export interface BackLinkProps { - /** Where it goes. The host turns this into a route change. */ - readonly onClick: () => void; - /** The parent, named. "Projects", a project's name, a batch's name. */ - readonly label: string; -} - -export function BackLink({ onClick, label }: BackLinkProps): JSX.Element { - return ( - - ); -} diff --git a/frontend/ui-core/src/patterns/Breadcrumb.tsx b/frontend/ui-core/src/patterns/Breadcrumb.tsx new file mode 100644 index 00000000..9d02e847 --- /dev/null +++ b/frontend/ui-core/src/patterns/Breadcrumb.tsx @@ -0,0 +1,131 @@ +/** + * Where you are, as the whole chain — one control, one rule, every screen. + * + * ## Why it is structural and never `navigate(-1)` + * + * VisionSet is an application, not a website. A person who walks into a sub-view + * has to be able to walk back out of it *from the screen*, and the place they land + * has to be the same place whether they clicked through, pasted a URL, reloaded, or + * walked forward from a sibling. History cannot promise that: on a fresh tab it + * leaves the app, and after walking forward through several assets it walks back + * through them one at a time. + * + * ## Why the whole chain, and not the step above + * + * This replaces a single-level back link, and the defect that retired it is worth + * keeping written down: the batch gallery's control read `← road-signs` — the + * *project's* name — while landing on the project's **Batches tab**. Both halves + * were right on their own. Naming the project is the most a one-level control can + * say, and the tab is where somebody leaving a batch belongs. Only the chain says + * both, and `Projects / road-signs / Batches` is the sentence that does it. + * + * A tab that lives in the query string (#171) is therefore a legitimate level: it + * is somewhere you were, so it is somewhere you can be sent back to. + * + * ## Ancestors only + * + * The page you are on is the `

` beneath this row, so it is never a crumb — + * which is also why no crumb carries `aria-current`. A breadcrumb that repeated + * the heading would spend a line telling somebody what they are already reading. + * + * ## One items array, two presentations + * + * Below `lg` the same list collapses to `← `, which is the shape + * the single-level control had. It is **CSS on one DOM node per crumb**, never a + * second list: a duplicated chain would be read twice by a screen reader and would + * give the two presentations two places to drift apart. + * + * ## The host owns the URLs; the screen owns the labels + * + * `ui-core` imports no router, so every destination arrives as a callback and + * `routes.tsx`'s `PARENT` table stays the one place a URL is spelled. The labels + * cannot live there — a project's name is behind a query in this package and + * `routes.tsx` does not fetch — so each screen composes its own items from the + * callbacks it was handed. A screen omits a level it has no callback for rather + * than rendering dead text, which is what keeps the empty list meaningful: no + * destinations, no control. + * + * ## It is not on the app rail + * + * The rail is top-level destinations (`DESIGN.md`, and the thin-app rule). Return + * navigation is per screen, so it lives with the screen — which is also what lets + * it name where it goes. + */ + +import { ArrowLeft } from "lucide-react"; +import { Fragment, type JSX } from "react"; + +export interface BreadcrumbItem { + /** The ancestor, named. "Projects", a project's name, "Batches". */ + readonly label: string; + /** Where it goes. The host turns this into a route change. */ + readonly onNavigate: () => void; +} + +export interface BreadcrumbProps { + /** + * The ancestor chain, root first, **excluding the current page**. Empty renders + * nothing at all rather than a dead control. + */ + readonly items: readonly BreadcrumbItem[]; +} + +export function Breadcrumb({ items }: BreadcrumbProps): JSX.Element | null { + if (items.length === 0) return null; + const last = items.length - 1; + + return ( + + ); +} diff --git a/frontend/ui-core/src/screens/DatasetScreen.tsx b/frontend/ui-core/src/screens/DatasetScreen.tsx index 705840b5..8ccba6f8 100644 --- a/frontend/ui-core/src/screens/DatasetScreen.tsx +++ b/frontend/ui-core/src/screens/DatasetScreen.tsx @@ -68,8 +68,6 @@ import { } from "../primitives/Select"; import { Table, TableBody, TableCell, TableHead, TableHeader, TableRow } from "../primitives/Table"; import { EmptyState, ErrorState } from "../patterns/AsyncStates"; -import { BackLink } from "../patterns/BackLink"; -import { parentLabel } from "../patterns/parentLabel"; import { formatWhen } from "../lib/format"; import { AssetThumbnail } from "./AssetThumbnail"; import { saveBlob } from "./download"; @@ -82,7 +80,6 @@ import { useExportRelease, useFormats, useJobArtifact, - useProject, useProjectDataset, usePublishRelease, useReleases, @@ -96,14 +93,21 @@ import { /** The 409 that means "say you meant it". Not `confirm`, and not destructive. */ const LOSSY = "LOSSY_EXPORT_NOT_CONSENTED"; +/* + * There is no `onBack` any more, and no breadcrumb. + * + * The trunk is a project **tab**, so its way out is the tab bar and the crumbs + * above it belong to the project page this renders inside — a second answer to + * "where am I", one panel further in, would contradict the first. The prop + * survived the move to a tab with no caller passing it, which is the dead + * flexibility the `information-architecture` rule exists to prevent. Its old + * route is a redirect, so nothing can reach this screen standalone. + */ export interface DatasetScreenProps { readonly projectId: string; - /** Up to the project this trunk belongs to. */ - readonly onBack?: () => void; } -export function DatasetScreen({ projectId, onBack }: DatasetScreenProps): JSX.Element { - const project = useProject(projectId); +export function DatasetScreen({ projectId }: DatasetScreenProps): JSX.Element { const dataset = useProjectDataset(projectId); const stats = useDatasetStats(dataset.data?.id); const releases = useReleases(dataset.data?.id); @@ -111,8 +115,6 @@ export function DatasetScreen({ projectId, onBack }: DatasetScreenProps): JSX.El return (
- {onBack !== undefined && } -

Dataset

diff --git a/frontend/ui-core/src/screens/GalleryScreen.tsx b/frontend/ui-core/src/screens/GalleryScreen.tsx index 8a674006..86d644bb 100644 --- a/frontend/ui-core/src/screens/GalleryScreen.tsx +++ b/frontend/ui-core/src/screens/GalleryScreen.tsx @@ -56,7 +56,7 @@ import { DialogTitle, } from "../primitives/Dialog"; import { AssetThumbnail } from "./AssetThumbnail"; -import { BackLink } from "../patterns/BackLink"; +import { Breadcrumb } from "../patterns/Breadcrumb"; import { parentLabel } from "../patterns/parentLabel"; import { ApproveDialog, BatchProgressBar, CompleteBatchButton } from "./BatchLifecycle"; import { CorrectionButton, CorrectionOf } from "./CorrectionBatch"; @@ -153,8 +153,25 @@ export interface GalleryScreenProps { * this prop is passed — see `Tile`. */ readonly onOpenAsset?: (asset: BatchAsset) => void; - /** Up to the project this batch belongs to. */ + /** + * Up to the **Batches tab** of the project this batch belongs to, which is this + * page's immediate parent. + * + * The label used to say `road-signs` and the destination used to be the tab, so + * the control named one place and went to another. Both halves were right — the + * tab is where somebody leaving a batch belongs, and the project is the most a + * one-level control could name — and only the chain says both. + */ readonly onBack?: () => void; + /** + * The project's own page, this batch's grandparent, and the project list above + * it. Two more navigation callbacks rather than a walk of the route table: the + * breadcrumb's *destinations* belong to the host — `ui-core` imports no router — + * while its *labels* belong here, because a project's name is behind a query + * this package makes and the host does not fetch. + */ + readonly onOpenProject?: () => void; + readonly onOpenProjects?: () => void; /** The project's schema tab, for the approve dialog's `SCHEMA_NOT_FOUND` remedy. */ readonly onOpenSchema?: () => void; /** @@ -187,6 +204,8 @@ export function GalleryScreen({ batchId, onOpenAsset, onBack, + onOpenProject, + onOpenProjects, onOpenSchema, onOpenDataset, onOpenBatch, @@ -365,7 +384,21 @@ export function GalleryScreen({ return (
- {onBack !== undefined && } + {/* The product's deepest padded page, and the whole reason the chain exists: + `Projects / road-signs / Batches`. A level is included only when the host + gave it somewhere to go, so a host that wired nothing renders no control + at all rather than dead text. */} + void; - /** Up to the project this is ingesting into. */ + /** Up to the project this is ingesting into — the immediate parent. */ readonly onBack?: () => void; + /** + * The project list, which is this page's grandparent. + * + * A second navigation callback rather than a walk of the route table: the + * breadcrumb's *destinations* belong to the host — `ui-core` imports no router + * — while its *labels* belong here, because a project's name is behind a query + * this package makes and the host does not fetch. + */ + readonly onOpenProjects?: () => void; /** The schema tab, for the labels foreshadowing banner. */ readonly onOpenSchema?: () => void; } @@ -264,6 +273,7 @@ export function IngestScreen({ projectId, onOpenBatch, onBack, + onOpenProjects, onOpenSchema, }: IngestScreenProps): JSX.Element { const project = useProject(projectId); @@ -443,7 +453,18 @@ export function IngestScreen({ return (
- {onBack !== undefined && } + {/* A level is included only when the host gave it somewhere to go, so a host + that wired nothing renders no control at all rather than dead text. */} +

Ingest

diff --git a/frontend/ui-core/src/screens/ProjectScreen.tsx b/frontend/ui-core/src/screens/ProjectScreen.tsx index d858d55d..088f92f3 100644 --- a/frontend/ui-core/src/screens/ProjectScreen.tsx +++ b/frontend/ui-core/src/screens/ProjectScreen.tsx @@ -88,7 +88,7 @@ import { useState, type ComponentType, type FormEvent, type JSX } from "react"; import { Async } from "../data/Async"; import { asApiError } from "../data/errors"; import { refusalProse } from "../data/refusals"; -import { BackLink } from "../patterns/BackLink"; +import { Breadcrumb } from "../patterns/Breadcrumb"; import { Badge } from "../primitives/Badge"; import { Button } from "../primitives/Button"; import { @@ -302,7 +302,11 @@ export function ProjectScreen({ return (
- {onBack !== undefined && } + {/* One ancestor, and it is the shortest chain in the product: a project's + parent is the list and nothing sits above it. */} + {(loaded) => ( diff --git a/frontend/ui-core/src/screens/navigation.test.tsx b/frontend/ui-core/src/screens/navigation.test.tsx index 87a71a62..40436766 100644 --- a/frontend/ui-core/src/screens/navigation.test.tsx +++ b/frontend/ui-core/src/screens/navigation.test.tsx @@ -1,29 +1,30 @@ /** - * The way out of every sub-view. + * Where you are, and the way out, on every sub-view. * * A screen with no return edge is complete against its own contract — a prop that * does not exist cannot be missing, which is why no other test notices. So the - * claim here is deliberately uniform and asserted - * once per screen: **passed a parent, the screen renders one control that names it - * and calls back; passed none, it renders nothing rather than a dead one.** + * claim here is deliberately uniform and asserted once per screen: **passed its + * ancestors, the screen renders the chain and each crumb calls back; passed none, + * it renders nothing rather than a row of dead text.** * - * That the control reaches the *right* URL is not knowable here — a parent is a + * That a crumb reaches the *right* URL is not knowable here — a destination is a * fact about the route table, which lives in `@visionset/app`. `e2e/navigation.spec.ts` * asserts it, and it navigates **by URL** so history is empty, because history is - * exactly what a back affordance must not rely on. + * exactly what a breadcrumb must not rely on. The same file owns the other claim + * jsdom cannot make: which crumbs are *visible* below `lg`, since a media query is + * a real-browser fact and both presentations are in the DOM here. */ import { QueryClient } from "@tanstack/react-query"; -import { render, screen, waitFor } from "@testing-library/react"; +import { render, screen, waitFor, within } from "@testing-library/react"; import { userEvent } from "@testing-library/user-event"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import type { JSX, ReactNode } from "react"; import { ApiProvider } from "../data/ApiProvider"; import { writeToken } from "../data/session"; -import { BackLink } from "../patterns/BackLink"; +import { Breadcrumb } from "../patterns/Breadcrumb"; import { parentLabel } from "../patterns/parentLabel"; -import { DatasetScreen } from "./DatasetScreen"; import { GalleryScreen } from "./GalleryScreen"; import { IngestScreen } from "./IngestScreen"; import { ProjectScreen } from "./ProjectScreen"; @@ -44,11 +45,11 @@ const NO_PROGRESS = { }; /** - * One stub for four screens. + * One stub for three screens. * * Every path any of them reads, answered with the smallest honest shape. The * screens are only ever asked one question here — "did you draw the way out?" — so - * a per-screen fixture would be four copies of the same setup for one assertion. + * a per-screen fixture would be three copies of the same setup for one assertion. */ function answer(path: string): unknown { if (path === `/projects/${PROJECT}`) { @@ -125,19 +126,71 @@ function mount(node: ReactNode): JSX.Element { } describe("the control itself", () => { - it("names where it goes, and calls back when pressed", async () => { - const onClick = vi.fn(); - render(); + it("renders every ancestor in order, and each one calls its own destination", async () => { + const projects = vi.fn(); + const project = vi.fn(); + const batches = vi.fn(); + render( + , + ); - const link = screen.getByTestId("back-link"); - expect(link.textContent).toContain("road-signs"); + const crumbs = within(screen.getByTestId("breadcrumb")).getAllByRole("button"); + expect(crumbs.map((crumb) => crumb.textContent)).toEqual([ + "Projects", + "road-signs", + "Batches", + ]); + + // Each one separately, because a chain wired to a single handler would render + // identically and pass any assertion made about the row as a whole. + await userEvent.click(crumbs[0]!); + await userEvent.click(crumbs[1]!); + expect(projects).toHaveBeenCalledTimes(1); + expect(project).toHaveBeenCalledTimes(1); + expect(batches).not.toHaveBeenCalled(); + }); + + it("collapses to the IMMEDIATE parent, not to the root", () => { + // The narrow presentation keeps exactly one crumb, and which one it keeps is + // the whole claim: a chain that collapsed to its root would send somebody to + // the project list from a batch, silently and structurally. + // + // *Visibility* is a media-query fact and belongs to the browser suite; what is + // knowable here is which crumb wears the collapsed slot. + render( + , + ); - await userEvent.click(link); - expect(onClick).toHaveBeenCalledTimes(1); + expect(screen.getByTestId("breadcrumb-parent").textContent).toBe("Batches"); + }); + + it("renders nothing at all rather than a row of dead text", () => { + // The rule the single-level control already kept, carried over intact: a host + // with nowhere to send anybody draws no affordance. A screen omits an ancestor + // it has no callback for, so an empty list is how "nowhere at all" arrives. + render(); + expect(screen.queryByTestId("breadcrumb")).toBeNull(); + }); + + it("carries the full label for a crumb the width cut short", () => { + render(); + expect(screen.getByTestId("breadcrumb-parent").title).toBe("a-very-long-project-name"); }); it("falls back to the noun rather than to nothing while a name is in flight", () => { - // A control that appeared as a bare arrow and then grew a name would move the + // A crumb that appeared as a bare arrow and then grew a name would move the // page under a cursor already aiming at it. expect(parentLabel("road-signs")).toBe("road-signs"); expect(parentLabel(undefined)).toBe("Project"); @@ -145,59 +198,74 @@ describe("the control itself", () => { }); }); -/** Every sub-view, and the parent each names. */ +/** + * Every sub-view and the chain it declares. + * + * The dataset is deliberately absent: it is a project **tab** now, so its way out + * is the tab bar and the crumbs above it belong to the project page it renders + * inside. Its `onBack` outlived that move with nobody passing it and is gone. + */ const SUBVIEWS = [ { name: "the project", - parent: "Projects", + chain: ["Projects"], sentinel: "project-screen", - render: (onBack?: () => void) => - , + render: (nav?: () => void) => + , }, { name: "ingest", - parent: "road-signs", + chain: ["Projects", "road-signs"], sentinel: "ingest-screen", - render: (onBack?: () => void) => - , + render: (nav?: () => void) => + , }, { name: "the gallery", - parent: "road-signs", + // The chain this whole change exists for. The old control read `road-signs` + // and landed on the Batches tab; the third crumb is what settles that. + chain: ["Projects", "road-signs", "Batches"], sentinel: "gallery", - render: (onBack?: () => void) => + render: (nav?: () => void) => , }, - { - name: "the dataset", - parent: "road-signs", - sentinel: "dataset-screen", - render: (onBack?: () => void) => - , - }, ] as const; -describe.each(SUBVIEWS)("$name", ({ parent, sentinel, render: renderScreen }) => { - it("draws one way out, naming its parent", async () => { - const onBack = vi.fn(); - render(mount(renderScreen(onBack))); +describe.each(SUBVIEWS)("$name", ({ chain, sentinel, render: renderScreen }) => { + it("draws its whole ancestor chain, in order", async () => { + render(mount(renderScreen(vi.fn()))); - // Waited for rather than read once: three of the four name a project whose + // Waited for rather than read once: two of the three name a project whose // query is still in flight on the first paint, and `parentLabel` deliberately // renders the noun until it lands. + await waitFor(() => { + const crumbs = within(screen.getByTestId("breadcrumb")).getAllByRole("button"); + expect(crumbs.map((crumb) => crumb.textContent)).toEqual([...chain]); + }); + }); + + it("puts its immediate parent in the collapsed slot, and calls back from it", async () => { + const onBack = vi.fn(); + render(mount(renderScreen(onBack))); + await waitFor(() => - expect(screen.getByTestId("back-link").textContent).toContain(parent), + expect(screen.getByTestId("breadcrumb-parent").textContent).toBe(chain[chain.length - 1]), ); // Re-queried rather than held: a screen whose queries settle after the first // paint re-renders around this control, and clicking the node captured before // that does nothing at all — silently, which is the worst way for a test to // pass. - await userEvent.click(screen.getByTestId("back-link")); + await userEvent.click(screen.getByTestId("breadcrumb-parent")); expect(onBack).toHaveBeenCalledTimes(1); }); @@ -208,7 +276,7 @@ describe.each(SUBVIEWS)("$name", ({ parent, sentinel, render: renderScreen }) => // loading state first, and asserting absence against a skeleton would pass // however the screen behaved afterwards. await screen.findByTestId(sentinel); - expect(screen.queryByTestId("back-link")).toBeNull(); + expect(screen.queryByTestId("breadcrumb")).toBeNull(); }); });