feat: Chat translation - #1214
Conversation
amyjko
left a comment
There was a problem hiding this comment.
This is a solid start! This is going in the right direction; you're using the right translation infrastructure, the message schema looks right to me. The core architecture looks solid.
Here are a few things to work on:
-
All the user-facing text you've added is hard-coded into English. You'll need to update UIText.ts, generate the new locale schema with
npm run schemas, runnpm run locales-fixto add them to the locales, then move the strings toen-US.json, then update the UI to use the various localized text components to render them. There are many examples across the platform for uses of<LocalizedText>. -
Make sure to run
npx prettierto keep formatting consistent. -
Translation is has a large number of sequential network round-trips.
translateMessagesloops over every message and awaits onetranslateMarkupTextcall each. For a long chat that's going to be slow. TheRawTranslatorinterface takes an array of texts and returns them 1:1 — so you can collect all messages sharing a source language, translate them in one call, and map the results back. Definitely add that performance improvement. -
The issue (#771) explicitly suggests optionally saving the translation so we don't re-request it. Right now translations live in transient $state and a message sent after you turn translation on won't get translated until you toggle the dropdown again. I think we should persist translations in the message schema rather than re-translating them, to keep costs down. They should be invalidated if the chat message is edited.
-
In the translation block you render both msg.language (source) above the divider and translations[msg.id].language (target) below. Two bare language codes stacked around a rule reads a little cryptically. Consider a clearer "from X → Y" display, or dropping the redundant source tag.
-
Remember to
run npm run check:now,npm test, andnpm run localesto verify everything.
amyjko
left a comment
There was a problem hiding this comment.
This is good progress! It's getting closer. The batching is done right
(grouped by source language, one call per group, groups running concurrently), the
translations are cached and reused, the error handling degrades per-batch instead of
failing the whole chat, and the strings are properly localized with the right format tags.
The architecture is sound overall.
I did a very careful review today, so the list is long, but everything should be within reach. Most of what's below is about making it actually run, and about some edge cases in chat that translation touches.
Why translation "is still in English"
ChatView.svelte imports functions directly from @db/firebase and bails when it's
falsy. But functions is undefined until something calls getFunctionsInstance()
(src/db/firebase.ts:109) — that's what lazily loads the Functions SDK and wires the
emulator. Nothing in the chat path calls it, so you're hitting the error branch almost
every time. It probably looked intermittent, because CreatorDatabase happens to call
getFunctionsInstance() when it fetches missing creators, so whether translation works
is a race with an unrelated fetch.
Every other call site in the codebase does await getFunctionsInstance() —
Translate.svelte:68, LocaleChooser.svelte:117, accountExists.ts:13, Join.svelte:45,
Login.svelte:84, localize/+page.svelte:802. None of them import functions directly.
Do the same and I think your last checkbox unblocks itself.
While you're there, drop the top-level import { httpsCallable } from 'firebase/functions'
and import it dynamically like src/db/projects/translate.ts does — the static import
defeats the lazy load and pulls the Functions SDK into every page that renders a chat, which is a lot of extra code for the initial bundle.
Rebase on current main before anything else
The branch is 58 commits behind and GitHub reports conflicts, and I think that's causing
most of the noise in this diff:
static/schemas/LocaleText.jsonshows +22,520 / −21,806, but if I normalize both sides through prettier, only 36 lines actually differ — the rest is 4-space → 2-space reindentation. Same forzh-TW(1,478 lines, 20 real),ja-JP,hi-IN. Runningnpx prettier --writeon every changed JSON, as CLAUDE.md asks, collapses this to a diff we can actually read.- More seriously, some existing translations lost content. In
es-MX,Paragraph.docwent from"\¶Párrafo 1.\n\nPárrafo 2.\n\nPárrafo 3.¶'tres párrafos'\"to just"\¶Párrafo 1."— text deleted, and an unterminated\…\block left behind. Same inRefine.doc,PropertyReference.doc,StructureDefinition.doc.bn-BDhas the same truncation plus whole strings quietly re-translated with markup markers dropped (/সীমাবদ্ধ/came back as the untranslated/restrict/) and the register changed. There's smaller damage inmr-IN,ne-NP,zh-CN,ja-JP,hi-IN,vi-VN,id-ID.
None of that is your fault — you ran the tooling as documented, but on a branch predating the fixes. main has since made npm run locales read-only (it reports drift instead of repairing it) and fixed the array-splitting rules that caused those truncations. So: merge main, revert all the locale and schema changes, re-run npm run schemas and npm run locales-fix, then prettier. The only locale changes left should be your four new keys and their $? placeholders.
Correctness
-
Translation bypasses moderation.
translateMessagesskips onlymsg.text === null, sopendingandremovedmessages still get translated — and the{#if translations[msg.id]}block is a sibling of<div class="what">, so it renders outside the moderation guard just above it. That means turning on translation shows reported and removed content to people who shouldn't see it, and writes a translated copy of it to the database. Please mirror the samemsg.moderation === undefined || msg.moderation === 'approved'condition. -
Deleted messages keep their translations.
deleteMessagenulls outtextbut leavestranslationsintact, so a deleted message's content lives on in translated form. This is the invalidation point from my last review — there's no edit path in chat, so delete and moderate are the equivalents. -
Translations aren't counted against the chat size cap.
Chat's constructor trims messages againstMAX_CHAT_MESSAGES_BYTES(128 KB) using onlymessage.text?.length. Cached translations are invisible to that estimate, so a chat translated into a few languages can be several times bigger than the trimmer thinks, and drift toward Firestore's hard 1 MB document limit. After that, sending a message starts failing. -
The write side isn't batched, even though the reads are. After batching the LLM calls, the caching loop calls
saveMessageTranslationonce per message, and each one runs a full transaction that reads and rewrites the entire chat document. Translating a 50-message chat is 50 serialized transactions on one doc. Do a singlesaveMessageTranslations(chat, language, Map<id, text>)that applies them all at once. -
A Small race condition
translations = nextis assigned after several awaits with no guard, so changing the dropdown twice quickly can let an older pass overwrite a newer one. Capture the target and bail if it changed.
A design question
Right now translations only recompute when the dropdown changes, so a message that arrives while translation is on stays untranslated until you toggle it off and on. We might consider an explicit translate button instead of triggering on drop down change.
Reuse
- The
RawTranslatoradapter inChatView.svelteis a copy of the one insrc/db/projects/translate.ts. Let's extract it once (something likegetFirebaseTranslator(functions): RawTranslator) and use it from both. - The ~130 lines of grouping and batching belong in
src/db/translateMarkup.ts, not in a
view component. Tat module already owns this layer, is deliberately backend-agnostic, and has tests. A plural sibling oftranslateMarkupTextwould fit naturally, and would be unit-testable. - For the language pickers:
Translatablehas ~161 languages, so both dropdowns are ~161 items in a narrow chat panel.Translate.sveltealready solves this withLocaleSearch+filterLocalesByQueryand renders entries withLocaleName— I'd reuse those instead of the hand-rolledlanguageOptions/languageName(). - Deduplicating the translatable locales down to bare language codes drops the region, which collapses
zh-CNandzh-TWintozh— Simplified and Traditional become indistinguishable, and the same happens topt-PT/pt-BR. Store the full locale string and compare withlocalesAreEqual, likeTranslate.sveltedoes. MessageSchemaV3andV4in one PR can just be a single V3. Also worth knowing: messages don't carry avfield,MessageSchemaLatestVersionisn't referenced anywhere, and the unknown-version union doesn't do anything since all the added fields are optional. I'd leave that machinery as you found it.src/db/chats/ChatDatabase.test.tsalready testsaddMessage,reportMessage,
moderateMessage, anddeleteMessageagainst a mocked Firestore. Tests for the language tag, the translation cache, and deletion clearing translations would be good.
Localization and accessibility
- The two
role="status"divs are live regions, and we route all dynamic announcements
throughAnnouncer.svelteviagetAnnouncer()— component-local live regions cross-talk with the central one on real screen readers. - For the visible error,
Notice.svelteis the component we use elsewhere (seeTranslate.svelte). - The visible label and the
Optionslabelprop are the same string, so the control gets
announced twice and has no distinct name. Pick one. - A few string issues: the "off" option is a hard-coded
'—'rather than a localized string;errorandmessageErrorare nearly identical, andmessageErrorsays "this chat" when it's about a single message; and the→between languages is hard-coded, which points the wrong way in RTL locales. ATemplate<['from','to']>would let translators control the wording and the direction — remember our template inputs are always named, never numbered. field.languageis correctly tagged[plain], butfieldis meant for input fields — it
probably belongs next totranslate.label.
Housekeeping
Prettier hasn't run on the source files either: there's a stray blank line inside the
@db/translateMarkup import, and ChatDatabase.svelte.ts re-indents some pre-existing code it didn't otherwise touch.
Suggested order
- Merge
main, revert the locale/schema churn, regenerate, run prettier. - Fix
getFunctionsInstance()and verify translation actually runs against the emulator. - Moderation guard and deletion invalidation.
- Extract the shared translator, move batching into
translateMarkup.ts, add tests. - Localization and accessibility cleanup.
npm run check:now,npm test,npm run locales— and read thelocalesoutput forx-marked errors rather than trusting the exit code alone.
This is close! Ping me when you're ready for another pass.
|
Thank you so much for all details! I will start working on it now and will let you know once its ready again |
There was a problem hiding this comment.
I reverted/copy pasted mutliple times , I m not sure why it keeps moving the import place
|
Hi @amyjko , I think I’ve made good progress on the work. I’d appreciate another review when you have time. I was thinking of updating the changelog once everything is finalized, or is it better to update it with every change instead? |
Context
In this pull request, I am implementing a feature to translate chat messages.
Related Issues
Verification
npm run devandnpm run emu, then opened Collaborate and Chat to verify that the dropdown renders correctly.localhost:4000and the Firestore emulator to verify that chat messages are sent and that the schema is updated.Checklist
languagefield (the language the message is written in).getTranslatableLocales().translateMarkupTextwith the LLM as the raw translator.