From 03021375077ade888e0f201b876d4c87e9a6f6a0 Mon Sep 17 00:00:00 2001 From: Jesus Armando Anaya Date: Mon, 10 Aug 2026 07:13:30 -0700 Subject: [PATCH 1/3] fix(annotator): a suggest click outside the asset is not a prompt MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pane spans the whole stage on purpose, so a press in the margin around the picture reaches the adapter with a coordinate outside the frame. A drag wants that — "make the box this big" survives leaving the picture. A prompt point cannot use it: there is nothing under the margin to segment. Measured before the fix, a click at (900, 700) on a 640x480 asset sent positive: [{"x":900,"y":700}], painted a dot there, and drew the answer. `withinBounds` joins `clampPoint` in core/geometry/primitives — the two questions a frame is asked, and the difference is whether a stray coordinate is work to be salvaged or an instruction that was never given. The adapter drops the press before the host hears about it, so no point is recorded, no request leaves and no preview moves: one guarantee rather than three. Asked in asset pixels, so zoom and pan are already accounted for. cf. #451. --- .../src/adapters/react/AnnotatorCanvas.tsx | 15 +++ .../src/core/geometry/primitives.test.ts | 45 ++++++- .../annotator/src/core/geometry/primitives.ts | 23 ++++ frontend/annotator/src/index.ts | 1 + frontend/app/e2e/annotate.spec.ts | 102 ++++++++++++++- .../src/annotator/suggestFlow.test.tsx | 118 +++++++++++++++++- 6 files changed, 296 insertions(+), 8 deletions(-) diff --git a/frontend/annotator/src/adapters/react/AnnotatorCanvas.tsx b/frontend/annotator/src/adapters/react/AnnotatorCanvas.tsx index df940d7a..bc93370b 100644 --- a/frontend/annotator/src/adapters/react/AnnotatorCanvas.tsx +++ b/frontend/annotator/src/adapters/react/AnnotatorCanvas.tsx @@ -128,6 +128,7 @@ import type { } from "react"; import { topmostAnnotationAt } from "../../core/geometry/hitTest"; +import { withinBounds } from "../../core/geometry/primitives"; import { assetTolerances } from "../../core/geometry/tolerance"; import { affordanceAt, viewerAffordanceAt } from "../../core/interaction/affordance"; import { transition } from "../../core/interaction/machine"; @@ -825,6 +826,20 @@ export function AnnotatorCanvas({ // A session with no handler swallows the press rather than falling through, // for the reason the prop's docstring gives. if (suggestion !== null) { + // The stage surround is not the asset. The pane spans the whole viewport + // on purpose (see the input surface's note below), so `point` here can be + // negative or past the edge — which a *drag* wants, because "make the box + // this big" survives leaving the picture, and which a prompt point cannot + // use: there is nothing under the margin for a segmenter to segment. So + // an out-of-frame press is dropped whole rather than clamped onto the + // edge, and dropped **here**, before the host is told: a point the host + // never hears about records no click, sends no request and moves no + // preview, which is one guarantee instead of three. + // + // Asked in asset pixels rather than in screen ones, so zoom and pan are + // already accounted for by `imagePoint` and there is no second transform + // to keep in step with the first. + if (!withinBounds(point, asset)) return; onSuggestPoint?.(point, event.altKey ? "negative" : "positive"); return; } diff --git a/frontend/annotator/src/core/geometry/primitives.test.ts b/frontend/annotator/src/core/geometry/primitives.test.ts index df788970..8906c2aa 100644 --- a/frontend/annotator/src/core/geometry/primitives.test.ts +++ b/frontend/annotator/src/core/geometry/primitives.test.ts @@ -1,6 +1,6 @@ /** - * The four primitives, and the one claim about `Bounds` a reader should not have - * to take on faith: that an `AssetDescriptor` is already a frame. + * The primitives, and the one claim about `Bounds` a reader should not have to + * take on faith: that an `AssetDescriptor` is already a frame. */ import { describe, expect, it } from "vitest"; @@ -10,6 +10,7 @@ import { clampPoint, closestPointOnSegment, distance, + withinBounds, type Bounds, } from "./primitives"; @@ -58,6 +59,46 @@ describe("a point pushed back inside the frame", () => { }); }); +/** + * The other question about a frame: not *where does this point go* but *was it + * in there at all*. A caller that must not salvage a stray coordinate asks this + * one — see the docstring for why the two exist side by side. + */ +describe("whether a point is inside the frame", () => { + const frame: Bounds = { width: 100, height: 50 }; + + it("says yes for a point in the middle", () => { + expect(withinBounds([40, 20], frame)).toBe(true); + }); + + it("counts the edges and the corners as inside, exactly as the clamp does", () => { + // The same range, stated twice: a point `clampPoint` would leave untouched + // is a point this must call inside, or the last row of pixels becomes a + // place where a press silently stops working. + for (const corner of [[0, 0], [100, 0], [0, 50], [100, 50]] as const) { + expect(withinBounds(corner, frame)).toBe(true); + expect(clampPoint(corner, frame)).toEqual([...corner]); + } + }); + + it("says no past each of the four edges, one axis at a time", () => { + expect(withinBounds([-0.001, 20], frame)).toBe(false); + expect(withinBounds([100.001, 20], frame)).toBe(false); + expect(withinBounds([40, -0.001], frame)).toBe(false); + expect(withinBounds([40, 50.001], frame)).toBe(false); + }); + + it("says no for a coordinate that is not a number", () => { + expect(withinBounds([Number.NaN, 20], frame)).toBe(false); + expect(withinBounds([40, Number.NaN], frame)).toBe(false); + }); + + it("takes an asset descriptor as its frame, like everything else here", () => { + expect(withinBounds([ASSET.width, ASSET.height], ASSET)).toBe(true); + expect(withinBounds([ASSET.width + 1, ASSET.height], ASSET)).toBe(false); + }); +}); + describe("a frame is a width and a height", () => { it("accepts an asset descriptor without conversion", () => { // The assignment is the assertion: if `AssetDescriptor` ever stopped diff --git a/frontend/annotator/src/core/geometry/primitives.ts b/frontend/annotator/src/core/geometry/primitives.ts index 8616672a..ce7e9fa8 100644 --- a/frontend/annotator/src/core/geometry/primitives.ts +++ b/frontend/annotator/src/core/geometry/primitives.ts @@ -79,3 +79,26 @@ export function closestPointOnSegment(p: Point, a: Point, b: Point): Point { export function clampPoint(p: Point, bounds: Bounds): Point { return [clamp(p[0], 0, bounds.width), clamp(p[1], 0, bounds.height)]; } + +/** + * Whether `p` is inside the frame at all — the question `clampPoint` answers by + * moving the point instead. + * + * The two are for different callers, and the difference is whether a stray + * coordinate is *work to be salvaged* or *an instruction that was never given*. + * A drag that left the picture still means "make the box this big", so it + * clamps; a click in the margin around the picture is not a click on anything, + * and clamping it would put a prompt point on the asset's edge that nobody + * placed there. + * + * Inclusive at both ends, matching `clampPoint`'s own range: the last row of + * pixels is part of the asset, and a rule that excluded `width` would make the + * edge a place where a press silently stopped working. + * + * `false` for a non-finite coordinate, which falls out of the comparisons rather + * than being tested for — the module note above says why nothing here guards + * against `NaN`, and here the honest answer for one happens to be "not inside". + */ +export function withinBounds(p: Point, bounds: Bounds): boolean { + return p[0] >= 0 && p[0] <= bounds.width && p[1] >= 0 && p[1] <= bounds.height; +} diff --git a/frontend/annotator/src/index.ts b/frontend/annotator/src/index.ts index 1201a12b..2242f684 100644 --- a/frontend/annotator/src/index.ts +++ b/frontend/annotator/src/index.ts @@ -63,6 +63,7 @@ export { clampPoint, closestPointOnSegment, distance, + withinBounds, type Bounds, } from "./core/geometry/primitives"; export { diff --git a/frontend/app/e2e/annotate.spec.ts b/frontend/app/e2e/annotate.spec.ts index ccf55fee..2b1b2b5b 100644 --- a/frontend/app/e2e/annotate.spec.ts +++ b/frontend/app/e2e/annotate.spec.ts @@ -160,6 +160,32 @@ function schemaOfSize(size: SchemaSize | undefined): typeof SCHEMA { }; } +/** + * A workspace with a segmenter in it, for the one scenario that needs the + * suggest tool to actually work. + * + * Off by default, because the interesting answer for every other test here is + * the empty list — that is the state the tool's explanation panel exists for, + * and it is what a workspace that has never been to the Inference section is in. + */ +const READY_SAM = { + id: "66666666-6666-4666-8666-666666666666", + name: "local sam", + connection_type: "local", + model_id: "facebook/sam2-hiera-base-plus", + model_revision: "main", + device: "cuda", + precision: "fp16", + endpoint_url: null, + setup_state: "ready", + allowed_actions: [], + capabilities: ["point_suggest"], + download: null, + integrity_check: null, + created_at: "2026-08-08T00:00:00Z", + updated_at: "2026-08-08T00:00:00Z", +}; + async function serveApi( page: Page, sent: Request[], @@ -167,6 +193,7 @@ async function serveApi( lifecycle: Lifecycle = openedWorld(), size?: SchemaSize, seeded: readonly Record[] = [], + suggestible = false, ): Promise { const stored: Record[] = [...seeded]; const batchBody = (): Record => ({ @@ -340,7 +367,23 @@ async function serveApi( // here: it is the state the panel's explanation exists for, and it is what a workspace // that has never been to the Inference section is in. if (path === "/inference/connections") { - return route.fulfill({ json: { items: [], total: 0 } }); + const items = suggestible ? [READY_SAM] : []; + return route.fulfill({ json: { items, total: items.length } }); + } + if (path === "/inference/suggest" && request.method() === "POST") { + // `SuggestionOut`: a `model_ref` and a `region` that wraps the geometry + // beside the score. A flatter shape is refused by the generated runtime + // check, which reads as "the server answered something this app does not + // recognise" and looks nothing like a stub bug. + return route.fulfill({ + json: { + model_ref: "facebook/sam2-hiera-base-plus@main", + region: { + geometry: { type: "bbox", x: 100, y: 100, width: 80, height: 60 }, + confidence: 0.91, + }, + }, + }); } return route.fulfill({ status: 500, json: { code: "NO_STUB", message: path } }); }); @@ -353,8 +396,9 @@ async function openJob( lifecycle?: Lifecycle, size?: SchemaSize, seeded?: readonly Record[], + suggestible?: boolean, ): Promise { - await serveApi(page, sent, progress, lifecycle, size, seeded); + await serveApi(page, sent, progress, lifecycle, size, seeded, suggestible); await page.goto(`/jobs/${JOB}`); await page.getByTestId("token-input").fill("a-token"); await page.getByTestId("token-submit").click(); @@ -2722,6 +2766,60 @@ test("the no-connection panel now has somewhere to send you (#424 D6)", async ({ await expect(page.getByTestId("inference-screen")).toBeVisible(); }); +/** + * The stage surround is not the asset — and this claim needs a browser, because + * jsdom has no surround. + * + * The pane spans the whole stage while the picture is fitted inside it with + * padding, so there is a margin around the image that is still the input + * surface. In jsdom every rectangle is zero, so that margin does not exist and a + * component test asserting anything about it would be asserting about nothing. + * Here the two rectangles are measured and the press is put between them. + * + * Run at a **non-default zoom**, because the rule has to be applied in asset + * pixels: one written against screen coordinates would pass at the fitted scale + * and refuse half the picture at any other. + */ +test("a suggest click in the margin around the picture asks nothing, at any zoom", async ({ + page, +}) => { + const sent: Request[] = []; + await openJob(page, sent, undefined, undefined, undefined, undefined, true); + + await page.getByTestId("tool-suggest").click(); + await expect(page.getByTestId("suggest-idle")).toBeVisible(); + + // Off the fitted scale, so nothing below can be true by accident of zoom 1. + await page.getByTestId("zoom-in").click(); + await expect(page.getByTestId("zoom-readout")).not.toHaveText("100%"); + + const asks = (): Request[] => + sent.filter((r) => r.method() === "POST" && r.url().endsWith("/inference/suggest")); + + // The `` is laid out at the asset's own size inside the scaled wrapper, + // so its box on screen *is* the picture's rectangle. + const picture = (await page.getByTestId("annotator-canvas").boundingBox())!; + const pane = (await page.getByTestId("annotator-pane").boundingBox())!; + // The fit leaves room on at least one axis, and this test is meaningless + // without it — so it is asserted rather than assumed. + expect(picture.x).toBeGreaterThan(pane.x + 2); + + // Left of the picture and inside the pane: on the stage, off the asset. + await page.mouse.click((pane.x + picture.x) / 2, picture.y + picture.height / 2); + // Nothing to wait for, so the absence is given a chance to be wrong: a press + // that *did* ask would have its request logged well inside this. + await expect(page.getByTestId("suggest-idle")).toBeVisible(); + expect(asks()).toHaveLength(0); + await expect(page.getByTestId("suggestion-shape")).toHaveCount(0); + + // The same gesture on the picture itself, so the absence above is a rule and + // not a broken fixture. + await page.mouse.click(picture.x + picture.width / 2, picture.y + picture.height / 2); + + await expect(page.getByTestId("suggestion-shape")).toBeVisible(); + expect(asks()).toHaveLength(1); +}); + /** * The notice surface, measured — which is the only way any of it can be claimed. * diff --git a/frontend/ui-core/src/annotator/suggestFlow.test.tsx b/frontend/ui-core/src/annotator/suggestFlow.test.tsx index 69b0a84e..5fef5cc2 100644 --- a/frontend/ui-core/src/annotator/suggestFlow.test.tsx +++ b/frontend/ui-core/src/annotator/suggestFlow.test.tsx @@ -268,17 +268,29 @@ async function arm(): Promise { await screen.findByTestId("suggest-panel"); } -/** One press on the canvas. `alt` is D2's negative point. */ -function clickCanvas(alt = false): void { +/** + * One press on the canvas, at a client position. + * + * jsdom's rect is all zeros and the initial view is identity, so a client + * coordinate here **is** an asset coordinate — which is what lets a test place a + * press inside or outside a 640 × 480 frame without any layout. `alt` is D2's + * negative point. + */ +function clickCanvasAt(clientX: number, clientY: number, alt = false): void { fireEvent.pointerDown(screen.getByTestId("annotator-pane"), { button: 0, - clientX: 100, - clientY: 100, + clientX, + clientY, altKey: alt, pointerId: 1, }); } +/** One press well inside the asset. `alt` is D2's negative point. */ +function clickCanvas(alt = false): void { + clickCanvasAt(100, 100, alt); +} + /** Every suggest request that has left, newest last. */ function asks(): readonly Record[] { return sent @@ -488,6 +500,104 @@ describe("a click asks the model", () => { }); }); +/** + * The stage surround is not the asset. + * + * The pane spans the whole viewport on purpose, so a press in the margin around + * the picture reaches the adapter with a coordinate outside the frame. A *drag* + * wants that — "make the box this big" survives leaving the picture. A prompt + * point cannot use it: there is nothing under the margin to segment. + * + * Before this, such a press was a prompt like any other. Measured on the fixture + * below: a click at (900, 700) on a 640 × 480 asset sent + * `positive: [{"x":900,"y":700}]`, painted a dot there, and drew whatever came + * back. + */ +describe("a press outside the asset is not a prompt", () => { + it("sends nothing and shows nothing", async () => { + await open(); + await arm(); + + clickCanvasAt(900, 700); + + // Waited on rather than asserted immediately, so this cannot pass merely by + // reading the log before a request had time to leave. + await waitFor(() => expect(screen.getByTestId("suggest-idle")).toBeTruthy()); + expect(asks()).toHaveLength(0); + expect(screen.queryByTestId("suggestion-shape")).toBeNull(); + }); + + it("swallows a negative press the same way", async () => { + await open(); + await arm(); + + clickCanvasAt(-5, 200, true); + + await waitFor(() => expect(screen.getByTestId("suggest-idle")).toBeTruthy()); + expect(asks()).toHaveLength(0); + }); + + it("records no point, so the next real click is still the first one", async () => { + await open(); + await arm(); + clickCanvasAt(900, 700); + clickCanvasAt(320, -40); + + clickCanvas(); + + await waitFor(() => expect(asks()).toHaveLength(1)); + // One point, not three. A dropped press that had quietly accumulated would + // send the model two coordinates nobody clicked on. + expect(asks()[0]["positive"]).toHaveLength(1); + expect(asks()[0]["negative"]).toEqual([]); + }); + + it("leaves a preview that is already showing exactly where it is", async () => { + await open(); + await arm(); + clickCanvas(); + await screen.findByTestId("suggestion-shape"); + + clickCanvasAt(900, 700); + + // Not a refine, and not a discard either: nothing happened at all, so the + // shape a person was about to accept is still there and still acceptable. + expect(screen.getByTestId("suggestion-shape")).toBeTruthy(); + expect(asks()).toHaveLength(1); + }); + + it("counts the asset's own edge as inside", async () => { + await open(); + await arm(); + + // The last row of pixels is part of the picture; a rule that excluded it + // would make the border a place where a press silently stopped working. + clickCanvasAt(640, 480); + + await waitFor(() => expect(asks()).toHaveLength(1)); + }); + + it("asks in asset pixels, so the same screen position changes answer under zoom", async () => { + await open(); + await arm(); + + // Outside at 1x: the asset is 640 wide and this is 700. + clickCanvasAt(700, 100); + await waitFor(() => expect(screen.getByTestId("suggest-idle")).toBeTruthy()); + expect(asks()).toHaveLength(0); + + // The same screen position, one zoom step in. jsdom's pane measures zero, so + // zooming about its centre leaves the pan at the origin and 700 screen + // pixels is now 560 asset pixels — inside. A bounds check written against + // screen coordinates would still refuse it. + await userEvent.click(screen.getByTestId("zoom-in")); + clickCanvasAt(700, 100); + + await waitFor(() => expect(asks()).toHaveLength(1)); + expect(asks()[0]["positive"]).toEqual([{ x: 560, y: 80 }]); + }); +}); + describe("the preview is outside the document and outside the history", () => { /** * **The mutation test for D4.** Turn a suggestion into a `stage` or an `add` From dfbb3778396a4a8a28a843c64bbc0d081363eada Mon Sep 17 00:00:00 2001 From: Jesus Armando Anaya Date: Mon, 10 Aug 2026 07:13:42 -0700 Subject: [PATCH 2/3] fix(annotator): the canvas says the class, and only for the shape you picked MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two rules on one element. The class label renders **only while its shape is selected** — a frame of forty boxes drew forty class names over the picture at all times, which hid the asset behind the annotations of it. The panel is the full inventory; the canvas answers what *this* one is. That rule is DESIGN.md's own, written under the v1 metrics kept as the reference; the React adapter has rendered the label unconditionally since the layer was first written, so this is the adapter starting to obey a rule that was already recorded. And the label says the class and nothing else. A confidence tells somebody whether to accept a proposal; once accepted the shape is a label like any other, so the number stays on the live suggestion preview and leaves the rest of the editor — no percentage on a canvas box, on a panel row, or in a tooltip. The Sparkles glyph is now the only provenance signal, and its tooltip carries the model_ref alone. Nothing is discarded: confidence and model_ref are stored unchanged, and the number's home is the batch review loop. `PaintedAnnotation` gives back the two fields it gained for the old label. A projected field with no reader is what a renderer starts writing next; absence is the rule, enforced where a component cannot reach around it. `confidencePercent` stays exported — one consumer today, and it is the spelling whoever shows the number next must import rather than respell. cf. #417, #425, #451, #512. --- DESIGN.md | 46 ++++-- .../annotator/src/adapters/react/Shapes.tsx | 42 ++--- .../src/adapters/react/paint.test.ts | 73 ++++---- .../annotator/src/adapters/react/paint.ts | 28 ++-- .../ui-core/src/annotator/AnnotatorPanel.tsx | 34 ++-- .../src/annotator/canvasLabel.test.tsx | 156 ++++++++++++++++++ frontend/ui-core/src/annotator/panel.test.tsx | 30 +++- 7 files changed, 286 insertions(+), 123 deletions(-) create mode 100644 frontend/ui-core/src/annotator/canvasLabel.test.tsx diff --git a/DESIGN.md b/DESIGN.md index e1d99b45..175f7055 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -1008,22 +1008,36 @@ gallery badges (#55) — and it **already exists, shipped and unit-tested**: outline; the class label renders only while selected, 11px / 700, anchored at the first vertex, never a pointer target. -**A model's work is marked; a person's is not.** `provenance: "model"` earns a mark on the -canvas label (`class · 62%`, and `class · model` where the model recorded no score) and a -`Sparkles` glyph with the score on the side-panel row, whose accessible name carries the -claim in words and whose tooltip carries the full `model_ref`. Never colour alone — class -colour is already user data and cannot also mean provenance. **Absence is the human case**: -no "manual" badge, no mark on the common path, because the row a reviewer sees a thousand -times is the one that must stay quiet. `import` provenance is unmarked until there is an -importer whose mark would mean something. - -**Confidence has one spelling, and it is whole percent.** `confidencePercent` in -`frontend/annotator/src/adapters/react/paint.ts` — the same shared-helper rule as -`classColor` directly above, and for the same reason: a live suggestion, a committed -annotation's canvas label and its panel row all show the same number, and two notations for -it is a number that disagrees with itself. Two decimals would claim a precision a -confidence does not have. A `null` confidence reads as absent — never as `0`, never as a -low score. +**The canvas label is part of what selection looks like.** A frame carrying forty boxes drew +forty class names over the picture at all times, which hides the asset behind the annotations +of it. The panel is the full inventory; the canvas answers *what is this one* for the shape +somebody picked, and an unselected shape is its box alone. This is also how a viewer's +selection reads, since a read-only frame paints no grips. + +**A model's work is marked; a person's is not.** `provenance: "model"` earns a `Sparkles` +glyph on the side-panel row, whose accessible name carries the claim in words and whose +tooltip carries the full `model_ref`. That glyph is the *only* provenance signal in the +editor — the canvas label is the class and nothing else. Never colour alone — class colour is +already user data and cannot also mean provenance. **Absence is the human case**: no "manual" +badge, no mark on the common path, because the row a reviewer sees a thousand times is the one +that must stay quiet. `import` provenance is unmarked until there is an importer whose mark +would mean something. + +**Confidence renders on the live suggestion preview and nowhere else in the editor.** The +number tells somebody whether to accept a proposal; once accepted, the shape is a label like +any other and the score is decoration on every subsequent reading of it. So no percentage on a +canvas box, on a panel row, or in a tooltip. It is not discarded — `confidence` and +`model_ref` are stored unchanged — and its home is the batch review loop, where a surface +showing it must also name *what it measures*: a point-prompted mask score and a detection's +prompt affinity are different quantities on different scales and cannot be pooled, thresholded +or sorted together. + +**Where it is shown, it has one spelling, and that is whole percent.** `confidencePercent` in +`frontend/annotator/src/adapters/react/paint.ts` — the same shared-helper rule as `classColor` +above: whoever shows the number imports it rather than respelling it, because two notations +for one quantity is a number that disagrees with itself. Two decimals would claim a precision +a confidence does not have. A `null` confidence reads as absent — never as `0`, never as a low +score. ## Libraries diff --git a/frontend/annotator/src/adapters/react/Shapes.tsx b/frontend/annotator/src/adapters/react/Shapes.tsx index 95069494..4ef8c07e 100644 --- a/frontend/annotator/src/adapters/react/Shapes.tsx +++ b/frontend/annotator/src/adapters/react/Shapes.tsx @@ -64,7 +64,7 @@ import type { PolygonGeometry, PolylineGeometry, } from "../../core/types"; -import { confidencePercent, screenPx } from "./paint"; +import { screenPx } from "./paint"; import type { PaintedAnnotation } from "./paint"; /** Stroke thickness, in screen pixels, for an unselected shape. */ @@ -273,29 +273,16 @@ export function Vertices({ points, color, zoom, hotIndex }: { ); } -/** - * What a label says: the class, and for a model's work what the model claimed. - * - * A suffix rather than a second element, so the shipped label metrics are - * untouched — same size, same weight, same anchor, same lift. The label only - * gets longer, and only on a selected shape, which is the one place it renders - * at all. - * - * A model-produced annotation always carries the separator, because - * `confidence` is optional on one and a shape marked only when the model - * happened to score itself is not a mark a reviewer can trust the absence of. - * `import` provenance reads as a person's for now — the importers that would - * make that distinction mean something are not built. - */ -export function labelText(shape: PaintedAnnotation): string { - if (shape.provenance !== "model") return shape.labelClass; - const score = shape.confidence === null ? "model" : confidencePercent(shape.confidence); - return `${shape.labelClass} · ${score}`; -} - /** * The class name, above the shape. * + * **The class and nothing else.** A confidence is a decision aid for + * accept-or-reject and stops being one the moment a shape is accepted, so the + * number lives on the live suggestion preview and nowhere else in the editor; + * which model produced a stored shape is the side panel's mark. A canvas that + * wrote either would be answering, on every selected shape, a question a + * reviewer asks about one. + * * `y` is the anchor itself — a plain asset coordinate — and the screen-pixel lift * above it rides on the CSS `translate` property instead of being subtracted here. * That is what keeps every one of this element's attributes free of the zoom: @@ -318,7 +305,7 @@ export function ShapeLabel({ shape }: { readonly shape: PaintedAnnotation }): JS translate: "0 var(--vs-label-lift)", }} > - {labelText(shape)} + {shape.labelClass} ); } @@ -352,7 +339,14 @@ export function PolylineShape({ geometry, color, hot, selected }: { } /** - * One committed annotation: its shape, its label, and its grips when selected. + * One committed annotation: its shape, and — while it is selected — its label + * and its grips. + * + * **The label is part of what selection looks like.** A frame carrying forty + * boxes drew forty class names over the picture at all times, which hid the + * asset behind the annotations of it; the panel is the full inventory, and the + * canvas answers "what is *this* one" for the shape somebody picked. It is also + * how a viewer's selection reads, since a read-only frame paints no grips. * * Grips and vertices are drawn only for a selected shape, which mirrors * `resolveTarget` — it looks for a handle or a vertex only among the *selected* @@ -386,7 +380,7 @@ export function AnnotationShape({ shape, zoom, handles }: ShapeProps): JSX.Eleme selected={shape.selected} /> )} - + {shape.selected && } {handles && shape.selected && shape.geometry.type === "bbox" && ( )} diff --git a/frontend/annotator/src/adapters/react/paint.test.ts b/frontend/annotator/src/adapters/react/paint.test.ts index f7f1437b..f142c290 100644 --- a/frontend/annotator/src/adapters/react/paint.test.ts +++ b/frontend/annotator/src/adapters/react/paint.test.ts @@ -11,7 +11,6 @@ import { ASSET, SCHEMA, annotation } from "../../core/state/_sample"; import { createDocument } from "../../core/state/document"; import { EMPTY_SELECTION, selectionOf } from "../../core/state/selection"; import type { Annotation } from "../../core/types"; -import { labelText } from "./Shapes"; import { SUGGESTION_DASH, SUGGESTION_OPACITY, @@ -327,7 +326,15 @@ describe("a pending suggestion, drawn as a proposal (#424)", () => { }); }); -describe("what a model produced, projected so the label can say so", () => { +/** + * Where a model's confidence lives, and where the draw list refuses to carry it. + * + * The preview shows the number because it is what an accept-or-reject decision + * is made on; a committed annotation does not, because by then the decision has + * been made. The projection is what enforces the second half: a renderer cannot + * write a score it was never given. + */ +describe("a committed annotation's confidence is not the canvas's business", () => { function predicted(overrides: Partial = {}): Annotation { return { ...annotation("m1"), @@ -349,45 +356,39 @@ describe("what a model produced, projected so the label can say so", () => { return painted; } - it("carries both fields onto the draw list", () => { - const shape = paintOne(predicted()); - expect(shape.provenance).toBe("model"); - expect(shape.confidence).toBe(0.62); - }); - - it("carries a person's own values too, so the label can tell them apart", () => { - const shape = paintOne(annotation("h1")); - expect(shape.provenance).toBe("human"); - expect(shape.confidence).toBeNull(); - }); - - it("writes the model's score beside the class, at the shipped spelling", () => { - expect(labelText(paintOne(predicted()))).toBe("sign · 62%"); + it("hands the renderer neither the score nor who produced it", () => { + // Absence in the projection, not a rule in the component: a `` that + // has never been given a confidence cannot start writing one. + const shape: Record = { ...paintOne(predicted()) }; + expect("confidence" in shape).toBe(false); + expect("provenance" in shape).toBe(false); }); - it("still marks a model's work when the model reported no score", () => { - // `confidence` is optional on a model-produced annotation, and a mark that - // appeared only when a model happened to score itself is a mark whose - // absence says nothing. - expect(labelText(paintOne(predicted({ confidence: null })))).toBe("sign · model"); + it("paints a model's shape exactly as it paints a person's", () => { + // Every field the two share, and there is now nothing else: the canvas + // cannot tell them apart, which is the point. + const mine = paintOne(annotation("m1")); + const theirs = paintOne(predicted()); + expect(theirs).toEqual({ ...mine, id: theirs.id }); }); - it("leaves a person's label exactly as it shipped", () => { - expect(labelText(paintOne(annotation("h1")))).toBe("sign"); - }); - - it("leaves an imported label alone, having no mark that would mean anything", () => { - const imported = predicted({ provenance: "import", model_ref: null, confidence: null }); - expect(labelText(paintOne(imported))).toBe("sign"); + it("keeps the number on the live preview, which is where the decision is", () => { + // The one surface in the editor that shows it, and `paintSuggestion` is + // what puts it there — see the `#424` block above for the full range. + const asked = withPoint(armed("sign"), [100, 120], "positive"); + const preview = answered(asked, asked.serial, { + geometry: { type: "bbox", x: 10, y: 20, width: 30, height: 40 }, + confidence: 0.62, + modelRef: "facebook/sam2-hiera-base-plus@main", + }); + const declared = SCHEMA.classes.find((one) => one.name === "sign"); + expect(paintSuggestion(preview, declared)?.label).toBe("sign 62%"); }); - it("spells the score the way the suggestion overlay already does", () => { - // One quantity, one spelling. `confidenceLabel` is the canvas's live - // suggestion; `labelText` is the committed annotation; a reader looking at - // both at once must not see two notations for the same number. - expect(labelText(paintOne(predicted({ confidence: 0.87 })))).toContain( - confidencePercent(0.87), - ); - expect(confidenceLabel("sign", 0.87)).toContain(confidencePercent(0.87)); + it("spells it one way, so no second surface can disagree with the first", () => { + // `confidencePercent` is exported for this: whoever shows the number + // imports it. Two `Math.round`s would be the same quantity in two + // notations, which is the defect the shared helper exists to prevent. + expect(confidenceLabel("sign", 0.87)).toBe(`sign ${confidencePercent(0.87)}`); }); }); diff --git a/frontend/annotator/src/adapters/react/paint.ts b/frontend/annotator/src/adapters/react/paint.ts index cd94514f..ed3d9f4d 100644 --- a/frontend/annotator/src/adapters/react/paint.ts +++ b/frontend/annotator/src/adapters/react/paint.ts @@ -53,7 +53,6 @@ import type { Point, PolygonGeometry, PolylineGeometry, - Provenance, } from "../../core/types"; /** A shape whose class the schema declares, ready to draw. */ @@ -66,17 +65,6 @@ export interface PaintedAnnotation { /** Under the pointer, or held by the drag in flight. */ readonly hot: boolean; readonly color: string; - /** - * Who drew it, projected so the label can say so. - * - * Carried rather than derived at the label because the draw list is where a - * renderer's questions are already answered — a `` that reached back - * into the document for provenance would be the one component that needs a - * document, and the only reason for it would be four characters of suffix. - */ - readonly provenance: Provenance; - /** How sure the model was, and `null` for every label a person drew. */ - readonly confidence: number | null; } /** @@ -188,8 +176,6 @@ function painted( selected: selection.has(annotation.id), hot: annotation.id === hotId, color: classColor(declared, annotation.label_class), - provenance: annotation.provenance, - confidence: annotation.confidence, }; } @@ -328,9 +314,17 @@ export function paintSuggestion( * have. A confidence outside `[0, 1]` cannot arrive — the kernel's * `PredictedRegion` refuses one — so nothing is clamped here. * - * Exported for the same reason `classColor` is: the panel shows the same - * quantity as the canvas, and a second spelling of it in `ui-core` would be a - * number that disagrees with itself across two surfaces. + * **One consumer today** — the live suggestion preview, through + * `confidenceLabel`. Exported anyway, for the reason `classColor` is exported: + * this is the spelling, and the surfaces that will show a confidence next are + * outside this package. A second `Math.round` in `ui-core` would be the same + * number disagreeing with itself across two screens, which is the defect the + * shared helper exists to make impossible rather than merely unlikely. + * + * The editor deliberately shows it nowhere else. A confidence helps somebody + * decide whether to accept a proposal; once accepted, the shape is a label like + * any other, and the number would be decoration on every subsequent reading of + * it. */ export function confidencePercent(confidence: number): string { return `${Math.round(confidence * 100)}%`; diff --git a/frontend/ui-core/src/annotator/AnnotatorPanel.tsx b/frontend/ui-core/src/annotator/AnnotatorPanel.tsx index 0fa591bf..dd73d104 100644 --- a/frontend/ui-core/src/annotator/AnnotatorPanel.tsx +++ b/frontend/ui-core/src/annotator/AnnotatorPanel.tsx @@ -72,7 +72,6 @@ import { annotationsInDrawOrder, classNamed, - confidencePercent, hotkeyForClass, isTaggableClass, randomUuid, @@ -383,7 +382,7 @@ function TagStrip({ } /** - * What a model produced, and how sure it was — on the rows that have one. + * That a model produced this one — on the rows that have one, and nothing more. * * Renders nothing at all for a person's work. That is the whole of the design: * accepting a predicted box is the act that most needs the reviewer to know what @@ -391,6 +390,14 @@ function TagStrip({ * earns no badge. Absence is the human case, so the row a reviewer sees a * thousand times is exactly the row that shipped. * + * **The confidence is deliberately not here.** It is a decision aid for + * accept-or-reject and it stops being one once the shape is accepted; a column + * of percentages down a settled list invites a reviewer to sort by a number + * that means different things per capability — a point-prompted mask score and + * a detection's prompt affinity are not the same quantity. Where the number + * does belong is the batch review loop, and it will be named there. In the + * editor it appears on the live suggestion preview alone. + * * Never colour alone: the glyph carries it, and the accessible name says it in * words, so neither a monochrome screen nor a screen reader depends on the * muted foreground this is tinted with. @@ -408,32 +415,17 @@ function ModelMark({ readonly index: number; }): JSX.Element | null { if (annotation.provenance !== "model") return null; - // `confidencePercent`, not a local `toFixed` — the canvas writes the same - // quantity beside the same annotation, and DESIGN.md's rule for `classColor` - // is the rule here too: ui-core imports the answer, it does not respell it. - const score = annotation.confidence === null ? null : confidencePercent(annotation.confidence); return ( {/* The full reference, which is far too long for the row and is exactly diff --git a/frontend/ui-core/src/annotator/canvasLabel.test.tsx b/frontend/ui-core/src/annotator/canvasLabel.test.tsx new file mode 100644 index 00000000..2c78617f --- /dev/null +++ b/frontend/ui-core/src/annotator/canvasLabel.test.tsx @@ -0,0 +1,156 @@ +/** + * What the canvas writes over a shape, and when. + * + * Two rules, one element. The label renders **only while its shape is + * selected** — a frame of forty boxes drawing forty class names hides the asset + * behind the annotations of it, and the panel is the full inventory. And it + * says **the class and nothing else** — a confidence belongs to the + * accept-or-reject decision on the live preview, and which model produced a + * stored shape is the panel row's mark. + * + * Here rather than in `@visionset/annotator`, which has no DOM to render into: + * its suites are pure by construction, and this is a claim about markup. It is + * jsdom-complete — the presence or absence of a `` node is not something + * a browser knows better — so `AnnotationLayer` is driven directly rather than + * through the page. The label's *metrics* are the browser's business and are + * unchanged by this file. + */ + +import { + AnnotationLayer, + EMPTY_SELECTION, + documentFromWire, + selectionOf, +} from "@visionset/annotator"; +import { render, screen } from "@testing-library/react"; +import { describe, expect, it } from "vitest"; +import type { JSX } from "react"; +import type { Selection } from "@visionset/annotator"; + +const SCHEMA = { + project_id: "11111111-1111-4111-8111-111111111111", + version: 1, + classes: [ + { name: "vehicle", geometry: "bbox", color: "#38bdf8", attributes: [] }, + { name: "pedestrian", geometry: "bbox", color: null, attributes: [] }, + ], +}; + +function annotation(id: string, labelClass: string, overrides: Record = {}): unknown { + return { + id, + asset_id: "asset-1", + label_class: labelClass, + schema_version: 1, + geometry: { type: "bbox", x: 10, y: 10, width: 20, height: 20 }, + attributes: {}, + provenance: "human", + model_ref: null, + confidence: null, + job_id: null, + ...overrides, + }; +} + +/** A model's own work: the three fields the accept path stamps together. */ +function predicted(id: string, labelClass: string, confidence: number | null = 0.62): unknown { + return annotation(id, labelClass, { + provenance: "model", + model_ref: "IDEA-Research/grounding-dino-tiny@abc123", + confidence, + }); +} + +function paint(annotations: readonly unknown[], selection: Selection = EMPTY_SELECTION): JSX.Element { + const document = documentFromWire({ + asset: { id: "asset-1", width: 100, height: 100 }, + schema: SCHEMA, + annotations, + }); + return ( + + + + ); +} + +/** Every class name the canvas is currently writing. */ +function labels(): readonly string[] { + return [...screen.getByTestId("annotation-layer").querySelectorAll("text")].map( + (node) => node.textContent ?? "", + ); +} + +describe("the class label is part of what selection looks like", () => { + it("writes nothing over an unselected shape", () => { + render(paint([annotation("a", "vehicle")])); + + // The box is drawn; the name over it is not. + expect(screen.getByTestId("annotation-layer").querySelector("rect")).not.toBeNull(); + expect(labels()).toEqual([]); + }); + + it("writes the class over the shape somebody picked", () => { + render(paint([annotation("a", "vehicle")], selectionOf(["a"]))); + + expect(labels()).toEqual(["vehicle"]); + }); + + it("names only the selection, in a frame carrying several shapes", () => { + render( + paint( + [ + annotation("a", "vehicle"), + annotation("b", "pedestrian"), + annotation("c", "vehicle"), + ], + selectionOf(["b"]), + ), + ); + + // One name on a canvas of three, which is the whole of the rule: the panel + // is the inventory and the canvas answers "what is *this* one". + expect(labels()).toEqual(["pedestrian"]); + }); + + it("names each of several selected shapes", () => { + render( + paint([annotation("a", "vehicle"), annotation("b", "pedestrian")], selectionOf(["a", "b"])), + ); + + expect(labels()).toEqual(["vehicle", "pedestrian"]); + }); +}); + +describe("the label is the class and nothing else", () => { + it("writes no score over a model's shape", () => { + render(paint([predicted("m", "vehicle")], selectionOf(["m"]))); + + // The number aided a decision that has already been made. It is still + // stored, and it still renders on the live suggestion preview. + expect(labels()).toEqual(["vehicle"]); + }); + + it("writes no provenance mark either — the panel row carries that", () => { + render(paint([predicted("m", "vehicle", null)], selectionOf(["m"]))); + + expect(labels()).toEqual(["vehicle"]); + }); + + it("draws a model's selected shape exactly as it draws a person's", () => { + const { container: mine } = render(paint([annotation("h", "vehicle")], selectionOf(["h"]))); + const person = mine.innerHTML; + const { container: theirs } = render(paint([predicted("h", "vehicle")], selectionOf(["h"]))); + + // Same id, same class, same geometry: the only difference in the fixture is + // the three fields the canvas has stopped being given. + expect(theirs.innerHTML).toBe(person); + }); +}); diff --git a/frontend/ui-core/src/annotator/panel.test.tsx b/frontend/ui-core/src/annotator/panel.test.tsx index 8d441940..3e9bd24c 100644 --- a/frontend/ui-core/src/annotator/panel.test.tsx +++ b/frontend/ui-core/src/annotator/panel.test.tsx @@ -486,11 +486,10 @@ describe("what a model produced, on the row a reviewer accepts it from", () => { }; } - it("marks it, and says how sure the model was", () => { + it("marks it — that a model drew it, and no more than that", () => { render(mount(storeWith([predicted("m")]))); expect(screen.getByTestId("object-model-0")).toBeDefined(); - expect(screen.getByTestId("object-confidence-0").textContent).toBe("62%"); }); it("says in words what the glyph says in a picture", () => { @@ -499,26 +498,38 @@ describe("what a model produced, on the row a reviewer accepts it from", () => { render(mount(storeWith([predicted("m")]))); expect(screen.getByTestId("object-model-0").getAttribute("aria-label")).toBe( - "Model-produced by IDEA-Research/grounding-dino-tiny@abc123, confidence 62%", + "Model-produced by IDEA-Research/grounding-dino-tiny@abc123", ); }); + it("shows no confidence anywhere on the row, nor in the name it announces", () => { + // The number is an accept-or-reject aid and this row is past that decision. + // Asserted on the whole row rather than on the element that used to hold + // it, so a percentage reappearing anywhere in it turns this red. + render(mount(storeWith([predicted("m")]))); + + const row = screen.getByTestId("object-row-0"); + expect(row.textContent).not.toContain("%"); + expect(row.textContent).not.toContain("62"); + expect(screen.getByTestId("object-model-0").getAttribute("aria-label")).not.toContain("%"); + }); + it("puts nothing at all on a label a person drew", () => { // The common path stays exactly as it shipped — absence is the human case, // so a reviewer's thousandth row gains no badge and no noise. render(mount(storeWith([annotation("h", "vehicle", "bbox")]))); expect(screen.queryByTestId("object-model-0")).toBeNull(); - expect(screen.queryByTestId("object-confidence-0")).toBeNull(); }); - it("still marks the model's work when no score was recorded", () => { + it("marks the model's work the same whether or not a score was recorded", () => { + // `confidence` is optional on a model-produced annotation, and the mark no + // longer depends on it at all — which is what makes the mark's absence mean + // "a person drew this" rather than "the model did not score itself". render(mount(storeWith([predicted("m", { confidence: null })]))); expect(screen.getByTestId("object-model-0")).toBeDefined(); - // Absent reads as absent — never as a zero, and never as a low score. - expect(screen.queryByTestId("object-confidence-0")).toBeNull(); - expect(screen.getByTestId("object-model-0").textContent).not.toContain("0"); + expect(screen.getByTestId("object-row-0").textContent).not.toContain("0"); }); it("marks nothing for an imported label, which has no model to name", () => { @@ -550,6 +561,7 @@ describe("what a model produced, on the row a reviewer accepts it from", () => { ); expect(screen.queryByTestId("object-model-0")).toBeNull(); - expect(screen.getByTestId("object-confidence-1").textContent).toBe("41%"); + expect(screen.getByTestId("object-model-1")).toBeDefined(); + expect(screen.getByTestId("object-row-1").textContent).not.toContain("41"); }); }); From b82841e32caa250fb71863ba934b1cf703801745 Mon Sep 17 00:00:00 2001 From: Jesus Armando Anaya Date: Mon, 10 Aug 2026 07:35:04 -0700 Subject: [PATCH 3/3] test(annotator): the committed layer's element budget is 440, not 660 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The itemised count is what the scene test exists to hold, and a selected-only label moves it: two elements per annotation on a frame nobody has picked a shape on rather than three. The `` was the most expensive of the three — a stroke, a paint order and a translate each — so a legibility decision pays here as well. --- frontend/app/e2e/perf.spec.ts | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/frontend/app/e2e/perf.spec.ts b/frontend/app/e2e/perf.spec.ts index 7f580d57..ba9e6412 100644 --- a/frontend/app/e2e/perf.spec.ts +++ b/frontend/app/e2e/perf.spec.ts @@ -107,14 +107,19 @@ test("the benchmark scene is 220 annotations, and the committed layer is one gro // What 220 annotations cost in SVG elements, itemised. A deliberate rendering // change moves these numbers and should; an accidental one — a wrapper added // per shape, a label drawn twice — shows up here rather than in a frame time - // nobody ran. Three elements per annotation is also the number the zoom - // scenario below multiplies by. + // nobody ran. + // + // **No labels, because nothing is selected.** The class label is part of what + // selection looks like, so a frame nobody has picked a shape on draws two + // elements per annotation rather than three: 660 → 440, measured. That is a + // legibility decision that happens to pay here — the `` was the most + // expensive of the three, carrying a stroke, a paint order and a translate. expect(nodes).toEqual({ - all: 660, + all: 440, groups: BENCH_ANNOTATIONS, rects: 200, polygons: 20, - labels: BENCH_ANNOTATIONS, + labels: 0, }); });