refactor(kit): prefer webhook events for dedup#245
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
hyochan
added
፦ refactor
🚽 migration
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughWebhook deduplication now uses normalized source-aware event identity, with a composite schema index, event-first lookup and recording paths, expanded test harness support, and coverage for Google, Apple, and Meta notification scenarios. ChangesWebhook deduplication
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant GoogleRTDN
participant ingestGoogleRtdn
participant lookupExistingEvent
participant ConvexDB
GoogleRTDN->>ingestGoogleRtdn: notification payload
ingestGoogleRtdn->>lookupExistingEvent: project, source, sourceNotificationId
lookupExistingEvent->>ConvexDB: query source-aware webhookEvents index
ConvexDB-->>lookupExistingEvent: existing event or no match
lookupExistingEvent-->>ingestGoogleRtdn: event id or fallback result
ingestGoogleRtdn-->>GoogleRTDN: deduped WebhookEvent or continue processing
Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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.
🧹 Nitpick comments (1)
packages/kit/convex/webhooks/internal.ts (1)
50-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate index-query construction between
lookupExistingEventandrecordWebhookEvent.The
by_project_and_source_and_notification_idlookup (.eq("projectId", ...).eq("source", ...).eq("sourceNotificationId", ...)) is duplicated verbatim in both functions. Since this is the dedup-critical query, extracting a small shared helper (e.g.findWebhookEventBySource(ctx, projectId, storedSource, sourceNotificationId)) would prevent the two call sites drifting out of sync in a future edit.♻️ Proposed helper extraction
+async function findWebhookEventBySource( + ctx: { db: DatabaseReader }, + projectId: Id<"projects">, + storedSource: StoredWebhookSource, + sourceNotificationId: string, +) { + return ctx.db + .query("webhookEvents") + .withIndex("by_project_and_source_and_notification_id", (q) => + q + .eq("projectId", projectId) + .eq("source", storedSource) + .eq("sourceNotificationId", sourceNotificationId), + ) + .unique(); +}Then call
findWebhookEventBySource(...)from bothlookupExistingEventandrecordWebhookEvent.Also applies to: 162-184
🤖 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 `@packages/kit/convex/webhooks/internal.ts` around lines 50 - 72, Extract the duplicated webhookEvents index query into a shared helper, such as findWebhookEventBySource, accepting the context, projectId, stored source, and sourceNotificationId. Replace the duplicated query construction in both lookupExistingEvent and recordWebhookEvent with this helper, preserving the existing storedSourceForDedupSource behavior and unique lookup semantics.
🤖 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.
Nitpick comments:
In `@packages/kit/convex/webhooks/internal.ts`:
- Around line 50-72: Extract the duplicated webhookEvents index query into a
shared helper, such as findWebhookEventBySource, accepting the context,
projectId, stored source, and sourceNotificationId. Replace the duplicated query
construction in both lookupExistingEvent and recordWebhookEvent with this
helper, preserving the existing storedSourceForDedupSource behavior and unique
lookup semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8ef4eeb8-a8d9-43f9-9a51-9bbd2677bc84
📒 Files selected for processing (8)
packages/kit/convex/schema.tspackages/kit/convex/subscriptions/horizonInternal.test.tspackages/kit/convex/subscriptions/horizonInternal.tspackages/kit/convex/test.setup.tspackages/kit/convex/webhooks/google.test.tspackages/kit/convex/webhooks/google.tspackages/kit/convex/webhooks/internal.test.tspackages/kit/convex/webhooks/internal.ts
|
Addressed the source-aware webhook event lookup duplication in 7f356e9. The shared typed helper preserves @coderabbitai review |
|
✅ Action performedReview finished.
|
1 participant
Summary
Rollout
This is phase 1 of the staged migration in #241. It intentionally keeps webhookIdempotencyKeys reads, writes, pruning, and deletion cascades for rollback and retention drain. A later deploy can stop key writes after production observation; table removal must wait at least WEBHOOK_RETENTION_MS and an explicit zero-row check. The webhook event retention cron remains required.
Refs #241
Verification
Summary by CodeRabbit