diff --git a/apps/dev/src/integration/translator/bootTestPayload.ts b/apps/dev/src/integration/translator/bootTestPayload.ts index e1bf7b08b..cd5ef330f 100644 --- a/apps/dev/src/integration/translator/bootTestPayload.ts +++ b/apps/dev/src/integration/translator/bootTestPayload.ts @@ -88,6 +88,7 @@ export async function bootTestPayload(opts?: { onTranslate?: (targetLng: string) => Promise | void; runner?: TaskRunnerProvider; lifecycle?: TranslationLifecycleCallbacks; + failedWindowMs?: number; /** Turn on container-granular rich-text translation, and declare the provider able to keep marks. */ inlineMarks?: boolean; /** @@ -164,6 +165,7 @@ export async function bootTestPayload(opts?: { levels: opts?.fieldSurface ? [documentLevel(), fieldLevel()] : [documentLevel()], provenance: true, ...(opts?.lifecycle ? { lifecycle: opts.lifecycle } : {}), + ...(opts?.failedWindowMs === undefined ? {} : { failedWindowMs: opts.failedWindowMs }), ...(opts?.inlineMarks ? { experimental: { inlineMarks: true } } : {}), }), ], diff --git a/apps/dev/src/integration/translator/locale-workflow-failure.int.test.ts b/apps/dev/src/integration/translator/locale-workflow-failure.int.test.ts index 81477fed5..d62fa32b6 100644 --- a/apps/dev/src/integration/translator/locale-workflow-failure.int.test.ts +++ b/apps/dev/src/integration/translator/locale-workflow-failure.int.test.ts @@ -89,11 +89,11 @@ describe("when one locale's provider fails", () => { []; expect( rows.map((row) => [row.input.target_lng, row.status]).sort(), - "each locale should report its own outcome" + "a locale is finished only on its own evidence; one whose attempt failed is still owed a retry, so it reads as pending rather than as a failure to act on" ).toEqual([ ["de", "completed"], ["es", "pending"], - ["fr", "failed"], + ["fr", "pending"], ]); // Payload backs a failed job off exponentially; only the manual-run endpoint lifts that, so the diff --git a/apps/dev/src/integration/translator/panel-shows-what-is-happening.int.test.ts b/apps/dev/src/integration/translator/panel-shows-what-is-happening.int.test.ts new file mode 100644 index 000000000..0feba5853 --- /dev/null +++ b/apps/dev/src/integration/translator/panel-shows-what-is-happening.int.test.ts @@ -0,0 +1,182 @@ +import { createPayloadJobsRunner } from "@focus-reactive/payload-plugin-translator"; +import { afterAll, beforeAll, describe, expect, it } from "vitest"; + +import { bootTestPayload } from "./bootTestPayload"; +import type { TestPayload } from "./bootTestPayload"; +import { callEndpoint } from "./callEndpoint"; + +let ctx: TestPayload; + +const JOBS = "payload-jobs" as "docs"; +const WORKFLOW = "translate_document_locales"; +const QUEUE = "translations"; +const WINDOW_DAYS = 3; +const WINDOW_MS = WINDOW_DAYS * 24 * 60 * 60 * 1000; + +type Entry = { id: string; status: string; collection_id: string; target_lng: string }; + +const create = async (title: string): Promise => { + const made = await ctx.payload.create({ + collection: "docs", + locale: "en", + data: { _status: "published", title, note: "A note" }, + }); + return String(made.id); +}; + +const enqueue = async (ids: string[], locales: string[]): Promise => { + await callEndpoint(ctx.payload, "post", "/translate/enqueue", { + body: { + source_lng: "en", + target_lng: locales, + collection_slug: "docs", + collection_id: ids, + strategy: "overwrite", + publish_on_translation: false, + }, + }); +}; + +const panel = async (): Promise<{ docs: Entry[]; failed_window_ms: number }> => { + const res = await callEndpoint(ctx.payload, "get", "/translate/collection/:collection_slug", { + routeParams: { collection_slug: "docs" }, + }); + return (res.data as { data: { docs: Entry[]; failed_window_ms: number } }).data; +}; + +const daysAgo = (days: number): string => + new Date(Date.now() - days * 24 * 60 * 60 * 1000).toISOString(); + +const ERROR = { name: "Error", message: "boom", cancelled: false, stack: "" }; + +/** + * `gave-up` and `retrying` are the only two failure shapes Payload writes: `handleWorkflowError.js` + * stores `error` on every attempt and sets `hasError` only once it stops retrying. + */ +const failureShape = (failure: "gave-up" | "retrying"): Record => { + if (failure === "gave-up") return { error: ERROR, hasError: true }; + return { error: ERROR }; +}; + +/** `payload.update` rewrites `updatedAt` with now, so a backdated run goes through `payload.db.updateOne`. */ +const seedRun = async ( + documentId: string, + targetLng: string, + state: { + failure?: "gave-up" | "retrying"; + completedAt?: string; + updatedAt?: string; + log?: Array>; + } +): Promise => { + const row = await ctx.payload.create({ + collection: JOBS, + data: { + workflowSlug: WORKFLOW, + queue: QUEUE, + input: { + collection_slug: "docs", + collection_id: documentId, + source_lng: "en", + target_lngs: [targetLng], + strategy: "overwrite", + }, + ...(state.failure === undefined ? {} : failureShape(state.failure)), + ...(state.completedAt === undefined ? {} : { completedAt: state.completedAt }), + ...(state.log === undefined ? {} : { log: state.log }), + }, + } as never); + + if (state.updatedAt === undefined) return; + await ctx.payload.db.updateOne({ + collection: JOBS, + id: (row as { id: unknown }).id, + data: { updatedAt: state.updatedAt }, + returning: false, + } as never); +}; + +const localesIn = (entries: Entry[]): string[] => entries.map((entry) => entry.target_lng).sort(); + +describe("the collection panel reports what is happening", () => { + beforeAll(async () => { + ctx = await bootTestPayload({ + runner: createPayloadJobsRunner({ autoRun: false }), + failedWindowMs: WINDOW_MS, + }); + }); + afterAll(async () => { + await ctx?.cleanup(); + }); + + it("names a run that is queued", async () => { + const id = await create("Queued"); + await enqueue([id], ["de"]); + + expect(localesIn((await panel()).docs)).toContain("de"); + }); + + it("says nothing about a run that completed", async () => { + const id = await create("Completed"); + await seedRun(id, "it", { completedAt: new Date().toISOString() }); + + const entries = (await panel()).docs.filter((entry) => entry.collection_id === id); + expect( + entries, + "a collection-wide total of finished work can never be both bounded and true" + ).toEqual([]); + }); + + it("names a run that failed inside the window", async () => { + const id = await create("Failed recently"); + await seedRun(id, "es", { failure: "gave-up", updatedAt: daysAgo(1) }); + + const entries = (await panel()).docs.filter((entry) => entry.collection_id === id); + expect(localesIn(entries), "a recent failure is a call to action").toEqual(["es"]); + expect( + entries.map((entry) => entry.status), + "a run the queue gave up on is the one thing this row exists to report" + ).toEqual(["failed"]); + }); + + it("says nothing about a run that failed before the window", async () => { + const id = await create("Failed long ago"); + await seedRun(id, "pt", { failure: "gave-up", updatedAt: daysAgo(WINDOW_DAYS + 1) }); + + const entries = (await panel()).docs.filter((entry) => entry.collection_id === id); + expect(entries, "a failure from months ago is not an action").toEqual([]); + }); + + it("treats a run that is still retrying as in flight, not as failed", async () => { + const id = await create("Retrying"); + await seedRun(id, "nl", { + failure: "retrying", + updatedAt: daysAgo(WINDOW_DAYS + 1), + log: [ + { + taskSlug: "translate_document", + taskID: "nl", + state: "failed", + executedAt: daysAgo(WINDOW_DAYS + 1), + completedAt: daysAgo(WINDOW_DAYS + 1), + error: { message: "boom" }, + input: { target_lng: "nl" }, + }, + ], + }); + + const entries = (await panel()).docs.filter((entry) => entry.collection_id === id); + expect( + localesIn(entries), + "hasError is set only on final give-up, so a retrying run is still work in flight" + ).toEqual(["nl"]); + expect( + entries.map((entry) => entry.status), + "telling someone to open a document over a run that is still retrying is a false alarm" + ).toEqual(["pending"]); + }); + + it("reports the window it counted failures over", async () => { + expect((await panel()).failed_window_ms).toBe(WINDOW_MS); + }); +}); diff --git a/apps/dev/src/integration/translator/read-access-reports.int.test.ts b/apps/dev/src/integration/translator/read-access-reports.int.test.ts index fa20fc55b..4c277adc9 100644 --- a/apps/dev/src/integration/translator/read-access-reports.int.test.ts +++ b/apps/dev/src/integration/translator/read-access-reports.int.test.ts @@ -126,10 +126,12 @@ describe("a report shows only what its caller may read", () => { }); it("still reports to a caller the rule allows", async () => { - const { data } = await collectionReport(allowed); + // The collection report answers only with work in flight, and this runner finishes every + // locale before it returns. + const { data } = await documentReport(allowed); expect( - (data as { data?: { docs?: unknown[] } }).data?.docs?.length, + (data as { data?: unknown[] }).data?.length, "hiding must not be a blanket refusal" ).toBeGreaterThan(0); }); diff --git a/apps/dev/src/integration/translator/status-reads-are-bounded.int.test.ts b/apps/dev/src/integration/translator/status-reads-are-bounded.int.test.ts new file mode 100644 index 000000000..6deb0c49a --- /dev/null +++ b/apps/dev/src/integration/translator/status-reads-are-bounded.int.test.ts @@ -0,0 +1,226 @@ +import { createPayloadJobsRunner } from "@focus-reactive/payload-plugin-translator"; +import { afterAll, afterEach, beforeAll, describe, expect, it } from "vitest"; + +import { bootTestPayload } from "./bootTestPayload"; +import type { TestPayload } from "./bootTestPayload"; +import { callEndpoint } from "./callEndpoint"; + +let ctx: TestPayload; + +const JOBS = "payload-jobs" as "docs"; +const WORKFLOW = "translate_document_locales"; +const QUEUE = "translations"; + +type JobRow = { id: unknown; input?: Record }; + +/** + * Every job row the plugin pulled out of the database while serving one request. + * + * The endpoints' answers cannot show this: today they narrow after the read, so a correct answer is + * exactly what an unbounded read produces. What the work changes is how much came back, which is + * only visible from the read itself. + */ +let loaded: JobRow[][] = []; +let restore: (() => void) | undefined; + +const watchJobReads = () => { + const real = ctx.payload.find.bind(ctx.payload); + loaded = []; + ctx.payload.find = (async (args: { collection: string }) => { + const result = await real(args as Parameters[0]); + if (args.collection === JOBS) loaded.push(result.docs as unknown as JobRow[]); + return result; + }) as typeof ctx.payload.find; + restore = () => { + ctx.payload.find = real; + }; +}; + +const rowsRead = (): JobRow[] => loaded.flat(); + +const create = async (title: string): Promise => { + const made = await ctx.payload.create({ + collection: "docs", + locale: "en", + data: { _status: "published", title, note: "A note" }, + }); + return String(made.id); +}; + +const enqueue = async (ids: string[], locales: string[]): Promise => { + await callEndpoint(ctx.payload, "post", "/translate/enqueue", { + body: { + source_lng: "en", + target_lng: locales, + collection_slug: "docs", + collection_id: ids, + strategy: "overwrite", + publish_on_translation: false, + }, + }); +}; + +/** + * A row as the plugin wrote them before document references became ID-agnostic. + * + * `storedAs` writes the id through the adapter rather than through the field, which is the only way + * to produce the shape that matters here: a legacy id stored as a **number**. Measured on all three + * adapters, a `where` comparing such a value against a document id finds nothing — so a document + * narrowing that matched the legacy id would drop the row silently. + */ +const seedLegacyRow = async ( + collectionSlug: string, + documentId: string, + storedAs?: unknown, + targetLng = "de" +): Promise => { + const input = { + collection: { relationTo: collectionSlug, value: documentId }, + source_lng: "en", + target_lngs: [targetLng], + strategy: "overwrite", + }; + const row = await ctx.payload.create({ + collection: JOBS, + data: { workflowSlug: WORKFLOW, queue: QUEUE, input }, + } as never); + if (storedAs === undefined) return; + await ctx.payload.db.updateOne({ + collection: JOBS, + id: (row as { id: unknown }).id, + data: { input: { ...input, collection: { relationTo: collectionSlug, value: storedAs } } }, + returning: false, + } as never); +}; + +const seedForeignRow = async (collectionSlug: string): Promise => { + await ctx.payload.create({ + collection: JOBS, + data: { + workflowSlug: WORKFLOW, + queue: QUEUE, + input: { + collection_slug: collectionSlug, + collection_id: "999", + source_lng: "en", + target_lngs: ["de"], + strategy: "overwrite", + }, + }, + } as never); +}; + +/** SQL ids are integers; Mongo ids are strings, where the number case cannot arise. */ +const numericIfPossible = (id: string): unknown => (/^\d+$/u.test(id) ? Number(id) : id); + +const slugOf = (row: JobRow): unknown => + row.input?.collection_slug ?? (row.input?.collection as { relationTo?: unknown })?.relationTo; + +const documentOf = (row: JobRow): unknown => + row.input?.collection_id ?? (row.input?.collection as { value?: unknown })?.value; + +describe("the status endpoints read only what they answer about", () => { + beforeAll(async () => { + ctx = await bootTestPayload({ runner: createPayloadJobsRunner({ autoRun: false }) }); + }); + afterAll(async () => { + await ctx?.cleanup(); + }); + afterEach(() => { + restore?.(); + restore = undefined; + }); + + it("loads no job row belonging to another collection", async () => { + const id = await create("Ours"); + await enqueue([id], ["de"]); + await seedForeignRow("somewhere-else"); + + watchJobReads(); + await callEndpoint(ctx.payload, "get", "/translate/collection/:collection_slug", { + routeParams: { collection_slug: "docs" }, + }); + + expect(rowsRead().length, "the read happened at all").toBeGreaterThan(0); + expect( + rowsRead() + .map(slugOf) + .filter((slug) => slug === "somewhere-else"), + "another collection's history is not this collection's panel to pay for" + ).toEqual([]); + }); + + it("still finds a row written in the legacy relationship shape", async () => { + const id = await create("Legacy"); + await seedLegacyRow("docs", id); + + const res = await callEndpoint(ctx.payload, "get", "/translate/collection/:collection_slug", { + routeParams: { collection_slug: "docs" }, + }); + + const docs = (res.data as { data: { docs: Array<{ collection_id: string }> } }).data.docs; + expect( + docs.map((entry) => entry.collection_id), + "rows written before the ID-agnostic migration still answer" + ).toContain(id); + }); + + it("finds a legacy row by document whichever way its id was stored", async () => { + const id = await create("Legacy, asked by document"); + await seedLegacyRow("docs", id, id, "de"); + await seedLegacyRow("docs", id, numericIfPossible(id), "fr"); + + const res = await callEndpoint( + ctx.payload, + "get", + "/translate/document/:collection_slug/:collection_id", + { routeParams: { collection_slug: "docs", collection_id: id } } + ); + + const body = res.data as { data: Array<{ input: { target_lng: string } }> }; + expect( + body.data.map((task) => task.input.target_lng).sort(), + "a relationship field stored a numeric collection's id as a number; the narrowing must offer both forms" + ).toEqual(["de", "fr"]); + }); + + it("loads only the asked document's rows, for rows written in the flat shape", async () => { + const asked = await create("Asked about"); + const other = await create("Not asked about"); + await enqueue([asked, other], ["de"]); + + watchJobReads(); + await callEndpoint(ctx.payload, "get", "/translate/document/:collection_slug/:collection_id", { + routeParams: { collection_slug: "docs", collection_id: asked }, + }); + + const flatRowsForOthers = rowsRead().filter( + (row) => row.input?.collection_slug !== undefined && documentOf(row) !== asked + ); + expect(rowsRead().length, "the read happened at all").toBeGreaterThan(0); + expect( + flatRowsForOthers.map(documentOf), + "one document's panel does not read another document's runs" + ).toEqual([]); + }); + + it("reports a locale whose run is older than every other locale's run", async () => { + const id = await create("Skewed history"); + + await enqueue([id], ["fr"]); + for (let i = 0; i < 3; i++) await enqueue([id], ["de"]); + + const res = await callEndpoint( + ctx.payload, + "get", + "/translate/document/:collection_slug/:collection_id", + { routeParams: { collection_slug: "docs", collection_id: id } } + ); + + const body = res.data as { data: Array<{ input: { target_lng: string } }> }; + expect( + body.data.map((task) => task.input.target_lng), + "a count bound would have dropped the oldest locale; a document bound must not" + ).toContain("fr"); + }); +}); diff --git a/apps/dev/src/integration/tsconfig.json b/apps/dev/src/integration/tsconfig.json new file mode 100644 index 000000000..e003a0d2b --- /dev/null +++ b/apps/dev/src/integration/tsconfig.json @@ -0,0 +1,8 @@ +{ + "extends": "../../tsconfig.json", + "compilerOptions": { + "incremental": false + }, + "exclude": ["node_modules"], + "include": ["**/*.ts"] +} diff --git a/apps/dev/tsconfig.check.json b/apps/dev/tsconfig.check.json index ea0c888eb..5c370ed04 100644 --- a/apps/dev/tsconfig.check.json +++ b/apps/dev/tsconfig.check.json @@ -1,7 +1,3 @@ { - "extends": "./tsconfig.json", - "compilerOptions": { - "incremental": false - }, - "include": ["src/integration/**/*.ts"] + "extends": "./src/integration/tsconfig.json" } diff --git a/apps/dev/tsconfig.json b/apps/dev/tsconfig.json index 3f587c0cd..5436fcc68 100644 --- a/apps/dev/tsconfig.json +++ b/apps/dev/tsconfig.json @@ -30,5 +30,5 @@ ".next/types/**/*.ts", ".next/dev/types/**/*.ts" ], - "exclude": ["node_modules"] + "exclude": ["node_modules", "src/integration"] } diff --git a/bun.lock b/bun.lock index ca389258b..4f6b28b1b 100644 --- a/bun.lock +++ b/bun.lock @@ -211,7 +211,7 @@ }, "packages/payload-plugin-ab": { "name": "@focus-reactive/payload-plugin-ab", - "version": "2.10.0", + "version": "2.10.1", "devDependencies": { "@payloadcms/ui": "3.90.1", "@types/node": "22.19.9", @@ -289,7 +289,7 @@ }, "packages/payload-plugin-comments": { "name": "@focus-reactive/payload-plugin-comments", - "version": "1.11.1", + "version": "1.11.3", "dependencies": { "@radix-ui/react-dropdown-menu": "^2.1.16", "@tanstack/react-query": "5.0.0", @@ -351,7 +351,7 @@ }, "packages/payload-plugin-json-form-builder": { "name": "@focus-reactive/payload-plugin-json-form-builder", - "version": "1.1.1", + "version": "1.5.0", "dependencies": { "clsx": "2.1.1", }, @@ -482,7 +482,7 @@ }, "packages/payload-plugin-translator": { "name": "@focus-reactive/payload-plugin-translator", - "version": "0.15.0", + "version": "0.16.0", "dependencies": { "@hookform/resolvers": "^3.9.0", "@radix-ui/react-popover": "^1.1.0", @@ -498,9 +498,12 @@ "@payloadcms/ui": "3.90.1", "@swc/cli": "0.4.0", "@swc/core": "1.7.0", + "@testing-library/dom": "10.4.1", + "@testing-library/react": "16.3.0", "@types/react": "19.2.9", "@vitest/coverage-v8": "3.2.4", "copyfiles": "2.4.1", + "jsdom": "26.1.0", "payload": "3.90.1", "react": "19.0.0", "typescript": "5.5.2", @@ -3962,6 +3965,10 @@ "@focus-reactive/payload-plugin-seo/typescript": ["typescript@5.7.3", "", { "bin": { "tsc": "bin/tsc", "tsserver": "bin/tsserver" } }, "sha512-84MVSjMEHP+FQRPy3pX9sTVV/INIex71s9TL2Gm5FG/WG1SqXeKyZ0k7/blY/4FdOzI12CBy1vGc4og/eus0fw=="], + "@focus-reactive/payload-plugin-translator/@testing-library/react": ["@testing-library/react@16.3.0", "", { "dependencies": { "@babel/runtime": "^7.12.5" }, "peerDependencies": { "@testing-library/dom": "^10.0.0", "@types/react": "^18.0.0 || ^19.0.0", "@types/react-dom": "^18.0.0 || ^19.0.0", "react": "^18.0.0 || ^19.0.0", "react-dom": "^18.0.0 || ^19.0.0" }, "optionalPeers": ["@types/react", "@types/react-dom"] }, "sha512-kFSyxiEDwv1WLl2fgsq6pPBbw5aWKrsY2/noi1Id0TK0UParSF62oFQFGHXIyaG4pp2tEub/Zlel+fjjZILDsw=="], + + "@focus-reactive/payload-plugin-translator/jsdom": ["jsdom@26.1.0", "", { "dependencies": { "cssstyle": "^4.2.1", "data-urls": "^5.0.0", "decimal.js": "^10.5.0", "html-encoding-sniffer": "^4.0.0", "http-proxy-agent": "^7.0.2", "https-proxy-agent": "^7.0.6", "is-potential-custom-element-name": "^1.0.1", "nwsapi": "^2.2.16", "parse5": "^7.2.1", "rrweb-cssom": "^0.8.0", "saxes": "^6.0.0", "symbol-tree": "^3.2.4", "tough-cookie": "^5.1.1", "w3c-xmlserializer": "^5.0.0", "webidl-conversions": "^7.0.0", "whatwg-encoding": "^3.1.1", "whatwg-mimetype": "^4.0.0", "whatwg-url": "^14.1.1", "ws": "^8.18.0", "xml-name-validator": "^5.0.0" }, "peerDependencies": { "canvas": "^3.0.0" }, "optionalPeers": ["canvas"] }, "sha512-Cvc9WUhxSMEo4McES3P7oK3QaXldCfNWp7pl2NNeiIFlCoLr3kfq9kb1fxftiwk1FLV7CvpvDfonxtzUDeSOPg=="], + "@focus-reactive/payload-plugin-translator/typescript": ["typescript@5.5.2", "", { "bin": { "tsc": "bin/tsc", "tsserver": "bin/tsserver" } }, "sha512-NcRtPEOsPFFWjobJEtfihkLCZCXZt/os3zf8nTxjVH3RvTSxjrCamJpbExGvYOF+tFHc3pA65qpdwPbzjohhew=="], "@focus-reactive/payload-plugin-translator/vitest": ["vitest@3.2.4", "", { "dependencies": { "@types/chai": "^5.2.2", "@vitest/expect": "3.2.4", "@vitest/mocker": "3.2.4", "@vitest/pretty-format": "^3.2.4", "@vitest/runner": "3.2.4", "@vitest/snapshot": "3.2.4", "@vitest/spy": "3.2.4", "@vitest/utils": "3.2.4", "chai": "^5.2.0", "debug": "^4.4.1", "expect-type": "^1.2.1", "magic-string": "^0.30.17", "pathe": "^2.0.3", "picomatch": "^4.0.2", "std-env": "^3.9.0", "tinybench": "^2.9.0", "tinyexec": "^0.3.2", "tinyglobby": "^0.2.14", "tinypool": "^1.1.1", "tinyrainbow": "^2.0.0", "vite": "^5.0.0 || ^6.0.0 || ^7.0.0-0", "vite-node": "3.2.4", "why-is-node-running": "^2.3.0" }, "peerDependencies": { "@edge-runtime/vm": "*", "@types/debug": "^4.1.12", "@types/node": "^18.0.0 || ^20.0.0 || >=22.0.0", "@vitest/browser": "3.2.4", "@vitest/ui": "3.2.4", "happy-dom": "*", "jsdom": "*" }, "optionalPeers": ["@edge-runtime/vm", "@types/debug", "@types/node", "@vitest/browser", "@vitest/ui", "happy-dom", "jsdom"], "bin": { "vitest": "vitest.mjs" } }, "sha512-LUCP5ev3GURDysTWiP47wRRUpLKMOfPh+yKTx3kVIEiu5KOMeqzpnYNsKyOoVrULivR8tLcks4+lga33Whn90A=="], @@ -4990,6 +4997,8 @@ "@focus-reactive/payload-plugin-seo/@types/node/undici-types": ["undici-types@8.3.0", "", {}, "sha512-j375ScV60dom+YkPFIfTLcOiPxkN/buHz5GobjLhixFuANaNs3C9l4GmrWqejgXWJ7BbJcFYpTEUkS1Ge8bpZQ=="], + "@focus-reactive/payload-plugin-translator/jsdom/rrweb-cssom": ["rrweb-cssom@0.8.0", "", {}, "sha512-guoltQEx+9aMf2gDZ0s62EcV8lsXR+0w8915TC3ITdn2YueuNjdAYh/levpU9nFaoChh9RUS5ZdQMrKfVEN9tw=="], + "@focus-reactive/payload-plugin-translator/vitest/@vitest/expect": ["@vitest/expect@3.2.4", "", { "dependencies": { "@types/chai": "^5.2.2", "@vitest/spy": "3.2.4", "@vitest/utils": "3.2.4", "chai": "^5.2.0", "tinyrainbow": "^2.0.0" } }, "sha512-Io0yyORnB6sikFlt8QW5K7slY4OjqNX9jmJQ02QDda8lyM6B5oNgVWoSoKPac8/kgnCUzuHQKrSLtu/uOqqrig=="], "@focus-reactive/payload-plugin-translator/vitest/@vitest/mocker": ["@vitest/mocker@3.2.4", "", { "dependencies": { "@vitest/spy": "3.2.4", "estree-walker": "^3.0.3", "magic-string": "^0.30.17" }, "peerDependencies": { "msw": "^2.4.9", "vite": "^5.0.0 || ^6.0.0 || ^7.0.0-0" }, "optionalPeers": ["msw", "vite"] }, "sha512-46ryTE9RZO/rfDd7pEqFl7etuyzekzEhUbTW3BvmeO/BcCMEgq59BKhek3dXDWgAj4oMK6OZi+vRr1wPW6qjEQ=="], diff --git a/packages/payload-plugin-translator/docs/plans/2026-10-12-bound-the-status-reads.md b/packages/payload-plugin-translator/docs/plans/2026-10-12-bound-the-status-reads.md new file mode 100644 index 000000000..2456a3134 --- /dev/null +++ b/packages/payload-plugin-translator/docs/plans/2026-10-12-bound-the-status-reads.md @@ -0,0 +1,358 @@ +# Bound what the status endpoints read + +Source: 2026-10-12-bound-the-status-reads.source.md + +## Parsing summary + +- Source: the owner's question in chat, plus issue #123 filed 2026-09-03. +- Ask: stop the two status endpoints loading every job row, and have the fix work on SQLite, + PostgreSQL and MongoDB alike. +- The owner half-remembers a narrowing already landing and asks for that to be checked first. +- #123 measured the problem, listed three options, and set its own priority to Low. That stands. +- Contradiction worth naming: #123's table shows a "narrowed to one collection" column as if it + shipped. It did not — the commit that would have carried it says narrowing was rejected. + +## The owner's premise, checked first + +**Partly right.** There *is* a narrowing, and it is not the one that bounds growth. + +`PayloadJobsTaskRunner.findRawJobs` sends exactly one condition, `ownJobs()`: + +```ts +{ or: [{ workflowSlug: { equals: workflowName } }, { taskSlug: { equals: taskName } }] } +``` + +That keeps other plugins' and the app's own jobs out of the result — real, and worth having. It does +not narrow by collection, by document, by age or by count, and `pagination: false` means every +matching row comes back. So this plugin's whole history still arrives on every panel open. + +The `excludeCompleted` filter added by #108 *is* a bounding narrowing, but the status endpoints +cannot use it — completed jobs are what they display — and they pass no filter at all. + +## The two reasons for filtering in memory, re-measured + +Both reasons are on the record. **One has expired and the other is answerable in a single query.** +Measured on Payload 3.90.1 against all three adapters, with a throwaway integration probe (since +removed; the dev containers were started for the run and stopped again afterwards). + +| What was asked of the database | SQLite | PostgreSQL | MongoDB | +|---|---|---|---| +| `input.collection_slug` `equals` | ✅ 1 | ✅ 1 | ✅ 1 | +| `input.collection_id` `equals` | ✅ 1 | ✅ 1 | ✅ 1 | +| `or` of the flat path **and** the legacy `input.collection.relationTo` | ✅ **2 of 2** | ✅ **2 of 2** | ✅ **2 of 2** | +| `limit: 1` + `sort: -createdAt`, reading `totalDocs` | ✅ ok, total 2 | ✅ ok, total 2 | ✅ ok, total 2 | +| legacy `input.collection.value` `in` (matching by **id**) | ❌ 0 | ❌ 0 | ✅ 1 | + +Readings: + +1. **The SQLite JSON-path bug worked around in `675b34a8` is fixed.** A `where` on a path inside the + job's `json` input now matches on every adapter. That was the first and larger reason for reading + everything and filtering afterwards, and it no longer applies. +2. **One query can cover both storage shapes.** An `or` across the flat `collection_slug` and the + legacy `collection.relationTo` returned both rows everywhere. The second reason — "a where clause + sees only the first" (`2c92472e`) — is answered by writing the clause as an `or`. Note it must + match on `relationTo` (a collection name, a string), not on `value`: matching the legacy id still + fails on SQL, which is the original bug surviving in the one place that compares a stored JSON + number against a string. +3. **Payload gives `limit`, `sort` and `totalDocs` on this collection**, so bounding is mechanically + available. + +The probe row that reads `всего=0` is incidental and worth recording: filtering by +`taskSlug: translate_document` alone returns nothing, because the row is a *workflow* job whose +`taskSlug` is null. That is why `ownJobs()` is an `or` — and it is the trap any future narrowing +must not walk into. + +## Who the two endpoints are actually for + +Measured by reading every consumer, not by reasoning about the response type. **The collection +endpoint has two audiences, and neither of them wants the archive.** + +**Audience one — the plugin's own panel — reads nothing but counts.** Every consumer of +`get-collection-status`: + +| Consumer | What it takes from the response | +|---|---| +| `CollectionTranslationProgress.tsx:63` | `data[key].length` — the number in a row | +| `panelStatus.ts:113-116` | `.length > 0` on four groups — picks the panel's marker | +| `useCollectionTranslationStatus.ts:47` | `.some(status is pending or running)` — whether to keep polling | +| `BulkTranslationDashboard.tsx:72` | the four lengths again — whether to show the panel at all | + +`id`, `collection_id` and `target_lng` cross the wire on every row and are read **nowhere**. The +panel is a bulk-translation progress indicator: a Queued row with a cancel button, a Translating row +that pulses. + +**Audience two — an external host — reads the rows, and only the live ones.** Those three fields are +not vestigial: #103 (`denis-urban`, closed by #177 two weeks ago) asked for them by name, to build +"the queue view for an admin dashboard". Its own words: *"what is in flight across this collection"*, +rendering *"the French translation of page 42 is running"*. In flight — not the archive. + +**What that audience is worth, decided by the owner (R10):** the HTTP contract is not public surface, +so a host building on it carries its own risk. It is weighed, not obeyed. The response may be narrowed; +it may not be gutted. + +That costs nothing here, because both audiences are served by the same **live** set, which +`completedAt: { exists: false }` already bounds — the filter #108 added and this research previously +recorded as unusable for these endpoints. + +**What that leaves.** Exactly one thing is unbounded and wanted: the panel's `Translated — N` count, +which today means *document-locale pairs completed in all of history*. And it cannot be answered by a +database aggregate, for a reason that is specific and worth stating: + +- the fan-out is per locale, from `input.target_lngs`, a **json array** — so one row is N tasks; +- per-locale completion is read from the job's `log` array, also json: a completed *row* can hold a + succeeded French and a failed German (`normalizeJob.ts:72-84`); +- `visibleIds` filters through the **content** collection's read rules after the fact, so a count over + the jobs table cannot honour access control. + +`limit: 1` + `totalDocs` is available and measured, but it counts job **rows** — neither documents nor +locale pairs. There is no exact count of the archive without reading the archive. + +## The live/archive split is exact + +Measured, not assumed, because option H's correctness rests on it: **does `completedAt: { exists: false }` +drop anything other than the `Translated` row?** It does not. + +1. Payload sets `completedAt` only on the success path — `runJobs/runJob/index.js:50`, under the + comment "Workflow has completed successfully". A run that exhausts its retries goes through + `handleWorkflowError` instead and keeps `completedAt` empty. +2. The workflow handler walks the locales in a loop and rethrows on the first failure + (`PayloadJobsRunnerProvider.ts`, `throw error` inside the `catch`), so the loop cannot reach its end + with a locale unfinished. + +**Therefore a job row carrying `completedAt` has every locale in `target_lngs` succeeded.** A completed +row cannot hold a failed locale, even though `normalizeJobLocales` is written to allow one +(`normalizeJob.ts:72-84` reads each locale's latest log entry and would mark a non-succeeded one +`failed`). + +So the four panel rows partition cleanly: `Translating`, `Queued` and `Failed` come from rows with +`completedAt` empty — each bounded by work in flight — and `Translated` is the archive and nothing +else. The unbounded read exists to produce one number in one row. + +## Codebase map + +| Thing | Path | +|---|---| +| collection-status endpoint | `src/server/features/get-collection-status/handler.ts:38` | +| document-status endpoint | `src/server/features/get-document-status/handler.ts:40` | +| the read | `src/server/modules/task-runner/payload-jobs-runner/PayloadJobsTaskRunner.ts` — `findByCollection`, `findRawJobs`, `ownJobs` | +| the filter contract | `src/server/modules/task-runner/TaskRunner.interface.ts` — `TaskFilter` | +| two-shape reference reader | `src/server/modules/task-runner/payload-jobs-runner/store/readCollectionRef.ts` | +| where the input shape is declared | `src/server/modules/task-runner/payload-jobs-runner/store/jobInput.schema.ts` | +| how Payload stores it | `node_modules/payload/dist/queues/config/collection.js` — `{ name: 'input', type: 'json' }` | +| the dashboard that opens the panel | `src/client/widgets/bulk-translation-dashboard/ui/BulkTranslationDashboard.tsx` | +| the client query | `src/client/entities/translation/api/queries/useCollectionTranslationStatus.ts` | + +Prior art: #108 (merged, bounded the enqueue/cancel paths), #123 (this), #122 (retention — closed +deliberately on 2026-09-04 by PR #129, which documented pruning as the **host's** job rather than +adding it to the plugin; `README.md:384`). No pruning exists inside the plugin, and none is planned. + +## Problem Statement + +Opening a translation status panel reads every translation job this plugin has ever created, with no +limit and no paging, and narrows to the requested collection afterwards in memory. The cost of +looking at status therefore grows with the project's whole translation history, and since #108 that +history no longer prunes itself. The two workarounds that forced the in-memory filter — a Payload +bug on SQLite, and two storage shapes for a document reference — no longer force it. + +## Requirements + +- R1 (in) The two status endpoints must stop reading rows that belong to other collections. + > "как мы можем исправить это? Так чтобы и не доставать все записи сразу" +- R2 (in) Whatever is done must behave the same on SQLite, PostgreSQL and MongoDB. + > "и при этом чтобы работало на всех бд?" +- R3 (out) Checking the owner's recollection about an existing narrowing — answered by this research + under "The owner's premise, checked first", so an implementation inherits the answer rather than + the task. + > "ПЛюс ко всему я что-то припоминаю, мы вроде сузили окно поиска. Это не должно возвращат ьвсе записи теперь." +- R4 (in) A document's status must not lose a locale whose most recent run is older than other + locales' runs. + > "`get-document-status` keeps the latest job **per target locale**, so if one locale's most recent job is older than N others, that locale's row silently disappears. A count bound trades a slow correct answer for a fast wrong one" +- R5 (in) A document translated long ago must still report that it was translated. + > "A document translated three months ago stops reporting a status, and the editor cannot tell "never translated" from "translated long ago"" +- R6 (out) Bounding by count alone. + > "A count bound trades a slow correct answer for a fast wrong one, which is the same failure the unpaginated read was avoiding." +- R7 (in) [inferred] Rows written before the ID-agnostic migration must keep being found — rests on: + `readCollectionRef` reads both shapes today, and `docs/DEPRECATIONS.md#jobs-input-collection-field` + keeps the legacy shape until the next major. +- R8 (out) Deferred with the batch, see the task contract's "Why Stage B was withdrawn". The collection panel keeps reporting on the **whole collection**, not on the documents the + admin list happens to be showing. + > "Я всё еще хочу показывать статус по всей коллекции а не тольк по дументам на странице, это буде твводить в заблужение." +- R9 (out) Deferred with the batch. The panel's own needs are counts per status; the endpoint must not read rows it only counts. + > "Нам же по сути надо просто посчитать количество сколько задач в каком статусе, для этого нам не нужно делать выборку ВСЕХ записей." +- R10 (out) Moot: the response shape is unchanged. `get-collection-status` may be narrowed but not broken outright: the document id and the + target locale stay on whatever rows it still returns. The HTTP contract is **not public surface** — + nothing in `src/index.ts` or the README describes it — so the host that filed #103 is a fact to weigh, + not a constraint that binds the design. + > "контракт эндпоинтов не публичный. То, что кто-то строит на нем свою админку его проблемы. Мы его полностью ломать не будем, но учитывать этот факт стоит." + +**Non-functional scan.** Performance — the point of the work. Security/access control — relevant: +`get-collection-status` filters results through `visibleIds` after the read, so narrowing the query +must not become a way to skip that check. Accessibility — N/A, no UI change. i18n — N/A. Observability +— relevant but minor: a bounded read that silently truncates should be distinguishable from an empty +one. + +## The options, with what each costs + +**A — narrow by collection in the query.** An `or` over the two shapes, measured working everywhere. +Divides rows by the number of translatable collections and nothing more: one collection's history +still grows linearly. Cheap, no information lost, no behaviour change. Does not satisfy R1 on its own +in the long run, but it is the floor under every other option. + +**B — narrow `get-document-status` by document.** `input.collection_id` is now queryable on all three +adapters. That endpoint asks about exactly one document, so its read becomes bounded by how many +times *that document* was translated — not by the table. This is a fix rather than a trade: nothing +is lost, R4 and R5 are untouched. The `documentIds` filter already exists in `TaskFilter` and is +applied in memory; this moves it into the query. + +**C — narrow `get-collection-status` by the documents the panel is showing.** ~~Pass the admin list's +page of ids and read only those.~~ **Rejected by the owner under R8:** a panel that says "the +collection" while reporting one page of it misleads the person reading it. Not revisited. + +**H — bound both endpoints to live work, and count the rest.** Falls out of "Who the two endpoints are +actually for": `completedAt: { exists: false }` is exact, bounded by what is in flight, and serves +both audiences — the panel's three in-progress rows and #103's queue view. The archive is then only +ever a number, never rows. Costs: the `Translated` count needs an answer of its own (the open question +below), and the response gains a shape the host did not have. + +**I — a plugin-owned index collection** (document × locale → latest state), as real columns. This is +option G, and it reads very differently now that the panel is known to want counts: four bounded +`count` queries answer it exactly, including the archive, with no json path and no fan-out in memory. +Costs: a collection, a migration, and a second source of truth that can drift from the job rows. Still +not justified while A, B and H are unspent — but it is the answer if the archive count must stay +exact. + +**D — bound by age.** Rejected by R5, and #123 already explains why. + +**E — bound by count.** Rejected by R6 and R4. + +**F — retention (#122).** Stops the table growing, which would remove the question for every endpoint +at once and lose no visible information. #123 says "if retention lands, this issue likely closes +unfixed", and the commit that created the need says retention went "from tidiness into a requirement". +**But retention is not coming from this plugin.** #122 was closed by documenting it as the host's +responsibility (PR #129, `README.md:384`), on the stated ground that `deleteJobOnComplete` is the +host's global setting and the plugin has no business overriding it. That reasoning holds — and its +consequence is that F is not an option this plugin can exercise. The escape hatch #123 leans on +belongs to someone else and may never be used, so the two endpoints have to be sized for a table +nobody is pruning. + + +## Acceptance Criteria (draft) + +| AC | Covers | Criterion | How it is checked | +|---|---|---|---| +| AC1 | R1, R2 | `get-document-status` for a document loads only that document's job rows | integration test counting rows returned by the runner, on all three adapters | +| AC2 | R1, R2, R7 | A collection filter written as an `or` over both reference shapes returns rows of both shapes | integration test seeding one row of each shape, on all three adapters | +| AC3 | R4 | A document whose French run is older than ten German runs still reports French | integration test with a skewed history | +| AC4 | R5 | A document translated long ago still reports its status | integration test with an aged row | +| AC5 | R2 | The narrowed query matches the same set the in-memory filter matched | the existing status integration tests stay green on all three adapters | +| AC6 | R1 | Opening the panel does not read rows of another collection | integration test with two collections, asserting the row count | +| AC7 | R8 | The panel's figures cover the whole collection, not the admin list's current page | integration test: a document outside the listed page still counts | +| AC8 | R9 | The endpoint does not load rows whose only use is being counted | integration test asserting rows read against an archive of known size | +| AC9 | R10 | Each returned row still names its document and target locale | the existing `handler.test.ts:144` stays green | + +## Open Questions + +1. **What the panel's `Translated — N` row should count** *[resolved — see Clarifications]* — the one remaining unbounded + figure, and the only decision left. Matters because: it is user-visible text in a *progress* panel, + and there is no exact count of the archive without reading the archive (see "Who the two endpoints + are actually for"). The candidates, with what each costs: + - *drop the row* — the panel becomes purely about work in flight, everything is bounded, nothing + else changes. Cheapest, and arguably a fix: "Translated — 1,284" in a bulk-progress panel tells + a person nothing they came for; + - *count this batch* — "42 of 50 done", bounded by the enqueue, closest to what a progress panel + means, but needs a notion of "this batch" the plugin does not have today; + - *keep it exact over all history* — forces option I, the index collection, with its migration and + its second source of truth; + - *count job rows instead of locale pairs* — one bounded query, but the number silently changes + meaning and would overcount a document translated twice. + + A note on scope: whichever is chosen, it is a change to what an editor sees, so it belongs to the + owner and not to an implementer. +2. **Is retention (#122) wanted, and with what policy** *[resolved]* — answered by the close of #122: + pruning is the host's, on the ground that `deleteJobOnComplete` is the host's global setting. So + this work cannot wait on retention landing in the plugin, and #123's "likely closes unfixed if + retention lands" does not apply. +3. ~~**Should #122 be reopened**~~ — no. It was not closed by oversight; it was declined with a reason + that still holds, and the decision is documented where a consumer reads it (`README.md:384`). +4. **Failure and empty states** *[non-blocking]* — a bounded read that hit its bound should be + distinguishable from an empty result. Nothing today says which it was. +5. **Access control** *[non-blocking]* — `visibleIds` filters after the read. Narrowing by ids supplied + by the client must not become a way to ask about documents the caller cannot see; the existing + check already covers it, and must be kept in that order. +7. **What the panel shows after a page reload** *[blocking]* — the batch's handles live in the enqueue + response, so reloading the admin list loses them. Matters because: it decides whether the panel + needs to persist the handle list (URL parameters or session storage, a client-side change) or + whether it may fall back to "what is still in flight for this collection" — which is bounded too, + but whose denominator shrinks as runs finish, so a reload during a 10-document batch could read + "3 of 3" where the person expects "7 of 10". +8. **Restate check** — "narrow the search window" was the owner's phrase and could mean either + "fewer rows per query" or "a shorter period of history". This research reads it as the first. + +## Clarifications + +**What `Translated — N` counts: the current run, not all history.** The owner's decision, and it +reframes the whole task — the figure was never meant to be a lifetime total: + +> "translated по идее должен показывать количество переведенных записей не вообще за всё время, а за +> текущий run… если пользователь выбрал 10 записей и нажал перевести, то надо показывать сколько +> записей из этих 10 переведено" + +**The batch is already identified, on the wire, today.** `POST /api/translate/enqueue` answers with +`assignments: [{ collection_slug, collection_id, target_lng, handle }]` — one entry per document and +locale, each naming the run that carries it (`enqueue-translation/handler.ts:101`, +`model.ts:36`). That shipped in #177 for issue #107. **The client throws it away**: +`useQueueDocumentTranslation.ts:27` types the response `{ data: null }` and returns `Promise`. + +So the panel can hold the batch's handles and ask for exactly those job rows. That query is +`id: { in: [...handles] }` — a primary-key lookup, bounded by the batch the person started, exact, and +needing nothing from a json path on any adapter. It is cheaper and more correct than every option +weighed above, and it makes the collection-wide archive read unnecessary rather than merely smaller. + +**The one gap it leaves: a page reload loses the handles**, because they live only in the client's +mutation result. See Open Question 7. + +## "The last run" is not a stored fact + +Asked by the owner: can the `Translated` figure be read from the last translation run, so it is always +shown without the client holding anything? **Not today.** + +A run, in the data, is one job row and it is about **one document** — `collection_slug`, +`collection_id`, `source_lng`, `target_lngs`, `strategy`, `publish_on_translation`, `requester_id`, +`requester_collection` (`jobInput.schema.ts:12-34`). Translating ten documents creates ten independent +rows. **Nothing records that those ten rows were one submission**, so "the last run across the +collection" cannot be queried — it was never written down. + +Three ways to make it queryable: + +| | How | Cost | +|---|---|---| +| **J** | stamp a batch mark into each row's `input` at enqueue; the panel reads the newest mark for the collection, then that mark's rows | one field. `input` is a `json` column, so **no migration**. A `where` on a path inside `input` is measured working on all three adapters | +| **K** | no new field — take the newest rows by `createdAt`, `sort: -createdAt` + `limit` | free and measured, but the bound is arbitrary and two editors submitting at once blend into one figure | +| **L** | a collection of batch entries: batch × document × locale × handle | exact, real columns, survives everything — but a new collection, a migration, and a second source of truth that can drift from the job rows | + +**J's one wrinkle, and its answer.** `enqueue` does not always create a row: when a locale is requested +for a document whose run is already live, it appends to that run instead (`extendJob`, +`PayloadJobsTaskRunner.ts:134`). Such a row carries the older batch mark, so the new batch would +under-report. Re-stamping the row with the new mark on append resolves it, and is correct for the +panel's purpose — the row genuinely is part of the newer submission now. + +J makes the archive read unnecessary rather than smaller, which closes #123 outright. + +## Risks & Constraints + +- The legacy reference shape leaves in the next major; an `or` written now becomes a single clause + then, and should be written so that deleting one branch is the whole change. +- `ownJobs()` is an `or` for a reason — a workflow row has no `taskSlug`. Any new clause must be + combined with it rather than replacing it. +- The collection-status read feeds an access-control filter applied afterwards. Order must not change. + +## Readiness + +- Unresolved `[blocking]` questions: **1** +- IN requirements with no acceptance criterion: **0** + +Needs answers first — one question, and a narrow one. Everything else is settled: A and B are free +and measured, C is rejected by the owner, F is not this plugin's to exercise, and H follows from the +consumers. What is soft is only what the `Translated` figure should mean. + +Next: needs-answers diff --git a/packages/payload-plugin-translator/docs/plans/2026-10-12-bound-the-status-reads.source.md b/packages/payload-plugin-translator/docs/plans/2026-10-12-bound-the-status-reads.source.md new file mode 100644 index 000000000..4d223b583 --- /dev/null +++ b/packages/payload-plugin-translator/docs/plans/2026-10-12-bound-the-status-reads.source.md @@ -0,0 +1,64 @@ +# Source + +## Owner's request, 2026-10-12 (chat, translated verbatim is not possible — recorded as asked, in Russian) + +> А почему мы не сужаем выборку сразу а делаем это вручную? Должна быть причина, посмотри предыдущие окммиты для этого места, может там есть объяснение + +> Окей. Давай теперь так: как мы можем исправить это? Так чтобы и не доставать все записи сразу и при этом чтобы работало на всех бд? ПЛюс ко всему я что-то припоминаю, мы вроде сузили окно поиска. Это не должно возвращат ьвсе записи теперь. + +## Issue #123, verbatim + +Split out of #108, which fixes the two paths that can be fixed cheaply and deliberately leaves this one alone. + +## What is not fixed by #108 + +`findByCollection` loads every translation job for a collection, unpaginated. #108 flattens that for **enqueue** and **cancel** by excluding completed jobs — a completed job can never be superseded or cancelled, so those paths simply stop caring how much history exists. + +The two status endpoints cannot use that lever, because the history *is* their content: + +- `get-collection-status` returns each job's `status`; excluding completed jobs would leave the panel empty immediately after everything translated successfully. +- `get-document-status` reduces to the latest job per target locale and reports `completed_at`; with completed jobs excluded there is nothing to reduce. + +#108 does give them the one free improvement — narrowing the query to the requested collection, which works on SQLite, Postgres and MongoDB alike. Measured, that divides the loaded rows by the number of translatable collections. It does **not** change the shape: the cost still grows linearly with that collection's history. + +## The three options, and what each costs an editor + +**Bound by recency** — fetch only jobs newer than some age. A document translated three months ago stops reporting a status, and the editor cannot tell "never translated" from "translated long ago". Note that *staleness* is computed separately, from provenance records, so "is this translation current" survives; what is lost is "was there a run, did it fail". + +**Bound by count** — fetch the first N, newest first. This has a trap worth stating plainly: `get-document-status` keeps the latest job **per target locale**, so if one locale's most recent job is older than N others, that locale's row silently disappears. A count bound trades a slow correct answer for a fast wrong one, which is the same failure the unpaginated read was avoiding. + +**Leave it** — accept linear growth. Defensible on the numbers: these endpoints are hit once when a human opens a panel, where 60 ms at 2000 jobs is imperceptible, whereas the enqueue path runs once per document in a bulk action, where the same curve is multiplied by the batch size. + +## Measured + +Synthetic history, identical on all three adapters. Rows loaded per call: + +| history | today | narrowed to one collection (#108) | +|---|---|---| +| 50 | 50 | 17 | +| 200 | 200 | 67 | +| 800 | 800 | 267 | +| 2000 | 2000 | 667 | + +## The alternative that removes the question + +#122 (retention for the plugin's own completed jobs) would stop the table growing at all, which fixes these endpoints without bounding anything and without any loss of visible information. If retention lands, this issue likely closes unfixed. + +## Priority + +Low, and deliberately so — the honest trigger for picking it up is a real project reporting a slow status panel, not a benchmark. Recorded so the next person to meet the linear curve knows it was measured, discussed, and left alone on purpose. + + +## Clarifications + +Owner, in chat, 2026-10-12, when asked what `get-collection-status` should return with no selection: + +> Нам же по сути надо просто посчитать количество сколько задач в каком статусе, для этого нам не нужно делать выборку ВСЕХ записей. Я всё еще хочу показывать статус по всей коллекции а не тольк по дументам на странице, это буде твводить в заблужение. + +Owner, in chat, 2026-10-12, on the weight issue #103's reporter carries: + +> контракт эндпоинтов не публичный. То, что кто-то строит на нем свою админку его проблемы. Мы его полностью ломать не будем, но учитывать этот факт стоит. + +Owner, in chat, 2026-10-12, deciding what the panel's `Translated` row counts: + +> Это и по смыслу и по функцоналу неверно. translated по идее должен показывать количество переведенных записей не вообще за всё время, а за текущий run, то есть сколько переведено записей за текущий запуск. Например если пользователь выбрал 10 записей и нажал перевести, то надо показывать сколько записей из этих 10 переведено, а не вообще. diff --git a/packages/payload-plugin-translator/docs/plans/2026-10-12-bound-the-status-reads.task.md b/packages/payload-plugin-translator/docs/plans/2026-10-12-bound-the-status-reads.task.md new file mode 100644 index 000000000..a670acefb --- /dev/null +++ b/packages/payload-plugin-translator/docs/plans/2026-10-12-bound-the-status-reads.task.md @@ -0,0 +1,400 @@ +# Bound what the status endpoints read — implementation contract + +Requirements: 2026-10-12-bound-the-status-reads.md + +> **Scope narrowed on 2026-10-13, after Stage B was built and reviewed: only Stage A ships.** +> The batch was withdrawn, not abandoned — D1–D8 below are kept as the record of why, because the +> next attempt should start from them rather than rediscover them. See **Why Stage B was withdrawn**. + +## Requirements / Task restatement + +Issue #123. The two status endpoints load every translation job the plugin has ever written — +`findRawJobs` passes `pagination: false` with one condition — then narrow by collection and by +document **in memory**. Retention was declined as the host's job (#122, closed by PR #129), so the +table never shrinks and the cost of opening a status panel grows with the project's whole history. + +Two things to build, and they are independent: + +**Stage A — send the narrowing to the database.** Both reasons for filtering in memory have expired, +measured on all three adapters. Pure fix: no new surface, no behaviour change. + +**Stage B — give the panel a batch.** `Translated — N` means "of the submission you just made, this +many are done", not a lifetime total. That requires a batch identity, which nothing stores today. + +## What already exists — platform · installed · project · verdict + +- **Platform (Payload 3.90.1).** `payload.find` takes `where` on a path inside the job's `json` + input, plus `sort`, `limit` and `totalDocs` — measured working on SQLite, PostgreSQL and MongoDB. + Nothing hand-written is needed to narrow a read. The job row's `createdAt` is a real column. +- **Installed.** `zod` already generates the registered `Field[]` from one schema declaration + (`store/toPayloadFields.ts`), so a stored field is added once and reaches types, admin and + validation. `crypto.randomUUID()` is already the id source in `SyncTaskRunner.ts:33`. +- **Project.** `RequestScope` (`shared/payload/RequestScope.shapes.ts`) already carries + per-submission metadata — `requester` — from the HTTP handler through `enqueue` into the stored + job input as `requester_id` / `requester_collection`. A batch id is the same kind of value on the + same path. `visibleIds` (`features/_lib/collection-utils.ts:37`) already answers access control in + one query however many ids. +- **Verdict.** Nothing new is built for Stage A. Stage B adds one stored field, one optional + interface method and one response field; everything else rides existing plumbing. + +## Why Stage B was withdrawn + +Built, reviewed, and taken back out. The reason is not that it failed — every criterion was met on +three adapters — but that reviewing it exposed a design fault the Phase 2 gate had not seen. + +**`TaskRunner.latestBatch?()` cannot be implemented from the contract that declares it.** A batch is +a three-step protocol: the enqueue handler mints an id onto `RequestScope.batch`; `queueWorkflow` +writes it into the **stored job input**; `latestBatch` reads it back. Only the third step appears on +the interface. A third-party runner reading `TaskRunner` is told to "return the newest submission" +and is never told that submissions arrive on the scope and must be persisted. No wording fixes that: +the fix is to put storage into the contract, which is precisely the obligation the recorded +follow-up says the interface should stop making +(`2026-10-06-lifecycle-run-context.task.md`, "Rework `TaskRunner` into a small required core plus +declared optional capabilities"). + +**The root cause is placement, and it is mine.** A batch is the plugin's own notion — it exists so a +progress panel can say "12 of 30". I stored it inside `payload-jobs`'s input shape, and everything +downstream followed from that: a field in someone else's storage, a field on a public scope type, and +a query that only one runner can answer. + +**Where it belongs instead:** the plugin keeps batches itself — `batch → the handles it covers` — +since `enqueue` already answers with those handles (`EnqueueAssignment.handle`, #107). Then no runner +contract changes at all, and progress is an ordinary task query. That is a collection and a +migration, which makes it its own piece of work rather than a field in this one. + +**What ships instead:** Stage A alone — the query narrowing. It closes the unbounded *document* read +outright and divides the *collection* read by the number of translatable collections. The +collection's own archive is still read, so #123 is improved, not closed. That is honest and it +changes no contract: no interface, no stored shape, no response shape. + +## Decisions + +### D1 — the batch id rides on `RequestScope`, not on `RequestShape` or `TaskInput` + +**Chosen:** `RequestScope.batch?: string`, written into the stored input by `queueWorkflow`. + +**What lost, and why:** `RequestShape` (`model/planEnqueue.ts:7`) is what `pickHost` matches a live +run against — all five of `sourceLng`, `strategy`, `publishOnTranslation`, `requesterId`, +`requesterCollection` must agree for a request to join a running job (`planEnqueue.ts:37-48`). A +batch id there would make every new submission fail to match, so it would create a **new row** +instead of appending — which both contradicts the owner's decision that an append joins the batch +already running, and manufactures the very "new row becomes the newest batch" the decision avoids. +`TaskInput` lost because a batch is per submission, not per task: N copies that can disagree. + +**The constraint it cites:** `pickHost`'s equality test, and `extendJob`'s write +(`PayloadJobsTaskRunner.ts:153`) which spreads `current.input` — so an append preserves the row's +existing batch with **no code at all**, which is exactly the decided behaviour. + +### D2 — the collection endpoint gets batch progress from one optional `TaskRunner` method + +**Chosen:** `latestBatch?(collectionSlug): Promise<{ id: string; tasks: Task[] } | null>` — the +runner returns rows, the handler filters them through `visibleIds` and counts, exactly as it already +does for `docs`. + +**What lost, and why:** extending `TaskFilter` with `batch?: string` cannot answer "which batch is +the latest" — the id has to be discovered first, so it needs a second call anyway and a magic +`"latest"` string. Reading the job rows directly from the handler lost because the handler holds +only a `TaskRunnerFactory`; reaching into the payload-jobs store would run the dependency backwards. + +**The constraint it cites:** `TaskRunner` is exported from `src/index.ts` and implemented outside +the package, so the method is **optional** — `SyncTaskRunner` does not implement it and the endpoint +answers `batch: null`, which is honest there: that runner finishes every locale inside `enqueue`. + +### D3 — the minted id is a server-side numeric timestamp + +**Chosen:** `String(Date.now())`, minted in `enqueue-translation/handler.ts`. **Decided by the owner +at the gate**, against the recommendation below. + +**What lost, and why:** `crypto.randomUUID()` was recommended — ordering no longer needs to come +from the id (D1 means no submission becomes the newest batch without creating a row, and the newest +is read off the greatest `createdAt`, a real column), so uniqueness was the only requirement left, +and two submissions in the same millisecond cannot collide under a UUID. The owner chose the +timestamp for legibility: a batch id that says when the submission happened is readable in the +admin's job list without a lookup. + +**The accepted cost, stated so it is not rediscovered as a defect:** two submissions reaching the +server in the same millisecond share a batch id and their panels show a common denominator. The +owner weighed this on 2026-10-12 — the only minting caller is a person pressing a button, and the +consequence is a wrong figure in a progress panel for the life of one batch, not corrupted data. + +**What is NOT accepted, and why the server mints:** a timestamp from a *browser* clock would be +poison, because the panel is shared — a clock running fast pins the panel to one person's batch for +everyone, indefinitely. The mint stays server-side, on the same clock that writes `createdAt`. + +### D4 — `docs` is bounded to live rows + +**Chosen:** `get-collection-status` keeps the `docs` array but adds `completedAt: { exists: false }`. + +**What lost, and why:** returning the archive lost because nothing reads it — the panel counts, and +#103's reporter asked for "what is in flight across this collection" in those words. Dropping `docs` +entirely lost because the owner's rule is that the response may be narrowed, not gutted. + +### D5 — the document filter offers the id in both forms the store may hold + +**Superseded twice, and the final answer is the simple one.** Worth keeping the path, because each +step was a measurement correcting the one before. + +1. The research doc said matching the legacy `input.collection.value` fails on SQL and works on + MongoDB. +2. A Phase 3 probe corrected that: the failure depends on the **stored type**, not the adapter. The + legacy relationship wrote a numeric collection's id as a **number**; comparing it against the + string the plugin holds matches on **no** adapter, MongoDB included. On that reading the filter + kept every legacy row and matched it in memory. +3. Asked why the narrowing was not simply done in the query, a third probe answered it: offering + **both forms** — `{ in: ["5", 5] }` — matches whichever the store holds, on all three adapters. + +**Chosen:** `ofDocuments` offers each id as a string and, when it is numeric, as a number too, on +both the flat `input.collection_id` and the legacy `input.collection.value`. The whole document +narrowing is therefore done by the database. + +**What lost, and why:** keeping every legacy row and matching it in memory (step 2) lost because it +left the read bounded only by collection for any project with pre-migration rows — a real cost, paid +for a limit that does not exist. + +**What this leaves in memory:** the `readCollectionRef` pass is no longer load-bearing. It now only +catches a row carrying **both** reference shapes with different values, which nothing writes. It is +kept so that `readCollectionRef` stays the single authority on which collection and document a row +belongs to, with the query as the narrowing rather than the decision. + +### A note on where Stage A is proven + +`ofCollection` / `ofDocuments` return a `Where` — data handed to Payload. A unit test over them +could only restate the literal they build, which proves nothing about whether the database agrees. +They are therefore proven by the integration criteria AC1–AC4 against real adapters, and carry no +unit test of their own. Recorded so a later reviewer does not read the absence as an oversight. + +### D6 — "newest submission" is the greatest batch id, found over a bounded window + +**This overturns a decision taken at the Phase 2 gate.** That decision said the newest batch could be +read off the row with the greatest `createdAt`, justified by the append case: a submission that only +appends creates no row, so it cannot displace the batch on show. That reasoning is sound and still +holds — but it is not the whole case, and a reviewer found the half it missed. + +**The disproof, reproduced on all three adapters.** `enqueue` queues a submission's documents in +sequential groups of `ENQUEUE_CONCURRENCY`, so a large submission goes on writing rows for seconds. +A smaller submission made *later* finishes writing first. When the large one's last rows land they +carry the greatest `createdAt`, and from then on — permanently, since no further writes follow — the +panel shows the **earlier** submission. The later submitter never sees their own work. + +``` +earlier=1791884660017 later=1791884660028 shown=1791884660017 SQLite +earlier=1791884662976 later=1791884662997 shown=1791884662976 PostgreSQL +earlier=1791884665909 later=1791884665930 shown=1791884665909 MongoDB +``` + +**Chosen:** newest means the **greatest batch id** — the moment the submission was made — taken in +memory over the newest `RECENT_BATCHED_ROWS` (500) rows that carry one. Decided by the owner: "Мы же +пишем в качестве номера партии таймстемп. Ну вот по нему и брать все записи с самым свежим +таймстемпом." + +**What lost, and why.** Sorting on the id itself (`sort: '-input.batch'`) is the obvious answer and +was **measured not to work**: PostgreSQL orders correctly, SQLite and MongoDB silently return the +wrong row. Taking the greatest id among *live* rows only is exact and needs no window, but the panel +would go blank the moment a submission finished — losing the "30 of 30" the owner asked for. + +**What the window costs, stated so it is not rediscovered as a defect:** a submission of more than +500 documents can still hide a later, smaller one. The read stays bounded and constant whatever the +collection's history, which is what #123 is about. + +### Placement + +| Piece | Lands in | +|---|---| +| collection + document conditions | `payload-jobs-runner/PayloadJobsTaskRunner.ts` — `findByCollection` | +| the `where` for the two stored reference shapes | a new unit beside `readCollectionRef.ts` in `store/` — it is knowledge of how Payload stores the reference | +| `batch` on the scope | `shared/payload/RequestScope.shapes.ts` | +| `batch` as a stored field | `store/jobInput.schema.ts` → `WorkflowInputSchema` | +| minting | `features/enqueue-translation/handler.ts` | +| `latestBatch` | `TaskRunner.interface.ts` + `PayloadJobsTaskRunner.ts` | +| counting + visibility | `features/get-collection-status/handler.ts` | + +`store/` holds "the shape of what a framework stores and the readers that turn it into ours" per +`CLAUDE.md`, which is what a `where` over two storage shapes is. `model/` must not import it back. + +### New surface + +- `RequestScope.batch?: string` — callers: `enqueue-translation/handler.ts` (writes), + `PayloadJobsTaskRunner.queueWorkflow` (reads). +- `TaskRunner.latestBatch?()` — callers: `get-collection-status/handler.ts`, and + `PayloadJobsTaskRunner` implements it. One caller today; it is a method on an interface that + already exists, not a new abstraction, and it has no inline alternative (D2). +- A `where` builder for the collection reference — callers: `findByCollection`, and the document + narrowing in the same method. + +### Written contract? + +**Yes, three things no signature states**, so `latestBatch` takes the red-test path in Phase 3: + +1. `latestBatch` returns `null` both when the runner has no notion of batches and when the + collection has none — the caller cannot tell those apart and must not need to. +2. The batch id is opaque. Callers compare it and hand it back; they never parse it. +3. An append to a live run does not change that run's batch, so a submission that only appends + leaves the newest batch where it was. + +### Escalate to /sp-architect? + +**No.** Stage B adds a field to an existing stored shape and an optional method to an existing +interface; the placement is named above and follows `CLAUDE.md`'s `store/` vs `model/` split. No new +seam, no new dependency, no migration (the column is `json`). + +## Acceptance Criteria + +| AC | Covers | Criterion | How it is checked | Passes when | +|---|---|---|---|---| +| AC1 | R1, R2 | `findByCollection` sends a collection condition; a read for collection X loads no row of collection Y | new integration test, two collections, asserting rows returned by the runner | the case runs green on sqlite, postgres and mongo | +| AC2 | R2, R7 | A row carrying the legacy `input.collection` relationship is still found | new integration test seeding one row of each shape | both rows come back, all three adapters | +| AC3 | R1, R2 | `get-document-status` loads only that document's rows | new integration test with two documents in one collection | only the asked document's rows are read | +| AC4 | R4, R5 | A document whose French run is older than ten German runs still reports French | new integration test with a skewed history | French appears in the answer | +| AC12 | — | No new type errors | `bun run check-types` from the package | no error naming a changed file | +| AC13 | — | No new lint findings | `bunx oxlint .` from the package | 47 warnings, 0 errors, or fewer | +| AC14 | — | Nothing already passing regressed | `bunx vitest run`; `bun run test:integration:sqlite` from `apps/dev` | ≥ 2057 unit tests and ≥ 224 integration tests pass | + +## Pre-flight + +Run against the untouched tree on `feat/translator-bound-status-reads`, before any edit. + +| AC | Command | Result | Classification | +|---|---|---|---| +| AC1–AC4 | the named tests | **do not exist yet** | change — each gets its red run in Phase 3, recorded with the failure reason | +| AC12 | `bun run check-types` | passes, 0 errors | invariant | +| AC13 | `bunx oxlint .` | 47 warnings, 0 errors | invariant | +| AC14 | `bunx vitest run` | 148 files, 2057 tests passed | invariant (baseline) | +| AC14 | `bun run test:integration:sqlite` | 58 files passed / 4 skipped, 224 passed / 7 skipped | invariant (baseline) | + +## Risk notes + +**Risk classification: HIGH**, and the reasons are specific rather than ceremonial: + +- `TaskRunner`, `TaskFilter` and `RequestScope` are all re-exported from `src/index.ts` and + implemented outside the package. Stage B touches all three. +- Stage B changes a **stored** shape and two endpoint responses. +- `findByCollection` is on the enqueue path as well as the status path, so Stage A's condition is + load-bearing for cancellation and supersede logic, not only for display. + +What that binds downstream: a test is required before any part is called done; the Phase 4 +regression sweep is mandatory and must name what was grepped and how many call sites were read; +Phase 5 runs three review angles rather than one. + +**Specific hazards:** + +- `ownJobs()` is an `or` because a workflow row carries no `taskSlug`. Every new condition is + **added to** it, never substituted — a replacement silently returns nothing. +- `visibleIds` runs **after** the read in both endpoints. Narrowing by ids must not become a way to + skip it; the order stays. +- The legacy `input.collection` branch leaves at the next major. The `or` must be written so that + deleting one branch is the whole change. +- A submission whose every locale is an append mints an id that names no row. The panel is correct + (it shows the running batch, per AC6), but the returned id is inert. Recorded rather than fixed — + nothing reads it today. + +## Human choices + +- **2026-10-12** — "Я всё еще хочу показывать статус по всей коллекции а не тольк по дументам на + странице, это буде твводить в заблужение." Narrowing the panel to the admin list's current page is + rejected. +- **2026-10-12** — "translated по идее должен показывать количество переведенных записей не вообще + за всё время, а за текущий run." The `Translated` figure is the current submission. +- **2026-10-12** — "16. идет в текущую партию" — an append-only submission joins the batch the live + row already carries; the row is not re-stamped. +- **2026-10-12** — "17. по нулям" — with no batch, the panel shows zeros; there is no fallback to + displaying live work. +- **2026-10-12** — "18. это сейчас разве нужно?" — a global multi-collection dashboard is out of + scope; nothing is built for it. +- **2026-10-12** — scope: both stages in one task rather than narrowing first. The whole diff is + therefore graded at HIGH risk, including Stage A. +- **2026-10-12** — the batch id is a numeric timestamp, not a UUID: "Числовой момент времени, как + решили раньше". See D3 for the cost that choice accepts. +- **2026-10-13** — `batch` accepts `boolean | string`, not `true | string`: `false` must mean "track + nothing" rather than a 400, so a caller computing the flag is never rejected. Measured: the + narrower schema rejected `false`. +- **2026-10-13** — a batch with no visible documents keeps answering with its id; see D7. +- **2026-10-13** — nothing goes in the README: "`batch: true` - это не публичный контракт как и + эндпоинты… Где у пользователя плагина доступ к этому свойству?" See D8. +- **2026-10-12** — "контракт эндпоинтов не публичный. То, что кто-то строит на нем свою админку его + проблемы. Мы его полностью ломать не будем, но учитывать этот факт стоит." + +## Red runs and mutations + +**Red run** — `docs/plans/artifacts/2026-10-13-bounded-status-reads-red-run.txt`. Two change-criteria +failed for their own reason (another collection's rows loaded; another document's rows loaded); the +two invariant criteria in the same file passed, as they must. Two criteria were *broken* rather than +red on the first attempt — a wrong response shape and an invalid `workflowSlug` — and were fixed and +re-run rather than counted. + +**Mutations, one per claim, each reverted:** + +| Mutation | Went red | Proves | +|---|---|---| +| `ofCollection` matches any row instead of one collection | "loads no job row belonging to another collection", alone | the collection narrowing is guarded | +| `ofDocuments` matches the legacy document id | "still finds a legacy row when asked about one document", on SQLite and PostgreSQL | D5 is guarded where the distinction exists; on MongoDB ids are not numeric, so the case cannot arise and the test says so | +| `extendJob` re-stamps the appended row's batch | "leaves a running submission's id alone…", alone | the owner's append decision is guarded | + +The first run of the second mutation turned **nothing** red. That was the finding that sent me back +to measure, and it is what corrected D5 — the fixture had to seed a number-stored id before the test +could discriminate. + +## Phase 4 — regression sweep + +Every public surface this diff touches, and what was read: + +| Surface | Verdict | +|---|---| +| `TaskRunner` (public, implemented outside the package) | gained an **optional** method; a third-party implementation that compiled before still compiles | +| `RequestScope` (public) | gained an **optional** field; additive | +| `TaskFilter` (public) | unchanged | +| client status types | **not** public — `GroupedCollectionTranslationStatus` was never re-exported from `src/index.ts`, so replacing it breaks no consumer | +| `findByCollection` — 3 callers, all read | `get-collection-status`, `get-document-status`, and `cancel-by-collection:30`. The third is the **destructive** one and was not in the original design; it passes no `documentIds`, so it reads the same row set as before, narrowed in the query instead of in memory. 13 unit + 5 integration cases green. | +| `jobInputFields.baseline.json` | changed by exactly one added field (`batch`, text, maxLength 256); the task list is untouched. It reaches a host's generated types, so it was updated deliberately. | + +### D7 — a batch with no visible documents still answers with its id + +Raised by a reviewer and **declined by the owner**: when a caller may read none of a batch's +documents, `GET /collection/:slug` still answers `batch: { id, 0, 0, 0, 0 }` rather than `null`, so +that caller can tell "a submission happened, not for me" from "no submission happened" — and the id +is a timestamp, so they learn when. + +**Decided: leave it.** "Если у пользователя есть как минимум права на чтения коллекции — то и +количество тогда он сможет прочитать. Если прав нету — то пользователь просто не сможет открыть +коллекцию." + +**One correction to that reasoning, recorded because it is load-bearing if the decision is ever +revisited.** The endpoint has **no collection-level read check of its own**. Two gates stand in +front of it: the host-configured `AccessGuard` (`withAccessCheck` — "may this user use the translator +at all"), and `visibleIds`, which applies the collection's rules per document. So the owner's +reasoning holds for the admin UI, where Payload hides a collection the user cannot read — but the +route can be called directly, and then only the host's guard applies. + +What leaks in that case is one fact with no document, locale or name attached: that a submission +happened, and when. A host whose guard is coarse has made that choice for the whole translator +surface, not for this field. + +### D8 — nothing from this work goes in the README + +`CLAUDE.md`'s "Documenting feature versions" asks for a `Since vX.Y.Z` note in the README beside new +**public API**, which it defines as anything re-exported from `src/index.ts`. Measured against that +definition, this work adds none: + +| Thing | Who can reach it | +|---|---| +| `batch` on `POST /translate/enqueue` | nobody outside the package. The only caller is the plugin's own panel, `BulkTranslationDashboard.tsx:84`. | +| `scope.batch` | written in one place, `PayloadJobsTaskRunner.ts:193`. A consumer has no way to set it. | +| the `{ batch, docs }` response | the HTTP contract, which the owner has ruled is not public surface. | +| `TaskRunner.latestBatch?()` | genuinely reachable — by someone writing their **own** runner. That reader is in the interface's JSDoc, not the README, and the method carries `@since 0.17.0` there. | + +**Decided: no README change.** Documenting the enqueue input or the response shape would publish an +internal contract and invite consumers to depend on it — the opposite of the owner's position that +the endpoints are not public. `src/index.ts` is unchanged, so the convention's own trigger never +fires. + +## Review log + +**2026-10-13 — three reviewers, one per angle (high risk).** + +| Angle | Result | +|---|---| +| correctness of the narrowing | no findings. Traced every row shape through `ofCollection` / `ofDocuments` and the in-memory pass, confirmed `ownJobs()` is combined rather than replaced on all four query paths, and checked `exists` on a nested JSON path in the adapters' own source. | +| the batch feature | **one major finding, confirmed and fixed** — see D6. One question raised and put to the owner: a caller who may read none of a batch's documents still receives the batch id with zero counts. | +| contract and tests | contract clean: the optional method and optional field are additive, `src/index.ts` untouched, ADR-0002 satisfied. Five test gaps; two (AC11, AC8a) had already been closed while the reviewer ran, two more closed after it (`latestBatch` unit coverage; the join-by-id path, now covered by the D6 regression test). The fifth is the README note, which is the owner's to write. | + +Mutations added for D6: restoring `limit: 1` turns "shows the submission made last, not the one whose +row landed last" red, alone, and nothing else. diff --git a/packages/payload-plugin-translator/docs/plans/2026-10-12-does-177-earn-its-closes.md b/packages/payload-plugin-translator/docs/plans/2026-10-12-does-177-earn-its-closes.md new file mode 100644 index 000000000..21d52d924 --- /dev/null +++ b/packages/payload-plugin-translator/docs/plans/2026-10-12-does-177-earn-its-closes.md @@ -0,0 +1,143 @@ +# Does #177 earn its four `Closes` keywords? + +Research, 2026-10-12. Read-only: no code touched, no issue changed, no commit made. + +## The question + +PR #177 carries `Closes #107, closes #110, closes #106, closes #103`, so merging it closes all four +automatically. Before that happens: is each of them genuinely answered, deliberately declined, or +quietly skipped? + +Verdicts use three values only — **closed** (the need is met), **declined on the record** (not met, +and the decision is written down with its alternative), **skipped** (not met and nowhere mentioned). + +## Sources read + +- The four issues in full, with their bodies as filed (all four say "Read against **0.10.1**"). +- `docs/plans/2026-10-06-lifecycle-run-context.task.md` — the contract for the original work, whose + `## Human choices` carries D1–D10 and whose `## Review log` carries an earlier deep review of this + same question. +- The code on `feat/translator-lifecycle-run-context`, read on disk (byte-identical to the branch). +- The tests named below, each opened rather than cited from the contract. + +## Codebase map + +| What an issue asks about | Where it lives now | +|---|---| +| the enqueue answer | `src/server/features/enqueue-translation/handler.ts:101`, shaped by `model.ts` → `toAssignmentOutputs` | +| the collection-status answer | `src/server/features/get-collection-status/handler.ts:45` | +| the host-facing callbacks | `src/server/modules/lifecycle/types.ts` | +| what a callback carries | `src/server/modules/lifecycle/taskMapping.ts` → `taskFromAssignment` | +| whether a second request starts a second run | `payload-jobs-runner/model/planEnqueue.ts`, `model/alreadyCovered.ts` | +| whether enqueue still supersedes | `payload-jobs-runner/PayloadJobsTaskRunner.ts`, pinned by `__tests__/PayloadJobsTaskRunner.test.ts:241` | + +## Issue by issue + +### #107 — "enqueue should return the job ids it created" → **closed** + +The answer now carries one entry per requested locale, each naming the run: + +``` +assignments: [{ collection_slug, collection_id, target_lng, handle }] +``` + +The issue's *suggested shape* was a locale-keyed map, `jobs: { fr: "…", de: "…" }`. That was +rejected on the record and the rejection holds up: the request accepts an **array** of documents +(`enqueue-translation/model.ts:15`, `collection_id: z.array(...).nonempty()`), so a map keyed by +locale collapses several documents onto one key. The flat list answers the stated need — "the ids +exist at that point; they are just not returned" — and names the document as well, which the map +could not. + +Proof: `apps/dev/src/integration/translator/enqueue-handles.int.test.ts`, which asserts one answer +per requested locale naming a run that really exists. + +### #103 — "collection status should return document id and target locale" → **closed** + +`get-collection-status/handler.ts:45` now maps each task to +`{ id, status, collection_id, target_lng }`, pinned by `handler.test.ts:144–146` with three entries +across two documents. + +The issue wrote `collectionId` / `targetLng` because it was naming the internal `Task`'s fields. The +wire format of this plugin is snake_case throughout — the same endpoint family takes `source_lng`, +`target_lng`, `collection_slug` on the way in — so the delivered names are consistency with the +surface the issue is about, not a substitution. The issue's own framing, "a response-mapping change, +not new plumbing", is exactly what happened. + +### #110 — "pass run context into lifecycle callbacks" → **two parts declined on the record, the need met; one part narrowed** + +Three distinct asks, and they ended differently. + +**The opaque `context` → declined, by the issue's own alternative.** The plugin does not accept a +host value and hand it back. It generates a run handle instead and puts it on every callback, +`onQueued` included. The issue offers this itself: *"A run id generated by the plugin and passed to +callbacks would also solve it."* Recorded as D1 with the rejected alternative. + +**The attempt counter → declined by name, and the need behind it met another way.** The contract +says so plainly: *"Rejected: an `attempt` counter, which is what #110 asks for by name. It lets a +host tell the events apart but still cannot say whether another try is coming, so it does not let +them turn retries back on — it answers the request, not the need."* The need was "must run with +`retries: { attempts: 0 }`". `onFailed` now fires once, and only when the locale will not be +translated — so retries can be switched back on. Proof: +`final-failure.int.test.ts:63`, "says nothing while the run is still retrying, and reports success +once it works". + +**"Removes the need for the single-open-run constraint" → narrowed, not erased, and recorded.** The +host no longer has to enforce that constraint to make its join unambiguous — the handle does that. +But the plugin still coalesces: a second request for a locale a live run already covers joins that +run and is answered with its handle (`model/alreadyCovered.ts`). So the issue's specific complaint — +*"a host cannot run two translations of the same pair concurrently even where that would be +harmless"* — still stands. This is D8, in the contract's own words: *"Semantically right — there was +one translation — but it means #110 is narrowed rather than erased, and the README should say so."* + +### #106 — "an onCancelled lifecycle callback" → **closed, and one of its two requirements has expired** + +`onCancelled(task)` exists (`lifecycle/types.ts:69`), fires once per locale the run still owed, and +fires **before** the row is deleted — which is what the issue asked for so a host can still read the +row. + +The `reason` argument was not delivered. That is not a gap: the issue names two paths, and the +second one no longer exists. Verified independently rather than taken from the contract — +`__tests__/PayloadJobsTaskRunner.test.ts:241`, "never cancels or deletes a job when enqueuing", is a +regression test asserting the plugin's own supersede path is gone. With explicit cancellation the +only path left, `reason` would carry one value. + +So the issue's premise *"the plugin cancels jobs on its own initiative and tells nobody"* is stale +relative to the branch, and saying `reason` is missing would be scoring the letter over the fact. + +## Is any of the four skipped? + +**No.** Every requirement is either met, or declined with its reason and its rejected alternative +recorded in `2026-10-06-lifecycle-run-context.task.md` under `## Human choices`. + +## The one thing that should block a clean conscience at merge + +**The README still documents the old behaviour, and it is the document a consumer reads.** + +`README.md:419` says today: + +> `onQueued` fires once; `onCompleted` / `onFailed` fire per execution attempt, so a retried task can +> emit `onFailed` more than once. + +That is precisely what #110 complained about and what this PR changed. The code says the opposite +(`lifecycle/types.ts:37`, "Each fires once per target locale, never per attempt"). The README also +does not mention the handle on `TranslationTask`, nor `onCancelled`, nor the coalescing that D8 says +it should state. + +So: the four issues are answered in the code and contradicted in the docs. A consumer who reads the +README after this merges would still believe they must run with retries disabled — the exact +condition #110 was filed to lift. + +`README.md` is the owner's to edit by standing agreement, which is why it is reported rather than +changed. It is the only outstanding item this research found. + +## Open questions + +1. **[non-blocking] Does the consumer who filed #110 accept "onFailed is final" in place of an + attempt counter?** Matters because: if they wanted per-attempt visibility for a progress display + rather than for retry control, the need is not met and the issue should stay open. The contract + already carries this as a handoff item, never scored. +2. **[non-blocking] Is the coalescing in D8 acceptable to them?** Matters because: #110's text calls + the single-open-run constraint "a real constraint we would rather not need". It is still there, + imposed by the plugin instead of by them. + +Both are questions for the consumer, not for the code, and neither changes a verdict above. diff --git a/packages/payload-plugin-translator/docs/plans/2026-10-14-one-answer-to-did-this-run-fail.task.md b/packages/payload-plugin-translator/docs/plans/2026-10-14-one-answer-to-did-this-run-fail.task.md new file mode 100644 index 000000000..ed5d960ad --- /dev/null +++ b/packages/payload-plugin-translator/docs/plans/2026-10-14-one-answer-to-did-this-run-fail.task.md @@ -0,0 +1,324 @@ +# One answer to "did this run fail" + +Follows: 2026-10-14-the-panel-shows-what-is-happening.task.md + +## Requirements / Task restatement + +Four defects found by five audits over this branch. Three are real behaviour; the fourth is a +check that cannot fail. + +1. **The status an editor sees and the window that ages failures out read different fields.** + `getJobStatus` (`store/normalizeJob.ts:10`) answers `failed` on `job.error`; `settledSince` + (`PayloadJobsTaskRunner.ts:59`) decides "gave up" on `hasError`. Payload writes them differently: + `handleWorkflowError.js:53-55` falls through its `if (!hasFinalError)` branch — that branch only + adjusts `waitUntil` — and reaches `updateJob({ error: errorJSON, hasError: hasFinalError })`. So + `error` is written on **every** failed attempt, `hasError` only on a final give-up. +2. **`findByCollection(slug, [])` answers with the whole collection** under the Payload runner + (`PayloadJobsTaskRunner.ts:297`, `documentIds?.length`), and with nothing under the sync runner + (`SyncTaskRunner.ts:105`, `documentIds`). +3. **`resolveSettings` accepts any number as `failedWindowMs`** (`settings.ts:80`). +4. **The invariant that forbids defect 2 cannot fail.** + +### Correction to the brief this task was given + +The brief said the shared conformance suite is not run for `PayloadJobsTaskRunner`. **That is +wrong.** `apps/dev/src/integration/translator/task-runner-contract.int.test.ts` already calls +`assertTaskRunnerContract("PayloadJobsTaskRunner", …)` against a real Payload, on all three +adapters. The actual defect is narrower and worse: the case `"answers about no documents, not about +all of them"` (`TaskRunner.invariants.ts:143`) sits inside `describe("an empty request is answered, +not refused")`, where nothing has been enqueued. `findByCollection(SLUG, [])` therefore returns `[]` +for every implementation — including one that does nothing. The obligation is written down and +**green for both runners for the wrong reason**. + +## What already exists — platform · installed · project · verdict + +- **Project.** `PayloadJobsRunnerProvider.ts:88` already validates a millisecond setting exactly as + defect 3 needs: `if (!Number.isFinite(staleJobTimeoutMs) || staleJobTimeoutMs <= 0) throw new + TranslatorConfigError(...)`. `TranslatorConfigError` lives at `core/errors/TranslatorConfigError.ts` + and is deliberately not exported from `src/index.ts`. +- **Project.** The shared conformance suite and its two call sites already exist and already run on + three adapters. Nothing new is needed to reach both runners. +- **Verdict.** No new file, no new dependency, no new abstraction. + +## Decisions + +### D1 — `getJobStatus` answers `failed` on `hasError` + +**Chosen:** `if (job.hasError) return "failed";` in place of `if (job.error)`. + +**What lost, and why:** + +- `if (job.hasError && job.error)` — the second conjunct can never be false. Every site that sets + `hasError: true` writes `error` in the same call (`handleTaskError.js:59` and `:112`, + `handleWorkflowError.js:54-55`), and `clearPickerBlockers` clears both together + (`PayloadJobsTaskRunner.ts`). A conjunct that cannot discriminate is noise. +- **Move the window to `error` instead of moving the status to `hasError`.** Lost on a requirement + already settled: `excludeGivenUpBefore`'s docblock states *"Runs still in flight are kept however + old"*. Keying the window on `error` would drop rows that are still retrying — hiding live work, + which is the opposite of what the panel is for. + +**The constraint it cites:** `node_modules/payload/dist/queues/errors/handleWorkflowError.js:53-55`, +read in the installed version. + +**What this changes for readers of `Task.status`** — 11 consumers, 4 affected, all only for a run +**between** attempts: + +| Consumer | Before | After | +|---|---|---| +| `useDocumentTranslation.ts:51` (polling) | `failed` → polling stops, panel freezes until reload | `pending` → polling continues | +| `get-document-status/model.ts:89`, `get-collection-status/handler.ts:50` | `failed` | `pending` | +| `statusRows.ts:94` | shows the attempt's error message | **shows nothing** — a real loss, accepted | +| `cancel-by-collection:35`, `stillOwed.ts:10`, `SyncRunnerProvider.ts:29` | unaffected — they read `running` / `!== completed`, and the sync runner sets status directly | unchanged | + +### D2 — the vacuous invariant moves to where rows exist, rather than a new check elsewhere + +**Chosen:** move `"answers about no documents, not about all of them"` out of the empty-request +block and into `describe("findByCollection finds tasks for a collection")` +(`TaskRunner.invariants.ts:439`), enqueueing a row for `SLUG` first — the idiom every sibling case +there already uses (`documentForThisCheck(n)`). + +**What lost, and why:** adding a case to `PayloadJobsTaskRunner.test.ts` against the mock Payload. +It lost twice: it would cover one runner in one adapter-free unit test where the shared suite covers +both runners on three adapters, and it would leave the vacuous case in place, still unable to fail. + +**The constraint it cites:** `apps/dev/src/integration/translator/task-runner-contract.int.test.ts` +already wires the suite to `PayloadJobsTaskRunner`. + +### D3 — `failedWindowMs` is validated where it is resolved, and zero stays legal + +**Chosen:** in `resolveSettings`, `!Number.isFinite(ms) || ms < 0` → `TranslatorConfigError`, +worded like the `staleJobTimeoutMs` precedent. + +**What lost, and why:** `<= 0`, matching that precedent exactly. It lost to a decision already +recorded in this package: `settings.test.ts` carries `"keeps a window of zero instead of reading it +as unset"`, whose stated reason is *"zero means report no failures at all — a real choice, not an +omission"*. Rejecting `0` would break a documented choice to buy symmetry with an unrelated setting. + +### D4 — the `documentIds` guard tests presence, not length + +**Chosen:** `PayloadJobsTaskRunner.ts:297` becomes `documentIds ? new Set(documentIds.map(String)) : undefined`, +matching `SyncTaskRunner.ts:105` character for character. + +**What lost:** keeping `?.length` and special-casing the empty list later. It lost because the two +runners would still differ in shape, and the shared suite grades them against one sentence. + +### Placement + +| Piece | Lands in | +|---|---| +| `hasError` in place of `error` | `payload-jobs-runner/store/normalizeJob.ts` | +| the empty-list guard | `payload-jobs-runner/PayloadJobsTaskRunner.ts` | +| `failedWindowMs` validation | `src/settings.ts` | +| the invariant moved + a new `excludeGivenUpBefore` case | `task-runner/__tests__/TaskRunner.invariants.ts` | +| `seedRun` learns to write `error`; two cases assert status | `apps/dev/src/integration/translator/panel-shows-what-is-happening.int.test.ts` | +| the tautological check deleted | `src/settings.test.ts` | + +### New surface + +**None.** No new exported symbol, no new file. + +### Written contract? + +**None new.** `TaskFilter`'s docblock already states what `excludeGivenUpBefore` means; D1 makes the +code agree with it rather than changing what is owed. No `/sp-red-test` path — this is a bug fix, so +the failing test comes first against the broken code (Phase 3, path A). + +### Escalate to /sp-architect? + +**No.** No new seam, no dependency, no data-model change. It makes two existing readers agree on one +field. + +## Acceptance Criteria + +| AC | Criterion | How it is checked | Passes when | +|---|---|---|---| +| AC1 | A run between attempts (Payload writes `error`, no `hasError`) reports `pending` | the case `"treats a run that is still retrying as in flight, not as failed"`, with `seedRun` writing `error`; `bun run test:integration:{sqlite,postgres,mongo}` | green on all three | +| AC2 | A run that gave up (`hasError` + `error`) reports `failed` | the case `"names a run that failed inside the window"`, asserting `status`, same three runs | green on all three | +| AC3 | `findByCollection(SLUG, [])` answers `[]` **while a row for `SLUG` exists** | the moved invariant case — package `bunx vitest run` for `SyncTaskRunner`, the three integration runs for `PayloadJobsTaskRunner` | green for both runners | +| AC4 | The shared suite states what `excludeGivenUpBefore` owes | new case in `TaskRunner.invariants.ts`; same two routes | green for both runners | +| AC5 | `failedWindowMs: NaN` is refused at config time, named | new check in `src/settings.test.ts` expecting `TranslatorConfigError` | throws, message names `failedWindowMs` | +| AC6 | `failedWindowMs: 0` still resolves to `0` | the existing check `"keeps a window of zero…"` | unchanged, green | +| AC7 | The tautological check is gone | `grep -c "defaults the failure window to three days" src/settings.test.ts` | `0` | +| AC8 | No new type errors | `bun run check-types` here and in `apps/dev`; `bunx tsc --noEmit -p tsconfig.json` in `apps/dev` | 0 · 0 · 2 (both pre-existing, untouched files) | +| AC9 | No new lint findings | `bunx oxlint .` here, `bunx oxlint src` in `apps/dev` | ≤ 46 / 0 errors · ≤ 5 | +| AC10 | Nothing else regressed | `bunx vitest run`; the three integration runs | ≥ 2096 unit (2097 less the deleted tautology, plus what is added); ≥ 235 / 240 / 233 | + +## Pre-flight — every check run against the untouched tree + +| AC | Result now | Classification | Evidence | +|---|---|---|---| +| AC1 | **FAILS** | change | `expected [ 'failed' ] to deeply equal [ 'pending' ]` — measured by seeding `error` the way Payload writes it | +| AC2 | **FAILS** | change | `expected [ 'pending' ] to deeply equal [ 'failed' ]` — measured by adding the status assertion | +| AC3 | **FAILS** for `PayloadJobsTaskRunner` | change | `expected [ { id: 'job-1', …(7) } ] to deeply equal []` — the invariant applied to that runner with a row present | +| AC4 | **FAILS** | change | `grep -c excludeGivenUpBefore TaskRunner.invariants.ts` → `0` | +| AC5 | **FAILS** | change | `NaN ?? default` → `NaN`; `new Date(Date.now() - NaN).toISOString()` → `RangeError: Invalid time value` | +| AC6 | passes | invariant | the check exists and is green | +| AC7 | **FAILS** | change | the grep returns `1` | +| AC8 | 0 · 0 · 2 | invariant | baseline | +| AC9 | 46 / 0 · 5 | invariant | baseline | +| AC10 | 2097 · 235 / 240 / 233 | invariant | baseline | + +Seven criteria fail now and must pass; three are invariants. No criterion already passes for the +thing it is meant to prove. + +## Risk notes + +**Risk classification: HIGH.** D1 changes a value that two HTTP bodies and four client readers +consume, including the one that decides whether the admin panel keeps polling. The change is +correct-by-measurement but its blast radius is the whole status surface. + +What that buys downstream, and will be honoured: `AskUserQuestion` at this gate; a failing test +before the fix, not after; an **evidenced** regression sweep in Phase 4 naming what was grepped and +how many call sites were read; three review angles in Phase 5b. + +**Hazards:** + +- **A real loss, accepted:** `statusRows.ts:94` shows `run.error?.message` only when the status is + `failed`. After D1 a run between attempts stops showing its error text. The editor sees "queued" + where they used to see the transient error. This is the price of removing the false alarm, and it + is the one user-visible regression in the change. +- The sibling cases in the `findByCollection` block are written as *filter out the wrong ones and + expect `[]`* — a shape that also passes on an empty result. The case this task moves there must + **not** copy that shape; it asserts the whole answer. The sibling weakness is out of scope and + recorded here rather than fixed. +- `seedRun` writes through `payload.db.updateOne` because `payload.update` rewrites `updatedAt`. + Teaching it `error` must not route the row back through `payload.update`, or every window test + starts passing for the wrong reason. + +## Human choices + +- **2026-10-14** — change the behaviour rather than leave the mismatch: the owner answered "да" to + changing what every reader of `Task.status` sees, after being shown that the panel reports a false + alarm and that such a row never ages out of the window. +- **2026-10-14** — scope held to four defects. The audits also found: the client widget chain + relaying `targetSelection` through 20 sites, `PayloadJobsTaskRunner.test.ts` holding 18 doubles + over 887 lines, `GetCollectionStatusOutput` bound to nothing, 24 comment verdicts, and a missing + README line. All deliberately left out as separate passes. + +- **2026-10-14** — the lost error text is accepted rather than compensated. Offered the alternative + of also showing `run.error?.message` on a `pending` run in `statusRows.ts`; the owner chose to keep + the scope at four defects and leave the client untouched. The transient error text disappears from + the document panel; it is shown today only under a label (`Failed`) that is itself the false alarm + this task removes. + +## Verification — 2026-10-14 + +| AC | Verdict | Evidence | +|---|---|---| +| AC1 | met | red before: `expected [ 'failed' ] to deeply equal [ 'pending' ]`; green after on sqlite · postgres · mongo | +| AC2 | met — **but the pre-flight was wrong**, see below | green before and after | +| AC3 | met | red before, through the shared suite against a real PostgreSQL: `expected [ { id: '22', …(31) } ] to deeply equal []` — 32 rows where none were owed. Green now for both runners | +| AC4 | met | the shared case is green for both runners and renamed to what it guards; the give-up branch it could not reach under `SyncTaskRunner` is now covered directly in `SyncTaskRunner.test.ts`, with both sides proven by mutation | +| AC5 | met | red before: `expected [Function] to throw an error`, three times; green now | +| AC6 | met | `"keeps a window of zero instead of reading it as unset"` unchanged and green | +| AC7 | met | `grep -c` → `0` | +| AC8 | met | package 0 · `apps/dev` check-types 0 · `apps/dev` tsconfig.json 2 (`payload.config.ts:166`, `vitest.integration.config.ts:21`, both pre-existing) | +| AC9 | met | `Found 46 warnings and 0 errors` · `apps/dev` 5 warnings. `sp-lint-delta` two-run: *No findings introduced by this change* | +| AC10 | met | package **2103** passed / 151 files; sqlite 236 · postgres 241 · mongo 234, all above baseline | + +### Two corrections to this contract's own pre-flight + +**AC2 was classified `change` and is actually an invariant.** The pre-flight claimed it failed. It did +— but for a fixture reason, not for the defect: the measurement that produced +`expected [ 'pending' ] to deeply equal [ 'failed' ]` added a status assertion while leaving the old +`seedRun`, which wrote no `error` at all. With a fixture that writes what Payload writes, a run the +queue gave up on carries **both** `error` and `hasError`, so the old rule answered `failed` too. AC2 +guards against the fix over-correcting; it never could have proved the fix was needed. + +**D4 needed more than the guard it named.** The decision said `documentIds ? … : undefined`, matching +`SyncTaskRunner` character for character. That alone sent `{ in: [] }` into the query through +`ofDocuments`, and **PostgreSQL rejected it** — error `42601`, `scanner_yyerror`: `IN ()` is a syntax +error there, while SQLite and MongoDB accept it. Found only by the full three-adapter run; the file +passed in isolation. The implementation therefore answers an empty list **without issuing a query**: + +```ts +const wanted = documentIds ? new Set(documentIds.map(String)) : undefined; +if (wanted?.size === 0) return []; +``` + +An empty list names no document, so the answer is empty by definition and no storage is involved. + +### Prevention — the sweep (high risk, so evidenced) + +Two greps over `src`, every match read: + +- **`hasError` readers — 7 matches.** The fix brings the one dissenter into line with code that was + already right: `model/owedOnCancel.ts:14` (`job.hasError === true ? [] : …`, whose docblock already + stated *"Payload leaves `hasError` set from that moment on"*) and `model/planEnqueue.ts:42` + (`job.hasError !== true`) both keyed on `hasError` before this change, as did `settledSince`. + `getJobStatus` was the only place reading `error` as proof of failure. +- **`error` readers — 34 matches**, of which 21 are zod validation results or React form state and are + unrelated. The rest were read individually: `normalizeJob.ts:43,83` carry the error *text* (correct + to key on `error` — the message exists from the first failed attempt), `normalizeJob.ts:44` and + `planEnqueue.ts:41` read cancellation out of the error payload, and `statusRows.ts:94` plus + `TranslationStatusList.tsx:189` are the accepted loss recorded under Human choices. + +Guardrails left behind: the moved invariant now holds **both** runners to the empty-list rule on three +adapters, and `"reads a run that failed an attempt but will retry as pending"` pins the status rule at +the unit level. + +### Comment gate + +One comment added by this task — the docblock on `failureShape` naming the two shapes Payload writes, +with its citation. Kept: the constraint itself is structural (the union `"gave-up" | "retrying"` makes +the invalid combination unexpressible), so the prose carries only the external fact. + +### Standing gate failure, unchanged + +`sp-diff-checks` reports one FAIL on `apps/dev/src/integration/translator/locale-workflow-failure.int.test.ts` +— one assertion line replaced (`["fr", "failed"]` → `["fr", "pending"]`). It predates this task and +belongs to the panel work; this task did not touch that file. After this change the assertion holds +for a second, better reason: the run-level rule now agrees with the locale-level one. + +## Review log + +**2026-10-14 · three angles, high risk.** + +- **Consumers of `Task.status`.** Walked all 11 independently and confirmed the list complete: the + only `status ===` match outside it, `TranslateFieldControl.tsx:107`, reads a different field. Four + consumers change behaviour and all four improve — `useDocumentTranslation.ts:51` resumes polling, + the two HTTP bodies stop reporting a false `failed`, and `panelStatus.ts` shows "running" instead + of a permanent red. Confirmed `SyncTaskRunner` needs no matching change: it holds its own in-memory + `Task`, has no retries, and marks a failure terminal at once — so it has no "will retry" state to + distinguish, and its `status === "failed"` cutoff does not diverge from the `hasError` column. + Confirmed `isCancelled(job.error)` still holds: the only producer of `{ cancelled: true }` in the + installed Payload is `JobCancelledError` in `runJobs/index.js`, which writes `hasError: true` in + the same `updateJob` call, and the plugin never throws an error carrying `.cancelled` itself. + **One finding, applied:** the comment at `cancel-by-collection/handler.ts:33` said a retrying job + "has no `pending` row" — true under the old rule, false under the new one. The filter's logic is + unchanged and still correct; the comment now states the constraint that survives (a job with any + locale in flight is not cancellable, so the filter excludes rather than selects). The deep review + of 2026-10-14 had raised the same line independently. +- **The storage boundary.** No findings. Verified that `ofDocuments` has exactly one production + caller and is now unreachable with an empty list, closing the PostgreSQL `IN ()` route entirely; + that `toTaskFilter` keeps "absent" and "empty" distinct; and that the other places building + `{ in: [...] }` — `cancel` and `visibleIds` — already guard an empty list. Confirmed independently + by reading `collection-utils.ts:43` (`if (ids.length === 0) return new Set();`). Also confirmed + `resolveSettings` is the only production route to `failedWindowMs`, so a bad value cannot reach the + date arithmetic another way. +- **Whether the new tests discriminate.** **One finding, applied, and it was mine.** The case this + task added to the shared suite — then named *"keeps work still in flight however old the cutoff + makes it"* — is a real check under `PayloadJobsTaskRunner` (with `autoRun: false` the row stays + `pending`, so removing the `{ hasError: { not_equals: true } }` branch of `settledSince` would fail + it) but **not** under `SyncTaskRunner`: that runner awaits its handler inside `enqueue`, and the + contract test's handler always succeeds, so the task is `completed` before the filter runs and the + branch `excludeGivenUpBefore && task.status === "failed"` (`SyncTaskRunner.ts:111`) is never + entered. Worse, that branch was covered by **nothing at all** — `excludeGivenUpBefore` appears in no + test of the sync runner. Verified both claims directly. + + Two repairs: the shared case is renamed to what it actually guards for both runners — + *"rejects nothing but a run that gave up, whatever the cutoff"* — and `SyncTaskRunner.test.ts` + gains a `describe("excludeGivenUpBefore")` that drives a task to `failed` with a rejecting handler + and checks both sides of the cutoff. Each side proven by a mutation, both reverted and the revert + verified: + + | Mutation on `SyncTaskRunner.ts` | Went red | + |---|---| + | the whole cutoff branch deleted | *"drops a run that gave up before the cutoff"* only | + | `<` inverted to `>` in the comparison | both cases | + + The reviewer also noted the `log` entry in *"treats a run that is still retrying…"* does not affect + the outcome for a single-locale run — harmless surplus in the fixture, left as is — and that + `toThrow(/failedWindowMs/u)` pins the setting's name but not the message's wording. Both recorded, + neither acted on. + diff --git a/packages/payload-plugin-translator/docs/plans/2026-10-14-settings-travel-as-one-value.task.md b/packages/payload-plugin-translator/docs/plans/2026-10-14-settings-travel-as-one-value.task.md new file mode 100644 index 000000000..c9074f36a --- /dev/null +++ b/packages/payload-plugin-translator/docs/plans/2026-10-14-settings-travel-as-one-value.task.md @@ -0,0 +1,236 @@ +# Settings travel as one value, not as a handful of scalars + +Follows: 2026-10-14-the-panel-shows-what-is-happening.task.md + +## Requirements / Task restatement + +`TranslationContext` (`src/server/modules/translation-levels/types.ts:19`) carries ten readonly +fields in a flat list. Four are **resolved settings**; six are wired dependencies. Adding a setting +means writing its name into every layer that merely passes it along, and into every fixture that +builds a context. + +Measured on `failedWindowMs`, the setting added last week — `grep -rn failedWindowMs src ../../apps/dev/src` +returns 30 mentions over 20 files. Split by what the mention does: + +| Kind | Sites | Where | +|---|---|---| +| **Carriage** — names the setting only to hand it on | **13** over 8 files | `types.ts:22` · `PluginConfigBuilder.ts:72,89` · `useDocTranslationApi.ts:21` · `createTranslationRoutes.ts:20,40,54` · `plugin.ts:160,194` · 4 fixtures | +| Declaration | 2 | `plugin.ts:61` (option), `plugin.ts:19` (default) | +| Reading | 15 | the status handler, the HTTP body, the client panel, `apps/dev` — all genuine | + +Carriage is the only kind this task removes. Reading is what a setting is *for* and must stay. + +## What already exists — platform · installed · project · verdict + +- **Project.** `src/types/` already holds leaf types that both `plugin.ts` and the server modules + name — `TargetSelection.ts`, `AccessGuard.ts`, `CollectionSchemaMap.ts`. The import edge a module + needs (`translation-levels/types.ts` → a shared declaration above it) is the one it already has + four times. +- **Project.** `src/server/modules/task-runner/__tests__/TaskRunner.invariants.ts` is the precedent + for a non-`.test.ts` file under `__tests__` shared by several suites — but this task needs no such + file, see D3. +- **Verdict.** One new file, no dependency, no pattern the package does not already use. + +## Decisions + +### D1 — the four settings become one named value on the context + +**Chosen:** `TranslationContext` loses `basePath`, `failedWindowMs`, `targetSelection` and +`inlineMarks`, and gains `readonly settings: TranslatorSettings`. + +**What lost:** leaving it alone. It lost on the 13 carriage sites above — a measured recurring cost, +and the package has added five settings before this one. + +**The constraint it cites:** `PluginConfigBuilderDeps = TranslationContext` and +`TranslationRoutesDeps = Pick` already derive from the context, so the layers +between `plugin.ts` and a feature never *read* these four values — they only relay them. A relay +should carry one parcel, not unpack and repack four. + +### D2 — the option and its resolved form are declared in the same file + +**Chosen:** a new `src/settings.ts` holding all four of: + +```ts +export type TranslatorSettingsOptions = { basePath?: string; failedWindowMs?: number; + targetSelection?: TargetSelectionMode; + experimental?: { inlineMarks?: boolean } }; +export type TranslatorSettings = { basePath: string; failedWindowMs: number; + targetSelection: TargetSelectionMode; inlineMarks: boolean }; +export const DEFAULT_FAILED_WINDOW_MS = 3 * 24 * 60 * 60 * 1000; +export const resolveSettings = (o: TranslatorSettingsOptions): TranslatorSettings => … +``` + +and `plugin.ts` composing the public type: `TranslatorPluginConfig = TranslatorSettingsOptions & { …the other seven fields }`. + +**What lost, and why — two candidates, and the first is what the brief for this task assumed:** + +- **`Required>`.** Wrong, because the context's values are *resolved*, + not raw: `basePath` is `normalizePath(rawBasePath)`, and `inlineMarks` is + `experimental?.inlineMarks === true` — there is no top-level option of that name. A derived type + would claim the context holds what the caller passed. It does not. +- **Declaring `TranslatorSettings` alone and leaving the options in `plugin.ts`.** Lost on the + arithmetic: a new setting would then cost three files (option, resolved type, default), and + `settings.ts` would have to `Pick` from `plugin.ts` while `plugin.ts` imports the resolver — a + cycle. Composing the public type out of the settings options removes both at once. + +**What this buys, stated as a count rather than a feeling:** the next setting costs **two files, +three edits** — `settings.ts` (option field, resolved field, default) and the one place that reads +it. Carriage goes to **zero**. Today it is eight files and fifteen edits for the same setting. + +### D3 — the parcel is opened at `createTranslationRoutes`, and no deeper + +**Chosen:** `TranslationRoutesDeps` takes `settings: TranslatorSettings`; inside, the eight route +factories keep receiving `basePath` as the positional argument they take today, and +`createGetCollectionStatusRoute` keeps receiving `failedWindowMs` in its config. Below that line the +diff is empty. The four fixtures write `settings: resolveSettings({})` — one line, and a new setting +costs them nothing, which is why no shared test fixture is needed. + +**What lost, and why:** pushing `settings` down to the leaf factories. It lost on the owner's +standing rule — *demand only what you touch*: `createCancelRoute` uses `basePath` and nothing else, +and a parameter whose other three fields it ignores is a signature that lies about what the answer +depends on. + +### D4 — `PluginConfigBuilder` keeps naming its fields + +**Chosen:** one property per context field; after D1 that is seven instead of ten. + +**What lost, and why:** having the class hold `deps` whole lost on a hard constraint — +`PluginConfigBuilder implements LevelContext`, and `LevelContext extends TranslationContext`, so the +fields must exist **on the instance**. The alternative is seven getters: the same line count plus an +indirection. + +### Placement + +| Piece | Lands in | +|---|---| +| `TranslatorSettingsOptions`, `TranslatorSettings`, `DEFAULT_FAILED_WINDOW_MS`, `resolveSettings` | `src/settings.ts` (new) | +| `TranslatorPluginConfig` composed from the options; one `resolveSettings` call in `init()` | `src/plugin.ts` | +| `readonly settings` replacing four scalars | `src/server/modules/translation-levels/types.ts`, `PluginConfigBuilder.ts` | +| relaying one field instead of two | `src/composition/levels/useDocTranslationApi.ts` | +| opening the parcel | `src/server/features/createTranslationRoutes.ts` | +| `ctx.settings.` at the readers | `src/composition/levels/{documentLevel,collectionLevel,fieldLevel}.ts` | + +Logic lives in `src/settings.ts` rather than `src/types/`, because `src/types/` holds declarations +with no behaviour and `resolveSettings` has defaults and a `normalizePath` call. + +### New surface + +`TranslatorSettings` (callers: the context, the builder, the three levels, the route bundle) and +`resolveSettings` (callers: `plugin.ts` and four fixtures). **Neither is added to `src/index.ts`** — +per ADR-0002, `TranslatorPluginConfig` stays the consumer-facing name and contextual typing covers +the rest. + +### Written contract? + +**None.** `resolveSettings` is total, pure, and says everything in its signature except the four +defaults, which `@default` tags on the option fields already carry. No `/sp-red-test` path. + +### Escalate to /sp-architect? + +**No.** It regroups fields on an internal type that four other internal types already derive from. +No new seam, no dependency, no data-model change. + +## Acceptance Criteria + +| AC | Criterion | How it is checked | Passes when | +|---|---|---|---| +| AC1 | No carriage layer names an individual setting | `grep -rnE "readonly (basePath\|failedWindowMs\|targetSelection\|inlineMarks)" src/server/modules/translation-levels/` | no match | +| AC2 | No context fixture names an individual setting | `grep -rn "failedWindowMs" src/composition/levels/*.test.ts src/server/modules/translation-levels/*.test.ts src/server/features/createTranslationRoutes.test.ts` | no match | +| AC3 | Carriage for one setting is gone end to end | `grep -rln "failedWindowMs" src` | the 8 carriage files are absent; only `settings.ts`, the status feature, and the client remain | +| AC4 | The public option surface is unchanged for a consumer | a scratch file assigning `{ basePath, failedWindowMs, targetSelection, experimental, … }` to `TranslatorPluginConfig` | compiles with no export added to `src/index.ts` | +| AC5 | Behaviour unchanged in the package | `bunx vitest run` | ≥ 2088 pass, 0 fail | +| AC6 | The settings reach the admin surfaces end to end | `apps/dev`: rebuild the plugin, then `bun run test:integration:{sqlite,postgres,mongo}` | ≥ 235 / 240 / 233 pass | +| AC7 | A non-default `basePath` still arrives normalised | the existing `plugin.test.ts:104` case (`build({ basePath: "/i18n" })`) | passes unchanged | +| AC8 | No new type errors | `bun run check-types` here and in `apps/dev`; `bunx tsc --noEmit -p tsconfig.json` in `apps/dev` | 0 · 0 · 2 (the two pre-existing, in files this task does not touch) | +| AC9 | No new lint findings | `bunx oxlint .` | ≤ 46 warnings, 0 errors | + +## Pre-flight — run against the untouched tree + +| AC | Result now | Classification | +|---|---|---| +| AC1 | **8 matches** (`PluginConfigBuilder.ts:71,72,78,79` · `types.ts:21,22,33,34`) | change — must reach 0 | +| AC2 | **4 matches** (`PluginConfigBuilder.test.ts:11` · `levels.test.ts:33` · `fieldLevel.test.ts:15` · `createTranslationRoutes.test.ts:14`) | change — must reach 0 | +| AC3 | **13 carriage sites over 8 files** | change — must reach 0 | +| AC4 | compiles | invariant — must keep compiling | +| AC5 | 2088 passed / 150 files | invariant (baseline) | +| AC6 | 235 / 240 / 233 | invariant (baseline) | +| AC7 | passes | invariant | +| AC8 | 0 · 0 · 2 | invariant | +| AC9 | 46 warnings, 0 errors | invariant | + +AC1's pre-flight corrects the figure this contract first carried: I wrote 4 from memory, the grep +returns 8, because `PluginConfigBuilder` restates every field the context declares. + +## Risk notes + +**Risk classification: LOW.** Nothing public changes — `TranslationContext` and `LevelContext` are +deliberately unexported, and the docblock on `TranslationLevel` says so: *"not externally +implementable yet, since LevelContext is intentionally not exported"*. Behaviour is unchanged, and +every relay is typed, so a mis-wired field is a compile error rather than a runtime surprise. That +buys one reviewer in Phase 5 rather than three, and a sweep over the four derived types rather than +an evidenced sweep of everything. + +**Hazards:** + +- `inlineMarks` is read by `wireTranslateRunner` **before** the builder is constructed + (`plugin.ts:177`). `resolveSettings` must therefore be called early in `init()`, right after the + `access` check, not beside the builder. +- `basePath` is normalised after destructuring today (`plugin.ts:174`). The normalisation moves + inside `resolveSettings`; if it is dropped, a custom base path silently loses it and only AC7 + catches it. +- Moving the four option declarations into `src/settings.ts` moves their JSDoc. A reader opening + `plugin.ts` to find `basePath` will no longer see it there. `TranslatorPluginConfig` remains the + exported name and hover text still resolves through the intersection, but this is a real + readability cost paid for the arithmetic in D2. +- `StalenessConfig` picks `provenanceServiceFactory` — a dependency, not a setting. It must come + through untouched; if it changes, the settings/dependency split was drawn wrong. + +## Human choices + +- **2026-10-14** — do it now rather than defer: "Не хочу откладывать. И так много работы." +- **2026-10-14** — no deprecation window for the context fields. Raised by the owner as an option; + dropped once the types were shown to be unexported, so there is no consumer to keep compatible. +- **2026-10-14** — not a one-field first step. The owner asked whether to start with `failedWindowMs` + alone; rejected because it leaves two ways to pass a setting with nothing forcing the new one, and + pays the full plumbing cost for none of the gain. + +- **2026-10-14** — the four option declarations move into `src/settings.ts` (D2 as written), chosen + at the Phase 2 gate over keeping them in `plugin.ts`. The owner accepted the readability cost named + in the hazards — `plugin.ts` no longer shows `basePath` — in exchange for one declaration site per + setting and no hand-written duplicate of the option shape. + +## Verification — 2026-10-14, after implementation + +| AC | Result | Verdict | +|---|---|---| +| AC1 | the grep returns nothing (was 8) | met | +| AC2 | the grep returns nothing (was 4) | met | +| AC3 | carriage gone from all 8 files. `failedWindowMs` now lives in `settings.ts` (declaration), `createTranslationRoutes.ts` (1 site, where the parcel opens), the status feature (4) and the client (5) — declaration and reading only | met | +| AC4 | a scratch consumer assigning all 11 option fields to `TranslatorPluginConfig` compiled; `src/index.ts` unchanged | met | +| AC5 | `2097 passed (2097)` over 151 files — the 2088 baseline plus 9 new | met | +| AC6 | sqlite `235 passed / 7 skipped` · postgres `240 passed / 2 skipped` · mongo `233 passed / 9 skipped` — all three at baseline | met | +| AC7 | `plugin.test.ts` "threads a custom basePath into every endpoint" passes unchanged | met | +| AC8 | package `0` · `apps/dev` check-types `0` · `apps/dev` tsconfig.json `2` (`payload.config.ts:166`, `vitest.integration.config.ts:21` — both pre-existing, untouched) | met | +| AC9 | `Found 46 warnings and 0 errors` here; `5 warnings` in `apps/dev` | met | + +The real `tsc` declaration build (`turbo run build`) was run as well, not only tsgo — it compiled +377 files and emitted declarations cleanly. + +### Prevention + +`src/settings.ts` is new code holding four defaults and one normalisation, and nothing covered them +directly: only `basePath` had a plugin-level case. `src/settings.test.ts` closes that with 9 checks, +each proven by a mutation that was reverted afterwards: + +| Mutation | Caught by | Other checks | +|---|---|---| +| `??` → `\|\|` on `failedWindowMs` | "keeps a window of zero instead of reading it as unset" | 8 stayed green | +| dropped `normalizePath(…)` | "normalises a base path the caller wrote loosely" | 8 stayed green | +| `inlineMarks === true` → `!== false` | the two absent-`experimental` cases + "fills every setting from nothing" | the explicit-value cases stayed green | + +The `??` mutation is the one worth having: `\|\|` would silently replace an explicit +`failedWindowMs: 0` — "report no failures at all" — with the three-day default. + +## Review log + +(empty) diff --git a/packages/payload-plugin-translator/docs/plans/2026-10-14-the-panel-shows-what-is-happening.task.md b/packages/payload-plugin-translator/docs/plans/2026-10-14-the-panel-shows-what-is-happening.task.md new file mode 100644 index 000000000..9c753374d --- /dev/null +++ b/packages/payload-plugin-translator/docs/plans/2026-10-14-the-panel-shows-what-is-happening.task.md @@ -0,0 +1,269 @@ +# The collection panel shows what is happening, not what has happened + +Follows: 2026-10-12-bound-the-status-reads.task.md + +## Requirements / Task restatement + +`GET /translate/collection/:slug` calls `findByCollection(collectionSlug)` with no filter +(`get-collection-status/handler.ts:38`), so it reads the collection's whole job history to draw four +numbers. The previous task narrowed that read to one collection; this one bounds it outright, by +changing what the panel is for. + +**The panel answers "what is this collection doing", not "what has it ever done".** It aggregates: +a translation started from a document, or by the auto-translate hook, shows up here too. It has no +life of its own. + +- **`Translated` goes away.** Not because the count is expensive but because nobody acts on it. + "Is my document translated" is answered by `GET /translate/document/:slug/:id`, which is complete + and, since the previous task, cheap. A collection-wide total is a statistic, and it can never be + both bounded and true. +- **`Failed` stays, bounded by a window**, and says so in the row. A failure is a call to action, + and it is the one figure a window improves rather than damages: a failure from three months ago + is not an action. + +## What already exists — platform · installed · project · verdict + +- **Platform.** Every field this needs is a **real column** on the job row — `completedAt`, + `hasError`, `processing`, plus Payload's automatic `createdAt` / `updatedAt` + (`node_modules/payload/dist/queues/config/collection.js`). No path inside the `json` input is + involved, so no adapter-specific behaviour is in play at all. +- **Project.** `TaskFilter` (`TaskRunner.interface.ts`) already carries `excludeCompleted`, a filter + that narrows the same read and is honoured by both runners. `TranslatorPluginConfig` + (`plugin.ts:19`) already carries optional settings with documented defaults — `basePath`, + `levels`, `provenance`. The client already renders nothing for a zero count + (`CollectionTranslationProgress.tsx:64`). +- **Verdict.** Nothing new is built. One optional field joins an existing filter, one row leaves an + existing list, one optional setting joins an existing options type. + +## Decisions + +### D1 — the bound is a second field on `TaskFilter`, not a new query path + +**Chosen:** `TaskFilter` gains one optional field meaning "drop failures older than this". It +composes with `excludeCompleted`, which already means "drop successes", so the panel asks for both +and gets exactly the live-plus-recently-failed set. + +The query the Payload runner then sends: + +``` +and: [ + { completedAt: { exists: false } }, // from excludeCompleted + { or: [ { hasError: { not_equals: true } }, // still in flight + { updatedAt: { greater_than: cutoff } } ] }, // or failed recently +] +``` + +**What lost, and why:** a dedicated method on `TaskRunner` lost for the reason the previous task +recorded at length — a capability bolted onto the execution contract that only one runner can answer +(`2026-10-12-bound-the-status-reads.task.md`, *Why Stage B was withdrawn*). Reading everything and +dropping stale failures in memory lost because failures are exactly what accumulates: it would leave +the read bounded by the project's whole failure history, which is the defect being fixed. + +**The constraint it cites:** `excludeCompleted` is the existing precedent for narrowing this same +call, it is already implemented by `PayloadJobsTaskRunner` and `SyncTaskRunner`, and it is already +used by a second caller (`cancel-by-collection/handler.ts:30`). + +### D2 — a runner that ignores the new field degrades, it does not break + +`TaskFilter` is re-exported from `src/index.ts`, so a third-party runner reads it. The field is +optional and a runner that does not honour it returns **more** rows than asked — stale failures in +the panel, never missing work. That is the same shape as `excludeCompleted`, whose docblock already +describes what it drops rather than demanding it. + +### D3 — the endpoint reports the window; the client does not learn it separately + +The `Failed` row has to name the window, so the client needs the number. + +**Chosen:** `GET /translate/collection/:slug` answers with the window alongside `docs`, and the client +renders what it was given. + +**What lost, and why:** carrying it through `TranslateKitConfigProvider` — which today holds only +`basePath` (`TranslateKitConfigContext.tsx:6`) — would mean plumbing a second value across the admin +boundary, and worse, the label could then disagree with the cutoff the query actually used. Taking +both from one place makes the label unable to lie. + +### D4 — the panel gets a rendered test, and the package gets the setup for one + +The component had no test and the package had no way to write one: `vitest.config.ts` sets +`environment: "node"` and nothing DOM-related was installed. + +**Chosen, by the owner at the gate:** add `jsdom`, `@testing-library/react` and +`@testing-library/dom` as dev dependencies of **this package only**, and opt the one client test into +`jsdom` with a per-file `@vitest-environment` docblock rather than switching the package's 148 other +test files. + +**What lost, and why:** asserting over the exported `ROWS` list and a label helper would have needed +no dependency, but it checks the data the panel is built from rather than what it renders — and the +criterion is about what an editor sees. + +**The constraint it cites:** this is not new to the repository. `payload-plugin-visual-editing` and +`payload-plugin-analytics` already declare both, `apps/cms` and `apps/dev` already declare `jsdom`, +and each does so in its own `package.json`. Versions here match `apps/cms` / `apps/dev` exactly +(`16.3.0`, `26.1.0`) because `bunfig.toml` hoists, and two ranges of one package in one tree is a +hazard worth not creating. + +### D5 — a locale is failed only once its run has given up + +**Added mid-build, with the owner's approval, after a reviewer found that the window's safety +argument did not survive contact with how a locale's status is derived.** + +Payload appends a log entry with `state: 'failed'` on **every** failed attempt, and marks the job +itself only when it gives up — `queues/errors/handleTaskError.js:110-119` pushes the entry while +setting `hasError: hasFinalError`. `normalizeJobLocales` read that entry alone and called the locale +failed, so a locale that was **still being retried** was reported as a failure. Measured, not +reasoned: a seeded job with a failed log entry and `hasError: false` answered `failed` where it owed +`pending`. + +That defect predates this task — `normalizeJob.ts` is untouched by the rest of the diff — but this +task is what makes it harmful: the new row tells the reader "open a document to see why" about work +that may still succeed on its own. + +**Chosen:** a locale is `completed` on its own evidence — its own log entry succeeded — and anything +else falls back to the **run's** status rather than being called failed. + +**What lost, and why:** leaving the derivation alone and softening the row's wording would have kept +the task smaller, but it leaves a counter that is wrong wherever it is read, including the document +panel. The owner chose the fix over the wording. + +**What it costs, stated plainly:** a locale whose last attempt failed is now indistinguishable from +one never attempted — both read `pending` while the run still owes them. That information is not +lost, only moved: the job's own log still records which locale failed, and +`locale-workflow-failure.int.test.ts` asserts it there. + +### Placement + +| Piece | Lands in | +|---|---| +| the filter field | `modules/task-runner/TaskRunner.interface.ts` — `TaskFilter` | +| the query | `payload-jobs-runner/PayloadJobsTaskRunner.ts` — `findByCollection` | +| the in-memory equivalent | `sync-runner/SyncTaskRunner.ts` — `findByCollection` | +| the window's value | `plugin.ts` → `createTranslationRoutes` → `GetCollectionStatusConfig` | +| asking for it | `features/get-collection-status/handler.ts` | +| the row and its label | `client/features/collection-translation-progress/ui/CollectionTranslationProgress.tsx` | +| the window on the wire | `features/get-collection-status/model.ts` — the output type | + +### New surface + +- One optional field on `TaskFilter` — callers: `get-collection-status/handler.ts`; implementers: + both runners. +- One optional setting on `TranslatorPluginConfig`, if the owner wants it configurable (gate + question 1). + +No new abstraction, no new file, no new module. + +### Written contract? + +**One thing a signature cannot state, and it is the reason the window works at all:** `hasError` is +set only when a run has **finally** given up — a run still retrying does not carry it +(`handleTaskError`). So the "in flight" branch keeps retrying work, and a failure becomes visible +only when it is certain, which is the same moment `onFailed` fires. That belongs in the field's +docblock; it is not something an implementer can infer from the name. + +### Escalate to /sp-architect? + +**No.** One optional field on an existing filter, one row removed from a list. No new seam, no +dependency, no data-model change, no migration. + +## Acceptance Criteria + +| AC | Criterion | How it is checked | Passes when | +|---|---|---|---| +| AC1 | `GET /translate/collection/:slug` returns no entry for a run that completed | new integration test: translate, let it finish, read the panel | no entry carries status `completed` | +| AC2 | It still returns a run that is queued or running | new integration test with `autoRun: false`, so the run stays queued | the queued locale is present | +| AC3 | It returns a run that failed inside the window | new integration test seeding a job with `hasError` and a recent `updatedAt` | the failed entry is present | +| AC4 | It does **not** return a run that failed before the window | same test, a second job with an old `updatedAt` | that entry is absent | +| AC5 | The job table is not read for completed rows at all | the same read-watching probe the sibling spec uses (`status-reads-are-bounded.int.test.ts`) | no row with `completedAt` set is loaded | +| AC6 | A run still retrying is reported as in flight, not as failed | new integration test: a job with an error but `hasError` unset | present, and not counted failed | +| AC7 | The panel no longer offers a `Translated` row | **new** rendered test for `CollectionTranslationProgress`, under `@vitest-environment jsdom` | a panel given a completed count renders no row naming it | +| AC8 | The `Failed` row names the window | the same rendered test | the rendered failed row's text contains the number of days | +| AC9 | `SyncTaskRunner` honours the new filter | new unit case in its existing test file | an old failure is dropped, a recent one kept | +| AC10 | The other two callers of `findByCollection` are unaffected | `bunx vitest run src/server/features/cancel-by-collection src/server/features/get-document-status` | stays green | +| AC11 | No new type errors, in either project of the dev app | `bun run check-types` in the package and in `apps/dev`; `bunx tsc --noEmit -p tsconfig.json` in `apps/dev` | package and `apps/dev` clean; the app's own count stays at 2, both in files this task does not touch | +| AC12 | No new lint findings | `bunx oxlint .` in the package; `bunx oxlint src` in `apps/dev` | 47 and 5, or fewer | +| AC13 | Nothing already passing regressed | `bunx vitest run`; `bun run test:integration:{sqlite,postgres,mongo}` | ≥ 2060 unit; ≥ 229 / 234 / 227 integration | + +## Pre-flight + +Run against the untouched tree on `feat/translator-bound-status-reads`, before any edit. + +| AC | Command | Result | Classification | +|---|---|---|---| +| AC1–AC6, AC9 | the named tests | do not exist yet | change — each gets its red run in Phase 3 | +| AC7, AC8 | the client panel test | **the component had no test and the package had no DOM environment** — both added at the gate, see D4 | change — the test is written in Phase 3 and goes red against the current `ROWS` list | +| AC10 | `bunx vitest run src/server/features/cancel-by-collection src/server/features/get-document-status` | 32 passed | invariant | +| AC11 | the three type-check commands | package **0**, `apps/dev` **0**, app project **2** | invariant | +| AC12 | `bunx oxlint .` · `bunx oxlint src` | **47** · **5** | invariant | +| AC13 | `bunx vitest run` · the three integration runs | 2060 · 229 / 234 / 227 | invariant (baseline) | + +## Risk notes + +**Risk classification: LOW.** One optional field on a filter that already exists and is already +honoured by both runners; one row removed from a client list; no stored shape, no migration, no new +method on any contract, no response-shape change. That buys: one reviewer rather than three in +Phase 5, and a regression sweep over the three callers of `findByCollection` rather than an +evidenced sweep of everything. + +**Hazards:** + +- `excludeCompleted`'s docblock says it "keeps running and failed ones — everything a re-enqueue can + still supersede". The new field narrows that set, so the two docblocks have to agree or the next + reader will believe the wrong one. +- `cancel-by-collection` passes `excludeCompleted: true` and must keep seeing **every** cancellable + run, including an old failure. It must not acquire the new field by accident. +- A window is a behaviour change the panel's own tests cannot see without a clock. Seeding + `updatedAt` directly is the only honest way to test the boundary, which means writing through the + adapter rather than through the field. + +## Human choices + +- **2026-10-14** — drop `Translated`, keep `Failed` windowed: "Уберем Translated и оставим failed." + The reasoning the owner accepted: the two rows are not equivalent — a failure is a call to action + and an old one stops being one, while a translated count is a statistic nobody acts on. +- **2026-10-14** — the collection panel is an aggregator with no life of its own: "если пользователь + запускает перевод в документе, то на уровне коллекции это тоже отобразится". +- **2026-10-14** — the window is a plugin setting defaulting to 3 days, not a constant. +- **2026-10-14** — the `Failed` row keeps its heading and carries the window in the small line beneath + it, where "Open a document to see why" already sits. +- **2026-10-14** — add the DOM test setup rather than testing the panel's data: "давай добавим". See + D4 for what that cost and why it is confined to this package. +- **2026-10-14** — fix the status derivation rather than soften the row's wording: "Да, чинить + normalizeJob.ts", told beforehand that it changes what the document panel shows for a locale whose + attempt failed. See D5. + +### D6 — the filter field is named for giving up, not for failing + +An abstraction audit measured the strongest thing a caller must agree with this boundary about, and +it was **meaning**: `excludeFailedBefore` said "failed" while the rule is "gave up". A third-party +runner reading the name alone would drop a run that failed one attempt and is still retrying, and +work in flight would vanish from the panel. Only the docblock said otherwise. + +**Chosen:** `excludeGivenUpBefore`. The meaning moves from prose into the name, and the docblock +keeps only what the name cannot carry — that the queue gives up after the last retry, with its +citation. Free now: the field ships first in 0.17.0, so nobody outside has it yet. + +### D7 — the panel does not show locales finished inside work still in flight + +Measured during the same audit: a live run reports `de=completed fr=pending es=pending`, so the +response does carry completed locales — bounded ones, since they exist only inside runs that have +not finished. The panel discards them. + +**Decided: leave it.** This is not the lifetime total D1 removed, and showing it would be defensible +— it is the progress figure the owner originally asked for. But it drains to zero as runs finish +rather than settling at "30 of 30", which is the behaviour the panel was deliberately moved away +from. The omission is now **structural rather than silent**: `ReportedStatus` excludes +`COMPLETED`, so `ROWS` cannot name a row the panel does not count and the query cannot fill one. + +## Review log + +**2026-10-14 — one reviewer (low risk).** Confirmed clean: `hasError` really is final-only; the two +other `findByCollection` callers are untouched; a long-quiet but still-pending run is not dropped, +because the window applies only where `hasError` is true; the new DOM dependencies and the +`*.test.tsx` include leave the other 148 test files on `node`. + +**One major finding, verified and fixed — D5.** The reviewer showed that the window's safety argument +held for the field being filtered on but not for the status being displayed. Reproduced it with a +seeded row before acting, then fixed the derivation. The finding also exposed that AC6 asserted +presence but not status; the criterion's own test was tightened before the fix, and went red. + +A second, speculative concern — a backoff gap longer than the window — was traced and did not bite: +the window applies only to runs carrying `hasError`, and a run between retries does not. diff --git a/packages/payload-plugin-translator/docs/plans/artifacts/2026-10-13-bounded-status-reads-red-run.txt b/packages/payload-plugin-translator/docs/plans/artifacts/2026-10-13-bounded-status-reads-red-run.txt new file mode 100644 index 000000000..a3d319013 --- /dev/null +++ b/packages/payload-plugin-translator/docs/plans/artifacts/2026-10-13-bounded-status-reads-red-run.txt @@ -0,0 +1,54 @@ +⎯⎯⎯⎯⎯⎯⎯ Failed Tests 2 ⎯⎯⎯⎯⎯⎯⎯ + + FAIL src/integration/translator/status-reads-are-bounded.int.test.ts > the status endpoints read only what they answer about > loads no job row belonging to another collection +AssertionError: another collection's history is not this collection's panel to pay for: expected [ 'somewhere-else' ] to deeply equal [] + +- Expected ++ Received + +- [] ++ [ ++ "somewhere-else", ++ ] + + ❯ src/integration/translator/status-reads-are-bounded.int.test.ts:129:7 + 127| rowsRead().map(slugOf).filter((slug) => slug === "somewhere-else… + 128| "another collection's history is not this collection's panel to … + 129| ).toEqual([]); + | ^ + 130| }); + 131| + +⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[1/2]⎯ + + FAIL src/integration/translator/status-reads-are-bounded.int.test.ts > the status endpoints read only what they answer about > loads only the asked document's rows, for rows written in the flat shape +AssertionError: one document's panel does not read another document's runs: expected [ '4', '999', '1' ] to deeply equal [] + +- Expected ++ Received + +- [] ++ [ ++ "4", ++ "999", ++ "1", ++ ] + + ❯ src/integration/translator/status-reads-are-bounded.int.test.ts:164:7 + 162| flatRowsForOthers.map(documentOf), + 163| "one document's panel does not read another document's runs" + 164| ).toEqual([]); + | ^ + 165| }); + 166| + +⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[2/2]⎯ + + + Test Files 1 failed (1) + Tests 2 failed | 2 passed (4) + Start at 10:48:17 + Duration 2.36s (transform 428ms, setup 0ms, import 1.91s, tests 343ms, environment 0ms) + +error: script "test:integration" exited with code 1 +error: script "test:integration:sqlite" exited with code 1 diff --git a/packages/payload-plugin-translator/docs/plans/artifacts/2026-10-14-normalizeJobLocales-red-run.txt b/packages/payload-plugin-translator/docs/plans/artifacts/2026-10-14-normalizeJobLocales-red-run.txt new file mode 100644 index 000000000..6e39cf4d8 --- /dev/null +++ b/packages/payload-plugin-translator/docs/plans/artifacts/2026-10-14-normalizeJobLocales-red-run.txt @@ -0,0 +1,298 @@ + + RUN v3.2.4 /Users/FocusReactive/Documents/Projects/payload-plugins/packages/payload-plugin-translator + + ❯ src/server/modules/task-runner/payload-jobs-runner/__tests__/normalizeJobLocales.contract.test.ts (19 tests | 19 failed) 9ms + × normalizeJobLocales > gives a run one row per entry of its locale list, in that order 3ms + → not implemented — red run + × normalizeJobLocales > reproduces a repeated locale as two rows 0ms + → not implemented — red run + × normalizeJobLocales > gives a job with no locale list one row for the single locale it carries 0ms + → not implemented — red run + × normalizeJobLocales > gives a job with an empty locale list one row for the single locale it carries 0ms + → not implemented — red run + × normalizeJobLocales > completes a locale whose own latest entry succeeded, although the run failed 0ms + → not implemented — red run + × normalizeJobLocales > dates a completed locale by its own entry, not by the run 0ms + → not implemented — red run + × normalizeJobLocales > leaves a completed locale without an error, although the run carries one 0ms + → not implemented — red run + × normalizeJobLocales > leaves a completed locale uncancelled, although the run carries an error 0ms + → not implemented — red run + × normalizeJobLocales > reports a locale whose latest entry failed as the run stands, not as the entry does 0ms + → not implemented — red run + × normalizeJobLocales > takes a locale's latest entry over its earlier attempt 0ms + → not implemented — red run + × normalizeJobLocales > does not complete a locale whose latest entry failed after an earlier success 0ms + → not implemented — red run + × normalizeJobLocales > reports a locale with no entry of its own as the run stands 0ms + → not implemented — red run + × normalizeJobLocales > gives every row of one run the same id 1ms + → not implemented — red run + × normalizeJobLocales > reads a run with a completedAt as completed 0ms + → not implemented — red run + × normalizeJobLocales > reads a run that is processing as running 0ms + → not implemented — red run + × normalizeJobLocales > reads a run with an error as failed 0ms + → not implemented — red run + × normalizeJobLocales > reads a run with no completedAt, no processing and no error as pending 0ms + → not implemented — red run + × normalizeJobLocales > reads a run as completed even though it is also processing and carries an error 0ms + → not implemented — red run + × normalizeJobLocales > reads a run as running even though it also carries an error 0ms + → not implemented — red run + +⎯⎯⎯⎯⎯⎯ Failed Tests 19 ⎯⎯⎯⎯⎯⎯⎯ + + FAIL src/server/modules/task-runner/payload-jobs-runner/__tests__/normalizeJobLocales.contract.test.ts > normalizeJobLocales > gives a run one row per entry of its locale list, in that order +Error: not implemented — red run + ❯ notImplemented __stubs__/normalizeJob.stub.ts:6:9 + 4| + 5| const notImplemented = (): never => { + 6| throw new Error("not implemented — red run"); + | ^ + 7| }; + 8| + ❯ src/server/modules/task-runner/payload-jobs-runner/__tests__/normalizeJobLocales.contract.test.ts:29:12 + +⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[1/19]⎯ + + FAIL src/server/modules/task-runner/payload-jobs-runner/__tests__/normalizeJobLocales.contract.test.ts > normalizeJobLocales > reproduces a repeated locale as two rows +Error: not implemented — red run + ❯ notImplemented __stubs__/normalizeJob.stub.ts:6:9 + 4| + 5| const notImplemented = (): never => { + 6| throw new Error("not implemented — red run"); + | ^ + 7| }; + 8| + ❯ src/server/modules/task-runner/payload-jobs-runner/__tests__/normalizeJobLocales.contract.test.ts:40:7 + +⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[2/19]⎯ + + FAIL src/server/modules/task-runner/payload-jobs-runner/__tests__/normalizeJobLocales.contract.test.ts > normalizeJobLocales > gives a job with no locale list one row for the single locale it carries +Error: not implemented — red run + ❯ notImplemented __stubs__/normalizeJob.stub.ts:6:9 + 4| + 5| const notImplemented = (): never => { + 6| throw new Error("not implemented — red run"); + | ^ + 7| }; + 8| + ❯ src/server/modules/task-runner/payload-jobs-runner/__tests__/normalizeJobLocales.contract.test.ts:48:12 + +⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[3/19]⎯ + + FAIL src/server/modules/task-runner/payload-jobs-runner/__tests__/normalizeJobLocales.contract.test.ts > normalizeJobLocales > gives a job with an empty locale list one row for the single locale it carries +Error: not implemented — red run + ❯ notImplemented __stubs__/normalizeJob.stub.ts:6:9 + 4| + 5| const notImplemented = (): never => { + 6| throw new Error("not implemented — red run"); + | ^ + 7| }; + 8| + ❯ src/server/modules/task-runner/payload-jobs-runner/__tests__/normalizeJobLocales.contract.test.ts:54:12 + +⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[4/19]⎯ + + FAIL src/server/modules/task-runner/payload-jobs-runner/__tests__/normalizeJobLocales.contract.test.ts > normalizeJobLocales > completes a locale whose own latest entry succeeded, although the run failed +Error: not implemented — red run + ❯ notImplemented __stubs__/normalizeJob.stub.ts:6:9 + 4| + 5| const notImplemented = (): never => { + 6| throw new Error("not implemented — red run"); + | ^ + 7| }; + 8| + ❯ src/server/modules/task-runner/payload-jobs-runner/__tests__/normalizeJobLocales.contract.test.ts:65:12 + +⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[5/19]⎯ + + FAIL src/server/modules/task-runner/payload-jobs-runner/__tests__/normalizeJobLocales.contract.test.ts > normalizeJobLocales > dates a completed locale by its own entry, not by the run +Error: not implemented — red run + ❯ notImplemented __stubs__/normalizeJob.stub.ts:6:9 + 4| + 5| const notImplemented = (): never => { + 6| throw new Error("not implemented — red run"); + | ^ + 7| }; + 8| + ❯ src/server/modules/task-runner/payload-jobs-runner/__tests__/normalizeJobLocales.contract.test.ts:78:12 + +⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[6/19]⎯ + + FAIL src/server/modules/task-runner/payload-jobs-runner/__tests__/normalizeJobLocales.contract.test.ts > normalizeJobLocales > leaves a completed locale without an error, although the run carries one +Error: not implemented — red run + ❯ notImplemented __stubs__/normalizeJob.stub.ts:6:9 + 4| + 5| const notImplemented = (): never => { + 6| throw new Error("not implemented — red run"); + | ^ + 7| }; + 8| + ❯ src/server/modules/task-runner/payload-jobs-runner/__tests__/normalizeJobLocales.contract.test.ts:91:12 + +⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[7/19]⎯ + + FAIL src/server/modules/task-runner/payload-jobs-runner/__tests__/normalizeJobLocales.contract.test.ts > normalizeJobLocales > leaves a completed locale uncancelled, although the run carries an error +Error: not implemented — red run + ❯ notImplemented __stubs__/normalizeJob.stub.ts:6:9 + 4| + 5| const notImplemented = (): never => { + 6| throw new Error("not implemented — red run"); + | ^ + 7| }; + 8| + ❯ src/server/modules/task-runner/payload-jobs-runner/__tests__/normalizeJobLocales.contract.test.ts:104:12 + +⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[8/19]⎯ + + FAIL src/server/modules/task-runner/payload-jobs-runner/__tests__/normalizeJobLocales.contract.test.ts > normalizeJobLocales > reports a locale whose latest entry failed as the run stands, not as the entry does +Error: not implemented — red run + ❯ notImplemented __stubs__/normalizeJob.stub.ts:6:9 + 4| + 5| const notImplemented = (): never => { + 6| throw new Error("not implemented — red run"); + | ^ + 7| }; + 8| + ❯ src/server/modules/task-runner/payload-jobs-runner/__tests__/normalizeJobLocales.contract.test.ts:116:7 + +⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[9/19]⎯ + + FAIL src/server/modules/task-runner/payload-jobs-runner/__tests__/normalizeJobLocales.contract.test.ts > normalizeJobLocales > takes a locale's latest entry over its earlier attempt +Error: not implemented — red run + ❯ notImplemented __stubs__/normalizeJob.stub.ts:6:9 + 4| + 5| const notImplemented = (): never => { + 6| throw new Error("not implemented — red run"); + | ^ + 7| }; + 8| + ❯ src/server/modules/task-runner/payload-jobs-runner/__tests__/normalizeJobLocales.contract.test.ts:130:12 + +⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[10/19]⎯ + + FAIL src/server/modules/task-runner/payload-jobs-runner/__tests__/normalizeJobLocales.contract.test.ts > normalizeJobLocales > does not complete a locale whose latest entry failed after an earlier success +Error: not implemented — red run + ❯ notImplemented __stubs__/normalizeJob.stub.ts:6:9 + 4| + 5| const notImplemented = (): never => { + 6| throw new Error("not implemented — red run"); + | ^ + 7| }; + 8| + ❯ src/server/modules/task-runner/payload-jobs-runner/__tests__/normalizeJobLocales.contract.test.ts:142:12 + +⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[11/19]⎯ + + FAIL src/server/modules/task-runner/payload-jobs-runner/__tests__/normalizeJobLocales.contract.test.ts > normalizeJobLocales > reports a locale with no entry of its own as the run stands +Error: not implemented — red run + ❯ notImplemented __stubs__/normalizeJob.stub.ts:6:9 + 4| + 5| const notImplemented = (): never => { + 6| throw new Error("not implemented — red run"); + | ^ + 7| }; + 8| + ❯ src/server/modules/task-runner/payload-jobs-runner/__tests__/normalizeJobLocales.contract.test.ts:153:12 + +⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[12/19]⎯ + + FAIL src/server/modules/task-runner/payload-jobs-runner/__tests__/normalizeJobLocales.contract.test.ts > normalizeJobLocales > gives every row of one run the same id +Error: not implemented — red run + ❯ notImplemented __stubs__/normalizeJob.stub.ts:6:9 + 4| + 5| const notImplemented = (): never => { + 6| throw new Error("not implemented — red run"); + | ^ + 7| }; + 8| + ❯ src/server/modules/task-runner/payload-jobs-runner/__tests__/normalizeJobLocales.contract.test.ts:160:17 + +⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[13/19]⎯ + + FAIL src/server/modules/task-runner/payload-jobs-runner/__tests__/normalizeJobLocales.contract.test.ts > normalizeJobLocales > reads a run with a completedAt as completed +Error: not implemented — red run + ❯ notImplemented __stubs__/normalizeJob.stub.ts:6:9 + 4| + 5| const notImplemented = (): never => { + 6| throw new Error("not implemented — red run"); + | ^ + 7| }; + 8| + ❯ src/server/modules/task-runner/payload-jobs-runner/__tests__/normalizeJobLocales.contract.test.ts:168:12 + +⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[14/19]⎯ + + FAIL src/server/modules/task-runner/payload-jobs-runner/__tests__/normalizeJobLocales.contract.test.ts > normalizeJobLocales > reads a run that is processing as running +Error: not implemented — red run + ❯ notImplemented __stubs__/normalizeJob.stub.ts:6:9 + 4| + 5| const notImplemented = (): never => { + 6| throw new Error("not implemented — red run"); + | ^ + 7| }; + 8| + ❯ src/server/modules/task-runner/payload-jobs-runner/__tests__/normalizeJobLocales.contract.test.ts:174:12 + +⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[15/19]⎯ + + FAIL src/server/modules/task-runner/payload-jobs-runner/__tests__/normalizeJobLocales.contract.test.ts > normalizeJobLocales > reads a run with an error as failed +Error: not implemented — red run + ❯ notImplemented __stubs__/normalizeJob.stub.ts:6:9 + 4| + 5| const notImplemented = (): never => { + 6| throw new Error("not implemented — red run"); + | ^ + 7| }; + 8| + ❯ src/server/modules/task-runner/payload-jobs-runner/__tests__/normalizeJobLocales.contract.test.ts:180:12 + +⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[16/19]⎯ + + FAIL src/server/modules/task-runner/payload-jobs-runner/__tests__/normalizeJobLocales.contract.test.ts > normalizeJobLocales > reads a run with no completedAt, no processing and no error as pending +Error: not implemented — red run + ❯ notImplemented __stubs__/normalizeJob.stub.ts:6:9 + 4| + 5| const notImplemented = (): never => { + 6| throw new Error("not implemented — red run"); + | ^ + 7| }; + 8| + ❯ src/server/modules/task-runner/payload-jobs-runner/__tests__/normalizeJobLocales.contract.test.ts:184:12 + +⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[17/19]⎯ + + FAIL src/server/modules/task-runner/payload-jobs-runner/__tests__/normalizeJobLocales.contract.test.ts > normalizeJobLocales > reads a run as completed even though it is also processing and carries an error +Error: not implemented — red run + ❯ notImplemented __stubs__/normalizeJob.stub.ts:6:9 + 4| + 5| const notImplemented = (): never => { + 6| throw new Error("not implemented — red run"); + | ^ + 7| }; + 8| + ❯ src/server/modules/task-runner/payload-jobs-runner/__tests__/normalizeJobLocales.contract.test.ts:194:12 + +⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[18/19]⎯ + + FAIL src/server/modules/task-runner/payload-jobs-runner/__tests__/normalizeJobLocales.contract.test.ts > normalizeJobLocales > reads a run as running even though it also carries an error +Error: not implemented — red run + ❯ notImplemented __stubs__/normalizeJob.stub.ts:6:9 + 4| + 5| const notImplemented = (): never => { + 6| throw new Error("not implemented — red run"); + | ^ + 7| }; + 8| + ❯ src/server/modules/task-runner/payload-jobs-runner/__tests__/normalizeJobLocales.contract.test.ts:202:12 + +⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[19/19]⎯ + + + Test Files 1 failed (1) + Tests 19 failed (19) + Start at 15:07:15 + Duration 330ms (transform 35ms, setup 0ms, collect 30ms, tests 9ms, environment 0ms, prepare 57ms) + diff --git a/packages/payload-plugin-translator/docs/plans/artifacts/2026-10-14-panel-red-run.txt b/packages/payload-plugin-translator/docs/plans/artifacts/2026-10-14-panel-red-run.txt new file mode 100644 index 000000000..3ed1c5dea --- /dev/null +++ b/packages/payload-plugin-translator/docs/plans/artifacts/2026-10-14-panel-red-run.txt @@ -0,0 +1,20 @@ +⎯⎯⎯⎯⎯⎯⎯ Failed Tests 3 ⎯⎯⎯⎯⎯⎯⎯ + + FAIL src/client/features/collection-translation-progress/ui/CollectionTranslationProgress.test.tsx > the collection progress panel > offers no row for work that finished + FAIL src/client/features/collection-translation-progress/ui/CollectionTranslationProgress.test.tsx > the collection progress panel > still names the work in flight + FAIL src/client/features/collection-translation-progress/ui/CollectionTranslationProgress.test.tsx > the collection progress panel > says how far back the failures it counts go +TypeError: Cannot read properties of undefined (reading 'length') + ❯ src/client/features/collection-translation-progress/ui/CollectionTranslationProgress.tsx:63:33 + 61|
    + 62| {ROWS.map(({ key, label, color }) => { + 63| const count = data[key].length; + | ^ + 64| if (count === 0) { + 65| return null; + ❯ CollectionTranslationProgress src/client/features/collection-translation-progress/ui/CollectionTranslationProgress.tsx:62:13 + ❯ Object.react_stack_bottom_frame ../../node_modules/react-dom/cjs/react-dom-client.development.js:25904:20 + ❯ renderWithHooks ../../node_modules/react-dom/cjs/react-dom-client.development.js:7662:22 + ❯ updateFunctionComponent ../../node_modules/react-dom/cjs/react-dom-client.development.js:10166:19 + ❯ beginWork ../../node_modules/react-dom/cjs/react-dom-client.development.js:11778:18 + ❯ runWithFiberInDEV ../../node_modules/react-dom/cjs/react-dom-client.development.js:874:13 + ❯ performUnitOfWork ../../node_modules/react-dom/cjs/react-dom-client.development.js:17641:22 diff --git a/packages/payload-plugin-translator/package.json b/packages/payload-plugin-translator/package.json index 735752785..64dcd88ab 100644 --- a/packages/payload-plugin-translator/package.json +++ b/packages/payload-plugin-translator/package.json @@ -83,9 +83,12 @@ "@payloadcms/ui": "3.90.1", "@swc/cli": "0.4.0", "@swc/core": "1.7.0", + "@testing-library/dom": "10.4.1", + "@testing-library/react": "16.3.0", "@types/react": "19.2.9", "@vitest/coverage-v8": "3.2.4", "copyfiles": "2.4.1", + "jsdom": "26.1.0", "payload": "3.90.1", "react": "19.0.0", "typescript": "5.5.2", diff --git a/packages/payload-plugin-translator/src/client/entities/translation/api/queries/useCollectionTranslationStatus.ts b/packages/payload-plugin-translator/src/client/entities/translation/api/queries/useCollectionTranslationStatus.ts index dfee3b70e..7ff04cf2e 100644 --- a/packages/payload-plugin-translator/src/client/entities/translation/api/queries/useCollectionTranslationStatus.ts +++ b/packages/payload-plugin-translator/src/client/entities/translation/api/queries/useCollectionTranslationStatus.ts @@ -7,8 +7,9 @@ import { useTranslateKitConfig } from "../../../../app/config/index.js"; import { handleNextApiError } from "../../../../shared/lib/errors/handleApiError.js"; import { DocumentTranslationStatus } from "../../model/enums.js"; import type { + CollectionTranslationPanel, CollectionTranslationStatus, - GroupedCollectionTranslationStatus, + ReportedStatus, } from "../../model/types.js"; type Props = { @@ -29,7 +30,7 @@ export function useCollectionTranslationStatus({ collection }: Props, options?: const queryClient = useQueryClient(); const { basePath } = useTranslateKitConfig(); - const query = useQuery({ + const query = useQuery({ queryKey: getCollectionTranslationQueryKey({ collection }), queryFn: async ({ signal }) => { return handleNextApiError(async () => { @@ -42,25 +43,25 @@ export function useCollectionTranslationStatus({ collection }: Props, options?: }); }, enabled: options?.enabled, - refetchInterval: (query) => { - const docs = query.state.data?.docs; - const pending = docs?.some((doc) => doc.status === DocumentTranslationStatus.PENDING); - const running = docs?.some((doc) => doc.status === DocumentTranslationStatus.RUNNING); - return pending || running ? POLLING_INTERVAL_MILLISECONDS : false; + refetchInterval: (current) => { + const docs = current.state.data?.docs ?? []; + const inFlight = docs.some( + (doc) => + doc.status === DocumentTranslationStatus.PENDING || + doc.status === DocumentTranslationStatus.RUNNING + ); + return inFlight ? POLLING_INTERVAL_MILLISECONDS : false; }, - select: (data): GroupedCollectionTranslationStatus => { - const grouped: GroupedCollectionTranslationStatus = { - [DocumentTranslationStatus.PENDING]: [], - [DocumentTranslationStatus.RUNNING]: [], - [DocumentTranslationStatus.COMPLETED]: [], - [DocumentTranslationStatus.FAILED]: [], + select: (data): CollectionTranslationPanel => { + const counts: Record = { + [DocumentTranslationStatus.PENDING]: 0, + [DocumentTranslationStatus.RUNNING]: 0, + [DocumentTranslationStatus.FAILED]: 0, }; - - data.docs.forEach((doc) => { - if (grouped[doc.status]) grouped[doc.status].push(doc); - }); - - return grouped; + for (const doc of data.docs) { + if (doc.status in counts) counts[doc.status as ReportedStatus] += 1; + } + return { counts, failedWindowMs: data.failed_window_ms }; }, }); diff --git a/packages/payload-plugin-translator/src/client/entities/translation/index.ts b/packages/payload-plugin-translator/src/client/entities/translation/index.ts index db653d017..152b32991 100644 --- a/packages/payload-plugin-translator/src/client/entities/translation/index.ts +++ b/packages/payload-plugin-translator/src/client/entities/translation/index.ts @@ -29,7 +29,8 @@ export type { DocumentTranslationRunning, CollectionTranslationStatus, CollectionTranslationStatusItem, - GroupedCollectionTranslationStatus, + CollectionTranslationPanel, + ReportedStatus, DocumentStaleness, StalenessLocale, } from "./model/types.js"; diff --git a/packages/payload-plugin-translator/src/client/entities/translation/model/panelStatus.test.ts b/packages/payload-plugin-translator/src/client/entities/translation/model/panelStatus.test.ts index db18ccdf8..f4ea5929d 100644 --- a/packages/payload-plugin-translator/src/client/entities/translation/model/panelStatus.test.ts +++ b/packages/payload-plugin-translator/src/client/entities/translation/model/panelStatus.test.ts @@ -9,7 +9,7 @@ import { MARKER_DOT, } from "./panelStatus.js"; import { STATE_DOT } from "./statusRows.js"; -import type { DocumentTranslation, GroupedCollectionTranslationStatus } from "./types.js"; +import type { CollectionTranslationPanel, DocumentTranslation, ReportedStatus } from "./types.js"; describe("derivePanelStatus", () => { it("failed outranks everything, including stale locales", () => { @@ -97,68 +97,40 @@ describe("deriveDocumentRunStatus", () => { describe("deriveCollectionPanelStatus", () => { const group = ( - over: Partial = {} - ): GroupedCollectionTranslationStatus => - ({ - [DocumentTranslationStatus.COMPLETED]: [], - [DocumentTranslationStatus.FAILED]: [], - [DocumentTranslationStatus.RUNNING]: [], - [DocumentTranslationStatus.PENDING]: [], + over: Partial> = {} + ): CollectionTranslationPanel => ({ + counts: { + [DocumentTranslationStatus.FAILED]: 0, + [DocumentTranslationStatus.RUNNING]: 0, + [DocumentTranslationStatus.PENDING]: 0, ...over, - }) as GroupedCollectionTranslationStatus; + }, + failedWindowMs: 259_200_000, + }); it("in-progress (running or pending) outranks failures", () => { expect( deriveCollectionPanelStatus( group({ - [DocumentTranslationStatus.RUNNING]: [ - { id: "a", status: DocumentTranslationStatus.RUNNING }, - ], - [DocumentTranslationStatus.FAILED]: [ - { id: "b", status: DocumentTranslationStatus.FAILED }, - ], + [DocumentTranslationStatus.RUNNING]: 1, + [DocumentTranslationStatus.FAILED]: 1, }) ) ).toEqual({ kind: "running" }); expect( deriveCollectionPanelStatus( group({ - [DocumentTranslationStatus.PENDING]: [ - { id: "a", status: DocumentTranslationStatus.PENDING }, - ], - [DocumentTranslationStatus.FAILED]: [ - { id: "b", status: DocumentTranslationStatus.FAILED }, - ], + [DocumentTranslationStatus.PENDING]: 1, + [DocumentTranslationStatus.FAILED]: 1, }) ) ).toEqual({ kind: "running" }); }); it("failed when something failed and nothing is in progress", () => { - expect( - deriveCollectionPanelStatus( - group({ - [DocumentTranslationStatus.FAILED]: [ - { id: "b", status: DocumentTranslationStatus.FAILED }, - ], - [DocumentTranslationStatus.COMPLETED]: [ - { id: "c", status: DocumentTranslationStatus.COMPLETED }, - ], - }) - ) - ).toEqual({ kind: "failed" }); - }); - - it("fresh when only completed", () => { - expect( - deriveCollectionPanelStatus( - group({ - [DocumentTranslationStatus.COMPLETED]: [ - { id: "c", status: DocumentTranslationStatus.COMPLETED }, - ], - }) - ) - ).toEqual({ kind: "fresh" }); + expect(deriveCollectionPanelStatus(group({ [DocumentTranslationStatus.FAILED]: 1 }))).toEqual({ + kind: "failed", + }); }); it("none when empty or undefined", () => { diff --git a/packages/payload-plugin-translator/src/client/entities/translation/model/panelStatus.ts b/packages/payload-plugin-translator/src/client/entities/translation/model/panelStatus.ts index 079bcc727..26276de01 100644 --- a/packages/payload-plugin-translator/src/client/entities/translation/model/panelStatus.ts +++ b/packages/payload-plugin-translator/src/client/entities/translation/model/panelStatus.ts @@ -1,6 +1,6 @@ import { DocumentTranslationStatus } from "./enums.js"; import type { StatusDotColor } from "./statusRows.js"; -import type { DocumentTranslation, GroupedCollectionTranslationStatus } from "./types.js"; +import type { CollectionTranslationPanel, DocumentTranslation } from "./types.js"; /** * The single aggregate signal shown on a translate trigger (document or collection). Detail lives in @@ -102,17 +102,15 @@ export function derivePanelStatus({ runStatus, staleLocales }: PanelStatusInput) } /** - * Collapse a collection's grouped job counts into one trigger marker. Priority: - * `in-progress (running/pending) → failed → fresh → none`. In-progress outranks failures because - * work is actively happening and self-resolves; once nothing is running, a failure is surfaced. - * There is no `stale` here — staleness is a per-document signal, not tracked for the bulk dashboard. + * The one marker a collection's trigger shows. Work in flight outranks a failure: it self-resolves, + * so a failure surfaces only once nothing is running. */ export function deriveCollectionPanelStatus( - status: GroupedCollectionTranslationStatus | undefined + panel: CollectionTranslationPanel | undefined ): PanelStatus { - if (!status) return { kind: "none" }; - if (status.running.length > 0 || status.pending.length > 0) return { kind: "running" }; - if (status.failed.length > 0) return { kind: "failed" }; - if (status.completed.length > 0) return { kind: "fresh" }; + if (!panel) return { kind: "none" }; + const { counts } = panel; + if (counts.running > 0 || counts.pending > 0) return { kind: "running" }; + if (counts.failed > 0) return { kind: "failed" }; return { kind: "none" }; } diff --git a/packages/payload-plugin-translator/src/client/entities/translation/model/types.ts b/packages/payload-plugin-translator/src/client/entities/translation/model/types.ts index 7926e1167..8646245bc 100644 --- a/packages/payload-plugin-translator/src/client/entities/translation/model/types.ts +++ b/packages/payload-plugin-translator/src/client/entities/translation/model/types.ts @@ -56,13 +56,21 @@ export type CollectionTranslationStatusItem = { id: string; status: DocumentTran export type CollectionTranslationStatus = { docs: Array; + failed_window_ms: number; }; -export type GroupedCollectionTranslationStatus = Record< +/** What the collection panel reports on: work under way, never work already finished. */ +export type ReportedStatus = Exclude< DocumentTranslationStatus, - CollectionTranslationStatusItem[] + DocumentTranslationStatus.COMPLETED >; +/** `counts.failed` reaches back only `failedWindowMs`, not over all time. */ +export type CollectionTranslationPanel = { + counts: Record; + failedWindowMs: number; +}; + /** One translated target locale's staleness relative to its source (#50). */ export type StalenessLocale = { target_lng: string; diff --git a/packages/payload-plugin-translator/src/client/features/collection-translation-progress/ui/CollectionTranslationProgress.test.tsx b/packages/payload-plugin-translator/src/client/features/collection-translation-progress/ui/CollectionTranslationProgress.test.tsx new file mode 100644 index 000000000..90dfe82fa --- /dev/null +++ b/packages/payload-plugin-translator/src/client/features/collection-translation-progress/ui/CollectionTranslationProgress.test.tsx @@ -0,0 +1,76 @@ +// @vitest-environment jsdom +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import { render, screen } from "@testing-library/react"; +import { describe, expect, it, vi } from "vitest"; + +import { DocumentTranslationStatus } from "../../../entities/translation/model/enums.js"; +import type { CollectionTranslationPanel } from "../../../entities/translation/model/types.js"; + +import { CollectionTranslationProgress } from "./CollectionTranslationProgress.js"; + +// `@payloadcms/ui` ships CSS imports Vitest's resolver cannot load, so the two leaves used here are stubbed. +vi.mock("@payloadcms/ui", () => ({ ShimmerEffect: () => null })); +vi.mock("@payloadcms/ui/icons/Trash", () => ({ TrashIcon: () => null })); + +const panel = ( + over: Partial> = {} +): CollectionTranslationPanel => ({ + counts: { + [DocumentTranslationStatus.PENDING]: 0, + [DocumentTranslationStatus.RUNNING]: 0, + [DocumentTranslationStatus.COMPLETED]: 0, + [DocumentTranslationStatus.FAILED]: 0, + ...over, + }, + failedWindowMs: 259_200_000, +}); + +const panelWithWindow = (ms: number): CollectionTranslationPanel => ({ + ...panel({ [DocumentTranslationStatus.FAILED]: 1 }), + failedWindowMs: ms, +}); + +const show = (data: CollectionTranslationPanel) => + render( + + + + ); + +describe("the collection progress panel", () => { + it("offers no row for work that finished", () => { + show(panel({ [DocumentTranslationStatus.COMPLETED]: 7 })); + + expect( + screen.queryByText(/translated/iu), + "a collection-wide total is a statistic nobody acts on; the document's own status answers it" + ).toBeNull(); + }); + + it("still names the work in flight", () => { + show(panel({ [DocumentTranslationStatus.RUNNING]: 2, [DocumentTranslationStatus.PENDING]: 5 })); + + expect(screen.queryByText(/translating/iu)).not.toBeNull(); + expect(screen.queryByText(/queued/iu)).not.toBeNull(); + }); + + it("says how far back the failures it counts go", () => { + show(panel({ [DocumentTranslationStatus.FAILED]: 4 })); + + expect( + screen.queryByText(/3 days/iu), + "without the window the number reads as every failure there has ever been" + ).not.toBeNull(); + }); + + it.each([ + [36 * 60 * 60 * 1000, /1 day 12 hours/iu, "two units, not flattened to the larger one"], + [24 * 60 * 60 * 1000, /\b1 day\b/iu, "a lone unit is singular"], + [90 * 60 * 1000, /1 hour 30 minutes/iu, "units smaller than a day are spelled too"], + [30 * 1000, /under a minute/iu, "a span too short to name still reads as a span"], + ])("spells a %i ms window in the units it divides into", (ms, phrase, why) => { + show(panelWithWindow(ms)); + + expect(screen.queryByText(phrase), why).not.toBeNull(); + }); +}); diff --git a/packages/payload-plugin-translator/src/client/features/collection-translation-progress/ui/CollectionTranslationProgress.tsx b/packages/payload-plugin-translator/src/client/features/collection-translation-progress/ui/CollectionTranslationProgress.tsx index 09afa0afe..a6c70e396 100644 --- a/packages/payload-plugin-translator/src/client/features/collection-translation-progress/ui/CollectionTranslationProgress.tsx +++ b/packages/payload-plugin-translator/src/client/features/collection-translation-progress/ui/CollectionTranslationProgress.tsx @@ -3,7 +3,10 @@ import { ShimmerEffect } from "@payloadcms/ui"; import { TrashIcon } from "@payloadcms/ui/icons/Trash"; -import type { GroupedCollectionTranslationStatus } from "../../../entities/translation/index.js"; +import type { + CollectionTranslationPanel, + ReportedStatus, +} from "../../../entities/translation/index.js"; import { ActionButton, DocumentTranslationStatus, @@ -15,27 +18,39 @@ import styles from "./styles.module.scss"; type CollectionTranslationProgressProps = { collection: string; - data: GroupedCollectionTranslationStatus | undefined; + data: CollectionTranslationPanel | undefined; isPending: boolean; }; -// Same colour + label vocabulary as the document status list (StatusBadge STATE_LABEL). Only the -// actively-processing group pulses — Queued is waiting, not working, so it stays static. const ROWS: Array<{ - key: DocumentTranslationStatus; + key: ReportedStatus; label: string; color: "blue" | "gray" | "green" | "red"; }> = [ { key: DocumentTranslationStatus.RUNNING, label: "Translating", color: "blue" }, { key: DocumentTranslationStatus.PENDING, label: "Queued", color: "gray" }, - { key: DocumentTranslationStatus.COMPLETED, label: "Translated", color: "green" }, { key: DocumentTranslationStatus.FAILED, label: "Failed", color: "red" }, ]; -/** - * Collection status as a compact list of grouped counts — one `[dot] label … count` row per - * non-zero group, mirroring the document popup's per-locale list rhythm (dot vocabulary shared). - */ +const UNITS = [ + { ms: 24 * 60 * 60 * 1000, one: "day", many: "days" }, + { ms: 60 * 60 * 1000, one: "hour", many: "hours" }, + { ms: 60 * 1000, one: "minute", many: "minutes" }, +] as const; + +/** Spelled in the units it divides into — "1 day 12 hours" rather than "36 hours". */ +const spanOf = (ms: number): string => { + const said: string[] = []; + let left = ms; + for (const unit of UNITS) { + const count = Math.floor(left / unit.ms); + if (count > 0) said.push(`${count} ${count === 1 ? unit.one : unit.many}`); + left -= count * unit.ms; + } + return said.length > 0 ? said.join(" ") : "under a minute"; +}; + +/** What the collection is doing now, one row per non-zero count. */ export function CollectionTranslationProgress({ collection, data, @@ -60,7 +75,7 @@ export function CollectionTranslationProgress({ return (
      {ROWS.map(({ key, label, color }) => { - const count = data[key].length; + const count = data.counts[key]; if (count === 0) { return null; } @@ -90,7 +105,9 @@ export function CollectionTranslationProgress({ {/* No per-doc error data in the grouped status API — point the user to where detail lives. */} {key === DocumentTranslationStatus.FAILED && ( -

      Open a document to see why

      +

      + Last {spanOf(data.failedWindowMs)} — open a document to see why +

      )} ); diff --git a/packages/payload-plugin-translator/src/client/widgets/bulk-translation-dashboard/ui/BulkTranslationDashboard.tsx b/packages/payload-plugin-translator/src/client/widgets/bulk-translation-dashboard/ui/BulkTranslationDashboard.tsx index 41ba12de6..9aa5a844b 100644 --- a/packages/payload-plugin-translator/src/client/widgets/bulk-translation-dashboard/ui/BulkTranslationDashboard.tsx +++ b/packages/payload-plugin-translator/src/client/widgets/bulk-translation-dashboard/ui/BulkTranslationDashboard.tsx @@ -71,10 +71,7 @@ export default function BulkTranslationDashboard({ const panelStatus = deriveCollectionPanelStatus(status); const hasAnyCount = !!( status && - (status.completed.length || - status.failed.length || - status.running.length || - status.pending.length) + (status.counts.failed || status.counts.running || status.counts.pending) ); const showStatus = collectionTranslationApi.isPending || hasAnyCount; diff --git a/packages/payload-plugin-translator/src/composition/levels/collectionLevel.ts b/packages/payload-plugin-translator/src/composition/levels/collectionLevel.ts index 9de24f812..ce85d4d50 100644 --- a/packages/payload-plugin-translator/src/composition/levels/collectionLevel.ts +++ b/packages/payload-plugin-translator/src/composition/levels/collectionLevel.ts @@ -22,7 +22,7 @@ export function collectionLevel(): TranslationLevel { useDocTranslationApi(ctx); ctx.addCollectionComponent( "beforeListTable", - () => new BulkDocumentTranslationDashboard(ctx.access, ctx.targetSelection) + () => new BulkDocumentTranslationDashboard(ctx.access, ctx.settings.targetSelection) ); }, }; diff --git a/packages/payload-plugin-translator/src/composition/levels/documentLevel.ts b/packages/payload-plugin-translator/src/composition/levels/documentLevel.ts index e124319ba..f6aea0f1a 100644 --- a/packages/payload-plugin-translator/src/composition/levels/documentLevel.ts +++ b/packages/payload-plugin-translator/src/composition/levels/documentLevel.ts @@ -26,7 +26,8 @@ export function documentLevel(): TranslationLevel { useDocTranslationApi(ctx); ctx.addCollectionComponent( "beforeDocumentControls", - (collection) => new TranslateDocumentExport(collection, ctx.access, ctx.targetSelection) + (collection) => + new TranslateDocumentExport(collection, ctx.access, ctx.settings.targetSelection) ); }, }; diff --git a/packages/payload-plugin-translator/src/composition/levels/fieldLevel.test.ts b/packages/payload-plugin-translator/src/composition/levels/fieldLevel.test.ts index 765dbe8bc..c9371003d 100644 --- a/packages/payload-plugin-translator/src/composition/levels/fieldLevel.test.ts +++ b/packages/payload-plugin-translator/src/composition/levels/fieldLevel.test.ts @@ -3,6 +3,7 @@ import type { CollectionSlug, Field } from "payload"; import type { CollectionSchemaMap } from "../../types/CollectionSchemaMap.js"; +import { resolveSettings } from "../../settings.js"; import { fieldLevel } from "./fieldLevel.js"; import type { LevelContext } from "../../server/modules/translation-levels/index.js"; @@ -11,15 +12,13 @@ import type { LevelContext } from "../../server/modules/translation-levels/index // contract test (route.test.ts), so we don't re-assert it here. const makeCtx = (): LevelContext => ({ collections: [], - basePath: "/translate", + settings: resolveSettings({}), access: undefined, taskRunnerFactory: { create: vi.fn() }, schemaMap: new Map([ ["posts" as CollectionSlug, [{ name: "title", type: "text" }] as Field[]], ]) as CollectionSchemaMap, translationProvider: { translate: vi.fn() }, - targetSelection: "single", - inlineMarks: false, addEndpoints: vi.fn(), addCollectionComponent: vi.fn(), }); diff --git a/packages/payload-plugin-translator/src/composition/levels/fieldLevel.ts b/packages/payload-plugin-translator/src/composition/levels/fieldLevel.ts index 8540a95f8..17d4f3336 100644 --- a/packages/payload-plugin-translator/src/composition/levels/fieldLevel.ts +++ b/packages/payload-plugin-translator/src/composition/levels/fieldLevel.ts @@ -34,8 +34,8 @@ export function fieldLevel(): TranslationLevel { schemaMap: ctx.schemaMap, translationProvider: ctx.translationProvider, access: ctx.access, - basePath: ctx.basePath, - inlineMarks: ctx.inlineMarks, + basePath: ctx.settings.basePath, + inlineMarks: ctx.settings.inlineMarks, }), ]); }, diff --git a/packages/payload-plugin-translator/src/composition/levels/levels.test.ts b/packages/payload-plugin-translator/src/composition/levels/levels.test.ts index 5ec037244..77497fe56 100644 --- a/packages/payload-plugin-translator/src/composition/levels/levels.test.ts +++ b/packages/payload-plugin-translator/src/composition/levels/levels.test.ts @@ -4,6 +4,7 @@ import type { CollectionConfig } from "payload"; import { TranslateDocumentExport } from "../../client/widgets/translate-document/index.js"; import { BulkDocumentTranslationDashboard } from "../../client/widgets/bulk-translation-dashboard/ui/BulkTranslationDashboard.export.js"; +import { resolveSettings } from "../../settings.js"; import { documentLevel } from "./documentLevel.js"; import { collectionLevel } from "./collectionLevel.js"; import type { @@ -29,13 +30,11 @@ const componentFor = (ctx: LevelContext, slot: CollectionAdminSlot) => { // we only assert the wiring (which primitive, which slot). const makeCtx = (): LevelContext => ({ collections: [{ slug: "posts", fields: [] } as unknown as CollectionConfig], - basePath: "/translate", + settings: resolveSettings({}), access: undefined, taskRunnerFactory: { create: vi.fn() }, schemaMap: new Map(), translationProvider: { translate: vi.fn() }, - targetSelection: "single", - inlineMarks: false, addEndpoints: vi.fn(), addCollectionComponent: vi.fn(), }); diff --git a/packages/payload-plugin-translator/src/composition/levels/useDocTranslationApi.ts b/packages/payload-plugin-translator/src/composition/levels/useDocTranslationApi.ts index 5545f3cc8..5648f60fc 100644 --- a/packages/payload-plugin-translator/src/composition/levels/useDocTranslationApi.ts +++ b/packages/payload-plugin-translator/src/composition/levels/useDocTranslationApi.ts @@ -17,7 +17,7 @@ export function useDocTranslationApi(ctx: LevelContext): void { availableCollections: new Set(ctx.collections.map((collection) => collection.slug)), }, access: ctx.access, - basePath: ctx.basePath, + settings: ctx.settings, provenanceServiceFactory: ctx.provenanceServiceFactory, }) ); diff --git a/packages/payload-plugin-translator/src/plugin.ts b/packages/payload-plugin-translator/src/plugin.ts index 286e88ec1..14fcd22f9 100644 --- a/packages/payload-plugin-translator/src/plugin.ts +++ b/packages/payload-plugin-translator/src/plugin.ts @@ -5,7 +5,6 @@ import { configureAutoTranslate } from "./server/modules/auto-translate/index.js import { configureProvenance } from "./server/modules/provenance/index.js"; import type { AccessGuard } from "./types/AccessGuard.js"; import type { CollectionSchemaMap } from "./types/CollectionSchemaMap.js"; -import type { TargetSelectionMode } from "./types/TargetSelection.js"; import { projectFieldsToFieldLike } from "./core/kernel/field-traversal/index.js"; import type { TranslationProvider } from "./core/domain/translation-providers/index.js"; import type { TaskRunnerProvider } from "./server/modules/task-runner/index.js"; @@ -14,9 +13,10 @@ import type { TranslationLifecycleCallbacks } from "./server/modules/lifecycle/i import { documentLevel, collectionLevel } from "./composition/levels/index.js"; import type { TranslationLevel } from "./server/modules/translation-levels/index.js"; import { PluginConfigBuilder } from "./server/modules/translation-levels/PluginConfigBuilder.js"; -import { normalizePath } from "./server/shared/index.js"; +import { resolveSettings } from "./settings.js"; +import type { TranslatorSettingsOptions } from "./settings.js"; -export type TranslatorPluginConfig = { +export type TranslatorPluginConfig = TranslatorSettingsOptions & { /** * Original collection configs (same objects passed to buildConfig). * IMPORTANT: Must be the original collection objects, NOT sanitized configs from payload.collections. @@ -43,13 +43,6 @@ export type TranslatorPluginConfig = { * @since 0.14.0 — previously optional, defaulting to open. */ access: AccessGuard; - /** - * Base path for all translation API endpoints. - * Useful to avoid conflicts with existing routes. - * @example '/my-translate' results in '/my-translate/enqueue', '/my-translate/run/:id', etc. - * @default '/translate' - */ - basePath?: string; /** * Which translation surfaces to enable, as level factories — `documentLevel()`, * `collectionLevel()`, `fieldLevel()`. Omit for the @@ -87,41 +80,6 @@ export type TranslatorPluginConfig = { * @since 0.7.0 */ lifecycle?: TranslationLifecycleCallbacks; - /** - * How the target-language field behaves in the translation forms. `'single'` (default) keeps - * today's one-locale-per-run behaviour, byte-identical. `'multi'` renders a compact multi-select so - * an editor can queue several target locales in one run — the run fans out one translation per - * `(document × target locale)`. Opt-in and fully backward-compatible; no schema, no migration. - * @default 'single' - * @since 0.10.0 - */ - targetSelection?: TargetSelectionMode; - /** - * Where switches for behaviour on its way to becoming the default live. The option itself is - * permanent; each **entry** is deprecated the day it ships, because it exists only so you can - * adopt a change early and the next major removes the switch, not the behaviour. - * - * @since 0.13.0 - */ - experimental?: { - /** - * Translate each rich-text container (paragraph, heading, list item) as one string with - * numbered inline marks, instead of node by node. - * - * Requires a provider declaring `capabilities.inlineMarks`; without one the plugin keeps - * translating node by node. A container whose reply comes back with damaged marks is left in - * its source language rather than half-written. - * - * Turning it back off stops future translations from splitting wrappers; documents already - * translated under it stay split. And the next major removes the flag itself, so it is a - * schedule, not a permanent safety valve. - * - * @default false - * @deprecated Transitional. Removed in the next major, when this becomes the only mode. - * @see docs/DEPRECATIONS.md#experimental-inline-marks — why this exists and when it goes - */ - inlineMarks?: boolean; - }; }; /** @deprecated Use `TranslatorPluginConfig` instead */ @@ -137,18 +95,9 @@ export class TranslateCollectionPlugin { init(): (config: Config) => Promise { return async (config) => { - const { - access, - translationProvider, - runner, - collections, - levels, - provenance, - lifecycle, - targetSelection = "single", - experimental, - basePath: rawBasePath = "/translate", - } = this.pluginConfig; + const { access, translationProvider, runner, collections, levels, provenance, lifecycle } = + this.pluginConfig; + const settings = resolveSettings(this.pluginConfig); if (!access) { throw new Error( @@ -161,13 +110,11 @@ export class TranslateCollectionPlugin { collections.map((col) => [col.slug, projectFieldsToFieldLike(col.fields)]) ); const collectionSlugs = new Set(schemaMap.keys()); - const basePath = normalizePath(rawBasePath); const provenanceModule = configureProvenance(provenance, schemaMap); - const inlineMarks = experimental?.inlineMarks === true; const { taskRunnerFactory, configModifier: runnerConfigModifier } = wireTranslateRunner({ - inlineMarks, + inlineMarks: settings.inlineMarks, translationProvider, schemaMap, provenanceServiceFactory: provenanceModule.serviceFactory, @@ -180,21 +127,19 @@ export class TranslateCollectionPlugin { const activeLevels = levels ?? [documentLevel(), collectionLevel()]; const builder = new PluginConfigBuilder({ collections, - basePath, + settings, access, taskRunnerFactory, schemaMap, translationProvider, provenanceServiceFactory: provenanceModule.serviceFactory, - targetSelection, - inlineMarks, }); for (const level of activeLevels) level.extend(builder); builder.addConfigModifier(runnerConfigModifier); builder.addConfigModifier(provenanceModule.configure(collectionSlugs)); builder.addConfigModifier(autoTranslateModule.configure(collectionSlugs)); - builder.addAdminProvider(new CacheProviderExport(basePath)); + builder.addAdminProvider(new CacheProviderExport(settings.basePath)); return builder.applyTo(config); }; diff --git a/packages/payload-plugin-translator/src/server/features/cancel-by-collection/handler.ts b/packages/payload-plugin-translator/src/server/features/cancel-by-collection/handler.ts index 15316c4a4..2a0cb0359 100644 --- a/packages/payload-plugin-translator/src/server/features/cancel-by-collection/handler.ts +++ b/packages/payload-plugin-translator/src/server/features/cancel-by-collection/handler.ts @@ -30,8 +30,8 @@ export class CancelByCollectionHandler { const rows = await runner.findByCollection(collectionSlug, { excludeCompleted: true }); if (rows.length === 0) return ServerResponse.noContent(); - // A job waiting to retry has every locale logged, so it has no `pending` row — filter by *not - // running* instead. + // A job is cancellable only while none of its locales is in flight, so this excludes rather + // than selects: one running locale protects the whole job. const running = new Set(rows.filter((row) => row.status === "running").map((row) => row.id)); const queuedJobIds = [...new Set(rows.map((row) => row.id))].filter((id) => !running.has(id)); if (queuedJobIds.length === 0) return ServerResponse.noContent(); diff --git a/packages/payload-plugin-translator/src/server/features/createTranslationRoutes.test.ts b/packages/payload-plugin-translator/src/server/features/createTranslationRoutes.test.ts index e2b783b19..e11188891 100644 --- a/packages/payload-plugin-translator/src/server/features/createTranslationRoutes.test.ts +++ b/packages/payload-plugin-translator/src/server/features/createTranslationRoutes.test.ts @@ -1,5 +1,6 @@ import { describe, it, expect, vi } from "vitest"; +import { resolveSettings } from "../../settings.js"; import { createTranslationRoutes } from "./createTranslationRoutes.js"; import type { TranslationRoutesDeps } from "./createTranslationRoutes.js"; @@ -10,7 +11,7 @@ import type { TranslationRoutesDeps } from "./createTranslationRoutes.js"; const deps = (basePath?: string): TranslationRoutesDeps => ({ taskRunnerFactory: { create: vi.fn() }, collectionConfig: { availableCollections: new Set() }, - basePath, + settings: resolveSettings({ basePath }), }); describe("createTranslationRoutes (bundle contract)", () => { diff --git a/packages/payload-plugin-translator/src/server/features/createTranslationRoutes.ts b/packages/payload-plugin-translator/src/server/features/createTranslationRoutes.ts index 717c42e85..cffd11f61 100644 --- a/packages/payload-plugin-translator/src/server/features/createTranslationRoutes.ts +++ b/packages/payload-plugin-translator/src/server/features/createTranslationRoutes.ts @@ -12,11 +12,10 @@ import { createGetDocumentStalenessRoute, createDismissStalenessRoute } from "./ export type TranslationRoutesDeps = Pick< TranslationContext, - "taskRunnerFactory" | "access" | "provenanceServiceFactory" + "taskRunnerFactory" | "access" | "provenanceServiceFactory" | "settings" > & { /** Collections the plugin manages — gates which slugs the routes accept. */ collectionConfig: { availableCollections: Set }; - basePath?: string; }; /** @@ -35,9 +34,10 @@ export function createTranslationRoutes({ taskRunnerFactory, collectionConfig, access, - basePath, + settings, provenanceServiceFactory, }: TranslationRoutesDeps): Endpoint[] { + const { basePath, failedWindowMs } = settings; const stalenessConfig = { availableCollections: collectionConfig.availableCollections, provenanceServiceFactory, @@ -48,7 +48,12 @@ export function createTranslationRoutes({ createCancelRoute(taskRunnerFactory, access, basePath), createCancelByCollectionRoute(collectionConfig, taskRunnerFactory, access, basePath), createGetDocumentStatusRoute(collectionConfig, taskRunnerFactory, access, basePath), - createGetCollectionStatusRoute(collectionConfig, taskRunnerFactory, access, basePath), + createGetCollectionStatusRoute( + { ...collectionConfig, failedWindowMs }, + taskRunnerFactory, + access, + basePath + ), createGetDocumentStalenessRoute(stalenessConfig, access, basePath), createDismissStalenessRoute(stalenessConfig, access, basePath), ]; diff --git a/packages/payload-plugin-translator/src/server/features/get-collection-status/handler.test.ts b/packages/payload-plugin-translator/src/server/features/get-collection-status/handler.test.ts index 9b357f872..3b4555aa9 100644 --- a/packages/payload-plugin-translator/src/server/features/get-collection-status/handler.test.ts +++ b/packages/payload-plugin-translator/src/server/features/get-collection-status/handler.test.ts @@ -57,6 +57,7 @@ describe("GetCollectionStatusHandler", () => { config = { availableCollections: new Set(["posts", "pages"] as CollectionSlug[]), + failedWindowMs: 259_200_000, }; handler = new GetCollectionStatusHandler(config, mockTaskRunnerFactory); @@ -103,7 +104,7 @@ describe("GetCollectionStatusHandler", () => { expect(response.status).toBe(200); const body = await response.json(); - expect(body.data).toEqual({ docs: [] }); + expect(body.data).toEqual({ docs: [], failed_window_ms: 259_200_000 }); }); it("asks which documents are visible once, not once per row", async () => { @@ -152,7 +153,13 @@ describe("GetCollectionStatusHandler", () => { await handler.handle(req); - expect(mockTaskRunner.findByCollection).toHaveBeenCalledWith("pages"); + expect( + mockTaskRunner.findByCollection, + "the panel asks what the collection is doing: work in flight, plus failures recent enough to act on" + ).toHaveBeenCalledWith("pages", { + excludeCompleted: true, + excludeGivenUpBefore: expect.any(Date), + }); }); it("creates task runner with request payload", async () => { diff --git a/packages/payload-plugin-translator/src/server/features/get-collection-status/handler.ts b/packages/payload-plugin-translator/src/server/features/get-collection-status/handler.ts index 89199d97c..260bd7f04 100644 --- a/packages/payload-plugin-translator/src/server/features/get-collection-status/handler.ts +++ b/packages/payload-plugin-translator/src/server/features/get-collection-status/handler.ts @@ -7,9 +7,7 @@ import { isCollectionAvailable, visibleIds } from "../_lib/collection-utils.js"; import { GetCollectionStatusInputSchema } from "./model.js"; import type { GetCollectionStatusConfig } from "./model.js"; -/** - * Gets translation status for all documents in a collection - */ +/** What a collection is doing: runs in flight, plus failures inside the configured window. */ export class GetCollectionStatusHandler { private readonly config: GetCollectionStatusConfig; private readonly taskRunnerFactory: TaskRunnerFactory; @@ -34,12 +32,17 @@ export class GetCollectionStatusHandler { return ServerResponse.badRequest("Collection not available for translation"); } + const { failedWindowMs } = this.config; const runner = this.taskRunnerFactory.create(req.payload); - const tasks = await runner.findByCollection(collectionSlug); + const tasks = await runner.findByCollection(collectionSlug, { + excludeCompleted: true, + excludeGivenUpBefore: new Date(Date.now() - failedWindowMs), + }); const ids = tasks.map((task) => String(task.input.collectionId)); const visible = await visibleIds(req.payload, collectionSlug, ids, req.user); return ServerResponse.success({ + failed_window_ms: failedWindowMs, docs: tasks .filter((task) => visible.has(String(task.input.collectionId))) .map((task) => ({ diff --git a/packages/payload-plugin-translator/src/server/features/get-collection-status/model.ts b/packages/payload-plugin-translator/src/server/features/get-collection-status/model.ts index 69fe0e87c..421a00408 100644 --- a/packages/payload-plugin-translator/src/server/features/get-collection-status/model.ts +++ b/packages/payload-plugin-translator/src/server/features/get-collection-status/model.ts @@ -28,6 +28,8 @@ export type CollectionStatusItem = { */ export type GetCollectionStatusOutput = { docs: CollectionStatusItem[]; + /** The failures in `docs` reach back only this far, in ms. */ + failed_window_ms: number; }; /** @@ -35,4 +37,5 @@ export type GetCollectionStatusOutput = { */ export type GetCollectionStatusConfig = { availableCollections: Set; + failedWindowMs: number; }; diff --git a/packages/payload-plugin-translator/src/server/features/get-collection-status/route.test.ts b/packages/payload-plugin-translator/src/server/features/get-collection-status/route.test.ts index a8fa9dbae..3c76f01bb 100644 --- a/packages/payload-plugin-translator/src/server/features/get-collection-status/route.test.ts +++ b/packages/payload-plugin-translator/src/server/features/get-collection-status/route.test.ts @@ -10,7 +10,7 @@ import type { GetCollectionStatusConfig } from "./model.js"; // Route-contract test: pin path + method + access-guard wiring so the Phase 1 // relocation of route registration stays behaviour-preserving. Handler logic // and the shared wrappers have their own unit tests. -const config: GetCollectionStatusConfig = { availableCollections: new Set() }; +const config: GetCollectionStatusConfig = { availableCollections: new Set(), failedWindowMs: 3 }; const denyAccess: AccessGuard = { check: vi.fn().mockReturnValue(false) }; describe("createGetCollectionStatusRoute (contract)", () => { diff --git a/packages/payload-plugin-translator/src/server/modules/task-runner/TaskRunner.interface.ts b/packages/payload-plugin-translator/src/server/modules/task-runner/TaskRunner.interface.ts index 4f26872d5..e2c3f8729 100644 --- a/packages/payload-plugin-translator/src/server/modules/task-runner/TaskRunner.interface.ts +++ b/packages/payload-plugin-translator/src/server/modules/task-runner/TaskRunner.interface.ts @@ -80,4 +80,12 @@ export type TaskFilter = { * supersede — so it is wider than the `pending` status. */ excludeCompleted?: boolean; + /** + * Runs still in flight are kept however old: the queue gives up only after the last retry, not on + * a failed attempt (`queues/errors/handleTaskError.js`). Reaches the database under the Payload + * Jobs runner. + * + * @since 0.17.0 + */ + excludeGivenUpBefore?: Date; }; diff --git a/packages/payload-plugin-translator/src/server/modules/task-runner/__tests__/TaskRunner.invariants.ts b/packages/payload-plugin-translator/src/server/modules/task-runner/__tests__/TaskRunner.invariants.ts index bc639ef4f..cab832edd 100644 --- a/packages/payload-plugin-translator/src/server/modules/task-runner/__tests__/TaskRunner.invariants.ts +++ b/packages/payload-plugin-translator/src/server/modules/task-runner/__tests__/TaskRunner.invariants.ts @@ -140,10 +140,6 @@ export function assertTaskRunnerContract(name: string, make: MakeTaskRunner): vo expect(reported, "no handle names work to stop, so nothing settled").toEqual([]); }); - - it("answers about no documents, not about all of them", async () => { - expect(await runner.findByCollection(SLUG, [])).toEqual([]); - }); }); describe("a handle is a non-empty string", () => { @@ -472,6 +468,27 @@ export function assertTaskRunnerContract(name: string, make: MakeTaskRunner): vo ).toEqual([]); }); + it("answers about no documents, not about all of them", async () => { + await runner.enqueue([input(documentForThisCheck(1), "de")]); + + expect( + await runner.findByCollection(SLUG, []), + "an empty list names no document, so it cannot mean every document" + ).toEqual([]); + }); + + it("rejects nothing but a run that gave up, whatever the cutoff", async () => { + const live = documentForThisCheck(1); + await runner.enqueue([input(live, "de")]); + + const tasks = await runner.findByCollection(SLUG, { excludeGivenUpBefore: new Date() }); + + expect( + tasks.map((task) => task.input.collectionId), + "a cutoff on give-up is not a cutoff on age" + ).toContain(live); + }); + it("drops tasks that have finished", async () => { await runner.enqueue([input(documentForThisCheck(1), "de")]); diff --git a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsTaskRunner.ts b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsTaskRunner.ts index 7949daf6d..b13277d44 100644 --- a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsTaskRunner.ts +++ b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/PayloadJobsTaskRunner.ts @@ -6,7 +6,14 @@ import { toTaskFilter } from "../toTaskFilter.js"; import type { EnqueueAssignment, Task, TaskInput, RunResult } from "../types.js"; import type { PayloadJobsRunnerConfig } from "./config.js"; import type { PayloadJob, StoredWorkflowInput } from "./store/index.js"; -import { assignmentOf, handleOf, normalizeJobLocales, readCollectionRef } from "./store/index.js"; +import { + assignmentOf, + handleOf, + normalizeJobLocales, + ofCollection, + ofDocuments, + readCollectionRef, +} from "./store/index.js"; import { owedOnCancel, planEnqueue } from "./model/index.js"; import type { RequestShape } from "./model/index.js"; import type { RequestScope } from "../../../shared/payload/RequestScope.shapes.js"; @@ -49,6 +56,10 @@ function requestKey(task: TaskInput, scope: RequestScope): string { return JSON.stringify(requestShape(task, scope)); } +const settledSince = (moment: Date): Where => ({ + or: [{ hasError: { not_equals: true } }, { updatedAt: { greater_than: moment.toISOString() } }], +}); + function documentKey(collectionSlug: string, collectionId: string): string { return `${collectionSlug}\u0000${collectionId}`; } @@ -245,11 +256,6 @@ export class PayloadJobsTaskRunner implements TaskRunner { return { success: true }; } - /** - * A job that exhausted its retries carries `hasError: true` and stays excluded from the picker even - * after its lock is cleared; only a manual `run()` recovers it. - * @returns how many locks were cleared. - */ async reclaimStaleJobs(): Promise { const cutoff = new Date(Date.now() - this.config.staleJobTimeoutMs).toISOString(); const result = await this.payload.update({ @@ -268,10 +274,6 @@ export class PayloadJobsTaskRunner implements TaskRunner { return result.docs.length; } - /** - * `payload.update`, not the adapter write `extendJob` uses, so the jobs collection's `beforeChange` - * hook still runs — it is what keeps a cancelled job cancelled. - */ private async clearPickerBlockers(taskId: string): Promise { await this.payload.update({ collection: this.config.jobsCollection, @@ -287,18 +289,20 @@ export class PayloadJobsTaskRunner implements TaskRunner { return Date.now() - parsed > this.config.staleJobTimeoutMs; } - /** - * Matched in memory: the collection reference has two stored shapes (see `readCollectionRef`), so a - * `where` on `input.collection_slug` would silently drop every pre-migration job. - */ async findByCollection( collectionSlug: CollectionSlug, filter?: Array | TaskFilter ): Promise { - const { documentIds, excludeCompleted } = toTaskFilter(filter); - const where = excludeCompleted ? { completedAt: { exists: false } } : undefined; - const wanted = documentIds?.length ? new Set(documentIds.map(String)) : undefined; - const jobs = await this.findRawJobs(where); + const { documentIds, excludeCompleted, excludeGivenUpBefore } = toTaskFilter(filter); + const wanted = documentIds ? new Set(documentIds.map(String)) : undefined; + if (wanted?.size === 0) return []; + + const narrowed: Where[] = [ofCollection(collectionSlug)]; + if (excludeCompleted) narrowed.push({ completedAt: { exists: false } }); + if (excludeGivenUpBefore) narrowed.push(settledSince(excludeGivenUpBefore)); + if (wanted) narrowed.push(ofDocuments([...wanted])); + + const jobs = await this.findRawJobs({ and: narrowed }); return jobs .filter((job) => { const ref = readCollectionRef(job.input); diff --git a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/__tests__/PayloadJobsTaskRunner.test.ts b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/__tests__/PayloadJobsTaskRunner.test.ts index 4104364a8..19256e2b8 100644 --- a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/__tests__/PayloadJobsTaskRunner.test.ts +++ b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/__tests__/PayloadJobsTaskRunner.test.ts @@ -754,8 +754,7 @@ describe("PayloadJobsTaskRunner", () => { }); }); - it("narrows the SQL where clause by the job's own slugs only", async () => { - // See `findByCollection`'s docblock: a `where` on slug or id drops every pre-migration job. + it("asks the database for the collection and the documents, in both stored shapes", async () => { await runner.findByCollection("posts" as CollectionSlug, [5, 6]); const whereArg = mockPayload.find.mock.calls[0][0].where; @@ -767,28 +766,60 @@ describe("PayloadJobsTaskRunner", () => { { taskSlug: { equals: "translate_document" } }, ], }, + { + and: [ + { + or: [ + { "input.collection_slug": { equals: "posts" } }, + { "input.collection.relationTo": { equals: "posts" } }, + ], + }, + { + or: [ + { "input.collection_id": { in: ["5", 5, "6", 6] } }, + { "input.collection.value": { in: ["5", 5, "6", 6] } }, + ], + }, + ], + }, ], }); - expect(JSON.stringify(whereArg)).not.toContain("collection_id"); - expect(JSON.stringify(whereArg)).not.toContain("collection.value"); }); - it("adds completedAt to the where clause when asked to exclude finished jobs", async () => { - await runner.findByCollection("posts" as CollectionSlug, { excludeCompleted: true }); + it("offers a document id in both forms the store may hold", async () => { + await runner.findByCollection("posts" as CollectionSlug, [5, 6]); const whereArg = mockPayload.find.mock.calls[0][0].where; - expect(whereArg).toEqual({ - and: [ - { - or: [ - { workflowSlug: { equals: "translate_document_locales" } }, - { taskSlug: { equals: "translate_document" } }, - ], - }, - { completedAt: { exists: false } }, + expect( + JSON.stringify(whereArg), + "the legacy relationship stored a numeric collection's id as a number, and a number never equals a string" + ).toContain('"in":["5",5,"6",6]'); + }); + + it("keeps its own-jobs condition when it narrows, rather than replacing it", async () => { + await runner.findByCollection("posts" as CollectionSlug); + + const whereArg = mockPayload.find.mock.calls[0][0].where; + expect( + whereArg.and[0], + "a workflow row carries no taskSlug, so dropping this or returns nothing" + ).toEqual({ + or: [ + { workflowSlug: { equals: "translate_document_locales" } }, + { taskSlug: { equals: "translate_document" } }, ], }); - expect(JSON.stringify(whereArg)).not.toContain("collection_id"); + }); + + it("adds completedAt to the where clause when asked to exclude finished jobs", async () => { + await runner.findByCollection("posts" as CollectionSlug, { excludeCompleted: true }); + + const whereArg = mockPayload.find.mock.calls[0][0].where; + expect(whereArg.and[1].and).toContainEqual({ completedAt: { exists: false } }); + expect( + JSON.stringify(whereArg), + "nothing asked about a document, so nothing narrows by one" + ).not.toContain("collection_id"); }); it("reads the deprecated array form as documentIds", async () => { diff --git a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/__tests__/normalizeJob.test.ts b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/__tests__/normalizeJob.test.ts index dd99662da..888c03006 100644 --- a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/__tests__/normalizeJob.test.ts +++ b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/__tests__/normalizeJob.test.ts @@ -116,10 +116,11 @@ describe("normalizeJob", () => { expect(task.completedAt).toBe("2024-01-01T01:00:00Z"); }); - it("returns failed status when error is set", () => { + it("returns failed status once the queue has given up", () => { const job: PayloadJob = { ...baseJob, error: { message: "Something went wrong" }, + hasError: true, }; const task = normalizeJob(job); expect(task.status).toBe("failed"); @@ -296,7 +297,32 @@ describe("normalizeJobLocales", () => { expect(rows.map((r) => r.input.targetLng)).toEqual(["de", "fr", "es"]); }); - it("reports each locale's own outcome from the job log, not the job's status", () => { + it("does not call a locale failed while its run is still retrying", () => { + const [de] = normalizeJobLocales({ + ...workflowJob, + input: { ...workflowJob.input, target_lngs: ["de"] }, + log: [{ state: "failed", input: { target_lng: "de" } }], + } as PayloadJob); + + expect( + de?.status, + "a locale whose last attempt failed but which will be tried again is still work in flight" + ).toBe("pending"); + }); + + it("calls a locale failed once its run has given up", () => { + const [de] = normalizeJobLocales({ + ...workflowJob, + input: { ...workflowJob.input, target_lngs: ["de"] }, + error: { message: "boom" }, + hasError: true, + log: [{ state: "failed", input: { target_lng: "de" } }], + } as PayloadJob); + + expect(de?.status).toBe("failed"); + }); + + it("takes a locale's success from its own log entry, not from the job's status", () => { const rows = normalizeJobLocales({ ...workflowJob, processing: true, @@ -305,10 +331,30 @@ describe("normalizeJobLocales", () => { { state: "failed", completedAt: "2024-01-01T00:02:00Z", input: { target_lng: "fr" } }, ], }); + expect( + rows.map((r) => [r.input.targetLng, r.status]), + "de finished on its own evidence; fr's attempt failed but the run has not given up, so it is still running" + ).toEqual([ + ["de", "completed"], + ["fr", "running"], + ["es", "running"], + ]); + }); + + it("keeps a finished locale finished even once the run gives up", () => { + const rows = normalizeJobLocales({ + ...workflowJob, + error: { message: "boom" }, + hasError: true, + log: [ + { state: "succeeded", completedAt: "2024-01-01T00:01:00Z", input: { target_lng: "de" } }, + { state: "failed", completedAt: "2024-01-01T00:02:00Z", input: { target_lng: "fr" } }, + ], + }); expect(rows.map((r) => [r.input.targetLng, r.status])).toEqual([ ["de", "completed"], ["fr", "failed"], - ["es", "running"], + ["es", "failed"], ]); }); diff --git a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/__tests__/normalizeJobLocales.contract.test.ts b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/__tests__/normalizeJobLocales.contract.test.ts new file mode 100644 index 000000000..a3bd6b26f --- /dev/null +++ b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/__tests__/normalizeJobLocales.contract.test.ts @@ -0,0 +1,215 @@ +import { describe, it, expect } from "vitest"; + +import { normalizeJobLocales } from "../store/normalizeJob.js"; +import type { PayloadJob } from "../store/types.js"; + +type StoredInput = NonNullable; + +const inputOf = (locales: Partial): StoredInput => ({ + collection_slug: "posts", + collection_id: "doc-1", + source_lng: "en", + strategy: "overwrite", + publish_on_translation: false, + ...locales, +}); + +const run = (overrides: Partial = {}): PayloadJob => ({ + id: 7, + createdAt: "2026-10-09T10:00:00Z", + updatedAt: "2026-10-09T10:01:00Z", + input: inputOf({ target_lngs: ["de"] }), + ...overrides, +}); + +describe("normalizeJobLocales", () => { + it("gives a run one row per entry of its locale list, in that order", () => { + const threeLocales = run({ input: inputOf({ target_lngs: ["de", "fr", "es"] }) }); + + expect(normalizeJobLocales(threeLocales).map((task) => task.input.targetLng)).toEqual([ + "de", + "fr", + "es", + ]); + }); + + it("reproduces a repeated locale as two rows", () => { + const repeated = run({ input: inputOf({ target_lngs: ["de", "fr", "de"] }) }); + + expect( + normalizeJobLocales(repeated).map((task) => task.input.targetLng), + "the caller decides whether a repeated locale is a mistake" + ).toEqual(["de", "fr", "de"]); + }); + + it("gives a job with no locale list one row for the single locale it carries", () => { + const beforeLists = run({ input: inputOf({ target_lng: "de" }) }); + + expect(normalizeJobLocales(beforeLists).map((task) => task.input.targetLng)).toEqual(["de"]); + }); + + it("gives a job with an empty locale list one row for the single locale it carries", () => { + const emptyList = run({ input: inputOf({ target_lngs: [], target_lng: "de" }) }); + + expect(normalizeJobLocales(emptyList).map((task) => task.input.targetLng)).toEqual(["de"]); + }); + + it("completes a locale whose own latest entry succeeded, although the run failed", () => { + const succeededInsideCancelledRun = run({ + error: { message: "provider down" }, + log: [ + { state: "succeeded", completedAt: "2026-10-09T10:05:00Z", input: { target_lng: "de" } }, + ], + }); + + expect(normalizeJobLocales(succeededInsideCancelledRun).map((task) => task.status)).toEqual([ + "completed", + ]); + }); + + it("dates a completed locale by its own entry, not by the run", () => { + const runFinishedLater = run({ + completedAt: "2026-10-09T10:09:00Z", + log: [ + { state: "succeeded", completedAt: "2026-10-09T10:05:00Z", input: { target_lng: "de" } }, + ], + }); + + expect(normalizeJobLocales(runFinishedLater).map((task) => task.completedAt)).toEqual([ + "2026-10-09T10:05:00Z", + ]); + }); + + it("leaves a completed locale without an error, although the run carries one", () => { + const succeededInsideCancelledRun = run({ + error: { message: "provider down" }, + log: [ + { state: "succeeded", completedAt: "2026-10-09T10:05:00Z", input: { target_lng: "de" } }, + ], + }); + + expect(normalizeJobLocales(succeededInsideCancelledRun).map((task) => task.error)).toEqual([ + undefined, + ]); + }); + + it("leaves a completed locale uncancelled, although the run was cancelled", () => { + const succeededInsideCancelledRun = run({ + error: { cancelled: true }, + log: [ + { state: "succeeded", completedAt: "2026-10-09T10:05:00Z", input: { target_lng: "de" } }, + ], + }); + + expect(normalizeJobLocales(succeededInsideCancelledRun).map((task) => task.cancelled)).toEqual([ + false, + ]); + }); + + it("reports a locale whose latest entry failed as the run stands, not as the entry does", () => { + const stillRetrying = run({ + processing: true, + log: [{ state: "failed", input: { target_lng: "de" } }], + }); + + expect( + normalizeJobLocales(stillRetrying).map((task) => task.status), + "the queue appends a failed entry on every attempt and gives up only once" + ).toEqual(["running"]); + }); + + it("takes a locale's latest entry over its earlier attempt", () => { + const succeededOnRetry = run({ + processing: true, + log: [ + { state: "failed", input: { target_lng: "de" } }, + { state: "succeeded", completedAt: "2026-10-09T10:05:00Z", input: { target_lng: "de" } }, + ], + }); + + expect(normalizeJobLocales(succeededOnRetry).map((task) => task.status)).toEqual(["completed"]); + }); + + it("does not complete a locale whose latest entry failed after an earlier success", () => { + const failedAfterSuccess = run({ + processing: true, + log: [ + { state: "succeeded", completedAt: "2026-10-09T10:05:00Z", input: { target_lng: "de" } }, + { state: "failed", input: { target_lng: "de" } }, + ], + }); + + expect(normalizeJobLocales(failedAfterSuccess).map((task) => task.status)).toEqual(["running"]); + }); + + it("reports a locale with no entry of its own as the run stands", () => { + const anotherLocaleSettled = run({ + processing: true, + log: [ + { state: "succeeded", completedAt: "2026-10-09T10:05:00Z", input: { target_lng: "fr" } }, + ], + }); + + expect(normalizeJobLocales(anotherLocaleSettled).map((task) => task.status)).toEqual([ + "running", + ]); + }); + + it("gives every row of one run the same id", () => { + const threeLocales = run({ input: inputOf({ target_lngs: ["de", "fr", "es"] }) }); + const ids = normalizeJobLocales(threeLocales).map((task) => task.id); + + expect(new Set(ids).size).toBe(1); + }); + + it("reads a run with a completedAt as completed", () => { + const finished = run({ completedAt: "2026-10-09T10:09:00Z" }); + + expect(normalizeJobLocales(finished).map((task) => task.status)).toEqual(["completed"]); + }); + + it("reads a run that is processing as running", () => { + const inFlight = run({ processing: true }); + + expect(normalizeJobLocales(inFlight).map((task) => task.status)).toEqual(["running"]); + }); + + it("reads a run the queue has given up on as failed", () => { + const broken = run({ error: { message: "provider down" }, hasError: true }); + + expect(normalizeJobLocales(broken).map((task) => task.status)).toEqual(["failed"]); + }); + + it("reads a run that failed an attempt but will retry as pending", () => { + const retrying = run({ error: { message: "provider down" } }); + + expect( + normalizeJobLocales(retrying).map((task) => task.status), + "Payload stores an error on every attempt and sets hasError only once it stops retrying" + ).toEqual(["pending"]); + }); + + it("reads a run with no completedAt, no processing and no error as pending", () => { + expect(normalizeJobLocales(run()).map((task) => task.status)).toEqual(["pending"]); + }); + + it("reads a run as completed even though it is also processing and carries an error", () => { + const finishedAfterTrouble = run({ + completedAt: "2026-10-09T10:09:00Z", + processing: true, + error: { message: "provider down" }, + }); + + expect(normalizeJobLocales(finishedAfterTrouble).map((task) => task.status)).toEqual([ + "completed", + ]); + }); + + it("reads a run as running even though it also carries an error", () => { + const retryingAfterTrouble = run({ processing: true, error: { message: "provider down" } }); + + expect(normalizeJobLocales(retryingAfterTrouble).map((task) => task.status)).toEqual([ + "running", + ]); + }); +}); diff --git a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/store/index.ts b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/store/index.ts index 20fa4627f..980d0e76c 100644 --- a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/store/index.ts +++ b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/store/index.ts @@ -3,5 +3,6 @@ export { TaskInputSchema, WorkflowInputSchema } from "./jobInput.schema.js"; export { toPayloadFields, withLegacyCollection } from "./toPayloadFields.js"; export { handleOf } from "./handleOf.js"; export { readCollectionRef } from "./readCollectionRef.js"; +export { ofCollection, ofDocuments } from "./jobsWhere.js"; export { normalizeJobLocales, latestLogByLocale, isCancelled } from "./normalizeJob.js"; export { assignmentOf } from "./assignmentOf.js"; diff --git a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/store/jobsWhere.ts b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/store/jobsWhere.ts new file mode 100644 index 000000000..7b983dc24 --- /dev/null +++ b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/store/jobsWhere.ts @@ -0,0 +1,31 @@ +import type { CollectionSlug, Where } from "payload"; + +/** + * Conditions over a job row's document reference, which Payload stores in a `json` column and this + * plugin has written in two shapes. Each is an `or` whose second branch is the deprecated one, so + * deleting that branch is the whole change when the legacy reference goes — + * docs/DEPRECATIONS.md#jobs-input-collection-field + */ + +export const ofCollection = (collectionSlug: CollectionSlug): Where => ({ + or: [ + { "input.collection_slug": { equals: collectionSlug } }, + { "input.collection.relationTo": { equals: collectionSlug } }, + ], +}); + +/** + * A document id reaches the store as a string, but the legacy reference was written through a + * relationship field, which stored a numeric collection's id as a **number**. Comparing one against + * the other matches nothing — on every adapter, MongoDB included — so both forms are offered and the + * store matches whichever it holds. + */ +const asStored = (documentId: string): Array => + /^\d+$/u.test(documentId) ? [documentId, Number(documentId)] : [documentId]; + +export const ofDocuments = (documentIds: readonly string[]): Where => { + const values = documentIds.flatMap(asStored); + return { + or: [{ "input.collection_id": { in: values } }, { "input.collection.value": { in: values } }], + }; +}; diff --git a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/store/normalizeJob.ts b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/store/normalizeJob.ts index 5b60386fa..a85c1b1d7 100644 --- a/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/store/normalizeJob.ts +++ b/packages/payload-plugin-translator/src/server/modules/task-runner/payload-jobs-runner/store/normalizeJob.ts @@ -7,7 +7,7 @@ import { readCollectionRef } from "./readCollectionRef.js"; function getJobStatus(job: PayloadJob): TaskStatus { if (job.completedAt) return "completed"; if (job.processing) return "running"; - if (job.error) return "failed"; + if (job.hasError) return "failed"; return "pending"; } @@ -59,8 +59,11 @@ export function latestLogByLocale(job: PayloadJob): Map { } /** - * One {@link Task} per target locale, its state read from {@link latestLogByLocale}. A pre-workflow - * job carries a single `target_lng` and expands to itself. + * One {@link Task} per entry of the run's `target_lngs`; a job without a list expands to itself. + * + * A locale is `completed` only on its own succeeded log entry; every other locale takes the run's + * status, because Payload appends a `failed` entry on every attempt and marks the job only once it + * gives up (`queues/errors/handleTaskError.js`). */ export function normalizeJobLocales(job: PayloadJob): Task[] { const targets = job.input?.target_lngs; @@ -75,7 +78,7 @@ export function normalizeJobLocales(job: PayloadJob): Task[] { const succeeded = entry.state === "succeeded"; return { ...base, - status: succeeded ? "completed" : "failed", + status: succeeded ? "completed" : base.status, completedAt: succeeded ? (entry.completedAt ?? undefined) : undefined, error: succeeded ? undefined : base.error, cancelled: succeeded ? false : base.cancelled, diff --git a/packages/payload-plugin-translator/src/server/modules/task-runner/sync-runner/SyncTaskRunner.ts b/packages/payload-plugin-translator/src/server/modules/task-runner/sync-runner/SyncTaskRunner.ts index 9a4fea391..5acea0b5b 100644 --- a/packages/payload-plugin-translator/src/server/modules/task-runner/sync-runner/SyncTaskRunner.ts +++ b/packages/payload-plugin-translator/src/server/modules/task-runner/sync-runner/SyncTaskRunner.ts @@ -100,7 +100,7 @@ export class SyncTaskRunner implements TaskRunner { collectionSlug: CollectionSlug, filter?: Array | TaskFilter ): Promise { - const { documentIds, excludeCompleted } = toTaskFilter(filter); + const { documentIds, excludeCompleted, excludeGivenUpBefore } = toTaskFilter(filter); const results: Task[] = []; const wanted = documentIds ? new Set(documentIds.map(String)) : undefined; @@ -108,6 +108,9 @@ export class SyncTaskRunner implements TaskRunner { if (task.input.collectionSlug !== collectionSlug) continue; if (wanted && !wanted.has(task.input.collectionId)) continue; if (excludeCompleted && task.completedAt) continue; + if (excludeGivenUpBefore && task.status === "failed") { + if (Date.parse(task.updatedAt) < excludeGivenUpBefore.getTime()) continue; + } results.push(task); } diff --git a/packages/payload-plugin-translator/src/server/modules/task-runner/sync-runner/__tests__/SyncTaskRunner.test.ts b/packages/payload-plugin-translator/src/server/modules/task-runner/sync-runner/__tests__/SyncTaskRunner.test.ts index 81b0f3fff..17ea5d065 100644 --- a/packages/payload-plugin-translator/src/server/modules/task-runner/sync-runner/__tests__/SyncTaskRunner.test.ts +++ b/packages/payload-plugin-translator/src/server/modules/task-runner/sync-runner/__tests__/SyncTaskRunner.test.ts @@ -39,6 +39,37 @@ describe("SyncTaskRunner", () => { ...overrides, }); + describe("excludeGivenUpBefore", () => { + const givenUp = async (): Promise => { + mockHandler = vi.fn().mockRejectedValue(new Error("provider down")); + runner = new SyncTaskRunner(mockPayload, contextOf(mockHandler), tasks); + await runner.enqueue([createInput()]); + }; + + it("drops a run that gave up before the cutoff", async () => { + await givenUp(); + + const kept = await runner.findByCollection("posts" as CollectionSlug, { + excludeGivenUpBefore: new Date(Date.now() + 1000), + }); + + expect(kept, "a failure older than the window is not an action to take").toEqual([]); + }); + + it("keeps a run that gave up after the cutoff", async () => { + await givenUp(); + + const kept = await runner.findByCollection("posts" as CollectionSlug, { + excludeGivenUpBefore: new Date(Date.now() - 1000), + }); + + expect( + kept.map((task) => task.status), + "a recent failure is still worth reporting" + ).toEqual(["failed"]); + }); + }); + describe("enqueue", () => { it("executes handler immediately", async () => { const input = createInput(); diff --git a/packages/payload-plugin-translator/src/server/modules/translation-levels/PluginConfigBuilder.test.ts b/packages/payload-plugin-translator/src/server/modules/translation-levels/PluginConfigBuilder.test.ts index c35269121..8d57ba325 100644 --- a/packages/payload-plugin-translator/src/server/modules/translation-levels/PluginConfigBuilder.test.ts +++ b/packages/payload-plugin-translator/src/server/modules/translation-levels/PluginConfigBuilder.test.ts @@ -3,16 +3,15 @@ import type { CollectionConfig, Config, Endpoint } from "payload"; import type { RawPayloadComponentExport } from "../../../types/PayloadComponentExport.js"; +import { resolveSettings } from "../../../settings.js"; import { PluginConfigBuilder } from "./PluginConfigBuilder.js"; const deps = (collections: Array<{ slug: string }> = []) => ({ collections: collections as unknown as CollectionConfig[], - basePath: "/translate", + settings: resolveSettings({}), taskRunnerFactory: { create: vi.fn() }, schemaMap: new Map(), translationProvider: { translate: vi.fn() }, - targetSelection: "single" as const, - inlineMarks: false, }); const ep = (method: string, path: string): Endpoint => diff --git a/packages/payload-plugin-translator/src/server/modules/translation-levels/PluginConfigBuilder.ts b/packages/payload-plugin-translator/src/server/modules/translation-levels/PluginConfigBuilder.ts index ab2bd9b9f..b9318dff1 100644 --- a/packages/payload-plugin-translator/src/server/modules/translation-levels/PluginConfigBuilder.ts +++ b/packages/payload-plugin-translator/src/server/modules/translation-levels/PluginConfigBuilder.ts @@ -3,11 +3,11 @@ import type { CollectionConfig, Config, Endpoint } from "payload"; import type { AccessGuard } from "../../../types/AccessGuard.js"; import type { RawPayloadComponentExport } from "../../../types/PayloadComponentExport.js"; import type { CollectionSchemaMap } from "../../../types/CollectionSchemaMap.js"; -import type { TargetSelectionMode } from "../../../types/TargetSelection.js"; import type { ConfigModifier } from "../../../types/ConfigModifier.js"; import type { TranslationProvider } from "../../../core/domain/translation-providers/index.js"; import type { TaskRunnerFactory } from "../task-runner/index.js"; import type { ProvenanceServiceFactory } from "../provenance/index.js"; +import type { TranslatorSettings } from "../../../settings.js"; import type { CollectionAdminSlot, LevelContext, TranslationContext } from "./types.js"; @@ -68,14 +68,12 @@ function attachToSlot( */ export class PluginConfigBuilder implements LevelContext { readonly collections: CollectionConfig[]; - readonly basePath: string; + readonly settings: TranslatorSettings; readonly access?: AccessGuard; readonly taskRunnerFactory: TaskRunnerFactory; readonly schemaMap: CollectionSchemaMap; readonly translationProvider: TranslationProvider; readonly provenanceServiceFactory?: ProvenanceServiceFactory; - readonly targetSelection: TargetSelectionMode; - readonly inlineMarks: boolean; private readonly endpoints: Endpoint[] = []; private readonly collectionComponents: CollectionComponent[] = []; @@ -84,14 +82,12 @@ export class PluginConfigBuilder implements LevelContext { constructor(deps: PluginConfigBuilderDeps) { this.collections = deps.collections; - this.basePath = deps.basePath; + this.settings = deps.settings; this.access = deps.access; this.taskRunnerFactory = deps.taskRunnerFactory; this.schemaMap = deps.schemaMap; this.translationProvider = deps.translationProvider; this.provenanceServiceFactory = deps.provenanceServiceFactory; - this.targetSelection = deps.targetSelection; - this.inlineMarks = deps.inlineMarks; } addEndpoints(endpoints: Endpoint[]): void { diff --git a/packages/payload-plugin-translator/src/server/modules/translation-levels/types.ts b/packages/payload-plugin-translator/src/server/modules/translation-levels/types.ts index a38d2c824..0a46e152e 100644 --- a/packages/payload-plugin-translator/src/server/modules/translation-levels/types.ts +++ b/packages/payload-plugin-translator/src/server/modules/translation-levels/types.ts @@ -3,10 +3,10 @@ import type { CollectionConfig, Endpoint } from "payload"; import type { AccessGuard } from "../../../types/AccessGuard.js"; import type { RawPayloadComponentExport } from "../../../types/PayloadComponentExport.js"; import type { CollectionSchemaMap } from "../../../types/CollectionSchemaMap.js"; -import type { TargetSelectionMode } from "../../../types/TargetSelection.js"; import type { TranslationProvider } from "../../../core/domain/translation-providers/index.js"; import type { TaskRunnerFactory } from "../task-runner/index.js"; import type { ProvenanceServiceFactory } from "../provenance/index.js"; +import type { TranslatorSettings } from "../../../settings.js"; export type CollectionAdminSlot = "beforeDocumentControls" | "beforeListTable"; @@ -18,7 +18,8 @@ export type CollectionAdminSlot = "beforeDocumentControls" | "beforeListTable"; */ export type TranslationContext = { readonly collections: CollectionConfig[]; - readonly basePath: string; + /** The plugin's resolved settings, relayed whole — see `src/settings.ts`. */ + readonly settings: TranslatorSettings; readonly access?: AccessGuard; readonly taskRunnerFactory: TaskRunnerFactory; /** Deep-cloned localized field schema per managed collection slug. */ @@ -27,10 +28,6 @@ export type TranslationContext = { readonly translationProvider: TranslationProvider; /** Builds a provenance service; absent when provenance is disabled (staleness then reports empty). */ readonly provenanceServiceFactory?: ProvenanceServiceFactory; - /** Resolved target-language selection mode (`'single'` default) — drives which target control the - * admin forms render. */ - readonly targetSelection: TargetSelectionMode; - readonly inlineMarks: boolean; }; /** diff --git a/packages/payload-plugin-translator/src/settings.test.ts b/packages/payload-plugin-translator/src/settings.test.ts new file mode 100644 index 000000000..7435ccb5f --- /dev/null +++ b/packages/payload-plugin-translator/src/settings.test.ts @@ -0,0 +1,45 @@ +import { describe, it, expect } from "vitest"; + +import { DEFAULT_FAILED_WINDOW_MS, resolveSettings } from "./settings.js"; + +describe("resolveSettings", () => { + it("fills every setting from nothing", () => { + expect(resolveSettings({})).toEqual({ + basePath: "/translate", + failedWindowMs: DEFAULT_FAILED_WINDOW_MS, + targetSelection: "single", + inlineMarks: false, + }); + }); + + it("normalises a base path the caller wrote loosely", () => { + expect(resolveSettings({ basePath: "i18n/" }).basePath).toBe("/i18n"); + }); + + it("keeps a window of zero instead of reading it as unset", () => { + expect( + resolveSettings({ failedWindowMs: 0 }).failedWindowMs, + "zero means report no failures at all — a real choice, not an omission" + ).toBe(0); + }); + + it.each([ + [undefined, false], + [{}, false], + [{ inlineMarks: false }, false], + [{ inlineMarks: true }, true], + ])("reads experimental %j as inlineMarks=%s", (experimental, expected) => { + expect(resolveSettings({ experimental }).inlineMarks).toBe(expected); + }); + + it.each([Number.NaN, Number.POSITIVE_INFINITY, -1])( + "refuses %p as a failure window instead of carrying it to the query", + (failedWindowMs) => { + expect(() => resolveSettings({ failedWindowMs })).toThrow(/failedWindowMs/u); + } + ); + + it("passes a chosen target-selection mode through", () => { + expect(resolveSettings({ targetSelection: "multi" }).targetSelection).toBe("multi"); + }); +}); diff --git a/packages/payload-plugin-translator/src/settings.ts b/packages/payload-plugin-translator/src/settings.ts new file mode 100644 index 000000000..e03136c03 --- /dev/null +++ b/packages/payload-plugin-translator/src/settings.ts @@ -0,0 +1,94 @@ +import { TranslatorConfigError } from "./core/errors/index.js"; +import { normalizePath } from "./server/shared/index.js"; + +import type { TargetSelectionMode } from "./types/TargetSelection.js"; + +/** + * The plugin options that resolve to a value rather than wire a dependency. They are declared here, + * beside the resolved shape and the defaults, so one setting is one place to edit. + * {@link TranslatorPluginConfig} composes them into the public option type. + */ +export type TranslatorSettingsOptions = { + /** + * Base path for all translation API endpoints. + * Useful to avoid conflicts with existing routes. + * @example '/my-translate' results in '/my-translate/enqueue', '/my-translate/run/:id', etc. + * @default '/translate' + */ + basePath?: string; + /** + * How far back (ms) the collection panel counts failures. Work still in flight is listed however + * old. + * @default 259200000 (3 days) + * @since 0.17.0 + */ + failedWindowMs?: number; + /** + * How the target-language field behaves in the translation forms. `'single'` (default) keeps + * today's one-locale-per-run behaviour, byte-identical. `'multi'` renders a compact multi-select so + * an editor can queue several target locales in one run — the run fans out one translation per + * `(document × target locale)`. Opt-in and fully backward-compatible; no schema, no migration. + * @default 'single' + * @since 0.10.0 + */ + targetSelection?: TargetSelectionMode; + /** + * Where switches for behaviour on its way to becoming the default live. The option itself is + * permanent; each **entry** is deprecated the day it ships, because it exists only so you can + * adopt a change early and the next major removes the switch, not the behaviour. + * + * @since 0.13.0 + */ + experimental?: { + /** + * Translate each rich-text container (paragraph, heading, list item) as one string with + * numbered inline marks, instead of node by node. + * + * Requires a provider declaring `capabilities.inlineMarks`; without one the plugin keeps + * translating node by node. A container whose reply comes back with damaged marks is left in + * its source language rather than half-written. + * + * Turning it back off stops future translations from splitting wrappers; documents already + * translated under it stay split. And the next major removes the flag itself, so it is a + * schedule, not a permanent safety valve. + * + * @default false + * @deprecated Transitional. Removed in the next major, when this becomes the only mode. + * @see docs/DEPRECATIONS.md#experimental-inline-marks — why this exists and when it goes + */ + inlineMarks?: boolean; + }; +}; + +/** + * {@link TranslatorSettingsOptions} after defaults and normalisation, as every layer below + * `plugin.ts` sees them. The levels, the route bundle and the admin panel read these; the layers + * between relay the whole value without naming a field. + */ +export type TranslatorSettings = { + basePath: string; + failedWindowMs: number; + targetSelection: TargetSelectionMode; + inlineMarks: boolean; +}; + +export const DEFAULT_BASE_PATH = "/translate"; +export const DEFAULT_FAILED_WINDOW_MS = 3 * 24 * 60 * 60 * 1000; +export const DEFAULT_TARGET_SELECTION: TargetSelectionMode = "single"; + +const failedWindowOf = (failedWindowMs: number | undefined): number => { + const resolved = failedWindowMs ?? DEFAULT_FAILED_WINDOW_MS; + if (!Number.isFinite(resolved) || resolved < 0) { + throw new TranslatorConfigError( + `[payload-plugin-translator] failedWindowMs must be a non-negative finite number (got ${resolved})` + ); + } + return resolved; +}; + +export const resolveSettings = (options: TranslatorSettingsOptions): TranslatorSettings => ({ + basePath: normalizePath(options.basePath ?? DEFAULT_BASE_PATH), + failedWindowMs: failedWindowOf(options.failedWindowMs), + targetSelection: options.targetSelection ?? DEFAULT_TARGET_SELECTION, + inlineMarks: options.experimental?.inlineMarks === true, +}); diff --git a/packages/payload-plugin-translator/vitest.config.ts b/packages/payload-plugin-translator/vitest.config.ts index 0f78dc014..6e387a82a 100644 --- a/packages/payload-plugin-translator/vitest.config.ts +++ b/packages/payload-plugin-translator/vitest.config.ts @@ -4,6 +4,6 @@ export default defineConfig({ test: { globals: true, environment: "node", - include: ["src/**/*.test.ts"], + include: ["src/**/*.test.ts", "src/**/*.test.tsx"], }, });