refactor: update analytics integration to use unified tracking methods across client and server #181
Conversation
…s across client and server
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughReplaces legacy capture-style analytics with a unified Changes
Sequence Diagram(s)sequenceDiagram
participant Comp as Client Component / Hook
participant Hook as useAnalytics()
participant AC as analytics (client)
participant Page as Server Page (app/[domain]/page)
participant AS as analytics (server)
participant PH as PostHog SDK
rect rgba(135,206,235,0.12)
Note over Comp,Hook: Client-side event flow (hook)
Comp->>Hook: call useAnalytics()
Hook-->>Comp: { track, trackException }
Comp->>AC: track(event, props)
AC->>PH: posthog.capture(event, props)
end
rect rgba(255,228,196,0.12)
Note over Page,AS: Server-side event during SSR
Page->>AS: analytics.track("report_viewed", { domain })
AS->>PH: posthog.capture(event, props)
end
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code graph analysis (1)lib/analytics/server.ts (1)
🔇 Additional comments (3)
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #181 +/- ##
==========================================
- Coverage 69.39% 69.38% -0.01%
==========================================
Files 103 103
Lines 2908 2904 -4
Branches 891 889 -2
==========================================
- Hits 2018 2015 -3
Misses 551 551
+ Partials 339 338 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/analytics/server.ts (1)
11-25: RestoregetServerPosthogexport or refactorinstrumentation.tsto fix breaking change.The function
getServerPosthogis no longer exported fromlib/analytics/server.ts, butinstrumentation.ts(line 17-18) still imports and calls it. This will cause a runtime error:export 'getServerPosthog' was not found in '@/lib/analytics/server'. Either re-export the function or update the instrumentation file to use the exportedanalyticsobject instead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (11)
app/[domain]/page.tsx(2 hunks)components/copy-button.tsx(1 hunks)components/domain/domain-report-header.tsx(3 hunks)components/domain/domain-report-view.test.tsx(0 hunks)components/domain/domain-report-view.tsx(1 hunks)components/domain/domain-suggestions-client.tsx(4 hunks)hooks/use-domain-export.ts(3 hunks)hooks/use-domain-search.ts(3 hunks)lib/analytics/client.ts(1 hunks)lib/analytics/server.ts(3 hunks)vitest.setup.ts(1 hunks)
💤 Files with no reviewable changes (1)
- components/domain/domain-report-view.test.tsx
🧰 Additional context used
🧬 Code graph analysis (7)
components/domain/domain-suggestions-client.tsx (1)
lib/analytics/client.ts (2)
analytics(25-28)useAnalytics(41-46)
components/domain/domain-report-header.tsx (1)
lib/analytics/client.ts (2)
analytics(25-28)useAnalytics(41-46)
app/[domain]/page.tsx (1)
lib/analytics/server.ts (1)
analytics(55-103)
lib/analytics/client.ts (1)
lib/analytics/server.ts (1)
analytics(55-103)
hooks/use-domain-export.ts (2)
lib/analytics/client.ts (1)
analytics(25-28)lib/analytics/server.ts (1)
analytics(55-103)
lib/analytics/server.ts (1)
lib/analytics/client.ts (1)
analytics(25-28)
hooks/use-domain-search.ts (2)
lib/analytics/client.ts (1)
analytics(25-28)lib/analytics/server.ts (1)
analytics(55-103)
🔇 Additional comments (19)
components/copy-button.tsx (1)
17-34: Verify: Analytics tracking removed - is this intentional?The
copy_clickedevent tracking has been completely removed from this component. Given that the PR is about refactoring analytics (not removing it), this appears to be an unintentional loss of telemetry.If this removal was intentional, please confirm. Otherwise, consider restoring analytics tracking using the new API:
+"use client"; + import { Check, ClipboardCheck, Copy } from "lucide-react"; import { useRef, useState } from "react"; import { toast } from "sonner"; import { Button } from "@/components/ui/button"; +import { useAnalytics } from "@/lib/analytics/client"; interface CopyButtonProps { value: string; label?: string; } export function CopyButton({ value, label }: CopyButtonProps) { + const analytics = useAnalytics(); const [copied, setCopied] = useState(false); const resetTimerRef = useRef<number | null>(null); const handleCopy = () => { navigator.clipboard.writeText(value); + + analytics.track("copy_clicked", { + label, + value_length: value.length, + }); toast.success("Copied!", {components/domain/domain-report-view.tsx (1)
79-117: LGTM - Analytics moved to server-side.The removal of client-side analytics side effects from this component aligns with the PR's objective to consolidate tracking. Based on the AI summary, domain page view tracking has been moved to the server-side in
app/[domain]/page.tsx.Please confirm that the server-side tracking in
app/[domain]/page.tsxcaptures the equivalent analytics events that were previously tracked here (e.g., registration state resolution events).components/domain/domain-report-header.tsx (2)
8-8: LGTM - Clean migration to new analytics API.The import has been correctly updated to use the new
useAnalyticshook.
25-37: LGTM - Proper hook usage and event tracking.The component correctly initializes analytics via the
useAnalytics()hook and usesanalytics.track()for event tracking, following the recommended pattern for React components.vitest.setup.ts (2)
4-20: LGTM - Comprehensive mock updates for new analytics API.The test mocks have been properly updated to reflect the new analytics API surface:
- Server analytics mock provides async
trackandtrackExceptionmethods- Client analytics mock provides both the
analyticsobject anduseAnalyticshook- All mocks use
vi.fn()for proper test spy capabilities
25-30: LGTM - ResizeObserver mock added.The ResizeObserver mock is correctly implemented with spies for
observe,unobserve, anddisconnectmethods, which is necessary for jsdom-based tests.hooks/use-domain-export.ts (3)
4-4: LGTM - Correct import for hook context.Using the
analyticsobject directly (rather thanuseAnalyticshook) is the correct pattern for custom hooks, as documented inlib/analytics/client.ts.
58-58: LGTM - Clean event tracking.The export click event is properly tracked using the new API.
87-92: LGTM - Proper exception handling.The error tracking correctly wraps non-Error objects into Error instances before passing to
trackException, ensuring type safety.hooks/use-domain-search.ts (3)
9-9: LGTM - Correct import for hook context.Using the
analyticsobject directly is appropriate for custom hooks, as per the API documentation.
82-85: LGTM - Search submission tracking.The event tracking has been properly migrated to the new API while preserving the event name and payload structure.
107-110: LGTM - Invalid input tracking.The error tracking maintains the same event name and payload structure with the new API.
lib/analytics/client.ts (2)
5-19: LGTM - Robust error handling.Both
trackandtrackExceptionfunctions are wrapped in try-catch blocks to prevent analytics failures from breaking the application. This is a best practice for analytics implementations.
21-28: LGTM - Clear API surface for non-React contexts.The
analyticsobject provides a clean interface for use in hooks and other non-component code.components/domain/domain-suggestions-client.tsx (4)
15-15: LGTM - Correct import for component usage.Using the
useAnalyticshook is the recommended pattern for React components.
33-33: LGTM - Proper hook initialization.The analytics hook is correctly initialized at the component level.
107-110: LGTM - Suggestion click tracking.The event tracking has been properly migrated while preserving the event name and payload structure.
118-120: LGTM - History clear tracking.The history clear event is properly tracked using the new API.
lib/analytics/server.ts (1)
27-49: No external usage detected—change is safe.Verification confirms that
getDistinctIdis not imported or referenced outsidelib/analytics/server.ts. The visibility change from exported to internal poses no breaking changes.
…ing to analytics.trackException
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/analytics/server.ts (1)
11-49: Critical:shutdown()on a shared PostHog client can break analytics after the first eventYou’re initializing a module-scoped
sharedClientand then callingawait client.shutdown()on everytrack/trackExceptioncall. In the PostHog Node SDK,shutdown()is meant as a terminal operation (flush + close) for short-lived processes. On a reused client, this can make subsequent captures no-op or error, effectively disabling server-side analytics once the first event is processed. This is the same issue previously flagged on earlier revisions.To keep the fire-and-forget semantics while avoiding tearing down the shared client, you can switch to
flush()(or explicitly resetsharedClientafter shutdown). For example:@@ export const analytics = { track: ( event: string, properties: Record<string, unknown>, distinctId?: string, ) => { @@ - // flush events to posthog + // flush events to PostHog without shutting down the shared client try { - await client.shutdown(); + await client.flush(); } catch { // no-op } @@ trackException: ( error: Error, properties: Record<string, unknown>, distinctId?: string | null, ) => { @@ - // flush events to posthog + // flush events to PostHog without shutting down the shared client try { - await client.shutdown(); + await client.flush(); } catch { // no-op }If you prefer to keep
shutdown()for some reason, you should at least clear the module-scoped client so the next call reinitializes a fresh instance:- try { - await client.shutdown(); - } catch { - // no-op - } + try { + await client.shutdown(); + } catch { + // no-op + } finally { + sharedClient = null; + }Please double-check the exact behavior in your
posthog-nodeversion to confirm whethershutdown()allows safe reuse of the client; if not, one of the above patterns will be safer long-term.Confirm in the official `posthog-node` documentation for your exact version whether `shutdown()` is intended for per-process teardown (and not for per-event use) and whether a client can safely be reused after calling `shutdown()`. Also check the recommended pattern for long-lived servers vs. short-lived serverless functions (e.g., using `flush()` for the former).Also applies to: 51-129
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
app/[domain]/page.tsx(2 hunks)instrumentation.ts(2 hunks)lib/analytics/client.ts(1 hunks)lib/analytics/server.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
lib/analytics/server.ts (1)
lib/analytics/client.ts (1)
analytics(26-29)
lib/analytics/client.ts (1)
lib/analytics/server.ts (1)
analytics(58-130)
app/[domain]/page.tsx (1)
lib/analytics/server.ts (1)
analytics(58-130)
instrumentation.ts (1)
lib/analytics/server.ts (1)
analytics(58-130)
🔇 Additional comments (2)
app/[domain]/page.tsx (1)
5-5: Server-side pageview tracking looks correct and non-blockingImporting the server analytics helper and calling
analytics.track("report_viewed", { domain: normalized })post-canonicalization is consistent with the new fire-and-forget server API and avoids blocking render.Also applies to: 51-52
lib/analytics/client.ts (1)
4-50: Client analytics wrapper and hook are clean and aligned with server APIThe
track/trackExceptionwrappers with defensive try/catch, exportedanalyticsobject, anduseAnalyticsreturning a memoized{ track, trackException }give a consistent, low-friction client API that matches the server-side shape. Nice job addressing the stable-reference concern withuseMemo.
…ed analytics performance
…ate limiting and analytics
Summary by CodeRabbit
New Features
Refactor
Tests
Chores