fix(i18n): queue remote message translations#2046
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:
📝 WalkthroughWalkthroughReplaces in-process AI translation with a cache-first D1-backed translation store and a translation queue. API computes a source checksum, returns cached/ready payloads or 202 pending, and enqueues work. Adds a Cloudflare translation worker to consume queue batches, call an AI binding, preserve placeholders, and update store/cache. Tests and deployment wiring added. ChangesTranslation queue + cache-backed workflow
Sequence DiagramsequenceDiagram
participant Client
participant API as Supabase<br/>API
participant Cache
participant Store as Translation<br/>Store (D1)
participant Queue as Translation<br/>Queue
participant Worker as Cloudflare<br/>Worker
participant AI as AI<br/>Service
Client->>API: POST /messages (targetLanguage)
API->>API: compute checksum
API->>Cache: check checksum+lang
alt Cache hit
Cache-->>API: ready payload
API-->>Client: 200 + payload
else Cache miss
API->>Store: read entry by checksum+lang
alt Store ready
Store-->>API: ready payload
API->>Cache: populate cache
API-->>Client: 200 + payload
else Store pending
alt pending fresh
API-->>Client: 202 Pending
else pending stale
API->>Queue: enqueue/requeue batch
API->>Store: touch entry
API-->>Client: 202 Pending
end
else No entry
API->>Store: insert pending entry
API->>Queue: enqueue initial batch
API-->>Client: 202 Pending
end
end
par Async processing
Queue-->>Worker: deliver MessageBatch
Worker->>AI: request translations (retries)
AI-->>Worker: translation results
Worker->>Worker: merge batches, preserve placeholders
Worker->>Store: write/update entry (pending→ready)
Worker->>Cache: write ready payload
Worker-->>Queue: ack
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
riderx
force-pushed
the
codex/translation-cache-queue
branch
from
369b334 to
67a5dea
Compare
💡 Codex Reviewcapgo/supabase/functions/_backend/public/translation.ts Lines 446 to 449 in 67a5dea  Store queued translations in shared storage, not edge cache
The queue worker writes capgo/supabase/functions/_backend/public/translation.ts Lines 502 to 506 in 67a5dea  Fail fast when AI binding is unavailable on /messages
The public handler now always queues work and returns ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
riderx
force-pushed
the
codex/translation-cache-queue
branch
from
67a5dea to
6333699
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Addressed the two Codex review points in 6333699: translation progress/ready state is now stored in shared database cache instead of edge-only Cache API, and /translation/messages returns translation_unavailable immediately when Workers AI is not bound. |
💡 Codex Reviewcapgo/supabase/functions/_backend/public/translation.ts Lines 523 to 525 in 4c6bcc3 Make translation job enqueue atomic per checksum/language
When two ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@coderabbitai review Also addressed the latest Codex atomic enqueue point in |
|
Tip For best results, initiate chat on the files or code changes.
[review] |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
supabase/functions/_backend/public/translation.ts (3)
355-361: 💤 Low valueRemove unnecessary type assertion.
Per static analysis, the type assertion
as D1Database | undefinedis redundant sincec.env.DB_STOREAPPSalready has the correct type from theBindingsinterface.♻️ Proposed fix
function getTranslationStore(c: Context) { - const store = c.env.DB_STOREAPPS as D1Database | undefined + const store = c.env.DB_STOREAPPS if (!store) quickError(503, 'translation_unavailable', 'Cloudflare D1 translation store is not configured') return store }🤖 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 `@supabase/functions/_backend/public/translation.ts` around lines 355 - 361, The local variable declaration in getTranslationStore currently uses a redundant type assertion on c.env.DB_STOREAPPS; remove the "as D1Database | undefined" so that store is directly assigned from c.env.DB_STOREAPPS (relying on the Bindings type), keep the existing null-check and quickError call (quickError(503, 'translation_unavailable', ...)) and return store as before to avoid changing runtime behavior.
363-369: 💤 Low valueRemove unnecessary type assertion.
Per static analysis, the type assertion
as Queue<Required<TranslationQueuePayload>> | undefinedis redundant.♻️ Proposed fix
function getTranslationQueue(c: Context) { - const queue = c.env.TRANSLATION_MESSAGES_QUEUE as Queue<Required<TranslationQueuePayload>> | undefined + const queue = c.env.TRANSLATION_MESSAGES_QUEUE if (!queue) quickError(503, 'translation_unavailable', 'Cloudflare translation queue is not configured') return queue }🤖 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 `@supabase/functions/_backend/public/translation.ts` around lines 363 - 369, The type assertion on c.env.TRANSLATION_MESSAGES_QUEUE in getTranslationQueue is redundant; remove the "as Queue<Required<TranslationQueuePayload>> | undefined" and simply assign const queue = c.env.TRANSLATION_MESSAGES_QUEUE (or, if you want an explicit type, use a proper TS annotation: const queue: Queue<Required<TranslationQueuePayload>> | undefined = c.env.TRANSLATION_MESSAGES_QUEUE) and leave the quickError check and return unchanged.
371-390: ⚖️ Poor tradeoffConsider optimizing repeated DDL and cleanup operations.
ensureTranslationStoreexecutesCREATE TABLE IF NOT EXISTSandCREATE INDEX IF NOT EXISTSon every D1 access path. Similarly,deleteExpiredTranslationStoreEntriesruns aDELETEquery on every read/write. While D1 handles these efficiently, this adds latency to every translation request.Consider:
- Using a module-level flag to skip DDL after first call within a worker instance
- Running cleanup less frequently (e.g., probabilistically or via separate cron)
Also applies to: 434-436
🤖 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 `@supabase/functions/_backend/public/translation.ts` around lines 371 - 390, The ensureTranslationStore function currently runs CREATE TABLE/INDEX on every call and deleteExpiredTranslationStoreEntries runs DELETE on every operation, causing extra latency; modify ensureTranslationStore to use a module-level boolean (e.g., translationStoreInitialized) that is checked and set after the first successful DDL run to skip subsequent CREATEs within the same worker, and change deleteExpiredTranslationStoreEntries to run cleanup less frequently — implement a probabilistic trigger (e.g., Math.random() threshold) inside the function or move cleanup to a separate scheduled job — ensure you reference and update the functions ensureTranslationStore and deleteExpiredTranslationStoreEntries and keep DDL/cleanup as fallback if the flag/probability path skips execution.cloudflare_workers/api/index.ts (1)
175-192: ⚡ Quick winConsider logging errors before retrying for observability.
The empty catch block silently swallows errors before retrying, which makes debugging failed translation batches difficult. Adding a log statement would improve observability without changing the retry behavior.
♻️ Proposed improvement for error logging
catch { + console.error('Translation queue consumer error for message', message.id) message.retry({ delaySeconds: 30 }) }🤖 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 `@cloudflare_workers/api/index.ts` around lines 175 - 192, The catch block in the queue method swallows errors; update the async function queue so the catch captures the error (e.g., catch (err)) and logs useful context before retrying: include the thrown error (err), the message payload (message.body) and any response status if available from translation_messages.fetch, then call message.retry({ delaySeconds: 30 }); reference the queue function, translation_messages.fetch call, message.ack and message.retry to locate where to add the logging (use console.error or your existing logger).
🤖 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.
Nitpick comments:
In `@cloudflare_workers/api/index.ts`:
- Around line 175-192: The catch block in the queue method swallows errors;
update the async function queue so the catch captures the error (e.g., catch
(err)) and logs useful context before retrying: include the thrown error (err),
the message payload (message.body) and any response status if available from
translation_messages.fetch, then call message.retry({ delaySeconds: 30 });
reference the queue function, translation_messages.fetch call, message.ack and
message.retry to locate where to add the logging (use console.error or your
existing logger).
In `@supabase/functions/_backend/public/translation.ts`:
- Around line 355-361: The local variable declaration in getTranslationStore
currently uses a redundant type assertion on c.env.DB_STOREAPPS; remove the "as
D1Database | undefined" so that store is directly assigned from
c.env.DB_STOREAPPS (relying on the Bindings type), keep the existing null-check
and quickError call (quickError(503, 'translation_unavailable', ...)) and return
store as before to avoid changing runtime behavior.
- Around line 363-369: The type assertion on c.env.TRANSLATION_MESSAGES_QUEUE in
getTranslationQueue is redundant; remove the "as
Queue<Required<TranslationQueuePayload>> | undefined" and simply assign const
queue = c.env.TRANSLATION_MESSAGES_QUEUE (or, if you want an explicit type, use
a proper TS annotation: const queue: Queue<Required<TranslationQueuePayload>> |
undefined = c.env.TRANSLATION_MESSAGES_QUEUE) and leave the quickError check and
return unchanged.
- Around line 371-390: The ensureTranslationStore function currently runs CREATE
TABLE/INDEX on every call and deleteExpiredTranslationStoreEntries runs DELETE
on every operation, causing extra latency; modify ensureTranslationStore to use
a module-level boolean (e.g., translationStoreInitialized) that is checked and
set after the first successful DDL run to skip subsequent CREATEs within the
same worker, and change deleteExpiredTranslationStoreEntries to run cleanup less
frequently — implement a probabilistic trigger (e.g., Math.random() threshold)
inside the function or move cleanup to a separate scheduled job — ensure you
reference and update the functions ensureTranslationStore and
deleteExpiredTranslationStoreEntries and keep DDL/cleanup as fallback if the
flag/probability path skips execution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8f53538d-e58f-4314-8352-d1a54413be4e
📒 Files selected for processing (5)
cloudflare_workers/api/index.tscloudflare_workers/api/wrangler.jsoncsupabase/functions/_backend/public/translation.tssupabase/functions/_backend/utils/cloudflare.tstests/translation-queue.unit.test.ts
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Addressed CodeRabbit nitpicks in Latest GitHub CI is green and there are no open review threads. |
![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: dc2d456973
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
supabase/functions/_backend/public/translation.ts (1)
380-382: ⚡ Quick winPrecompute the source checksum once per isolate.
The source catalog is static during runtime; re-hashing on every request adds avoidable CPU on a hot path.
♻️ Suggested refactor
export const sourceMessageCatalog = sourceMessages as Record<string, string> +const sourceCatalogChecksumPromise = sha256Hex(JSON.stringify(sourceMessageCatalog)) let translationStoreInitialized = false let lastTranslationStoreCleanupAt = 0 @@ export async function currentSourceChecksum() { - return sha256Hex(JSON.stringify(sourceMessageCatalog)) + return sourceCatalogChecksumPromise }🤖 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 `@supabase/functions/_backend/public/translation.ts` around lines 380 - 382, Precompute the checksum at module load and have currentSourceChecksum return the cached value to avoid re-hashing on every request: compute a const like precomputedSourceChecksum = sha256Hex(JSON.stringify(sourceMessageCatalog)) (ensuring sourceMessageCatalog is already initialized) and change the currentSourceChecksum function to simply return that cached precomputedSourceChecksum; reference sha256Hex, sourceMessageCatalog and currentSourceChecksum when making the change.
🤖 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 `@supabase/functions/_backend/public/translation.ts`:
- Around line 409-411: The cache write call cacheHelper.putJson in the ready
flow is best-effort and must not abort the response; wrap the
cacheHelper.putJson(readyRequest, payload, CACHE_TTL_SECONDS) call inside a
try/catch, on error log the failure (include readyRequest and the error) and
continue without rethrowing, then still set the response Cache-Control header
via c.header('Cache-Control', `public, max-age=0,
s-maxage=${CACHE_TTL_SECONDS}`); apply the same try/catch pattern to the other
occurrence of cacheHelper.putJson noted in the review (around the ready flow at
the second spot).
---
Nitpick comments:
In `@supabase/functions/_backend/public/translation.ts`:
- Around line 380-382: Precompute the checksum at module load and have
currentSourceChecksum return the cached value to avoid re-hashing on every
request: compute a const like precomputedSourceChecksum =
sha256Hex(JSON.stringify(sourceMessageCatalog)) (ensuring sourceMessageCatalog
is already initialized) and change the currentSourceChecksum function to simply
return that cached precomputedSourceChecksum; reference sha256Hex,
sourceMessageCatalog and currentSourceChecksum when making the change.
🪄 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: 9b0b3483-585b-4731-b027-bbe49d4f786f
📒 Files selected for processing (1)
supabase/functions/_backend/public/translation.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6ee85553a
ℹ️ 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".
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 `@cloudflare_workers/translation/index.ts`:
- Around line 278-355: Before translating or enqueuing the next batch, make
batch ownership idempotent by performing a compare-and-set (lease) on the
translation store keyed by (checksum, targetLanguage, nextBatchIndex); in the
handler around normalizeBatchIndex/body.batchIndex and before calling
translateBatch or enqueueTranslationBatch, read the current storedEntry and
attempt an atomic update that claims the nextBatchIndex only if it still equals
the expected value (use or add a helper like compareAndSetTranslationEntry or a
D1 UPDATE ... WHERE nextBatchIndex = ? returning success); if the CAS fails
(meaning nextBatchIndex has advanced) then treat the message as stale and return
immediately (do not call translateBatch or enqueueTranslationBatch), and if CAS
succeeds proceed to translateBatch and then update the store via
writeTranslationStoreEntry and enqueue the following batch.
🪄 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: cbe396bd-291f-4c4d-ba84-d5bb4d879000
📒 Files selected for processing (3)
cloudflare_workers/translation/index.tssupabase/functions/_backend/public/translation.tstests/translation-queue.unit.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/translation-queue.unit.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cloudflare_workers/translation/index.ts (1)
266-268: 💤 Low valueAdd explicit return after
quickErrorfor clarity.While
quickErrorlikely throws (standard Hono pattern), adding an explicit return makes the control flow unambiguous and helps readers and static analysis tools understand thataiis guaranteed to be defined afterwards.Suggested change
const ai = c.env.AI as AiBinding | undefined if (!ai) - quickError(503, 'translation_unavailable', 'Workers AI binding is not configured') + return quickError(503, 'translation_unavailable', 'Workers AI binding is not configured')🤖 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 `@cloudflare_workers/translation/index.ts` around lines 266 - 268, The guard that checks c.env.AI uses quickError but doesn't explicitly return; make the control flow unambiguous by returning the call to quickError when ai is falsy (e.g., change the branch so it does "return quickError(...)" or add an explicit "return" immediately after the quickError call) so that after the check ai is guaranteed defined; update the statement referencing ai and quickError accordingly.
🤖 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.
Nitpick comments:
In `@cloudflare_workers/translation/index.ts`:
- Around line 266-268: The guard that checks c.env.AI uses quickError but
doesn't explicitly return; make the control flow unambiguous by returning the
call to quickError when ai is falsy (e.g., change the branch so it does "return
quickError(...)" or add an explicit "return" immediately after the quickError
call) so that after the check ai is guaranteed defined; update the statement
referencing ai and quickError accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6e87791d-5007-406c-977a-7fc375f96208
📒 Files selected for processing (3)
cloudflare_workers/translation/index.tssupabase/functions/_backend/public/translation.tstests/translation-queue.unit.test.ts
✅ Files skipped from review due to trivial changes (1)
- tests/translation-queue.unit.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- supabase/functions/_backend/public/translation.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 81cd5ab7fc
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b08f1d96f0
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f3674f5ed
ℹ️ 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".
|
1 participant
Summary (AI generated)
cloudflare_workers/translation.Motivation (AI generated)
The translation flow needs async queue behavior so production requests do not timeout while Workers AI translates the full message catalog. Splitting the consumer into its own worker keeps product API deployment smaller and makes the translation worker independently deployable.
Business Impact (AI generated)
This keeps non-English app translations reliable without adding load to Postgres or blocking normal API traffic. It also reduces operational risk by isolating Workers AI translation failures from the core API worker.
Test Plan (AI generated)
bunx eslint cloudflare_workers/api/index.ts cloudflare_workers/translation/index.ts supabase/functions/_backend/public/translation.ts tests/translation-queue.unit.test.tsbunx vitest run tests/translation-queue.unit.test.tsbun lintbun typecheckbunx wrangler deploy --config cloudflare_workers/api/wrangler.jsonc --env=alpha --dry-run --outdir /tmp/capgo-api-dry-run --minifybunx wrangler deploy --config cloudflare_workers/translation/wrangler.jsonc --env=alpha --dry-run --outdir /tmp/capgo-translation-dry-run --minifybunx wrangler deploy --config cloudflare_workers/api/wrangler.jsonc --env=prod --dry-run --outdir /tmp/capgo-api-prod-dry-run --minify --upload-source-mapsbunx wrangler deploy --config cloudflare_workers/api/wrangler.jsonc --env=preprod --dry-run --outdir /tmp/capgo-api-preprod-dry-run --minifybunx wrangler deploy --config cloudflare_workers/translation/wrangler.jsonc --env=prod --dry-run --outdir /tmp/capgo-translation-prod-dry-run --minify --upload-source-mapsbunx wrangler deploy --config cloudflare_workers/translation/wrangler.jsonc --env=preprod --dry-run --outdir /tmp/capgo-translation-preprod-dry-run --minifyGenerated with AI
Summary by CodeRabbit
New Features
Tests
Chores