fix: deduplicate dashboard engagement usage#425
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Deploying usesend with
|
| Latest commit: |
90f205c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://99b8f676.usesend.pages.dev |
| Branch Preview URL: | https://codex-fix-dashboard-usage-co.usesend.pages.dev |
Walkthrough
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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
apps/web/src/server/service/ses-hook-parser.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. apps/web/src/server/service/ses-hook-parser.unit.test.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. 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 |
![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 (1)
apps/web/src/server/service/ses-hook-parser.unit.test.ts (1)
144-186: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo coverage for the reused campaign-analytics dedup path.
This PR also changes campaign-analytics dedup (Line 293 of the parser) to reuse
existingMailEvent, but the suite only exercises non-campaign engagement events (campaignId: null). Adding a case withcampaignIdset and a duplicate status event would validate thatupdateCampaignAnalyticsis skipped on the second call, mirroring the intent described in the PR objectives.🤖 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 `@apps/web/src/server/service/ses-hook-parser.unit.test.ts` around lines 144 - 186, Extend the “parseSesHook dashboard engagement usage” tests to cover events with a non-null campaignId and duplicate Open/Click statuses. Mock the reused existingMailEvent path as needed, invoke parseSesHook twice, and assert updateCampaignAnalytics is called only for the first event while emailEvent.create still records both events.
🤖 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 `@apps/web/src/server/service/ses-hook-parser.ts`:
- Around line 104-116: Make deduplication in the email-event creation flow
atomic rather than relying on the `existingMailEvent` lookup. Add a database
uniqueness constraint for the `(emailId, status)` pair and use an upsert or
conflict-ignoring insert when creating `EmailEvent`; alternatively,
transactionally lock the record around the lookup and create. Preserve the
existing campaign and engagement-event eligibility behavior.
---
Nitpick comments:
In `@apps/web/src/server/service/ses-hook-parser.unit.test.ts`:
- Around line 144-186: Extend the “parseSesHook dashboard engagement usage”
tests to cover events with a non-null campaignId and duplicate Open/Click
statuses. Mock the reused existingMailEvent path as needed, invoke parseSesHook
twice, and assert updateCampaignAnalytics is called only for the first event
while emailEvent.create still records both events.
🪄 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: CHILL
Plan: Pro
Run ID: d35d5773-1c60-4336-8ecc-c9877a9c2c54
📒 Files selected for processing (2)
apps/web/src/server/service/ses-hook-parser.tsapps/web/src/server/service/ses-hook-parser.unit.test.ts
| const isEngagementEvent = [EmailStatus.OPENED, EmailStatus.CLICKED].includes( | ||
| mailStatus, | ||
| ); | ||
| const existingMailEvent = | ||
| email.campaignId || isEngagementEvent | ||
| ? await db.emailEvent.findFirst({ | ||
| where: { | ||
| emailId: email.id, | ||
| status: mailStatus, | ||
| }, | ||
| }) | ||
| : null; | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether EmailEvent has a uniqueness guarantee on (emailId, status)
# that would make this check-then-act pattern safe under concurrency.
fd -e prisma | xargs -I{} grep -n -A 15 "model EmailEvent" {}Repository: usesend/useSend
Length of output: 634
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant parser flow around the lookup, duplicate checks, and create path.
file="apps/web/src/server/service/ses-hook-parser.ts"
sed -n '90,130p' "$file"
printf '\n---\n'
sed -n '198,220p' "$file"
printf '\n---\n'
sed -n '286,320p' "$file"
printf '\n=== Prisma model ===\n'
fd -e prisma | xargs -I{} sh -c 'grep -n -A 20 "model EmailEvent" "{}"'Repository: usesend/useSend
Length of output: 3142
Dedup still races under concurrent deliveries
findFirst can return null in two parallel invocations before either reaches db.emailEvent.create(), and EmailEvent has no uniqueness constraint on (emailId, status) to block the duplicate insert. Move this guard to the DB layer with a unique constraint + upsert/ON CONFLICT DO NOTHING, or wrap the lookup and create in a transaction/lock.
🤖 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 `@apps/web/src/server/service/ses-hook-parser.ts` around lines 104 - 116, Make
deduplication in the email-event creation flow atomic rather than relying on the
`existingMailEvent` lookup. Add a database uniqueness constraint for the
`(emailId, status)` pair and use an upsert or conflict-ignoring insert when
creating `EmailEvent`; alternatively, transactionally lock the record around the
lookup and create. Preserve the existing campaign and engagement-event
eligibility behavior.
1 participant
Summary
Root cause
The SES webhook parser incremented
DailyEmailUsagebefore checking whether an event with the same email and status already existed. Campaign analytics performed this deduplication, but dashboard usage did not, so repeated open and click webhooks inflated dashboard totals.Verification
eslint src/server/service/ses-hook-parser.ts src/server/service/ses-hook-parser.unit.test.ts --max-warnings 0git diff --checkMigration notes
No database migration required.
Closes #101
Summary by cubic
Fixes inflated dashboard engagement metrics by deduplicating SES open/click events. Only the first open and first click per email increment usage; the same event lookup is reused for campaign analytics to keep counts consistent.
DailyEmailUsageforOPENED/CLICKED.Written for commit 90f205c. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests