-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Add support for mode="deferred"
in /track/lead
#2815
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughUpdates track-lead to accept rawBody and workspace, make clickId optional with customerExternalId fallback/upsert, add Redis dedup and Tinybird/Redis click lookup fallback, validate link↔workspace, persist avatars to R2, support mode ("async"|"wait"|"deferred") flows and eventQuantity, and change response to { click, customer }. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant API as trackLead
participant DB as Prisma
participant TB as Tinybird
participant R as Redis
participant S as R2
participant BG as Background Worker
participant WH as Webhook
C->>API: POST /track/lead { clickId?, customerExternalId, mode, eventName, eventQuantity, ... }
API->>DB: Find customer by { workspaceId, externalId }
alt clickId omitted
alt customer exists with clickId
API->>API: reuse customer's clickId
else
API-->>C: 400 bad_request (missing clickId)
end
end
API->>TB: Fetch click data by clickId
alt not found in Tinybird
API->>R: check clickIdCache
alt cache miss
API-->>C: 404 not_found (click data)
end
end
API->>DB: Validate link.projectId == workspace.id
alt customer missing
API->>DB: Create customer (finalCustomerId, externalId, avatar...)
API->>S: Persist avatar to R2 (if needed)
end
API->>R: Deduplicate via key trackLead:<ws>:<externalId>:<event>
alt duplicate found
API-->>C: 200 { click, customer } (duplicate bypass)
else
API->>API: Build lead event(s) with nanoid (repeat for eventQuantity)
alt mode == "wait"
API->>BG: process synchronously (recordLeadSync, counts, commissions, workflows)
API->>WH: emit lead.created webhook
API-->>C: 200 { click, customer }
else
API->>R: cache/enqueue payload / waitUntil background work
API->>BG: background processing
API-->>C: 202/200 { click, customer }
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/web/lib/api/conversions/track-sale.ts (1)
265-275
: Prevent Zod parse crashes: coalesce possibly undefined fields to null
invoiceId
andmetadata
may beundefined
based on the inputs; the schema expects nullable, not optional. Parsing will throw at runtime if they’reundefined
.Apply:
return trackSaleResponseSchema.parse({ eventName, customer, sale: { amount, currency, - invoiceId, + invoiceId: invoiceId ?? null, paymentProcessor, - metadata, + metadata: metadata ?? null, }, });apps/web/lib/zod/schemas/leads.ts (1)
53-57
: Constrain eventQuantity to safe valuesAs-is, non-integers (e.g., 0.5) can cause
Array(eventQuantity)
to throw; 0 leads to a single event. Constrain to positive integers.- eventQuantity: z - .number() + eventQuantity: z + .number() + .int("eventQuantity must be an integer") + .positive("eventQuantity must be > 0") .nullish()apps/web/lib/api/conversions/track-lead.ts (1)
66-81
: Deferred mode currently never results in a recorded lead (blocked by dedupe) and breaks sale linkage
- First request with
mode="deferred"
sets the NX dedupe key (Lines 66-81) but skips ingestion. Subsequent requests won’t execute the ingestion path becauseok
is false.- Only “wait” mode caches a lead payload in Redis (Lines 197-224); “deferred” does not, yet
/track/sale
looks upleadCache:*
as its fallback.- Non-deferred post-processing runs later (Lines 260-329); deferred skips it entirely, leaving nothing for
/track/sale
to associate.Result: sales with
leadEventName
after a deferred lead will fail to find a lead (Tinybird or cache).Fix with minimal surface area: cache the lead payload on deferred (like “wait”), without ingesting to Tinybird.
/track/sale
will then resolve vialeadCache
.- if (mode == "async" || mode == "deferred") { + if (mode === "async" || mode === "deferred") { customer = await upsertCustomer(); - // for async mode, record the lead event right away - if (mode === "async") { - await recordLead(createLeadEventPayload(customer.id)); - } + // for async mode, record the lead event right away + if (mode === "async") { + await recordLead(createLeadEventPayload(customer.id)); + } else { + // for deferred mode, cache a resolvable lead payload so /track/sale can link to it + const leadEventPayload = createLeadEventPayload(customer.id); + const cacheLeadEventPayload = Array.isArray(leadEventPayload) + ? leadEventPayload[0] + : leadEventPayload; + await Promise.all([ + redis.set(`leadCache:${customer.id}`, cacheLeadEventPayload, { + ex: 60 * 30, // 30 min to comfortably cover lead->sale flows + }), + redis.set( + `leadCache:${customer.id}:${stringifiedEventName}`, + cacheLeadEventPayload, + { ex: 60 * 30 }, + ), + ]); + } }Optional follow-up (nice-to-have): later, teach
/track/sale
to ingest the cached deferred lead into Tinybird upon first sale, so analytics contain the lead as a first-class event.Also applies to: 197-224, 225-235, 260-329
🧹 Nitpick comments (5)
apps/web/lib/zod/schemas/leads.ts (3)
9-12
: Document fallback behavior for optional clickIdSince
clickId
is now optional and the API falls back to an existing customer’sclickId
(see track-lead.ts), update the description to state this behavior and the error thrown when neither is available.
16-20
: Align eventName normalization with server logicServer normalizes with
toLowerCase().replaceAll(" ", "-")
for dedupe keys. Consider adding a schema transform or validation pattern to avoid mismatches (e.g., multiple spaces, non-ASCII).
59-63
: Clarify deferred mode semantics in schema descriptionExplain that
deferred
creates/updates the customer but defers lead ingestion; also mention how the later linkage occurs (e.g., via Redis lead cache used by/track/sale
). This reduces ambiguity for integrators.apps/web/lib/api/conversions/track-lead.ts (2)
228-235
: Use strict equalityPrefer
===
for mode checks.- if (mode == "async" || mode == "deferred") { + if (mode === "async" || mode === "deferred") {
63-63
: Normalize eventName robustly
toLowerCase().replaceAll(" ", "-")
is brittle for multiple/Unicode whitespace. Consider a shared slug util to avoid duped keys that differ only by spacing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/web/lib/api/conversions/track-lead.ts
(4 hunks)apps/web/lib/api/conversions/track-sale.ts
(1 hunks)apps/web/lib/zod/schemas/leads.ts
(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/web/lib/api/conversions/track-sale.ts (1)
apps/web/lib/zod/schemas/sales.ts (1)
trackSaleResponseSchema
(69-89)
apps/web/lib/api/conversions/track-lead.ts (6)
apps/web/lib/api/errors.ts (1)
DubApiError
(75-92)apps/web/lib/partners/create-partner-commission.ts (1)
createPartnerCommission
(25-341)apps/web/lib/api/workflows/execute-workflows.ts (1)
executeWorkflows
(17-122)apps/web/lib/webhook/publish.ts (1)
sendWorkspaceWebhook
(7-36)apps/web/lib/webhook/transform.ts (1)
transformLeadEventData
(60-78)apps/web/lib/zod/schemas/leads.ts (1)
trackLeadResponseSchema
(76-86)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Vade Review
- GitHub Check: build
🔇 Additional comments (1)
apps/web/lib/zod/schemas/leads.ts (1)
27-27
: Good: enforce non-empty externalIdMaking
customerExternalId
non-empty avoids silent “anonymous” customers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
♻️ Duplicate comments (2)
apps/web/lib/api/conversions/track-lead.ts (2)
194-195
: Remove PII-bearing console logs (reintroduced)customer JSON and lead recording responses can include PII. These logs were flagged and removed earlier; they should not be present.
Apply:
- console.log(`customer created: ${JSON.stringify(customer, null, 2)}`); @@ - const res = await recordLead(createLeadEventPayload(customer.id)); - console.log("lead event recorded:", res); + await recordLead(createLeadEventPayload(customer.id));Also applies to: 229-230
261-288
: Keep per-customer stats consistent: increment customer.leadsLink and workspace usage are updated, but the customer’s leads count isn’t. Update it atomically.
Apply:
- const [link, _project] = await Promise.all([ + const [link, _project, _customer] = await Promise.all([ // update link leads count prisma.link.update({ @@ }), // update workspace events usage prisma.project.update({ @@ }), + // update customer leads count + prisma.customer.update({ + where: { id: customer!.id }, + data: { + leads: { increment: eventQuantity ?? 1 }, + }, + }), ]);
🧹 Nitpick comments (3)
apps/web/tests/tracks/track-lead.test.ts (1)
158-179
: Consider asserting the effect of eventQuantityNice scenario. Add an assertion that the side-effect reflects quantity (e.g., link.leads or workspace usage increased by 2) to catch regressions.
apps/web/lib/api/conversions/track-lead.ts (2)
143-144
: Nit: error code semanticsUsing not_found for “link does not belong to workspace” avoids info leakage; if you prefer explicit semantics, consider forbidden. Not blocking.
123-123
: Nit: typo in comment“from the from the” → “from the”.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/lib/api/conversions/track-lead.ts
(8 hunks)apps/web/tests/tracks/track-lead.test.ts
(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/web/tests/tracks/track-lead.test.ts (2)
apps/web/tests/utils/helpers.ts (1)
randomCustomer
(7-17)apps/web/lib/types.ts (1)
TrackLeadResponse
(388-388)
apps/web/lib/api/conversions/track-lead.ts (9)
apps/web/lib/api/errors.ts (1)
DubApiError
(75-92)apps/web/lib/types.ts (1)
ClickEventTB
(522-522)apps/web/lib/api/create-id.ts (1)
createId
(60-69)packages/utils/src/constants/main.ts (1)
R2_URL
(81-81)apps/web/lib/partners/create-partner-commission.ts (1)
createPartnerCommission
(25-341)apps/web/lib/api/workflows/execute-workflows.ts (1)
executeWorkflows
(17-122)apps/web/lib/webhook/publish.ts (1)
sendWorkspaceWebhook
(7-36)apps/web/lib/webhook/transform.ts (1)
transformLeadEventData
(60-78)apps/web/lib/zod/schemas/leads.ts (1)
trackLeadResponseSchema
(76-86)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Vade Review
- GitHub Check: build
🔇 Additional comments (1)
apps/web/tests/tracks/track-lead.test.ts (1)
70-89
: LGTM: good idempotency coverage with same customerExternalIdThis verifies duplicate requests return the same payload. No issues spotted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/lib/api/conversions/track-lead.ts (1)
148-168
: event_id mismatch when eventQuantity > 1; partner commission references a non-existent idWhen eventQuantity is set, payload event_ids are regenerated, but commission uses leadEventId declared earlier, which won’t match any ingested event. Generate IDs up front and pass the primary one to both payload and commission.
- const leadEventId = nanoid(16); + const eventCount = Math.max(1, eventQuantity ?? 1); + const eventIds = Array.from({ length: eventCount }, () => nanoid(16)); + const primaryEventId = eventIds[0]; @@ - const createLeadEventPayload = (customerId: string) => { + const createLeadEventPayload = (customerId: string) => { const basePayload = { ...clickData, - event_id: leadEventId, + event_id: primaryEventId, event_name: eventName, customer_id: customerId, metadata: metadata ? JSON.stringify(metadata) : "", }; - return eventQuantity - ? Array(eventQuantity) - .fill(null) - .map(() => ({ - ...basePayload, - event_id: nanoid(16), - })) - : basePayload; + return eventCount > 1 + ? Array.from({ length: eventCount }, (_, i) => ({ + ...basePayload, + event_id: eventIds[i], + })) + : basePayload; };- eventId: leadEventId, + eventId: primaryEventId,Also applies to: 290-299
♻️ Duplicate comments (6)
apps/web/lib/api/conversions/track-lead.ts (6)
68-85
: Fix deferred-mode dedupe: priming call blocks the actual eventUsing the same SET NX key for “deferred” and the real event means the subsequent async/wait call is skipped entirely. Split keys by mode so deferred cannot block event recording, and include mode in the cached payload.
- // deduplicate lead events – only record 1 unique event for the same customer and event name - // TODO: Maybe we can replace this to rely only on MySQL directly since we're checking the customer above? - const ok = await redis.set( - `trackLead:${workspace.id}:${customerExternalId}:${stringifiedEventName}`, + // deduplicate by (customer,event,kind) where kind ∈ {deferred,event} + const dedupeSuffix = mode === "deferred" ? "deferred" : "event"; + const dedupeKey = `trackLead:${workspace.id}:${customerExternalId}:${stringifiedEventName}:${dedupeSuffix}`; + const ok = await redis.set( - { + dedupeKey, + { timestamp: Date.now(), clickId, eventName, customerExternalId, customerName, customerEmail, customerAvatar, + mode, }, { ex: 60 * 60 * 24 * 7, // cache for 1 week nx: true, }, );
170-177
: Avatar uploads can be orphaned for existing customers; use existing id for path and persist DB updateWhen customer exists, avatar path is built with a new finalCustomerId and upload runs without updating DB → orphaned file and stale avatar. Derive path from existing customer.id when present and update avatar after upload.
- const finalCustomerId = createId({ prefix: "cus_" }); - const finalCustomerName = + const newCustomerId = createId({ prefix: "cus_" }); + const finalCustomerName = customerName || customerEmail || generateRandomName(); - const finalCustomerAvatar = - customerAvatar && !isStored(customerAvatar) - ? `${R2_URL}/customers/${finalCustomerId}/avatar_${nanoid(7)}` - : customerAvatar; + const avatarCustomerId = customer?.id ?? newCustomerId; + const nextAvatarPath = + customerAvatar && !isStored(customerAvatar) + ? `${R2_URL}/customers/${avatarCustomerId}/avatar_${nanoid(7)}` + : customer?.avatar ?? customerAvatar ?? null; @@ - customer = await prisma.customer.create({ + customer = await prisma.customer.create({ data: { - id: finalCustomerId, + id: newCustomerId, name: finalCustomerName, email: customerEmail, - avatar: finalCustomerAvatar, + avatar: nextAvatarPath, externalId: customerExternalId, projectId: workspace.id, projectConnectId: workspace.stripeConnectId, clickId: clickData.click_id, linkId: clickData.link_id, country: clickData.country, clickedAt: new Date(clickData.timestamp + "Z"), }, }); - console.log(`customer created: ${JSON.stringify(customer, null, 2)}`); } @@ - if ( - customerAvatar && - !isStored(customerAvatar) && - finalCustomerAvatar - ) { + if (customerAvatar && !isStored(customerAvatar) && nextAvatarPath) { // persist customer avatar to R2 await storage.upload( - finalCustomerAvatar.replace(`${R2_URL}/`, ""), + nextAvatarPath.replace(`${R2_URL}/`, ""), customerAvatar, { width: 128, height: 128, }, ); + if (customer && customer.avatar !== nextAvatarPath) { + await prisma.customer.update({ + where: { id: customer.id }, + data: { avatar: nextAvatarPath }, + }); + } }Also applies to: 241-255, 178-196
178-196
: Remove PII logging (customer dump)Logging the full customer object leaks PII to production logs. Delete this log or guard/sanitize.
- console.log(`customer created: ${JSON.stringify(customer, null, 2)}`);
224-231
: Remove noisy log; may include PII in responseDrop the “lead event recorded” console.log.
- if (mode === "async") { - const res = await recordLead(createLeadEventPayload(customer.id)); - console.log("lead event recorded:", res); - } + if (mode === "async") { + await recordLead(createLeadEventPayload(customer.id)); + }
262-289
: Update customer leads count to keep per-customer stats consistentPost-processing bumps link and workspace but not customer.leads. Increment it in the same Promise.all.
- const [link, _project] = await Promise.all([ + const [link, _project, _customer] = await Promise.all([ // update link leads count prisma.link.update({ @@ }), // update workspace events usage prisma.project.update({ @@ }), + // update customer leads count + prisma.customer.update({ + where: { id: customer!.id }, + data: { + leads: { increment: eventQuantity ?? 1 }, + }, + }), ]);
328-336
: Idempotency race: second request can 404 before customer is persisted; add cached fallbackIf a duplicate request arrives after NX succeeds but before the first write, customer is null and we 404. Fall back to the cached dedupe payload to return a stable 200.
- if (!customer) { - throw new DubApiError({ - code: "not_found", - message: `Customer not found for externalId: ${customerExternalId}`, - }); - } + if (!customer) { + const baseKey = `trackLead:${workspace.id}:${customerExternalId}:${stringifiedEventName}`; + const cached = + (await redis.get<any>(`${baseKey}:event`)) ?? + (await redis.get<any>(`${baseKey}:deferred`)); + if (cached?.clickId) { + const lead = trackLeadResponseSchema.parse({ + click: { id: cached.clickId }, + customer: { + name: cached.customerName ?? null, + email: cached.customerEmail ?? null, + avatar: cached.customerAvatar ?? null, + externalId: customerExternalId, + }, + }); + return { + ...lead, + clickId: cached.clickId, + customerName: lead.customer.name, + customerEmail: lead.customer.email, + customerAvatar: lead.customer.avatar, + }; + } + throw new DubApiError({ + code: "not_found", + message: `Customer not found for externalId: ${customerExternalId}`, + }); + }
🧹 Nitpick comments (2)
apps/web/lib/api/conversions/track-lead.ts (2)
313-323
: Gate webhook delivery by workspace.webhookEnabledAvoid unnecessary work when webhooks are disabled at the workspace level.
- await sendWorkspaceWebhook({ + if (workspace.webhookEnabled) { + await sendWorkspaceWebhook({ trigger: "lead.created", data: transformLeadEventData({ ...clickData, eventName, link, customer, }), workspace, - }); + }); + }
233-240
: rawBody may contain PII; consider sanitizing before loggingIf rawBody includes emails/names, you’ll persist PII. Log only whitelisted fields or mask sensitive ones.
- await logConversionEvent({ + const safeBody = JSON.stringify( + { + eventName, + eventQuantity, + customerExternalId, + clickId, + mode, + metadata, + }, + ); + await logConversionEvent({ workspace_id: workspace.id, link_id: clickData.link_id, path: "/track/lead", - body: JSON.stringify(rawBody), + body: safeBody, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/lib/api/conversions/track-lead.ts
(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: api-tests
- GitHub Check: Vade Review
/bug0 run |
1 similar comment
/bug0 run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/web/lib/api/conversions/track-sale.ts (1)
265-275
: Fix parse crash when invoiceId/metadata are omittedtrackSaleResponseSchema marks invoiceId/metadata as nullable, not undefined. Passing undefined will fail parse. Default them to null.
return trackSaleResponseSchema.parse({ eventName, - customer, + customer, sale: { amount, currency, - invoiceId, + invoiceId: invoiceId ?? null, paymentProcessor, - metadata, + metadata: metadata ?? null, }, });apps/web/lib/api/conversions/track-lead.ts (1)
68-85
: Deferred flow blocked by dedupe; use separate keys for “deferred” vs “event”The first deferred call sets NX on the same key the subsequent async call needs, so the actual lead never records. Split keys by mode.
- // deduplicate lead events – only record 1 unique event for the same customer and event name - // TODO: Maybe we can replace this to rely only on MySQL directly since we're checking the customer above? - const ok = await redis.set( - `trackLead:${workspace.id}:${customerExternalId}:${stringifiedEventName}`, + // Deduplicate per intent: a "deferred" priming call must not block the actual event + const dedupeSuffix = mode === "deferred" ? "deferred" : "event"; + const dedupeKey = `trackLead:${workspace.id}:${customerExternalId}:${stringifiedEventName}:${dedupeSuffix}`; + const ok = await redis.set( - { + dedupeKey, + { timestamp: Date.now(), clickId, eventName, customerExternalId, customerName, customerEmail, customerAvatar, + mode, }, { ex: 60 * 60 * 24 * 7, // cache for 1 week nx: true, }, );
♻️ Duplicate comments (5)
apps/web/tests/tracks/track-lead.test.ts (1)
87-119
: Test doesn’t verify that deferred → async actually records the leadCurrently only the payload is asserted; missed side-effects let a regression slip (e.g., dedupe blocking async record).
Minimal clarity tweak and a side-effect assertion:
const response2 = await http.post<TrackLeadResponse>({ path: "/track/lead", body: { eventName: "Mode=Deferred Signup", customerExternalId: customer2.externalId, + mode: "async", }, });
Consider additionally asserting that link.leads (or workspace.usage) increments after response2.
apps/web/lib/api/conversions/track-lead.ts (4)
87-94
: Avatar path may orphan uploads for existing customersWhen customer exists, avatar path still uses a brand new finalCustomerId. Use the existing customer.id for pathing.
- const finalCustomerId = createId({ prefix: "cus_" }); + const finalCustomerId = createId({ prefix: "cus_" }); const finalCustomerName = customerName || customerEmail || generateRandomName(); - const finalCustomerAvatar = - customerAvatar && !isStored(customerAvatar) - ? `${R2_URL}/customers/${finalCustomerId}/avatar_${nanoid(7)}` - : customerAvatar; + const avatarCustomerId = customer?.id ?? finalCustomerId; + const finalCustomerAvatar = + customerAvatar && !isStored(customerAvatar) + ? `${R2_URL}/customers/${avatarCustomerId}/avatar_${nanoid(7)}` + : customer?.avatar ?? customerAvatar ?? null;Also, after uploading for an existing customer, update customer.avatar if it differs.
262-289
: Increment customer.leads alongside link/projectKeep per-customer stats consistent with link/workspace updates.
- const [link, _project] = await Promise.all([ + const [link, _project, _customer] = await Promise.all([ // update link leads count prisma.link.update({ where: { id: clickData.link_id, }, data: { leads: { increment: eventQuantity ?? 1, }, }, include: includeTags, }), // update workspace events usage prisma.project.update({ where: { id: workspace.id, }, data: { usage: { increment: eventQuantity ?? 1, }, }, }), + // update customer leads count + prisma.customer.update({ + where: { id: customer!.id }, + data: { leads: { increment: eventQuantity ?? 1 } }, + }), ]);
178-196
: Remove PII logging of customer objectsconsole.log leaks PII (name/email/etc.) to prod logs. Remove it.
customer = await prisma.customer.create({ data: { id: finalCustomerId, name: finalCustomerName, email: customerEmail, avatar: finalCustomerAvatar, externalId: customerExternalId, projectId: workspace.id, projectConnectId: workspace.stripeConnectId, clickId: clickData.click_id, linkId: clickData.link_id, country: clickData.country, clickedAt: new Date(clickData.timestamp + "Z"), }, }); - console.log(`customer created: ${JSON.stringify(customer, null, 2)}`);
229-231
: Remove verbose lead event loggingAvoid dumping event responses to logs.
- const res = await recordLead(createLeadEventPayload(customer.id)); - console.log("lead event recorded:", res); + await recordLead(createLeadEventPayload(customer.id));
🧹 Nitpick comments (3)
apps/web/lib/api/conversions/track-sale.ts (1)
267-268
: Whitelist customer fields in responseAvoid leaking extra customer fields by passing only schema-allowed properties.
- customer, + customer: customer + ? { + id: customer.id, + name: customer.name, + email: customer.email, + avatar: customer.avatar, + externalId: customer.externalId, + } + : null,apps/web/tests/tracks/track-lead.test.ts (1)
154-175
: Also assert eventQuantity side-effectsValidate that leads/usage increased by 2 to catch ingestion regressions.
apps/web/lib/api/conversions/track-lead.ts (1)
132-133
: Nit: comment typo“from the from” → “from the”.
- // get the referral link from the from the clickData + // get the referral link from the clickData
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/web/lib/api/conversions/track-lead.ts
(8 hunks)apps/web/lib/api/conversions/track-sale.ts
(1 hunks)apps/web/tests/tracks/track-lead-client.test.ts
(1 hunks)apps/web/tests/tracks/track-lead.test.ts
(3 hunks)apps/web/tests/tracks/track-sale-client.test.ts
(0 hunks)apps/web/tests/tracks/track-sale.test.ts
(0 hunks)
💤 Files with no reviewable changes (2)
- apps/web/tests/tracks/track-sale.test.ts
- apps/web/tests/tracks/track-sale-client.test.ts
🧰 Additional context used
🧬 Code graph analysis (3)
apps/web/lib/api/conversions/track-sale.ts (1)
apps/web/lib/zod/schemas/sales.ts (1)
trackSaleResponseSchema
(69-89)
apps/web/lib/api/conversions/track-lead.ts (8)
apps/web/lib/api/errors.ts (1)
DubApiError
(75-92)apps/web/lib/api/create-id.ts (1)
createId
(60-69)packages/utils/src/constants/main.ts (1)
R2_URL
(81-81)apps/web/lib/types.ts (1)
ClickEventTB
(522-522)apps/web/lib/partners/create-partner-commission.ts (1)
createPartnerCommission
(25-341)apps/web/lib/webhook/publish.ts (1)
sendWorkspaceWebhook
(7-36)apps/web/lib/webhook/transform.ts (1)
transformLeadEventData
(60-78)apps/web/lib/zod/schemas/leads.ts (1)
trackLeadResponseSchema
(76-86)
apps/web/tests/tracks/track-lead.test.ts (2)
apps/web/tests/utils/helpers.ts (1)
randomCustomer
(7-17)apps/web/lib/types.ts (1)
TrackLeadResponse
(388-388)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Vade Review
- GitHub Check: build
🔇 Additional comments (3)
apps/web/tests/tracks/track-lead-client.test.ts (1)
47-52
: LGTM: matches new response shape{ click, customer }
Assertion aligns with the updated schema.
apps/web/tests/tracks/track-lead.test.ts (1)
66-85
: Idempotent duplicate test looks goodCovers same externalId with same clickId returning identical payload.
apps/web/lib/api/conversions/track-lead.ts (1)
328-338
: LGTM: response aligns with new{ click, customer }
schemaFallback customer fields are correctly null-safe.
/bug0 run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (5)
apps/web/lib/api/conversions/track-lead.ts (5)
68-85
: Deferred vs. event dedupe key conflation blocks the second callThe same Redis NX key is used for both the “deferred” priming call and the actual event call, so the second call is skipped and no lead/stats/webhooks run. Use separate keys per mode to keep flows independent.
- // deduplicate lead events – only record 1 unique event for the same customer and event name - // TODO: Maybe we can replace this to rely only on MySQL directly since we're checking the customer above? - const ok = await redis.set( - `trackLead:${workspace.id}:${customerExternalId}:${stringifiedEventName}`, + // deduplicate per flow – don't let a deferred priming call block the actual event + const dedupeSuffix = mode === "deferred" ? "deferred" : "event"; + const dedupeKey = `trackLead:${workspace.id}:${customerExternalId}:${stringifiedEventName}:${dedupeSuffix}`; + const ok = await redis.set( + dedupeKey, { timestamp: Date.now(), clickId, eventName, customerExternalId, customerName, customerEmail, - customerAvatar, + customerAvatar, + mode, }, { ex: 60 * 60 * 24 * 7, // cache for 1 week nx: true, }, );
240-254
: Persisted avatar should update existing customer.avatarAfter uploading, update the DB for existing customers so the record points to the stored asset.
- if ( - customerAvatar && - !isStored(customerAvatar) && - finalCustomerAvatar - ) { + if (customerAvatar && !isStored(customerAvatar) && nextAvatarPath) { // persist customer avatar to R2 await storage.upload( - finalCustomerAvatar.replace(`${R2_URL}/`, ""), + nextAvatarPath.replace(`${R2_URL}/`, ""), customerAvatar, { width: 128, height: 128, }, ); + if (customer && customer.avatar !== nextAvatarPath) { + await prisma.customer.update({ + where: { id: customer.id }, + data: { avatar: nextAvatarPath }, + }); + } }
87-94
: Avatar uploads can be orphaned; path should use existing customer.id and update DBAvatar path is derived from a new ID even for existing customers, and the DB isn’t updated after uploading — leading to orphaned blobs. Use the existing customer.id when present and persist the new path.
- const finalCustomerId = createId({ prefix: "cus_" }); + const newCustomerId = createId({ prefix: "cus_" }); const finalCustomerName = customerName || customerEmail || generateRandomName(); - const finalCustomerAvatar = - customerAvatar && !isStored(customerAvatar) - ? `${R2_URL}/customers/${finalCustomerId}/avatar_${nanoid(7)}` - : customerAvatar; + const avatarCustomerId = customer?.id ?? newCustomerId; + const nextAvatarPath = + customerAvatar && !isStored(customerAvatar) + ? `${R2_URL}/customers/${avatarCustomerId}/avatar_${nanoid(7)}` + : customer?.avatar ?? customerAvatar ?? null;
178-195
: Align created customer record with new avatar path variable namesFollow-through from the avatar fix: ensure created record uses the new IDs/paths.
- customer = await prisma.customer.create({ + customer = await prisma.customer.create({ data: { - id: finalCustomerId, + id: newCustomerId, name: finalCustomerName, email: customerEmail, - avatar: finalCustomerAvatar, + avatar: nextAvatarPath, externalId: customerExternalId, projectId: workspace.id, projectConnectId: workspace.stripeConnectId, clickId: clickData.click_id, linkId: clickData.link_id, country: clickData.country, clickedAt: new Date(clickData.timestamp + "Z"), }, });
261-288
: Keep per-customer stats consistent: increment customer.leads tooOnly link and workspace usage are incremented. Update the customer’s leads count alongside.
- const [link, _project] = await Promise.all([ + const [link, _project, _customer] = await Promise.all([ // update link leads count prisma.link.update({ where: { id: clickData.link_id, }, data: { leads: { increment: eventQuantity ?? 1, }, }, include: includeTags, }), // update workspace events usage prisma.project.update({ where: { id: workspace.id, }, data: { usage: { increment: eventQuantity ?? 1, }, }, }), + // update customer leads count + prisma.customer.update({ + where: { id: customer!.id }, + data: { leads: { increment: eventQuantity ?? 1 } }, + }), ]);
🧹 Nitpick comments (5)
apps/web/lib/api/conversions/track-lead.ts (5)
289-303
: eventQuantity > 1 maps multiple lead events to a single commission eventIdWhen recording multiple events, using one eventId in commissions can mislead cross-references. Prefer omitting eventId when quantity > 1.
await createPartnerCommission({ event: "lead", programId: link.programId, partnerId: link.partnerId, linkId: link.id, - eventId: leadEventId, + eventId: eventQuantity ? undefined : leadEventId, customerId: customer.id, quantity: eventQuantity ?? 1, context: { customer: { country: customer.country, }, }, });
52-64
: Deferred flow requires clickId, which limits the “priming” use-caseThrowing when clickId is absent prevents a first “deferred” call from creating/priming a new customer. If that’s intentional, clarify in docs and error text; otherwise, allow a true priming call (cache payload, return 202-like response, and create on the actual event).
Would you like a patch to support a no-clickId “priming” request (store payload under the deferred dedupe key and short-circuit without Tinybird lookups)?
42-50
: Narrow the select on findUnique to reduce I/OOnly id/clickId/avatar are read later. Use select to cut payload and index load.
- let customer = await prisma.customer.findUnique({ - where: { - projectId_externalId: { - projectId: workspace.id, - externalId: customerExternalId, - }, - }, - }); + let customer = await prisma.customer.findUnique({ + where: { + projectId_externalId: { + projectId: workspace.id, + externalId: customerExternalId, + }, + }, + select: { id: true, clickId: true, avatar: true, country: true }, + });
232-239
: rawBody may contain PII; consider redaction before logginglogConversionEvent persists the full request body. Mask common fields (email, name, identifiers) to avoid sensitive data leakage in analytics logs.
If helpful, I can add a small redaction util and wire it here.
156-176
: Guard eventQuantityEnsure zod clamps to a sane range (e.g., 1–100) to avoid excessive fan-out and cost. If not already enforced in the schema, add min/max.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/lib/api/conversions/track-lead.ts
(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/lib/api/conversions/track-lead.ts (9)
apps/web/lib/api/errors.ts (1)
DubApiError
(75-92)apps/web/lib/api/create-id.ts (1)
createId
(60-69)packages/utils/src/constants/main.ts (1)
R2_URL
(81-81)apps/web/lib/types.ts (1)
ClickEventTB
(522-522)apps/web/lib/partners/create-partner-commission.ts (1)
createPartnerCommission
(25-341)apps/web/lib/api/workflows/execute-workflows.ts (1)
executeWorkflows
(17-122)apps/web/lib/webhook/publish.ts (1)
sendWorkspaceWebhook
(7-36)apps/web/lib/webhook/transform.ts (1)
transformLeadEventData
(60-78)apps/web/lib/zod/schemas/leads.ts (1)
trackLeadResponseSchema
(76-86)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Vade Review
- GitHub Check: build
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/lib/api/conversions/track-lead.ts (1)
174-181
: Ensure partner commission eventId matches a recorded event for batched eventsWhen
eventQuantity > 1
, none of the recorded events usesleadEventId
, yet commissions reference it. Keep the first event’s ID equal toleadEventId
.- return eventQuantity - ? Array(eventQuantity) - .fill(null) - .map(() => ({ - ...basePayload, - event_id: nanoid(16), - })) - : basePayload; + return eventQuantity + ? Array(eventQuantity) + .fill(null) + .map((_, i) => ({ + ...basePayload, + event_id: i === 0 ? leadEventId : nanoid(16), + })) + : basePayload;
♻️ Duplicate comments (2)
apps/web/lib/api/conversions/track-lead.ts (2)
275-302
: Also increment the customer’s leads count to keep per-customer stats in syncLink and workspace are updated, but the customer’s
leads
isn’t.- const [link, _project] = await Promise.all([ + const [link, _project, _customer] = await Promise.all([ // update link leads count prisma.link.update({ @@ }), + // update customer leads count + prisma.customer.update({ + where: { id: customer!.id }, + data: { + leads: { + increment: eventQuantity ?? 1, + }, + }, + }), ]);
66-74
: Avatar uploads can be orphaned for existing customers; use existing customer.id and persist DB after uploadPath uses a new ID even when the customer exists, and the DB isn’t updated after upload.
const finalCustomerName = customerName || customerEmail || generateRandomName(); - const finalCustomerAvatar = - customerAvatar && !isStored(customerAvatar) - ? `${R2_URL}/customers/${finalCustomerId}/avatar_${nanoid(7)}` - : customerAvatar; + const avatarCustomerId = customer?.id ?? finalCustomerId; + const finalCustomerAvatar = + customerAvatar && !isStored(customerAvatar) + ? `${R2_URL}/customers/${avatarCustomerId}/avatar_${nanoid(7)}` + : customer?.avatar ?? customerAvatar ?? null; @@ // persist customer avatar to R2 await storage.upload( finalCustomerAvatar.replace(`${R2_URL}/`, ""), customerAvatar, { width: 128, height: 128, }, ); + // ensure DB points to the uploaded asset for existing customers + if (customer && customer.avatar !== finalCustomerAvatar) { + await prisma.customer.update({ + where: { id: customer.id }, + data: { avatar: finalCustomerAvatar }, + }); + }Also applies to: 254-268
🧹 Nitpick comments (1)
apps/web/lib/api/conversions/track-lead.ts (1)
138-138
: Nit: comment typo“from the from the” → “from the”.
- // get the referral link from the from the clickData + // get the referral link from the clickData
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/lib/api/conversions/track-lead.ts
(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/lib/api/conversions/track-lead.ts (8)
apps/web/lib/api/errors.ts (1)
DubApiError
(75-92)apps/web/lib/api/create-id.ts (1)
createId
(60-69)packages/utils/src/constants/main.ts (1)
R2_URL
(81-81)apps/web/lib/types.ts (1)
ClickEventTB
(522-522)apps/web/lib/api/workflows/execute-workflows.ts (1)
executeWorkflows
(17-122)apps/web/lib/webhook/publish.ts (1)
sendWorkspaceWebhook
(7-36)apps/web/lib/webhook/transform.ts (1)
transformLeadEventData
(60-78)apps/web/lib/zod/schemas/leads.ts (1)
trackLeadResponseSchema
(76-86)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Vade Review
- GitHub Check: build
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/web/lib/api/conversions/track-lead.ts (1)
66-73
: Potential issue with avatar path generation for existing customersThe
finalCustomerAvatar
path usesfinalCustomerId
even when the customer already exists. This could create inconsistent avatar paths and potential orphaned uploads when updating existing customers.Consider using the existing customer's ID when available:
+const avatarCustomerId = customer?.id ?? finalCustomerId; const finalCustomerAvatar = customerAvatar && !isStored(customerAvatar) - ? `${R2_URL}/customers/${finalCustomerId}/avatar_${nanoid(7)}` + ? `${R2_URL}/customers/${avatarCustomerId}/avatar_${nanoid(7)}` : customerAvatar;
🧹 Nitpick comments (1)
apps/web/lib/api/conversions/track-lead.ts (1)
341-352
: Handle potential null customer in responseWhen
customer
is null (which could happen in edge cases), the fallback usesfinalCustomerName
and other properties, but there's a potential issue with the response structure.Consider ensuring customer is always populated or handle the null case more explicitly:
return trackLeadResponseSchema.parse({ click: { id: clickId, }, - customer: customer ?? { + customer: customer ? { + name: customer.name, + email: customer.email, + avatar: customer.avatar, + externalId: customer.externalId, + } : { name: finalCustomerName, email: customerEmail || null, avatar: finalCustomerAvatar || null, externalId: customerExternalId, }, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/lib/api/conversions/track-lead.ts
(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/lib/api/conversions/track-lead.ts (9)
apps/web/lib/api/errors.ts (1)
DubApiError
(75-92)apps/web/lib/api/create-id.ts (1)
createId
(60-69)packages/utils/src/constants/main.ts (1)
R2_URL
(81-81)apps/web/lib/types.ts (1)
ClickEventTB
(522-522)apps/web/lib/partners/create-partner-commission.ts (1)
createPartnerCommission
(25-341)apps/web/lib/api/workflows/execute-workflows.ts (1)
executeWorkflows
(17-122)apps/web/lib/webhook/publish.ts (1)
sendWorkspaceWebhook
(7-36)apps/web/lib/webhook/transform.ts (1)
transformLeadEventData
(60-78)apps/web/lib/zod/schemas/leads.ts (1)
trackLeadResponseSchema
(76-86)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Vade Review
🔇 Additional comments (6)
apps/web/lib/api/conversions/track-lead.ts (6)
42-64
: LGTM! Clean implementation of customer lookup and clickId fallbackThe customer lookup by
projectId_externalId
and fallback logic for missingclickId
is well-implemented. The error message clearly explains the requirement for deferred mode tracking.
75-98
: LGTM! Fixed deduplication logicThe deduplication logic now correctly handles Redis
SET NX
responses wherenull
indicates the key already exists (duplicate event).
100-103
: LGTM! Clear conditional flow for duplicate handlingThe inverted condition properly allows processing for non-duplicate events and correctly handles deferred mode.
184-209
: LGTM! Proper upsert pattern with race condition handlingUsing
upsert
instead ofcreate
properly handles potential race conditions where multiple requests could try to create the same customer simultaneously.
275-336
: LGTM! Comprehensive background processing for non-deferred modesThe conditional processing correctly skips database updates, partner commissions, workflows, and webhooks for deferred mode while handling them properly for async and wait modes.
241-244
: No changes needed—deferred→async flow is covered by tests
The test “track a lead with mode = 'deferred' + track it again after with mode = 'async' and no clickId” in apps/web/tests/tracks/track-lead.test.ts already verifies that an async call processes a deferred lead correctly without being blocked by the initial deduplication key.
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/lib/api/conversions/track-lead.ts (1)
164-183
: Clamp eventQuantity at runtime and reuse the sanitized valueEven with schema constraints, runtime clamping is prudent and ensures consistent increments and payload fan-out.
@@ const leadEventId = nanoid(16); + const qty = + eventQuantity && Number.isFinite(eventQuantity) + ? Math.min(Math.max(1, Math.floor(eventQuantity)), 1000) + : 1; @@ - return eventQuantity - ? Array(eventQuantity) + return qty > 1 + ? Array(qty) .fill(null) .map(() => ({ ...basePayload, event_id: nanoid(16), })) : basePayload; @@ - prisma.link.update({ + prisma.link.update({ @@ - leads: { - increment: eventQuantity ?? 1, - }, + leads: { increment: qty }, @@ - prisma.project.update({ + prisma.project.update({ @@ - usage: { - increment: eventQuantity ?? 1, - }, + usage: { increment: qty }, @@ - quantity: eventQuantity ?? 1, + quantity: qty,Also applies to: 275-301, 311-312
♻️ Duplicate comments (3)
apps/web/lib/api/conversions/track-lead.ts (2)
275-301
: Keep per-customer stats consistent: increment customer.leads tooMirror link/project increments on the customer to avoid skewed dashboards.
- const [link, _project] = await Promise.all([ + const [link, _project, _customer] = await Promise.all([ // update link leads count prisma.link.update({ @@ }), // update workspace events usage prisma.project.update({ @@ }), + // update customer leads count + prisma.customer.update({ + where: { id: customer!.id }, + data: { leads: { increment: qty } }, + }), ]);
66-73
: Avatar uploads can orphan for existing customers; use existing id and persist pathReintroducing prior note: derive the avatar path from customer.id when available and update DB after upload to avoid dangling files.
@@ - const finalCustomerId = createId({ prefix: "cus_" }); + const finalCustomerId = createId({ prefix: "cus_" }); const finalCustomerName = customerName || customerEmail || generateRandomName(); - const finalCustomerAvatar = - customerAvatar && !isStored(customerAvatar) - ? `${R2_URL}/customers/${finalCustomerId}/avatar_${nanoid(7)}` - : customerAvatar; + const avatarCustomerId = customer?.id ?? finalCustomerId; + const nextAvatarPath = + customerAvatar && !isStored(customerAvatar) + ? `${R2_URL}/customers/${avatarCustomerId}/avatar_${nanoid(7)}` + : customer?.avatar ?? customerAvatar ?? null; @@ customer = await prisma.customer.upsert({ @@ create: { id: finalCustomerId, name: finalCustomerName, email: customerEmail, - avatar: finalCustomerAvatar, + avatar: nextAvatarPath, @@ update: {}, }); @@ - if ( - customerAvatar && - !isStored(customerAvatar) && - finalCustomerAvatar - ) { + if (customerAvatar && !isStored(customerAvatar) && nextAvatarPath) { // persist customer avatar to R2 await storage.upload( - finalCustomerAvatar.replace(`${R2_URL}/`, ""), + nextAvatarPath.replace(`${R2_URL}/`, ""), customerAvatar, { width: 128, height: 128, }, ); + // ensure existing customers point to the uploaded asset + if (customer && customer.avatar !== nextAvatarPath) { + await prisma.customer.update({ + where: { id: customer.id }, + data: { avatar: nextAvatarPath }, + }); + } } @@ return trackLeadResponseSchema.parse({ click: { id: clickId, }, - customer: customer ?? { + customer: customer ?? { name: finalCustomerName, - email: customerEmail || null, - avatar: finalCustomerAvatar || null, + email: customerEmail || null, + avatar: nextAvatarPath || null, externalId: customerExternalId, }, });Also applies to: 184-209, 254-268, 341-351
apps/web/tests/tracks/track-lead.test.ts (1)
87-119
: Set mode='async' on the second call and assert a side-effect.Without explicitly passing
mode: "async"
, the second call may rely on defaults and can be misread if dedupe prevents recording. Make intent explicit; also consider asserting a side-effect (e.g., usage increment/webhook fired) to prove the deferred→async flow actually records.const response2 = await http.post<TrackLeadResponse>({ path: "/track/lead", body: { eventName: "Mode=Deferred Signup", customerExternalId: customer2.externalId, + mode: "async", }, });
If the harness exposes a way to verify persistence/usage increments, add an assertion after
response2
to confirm the lead was recorded.
🧹 Nitpick comments (4)
apps/web/lib/api/conversions/track-sale.ts (1)
48-55
: Unify early-return payloads with the response schemaThe two early returns still handcraft objects. To keep a single contract and catch type drift, parse them through trackSaleResponseSchema too.
@@ - if (!ok) { - return { - eventName, - customer: null, - sale: null, - }; - } + if (!ok) { + return trackSaleResponseSchema.parse({ + eventName, + customer: null, + sale: null, + }); + } @@ - return { - eventName, - customer: null, - sale: null, - }; + return trackSaleResponseSchema.parse({ + eventName, + customer: null, + sale: null, + });Also applies to: 77-82
apps/web/tests/tracks/track-lead-client.test.ts (1)
47-52
: Make the assertion resilient to future fields/normalization.Strict equality on the entire
customer
object is brittle if the API adds fields or normalizes email/avatar. Prefer partial match.- expect(leadResponse).toStrictEqual({ + expect(leadResponse).toMatchObject({ click: { id: clickId, }, - customer: customer, + customer: expect.objectContaining({ + externalId: customer.externalId, + name: customer.name, + email: customer.email, + avatar: customer.avatar, + }), });apps/web/tests/tracks/track-lead.test.ts (2)
66-85
: Good idempotency check; consider codifying update semantics.Add a follow-up duplicate call with changed non-key fields (e.g., name/email) to assert whether duplicates with the same
customerExternalId
update or preserve original values. This locks behavior and prevents regressions.
154-175
: Exercise eventQuantity edge cases and effects.Add a negative-path test (e.g.,
eventQuantity: 0
or< 0
) and, if possible, a side-effect assertion that quantity >1 impacts downstream counts/ingestion.Example additional test (placement outside this block is fine):
test("rejects non-positive eventQuantity", async () => { const customer = randomCustomer(); const res = await http.post<TrackLeadResponse>({ path: "/track/lead", body: { clickId: trackedClickId, eventName: "Start Trial", customerExternalId: customer.externalId, customerName: customer.name, customerEmail: customer.email, customerAvatar: customer.avatar, eventQuantity: 0, }, }); expect(res.status).toBeGreaterThanOrEqual(400); });Also consider changing the helper to allow additive fields:
// Update expectValidLeadResponse to use objectContaining expect(response.data).toMatchObject({ click: { id: clickId }, customer: expect.objectContaining(customer), });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/web/lib/api/conversions/track-lead.ts
(8 hunks)apps/web/lib/api/conversions/track-sale.ts
(1 hunks)apps/web/lib/zod/schemas/leads.ts
(3 hunks)apps/web/tests/tracks/track-lead-client.test.ts
(1 hunks)apps/web/tests/tracks/track-lead.test.ts
(3 hunks)apps/web/tests/tracks/track-sale-client.test.ts
(0 hunks)apps/web/tests/tracks/track-sale.test.ts
(0 hunks)
💤 Files with no reviewable changes (2)
- apps/web/tests/tracks/track-sale-client.test.ts
- apps/web/tests/tracks/track-sale.test.ts
🔇 Additional comments (3)
apps/web/lib/api/conversions/track-sale.ts (1)
265-275
: Response-shape simplification looks goodReturning the parsed schema with nested sale is consistent and reduces drift with tests.
apps/web/lib/zod/schemas/leads.ts (1)
52-57
: Addition of mode="deferred" is clear and consistentEnum + default reads well and matches the new flow.
apps/web/lib/api/conversions/track-lead.ts (1)
75-98
: Dedup logic now correctly treats NX “OK” as first-seenGood fix; this prevents skipping first-time events.
clickId: z | ||
.string({ required_error: "clickId is required" }) | ||
.string() | ||
.trim() | ||
.min(1, "clickId is required") | ||
.nullish() | ||
.describe( | ||
"The unique ID of the click that the lead conversion event is attributed to. You can read this value from `dub_id` cookie.", | ||
"The unique ID of the click that the lead conversion event is attributed to. You can read this value from `dub_id` cookie. If not provided, Dub will try to find an existing customer with the provided `customerExternalId` and use the `clickId` from the customer if found.", | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Prevent empty clickId strings
As written, "" passes validation but later yields not_found. Require non-empty when present.
- clickId: z
- .string()
- .trim()
- .nullish()
+ clickId: z
+ .string()
+ .trim()
+ .min(1, "clickId cannot be empty")
+ .nullish()
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
clickId: z | |
.string({ required_error: "clickId is required" }) | |
.string() | |
.trim() | |
.min(1, "clickId is required") | |
.nullish() | |
.describe( | |
"The unique ID of the click that the lead conversion event is attributed to. You can read this value from `dub_id` cookie.", | |
"The unique ID of the click that the lead conversion event is attributed to. You can read this value from `dub_id` cookie. If not provided, Dub will try to find an existing customer with the provided `customerExternalId` and use the `clickId` from the customer if found.", | |
), | |
clickId: z | |
.string() | |
.trim() | |
.min(1, "clickId cannot be empty") | |
.nullish() | |
.describe( | |
"The unique ID of the click that the lead conversion event is attributed to. You can read this value from `dub_id` cookie. If not provided, Dub will try to find an existing customer with the provided `customerExternalId` and use the `clickId` from the customer if found.", | |
), |
🤖 Prompt for AI Agents
In apps/web/lib/zod/schemas/leads.ts around lines 8-14, the clickId schema
currently allows empty strings ("" passes after trim) which later causes
not_found; update the schema so that when clickId is present (not
null/undefined) it must be a non-empty string — either replace the chain with a
nullable/optional string plus a refine that returns true for null/undefined or
for strings with length > 0 (e.g. .refine(v => v == null || v.length > 0,
"clickId cannot be empty")), or use an equivalent approach (.min(1) guarded by a
null check) so empty trimmed strings are rejected while still allowing
null/undefined.
eventQuantity: z | ||
.number() | ||
.nullish() | ||
.describe( | ||
"The numerical value associated with this lead event (e.g., number of provisioned seats in a free trial). If defined as N, the lead event will be tracked N times.", | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Bound and sanitize eventQuantity to avoid resource exhaustion
This value drives array allocation and downstream increments. Constrain to safe integers in a reasonable range (e.g., 1..1000).
- eventQuantity: z
- .number()
- .nullish()
+ eventQuantity: z
+ .number()
+ .int("eventQuantity must be an integer")
+ .positive("eventQuantity must be >= 1")
+ .max(1000, "eventQuantity must be <= 1000")
+ .nullish()
.describe(
- "The numerical value associated with this lead event (e.g., number of provisioned seats in a free trial). If defined as N, the lead event will be tracked N times.",
+ "The numerical value associated with this lead event (e.g., provisioned seats). If defined as N, the event will be tracked N times (1–1000).",
),
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
eventQuantity: z | |
.number() | |
.nullish() | |
.describe( | |
"The numerical value associated with this lead event (e.g., number of provisioned seats in a free trial). If defined as N, the lead event will be tracked N times.", | |
), | |
eventQuantity: z | |
.number() | |
.int("eventQuantity must be an integer") | |
.positive("eventQuantity must be >= 1") | |
.max(1000, "eventQuantity must be <= 1000") | |
.nullish() | |
.describe( | |
"The numerical value associated with this lead event (e.g., provisioned seats). If defined as N, the event will be tracked N times (1–1000).", | |
), |
Summary by CodeRabbit
New Features
Enhancements
Tests