fix(plugin): queue notification writes outside request path#2478
Open
riderx wants to merge 17 commits into
Open
fix(plugin): queue notification writes outside request path#2478riderx wants to merge 17 commits into
riderx wants to merge 17 commits into
Conversation
Contributor
|
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 a KV-backed plugin notification queue, a trigger POST to process queued items, a scheduled worker flush invoking the trigger, context flags and predicates to gate Supabase fallback writes and require read-replicas, local read-replica env routing, Cloudflare middleware wiring, and aligned unit/integration tests with Cloudflare gating. ChangesPlugin notification queueing system
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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. Comment |
Contributor
Merging this PR will not alter performance
Comparing 
Footnotes
|
riderx
force-pushed
the
codex/plugin-notification-kv-queue
branch
from
e08c0e3 to
492da16
Compare
riderx
force-pushed
the
codex/plugin-notification-kv-queue
branch
from
492da16 to
2f3da4c
Compare
riderx
force-pushed
the
codex/plugin-notification-kv-queue
branch
from
2f3da4c to
51eac6b
Compare
riderx
force-pushed
the
codex/plugin-notification-kv-queue
branch
from
51eac6b to
c1378ed
Compare
riderx
force-pushed
the
codex/plugin-notification-kv-queue
branch
from
c1378ed to
e0d84d8
Compare
![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: e0d84d859b
ℹ️ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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 `@cloudflare_workers/api/index.ts`:
- Line 65: The imported alias plugin_notifications should be renamed to
camelCase (e.g., pluginNotifications) and all its references updated: change the
import statement importing app from
'../../supabase/functions/_backend/triggers/plugin_notifications.ts' to use
pluginNotifications, and update where it is wired into appTriggers (the
registration that still uses plugin_notifications and the route
'/plugin_notifications') to reference pluginNotifications while leaving the
route path string unchanged.
In `@cloudflare_workers/plugin/index.ts`:
- Around line 41-54: Scheduled flushes can overlap and cause duplicate
deliveries because flushQueuedPluginNotifications deletes KV only after
postPluginNotificationBatch succeeds; add a single-flight guard or server-side
dedupe. Wrap the scheduled handler invocation of flushQueuedPluginNotifications
(in scheduled() where you call scheduledApp.fetch) with a lock: obtain a
short-lived lock (preferably via a Durable Object or by setting a KV lock key
with a TTL and failing fast if it exists) before running
flushQueuedPluginNotifications, and release it after completion; alternatively
implement idempotent delivery inside
flushQueuedPluginNotifications/postPluginNotificationBatch by writing a
processed-set entry per queue key/uniqId (skip if already present) before
sending. Ensure the lock key and the processed-set use unique names and short
TTLs to avoid deadlocks and reference flushQueuedPluginNotifications,
postPluginNotificationBatch, scheduledApp and the scheduled() handler when
making changes.
In `@supabase/functions/_backend/plugins/channel_self.ts`:
- Around line 615-618: The early return inside the branch checking
canUseReadReplica && shouldSkipChannelSelfPostgresFallback(c) skips the same
device/IP rate-limit counter updates that run in the finally block of
runChannelSelfWithPgClient; move or duplicate the counter-update logic to
execute before calling logSkippedSupabaseWrite and simpleError200 so counters
are incremented for this path too. Concretely, call the same helper that updates
device/IP counters (or extract that logic from runChannelSelfWithPgClient's
finally into a shared function) and invoke it here prior to returning, while
keeping logSkippedSupabaseWrite and simpleError200 as-is.
In `@supabase/functions/_backend/utils/notifications.ts`:
- Around line 275-276: The fallback branch creates a new PG client via
getPgClient(c) wrapped by createDrizzleClient but never closes it, leaking
connections; update the code around effectiveWriteClient and the call to
insertNotificationClaim so that when writeClient is undefined you create the
client, call insertNotificationClaim inside a try block, and always close the
internally-created PG client in finally (i.e., close the pool returned by
getPgClient or call the appropriate teardown on the created drizzle client);
apply the same try/finally pattern to the second occurrence around the other
effectiveWriteClient/createDrizzleClient use at the later site.
In `@supabase/functions/_backend/utils/plugin_notification_flush.ts`:
- Around line 54-127: SonarCloud flags flushQueuedPluginNotifications for high
cognitive complexity; extract the inner batch processing (the block that handles
items array: calling postPluginNotificationBatch, deleting keys on success,
incrementing transferred/failed counts, and catching/logging errors) into a new
helper function processBatch(c: Context, store: KVNamespace, items:
PluginNotificationQueueItem[], itemKeys: string[]): Promise<{
transferred:number; deleted:number; failed:number }>, and call it from the loop
in flushQueuedPluginNotifications; keep existing calls to
parsePluginNotificationQueueItem, postPluginNotificationBatch and
cloudlogErr/cloudlog but move the try/catch and counter updates into
processBatch so the main loop only handles listing, reading, parsing, pushing
items/itemKeys, and aggregating results from processBatch.
In `@supabase/functions/_backend/utils/plugin_notification_queue.ts`:
- Around line 90-110: queuePluginOrgMembersNotification currently takes 8 scalar
parameters (c, eventName, preferenceKey, eventData, orgId, uniqId, cron,
audience), which exceeds the recommended parameter count; refactor its signature
to accept an options object as the second parameter (e.g., options: { eventName,
preferenceKey, eventData, orgId, uniqId, cron, audience }) and update the
function body to read those properties (and pass them into
enqueuePluginNotification either by spreading options or by explicit fields) to
reduce parameter count; note you must also update all call sites to construct
and pass the new options object or defer this change if broader refactoring
across notification helpers is not feasible now.
- Around line 60-63: Remove the unnecessary type assertion on payload: when
constructing const payload = { ...item, enqueuedAt: new Date().toISOString() },
drop the trailing "as PluginNotificationQueueItem" since TypeScript can infer
the discriminated union from item (which already contains type: 'org' |
'org_members') and the added enqueuedAt field; update the payload declaration to
use the plain object and ensure the compiler still type-checks in the
surrounding code that uses payload (references: payload, item, enqueuedAt,
PluginNotificationQueueItem).
In `@supabase/functions/_backend/utils/supabase_write_guard.ts`:
- Around line 15-22: The getBooleanContextFlag function uses an explicit type
assertion on the Context parameter which is unnecessary; remove the long cast
"(c as Context & { get: (key: string) => unknown })" and call c.get(...)
directly (or cast the key to any) inside the existing try/catch so the runtime
behavior stays the same and TypeScript is satisfied; update the function body to
use c.get(key) === true while keeping the catch branch returning false.
In `@tests/stats-download.test.ts`:
- Line 12: The comment above the USE_CLOUDFLARE flag in
tests/stats-download.test.ts is inaccurate; replace or remove the line
referencing "serialize paths for consistency" so it correctly states that
USE_CLOUDFLARE is used to conditionally skip tests that expect primary database
writes (or simply note it toggles Cloudflare-worker behavior), and ensure the
comment directly references the USE_CLOUDFLARE flag so future readers understand
its purpose when reading the test file.
🪄 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: d1a6fe45-caac-4a5a-a4ab-8653bb41252c
📒 Files selected for processing (22)
cloudflare_workers/api/index.tscloudflare_workers/plugin/index.tscloudflare_workers/plugin/wrangler.jsoncscripts/start-cloudflare-workers.shsupabase/functions/_backend/plugins/channel_self.tssupabase/functions/_backend/plugins/stats.tssupabase/functions/_backend/triggers/plugin_notifications.tssupabase/functions/_backend/utils/cloudflare.tssupabase/functions/_backend/utils/hono.tssupabase/functions/_backend/utils/notifications.tssupabase/functions/_backend/utils/org_email_notifications.tssupabase/functions/_backend/utils/pg.tssupabase/functions/_backend/utils/plugin_notification_flush.tssupabase/functions/_backend/utils/plugin_notification_queue.tssupabase/functions/_backend/utils/stats.tssupabase/functions/_backend/utils/supabase_write_guard.tstests/channel_self.test.tstests/key_id_e2e.test.tstests/plugin-supabase-write-guard.unit.test.tstests/stats-download.test.tstests/stats.test.tstests/updates.test.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Cap-go/capacitor-updater(manual)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a6902d91a
ℹ️ 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: 726d83be83
ℹ️ 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: 3314d7eeed
ℹ️ 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: 1e1cfdb466
ℹ️ 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".
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
supabase/functions/_backend/utils/org_email_notifications.ts (1)
593-719:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClose the locally-created write client in
sendNotifToOrgMembersOnce.Line 593 creates a new PG client for this helper, but the function never closes it. Because this path has many early returns, every normal invocation can leak the caller-owned pool/connection unless you mirror the
try/finallycleanup pattern already added innotifications.ts.Suggested fix
-import { getDrizzleClient, getPgClient, logPgError } from './pg.ts' +import { closeClient, getDrizzleClient, getPgClient, logPgError } from './pg.ts'- const writeClient = getDrizzleClient(getPgClient(c)) - const alreadySentForOrg = await hasNotifOrgClaim(c, writeClient, eventName, orgId, uniqId) - if (alreadySentForOrg === null) { - cloudlog({ - requestId: c.get('requestId'), - message: 'sendNotifToOrgMembersOnce: org claim lookup failed', - eventName, - preferenceKey, - orgId, - uniqId, - }) - return false - } - if (alreadySentForOrg) { - cloudlog({ - requestId: c.get('requestId'), - message: 'sendNotifToOrgMembersOnce: org already claimed', - eventName, - preferenceKey, - orgId, - uniqId, - }) - return false - } - - const { recipients, resolutionFailed } = await getPreparedEligibleEmailTargets(c, orgId, preferenceKey, writeClient, audience) - if (!recipients) { - cloudlog({ requestId: c.get('requestId'), message: 'sendNotifToOrgMembersOnce: org not found', orgId }) - return false - } - if (resolutionFailed) { - cloudlog({ - requestId: c.get('requestId'), - message: 'sendNotifToOrgMembersOnce: recipient resolution failed', - eventName, - preferenceKey, - orgId, - }) - return false - } - - const { managementEmail, allEmails, primaryEmail, additionalEmails } = recipients - if (allEmails.length === 0 || !primaryEmail) { - cloudlog({ - requestId: c.get('requestId'), - message: 'sendNotifToOrgMembersOnce: no eligible recipients', - eventName, - preferenceKey, - orgId, - }) - return false - } - - const recipientEmails = [primaryEmail, ...additionalEmails] - const recipientEntries: { email: string, recipientUniqId: string, wasAlreadyClaimedBeforeRun: boolean }[] = [] - for (const email of recipientEmails) { - const recipientUniqId = await buildOneTimeRecipientNotifUniqId(uniqId, email) - const wasAlreadyClaimedBeforeRun = await hasNotifOrgClaim(c, writeClient, eventName, orgId, recipientUniqId) - if (wasAlreadyClaimedBeforeRun === null) { - cloudlog({ - requestId: c.get('requestId'), - message: 'sendNotifToOrgMembersOnce: recipient claim lookup failed', - eventName, - preferenceKey, - orgId, - recipientUniqId, - }) - return false - } - recipientEntries.push({ email, recipientUniqId, wasAlreadyClaimedBeforeRun }) - } - - const sendResults: { cleanupFailed: boolean, email: string, recipientUniqId: string, sent: boolean, wasAlreadyClaimedBeforeRun: boolean }[] = [] - for (const recipient of recipientEntries) { - if (recipient.wasAlreadyClaimedBeforeRun) { - sendResults.push({ ...recipient, sent: false, cleanupFailed: false }) - continue - } - - const sendResult = await sendNotifOrgOnce(c, eventName, eventData, orgId, recipient.recipientUniqId, recipient.email, drizzleClient, writeClient) - sendResults.push({ ...recipient, ...sendResult }) - } - - const sentEmails = sendResults - .filter(result => result.sent) - .map(result => result.email) - const cleanupFailedEmails = sendResults - .filter(result => result.cleanupFailed) - .map(result => result.email) - const alreadyClaimedBeforeRunEmails = sendResults - .filter(result => result.wasAlreadyClaimedBeforeRun) - .map(result => result.email) - - if (cleanupFailedEmails.length > 0) { - cloudlog({ - requestId: c.get('requestId'), - message: 'sendNotifToOrgMembersOnce: recipient cleanup failed', - eventName, - preferenceKey, - orgId, - cleanupFailedRecipients: cleanupFailedEmails, - }) - return false - } - - const unresolvedResults = sendResults.filter(result => !result.sent && !result.wasAlreadyClaimedBeforeRun) - if (unresolvedResults.length > 0) - return false - - const firstOrgSend = await claimNotifOrgOnce(c, eventName, orgId, uniqId, writeClient) - - cloudlog({ - requestId: c.get('requestId'), - message: 'sendNotifToOrgMembersOnce: delivered', - eventName, - preferenceKey, - orgId, - primaryEmail, - additionalRecipients: additionalEmails.length, - deliveredRecipients: sentEmails.length, - alreadyClaimedRecipients: alreadyClaimedBeforeRunEmails.length, - firstOrgSend, - managementEmailIncluded: !!managementEmail, - }) - - return firstOrgSend + const ownedPgClient = getPgClient(c) + const writeClient = getDrizzleClient(ownedPgClient) + try { + const alreadySentForOrg = await hasNotifOrgClaim(c, writeClient, eventName, orgId, uniqId) + if (alreadySentForOrg === null) { + cloudlog({ + requestId: c.get('requestId'), + message: 'sendNotifToOrgMembersOnce: org claim lookup failed', + eventName, + preferenceKey, + orgId, + uniqId, + }) + return false + } + if (alreadySentForOrg) { + cloudlog({ + requestId: c.get('requestId'), + message: 'sendNotifToOrgMembersOnce: org already claimed', + eventName, + preferenceKey, + orgId, + uniqId, + }) + return false + } + + const { recipients, resolutionFailed } = await getPreparedEligibleEmailTargets(c, orgId, preferenceKey, writeClient, audience) + if (!recipients) { + cloudlog({ requestId: c.get('requestId'), message: 'sendNotifToOrgMembersOnce: org not found', orgId }) + return false + } + if (resolutionFailed) { + cloudlog({ + requestId: c.get('requestId'), + message: 'sendNotifToOrgMembersOnce: recipient resolution failed', + eventName, + preferenceKey, + orgId, + }) + return false + } + + const { managementEmail, allEmails, primaryEmail, additionalEmails } = recipients + if (allEmails.length === 0 || !primaryEmail) { + cloudlog({ + requestId: c.get('requestId'), + message: 'sendNotifToOrgMembersOnce: no eligible recipients', + eventName, + preferenceKey, + orgId, + }) + return false + } + + const recipientEmails = [primaryEmail, ...additionalEmails] + const recipientEntries: { email: string, recipientUniqId: string, wasAlreadyClaimedBeforeRun: boolean }[] = [] + for (const email of recipientEmails) { + const recipientUniqId = await buildOneTimeRecipientNotifUniqId(uniqId, email) + const wasAlreadyClaimedBeforeRun = await hasNotifOrgClaim(c, writeClient, eventName, orgId, recipientUniqId) + if (wasAlreadyClaimedBeforeRun === null) { + cloudlog({ + requestId: c.get('requestId'), + message: 'sendNotifToOrgMembersOnce: recipient claim lookup failed', + eventName, + preferenceKey, + orgId, + recipientUniqId, + }) + return false + } + recipientEntries.push({ email, recipientUniqId, wasAlreadyClaimedBeforeRun }) + } + + const sendResults: { cleanupFailed: boolean, email: string, recipientUniqId: string, sent: boolean, wasAlreadyClaimedBeforeRun: boolean }[] = [] + for (const recipient of recipientEntries) { + if (recipient.wasAlreadyClaimedBeforeRun) { + sendResults.push({ ...recipient, sent: false, cleanupFailed: false }) + continue + } + + const sendResult = await sendNotifOrgOnce(c, eventName, eventData, orgId, recipient.recipientUniqId, recipient.email, drizzleClient, writeClient) + sendResults.push({ ...recipient, ...sendResult }) + } + + const sentEmails = sendResults + .filter(result => result.sent) + .map(result => result.email) + const cleanupFailedEmails = sendResults + .filter(result => result.cleanupFailed) + .map(result => result.email) + const alreadyClaimedBeforeRunEmails = sendResults + .filter(result => result.wasAlreadyClaimedBeforeRun) + .map(result => result.email) + + if (cleanupFailedEmails.length > 0) { + cloudlog({ + requestId: c.get('requestId'), + message: 'sendNotifToOrgMembersOnce: recipient cleanup failed', + eventName, + preferenceKey, + orgId, + cleanupFailedRecipients: cleanupFailedEmails, + }) + return false + } + + const unresolvedResults = sendResults.filter(result => !result.sent && !result.wasAlreadyClaimedBeforeRun) + if (unresolvedResults.length > 0) + return false + + const firstOrgSend = await claimNotifOrgOnce(c, eventName, orgId, uniqId, writeClient) + + cloudlog({ + requestId: c.get('requestId'), + message: 'sendNotifToOrgMembersOnce: delivered', + eventName, + preferenceKey, + orgId, + primaryEmail, + additionalRecipients: additionalEmails.length, + deliveredRecipients: sentEmails.length, + alreadyClaimedRecipients: alreadyClaimedBeforeRunEmails.length, + firstOrgSend, + managementEmailIncluded: !!managementEmail, + }) + + return firstOrgSend + } + finally { + await closeClient(c, ownedPgClient) + }🤖 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/utils/org_email_notifications.ts` around lines 593 - 719, sendNotifToOrgMembersOnce creates a local PG client via getDrizzleClient(getPgClient(c)) stored in writeClient but never closes it on early returns; wrap the body of sendNotifToOrgMembersOnce in a try/finally (or ensure a finally) that calls writeClient.release() / writeClient.end() (matching how other helpers in notifications.ts clean up the client) so the client is always closed after use, and keep all existing early-return checks (hasNotifOrgClaim, getPreparedEligibleEmailTargets, recipient loop, cleanup failures, etc.) inside the try block; reference functions/variables: sendNotifToOrgMembersOnce, getDrizzleClient, getPgClient, writeClient, and claimNotifOrgOnce.
🤖 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 `@cloudflare_workers/api/index.ts`:
- Around line 191-194: The scheduled handler currently calls appScheduled.fetch
inside ctx.waitUntil without inspecting the returned Response; update the
scheduled(_controller, env, ctx) flow to await or then-check the fetch Response
from appScheduled.fetch and inspect response.ok and the JSON payload for the
flush result (the endpoint `/flush-plugin-notifications` returns a body that
includes the result of flushQueuedPluginNotifications with a failed count). If
response.ok is false or the parsed JSON indicates failed > 0 (or an explicit
failure status), log/error or throw so the failure is surfaced (and ensure
ctx.waitUntil receives a rejected promise on failure), referencing the scheduled
function and appScheduled.fetch and the flushQueuedPluginNotifications result
payload to find the relevant code to change.
In `@tests/plugin-supabase-write-guard.unit.test.ts`:
- Around line 55-58: The helper readJsoncConfig uses a regex that only strips
full-line // comments and will fail if block comments (/* */) or inline trailing
// are introduced; update readJsoncConfig to use a robust JSONC parser (e.g.,
import and use jsonc-parser's parse or use `@types/jsonc`) to parse the file
contents, or at minimum improve the stripping logic to remove /*...*/ and inline
// comments before JSON.parse; locate the function readJsoncConfig in the test
file and replace the current raw.replace+JSON.parse approach with a
jsonc-parser-based parse call (or an enhanced regex strip) so future comments in
wrangler.jsonc are handled safely.
---
Duplicate comments:
In `@supabase/functions/_backend/utils/org_email_notifications.ts`:
- Around line 593-719: sendNotifToOrgMembersOnce creates a local PG client via
getDrizzleClient(getPgClient(c)) stored in writeClient but never closes it on
early returns; wrap the body of sendNotifToOrgMembersOnce in a try/finally (or
ensure a finally) that calls writeClient.release() / writeClient.end() (matching
how other helpers in notifications.ts clean up the client) so the client is
always closed after use, and keep all existing early-return checks
(hasNotifOrgClaim, getPreparedEligibleEmailTargets, recipient loop, cleanup
failures, etc.) inside the try block; reference functions/variables:
sendNotifToOrgMembersOnce, getDrizzleClient, getPgClient, writeClient, and
claimNotifOrgOnce.
🪄 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: dfa7d8bb-9280-48cb-bb5a-44035a79ab62
📒 Files selected for processing (7)
cloudflare_workers/api/index.tscloudflare_workers/api/wrangler.jsonccloudflare_workers/plugin/index.tscloudflare_workers/plugin/wrangler.jsoncsupabase/functions/_backend/utils/notifications.tssupabase/functions/_backend/utils/org_email_notifications.tstests/plugin-supabase-write-guard.unit.test.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Cap-go/capacitor-updater(manual)
💤 Files with no reviewable changes (1)
- cloudflare_workers/plugin/wrangler.jsonc
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?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: 5ab7ecda18
ℹ️ 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: bb6263fa05
ℹ️ 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: a9ebe16d86
ℹ️ 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".
|
jinhongliang991013
left a comment
jinhongliang991013
left a comment
There was a problem hiding this comment.
Reviewed the current queue/trigger path after the previous reliability fixes. One remaining delivery gap affects org-member notifications with more than one eligible recipient.
| if (item.type === 'org') | ||
| return await sendNotifOrg(c, item.eventName, item.eventData, item.orgId, item.uniqId, item.cron, item.managementEmail, drizzleClient) | ||
|
|
||
| return await sendNotifToOrgMembers(c, item.eventName, item.preferenceKey, item.eventData, item.orgId, item.uniqId, item.cron, drizzleClient, item.audience) |
There was a problem hiding this comment.
Do not acknowledge this queued item until every intended recipient has a durable delivery result. sendNotifToOrgMembers awaits Bento only for primaryEmail; each additionalEmail is launched through backgroundTask(c, trackBentoEvent(...)) and the helper immediately returns true. This trigger converts that true to delivered, so the flusher deletes the KV item. If Bento returns false for any additional admin/billing recipient, that result exists only inside waitUntil and is never observed or retried; the queue still reports success and that recipient permanently misses the email. The queued path needs awaited/per-recipient delivery tracking (and idempotency) before returning delivered, rather than reusing the helper's fire-and-forget contract.
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
2 participants
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)
plugin_notificationstrigger to process transferred items through the existing notification helpers outside the hot plugin path.Motivation (AI generated)
Plugin endpoints are hot public device endpoints and should not read or write the primary Supabase database for notification side effects. Queuing notification work in KV lets plugin requests stay replica/KV/Analytics-only while preserving the email workflow through a controlled background trigger.
Business Impact (AI generated)
This reduces primary database pressure and tail latency risk on
/updates,/stats, and/channel_self, while keeping billing/plugin/channel-self alert emails flowing through a delayed background path. It also makes missing Worker bindings fail closed instead of silently writing to Supabase from plugin traffic.Test Plan (AI generated)
bunx vitest run tests/plugin-supabase-write-guard.unit.test.tsbun typecheckbun lint:backendbun lint(passes with existingsrc/services/compatibilityEvents.tswarning)bunx wrangler deploy --dry-run --config cloudflare_workers/plugin/wrangler.jsonc --env prod_eubunx wrangler deploy --dry-run --config cloudflare_workers/api/wrangler.jsonc --env prodGenerated with AI
Summary by CodeRabbit
New Features
Infrastructure
Bug Fixes
Tests