From ae9265061caf4d825e7ae78977d9a5c85339632a Mon Sep 17 00:00:00 2001 From: lou Date: Wed, 22 Apr 2026 09:10:58 -0600 Subject: [PATCH 1/4] Add experimental browser-native translation --- .gitignore | 3 + src/components/message/MessageActions.tsx | 22 +- src/components/message/MessageItem.tsx | 194 ++++++++++++++++- .../message/ReactionsWithActions.tsx | 12 ++ src/components/mobile/MessageBottomSheet.tsx | 26 ++- src/lib/browserTranslation.ts | 164 +++++++++++++++ src/lib/settings/definitions/allSettings.ts | 32 ++- src/store/index.ts | 2 + src/store/types.ts | 1 + tests/App.test.tsx | 1 + .../components/ChannelSettingsModal.test.tsx | 1 + .../LinkSecurityWarningModal.test.tsx | 1 + .../components/MediaCommentsSidebar.test.tsx | 5 +- tests/components/MessageActions.test.tsx | 70 ++++++ .../MessageItem.translation.test.tsx | 199 ++++++++++++++++++ tests/components/MetadataDisplay.test.tsx | 1 + tests/lib/browserTranslation.test.ts | 195 +++++++++++++++++ tests/store/index.test.ts | 9 + tests/store/join.test.ts | 1 + tests/store/localStorage.test.ts | 6 + 20 files changed, 922 insertions(+), 23 deletions(-) create mode 100644 src/lib/browserTranslation.ts create mode 100644 tests/components/MessageActions.test.tsx create mode 100644 tests/components/MessageItem.translation.test.tsx create mode 100644 tests/lib/browserTranslation.test.ts diff --git a/.gitignore b/.gitignore index 74f50bc2..69c6cd38 100644 --- a/.gitignore +++ b/.gitignore @@ -25,3 +25,6 @@ yarn-error.log* # Pre commit /.husky tsconfig.tsbuildinfo + +# private fork-only notes +/.fork-local diff --git a/src/components/message/MessageActions.tsx b/src/components/message/MessageActions.tsx index 1ca03b6e..cafc526b 100644 --- a/src/components/message/MessageActions.tsx +++ b/src/components/message/MessageActions.tsx @@ -1,5 +1,5 @@ import type React from "react"; -import { FaExpand, FaReply, FaTrash } from "react-icons/fa"; +import { FaExpand, FaLanguage, FaReply, FaTrash } from "react-icons/fa"; import type { MessageType } from "../../types"; import { MdAddReaction } from "./icons"; @@ -7,11 +7,14 @@ interface MessageActionsProps { message: MessageType; onReplyClick: () => void; onReactClick: (buttonElement: Element) => void; + onTranslateClick?: () => void; onRedactClick?: () => void; onOpenMedia?: () => void; canRedact?: boolean; canReply?: boolean; + canTranslate?: boolean; canOpenMedia?: boolean; + isTranslating?: boolean; inline?: boolean; } @@ -19,11 +22,14 @@ export const MessageActions: React.FC = ({ message, onReplyClick, onReactClick, + onTranslateClick, onRedactClick, onOpenMedia, canRedact = false, canReply = !!message.msgid, + canTranslate = false, canOpenMedia = false, + isTranslating = false, inline = false, }) => { return ( @@ -62,6 +68,20 @@ export const MessageActions: React.FC = ({ )} + {canTranslate && onTranslateClick && ( + + )} + ), + StandardReplyNotification: () => null, + SystemMessage: () => null, + WhisperMessage: () => null, + }; +}); + +import { MessageItem } from "../../src/components/message/MessageItem"; +import type { MessageType } from "../../src/types"; + +const message: MessageType = { + id: "msg-1", + msgid: "abc123", + type: "message", + content: "hello world", + timestamp: new Date("2026-04-22T12:00:00Z"), + userId: "alice", + channelId: "channel-1", + serverId: "server-1", + reactions: [], + replyMessage: null, + mentioned: [], +}; + +describe("MessageItem translation", () => { + beforeEach(() => { + mockAvailability.mockReset(); + mockTranslate.mockReset(); + mockTargetLanguage.mockReset(); + mockState.globalSettings.translationTargetLanguage = "es"; + mockAvailability.mockResolvedValue("available"); + mockTranslate.mockResolvedValue("hola mundo"); + mockTargetLanguage.mockReturnValue("es"); + }); + + test("uses the explicit target language setting and renders translated output", async () => { + render( + , + ); + + fireEvent.click( + screen.getByRole("button", { name: /translate from actions/i }), + ); + + await waitFor(() => { + expect(screen.getByText("hola mundo")).toBeInTheDocument(); + }); + + expect(mockTargetLanguage).toHaveBeenCalledWith("es"); + expect(mockTranslate).toHaveBeenCalledWith( + expect.objectContaining({ + sourceLanguage: "en", + targetLanguage: "es", + text: "hello world", + }), + ); + }); +}); diff --git a/tests/components/MetadataDisplay.test.tsx b/tests/components/MetadataDisplay.test.tsx index c5d73dc7..12d9a833 100644 --- a/tests/components/MetadataDisplay.test.tsx +++ b/tests/components/MetadataDisplay.test.tsx @@ -189,6 +189,7 @@ describe("Metadata Display Features", () => { autoFallbackToSingleLine: true, mediaVisibilityLevel: 3, enableMarkdownRendering: false, + translationTargetLanguage: "", awayMessage: "", quitMessage: "ObsidianIRC - Bringing IRC to the future", }, diff --git a/tests/lib/browserTranslation.test.ts b/tests/lib/browserTranslation.test.ts new file mode 100644 index 00000000..ed776d81 --- /dev/null +++ b/tests/lib/browserTranslation.test.ts @@ -0,0 +1,195 @@ +import { afterEach, describe, expect, test, vi } from "vitest"; +import { + canUseBrowserTranslation, + getBrowserTranslationAvailability, + getMessageSourceLanguage, + getPreferredTranslationTargetLanguage, + getPreferredTranslationTargetLanguageFromSetting, + normalizeTranslationLanguageTag, + translateWithBrowser, +} from "../../src/lib/browserTranslation"; + +function setSecureContext(value: boolean) { + Object.defineProperty(window, "isSecureContext", { + configurable: true, + value, + }); +} + +afterEach(() => { + vi.unstubAllGlobals(); + vi.restoreAllMocks(); + setSecureContext(true); +}); + +describe("browserTranslation", () => { + test("normalizes translation language tags to primary subtags", () => { + expect(normalizeTranslationLanguageTag("en-US")).toBe("en"); + expect(normalizeTranslationLanguageTag(" ES ")).toBe("es"); + expect(normalizeTranslationLanguageTag(undefined)).toBeNull(); + }); + + test("derives preferred target language from navigator", () => { + Object.defineProperty(window, "navigator", { + configurable: true, + value: { + language: "pt-BR", + languages: ["pt-BR", "en-US"], + }, + }); + + expect(getPreferredTranslationTargetLanguage()).toBe("pt"); + }); + + test("prefers an explicit target language setting over navigator", () => { + Object.defineProperty(window, "navigator", { + configurable: true, + value: { + language: "pt-BR", + languages: ["pt-BR", "en-US"], + }, + }); + + expect(getPreferredTranslationTargetLanguageFromSetting("es-MX")).toBe( + "es", + ); + }); + + test("derives source language from message tags with english fallback", () => { + expect( + getMessageSourceLanguage({ + "+draft/language": "fr-CA", + }), + ).toBe("fr"); + expect(getMessageSourceLanguage()).toBe("en"); + }); + + test("reports unsupported when Translator is missing", async () => { + setSecureContext(true); + + expect(canUseBrowserTranslation()).toBe(false); + await expect( + getBrowserTranslationAvailability({ + sourceLanguage: "en", + targetLanguage: "es", + }), + ).resolves.toBe("unsupported"); + }); + + test("reports insecure-context before touching Translator", async () => { + const availability = vi.fn(); + setSecureContext(false); + vi.stubGlobal("Translator", { availability, create: vi.fn() }); + + expect(canUseBrowserTranslation()).toBe(false); + await expect( + getBrowserTranslationAvailability({ + sourceLanguage: "en", + targetLanguage: "es", + }), + ).resolves.toBe("insecure-context"); + expect(availability).not.toHaveBeenCalled(); + }); + + test("maps native availability values", async () => { + setSecureContext(true); + vi.stubGlobal("Translator", { + availability: vi.fn().mockResolvedValue("downloadable"), + create: vi.fn(), + }); + + await expect( + getBrowserTranslationAvailability({ + sourceLanguage: "en", + targetLanguage: "es", + }), + ).resolves.toBe("downloadable"); + }); + + test("returns unavailable for same-language requests", async () => { + const availability = vi.fn(); + setSecureContext(true); + vi.stubGlobal("Translator", { + availability, + create: vi.fn(), + }); + + await expect( + getBrowserTranslationAvailability({ + sourceLanguage: "en", + targetLanguage: "en", + }), + ).resolves.toBe("unavailable"); + expect(availability).not.toHaveBeenCalled(); + }); + + test("creates, translates, reports progress, and destroys", async () => { + const addEventListener = vi.fn( + ( + _event: string, + listener: (event: Pick) => void, + ) => { + listener({ loaded: 0.5 }); + }, + ); + const destroy = vi.fn(); + const translate = vi.fn().mockResolvedValue("hola"); + const create = vi.fn().mockResolvedValue({ translate, destroy }); + const progress = vi.fn(); + + setSecureContext(true); + vi.stubGlobal("Translator", { + availability: vi.fn(), + create, + }); + + await expect( + translateWithBrowser({ + sourceLanguage: "en", + targetLanguage: "es", + text: "hello", + onDownloadProgress: progress, + }), + ).resolves.toBe("hola"); + + expect(create).toHaveBeenCalledOnce(); + expect(create).toHaveBeenCalledWith( + expect.objectContaining({ + sourceLanguage: "en", + targetLanguage: "es", + monitor: expect.any(Function), + }), + ); + + const [{ monitor }] = create.mock.calls[0]; + monitor({ addEventListener } as unknown as EventTarget); + + expect(addEventListener).toHaveBeenCalledWith( + "downloadprogress", + expect.any(Function), + ); + expect(progress).toHaveBeenCalledWith(0.5); + expect(translate).toHaveBeenCalledWith("hello", { signal: undefined }); + expect(destroy).toHaveBeenCalledOnce(); + }); + + test("still destroys the translator when translation fails", async () => { + const destroy = vi.fn(); + const translate = vi.fn().mockRejectedValue(new Error("boom")); + + setSecureContext(true); + vi.stubGlobal("Translator", { + availability: vi.fn(), + create: vi.fn().mockResolvedValue({ translate, destroy }), + }); + + await expect( + translateWithBrowser({ + sourceLanguage: "en", + targetLanguage: "es", + text: "hello", + }), + ).rejects.toThrow("boom"); + expect(destroy).toHaveBeenCalledOnce(); + }); +}); diff --git a/tests/store/index.test.ts b/tests/store/index.test.ts index 08dce17f..f6dd909a 100644 --- a/tests/store/index.test.ts +++ b/tests/store/index.test.ts @@ -58,6 +58,15 @@ describe("Store", () => { const newTheme = useStore.getState().ui.isDarkMode; expect(newTheme).not.toBe(initialTheme); }); + + test("should update translation target language", () => { + const { updateGlobalSettings } = useStore.getState(); + + updateGlobalSettings({ translationTargetLanguage: "es" }); + expect(useStore.getState().globalSettings.translationTargetLanguage).toBe( + "es", + ); + }); }); describe("server selection", () => { diff --git a/tests/store/join.test.ts b/tests/store/join.test.ts index 935bfaad..a34b3547 100644 --- a/tests/store/join.test.ts +++ b/tests/store/join.test.ts @@ -38,6 +38,7 @@ function setupServer(channelOverrides: Partial = {}) { globalSettings: { showEvents: true, showJoinsParts: true, + translationTargetLanguage: "", }, } as unknown as AppState); } diff --git a/tests/store/localStorage.test.ts b/tests/store/localStorage.test.ts index 67df7fe7..26d872f8 100644 --- a/tests/store/localStorage.test.ts +++ b/tests/store/localStorage.test.ts @@ -72,6 +72,12 @@ describe("settings.load migration", () => { // Old flag is left as-is when not migrating (no need to clean it up) }); + test("preserves explicit translation target language", () => { + mockStored({ translationTargetLanguage: "es" }); + const result = settings.load(); + expect(result.translationTargetLanguage).toBe("es"); + }); + test("returns empty object on invalid JSON", () => { vi.mocked(window.localStorage.getItem).mockReturnValue("not-json"); const result = settings.load(); From a28aaa8d53d25ab241a3b7b6e198510a0860f3cb Mon Sep 17 00:00:00 2001 From: lou Date: Wed, 22 Apr 2026 10:23:19 -0600 Subject: [PATCH 2/4] Refine browser translation review fixes --- src/components/message/MessageItem.tsx | 22 ++- src/components/mobile/MessageBottomSheet.tsx | 4 + src/lib/browserTranslation.ts | 167 +++++++++++++----- src/lib/settings/definitions/allSettings.ts | 12 ++ src/store/index.ts | 1 - .../MessageItem.translation.test.tsx | 46 ++++- tests/lib/browserTranslation.test.ts | 78 +++++--- 7 files changed, 261 insertions(+), 69 deletions(-) diff --git a/src/components/message/MessageItem.tsx b/src/components/message/MessageItem.tsx index 472bab92..5fa2edbb 100644 --- a/src/components/message/MessageItem.tsx +++ b/src/components/message/MessageItem.tsx @@ -11,6 +11,7 @@ import { useLongPress } from "../../hooks/useLongPress"; import { useMediaQuery } from "../../hooks/useMediaQuery"; import { canUseBrowserTranslation, + detectMessageSourceLanguage, getBrowserTranslationAvailability, getMessageSourceLanguage, getPreferredTranslationTargetLanguageFromSetting, @@ -478,7 +479,18 @@ export const MessageItem = memo((props: MessageItemProps) => { const targetLanguage = getPreferredTranslationTargetLanguageFromSetting( translationTargetLanguage, ); - const sourceLanguage = getMessageSourceLanguage(message.tags); + const sourceLanguage = + getMessageSourceLanguage(message.tags) ?? + (await detectMessageSourceLanguage({ text: messageContent })); + + if (!sourceLanguage) { + setTranslationState({ + status: "error", + targetLanguage, + message: "Could not determine the message language for translation.", + }); + return; + } if (sourceLanguage === targetLanguage) { setTranslationState({ @@ -980,9 +992,11 @@ export const MessageItem = memo((props: MessageItemProps) => { {translationState.status === "translated" && translatedHtmlContent && ( -
- {translatedHtmlContent} -
+ +
+ {translatedHtmlContent} +
+ )} {translationState.status === "downloading" && (
diff --git a/src/components/mobile/MessageBottomSheet.tsx b/src/components/mobile/MessageBottomSheet.tsx index e2ed38d7..57e99dc0 100644 --- a/src/components/mobile/MessageBottomSheet.tsx +++ b/src/components/mobile/MessageBottomSheet.tsx @@ -44,6 +44,7 @@ const MessageBottomSheet: React.FC = ({ icon: React.ReactNode; onClick: (e: React.MouseEvent) => void; className?: string; + disabled?: boolean; }[] = []; if (canReply && onReply) { @@ -73,10 +74,12 @@ const MessageBottomSheet: React.FC = ({ label: isTranslating ? "Translating" : "Translate", icon: , onClick: () => { + if (isTranslating) return; onTranslate(); onClose(); }, className: isTranslating ? "text-sky-300/70" : undefined, + disabled: isTranslating, }); } @@ -114,6 +117,7 @@ const MessageBottomSheet: React.FC = ({ className={`w-full flex items-center gap-3 px-4 py-3 rounded-lg active:bg-discord-dark-400 text-left ${action.className || "text-white"}`} style={{ minHeight: "48px" }} onClick={action.onClick} + disabled={action.disabled} > {action.icon} {action.label} diff --git a/src/lib/browserTranslation.ts b/src/lib/browserTranslation.ts index 9a3cad0c..d61746bc 100644 --- a/src/lib/browserTranslation.ts +++ b/src/lib/browserTranslation.ts @@ -20,21 +20,96 @@ export interface BrowserTranslationRequest extends TranslationLanguagePair { onDownloadProgress?: (progress: number) => void; } +export interface BrowserLanguageDetectionRequest { + text: string; + signal?: AbortSignal; + onDownloadProgress?: (progress: number) => void; +} + +interface CreateMonitorLike { + addEventListener( + type: "downloadprogress", + listener: (event: Event) => void, + ): void; +} + +interface BrowserLanguageDetectionResult { + detectedLanguage?: string; +} + +interface BrowserLanguageDetectorInstance { + detect(input: string): Promise; + destroy(): void; +} + +interface BrowserLanguageDetectorStatic { + create(options?: { + signal?: AbortSignal; + monitor?: (monitor: CreateMonitorLike) => void; + }): Promise; +} + +interface BrowserTranslatorInstance { + translate(input: string, options?: { signal?: AbortSignal }): Promise; + destroy(): void; +} + +interface BrowserTranslatorStatic { + availability( + options: TranslationLanguagePair, + ): Promise; + create(options: { + sourceLanguage: string; + targetLanguage: string; + signal?: AbortSignal; + monitor?: (monitor: CreateMonitorLike) => void; + }): Promise; +} + +function getLanguageDetectorApi(): BrowserLanguageDetectorStatic | null { + const maybeLanguageDetector = ( + globalThis as typeof globalThis & { + LanguageDetector?: BrowserLanguageDetectorStatic; + } + ).LanguageDetector; + return maybeLanguageDetector ?? null; +} + +function getTranslatorApi(): BrowserTranslatorStatic | null { + const maybeTranslator = ( + globalThis as typeof globalThis & { + Translator?: BrowserTranslatorStatic; + } + ).Translator; + return maybeTranslator ?? null; +} + +/** + * Canonicalizes a BCP 47 language tag without discarding script or region subtags. + */ export function normalizeTranslationLanguageTag( language: string | null | undefined, ): string | null { const trimmed = language?.trim(); if (!trimmed) return null; - const [primarySubtag] = trimmed.split("-"); - const normalized = primarySubtag?.toLowerCase(); - return normalized || null; + try { + return Intl.getCanonicalLocales(trimmed)[0] ?? null; + } catch { + return null; + } } +/** + * Resolves the preferred target language, falling back to the browser locale. + */ export function getPreferredTranslationTargetLanguage(): string { return getPreferredTranslationTargetLanguageFromSetting(); } +/** + * Resolves the preferred target language using an explicit setting when present. + */ export function getPreferredTranslationTargetLanguageFromSetting( explicitLanguage?: string | null, ): string { @@ -54,56 +129,63 @@ export function getPreferredTranslationTargetLanguageFromSetting( return "en"; } +/** + * Reads the source language from IRC message metadata when one is available. + */ export function getMessageSourceLanguage( tags?: Record, -): string { - return ( - normalizeTranslationLanguageTag( - tags?.["+draft/language"] ?? - tags?.["draft/language"] ?? - tags?.["+language"] ?? - tags?.language, - ) ?? "en" +): string | null { + return normalizeTranslationLanguageTag( + tags?.["+draft/language"] ?? + tags?.["draft/language"] ?? + tags?.["+language"] ?? + tags?.language, ); } -interface CreateMonitorLike { - addEventListener( - type: "downloadprogress", - listener: (event: Event) => void, - ): void; +export function canUseBrowserTranslation(): boolean { + return window.isSecureContext && getTranslatorApi() !== null; } -interface BrowserTranslatorInstance { - translate(input: string, options?: { signal?: AbortSignal }): Promise; - destroy(): void; -} +/** + * Detects the source language for untagged messages using the browser detector API. + */ +export async function detectMessageSourceLanguage({ + text, + signal, + onDownloadProgress, +}: BrowserLanguageDetectionRequest): Promise { + if (!window.isSecureContext) return null; -interface BrowserTranslatorStatic { - availability( - options: TranslationLanguagePair, - ): Promise; - create(options: { - sourceLanguage: string; - targetLanguage: string; - signal?: AbortSignal; - monitor?: (monitor: CreateMonitorLike) => void; - }): Promise; -} + const languageDetectorApi = getLanguageDetectorApi(); + if (!languageDetectorApi) return null; -function getTranslatorApi(): BrowserTranslatorStatic | null { - const maybeTranslator = ( - globalThis as typeof globalThis & { - Translator?: BrowserTranslatorStatic; + try { + const detector = await languageDetectorApi.create({ + signal, + monitor: onDownloadProgress + ? (monitor) => { + monitor.addEventListener("downloadprogress", (event) => { + onDownloadProgress((event as ProgressEvent).loaded); + }); + } + : undefined, + }); + + try { + const results = await detector.detect(text); + return normalizeTranslationLanguageTag(results[0]?.detectedLanguage); + } finally { + detector.destroy(); } - ).Translator; - return maybeTranslator ?? null; -} - -export function canUseBrowserTranslation(): boolean { - return window.isSecureContext && getTranslatorApi() !== null; + } catch { + return null; + } } +/** + * Checks whether the runtime supports a requested translation pair. + */ export async function getBrowserTranslationAvailability({ sourceLanguage, targetLanguage, @@ -127,6 +209,9 @@ export async function getBrowserTranslationAvailability({ } } +/** + * Creates a translator, runs a translation, and disposes the translator instance. + */ export async function translateWithBrowser({ sourceLanguage, targetLanguage, diff --git a/src/lib/settings/definitions/allSettings.ts b/src/lib/settings/definitions/allSettings.ts index bab6d1ea..70c045cf 100644 --- a/src/lib/settings/definitions/allSettings.ts +++ b/src/lib/settings/definitions/allSettings.ts @@ -260,6 +260,18 @@ const preferenceSettings: SettingDefinition[] = [ type: "text", defaultValue: "", placeholder: "e.g. en, es, ja", + validation: { + custom: (value) => { + const text = String(value).trim(); + if (!text) return true; + + try { + return Intl.getCanonicalLocales(text).length > 0; + } catch { + return "Enter a valid BCP 47 language tag such as en, es-MX, or zh-Hant."; + } + }, + }, searchKeywords: ["translate", "translation", "language", "locale"], priority: 3, }, diff --git a/src/store/index.ts b/src/store/index.ts index 30f8ffb6..81e7a07f 100644 --- a/src/store/index.ts +++ b/src/store/index.ts @@ -938,7 +938,6 @@ const useStore = create((set, get) => ({ mediaVisibilityLevel: 1 as MediaVisibilityLevel, // Markdown settings enableMarkdownRendering: false, - // Translation settings translationTargetLanguage: "", // Status messages awayMessage: "", diff --git a/tests/components/MessageItem.translation.test.tsx b/tests/components/MessageItem.translation.test.tsx index 8d3f6961..c6faf7bb 100644 --- a/tests/components/MessageItem.translation.test.tsx +++ b/tests/components/MessageItem.translation.test.tsx @@ -21,6 +21,8 @@ const mockState = { }; const mockAvailability = vi.fn(); +const mockDetectLanguage = vi.fn(); +const mockSourceLanguage = vi.fn(); const mockTranslate = vi.fn(); const mockTargetLanguage = vi.fn(); @@ -39,9 +41,11 @@ vi.mock("../../src/hooks/useMediaQuery", () => ({ vi.mock("../../src/lib/browserTranslation", () => ({ canUseBrowserTranslation: () => true, + detectMessageSourceLanguage: (...args: unknown[]) => + mockDetectLanguage(...args), getBrowserTranslationAvailability: (...args: unknown[]) => mockAvailability(...args), - getMessageSourceLanguage: () => "en", + getMessageSourceLanguage: (...args: unknown[]) => mockSourceLanguage(...args), getPreferredTranslationTargetLanguageFromSetting: (...args: unknown[]) => mockTargetLanguage(...args), translateWithBrowser: (...args: unknown[]) => mockTranslate(...args), @@ -154,10 +158,14 @@ const message: MessageType = { describe("MessageItem translation", () => { beforeEach(() => { mockAvailability.mockReset(); + mockDetectLanguage.mockReset(); + mockSourceLanguage.mockReset(); mockTranslate.mockReset(); mockTargetLanguage.mockReset(); mockState.globalSettings.translationTargetLanguage = "es"; mockAvailability.mockResolvedValue("available"); + mockDetectLanguage.mockResolvedValue("fr-CA"); + mockSourceLanguage.mockReturnValue("en"); mockTranslate.mockResolvedValue("hola mundo"); mockTargetLanguage.mockReturnValue("es"); }); @@ -196,4 +204,40 @@ describe("MessageItem translation", () => { }), ); }); + + test("detects the source language when the message has no language tag", async () => { + mockState.globalSettings.translationTargetLanguage = ""; + mockSourceLanguage.mockReturnValue(null); + mockTargetLanguage.mockReturnValue("pt-BR"); + + render( + , + ); + + fireEvent.click( + screen.getByRole("button", { name: /translate from actions/i }), + ); + + await waitFor(() => { + expect(mockDetectLanguage).toHaveBeenCalledWith({ text: "hello world" }); + }); + expect(mockTranslate).toHaveBeenCalledWith( + expect.objectContaining({ + sourceLanguage: "fr-CA", + targetLanguage: "pt-BR", + }), + ); + }); }); diff --git a/tests/lib/browserTranslation.test.ts b/tests/lib/browserTranslation.test.ts index ed776d81..5945451a 100644 --- a/tests/lib/browserTranslation.test.ts +++ b/tests/lib/browserTranslation.test.ts @@ -1,6 +1,7 @@ import { afterEach, describe, expect, test, vi } from "vitest"; import { canUseBrowserTranslation, + detectMessageSourceLanguage, getBrowserTranslationAvailability, getMessageSourceLanguage, getPreferredTranslationTargetLanguage, @@ -9,6 +10,11 @@ import { translateWithBrowser, } from "../../src/lib/browserTranslation"; +const originalNavigatorDescriptor = Object.getOwnPropertyDescriptor( + window, + "navigator", +); + function setSecureContext(value: boolean) { Object.defineProperty(window, "isSecureContext", { configurable: true, @@ -16,52 +22,80 @@ function setSecureContext(value: boolean) { }); } +function setNavigatorLanguages(language: string, languages: string[]) { + Object.defineProperty(window, "navigator", { + configurable: true, + value: { + ...window.navigator, + language, + languages, + }, + }); +} + afterEach(() => { vi.unstubAllGlobals(); vi.restoreAllMocks(); setSecureContext(true); + if (originalNavigatorDescriptor) { + Object.defineProperty(window, "navigator", originalNavigatorDescriptor); + } }); describe("browserTranslation", () => { - test("normalizes translation language tags to primary subtags", () => { - expect(normalizeTranslationLanguageTag("en-US")).toBe("en"); - expect(normalizeTranslationLanguageTag(" ES ")).toBe("es"); + test("preserves and canonicalizes full BCP 47 language tags", () => { + expect(normalizeTranslationLanguageTag("en-US")).toBe("en-US"); + expect(normalizeTranslationLanguageTag("zh-hant")).toBe("zh-Hant"); expect(normalizeTranslationLanguageTag(undefined)).toBeNull(); }); test("derives preferred target language from navigator", () => { - Object.defineProperty(window, "navigator", { - configurable: true, - value: { - language: "pt-BR", - languages: ["pt-BR", "en-US"], - }, - }); + setNavigatorLanguages("pt-BR", ["pt-BR", "en-US"]); - expect(getPreferredTranslationTargetLanguage()).toBe("pt"); + expect(getPreferredTranslationTargetLanguage()).toBe("pt-BR"); }); test("prefers an explicit target language setting over navigator", () => { - Object.defineProperty(window, "navigator", { - configurable: true, - value: { - language: "pt-BR", - languages: ["pt-BR", "en-US"], - }, - }); + setNavigatorLanguages("pt-BR", ["pt-BR", "en-US"]); expect(getPreferredTranslationTargetLanguageFromSetting("es-MX")).toBe( - "es", + "es-MX", ); }); - test("derives source language from message tags with english fallback", () => { + test("falls back to the browser language when the setting is empty", () => { + setNavigatorLanguages("pt-BR", ["pt-BR", "en-US"]); + + expect(getPreferredTranslationTargetLanguageFromSetting("")).toBe("pt-BR"); + }); + + test("derives source language from message tags without forcing english", () => { expect( getMessageSourceLanguage({ "+draft/language": "fr-CA", }), - ).toBe("fr"); - expect(getMessageSourceLanguage()).toBe("en"); + ).toBe("fr-CA"); + expect(getMessageSourceLanguage()).toBeNull(); + }); + + test("detects source language with the browser language detector", async () => { + const detect = vi.fn().mockResolvedValue([{ detectedLanguage: "pt-BR" }]); + const destroy = vi.fn(); + + vi.stubGlobal("LanguageDetector", { + create: vi.fn().mockResolvedValue({ detect, destroy }), + }); + + await expect( + detectMessageSourceLanguage({ text: "ola mundo" }), + ).resolves.toBe("pt-BR"); + expect(destroy).toHaveBeenCalledOnce(); + }); + + test("returns null when language detection is unavailable", async () => { + await expect( + detectMessageSourceLanguage({ text: "hola mundo" }), + ).resolves.toBeNull(); }); test("reports unsupported when Translator is missing", async () => { From 917707102b33c15ade4d059dd59e8ed92282cc6b Mon Sep 17 00:00:00 2001 From: louzt <179385168+louzt@users.noreply.github.com> Date: Wed, 22 Apr 2026 15:58:21 -0600 Subject: [PATCH 3/4] fix: harden browser translation lifecycle and detection quality --- src/components/message/MessageItem.tsx | 176 +++++++++++++----- src/lib/browserTranslation.ts | 26 ++- .../MessageItem.translation.test.tsx | 71 ++++++- tests/lib/browserTranslation.test.ts | 61 +++++- 4 files changed, 277 insertions(+), 57 deletions(-) diff --git a/src/components/message/MessageItem.tsx b/src/components/message/MessageItem.tsx index 5fa2edbb..7018d858 100644 --- a/src/components/message/MessageItem.tsx +++ b/src/components/message/MessageItem.tsx @@ -3,6 +3,7 @@ import { memo, startTransition, useCallback, + useEffect, useMemo, useRef, useState, @@ -186,6 +187,8 @@ export const MessageItem = memo((props: MessageItemProps) => { const [translationState, setTranslationState] = useState({ status: "idle", }); + const activeTranslationAbortRef = useRef(null); + const activeTranslationRunRef = useRef(0); const longPress = useLongPress({ onLongPress: () => setSheetOpen(true), }); @@ -374,6 +377,43 @@ export const MessageItem = memo((props: MessageItemProps) => { // message.content is already combined for multiline messages by the IRC client const messageContent = message.content; + const translationMessageKey = `${message.id ?? ""}:${message.msgid ?? ""}:${messageContent}`; + const lastTranslationMessageKeyRef = useRef(translationMessageKey); + + const cancelActiveTranslation = useCallback(() => { + activeTranslationRunRef.current += 1; + activeTranslationAbortRef.current?.abort(); + activeTranslationAbortRef.current = null; + }, []); + + const beginTranslationRun = useCallback(() => { + activeTranslationAbortRef.current?.abort(); + + const controller = new AbortController(); + const runId = activeTranslationRunRef.current + 1; + + activeTranslationRunRef.current = runId; + activeTranslationAbortRef.current = controller; + + return { controller, runId }; + }, []); + + const isActiveTranslationRun = useCallback( + (runId: number, signal?: AbortSignal) => + activeTranslationRunRef.current === runId && !(signal?.aborted ?? false), + [], + ); + + useEffect(() => { + if (lastTranslationMessageKeyRef.current !== translationMessageKey) { + lastTranslationMessageKeyRef.current = translationMessageKey; + setTranslationState({ status: "idle" }); + } + + return () => { + cancelActiveTranslation(); + }; + }, [cancelActiveTranslation, translationMessageKey]); const htmlContent = useMemo( () => @@ -476,71 +516,87 @@ export const MessageItem = memo((props: MessageItemProps) => { const handleTranslateMessage = useCallback(async () => { if (!messageContent.trim()) return; - const targetLanguage = getPreferredTranslationTargetLanguageFromSetting( - translationTargetLanguage, - ); - const sourceLanguage = - getMessageSourceLanguage(message.tags) ?? - (await detectMessageSourceLanguage({ text: messageContent })); + const { controller, runId } = beginTranslationRun(); + const { signal } = controller; + const commitTranslationState = (nextState: TranslationState) => { + if (!isActiveTranslationRun(runId, signal)) return false; - if (!sourceLanguage) { - setTranslationState({ - status: "error", - targetLanguage, - message: "Could not determine the message language for translation.", - }); - return; - } + setTranslationState(nextState); + return true; + }; - if (sourceLanguage === targetLanguage) { - setTranslationState({ - status: "error", - targetLanguage, - message: "Message already matches your preferred language.", - }); - return; - } + try { + const targetLanguage = getPreferredTranslationTargetLanguageFromSetting( + translationTargetLanguage, + ); + const sourceLanguage = + getMessageSourceLanguage(message.tags) ?? + (await detectMessageSourceLanguage({ text: messageContent, signal })); - const availability = await getBrowserTranslationAvailability({ - sourceLanguage, - targetLanguage, - }); + if (!sourceLanguage) { + commitTranslationState({ + status: "error", + targetLanguage, + message: "Could not determine the message language for translation.", + }); + return; + } - if ( - availability === "unsupported" || - availability === "insecure-context" || - availability === "unavailable" - ) { - const errorMessage = - availability === "unsupported" - ? "Translation is not supported in this browser." - : availability === "insecure-context" - ? "Translation requires a secure context." - : "This language pair is unavailable."; - setTranslationState({ - status: "error", + if (sourceLanguage === targetLanguage) { + commitTranslationState({ + status: "error", + targetLanguage, + message: "Message already matches your preferred language.", + }); + return; + } + + const availability = await getBrowserTranslationAvailability({ + sourceLanguage, targetLanguage, - message: errorMessage, }); - return; - } - setTranslationState( - availability === "available" - ? { status: "translating", targetLanguage } - : { status: "downloading", progress: 0, targetLanguage }, - ); + if (!isActiveTranslationRun(runId, signal)) return; + + if ( + availability === "unsupported" || + availability === "insecure-context" || + availability === "unavailable" + ) { + const errorMessage = + availability === "unsupported" + ? "Translation is not supported in this browser." + : availability === "insecure-context" + ? "Translation requires a secure context." + : "This language pair is unavailable."; + commitTranslationState({ + status: "error", + targetLanguage, + message: errorMessage, + }); + return; + } + + if ( + !commitTranslationState( + availability === "available" + ? { status: "translating", targetLanguage } + : { status: "downloading", progress: 0, targetLanguage }, + ) + ) { + return; + } - try { const translatedText = await translateWithBrowser({ sourceLanguage, targetLanguage, text: messageContent, + signal, onDownloadProgress: availability === "available" ? undefined : (progress) => { - setTranslationState({ + commitTranslationState({ status: "downloading", progress, targetLanguage, @@ -548,7 +604,11 @@ export const MessageItem = memo((props: MessageItemProps) => { }, }); + if (!isActiveTranslationRun(runId, signal)) return; + startTransition(() => { + if (!isActiveTranslationRun(runId, signal)) return; + setTranslationState({ status: "translated", targetLanguage, @@ -556,15 +616,29 @@ export const MessageItem = memo((props: MessageItemProps) => { }); }); } catch (error) { + if (!isActiveTranslationRun(runId, signal)) return; + if (signal.aborted) return; + if (error instanceof Error && error.name === "AbortError") return; + const errorMessage = error instanceof Error ? error.message : "Translation failed."; - setTranslationState({ + commitTranslationState({ status: "error", targetLanguage, message: errorMessage, }); + } finally { + if (isActiveTranslationRun(runId, signal)) { + activeTranslationAbortRef.current = null; + } } - }, [message.tags, messageContent, translationTargetLanguage]); + }, [ + beginTranslationRun, + isActiveTranslationRun, + message.tags, + messageContent, + translationTargetLanguage, + ]); // Handle system messages if (isSystem) { diff --git a/src/lib/browserTranslation.ts b/src/lib/browserTranslation.ts index d61746bc..384fe3bf 100644 --- a/src/lib/browserTranslation.ts +++ b/src/lib/browserTranslation.ts @@ -35,8 +35,12 @@ interface CreateMonitorLike { interface BrowserLanguageDetectionResult { detectedLanguage?: string; + confidence?: number; } +const MIN_BROWSER_LANGUAGE_DETECTION_TEXT_LENGTH = 20; +const MIN_BROWSER_LANGUAGE_DETECTION_CONFIDENCE = 0.5; + interface BrowserLanguageDetectorInstance { detect(input: string): Promise; destroy(): void; @@ -156,6 +160,9 @@ export async function detectMessageSourceLanguage({ onDownloadProgress, }: BrowserLanguageDetectionRequest): Promise { if (!window.isSecureContext) return null; + if (text.trim().length < MIN_BROWSER_LANGUAGE_DETECTION_TEXT_LENGTH) { + return null; + } const languageDetectorApi = getLanguageDetectorApi(); if (!languageDetectorApi) return null; @@ -174,7 +181,24 @@ export async function detectMessageSourceLanguage({ try { const results = await detector.detect(text); - return normalizeTranslationLanguageTag(results[0]?.detectedLanguage); + + for (const result of results) { + const normalizedLanguage = normalizeTranslationLanguageTag( + result.detectedLanguage, + ); + + if (!normalizedLanguage || normalizedLanguage === "und") continue; + if ( + typeof result.confidence === "number" && + result.confidence < MIN_BROWSER_LANGUAGE_DETECTION_CONFIDENCE + ) { + continue; + } + + return normalizedLanguage; + } + + return null; } finally { detector.destroy(); } diff --git a/tests/components/MessageItem.translation.test.tsx b/tests/components/MessageItem.translation.test.tsx index c6faf7bb..07a47c7b 100644 --- a/tests/components/MessageItem.translation.test.tsx +++ b/tests/components/MessageItem.translation.test.tsx @@ -141,6 +141,18 @@ vi.mock("../../src/components/message/index", async () => { import { MessageItem } from "../../src/components/message/MessageItem"; import type { MessageType } from "../../src/types"; +function createDeferred() { + let resolve!: (value: T | PromiseLike) => void; + let reject!: (reason?: unknown) => void; + + const promise = new Promise((res, rej) => { + resolve = res; + reject = rej; + }); + + return { promise, resolve, reject }; +} + const message: MessageType = { id: "msg-1", msgid: "abc123", @@ -231,7 +243,9 @@ describe("MessageItem translation", () => { ); await waitFor(() => { - expect(mockDetectLanguage).toHaveBeenCalledWith({ text: "hello world" }); + expect(mockDetectLanguage).toHaveBeenCalledWith( + expect.objectContaining({ text: "hello world" }), + ); }); expect(mockTranslate).toHaveBeenCalledWith( expect.objectContaining({ @@ -240,4 +254,59 @@ describe("MessageItem translation", () => { }), ); }); + + test("ignores stale translation results when a newer request starts", async () => { + const firstTranslation = createDeferred(); + const secondTranslation = createDeferred(); + + mockTranslate + .mockReturnValueOnce(firstTranslation.promise) + .mockReturnValueOnce(secondTranslation.promise); + + render( + , + ); + + const translateButton = screen.getByRole("button", { + name: /translate from actions/i, + }); + + fireEvent.click(translateButton); + + await waitFor(() => { + expect(mockTranslate).toHaveBeenCalledTimes(1); + }); + + fireEvent.click(translateButton); + + await waitFor(() => { + expect(mockTranslate).toHaveBeenCalledTimes(2); + }); + + firstTranslation.resolve("hola viejo"); + + await waitFor(() => { + expect(screen.queryByText("hola viejo")).not.toBeInTheDocument(); + }); + + secondTranslation.resolve("hola nuevo"); + + await waitFor(() => { + expect(screen.getByText("hola nuevo")).toBeInTheDocument(); + }); + + expect(screen.queryByText("hola viejo")).not.toBeInTheDocument(); + }); }); diff --git a/tests/lib/browserTranslation.test.ts b/tests/lib/browserTranslation.test.ts index 5945451a..a5d5570e 100644 --- a/tests/lib/browserTranslation.test.ts +++ b/tests/lib/browserTranslation.test.ts @@ -1,4 +1,4 @@ -import { afterEach, describe, expect, test, vi } from "vitest"; +import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; import { canUseBrowserTranslation, detectMessageSourceLanguage, @@ -42,6 +42,10 @@ afterEach(() => { } }); +beforeEach(() => { + setSecureContext(true); +}); + describe("browserTranslation", () => { test("preserves and canonicalizes full BCP 47 language tags", () => { expect(normalizeTranslationLanguageTag("en-US")).toBe("en-US"); @@ -79,7 +83,9 @@ describe("browserTranslation", () => { }); test("detects source language with the browser language detector", async () => { - const detect = vi.fn().mockResolvedValue([{ detectedLanguage: "pt-BR" }]); + const detect = vi + .fn() + .mockResolvedValue([{ detectedLanguage: "pt-BR", confidence: 0.91 }]); const destroy = vi.fn(); vi.stubGlobal("LanguageDetector", { @@ -87,14 +93,61 @@ describe("browserTranslation", () => { }); await expect( - detectMessageSourceLanguage({ text: "ola mundo" }), + detectMessageSourceLanguage({ text: "ola mundo como voce esta hoje" }), ).resolves.toBe("pt-BR"); expect(destroy).toHaveBeenCalledOnce(); }); - test("returns null when language detection is unavailable", async () => { + test("returns null for short text because language detection would be unreliable", async () => { + const detect = vi.fn(); + + vi.stubGlobal("LanguageDetector", { + create: vi.fn().mockResolvedValue({ detect, destroy: vi.fn() }), + }); + await expect( detectMessageSourceLanguage({ text: "hola mundo" }), + ).resolves.toBe(null); + expect(detect).not.toHaveBeenCalled(); + }); + + test("returns null for low-confidence language detection results", async () => { + const detect = vi + .fn() + .mockResolvedValue([{ detectedLanguage: "pt-BR", confidence: 0.2 }]); + const destroy = vi.fn(); + + vi.stubGlobal("LanguageDetector", { + create: vi.fn().mockResolvedValue({ detect, destroy }), + }); + + await expect( + detectMessageSourceLanguage({ text: "ola mundo como voce esta hoje" }), + ).resolves.toBeNull(); + expect(destroy).toHaveBeenCalledOnce(); + }); + + test("returns null for undetermined language detection results", async () => { + const detect = vi + .fn() + .mockResolvedValue([{ detectedLanguage: "und", confidence: 0.99 }]); + const destroy = vi.fn(); + + vi.stubGlobal("LanguageDetector", { + create: vi.fn().mockResolvedValue({ detect, destroy }), + }); + + await expect( + detectMessageSourceLanguage({ text: "ola mundo como voce esta hoje" }), + ).resolves.toBeNull(); + expect(destroy).toHaveBeenCalledOnce(); + }); + + test("returns null when language detection is unavailable", async () => { + await expect( + detectMessageSourceLanguage({ + text: "hola mundo desde una prueba larga", + }), ).resolves.toBeNull(); }); From 7d2f23ebe754d6c156fa7f2b6a1b976f51955a23 Mon Sep 17 00:00:00 2001 From: louzt <179385168+louzt@users.noreply.github.com> Date: Wed, 22 Apr 2026 16:27:18 -0600 Subject: [PATCH 4/4] test: mirror MessageItem translation test path and stub missing browser APIs Align the MessageItem translation test with the mirrored component path expected by project review guidance, and make browserTranslation tests deterministic by explicitly stubbing missing LanguageDetector/Translator globals where absence is the behavior under test. --- .../MessageItem.translation.test.tsx | 30 +++++++++---------- tests/lib/browserTranslation.test.ts | 3 ++ 2 files changed, 18 insertions(+), 15 deletions(-) rename tests/components/{ => message}/MessageItem.translation.test.tsx (90%) diff --git a/tests/components/MessageItem.translation.test.tsx b/tests/components/message/MessageItem.translation.test.tsx similarity index 90% rename from tests/components/MessageItem.translation.test.tsx rename to tests/components/message/MessageItem.translation.test.tsx index 07a47c7b..c7ce8afd 100644 --- a/tests/components/MessageItem.translation.test.tsx +++ b/tests/components/message/MessageItem.translation.test.tsx @@ -26,7 +26,7 @@ const mockSourceLanguage = vi.fn(); const mockTranslate = vi.fn(); const mockTargetLanguage = vi.fn(); -vi.mock("../../src/hooks/useLongPress", () => ({ +vi.mock("../../../src/hooks/useLongPress", () => ({ useLongPress: () => ({ onTouchStart: vi.fn(), onTouchMove: vi.fn(), @@ -35,11 +35,11 @@ vi.mock("../../src/hooks/useLongPress", () => ({ }), })); -vi.mock("../../src/hooks/useMediaQuery", () => ({ +vi.mock("../../../src/hooks/useMediaQuery", () => ({ useMediaQuery: () => false, })); -vi.mock("../../src/lib/browserTranslation", () => ({ +vi.mock("../../../src/lib/browserTranslation", () => ({ canUseBrowserTranslation: () => true, detectMessageSourceLanguage: (...args: unknown[]) => mockDetectLanguage(...args), @@ -51,19 +51,19 @@ vi.mock("../../src/lib/browserTranslation", () => ({ translateWithBrowser: (...args: unknown[]) => mockTranslate(...args), })); -vi.mock("../../src/lib/ircClient", () => ({ +vi.mock("../../../src/lib/ircClient", () => ({ default: { getCurrentUser: () => ({ username: "bob" }), }, })); -vi.mock("../../src/lib/ircUtils", () => ({ +vi.mock("../../../src/lib/ircUtils", () => ({ isUrlFromFilehost: () => false, isUserVerified: () => false, processMarkdownInText: (content: string) => content, })); -vi.mock("../../src/lib/mediaUtils", () => ({ +vi.mock("../../../src/lib/mediaUtils", () => ({ canShowMedia: () => false, extractMediaFromMessage: () => [], mediaLevelToSettings: () => ({ @@ -73,11 +73,11 @@ vi.mock("../../src/lib/mediaUtils", () => ({ }), })); -vi.mock("../../src/lib/messageFormatter", () => ({ +vi.mock("../../../src/lib/messageFormatter", () => ({ stripIrcFormatting: (content: string) => content, })); -vi.mock("../../src/store", () => { +vi.mock("../../../src/store", () => { const useStore = Object.assign( (selector: (state: typeof mockState) => unknown) => selector(mockState), { getState: () => mockState }, @@ -90,25 +90,25 @@ vi.mock("../../src/store", () => { }; }); -vi.mock("../../src/components/mobile/MessageBottomSheet", () => ({ +vi.mock("../../../src/components/mobile/MessageBottomSheet", () => ({ default: () => null, })); -vi.mock("../../src/components/ui/LinkWrapper", () => ({ +vi.mock("../../../src/components/ui/LinkWrapper", () => ({ EnhancedLinkWrapper: ({ children }: { children: React.ReactNode }) => ( <>{children} ), })); -vi.mock("../../src/components/message/InviteMessage", () => ({ +vi.mock("../../../src/components/message/InviteMessage", () => ({ InviteMessage: () => null, })); -vi.mock("../../src/components/message/MediaPreview", () => ({ +vi.mock("../../../src/components/message/MediaPreview", () => ({ MediaPreview: () => null, })); -vi.mock("../../src/components/message/index", async () => { +vi.mock("../../../src/components/message/index", async () => { const ReactModule = await import("react"); return { @@ -138,8 +138,8 @@ vi.mock("../../src/components/message/index", async () => { }; }); -import { MessageItem } from "../../src/components/message/MessageItem"; -import type { MessageType } from "../../src/types"; +import { MessageItem } from "../../../src/components/message/MessageItem"; +import type { MessageType } from "../../../src/types"; function createDeferred() { let resolve!: (value: T | PromiseLike) => void; diff --git a/tests/lib/browserTranslation.test.ts b/tests/lib/browserTranslation.test.ts index a5d5570e..762bd4d3 100644 --- a/tests/lib/browserTranslation.test.ts +++ b/tests/lib/browserTranslation.test.ts @@ -144,6 +144,8 @@ describe("browserTranslation", () => { }); test("returns null when language detection is unavailable", async () => { + vi.stubGlobal("LanguageDetector", undefined); + await expect( detectMessageSourceLanguage({ text: "hola mundo desde una prueba larga", @@ -153,6 +155,7 @@ describe("browserTranslation", () => { test("reports unsupported when Translator is missing", async () => { setSecureContext(true); + vi.stubGlobal("Translator", undefined); expect(canUseBrowserTranslation()).toBe(false); await expect(