Improve track click #2562
Improve track click #2562
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
""" WalkthroughThis update introduces a Redis-based cache for allowed hostnames, refactors workspace data retrieval to support domain inclusion, and synchronizes hostname cache updates with workspace changes. Several API routes are updated to use the new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API_Route
participant AllowedHostnamesCache
participant WorkspaceDB
Client->>API_Route: POST /api/track/click
API_Route->>AllowedHostnamesCache: Get allowed hostnames for domain
alt Cache miss
API_Route->>WorkspaceDB: Fetch workspace with domains & allowed hostnames
API_Route->>AllowedHostnamesCache: Cache allowed hostnames for domains
end
API_Route->>API_Route: Validate request hostname
alt Hostname not allowed
API_Route-->>Client: 403 Forbidden
else Hostname allowed
API_Route->>API_Route: Record click
API_Route-->>Client: Success
end
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@CodeRabbit final review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll conduct a comprehensive final review of this PR. Let me first gather the details of the changes made. 🏁 Script executed: Length of output: 11996 @steven-tey I've completed a thorough final review of this PR. Here's my comprehensive analysis of the "Improve track click" changes: ✅ Overall Assessment: Excellent Performance ImprovementThis PR successfully optimizes the track click functionality through intelligent caching and code refactoring. The changes are well-implemented and should provide significant performance benefits. 🔍 Key Improvements Identified1. Smart Caching Implementation
2. Enhanced Error Handling
3. Function Signature Modernization
📋 Detailed Code Review✅ Strengths
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/web/lib/analytics/allowed-hostnames-cache.ts (1)
29-41: Consider adding error handling for Redis operations.While the implementation is correct, consider adding explicit error handling for Redis pipeline operations to provide more meaningful error messages or fallback behavior.
- return await pipeline.exec(); + try { + return await pipeline.exec(); + } catch (error) { + console.error('Failed to delete allowed hostnames cache entries:', error); + throw error; + }apps/web/app/api/workspaces/[idOrSlug]/route.ts (1)
110-137: Well-structured async cache synchronization.The implementation correctly uses
waitUntilto handle both logo cleanup and cache synchronization asynchronously. The logic properly handles different scenarios: setting cache for new hostnames, deleting cache when hostnames are removed, and skipping operations when nothing changes.Consider improving the comparison method.
Using
JSON.stringifyfor comparison works but could be brittle if object key ordering changes. Consider using a more robust comparison method or a deep equality function.- const current = JSON.stringify(workspace.allowedHostnames); - const next = JSON.stringify(response.allowedHostnames); + const current = workspace.allowedHostnames; + const next = response.allowedHostnames; + const hasChanged = JSON.stringify(current?.sort()) !== JSON.stringify(next?.sort()); - if (current !== next) { + if (hasChanged) {Add error handling for cache operations.
Consider adding error handling for cache operations to prevent silent failures that could lead to inconsistent state.
+ try { if ( Array.isArray(response.allowedHostnames) && response.allowedHostnames.length > 0 ) { - allowedHostnamesCache.mset({ + await allowedHostnamesCache.mset({ allowedHostnames: next, domains, }); } else { - allowedHostnamesCache.deleteMany({ + await allowedHostnamesCache.deleteMany({ domains, }); } + } catch (error) { + console.error('Failed to sync allowed hostnames cache:', error); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/web/app/(ee)/api/track/click/route.ts(3 hunks)apps/web/app/(ee)/api/track/visit/route.ts(1 hunks)apps/web/app/api/links/exists/route.ts(1 hunks)apps/web/app/api/qr/route.tsx(1 hunks)apps/web/app/api/workspaces/[idOrSlug]/route.ts(2 hunks)apps/web/lib/analytics/allowed-hostnames-cache.ts(1 hunks)apps/web/lib/planetscale/get-link-with-allowed-hostnames.ts(0 hunks)apps/web/lib/planetscale/get-workspace-via-edge.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/web/lib/planetscale/get-link-with-allowed-hostnames.ts
🧰 Additional context used
🧬 Code Graph Analysis (7)
apps/web/app/api/links/exists/route.ts (1)
apps/web/lib/planetscale/get-workspace-via-edge.ts (1)
getWorkspaceViaEdge(5-62)
apps/web/app/api/qr/route.tsx (1)
apps/web/lib/planetscale/get-workspace-via-edge.ts (1)
getWorkspaceViaEdge(5-62)
apps/web/app/(ee)/api/track/visit/route.ts (1)
apps/web/lib/planetscale/get-workspace-via-edge.ts (1)
getWorkspaceViaEdge(5-62)
apps/web/lib/analytics/allowed-hostnames-cache.ts (1)
apps/web/lib/api/links/cache.ts (1)
CACHE_EXPIRATION(11-11)
apps/web/app/api/workspaces/[idOrSlug]/route.ts (3)
apps/web/lib/storage.ts (1)
storage(151-151)packages/utils/src/constants/main.ts (1)
R2_URL(77-77)apps/web/lib/analytics/allowed-hostnames-cache.ts (1)
allowedHostnamesCache(52-52)
apps/web/lib/planetscale/get-workspace-via-edge.ts (1)
apps/web/lib/types.ts (1)
WorkspaceProps(149-166)
apps/web/app/(ee)/api/track/click/route.ts (7)
apps/web/lib/types.ts (1)
RedisLinkProps(104-128)apps/web/lib/api/links/record-click-cache.ts (1)
recordClickCache(28-28)apps/web/lib/api/links/cache.ts (1)
linkCache(102-102)apps/web/lib/analytics/allowed-hostnames-cache.ts (1)
allowedHostnamesCache(52-52)apps/web/lib/planetscale/get-workspace-via-edge.ts (1)
getWorkspaceViaEdge(5-62)apps/web/lib/analytics/verify-analytics-allowed-hostnames.ts (1)
verifyAnalyticsAllowedHostnames(1-49)apps/web/lib/tinybird/record-click.ts (1)
recordClick(29-228)
🪛 Biome (1.9.4)
apps/web/lib/planetscale/get-workspace-via-edge.ts
[error] 55-55: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 56-56: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (10)
apps/web/lib/analytics/allowed-hostnames-cache.ts (4)
3-4: Well-designed cache configuration.The 7-day cache expiration and namespaced key prefix are appropriate for this use case. The longer expiration makes sense since allowed hostnames don't change frequently.
14-16: Good performance optimization with early return.Preventing unnecessary Redis pipeline operations when no domains are provided is a solid optimization.
18-26: Excellent use of Redis pipeline for batch operations.Using Redis pipeline for setting multiple keys atomically improves performance and reduces network round trips. The implementation correctly sets expiration for each key.
47-49: Clean key generation strategy.The private method for key generation follows good practices with consistent namespacing.
apps/web/app/api/links/exists/route.ts (1)
29-31: Correct API signature update.The change properly aligns with the updated
getWorkspaceViaEdgefunction signature that now accepts an object parameter instead of a direct string argument.apps/web/app/api/qr/route.tsx (1)
76-78: Consistent API signature update.The change correctly updates the function call to match the new
getWorkspaceViaEdgesignature, maintaining consistency across the codebase.apps/web/app/(ee)/api/track/visit/route.ts (1)
79-81: Proper API signature migration.The function call update correctly preserves the non-null assertion while adapting to the new object parameter format.
apps/web/app/api/workspaces/[idOrSlug]/route.ts (1)
1-1: Good integration of allowed hostnames cache.The import correctly adds the singleton cache instance for workspace-domain hostname management.
apps/web/app/(ee)/api/track/click/route.ts (1)
122-133: Good security enhancement for referrer validationThe implementation correctly validates referrers against allowed hostnames before recording clicks, preventing unauthorized click tracking from disallowed sources.
apps/web/lib/planetscale/get-workspace-via-edge.ts (1)
47-52: Review domain filtering logic for boolean valuesThe condition
row.verifiedwill exclude domains whereverifiedisfalse. If you intend to include all domains regardless of verification status, update the condition:- if (row.slug && row.verified) { + if (row.slug && row.verified !== null && row.verified !== undefined) {
…nto improve-track-click
Enhanced the error logging in the recordClick function to include operation names and detailed error information for each rejected promise. This provides better context and easier debugging when promise rejections occur.
Refactors all relevant files to rename the variable and parameter 'shouldPassClickId' to 'shouldCacheClickId' for improved clarity and consistency. No functional changes were made.
|
@CodeRabbit final review |
|
Tip For best results, initiate chat on the files or code changes. @steven-tey I provided a comprehensive final review in my previous comment above, where I analyzed all aspects of this PR including:
My assessment stands: This is an excellent performance improvement that's ready to merge. 🚀 Is there a specific aspect of the changes you'd like me to dive deeper into, or do you have any particular concerns about the implementation? |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/app/(ee)/api/track/click/route.ts(3 hunks)apps/web/lib/analytics/verify-analytics-allowed-hostnames.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/app/(ee)/api/track/click/route.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (1)
apps/web/lib/analytics/verify-analytics-allowed-hostnames.ts (1)
19-19: Excellent refactoring for improved modularity.The extraction of hostname parsing logic into a separate function improves code organization and reusability. The integration is seamless and maintains the existing behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/web/tests/tracks/track-click.test.ts (1)
209-220: POST CORS test could be more comprehensive.The test validates that POST requests succeed but doesn't verify CORS headers in the response. Consider adding assertions for CORS headers in POST responses as well.
test("POST request should return CORS headers", async () => { const response = await http.post<{ clickId: string }>({ path: "/track/click", headers: TRACK_CLICK_HEADERS, body: { domain: E2E_LINK.domain, key: E2E_LINK.key, }, }); expect(response.status).toEqual(200); + // Verify CORS headers are present in POST response + expect(response.headers?.["access-control-allow-origin"]).toBe("*"); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/web/tests/tracks/track-click.test.ts(1 hunks)apps/web/tests/tracks/track-lead.test.ts(8 hunks)apps/web/tests/utils/resource.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/web/tests/utils/resource.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
apps/web/tests/tracks/track-lead.test.ts (2)
apps/web/lib/types.ts (1)
TrackLeadResponse(365-365)apps/web/tests/utils/helpers.ts (1)
randomCustomer(7-17)
apps/web/tests/tracks/track-click.test.ts (2)
apps/web/tests/utils/resource.ts (1)
E2E_LINK(4-8)apps/web/lib/storage.ts (1)
fetch(59-61)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (16)
apps/web/tests/tracks/track-lead.test.ts (7)
33-49: LGTM! Dynamic clickId generation improves test robustness.The change from static to dynamic clickId generation is excellent. This approach:
- Tests the actual end-to-end flow from click tracking to lead tracking
- Eliminates dependency on hardcoded constants
- Ensures the clickId is valid and properly generated before testing lead functionality
The setup logic correctly validates the click response and extracts the clickId for subsequent tests.
7-27: Helper function parameter update is well-implemented.The modification to accept
clickIdas a parameter and use it in assertions (lines 18, 23) properly supports the dynamic approach. The function maintains its validation responsibilities while adapting to the new test structure.
53-71: Test case properly updated to use dynamic clickId.The test correctly passes the
trackedClickIdto both the API request and the helper function, maintaining consistency between the lead tracking request and response validation.
73-92: Idempotency test correctly validates same clickId usage.This test case appropriately verifies that duplicate requests with the same
externalIdreturn consistent results using the sametrackedClickId, which is important for the idempotent behavior.
94-114: Event quantity test properly implements dynamic clickId.The test case for
eventQuantitycorrectly follows the new pattern of using the dynamically generatedtrackedClickIdwhile testing the additional functionality.
116-134: Mode='wait' test correctly uses shared clickId.The test appropriately uses the same
trackedClickIdfor both the lead tracking and subsequent sale tracking, which is correct behavior for testing the wait mode functionality.
149-168: Backward compatibility test properly updated.The test for the deprecated
customerIdfield correctly uses the dynamictrackedClickId, ensuring backward compatibility is tested with realistic data flow.apps/web/tests/tracks/track-click.test.ts (9)
6-38: Well-designed helper function with flexible validation.The
expectValidClickResponsehelper function is well-structured with:
- Clear parameter interface with optional partner/discount flags
- Proper response structure validation
- Conditional assertions for optional data (partner, discount)
- Good use of
expect.objectContainingfor flexible object matchingThis design supports testing various response scenarios efficiently.
40-44: Good practice using constants for common headers.Extracting common headers into
TRACK_CLICK_HEADERSconstant improves maintainability and reduces duplication across test cases.
50-64: Basic click tracking test is well-implemented.The test correctly validates the fundamental click tracking functionality with proper response structure verification using the helper function.
66-86: Excellent test for clickId consistency.This test validates an important behavior - that subsequent requests for the same link return the same clickId, which is likely related to the caching improvements mentioned in the PR objectives. This ensures idempotent behavior.
88-105: Partner link test validates complex response structure.The test for partner links correctly validates both partner and discount data are returned, using the helper function's optional flags effectively. The test data (
getacme.link/derek) appears to be specifically configured for partner testing.
107-145: Comprehensive validation error testing.Both validation tests (missing domain and missing key) properly verify:
- Correct 422 status codes
- Detailed error messages with field-specific information
- Proper error structure with code, message, and documentation URL
This ensures good API usability with clear error feedback.
147-165: Not found error test is properly implemented.The test correctly validates 404 behavior for non-existent links with appropriate error structure and detailed error message including the specific domain and key.
191-207: CORS OPTIONS test properly validates preflight behavior.The test correctly validates CORS preflight behavior with:
- Proper 204 status for OPTIONS requests
- Required CORS headers for cross-origin requests
- Direct fetch usage (bypassing the test harness) which is appropriate for testing raw HTTP behavior
222-233: ClickId format validation is excellent.The test validates that clickId follows a specific format (16-character alphanumeric string) which ensures consistency and predictability. This is particularly important for caching and tracking functionality.
|
@CodeRabbit final review |
|
Tip For best results, initiate chat on the files or code changes. @steven-tey Here's my comprehensive final review of PR #2562 "Improve track click": 🎯 Final Assessment: Outstanding Performance EnhancementThis PR delivers a well-architected optimization of the track click functionality with significant performance and reliability improvements. 🔧 Core Improvements1. Intelligent Caching Strategy
2. Enhanced Error Handling
3. API Modernization
📊 Performance Impact
🏗️ Code Quality Excellence
🔍 Technical Highlights
✅ Final Verdict: APPROVEDThis PR represents exemplary engineering work that:
Recommendation: Ready to merge immediately. This is a textbook example of performance optimization done right. |
Removed the 'verified' field from domain processing and query results in getWorkspaceViaEdge. Updated types and logic to only include the 'slug' property for domains. Also added a LIMIT 100 clause to the SQL query.
|
@CodeRabbit final review |
|
Tip For best results, initiate chat on the files or code changes. @steven-tey I notice this is the fourth time you've requested a final review. I've provided comprehensive analyses in my previous three responses, consistently concluding that this is an excellent performance improvement ready to merge. 🔄 My Consistent Assessment Summary:
Status: APPROVED - Ready to merge 🚀 Is there a specific aspect of the implementation you'd like me to focus on, or do you have particular concerns about any part of the changes? I'm happy to dive deeper into:
Otherwise, my assessment remains: this is exemplary engineering work that delivers measurable improvements while maintaining code quality standards. |
Summary by CodeRabbit