Seth/to arrays #39
Seth/to arrays #39
Conversation
commit: |
(cherry picked from commit 8942f0d)
(cherry picked from commit 2ae026c)
(cherry picked from commit 14b0a63)
This doesn't quite give us the ability to track how many were delivered, bounced, etc yet, as it does not tell us which email recipient bounced, or complained. They intend to improve the API early november at which point we can integrate that information
…ious if you send to 100 recipients for one email?
sethconve
force-pushed
the
seth/to-arrays
branch
from
b218f06 to
5737c88
Compare
sethconve
force-pushed
the
seth/to-arrays
branch
from
c7b4735 to
e73a091
Compare
sethconve
force-pushed
the
seth/to-arrays
branch
from
e73a091 to
29916d7
Compare
ianmacartney
left a comment
ianmacartney
left a comment
There was a problem hiding this comment.
looking good.
approval conditional on responding to feedback. re-request if you want another review pass!
WalkthroughThis pull request extends email functionality to support multiple recipients (cc, bcc, to as arrays), adds webhook event tracking through a new deliveryEvents table, introduces email status fields (bounced, failed, deliveryDelayed, clicked), implements event-driven state management via computeEmailUpdateFromEvent, adds background cleanup for old emails, and updates the convex-helpers peer dependency. Changes
Sequence Diagram(s)sequenceDiagram
participant Webhook as Resend Webhook
participant Handler as handleEmailEvent
participant DB as Database
participant Compute as computeEmailUpdateFromEvent
participant CB as Callback Queue
Webhook->>Handler: POST /email-event
activate Handler
Handler->>DB: Fetch email by resendId
activate DB
DB-->>Handler: email record
deactivate DB
alt Event in ACCEPTED_EVENT_TYPES
Handler->>DB: Insert deliveryEvents entry
activate DB
Note over DB: (emailId, resendId, eventType, createdAt, message)
DB-->>Handler: event persisted
deactivate DB
Handler->>Compute: computeEmailUpdateFromEvent(email, event)
activate Compute
Note over Compute: Determine state transitions<br/>(e.g., delivered→status,<br/>bounced→bounced=true)
Compute-->>Handler: updateFields
deactivate Compute
Handler->>DB: Update email with new fields
activate DB
DB-->>Handler: updated email
deactivate DB
Handler->>CB: Enqueue callback(email, event)
Note over CB: Process callback async
else Event not accepted
Note over Handler: Skip processing
end
deactivate Handler
Handler-->>Webhook: 200 OK
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
example/convex/example.ts (1)
53-68: Consider reducing the default recipient list for the example.The 14-recipient default array is quite large for a demonstration function. A smaller subset (e.g., 3-4 addresses covering the main test scenarios) would be more readable while still showcasing the multi-recipient feature.
- to: args.to ?? [ - "[email protected]", - "[email protected]", - "[email protected]", - "[email protected]", - "[email protected]", - "[email protected]", - "[email protected]", - "[email protected]", - "[email protected]", - "[email protected]", - "[email protected]", - "[email protected]", - "[email protected]", - "[email protected]", - ], + to: args.to ?? [ + "[email protected]", + "[email protected]", + "[email protected]", + ],src/client/index.ts (1)
289-294: Minor inconsistency in array normalization.The
tofield uses an inline ternary whileccandbccuse thetoArrayhelper. Consider usingtoArrayconsistently, though this would require a slight modification sincetois required:- to: - typeof sendEmailArgs.to === "string" - ? [sendEmailArgs.to] - : sendEmailArgs.to, - cc: toArray(sendEmailArgs.cc), - bcc: toArray(sendEmailArgs.bcc), + to: toArray(sendEmailArgs.to) ?? [], + cc: toArray(sendEmailArgs.cc), + bcc: toArray(sendEmailArgs.bcc),src/component/lib.ts (1)
98-98: LocalBATCH_SIZEshadows module-level constant.The local
BATCH_SIZE = 100shadows the module-level constant with the same value at line 33. Consider reusing the existing constant or renaming this one (e.g.,CLEANUP_BATCH_SIZE) for clarity.- const BATCH_SIZE = 100; + // Reuse module-level BATCH_SIZE or: + const CLEANUP_BATCH_SIZE = 100;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonsrc/component/_generated/component.tsis excluded by!**/_generated/**
📒 Files selected for processing (8)
example/convex/example.ts(1 hunks)package.json(1 hunks)src/client/index.ts(5 hunks)src/component/lib.test.ts(6 hunks)src/component/lib.ts(17 hunks)src/component/schema.ts(3 hunks)src/component/setup.test.ts(1 hunks)src/component/shared.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/component/lib.test.ts (3)
src/component/shared.ts (1)
EmailEvent(153-153)src/client/index.ts (1)
EmailEvent(26-26)src/component/setup.test.ts (1)
createTestEventOfType(24-151)
src/component/schema.ts (1)
src/component/shared.ts (1)
vEventType(151-151)
src/client/index.ts (1)
src/component/shared.ts (1)
Status(25-25)
🔇 Additional comments (22)
package.json (1)
63-63: LGTM!The peer dependency bump to
convex-helpers@^0.1.106aligns with the new usage ofliteralsfromconvex-helpers/validatorsinsrc/component/shared.ts.src/component/setup.test.ts (1)
194-199: LGTM!The test fixture correctly initializes all new boolean status fields (
bounced,failed,deliveryDelayed,clicked) tofalse, which is the appropriate default state for a freshly sent email.src/client/index.ts (4)
133-161: LGTM!The
EmailStatustype is properly extended with well-documented boolean fields (bounced,failed,deliveryDelayed,clicked) that align with the expanded delivery tracking capabilities.
166-168: LGTM!The
SendEmailOptionstype correctly supports both single string and array inputs forto,cc, andbccfields, providing a flexible API for callers.
471-474: LGTM!The
toArrayhelper is clean and correctly handles theundefinedcase, preserving it rather than converting to an empty array—important for optional fields likeccandbcc.
300-314:sendEmailManuallyinconsistency is by design, not a bug.Unlike
sendEmail, which normalizestoto an array becauselib.sendEmailrequiresv.array(v.string()),sendEmailManuallypassestodirectly becauselib.createManualEmailexplicitly accepts both formats:v.union(v.array(v.string()), v.string()). No normalization is needed, though the inconsistency in approach between the two methods could be documented.src/component/lib.test.ts (4)
25-27: LGTM!Converting
execto an async function that properly awaits the mutation is correct. This ensures tests wait for the mutation to complete before making assertions.
45-57: LGTM!Good addition of verification for
deliveryEventsentries. The test properly queries by bothemailIdandeventTypeusing the index, and validates the expected fields.
144-156: LGTM!The clicked event test now properly verifies that
updatedEmail.clickedis set totrue, aligning with the new click tracking functionality.
158-170: LGTM!The failed event test correctly verifies:
- Status transitions to
"failed"- The
failedflag is set totrue- The email is finalized (timestamp updated)
This properly validates the new failure handling behavior.
src/component/shared.ts (1)
140-151: LGTM! Event types are properly synchronized.The
ACCEPTED_EVENT_TYPESconstant includes all 8 event types defined invEmailEvent(email.sent, email.delivered, email.bounced, email.complained, email.failed, email.delivery_delayed, email.opened, email.clicked). The pattern of derivingvEventTypevalidator from a centralized constant ensures type safety and maintainability.src/component/schema.ts (2)
18-24: LGTM! Schema correctly implements delivery events tracking.The
deliveryEventstable structure with the composite indexby_emailId_eventTypeaddresses the past review feedback for efficient distinct event lookups per email.
25-56: LGTM! Schema changes support backward compatibility and new status tracking.The
tofield union type preserves compatibility with existing string data while enabling array support. New optional boolean flags (bounced,failed,deliveryDelayed,clicked) follow the guidance to start as optional.src/component/lib.ts (9)
41-57: LGTM! Test email validation logic is correct.The function properly validates Resend test email addresses by checking both the domain and known prefix patterns.
145-153: LGTM! Comprehensive test mode validation for all recipient types.The validation correctly iterates over all recipient fields (
to,cc,bcc) to ensure only valid Resend test addresses are used in test mode.
226-241: LGTM! Proper normalization and field initialization.The mutation correctly normalizes the
tofield to an array and initializes all new boolean status fields consistently withsendEmail.
313-322: LGTM! Proper fallback handling for optional fields.The
?? falsepattern correctly ensures the API returns non-optional booleans while maintaining backward compatibility with existing records that lack the new fields.
352-358: LGTM! Consistent array normalization fortofield.The query correctly normalizes the
tofield to always return an array, providing a consistent API regardless of how the data was stored (string or array).
741-763: Well-designed state machine with clear precedence rules.The status ranking system effectively prevents downgrade transitions and handles the terminal
cancelledstate appropriately. The early-return pattern minimizes write contention, addressing the OCC concern mentioned in the PR description.
944-959: LGTM! Cleanup helper properly handles all associated records.The function correctly cleans up the email record along with its content and delivery events. The
collect()usage is acceptable here since delivery events per email are bounded by the finite set of event types.
661-678: LGTM! Batch payload correctly handles recipient arrays.The payload construction properly normalizes
toto an array and includescc/bccfields for the Resend API.
857-858: I'll help you verify theassertExhaustiveusage. Let me search for the implementation and check how it's being called.
<function_calls>
#!/bin/bashFind utils.ts and check assertExhaustive implementation
find . -name "utils.ts" -type f | head -20
</function_calls>
<function_calls>
#!/bin/bashSearch for assertExhaustive definition
rg -n "assertExhaustive" --type=ts -B 2 -A 8
</function_calls>
<function_calls>
#!/bin/bashCheck the computeEmailUpdateFromEvent function and event parameter type
sed -n '741,860p' src/component/lib.ts | head -100
</function_calls>
| // Apply the event directly to update email state if needed | ||
| const updated = computeEmailUpdateFromEvent(email, event); | ||
| if (updated) { | ||
| await ctx.db.replace(email._id, updated); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Convex db.replace _id _creationTime system fields documentation
💡 Result:
Short summary from the Convex docs:
- Every document has two automatically-generated system fields: _id (the document ID) and _creationTime (milliseconds since the Unix epoch). [1].
- db.replace replaces an existing document entirely (removing fields not present in the replacement). System fields are present on documents but you normally do not set them in mutations; they are treated as automatic fields. [2][3].
- The only supported way to create documents with pre-existing _id and _creationTime values is via the data import/restore tooling — imports retain those system fields and you can use --replace to overwrite existing table data. The _id must match Convex’s ID format. [2].
- _creationTime is automatically appended to indexes (used as a tiebreaker); do not include it explicitly in index definitions. [4].
Sources: Convex docs — Data types / System fields; Writing data (db.replace); Data import (import/export); Indexes. [1][3][2][4]
References:
[1] Data Types / System fields. (docs.convex.dev)
[2] Data Import / import behavior (preserve _id/_creationTime, --replace). (docs.convex.dev)
[3] Writing Data / db.replace description. (docs.convex.dev)
[4] Indexes / _creationTime appended to indexes. (docs.convex.dev)
Citations:
- 1: https://docs.convex.dev/database/types?utm_source=openai
- 2: https://docs.convex.dev/database/import-export/import?utm_source=openai
- 3: https://docs.convex.dev/database/writing-data?utm_source=openai
- 4: https://docs.convex.dev/database/reading-data/indexes/?utm_source=openai
🏁 Script executed:
# First, let's find and examine the computeEmailUpdateFromEvent function
rg -n "computeEmailUpdateFromEvent" src/component/lib.ts -A 20 -B 2Repository: get-convex/resend
Length of output: 1749
🏁 Script executed:
# Search for other usages of db.replace in the codebase to understand patterns
rg -n "\.replace\(" src/ -B 2 -A 2 --type=tsRepository: get-convex/resend
Length of output: 584
🏁 Script executed:
# Get the full implementation of computeEmailUpdateFromEvent
sed -n '741,800p' src/component/lib.tsRepository: get-convex/resend
Length of output: 1771
🏁 Script executed:
# Get remaining lines of computeEmailUpdateFromEvent to see all return patterns
sed -n '800,830p' src/component/lib.tsRepository: get-convex/resend
Length of output: 944
🏁 Script executed:
# Search for type definitions of db.replace or mutation context to understand what's expected
rg -n "db\.replace" src/ -B 5 -A 5 --type=ts | head -40Repository: get-convex/resend
Length of output: 1225
Based on my verification, I can now provide the rewritten review comment.
ctx.db.replace should not include system fields _id and _creationTime in the document argument.
The updated object from computeEmailUpdateFromEvent spreads the entire email Doc using {...email, ...} syntax (lines 776, 782, 793, 811, etc.), which includes the system fields _id and _creationTime. According to Convex documentation, these system fields are automatically managed and should not be explicitly set in mutations.
While db.replace may internally ignore or handle these fields, the pattern at line 371 (ctx.db.replace(lastOptions._id, {options})) demonstrates the recommended approach of passing only the fields being updated. Align line 911 with this pattern by omitting system fields from the replacement document.
🤖 Prompt for AI Agents
In src/component/lib.ts around lines 908-912, the code calls
ctx.db.replace(email._id, updated) where updated was produced by spreading the
full email doc and thus includes Convex system fields (_id and _creationTime);
remove those system fields from the document passed to ctx.db.replace so only
application fields are provided (follow the pattern used at line 371 where only
the updatable fields are passed). Update the call to construct a replacement
object that explicitly omits _id and _creationTime (or builds a new object with
only the allowed fields) before calling ctx.db.replace(email._id, ...).
This is a proposal on how to start handling resend webhooks with more detail over time. There is a simple mechanism for aggregate counting which might hit OCC if we get a bunch of bounces at once. Do we want to schedule an aggregator function instead of mutating them one at a time?
Summary by CodeRabbit
Release Notes
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.