feat(viewer): add VIEWER_LANGUAGE env + EN/DE locales#541
Open
ChristianWalterMedia wants to merge 13 commits into
Open
feat(viewer): add VIEWER_LANGUAGE env + EN/DE locales#541ChristianWalterMedia wants to merge 13 commits into
ChristianWalterMedia wants to merge 13 commits into
Conversation
Inventory of visible English strings in the viewer (~237 keys across nav, dashboard, memories, sessions, timeline, lessons, actions, crystals, audit, activity, profile, replay, graph, status, types, table, buttons, loading, modal). Source of truth for upcoming i18n work; no runtime behavior change yet. Signed-off-by: Christian Walter <chris.walter@mail.de>
Adds src/viewer/locales.ts with:
- resolveViewerLanguage() — reads VIEWER_LANGUAGE env, normalizes
de-DE/de_DE → de, lowercases, defaults to en
- loadLocale(lang) — reads src/viewer/locales/<lang>.json with
process-level cache, returns {} when missing (no throw)
- buildLocaleBundle(lang) — returns { lang, messages, fallback } with
en fallback for non-en languages, empty fallback for en itself
Tests cover env normalization, missing-file handling, and bundle shape.
No wiring into the rendered viewer yet — that comes in the next commit.
Signed-off-by: Christian Walter <chris.walter@mail.de>
Adds VIEWER_LOCALE_PLACEHOLDER and wires buildLocaleBundle() through renderViewerDocument(). The bundle JSON is injected as window.__AM_LOCALE__ inside the existing nonced <script> tag — no new script element, no external fetch, the CSP nonce model is unchanged. < is escaped to < in the payload so a translation containing '</script' cannot break out of the inline script. The viewer template gets a single seeding line that holds the placeholder; the runtime t() helper arrives in the next commit. Signed-off-by: Christian Walter <chris.walter@mail.de>
Adds a 30-line IIFE in the viewer's inline script:
- t(key, vars) — dot-path lookup against window.__AM_LOCALE__.messages
with en fallback per key and {placeholder} interpolation
- applyI18n(root) — replaces textContent on [data-i18n] elements and
attributes on [data-i18n-attr="attr:key,attr:key"] elements
- DOMContentLoaded hook runs applyI18n(document) once and sets the
<html lang> attribute
Markup tagging and dynamic-string conversion come in the next two
commits.
Signed-off-by: Christian Walter <chris.walter@mail.de>
Tags visible static elements with data-i18n="<namespace>.<key>" matching
the keys in src/viewer/locales/en.json. The original English text stays
in place as a no-JS fallback; the DOMContentLoaded pass added in the
previous commit overwrites textContent (and selected attributes via
data-i18n-attr) with the resolved locale string.
Dynamic strings built inside the inline <script> render functions are
not touched here — those go through t() calls in the next commit.
No visible change in the en case: t("nav.dashboard") returns
"Dashboard", identical to the literal already in markup.
Signed-off-by: Christian Walter <chris.walter@mail.de>
Converts template-literal strings inside the viewer's inline script (renderDashboard, renderMemories, renderSessions, renderTimeline, renderLessons, renderActions, renderCrystals, renderAudit, renderActivity, renderProfile, renderReplay, empty-state generators, table headers, type badges, status labels, loading placeholders) to call t(key) instead of embedding English literals. API enum values (m.type, node.type, s.status, etc.) are untouched — only their display form is mapped via t(). Filters, search, and persistence keep operating on the raw enum strings. Adds var t = window.t at module scope so that render functions can call t() as a plain identifier in VM sandbox contexts (required by the viewer-session-id unit tests). Signed-off-by: Christian Walter <chris.walter@mail.de>
Mirrors the structure of src/viewer/locales/en.json with German values for ~237 UI strings: nav, dashboard cards, gauges, first-run hero, memories empty state, sessions table, status labels (AKTIV/ ABGESCHLOSSEN/…), type and graph-node-type display names, replay detail labels, modal text, and per-tab content for timeline, lessons, actions, crystals, audit, activity, profile, replay, graph. Adds a structural-parity test that fails if a future en.json change adds a top-level key without a corresponding de.json entry. Signed-off-by: Christian Walter <chris.walter@mail.de>
Extends the build script so src/viewer/locales/*.json is copied to dist/viewer/locales/ during build. Without this, an npm-installed agentmemory would not find any locale files at runtime and would silently fall back to the empty-locale path. Signed-off-by: Christian Walter <chris.walter@mail.de>
- README gains a 'Viewer language' subsection showing the env switch and pointing contributors at CONTRIBUTING.md. - .env.example documents VIEWER_LANGUAGE next to the other UI flags. - CONTRIBUTING.md gets a 'Contributing a translation' how-to. - CHANGELOG.md records the change under Unreleased. Signed-off-by: Christian Walter <chris.walter@mail.de>
Final i18n coverage pass. Adds six new keys covering compound or
suffix strings that the per-render conversion missed:
- dashboard.edges_count — '{n} edges' suffix on Graph Nodes card
- dashboard.token_savings_sub — Token Savings compound sub-label
- timeline.no_obs_filter_suffix — empty-state filter branch
- timeline.no_obs_session_suffix — empty-state session branch
- actions.three_ways_intro — Actions empty-state intro line
- audit.empty_body — Audit empty-state second paragraph
en.json / de.json both updated; structural parity test still green.
Signed-off-by: Christian Walter <chris.walter@mail.de>
|
@ChristianWalterMedia is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThis PR adds viewer i18n: server builds/injects a locale bundle (from VIEWER_LANGUAGE), client-side t()/applyI18n runtime applies translations, ships ChangesViewer Internationalization
Sequence DiagramsequenceDiagram
participant Server
participant Filesystem
participant Browser
Server->>Filesystem: read src/viewer/locales/<lang>.json
Filesystem-->>Server: locale JSON
Server->>Server: buildLocaleBundle(lang) (messages + fallback)
Server->>Browser: renderViewerDocument() injects window.__AM_LOCALE__ in nonce script
Browser->>Browser: on DOMContentLoaded run window.applyI18n()
Browser->>Browser: t()/tRaw() resolve keys and update DOM elements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/viewer/locales.ts (1)
10-25: 💤 Low valueConsider caching the locales directory path at module level.
The
localesDir()function is called on everyloadLocalecache miss, but its result is stable for the process lifetime. Caching the result at module scope would avoid redundant filesystem probes.♻️ Proposed optimization
+let cachedLocalesDir: string | null = null; + function localesDir(): string { + if (cachedLocalesDir !== null) return cachedLocalesDir; const base = dirname(fileURLToPath(import.meta.url)); for (const candidate of [ join(base, "..", "src", "viewer", "locales"), join(base, "..", "viewer", "locales"), join(base, "viewer", "locales"), ]) { try { readdirSync(candidate); + cachedLocalesDir = candidate; return candidate; } catch { // candidate path does not exist; try next } } + cachedLocalesDir = ""; return ""; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/viewer/locales.ts` around lines 10 - 25, Cache the resolved locales directory at module scope instead of computing it on every call: compute the directory once using the existing logic in localesDir (which probes join(...) candidates and calls readdirSync) and store the result in a module-level constant (e.g., LOCALES_DIR or resolvedLocalesDir), then have localesDir() or loadLocale() read from that cached constant so subsequent cache misses avoid repeated filesystem probes; ensure the initial computation preserves the existing fallback behavior (returning empty string if none found).test/viewer-i18n.test.ts (1)
110-116: ⚡ Quick winConsider extending structural parity to nested keys.
The current test validates only top-level key parity between
enanddelocale files. While this matches the PR's stated "top-level key parity" scope, nested structure mismatches (e.g.,enhasnav.dashboard.titlebutdehasnav.dashboard.header) would cause silent fallback-to-English at runtime. Extending the test to compare all leaf key paths would catch translation gaps more comprehensively.🔍 Example nested key parity check
describe("viewer i18n: structural parity en ↔ de", () => { it("de.json has every top-level key that en.json has", () => { const en = loadLocale("en"); const de = loadLocale("de"); for (const key of Object.keys(en)) { expect(de[key], `missing top-level key '${key}' in de.json`).toBeDefined(); } }); + + it("de.json has every nested key path that en.json has", () => { + function getLeafPaths(obj: unknown, prefix = ""): string[] { + if (typeof obj !== "object" || obj === null) return []; + const paths: string[] = []; + for (const [key, val] of Object.entries(obj)) { + const path = prefix ? `${prefix}.${key}` : key; + if (typeof val === "string") { + paths.push(path); + } else if (typeof val === "object" && val !== null) { + paths.push(...getLeafPaths(val, path)); + } + } + return paths; + } + const en = loadLocale("en"); + const de = loadLocale("de"); + const enPaths = getLeafPaths(en); + const dePaths = new Set(getLeafPaths(de)); + for (const path of enPaths) { + expect(dePaths.has(path), `missing key path '${path}' in de.json`).toBe(true); + } + }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/viewer-i18n.test.ts` around lines 110 - 116, The test only compares top-level keys; extend it to collect and compare all leaf key paths so nested mismatches are caught. Add a helper (e.g., getLeafPaths(obj): string[]) that recursively builds dot-delimited paths for every leaf in the loaded locale objects, then in the test call loadLocale("en") and loadLocale("de"), compute getLeafPaths(en) and getLeafPaths(de), and assert for each path from en that de[path] is defined (use the same expect message). Update the existing test block that declares en and de and the for-loop to use these leaf paths instead of Object.keys.src/viewer/index.html (1)
1025-1033: 💤 Low valueDocument safety constraints for
data-i18n-attrto prevent future misuse.The
data-i18n-attrmechanism sets arbitrary attributes from translated strings without validation. While not currently exploited, using it forhref,src, or event-handler attributes would create XSS vectors.Safe:
<input data-i18n-attr="placeholder:actions.search_placeholder">Unsafe (do not use):
<a data-i18n-attr="href:footer.docs_link"> <!-- href could become javascript: --> <img data-i18n-attr="src:branding.logo_url"> <!-- src could load attacker-controlled image -->Add an inline comment documenting the constraint, or implement an attribute allowlist:
root.querySelectorAll("[data-i18n-attr]").forEach(function (el) { + // SAFETY: only use for non-executable attributes (placeholder, title, aria-label). + // Never use for href, src, or on* event handlers. el.getAttribute("data-i18n-attr").split(",").forEach(function (pair) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/viewer/index.html` around lines 1025 - 1033, The code currently sets arbitrary attributes from data-i18n-attr via root.querySelectorAll(...) and el.setAttribute(attr, window.t(key)), which can create XSS if attrs like href, src or event handlers are used; add either an inline comment above this block documenting that data-i18n-attr must only be used for safe attributes (e.g., placeholder, title, alt, aria-*), or implement an explicit allowlist check inside the loop that only permits known-safe attribute names (e.g., "placeholder","title","alt","aria-label","aria-labelledby") and skips/logs any others before calling el.setAttribute; ensure you reference the same symbols (data-i18n-attr, window.t, el.setAttribute, root.querySelectorAll) when changing logic or adding the comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.env.example:
- Line 107: Update the VIEWER_LANGUAGE comment to avoid suggesting editing a
src/ tree for packaged/npm installs: locate the commented env variable
"VIEWER_LANGUAGE" and change the sentence that currently reads “Drop
src/viewer/locales/<lang>.json to add more” to either instruct contributors to
"contribute a locale via PR" or explicitly state that the src/... path applies
only to source checkouts (and packaged installs must submit PRs or use other
extension mechanisms); keep the note about built-in fallbacks (en, de) and
fallback behavior intact.
In `@README.md`:
- Around line 919-929: Update the "Viewer language" section so it doesn’t imply
users can add locale files in packaged installs; change the sentence that
references src/viewer/locales/en.json to clearly state this is for source
checkouts or for contributing via a PR (e.g., “for source checkouts or to
contribute a translation via PR, copy src/viewer/locales/en.json, translate, and
open a PR”), and mirror the same clarification at the other occurrence in the
README; edit the "Viewer language" heading content and the duplicate line near
the later section to explicitly call out “source checkouts / contribution”
instead of suggesting runtime modification in packaged installs.
In `@src/viewer/index.html`:
- Around line 1013-1019: window.t currently returns raw translation strings from
getPath(bundle.messages, key) and getPath(bundle.fallback, key) which enables
XSS; change it to HTML-escape the translation text before interpolation (i.e.,
call esc(...) on the resolved v and fb) and then pass the escaped string into
interpolate so variables can still be inserted safely; if esc() is defined
later, hoist it above window.t or inline equivalent escaping logic to ensure
translations are sanitized before being returned by window.t.
In `@src/viewer/locales.ts`:
- Around line 27-43: The loadLocale(lang: string) function accepts an
unvalidated lang and builds a file path (via localesDir() and join) which can
enable path traversal; update loadLocale to validate and normalize lang before
using it: reject or sanitize any input containing path separators or traversal
sequences (e.g., '/', '\\', '..') and enforce a strict language-tag pattern (for
example allow only ASCII letters, digits, hyphen and underscore, or a regex such
as simple BCP47-like subset) or check that lang exists in the localesDir listing
before reading; ensure you still use cache.set(lang, {}) for rejected/unknown
values and keep failures in the same try/catch flow, referring to loadLocale,
localesDir, and cache in your changes.
---
Nitpick comments:
In `@src/viewer/index.html`:
- Around line 1025-1033: The code currently sets arbitrary attributes from
data-i18n-attr via root.querySelectorAll(...) and el.setAttribute(attr,
window.t(key)), which can create XSS if attrs like href, src or event handlers
are used; add either an inline comment above this block documenting that
data-i18n-attr must only be used for safe attributes (e.g., placeholder, title,
alt, aria-*), or implement an explicit allowlist check inside the loop that only
permits known-safe attribute names (e.g.,
"placeholder","title","alt","aria-label","aria-labelledby") and skips/logs any
others before calling el.setAttribute; ensure you reference the same symbols
(data-i18n-attr, window.t, el.setAttribute, root.querySelectorAll) when changing
logic or adding the comment.
In `@src/viewer/locales.ts`:
- Around line 10-25: Cache the resolved locales directory at module scope
instead of computing it on every call: compute the directory once using the
existing logic in localesDir (which probes join(...) candidates and calls
readdirSync) and store the result in a module-level constant (e.g., LOCALES_DIR
or resolvedLocalesDir), then have localesDir() or loadLocale() read from that
cached constant so subsequent cache misses avoid repeated filesystem probes;
ensure the initial computation preserves the existing fallback behavior
(returning empty string if none found).
In `@test/viewer-i18n.test.ts`:
- Around line 110-116: The test only compares top-level keys; extend it to
collect and compare all leaf key paths so nested mismatches are caught. Add a
helper (e.g., getLeafPaths(obj): string[]) that recursively builds dot-delimited
paths for every leaf in the loaded locale objects, then in the test call
loadLocale("en") and loadLocale("de"), compute getLeafPaths(en) and
getLeafPaths(de), and assert for each path from en that de[path] is defined (use
the same expect message). Update the existing test block that declares en and de
and the for-loop to use these leaf paths instead of Object.keys.
🪄 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: 374bff84-1614-4628-a9d0-eae80987f0d1
📒 Files selected for processing (13)
.env.exampleCHANGELOG.mdCONTRIBUTING.mdREADME.mdpackage.jsonsrc/auth.tssrc/viewer/document.tssrc/viewer/index.htmlsrc/viewer/locales.tssrc/viewer/locales/de.jsonsrc/viewer/locales/en.jsontest/viewer-i18n.test.tstest/viewer-security.test.ts
Addresses CodeRabbit review on rohitg00#541. t() now HTML-escapes the resolved translation string before interpolation. Translations arrive via community PRs; escaping at the boundary stops a malicious translation from injecting markup at the ~200 sites that concatenate t() output into innerHTML. Interpolation vars stay raw so the hardcoded HTML fragments used in memories.title_intro (e.g. <code>memory_remember</code>) still render. A new tRaw() returns the unescaped value, for the small number of callers that assign to textContent or setAttribute on safe attributes — textContent treats entities literally, so escape would otherwise show "&" as four characters in the DOM. data-i18n-attr now enforces a SAFE_I18N_ATTRS allowlist (placeholder, title, alt, aria-label/labelledby/describedby/ roledescription). Any other attribute name is silently skipped, so a contributor cannot turn the mechanism into an href:/src:/onclick: injection vector by adding a data-i18n-attr to an element. loadLocale() validates lang against /^[a-z]{2,3}$/i before touching the filesystem. resolveViewerLanguage() already normalizes inputs down to the primary subtag, so callers in this codebase are unchanged; this is a defence-in-depth guard for the exported boundary. Tests: - new: rejects path-traversal sequences (../etc/passwd, ..\\windows, en/../en) - new: rejects non-language inputs ("", "123", "en-US", single letter, 4+ letters) - new: verifies the IIFE in index.html ships escI18n() wired into t() - new: verifies SAFE_I18N_ATTRS allowlist exists and excludes href/src/on* - new: structural parity now covers every nested leaf path, not just top level - new: placeholder-marker parity between en.json and de.json Signed-off-by: Christian Walter <chris.walter@mail.de>
Addresses CodeRabbit review on rohitg00#541. Both README.md (Viewer language section + the env table at the bottom) and .env.example referenced 'Drop src/viewer/locales/<lang>.json', which is misleading for npm/global installs that ship dist/ without a src/ tree. Rewords to explicitly call out the PR-against-repo path (source checkout required) so users do not assume a runtime drop-in mechanism exists. Signed-off-by: Christian Walter <chris.walter@mail.de>
Author
|
Thanks for the review! Pushed two new commits addressing the feedback:
All commits signed off; no force-push, history is |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/viewer/index.html (1)
944-944:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLocalize the runtime WS states too.
This initial label is translated, but
startPolling()and the reconnect path still set hardcoded English strings (polling · …andconnecting...). After the first socket transition the badge flips back to English.Suggested direction
- setWsStatus('polling · ' + (POLL_INTERVAL_MS / 1000) + 's', 'disconnected'); + setWsStatus(t('status.polling', { seconds: POLL_INTERVAL_MS / 1000 }), 'disconnected'); - setWsStatus('connecting...', 'disconnected'); + setWsStatus(t('status.connecting'), 'disconnected');You'd also need matching keys in the locale bundles.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/viewer/index.html` at line 944, The runtime WebSocket status strings are being set in code (e.g., in startPolling() and the reconnect logic) to hardcoded English, causing the badge (element id "ws-status") to revert to English after transitions; update those places to use the i18n/localization lookup (the same mechanism used for the initial data-i18n attribute) instead of literal strings, reference the "ws-status" element by id when updating textContent/class, and add matching keys for the new states (e.g., polling, connecting, reconnecting) to the locale bundles so translations are available at runtime.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/viewer/index.html`:
- Around line 1048-1050: SAFE_I18N_ATTRS currently includes IDREF ARIA
attributes that must reference element IDs; remove "aria-labelledby" and
"aria-describedby" from the SAFE_I18N_ATTRS Set so i18n text is not inserted
into IDREF attributes (keep "aria-label" and "aria-roledescription"); update
both occurrences (the Set definitions around SAFE_I18N_ATTRS) and ensure any
logic that reads data-i18n-attr still allows the other attributes unchanged.
- Line 1115: The sandboxed VM requires plain identifier aliases for global
functions; add an alias for tRaw the same way t is aliased so bare tRaw() calls
don't throw ReferenceError. Update the initialization that creates var t =
window.t; to also declare var tRaw = window.tRaw; (or replace bare tRaw() usages
with window.tRaw() consistently) so references in the code (e.g., the button
text update and the other occurrences that call tRaw) resolve correctly in the
VM.
In `@src/viewer/locales.ts`:
- Around line 35-40: In loadLocale, normalize the incoming lang (e.g., lang =
lang.trim().toLowerCase()) before running VALID_LANG, using it as the cache key
and when constructing the locale filename so lookups use a canonical form;
update all references to the original lang in this function
(VALID_LANG.test(lang), cache.get/set(lang), and the file lookup logic) to use
the normalized variable so loadLocale("EN") will match en.json and the cache
consistently.
In `@test/viewer-i18n.test.ts`:
- Around line 193-195: The test currently skips comparisons when deVal is falsy,
so empty-string translations are ignored; change the check that skips missing
German values from a truthiness test to an explicit undefined/null check (e.g.,
only continue when deVal is undefined or null) so that empty strings are tested
for placeholder parity with enMarkers; update the conditional around deVal (used
with path and enMarkers in viewer-i18n.test.ts) to only skip genuinely missing
entries rather than empty-string values.
---
Outside diff comments:
In `@src/viewer/index.html`:
- Line 944: The runtime WebSocket status strings are being set in code (e.g., in
startPolling() and the reconnect logic) to hardcoded English, causing the badge
(element id "ws-status") to revert to English after transitions; update those
places to use the i18n/localization lookup (the same mechanism used for the
initial data-i18n attribute) instead of literal strings, reference the
"ws-status" element by id when updating textContent/class, and add matching keys
for the new states (e.g., polling, connecting, reconnecting) to the locale
bundles so translations are available at runtime.
🪄 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: 6117a243-ee98-4ed2-804b-3b4697ced024
📒 Files selected for processing (5)
.env.exampleREADME.mdsrc/viewer/index.htmlsrc/viewer/locales.tstest/viewer-i18n.test.ts
✅ Files skipped from review due to trivial changes (2)
- .env.example
- README.md
Four follow-up findings from the review on rohitg00#541. 1. SAFE_I18N_ATTRS no longer includes IDREF ARIA attributes. aria-labelledby and aria-describedby must reference element IDs, not free text — the first caller using data-i18n-attr on either would have silently broken the accessible name/description wiring. Removed both; kept aria-label and aria-roledescription. 2. Aliased tRaw beside t for the VM sandbox. The viewer unit tests run render functions inside a VM where window !== globalThis. There was already a 'var t = window.t;' module-scope alias for the same reason; the new tRaw() calls added in the previous commit would have thrown ReferenceError in that environment. Added 'var tRaw = window.tRaw;'. 3. loadLocale() now normalizes lang before validate/cache/file. VALID_LANG was case-insensitive, so loadLocale("EN") passed validation but then tried to read EN.json (which does not exist) and cached the failure under the uppercase key. Now lowercases and trims before everything, so loadLocale("EN"), " en ", and "En" all resolve to the same en.json bundle and the same cache slot. VALID_LANG simplified to /^[a-z]{2,3}$/ (no /i). 4. Placeholder-parity test no longer hides empty-string regressions. Previously 'if (!deVal) continue;' skipped both missing keys and intentional empty-string translations. Switched to a typeof check so empty strings are still validated for placeholder parity. Signed-off-by: Christian Walter <chris.walter@mail.de>
Author
|
Second-pass feedback addressed in
Tests: 1060 / 1060 (+1 new test for Branch: |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/viewer/locales.ts`:
- Around line 37-57: buildLocaleBundle currently returns and compares the raw
lang string while loadLocale canonicalizes it; update buildLocaleBundle to
normalize the incoming lang the same way (trim and toLowerCase and validate with
VALID_LANG) before any comparisons or cache lookups so that messages, fallback,
and the returned lang use the canonical form; ensure when invalid or missing
locale you still set/return the canonical empty cache entry and return lang as
the normalized value (refer to buildLocaleBundle, loadLocale, VALID_LANG,
messages, fallback, and lang).
🪄 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: 11615e5a-ad07-47e5-b161-cbdd3a484fc9
📒 Files selected for processing (3)
src/viewer/index.htmlsrc/viewer/locales.tstest/viewer-i18n.test.ts
Comment on lines
+37
to
+57
| // Normalize before validate/cache/file so loadLocale("EN") and loadLocale(" en ") | ||
| // both resolve to en.json with a single cache entry. | ||
| const normalized = lang.trim().toLowerCase(); | ||
| if (!VALID_LANG.test(normalized)) { | ||
| cache.set(normalized, {}); | ||
| return {}; | ||
| } | ||
| if (cache.has(normalized)) return cache.get(normalized)!; | ||
| const dir = localesDir(); | ||
| if (!dir) { | ||
| cache.set(normalized, {}); | ||
| return {}; | ||
| } | ||
| try { | ||
| const text = readFileSync(join(dir, `${normalized}.json`), "utf-8"); | ||
| const data = JSON.parse(text) as Locale; | ||
| cache.set(normalized, data); | ||
| return data; | ||
| } catch { | ||
| cache.set(normalized, {}); | ||
| return {}; |
There was a problem hiding this comment.
Normalize buildLocaleBundle() on the same canonical form.
loadLocale() now canonicalizes casing/whitespace, but buildLocaleBundle() still compares and returns the raw lang. buildLocaleBundle("EN") produces messages === en, fallback === en, and lang === "EN", so callers that bypass resolveViewerLanguage() ship a duplicated English bundle and set a non-canonical document language.
Suggested fix
export function buildLocaleBundle(lang: string): LocaleBundle {
- const messages = loadLocale(lang);
- const fallback = lang === FALLBACK_LANG ? {} : loadLocale(FALLBACK_LANG);
- return { lang, messages, fallback };
+ const normalized = lang.trim().toLowerCase();
+ const messages = loadLocale(normalized);
+ const fallback = normalized === FALLBACK_LANG ? {} : loadLocale(FALLBACK_LANG);
+ return { lang: normalized, messages, fallback };
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/viewer/locales.ts` around lines 37 - 57, buildLocaleBundle currently
returns and compares the raw lang string while loadLocale canonicalizes it;
update buildLocaleBundle to normalize the incoming lang the same way (trim and
toLowerCase and validate with VALID_LANG) before any comparisons or cache
lookups so that messages, fallback, and the returned lang use the canonical
form; ensure when invalid or missing locale you still set/return the canonical
empty cache entry and return lang as the normalized value (refer to
buildLocaleBundle, loadLocale, VALID_LANG, messages, fallback, and lang).
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
Adds an
.env-driven viewer UI translation mechanism with English and German shipped. Designed so additional languages arrive as drop-in JSON files via community PRs — no code changes required.Closes #483.
How it works
src/viewer/locales/{en,de}.jsonhold the UI strings (243 keys each, structurally identical).src/viewer/locales.tsresolvesVIEWER_LANGUAGE(defaultsen, normalizesde-DE/de_DE/DE/ whitespace → canonical form).src/viewer/document.tsinjects the locale bundle into the rendered viewer HTML via the same nonced-script-placeholder pattern already used for__AGENTMEMORY_VIEWER_NONCE__and__AGENTMEMORY_VERSION__. No new<script>tag, no external fetch, the existing CSP nonce model is preserved.src/viewer/index.htmlexposest(key, vars)with EN-per-key fallback and{placeholder}interpolation, plus aDOMContentLoadedpass that replaces[data-i18n]text content and[data-i18n-attr="attr:key"]attributes.t()instead of embedded English literals.What translates
UI chrome (tabs, dashboard cards, table headers, empty states, buttons, search placeholders, filter labels, modal text) plus display-only mapping for backend enums (
type, graph node types, status labels). API values, search, filters, and persistence keep operating on the raw enum strings — CSS class names like.type-architecturestay raw.Adding a new language
src/viewer/locales/en.jsonto<lang>.json.{placeholder}markers identical.Documented in
CONTRIBUTING.md.Test plan
npm test— 1053 tests pass (1034 baseline + 19 new i18n tests covering env normalization, locale loading, bundle building, document injection, CSP-safe<escape,t()semantics, and structural parity)VIEWER_LANGUAGE=en agentmemory && open localhost:3113— viewer is visually identical tomainVIEWER_LANGUAGE=de agentmemory && open localhost:3113— UI renders in German<script>(new test inviewer-security.test.ts)Commits
Ten focused commits:
feat(viewer): extract UI strings into en.json locale sourcefeat(viewer): introduce locale loader and VIEWER_LANGUAGE resolverfeat(viewer): inject locale bundle into rendered documentfeat(viewer): add t() helper and data-i18n DOM passfeat(viewer): tag static markup with data-i18n attributesfeat(viewer): route dynamic-render strings through t()feat(viewer): add German (de) translationbuild(viewer): ship locale files in the published packagedocs: document VIEWER_LANGUAGE and translation contributionsfeat(viewer): cover remaining hardcoded English stringsOut of scope
UI language picker (would conflict with
.env-driven config pattern), per-userlocalStorage, translation of user-generated memory content, plural rules (CLDR), RTL languages, date/number localization (existingtoLocaleString()calls already respect browser locale).Summary by CodeRabbit
New Features
Documentation
Tests