[codex] stop repeated onboarding reminder emails#1891
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:
📝 WalkthroughWalkthroughAdds one-time, claim-based notification APIs and integrates per-org/per-recipient "send once" delivery; refactors org email target selection and updates onboarding notification flow and tests. Changes
Sequence DiagramsequenceDiagram
participant Caller as Caller
participant API as sendNotifOrgOnce()
participant Insert as insertNotificationClaim()
participant DB as Database
participant Track as trackBentoEvent()
participant Delete as deleteNotificationClaim()
Caller->>API: sendNotifOrgOnce(eventName, eventData, orgId, uniqId, recipient)
API->>Insert: attempt insertNotificationClaim(orgId, eventName, uniqId)
Insert->>DB: INSERT ... ON CONFLICT DO NOTHING RETURNING
DB-->>Insert: inserted[] (0 or 1 row)
alt claim existed
Insert-->>API: false (no insert)
API-->>Caller: false (skip send)
else new claim created
Insert-->>API: true
API->>Track: trackBentoEvent(eventData, recipient, ...)
Track-->>API: success / failure
alt track success
API-->>Caller: true
else track failure
API->>Delete: deleteNotificationClaim(orgId, eventName, uniqId)
Delete->>DB: DELETE WHERE owner_org=... AND event=... AND uniq_id=...
DB-->>Delete: deleted count
API-->>Caller: false
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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: b0e39c94dd
ℹ️ 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".
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5a0738391
ℹ️ 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: fea65b410c
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@supabase/functions/_backend/utils/notifications.ts`:
- Around line 266-299: sendNotifOrgOnce currently claims the one-time notif via
claimNotifOrgOnce before calling trackBentoEvent but only deletes the claim when
trackBentoEvent returns a falsy result; if trackBentoEvent throws the catch path
never rolls back the claim. Modify sendNotifOrgOnce so any exception from
trackBentoEvent also deletes the claim: wrap the trackBentoEvent call in its own
try/catch (or add a finally) that on error constructs a cleanup client via
createDrizzleClient(getPgClient(c)), calls
deleteNotificationClaim(cleanupClient, eventName, orgId, uniqId), logs any
cleanup error with logPgError, then rethrows or returns false as appropriate;
keep existing cloudlog/logPgError usage and ensure the same cleanup runs for
thrown errors as for falsy responses.
In `@supabase/functions/_backend/utils/org_email_notifications.ts`:
- Around line 594-623: The current code calls claimNotifOrgOnce() as soon as any
recipient succeeds, which prevents retries for failed addresses; modify the flow
so claimNotifOrgOnce(eventName, orgId, uniqId) is only invoked when every
recipient was delivered (i.e., sentEmails.length === recipientEmails.length). If
only a partial set succeeded, do not call claimNotifOrgOnce; instead log the
partial delivery (include sentEmails and failed count) and return false so
remaining recipient uniq IDs can be retried later. Keep existing
variables/functions names: recipientEmails, sendResults, sentEmails,
claimNotifOrgOnce, and managementEmail for context.
🪄 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: 72fa3215-f59f-4e47-b664-7bf7cfb5ae0b
📒 Files selected for processing (4)
.github/workflows/tests.ymlsupabase/functions/_backend/utils/notifications.tssupabase/functions/_backend/utils/org_email_notifications.tstests/org-email-notifications-send-once.unit.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/tests.yml
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa0c2bc57f
ℹ️ 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
♻️ Duplicate comments (1)
supabase/functions/_backend/utils/org_email_notifications.ts (1)
594-638:⚠️ Potential issue | 🟠 MajorDon't finalize the org-level claim after a partial recipient send.
sendNotifOrgOnce()on Line 599 returnsfalseboth for already-claimed recipients and for delivery failures. Because Lines 607-620 only re-check claim state whensentEmails.length === 0, a run with one success and one failed/unclaimed recipient still reaches Line 622 and suppresses retries for the failed address on the next invocation. The org claim should only be written once every recipient is either newly sent or already claimed.Suggested fix
const sentEmails = sendResults .filter(result => result.sent) .map(result => result.email) - let allRecipientsAlreadyClaimed = false - if (sentEmails.length === 0) { - allRecipientsAlreadyClaimed = true - for (const result of sendResults) { + let allRecipientsFinalized = true + for (const result of sendResults) { + if (result.sent) + continue + const recipientAlreadyClaimed = await hasNotifOrgClaim(c, drizzleClient, eventName, orgId, result.recipientUniqId) if (!recipientAlreadyClaimed) { - allRecipientsAlreadyClaimed = false + allRecipientsFinalized = false break } - } - } - - if (sentEmails.length === 0 && !allRecipientsAlreadyClaimed) + } + + if (!allRecipientsFinalized) return false @@ - allRecipientsAlreadyClaimed, + allRecipientsFinalized, firstOrgSend, @@ - return sentEmails.length > 0 ? firstOrgSend : allRecipientsAlreadyClaimed + return sentEmails.length > 0 ? firstOrgSend : allRecipientsFinalized🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_backend/utils/org_email_notifications.ts` around lines 594 - 638, The org-level claim is being written even when some recipients failed delivery because sendNotifOrgOnce() returns false for both "already claimed" and delivery failure; update the post-send logic so you only call claimNotifOrgOnce() when every recipient is either successfully sent or already claimed: after the send loop (which uses buildOneTimeRecipientNotifUniqId and sendNotifOrgOnce), for each result with sent === false call hasNotifOrgClaim(c, drizzleClient, eventName, orgId, recipientUniqId) and if any of those return false, do not call claimNotifOrgOnce and return false; only when all non-sent results are already claimed should you call claimNotifOrgOnce(...) and proceed to log/return as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/org-email-notifications-send-once.unit.test.ts`:
- Around line 104-168: The two tests using it.concurrent() ("does not send
recipient notifications when the org-level claim already exists" and "backfills
the org-level claim once all recipient claims already exist") race on the shared
vi.hoisted() mocks and beforeEach() clears; change those two tests from
it.concurrent(...) to plain sequential it(...) so their mock setups
(vi.clearAllMocks(), hasNotifOrgClaimMock overrides, sendNotifOrgOnceMock,
claimNotifOrgOnceMock) do not interleave and assertions remain deterministic.
---
Duplicate comments:
In `@supabase/functions/_backend/utils/org_email_notifications.ts`:
- Around line 594-638: The org-level claim is being written even when some
recipients failed delivery because sendNotifOrgOnce() returns false for both
"already claimed" and delivery failure; update the post-send logic so you only
call claimNotifOrgOnce() when every recipient is either successfully sent or
already claimed: after the send loop (which uses
buildOneTimeRecipientNotifUniqId and sendNotifOrgOnce), for each result with
sent === false call hasNotifOrgClaim(c, drizzleClient, eventName, orgId,
recipientUniqId) and if any of those return false, do not call claimNotifOrgOnce
and return false; only when all non-sent results are already claimed should you
call claimNotifOrgOnce(...) and proceed to log/return as before.
🪄 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: 2c885a28-ac13-4439-8a77-671152fea72d
📒 Files selected for processing (3)
supabase/functions/_backend/utils/notifications.tssupabase/functions/_backend/utils/org_email_notifications.tstests/org-email-notifications-send-once.unit.test.ts
✅ Files skipped from review due to trivial changes (1)
- supabase/functions/_backend/utils/notifications.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a3573182b4
ℹ️ 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: dd1dc6c2ec
ℹ️ 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: 8db3c01d05
ℹ️ 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: 5d1131bb84
ℹ️ 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: db89fce884
ℹ️ 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: 1f55b42ed2
ℹ️ 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".
| const unresolvedResults = sendResults.filter(result => !result.sent && !result.wasAlreadyClaimedBeforeRun) | ||
| if (unresolvedResults.length > 0) | ||
| return false | ||
|
|
||
| const firstOrgSend = await claimNotifOrgOnce(c, eventName, orgId, uniqId, writeClient) |
There was a problem hiding this comment.
This completion gate treats any wasAlreadyClaimedBeforeRun recipient as safely delivered, so a stale claim left behind by an earlier cleanupFailed path can cause a false success on a later run: unresolvedResults becomes empty and claimNotifOrgOnce(...) is executed even though that recipient never received the email. In that scenario the org-level once-claim is persisted and future retries are suppressed for the missed recipient.
Useful? React with 👍 / 👎.
1 participant
Summary (AI generated)
Motivation (AI generated)
The onboarding reminder for organizations whose trial expired but never completed onboarding was using a recurring notification path. That let the same organization receive the same reminder again over time, which created a loop of alarming emails. The reminder also lacked enough org-specific context for users who belong to multiple organizations.
Business Impact (AI generated)
This reduces avoidable support noise and user anxiety from repeated or ambiguous billing/onboarding emails. It keeps onboarding messaging accurate per organization, which should improve trust in Capgo's operational emails and lower the chance that users ignore legitimate account reminders.
Test Plan (AI generated)
bunx eslint supabase/functions/_backend/utils/notifications.ts supabase/functions/_backend/utils/org_email_notifications.ts supabase/functions/_backend/utils/plans.ts tests/org-email-notifications.unit.test.ts tests/plans-onboarding-reminder.unit.test.tsbunx vitest run tests/org-email-notifications.unit.test.ts tests/plans-onboarding-reminder.unit.test.tsGenerated with AI
Summary by CodeRabbit
New Features
Improvements
Tests
Chores