Feat: Browser-native message translation (distinct from UI localization)#173
Open
louzt wants to merge 4 commits into
Open
Feat: Browser-native message translation (distinct from UI localization)#173louzt wants to merge 4 commits into
louzt wants to merge 4 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughAdds browser-based message translation: new browserTranslation library, UI integration in message components and mobile sheet, a new global setting for translation target language with store/types updates, and comprehensive tests for detection, availability, and translation flows. Changes
Sequence DiagramsequenceDiagram
participant User
participant MessageUI as Message UI
participant TranslationFlow as Translation Handler
participant BrowserAPI as Browser Translator API
participant Store as State Store
participant DOM
User->>MessageUI: Click "Translate"
MessageUI->>TranslationFlow: onTranslateClick()
TranslationFlow->>Store: read translationTargetLanguage
TranslationFlow->>BrowserAPI: getBrowserTranslationAvailability(source,target)
alt unavailable / insecure / unsupported
BrowserAPI-->>TranslationFlow: unavailable|unsupported|insecure-context
TranslationFlow->>Store: set translationState.error
TranslationFlow->>DOM: render error banner
else available / downloadable / downloading
BrowserAPI-->>TranslationFlow: availability
TranslationFlow->>Store: set downloading/translating
TranslationFlow->>BrowserAPI: translateWithBrowser({source,target,text,signal,onDownloadProgress})
BrowserAPI->>BrowserAPI: create translator, emit downloadprogress
BrowserAPI-->>TranslationFlow: translated text
TranslationFlow->>Store: set translated state
Store->>DOM: render translated content banner
end
DOM-->>User: show translation result/status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
src/store/index.ts (1)
941-942: Drop the redundant section comment.Line 942 is self-explanatory, so the new comment on Line 941 can be removed.
♻️ Proposed cleanup
- // Translation settings translationTargetLanguage: "",As per coding guidelines, Comments should explain why, not what; omit comments if code is self-explanatory; keep comments to one line in most cases.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/store/index.ts` around lines 941 - 942, Remove the redundant one-line comment "// Translation settings" that precedes the translationTargetLanguage property; the property translationTargetLanguage: "" is self-explanatory so simply delete that comment line near the translationTargetLanguage declaration (in the same block) to comply with the guideline that comments explain why not what.src/lib/settings/definitions/allSettings.ts (1)
252-265: Consider validating the BCP 47 input.The description advertises BCP 47 semantics ("e.g. en, es, ja"), but the setting accepts any free-form string and is consumed downstream by
Intl/Translator APIs. An invalid tag will silently fail at translation time with no user feedback. A lightweightvalidation.pattern(e.g./^[A-Za-z]{2,3}(-[A-Za-z0-9]{2,8})*$/) or anIntl.getCanonicalLocalescheck in the consumer would catch typos early.🛠️ Proposed fix
type: "text", defaultValue: "", placeholder: "e.g. en, es, ja", searchKeywords: ["translate", "translation", "language", "locale"], priority: 3, + validation: { + pattern: /^$|^[A-Za-z]{2,3}(-[A-Za-z0-9]{2,8})*$/, + }, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/settings/definitions/allSettings.ts` around lines 252 - 265, The translationTargetLanguage setting (id "preferences.translationTargetLanguage", key "translationTargetLanguage") currently accepts any string; add validation to ensure BCP 47-like tags so invalid values are caught early — either add a validation.pattern property (e.g. a regex for primary language subtag plus optional extensions) and a helpful validation message to the settings object in allSettings.ts, or validate/canonicalize the value in the consumer using Intl.getCanonicalLocales (e.g. in the Translator or wherever the setting is read) and surface a user-facing error or fallback when the tag is invalid; update the setting definition and the consumer (Translator/Intl usage) accordingly to prevent silent failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/message/MessageItem.tsx`:
- Around line 981-985: The translated HTML is rendered outside the link-warning
flow so URLs in translatedHtmlContent can bypass ExternalLinkWarningModal; wrap
the translated content with the same EnhancedLinkWrapper used for the original
message (i.e. render translatedHtmlContent inside <EnhancedLinkWrapper ...> when
translationState.status === "translated") so that clicks still route through the
app's external-link handling and openExternalUrl/ExternalLinkWarningModal path;
ensure you pass the same props/handlers to EnhancedLinkWrapper that the original
message uses so the warning flow is preserved.
In `@src/components/mobile/MessageBottomSheet.tsx`:
- Around line 71-81: The Translate action is still clickable while
isTranslating, causing duplicate requests; update the action object pushed
inside the canTranslate && onTranslate block (the object with
label/icon/onClick/className) to prevent clicks when isTranslating by either 1)
adding a disabled: isTranslating flag to that action and make the action
renderer (the component that maps actions to a <button>) consume and pass
disabled to the button, or 2) guard the existing onClick handler to no-op when
isTranslating (e.g., return early if isTranslating before calling onTranslate()
and onClose()). Ensure the change references the same action shape used by
MessageBottomSheet so the rendered button becomes non-interactive and accessible
while translating.
In `@src/lib/browserTranslation.ts`:
- Around line 23-31: The function normalizeTranslationLanguageTag currently
truncates BCP 47 tags by splitting on "-" and returning only the primary subtag;
update normalizeTranslationLanguageTag to preserve the full BCP 47 tag
(including script/region subtags) by simply trimming and returning the full
trimmed string (or null for empty input) instead of splitting and returning only
primarySubtag; keep the function name normalizeTranslationLanguageTag and ensure
it still returns string | null.
- Around line 57-67: The getMessageSourceLanguage function currently defaults to
"en" when no language tags exist; change it to return string | null instead and
stop defaulting to "en" so callers can handle unknown language explicitly (e.g.,
call the Language Detector API). Update the implementation in
getMessageSourceLanguage (and any uses) to return
normalizeTranslationLanguageTag(...) ?? null (remove the "en" fallback) and
adjust callers to detect language via the Language Detector API or disable
translation when null is returned; keep references to
normalizeTranslationLanguageTag and getMessageSourceLanguage to locate and
update all call sites.
In `@tests/components/MessageItem.translation.test.tsx`:
- Around line 154-198: The current test only verifies that MessageItem calls
getPreferredTranslationTargetLanguageFromSetting (mockTargetLanguage) with "es"
but doesn't exercise the explicit-vs-fallback branch inside that helper; add a
companion unit test in tests/lib/browserTranslation.test.ts that imports
getPreferredTranslationTargetLanguageFromSetting (or the exported helper) and
asserts that when mockState.globalSettings.translationTargetLanguage is ""
(empty string) the function returns the browser language (simulate
navigator.language or mock its getter) and when it's "es" it returns "es";
alternatively, in the existing MessageItem test remove/move the mock of
getPreferredTranslationTargetLanguageFromSetting so the real helper runs, or add
a second MessageItem test that sets translationTargetLanguage="" and verifies
mockTranslate is called with targetLanguage equal to the browser locale to cover
the fallback path.
In `@tests/lib/browserTranslation.test.ts`:
- Around line 32-55: The tests overwrite window.navigator and leak its mocked
descriptor across tests; update the test file to save the original navigator
descriptor before each modification and restore it in an afterEach (or use
vi.stubGlobal("navigator", ...) to stub and automatically restore), ensuring
calls that set navigator for getPreferredTranslationTargetLanguage and
getPreferredTranslationTargetLanguageFromSetting are reverted so later tests
aren't order-dependent.
---
Nitpick comments:
In `@src/lib/settings/definitions/allSettings.ts`:
- Around line 252-265: The translationTargetLanguage setting (id
"preferences.translationTargetLanguage", key "translationTargetLanguage")
currently accepts any string; add validation to ensure BCP 47-like tags so
invalid values are caught early — either add a validation.pattern property (e.g.
a regex for primary language subtag plus optional extensions) and a helpful
validation message to the settings object in allSettings.ts, or
validate/canonicalize the value in the consumer using Intl.getCanonicalLocales
(e.g. in the Translator or wherever the setting is read) and surface a
user-facing error or fallback when the tag is invalid; update the setting
definition and the consumer (Translator/Intl usage) accordingly to prevent
silent failures.
In `@src/store/index.ts`:
- Around line 941-942: Remove the redundant one-line comment "// Translation
settings" that precedes the translationTargetLanguage property; the property
translationTargetLanguage: "" is self-explanatory so simply delete that comment
line near the translationTargetLanguage declaration (in the same block) to
comply with the guideline that comments explain why not what.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4a263465-7172-40cd-b1eb-8c0412c4b0ff
📒 Files selected for processing (20)
.gitignoresrc/components/message/MessageActions.tsxsrc/components/message/MessageItem.tsxsrc/components/message/ReactionsWithActions.tsxsrc/components/mobile/MessageBottomSheet.tsxsrc/lib/browserTranslation.tssrc/lib/settings/definitions/allSettings.tssrc/store/index.tssrc/store/types.tstests/App.test.tsxtests/components/ChannelSettingsModal.test.tsxtests/components/LinkSecurityWarningModal.test.tsxtests/components/MediaCommentsSidebar.test.tsxtests/components/MessageActions.test.tsxtests/components/MessageItem.translation.test.tsxtests/components/MetadataDisplay.test.tsxtests/lib/browserTranslation.test.tstests/store/index.test.tstests/store/join.test.tstests/store/localStorage.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/components/MessageItem.translation.test.tsx (1)
1-1: Move this test undertests/components/message/.This test covers
src/components/message/MessageItem.tsx, so the mirrored path should betests/components/message/MessageItem.translation.test.tsx.As per coding guidelines, “Test files must mirror
src/directory structure and use Vitest with renderHook + act for React hooks.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/components/MessageItem.translation.test.tsx` at line 1, Move the test file from tests/components/MessageItem.translation.test.tsx into a mirrored path for the component under tests/components/message/MessageItem.translation.test.tsx; update any relative imports if needed and ensure test tooling matches project guidelines (use Vitest runner and, where React hooks are exercised, use renderHook + act). Confirm the test file name and directory match the source component src/components/message/MessageItem.tsx and update any import paths or test config references so the moved test still resolves component imports and runs under Vitest.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/message/MessageItem.tsx`:
- Around line 476-567: handleTranslateMessage can have its async results
overwrite newer state; add a stale-request guard using a requestId (useRef) that
you increment at the start of each translation attempt and capture into a local
const before any awaits, then before every setTranslationState (including in
onDownloadProgress, success branch, and catch) verify the captured id ===
current ref value and skip updates if not; also increment the requestId in a
useEffect cleanup on unmount so in-flight promises are ignored. Apply this guard
around calls to detectMessageSourceLanguage, getBrowserTranslationAvailability,
and translateWithBrowser (and any onDownloadProgress callbacks) so stale
responses don't update the UI.
In `@src/lib/browserTranslation.ts`:
- Around line 36-41: Update the BrowserLanguageDetectionResult interface to
include a confidence?: number field, and when calling the
BrowserLanguageDetectorInstance.detect(...) and using its first result (the
consumer around the current detection/usage site), guard the result by ensuring
result.detectedLanguage is not 'und' and result.confidence is above a reasonable
threshold (e.g., >= 0.5); if those checks fail, treat the detection as
undefined/fallback instead of using it as the source language to avoid
low-confidence or undetermined tags causing bad translations.
In `@tests/lib/browserTranslation.test.ts`:
- Around line 36-43: The test suite currently only calls setSecureContext(true)
in the afterEach cleanup which lets tests that run in isolation see
window.isSecureContext as false (causing detectMessageSourceLanguage to return
null); add a beforeEach that calls setSecureContext(true) (and any necessary
setup of window.navigator using originalNavigatorDescriptor if used) so the
secure context is initialized before each test, and keep the existing afterEach
cleanup (vi.unstubAllGlobals, vi.restoreAllMocks, setSecureContext(true),
Object.defineProperty(window, "navigator", originalNavigatorDescriptor)) to
restore state afterwards.
---
Nitpick comments:
In `@tests/components/MessageItem.translation.test.tsx`:
- Line 1: Move the test file from
tests/components/MessageItem.translation.test.tsx into a mirrored path for the
component under tests/components/message/MessageItem.translation.test.tsx;
update any relative imports if needed and ensure test tooling matches project
guidelines (use Vitest runner and, where React hooks are exercised, use
renderHook + act). Confirm the test file name and directory match the source
component src/components/message/MessageItem.tsx and update any import paths or
test config references so the moved test still resolves component imports and
runs under Vitest.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1d12f1c2-23bf-4fd7-b669-8e93f5468513
📒 Files selected for processing (7)
src/components/message/MessageItem.tsxsrc/components/mobile/MessageBottomSheet.tsxsrc/lib/browserTranslation.tssrc/lib/settings/definitions/allSettings.tssrc/store/index.tstests/components/MessageItem.translation.test.tsxtests/lib/browserTranslation.test.ts
✅ Files skipped from review due to trivial changes (1)
- src/store/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/mobile/MessageBottomSheet.tsx
- src/lib/settings/definitions/allSettings.ts
louzt
changed the title
louzt
force-pushed
the
feat/issue-101-translation-exploration
branch
from
46e17b4 to
9177071
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/components/MessageItem.translation.test.tsx`:
- Line 1: The test file currently at
tests/components/MessageItem.translation.test.tsx should be moved to mirror the
source component path: create
tests/components/message/MessageItem.translation.test.tsx and delete the old
file; update any import paths inside the test that reference the component
(ensure imports point to src/components/message/MessageItem.tsx) and adjust any
test runner/config references if they rely on file globs so it continues to
match tests/**/*.test.{ts,tsx} conventions.
In `@tests/lib/browserTranslation.test.ts`:
- Around line 146-162: The tests rely on LanguageDetector/Translator being
absent but don't explicitly stub them, making behavior env-dependent; before the
tests that call detectMessageSourceLanguage, canUseBrowserTranslation, and
getBrowserTranslationAvailability, call vi.stubGlobal('LanguageDetector',
undefined) and vi.stubGlobal('Translator', undefined) to force those globals to
be undefined, and ensure you restore them after (e.g., in afterEach or afterAll
via vi.unstubAllGlobals or explicit re-stubs) so other tests are unaffected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4332f79f-34db-48bd-b441-1cce4b5139c4
📒 Files selected for processing (4)
src/components/message/MessageItem.tsxsrc/lib/browserTranslation.tstests/components/MessageItem.translation.test.tsxtests/lib/browserTranslation.test.ts
✅ Files skipped from review due to trivial changes (1)
- src/components/message/MessageItem.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/browserTranslation.ts
…er 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR adds an optional client-side translation action to individual messages as progressive enhancement. It is intentionally scoped as a local browser feature and does not claim to implement issue #101 as written, since #101 asks for an IRC-side/server-side translation facility.
This PR focuses exclusively on dynamic chat message translation. Unlike the i18n efforts in #171 (which localize the static UI/dictionaries), this implementation uses the Browser AI APIs to translate real-time user content on demand. Both features are complementary: #171 makes the app's interface accessible, while this PR makes the conversation accessible.
What this does
What this does not do
Follow-up hardening included on this branch
Since opening the PR, this branch also picked up narrow hardening specific to the browser-translation flow itself:
undMessageItemHow this now composes with the other translation tracks
This PR still stands on its own as a browser-native, backend-free enhancement.
That said, the surrounding follow-up work means the client-side path is now better prepared for maintainers who want to evaluate a self-hosted backend later:
So while this PR does not itself add a backend, it does help prove and stabilize the user-facing interaction model. Combined with #174 and #175, the remaining work for a self-hosted backend experiment shifts more toward deployment / proxy / service configuration than toward re-inventing the frontend UX.
Self-hosted deployment follow-up
For maintainers who want a concrete next-step document for self-hosted translation infrastructure, I wrote a separate follow-up note here:
That note is intentionally framed as deployment guidance and adapter follow-up, not as proof that #173 already delivers IRC-side translation.
Why this split is useful
This gives users an immediate privacy-preserving local enhancement where supported, while keeping the door open for:
It also keeps review boundaries cleaner:
Standards reference
MDN Translator API:
https://developer.mozilla.org/en-US/docs/Web/API/Translator
MDN Translator and Language Detector APIs:
https://developer.mozilla.org/en-US/docs/Web/API/Translator_and_Language_Detector_APIs/Using