Implement attribution tracking and initialization across the application - #1164
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Pull Request Review: Attribution Tracking ImplementationOverviewThis PR introduces comprehensive attribution tracking across the application, capturing marketing attribution data (UTM parameters, referral codes, ad click IDs) and integrating it into analytics events. The implementation is well-structured with proper separation of concerns. ✅ Strengths1. Excellent Architecture & Separation of Concerns
2. Comprehensive Attribution Coverage
3. Security & Privacy
4. Robust Error Handling
5. Good Testing Practices
🔴 Critical Issues1. Race Condition in Signup Flow
|
Pull Request Review: Attribution Tracking ImplementationOverviewThis PR introduces a comprehensive attribution tracking system to capture and persist marketing attribution data (UTM parameters, referral codes, advertising click IDs) across web and mobile platforms. The implementation is well-architected with proper separation of concerns. ✅ StrengthsArchitecture & Design
Code Quality
🔍 Issues & RecommendationsHIGH PRIORITY - Potential Bugs1. Race Condition in signup/creating.tsx (Lines 117-118, 148-149)// Three separate hydration checks can cause timing issues
if (!_hasHydrated || !userStoreHydrated || !_attributionHydrated) return;Issue: If attribution store hydrates last, there's a brief window where signup could proceed without attribution. Recommendation: Add a combined ready state or use Promise.all() to wait for all stores. 2. Missing Error Recovery in useAttributionInitialization.ts (Line 48)hasInitialized.current = true; // Set immediately before async workIssue: If initialization fails, the flag prevents retry attempts. Users who fail to capture initial attribution will never retry. Recommendation: try {
// initialization logic
hasInitialized.current = true; // Only set on success
} catch (error) {
hasInitialized.current = false; // Allow retry
Sentry.captureException(error);
}3. Duplicate Attribution Parsing LogicThe same URL parsing logic exists in both:
Issue: Two implementations can drift and cause inconsistencies. The lib version has Recommendation: Remove duplication - have the store import and use the lib function. MEDIUM PRIORITY - Code Quality4. Inconsistent Console LoggingThroughout the codebase, Recommendation: if (__DEV__) {
console.log('Attribution captured:', data);
}5. Attribution Channel Logic Could Be Simplified (lib/attribution.ts:291-359)The Recommendation: Use a clearer waterfall pattern: export const getAttributionChannel = (data: Partial<AttributionData>): AttributionChannel => {
if (data.referral_code) return 'referral';
if (data.gclid || data.msclkid) return 'paid_search';
if (data.fbclid || data.ttclid) return 'paid_social';
const medium = data.utm_medium?.toLowerCase();
const source = data.utm_source?.toLowerCase();
// ... continue with simplified logic
};6. Memory Leak Risk in useAttributionInitialization (Lines 101-111)The deep link listener is added but only cleaned up on unmount. If the effect runs multiple times (due to dependency changes), multiple listeners could accumulate. Current code is OK since there's a guard ( Recommendation: Store the subscription in a ref and remove any existing subscription before adding a new one. 7. Removed File Without Migration (app/register.tsx deleted)The PR deletes 249 lines from Action Required: Verify all routes/links referencing LOW PRIORITY - Performance & Best Practices8. Unused Return Value in useAttribution.ts (Line 183-200)The Recommendation: Either implement it properly or remove it and document that users should use 9. Validation Result Not Used (useAttribution.ts:95-100)Validation warnings are logged but not exposed to users or acted upon. Recommendation: Consider exposing 10. Magic Numbers (store/useAttributionStore.ts:431)isAttributionExpired: (windowDays: number = 30) => {The 30-day default is hardcoded in multiple places. Recommendation: Extract to a named constant: export const DEFAULT_ATTRIBUTION_WINDOW_DAYS = 30;SECURITY CONSIDERATIONS11. PII Sanitization Could Be More Robust (store/useAttributionStore.ts:166-168)The regex pattern may not catch all PII variations (international phone numbers, emails with non-ASCII characters). Recommendation: Consider using a dedicated PII detection library or more comprehensive patterns. 12. Attribution Data in Sentry (Multiple locations)Attribution data (including potentially sensitive referral codes) is sent to Sentry in error contexts. Recommendation: Review Sentry scrubbing rules to ensure no sensitive attribution data leaks. 📋 Testing RecommendationsThis PR lacks test coverage. Critical areas needing tests:
Suggested test file structure: 🎯 Action ItemsMust Fix Before Merge
Should Fix Before Merge
Nice to Have
📊 Performance ImpactMinimal - The attribution tracking is lightweight:
Recommendation: Monitor attribution store size over time. If users accumulate large attribution objects, consider pruning old data. 🔒 Security AssessmentOverall: Good with minor concerns:
🎓 Documentation QualityExcellent - The code is well-documented with:
Minor improvement: Add a README or docs/attribution.md explaining:
Final RecommendationApprove with Changes - This is a solid implementation of a complex feature. The architecture is sound and the code quality is high. However, the bugs identified in Issues #1, #2, #3, and #7 should be fixed before merging to production. The lack of tests is concerning for such a critical feature. I strongly recommend adding at least basic unit tests for the parsing and validation logic before merge. Code Quality Score: 8/10 Great work on the implementation! The attribution system is well-designed and will provide valuable marketing insights. Address the critical bugs and add tests, and this will be production-ready. |
PR Review: Attribution Tracking ImplementationOverviewThis PR introduces a comprehensive attribution tracking system to capture marketing attribution data (UTM parameters, referral codes, click IDs) across web and mobile platforms. The implementation is well-structured with proper separation of concerns using hooks, stores, and utility functions. ✅ StrengthsArchitecture & Design
Code Quality
Integration
🐛 Critical Issues1. Missing Import in ErrorBoundary.tsxLocation: const handleRetry = useCallback(() => {Issue: Fix: import { useCallback, useEffect, useRef } from 'react';Impact: This will cause a runtime error when the ErrorBoundary is rendered. 2. Excessive console.warn UsageLocations: Throughout Issue: Many Recommendation:
3. Potential Race Condition in signup/creating.tsxLocation: const _attributionHydrated = useAttributionStore(state => state._hasHydrated);
// ...
if (!_hasHydrated || !userStoreHydrated || !_attributionHydrated) return;Issue: While the PR correctly waits for attribution store hydration, there's a risk that if MMKV is slow, the user might experience signup delays. Recommendation: Add a timeout fallback (e.g., 5 seconds) to proceed with signup even if attribution isn't hydrated, logging a warning instead.
|
PR Review: Attribution Tracking ImplementationOverviewThis PR implements a comprehensive attribution tracking system for marketing analytics, capturing UTM parameters, advertising click IDs, and referral codes throughout the user journey. The implementation is well-architected with clear separation of concerns. ✅ StrengthsArchitecture & Design
Code Quality
Integration
🐛 Potential Issues1. Race Condition in Signup Flow (
|
1304916 to
0d455e7
Compare
Pull Request Review: Attribution Tracking ImplementationSummaryThis PR implements a comprehensive attribution tracking system for marketing analytics. The implementation adds UTM parameter tracking, referral code management, and advertising click ID capture across web and mobile platforms. Overall, this is a well-structured and thoughtfully designed implementation with good separation of concerns. ✅ Strengths1. Architecture & Code Quality
2. Robust Error Handling
3. Data Persistence
4. Analytics Integration
🔴 Critical Issues1. Race Condition in Signup Flow
|
- Introduced `useAttribution` and `useAttributionInitialization` hooks for managing attribution data. - Integrated attribution tracking in various components and hooks, including signup, deposit, and KYC processes. - Removed the `register.tsx` file as part of the refactor. - Enhanced analytics tracking by enriching events with attribution data, including UTM parameters and referral codes. - Updated the `track` and `trackIdentity` functions to automatically include attribution context. - Added utility functions for parsing and managing attribution data from URLs and deep links. - Established a persistent store for attribution data using Zustand and MMKV for efficient state management.
- Updated handleRetry to utilize useCallback for better performance and to prevent unnecessary re-renders. - Added dependencies to the useCallback hook to ensure it has access to the latest error and retry props.
- Replaced the import of `useEffect` with `useCallback` to optimize the component's performance. - Ensured the `path` import is correctly positioned for better code organization.
- Implemented comprehensive tracking for user interactions in the deposit flow, including viewing and selecting deposit methods, form submissions, and session management. - Added tracking for KYC steps and user actions in the KYC process to improve analytics insights. - Refactored components to utilize hooks for tracking events, ensuring consistent and efficient data collection. - Enhanced error handling and user feedback mechanisms in deposit-related components. - Updated tracking events in the constants file to reflect new analytics requirements.
- Remove /register route from app layout and path constants - Fix referral code preservation during signup flow reset - Add hydration tracking to referral store to prevent race conditions - Improve attribution initialization to wait for both stores - Add referral code sync verification with Sentry error tracking - Implement multi-source fallback for referral code (URL > referral > attribution)
bfa2b67 to
f487b6c
Compare
PR Review: Attribution Tracking Implementation🎯 OverviewThis PR implements a comprehensive attribution tracking system across the application. The implementation is well-structured with good separation of concerns. I've identified several areas for improvement around security, error handling, and best practices. ✅ StrengthsArchitecture & Design
Implementation Quality
🚨 Critical Issues1. Security: PII Detection Pattern IncompleteLocation: The PII detection regex only checks for emails, phones, and SSNs. Missing common PII patterns: // Current (incomplete)
const piiPattern = /[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}|(\+?\d{1,3}[-.\s]?)?\(?\d{3}\)?[-.\s]?\d{3}[-.\s]?\d{4}/;
// Recommended: Add credit cards, IP addresses, etc.
const creditCardPattern = /\b\d{4}[\s-]?\d{4}[\s-]?\d{4}[\s-]?\d{4}\b/;
const ipPattern = /\b(?:\d{1,3}\.){3}\d{1,3}\b/;Impact: High - Could inadvertently track/store PII in analytics 2. Race Condition in InitializationLocation: The hasInitialized.current = true; // Line 51 - set BEFORE initializeAttribution() completes
const initializeAttribution = async () => {
// async work here...
}Impact: Medium - Could allow multiple initialization attempts in fast navigation 3. Silent Error SwallowingLocation: Multiple files ( Errors are logged but don't prevent execution, potentially masking issues: Promise.allSettled([...]) // Continues even if all providers failImpact: Medium - Analytics failures invisible to monitoring
|
Implement attribution tracking and initialization across the application
…o-master Merge pull request #1164 from Solid-Money/fix/analytics-attribution
useAttributionanduseAttributionInitializationhooks for managing attribution data.register.tsxfile as part of the refactor.trackandtrackIdentityfunctions to automatically include attribution context.