feat(i18n): translate dynamic content by active language#2260
feat(i18n): translate dynamic content by active language#2260lspassos1 wants to merge 2 commits intokoala73:mainfrom
Conversation
|
@lspassos1 is attempting to deploy a commit to the Elie Team on Vercel. A member of the Team first needs to authorize it. |
|
@koala73 One architecture note to make explicit: this PR closes #644 using the translation path already present in the app, so it does not introduce a new translation vendor or browser-only runtime dependency. If we later want broader or cheaper translation coverage, that should be a separate product/infra decision because it changes cost, latency, cache strategy, and offline/runtime support. |
Greptile SummaryThis PR introduces end-to-end language-awareness for the three main dynamic-content surfaces — News panels, Country Brief, and Daily Market Brief — by adding a shared Key changes:
Notable issues found:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as UI / Panel
participant CT as content-translation.ts
participant Mem as In-Memory Cache
participant LS as localStorage Cache
participant Trans as summarization.translateText
UI->>CT: translateContentText(text, targetLang)
CT->>CT: shouldTranslateContent(targetLang, sourceLang)?
alt targetLang is 'en' or same as sourceLang
CT-->>UI: return original text
else needs translation
CT->>Mem: getCachedContentTranslation(text, lang)
alt memory hit
Mem-->>CT: cached translation
CT-->>UI: cached translation
else memory miss
CT->>LS: readStoredTranslation(text, lang)
alt localStorage hit
LS-->>CT: stored translation
CT->>Mem: warm memory cache
CT-->>UI: cached translation
else cache miss
CT->>CT: check inFlight Map (dedup)
alt request already in-flight
CT-->>UI: await same Promise
else new request
CT->>Trans: translateText(text, lang)
Trans-->>CT: translated string
CT->>Mem: memoryCache.set(key, result)
CT->>LS: persistTranslation(...)
CT-->>UI: translated string
end
end
end
end
|
src/app/data-loader.ts
Outdated
| console.warn('[DailyBrief] Failed to build daily market brief:', error); | ||
| const timezone = Intl.DateTimeFormat().resolvedOptions().timeZone || 'UTC'; | ||
| const cached = await getCachedDailyMarketBrief(timezone).catch(() => null); | ||
| const cached = await getCachedDailyMarketBrief(timezone, getCurrentLanguage()).catch(() => null); |
There was a problem hiding this comment.
lang re-read in catch block may differ from the value used in the try block
lang is declared with const inside the try block (line 1393), so it is not in scope here. The catch handler calls getCurrentLanguage() a second time. If the user switches languages during the await buildDailyMarketBrief(…) call (a real possibility given the async summarization step), the error-path cache lookup will use a different language than the generation that just failed, potentially surfacing the wrong language's cached brief in the error fallback path.
Capture lang before the try block so both paths share the same snapshot:
| const cached = await getCachedDailyMarketBrief(timezone, getCurrentLanguage()).catch(() => null); | |
| const cached = await getCachedDailyMarketBrief(timezone, lang).catch(() => null); |
(This requires hoisting const lang = getCurrentLanguage(); to before the this.ctx.inFlight.add('dailyMarketBrief'); line so it's accessible in the catch.)
| function persistTranslation(text: string, targetLang: string, translated: string): void { | ||
| try { | ||
| const normalizedText = normalizeText(text); | ||
| localStorage.setItem( | ||
| storageKey(normalizedText, targetLang), | ||
| JSON.stringify({ source: normalizedText, translated } satisfies CachedTranslationEntry), | ||
| ); | ||
| } catch { | ||
| // Ignore storage failures and keep the in-memory cache hot. | ||
| } | ||
| } |
There was a problem hiding this comment.
localStorage translation cache has no TTL or size limit
Translations are written to localStorage indefinitely. On a dashboard that aggregates 30+ live news feeds and refreshes frequently, the number of distinct translated headlines will grow quickly. Each entry stores both the source text and the translated text, which for lengthy news headlines could accumulate to tens of MB over weeks of use — especially in multilingual newsrooms.
Since the cache has no eviction, the catch { } on setItem is the only safety net; once localStorage is full, all subsequent translation calls fall back to the network every time (in-memory cache aside, which is reset on page reload).
Consider adding a simple epoch/version key and a max-item count guard, or at minimum documenting the unbounded growth characteristic so it can be addressed in a follow-up.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a47d0b6c43
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| setTimeout(() => { | ||
| void this.translateBriefCopy(targetLang, requestId); |
There was a problem hiding this comment.
Defer brief-label translation until content render completes
renderBrief() schedules translateBriefCopy() with setTimeout(..., 0), but Panel.setContent() applies HTML with a 150ms debounce, so the translation pass runs against stale/empty DOM and usually finds no [data-brief-copy] nodes. In a non-English UI with uncached labels, "Action Plan"/"Risk Watch"/"Linked headline" remain English indefinitely because no second translation pass is triggered after the debounced render finishes.
Useful? React with 👍 / 👎.
src/app/data-loader.ts
Outdated
| console.warn('[DailyBrief] Failed to build daily market brief:', error); | ||
| const timezone = Intl.DateTimeFormat().resolvedOptions().timeZone || 'UTC'; | ||
| const cached = await getCachedDailyMarketBrief(timezone).catch(() => null); | ||
| const cached = await getCachedDailyMarketBrief(timezone, getCurrentLanguage()).catch(() => null); |
There was a problem hiding this comment.
Reuse the original language in daily-brief error fallback
The catch path re-reads getCurrentLanguage() before loading cached data. If the user switches language while a brief build is in flight and that build fails, recovery looks up a different language key than the one used earlier in the same request, misses an existing cache entry, and then overwrites the panel with an error despite valid cached content being available. Reusing the initially captured language avoids this race.
Useful? React with 👍 / 👎.
Summary
Closes #644 by translating dynamic content into the active UI language across the main news and briefing surfaces, while reusing the translation path already present in the app.
Root cause
The interface could switch languages, but dynamic content still remained in its source language or was forced to English in some paths. The biggest gaps were news headlines, country brief fallback copy, and the Daily Market Brief generation/cache path.
Changes
Validation
npx tsx --test tests/content-translation.test.mts tests/daily-market-brief.test.mtsnpm run typecheckRisk
Low to medium. The behavior change is intentionally broad across content surfaces, but it stays within the existing translation architecture and falls back to original text when translation is unavailable.
Architecture note
@koala73 This patch intentionally stays on the existing translation path already used in the app. It does not introduce a new translation vendor, browser-only API, or offline translation runtime.
If we want broader translation coverage later, the follow-up decision is whether to keep the current path or move to a dedicated/offline provider. That choice affects cost, latency, cache strategy, and offline/runtime support, but it is not changed in this PR.