fix(backend): harden manifest size processing#2293
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a ChangesBackfill script: target-aware env and DB
Queue consumer and manifest dispatch
Tests
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 docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Merging this PR will not alter performance
Comparing 
Footnotes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@scripts/backfill_manifest_file_sizes.mjs`:
- Around line 58-81: getTarget() currently builds envPaths so that '../.env' is
loaded before the prod-specific file, causing generic DB keys in process.env to
outrank prod-only keys; update the logic so the prod branch does not load the
generic '../.env' (or at minimum loads the prod-specific file last/solely)
and/or change getDatabaseUrl() to resolve against a passed-in env object parsed
from the selected envPaths instead of reading merged process.env; locate and
change the envPaths construction and the call sites of getDatabaseUrl() so the
DB URL is derived only from the intended target's env file(s) (symbols:
getTarget, envPaths, getDatabaseUrl).
- Around line 102-108: The isLocalDatabaseUrl function fails to recognize
bracketed IPv6 loopback hosts because new URL returns '[::1]'; update
isLocalDatabaseUrl to normalize the hostname by stripping surrounding square
brackets (if hostname startsWith('[') and endsWith(']') then hostname =
hostname.slice(1, -1)) before checking against the local host list, and also
extend the catch fallback to look for both '::1' and '[::1]' in the raw
databaseUrl string; this ensures bracketed IPv6 literals are treated as local.
🪄 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: 55375c12-d3e3-43c0-80b2-9f9beec0eb35
📒 Files selected for processing (1)
scripts/backfill_manifest_file_sizes.mjs
riderx
changed the title
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
supabase/functions/_backend/triggers/queue_consumer.ts (1)
240-251: 💤 Low valueExtract nested ternary for readability.
The nested ternary at lines 243-245 is hard to follow. Consider extracting the message resolution to a separate variable.
♻️ Proposed refactor
if (cause && typeof cause === 'object' && 'error' in cause) { const causeData = cause as { error?: unknown message?: unknown moreInfo?: unknown } + const errorMessage = typeof causeData.message === 'string' + ? causeData.message + : typeof maybeException.message === 'string' + ? maybeException.message + : 'Queue handler failed' return queueFailureResponse( typeof causeData.error === 'string' ? causeData.error : 'http_exception', - typeof causeData.message === 'string' - ? causeData.message - : typeof maybeException.message === 'string' - ? maybeException.message - : 'Queue handler failed', + errorMessage, causeData.moreInfo && typeof causeData.moreInfo === 'object' ? causeData.moreInfo as Record<string, unknown> : {}, maybeException.status, ) }🤖 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/triggers/queue_consumer.ts` around lines 240 - 251, The nested ternary used to compute the error message (checking causeData.message and maybeException.message) is hard to read; extract that logic into a clearly named local variable (e.g., resolvedMessage or messageToLog) before the call that currently inlines it, using the same type checks (typeof causeData.message === 'string' ? causeData.message : typeof maybeException.message === 'string' ? maybeException.message : 'Queue handler failed'), then replace the nested ternary in the argument list with that variable; keep the existing symbols (causeData, maybeException) and ensure the variable is used where the current ternary result is passed.
🤖 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 `@supabase/functions/_backend/triggers/queue_consumer.ts`:
- Around line 240-251: The nested ternary used to compute the error message
(checking causeData.message and maybeException.message) is hard to read; extract
that logic into a clearly named local variable (e.g., resolvedMessage or
messageToLog) before the call that currently inlines it, using the same type
checks (typeof causeData.message === 'string' ? causeData.message : typeof
maybeException.message === 'string' ? maybeException.message : 'Queue handler
failed'), then replace the nested ternary in the argument list with that
variable; keep the existing symbols (causeData, maybeException) and ensure the
variable is used where the current ternary result is passed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2a869fad-25e9-42a0-a06a-8b86ff3de3e2
📒 Files selected for processing (4)
supabase/functions/_backend/triggers/on_manifest_create.tssupabase/functions/_backend/triggers/queue_consumer.tstests/app-error-cases.test.tstests/queue-consumer-message-shape.unit.test.ts
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary (AI generated)
scripts/backfill_manifest_file_sizes.mjsto the production env target and refuse local Postgres URLs unless--target=localis explicit.on_manifest_createqueue messages directly inside the queue consumer instead of doing one HTTP self-call per manifest row.tests/app-error-cases.test.tsby using the existing retrying fetch helper for transient Edge socket closes.Motivation (AI generated)
The production failure was not the CLI-provided size. Manifest rows were inserted with
file_size = 0, then the queue consumer attempted many internal trigger HTTP calls. A transport timeout rejected the batch-levelPromise.all, so some files got updated and the rest stayed at zero until the queue exhausted its 5 reads and archived them. The previous logs hid the exact per-file cause because the batch aborted before recording each result.Business Impact (AI generated)
New bundles should stop drifting into partial
metadata not foundstates. Existing bad rows can be repaired with the production-safe backfill script after deploy, without accidentally targeting local Supabase.Test Plan (AI generated)
bun test tests/queue-consumer-message-shape.unit.test.tsbun test tests/backend-alert-resilience.unit.test.ts -t manifestbun lint:backendbun lintbun typecheckgit diff --checkbun run cli:build && vue-tsc --noEmitRun testspassed on PR and push workflows after rerunning the transient upstream-response failure.Summary by CodeRabbit
New Features
Bug Fixes
Chores