feat: replace static locale bundles with dynamic worker translations#1884
feat: replace static locale bundles with dynamic worker translations#1884
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a page-translation system: client-side DOM translation composable invoked on app mount, a Supabase worker translation API using Workers AI with caching and rate-limits, a new Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser/Client
participant App as Vue App
participant Composable as usePageTranslation
participant Cache as Cache Storage
participant API as /translation Endpoint
participant AI as Workers AI Model
Browser->>App: Load / Navigate
App->>Composable: init usePageTranslation()
Composable->>Composable: Scan DOM → extract & normalize strings
Composable->>Composable: Deduplicate, compute requestHash
Composable->>Cache: GET translations by requestHash
alt Cache Hit
Cache-->>Composable: cached translations
else Cache Miss
Composable->>API: POST /translation/page {pagePath, targetLanguage, strings}
API->>API: Protect tokens, segment & batch
API->>AI: POST translate (model, batch)
AI-->>API: Return translated_text
API->>API: Restore tokens, map segments → translations
API->>Cache: Store translations with TTL
API-->>Composable: Return translations + requestHash
end
Composable->>Composable: Apply translations to DOM nodes/attributes
rect rgba(200,150,255,0.5)
Note over Composable,Browser: MutationObserver watches DOM and retriggers translation
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f3528351e6
ℹ️ 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".
| segment.element.setAttribute(segment.attr, translated) | ||
| } | ||
|
|
||
| const attrRecord = getAttributeRecord(segment.element, segment.attr, lang) |
There was a problem hiding this comment.
Calling getAttributeRecord() after setAttribute() makes the helper treat the translated attribute as a manual change and overwrite record.source with the translated text. That breaks language switching for attribute-based strings (placeholder, title, aria-label, and input value): when users switch back to English (or to another locale), restoreSourceContent() no longer has the original source value to restore, so attributes remain stuck in the previous translation.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
tests/dynamic-translation.unit.test.ts (1)
54-67: Prefer explicit typing overas anytype assertion.The
as anycast on line 67 bypasses TypeScript's validation of theProtectedEntryinterface structure. Since the test entries already match the expected interface (per context snippet 4), you can type them properly.♻️ Proposed fix
+import type { ProtectedEntry } from '../supabase/functions/_backend/public/translation.ts' + const entries = [ { marker: '[[CAPGO_SEGMENT_0000]]', protectedText: first.protectedText, source: 'Open settings for {app}', tokens: first.tokens, }, { marker: '[[CAPGO_SEGMENT_0001]]', protectedText: second.protectedText, source: 'Bundle uploads', tokens: second.tokens, }, - ] as any + ] satisfies ProtectedEntry[]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/dynamic-translation.unit.test.ts` around lines 54 - 67, The test uses a loose `as any` on the entries array which hides type errors; replace it with an explicit type annotation (e.g., declare `entries` as `ProtectedEntry[]`) and remove the `as any` cast so TypeScript validates the shape matches the `ProtectedEntry` interface; ensure you reference the same `first` and `second` values and that `tokens`/`protectedText` fields conform to `ProtectedEntry` (import or reference `ProtectedEntry` if needed).src/pages/accountDisabled.vue (1)
113-122: Inconsistent translation approach within the same component.This paragraph now uses hardcoded English text while the rest of the component still uses
t()translation keys (e.g., lines 111, 125, 128, 140). This creates a mixed approach where some strings go through the i18n shim and others rely solely on dynamic page translation.Consider either:
- Converting this text to use a translation key for consistency
- Documenting that hardcoded English is intentional for dynamic translation
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/accountDisabled.vue` around lines 113 - 122, The paragraph contains hardcoded English while the rest of accountDisabled.vue uses the i18n helper t(); replace the hardcoded strings with translation keys and call t() (for example use t('accountDisabled.contactSupport') for the whole sentence and t('accountDisabled.capgoSupport') for the link text) so the component is consistent, or alternatively document intentional English usage — locate the paragraph containing the anchor to https://support.capgo.app/ and update it to use t() around both the surrounding sentence and the anchor text (keeping the href and target attributes unchanged).src/services/i18n.ts (1)
8-21: Hard page reload on language change affects UX.The
window.location.reload()call triggers a full page reload after changing the language. This is a deliberate design choice for the dynamic translation system, but it's a notable UX change from an in-place language switch.A few observations:
- The return value
nextLanguagewill never be used in browser environments sincereload()interrupts execution- Any unsaved form state will be lost on language change
If this is the intended behavior for the dynamic translation architecture, consider adding a brief comment explaining the design rationale.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/i18n.ts` around lines 8 - 21, The changeLanguage function uses a hard page reload (window.location.reload()) which interrupts execution and will discard unsaved state and make the returned nextLanguage unused in the browser; if this is intentional, add a brief comment above the reload explaining the design rationale (e.g., dynamic translation requires a full reload to rehydrate bundles/replace i18n context, and the trade-off of lost in-memory form state), mention that getSelectedLanguage/normalizeLanguage/loadLanguageAsync are still used before reload, and optionally document that callers should persist any unsaved data before invoking changeLanguage.src/modules/i18n.ts (2)
72-85: String params bypass interpolation - document this behavior.When
paramsis a string (line 73-74),interpolateMessagereturns it directly, bypassing the message catalog entirely. This appears to be intentional for compatibility, but could be surprising. Consider adding a brief comment:function interpolateMessage(message: string, params?: MessageParams): string { + // String params override the message entirely (legacy compatibility) if (typeof params === 'string') return params🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/i18n.ts` around lines 72 - 85, The function interpolateMessage currently treats a string params specially (if typeof params === 'string') and returns params directly, bypassing the original message and any placeholder interpolation; add a short comment above the typeof params === 'string' check explaining this intentional compatibility behavior (that passing a string opts out of interpolation and returns the string as-is), and ensure the comment references interpolateMessage and the params parameter so future readers understand why the code returns params instead of processing message placeholders.
144-151: Fallback key transformation may produce unexpected results.
getSourceMessagetransforms missing keys by replacing hyphens with spaces (line 150):key.replaceAll('-', ' '). For a key likeerror-not-found, this produces"error not found"which may be acceptable, but for keys likeapi-key-invalid, it produces"api key invalid"which lacks proper capitalization.Consider whether this fallback behavior is intentional or if returning the raw key would be more appropriate for debugging:
- return key.replaceAll('-', ' ') + return key // Return raw key to make missing translations obvious🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/i18n.ts` around lines 144 - 151, The fallback in getSourceMessage currently transforms missing keys with key.replaceAll('-', ' '), which can produce misleading, uncapitalized strings; change the fallback to return the raw key instead. Update the getSourceMessage function (which reads from messageCatalog) so that if no resolved value and no defaultMessage exist it returns key unchanged (remove the replaceAll transformation) to preserve the original identifier for clearer debugging and consistency.supabase/functions/_backend/public/translation.ts (2)
208-223: Sequential batch processing is acceptable but consider parallel execution for large payloads.The current sequential
for...ofloop processes batches one at a time. For pages with many strings hitting multiple batches, this adds latency. If Workers AI supports concurrent requests, consider parallel execution withPromise.all:- for (const batch of batches) { - const segmentedText = buildSegmentedText(batch) - const result = await ai.run(TRANSLATION_MODEL, { - text: segmentedText, - source_lang: 'en', - target_lang: targetLanguage, - }) as { translated_text?: string } - - const translatedText = typeof result?.translated_text === 'string' - ? result.translated_text - : '' - - parseSegmentedTranslation(translatedText, batch).forEach((value, key) => { - translations.set(key, value) - }) - } + const batchResults = await Promise.all(batches.map(async (batch) => { + const segmentedText = buildSegmentedText(batch) + const result = await ai.run(TRANSLATION_MODEL, { + text: segmentedText, + source_lang: 'en', + target_lang: targetLanguage, + }) as { translated_text?: string } + + const translatedText = typeof result?.translated_text === 'string' + ? result.translated_text + : '' + + return parseSegmentedTranslation(translatedText, batch) + })) + + batchResults.forEach(batchTranslations => { + batchTranslations.forEach((value, key) => { + translations.set(key, value) + }) + })However, sequential may be intentional to respect rate limits. If so, a brief comment would help future maintainers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_backend/public/translation.ts` around lines 208 - 223, The current loop over batches performs sequential AI calls which increases latency; either (A) parallelize the per-batch translation calls by mapping batches to promises that call buildSegmentedText(...) and ai.run(TRANSLATION_MODEL, {...}) then await Promise.all and merge results into translations using parseSegmentedTranslation(...), or (B) if sequential behavior is intentional to respect rate limits, add a short clarifying comment above the for...of loop referencing batches, buildSegmentedText, ai.run, TRANSLATION_MODEL, parseSegmentedTranslation, and translations to explain why requests must be serialized; apply one of these two changes so maintainers know whether concurrency is safe.
171-193: Segment marker parsing may silently fail on reordered or missing markers.If the AI model reorders segments or omits markers in the translated output,
parseSegmentedTranslationfalls back to the source text (line 180), which is a safe default. However, whenend < 0(next marker not found), the logic at line 185 slices totranslatedText.length, which could capture content from subsequent segments that were also missing their markers.Consider adding logging when markers are missing to help diagnose translation quality issues:
if (start < 0) { + // Marker not found in translated output - model may have dropped it translations.set(entry.source, entry.source) return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_backend/public/translation.ts` around lines 171 - 193, parseSegmentedTranslation silently grabs too much text when a next marker is missing; update parseSegmentedTranslation to detect and handle missing or reordered markers: instead of unconditionally slicing to translatedText.length when end < 0, search for the earliest occurrence of any other entry.marker after start to compute a safer end boundary, and if no marker is found log a warning (including entry.marker and index) and treat the segment as missing (use entry.source or skip restoring) rather than risking swallowing subsequent segments; add a warning log near the start<0 and end<0 branches and keep restoreTranslationTokens/translations.set calls unchanged otherwise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@formkit.config.ts`:
- Around line 17-18: Replace the hardcoded FormKit locale with the app's
reactive language value: import selectedLanguage from your i18n module (the
exported ref used by changeLanguage) in formkit.config.ts and replace locale:
'en' with locale: selectedLanguage.value so FormKit initializes with the current
selected language; if you need live updates without reload, also watch
selectedLanguage and call FormKit's locale setter (or reinitialize FormKit) when
selectedLanguage changes.
In `@src/composables/usePageTranslation.ts`:
- Line 269: The module-level flag translationDisabled is set to true on certain
errors (e.g., in usePageTranslation) with no recovery; update the logic around
translationDisabled to allow recovery by resetting it on route/navigation
changes or by implementing a retry/backoff before marking it permanent: identify
where translationDisabled is toggled (the translationDisabled variable and the
error-handling branches around the response status checks and TypeError handlers
referenced in the diff) and modify them to (1) perform a limited number of
retries with exponential backoff when fetch/translate fails, and only set
translationDisabled to true after retries are exhausted, or (2) clear/reset
translationDisabled when the route changes (hook into the router/navigation
event in usePageTranslation) so transient errors don’t permanently disable
translations.
---
Nitpick comments:
In `@src/modules/i18n.ts`:
- Around line 72-85: The function interpolateMessage currently treats a string
params specially (if typeof params === 'string') and returns params directly,
bypassing the original message and any placeholder interpolation; add a short
comment above the typeof params === 'string' check explaining this intentional
compatibility behavior (that passing a string opts out of interpolation and
returns the string as-is), and ensure the comment references interpolateMessage
and the params parameter so future readers understand why the code returns
params instead of processing message placeholders.
- Around line 144-151: The fallback in getSourceMessage currently transforms
missing keys with key.replaceAll('-', ' '), which can produce misleading,
uncapitalized strings; change the fallback to return the raw key instead. Update
the getSourceMessage function (which reads from messageCatalog) so that if no
resolved value and no defaultMessage exist it returns key unchanged (remove the
replaceAll transformation) to preserve the original identifier for clearer
debugging and consistency.
In `@src/pages/accountDisabled.vue`:
- Around line 113-122: The paragraph contains hardcoded English while the rest
of accountDisabled.vue uses the i18n helper t(); replace the hardcoded strings
with translation keys and call t() (for example use
t('accountDisabled.contactSupport') for the whole sentence and
t('accountDisabled.capgoSupport') for the link text) so the component is
consistent, or alternatively document intentional English usage — locate the
paragraph containing the anchor to https://support.capgo.app/ and update it to
use t() around both the surrounding sentence and the anchor text (keeping the
href and target attributes unchanged).
In `@src/services/i18n.ts`:
- Around line 8-21: The changeLanguage function uses a hard page reload
(window.location.reload()) which interrupts execution and will discard unsaved
state and make the returned nextLanguage unused in the browser; if this is
intentional, add a brief comment above the reload explaining the design
rationale (e.g., dynamic translation requires a full reload to rehydrate
bundles/replace i18n context, and the trade-off of lost in-memory form state),
mention that getSelectedLanguage/normalizeLanguage/loadLanguageAsync are still
used before reload, and optionally document that callers should persist any
unsaved data before invoking changeLanguage.
In `@supabase/functions/_backend/public/translation.ts`:
- Around line 208-223: The current loop over batches performs sequential AI
calls which increases latency; either (A) parallelize the per-batch translation
calls by mapping batches to promises that call buildSegmentedText(...) and
ai.run(TRANSLATION_MODEL, {...}) then await Promise.all and merge results into
translations using parseSegmentedTranslation(...), or (B) if sequential behavior
is intentional to respect rate limits, add a short clarifying comment above the
for...of loop referencing batches, buildSegmentedText, ai.run,
TRANSLATION_MODEL, parseSegmentedTranslation, and translations to explain why
requests must be serialized; apply one of these two changes so maintainers know
whether concurrency is safe.
- Around line 171-193: parseSegmentedTranslation silently grabs too much text
when a next marker is missing; update parseSegmentedTranslation to detect and
handle missing or reordered markers: instead of unconditionally slicing to
translatedText.length when end < 0, search for the earliest occurrence of any
other entry.marker after start to compute a safer end boundary, and if no marker
is found log a warning (including entry.marker and index) and treat the segment
as missing (use entry.source or skip restoring) rather than risking swallowing
subsequent segments; add a warning log near the start<0 and end<0 branches and
keep restoreTranslationTokens/translations.set calls unchanged otherwise.
In `@tests/dynamic-translation.unit.test.ts`:
- Around line 54-67: The test uses a loose `as any` on the entries array which
hides type errors; replace it with an explicit type annotation (e.g., declare
`entries` as `ProtectedEntry[]`) and remove the `as any` cast so TypeScript
validates the shape matches the `ProtectedEntry` interface; ensure you reference
the same `first` and `second` values and that `tokens`/`protectedText` fields
conform to `ProtectedEntry` (import or reference `ProtectedEntry` if needed).
🪄 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: bd4e621f-30d8-4eb5-8652-09e529ba8def
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (37)
cloudflare_workers/api/index.tscloudflare_workers/api/wrangler.jsoncformkit.config.tsmessages/README.mdmessages/de.jsonmessages/es.jsonmessages/fr.jsonmessages/hi.jsonmessages/id.jsonmessages/it.jsonmessages/ja.jsonmessages/ko.jsonmessages/pl.jsonmessages/pt-br.jsonmessages/ru.jsonmessages/tr.jsonmessages/vi.jsonmessages/zh-cn.jsonpackage.jsonproject.inlang/settings.jsonscripts/supabase-worktree.tssrc/App.vuesrc/auto-imports.d.tssrc/components.d.tssrc/components/LangSelector.vuesrc/composables/usePageTranslation.tssrc/modules/i18n.tssrc/pages/accountDisabled.vuesrc/services/i18n.tssrc/shims/vueI18n.tssupabase/config.tomlsupabase/functions/_backend/public/translation.tssupabase/functions/_backend/utils/cloudflare.tstemplatestests/dynamic-translation.unit.test.tstsconfig.jsonvite.config.mts
💤 Files with no reviewable changes (3)
- messages/README.md
- project.inlang/settings.json
- package.json
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bac49ed259
ℹ️ 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".
| const text = normalizeWhitespace(entry) | ||
| if (!text) |
There was a problem hiding this comment.
This normalization collapses internal whitespace before strings become translation keys, but the frontend later applies responses by exact segment.source lookup. For any source text that contains embedded newlines or repeated spaces (a common shape for formatted Vue text nodes), the response key no longer matches and that segment is silently left untranslated. Keep the original string as the response key (or carry an explicit original→normalized mapping) while using normalized text only for dedupe/batching.
Useful? React with 👍 / 👎.
| if (response.status === 404 || response.status === 501 || response.status === 503) { | ||
| translationDisabled = true | ||
| return {} |
There was a problem hiding this comment.
Treating HTTP 503 as a permanent disable condition sets translationDisabled for the lifetime of the app session, so one temporary worker outage causes all later translation attempts to be skipped until a full page reload. Because 503 is a transient service-unavailable signal, this turns a short incident into persistent translation loss for active users.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 458c8e9238
ℹ️ 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".
| } | ||
|
|
||
| async function fetchTranslations(strings: string[], lang: string, pagePath: string, signal: AbortSignal) { | ||
| const response = await fetch(`${defaultApiHost}/translation/page`, { |
There was a problem hiding this comment.
The frontend now always posts page strings to defaultApiHost + '/translation/page', but this commit only wires /translation in the Cloudflare API router; a repo-wide check of supabase/functions/*/index.ts shows no translation function entrypoint. In environments where defaultApiHost points to Supabase Functions (notably local localhost:54321/functions/v1), requests return 404, translationDisabled is set, and users are stuck with English-only UI after static locale bundles were removed.
Useful? React with 👍 / 👎.
|
|
||
| app.use('*', useCors) | ||
|
|
||
| app.post('/page', async (c) => { |
There was a problem hiding this comment.
This new endpoint executes Workers AI translation for arbitrary caller-provided text but is mounted with open CORS and no auth/rate-limit middleware, so any third party can automate calls and consume model quota/cost. Because each request can trigger multiple ai.run(...) calls, unauthenticated public access can degrade service for real users and create avoidable billing spikes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/modules/i18n.ts (1)
72-76: Don't preserve inline English fallback copy in the coret()path.Treating a string
paramsvalue ordefaultMessageas source copy keeps the forbiddent('key', 'English fallback')pattern alive and hides missingmessages/en.jsonentries instead of surfacing them during development. If you still need a migration shim, keep it in a temporary adapter rather than the default translator.As per coding guidelines, "
messages/en.json: Updatemessages/en.jsonwhen text is missing instead of using inline fallback text in translation calls" and "**/*.{ts,tsx,vue,js}: Never pass inline fallback text as the second argument to translation calls; use translation keys only."Also applies to: 146-156
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/i18n.ts` around lines 72 - 76, Remove the special-case that treats a string second argument as inline English fallback in the core translator: in interpolateMessage, delete the branch that returns params when typeof params === 'string' and instead treat string inputs as invalid/missing params so missing translations surface; similarly remove or change any analogous handling in the related translation path referenced at lines ~146-156 so defaultMessage/string fallbacks are not honored by t(). If you need a temporary shim for legacy callers, implement it outside the core functions (e.g., a migration adapter) and update callers to pass proper MessageParams or add keys to messages/en.json.
🤖 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/auto-imports.d.ts`:
- Line 48: The auto-import types were emitted with relative paths (e.g., the
declaration for isAdminRole: typeof import('./stores/organization').isAdminRole)
but our frontend standard requires src imports to use the ~/ alias; update the
auto-import generation configuration (the script or tool that produces
src/auto-imports.d.ts) so it rewrites or emits imports using the ~/ prefix
instead of ./ relative paths, regenerate the auto-imports file, and verify
declarations such as isAdminRole (and the other listed entries) now reference
typeof import('~/stores/organization').isAdminRole.
In `@src/services/i18n.ts`:
- Around line 4-5: The current change repurposed getEmoji(lang) to look up a
language->country mapping, which breaks callers that pass an already-normalized
country code; revert getEmoji to its original country-code behavior and add a
new helper for language-based lookups: implement getLanguageEmoji(lang) that
returns countryCodeToFlagEmoji(getLanguageConfig(lang).countryCode), and restore
or keep getEmoji(countryCode) to call
countryCodeToFlagEmoji(normalizeCountryCode(countryCode)) so existing callers
using normalizeCountryCode(...) keep correct flags; update exports accordingly
and replace any new calls that intended a language lookup to use
getLanguageEmoji.
In `@supabase/functions/_backend/public/translation.ts`:
- Around line 177-193: The code in parseSegmentedTranslation uses console.warn
(lines handling missing marker/boundary) which CI rejects; change these to
structured backend logging by accepting a Hono Context (or a logger/requestId)
into parseSegmentedTranslation and replace console.warn calls with cloudlog()
including c.get('requestId') and context metadata; update all callers of
parseSegmentedTranslation to pass the Context<MiddlewareKeyVariables> (or a
small logger object) so you can call cloudlog({ msg: 'Translation segment marker
missing...', requestId: c.get('requestId'), index, marker: entry.marker }) and
similarly for the boundary warning, and keep translations.set behavior the same.
- Around line 247-290: The /page route currently allows unauthenticated callers
to trigger translateStrings (and thus c.env.AI.run) on cache misses; add quota
control early in the handler (immediately after parsing body and before
computing requestHash or invoking CacheHelper/translateStrings) by validating a
signed token or enforcing a per-IP rate limit; implement or call a helper like
verifySignedToken(req) or rateLimiter.check(c.ip) and return a 401/429
quickError on failure so only authorized/limited callers can reach
translateStrings and the CacheHelper.putJson path.
In `@tests/dynamic-translation.unit.test.ts`:
- Around line 3-9: The import mixes a value import with a type, violating the
ESLint rule; remove ProtectedEntry from the combined import of
normalizeTranslationStrings, parseSegmentedTranslation,
protectTranslationTokens, restoreTranslationTokens and instead add a separate
type-only import: use "import type { ProtectedEntry } from '.../translation.ts'"
while keeping the other named imports as value imports so the tests follow the
project's consistent type-import style.
---
Nitpick comments:
In `@src/modules/i18n.ts`:
- Around line 72-76: Remove the special-case that treats a string second
argument as inline English fallback in the core translator: in
interpolateMessage, delete the branch that returns params when typeof params ===
'string' and instead treat string inputs as invalid/missing params so missing
translations surface; similarly remove or change any analogous handling in the
related translation path referenced at lines ~146-156 so defaultMessage/string
fallbacks are not honored by t(). If you need a temporary shim for legacy
callers, implement it outside the core functions (e.g., a migration adapter) and
update callers to pass proper MessageParams or add keys to messages/en.json.
🪄 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: e76a8b98-1246-4c52-9b33-dc010032984f
📒 Files selected for processing (10)
formkit.config.tsmessages/en.jsonsrc/auto-imports.d.tssrc/components.d.tssrc/composables/usePageTranslation.tssrc/modules/i18n.tssrc/pages/accountDisabled.vuesrc/services/i18n.tssupabase/functions/_backend/public/translation.tstests/dynamic-translation.unit.test.ts
💤 Files with no reviewable changes (1)
- src/components.d.ts
✅ Files skipped from review due to trivial changes (1)
- src/pages/accountDisabled.vue
🚧 Files skipped from review as they are similar to previous changes (2)
- src/composables/usePageTranslation.ts
- formkit.config.ts
| const ignorableWatch: typeof import('@vueuse/core').ignorableWatch | ||
| const inject: typeof import('vue').inject | ||
| const injectLocal: typeof import('@vueuse/core').injectLocal | ||
| const isAdminRole: typeof import('./stores/organization').isAdminRole |
There was a problem hiding this comment.
Use ~/ alias in new auto-import type paths.
Line 48, Line 55, Line 101, Line 231, Line 396, Line 403, Line 448, and Line 578 add new relative import paths (./...). Please update the auto-import generation config so these emit ~/... paths, then regenerate this file to keep frontend imports aligned with repo standards.
As per coding guidelines: Import using \~/` alias for `src/` directory in frontend code instead of relative paths`.
Also applies to: 55-55, 101-101, 231-231, 396-396, 403-403, 448-448, 578-578
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/auto-imports.d.ts` at line 48, The auto-import types were emitted with
relative paths (e.g., the declaration for isAdminRole: typeof
import('./stores/organization').isAdminRole) but our frontend standard requires
src imports to use the ~/ alias; update the auto-import generation configuration
(the script or tool that produces src/auto-imports.d.ts) so it rewrites or emits
imports using the ~/ prefix instead of ./ relative paths, regenerate the
auto-imports file, and verify declarations such as isAdminRole (and the other
listed entries) now reference typeof
import('~/stores/organization').isAdminRole.
| if (start < 0) { | ||
| console.warn('Translation segment marker missing from model output', { index, marker: entry.marker }) | ||
| translations.set(entry.source, entry.source) | ||
| return | ||
| } | ||
|
|
||
| const contentStart = start + entry.marker.length | ||
| const markerBoundaries = entries | ||
| .slice(index + 1) | ||
| .map(nextEntry => translatedText.indexOf(nextEntry.marker, contentStart)) | ||
| .filter(boundary => boundary >= 0) | ||
| .sort((left, right) => left - right) | ||
|
|
||
| const end = markerBoundaries[0] ?? translatedText.length | ||
| if (entries[index + 1] && markerBoundaries.length === 0) { | ||
| console.warn('Translation segment boundary missing from model output', { index, marker: entry.marker }) | ||
| translations.set(entry.source, entry.source) |
There was a problem hiding this comment.
Replace console.warn with backend logging.
CI is already rejecting console.* under supabase/functions, so these warnings block the merge. Bubble the anomaly back to the route or pass a logger/requestId into parseSegmentedTranslation() so it can emit through cloudlog() instead.
As per coding guidelines, "All endpoints must receive Hono Context<MiddlewareKeyVariables> object and use c.get('requestId') for structured logging with cloudlog()."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@supabase/functions/_backend/public/translation.ts` around lines 177 - 193,
The code in parseSegmentedTranslation uses console.warn (lines handling missing
marker/boundary) which CI rejects; change these to structured backend logging by
accepting a Hono Context (or a logger/requestId) into parseSegmentedTranslation
and replace console.warn calls with cloudlog() including c.get('requestId') and
context metadata; update all callers of parseSegmentedTranslation to pass the
Context<MiddlewareKeyVariables> (or a small logger object) so you can call
cloudlog({ msg: 'Translation segment marker missing...', requestId:
c.get('requestId'), index, marker: entry.marker }) and similarly for the
boundary warning, and keep translations.set behavior the same.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de647fe04d
ℹ️ 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".
| return | ||
|
|
||
| const uniqueSources = [...new Set(segments.map(segment => segment.source))] | ||
| const requestedPath = route.fullPath |
There was a problem hiding this comment.
route.fullPath includes query strings and hash fragments, and this value is forwarded as pagePath in every translation request. In this codebase several auth flows carry sensitive one-time tokens in query/hash (for example login/recovery/sso routes), so a non-English session can unintentionally transmit those secrets to /translation/page even though translation only needs a stable page identifier. Use a sanitized path (e.g., route.path) or strip query/hash before building the request.
Useful? React with 👍 / 👎.
| if (!controller.signal.aborted && error instanceof TypeError) { | ||
| translationDisabled = true | ||
| transientRetryCount = 0 |
There was a problem hiding this comment.
A TypeError from fetch is usually a transient transport failure (temporary offline, DNS hiccup, brief network outage), but this branch permanently flips translationDisabled for the current page session. After one transient failure, later mutation-driven translation attempts will keep short-circuiting until the user changes route/language or reloads, so translation does not recover automatically when connectivity returns.
Useful? React with 👍 / 👎.
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
tests/dynamic-translation.unit.test.ts (1)
3-9:⚠️ Potential issue | 🟡 MinorSplit
ProtectedEntryinto a top-level type import.This still triggers
import/consistent-type-specifier-styleand keeps the test file red in lint.♻️ Proposed fix
+import type { ProtectedEntry } from '../supabase/functions/_backend/public/translation.ts' import { normalizeTranslationStrings, parseSegmentedTranslation, - type ProtectedEntry, protectTranslationTokens, restoreTranslationTokens, } from '../supabase/functions/_backend/public/translation.ts'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/dynamic-translation.unit.test.ts` around lines 3 - 9, The import line includes ProtectedEntry alongside value imports which triggers the import/consistent-type-specifier-style lint rule; change the import from '../supabase/functions/_backend/public/translation.ts' to import the runtime symbols (normalizeTranslationStrings, parseSegmentedTranslation, protectTranslationTokens, restoreTranslationTokens) as normal imports and pull ProtectedEntry as a top-level type-only import (import type { ProtectedEntry } from ...), ensuring ProtectedEntry is imported with the "import type" specifier to satisfy the linter.supabase/functions/_backend/public/translation.ts (1)
248-290:⚠️ Potential issue | 🟠 Major
/translation/pagestill needs quota protection.This route remains public and unthrottled, so callers can generate arbitrary cache-miss bodies and force
c.env.AI.run()on demand. Please add a signed caller check or at least per-IP/API-key rate limiting before the cache/translation path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_backend/public/translation.ts` around lines 248 - 290, Add an access-control check before computing/serving cache-miss translations in the app.post('/page') handler: validate a signed caller token (e.g., verify a signature/header using an env secret) or enforce per-IP/API-key rate limiting (use CF-Connecting-IP or an API-Key header and a RateLimiter helper) before the cache lookup and before calling translateStrings / c.env.AI.run(); if validation/rate limit fails return 401/429 and short-circuit. Update the handler around the start of the route (before sha256Hex, CacheHelper.buildRequest, and translateStrings) and reuse/introduce helpers like verifySignature(...) or RateLimiter.check(...) so legitimate cached hits still work but cache-miss AI runs are protected.
🤖 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/composables/usePageTranslation.ts`:
- Around line 410-414: The retry loop for RetryableTranslationError currently
keeps scheduling retries forever once the TRANSIENT_RETRY_DELAYS_MS table is
exhausted; modify the logic in the block that checks controller.signal.aborted
and error instanceof RetryableTranslationError to cap retries by using
transientRetryCount and TRANSIENT_RETRY_DELAYS_MS.length: if transientRetryCount
is greater than or equal to TRANSIENT_RETRY_DELAYS_MS.length, do not call
scheduleTranslation again—mark the translation as failed (or emit/dispatch a
final failure state) and stop retrying; otherwise compute retryDelay as now,
increment transientRetryCount, and call scheduleTranslation as before. Ensure
you reference controller.signal.aborted, RetryableTranslationError,
TRANSIENT_RETRY_DELAYS_MS, transientRetryCount, and scheduleTranslation when
making the change.
- Around line 330-373: The translatePage function currently scrapes
document.body via collectSegments(root, lang), which risks sending sensitive
data; change the root selection to opt-in only (e.g., query for a specific
container or attribute) and abort if not found: replace using document.body with
a scoped selector such as document.querySelector('[data-capgo-translate]') (or
respect an existing configuration flag) so only marked subtrees are collected by
collectSegments; ensure the early-return logic (restoreSourceContent,
lastRequestHash, transientRetryCount) still runs if no opt-in root exists, and
update any tests or callers that assumed body-level behavior.
---
Duplicate comments:
In `@supabase/functions/_backend/public/translation.ts`:
- Around line 248-290: Add an access-control check before computing/serving
cache-miss translations in the app.post('/page') handler: validate a signed
caller token (e.g., verify a signature/header using an env secret) or enforce
per-IP/API-key rate limiting (use CF-Connecting-IP or an API-Key header and a
RateLimiter helper) before the cache lookup and before calling translateStrings
/ c.env.AI.run(); if validation/rate limit fails return 401/429 and
short-circuit. Update the handler around the start of the route (before
sha256Hex, CacheHelper.buildRequest, and translateStrings) and reuse/introduce
helpers like verifySignature(...) or RateLimiter.check(...) so legitimate cached
hits still work but cache-miss AI runs are protected.
In `@tests/dynamic-translation.unit.test.ts`:
- Around line 3-9: The import line includes ProtectedEntry alongside value
imports which triggers the import/consistent-type-specifier-style lint rule;
change the import from '../supabase/functions/_backend/public/translation.ts' to
import the runtime symbols (normalizeTranslationStrings,
parseSegmentedTranslation, protectTranslationTokens, restoreTranslationTokens)
as normal imports and pull ProtectedEntry as a top-level type-only import
(import type { ProtectedEntry } from ...), ensuring ProtectedEntry is imported
with the "import type" specifier to satisfy the linter.
🪄 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: 3492e4e9-5f3f-4bf1-ba61-249fb18ce4df
📒 Files selected for processing (3)
src/composables/usePageTranslation.tssupabase/functions/_backend/public/translation.tstests/dynamic-translation.unit.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/services/i18n.ts (1)
8-10: ReusegetEmoji()ingetLanguageEmoji()to avoid normalization drift.
getLanguageEmojicurrently bypasses the normalization path ingetEmoji. Reusing the helper keeps behavior centralized.♻️ Proposed refactor
export function getLanguageEmoji(lang: string) { - return countryCodeToFlagEmoji(getLanguageConfig(lang).countryCode) + return getEmoji(getLanguageConfig(lang).countryCode) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/i18n.ts` around lines 8 - 10, getLanguageEmoji should call the centralized normalization helper getEmoji instead of directly using countryCodeToFlagEmoji to avoid divergent normalization logic; update getLanguageEmoji(lang) to retrieve the country code via getLanguageConfig(lang).countryCode and pass that into getEmoji so the emoji generation and normalization remain centralized in getEmoji rather than duplicating behaviour in getLanguageEmoji.supabase/functions/_backend/public/translation.ts (3)
190-225: Logging inparseSegmentedTranslationlacks request context.The
cloudlog()calls at lines 197 and 211 don't includerequestId. Since this function is exported and doesn't receive the Hono Context, consider either:
- Passing an optional
requestIdparameter for correlation, or- Accepting this limitation since the marker/index info provides sufficient debugging context.
This is a minor observability improvement, not blocking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_backend/public/translation.ts` around lines 190 - 225, The cloudlog calls inside parseSegmentedTranslation are missing request correlation; update the exported function parseSegmentedTranslation to accept an optional requestId parameter (e.g., parseSegmentedTranslation(translatedText: string, entries: ProtectedEntry[], requestId?: string)) and include that requestId in the two cloudlog calls (the one for missing marker and the one for missing boundary) so logs include { message, index, marker, requestId }; ensure the function signature change is propagated to callers and keep existing behavior when requestId is undefined, and continue to use restoreTranslationTokens and entries as before.
280-305: Consider adding a timeout to Workers AI calls.The
ai.run()calls have no timeout protection. If the Workers AI service becomes slow or unresponsive, requests will hang indefinitely until the worker execution limit is reached. Consider wrapping withPromise.raceor using AbortController with a reasonable timeout (e.g., 30 seconds per batch).♻️ Proposed timeout pattern
+const AI_TIMEOUT_MS = 30_000 + +async function withTimeout<T>(promise: Promise<T>, ms: number, message: string): Promise<T> { + const timeout = new Promise<never>((_, reject) => + setTimeout(() => reject(new Error(message)), ms), + ) + return Promise.race([promise, timeout]) +} + async function translateStrings(ai: { run: (model: string, input: unknown) => Promise<unknown> }, strings: string[], targetLanguage: string) { // ... for (const batch of batches) { const segmentedText = buildSegmentedText(batch) - const result = await ai.run(TRANSLATION_MODEL, { + const result = await withTimeout(ai.run(TRANSLATION_MODEL, { text: segmentedText, source_lang: 'en', target_lang: targetLanguage, - }) as { translated_text?: string } + }), AI_TIMEOUT_MS, 'Translation timed out') as { translated_text?: string }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_backend/public/translation.ts` around lines 280 - 305, translateStrings currently calls ai.run(TRANSLATION_MODEL, ...) for each batch with no timeout, which can hang; update translateStrings to enforce a per-batch timeout (e.g., 30s) by wrapping the ai.run call in a timeout strategy (either Promise.race with a delay-reject promise or using AbortController if ai.run supports signal) so that slow/unresponsive Workers AI requests are aborted and handled; ensure you propagate or handle the timeout error (reject/log and continue or retry as appropriate) where parseSegmentedTranslation is called, and keep the existing batching logic (buildBatches, buildSegmentedText, parseSegmentedTranslation) intact.
247-258: Race condition in rate limit increment allows brief bursts past limit.The read-modify-write pattern on lines 249-255 is not atomic. Under concurrent requests, multiple callers can read the same
count, each increment it, and all pass the limit check before any write completes. This is acceptable for an edge-cache-based rate limiter where eventual consistency is expected, but be aware that actual throughput can briefly exceed the configured limit under burst traffic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_backend/public/translation.ts` around lines 247 - 258, The current read-modify-write using CacheHelper.matchJson + putJson on TranslationRateLimitEntry is vulnerable to race conditions; change to an atomic increment or compare-and-set loop so concurrent callers cannot overshoot the limit. Specifically, replace the matchJson/putJson pattern around cacheKey with either (1) an atomic increment operation provided by the cache (e.g., a CacheHelper.increment or Redis INCR-style call) and only set/reset the TTL when the counter is created, or (2) a CAS loop that reads the entry along with a version/etag and retries putJson until successful, then evaluate getTranslationIpRateLimit(limit) against the result; keep references to CacheHelper, buildRequest/TRANSLATION_IP_RATE_PATH, matchJson/putJson/TranslationRateLimitEntry, TRANSLATION_IP_RATE_TTL_SECONDS and getTranslationIpRateLimit when applying the change.tests/dynamic-translation.unit.test.ts (1)
1-9: Fix import ordering per ESLint rules.ESLint flags that the type import from the backend should come before the value import from the frontend (parent-type before parent). The type import separation from the past review comment has been addressed.
♻️ Proposed fix for import order
import { describe, expect, it } from 'vitest' -import { getWorkerLanguageCode, normalizeLanguage } from '../src/modules/i18n' import type { ProtectedEntry } from '../supabase/functions/_backend/public/translation.ts' import { normalizeTranslationStrings, parseSegmentedTranslation, protectTranslationTokens, restoreTranslationTokens, } from '../supabase/functions/_backend/public/translation.ts' +import { getWorkerLanguageCode, normalizeLanguage } from '../src/modules/i18n'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/dynamic-translation.unit.test.ts` around lines 1 - 9, Import ordering violates ESLint: move the type-only import for ProtectedEntry so it appears before the value imports from the frontend and keep the other named imports together; specifically, place "import type { ProtectedEntry } from '../supabase/functions/_backend/public/translation.ts'" above the imports of normalizeTranslationStrings, parseSegmentedTranslation, protectTranslationTokens, and restoreTranslationTokens, ensuring getWorkerLanguageCode and normalizeLanguage imports remain grouped and the file imports follow ESLint's parent-type-before-parent ordering.
🤖 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/services/i18n.ts`:
- Around line 23-24: The unconditional window.location.reload() call in
src/services/i18n.ts should be removed from the shared service and made
caller-controlled: change the function that currently invokes
window.location.reload() (the language change / setter function in this file) to
accept an optional parameter (e.g., reload?: boolean) or an optional callback
(e.g., onReload?: () => void), defaulting to false/no-op, and only perform the
reload when that parameter is true or the callback is provided; alternatively
emit a 'languageChanged' event (or return a status) so callers can decide to
call window.location.reload() themselves — update all internal callers to pass
the desired behavior and add tests for both reload and no-reload flows.
---
Nitpick comments:
In `@src/services/i18n.ts`:
- Around line 8-10: getLanguageEmoji should call the centralized normalization
helper getEmoji instead of directly using countryCodeToFlagEmoji to avoid
divergent normalization logic; update getLanguageEmoji(lang) to retrieve the
country code via getLanguageConfig(lang).countryCode and pass that into getEmoji
so the emoji generation and normalization remain centralized in getEmoji rather
than duplicating behaviour in getLanguageEmoji.
In `@supabase/functions/_backend/public/translation.ts`:
- Around line 190-225: The cloudlog calls inside parseSegmentedTranslation are
missing request correlation; update the exported function
parseSegmentedTranslation to accept an optional requestId parameter (e.g.,
parseSegmentedTranslation(translatedText: string, entries: ProtectedEntry[],
requestId?: string)) and include that requestId in the two cloudlog calls (the
one for missing marker and the one for missing boundary) so logs include {
message, index, marker, requestId }; ensure the function signature change is
propagated to callers and keep existing behavior when requestId is undefined,
and continue to use restoreTranslationTokens and entries as before.
- Around line 280-305: translateStrings currently calls
ai.run(TRANSLATION_MODEL, ...) for each batch with no timeout, which can hang;
update translateStrings to enforce a per-batch timeout (e.g., 30s) by wrapping
the ai.run call in a timeout strategy (either Promise.race with a delay-reject
promise or using AbortController if ai.run supports signal) so that
slow/unresponsive Workers AI requests are aborted and handled; ensure you
propagate or handle the timeout error (reject/log and continue or retry as
appropriate) where parseSegmentedTranslation is called, and keep the existing
batching logic (buildBatches, buildSegmentedText, parseSegmentedTranslation)
intact.
- Around line 247-258: The current read-modify-write using CacheHelper.matchJson
+ putJson on TranslationRateLimitEntry is vulnerable to race conditions; change
to an atomic increment or compare-and-set loop so concurrent callers cannot
overshoot the limit. Specifically, replace the matchJson/putJson pattern around
cacheKey with either (1) an atomic increment operation provided by the cache
(e.g., a CacheHelper.increment or Redis INCR-style call) and only set/reset the
TTL when the counter is created, or (2) a CAS loop that reads the entry along
with a version/etag and retries putJson until successful, then evaluate
getTranslationIpRateLimit(limit) against the result; keep references to
CacheHelper, buildRequest/TRANSLATION_IP_RATE_PATH,
matchJson/putJson/TranslationRateLimitEntry, TRANSLATION_IP_RATE_TTL_SECONDS and
getTranslationIpRateLimit when applying the change.
In `@tests/dynamic-translation.unit.test.ts`:
- Around line 1-9: Import ordering violates ESLint: move the type-only import
for ProtectedEntry so it appears before the value imports from the frontend and
keep the other named imports together; specifically, place "import type {
ProtectedEntry } from '../supabase/functions/_backend/public/translation.ts'"
above the imports of normalizeTranslationStrings, parseSegmentedTranslation,
protectTranslationTokens, and restoreTranslationTokens, ensuring
getWorkerLanguageCode and normalizeLanguage imports remain grouped and the file
imports follow ESLint's parent-type-before-parent ordering.
🪄 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: 3bb31a79-88e8-4076-b203-a64543640b8f
📒 Files selected for processing (5)
src/components/LangSelector.vuesrc/composables/usePageTranslation.tssrc/services/i18n.tssupabase/functions/_backend/public/translation.tstests/dynamic-translation.unit.test.ts
✅ Files skipped from review due to trivial changes (1)
- src/components/LangSelector.vue
🚧 Files skipped from review as they are similar to previous changes (1)
- src/composables/usePageTranslation.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f5a27dd096
ℹ️ 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".
| const rateLimitStatus = await recordTranslationRequest(c) | ||
| if (rateLimitStatus.limited) { |
There was a problem hiding this comment.
The handler increments/checks the per-IP limiter before reading the translation cache, so even cache hits consume quota and can start returning 429 once the shared IP crosses the threshold. In practice, repeated requests for already-cached pages (for example, multiple users behind one NAT) can get blocked even though no Workers AI call would be made, which turns a cost-control guard into a user-visible availability regression.
Useful? React with 👍 / 👎.
src/modules/i18n.ts
Outdated
| const knownSourceTexts = new Set([ | ||
| ...Object.values(messageCatalog), | ||
| ...languageOptions.map(option => option.label), | ||
| ].map(value => normalizeKnownSourceText(value)).filter(value => value && !value.includes('{'))) |
There was a problem hiding this comment.
Filtering out every catalog entry containing {...} prevents interpolated UI copy from ever being considered translatable. usePageTranslation only sends text when isKnownSourceText(...) is true (outside explicit opt-in zones), so messages rendered from keys like "... {count} ..." become values such as "2 minutes ago" that no longer match the allowlist and remain stuck in English.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d83379d0c1
ℹ️ 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".
| const strings = normalizeTranslationStrings(body.strings) | ||
| const requestHash = await sha256Hex(JSON.stringify({ | ||
| model: TRANSLATION_MODEL, | ||
| pagePath: body.pagePath ?? '', |
There was a problem hiding this comment.
Including pagePath in the server-side requestHash makes cache entries depend on concrete URLs instead of just the text/language payload. Because the client sends route.path (which contains route params on pages like /app/:app/...), identical strings on different app/device/bundle pages miss the cache and trigger fresh Workers AI calls, increasing latency/cost and causing avoidable rate-limit pressure. Hash against stable content inputs (or a route template/name) instead of the concrete path.
Useful? React with 👍 / 👎.
| if (controller.signal.aborted) | ||
| return |
There was a problem hiding this comment.
Requests are force-aborted after REQUEST_TIMEOUT_MS, but handleTranslationError returns immediately whenever controller.signal.aborted is true. That also swallows timeout-driven aborts, so a temporary slow response can leave the page untranslated with no retry/backoff unless a later DOM mutation or navigation happens. Timeout AbortError should be handled as a transient failure path rather than ignored.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50da775e35
ℹ️ 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".
| locales: { en }, | ||
| // FormKit keeps English source strings and the runtime page translator localizes | ||
| // the rendered validation copy, so we do not ship per-locale FormKit bundles. | ||
| locale: SOURCE_FORMKIT_LOCALE, |
There was a problem hiding this comment.
This hard-codes FormKit to English-only (locales: { en }, locale: 'en'), but the new page translator only sends allowlisted source-catalog text (isKnownSourceText) for translation. FormKit’s built-in validation copy (required/email/min/etc.) is not in the app catalog, so those errors now remain English in non-English sessions across login/register/settings forms. This is a user-facing localization regression introduced by removing the bundled FormKit locales without an equivalent translation path for validation strings.
Useful? React with 👍 / 👎.
| if (element.closest(NO_TRANSLATE_SELECTOR) || SKIP_TAGS.has(element.tagName)) | ||
| return |
There was a problem hiding this comment.
The attribute collector exits early for every TEXTAREA element, which also drops translatable attributes like placeholder/title/aria-label on textareas. With this commit’s English-source runtime translation flow, textarea placeholders (for example in role assignment reason fields) stay stuck in English for non-English locales. The skip should only avoid translating textarea content/user input, not its UI attributes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 43d5cb62ce
ℹ️ 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".
| function getTranslationRoot() { | ||
| return document.querySelector<HTMLElement>(TRANSLATION_ROOT_SELECTOR) | ||
| } |
There was a problem hiding this comment.
getTranslationRoot() limits scanning to the element marked data-capgo-translation-root, but several critical surfaces (for example DialogV2 uses <Teleport to="body">) render outside that subtree. Since this commit also makes t() always return English source text, modal titles/buttons/descriptions shown in teleported overlays will never be post-translated for non-English users. Expand the observed root (or add a second observer/root) so body-level teleports are translated too.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b3c5cd6b0
ℹ️ 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".
| function shouldSkipTranslation(root: TranslationRoot, lang: string) { | ||
| if (isEnglishLocale(lang) || translationDisabled) { | ||
| resetTranslationState(root) | ||
| return true |
There was a problem hiding this comment.
When selectedLanguage is English (the default path), shouldSkipTranslation() still calls resetTranslationState(root), which runs restoreSourceContent() over the entire translation root. Because the mutation observer is always active and schedules translation on every relevant DOM change, this turns normal UI updates (typing, reactive rerenders, list updates) into repeated full-tree walks for English sessions, creating a significant performance regression for the primary locale. Skip the restore walk unless we are actually reverting previously translated content.
Useful? React with 👍 / 👎.
| if (start < 0) { | ||
| cloudlog({ message: 'Translation segment marker missing from model output', index, marker: entry.marker }) | ||
| translations.set(entry.source, entry.source) | ||
| return |
There was a problem hiding this comment.
If Workers AI output drops a segment marker, parseSegmentedTranslation() silently maps that entry back to entry.source and continues. The caller then treats this as a successful translation and caches the payload for 7 days, so one malformed model response can pin untranslated English text for all matching requests until cache expiry. Marker-loss should be treated as a retriable failure (or at least excluded from cache writes) instead of being cached as final output.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7bb414eace
ℹ️ 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".
| if (response.status === 404 || response.status === 501 || response.status === 503) { | ||
| // Supabase-only/local runtimes intentionally do not expose translation | ||
| // bundles, and workers without AI bindings should keep the source English UI. | ||
| messageCatalogTranslationDisabled = true | ||
| return messageCatalog |
There was a problem hiding this comment.
This branch treats 503 Service Unavailable as a permanent disable signal by setting messageCatalogTranslationDisabled, and later calls short-circuit at line 261 to English forever in the current tab. If Workers AI has a brief outage or warmup delay, users who selected a non-English locale will not recover translated message catalogs until a full reload, so 503 should be handled as transient instead of disabling the feature.
Useful? React with 👍 / 👎.
src/modules/i18n.ts
Outdated
| translatedMessageCatalogs.set(normalized, messages) | ||
| if (messages !== messageCatalog) |
There was a problem hiding this comment.
fetchTranslatedMessageCatalog returns messageCatalog on network/transient failures, and this code stores that fallback under the target locale. After one temporary fetch failure (offline blip, DNS error, timeout), ensureMessageCatalogLoaded sees an existing catalog and never retries, so that locale stays English for the whole session even after connectivity recovers.
Useful? React with 👍 / 👎.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa94bde8fd
ℹ️ 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".
| if (typeof params === 'string') | ||
| return params |
There was a problem hiding this comment.
Returning params directly when it is a string changes t(key, defaultMsg) behavior into an unconditional override, so translated catalogs are ignored for those calls. In this repo, src/utils/invites.ts calls t('too-recent-invitation-cancelation', options.cancelledFallback), which now always renders the English fallback text in non-English sessions even when a translated key is available. Keep key-based lookup as the primary path and use the string only as a fallback when the key is actually missing.
Useful? React with 👍 / 👎.
1 participant
Summary (AI generated)
vue-i18ncompatibility shimMotivation (AI generated)
Maintaining large static translation catalogs in the repo is expensive and quickly drifts from the actual UI. This change moves translation to the edge so Capgo can ship English source text once, translate on demand with Workers AI, and cache the result close to the user instead of versioning locale files in the application.
Business Impact (AI generated)
This reduces translation maintenance cost, expands language coverage to the highest-priority developer languages without adding more repo weight, and makes localized experiences available immediately after cache warmup. It also keeps translation latency and cost bounded by reusing Cloudflare cache instead of translating every request from origin, while preserving a safe English fallback when the worker route is unavailable.
Test Plan (AI generated)
bun run lintbun run lint:backendbun run typecheckbunx vitest run tests/dynamic-translation.unit.test.tsbun run build/translation/pageis unavailable instead of breaking navigation or rendering.route.pathonly and do not include query-string or hash tokens.data-capgo-translate.Screenshots (AI generated)
Generated with AI