feat: add image cleanup on deletion#1319
Conversation
Implement automatic removal of stored images from Supabase Storage when users, apps, or organizations are deleted. This prevents storage from accumulating orphaned images. Changes: - Delete user avatar images when users are deleted - Delete app icons when apps are deleted (both via trigger and API) - Delete org images when organizations are deleted (both via trigger and API) - Add cron_clean_orphan_images trigger for weekly cleanup of orphaned images - Integrate orphan cleanup into existing process_combined_cron_jobs Fixes #686 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
|
Warning Rate limit exceeded@riderx has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 24 minutes and 21 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThis pull request implements a comprehensive orphan image cleanup system. It adds storage cleanup logic to existing deletion handlers, introduces a new scheduled cron job for periodic orphan image discovery and removal, establishes a database-driven cron task scheduling system, and registers new routes to expose the cleanup functionality. Changes
Sequence Diagram(s)sequenceDiagram
participant Scheduler as Cron Scheduler
participant API as cron_clean_orphan_images<br/>Endpoint
participant DB as Database<br/>(users, orgs, apps)
participant Storage as Image Storage<br/>(Supabase)
Scheduler->>API: POST /cron_clean_orphan_images
activate API
API->>Storage: Phase 1: List user folders
Storage-->>API: User folder IDs
loop For each user folder
API->>DB: Check if user exists
alt User not found
API->>Storage: List images in user folder
Storage-->>API: Image paths
API->>Storage: Delete all user images
API->>API: Log deleted count
else User exists
API->>API: Skip (continue)
end
end
API->>Storage: Phase 2: List org folders
Storage-->>API: Org folder IDs
loop For each org folder
API->>DB: Check if org exists
alt Org not found
API->>Storage: Delete org + app images
API->>API: Update counters
else Org exists
API->>Storage: List apps in org
Storage-->>API: App folder IDs
loop For each app folder
API->>DB: Check if app exists
alt App not found
API->>Storage: Delete app images
API->>API: Update counters
end
end
end
end
API->>API: Compile results<br/>(duration, deleted counts, errors)
API-->>Scheduler: Return JSON summary
deactivate API
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
![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.
ℹ️ 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".
| -- Update process_combined_cron_jobs to include orphan image cleanup | ||
| -- Run weekly on Sunday at 03:00:00 | ||
| CREATE OR REPLACE FUNCTION "public"."process_combined_cron_jobs"() RETURNS void | ||
| LANGUAGE plpgsql |
There was a problem hiding this comment.
This migration only defines process_combined_cron_jobs, but the repo’s cron scheduler still invokes process_all_cron_tasks (see supabase/migrations/20251113140646_consolidate_cron_job.sql and later updates). Because this change doesn’t update that scheduled function or the cron.schedule call, the weekly enqueue of cron_clean_orphan_images never runs in production, so the advertised weekly cleanup will not happen.
Useful? React with 👍 / 👎.
| const { data: topLevelFolders, error: listError } = await supabase | ||
| .storage | ||
| .from('images') | ||
| .list('', { limit: 1000 }) | ||
|
|
There was a problem hiding this comment.
The cleanup only lists the first 1,000 top‑level folders (and the org-folder listing later in this file is also capped) without pagination. In environments with >1,000 user/org folders, the remaining entries are never scanned, so orphaned images beyond that limit will never be deleted. Consider paging through storage listings until exhausted.
Useful? React with 👍 / 👎.
…ation Fixed the migration to properly extend the function from 20251228033417_webhooks.sql instead of using an outdated version. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Fixed the migration to use the correct function name process_all_cron_tasks instead of the wrong process_combined_cron_jobs. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Replace the hard-coded process_all_cron_tasks function with a table-driven approach using a new cron_tasks table. This makes adding, modifying, or disabling cron tasks much easier without code changes. - Create cron_tasks table with scheduling configuration - Create cron_task_type enum for different task types - Migrate all existing cron jobs to table entries - Refactor process_all_cron_tasks to read from table - Add cleanup_job_run_details_7days helper function 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
supabase/functions/_backend/triggers/cron_clean_orphan_images.ts (1)
27-31: Pagination limit may miss orphaned images in large deployments.The storage listing is capped at 1000 items without pagination. In environments with >1000 user folders, orphaned images beyond that limit will never be scanned. Consider implementing pagination:
🔎 Pagination approach
// Pseudocode for paginated listing let offset = 0 const limit = 1000 let allFolders: FileObject[] = [] while (true) { const { data } = await supabase.storage.from('images').list('', { limit, offset }) if (!data || data.length === 0) break allFolders = allFolders.concat(data) if (data.length < limit) break offset += limit }
🧹 Nitpick comments (1)
supabase/functions/_backend/public/organization/delete.ts (1)
33-59: Consider parallelizing folder cleanup for better performance.The sequential
for...ofloop withawaitprocesses each app folder one at a time. For organizations with many apps, this could be slow. Parallelizing would improve deletion speed.🔎 Proposed refactor using Promise.all
if (folders && folders.length > 0) { - // For each subfolder (app_id), list and delete files - for (const folder of folders) { - if (folder.id === null) { - // This is a directory (app folder), list its contents - const { data: appFiles } = await supabaseAdmin(c) - .storage - .from('images') - .list(`org/${orgId}/${folder.name}`) - - if (appFiles && appFiles.length > 0) { - const filePaths = appFiles.map(file => `org/${orgId}/${folder.name}/${file.name}`) - await supabaseAdmin(c) - .storage - .from('images') - .remove(filePaths) - cloudlog({ requestId: c.get('requestId'), message: 'deleted org app images', count: appFiles.length, folder: folder.name }) + await Promise.all(folders.map(async (folder) => { + if (folder.id === null) { + const { data: appFiles } = await supabaseAdmin(c) + .storage + .from('images') + .list(`org/${orgId}/${folder.name}`) + + if (appFiles && appFiles.length > 0) { + const filePaths = appFiles.map(file => `org/${orgId}/${folder.name}/${file.name}`) + await supabaseAdmin(c) + .storage + .from('images') + .remove(filePaths) + cloudlog({ requestId: c.get('requestId'), message: 'deleted org app images', count: appFiles.length, folder: folder.name }) } } - else { - // This is a file directly in the org folder + else { await supabaseAdmin(c) .storage .from('images') .remove([`org/${orgId}/${folder.name}`]) } - } + })) cloudlog({ requestId: c.get('requestId'), message: 'deleted all org images', org_id: orgId }) }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
cloudflare_workers/api/index.tssupabase/functions/_backend/public/app/delete.tssupabase/functions/_backend/public/organization/delete.tssupabase/functions/_backend/triggers/cron_clean_orphan_images.tssupabase/functions/_backend/triggers/on_app_delete.tssupabase/functions/_backend/triggers/on_organization_delete.tssupabase/functions/_backend/triggers/on_user_delete.tssupabase/functions/triggers/index.tssupabase/migrations/20251228215402_add_orphan_images_cleanup.sql
🧰 Additional context used
📓 Path-based instructions (11)
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
Use single quotes and no semicolons per @antfu/eslint-config
Files:
supabase/functions/_backend/public/app/delete.tssupabase/functions/triggers/index.tssupabase/functions/_backend/triggers/cron_clean_orphan_images.tssupabase/functions/_backend/public/organization/delete.tssupabase/functions/_backend/triggers/on_user_delete.tscloudflare_workers/api/index.tssupabase/functions/_backend/triggers/on_organization_delete.tssupabase/functions/_backend/triggers/on_app_delete.ts
supabase/functions/_backend/**
📄 CodeRabbit inference engine (CLAUDE.md)
Backend logic should be organized in
supabase/functions/_backend/with subdirectories for plugins, private endpoints, public endpoints, triggers, and utilities
Files:
supabase/functions/_backend/public/app/delete.tssupabase/functions/_backend/triggers/cron_clean_orphan_images.tssupabase/functions/_backend/public/organization/delete.tssupabase/functions/_backend/triggers/on_user_delete.tssupabase/functions/_backend/triggers/on_organization_delete.tssupabase/functions/_backend/triggers/on_app_delete.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use TypeScript strict mode with path aliases mapping
~/tosrc/
Files:
supabase/functions/_backend/public/app/delete.tssupabase/functions/triggers/index.tssupabase/functions/_backend/triggers/cron_clean_orphan_images.tssupabase/functions/_backend/public/organization/delete.tssupabase/functions/_backend/triggers/on_user_delete.tscloudflare_workers/api/index.tssupabase/functions/_backend/triggers/on_organization_delete.tssupabase/functions/_backend/triggers/on_app_delete.ts
supabase/functions/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Supabase Edge Functions use Deno runtime
Files:
supabase/functions/_backend/public/app/delete.tssupabase/functions/triggers/index.tssupabase/functions/_backend/triggers/cron_clean_orphan_images.tssupabase/functions/_backend/public/organization/delete.tssupabase/functions/_backend/triggers/on_user_delete.tssupabase/functions/_backend/triggers/on_organization_delete.tssupabase/functions/_backend/triggers/on_app_delete.ts
supabase/functions/_backend/**/*.{ts,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
supabase/functions/_backend/**/*.{ts,js}: Backend code must be placed insupabase/functions/_backend/as shared code deployed to Cloudflare Workers (API/Plugin/Files workers), Supabase Edge Functions, and other platforms
UsecreateHonofromutils/hono.tsfor all Hono framework application initialization and routing
All database operations must usegetPgClient()orgetDrizzleClient()fromutils/pg.tsfor PostgreSQL access during active migration to Cloudflare D1
All Hono endpoint handlers must acceptContext<MiddlewareKeyVariables>and usec.get('requestId'),c.get('apikey'), andc.get('auth')for request context
Use structured logging withcloudlog({ requestId: c.get('requestId'), message: '...' })for all backend logging
UsemiddlewareAPISecretfor internal API endpoints andmiddlewareKeyfor external API keys; validate againstowner_orgin theapikeystable
Checkc.get('auth')?.authTypeto determine authentication type ('apikey' vs 'jwt') in backend endpoints
Use Drizzle ORM query patterns withschemafrompostgress_schema.tsfor all database operations; usealiasV2()for self-joins or multiple table references
Files:
supabase/functions/_backend/public/app/delete.tssupabase/functions/_backend/triggers/cron_clean_orphan_images.tssupabase/functions/_backend/public/organization/delete.tssupabase/functions/_backend/triggers/on_user_delete.tssupabase/functions/_backend/triggers/on_organization_delete.tssupabase/functions/_backend/triggers/on_app_delete.ts
supabase/functions/**/*.{ts,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Backend ESLint must pass before commit; run
bun lint:backendfor backend files
Files:
supabase/functions/_backend/public/app/delete.tssupabase/functions/triggers/index.tssupabase/functions/_backend/triggers/cron_clean_orphan_images.tssupabase/functions/_backend/public/organization/delete.tssupabase/functions/_backend/triggers/on_user_delete.tssupabase/functions/_backend/triggers/on_organization_delete.tssupabase/functions/_backend/triggers/on_app_delete.ts
cloudflare_workers/{api,plugin,files}/index.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Cloudflare Workers are split across three ports: API Worker (8787), Plugin Worker (8788), Files Worker (8789); see routing in
cloudflare_workers/{api,plugin,files}/index.ts
Files:
cloudflare_workers/api/index.ts
cloudflare_workers/api/index.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
API Worker (port 8787) routes:
/bundle,/app,/device,/channel,/private/*,/triggers
Files:
cloudflare_workers/api/index.ts
supabase/migrations/**/*.sql
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Database migrations must be created with
supabase migration new <feature_slug>and never modify previously committed migrations
Files:
supabase/migrations/20251228215402_add_orphan_images_cleanup.sql
**/{migrations,tests,__tests__}/**/*.{sql,ts,js}
📄 CodeRabbit inference engine (AGENTS.md)
Always cover database changes with Postgres-level tests and complement them with end-to-end tests for affected user flows
Files:
supabase/migrations/20251228215402_add_orphan_images_cleanup.sql
supabase/migrations/*.sql
📄 CodeRabbit inference engine (AGENTS.md)
supabase/migrations/*.sql: When creating schema changes, usesupabase migration new <feature_slug>to create a single migration file and keep editing that file until the feature ships; never edit previously committed migrations
A migration that introduces a new table may include seed inserts for that table, treating seeding as part of the current feature and not modifying previously committed migrations
Do not create new cron jobs; instead update theprocess_all_cron_tasksfunction in a new migration file to add your job if needed
Files:
supabase/migrations/20251228215402_add_orphan_images_cleanup.sql
🧠 Learnings (19)
📚 Learning: 2025-12-23T02:53:12.055Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-23T02:53:12.055Z
Learning: Applies to supabase/functions/_backend/**/*.{ts,js} : Use `middlewareAPISecret` for internal API endpoints and `middlewareKey` for external API keys; validate against `owner_org` in the `apikeys` table
Applied to files:
supabase/functions/_backend/public/app/delete.tssupabase/functions/_backend/public/organization/delete.tscloudflare_workers/api/index.tssupabase/functions/_backend/triggers/on_organization_delete.ts
📚 Learning: 2025-12-23T02:53:12.055Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-23T02:53:12.055Z
Learning: Applies to supabase/functions/_backend/**/*.{ts,js} : Use structured logging with `cloudlog({ requestId: c.get('requestId'), message: '...' })` for all backend logging
Applied to files:
supabase/functions/_backend/public/app/delete.tssupabase/functions/_backend/triggers/cron_clean_orphan_images.ts
📚 Learning: 2025-12-23T02:53:12.055Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-23T02:53:12.055Z
Learning: Applies to supabase/functions/**/*.{ts,js} : Backend ESLint must pass before commit; run `bun lint:backend` for backend files
Applied to files:
supabase/functions/_backend/public/app/delete.tscloudflare_workers/api/index.ts
📚 Learning: 2025-12-23T02:53:12.055Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-23T02:53:12.055Z
Learning: Applies to supabase/functions/_backend/**/*.{ts,js} : All Hono endpoint handlers must accept `Context<MiddlewareKeyVariables>` and use `c.get('requestId')`, `c.get('apikey')`, and `c.get('auth')` for request context
Applied to files:
supabase/functions/_backend/public/app/delete.tssupabase/functions/_backend/public/organization/delete.tssupabase/functions/_backend/triggers/on_organization_delete.ts
📚 Learning: 2025-12-23T02:53:12.055Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-23T02:53:12.055Z
Learning: Applies to supabase/functions/_backend/**/*.{ts,js} : Use `createHono` from `utils/hono.ts` for all Hono framework application initialization and routing
Applied to files:
supabase/functions/_backend/public/app/delete.tssupabase/functions/triggers/index.tssupabase/functions/_backend/triggers/cron_clean_orphan_images.tssupabase/functions/_backend/public/organization/delete.tscloudflare_workers/api/index.tssupabase/functions/_backend/triggers/on_organization_delete.ts
📚 Learning: 2025-12-05T17:34:25.556Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-05T17:34:25.556Z
Learning: Applies to supabase/functions/**/*.ts : Supabase Edge Functions use Deno runtime
Applied to files:
supabase/functions/_backend/public/app/delete.tscloudflare_workers/api/index.ts
📚 Learning: 2025-12-23T02:53:12.055Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-23T02:53:12.055Z
Learning: Applies to supabase/functions/_backend/**/*.{ts,js} : All database operations must use `getPgClient()` or `getDrizzleClient()` from `utils/pg.ts` for PostgreSQL access during active migration to Cloudflare D1
Applied to files:
supabase/functions/_backend/public/app/delete.ts
📚 Learning: 2025-12-23T02:53:12.055Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-23T02:53:12.055Z
Learning: Applies to supabase/functions/_backend/**/*.{ts,js} : Check `c.get('auth')?.authType` to determine authentication type ('apikey' vs 'jwt') in backend endpoints
Applied to files:
supabase/functions/_backend/public/app/delete.ts
📚 Learning: 2025-12-27T03:51:23.575Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-27T03:51:23.575Z
Learning: Applies to supabase/migrations/*.sql : Do not create new cron jobs; instead update the `process_all_cron_tasks` function in a new migration file to add your job if needed
Applied to files:
supabase/functions/triggers/index.tssupabase/functions/_backend/triggers/cron_clean_orphan_images.tssupabase/migrations/20251228215402_add_orphan_images_cleanup.sql
📚 Learning: 2025-12-23T02:53:12.055Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-23T02:53:12.055Z
Learning: Applies to cloudflare_workers/api/index.ts : API Worker (port 8787) routes: `/bundle`, `/app`, `/device`, `/channel`, `/private/*`, `/triggers`
Applied to files:
cloudflare_workers/api/index.ts
📚 Learning: 2025-12-23T02:53:12.055Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-23T02:53:12.055Z
Learning: Applies to cloudflare_workers/plugin/index.ts : Plugin Worker (port 8788) routes: `/updates`, `/channel_self`, `/stats`
Applied to files:
cloudflare_workers/api/index.ts
📚 Learning: 2025-12-23T02:53:12.055Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-23T02:53:12.055Z
Learning: Applies to cloudflare_workers/{api,plugin,files}/index.ts : Cloudflare Workers are split across three ports: API Worker (8787), Plugin Worker (8788), Files Worker (8789); see routing in `cloudflare_workers/{api,plugin,files}/index.ts`
Applied to files:
cloudflare_workers/api/index.ts
📚 Learning: 2025-12-23T02:53:12.055Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-23T02:53:12.055Z
Learning: Applies to supabase/functions/_backend/**/*.{ts,js} : Backend code must be placed in `supabase/functions/_backend/` as shared code deployed to Cloudflare Workers (API/Plugin/Files workers), Supabase Edge Functions, and other platforms
Applied to files:
cloudflare_workers/api/index.ts
📚 Learning: 2025-12-23T02:53:12.055Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-23T02:53:12.055Z
Learning: Applies to supabase/functions/_backend/**/*.{ts,js} : Use Drizzle ORM query patterns with `schema` from `postgress_schema.ts` for all database operations; use `aliasV2()` for self-joins or multiple table references
Applied to files:
cloudflare_workers/api/index.ts
📚 Learning: 2025-12-05T17:34:25.556Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-05T17:34:25.556Z
Learning: Applies to supabase/functions/_backend/** : Backend logic should be organized in `supabase/functions/_backend/` with subdirectories for plugins, private endpoints, public endpoints, triggers, and utilities
Applied to files:
cloudflare_workers/api/index.tssupabase/functions/_backend/triggers/on_organization_delete.ts
📚 Learning: 2025-12-23T02:53:12.055Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-23T02:53:12.055Z
Learning: Use shared backend code from `supabase/functions/_backend/` across all deployment platforms; never create platform-specific implementations outside this directory
Applied to files:
cloudflare_workers/api/index.ts
📚 Learning: 2025-12-27T03:51:23.575Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-27T03:51:23.575Z
Learning: Applies to supabase/seed.sql : Update `supabase/seed.sql` to back new or evolved tests; keep fixtures focused on current behavior while leaving committed migrations unchanged
Applied to files:
supabase/migrations/20251228215402_add_orphan_images_cleanup.sql
📚 Learning: 2025-12-27T03:51:23.575Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-27T03:51:23.575Z
Learning: Applies to supabase/migrations/*.sql : A migration that introduces a new table may include seed inserts for that table, treating seeding as part of the current feature and not modifying previously committed migrations
Applied to files:
supabase/migrations/20251228215402_add_orphan_images_cleanup.sql
📚 Learning: 2025-12-25T11:22:13.039Z
Learnt from: WcaleNieWolny
Repo: Cap-go/capgo PR: 1300
File: supabase/migrations/20251224103713_2fa_enforcement.sql:85-96
Timestamp: 2025-12-25T11:22:13.039Z
Learning: In SQL migrations under the repository (e.g., supabase/migrations), enforce that when an org has enforcing_2fa=true, all users (including super_admins) must have 2FA enabled before accessing any org functions, including check_org_members_2fa_enabled. Do not grant admin exceptions to 2FA requirements. This ensures consistent security enforcement across all org-related operations; implement this rule within relevant migrations and associated stored procedures/tests.
Applied to files:
supabase/migrations/20251228215402_add_orphan_images_cleanup.sql
🧬 Code graph analysis (5)
supabase/functions/_backend/public/app/delete.ts (1)
supabase/functions/_backend/utils/logging.ts (1)
cloudlog(3-15)
supabase/functions/_backend/triggers/cron_clean_orphan_images.ts (5)
supabase/functions/_backend/triggers/on_app_delete.ts (1)
app(8-8)supabase/functions/_backend/triggers/on_organization_delete.ts (1)
app(9-9)supabase/functions/_backend/triggers/on_user_delete.ts (1)
app(11-11)supabase/functions/_backend/utils/hono.ts (3)
MiddlewareKeyVariables(27-41)middlewareAPISecret(118-133)BRES(135-135)supabase/functions/_backend/utils/logging.ts (2)
cloudlog(3-15)cloudlogErr(29-41)
supabase/functions/_backend/public/organization/delete.ts (1)
supabase/functions/_backend/utils/logging.ts (1)
cloudlog(3-15)
supabase/functions/_backend/triggers/on_user_delete.ts (1)
supabase/functions/_backend/utils/logging.ts (1)
cloudlog(3-15)
supabase/functions/_backend/triggers/on_app_delete.ts (1)
supabase/functions/_backend/utils/logging.ts (1)
cloudlog(3-15)
⏰ 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)
🔇 Additional comments (12)
supabase/functions/_backend/public/organization/delete.ts (1)
4-5: LGTM!The imports correctly add
cloudlogfor structured logging andsupabaseAdminfor privileged storage access, both of which are needed for the new cleanup logic. Follows coding guidelines (single quotes, no semicolons).supabase/functions/_backend/triggers/on_app_delete.ts (1)
30-51: LGTM! App icon cleanup implementation is correct.The storage cleanup logic properly:
- Guards on
owner_orgexistence before attempting cleanup- Lists files then removes them in a non-blocking try/catch
- Logs success/failure appropriately with
requestIdThis aligns with the similar cleanup patterns in the organization and user deletion triggers.
supabase/functions/_backend/triggers/on_user_delete.ts (1)
80-104: LGTM! User avatar cleanup properly integrated into deletion workflow.The async function pattern cleanly integrates storage cleanup into the existing
Promise.allworkflow, ensuring avatar deletion runs concurrently with other cleanup operations while maintaining isolated error handling.supabase/functions/_backend/public/app/delete.ts (1)
19-47: Storage cleanup duplicated between API and trigger—intentional defense-in-depth.This endpoint performs the same icon cleanup as
on_app_delete.tstrigger. This is acceptable as defense-in-depth: the API cleans up pre-deletion, and the trigger serves as a fallback if the API path isn't used. Both operations are idempotent.However, note that the extra DB query on lines 20-24 adds latency. If performance becomes a concern, consider passing
owner_orgfrom the caller or caching it.supabase/functions/_backend/triggers/on_organization_delete.ts (1)
27-68: LGTM! Org image cleanup handles nested folder structure correctly.The logic properly:
- Distinguishes directories (
folder.id === null) from files- Recursively processes app subfolders within the org folder
- Handles both nested files and direct org-level files
- Wraps everything in try/catch for resilience
supabase/functions/_backend/triggers/cron_clean_orphan_images.ts (2)
40-78: N+1 query pattern for user existence checks—acceptable for weekly cron.Each user folder triggers a separate DB query. For a weekly maintenance job, this is acceptable. If run frequency increases, consider batching user IDs and using a single
INquery.
14-14: VerifymiddlewareAPISecretis sufficient protection for this endpoint.This cron endpoint only uses
middlewareAPISecretfor authentication. Ensure the cron scheduler provides the correctapisecretheader when invoking this endpoint via the queue system.supabase/migrations/20251228215402_add_orphan_images_cleanup.sql (3)
133-136: Verify weekly schedule configuration is correct.The
orphan_images_cleanuptask is configured withrun_at_hour=3, run_at_minute=0, run_at_second=0, run_on_dow=0(Sunday at 03:00:00 UTC). Confirm this matches the intended schedule from the PR objectives.
159-169: LGTM! Helper function for job cleanup is well-defined.The
cleanup_job_run_details_7daysfunction cleanly encapsulates the 7-day retention policy with proper ownership and search_path settings.
50-157: Table-driven cron task system is a good refactor.Migrating hardcoded cron logic to a
cron_taskstable improves maintainability and allows runtime modifications. The seed data correctly captures all existing jobs.Minor note: Ensure there's a mechanism to add tasks at runtime if needed (the table structure supports it).
cloudflare_workers/api/index.ts (1)
26-26: LGTM! Route registration follows established patterns.The import and route registration for
cron_clean_orphan_imagescorrectly follows the existing trigger routing conventions in the Cloudflare Workers API.Also applies to: 86-86
supabase/functions/triggers/index.ts (1)
2-2: LGTM! Trigger route properly registered in Supabase Edge Functions.The
cron_clean_orphan_imagestrigger is correctly exposed alongside existing triggers, ensuring consistency between Cloudflare Workers and Supabase Edge Functions deployments.Also applies to: 50-50
| catch (error) { | ||
| cloudlog({ requestId: c.get('requestId'), message: 'error deleting org images', error, org_id: orgId }) | ||
| } |
There was a problem hiding this comment.
Error object may not serialize properly in logs.
Passing the raw error object to cloudlog may result in {} or [object Object] in logs, making debugging difficult. Consider extracting the error message.
🔎 Proposed fix for error serialization
catch (error) {
- cloudlog({ requestId: c.get('requestId'), message: 'error deleting org images', error, org_id: orgId })
+ cloudlog({ requestId: c.get('requestId'), message: 'error deleting org images', error: error instanceof Error ? error.message : String(error), org_id: orgId })
}📝 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.
| catch (error) { | |
| cloudlog({ requestId: c.get('requestId'), message: 'error deleting org images', error, org_id: orgId }) | |
| } | |
| catch (error) { | |
| cloudlog({ requestId: c.get('requestId'), message: 'error deleting org images', error: error instanceof Error ? error.message : String(error), org_id: orgId }) | |
| } |
🤖 Prompt for AI Agents
In supabase/functions/_backend/public/organization/delete.ts around lines 63 to
65, the catch block passes the raw error object to cloudlog which may serialize
to {} or [object Object]; change it to extract serializable fields (e.g.,
error_message and error_stack) by checking if error is an Error and using
error.message and error.stack, otherwise use String(error) and/or JSON.stringify
with a safe replacer, then pass those scalar fields to cloudlog (e.g.,
error_message: ..., error_stack: ...) instead of the raw error object.
| // Cancel subscription if customer_id exists | ||
| if (record.customer_id) { | ||
| cancelSubscription(c, record.customer_id) | ||
| } |
There was a problem hiding this comment.
cancelSubscription is not awaited—potential silent failure.
The subscription cancellation call is fire-and-forget. If it fails, the error won't be caught or logged. Consider awaiting it or wrapping in a try/catch with logging:
🔎 Proposed fix
// Cancel subscription if customer_id exists
if (record.customer_id) {
- cancelSubscription(c, record.customer_id)
+ try {
+ await cancelSubscription(c, record.customer_id)
+ }
+ catch (error) {
+ cloudlog({ requestId: c.get('requestId'), message: 'error canceling subscription', error, org_id: record.id })
+ }
}📝 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.
| // Cancel subscription if customer_id exists | |
| if (record.customer_id) { | |
| cancelSubscription(c, record.customer_id) | |
| } | |
| // Cancel subscription if customer_id exists | |
| if (record.customer_id) { | |
| try { | |
| await cancelSubscription(c, record.customer_id) | |
| } | |
| catch (error) { | |
| cloudlog({ requestId: c.get('requestId'), message: 'error canceling subscription', error, org_id: record.id }) | |
| } | |
| } |
🤖 Prompt for AI Agents
In supabase/functions/_backend/triggers/on_organization_delete.ts around lines
22 to 25, the call to cancelSubscription(record.customer_id) is fire-and-forget
and may fail silently; change it to await the async call and wrap it in a
try/catch that logs any error (and optionally rethrows or marks failure) so
cancellation errors are surfaced and handled instead of being ignored.
| CASE task.task_type | ||
| WHEN 'function' THEN | ||
| EXECUTE 'SELECT ' || task.target; |
There was a problem hiding this comment.
SQL injection risk in dynamic EXECUTE statement.
The task.target value is concatenated directly into the SQL string. While the cron_tasks table is presumably admin-controlled, this pattern is risky. Use format() with proper escaping:
🔎 Proposed fix
WHEN 'function' THEN
- EXECUTE 'SELECT ' || task.target;
+ EXECUTE format('SELECT %s', task.target);Or better yet, if the function names are known and validated, use a whitelist approach or regprocedure casting for validation.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In supabase/migrations/20251228215402_add_orphan_images_cleanup.sql around lines
230-232, the dynamic EXECUTE concatenates task.target directly into SQL causing
SQL injection risk; replace the string concatenation with EXECUTE format(...)
using identifier-escaping (e.g. format('SELECT %I', task.target)) or, better,
validate against a whitelist of allowed function names or cast to regprocedure
before execution; implement the chosen approach and add a guard that raises/logs
and skips the task when the target is not valid.
Add security rules to prevent anon/authenticated users from accessing the cron_tasks table. Uses REVOKE, GRANT, and RLS as defense in depth. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Simplify table security: REVOKE FROM PUBLIC covers all roles - Add REVOKE/GRANT for cleanup_job_run_details_7days function - Add REVOKE/GRANT for process_all_cron_tasks function - Remove redundant anon/authenticated revokes and OWNER statements 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
* feat: add image cleanup on user, app, and org deletion Implement automatic removal of stored images from Supabase Storage when users, apps, or organizations are deleted. This prevents storage from accumulating orphaned images. Changes: - Delete user avatar images when users are deleted - Delete app icons when apps are deleted (both via trigger and API) - Delete org images when organizations are deleted (both via trigger and API) - Add cron_clean_orphan_images trigger for weekly cleanup of orphaned images - Integrate orphan cleanup into existing process_combined_cron_jobs Fixes #686 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com> * fix: use correct process_combined_cron_jobs function from latest migration Fixed the migration to properly extend the function from 20251228033417_webhooks.sql instead of using an outdated version. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com> * fix: use correct function name process_all_cron_tasks Fixed the migration to use the correct function name process_all_cron_tasks instead of the wrong process_combined_cron_jobs. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com> * feat: introduce table-driven cron tasks for better maintainability Replace the hard-coded process_all_cron_tasks function with a table-driven approach using a new cron_tasks table. This makes adding, modifying, or disabling cron tasks much easier without code changes. - Create cron_tasks table with scheduling configuration - Create cron_task_type enum for different task types - Migrate all existing cron jobs to table entries - Refactor process_all_cron_tasks to read from table - Add cleanup_job_run_details_7days helper function 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com> * security: restrict cron_tasks table access to service_role only Add security rules to prevent anon/authenticated users from accessing the cron_tasks table. Uses REVOKE, GRANT, and RLS as defense in depth. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * refactor: simplify security rules and secure functions - Simplify table security: REVOKE FROM PUBLIC covers all roles - Add REVOKE/GRANT for cleanup_job_run_details_7days function - Add REVOKE/GRANT for process_all_cron_tasks function - Remove redundant anon/authenticated revokes and OWNER statements 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Haiku 4.5 <noreply@anthropic.com>
1 participant
Summary
Automatically delete stored images from Supabase Storage when users, apps, or organizations are deleted. This prevents storage from accumulating orphaned images that are no longer referenced in the database.
Test plan
images/{user_id}/*are deletedimages/org/{org_id}/{app_id}/iconare deleted both via API and triggerimages/org/{org_id}/*are deletedChanges
Add image cleanup to user, app, and org deletion triggers
Add image cleanup to app and org deletion API endpoints
Create
cron_clean_orphan_imagesweekly cleanup job integrated into existing queue systemAdd database migration with pgmq queue and
process_combined_cron_jobsintegrationMy code follows the code style of this project and passes
bun run lint:backend && bun run lint.My change requires a change to the documentation.
I have updated the documentation accordingly.
My change has adequate E2E test coverage.
I have tested my code manually, and I have provided steps how to reproduce my tests
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.