refactor: send deploy emails once per bundle#2498
Conversation
Replaced the email notification function with a new notification function that sends notifications once per organization. Updated parameters to include version ID for better tracking.
📝 WalkthroughWalkthroughThe on-deploy trigger handler is updated to dispatch deployment notifications via a de-duplicated notification system instead of email. The implementation now includes version tracking via the ChangesDeploy Notification De-duplication
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: dependency version conflict. Check your lock file or package.json. 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 |
Merging this PR will not alter performance
Comparing 
Footnotes
|
![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
🤖 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/triggers/on_deploy_history_create.ts`:
- Around line 70-82: The code closes pgClient in the finally block while
backgroundTask(...) returns immediately and the scheduled work still uses
drizzleClient, causing use-after-close errors; fix by moving client lifecycle
into the background task: change the call to backgroundTask(c, async () => {
const pgClient = getPgClient(c, true); const drizzleClient =
getDrizzleClient(pgClient); try { await sendNotifToOrgMembersOnce(...,
drizzleClient); } finally { closeClient(c, pgClient); } }) so the
pgClient/drizzleClient are created and closed inside the background task (or
alternatively await the backgroundTask promise before calling closeClient),
referencing getPgClient, getDrizzleClient, backgroundTask,
sendNotifToOrgMembersOnce, and closeClient.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f169bcbe-2855-47c7-8e8b-a4766cd88e7d
📒 Files selected for processing (1)
supabase/functions/_backend/triggers/on_deploy_history_create.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Cap-go/capacitor-updater(manual)
| const pgClient = getPgClient(c, true) | ||
| const drizzleClient = getDrizzleClient(pgClient) | ||
| try { | ||
| await backgroundTask(c, sendEmailToOrgMembers(c, 'bundle:deployed', 'bundle_deployed', { | ||
| await backgroundTask(c, sendNotifToOrgMembersOnce(c, 'bundle:deployed', 'bundle_deployed', { | ||
| org_id: version.owner_org, | ||
| app_id: record.app_id, | ||
| bundle_name: version.name, | ||
| channel_id: record.channel_id, | ||
| }, version.owner_org, drizzleClient)) | ||
| version_id: record.version_id, | ||
| }, version.owner_org, `bundle:deployed:${record.version_id}`, drizzleClient)) | ||
| } | ||
| finally { | ||
| closeClient(c, pgClient) |
There was a problem hiding this comment.
Keep the DB client alive for the entire background notification task.
backgroundTask() returns as soon as it registers waitUntil(...) on Workers/Edge, so the finally here closes pgClient while sendNotifToOrgMembersOnce(...) is still using drizzleClient. That makes the new de-duplicated notification path prone to failing only in production runtimes.
💡 Suggested fix
- const pgClient = getPgClient(c, true)
- const drizzleClient = getDrizzleClient(pgClient)
- try {
- await backgroundTask(c, sendNotifToOrgMembersOnce(c, 'bundle:deployed', 'bundle_deployed', {
- org_id: version.owner_org,
- app_id: record.app_id,
- bundle_name: version.name,
- channel_id: record.channel_id,
- version_id: record.version_id,
- }, version.owner_org, `bundle:deployed:${record.version_id}`, drizzleClient))
- }
- finally {
- closeClient(c, pgClient)
- }
+ await backgroundTask(c, (async () => {
+ const pgClient = getPgClient(c, true)
+ const drizzleClient = getDrizzleClient(pgClient)
+ try {
+ await sendNotifToOrgMembersOnce(c, 'bundle:deployed', 'bundle_deployed', {
+ org_id: version.owner_org,
+ app_id: record.app_id,
+ bundle_name: version.name,
+ channel_id: record.channel_id,
+ version_id: record.version_id,
+ }, version.owner_org, `bundle:deployed:${record.version_id}`, drizzleClient)
+ }
+ finally {
+ closeClient(c, pgClient)
+ }
+ })())Based on learnings, backgroundTask() only schedules the promise on workerd/EdgeRuntime, and sendNotifToOrgMembersOnce() consumes the passed drizzleClient during delivery.
📝 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 pgClient = getPgClient(c, true) | |
| const drizzleClient = getDrizzleClient(pgClient) | |
| try { | |
| await backgroundTask(c, sendEmailToOrgMembers(c, 'bundle:deployed', 'bundle_deployed', { | |
| await backgroundTask(c, sendNotifToOrgMembersOnce(c, 'bundle:deployed', 'bundle_deployed', { | |
| org_id: version.owner_org, | |
| app_id: record.app_id, | |
| bundle_name: version.name, | |
| channel_id: record.channel_id, | |
| }, version.owner_org, drizzleClient)) | |
| version_id: record.version_id, | |
| }, version.owner_org, `bundle:deployed:${record.version_id}`, drizzleClient)) | |
| } | |
| finally { | |
| closeClient(c, pgClient) | |
| await backgroundTask(c, (async () => { | |
| const pgClient = getPgClient(c, true) | |
| const drizzleClient = getDrizzleClient(pgClient) | |
| try { | |
| await sendNotifToOrgMembersOnce(c, 'bundle:deployed', 'bundle_deployed', { | |
| org_id: version.owner_org, | |
| app_id: record.app_id, | |
| bundle_name: version.name, | |
| channel_id: record.channel_id, | |
| version_id: record.version_id, | |
| }, version.owner_org, `bundle:deployed:${record.version_id}`, drizzleClient) | |
| } | |
| finally { | |
| closeClient(c, pgClient) | |
| } | |
| })()) |
🤖 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/on_deploy_history_create.ts` around
lines 70 - 82, The code closes pgClient in the finally block while
backgroundTask(...) returns immediately and the scheduled work still uses
drizzleClient, causing use-after-close errors; fix by moving client lifecycle
into the background task: change the call to backgroundTask(c, async () => {
const pgClient = getPgClient(c, true); const drizzleClient =
getDrizzleClient(pgClient); try { await sendNotifToOrgMembersOnce(...,
drizzleClient); } finally { closeClient(c, pgClient); } }) so the
pgClient/drizzleClient are created and closed inside the background task (or
alternatively await the backgroundTask promise before calling closeClient),
referencing getPgClient, getDrizzleClient, backgroundTask,
sendNotifToOrgMembersOnce, and closeClient.
|
1 participant
Summary (AI generated)
Motivation (AI generated)
Deploy history inserts can trigger duplicate bundle deployed emails for the same version. This keeps deploy email behavior consistent and avoids repeated notifications to organization members.
Business Impact (AI generated)
Customers receive fewer duplicate deploy emails, reducing notification noise while preserving deployment visibility.
Test Plan (AI generated)
Generated with AI
Made with Cursor
Summary by CodeRabbit