From 72416bbb048e45694048d981a58554b9c3dcec58 Mon Sep 17 00:00:00 2001 From: Jacob Cable Date: Fri, 11 Sep 2026 16:29:07 +0100 Subject: [PATCH 1/2] fix(firestore-translate-text): route a null input like the extension The kit guarded the object branch with `input !== null`, so a `null` input reached `translateSingle` and was sent to the translation API as a single value. The extension routes on `typeof input === "object"` alone, so `null` reaches `translateMultiple` and fails there. Drop the guard for parity: `null` now produces the extension's `TypeError`, one error event, and no write. --- kits/firestore-translate-text/CHANGELOG.md | 1 + .../src/translate/translateDocument.ts | 2 +- .../tests/handlers.test.ts | 18 ++++++++++++++++++ .../tests/translate-document.test.ts | 17 ++++++++++------- 4 files changed, 30 insertions(+), 8 deletions(-) diff --git a/kits/firestore-translate-text/CHANGELOG.md b/kits/firestore-translate-text/CHANGELOG.md index bc1c5b451..18c3363c4 100644 --- a/kits/firestore-translate-text/CHANGELOG.md +++ b/kits/firestore-translate-text/CHANGELOG.md @@ -1,3 +1,4 @@ +- fix: a `null` input on update now fails the same way as the extension (a `TypeError` from `translateMultiple`, one error event, no write), instead of being sent to the translation API as a single value. - 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 diff --git a/kits/firestore-translate-text/src/translate/translateDocument.ts b/kits/firestore-translate-text/src/translate/translateDocument.ts index 36e23bd50..78c5887e2 100644 --- a/kits/firestore-translate-text/src/translate/translateDocument.ts +++ b/kits/firestore-translate-text/src/translate/translateDocument.ts @@ -41,7 +41,7 @@ export const translateDocument = async ( return; } - if (typeof input === "object" && input !== null) { + if (typeof input === "object") { return translateMultiple( input as Record, languages, diff --git a/kits/firestore-translate-text/tests/handlers.test.ts b/kits/firestore-translate-text/tests/handlers.test.ts index 659e158f7..b46407e0b 100644 --- a/kits/firestore-translate-text/tests/handlers.test.ts +++ b/kits/firestore-translate-text/tests/handlers.test.ts @@ -342,6 +342,24 @@ describe("handleDocumentWrite", () => { expect(translateClassMethod).not.toHaveBeenCalled(); }); + test("fails without writing when an update sets the input to null", async () => { + const after = makeSnapshot({ input: null }); + + await expect( + handleDocumentWrite( + makeEvent(makeSnapshot({ input: "hello" }), after), + context() + ) + ).resolves.toBeUndefined(); + + expect(logger.error).toHaveBeenCalledWith( + ...messages.error(expect.any(TypeError)) + ); + expect(events.recordErrorEvent).toHaveBeenCalledWith(expect.any(TypeError)); + expect(translateClassMethod).not.toHaveBeenCalled(); + expect(firestore.update).not.toHaveBeenCalled(); + }); + test("skips processing if there is no input on the before and after snapshots", async () => { const snapshot = makeSnapshot({ notTheInput: "hello" }); diff --git a/kits/firestore-translate-text/tests/translate-document.test.ts b/kits/firestore-translate-text/tests/translate-document.test.ts index b668ef92b..bb678ac09 100644 --- a/kits/firestore-translate-text/tests/translate-document.test.ts +++ b/kits/firestore-translate-text/tests/translate-document.test.ts @@ -104,16 +104,19 @@ describe("translateDocument", () => { ); }); - test("treats a null input as a single translation, uncoerced", async () => { + test("routes a null input through translateMultiple, as the extension does", async () => { const snapshot = makeSnapshot({ input: null }); - await translateDocument( - snapshot, - makeService({ extractLanguages: vi.fn(() => ["en"]) }), - makeConfig() - ); + await expect( + translateDocument( + snapshot, + makeService({ extractLanguages: vi.fn(() => ["en"]) }), + makeConfig() + ) + ).rejects.toThrow(TypeError); - expect(translateString).toHaveBeenCalledWith(null, "en"); + expect(translateString).not.toHaveBeenCalled(); + expect(updateTranslations).not.toHaveBeenCalled(); }); test("exits early when the input field is a translation output path", async () => { From 335854af4523e60b46787f55c27986a346dab185 Mon Sep 17 00:00:00 2001 From: Jacob Cable Date: Fri, 11 Sep 2026 17:03:41 +0100 Subject: [PATCH 2/2] test(firestore-translate-text): pin the translateMultiple route and single error event for a null input The null-input tests asserted only that the call rejected with a TypeError, which any early throw satisfied. They now observe the route itself: the unit test wraps the real `translateMultiple` in a spy and asserts it receives `null`, and the handler test counts one error log and one error event and asserts `translateSingle`'s opening log never fires. Also corrects the changelog line: the Genkit path threw before reaching the API, so only the Google Translate path sent the value. --- kits/firestore-translate-text/CHANGELOG.md | 2 +- .../tests/handlers.test.ts | 8 ++++++++ .../tests/translate-document.test.ts | 20 ++++++++++++++----- 3 files changed, 24 insertions(+), 6 deletions(-) diff --git a/kits/firestore-translate-text/CHANGELOG.md b/kits/firestore-translate-text/CHANGELOG.md index 18c3363c4..293213e86 100644 --- a/kits/firestore-translate-text/CHANGELOG.md +++ b/kits/firestore-translate-text/CHANGELOG.md @@ -1,4 +1,4 @@ -- fix: a `null` input on update now fails the same way as the extension (a `TypeError` from `translateMultiple`, one error event, no write), instead of being sent to the translation API as a single value. +- fix: a `null` input on update now fails the same way as the extension (a `TypeError` from `translateMultiple`, one error event, no write), instead of being routed to `translateSingle`. - 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 diff --git a/kits/firestore-translate-text/tests/handlers.test.ts b/kits/firestore-translate-text/tests/handlers.test.ts index b46407e0b..80d0c031c 100644 --- a/kits/firestore-translate-text/tests/handlers.test.ts +++ b/kits/firestore-translate-text/tests/handlers.test.ts @@ -355,7 +355,15 @@ describe("handleDocumentWrite", () => { expect(logger.error).toHaveBeenCalledWith( ...messages.error(expect.any(TypeError)) ); + expect(logger.error).toHaveBeenCalledTimes(1); expect(events.recordErrorEvent).toHaveBeenCalledWith(expect.any(TypeError)); + expect(events.recordErrorEvent).toHaveBeenCalledTimes(1); + expect(logger.log).not.toHaveBeenCalledWith( + messages.translateInputStringToAllLanguages( + null as never, + defaultLanguages + ) + ); expect(translateClassMethod).not.toHaveBeenCalled(); expect(firestore.update).not.toHaveBeenCalled(); }); diff --git a/kits/firestore-translate-text/tests/translate-document.test.ts b/kits/firestore-translate-text/tests/translate-document.test.ts index bb678ac09..f3b5bf052 100644 --- a/kits/firestore-translate-text/tests/translate-document.test.ts +++ b/kits/firestore-translate-text/tests/translate-document.test.ts @@ -18,11 +18,18 @@ import { beforeEach, describe, expect, test, vi } from "vitest"; vi.mock("firebase-functions", () => import("./mocks/firebase-functions")); vi.mock("../src/events"); +vi.mock("../src/translate/translateMultiple", async (importOriginal) => { + const mod = await importOriginal< + typeof import("../src/translate/translateMultiple") + >(); + return { ...mod, translateMultiple: vi.fn(mod.translateMultiple) }; +}); import * as events from "../src/events"; import { messages } from "../src/logs/messages"; import type { TranslationService } from "../src/translate"; import { translateDocument } from "../src/translate/translateDocument"; +import { translateMultiple } from "../src/translate/translateMultiple"; import { translateSingle } from "../src/translate/translateSingle"; import { defaultLanguages, @@ -106,15 +113,18 @@ describe("translateDocument", () => { test("routes a null input through translateMultiple, as the extension does", async () => { const snapshot = makeSnapshot({ input: null }); + const service = makeService({ extractLanguages: vi.fn(() => ["en"]) }); await expect( - translateDocument( - snapshot, - makeService({ extractLanguages: vi.fn(() => ["en"]) }), - makeConfig() - ) + translateDocument(snapshot, service, makeConfig()) ).rejects.toThrow(TypeError); + expect(translateMultiple).toHaveBeenCalledWith( + null, + ["en"], + snapshot, + service + ); expect(translateString).not.toHaveBeenCalled(); expect(updateTranslations).not.toHaveBeenCalled(); });