fix: use drizzle to avoid rows soft-limit for manifest delete && r2 fileExists fixes #1218
Conversation
WalkthroughReplaced Supabase manifest fetch with a Drizzle/Postgres query in the version-update trigger; added try/catch/finally to wrap retrieval and cleanup (ensuring client closure); prepare per-entry DB deletes and optional S3 deletes and run them via backgroundTask; S3 existence check now uses presigned HEAD with content-length guard. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant T as on_version_update Trigger
participant DC as Drizzle Client
participant DB as Postgres (manifest)
participant BG as backgroundTask
participant S3 as Object Storage
participant L as Logger
T->>DC: init()
T->>DB: SELECT manifest WHERE app_version_id=...
DB-->>T: manifestEntries[]
alt entries found
loop per entry
T->>DB: DELETE manifest row
opt entry has S3 key
T->>BG: prepare/enqueue S3 delete task (key)
end
end
T->>BG: run queued deletions async
BG->>S3: DELETE via presigned DELETE URL
S3-->>BG: response (2xx / other)
else no entries
Note over T: skip deletions
end
T-->>T: try/catch errors
T->>L: log error (if any)
T->>DC: close() (finally)
sequenceDiagram
autonumber
participant Util as s3.checkIfExist
participant Presigner as Presign Service
participant HTTP as HTTP (HEAD)
participant S3 as Object Storage
Util->>Presigner: getPresignedHEAD(key)
Presigner-->>Util: presigned HEAD URL
Util->>HTTP: HEAD presigned URL
HTTP-->>Util: 200 + headers or error
alt 200 with content-length > 0
Note right of Util: return true
else any other response / error
Note right of Util: return false
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
supabase/functions/_backend/triggers/on_version_update.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
{src,supabase/functions/_backend}/**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
Enforce code style per @antfu/eslint-config: single quotes and no semicolons
Files:
supabase/functions/_backend/triggers/on_version_update.ts
supabase/functions/_backend/triggers/**/*.{ts,tsx,js,sql}
📄 CodeRabbit inference engine (CLAUDE.md)
Place database triggers and CRON-related functions under supabase/functions/_backend/triggers/
Files:
supabase/functions/_backend/triggers/on_version_update.ts
supabase/functions/_backend/**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Write backend code as Deno-compatible Supabase Edge Functions
Files:
supabase/functions/_backend/triggers/on_version_update.ts
🧬 Code graph analysis (1)
supabase/functions/_backend/triggers/on_version_update.ts (5)
supabase/functions/_backend/utils/pg.ts (3)
getPgClient(55-59)getDrizzleClient(61-63)closeClient(65-68)supabase/functions/_backend/utils/postgress_schema.ts (1)
manifest(68-77)supabase/functions/_backend/utils/supabase.ts (1)
supabaseAdmin(74-83)supabase/functions/_backend/utils/loggin.ts (1)
cloudlog(3-15)supabase/functions/_backend/utils/utils.ts (1)
backgroundTask(97-110)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run tests
| const count = v.count ?? 0 | ||
| if (!count) { | ||
| return supabaseAdmin(c) | ||
| .from('manifest') | ||
| .delete() | ||
| .eq('id', entry.id) | ||
| } | ||
| cloudlog({ requestId: c.get('requestId'), message: 'deleted manifest file from S3', s3_path: entry.s3_path }) | ||
| return Promise.all([ | ||
| s3.deleteObject(c, entry.s3_path), | ||
| supabaseAdmin(c) | ||
| .from('manifest') | ||
| .delete() | ||
| .eq('id', entry.id), | ||
| ]) as any | ||
| })) |
There was a problem hiding this comment.
Fix inverted S3 deletion guard
count represents other manifest rows referencing the same asset. The current condition skips deleting the S3 object when count is zero (no other references) and does delete it when count is positive, which will wipe shared artifacts and strand orphans. Please flip the guard so S3 deletion only happens when no other rows point at the file.
- const count = v.count ?? 0
- if (!count) {
- return supabaseAdmin(c)
- .from('manifest')
- .delete()
- .eq('id', entry.id)
- }
- cloudlog({ requestId: c.get('requestId'), message: 'deleted manifest file from S3', s3_path: entry.s3_path })
- return Promise.all([
- s3.deleteObject(c, entry.s3_path),
- supabaseAdmin(c)
- .from('manifest')
- .delete()
- .eq('id', entry.id),
- ]) as any
+ const count = v.count ?? 0
+ if (count && count > 0) {
+ return supabaseAdmin(c)
+ .from('manifest')
+ .delete()
+ .eq('id', entry.id)
+ }
+ cloudlog({ requestId: c.get('requestId'), message: 'deleted manifest file from S3', s3_path: entry.s3_path })
+ return Promise.all([
+ s3.deleteObject(c, entry.s3_path),
+ supabaseAdmin(c)
+ .from('manifest')
+ .delete()
+ .eq('id', entry.id),
+ ]) as any📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const count = v.count ?? 0 | |
| if (!count) { | |
| return supabaseAdmin(c) | |
| .from('manifest') | |
| .delete() | |
| .eq('id', entry.id) | |
| } | |
| cloudlog({ requestId: c.get('requestId'), message: 'deleted manifest file from S3', s3_path: entry.s3_path }) | |
| return Promise.all([ | |
| s3.deleteObject(c, entry.s3_path), | |
| supabaseAdmin(c) | |
| .from('manifest') | |
| .delete() | |
| .eq('id', entry.id), | |
| ]) as any | |
| })) | |
| const count = v.count ?? 0 | |
| if (count && count > 0) { | |
| return supabaseAdmin(c) | |
| .from('manifest') | |
| .delete() | |
| .eq('id', entry.id) | |
| } | |
| cloudlog({ requestId: c.get('requestId'), message: 'deleted manifest file from S3', s3_path: entry.s3_path }) | |
| return Promise.all([ | |
| s3.deleteObject(c, entry.s3_path), | |
| supabaseAdmin(c) | |
| .from('manifest') | |
| .delete() | |
| .eq('id', entry.id), | |
| ]) as any |
🤖 Prompt for AI Agents
In supabase/functions/_backend/triggers/on_version_update.ts around lines 139 to
154 the S3-deletion guard is inverted: currently it skips S3 deletion when count
=== 0 and deletes S3 when count > 0; flip the logic so that when count === 0 (no
other manifest rows reference the asset) you delete the S3 object and the
manifest row, and when count > 0 you only delete the manifest row (no S3
deletion). Move or add the cloudlog to the branch where S3 is actually deleted,
and ensure the function returns the appropriate Promise (Promise.all for
S3+manifest deletion, single supabaseAdmin delete for manifest-only) with
correct typing.
WcaleNieWolny
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
supabase/functions/_backend/utils/s3.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
{src,supabase/functions/_backend}/**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
Enforce code style per @antfu/eslint-config: single quotes and no semicolons
Files:
supabase/functions/_backend/utils/s3.ts
supabase/functions/_backend/utils/**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Put shared utilities and database schemas under supabase/functions/_backend/utils/
Files:
supabase/functions/_backend/utils/s3.ts
supabase/functions/_backend/**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Write backend code as Deno-compatible Supabase Edge Functions
Files:
supabase/functions/_backend/utils/s3.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run tests
- GitHub Check: Analyze (java-kotlin)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
supabase/functions/_backend/utils/s3.ts (1)
76-92: Critical: Missing S3 client initialization.The
clientvariable is referenced on line 81 but never initialized. This will cause aReferenceError: client is not definedat runtime.Apply this diff to initialize the client:
async function checkIfExist(c: Context, fileId: string | null) { if (!fileId) { return false } + const client = initS3(c) try { const url = await client.getPresignedUrl('HEAD', fileId)
🧹 Nitpick comments (2)
supabase/functions/_backend/utils/s3.ts (2)
85-85: Consider specifying radix parameter forparseInt.While
Number.parseIntdefaults to base 10 for decimal strings, explicitly passing the radix parameter is a best practice that prevents edge cases and improves code clarity.Apply this diff:
- const contentLength = Number.parseInt(response.headers.get('content-length') || '0') + const contentLength = Number.parseInt(response.headers.get('content-length') || '0', 10)
67-74: Handle errors for deleteObject calls consistently
In on_version_update.ts at line 148 the call tos3.deleteObject(c, entry.s3_path)insidePromise.allisn’t wrapped in try/catch (unlike the v2 call at line 178). Wrap it or handle its rejection to match the later block and avoid unhandled promise failures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
supabase/functions/_backend/utils/s3.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
{src,supabase/functions/_backend}/**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
Enforce code style per @antfu/eslint-config: single quotes and no semicolons
Files:
supabase/functions/_backend/utils/s3.ts
supabase/functions/_backend/utils/**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Put shared utilities and database schemas under supabase/functions/_backend/utils/
Files:
supabase/functions/_backend/utils/s3.ts
supabase/functions/_backend/**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Write backend code as Deno-compatible Supabase Edge Functions
Files:
supabase/functions/_backend/utils/s3.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run tests
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
supabase/functions/_backend/utils/s3.ts (3)
67-74: Add error handling and timeout for reliability.The function lacks error handling, so fetch failures will throw and propagate to the caller. This is inconsistent with
checkIfExist(which catches all errors and returns false). Additionally, the fetch call has no timeout, which could lead to hanging requests.Consider adding structured error handling and a timeout:
async function deleteObject(c: Context, fileId: string) { - const client = initS3(c) - const url = await client.getPresignedUrl('DELETE', fileId) - const response = await fetch(url, { - method: 'DELETE', - }) - return response.status >= 200 && response.status < 300 + try { + const client = initS3(c) + const url = await client.getPresignedUrl('DELETE', fileId) + const response = await fetch(url, { + method: 'DELETE', + signal: AbortSignal.timeout(10000), // 10 second timeout + }) + return response.status >= 200 && response.status < 300 + } + catch (error) { + cloudlog({ requestId: c.get('requestId'), message: 'deleteObject failed', fileId, error }) + return false + } }
86-87: Add radix parameter to Number.parseInt for clarity.For clarity and to avoid potential edge cases with number parsing, specify the radix explicitly:
- const contentLength = Number.parseInt(response.headers.get('content-length') || '0') + const contentLength = Number.parseInt(response.headers.get('content-length') || '0', 10)
80-88: Add timeout to prevent hanging requests.The fetch call lacks a timeout, which could cause the request to hang indefinitely. Consider adding a timeout for better reliability:
const url = await client.getPresignedUrl('HEAD', fileId) const response = await fetch(url, { method: 'HEAD', + signal: AbortSignal.timeout(10000), // 10 second timeout })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
supabase/functions/_backend/utils/s3.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
{src,supabase/functions/_backend}/**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
Enforce code style per @antfu/eslint-config: single quotes and no semicolons
Files:
supabase/functions/_backend/utils/s3.ts
supabase/functions/_backend/utils/**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Put shared utilities and database schemas under supabase/functions/_backend/utils/
Files:
supabase/functions/_backend/utils/s3.ts
supabase/functions/_backend/**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Write backend code as Deno-compatible Supabase Edge Functions
Files:
supabase/functions/_backend/utils/s3.ts
🧬 Code graph analysis (1)
supabase/functions/_backend/utils/s3.ts (1)
scripts/check_r2.ts (1)
initS3(255-279)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run tests
- GitHub Check: Analyze (java-kotlin)
|
2 participants
Summary by CodeRabbit
Bug Fixes
Refactor
Chores