+
Skip to content

Conversation

steven-tey
Copy link
Collaborator

@steven-tey steven-tey commented Sep 8, 2025

Summary by CodeRabbit

  • New Features

    • Added a "deferred" lead-tracking mode and multi-event tracking (eventQuantity).
  • Enhancements

    • Allow tracking without clickId by resolving from customerExternalId and caching attribution.
    • Request schema tightened (customerExternalId required; clickId optional; mode adds "deferred").
    • Responses now return nested click and customer objects; sale details are nested under sale.
    • Mode-aware deduplication, avatar persistence, clearer workspace/link errors, and refined background handling for usage, partners, workflows, and webhooks.
  • Tests

    • Updated tests for deferred/async flows, duplicate customerExternalId flows, and eventQuantity scenarios.

Copy link
Contributor

vercel bot commented Sep 8, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Updated (UTC)
dub Ready Ready Preview Sep 8, 2025 5:24pm

Copy link
Contributor

coderabbitai bot commented Sep 8, 2025

Walkthrough

Updates 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

Cohort / File(s) Summary
Lead tracking API flow
apps/web/lib/api/conversions/track-lead.ts
Signature and input shape changed (adds rawBody, workspace); import trimmed to only WorkflowTrigger; makes clickId optional and looks up customer by workspace.id+customerExternalId; inline upsert for missing customers; computes finalCustomerId/avatar path; Redis per-event dedupe (1w, NX); Tinybird click lookup with Redis fallback; validates link belongs to workspace; mode-aware processing (async/wait/deferred) with sync vs background handling; emits webhooks, partner/workflow side effects in non-deferred flows; returns parsed { click, customer }.
Schema adjustments
apps/web/lib/zod/schemas/leads.ts
trackLeadRequestSchema changed: clickId is now optional/nullish; customerExternalId now required non-empty; eventName validation cleaned; mode enum extended to include "deferred" (default remains "async").
Tests — lead tracking
apps/web/tests/tracks/track-lead.test.ts
apps/web/tests/tracks/track-lead-client.test.ts
Test expectations updated to nested click and customer objects (removed top-level clickId, customerName, customerEmail, customerAvatar); added/renamed tests for deferred two-step flow and retained eventQuantity coverage.
Sale response shape
apps/web/lib/api/conversions/track-sale.ts
Simplified return: return parsed trackSaleResponseSchema directly with sale fields nested under sale (removed duplicate top-level sale fields).
Tests — sale tracking
apps/web/tests/tracks/track-sale-client.test.ts
apps/web/tests/tracks/track-sale.test.ts
Adjusted assertions to expect sale fields inside sale object instead of at root; removed duplicate root-level checks.

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
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Suggested reviewers

  • devkiran

Poem

I nibble bytes and hop through leads,
Deferred hops, async seeds, and queued deeds,
A click, a customer, avatars take flight,
I cache, dedupe, enqueue — carrots all night,
Tinybird hums while background workers bite. 🥕🐇

✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch deferred-mode

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 and metadata may be undefined based on the inputs; the schema expects nullable, not optional. Parsing will throw at runtime if they’re undefined.

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 values

As-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 because ok is false.
  • Only “wait” mode caches a lead payload in Redis (Lines 197-224); “deferred” does not, yet /track/sale looks up leadCache:* 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 via leadCache.

-        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 clickId

Since clickId is now optional and the API falls back to an existing customer’s clickId (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 logic

Server 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 description

Explain 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 equality

Prefer === 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

📥 Commits

Reviewing files that changed from the base of the PR and between e643135 and b902600.

📒 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 externalId

Making customerExternalId non-empty avoids silent “anonymous” customers.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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.leads

Link 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 eventQuantity

Nice 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 semantics

Using 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

📥 Commits

Reviewing files that changed from the base of the PR and between fcbbbd6 and 305047c.

📒 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 customerExternalId

This verifies duplicate requests return the same payload. No issues spotted.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 id

When 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 event

Using 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 update

When 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 response

Drop 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 consistent

Post-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 fallback

If 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.webhookEnabled

Avoid 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 logging

If 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

📥 Commits

Reviewing files that changed from the base of the PR and between 305047c and 8d6e9fa.

📒 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

@panda-sandeep
Copy link

/bug0 run

1 similar comment
@panda-sandeep
Copy link

/bug0 run

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 omitted

trackSaleResponseSchema 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 lead

Currently 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 customers

When 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/project

Keep 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 objects

console.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 logging

Avoid 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 response

Avoid 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-effects

Validate 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

📥 Commits

Reviewing files that changed from the base of the PR and between 8d6e9fa and 847343c.

📒 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 good

Covers 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 } schema

Fallback customer fields are correctly null-safe.

@panda-sandeep
Copy link

/bug0 run

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 call

The 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.avatar

After 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 DB

Avatar 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 names

Follow-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 too

Only 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 eventId

When 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-case

Throwing 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/O

Only 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 logging

logConversionEvent 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 eventQuantity

Ensure 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

📥 Commits

Reviewing files that changed from the base of the PR and between 847343c and 17b177a.

📒 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

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 events

When eventQuantity > 1, none of the recorded events uses leadEventId, yet commissions reference it. Keep the first event’s ID equal to leadEventId.

-      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 sync

Link 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 upload

Path 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

📥 Commits

Reviewing files that changed from the base of the PR and between 17b177a and 043dda8.

📒 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

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 customers

The finalCustomerAvatar path uses finalCustomerId 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 response

When customer is null (which could happen in edge cases), the fallback uses finalCustomerName 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

📥 Commits

Reviewing files that changed from the base of the PR and between 043dda8 and ce78a95.

📒 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 fallback

The customer lookup by projectId_externalId and fallback logic for missing clickId is well-implemented. The error message clearly explains the requirement for deferred mode tracking.


75-98: LGTM! Fixed deduplication logic

The deduplication logic now correctly handles Redis SET NX responses where null indicates the key already exists (duplicate event).


100-103: LGTM! Clear conditional flow for duplicate handling

The inverted condition properly allows processing for non-duplicate events and correctly handles deferred mode.


184-209: LGTM! Proper upsert pattern with race condition handling

Using upsert instead of create 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 modes

The 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.

@steven-tey
Copy link
Collaborator Author

@coderabbitai full review

Copy link
Contributor

coderabbitai bot commented Sep 8, 2025

✅ Actions performed

Full review triggered.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 value

Even 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 too

Mirror 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 path

Reintroducing 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 schema

The 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

📥 Commits

Reviewing files that changed from the base of the PR and between c8b169f and ce78a95.

📒 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 good

Returning 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 consistent

Enum + 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-seen

Good fix; this prevents skipping first-time events.

Comment on lines 8 to 14
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.",
),
Copy link
Contributor

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.

Suggested change
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.

Comment on lines 58 to 63
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.",
),
Copy link
Contributor

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.

Suggested change
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).",
),

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

点击 这是indexloc提供的php浏览器服务,不要输入任何密码和下载