Skip to content

fix(analytics): add posthog ingest proxy and document env vars#3915

Closed
waleedlatif1 wants to merge 1 commit intostagingfrom
fix/posthog-followup
Closed

fix(analytics): add posthog ingest proxy and document env vars#3915
waleedlatif1 wants to merge 1 commit intostagingfrom
fix/posthog-followup

Conversation

@waleedlatif1
Copy link
Copy Markdown
Collaborator

Summary

  • Add /ingest reverse proxy rewrites to next.config.ts — without these, all client-side PostHog events were 404ing since the client is configured to send to /ingest
  • Document NEXT_PUBLIC_POSTHOG_KEY and NEXT_PUBLIC_POSTHOG_ENABLED in .env.example

Type of Change

  • Bug fix

Testing

Tested manually

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@vercel
Copy link
Copy Markdown

vercel bot commented Apr 3, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Apr 3, 2026 8:21am

Request Review

@cursor
Copy link
Copy Markdown

cursor bot commented Apr 3, 2026

PR Summary

Low Risk
Low risk Next.js config/doc change that only affects requests under /ingest and optional analytics initialization; main app logic is unchanged.

Overview
Fixes client-side PostHog event delivery by adding Next.js rewrites to proxy /ingest/* (and /ingest/static/*) to PostHog’s US endpoints, avoiding the previous 404s when the client sent events to a local /ingest path.

Documents the optional analytics env vars in apps/sim/.env.example (NEXT_PUBLIC_POSTHOG_KEY and NEXT_PUBLIC_POSTHOG_ENABLED), including behavior when analytics is disabled.

Written by Cursor Bugbot for commit 14dd40c. Configure here.

@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

Closing — the /ingest proxy is already handled at the infrastructure level since session recordings are flowing correctly. The next.config.ts rewrite is only needed when Next.js itself needs to proxy PostHog traffic. Keeping the .env.example docs only.

@waleedlatif1 waleedlatif1 deleted the fix/posthog-followup branch April 3, 2026 08:22
Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

{
source: '/ingest/:path*',
destination: 'https://us.i.posthog.com/:path*',
},
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New rewrite rules are dead code due to existing route

Low Severity

The new /ingest rewrite rules will never be reached because an existing App Router route handler at app/ingest/[[...path]]/route.ts already handles all /ingest/* requests. Next.js evaluates filesystem routes (including API route handlers) before flat-array rewrites, so the optional catch-all route always takes precedence. These rewrites are effectively dead code and duplicate the proxy logic already implemented in the route handler.

Fix in Cursor Fix in Web

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Apr 3, 2026

Greptile Summary

This PR attempts to fix client-side PostHog event 404s by adding /ingest reverse-proxy rewrites to next.config.ts and documenting the NEXT_PUBLIC_POSTHOG_KEY / NEXT_PUBLIC_POSTHOG_ENABLED env vars in .env.example. The env var documentation is a clean, accurate improvement. However, the next.config.ts rewrite entries directly conflict with prior deliberate architecture.

  • Dead-code rewrites: PR fix(posthog): replace proxy rewrite with route handler for reliable body streaming #3187 (c471627ce) explicitly replaced these exact same Next.js rewrites with a dedicated App Router Route Handler at app/ingest/[[...path]]/route.ts for "reliable body streaming." Because rewrites() returns a plain array (treated as afterFiles by Next.js), rewrites only fire when no matching filesystem route exists — and the route handler covers all /ingest/* paths, so the new rewrites will never execute.
  • Risk of regression: If the route handler is ever removed (e.g., someone assumes the rewrites cover it), PostHog proxying would fall back to raw Next.js rewrites that don't strip auth cookies and don't guarantee reliable streaming of POST bodies.
  • .env.example changes: Accurate and well-documented; matches actual usage in posthog-provider.tsx and lib/core/config/env.ts.

Confidence Score: 4/5

Safe to merge as-is (behavior is unchanged since the route handler wins), but the rewrites should be removed to prevent future confusion and regression risk.

The .env.example change is straightforwardly correct. The next.config.ts rewrites are currently dead code (the existing route handler handles all /ingest/* traffic first), so they don't break anything today. However, they directly contradict a deliberate prior fix (PR #3187) and create a latent regression risk if the route handler is ever removed. A P1 issue warrants a 4/5 rather than 5/5.

apps/sim/next.config.ts — the two new /ingest rewrites are redundant with app/ingest/[[...path]]/route.ts and should be removed.

Important Files Changed

Filename Overview
apps/sim/next.config.ts Adds two PostHog ingest rewrites that are dead code — app/ingest/[[...path]]/route.ts (added by PR #3187 to replace these same rewrites) already handles all /ingest/* traffic with proper header stripping and body streaming; afterFiles rewrites never fire when a matching Route Handler exists.
apps/sim/.env.example Documents the two PostHog env vars (NEXT_PUBLIC_POSTHOG_KEY, NEXT_PUBLIC_POSTHOG_ENABLED) that are already used in the codebase but were previously undocumented; changes are accurate and match usage in posthog-provider.tsx and env.ts.

Sequence Diagram

sequenceDiagram
    participant Browser
    participant NextJS as Next.js Router
    participant RH as app/ingest/[[...path]]/route.ts
    participant Rewrite as next.config.ts rewrites
    participant PH as PostHog (us.i.posthog.com)

    Browser->>NextJS: POST /ingest/decide
    Note over NextJS: Checks filesystem routes first (afterFiles)
    NextJS->>RH: Route Handler matches /ingest/[[...path]]
    Note over RH: Strips cookie & connection headers<br/>Sets Host header<br/>Streams body with duplex:'half'
    RH->>PH: Proxied request (no cookies)
    PH-->>RH: Response
    RH-->>Browser: Forwarded response

    Note over Rewrite: These rewrites never fire —<br/>shadowed by the route handler above
Loading

Reviews (1): Last reviewed commit: "fix(analytics): add posthog ingest proxy..." | Re-trigger Greptile

Comment on lines +421 to +427
source: '/ingest/static/:path*',
destination: 'https://us-assets.i.posthog.com/static/:path*',
},
{
source: '/ingest/:path*',
destination: 'https://us.i.posthog.com/:path*',
},
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Rewrites conflict with (and are shadowed by) the existing route handler

PR #3187 (c471627ce) explicitly replaced these exact rewrites with app/ingest/[[...path]]/route.ts specifically for "reliable body streaming." That route handler already proxies all /ingest/* requests to us.i.posthog.com / us-assets.i.posthog.com, and does so with careful header handling (strips cookie, connection headers; sets Host; removes content-encoding on the response).

When rewrites() returns a plain array, Next.js treats them as afterFiles rewrites — meaning they only apply when no matching filesystem route exists. Since app/ingest/[[...path]]/route.ts is a valid App Router Route Handler for exactly these paths, Next.js will route traffic through the handler first, and these two rewrites will never execute.

In short, the new rewrites are dead code. If they were ever made to execute (e.g., if the route handler is later removed or if a future Next.js version changes precedence), they would silently regress the intentional improvements from #3187: request bodies may not stream reliably, and auth/session cookies would be forwarded to PostHog rather than stripped.

Consider removing these two rewrite entries to avoid confusion and keep the proxy logic consolidated in the route handler.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant