Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds client-side conversion-tracking: new demo pages, SDK types and hooks (trackLead, trackSale), a new script extension that POSTS to API endpoints using a publishable key, expanded script build targets and package exports, and minor base-script public API augmentation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant P as Page / App
participant S as Stub (window.dubAnalytics)
participant B as Base Script (_dubAnalytics)
participant E as Conversion Ext
participant API as API Host
U->>P: Click "Track Lead"/"Track Sale"
P->>S: dubAnalytics.trackLead/trackSale(event)
Note right of S: If real script not loaded, call queued via qm
rect rgba(230,240,255,0.5)
P-->>B: Inject script (data-api-host, data-publishable-key)
B-->>E: Load conversion-tracking extension
end
E->>B: Read api host (a) and key (k)
E->>S: Attach trackLead/trackSale handlers
E->>E: Drain queued lead/sale events
alt Lead
E->>API: POST /track/lead/client (Bearer <key>, JSON)
API-->>E: JSON response
else Sale
E->>API: POST /track/sale/client (Bearer <key>, JSON)
API-->>E: JSON response
end
E-->>P: Promise resolves
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (12)
packages/script/build.js (1)
79-92: Consider an “all-in-one” bundle that also includes client-conversion-trackingIf consumers rely on the combined artifact for convenience, consider adding the new extension there as well (or producing a new “all features” bundle), to avoid forcing multiple <script> tags.
Proposed diff to include the extension and emit a distinct filename:
// Complete script with concatenated feature names esbuild.build({ ...baseConfig, stdin: { contents: combineFiles([ 'src/base.js', 'src/extensions/site-visit.js', 'src/extensions/outbound-domains.js', + 'src/extensions/client-conversion-tracking.js', ]), resolveDir: __dirname, sourcefile: 'combined.js', }, - outfile: 'dist/analytics/script.site-visit.outbound-domains.js', + outfile: + 'dist/analytics/script.site-visit.outbound-domains.client-conversion-tracking.js', }),packages/script/src/base.js (1)
12-12: Reading publishable key — handle absence and document usageGood to surface PUBLISHABLE_KEY via script attribute. If the key is required by downstream extensions, consider warning once when it’s absent to aid integration debugging.
Suggested minimal guard:
-const PUBLISHABLE_KEY = script.getAttribute('data-publishable-key'); +const PUBLISHABLE_KEY = script.getAttribute('data-publishable-key'); +if (!PUBLISHABLE_KEY) { + console.warn('[dubAnalytics] data-publishable-key is missing.'); +}If you want to harden script detection (edge cases where document.currentScript is null), I can propose a small helper outside this hunk.
apps/nextjs/app/client-tracking/client-conversion-tracking.tsx (2)
8-14: Avoid hard-coded identifiers in demo handler; consider making payload dynamic.Using fixed clickId/customerExternalId is fine for a demo, but easy to copy into production. Prefer deriving these from app state/props or an input control.
Apply this small refactor to keep the handler stable across renders and ready for real data:
-import { useAnalytics } from '@dub/analytics/react'; +import { useAnalytics } from '@dub/analytics/react'; +import { useCallback } from 'react'; export function ClientConversionTracking() { const { trackLead, trackSale } = useAnalytics(); - const handleTrackLead = () => { + const handleTrackLead = useCallback(() => { trackLead({ clickId: 'W13FJbgeLIGdlx7s', eventName: 'Account created', customerExternalId: '1234567890', }); - }; + }, [trackLead]);
16-23: Confirm amount units and consider including currency/invoiceId in the sale payload.Backends often expect amount in the smallest currency unit (e.g., cents). If that’s the case, 5000 implies $50. Also, providing currency and invoiceId improves traceability.
Proposed tweak:
const handleTrackSale = () => { trackSale({ eventName: 'Purchase completed', customerExternalId: 'CXvG5QOLi8QKBA2jYmDh', paymentProcessor: 'stripe', - amount: 5000, + amount: 5000, // cents? verify backend expectation + currency: 'usd', + invoiceId: 'inv_demo_001', }); };packages/web/src/generic.ts (1)
22-27: Stubbing trackLead/trackSale unconditionally can silently queue unsupported calls. Add a warning when publishableKey is absent.If publishableKey isn’t provided, the client-conversion-tracking feature isn’t loaded, but these methods will still enqueue. A direct warning helps developers notice misconfiguration early.
Apply after the IIFE:
})(window as DubAnalyticsWindow, 'dubAnalytics'); + // Warn if conversion tracking methods are used without enabling the feature + if (!props.publishableKey) { + const warn = (m: string) => + // eslint-disable-next-line no-console + console.warn( + `[Dub Web Analytics] ${m} is disabled: publishableKey not set in inject().`, + ); + // Replace stubs with warnings to avoid silent queuing + (window as any).dubAnalytics.trackLead = () => warn('trackLead'); + (window as any).dubAnalytics.trackSale = () => warn('trackSale'); + }packages/script/src/extensions/client-conversion-tracking.js (2)
46-53: Return the Promise from public wrappers to enable awaiting results.This improves composability and testability without changing behavior for current callers.
- window.dubAnalytics.trackLead = function (...args) { - trackLead(...args); - }; + window.dubAnalytics.trackLead = function (...args) { + return trackLead(...args); + }; - window.dubAnalytics.trackSale = function (...args) { - trackSale(...args); - }; + window.dubAnalytics.trackSale = function (...args) { + return trackSale(...args); + };
70-75: Ensure init runs even if the page has already loaded.If this script is appended after the load event, the listener won’t fire. Check readyState and fallback to DOMContentLoaded.
-if (window._dubAnalytics) { - initClientConversionTracking(); -} else { - window.addEventListener('load', initClientConversionTracking); -} +if (window._dubAnalytics) { + initClientConversionTracking(); +} else if (document.readyState === 'complete' || document.readyState === 'interactive') { + initClientConversionTracking(); +} else { + window.addEventListener('DOMContentLoaded', initClientConversionTracking); +}apps/html/conversion-tracking.html (1)
26-39: Avoid committing real-looking publishable keys.Even if publishable, keys shouldn’t live in the repo. Prefer an env-injected value or placeholder in demos.
- script.setAttribute( - 'data-publishable-key', - 'dub_pk_BgyBCEJCPCGN3RN7oieLVHRs', - ); + // Replace with your publishable key for local testing + script.setAttribute('data-publishable-key', 'YOUR_PUBLISHABLE_KEY');packages/web/src/use-analytics.ts (1)
46-50: Tighten state type to avoid null spread risk and improve inference.State is initialized with a non-null object but typed as PartnerData | null, forcing a possibly-null spread later.
- const [data, setData] = useState<PartnerData | null>({ + const [data, setData] = useState<PartnerData>({ partner: null, discount: null, });No other changes required, as all setData usages assign a non-null object.
Also applies to: 90-95
packages/web/src/types.ts (3)
176-176: Use JSON-safe metadata instead of any.metadata as Record<string, any> invites non-serializable data (functions, DOM nodes) and runtime errors during JSON.stringify.
Apply this change in both interfaces:
- metadata?: Record<string, any>; + metadata?: Record<string, JsonValue>;Then add these JSON utility types once in this file:
// JSON-safe value types for payloads export type JsonPrimitive = string | number | boolean | null; export type JsonValue = JsonPrimitive | JsonValue[] | { [k: string]: JsonValue };Also applies to: 186-186
175-175: Normalize environment “mode”, add idempotency for sales, and document amount/currency.
- Constrain mode to a known set and expose it on sales too.
- Add an optional idempotencyKey to prevent duplicate sale captures.
- Be explicit about amount units and currency format to avoid rounding/locale bugs.
Proposed tweaks:
export interface TrackLeadInput { clickId: string; eventName: string; customerExternalId: string; customerName?: string | null; customerEmail?: string | null; customerAvatar?: string | null; - mode?: string; + /** 'test' for sandbox events, 'live' for production. Defaults to 'live'. */ + mode?: 'test' | 'live'; metadata?: Record<string, JsonValue>; } export interface TrackSaleInput { eventName: string; customerExternalId: string; paymentProcessor: string; - amount: number; + /** Amount in the smallest currency unit (e.g., cents). */ + amount: number; + /** 'test' for sandbox events, 'live' for production. Defaults to 'live'. */ + mode?: 'test' | 'live'; + /** Optional idempotency token to dedupe retries. */ + idempotencyKey?: string; invoiceId?: string | null; - currency?: string; + /** ISO 4217 currency code, e.g., 'USD'. Uppercase 3 letters. */ + currency?: string; - metadata?: Record<string, any>; + metadata?: Record<string, JsonValue>; }Optional follow-up: if you want stronger typing for payment processors without losing flexibility, consider:
export type PaymentProcessor = 'stripe' | 'paddle' | 'lemon-squeezy' | 'paypal' | (string & {}); // and then: paymentProcessor: PaymentProcessor;Also applies to: 179-187
172-174: PII fields on the client: add guidance and consider renaming avatar.These are personally identifiable. Add short guidance on consent/PII handling, and consider clarifying the avatar field as a URL.
Minimal doc tweak:
- customerName?: string | null; - customerEmail?: string | null; - customerAvatar?: string | null; + /** Optional display name. Only send if the user has consented. */ + customerName?: string | null; + /** Optional email (plain text). Prefer hashing where possible. Only send with consent. */ + customerEmail?: string | null; + /** Optional avatar URL (https). */ + customerAvatar?: string | null;If this is unreleased, consider renaming to customerAvatarUrl for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
apps/html/conversion-tracking.html(1 hunks)apps/nextjs/app/client-tracking/client-conversion-tracking.tsx(1 hunks)apps/nextjs/app/client-tracking/page.tsx(1 hunks)packages/script/build.js(1 hunks)packages/script/package.json(1 hunks)packages/script/src/base.js(2 hunks)packages/script/src/extensions/client-conversion-tracking.js(1 hunks)packages/web/src/generic.ts(3 hunks)packages/web/src/react.tsx(2 hunks)packages/web/src/types.ts(2 hunks)packages/web/src/use-analytics.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
apps/nextjs/app/client-tracking/page.tsx (1)
apps/nextjs/app/client-tracking/client-conversion-tracking.tsx (1)
ClientConversionTracking(5-41)
apps/nextjs/app/client-tracking/client-conversion-tracking.tsx (2)
packages/web/src/use-analytics.ts (1)
useAnalytics(45-96)packages/script/src/extensions/client-conversion-tracking.js (2)
trackLead(5-22)trackSale(25-42)
packages/script/src/extensions/client-conversion-tracking.js (1)
packages/script/src/base.js (4)
API_HOST(11-11)PUBLISHABLE_KEY(12-12)queueManager(127-157)method(135-135)
packages/script/src/base.js (1)
packages/script/src/extensions/client-conversion-tracking.js (1)
queueManager(57-57)
packages/web/src/use-analytics.ts (3)
packages/web/src/types.ts (2)
TrackLeadInput(168-177)TrackSaleInput(179-187)packages/script/src/extensions/client-conversion-tracking.js (2)
trackLead(5-22)trackSale(25-42)packages/web/src/utils.tsx (1)
isDubAnalyticsReady(5-7)
packages/web/src/types.ts (1)
packages/web/src/react.tsx (2)
TrackLeadInput(41-41)TrackSaleInput(42-42)
packages/web/src/generic.ts (1)
packages/script/src/base.js (1)
script(3-3)
🔇 Additional comments (12)
packages/script/build.js (1)
65-77: Client conversion tracking build step — order and config LGTMThe new build step concatenates base.js first and then the extension, uses the shared baseConfig, and writes to the expected outfile. No issues spotted.
apps/nextjs/app/client-tracking/page.tsx (1)
1-5: Client component correctly marked withuse clientConfirmed that
apps/nextjs/app/client-tracking/client-conversion-tracking.tsxbegins with the'use client'directive, so the component is properly marked for client-side rendering. No further changes needed.packages/script/src/base.js (1)
275-277: Expose publishable key and queue manager — confirm public API stabilityExposing
k(publishable key) andqm(queue manager) is practical for extensions. Marking this as public API means we should treat these keys as stable; consider adding them to the README so integrators know they’re supported and not internal.Additionally, verify the client-conversion-tracking extension:
- Reads the key from
_dubAnalytics.k.- Flushes queued
trackLead/trackSalecalls analogous to how base flushestrackClick.If helpful, I can generate a quick trace checklist or tests for the queue behavior.
packages/web/src/react.tsx (1)
3-9: ✔ Verified useAnalytics hook and type exports
packages/web/src/use-analytics.ts: confirmstrackLead(lines 70–75) andtrackSale(lines 78–83) are defined and returned (lines 91–94).packages/web/src/types.ts: only one declaration each forTrackLeadInput(line 168) andTrackSaleInput(line 179); no duplicates found.All checks pass—approving these changes.
apps/nextjs/app/client-tracking/client-conversion-tracking.tsx (1)
25-41: UI wiring looks good.Buttons correctly invoke handlers; Tailwind classes are fine for the demo.
packages/web/src/generic.ts (1)
35-41: Feature flagging for client-conversion-tracking is sensible.Pushing client-conversion-tracking when publishableKey is supplied is a clean gating mechanism.
apps/html/conversion-tracking.html (2)
18-23: Stub shape matches the web API.Good job exposing trackClick, trackLead, and trackSale on the stub to queue calls pre-load.
44-50: PII caution for demo lead payload.customerEmail and customerName are fine in a demo, but remind integrators to ensure their policies/DPAs permit sending this PII to the analytics endpoint.
Would you like a short “privacy considerations” section added to the demo HTML explaining what’s sent and why?
packages/web/src/use-analytics.ts (1)
70-85: New wrappers look consistent and safe.Guards match trackClick behavior; forwarding to window.dubAnalytics is correct.
packages/web/src/types.ts (3)
169-169: Should clickId be optional and auto-populated from cookies?If your client script already derives the click ID from the cookie, making this optional reduces integrator friction and avoids mismatch bugs.
If you decide to relax it:
- clickId: string; + clickId?: string;Please confirm the runtime fills it from the cookie when omitted and surfaces a clear error if neither is present.
168-187: Single source of truth forTrackLeadInputandTrackSaleInput
I ran the ripgrep command acrosspackages/**:rg -n --glob 'packages/**' -C1 'export\s+interface\s+Track(Lead|Sale)Input\b'and confirmed the only definitions are in
packages/web/src/types.tsat lines 168 and 179. No duplicate declarations were found.
8-12: Document publishableKey requirement and safe usage in types.tsThe existing implementation correctly reads
props.publishableKeyand injects it asdata-publishable-keyonly when provided. To clarify when this key is needed and avoid confusion with secret keys, please apply the following doc-only update inpackages/web/src/types.ts:/** - * The publishable key for client conversion tracking - * @example 'dub_pk_BgyBCEJCPCGN3RN7oieLVHRs' + * Publishable (public) key used for client-side conversion tracking (trackLead/trackSale). + * Required when enabling the client-conversion-tracking feature. Do not use your secret key. + * Typically starts with "dub_pk_". + * @example 'dub_pk_BgyBCEJCPCGN3RN7oieLVHRs' */ publishableKey?: string;
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/script/package.json (1)
9-13: Verify new conversion-tracking bundles are included in the published tarball.The four newly listed dist files look consistent with build.js outputs. Please run an npm pack dry-run to ensure they’re actually present at publish time and catch any path/name drift early.
Run from repo root:
#!/bin/bash set -euo pipefail cd packages/script echo "== npm pack --dry-run contents ==" npm pack --dry-run > pack.txt echo echo "== Expecting 8 analytics bundles ==" expected=( "dist/analytics/script.js" "dist/analytics/script.site-visit.js" "dist/analytics/script.outbound-domains.js" "dist/analytics/script.conversion-tracking.js" "dist/analytics/script.site-visit.outbound-domains.js" "dist/analytics/script.site-visit.conversion-tracking.js" "dist/analytics/script.outbound-domains.conversion-tracking.js" "dist/analytics/script.site-visit.outbound-domains.conversion-tracking.js" ) for f in "${expected[@]}"; do if ! rg -n --fixed-strings "$f" pack.txt >/dev/null; then echo "MISSING in pack: $f" else echo "OK: $f" fi done
🧹 Nitpick comments (5)
apps/nextjs/app/conversion-tracking/page-client.tsx (2)
8-23: Await and handle errors from tracking calls.Handlers currently drop the returned Promise and any errors. Make them async and handle failures to avoid unhandled rejections and to provide feedback in demos.
- const handleTrackLead = () => { - trackLead({ + const handleTrackLead = async () => { + try { + await trackLead({ clickId: 'W13FJbgeLIGdlx7s', eventName: 'Account created', customerExternalId: '1234567890', - }); + }); + console.log('[demo] Lead tracked'); + } catch (err) { + console.error('[demo] trackLead failed', err); + alert('Lead tracking failed — see console for details.'); + } - }; + };
16-23: Do the same for trackSale; optionally disable the buttons while a request is in-flight.Mirrors the lead handler change. If you’d like, add a tiny pending state to prevent double submissions in demos.
- const handleTrackSale = () => { - trackSale({ + const handleTrackSale = async () => { + try { + await trackSale({ eventName: 'Purchase completed', customerExternalId: 'CXvG5QOLi8QKBA2jYmDh', paymentProcessor: 'stripe', amount: 5000, - }); + }); + console.log('[demo] Sale tracked'); + } catch (err) { + console.error('[demo] trackSale failed', err); + alert('Sale tracking failed — see console for details.'); + } - }; + };If you decide to add pending UI, introduce:
const [pending, setPending] = useState<'lead'|'sale'|null>(null);- Toggle disabled on buttons with
disabled={pending !== null}and update state in the try/finally blocks.- <button + <button + disabled={false /* set to pending !== null if you add state */} onClick={handleTrackLead}Additional import (top of file) if you add state:
+import { useState } from 'react';Also applies to: 33-38
packages/script/src/extensions/conversion-tracking.js (3)
15-33: Harden network calls: handle network/JSON errors and add Accept header.Currently a non-JSON error body or a network failure will throw before your error logging runs. Wrap in try/catch and default error shapes.
- const trackLead = async (input) => { - const response = await fetch(`${API_HOST}/track/lead/client`, { + const trackLead = async (input) => { + try { + const response = await fetch(`${API_HOST}/track/lead/client`, { method: 'POST', headers: { 'Content-Type': 'application/json', + Accept: 'application/json', Authorization: `Bearer ${PUBLISHABLE_KEY}`, }, body: JSON.stringify(input), - }); - - const result = await response.json(); - - if (!response.ok) { - console.error('[dubAnalytics] trackLead failed', result.error); - } - - return result; + }); + const text = await response.text(); + const result = text ? JSON.parse(text) : {}; + if (!response.ok) { + console.error('[dubAnalytics] trackLead failed', result?.error || result); + } + return result; + } catch (err) { + console.error('[dubAnalytics] trackLead failed (network/parse error)', err); + throw err; + } }; @@ - const trackSale = async (input) => { - const response = await fetch(`${API_HOST}/track/sale/client`, { + const trackSale = async (input) => { + try { + const response = await fetch(`${API_HOST}/track/sale/client`, { method: 'POST', headers: { 'Content-Type': 'application/json', + Accept: 'application/json', Authorization: `Bearer ${PUBLISHABLE_KEY}`, }, body: JSON.stringify(input), - }); - - const result = await response.json(); - - if (!response.ok) { - console.error('[dubAnalytics] trackSale failed', result.error); - } - - return result; + }); + const text = await response.text(); + const result = text ? JSON.parse(text) : {}; + if (!response.ok) { + console.error('[dubAnalytics] trackSale failed', result?.error || result); + } + return result; + } catch (err) { + console.error('[dubAnalytics] trackSale failed (network/parse error)', err); + throw err; + } };Also applies to: 35-53
65-84: Avoid losing queued events on failure; process and requeue on error.The filter-based approach fires async calls without awaiting and drops items unconditionally. A transient failure will permanently discard those events.
- const existingQueue = queueManager.queue || []; - - const remainingQueue = existingQueue.filter(([method, ...args]) => { - if (method === 'trackLead') { - trackLead(...args); - return false; - } else if (method === 'trackSale') { - trackSale(...args); - return false; - } - - return true; - }); - - // Update the queue with remaining items - queueManager.queue = remainingQueue; + const existingQueue = Array.isArray(queueManager.queue) ? queueManager.queue.slice() : []; + const keep = []; + for (const item of existingQueue) { + const [method, ...args] = item; + if (method === 'trackLead') { + trackLead(...args).catch(() => queueManager.queue.push(item)); + } else if (method === 'trackSale') { + trackSale(...args).catch(() => queueManager.queue.push(item)); + } else { + keep.push(item); + } + } + queueManager.queue = keep;
87-92: Initialization may miss late base script loads.If this extension loads after window load has already fired and _dubAnalytics wasn’t present at evaluation time, init never runs. Consider a short-lived poll or listen to DOMContentLoaded as a backup.
Example minimal tweak:
-if (window._dubAnalytics) { - initConversionTracking(); -} else { - window.addEventListener('load', initConversionTracking); -} +if (window._dubAnalytics) { + initConversionTracking(); +} else { + const tryInit = () => window._dubAnalytics && (initConversionTracking(), true); + if (!tryInit()) { + window.addEventListener('DOMContentLoaded', tryInit, { once: true }); + window.addEventListener('load', tryInit, { once: true }); + const iv = setInterval(() => { if (tryInit()) clearInterval(iv); }, 100); + setTimeout(() => clearInterval(iv), 5000); // stop polling after 5s + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
apps/html/conversion-tracking.html(1 hunks)apps/nextjs/app/conversion-tracking/page-client.tsx(1 hunks)apps/nextjs/app/conversion-tracking/page.tsx(1 hunks)packages/script/build.js(5 hunks)packages/script/package.json(1 hunks)packages/script/src/extensions/conversion-tracking.js(1 hunks)packages/web/src/generic.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/html/conversion-tracking.html
- packages/web/src/generic.ts
🧰 Additional context used
🧬 Code graph analysis (3)
apps/nextjs/app/conversion-tracking/page.tsx (1)
apps/nextjs/app/conversion-tracking/page-client.tsx (1)
ConversionTrackingPageClient(5-41)
apps/nextjs/app/conversion-tracking/page-client.tsx (1)
packages/script/src/extensions/conversion-tracking.js (2)
trackLead(15-32)trackSale(35-52)
packages/script/src/extensions/conversion-tracking.js (1)
packages/script/src/base.js (2)
PUBLISHABLE_KEY(12-12)method(135-135)
🔇 Additional comments (3)
apps/nextjs/app/conversion-tracking/page.tsx (1)
3-5: LGTM — thin server component wrapper.Simple wrapper defers to the client component as intended.
apps/nextjs/app/conversion-tracking/page-client.tsx (1)
20-21: Ensure the correct currency unit foramountThe
TrackSaleInput.amountfield is a plainnumber, but it’s not immediately clear whether it should be sent in minor units (e.g. cents) or major units (e.g. dollars). You’re currently passing5000, which could mean $50.00 if interpreted as cents, or 5,000.00 in major units. To avoid downstream confusion:
- Verify the public API spec or backend implementation for
/track/sale/clientto confirm whetheramountis expected in minor currency units.- Once confirmed, update the client types or add inline documentation (e.g. JSDoc) to make this explicit.
- Optionally, consider renaming the field (e.g.
amountCents) or using a decimal/string type if the API expects major units.packages/script/build.js (1)
27-140: Alldist/analyticsoutputs in build.js match the entries in package.json’sfiles[]. No discrepancies detected.
- Verified the eight hard-coded
outfilepaths in packages/script/build.js align exactly with the eightdist/analytics/*.jsentries in packages/script/package.json.- Build matrix is correctly reflected in package metadata.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/web/src/types.ts (4)
172-174: PII handling guidance (email/name/avatar)Since these fields may carry PII, add a brief note in the JSDoc to encourage hashing or minimization where possible, and to avoid sending sensitive data if not needed. Helps downstream users stay compliant.
168-177: Constrainmetadatato JSON‐serializable values; upgrade property docsWe found two occurrences of
metadata?: Record<string, unknown>in packages/web/src/types.ts (lines 176 and 186). Let’s tighten both and align docs:• packages/web/src/types.ts:176
• packages/web/src/types.ts:186Apply this diff in those two spots:
--- a/packages/web/src/types.ts +++ b/packages/web/src/types.ts @@ -169,8 +169,11 @@ export interface TrackLeadInput { customerAvatar?: string; - mode?: string; - metadata?: Record<string, unknown>; + /** Optional environment label (e.g., 'test' or 'live'). */ + mode?: string; + /** Structured metadata; must be JSON‐serializable. */ + metadata?: Record<string, JSONValue>; } export interface AnotherInput { @@ -183,8 +186,11 @@ export interface AnotherInput { someOtherField: number; - mode?: string; - metadata?: Record<string, unknown>; + /** Optional environment label (e.g., 'test' or 'live'). */ + mode?: string; + /** Structured metadata; must be JSON‐serializable. */ + metadata?: Record<string, JSONValue>; }And add these definitions near the top of the file (once):
// Place near the top of this file export type JSONPrimitive = string | number | boolean | null; export type JSONValue = JSONPrimitive | { [k: string]: JSONValue } | JSONValue[];
8-12: ClarifypublishableKeyJSDoc and add optional runtime format checkChecked across
packages/webwithrg -nC2 '\bpublishableKey\b|publicKey|pubKey': onlypublishableKeyappears intypes.tsandgeneric.ts—no other variants found.
- Update the JSDoc in
packages/web/src/types.ts(lines 8–12) to spell out that this key is required client-side fortrackLead/trackSaleand must begin withdub_pk_.- (Optional) In
packages/web/src/generic.ts(around lines 55–57), add a runtime check to warn if the prop doesn’t start withdub_pk_before setting the<script>attribute.Apply this doc-only diff to
types.ts:/** - * The publishable key for client conversion tracking + * Publishable key used for client-side conversion tracking (`trackLead`/`trackSale`). + * Required when sending conversion events from the browser. + * Must start with "dub_pk_" (e.g. `dub_pk_BgyBCEJCPCGN3RN7oieLVHRs`). * @example 'dub_pk_BgyBCEJCPCGN3RN7oieLVHRs' */ publishableKey?: string;Optional runtime safeguard in
generic.ts:if (props.publishableKey) { if (!props.publishableKey.startsWith('dub_pk_')) { console.warn('Invalid publishableKey format. Expected prefix "dub_pk_".'); } script.setAttribute('data-publishable-key', props.publishableKey); }
179-187: Optional: ClarifyTrackSaleInputsemantics and document fieldsTo reduce ambiguity and aid future maintainers, you can add JSDoc comments to:
amount: specify whether it’s in major (e.g. dollars) or minor units (e.g. cents), and recommend using integers to avoid floating-point rounding.currency: note that it should be an ISO 4217 code (e.g. “USD”, “EUR”).metadata: describe it as arbitrary JSON-safe data (you may keepRecord<string, unknown>or, if you prefer, introduce aJSONValueunion type first).If you decide to use a
JSONValuealias, you’ll need to define it in this file (it isn’t present in the repo):export type JSONValue = | string | number | boolean | null | JSONValue[] | { [key: string]: JSONValue };Suggested diff:
export interface TrackSaleInput { eventName: string; customerExternalId: string; paymentProcessor?: string; - amount: number; + /** Amount in minor currency units (e.g., cents). Use integers to avoid floating-point errors. */ + amount: number; invoiceId?: string; - currency?: string; - metadata?: Record<string, unknown>; + /** ISO 4217 currency code (e.g., 'USD', 'EUR'). */ + currency?: string; + /** Additional JSON-safe data. */ + metadata?: Record<string, unknown>; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/script/src/extensions/conversion-tracking.js(1 hunks)packages/web/src/types.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/script/src/extensions/conversion-tracking.js
🧰 Additional context used
🧬 Code graph analysis (1)
packages/web/src/types.ts (1)
packages/web/src/react.tsx (2)
TrackLeadInput(41-41)TrackSaleInput(42-42)
🔇 Additional comments (1)
packages/web/src/types.ts (1)
168-187: No duplicateTrackLeadInput/TrackSaleInputdeclarations found
A repo-wide search for both interfaces only returned the declarations at packages/web/src/types.ts:168 and 179, with no other occurrences. This concern can be resolved.
There was a problem hiding this comment.
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)
packages/web/src/generic.ts (1)
29-53: Add subpath exports for all analytics bundles in package.jsonThe
exportsfield is currently missing, so consumers cannot import the new conversion-tracking builds. Please add an"exports"section that maps each bundle under your package name:diff --git a/packages/script/package.json b/packages/script/package.json index abcdef0..1234567 100644 --- a/packages/script/package.json +++ b/packages/script/package.json @@ "files": [ "dist/analytics/script.js", "dist/analytics/script.site-visit.js", "dist/analytics/script.outbound-domains.js", "dist/analytics/script.conversion-tracking.js", "dist/analytics/script.site-visit.outbound-domains.js", "dist/analytics/script.site-visit.conversion-tracking.js", "dist/analytics/script.outbound-domains.conversion-tracking.js", "dist/analytics/script.site-visit.outbound-domains.conversion-tracking.js" ], + "exports": { + ".": "./dist/analytics/script.js", + "./site-visit": "./dist/analytics/script.site-visit.js", + "./outbound-domains": "./dist/analytics/script.outbound-domains.js", + "./conversion-tracking": "./dist/analytics/script.conversion-tracking.js", + "./site-visit.outbound-domains": "./dist/analytics/script.site-visit.outbound-domains.js", + "./site-visit.conversion-tracking": "./dist/analytics/script.site-visit.conversion-tracking.js", + "./outbound-domains.conversion-tracking": "./dist/analytics/script.outbound-domains.conversion-tracking.js", + "./site-visit.outbound-domains.conversion-tracking": "./dist/analytics/script.site-visit.outbound-domains.conversion-tracking.js" + }, "scripts": { "prebuild": "mkdir -p dist/analytics", "build": "node build.js" },• targets the four conversion-tracking variants plus the base, site-visit, outbound-domains, and their combination bundles.
• Ensures consumers can import, e.g.import '@dub/analytics-script/conversion-tracking'.
♻️ Duplicate comments (3)
apps/html/conversion-tracking.html (1)
54-61: Verify sale payload requirements (currency, optional fields).eventName is now present—nice. Confirm whether TrackSaleInput requires currency and/or paymentProcessor; if so, add them and ensure casing matches backend expectations (often 'USD').
Suggested change:
onclick="dubAnalytics.trackSale({ eventName: 'Purchase completed', customerExternalId: 'CXvG5QOLi8QKBA2jYmDh', invoiceId: Math.random().toString(36).substring(2, 15), amount: 5000, // defaults to usd cents, use `currency` prop to specify a different currency + // If required by your API: + // paymentProcessor: 'stripe', + // currency: 'USD', })"If the API truly defaults to USD cents server-side, feel free to ignore the currency addition.
packages/web/src/generic.ts (1)
55-57: Ensure base script actually reads data-publishable-key into window._dubAnalytics.k (and API host into .a).The extension relies on _dubAnalytics.k and .a. Please confirm base.js sets these from the injected script tag.
Run:
#!/bin/bash set -euo pipefail rg -n -C2 "window\._dubAnalytics\s*=" packages/script/src rg -n -C2 "window\._dubAnalytics\.(k|a)\b" packages/script/src rg -n -C2 "data-publishable-key|data-api-host" packages/script/srcIf missing, add:
- h: HOSTNAME, + h: HOSTNAME, + k: script.getAttribute('data-publishable-key') || '', + a: script.getAttribute('data-api-host') || '',packages/script/src/extensions/conversion-tracking.js (1)
66-75: Return Promises from public API and ensure window.dubAnalytics exists.Currently wrappers don’t return, so callers get undefined; also they no-op if dubAnalytics is falsy. Initialize the object and return the underlying call.
Apply:
- // Add methods to the global dubAnalytics object for direct calls - if (window.dubAnalytics) { - window.dubAnalytics.trackLead = function (...args) { - trackLead(...args); - }; - - window.dubAnalytics.trackSale = function (...args) { - trackSale(...args); - }; - } + // Add methods to the global dubAnalytics object for direct calls + const api = (window.dubAnalytics = window.dubAnalytics || {}); + api.trackLead = function (...args) { + return trackLead(...args); + }; + api.trackSale = function (...args) { + return trackSale(...args); + };
🧹 Nitpick comments (14)
apps/nextjs/app/conversion-tracking/page-client.tsx (2)
8-13: Optional: memoize handlers to keep stable identitiesIf these handlers are passed down or trigger effects, memoizing avoids unnecessary re-renders.
Apply this diff:
-'use client'; - -import { useAnalytics } from '@dub/analytics/react'; +'use client'; + +import { useCallback } from 'react'; +import { useAnalytics } from '@dub/analytics/react'; @@ -export function ConversionTrackingPageClient() { - const { trackLead, trackSale } = useAnalytics(); +export function ConversionTrackingPageClient() { + const { trackLead, trackSale } = useAnalytics(); @@ - const handleTrackLead = () => { - trackLead({ - eventName: 'Account created', - customerExternalId: '1234567890', - }); - }; + const handleTrackLead = useCallback(() => { + trackLead({ + eventName: 'Account created', + customerExternalId: '1234567890', + }); + }, [trackLead]); @@ - const handleTrackSale = () => { - trackSale({ - eventName: 'Purchase completed', - customerExternalId: 'CXvG5QOLi8QKBA2jYmDh', - amount: 5000, // defaults to usd cents, use `currency` prop to specify a different currency - }); - }; + const handleTrackSale = useCallback(() => { + trackSale({ + eventName: 'Purchase completed', + customerExternalId: 'CXvG5QOLi8QKBA2jYmDh', + amount: 5000, // defaults to usd cents; consider `currency` for clarity + }); + }, [trackSale]);Also applies to: 15-21
17-20: Be explicit about currency in demosAdding
currency: 'USD'reinforces the cents contract and avoids confusion for integrators skimming the sample.Apply this tiny diff:
- amount: 5000, // defaults to usd cents, use `currency` prop to specify a different currency + amount: 5000, + currency: 'USD', // explicit for demo; defaults to USD if omittedpackages/script/build.js (3)
21-25: Harden copy step for missing public/_redirects.fs.copyFileSync will throw if public/_redirects doesn’t exist in some environments. Guard or no-op on ENOENT to keep builds reproducible.
Apply:
-// Copy _redirects to dist folder -fs.copyFileSync( - path.join(__dirname, 'public/_redirects'), - path.join(__dirname, 'dist/_redirects'), -); +// Copy _redirects to dist folder (optional) +try { + fs.copyFileSync( + path.join(__dirname, 'public/_redirects'), + path.join(__dirname, 'dist/_redirects'), + ); +} catch (err) { + if (err.code !== 'ENOENT') throw err; +}
27-140: DRY the 8-target matrix to reduce maintenance risk.Today’s manual duplication is easy to desync from packages/web feature ordering. Generate targets programmatically.
Apply:
-// Build all variants (8 total combinations of 3 extensions) -Promise.all([ - // 1. Base script (no extensions) - esbuild.build({ ... }), - // 2. Site visit only - esbuild.build({ ... }), - // 3. Outbound domains only - esbuild.build({ ... }), - // 4. Conversion tracking only - esbuild.build({ ... }), - // 5. Site visit + Outbound domains - esbuild.build({ ... }), - // 6. Site visit + Conversion tracking - esbuild.build({ ... }), - // 7. Outbound domains + Conversion tracking - esbuild.build({ ... }), - // 8. All extensions combined - esbuild.build({ ... }), -]).catch(() => process.exit(1)); +// Build all variants (8 total combinations of 3 extensions) +const combos = [ + { files: ['src/base.js'], out: 'dist/analytics/script.js' }, + { files: ['src/base.js', 'src/extensions/site-visit.js'], out: 'dist/analytics/script.site-visit.js' }, + { files: ['src/base.js', 'src/extensions/outbound-domains.js'], out: 'dist/analytics/script.outbound-domains.js' }, + { files: ['src/base.js', 'src/extensions/conversion-tracking.js'], out: 'dist/analytics/script.conversion-tracking.js' }, + { files: ['src/base.js', 'src/extensions/site-visit.js', 'src/extensions/outbound-domains.js'], out: 'dist/analytics/script.site-visit.outbound-domains.js' }, + { files: ['src/base.js', 'src/extensions/site-visit.js', 'src/extensions/conversion-tracking.js'], out: 'dist/analytics/script.site-visit.conversion-tracking.js' }, + { files: ['src/base.js', 'src/extensions/outbound-domains.js', 'src/extensions/conversion-tracking.js'], out: 'dist/analytics/script.outbound-domains.conversion-tracking.js' }, + { files: ['src/base.js', 'src/extensions/site-visit.js', 'src/extensions/outbound-domains.js', 'src/extensions/conversion-tracking.js'], out: 'dist/analytics/script.site-visit.outbound-domains.conversion-tracking.js' }, +]; + +Promise.all( + combos.map(({ files, out }) => + esbuild.build({ + ...baseConfig, + stdin: { + contents: files.length === 1 ? fs.readFileSync(files[0], 'utf8') : combineFiles(files), + resolveDir: __dirname, + sourcefile: files.length === 1 ? path.basename(files[0]) : 'combined.js', + }, + outfile: out, + }), + ), +).catch(() => process.exit(1));
5-10: Optional: add dev sourcemaps toggle.Helps debugging in demo apps without bloating prod.
Apply:
const baseConfig = { bundle: true, minify: true, format: 'iife', target: 'es2015', + sourcemap: process.env.SOURCEMAP === '1', };apps/html/conversion-tracking.html (2)
26-39: Confirm script path resolves in your dev/deploy setup.This HTML lives under apps/html, while the build emits packages/script/dist/analytics. The relative ./analytics/... path may 404 unless your static host maps or copies assets.
Two options:
- Point to the CDN build for the demo page:
- script.src = './analytics/script.conversion-tracking.js'; + script.src = 'https://www.dubcdn.com/analytics/script.conversion-tracking.js';
- Or adjust the relative path to the local build output (if you serve directly from repo):
- script.src = './analytics/script.conversion-tracking.js'; + script.src = '../../packages/script/dist/analytics/script.conversion-tracking.js';Please verify which path your preview environment expects. I can wire a copy step if needed.
32-36: Avoid committing real keys; confirm this is a publishable (public) key.Publishable keys are fine client-side; avoid accidentally using a secret key. Consider reading from a query param or local .env-injected variable to keep the sample portable.
packages/web/src/generic.ts (1)
87-90: Use console.error for load failures.Semantically this is an error rather than an info log.
- console.log(`[Dub Web Analytics] failed to load script from ${src}.`); + console.error(`[Dub Web Analytics] failed to load script from ${src}.`);packages/web/src/use-analytics.ts (3)
16-25: Expose Promise-returning signatures for trackLead/trackSale.The extension methods are async; returning their Promises enables callers to await results and handle errors.
Apply:
interface Window { DubAnalytics: PartnerData; dubAnalytics: ((event: 'ready', callback: () => void) => void) & { - trackClick: (event: TrackClickInput) => void; - trackLead: (event: TrackLeadInput) => void; - trackSale: (event: TrackSaleInput) => void; + trackClick: (event: TrackClickInput) => void; + trackLead: (event: TrackLeadInput) => Promise<unknown>; + trackSale: (event: TrackSaleInput) => Promise<unknown>; }; }And return the underlying calls in the hook (see below).
70-85: Return the underlying Promises from the hook wrappers.This keeps the React API ergonomic for flows that need to block on completion.
- const trackLead = useCallback((event: TrackLeadInput) => { + const trackLead = useCallback((event: TrackLeadInput) => { if (!isDubAnalyticsReady()) { - return; + return Promise.resolve(undefined); } - - window.dubAnalytics.trackLead(event); + return window.dubAnalytics.trackLead(event); }, []); - const trackSale = useCallback((event: TrackSaleInput) => { + const trackSale = useCallback((event: TrackSaleInput) => { if (!isDubAnalyticsReady()) { - return; + return Promise.resolve(undefined); } - - window.dubAnalytics.trackSale(event); + return window.dubAnalytics.trackSale(event); }, []);
46-50: Tighten state typing to avoid null spreads.data is never set to null; remove the union and avoid potential spread-of-null issues in TS.
-export function useAnalytics() { - const [data, setData] = useState<PartnerData | null>({ +export function useAnalytics() { + const [data, setData] = useState<PartnerData>({ partner: null, discount: null, }); @@ return { - ...data, + ...data, trackClick, trackLead, trackSale, };Also applies to: 90-96
packages/script/src/extensions/conversion-tracking.js (2)
28-36: Make network handling resilient (keepalive, safe parsing, useful errors).Handle non-JSON/empty bodies and keep requests alive during unload.
- const response = await fetch(`${API_HOST}/track/lead/client`, { + const response = await fetch(`${API_HOST.replace(/\/$/, '')}/track/lead/client`, { method: 'POST', headers: { 'Content-Type': 'application/json', Authorization: `Bearer ${PUBLISHABLE_KEY}`, }, - body: JSON.stringify(requestBody), + body: JSON.stringify(requestBody), + keepalive: true, }); - - const result = await response.json(); + const text = await response.text().catch(() => ''); + let result; + try { result = text ? JSON.parse(text) : null; } catch { result = text || null; } if (!response.ok) { - console.error('[dubAnalytics] trackLead failed', result.error); + console.error('[dubAnalytics] trackLead failed', (result && (result.error || result.message)) || result || response.statusText); } return result;- const response = await fetch(`${API_HOST}/track/sale/client`, { + const response = await fetch(`${API_HOST.replace(/\/$/, '')}/track/sale/client`, { method: 'POST', headers: { 'Content-Type': 'application/json', Authorization: `Bearer ${PUBLISHABLE_KEY}`, }, - body: JSON.stringify(input), + body: JSON.stringify(input), + keepalive: true, }); - - const result = await response.json(); + const text = await response.text().catch(() => ''); + let result; + try { result = text ? JSON.parse(text) : null; } catch { result = text || null; } if (!response.ok) { - console.error('[dubAnalytics] trackSale failed', result.error); + console.error('[dubAnalytics] trackSale failed', (result && (result.error || result.message)) || result || response.statusText); } return result;Also applies to: 48-56, 37-44, 57-64
9-17: Optional: attach no-op API methods even when config is missing.Returning early prevents attaching trackLead/trackSale, which can surprise callers. Consider attaching stubs that warn and return a rejected Promise.
if (!API_HOST) { - console.warn('[dubAnalytics] Missing API_HOST'); - return; + console.warn('[dubAnalytics] Missing API_HOST'); } if (!PUBLISHABLE_KEY) { - console.warn('[dubAnalytics] Missing PUBLISHABLE_KEY'); - return; + console.warn('[dubAnalytics] Missing PUBLISHABLE_KEY'); } + if (!API_HOST || !PUBLISHABLE_KEY) { + const api = (window.dubAnalytics = window.dubAnalytics || {}); + const err = (msg) => Promise.reject(new Error(`[dubAnalytics] ${msg}`)); + api.trackLead = () => err('trackLead unavailable: missing API config'); + api.trackSale = () => err('trackSale unavailable: missing API config'); + return; + }packages/web/src/types.ts (1)
8-13: Tighten publishableKey type and clarify intended usageLooks good. To prevent accidental use of secret keys and add editor-time safety, consider narrowing the type with a template literal and strengthen the JSDoc to emphasize “publishable” only.
/** * The publishable key for client conversion tracking * @example 'dub_pk_BgyBCEJCPCGN3RN7oieLVHRs' */ - publishableKey?: string; + /** Publishable (client-side) key. Must start with "dub_pk_". Never pass secret keys here. */ + publishableKey?: `dub_pk_${string}`;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
apps/html/conversion-tracking.html(1 hunks)apps/nextjs/app/conversion-tracking/page-client.tsx(1 hunks)apps/nextjs/app/conversion-tracking/page.tsx(1 hunks)packages/script/build.js(5 hunks)packages/script/package.json(1 hunks)packages/script/src/base.js(2 hunks)packages/script/src/extensions/conversion-tracking.js(1 hunks)packages/web/src/generic.ts(3 hunks)packages/web/src/react.tsx(2 hunks)packages/web/src/types.ts(2 hunks)packages/web/src/use-analytics.ts(3 hunks)
🔇 Additional comments (12)
packages/script/src/base.js (1)
275-277: Approve conditional exposure ofk
- Confirmed that the only consumer of
window._dubAnalytics.kis inpackages/script/src/extensions/conversion-tracking.js, where it’s destructured intoPUBLISHABLE_KEYand immediately checked for truthiness before use. Omittingkwhen falsy yieldsundefinedon destructuring—identical to the current behavior wherek: undefined—so this change is safe.- Confirmed that
qmis read in the same extension (if (window._dubAnalytics && window._dubAnalytics.qm) { … }) to process any queued events, so its public exposure remains intentional.Your proposed diff is good to merge:
- k: PUBLISHABLE_KEY, - qm: queueManager, + ...(PUBLISHABLE_KEY ? { k: PUBLISHABLE_KEY } : {}), + qm: queueManager,apps/nextjs/app/conversion-tracking/page.tsx (1)
1-5: LGTM — thin server wrapper keeps concerns cleanThe page simply delegates to the client component, which is appropriate for this demo flow.
packages/web/src/react.tsx (2)
37-43: Re-exporting the new types improves DXConsumers won’t need to deep-import types—nice touch.
3-9: Verified type-only imports for new surfaces
Confirmed that bothTrackLeadInputandTrackSaleInputare exported as interfaces inpackages/web/src/types.ts(lines 168–170 and 179–181), so importing them withimport typeis correct and incurs no runtime cost.packages/script/package.json (1)
9-13: Verify build dependencies and dist artifactsBefore verifying the new analytics bundles, ensure you’ve installed all build dependencies; otherwise the
build.jsstep will fail (as seen with the missingesbuildmodule).Suggested local check:
#!/bin/bash set -euo pipefail cd packages/script # Install dependencies so esbuild is available npm ci # Build artifacts npm run prebuild npm run build # Verify files exist for f in \ dist/analytics/script.conversion-tracking.js \ dist/analytics/script.site-visit.outbound-domains.js \ dist/analytics/script.site-visit.conversion-tracking.js \ dist/analytics/script.outbound-domains.conversion-tracking.js \ dist/analytics/script.site-visit.outbound-domains.conversion-tracking.js do if [[ -f "$f" ]]; then echo "OK: $f" else echo "MISSING: $f" >&2 exit 1 fi done # Preview published contents npm pack --dry-run 2>/dev/null | sed -n '1,200p'• Confirm that
esbuild(and any other build tool) is declared inpackage.jsonand installed
• Cross-check the output ofnpm pack --dry-runto ensure all fivedist/analytics/*.jsfiles are includedpackages/script/build.js (1)
65-77: Conversion-tracking only target naming aligns with web feature gating.script.conversion-tracking.js matches the features order used in packages/web/src/generic.ts.
apps/html/conversion-tracking.html (1)
18-23: Stub shape LGTM; methods are queued with method name first.This matches the extension’s queue-drain contract of [method, ...args].
packages/web/src/generic.ts (2)
22-27: Public method wiring LGTM.trackLead and trackSale are exposed consistently with trackClick.
33-41: Feature ordering matches build outputs.['site-visit','outbound-domains','conversion-tracking'] aligns with the emitted filenames.
packages/script/src/extensions/conversion-tracking.js (1)
77-96: Queue draining approach LGTM.Filters out handled conversion events and leaves other queued calls intact.
packages/web/src/types.ts (2)
168-177: TrackLeadInput: enhance type safety, restrictmode, and document PII/Small refinements improve safety and developer experience:
- Use explicit JSDoc comments to clarify intent and mark PII-bearing fields.
- Constrain
modeto the known values ('test' | 'live') rather than a free-form string.- Switch
metadatato a JSON-serializable type to prevent functions/symbols from slipping through.export interface TrackLeadInput { - clickId?: string; // falls back to dub_id cookie - eventName: string; - customerExternalId: string; - customerName?: string; - customerEmail?: string; - customerAvatar?: string; - mode?: string; - metadata?: Record<string, unknown>; + /** Explicit click ID; if omitted, SDK will fall back to the `dub_id` cookie. */ + clickId?: string; + /** Event name to display in analytics (default suggestion: `"lead"`). */ + eventName: string; + /** Stable unique identifier from your system (e.g., userId, contactId). */ + customerExternalId: string; + /** End-customer’s display name (PII). */ + customerName?: string; + /** End-customer’s email address (PII). */ + customerEmail?: string; + /** HTTPS URL pointing to the customer’s avatar image. */ + customerAvatar?: string; + /** Environment mode. */ + mode?: 'test' | 'live'; + /** Free-form, JSON-serializable metadata (kept under ~2 KB). */ + metadata?: JsonObject; }Add these JSON-safe utility types (near the top of
packages/web/src/types.ts):export type JsonPrimitive = string | number | boolean | null; export type JsonValue = JsonPrimitive | JsonObject | JsonValue[]; export interface JsonObject { [key: string]: JsonValue }
⚠️ We weren’t able to locate server-side validation for whichmodevalues are allowed or any payload-size limits onmetadata. Please confirm on the backend:
- That
modetruly accepts only'test' | 'live'(or widen the union if needed).- Any maximum size (e.g. ~2 KB) enforced for the
metadatafield.
179-187: Clarify TrackSaleInput semantics and reuse JSON metadataMinor typing/documentation tweaks to reduce footguns in
TrackSaleInput:
- Amount: specify “smallest currency unit” (e.g., cents) to avoid floating-point/rounding issues.
- Currency: prefer ISO 4217 uppercase codes via a helper type.
- Metadata: reuse a
JsonObjecttype instead of a rawRecord<string, unknown>.- PaymentProcessor: if your backend enumerates allowed processors (e.g.
'stripe' | 'paddle'), narrow this type.export interface TrackSaleInput { - eventName: string; - customerExternalId: string; - paymentProcessor?: string; - amount: number; - invoiceId?: string; - currency?: string; - metadata?: Record<string, unknown>; + /** Event name to display in analytics (recommended default: "sale"). */ + eventName: string; + /** Stable unique identifier from your system (e.g., userId, accountId). */ + customerExternalId: string; + /** Payment processor used for the transaction (e.g., 'stripe'). */ + paymentProcessor?: string; + /** Amount in the smallest currency unit (e.g., cents). */ + amount: number; + /** Provider-side invoice/charge ID. */ + invoiceId?: string; + /** ISO 4217 currency code (uppercase). */ + currency?: CurrencyCode; + /** Free-form, JSON-serializable metadata (kept under 2 KB). */ + metadata?: JsonObject; }Add alongside your other JSON/utility types:
export type CurrencyCode = Uppercase<string>;Next steps ():
- Confirm with your backend or the external tracking service that:
amountis indeed interpreted in minor units.currencymust be uppercase ISO 4217 codes.- If you already know the exact set of payment processors, define a
PaymentProcessorunion and swappaymentProcessor?: stringforpaymentProcessor?: PaymentProcessor.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
packages/web/src/types.ts (3)
8-12: Clarify publishableKey usage and prevent secret-key misuseNice addition. Let’s make it foolproof for integrators and slightly safer at the type level.
Apply this diff to strengthen the docs and (optionally) brand the type:
/** - * The publishable key for client conversion tracking - * @example 'dub_pk_BgyBCEJCPCGN3RN7oieLVHRs' + * The publishable key for client conversion tracking. + * Public, client-safe. Never embed secret keys (e.g. starting with 'dub_sk_') in client code. + * Required for `trackLead` / `trackSale` to function. + * @example 'dub_pk_BgyBCEJCPCGN3RN7oieLVHRs' */ - publishableKey?: string; + publishableKey?: PublishableKey;Add this helper type once in the file (top-level is fine) to brand the key without changing runtime behavior:
export type PublishableKey = string & { readonly __brand: 'DubPublishableKey' };
168-177: Tighten TrackLeadInput types and document PII + metadata constraintsCurrent shape is good; a couple of small type-level nudges will prevent common integration mistakes and clarify PII handling.
Apply this diff to (1) restrict mode to known values and (2) ensure metadata is JSON-serializable:
-export interface TrackLeadInput { - clickId?: string; // falls back to dub_id cookie - eventName: string; - customerExternalId: string; - customerName?: string; - customerEmail?: string; - customerAvatar?: string; - mode?: string; - metadata?: Record<string, unknown>; -} +export interface TrackLeadInput { + /** Optional; falls back to dub_id cookie if omitted. */ + clickId?: string; + /** Stable event identifier for your lead (e.g. 'lead' or 'newsletter_signup'). */ + eventName: string; + /** Your user/customer identifier in your system. */ + customerExternalId: string; + /** Optional PII. Ensure consent and avoid logging to console/third-party tools. */ + customerName?: string; + /** Optional PII. If used for matching, prefer hashing server-side before storage. */ + customerEmail?: string; + customerAvatar?: string; + /** Environment flag; helps separate test vs. live data. */ + mode?: 'test' | 'live'; + /** JSON-serializable metadata only (no functions/DOM nodes). */ + metadata?: Record<string, JsonValue>; +}Add this JSON helper once (top-level) if you adopt the change above:
export type JsonValue = | string | number | boolean | null | { [key: string]: JsonValue } | JsonValue[];I can push a follow-up commit that propagates the stricter types where these payloads are created.
179-187: Define money/currency semantics to avoid rounding/aggregation errors in TrackSaleInputI’ve verified across the repo to ensure this change won’t introduce breaking behavior:
- No stale references to the removed
AllowedPropertyValuesalias.- Exactly one declaration each for
TrackLeadInputandTrackSaleInputinpackages/web/src/types.ts.trackSaleis invoked in:
•packages/web/src/use-analytics.ts(passes a fullTrackSaleInputobject)
•packages/script/src/extensions/conversion-tracking.js(proxying calls)
•apps/html/conversion-tracking.html
•apps/nextjs/app/conversion-tracking/page-client.tsx
In all HTML/JSX callsites,amountis already provided in minor units (e.g. 5000 for $50.00) andcurrencyis omitted—matching the proposed “minor units” convention and optional currency field.- No
mode:properties that would conflict.- No direct uses of
metadatafound; please manually confirm that any existing metadata objects are JSON-serializable before narrowing its type toJsonValue.With that in mind, apply this diff to clarify units and tighten metadata:
export interface TrackSaleInput { - eventName: string; - customerExternalId: string; - paymentProcessor?: string; - amount: number; - invoiceId?: string; - currency?: string; - metadata?: Record<string, unknown>; + /** Stable event identifier for your sale (e.g. 'sale' or 'purchase'). */ + eventName: string; + /** Your user/customer identifier in your system. */ + customerExternalId: string; + /** Optional, e.g. 'stripe', 'paddle'. Document expected values in public docs. */ + paymentProcessor?: string; + /** Monetary amount in minor units (e.g. cents). Must be non-negative. */ + amount: number; + /** Your invoice or order identifier. */ + invoiceId?: string; + /** ISO 4217 currency code (e.g. 'USD', 'EUR'). Uppercase recommended. */ + currency?: string; + /** JSON-serializable metadata only (no functions/DOM nodes). */ + metadata?: Record<string, JsonValue>; }apps/nextjs/app/conversion-tracking/page-client.tsx (5)
8-14: Handle async and surface failures when tracking a leadIf
trackLeadis async, the click handler is currently fire‑and‑forget. Wrap intry/catchandawaitso errors are visible during manual testing and in CI E2E runs.Apply this diff:
- const handleTrackLead = () => { - trackLead({ + const handleTrackLead = async () => { + try { + await trackLead({ eventName: 'Account created', customerExternalId: '1234567890', - }); - }; + }); + console.info('[analytics] lead tracked'); + } catch (err) { + console.error('[analytics] failed to track lead', err); + } + };
15-21: Do the same for sales; also make currency explicit to avoid unit ambiguityClarify units and add basic error handling. Making
currencyexplicit helps avoid confusion about minor units across locales.Apply this diff:
- const handleTrackSale = () => { - trackSale({ + const handleTrackSale = async () => { + try { + await trackSale({ eventName: 'Purchase completed', customerExternalId: 'CXvG5QOLi8QKBA2jYmDh', - amount: 5000, // defaults to usd cents, use `currency` prop to specify a different currency - }); - }; + amount: 5000, // amount in minor units (cents) + currency: 'USD', + }); + console.info('[analytics] sale tracked'); + } catch (err) { + console.error('[analytics] failed to track sale', err); + } + };
25-36: Add button type and keyboard focus styles; include stable test idsThis prevents accidental form submission when nested in a form, improves a11y, and helps E2E tests.
Apply this diff:
- <button - onClick={handleTrackLead} - className="px-4 py-2 bg-blue-500 text-white rounded hover:bg-blue-600 transition-colors" - > + <button + type="button" + onClick={handleTrackLead} + data-testid="track-lead" + className="px-4 py-2 bg-blue-500 text-white rounded hover:bg-blue-600 transition-colors focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-offset-2 focus-visible:ring-blue-600" + > Track Lead </button> - <button - onClick={handleTrackSale} - className="px-4 py-2 bg-green-500 text-white rounded hover:bg-green-600 transition-colors" - > + <button + type="button" + onClick={handleTrackSale} + data-testid="track-sale" + className="px-4 py-2 bg-green-500 text-white rounded hover:bg-green-600 transition-colors focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-offset-2 focus-visible:ring-green-600" + > Track Sale </button>
8-21: If you prefer to keep handlers non-async, at least make the intent explicitPrefixing with
voidcommunicates that you are intentionally discarding the promise and satisfies “no-floating-promises” linters.Apply this minimal diff instead:
- const handleTrackLead = () => { - trackLead({ + const handleTrackLead = () => { + void trackLead({ eventName: 'Account created', customerExternalId: '1234567890', }); }; - const handleTrackSale = () => { - trackSale({ + const handleTrackSale = () => { + void trackSale({ eventName: 'Purchase completed', customerExternalId: 'CXvG5QOLi8QKBA2jYmDh', amount: 5000, // defaults to usd cents, use `currency` prop to specify a different currency }); };
19-19: Tiny wording nit: “prop” → “field” (this isn’t a React prop)Also consider capitalizing USD and clarifying “minor units”.
Apply this diff:
- amount: 5000, // defaults to usd cents, use `currency` prop to specify a different currency + amount: 5000, // defaults to USD minor units (cents); set the `currency` field to use a different currency
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
apps/html/conversion-tracking.html(1 hunks)apps/nextjs/app/conversion-tracking/page-client.tsx(1 hunks)packages/script/src/extensions/conversion-tracking.js(1 hunks)packages/web/src/types.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/html/conversion-tracking.html
- packages/script/src/extensions/conversion-tracking.js
🔇 Additional comments (2)
apps/nextjs/app/conversion-tracking/page-client.tsx (2)
5-7: Straightforward client demo wiring looks goodUsing a client component and pulling
trackLead/trackSalefromuseAnalytics()is the right shape for a minimal demo. No correctness concerns here.
1-7: Analytics initialization confirmed in RootLayoutThe
Analyticscomponent (aliased asDubAnalytics) from@dub/analytics/reactis imported and rendered inapps/nextjs/app/layout.tsx, ensuring the script bootstrap and global context are initialized for all pages, includingConversionTrackingPageClient. No further action is required.
2 participants
Summary by CodeRabbit