diff --git a/DESIGN.md b/DESIGN.md index 2539a02..d19a581 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -195,19 +195,33 @@ Coral appears in the product in exactly two places, and adding a third is a desi decision rather than a styling one — raise it in review, do not put it in a diff: 1. **The wordmark** in the rail (`frontend/app/src/shell/AppShell.tsx`). -2. **The ingest progress bar's fill** (`Progress` in `frontend/ui-core/src/primitives/Feedback.tsx`). +2. **The `Progress` primitive's fill** (`frontend/ui-core/src/primitives/Feedback.tsx`) — the + ingest bar, the weight download's, and any bar that comes after them. A progress bar is the one piece of chrome a person watches rather than reads, which is where the coral buys attention instead of spending it. The styleguide shows a `brand` swatch as well — a styleguide is where a value is inspected, not where it is used. +The second entry names the **primitive** rather than one of its callers, and the recorded +reason is why: it argues about what a progress bar *is*, not about which screen has one. A +rule phrased as *the ingest bar* would make the weight download's bar look like the third +site this list forbids, and the honest reading — one component, used twice — would have had +to be argued from scratch every time a bar appeared. + To check the invariant: ``` git grep -nE '\b(bg|text|border|ring|fill|stroke)-brand\b' -- frontend ``` -Three hits is the expected state: the two above, plus the styleguide swatch. +Four lines match and **three of them are usages**: the two above, plus the styleguide +swatch. The fourth is the `--color-brand` comment in `frontend/ui-core/src/styles.css`, +which states this rule rather than applying it, and which says the same two sites this list +does. + +Three stays three however many bars there are, because the fill lives inside the primitive +and a second caller adds no occurrence of its own — which is exactly what makes the +invariant checkable by this grep after the change above. ### One filled button per view diff --git a/frontend/app/e2e/inference.spec.ts b/frontend/app/e2e/inference.spec.ts new file mode 100644 index 0000000..78578ac --- /dev/null +++ b/frontend/app/e2e/inference.spec.ts @@ -0,0 +1,201 @@ +/** + * The Inference section in a real browser: a weight download somebody watches. + * + * ## Why this is not `inference.test.tsx` + * + * The claim is **recovery**, and recovery is a claim about a page that did not + * start the thing it is showing. jsdom can assert that a component renders a prop; + * only a browser can throw the whole application away — the React tree, the query + * cache, every closure that might have been holding a job id — and prove that what + * comes back still knows a transfer is running. `page.reload()` is the assertion, + * and there is no jsdom equivalent of it. + * + * It is also where the *poll* is real: the bar below moves because a timer fired + * and an answer changed, with nothing clicked after the screen opened. + * + * What is **not** here is the counting half — *and it stops asking once nothing is + * moving*. That is a claim that nothing happens over an interval, and the only way + * to make it in a browser is to wait on a clock, which `tests/scripts/e2e_discipline` + * forbids for the reason it gives. It is asserted in `inference.test.tsx`, which + * can count requests without a browser scheduler deciding the outcome. + * + * ## The wire is stubbed and the screen is not + * + * `page.route` answers `/api/inference/connections` from a mutable fixture, so the + * bodies are what the server would send — `_wire.ts`'s rule, and the generated + * runtime checks enforce it: a row missing a required field is rejected before the + * screen renders, which is exactly what that gate is for. + */ + +import { expect, test, type Page } from "@playwright/test"; + +const CONNECTION = "22222222-2222-4222-8222-222222222222"; +const JOB = "44444444-4444-4444-8444-444444444444"; + +const GIGABYTE = 1_000_000_000; + +interface Download { + readonly state: string; + readonly bytes_done: number; + readonly bytes_total: number | null; + readonly error?: string | null; +} + +/** What the server answers for one local connection, in the state a test wants. */ +function connection(setup: "not_set_up" | "ready", download: Download | null): unknown { + return { + id: CONNECTION, + name: "sam2-local", + connection_type: "local", + model_id: "facebook/sam2.1-hiera-base-plus", + model_revision: "b73207", + device: "cuda", + precision: "fp16", + endpoint_url: null, + setup_state: setup, + allowed_actions: ["download_weights", "update", "delete"], + capabilities: setup === "ready" ? ["point_suggest"] : [], + download: + download === null + ? null + : { job_id: JOB, error: null, ...download }, + created_at: "2026-08-08T00:00:00Z", + updated_at: "2026-08-08T00:00:00Z", + }; +} + +/** + * A workspace whose one connection is whatever `next` last returned. + * + * Mutable rather than frozen, and derived rather than replayed, for the reason + * `refactor-protocol` states about doubles: a stub that answers from a script + * makes a test assert against its own fixture, and a stub whose answer depends on + * registration order makes it assert against the order too. `next` is asked on + * every request, so what a test changes is the *workspace*, and the screen reads + * it the way it reads a server. + */ +async function serveApi(page: Page, next: () => unknown): Promise { + await page.route("**/api/session", (route) => route.fulfill({ json: { issued: false } })); + await page.route("**/api/inference/connections*", (route) => + route.fulfill({ status: 200, json: { items: [next()], total: 1 } }), + ); + await page.route("**/api/projects**", (route) => + route.fulfill({ status: 200, json: { items: [], total: 0 } }), + ); +} + +async function openInference(page: Page): Promise { + await page.goto("/"); + await page.getByTestId("token-input").fill("a-token"); + await page.getByTestId("token-submit").click(); + await page.getByTestId("rail-inference").click(); + await expect(page.getByTestId("inference-screen")).toBeVisible(); +} + +test("a page that never started the download still shows it", async ({ page }) => { + // The shipped bug, in the one shape that could not be tested without a browser: + // the job id lived in a component, so only the mount that pressed the button + // could see the transfer. Everything else — a reload, a second tab, a colleague + // — got `Not set up` beside a download that was still running. + await serveApi(page, () => + connection("not_set_up", { state: "running", bytes_done: 0.4 * GIGABYTE, bytes_total: 1.6 * GIGABYTE }), + ); + await openInference(page); + + await expect(page.getByTestId("download-progress-prose")).toHaveText("400.0 MB of 1.6 GB · 25%"); + await expect(page.getByTestId("download-bar")).toHaveAttribute("aria-valuenow", "25"); + + // Throw the application away. Nothing survives this that a client is holding. + await page.reload(); + await expect(page.getByTestId("inference-screen")).toBeVisible(); + + await expect(page.getByTestId("download-progress-prose")).toHaveText("400.0 MB of 1.6 GB · 25%"); + await expect(page.getByTestId("download-bar")).toHaveAttribute("aria-valuenow", "25"); +}); + +test("a transfer left running is where it got to when you come back", async ({ page }) => { + // Navigating away is the ordinary way somebody loses sight of a download, and + // the poll that was watching it goes with the screen. What comes back is read + // from the wire, so it shows the *current* number rather than the last one this + // browser happened to see. + let done = 0.4 * GIGABYTE; + await serveApi(page, () => + connection("not_set_up", { state: "running", bytes_done: done, bytes_total: 1.6 * GIGABYTE }), + ); + await openInference(page); + await expect(page.getByTestId("download-progress-prose")).toContainText("25%"); + + await page.getByTestId("rail-projects").click(); + await expect(page.getByTestId("inference-screen")).toHaveCount(0); + // The worker keeps going, because nothing about it was ever this browser's. + done = 1.2 * GIGABYTE; + + await page.getByTestId("rail-inference").click(); + await expect(page.getByTestId("download-progress-prose")).toHaveText("1.2 GB of 1.6 GB · 75%"); +}); + +test("the bar moves on the poll alone, and stops being a bar when it lands", async ({ page }) => { + // Nothing is clicked after the screen opens. Every number below arrives because + // a timer fired, a request went out, and the answer changed — which is the one + // part of this feature that only a browser can be asked about. + let done = 0.8 * GIGABYTE; + let settled = false; + await serveApi(page, () => + settled + ? connection("ready", { + state: "succeeded", + bytes_done: 1.6 * GIGABYTE, + bytes_total: 1.6 * GIGABYTE, + }) + : connection("not_set_up", { + state: "running", + bytes_done: done, + bytes_total: 1.6 * GIGABYTE, + }), + ); + await openInference(page); + await expect(page.getByTestId("download-progress-prose")).toContainText("50%"); + + done = 1.4 * GIGABYTE; + await expect(page.getByTestId("download-progress-prose")).toContainText("88%"); + + settled = true; + await expect(page.getByTestId("connection-status")).toContainText("Ready"); + // The success treatment is the row's own status; the bar goes with the transfer. + await expect(page.getByTestId("download-progress")).toHaveCount(0); +}); + +test("arriving after a transfer finished shows the row and no bar", async ({ page }) => { + // The record stays on the connection once the job settles — it answers *what + // happened last time* — and a settled record is not something to draw a bar for. + await serveApi(page, () => + connection("ready", { + state: "succeeded", + bytes_done: 1.6 * GIGABYTE, + bytes_total: 1.6 * GIGABYTE, + }), + ); + await openInference(page); + + await expect(page.getByTestId("connection-status")).toContainText("Ready"); + await expect(page.getByTestId("download-progress")).toHaveCount(0); + await expect(page.getByTestId("download-error")).toHaveCount(0); +}); + +test("a transfer that failed while nobody was watching still says why", async ({ page }) => { + await serveApi(page, () => + connection("not_set_up", { + state: "failed", + bytes_done: 0.3 * GIGABYTE, + bytes_total: 1.6 * GIGABYTE, + error: "could not fetch facebook/sam2.1-hiera-base-plus at b73207: the connection was lost", + }), + ); + await openInference(page); + + const shown = page.getByTestId("download-error"); + await expect(shown).toContainText("the connection was lost"); + await expect(shown).toContainText("still Not set up"); + // The remedy is the action the connection declares, not a second control. + await expect(page.getByTestId("download-weights")).toBeEnabled(); +}); diff --git a/frontend/ui-core/src/data/inferenceQueries.ts b/frontend/ui-core/src/data/inferenceQueries.ts index 527d7ad..953e789 100644 --- a/frontend/ui-core/src/data/inferenceQueries.ts +++ b/frontend/ui-core/src/data/inferenceQueries.ts @@ -96,6 +96,32 @@ export type ConnectionType = components["schemas"]["ConnectionType"]; export type ConnectionSetupState = components["schemas"]["ConnectionSetupState"]; export type Precision = components["schemas"]["Precision"]; export type DownloadSizeOut = components["schemas"]["DownloadSizeOut"]; +export type WeightDownload = components["schemas"]["WeightDownloadOut"]; + +/** + * Whether this connection's weights are arriving right now. + * + * Named for the *live* states rather than for the settled ones, which is the + * opposite of `usePollingQuery`'s rule and is deliberate: there, the terminal + * states are the enumerated ones and a predicate written the other way round + * would silently keep polling something added later. Here the live pair is what + * the domain enumerates — a job is `queued` then `running` and everything else is + * an ending — so this is the set that cannot grow without a kernel change. + */ +export function isDownloading(connection: Connection): boolean { + const state = connection.download?.state; + return state === "queued" || state === "running"; +} + +/** + * How often the connection list is re-read while a transfer is in flight. + * + * The same two seconds `DEFAULT_POLL_MS` uses, and for its stated reason: under + * the threshold where a person decides nothing is happening, and slow enough not + * to compete with the work being measured. The worker writes progress at most + * twice this often, so a faster poll would read the same number twice. + */ +export const DOWNLOAD_POLL_MS = 2_000; export const inferenceKeys = { connections: () => ["inference", "connections"] as const, @@ -116,6 +142,22 @@ export const inferenceKeys = { * Unfiltered on purpose: "none configured" and "one configured but its weights * are not here" are different sentences with different remedies, and a filtered * list would make them look identical. + * + * ## Why this polls, and only sometimes + * + * A weight download is a background job the server owns, and the wire says how + * far it has got — so a screen showing one has to re-read something. It re-reads + * *this*, because the connection is where the download lives: nothing has to + * remember a job id, and a page that arrives mid-transfer sees it on its first + * fetch. Recovery after a navigation, a reload or a fresh tab is then a property + * of the shape rather than a feature anybody wrote. + * + * The interval is a function of the answer, which is what stops it: `false` the + * moment no row reports a live transfer, and `false` before the first one lands. + * That last part is where this departs from `usePollingQuery`, which keeps asking + * when it has no data — the right rule for a job somebody is waiting on, and the + * wrong one here, because this list is also the annotator's read and a broken + * endpoint would be polled forever behind a screen that never asked for one. */ export function useConnections(enabled = true): UseQueryResult { const client = useApiClient(); @@ -124,6 +166,14 @@ export function useConnections(enabled = true): UseQueryResult unwrap(await client.GET("/inference/connections", {}), checkListInferenceConnections), + refetchInterval: (query) => { + const page = query.state.data; + if (page === undefined) return false; + return page.items.some(isDownloading) ? DOWNLOAD_POLL_MS : false; + }, + // A download started and left to run is the ordinary way this is used, so a + // backgrounded tab must not come back to a bar frozen where it was hidden. + refetchIntervalInBackground: true, }); } diff --git a/frontend/ui-core/src/screens/InferenceScreen.tsx b/frontend/ui-core/src/screens/InferenceScreen.tsx index c77a5c4..fcdccac 100644 --- a/frontend/ui-core/src/screens/InferenceScreen.tsx +++ b/frontend/ui-core/src/screens/InferenceScreen.tsx @@ -63,6 +63,24 @@ * before the job row says so. The settle-invalidation is what makes the row * agree, and the prose describes a state the workspace is already in. * + * ## A download is watched, not owned + * + * Everything this screen shows about a transfer comes off `ConnectionOut.download` + * and nothing from client-held state. That is what makes the screen a viewport + * rather than the owner: arriving mid-download — a reload, a second tab, a return + * visit, another machine — shows the bar and the prose on the first fetch, because + * the row it was going to list says so anyway. + * + * The list re-reads itself while any row reports a live transfer and stops the + * moment none does (`useConnections`). Nothing the browser does reaches the job: + * it runs in a worker process the server owns, so navigating away or closing the + * tab neither cancels nor pauses it — only the poll stops. + * + * The integrity check still follows a job id this screen holds, and the asymmetry + * is deliberate rather than forgotten: a check is a different question with a + * different vocabulary and it is not on the connection's wire model, so there is + * nothing to read it off. It keeps the coupling the download shed. + * * ## The size is asked for before the connection exists * * The local form shows what a download would cost *before* @@ -88,6 +106,7 @@ import { useEffect, useState, type FormEvent, type JSX } from "react"; import { Async } from "../data/Async"; import { asApiError } from "../data/errors"; import { + isDownloading, useCheckIntegrity, useConnections, useCreateConnection, @@ -98,9 +117,11 @@ import { useUpdateConnection, type Connection, type ConnectionType, + type WeightDownload, } from "../data/inferenceQueries"; import { Badge } from "../primitives/Badge"; import { Button } from "../primitives/Button"; +import { Progress } from "../primitives/Feedback"; import { Dialog, DialogContent, @@ -260,8 +281,8 @@ function ConnectionRow({ }): JSX.Element { const can = new Set(connection.allowed_actions); const ready = connection.setup_state === "ready"; - const weights = useWeightsRun(connection, useDownloadWeights(), "DOWNLOAD_FAILED"); - const integrity = useWeightsRun(connection, useCheckIntegrity(), "WEIGHTS_DAMAGED"); + const weights = useDownloadRun(connection); + const integrity = useIntegrityRun(connection); return ( {connection.name} @@ -369,11 +390,13 @@ function ConnectionRow({ )} - {weights.running && weights.progress !== null && ( - - {weights.progress} - - )} + {/* + Rendered off the wire and not off `weights.running`, so a page that + arrived mid-transfer shows it on its first fetch — the whole point of + the download living on the connection. It disappears when the job + settles, at which point the row's own status says what happened. + */} + {weights.live !== null && } {integrity.running && integrity.progress !== null && ( {integrity.progress} files @@ -410,46 +433,72 @@ function ConnectionRow({ } /** - * One launched-and-polled job on a connection row: how to start it, and what it - * is saying. + * The connection's weight transfer, read off the row rather than followed. + * + * **Nothing here remembers that a download was started**, and that is the whole + * of why leaving the screen no longer loses one. The wire says which job, what + * state and how many bytes; this reads it. A version of this that held the job id + * from the `202` — which is what shipped — could only show a transfer the *same + * mount* had launched, so a reload, a second tab, or walking to another screen + * and back all produced `Not set up` beside a download that was still running. * - * 202 and poll, the contract the export route uses. It lives on the *row* rather - * than inside a control because the controls are menu items, and a menu closes - * when one is chosen — a job whose progress and refusal lived inside the item - * would take both with it on the way out. + * The mutation is still here because starting one is still an event, and its own + * refusal (a 409, a missing runtime) is answered to the request rather than to a + * job that was never created. + * + * A settled transfer stays readable, which is what makes a failure survivable: the + * row goes on carrying the sentence until a retry replaces the record. + */ +function useDownloadRun(connection: Connection): { + readonly start: () => void; + readonly running: boolean; + readonly live: WeightDownload | null; + readonly failure: { readonly code: string; readonly message: string } | null; +} { + const mutation = useDownloadWeights(); + const download = connection.download ?? null; + const live = isDownloading(connection); + return { + start: () => mutation.mutate(connection.id), + // `isPending` covers the gap between the click and the re-read that brings + // the queued job back, which is the one moment the wire has not caught up. + running: mutation.isPending || live, + live: live ? download : null, + failure: mutation.isError + ? asApiError(mutation.error) + : download?.state === "failed" + ? { code: "DOWNLOAD_FAILED", message: download.error ?? "The download did not finish." } + : null, + }; +} + +/** + * The integrity check on a connection row: how to start it, and what it is saying. + * + * 202 and poll, the contract the export route uses, and the shape the download + * used to share before its progress moved onto the connection. It lives on the + * *row* rather than inside a control because the control is a menu item, and a + * menu closes when one is chosen — a job whose progress and refusal lived inside + * the item would take both with it on the way out. * * **The list is re-read when the job settles.** What the `202` changes is the * declaration; what the *completion* changes is `setup_state` and, with it, the - * row's whole meaning. Without a re-read at that moment a finished download leaves - * `Not set up` on screen until somebody reloads the page. A settled job is a - * mutation like any other, so it invalidates what it touched — including the other - * direction, where a check that finds damage moves the row from `Ready` to `Not set - * up`. - * - * **Taken as a parameter rather than chosen inside**, because the row has two jobs - * to run. Two calls means two independent poll states, which is what - * keeps a running check from reading as a running download; passing the mutation - * in is what lets one body serve both without a `kind` flag branching over - * everything it does. + * row's whole meaning — a check that finds damage moves the row from `Ready` to + * `Not set up`. A settled job is a mutation like any other, so it invalidates + * what it touched. + * + * It keeps the job id the download gave up, and the asymmetry is honest rather + * than an oversight: a check is not on the connection's wire model, so there is + * nothing to read it off. It therefore has the coupling the download no longer + * has — a reload loses a check in flight — and `#492` records that. */ -function useWeightsRun( - connection: Connection, - mutation: { - readonly mutate: ( - id: string, - options: { readonly onSuccess: (queued: { readonly id: string }) => void }, - ) => void; - readonly isPending: boolean; - readonly isError: boolean; - readonly error: unknown; - }, - failed: string, -): { +function useIntegrityRun(connection: Connection): { readonly start: () => void; readonly running: boolean; readonly progress: string | null; readonly failure: { readonly code: string; readonly message: string } | null; } { + const mutation = useCheckIntegrity(); const refresh = useRefreshConnections(); const [jobId, setJobId] = useState(null); const job = useBackgroundJob(jobId); @@ -478,11 +527,70 @@ function useWeightsRun( failure: mutation.isError ? asApiError(mutation.error) : state === "failed" - ? { code: failed, message: job.data?.error ?? "The job did not finish." } + ? { code: "WEIGHTS_DAMAGED", message: job.data?.error ?? "The job did not finish." } : null, }; } +/** + * A transfer in flight: a bar somebody watches, and a sentence they can read. + * + * Both, never one — a bar alone cannot say *queued*, and prose alone makes + * somebody read a number every two seconds to find out whether anything is + * moving. Sizes rather than a bare percentage, because "38%" of an unstated + * amount answers neither *how much longer* nor *how much disk*. + * + * **Three renderings, and the middle one is why this is not one line.** + * + * - *Queued* — no bytes yet, and the honest bar is empty with a sentence saying + * what it is waiting for. + * - *Transferring* — determinate, and it is the whole point of the feature. + * - *Settling* — every byte is here and the job has not finished, because a + * download ends by reading what arrived and recording the connection ready. + * The bar is full and the sentence names that phase, so a bar sitting at 100% + * for a second reads as a step rather than as a stall. + * + * A transfer whose published total could not be read gets the sentence and **no + * bar at all**. `Progress` renders an indeterminate value as an empty track, + * which reads as *0%* — a lie in the one case where the truth is *this is going, + * and nobody can say how far*. Giving the primitive an indeterminate animation is + * a design-system change and not this screen's to make. + */ +function DownloadProgress({ download }: { readonly download: WeightDownload }): JSX.Element { + const { state, bytes_done: done, bytes_total: total } = download; + const known = total !== null && total > 0; + const settling = known && done >= total; + const percent = known ? Math.min(100, Math.round((done / total) * 100)) : null; + return ( +
+ {known && ( + + )} + {/* + Tabular figures, `DESIGN.md`'s Numbers rule: the number changes every two + seconds and the words after it must not move under a reader's eye. + */} + + {state === "queued" + ? "Queued — starts as soon as a worker is free." + : settling + ? "Checking what arrived…" + : known + ? `${bytes(done)} of ${bytes(total)} · ${percent}%` + : `${bytes(done)} so far — the published size could not be read.`} + +
+ ); +} + /** * Two steps on the way in, one on the way back. * @@ -872,12 +980,34 @@ function DownloadSizeLine({ ); } +/** + * One decimal place, in the reader's own locale. See {@link bytes}. + * + * Built once at module scope rather than per call: constructing an + * `Intl.NumberFormat` is the expensive half of formatting, and this one is called + * on every row of a list that re-reads itself twice a second while a download + * runs. + */ +const SCALED = new Intl.NumberFormat(undefined, { + minimumFractionDigits: 1, + maximumFractionDigits: 1, +}); + /** * Bytes as somebody reads them. * * Decimal units, because a download size is what a network moves and what a * publisher quotes; binary units would put a different number on screen from the * one the model's own page shows. + * + * **One helper, and every size on this screen goes through it** — the curated + * list's entries, the form's line before a confirm, and both halves of a transfer + * in flight. `DESIGN.md`'s Numbers rule states the reason: `1.2 GB` and `1,2 GB` + * on one screen is exactly how a call-site decision goes wrong. + * + * Locale-aware for the same rule's sake, and a whole number of bytes stays whole: + * `512 B` rather than `512.0 B`, because a unit small enough to count exactly is + * one nobody wants rounded. */ export function bytes(count: number): string { const units = ["B", "kB", "MB", "GB", "TB"]; @@ -887,7 +1017,7 @@ export function bytes(count: number): string { value /= 1000; unit += 1; } - return `${unit === 0 ? value : value.toFixed(1)} ${units[unit]}`; + return `${unit === 0 ? value.toLocaleString() : SCALED.format(value)} ${units[unit]}`; } function DeleteConnectionDialog({ diff --git a/frontend/ui-core/src/screens/inference.test.tsx b/frontend/ui-core/src/screens/inference.test.tsx index e263103..9e44f8b 100644 --- a/frontend/ui-core/src/screens/inference.test.tsx +++ b/frontend/ui-core/src/screens/inference.test.tsx @@ -28,7 +28,7 @@ import type { JSX, ReactNode } from "react"; import { ApiProvider } from "../data/ApiProvider"; import { InferenceScreen, bytes } from "./InferenceScreen"; import { CURATED_MODELS, DEFAULT_MODEL } from "./inferenceCatalog"; -import type { Connection } from "../data/inferenceQueries"; +import { DOWNLOAD_POLL_MS, type Connection } from "../data/inferenceQueries"; const API = "http://visionset.test"; @@ -117,6 +117,31 @@ function listing(rows: readonly Connection[]): void { }); } +/** + * The transfer a connection reports, as the wire spells it. + * + * A helper rather than a literal per test for `connection`'s reason: every field + * is required on the wire, and the generated runtime check refuses a response + * missing one — which renders as this screen's error card and reads as a + * component bug rather than as a stub that lied. + */ +type WeightDownload = NonNullable; + +function downloadOf( + state: WeightDownload["state"], + done: number, + total: number | null, + error: string | null = null, +): WeightDownload { + return { + job_id: "44444444-4444-4444-8444-444444444444", + state, + bytes_done: done, + bytes_total: total, + error, + }; +} + function job(state: string, processed = 0, total: number | null = null): unknown { return { id: "job-1", @@ -246,13 +271,91 @@ it("renders a refused download as prose carrying the install command", async () expect(shown.textContent).toContain('pip install "visionset[local-inference]"'); }); -it("watches the job a download hands back", async () => { +it("shows a transfer nobody on this page started", async () => { + // The whole point of the download living on the connection. Nothing is clicked + // here: the screen mounts onto a workspace where a download is already running, + // which is what a reload, a second tab, or a return visit looks like — and what + // used to render as `Not set up` beside a button somebody had already pressed. + listing([connection({ download: downloadOf("running", 400_000_000, 1_600_000_000) })]); + render(mount()); + + const shown = await screen.findByTestId("download-progress-prose"); + expect(shown.textContent).toBe("400.0 MB of 1.6 GB · 25%"); + expect(screen.getByTestId("download-bar").getAttribute("aria-valuenow")).toBe("25"); + // And it did so without asking about a job at all. + expect(sent.some((one) => one.url.includes("/background-jobs/"))).toBe(false); +}); + +it("names the queue rather than drawing a bar that has not moved", async () => { + listing([connection({ download: downloadOf("queued", 0, null) })]); + render(mount()); + + expect((await screen.findByTestId("download-progress-prose")).textContent).toContain("Queued"); + // A worker has not touched it, so there is no total either. + expect(screen.queryByTestId("download-bar")).toBeNull(); +}); + +it("names the phase after the bytes rather than sitting full", async () => { + // A download ends by reading what arrived and recording the connection ready, + // so every byte can be here while the job is still running. A full bar with no + // sentence reads as a stall. + listing([connection({ download: downloadOf("running", 1_600_000_000, 1_600_000_000) })]); + render(mount()); + + expect((await screen.findByTestId("download-progress-prose")).textContent).toBe( + "Checking what arrived…", + ); + expect(screen.getByTestId("download-bar").getAttribute("data-phase")).toBe("settling"); +}); + +it("draws no bar when the published size could not be read", async () => { + // Sizing reaches the hub's listing and the transfer reaches its files, so one + // can fail while the other runs. `Progress` renders an indeterminate value as + // an empty track, which would read as 0% — a lie in the one case where the + // truth is "this is going, and nobody can say how far". + listing([connection({ download: downloadOf("running", 700_000_000, null) })]); + render(mount()); + + const shown = await screen.findByTestId("download-progress-prose"); + expect(shown.textContent).toContain("700.0 MB so far"); + expect(shown.textContent).toContain("could not be read"); + expect(screen.queryByTestId("download-bar")).toBeNull(); +}); + +it("never re-reads a list nothing is moving in", async () => { + // The other half of the conditional poll, and the half a browser cannot be + // asked about: asserting that nothing happens over an interval means waiting on + // a clock, which `tests/scripts/e2e_discipline` forbids in a spec for the + // reason it gives. Here the request log is the state, and jsdom's scheduler is + // not the thing under test. + let reads = 0; + handlers.push((request) => { + if (request.method !== "GET" || !new URL(request.url).pathname.endsWith("/connections")) return; + reads += 1; + return { + status: 200, + body: { + items: [connection({ setup_state: "ready", allowed_actions: ["update", "delete"] })], + total: 1, + }, + }; + }); + + render(mount()); + await waitFor(() => + expect(screen.getByTestId("connection-status").textContent).toContain("Ready"), + ); + + const first = reads; + await new Promise((done) => setTimeout(done, DOWNLOAD_POLL_MS * 2)); + expect(reads).toBe(first); +}, 15_000); + +it("shows no progress for a connection that has never been downloaded", async () => { listing([connection()]); - on("POST", /\/download$/, { status: 202, body: job("queued") }); - on("GET", /^\/background-jobs\/job-1$/, { status: 200, body: job("running", 2, 5) }); render(mount()); - await userEvent.click(await screen.findByTestId("download-weights")); - expect((await screen.findByTestId("download-progress")).textContent).toContain("2 of 5"); + await screen.findByTestId("connections-table"); + expect(screen.queryByTestId("download-progress")).toBeNull(); }); // --- creating ------------------------------------------------------------------ @@ -562,47 +665,61 @@ it("shows a curated model at another revision as a custom connection", async () // --- the download's whole life -------------------------------------------------- -it("refreshes the row when the job finishes, with no reload", async () => { - // The bug this closes: the `202` invalidated the list, and nothing invalidated - // it again when the work actually finished — so the row sat at `Not set up` - // until the page was reloaded. - let ready = false; +it("follows a transfer to its end with no reload and no click", async () => { + // The bug this closes twice over: the `202` invalidated the list and nothing + // invalidated it again when the work finished, so the row sat at `Not set up` + // until somebody reloaded — and the only thing watching was a job id this page + // had to have launched itself. + // + // Now the list re-reads itself while the wire says a transfer is live, and + // stops when it says one is not. The screen never asks about a job. + let reads = 0; handlers.push((request) => { if (request.method !== "GET" || !new URL(request.url).pathname.endsWith("/connections")) return; - const row = ready - ? connection({ setup_state: "ready", allowed_actions: ["download_weights", "update", "delete"] }) - : connection(); + reads += 1; + const row = + reads < 2 + ? connection({ download: downloadOf("running", 800_000_000, 1_600_000_000) }) + : connection({ + setup_state: "ready", + allowed_actions: ["download_weights", "update", "delete"], + download: downloadOf("succeeded", 1_600_000_000, 1_600_000_000), + }); return { status: 200, body: { items: [row], total: 1 } }; }); - on("POST", /\/download$/, { status: 202, body: job("queued") }); - handlers.push((request) => { - if (!request.url.includes("/background-jobs/")) return; - // The job settles, and the row it moved is what the next listing answers. - ready = true; - return { status: 200, body: job("succeeded", 1, 1) }; - }); render(mount()); - await userEvent.click(await screen.findByTestId("download-weights")); - await waitFor(() => - expect(screen.getByTestId("connection-status").textContent).toContain("Ready"), + expect((await screen.findByTestId("download-progress-prose")).textContent).toContain("50%"); + + await waitFor( + () => expect(screen.getByTestId("connection-status").textContent).toContain("Ready"), + { timeout: 5_000 }, ); - // And the row's control follows the state it is now in. + // The bar goes with the transfer: the row's own status is the success treatment. + expect(screen.queryByTestId("download-progress")).toBeNull(); expect(screen.queryByTestId("download-weights")).toBeNull(); -}); + expect(sent.some((one) => one.url.includes("/background-jobs/"))).toBe(false); + + // And the poll stops, rather than re-reading a list nothing is moving. + const settled = reads; + await new Promise((done) => setTimeout(done, DOWNLOAD_POLL_MS * 1.5)); + expect(reads).toBe(settled); +}, 15_000); it("surfaces a failed download as prose, and leaves the same action as the retry", async () => { - listing([connection()]); - on("POST", /\/download$/, { status: 202, body: job("queued") }); - on("GET", /^\/background-jobs\/job-1$/, { - status: 200, - body: { - ...(job("failed") as Record), - error: "could not fetch facebook/sam2.1-hiera-base-plus at b73207: the connection was lost", - }, - }); + // Read off the row, so a transfer that died while nobody was watching still has + // its sentence when somebody comes back to the screen. Nothing is clicked here. + listing([ + connection({ + download: downloadOf( + "failed", + 300_000_000, + 1_600_000_000, + "could not fetch facebook/sam2.1-hiera-base-plus at b73207: the connection was lost", + ), + }), + ]); render(mount()); - await userEvent.click(await screen.findByTestId("download-weights")); const shown = await screen.findByTestId("download-error"); expect(shown.textContent).toContain("the connection was lost"); diff --git a/frontend/ui-core/src/styles.css b/frontend/ui-core/src/styles.css index 44c5f12..18edeb2 100644 --- a/frontend/ui-core/src/styles.css +++ b/frontend/ui-core/src/styles.css @@ -42,7 +42,8 @@ * Robomous coral, and the whole reason the palette above is grey. * * It appears in exactly **two** places in the product — the wordmark in the - * rail, and the ingest progress bar's fill — and `DESIGN.md` names both. That + * rail, and the `Progress` primitive's fill, whatever is being measured — and + * `DESIGN.md` names both. That * scarcity is the token's contract, not a coincidence of the current screens: * a third `bg-brand` is a design decision, not a styling one, and belongs in a * review rather than in a diff.