-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Leverage clickIdCache to reduce load on Tinybird #2981
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
💡 Enable Vercel Agent with $100 free credit for automated AI reviews |
WalkthroughThe PR refactors click data retrieval infrastructure by converting Changes
Sequence DiagramsequenceDiagram
participant Caller as API Route/Function
participant GetClickEvent as getClickEvent()
participant Redis as Redis Cache
participant Tinybird as Tinybird Pipe
Caller->>GetClickEvent: getClickEvent({ clickId })
GetClickEvent->>Redis: redis.get(clickIdCache:clickId)
alt Cache Hit
Redis-->>GetClickEvent: ClickEventTB (cached)
GetClickEvent->>GetClickEvent: Transform fields<br/>(timestamp, qr, bot)
GetClickEvent-->>Caller: clickData (transformed)
else Cache Miss
GetClickEvent->>Tinybird: getClickEventTB({ clickId })
Tinybird-->>GetClickEvent: { data: [...] }
GetClickEvent->>GetClickEvent: Extract data[0]
GetClickEvent-->>Caller: clickData (first item)
else Error
Tinybird-->>GetClickEvent: Error
GetClickEvent->>GetClickEvent: Log error
GetClickEvent-->>Caller: null
end
Caller->>Caller: Check if clickData exists<br/>Throw not_found if falsy
Caller->>Caller: Use clickData properties<br/>(link_id, country, timestamp)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The refactor follows a consistent, homogeneous pattern across multiple files (click data access simplification), reducing cognitive overhead. However, the scope spans 11 files with varying context (API routes, conversion tracking, scripts, utilities), and the core function signature change requires verification that all call sites correctly consume the new return type. The addition of Redis caching logic adds a layer of complexity to verify. Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/web/lib/api/conversions/track-sale.ts (1)
62-62
: Remove unused outer variable declaration.The
clickData
variable declared at Line 62 is shadowed by the declaration at Line 154 and never used. Consider removing the outer declaration to eliminate confusion.Apply this diff:
let existingCustomer: Customer | null = null; let newCustomer: Customer | null = null; - let clickData: ClickEventTB | null = null; let leadEventData: LeadEventTB | null = null;
Also applies to: 154-156
apps/web/lib/tinybird/get-click-event.ts (1)
15-15
: Add explicit return type annotation.For better type safety and clarity, add an explicit return type to the function signature.
Apply this diff:
-export const getClickEvent = async ({ clickId }: { clickId: string }) => { +export const getClickEvent = async ({ clickId }: { clickId: string }): Promise<ClickEventTB | null> => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/web/app/(ee)/api/shopify/pixel/route.ts
(1 hunks)apps/web/app/(ee)/api/stripe/integration/webhook/checkout-session-completed.ts
(1 hunks)apps/web/app/(ee)/api/stripe/integration/webhook/utils/create-new-customer.ts
(1 hunks)apps/web/lib/api/conversions/track-lead.ts
(2 hunks)apps/web/lib/api/conversions/track-sale.ts
(1 hunks)apps/web/lib/integrations/shopify/create-lead.ts
(2 hunks)apps/web/lib/tinybird/get-click-event.ts
(1 hunks)apps/web/lib/tinybird/record-click.ts
(1 hunks)apps/web/lib/webhook/transform.ts
(1 hunks)apps/web/scripts/migrations/backfill-attribution.ts
(1 hunks)apps/web/scripts/tinybird/update-click-event.ts
(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-02T22:46:22.739Z
Learnt from: steven-tey
PR: dubinc/dub#2924
File: apps/web/lib/api/conversions/track-lead.ts:7-7
Timestamp: 2025-10-02T22:46:22.739Z
Learning: In apps/web/lib/api/conversions/track-lead.ts, lead events are cached in Redis for 5 minutes (keys: `leadCache:${customer.id}` and `leadCache:${customer.id}:${stringifiedEventName}`) to provide immediate data availability while Tinybird ingestion happens asynchronously. This caching pattern allows for async-only recording without breaking "wait" mode semantics.
Applied to files:
apps/web/lib/api/conversions/track-lead.ts
apps/web/lib/tinybird/record-click.ts
🧬 Code graph analysis (9)
apps/web/scripts/migrations/backfill-attribution.ts (1)
apps/web/lib/tinybird/get-click-event.ts (1)
getClickEvent
(15-41)
apps/web/lib/integrations/shopify/create-lead.ts (2)
apps/web/lib/tinybird/get-click-event.ts (1)
getClickEvent
(15-41)apps/web/lib/api/errors.ts (1)
DubApiError
(75-92)
apps/web/lib/api/conversions/track-lead.ts (1)
apps/web/lib/tinybird/get-click-event.ts (1)
getClickEvent
(15-41)
apps/web/scripts/tinybird/update-click-event.ts (1)
apps/web/lib/tinybird/get-click-event.ts (1)
getClickEvent
(15-41)
apps/web/lib/api/conversions/track-sale.ts (1)
apps/web/lib/tinybird/get-click-event.ts (1)
getClickEvent
(15-41)
apps/web/app/(ee)/api/stripe/integration/webhook/checkout-session-completed.ts (1)
apps/web/lib/tinybird/get-click-event.ts (1)
getClickEvent
(15-41)
apps/web/lib/tinybird/record-click.ts (1)
apps/web/lib/upstash/redis.ts (1)
redis
(4-7)
apps/web/lib/tinybird/get-click-event.ts (3)
apps/web/lib/tinybird/client.ts (1)
tb
(3-6)apps/web/lib/upstash/redis.ts (1)
redis
(4-7)apps/web/lib/types.ts (1)
ClickEventTB
(559-559)
apps/web/app/(ee)/api/stripe/integration/webhook/utils/create-new-customer.ts (1)
apps/web/lib/tinybird/get-click-event.ts (1)
getClickEvent
(15-41)
⏰ 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: build
🔇 Additional comments (13)
apps/web/lib/tinybird/record-click.ts (1)
170-176
: Cache expiry extension looks good.The 1-day cache duration aligns with the PR objective to reduce load on Tinybird. The comment and code are consistent.
apps/web/app/(ee)/api/shopify/pixel/route.ts (1)
36-42
: LGTM!The validation logic correctly handles the updated
getClickEvent
return type, which now returnsclickData
directly instead of a nested structure.apps/web/app/(ee)/api/stripe/integration/webhook/checkout-session-completed.ts (1)
72-76
: LGTM!The updated call to
getClickEvent
correctly uses the simplified return value, and the null check appropriately handles missing click data.apps/web/app/(ee)/api/stripe/integration/webhook/utils/create-new-customer.ts (1)
29-35
: LGTM!The variable rename from
clickEvent
toclickData
and the simplified null check correctly reflect the updatedgetClickEvent
return type.apps/web/lib/integrations/shopify/create-lead.ts (1)
38-47
: LGTM! Good addition of error handling.The updated code correctly uses the simplified
clickData
return value and adds proper error handling withDubApiError
when click data is not found, improving robustness over the previous implementation.apps/web/scripts/migrations/backfill-attribution.ts (2)
39-46
: LGTM!The variable rename and simplified null check correctly reflect the updated
getClickEvent
return type.
65-66
: LGTM!Field access correctly updated to use
clickData
properties directly.apps/web/scripts/tinybird/update-click-event.ts (1)
10-14
: LGTM!The updated call to
getClickEvent
correctly uses the simplified return value, and the null check appropriately handles missing data.apps/web/lib/api/conversions/track-lead.ts (2)
9-9
: LGTM: Import cleanup aligns with refactoring.Removing
ClickEventTB
from imports is correct since the file no longer needs to reference this type directly—getClickEvent
now returns the typed data.
107-115
: LGTM: Simplified click data retrieval.The refactoring successfully eliminates nested data extraction and cache fallback logic. The direct error handling is clearer and maintains the same behavior.
apps/web/lib/api/conversions/track-sale.ts (1)
154-163
: LGTM: Consistent refactoring pattern.The simplified click data retrieval aligns with the changes in track-lead.ts and correctly handles the null case.
apps/web/lib/tinybird/get-click-event.ts (2)
30-40
: LGTM: Robust error handling.The error handling properly logs failures with context and returns null, allowing consumers to handle missing data gracefully.
21-33
: Apply consistent data transformation to both cache and Tinybird paths.The cached data undergoes transformation (lines 22-27), converting
timestamp
format andqr
/bot
to explicit numbers, but the Tinybird data is returned unmodified (line 33). This creates inconsistent output formats depending on the data source.The schema at
apps/web/lib/zod/schemas/clicks.ts
definestimestamp
,qr
, andbot
, but the cache path explicitly transforms the timestamp (replacing "T" and "Z") while the Tinybird fallback bypasses this transformation. Ensure both paths return data in the same format by either:
- Applying the same transformation to the Tinybird response before returning it, or
- Documenting that Tinybird already returns data in the transformed format
Summary by CodeRabbit
Release Notes