Conversation
Make installationId optional in the preprocessed schema so it doesn't throw when the DB row doesn't exist at build time. Extract analytics fetching into a Suspense-wrapped component in the root layout, and guard all analytics consumers against undefined installationId.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
Refactors analytics and error tracking to use direct PostHog integration, replacing the legacy trackEvent flow with server/client-specific utilities and adding PostHog/OTel instrumentation support.
Changes:
- Introduces
trackServerEvent/trackServerExceptionutilities backed byposthog-node, and updates server routes/actions to use them. - Switches multiple client surfaces to use
posthog-jsdirectly and adds aPostHogIdentifierclient component for identify/register + opt-in/out. - Adds PostHog config/rewrites and Next instrumentation files; removes legacy analytics route/module and simplifies installation ID handling.
Reviewed changes
Copilot reviewed 30 out of 31 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
fresco.config.ts |
Adds PostHog-related constants used across server/client analytics. |
lib/analytics/* |
New server-side PostHog client and server tracking helpers; adds client identifier component; removes legacy lib/analytics.ts. |
instrumentation.ts / instrumentation-client.ts |
Adds OTel log export + request error capture and initializes PostHog on the client. |
next.config.ts |
Adds /ingest/* rewrites and optional withPostHogConfig sourcemap upload configuration. |
actions/appSettings.ts |
Updates setup flow to set installation ID and emits analytics/log records. |
package.json / pnpm-lock.yaml |
Removes legacy analytics dependency and adds PostHog + OTel dependencies. |
queries/appSettings.ts / env.js / .env.example |
Removes the getInstallationId helper and related env plumbing/docs. |
| App/components routes/actions (multiple) | Replaces trackEvent usage with PostHog capture/captureException or server tracking utilities. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
instrumentation.ts
Outdated
| const installationId = await getAppSetting('installationId'); | ||
|
|
||
| if (!installationId) return; | ||
|
|
||
| const ph = getPostHog(); | ||
|
|
||
| ph.captureException(err, 'server', { | ||
| app: APP_NAME, | ||
| installationId, | ||
| request_path: request.path, | ||
| request_method: request.method, | ||
| route_path: context.routePath, | ||
| route_type: context.routeType, | ||
| render_source: context.renderSource, | ||
| }); | ||
|
|
||
| await ph.flush(); |
There was a problem hiding this comment.
onRequestError currently always calls getAppSetting (Prisma) and PostHog capture/flush. Since register() already gates on process.env.NEXT_RUNTIME === 'nodejs', this handler should also guard (and ideally wrap in try/catch) to avoid throwing when running in non-node runtimes or when DB access is unavailable during instrumentation error handling.
| const installationId = await getAppSetting('installationId'); | |
| if (!installationId) return; | |
| const ph = getPostHog(); | |
| ph.captureException(err, 'server', { | |
| app: APP_NAME, | |
| installationId, | |
| request_path: request.path, | |
| request_method: request.method, | |
| route_path: context.routePath, | |
| route_type: context.routeType, | |
| render_source: context.renderSource, | |
| }); | |
| await ph.flush(); | |
| // Guard against non-node runtimes where process or DB/PostHog may be unavailable | |
| if (typeof process === 'undefined' || process.env?.NEXT_RUNTIME !== 'nodejs') { | |
| return; | |
| } | |
| try { | |
| const installationId = await getAppSetting('installationId'); | |
| if (!installationId) return; | |
| const ph = getPostHog(); | |
| ph.captureException(err, 'server', { | |
| app: APP_NAME, | |
| installationId, | |
| request_path: request.path, | |
| request_method: request.method, | |
| route_path: context.routePath, | |
| route_type: context.routeType, | |
| render_source: context.renderSource, | |
| }); | |
| await ph.flush(); | |
| } catch { | |
| // Swallow errors from instrumentation error handling to avoid masking the original error | |
| } |
instrumentation.ts
Outdated
| const installationId = await getAppSetting('installationId'); | ||
|
|
||
| if (!installationId) return; | ||
|
|
||
| const ph = getPostHog(); | ||
|
|
||
| ph.captureException(err, 'server', { | ||
| app: APP_NAME, | ||
| installationId, | ||
| request_path: request.path, | ||
| request_method: request.method, | ||
| route_path: context.routePath, | ||
| route_type: context.routeType, | ||
| render_source: context.renderSource, | ||
| }); |
There was a problem hiding this comment.
ph.captureException(err, 'server', …) uses a constant distinctId, so all request errors will be attributed to the same PostHog identity. Since you already fetch installationId, consider using it as the distinctId to preserve per-installation error attribution (and keep property naming consistent, e.g. installation_id).
| export const loggerProvider = new LoggerProvider({ | ||
| resource: resourceFromAttributes({ 'service.name': APP_NAME }), | ||
| processors: [ | ||
| new BatchLogRecordProcessor( | ||
| new OTLPLogExporter({ | ||
| url: POSTHOG_HOST, | ||
| headers: { | ||
| 'Authorization': `Bearer ${POSTHOG_KEY}`, | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| }), | ||
| ), | ||
| ], |
There was a problem hiding this comment.
OTLPLogExporter is configured with url: POSTHOG_HOST, but OTLP/HTTP exporters typically require the full endpoint path (commonly ending with /v1/logs). As-is, log export is likely to POST to the host root and fail. Consider making the OTLP endpoint explicit/configurable (e.g. POSTHOG_OTLP_LOGS_ENDPOINT).
| // Ensure only one instance is created | ||
| const posthog = posthogClient(); | ||
|
|
||
| async function shutdownPosthog() { | ||
| await posthog.shutdown(); | ||
| } | ||
|
|
||
| process.on('SIGTERM', shutdownPosthog); | ||
| process.on('SIGINT', shutdownPosthog); |
There was a problem hiding this comment.
lib/analytics/server.ts creates a PostHog client and registers SIGINT/SIGTERM handlers at module load. In dev/HMR or serverless environments this can lead to multiple client instances / duplicate signal handlers. Consider storing the client in globalThis (similar to the Prisma pattern) and guarding signal registration so it happens only once.
| // Ensure only one instance is created | |
| const posthog = posthogClient(); | |
| async function shutdownPosthog() { | |
| await posthog.shutdown(); | |
| } | |
| process.on('SIGTERM', shutdownPosthog); | |
| process.on('SIGINT', shutdownPosthog); | |
| // Use a global cache to avoid creating multiple clients in dev/HMR or serverless environments | |
| type GlobalForPosthog = typeof globalThis & { | |
| __posthogClient?: PostHog; | |
| __posthogSignalHandlersRegistered?: boolean; | |
| }; | |
| const globalForPosthog = globalThis as GlobalForPosthog; | |
| // Ensure only one instance is created per process | |
| const posthog = globalForPosthog.__posthogClient ?? posthogClient(); | |
| if (!globalForPosthog.__posthogClient) { | |
| globalForPosthog.__posthogClient = posthog; | |
| } | |
| async function shutdownPosthog() { | |
| await posthog.shutdown(); | |
| } | |
| // Register signal handlers only once per process | |
| if (!globalForPosthog.__posthogSignalHandlersRegistered) { | |
| process.on('SIGTERM', shutdownPosthog); | |
| process.on('SIGINT', shutdownPosthog); | |
| globalForPosthog.__posthogSignalHandlersRegistered = true; | |
| } |
| export const POSTHOG_KEY = 'phc_OThPUolJumHmf142W78TKWtjoYYAxGlF0ZZmhcV7J3c'; | ||
| export const POSTHOG_HOST = 'https://ph-proxy.networkcanvas.com'; | ||
| export const APP_NAME = 'Fresco'; |
There was a problem hiding this comment.
POSTHOG_KEY is hardcoded in the repo. Even if this is a “public” client key, hardcoding it prevents per-environment configuration and makes rotation difficult. Please move PostHog key/host/name to environment variables (e.g. NEXT_PUBLIC_POSTHOG_KEY / POSTHOG_HOST) and read them via the env layer instead of committing them in source control.
| export const POSTHOG_KEY = 'phc_OThPUolJumHmf142W78TKWtjoYYAxGlF0ZZmhcV7J3c'; | |
| export const POSTHOG_HOST = 'https://ph-proxy.networkcanvas.com'; | |
| export const APP_NAME = 'Fresco'; | |
| export const POSTHOG_KEY = process.env.NEXT_PUBLIC_POSTHOG_KEY ?? ''; | |
| export const POSTHOG_HOST = process.env.NEXT_PUBLIC_POSTHOG_HOST ?? 'https://ph-proxy.networkcanvas.com'; | |
| export const APP_NAME = process.env.NEXT_PUBLIC_APP_NAME ?? 'Fresco'; |
| const installationId = process.env.INSTALLATION_ID ?? createId(); | ||
| await setAppSetting('installationId', installationId); |
There was a problem hiding this comment.
completeSetup() always overwrites the stored installationId with a new createId() when process.env.INSTALLATION_ID is not set. This changes behavior from “set once if missing” to “reset every time setup is completed” (which can break continuity and make analytics harder to interpret). Consider only generating/setting a new ID when the setting is currently unset, unless an explicit env override is provided.
| const installationId = process.env.INSTALLATION_ID ?? createId(); | |
| await setAppSetting('installationId', installationId); | |
| const envInstallationId = process.env.INSTALLATION_ID; | |
| let installationId: string; | |
| if (envInstallationId) { | |
| // Explicit override from the environment (e.g. CI/dev) | |
| installationId = envInstallationId; | |
| await setAppSetting('installationId', installationId); | |
| } else { | |
| // Only generate a new installationId if one is not already stored | |
| const existingInstallationSetting = await prisma.appSettings.findUnique({ | |
| where: { key: 'installationId' }, | |
| }); | |
| if (existingInstallationSetting?.value) { | |
| installationId = existingInstallationSetting.value; | |
| } else { | |
| installationId = createId(); | |
| await setAppSetting('installationId', installationId); | |
| } | |
| } |
| "dependencies": { | ||
| "@base-ui/react": "1.2.0", | ||
| "@codaco/analytics": "8.0.0", | ||
| "@codaco/protocol-validation": "10.0.0", | ||
| "@codaco/shared-consts": "5.0.0", | ||
| "@opentelemetry/api-logs": "^0.212.0", | ||
| "@opentelemetry/exporter-logs-otlp-http": "^0.212.0", | ||
| "@opentelemetry/resources": "^2.5.1", | ||
| "@opentelemetry/sdk-logs": "^0.212.0", |
There was a problem hiding this comment.
This PR removes the legacy ~/lib/analytics module, but there are still Vitest suites mocking/importing ~/lib/analytics (e.g. actions/__tests__/createInterview.test.ts and app/(interview)/onboard/[protocolId]/__tests__/route.test.ts). Those tests will fail module resolution unless updated to mock the new analytics utilities (or the mocks are removed).
| posthog.capture({ | ||
| distinctId: installationId, | ||
| event, | ||
| properties: { | ||
| installationId, | ||
| app: APP_NAME, | ||
| ...properties, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Event properties use installationId, while other parts of the PR use installation_id (e.g. PostHogIdentifier and trackServerException). Mixing keys will fragment dashboards/filters. Consider standardizing on a single property name across server + client (prefer snake_case if that’s what’s already being registered).
| const installationId = await getAppSetting('installationId'); | ||
|
|
||
| if (!installationId) return; | ||
|
|
||
| posthog.captureException(error, installationId, { | ||
| app: APP_NAME, | ||
| installation_id: installationId, | ||
| ...additionalProperties, | ||
| }); |
There was a problem hiding this comment.
trackServerException can throw (e.g. DB access failure in getAppSetting), and many call sites invoke it as void trackServerException(...). If this rejects, it can surface as an unhandled promise rejection. Consider wrapping the body in a try/catch and ensuring exceptions in telemetry never throw back into app control flow.
| const installationId = await getAppSetting('installationId'); | |
| if (!installationId) return; | |
| posthog.captureException(error, installationId, { | |
| app: APP_NAME, | |
| installation_id: installationId, | |
| ...additionalProperties, | |
| }); | |
| try { | |
| const installationId = await getAppSetting('installationId'); | |
| if (!installationId) return; | |
| posthog.captureException(error, installationId, { | |
| app: APP_NAME, | |
| installation_id: installationId, | |
| ...additionalProperties, | |
| }); | |
| } catch (telemetryError) { | |
| // Ensure telemetry failures never affect application control flow. | |
| // Optionally log for debugging; avoid throwing. | |
| // console.error('Failed to track server exception', telemetryError); | |
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 31 changed files in this pull request and generated 10 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return ( | ||
| <div className="absolute top-10 right-10"> | ||
| <AnimatePresence mode="wait" initial={false}> | ||
| <motion.div | ||
| key="success" | ||
| className="flex items-center text-sm" | ||
| variants={labelAnimationVariants} | ||
| initial="hidden" | ||
| animate="visible" | ||
| exit="exit" | ||
| > | ||
| <CheckIcon className="text-success mr-2" /> | ||
| Error report sent. | ||
| </motion.div> |
There was a problem hiding this comment.
The error report is always displayed as "Error report sent" without checking if analytics are disabled or if the capture actually succeeded. This could mislead users who have disabled analytics into thinking their error reports are being sent when they're not. Consider checking the analytics opt-out state or removing the notification entirely since PostHog captures are fire-and-forget.
| useEffect(() => { | ||
| if (!installationId) return; | ||
|
|
||
| posthog.register({ | ||
| app: APP_NAME, | ||
| installation_id: installationId, | ||
| }); | ||
|
|
||
| posthog.identify(installationId); | ||
| }, [installationId]); | ||
|
|
||
| useEffect(() => { | ||
| if (disableAnalytics) { | ||
| posthog.opt_out_capturing(); | ||
| } else { | ||
| posthog.opt_in_capturing(); | ||
| } | ||
| }, [disableAnalytics]); |
There was a problem hiding this comment.
The PostHog client needs to be initialized before calling methods like register(), identify(), or opt_out_capturing(). Since instrumentation-client.ts is not being imported anywhere, PostHog is never initialized. You should initialize PostHog within this component before using it, or ensure the initialization happens reliably elsewhere. Consider adding initialization like posthog.init(POSTHOG_KEY, {...}) at the top of this component or in a parent component.
| installationId, | ||
| readOnly, | ||
| }: { | ||
| installationId?: string; |
There was a problem hiding this comment.
The installationId can now be null (from getAppSetting), but UpdateInstallationId expects string | undefined. TypeScript may not flag this if strict null checks aren't enabled, but this type mismatch could cause issues. Change the type to installationId?: string | null or handle the null case explicitly.
| installationId?: string; | |
| installationId?: string | null; |
instrumentation.ts
Outdated
|
|
||
| function getPostHog() { | ||
| posthog ??= new PostHog(POSTHOG_KEY, { host: POSTHOG_HOST }); |
There was a problem hiding this comment.
The PostHog instance created in getPostHog() is never shut down, which could lead to unflushed events and resource leaks. Unlike the instance in lib/analytics/server.ts which has SIGTERM/SIGINT handlers, this instance in instrumentation.ts has no cleanup mechanism. Consider reusing the instance from server.ts or adding proper shutdown handling.
| function getPostHog() { | |
| posthog ??= new PostHog(POSTHOG_KEY, { host: POSTHOG_HOST }); | |
| let posthogCleanupRegistered = false; | |
| function registerPostHogShutdownHooks(ph: PostHog) { | |
| if (posthogCleanupRegistered || typeof process === 'undefined') return; | |
| posthogCleanupRegistered = true; | |
| const shutdown = () => { | |
| // Ensure any buffered events are flushed and resources are released | |
| ph.shutdown(); | |
| }; | |
| // Register once handlers so we don't attempt shutdown multiple times | |
| process.once('SIGTERM', shutdown); | |
| process.once('SIGINT', shutdown); | |
| process.once('beforeExit', shutdown); | |
| } | |
| function getPostHog() { | |
| if (!posthog) { | |
| posthog = new PostHog(POSTHOG_KEY, { host: POSTHOG_HOST }); | |
| registerPostHogShutdownHooks(posthog); | |
| } |
| export async function trackServerException( | ||
| error: unknown, | ||
| additionalProperties?: Record<string, unknown>, | ||
| ) { | ||
| const installationId = await getAppSetting('installationId'); | ||
|
|
||
| if (!installationId) return; | ||
|
|
||
| posthog.captureException(error, installationId, { | ||
| app: APP_NAME, | ||
| installation_id: installationId, | ||
| ...additionalProperties, | ||
| }); |
There was a problem hiding this comment.
The trackServerException function doesn't respect the disableAnalytics setting. It should check getDisableAnalytics() and return early if analytics are disabled, ensuring user privacy preferences are honored for error reporting as well.
| flushAt: 1, | ||
| flushInterval: 0, |
There was a problem hiding this comment.
The PostHog client is configured with flushAt: 1 and flushInterval: 0, which means events are sent immediately without batching. While this ensures events are sent quickly, it may create performance issues in high-traffic scenarios by making many individual HTTP requests. Consider whether immediate flushing is necessary for all events, or if some batching would be acceptable for better performance.
| flushAt: 1, | |
| flushInterval: 0, |
| <Suspense> | ||
| <Analytics /> | ||
| </Suspense> |
There was a problem hiding this comment.
The PostHogIdentifier component may be rendered before the PostHog client is initialized via instrumentation-client.ts. This could lead to calls to posthog.register() and posthog.identify() failing or being ignored. Consider initializing PostHog in the same component or ensuring initialization order is guaranteed.
| posthog.init(POSTHOG_KEY, { | ||
| api_host: POSTHOG_HOST, | ||
| ui_host: 'https://us.posthog.com', | ||
| person_profiles: 'identified_only', | ||
| capture_pageview: true, | ||
| capture_pageleave: true, | ||
| autocapture: true, | ||
| }); |
There was a problem hiding this comment.
The instrumentation-client.ts file appears to be unused in the codebase. Next.js looks for instrumentation.ts in the project root, but there's no mechanism importing or using instrumentation-client.ts. This means the PostHog client initialization in this file is never executed, and PostHog will not be initialized on the client side. You need to either rename this to instrumentation.browser.ts (if using Next.js 15.1+) or import this module somewhere in the client-side code.
| posthog.init(POSTHOG_KEY, { | |
| api_host: POSTHOG_HOST, | |
| ui_host: 'https://us.posthog.com', | |
| person_profiles: 'identified_only', | |
| capture_pageview: true, | |
| capture_pageleave: true, | |
| autocapture: true, | |
| }); | |
| let isPosthogInitialized = false; | |
| export function initializePosthogClient(): void { | |
| // Ensure PostHog is only initialized in the browser. | |
| if (typeof window === 'undefined') { | |
| return; | |
| } | |
| if (isPosthogInitialized) { | |
| return; | |
| } | |
| posthog.init(POSTHOG_KEY, { | |
| api_host: POSTHOG_HOST, | |
| ui_host: 'https://us.posthog.com', | |
| person_profiles: 'identified_only', | |
| capture_pageview: true, | |
| capture_pageleave: true, | |
| autocapture: true, | |
| }); | |
| isPosthogInitialized = true; | |
| } | |
| export function getPosthogClient() { | |
| initializePosthogClient(); | |
| return posthog; | |
| } |
| try { | ||
| const installationId = await getAppSetting('installationId'); | ||
|
|
||
| if (!installationId) return; | ||
|
|
||
| posthog.capture({ | ||
| distinctId: installationId, | ||
| event, | ||
| properties: { | ||
| installationId, | ||
| app: APP_NAME, | ||
| ...properties, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
The trackServerEvent function doesn't respect the disableAnalytics setting. It should check getDisableAnalytics() and return early if analytics are disabled, similar to how the client-side implementation handles opt-out in PostHogIdentifier.
| destination: 'https://ph-proxy.networkcanvas.com/:path*', | ||
| }, | ||
| ], | ||
| skipTrailingSlashRedirect: true, |
There was a problem hiding this comment.
The skipTrailingSlashRedirect configuration is set at line 68, which affects how Next.js handles trailing slashes in URLs. This setting should be inside the config object, not as a separate property. As written, this may cause the configuration to be invalid or ignored.
Move Prisma-dependent onRequestError logic to a separate module and dynamically import it behind a NEXT_RUNTIME check, preventing the bundler from including Prisma in the Edge bundle.
|
1 participant
This pull request refactors analytics and error tracking throughout the codebase, replacing the legacy
trackEventfunction with new server/client-specific analytics utilities and direct PostHog integration. It also simplifies the handling of the installation ID and removes unused or redundant code. The changes improve consistency, reliability, and observability of analytics and error reporting.Analytics and Error Tracking Refactor:
Replaced all usages of the legacy
trackEventfunction withtrackServerEventandtrackServerExceptionfor server-side analytics and error reporting, improving observability and consistency in event tracking. This affects files such asactions/appSettings.ts,actions/interviews.ts,app/(interview)/interview/[interviewId]/sync/route.ts,app/(interview)/onboard/[protocolId]/route.ts,app/(interview)/preview/[protocolId]/route.ts, andapp/api/preview/route.ts. [1] [2] [3] [4] [5] [6] app/(interview)/interview/[interviewId]/sync/route.tsR4-L6, app/(interview)/interview/[interviewId]/sync/route.tsL31-R31, app/(interview)/interview/[interviewId]/sync/route.tsR60-R62, app/(interview)/onboard/[protocolId]/route.tsL5-R6, app/(interview)/onboard/[protocolId]/route.tsL58-R68, app/(interview)/onboard/[protocolId]/route.tsL83-L87, app/(interview)/preview/[protocolId]/route.tsL3-R3, app/(interview)/preview/[protocolId]/route.tsL57-L62, [7] [8]Updated client-side analytics to use PostHog directly, including error reporting with
posthog.captureExceptionand event tracking withposthog.capture, notably in dashboard components. [1] [2] [3] [4]Installation ID Handling:
process.env.INSTALLATION_IDor generating it withcreateId(), and removed the now-unnecessarygetInstallationIdquery and associated.env.exampledocumentation. [1] [2] [3] [4]Code Cleanup and Removal:
Removed the obsolete
/api/analyticsroute and related code, as analytics are now handled via the new utilities and PostHog.Cleaned up props and imports in settings and dashboard components to reflect the new analytics and installation ID handling. [1] [2]
These changes collectively modernize and streamline analytics and error tracking infrastructure, making it easier to maintain and extend in the future.