From abf242629b96890a1fc8d7bfc56094017815cffd Mon Sep 17 00:00:00 2001 From: Jesus Armando Anaya Date: Sat, 8 Aug 2026 00:00:40 -0700 Subject: [PATCH] fix(annotator): the read-only mode closes its add-a-class doors, and a skipped frame in a closed batch still explains itself (#423) --- frontend/app/cycle/cycle.spec.ts | 41 ++++++++++++++ frontend/app/e2e/annotate.spec.ts | 53 +++++++++++++++++++ .../ui-core/src/annotator/AnnotationPage.tsx | 17 ++++-- .../ui-core/src/annotator/ClassRegion.tsx | 14 ++++- .../src/annotator/classRegion.test.tsx | 15 ++++++ 5 files changed, 133 insertions(+), 7 deletions(-) diff --git a/frontend/app/cycle/cycle.spec.ts b/frontend/app/cycle/cycle.spec.ts index b1d5b950..82abfccc 100644 --- a/frontend/app/cycle/cycle.spec.ts +++ b/frontend/app/cycle/cycle.spec.ts @@ -591,6 +591,47 @@ test("the whole cycle, from opening the app to a downloaded export", async ({ pa }); }); + await test.step("the completed batch reopens as a viewer, from the tile and the address bar", async () => { + /* + * The step the walk never took (#423): every earlier read-only claim ran + * against stubs, so nothing proved that a *real* completed batch — whose + * asset declarations the kernel computes — reopens as a viewer. The two + * entries below are the two roads in: the gallery tile, and a reload of the + * job URL with no cache to inherit. + */ + await page.getByTestId("open-batch-cycle-batch").click(); + await expect(page.getByTestId("gallery")).toBeVisible(); + const tiles = page.getByTestId(/^tile-/); + await expect(tiles).toHaveCount(3); + const assetId = (await tiles.first().getAttribute("data-testid"))!.replace("tile-", ""); + await tiles.first().getByTestId(`open-${assetId}`).click(); + await expect(page.getByTestId("annotation-page")).toBeVisible(); + + await expect(page.getByTestId("readonly-banner")).toContainText(/viewing only/i); + await expect(page.getByTestId("banner-create-correction")).toBeVisible(); + await expect(page.getByTestId("tool-palette")).toHaveCount(0); + await expect(page.getByTestId("class-add")).toBeDisabled(); + + // A full draw gesture writes nothing and dirties nothing. + const canvas = page.getByTestId("annotator-canvas"); + const box = (await canvas.boundingBox())!; + await page.getByTestId("annotator-root").focus(); + await page.keyboard.press("1"); + await page.mouse.move(box.x + box.width * 0.3, box.y + box.height * 0.3); + await page.mouse.down(); + await page.mouse.move(box.x + box.width * 0.6, box.y + box.height * 0.6, { steps: 8 }); + await page.mouse.up(); + await expect(page.getByTestId("save-state")).toContainText("Saved"); + + // The address bar is the second entry: a cold load of the same job answers + // the same mode, with nothing inherited from the session above. + await page.reload(); + await expect(page.getByTestId("readonly-banner")).toContainText(/viewing only/i); + + await openProject(page, PROJECT, "batches"); + await expect(page.getByTestId("batches-table")).toBeVisible(); + }); + await test.step("promote the completed batch into the trunk", async () => { // The trunk carries assets only, and promotion is a **union** against current // membership — idempotent, with no log entry when nothing changed. diff --git a/frontend/app/e2e/annotate.spec.ts b/frontend/app/e2e/annotate.spec.ts index b41371e5..3a343782 100644 --- a/frontend/app/e2e/annotate.spec.ts +++ b/frontend/app/e2e/annotate.spec.ts @@ -1284,6 +1284,59 @@ test("a completed batch's canvas cannot be drawn on, however hard it is asked", expect(sent.filter((r) => r.method() === "POST" && r.url().includes("/annotations"))).toEqual([]); }); +/** + * The doors #422 left open (#423). The classes region renders in the read-only + * mode — which classes exist stays true there — but its create paths are writes: + * each one opens the add-a-class dialog, and the dialog publishes a real schema + * version from a page that has just said it is a viewer. + */ +test("a completed batch's viewer leaves no door into the add-a-class dialog", async ({ page }) => { + const sent: Request[] = []; + await openJob(page, sent, undefined, { batch: "completed", job: "completed" }); + await expect(page.getByTestId("readonly-banner")).toBeVisible(); + + // The `+` stays on screen — hiding it would be a control that comes and goes — + // but it is a refusal, and it says why. + const add = page.getByTestId("class-add"); + await expect(add).toBeDisabled(); + await expect(add).toHaveAttribute("title", /completed/i); + + // Nothing matches what was typed: the `Create class` row must not appear, and + // the Enter fallthrough that would have created it must be dead too. + await page.getByTestId("class-filter").fill("a-class-nobody-declared"); + await expect(page.getByTestId("class-create")).toHaveCount(0); + await page.getByTestId("class-filter").press("Enter"); + await expect(page.getByTestId("add-class-dialog")).toHaveCount(0); +}); + +/** + * The one frame where the read-only mode said nothing at all (#423): the banner + * rendered only while the frame was not skipped — a guard older than the + * correction link that now lives inside it — so a skipped frame in a completed + * batch showed no "viewing only", no route onward, and a skipped notice whose + * "Un-skip it" names a move the wire withholds there. + */ +test("a skipped frame in a completed batch still says viewing only, and names the correction path", async ({ + page, +}) => { + const sent: Request[] = []; + await openJob(page, sent, progressStore({ "asset-1": "skipped", "asset-2": "annotated" }), { + batch: "completed", + job: "completed", + }); + + const banner = page.getByTestId("readonly-banner"); + await expect(banner).toBeVisible(); + await expect(banner).toContainText(/viewing only/i); + await expect(page.getByTestId("banner-create-correction")).toBeVisible(); + + // The notice's remedy is not available here, so the banner is the one surface + // that speaks — two banners saying different things about one frame is how a + // person learns to trust neither. + await expect(page.getByTestId("skipped-notice")).toHaveCount(0); + await expect(page.getByTestId("unskip")).toBeDisabled(); +}); + /** * The founder's decision in #123, and the half no unit test can reach: **the * clipboard survives moving to the next frame.** diff --git a/frontend/ui-core/src/annotator/AnnotationPage.tsx b/frontend/ui-core/src/annotator/AnnotationPage.tsx index f18c7bac..60de28c3 100644 --- a/frontend/ui-core/src/annotator/AnnotationPage.tsx +++ b/frontend/ui-core/src/annotator/AnnotationPage.tsx @@ -1763,11 +1763,15 @@ function Workspace({ fail" is what shipped, and what it looked like from the other side was a working editor that lost your work. - Not rendered for a skipped frame — the notice below is the same fact with - the remedy attached, and two banners saying one thing is how a person - learns to ignore both. + Not rendered for a skipped frame **in an open batch** — the notice below + is the same fact with the remedy attached, and two banners saying one + thing is how a person learns to ignore both. In a closed batch the yield + runs the other way (#423): the notice's Un-skip is a move the wire + withholds there, so this banner — and the correction route it carries — + is the one surface that can still say something actionable. The old + guard predated the correction link and hid it on exactly that frame. */} - {readOnly && !skipped && ( + {readOnly && (!skipped || closedBecause !== null) && (

)} - {skipped && ( + {/* Only while the batch is open: "Un-skip it" is this notice's whole + remedy, and in a closed batch the wire withholds that move — the + read-only banner above speaks for that frame instead (#423). */} + {skipped && closedBecause === null && (

0 ? null : filter.trim(); + onAddClass === undefined || refusal !== undefined || query === "" || shown.length > 0 + ? null + : filter.trim(); /** * Enter takes the first match, which is the typeahead the top-bar field had. @@ -167,7 +174,10 @@ export function ClassRegion({ className="size-6" aria-label="Add a class" data-testid="class-add" - disabled={onAddClass === undefined} + // Refusing closes this door too, with the rows' own sentence on it — + // hiding it would be a control that comes and goes between frames. + disabled={onAddClass === undefined || refusal !== undefined} + {...(refusal === undefined ? {} : { title: refusal })} onClick={() => onAddClass?.("")} > diff --git a/frontend/ui-core/src/annotator/classRegion.test.tsx b/frontend/ui-core/src/annotator/classRegion.test.tsx index bceece00..b31112ba 100644 --- a/frontend/ui-core/src/annotator/classRegion.test.tsx +++ b/frontend/ui-core/src/annotator/classRegion.test.tsx @@ -151,4 +151,19 @@ describe("the region's refusals", () => { expect(screen.getByTestId("classes-empty")).toBeDefined(); expect(screen.queryByTestId("class-list")).toBeNull(); }); + + it("closes every door into the add-a-class dialog while it is refusing", async () => { + // The create paths are writes the same way arming is: a viewer that can + // publish a schema version is a read-only mode with a hole in it (#423). + const onAddClass = vi.fn(); + render(mount(3, { refusal: "This batch is completed.", onAddClass })); + + const add = screen.getByTestId("class-add"); + expect(add.hasAttribute("disabled")).toBe(true); + expect(add.getAttribute("title")).toBe("This batch is completed."); + + await userEvent.type(screen.getByTestId("class-filter"), "nothing-declared{Enter}"); + expect(screen.queryByTestId("class-create")).toBeNull(); + expect(onAddClass).not.toHaveBeenCalled(); + }); });