From c667d6e6afef653536f9569c3696a238fd663f5a Mon Sep 17 00:00:00 2001 From: Armando Anaya Date: Tue, 4 Aug 2026 09:34:57 -0700 Subject: [PATCH 1/2] refactor(ui-core): the client reads capabilities instead of mirroring the kernel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `batchState.ts` carried `canSkip`/`canRestore`, and its own docstring said what they were: "a mirror of two rows of the kernel's `ASSET_PROGRESS_TRANSITIONS`". The mirror reproduced the progress dimension and dropped the batch-state one — `JobService.mark` runs `require_open_batch` first, deliberately, before it even reaches the no-op check — so on an `approved` or `completed` batch the gallery's bulk bar drew both buttons enabled over frames the kernel refuses without looking at their progress at all (audit finding F1). `allowed_actions` landed on the wire with #304, derived in `kernel/domain/capabilities.py` from those same tables. So this deletes the mirrors rather than fixing them: `canSkip`, `canRestore` and `isApprovable` are gone, and `data/capabilities.ts` is the one seam a screen asks. Three call sites moved: - The gallery's approve button, `isApprovable(state)` -> `declares(batch, approve)`. - The gallery's bulk bar, per-frame `allowed_actions` for the counts. On a batch that is closed to writing every list is empty by construction, so instead of two zeroed buttons the bar states the batch-level reason once and disables them with it. The *selection* stays: choosing a set of frames is the first half of making a correction batch out of them. - `BatchesScreen`'s `Lifecycle` chain, which was a fourth hand-mirror — correct today only because those four rows happen to be one-in one-out, and unable to express `promote`, which is not a transition at all. `hasJobs` survives, re-documented and renamed at its call site to `showsProgress`. It answers whether a draft's documented-zero counts are data, which is a display question; it used to double as the permission gate, which is how it came to be true for two states that refuse every write. Action names are constants (`BATCH_ACTION`, `JOB_ACTION`, `ASSET_ACTION`), so the wire's vocabulary has one spelling in the client and a free-string literal cannot be scattered past a rename. --- .../ui-core/src/data/capabilities.test.ts | 210 ++++++++++++++++++ frontend/ui-core/src/data/capabilities.ts | 149 +++++++++++++ frontend/ui-core/src/data/refusals.ts | 113 ++++++++++ frontend/ui-core/src/index.ts | 23 +- .../ui-core/src/screens/BatchesScreen.tsx | 41 +++- .../ui-core/src/screens/GalleryScreen.tsx | 138 ++++++++---- .../ui-core/src/screens/batchState.test.ts | 54 ----- frontend/ui-core/src/screens/batchState.ts | 54 +---- frontend/ui-core/src/screens/gallery.test.tsx | 167 +++++++++++++- frontend/ui-core/src/screens/queries.ts | 28 ++- 10 files changed, 817 insertions(+), 160 deletions(-) create mode 100644 frontend/ui-core/src/data/capabilities.test.ts create mode 100644 frontend/ui-core/src/data/capabilities.ts create mode 100644 frontend/ui-core/src/data/refusals.ts diff --git a/frontend/ui-core/src/data/capabilities.test.ts b/frontend/ui-core/src/data/capabilities.test.ts new file mode 100644 index 00000000..d28abe68 --- /dev/null +++ b/frontend/ui-core/src/data/capabilities.test.ts @@ -0,0 +1,210 @@ +/** + * The client's reading of `allowed_actions`, and the vocabulary beside it. + * + * ## What these can and cannot prove + * + * They cannot prove a declaration is *right* — that is + * `tests/kernel/test_capabilities.py`, which sweeps the full state matrices + * against the kernel's own tables and fails if either side moves alone. Nothing + * here re-derives a legality rule, because re-deriving one in the client is the + * defect the whole change exists to remove. + * + * What they prove is the reading: that "not declared" is answered `false` and not + * something friendlier, that an unloaded resource declares nothing, that the + * action names the screens import are the names the wire uses, and that the + * withheld sentences are keyed on the states that actually withhold. + * + * The state-matrix half — which control is offered in which state — is asserted + * where the controls are, in `screens/gallery.test.tsx` and + * `screens/batchLifecycle.test.tsx`, by handing the screen a resource whose + * declarations say one thing and checking the screen renders that thing. + */ + +import { describe, expect, it } from "vitest"; + +import { + ASSET_ACTION, + BATCH_ACTION, + declares, + declaring, + JOB_ACTION, + withheldBecause, + type AssetAction, + type BatchAction, + type JobAction, +} from "./capabilities"; +import { groupRefusals, refusalProse, type Refusal } from "./refusals"; +import { ApiError } from "./errors"; + +describe("reading a declaration", () => { + it("offers an action the resource declares", () => { + expect(declares({ allowed_actions: ["approve"] as BatchAction[] }, BATCH_ACTION.approve)).toBe( + true, + ); + }); + + it("withholds an action the resource does not declare", () => { + // The strong half of the contract, and the only one worth building on: a + // declaration can still be refused by something no pure function can see, + // but an *absent* declaration is a guaranteed refusal. + expect(declares({ allowed_actions: ["promote"] as BatchAction[] }, BATCH_ACTION.approve)).toBe( + false, + ); + }); + + it("treats a resource that has not loaded as declaring nothing", () => { + // Not defensive coding. A screen that offered an action because the answer + // had not arrived yet would be offering it on the strength of not knowing, + // which is the mistake the whole module exists to remove. Pending chrome is + // the caller's answer, not a hopeful control. + expect(declares(undefined, BATCH_ACTION.approve)).toBe(false); + expect(declares(null, BATCH_ACTION.approve)).toBe(false); + }); + + it("counts the resources declaring an action, keeping the resources themselves", () => { + // A bulk control needs the targets, not a boolean — and counting them in + // each caller is how two spellings of "which frames can be skipped" start to + // disagree. + const frames = [ + { id: "a", allowed_actions: ["annotate", "skip"] as AssetAction[] }, + { id: "b", allowed_actions: ["restore"] as AssetAction[] }, + { id: "c", allowed_actions: [] as AssetAction[] }, + { id: "d", allowed_actions: ["annotate", "skip"] as AssetAction[] }, + ]; + expect(declaring(frames, ASSET_ACTION.skip).map((one) => one.id)).toEqual(["a", "d"]); + expect(declaring(frames, ASSET_ACTION.restore).map((one) => one.id)).toEqual(["b"]); + expect(declaring(frames, ASSET_ACTION.accept)).toEqual([]); + }); + + it("declares nothing for a frame with an empty action list", () => { + // The shape a closed batch produces for every one of its frames: the kernel + // returns `[]` from `asset_actions` when the batch is not `in_annotation`, + // whatever the frame's own progress is. That is the batch-state dimension + // the old client-side mirror dropped. + const settled = { allowed_actions: [] as AssetAction[] }; + for (const action of Object.values(ASSET_ACTION)) { + expect(declares(settled, action)).toBe(false); + } + }); +}); + +describe("the action names the client imports", () => { + /** + * Each constant is its own wire value. + * + * `satisfies Record` already makes a *wrong* value fail to + * compile — the point of this test is the other direction, that the constant + * carries the wire's spelling rather than a camel-cased one. `submit_for_review` + * is the case that would break silently: the key is `submitForReview` and the + * value must not be. + */ + it("spells each action the way the wire does", () => { + expect(BATCH_ACTION.editMembership).toBe("edit_membership"); + expect(ASSET_ACTION.submitForReview).toBe("submit_for_review"); + expect(ASSET_ACTION.returnToAnnotator).toBe("return_to_annotator"); + expect(JOB_ACTION.complete).toBe("complete"); + }); + + it("names every action the wire has, so a new one cannot be reached by a literal", () => { + // The generated unions are the source; these lists are asserted against them + // by `tsc` through `satisfies`. What this adds is the count — a seventh batch + // action arriving on the wire with no constant here is a rename nobody can + // perform, because the screens would have to spell it as a free string. + expect(Object.values(BATCH_ACTION).sort()).toEqual( + ["approve", "complete", "delete", "edit_membership", "promote", "repin", "start"].sort(), + ); + expect(Object.values(JOB_ACTION).sort()).toEqual(["complete", "start"].sort()); + expect(Object.values(ASSET_ACTION).sort()).toEqual( + [ + "accept", + "annotate", + "restore", + "return_to_annotator", + "skip", + "submit_for_review", + ].sort(), + ); + }); +}); + +describe("why an action is not on offer", () => { + it("names the batch state, and names the remedy where there is one", () => { + expect(withheldBecause("draft")).toMatch(/approve/i); + expect(withheldBecause("approved")).toMatch(/start/i); + // The forward-only correction model, said out loud. A refusal that names the + // route onward is the difference between a dead end and a next step. + expect(withheldBecause("completed")).toMatch(/correction batch/i); + }); + + it("offers no sentence for the state where everything is available", () => { + // `in_annotation` withholds nothing at the batch level, so a sentence here + // would be a cause invented to fill a slot — and the caller renders the + // per-frame reason instead. + expect(withheldBecause("in_annotation")).toBeNull(); + }); + + it("offers no sentence for a state it has never heard of", () => { + expect(withheldBecause(undefined)).toBeNull(); + expect(withheldBecause("archived")).toBeNull(); + }); +}); + +describe("turning a refusal into a sentence", () => { + const refused = (code: string, message = "kernel wording"): ApiError => + new ApiError({ code, message }, 409); + + it("restates a code the vocabulary knows", () => { + expect(refusalProse(refused("BATCH_NOT_IN_ANNOTATION"))).toBe( + "This batch is not open for annotation any more.", + ); + }); + + it("keeps the server's own message for a code it does not know", () => { + // Falling through to "Something went wrong" would discard the one + // description the kernel actually wrote for a person. An entry exists to + // *improve* on a message, never to be the only one there is. + expect(refusalProse(refused("SOME_NEW_REFUSAL", "The widget is out of cheese."))).toBe( + "The widget is out of cheese.", + ); + }); + + it("falls back to the code only when there is no message at all", () => { + expect(refusalProse(refused("SOME_NEW_REFUSAL", ""))).toContain("SOME_NEW_REFUSAL"); + }); + + it("survives something that is not an ApiError", () => { + expect(refusalProse(new Error("boom"))).toBeTruthy(); + }); +}); + +describe("saying N refusals once", () => { + it("groups by code and counts", () => { + // A bulk move over forty frames that hits one rule hits it forty times, and + // forty identical sentences is not more information than one. This is what + // turns "0 moved, 40 refused" into something a person can act on. + const refusals: Refusal[] = Array.from({ length: 40 }, () => ({ + code: "BATCH_NOT_IN_ANNOTATION", + message: "kernel wording", + })); + const grouped = groupRefusals(refusals); + expect(grouped).toHaveLength(1); + expect(grouped[0]?.count).toBe(40); + expect(grouped[0]?.prose).toBe("This batch is not open for annotation any more."); + }); + + it("keeps distinct codes apart, in the order they first happened", () => { + const grouped = groupRefusals([ + { code: "ASSET_NOT_WRITABLE", message: "" }, + { code: "BATCH_NOT_IN_ANNOTATION", message: "" }, + { code: "ASSET_NOT_WRITABLE", message: "" }, + ]); + expect(grouped.map((one) => [one.code, one.count])).toEqual([ + ["ASSET_NOT_WRITABLE", 2], + ["BATCH_NOT_IN_ANNOTATION", 1], + ]); + }); + + it("says nothing when nothing was refused", () => { + expect(groupRefusals([])).toEqual([]); + }); +}); diff --git a/frontend/ui-core/src/data/capabilities.ts b/frontend/ui-core/src/data/capabilities.ts new file mode 100644 index 00000000..c3fbfdb4 --- /dev/null +++ b/frontend/ui-core/src/data/capabilities.ts @@ -0,0 +1,149 @@ +/** + * What the wire says a resource can be asked to do — the client's only source of + * legality. + * + * ## The rule this module exists to make cheap + * + * **The frontend never decides what is legal. It renders what the wire declares.** + * `allowed_actions` arrives on `BatchOut`, `JobOut` and `BatchAssetOut`, derived + * in `visionset.kernel.domain.capabilities` from the same tables and named sets + * the services consult. There is no second encoding anywhere, and this module is + * the seam that keeps it that way: everything a screen needs to ask about + * legality is one `declares(...)` call. + * + * ## What was here before, and why it had to go + * + * `batchState.ts` carried `canSkip`/`canRestore`, self-described as "a mirror of + * two rows of the kernel's `ASSET_PROGRESS_TRANSITIONS`" — and the mirror had + * already drifted, by reproducing the *progress* dimension and dropping the + * *batch-state* one. `JobService.mark` checks the batch first, deliberately, + * before it even reaches the no-op short-circuit. So the bulk bar offered + * `Mark skipped` on a completed batch, the kernel refused every frame, and the + * user was told "0 moved, N refused" with the reason destroyed on the way up. + * `isApprovable` and `hasJobs`-as-permission were the same shape with less blast + * radius. All three are gone; this is what replaced them. + * + * A drifted mirror is not a bug that gets fixed once — it is a bug that returns + * every time the kernel grows a precondition the client did not hear about. The + * declaration cannot drift, because the kernel computes it from its own tables. + * + * ## Declarations are not promises + * + * A declared action is one the resource's *state* does not refuse. It can still + * fail on something no pure function of that state can see: `approve` on a + * project with no schema, `complete` while a job is outstanding. The converse is + * the strong half and the one worth building on — **an action that is not + * declared will be refused** — which is what makes "don't offer it" correct + * rather than merely tidy. + * + * So a call site still renders the refusal. `declares` decides what to *offer*; + * it never decides that a mutation cannot fail. + */ + +import type { components } from "../generated/api.js"; + +/** What can be asked of a batch. The order is the kernel's declaration order. */ +export type BatchAction = components["schemas"]["BatchAction"]; +/** What can be asked of an annotation job. */ +export type JobAction = components["schemas"]["JobAction"]; +/** What can be asked of one asset inside a batch. */ +export type AssetAction = components["schemas"]["AssetAction"]; + +/** + * The action names, once. + * + * Free-string literals compile — the generated unions are string unions, so + * `declares(batch, "aprove")` fails but `declares(batch, "approve")` scattered + * across nine files is a rename nobody can perform. These constants are what a + * screen imports, so the wire's vocabulary has exactly one spelling in the + * client and `tsc` finds every use of it. + */ +export const BATCH_ACTION = { + approve: "approve", + start: "start", + complete: "complete", + repin: "repin", + promote: "promote", + editMembership: "edit_membership", + delete: "delete", +} as const satisfies Record; + +export const JOB_ACTION = { + start: "start", + complete: "complete", +} as const satisfies Record; + +export const ASSET_ACTION = { + annotate: "annotate", + skip: "skip", + restore: "restore", + submitForReview: "submit_for_review", + accept: "accept", + returnToAnnotator: "return_to_annotator", +} as const satisfies Record; + +/** Anything the wire declares actions for. */ +export interface Capable { + readonly allowed_actions: readonly A[]; +} + +/** + * Does this resource declare that action right now? + * + * `undefined` answers `false`, and that is the honest reading rather than a + * convenience: a resource that has not loaded has declared nothing, and offering + * an action on the strength of not knowing is the mistake this module exists to + * remove. Screens render the control's absence as pending chrome, not as a + * refusal. + */ +export function declares( + resource: Capable | undefined | null, + action: A, +): boolean { + return resource != null && resource.allowed_actions.includes(action); +} + +/** + * How many of these resources declare the action — the count a bulk control puts + * on its own button. + * + * Separate from `declares` because a bulk bar needs the *targets*, not a boolean, + * and counting them in each caller is how two spellings of "which frames can be + * skipped" start disagreeing. + */ +export function declaring>( + resources: readonly T[], + action: A, +): readonly T[] { + return resources.filter((one) => one.allowed_actions.includes(action)); +} + +/** + * Why an action is not on offer, in the words a person can act on. + * + * The `ui-capabilities` rule is **disabled-with-reason over hidden** for an + * action that is meaningful on this screen but not available in this state — and + * a disabled control with no reason is the same dead end as a silent refusal. + * The batch's state is the reason in every case here, because it is the + * dimension the old mirrors dropped. + * + * `null` means "no sentence to offer": either the state permits it after all, or + * the state is unknown. A caller renders nothing rather than inventing a cause. + * + * The forward-only correction model is what the `completed` sentences say out + * loud. A completed batch is immutable as a workflow unit; the legitimate intent + * behind wanting to edit one is served by a correction batch, and naming that is + * the difference between a refusal and a route onward. + */ +export function withheldBecause(state: string | undefined | null): string | null { + switch (state) { + case "draft": + return "This batch has not been approved yet — approve it to cut its jobs."; + case "approved": + return "This batch has not been started yet — start it to begin annotating."; + case "completed": + return "This batch is completed — corrections happen in a correction batch."; + default: + return null; + } +} diff --git a/frontend/ui-core/src/data/refusals.ts b/frontend/ui-core/src/data/refusals.ts new file mode 100644 index 00000000..1d5c1edc --- /dev/null +++ b/frontend/ui-core/src/data/refusals.ts @@ -0,0 +1,113 @@ +/** + * One vocabulary for every refusal the kernel can hand a person. + * + * ## Why one map and not one per screen + * + * A refusal reached the user in three different shapes depending on which screen + * they were on: the full `{code}: {message}` in six places, a bare + * `BATCH_NOT_IN_ANNOTATION` badge in three, and a humanized sentence in two — + * each map written where it was needed and none of them aware of the others. + * The bare-code sites are the ones worth naming: a kernel identifier in front of + * a user is not an error message, and #292 removed that class of rendering from + * one screen without anywhere to put the rule. + * + * This is that place. A code has one sentence, product-wide, and a screen that + * wants a remedy adds it *beside* the sentence rather than instead of it. + * + * ## The fall-through is deliberate + * + * A code with no entry keeps the server's own `message`, which is written for a + * person and is usually good. Falling through to "Something went wrong" would + * discard the one description the kernel actually wrote. The code is appended + * only when there is no message at all — at that point the identifier is the + * only fact there is, and it is what a bug report should quote. + * + * Entries are for codes whose server message is *correct but unhelpful in + * context*: the kernel says what rule was broken, and the product says what the + * person can do about it. + */ + +import { asApiError } from "./errors.js"; + +/** + * The codes worth restating, and what they say instead. + * + * Keyed on the kernel's `code`, which is a stable public contract by + * construction — `server/errors.py` holds them as literals precisely so a Python + * rename cannot silently break a client. + */ +export const REFUSAL_PROSE: Record = { + // The batch-state family. These three are the ones a capability declaration + // now pre-empts, so reaching one means the batch moved under the press — + // another tab, another person — rather than a control that should not have + // been offered. + BATCH_NOT_IN_ANNOTATION: "This batch is not open for annotation any more.", + BATCH_NOT_EDITABLE: "This batch can no longer be edited — only a draft can be.", + BATCH_NOT_COMPLETE: "Some of this batch's jobs are still unfinished.", + BATCH_IMMUTABLE: "This batch is completed, and completed batches are kept.", + INVALID_TRANSITION: "This has already moved on — reload to see where it is now.", + + // The per-asset family. + ASSET_NOT_WRITABLE: "This frame's labeling is settled — its labels cannot be changed here.", + JOB_NOT_COMPLETE: "Some frames still need annotating or skipping.", + ASSET_NOT_IN_JOB: "This frame is not part of this job.", + + // The schema family. `SCHEMA_NOT_FOUND` is the one with a remedy the screen + // supplies (a link to the schema tab), so the sentence sets that up. + SCHEMA_NOT_FOUND: "This project has no label schema yet — one is needed before approving.", + DESTRUCTIVE_SCHEMA_CHANGE: "This change removes part of the contract already in use.", + SCHEMA_CHANGE_WOULD_ORPHAN: "Annotations already exist under a class this change removes.", + + // Infrastructure the user can act on. + WORKSPACE_BUSY: "The workspace is busy — try again in a moment.", + NOT_A_WORKSPACE: "This server is not pointed at a workspace.", +}; + +/** + * What to show a person for one failed request. + * + * Takes the raw `unknown` a mutation's `error` carries rather than an `ApiError`, + * because every call site has the former and converting at each of them is how a + * site ends up rendering `[object Object]`. + */ +export function refusalProse(cause: unknown): string { + const error = asApiError(cause); + const known = REFUSAL_PROSE[error.code]; + if (known !== undefined) return known; + if (error.message.length > 0) return error.message; + return `The server refused this (${error.code}).`; +} + +/** One refusal, kept with the thing it happened to. */ +export interface Refusal { + readonly code: string; + readonly message: string; +} + +/** + * N refusals, said once each, with how many times each happened. + * + * A bulk move over forty frames that hits one rule hits it forty times, and + * forty identical sentences is not more information than one. Grouping by code + * is what turns "0 moved, 40 refused" into a sentence somebody can act on. + * + * Insertion-ordered: the first refusal's code leads, which for a bulk move over + * a homogeneous selection is the only one there is. + */ +export function groupRefusals( + refusals: readonly Refusal[], +): readonly { readonly code: string; readonly prose: string; readonly count: number }[] { + const byCode = new Map(); + for (const refusal of refusals) { + const seen = byCode.get(refusal.code); + if (seen !== undefined) { + seen.count += 1; + continue; + } + byCode.set(refusal.code, { + prose: REFUSAL_PROSE[refusal.code] ?? (refusal.message.length > 0 ? refusal.message : refusal.code), + count: 1, + }); + } + return [...byCode].map(([code, { prose, count }]) => ({ code, prose, count })); +} diff --git a/frontend/ui-core/src/index.ts b/frontend/ui-core/src/index.ts index 01f99792..9d0dec61 100644 --- a/frontend/ui-core/src/index.ts +++ b/frontend/ui-core/src/index.ts @@ -193,12 +193,33 @@ export { GalleryScreen, type GalleryScreenProps } from "./screens/GalleryScreen. export { ApproveDialog, BatchProgressBar } from "./screens/BatchLifecycle.js"; export { batchStateLabel, - isApprovable, segmentCounts, segmentOf, relativeAge, type Segment, } from "./screens/batchState.js"; + +// What the wire declares a resource can be asked to do — the client's only +// source of legality, and the replacement for the `canX(state)` helpers that +// hand-mirrored the kernel's tables and drifted. See `data/capabilities.ts`. +export { + ASSET_ACTION, + BATCH_ACTION, + JOB_ACTION, + declares, + declaring, + withheldBecause, + type AssetAction, + type BatchAction, + type Capable, + type JobAction, +} from "./data/capabilities.js"; +export { + groupRefusals, + refusalProse, + REFUSAL_PROSE, + type Refusal, +} from "./data/refusals.js"; export { DatasetScreen, type DatasetScreenProps } from "./screens/DatasetScreen.js"; export { saveBlob } from "./screens/download.js"; export { AssetThumbnail, type AssetThumbnailProps } from "./screens/AssetThumbnail.js"; diff --git a/frontend/ui-core/src/screens/BatchesScreen.tsx b/frontend/ui-core/src/screens/BatchesScreen.tsx index 278e1241..77f2d8af 100644 --- a/frontend/ui-core/src/screens/BatchesScreen.tsx +++ b/frontend/ui-core/src/screens/BatchesScreen.tsx @@ -34,7 +34,8 @@ import { ArrowUpFromLine, Layers, Play } from "lucide-react"; import { useState, type JSX } from "react"; import { Async } from "../data/Async"; -import { asApiError } from "../data/errors"; +import { BATCH_ACTION, declares } from "../data/capabilities"; +import { refusalProse } from "../data/refusals"; import { Badge } from "../primitives/Badge"; import { Button } from "../primitives/Button"; import { FieldError } from "../primitives/Input"; @@ -152,7 +153,22 @@ export function BatchesScreen({ ); } -/** One action per state, and nothing that would be refused. */ +/** + * One action per state, and nothing that would be refused. + * + * **Which action, from the batch's own `allowed_actions`.** The chain used to + * read `batch.state` and decide for itself — a fourth hand-mirror of + * `BATCH_TRANSITIONS`, correct today only because those four rows happen to be + * one-in one-out. The kernel derives the declaration from the same table, plus + * the named sets a table row cannot express (`PROMOTABLE_STATES` for promote, + * which is not a transition at all), so asking it is both shorter and the only + * version that cannot drift. + * + * `promote` is checked before the transitions because it is the one action here + * that leaves the batch where it is — a completed batch declares `promote` and + * nothing else, and the ordering says so rather than relying on the states being + * mutually exclusive. + */ function Lifecycle({ batch, onApprove, @@ -163,7 +179,7 @@ function Lifecycle({ const start = useBatchTransition(batch.id, "start"); const promote = usePromoteBatch(batch.projectId ?? ""); - if (batch.state === "completed") { + if (declares(batch, BATCH_ACTION.promote)) { // The last move, and the only one that is not a state transition: promotion // adds the batch's assets to the trunk. Idempotent — a **union** against // current membership, with no log entry when nothing changed — so pressing it @@ -183,13 +199,13 @@ function Lifecycle({ {promote.isError && ( - {asApiError(promote.error).code} + {refusalProse(promote.error)} )} ); } - if (batch.state === "draft") { + if (declares(batch, BATCH_ACTION.approve)) { return ( ); } - if (batch.state === "approved") { + if (declares(batch, BATCH_ACTION.start)) { return (
- {start.isError && {asApiError(start.error).code}} + {start.isError && ( + + {refusalProse(start.error)} + + )}
); } - if (batch.state === "in_annotation") { + if (declares(batch, BATCH_ACTION.complete)) { // Completion is derived at two levels and neither is implicit, so closing a // batch means closing its jobs first — which nothing in the browser did // outside the annotator. `CompleteBatchButton` owns the chain and the reason // (#301); the gallery header renders the same control. return ; } - // Every state is answered above; `approved` and `in_annotation` are the two - // middle rows and `draft`/`completed` the ends. + // Nothing declared, so nothing offered. Reached while the batch is loading (no + // declaration yet) and, in principle, for a state a newer server has that this + // build does not — which is the right answer to both. return null; } diff --git a/frontend/ui-core/src/screens/GalleryScreen.tsx b/frontend/ui-core/src/screens/GalleryScreen.tsx index 1a5026df..475b19a0 100644 --- a/frontend/ui-core/src/screens/GalleryScreen.tsx +++ b/frontend/ui-core/src/screens/GalleryScreen.tsx @@ -45,7 +45,6 @@ import { useWindowVirtualizer } from "@tanstack/react-virtual"; import { Check, PlayCircle, SkipForward, Undo2, X } from "lucide-react"; import { Async } from "../data/Async"; -import { asApiError } from "../data/errors"; import { readStep, writePref } from "../data/prefs"; import { useAssetAnnotations } from "../annotator/jobQueries"; import { Badge } from "../primitives/Badge"; @@ -54,15 +53,21 @@ import { AssetThumbnail } from "./AssetThumbnail"; import { BackLink } from "../patterns/BackLink"; import { parentLabel } from "../patterns/parentLabel"; import { ApproveDialog, BatchProgressBar, CompleteBatchButton } from "./BatchLifecycle"; +import { + ASSET_ACTION, + BATCH_ACTION, + declares, + declaring, + withheldBecause, + type AssetAction, +} from "../data/capabilities"; +import { groupRefusals, refusalProse } from "../data/refusals"; import { BATCH_STATE_VARIANT, batchStateLabel, - canRestore, - canSkip, earliestArrival, inSegment, hasJobs, - isApprovable, mayHaveAnnotations, progressDot, progressLabel, @@ -270,10 +275,21 @@ export function GalleryScreen({ [shown], ); - // Before approval there are no jobs, so there is no progress to describe, no - // states to filter between and nothing a selection could act on. Everything - // downstream of this is hidden rather than rendered as zero — see `hasJobs`. - const working = hasJobs(batch.data?.state); + // Before approval there are no jobs, so there is no progress to describe and + // no states to filter between. Everything downstream of this is hidden rather + // than rendered as zero — see `hasJobs`. + // + // **This is a display question and nothing else.** It used to double as the + // permission gate — `working` was true for `approved`, `in_annotation` and + // `completed` alike, so the bulk bar was live in two states where the kernel + // refuses every write. What the bar may *do* now comes from each frame's own + // `allowed_actions`; what the screen may *show* is still this. + // + // Selection stays on wherever there is progress to see, including a completed + // batch: choosing a set of frames is the first half of making a correction + // batch out of them, and the bar states why its moves are unavailable rather + // than the screen refusing to let anything be picked. + const showsProgress = hasJobs(batch.data?.state); const counts = batch.data === undefined ? { all: total, unannotated: total, review: 0, done: 0 } : segmentCounts(batch.data.progress); @@ -285,7 +301,7 @@ export function GalleryScreen({ setApproving(true)} {...(onOpenAsset === undefined ? {} @@ -310,10 +326,10 @@ export function GalleryScreen({ onSegment={setSegment} density={density} onDensity={chooseDensity} - showSegments={working} + showSegments={showsProgress} /> - {working && ( + {showsProgress && ( toggle(row.index * columns + offset, modifiers), @@ -407,9 +423,10 @@ export function GalleryScreen({ )} - {working && ( + {showsProgress && ( setSelected(new Set())} @@ -442,14 +459,14 @@ export function GalleryScreen({ function BatchHeader({ batch, assets, - working, + showsProgress, onApprove, onStartAnnotating, }: { readonly batch: Batch | undefined; readonly assets: readonly BatchAsset[]; /** False for a draft, whose counts are documented zeros rather than data. */ - readonly working: boolean; + readonly showsProgress: boolean; readonly onApprove: () => void; readonly onStartAnnotating?: () => void; }): JSX.Element { @@ -513,7 +530,7 @@ function BatchHeader({ approval carries a partition, pins the schema and cuts the jobs, and has no route back. See `BatchLifecycle`. */} - {isApprovable(batch?.state) && ( + {declares(batch, BATCH_ACTION.approve) && ( @@ -558,7 +575,7 @@ function BatchHeader({ been created yet, and it made the screen look broken. The frame count is already in the facts line above, which is the honest number here. */} - {batch !== undefined && working && ( + {batch !== undefined && showsProgress && ( )} @@ -925,8 +942,7 @@ function ProgressDot({ asset }: { readonly asset: BatchAsset }): JSX.Element { * `Mark skipped` shipped alone, and `skipped → unannotated` — which the kernel * calls "the decision was reversed while the job is open" — had **no spelling * anywhere in the browser**. A mis-aimed shift-click over forty frames was - * unrecoverable without opening each one in the annotator. `Restore` is that edge, - * and `canRestore` owns why it is `skipped` alone. + * unrecoverable without opening each one in the annotator. `Restore` is that edge. * * `Delete frames` is still absent and still a scope fact: batch membership editing * is not on the wire (#281), and after approval the kernel refuses it outright — @@ -943,22 +959,41 @@ function ProgressDot({ asset }: { readonly asset: BatchAsset }): JSX.Element { * screen agreeing it had worked while the person watched it not work. Selection * was never the broken part. * - * Filtering by `canSkip`/`canRestore` fixes both halves at once: no request is - * sent that cannot change anything, so `moved` means moved; and the counts on the - * buttons say what the selection *is* before anything is pressed. A press that - * lands flips which button is enabled, which is the confirmation the bar used to - * be unable to give. + * Counting each button's targets from `allowed_actions` fixes both halves at + * once: no request is sent that cannot change anything, so `moved` means moved; + * and the counts on the buttons say what the selection *is* before anything is + * pressed. A press that lands flips which button is enabled, which is the + * confirmation the bar used to be unable to give. + * + * ## Where the counts come from, and why that changed + * + * They came from `canSkip`/`canRestore` — client-side mirrors of two rows of + * `ASSET_PROGRESS_TRANSITIONS` that reproduced the *progress* dimension and + * dropped the *batch-state* one. `JobService.mark` checks the batch first, so on + * an `approved` or `completed` batch every button here was enabled, every request + * was refused, and the bar reported "0 moved, N refused" with the reason gone. + * + * Now each target is a frame whose own `allowed_actions` names the move, which + * the kernel derived from both dimensions. On a batch that is not open the lists + * are empty by construction — so instead of two zeroed buttons the bar states the + * batch-level reason once, and the buttons are **disabled with it**. The + * selection survives, because choosing a set of frames is the first half of + * making a correction batch out of them. * * The bar still reports a **partial** outcome, because the mutation is N requests - * and forty of fifty succeeding is a real state — see `useBulkSetProgress`. + * and forty of fifty succeeding is a real state — and it now reports *why* the + * rest were refused, grouped by code. See `useBulkSetProgress`. */ function BulkBar({ batchId, + batchState, selected, assets, onClear, }: { readonly batchId: string; + /** The batch's own state — the reason a move is unavailable, when it is. */ + readonly batchState: string | undefined; readonly selected: ReadonlySet; readonly assets: readonly BatchAsset[]; readonly onClear: () => void; @@ -967,13 +1002,24 @@ function BulkBar({ // A job id is null exactly while the batch is a draft, and a draft renders no // selection at all — so this filter is about the *frames*, not about the state. const chosen = assets.filter((one) => selected.has(one.id) && one.job_id !== null); - const targets = (move: (progress: BatchAsset["progress"]) => boolean) => - chosen - .filter((one) => move(one.progress)) - .map((one) => ({ jobId: one.job_id ?? "", assetId: one.id })); + const targets = (action: AssetAction) => + declaring(chosen, action).map((one) => ({ jobId: one.job_id ?? "", assetId: one.id })); + + const skippable = targets(ASSET_ACTION.skip); + const restorable = targets(ASSET_ACTION.restore); - const skippable = targets(canSkip); - const restorable = targets(canRestore); + /** + * Why nothing here can be pressed, when nothing can. + * + * Two different silences used to look identical: a batch that is closed to + * writing (every frame declares nothing) and a selection of `accepted` frames + * in an open batch (those frames declare nothing). The first is about the + * batch and has a remedy; the second is about the frames and does not. Asking + * whether *any* frame in the whole listing declares a move is what tells them + * apart — if none does, it is the batch. + */ + const batchIsOpen = assets.some((one) => one.allowed_actions.length > 0); + const withheld = batchIsOpen ? null : withheldBecause(batchState); if (selected.size === 0) return null; @@ -993,6 +1039,7 @@ function BulkBar({ size="sm" data-testid="bulk-skip" disabled={skippable.length === 0 || bulk.isPending} + {...(withheld === null ? {} : { title: withheld })} onClick={() => bulk.mutate({ targets: skippable, progress: "skipped" })} > { readonly allowed_actions: readonly A[]; } +/* + * A note on the `NoInfer` below, because it looks like decoration and is not. + * + * Without it, `A` is inferred from *both* parameters, and the action argument is + * a string literal — so `declaring(frames, ASSET_ACTION.skip)` binds `A` to + * `"skip"`, which then requires the frames to be `Capable<"skip">` and rejects a + * perfectly good `BatchAsset[]`. Pinning inference to the resources is what makes + * the resource the subject of the question, which is what it is: the action is + * the thing being asked *about*, and it is already constrained to the wire's + * union by the constants above. + */ + /** * Does this resource declare that action right now? * @@ -98,7 +110,7 @@ export interface Capable { */ export function declares( resource: Capable | undefined | null, - action: A, + action: NoInfer, ): boolean { return resource != null && resource.allowed_actions.includes(action); } @@ -113,7 +125,7 @@ export function declares( */ export function declaring>( resources: readonly T[], - action: A, + action: NoInfer, ): readonly T[] { return resources.filter((one) => one.allowed_actions.includes(action)); }