Conversation
|
β Deploy Preview for learncarddocs canceled.
|
β Deploy Preview for staging-learncardapp ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
π Hey there! It looks like you modified code, but didn't update the documentation in If this PR introduces new features, changes APIs, or modifies behavior that users or developers need to know about, please consider updating the docs. π Windsurf TipYou can ask Windsurf to help:
Windsurf will review your changes and suggest appropriate documentation updates based on what was modified. π Documentation Guide
This is an automated reminder. If no docs are needed, feel free to ignore this message. |
There was a problem hiding this comment.
β¨ PR Review
The PR introduces a well-structured analytics abstraction layer with provider-agnostic tracking. However, there are critical issues with event loss during initialization, type safety violations in the Firebase provider, and performance problems from unstable dependencies causing infinite effect re-runs.
3 issues detected:
π Performance - Non-memoized function dependencies cause infinite effect re-execution π οΈ
Details: The useEffect has dependencies on
getDIDandidentifyfunctions. ThegetDIDfunction from useWallet() is not memoized and creates a new reference on every render, causing this effect to re-run constantly. This leads to redundant identify() calls and unnecessary performance overhead.
File:apps/learn-card-app/src/analytics/useSetAnalyticsUserId.ts (44-44)
π οΈ A suggested code correction is included in the review comments.π Bug - Unsafe type cast forces null through string-typed API that may not accept it π οΈ
Details: The reset() function uses a dangerous type cast
null as unknown as stringto bypass TypeScript's type checking when calling setUserId. This could cause runtime failures if Firebase Analytics doesn't properly handle null values, and indicates the API is being used incorrectly.
File:apps/learn-card-app/src/analytics/providers/firebase.ts (54-54)
π οΈ A suggested code correction is included in the review comments.π Bug - Analytics events fired before async provider initialization are lost in production
Details: The provider initializes as NoopProvider and asynchronously loads the real provider. Between mount and provider initialization completing, any analytics calls (track, identify, etc.) are sent to NoopProvider which only logs in dev mode, causing production events to be lost. Critical startup events like user identification or first page views may never be tracked.
File:apps/learn-card-app/src/analytics/context.tsx
Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using.
π‘ Tip: You can customize your AI Review using Guidelines Learn how
| if (currentUser) { | ||
| setUserId(); | ||
| } | ||
| }, [currentUser, currentLCNUser, getDID, identify, options.debug]); |
There was a problem hiding this comment.
π Performance - Infinite Effect Loop: Remove getDID and identify from the dependency array, or ensure useWallet() memoizes the getDID function with useCallback. The effect should primarily depend on currentUser changes, and the functions can be safely used without being dependencies since they're stable across the hook's lifecycle.
| }, [currentUser, currentLCNUser, getDID, identify, options.debug]); | |
| }, [currentUser, currentLCNUser, options.debug]); |
Is this review accurate? Use π or π to rate it
If you want to tell us more, use /gs feedback e.g. /gs feedback this review doesn't make sense, I disagree, and it keeps repeating over and over
|
|
||
| async reset(): Promise<void> { | ||
| try { | ||
| await FirebaseAnalytics.setUserId({ userId: null as unknown as string }); |
There was a problem hiding this comment.
π Bug - Type Safety Violation: Use an empty string '' instead of null, or check the Firebase Analytics documentation for the proper way to clear/reset the user ID. If null is genuinely required, add error handling and documentation explaining why this unsafe cast is necessary.
| await FirebaseAnalytics.setUserId({ userId: null as unknown as string }); | |
| await FirebaseAnalytics.setUserId({ userId: '' }); |
Is this review accurate? Use π or π to rate it
If you want to tell us more, use /gs feedback e.g. /gs feedback this review doesn't make sense, I disagree, and it keeps repeating over and over
Overview
π Relevant Jira Issues
Fixes: LC-1534
π What is the context and goal of this PR?
This PR introduces a provider-agnostic analytics abstraction layer for
learn-card-app. The goal is to decouple event tracking from specific analytics backends (Firebase), enabling us to:π₯΄ TL;DR:
New
@analyticsmodule with pluggable providers (Firebase, PostHog, Noop). All existinguseFirebaseAnalyticscalls migrated touseAnalyticswith typed events. SetVITE_ANALYTICS_PROVIDER=posthogto switch providers.π‘ Feature Breakdown
New Analytics Module (
src/analytics/)types.tsAnalyticsProviderinterface definitionproviders/noop.tsproviders/posthog.tscontext.tsxuseAnalytics()hookuseSetAnalyticsUserId.ts@analyticsaliasUsage:
Configuration:
π Important tradeoffs made:
useFirebaseAnalyticsrather than rewriting to maintain compatibilityπ Types of Changes
π³ Does This Create Any New Technical Debt?
Testing
π¬ How Can Someone QA This?
Test Firebase provider (default behavior):
VITE_ANALYTICS_PROVIDERsetTest PostHog provider:
VITE_ANALYTICS_PROVIDER=posthogand addVITE_POSTHOG_KEYclaim_boostevent in PostHog dashboardboostCMS_publisheventTest Noop provider:
VITE_ANALYTICS_PROVIDER=noopTest user identification:
π± π₯ Which devices would you like help testing on?
iOS Simulator / Android Simulator / Chromium
π§ͺ Code Coverage
No new tests added - this is a refactor of existing analytics calls with new provider support. Manual QA via analytics dashboards.
Documentation
π Documentation Checklist
Internal/AI Docs
π Documentation Notes
Internal refactor with new provider support. Configuration is via standard env vars documented in ticket.
β PR Checklist
@analyticsin learn-card-app)π Ready to squash-and-merge?: