From 12f500206be9021b8164ec6f05c2f170ce20bc63 Mon Sep 17 00:00:00 2001 From: Jacob Cable Date: Tue, 8 Sep 2026 12:55:09 +0100 Subject: [PATCH] fix(firestore-translate-text): emit start and completion events on every write `handleDocumentWrite` returned before recording anything when the gen2 event arrived with no change data, so neither `onStart` nor `onCompletion` was published for that invocation. The extension's `fstranslate` is a gen1 `onWrite` and has no such path: every call publishes `onStart` first and `onCompletion` last, with each early return recording completion before it leaves. The guard now sits after `logs.start` and `recordStartEvent`, and records `onCompletion` before returning, so the no-data path behaves like the other no-op branches (delete, unchanged input, missing input). `onStart` carries `{ data: undefined, params }` there; the payload shape itself is unchanged and is being restored to the extension's `{ change, context }` separately. Every other branch already matched the extension: `onSuccess` comes only from `updateTranslations`, and a translator failure records one `onError` per layer (translator, `translateSingle`, handler) before `onCompletion`. Those sequences are now pinned by a `lifecycle events` block that asserts the exact event order for each branch, so a future early return cannot drop the pair unnoticed. 112 tests and `tsc --noEmit` pass. No live deploy was run. Fixes #3020 Parity ledger: #2974, firestore-translate-text. --- kits/firestore-translate-text/CHANGELOG.md | 1 + kits/firestore-translate-text/src/handlers.ts | 9 +- .../tests/handlers.test.ts | 191 +++++++++++++++++- 3 files changed, 195 insertions(+), 6 deletions(-) diff --git a/kits/firestore-translate-text/CHANGELOG.md b/kits/firestore-translate-text/CHANGELOG.md index 2f16398052..bc1c5b451a 100644 --- a/kits/firestore-translate-text/CHANGELOG.md +++ b/kits/firestore-translate-text/CHANGELOG.md @@ -1,3 +1,4 @@ +- fix: `onStart` and `onCompletion` are now published on every invocation, including a write event that arrives without change data; that path used to return before either event was recorded, so subscribers missed a lifecycle pair the extension always emitted. - Construct the translation client once per process instead of on every invocation, matching the legacy extension. This changes the exported `HandlerContext` type from `{ firestore, config, googleAiApiKey? }` to `{ config, service }`: `handleDocumentWrite` no longer builds the `TranslationService` itself and instead expects it on the context (build one with `createTranslationService`) - Initial release of kit, see README for differences between the legacy extension and this kit - The Google AI API key (and any other secret-shaped config value) is now masked as `` in the config logged at startup and on each invocation; the legacy extension logs it in cleartext diff --git a/kits/firestore-translate-text/src/handlers.ts b/kits/firestore-translate-text/src/handlers.ts index 3c187525ef..3c2e617b25 100644 --- a/kits/firestore-translate-text/src/handlers.ts +++ b/kits/firestore-translate-text/src/handlers.ts @@ -54,15 +54,16 @@ export async function handleDocumentWrite( event: TranslateWriteEvent, ctx: HandlerContext ): Promise { - if (!event.data) { - return; - } - const { config, service } = ctx; logs.start(config); await events.recordStartEvent({ data: event.data, params: event.params }); + if (!event.data) { + await events.recordCompletionEvent({ params: event.params }); + return; + } + const { languages, inputFieldName, outputFieldName } = config; if (validators.fieldNamesMatch(inputFieldName, outputFieldName)) { diff --git a/kits/firestore-translate-text/tests/handlers.test.ts b/kits/firestore-translate-text/tests/handlers.test.ts index 58f9560185..a2c523e7f0 100644 --- a/kits/firestore-translate-text/tests/handlers.test.ts +++ b/kits/firestore-translate-text/tests/handlers.test.ts @@ -83,13 +83,21 @@ describe("handleDocumentWrite", () => { }); test("skips events without change data", async () => { + const event = makeEvent(undefined, undefined); + await expect( - handleDocumentWrite(makeEvent(undefined, undefined), context()) + handleDocumentWrite(event, context()) ).resolves.toBeUndefined(); expect(translateClassMethod).not.toHaveBeenCalled(); expect(firestore.update).not.toHaveBeenCalled(); - expect(events.recordStartEvent).not.toHaveBeenCalled(); + expect(events.recordStartEvent).toHaveBeenCalledWith({ + data: undefined, + params: event.params, + }); + expect(events.recordCompletionEvent).toHaveBeenCalledWith({ + params: event.params, + }); }); test("records start and completion events", async () => { @@ -391,4 +399,183 @@ describe("handleDocumentWrite", () => { ); expect(JSON.stringify(logger.log.mock.calls)).not.toContain("super-secret"); }); + + /** + * Every invocation of the extension's `fstranslate` publishes `onStart` + * first and `onCompletion` last, whichever branch it takes in between; the + * early returns are not exempt. Each test here pins the full event sequence + * for one branch. + */ + describe("lifecycle events", () => { + const EVENT_SPIES = { + start: events.recordStartEvent, + success: events.recordSuccessEvent, + error: events.recordErrorEvent, + completion: events.recordCompletionEvent, + } as const; + + function publishedEvents(): Array { + return Object.entries(EVENT_SPIES) + .flatMap(([name, spy]) => + vi + .mocked(spy) + .mock.invocationCallOrder.map( + (order) => [name as keyof typeof EVENT_SPIES, order] as const + ) + ) + .sort(([, a], [, b]) => a - b) + .map(([name]) => name); + } + + test("start then completion when the event carries no change", async () => { + await handleDocumentWrite(makeEvent(undefined, undefined), context()); + + expect(publishedEvents()).toEqual(["start", "completion"]); + }); + + test("start then completion when the input and output fields match", async () => { + await handleDocumentWrite( + makeEvent(makeSnapshot(), makeSnapshot({ input: "hello" })), + context({ inputFieldName: "input", outputFieldName: "input" }) + ); + + expect(publishedEvents()).toEqual(["start", "completion"]); + }); + + test("start then completion when the input field is a configured translation path", async () => { + await handleDocumentWrite( + makeEvent(makeSnapshot(), makeSnapshot({ input: "hello" })), + context({ + inputFieldName: "translated.en", + outputFieldName: "translated", + }) + ); + + expect(publishedEvents()).toEqual(["start", "completion"]); + }); + + test("start then completion when the document's languages make the input field a translation path", async () => { + await handleDocumentWrite( + makeEvent( + makeSnapshot(), + makeSnapshot({ translated: { fr: "hello" }, langs: ["fr"] }) + ), + context({ + inputFieldName: "translated.fr", + outputFieldName: "translated", + languages: "en", + }) + ); + + expect(publishedEvents()).toEqual(["start", "completion"]); + expect(firestore.update).not.toHaveBeenCalled(); + }); + + test("start, success, completion when a document is created with input", async () => { + await handleDocumentWrite( + makeEvent(makeSnapshot(), makeSnapshot({ input: "hello" })), + context() + ); + + expect(publishedEvents()).toEqual(["start", "success", "completion"]); + }); + + test("start then completion when a document is created without input", async () => { + await handleDocumentWrite( + makeEvent(makeSnapshot(), makeSnapshot({ changed: 123 })), + context() + ); + + expect(publishedEvents()).toEqual(["start", "completion"]); + }); + + test("start then completion when a document is deleted", async () => { + await handleDocumentWrite( + makeEvent( + makeSnapshot({ input: "hello" }), + makeSnapshot({ input: "hello" }, { exists: false }) + ), + context() + ); + + expect(publishedEvents()).toEqual(["start", "completion"]); + }); + + test("start then completion when neither snapshot has input", async () => { + const snapshot = makeSnapshot({ notTheInput: "hello" }); + + await handleDocumentWrite(makeEvent(snapshot, snapshot), context()); + + expect(publishedEvents()).toEqual(["start", "completion"]); + }); + + test("start, success, completion when the input field is removed", async () => { + await handleDocumentWrite( + makeEvent( + makeSnapshot({ input: "hello" }), + makeSnapshot({}, { exists: true }) + ), + context() + ); + + expect(publishedEvents()).toEqual(["start", "success", "completion"]); + }); + + test("start then completion when the input is unchanged", async () => { + await handleDocumentWrite( + makeEvent( + makeSnapshot({ input: "hello" }), + makeSnapshot({ input: "hello", changed: 123 }) + ), + context() + ); + + expect(publishedEvents()).toEqual(["start", "completion"]); + }); + + test("start, success, completion when the input changes", async () => { + await handleDocumentWrite( + makeEvent( + makeSnapshot({ input: "goodbye" }), + makeSnapshot({ input: "hello" }) + ), + context() + ); + + expect(publishedEvents()).toEqual(["start", "success", "completion"]); + }); + + test("start, one error per layer, completion when a string translation fails", async () => { + translateClassMethod.mockRejectedValueOnce(new Error("boom")); + + await handleDocumentWrite( + makeEvent(makeSnapshot(), makeSnapshot({ input: "hello" })), + context() + ); + + expect(publishedEvents()).toEqual([ + "start", + "error", + "error", + "error", + "completion", + ]); + }); + + test("start, one error per layer, completion when a map translation fails", async () => { + translateClassMethod.mockRejectedValueOnce(new Error("boom")); + + await handleDocumentWrite( + makeEvent(makeSnapshot(), makeSnapshot({ input: { one: "hello" } })), + context() + ); + + expect(publishedEvents()).toEqual([ + "start", + "error", + "error", + "completion", + ]); + }); + }); });