Repository navigation
Route browser analytics through first-party API - #7484
lawrencecchen wants to merge 7 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAnalytics tracking now routes through shared client helpers and a new server endpoint. Browser events are validated and forwarded server-side, while click, pageview, and waitlist flows use the new helper APIs and env gating. ChangesAnalytics Proxy Migration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 25✅ Passed checks (25 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Greptile SummaryThis PR moves browser analytics through a first-party server route. The main changes are:
Confidence Score: 4/5This is close, but the analytics proxy should be tightened before merging.
web/app/api/analytics/browser-events/route.ts
|
| Filename | Overview |
|---|---|
| web/app/api/analytics/browser-events/route.ts | Adds the browser analytics route with rate limiting, validation, and PostHog forwarding; the generic forwarding path still needs to filter mutation properties. |
| web/services/analytics/browserEventPolicy.ts | Adds the browser event allowlist and bounded property sanitization used by the new route. |
| web/app/lib/analytics.ts | Adds the client helper that sends browser analytics through the first-party route and maintains a fallback distinct ID. |
| web/app/[locale]/components/waitlist-dialog.tsx | Routes waitlist signup tracking through the new helper with server-validated enrollment properties. |
| web/app/[locale]/posthog.tsx | Disables PostHog SDK network capture and routes pageviews through the first-party helper. |
Reviews (6): Last reviewed commit: "Validate proxied waitlist analytics" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return jsonResponse({ error: event.error }, 400); | ||
| } | ||
|
|
||
| const forwarded = await forwardBrowserEvent(event.value); |
There was a problem hiding this comment.
Any caller can POST an allowlisted event with an arbitrary distinctId and properties, and this line forwards it to PostHog from the cmux server. A simple script can generate fake pageviews, clicks, or waitlist signups through /api/analytics/browser-events, which can poison funnels or burn analytics quota unless the route adds a compulsory abuse control before forwarding.
| email, | ||
| platforms, | ||
| location, | ||
| $set: { email, ...enrollment }, |
There was a problem hiding this comment.
The signup event now uses the email address as the forwarded distinct_id, while pageviews and clicks still use the local anonymous PostHog ID from getAnalyticsDistinctId(). Because this path does not send an alias or $anon_distinct_id, a normal visitor who browses anonymously and then joins the waitlist can end up with the waitlist event on a separate email-keyed person, breaking attribution from the earlier pageviews and clicks.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 9686abc. Configure here.
| if (typeof window !== "undefined") { | ||
| posthog.init("phc_opOVu7oFzR9wD3I6ZahFGOV2h3mqGpl5EHyQvmHciDP", { | ||
| api_host: "https://r.cmux.com", | ||
| api_host: "/api/analytics/posthog-sdk-disabled", |
There was a problem hiding this comment.
Missing PostHog SDK stub route
Medium Severity
posthog.init now sets api_host to /api/analytics/posthog-sdk-disabled, but the web app has no matching API handler under app/api/analytics. The SDK still issues requests to paths such as /e/ beneath that host, so they return 404 instead of a deliberate no-op sink.
Reviewed by Cursor Bugbot for commit 9686abc. Configure here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/app/api/analytics/browser-events/route.ts`:
- Around line 38-52: The PostHog forward in the browser events route can hang
indefinitely if the upstream stalls; update the fetch in the browser-events
route handler to use the existing AbortSignal.timeout(...) pattern so the
request is canceled before the platform timeout. Keep the change scoped to the
fetch that posts to POSTHOG_HOST/batch and preserve the current payload and
headers.
In `@web/app/lib/analytics.ts`:
- Around line 48-55: The fallback in getAnalyticsDistinctId currently returns a
shared "anonymous" value, causing unrelated users to collapse into one analytics
identity. Update getAnalyticsDistinctId to generate a per-browser fallback
distinct id when posthog.get_distinct_id() is missing or invalid, and persist it
in a first-party browser store so it is reused across sessions. Reuse the
existing getAnalyticsDistinctId entry point and keep the returned value stable
and non-empty for each browser.
In `@web/tests/browser-analytics-route.test.ts`:
- Around line 48-49: The test in browser-analytics-route.test.ts is asserting a
hardcoded production PostHog key instead of the configured test project key.
Update the expectation in the route test to use the test fixture value already
emitted by the route, or import and assert the shared configured constant used
by the analytics setup. Use the existing browser analytics route test assertions
and the emitted api_key field to locate the check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9014d2d2-167c-4bf2-8019-962910e2be55
📒 Files selected for processing (14)
web/app/[locale]/(landing)/tracked-link.tsxweb/app/[locale]/components/download-button.tsxweb/app/[locale]/components/faq-platform-answer.tsxweb/app/[locale]/components/github-button.tsxweb/app/[locale]/components/github-stars.tsxweb/app/[locale]/components/nav-links.tsxweb/app/[locale]/components/pro-cta-link.tsxweb/app/[locale]/components/waitlist-callout.tsxweb/app/[locale]/components/waitlist-dialog.tsxweb/app/[locale]/posthog.tsxweb/app/api/analytics/browser-events/route.tsweb/app/lib/analytics.tsweb/services/analytics/browserEventPolicy.tsweb/tests/browser-analytics-route.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/app/api/analytics/browser-events/route.ts (1)
61-84: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCancel the response body before returning. Unread
fetchbodies keep the Node/undici socket occupied until they’re drained or canceled, which can exhaust the pool on this forwarding path.♻️ Proposed fix
if (!response.ok) { + await response.body?.cancel(); return { ok: false, status: response.status >= 500 ? 502 : 400 }; } + await response.body?.cancel(); return { ok: true };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/app/api/analytics/browser-events/route.ts` around lines 61 - 84, The forwarding logic in the browser analytics route leaves the fetch response body unread, which can keep the undici socket busy; update the try block around the POSTHOG fetch so the Response is explicitly canceled before every return path. In the handler that performs the `fetch` call, ensure the body is canceled after checking `response.ok` and also when handling non-OK responses or errors, so the socket is released promptly and the forwarding path does not exhaust the pool.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@web/app/api/analytics/browser-events/route.ts`:
- Around line 61-84: The forwarding logic in the browser analytics route leaves
the fetch response body unread, which can keep the undici socket busy; update
the try block around the POSTHOG fetch so the Response is explicitly canceled
before every return path. In the handler that performs the `fetch` call, ensure
the body is canceled after checking `response.ok` and also when handling non-OK
responses or errors, so the socket is released promptly and the forwarding path
does not exhaust the pool.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 11548e69-74fe-4df1-bd40-ffd857651484
📒 Files selected for processing (2)
web/app/api/analytics/browser-events/route.tsweb/tests/browser-analytics-route.test.ts
| const BROWSER_ANALYTICS_FORWARD_TIMEOUT_MS = 3_000; | ||
|
|
||
| export async function POST(request: Request): Promise<Response> { | ||
| if (process.env.VERCEL === "1") { |
There was a problem hiding this comment.
This route only checks checkRateLimit when VERCEL === "1". When the app runs in a self-hosted or non-Vercel environment, the same public endpoint skips that guard and still forwards allowlisted events with caller-controlled distinctId, timestamp, $set, and $set_once to PostHog using the server-side project key. That leaves an unauthenticated event proxy reachable in those deployments, so forged pageviews, clicks, or waitlist signups can still poison analytics or mutate person properties. The route should fail closed unless a real abuse control is active.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/app/lib/analytics.ts`:
- Around line 15-17: `getFallbackDistinctId` currently prefers `localStorage`
over the module-level `memoryFallbackDistinctId`, which can let a later storage
read overwrite an already-cached in-memory id and split a tab’s session. Update
the fallback resolution in `web/app/lib/analytics.ts` so the function returns
`memoryFallbackDistinctId` immediately when it is already set, and only consults
`FALLBACK_DISTINCT_ID_KEY` or generate a new id when memory is empty. Keep
`memoryFallbackDistinctId` and storage in sync when a new value is chosen, and
add a reset hook for the module-level cache if needed for tests, matching the
companion `analytics-client.test.ts` expectations.
In `@web/tests/analytics-client.test.ts`:
- Around line 23-27: The analytics tests are order-dependent because the
module-level memoryFallbackDistinctId cache in analytics.ts is not reset between
tests. Add a test-only reset hook on the analytics module (or equivalent helper)
and invoke it from the beforeEach in analytics-client.test.ts alongside the
existing localStorage setup. Make sure the reset clears the cached fallback id
so getFallbackDistinctId exercises the fresh-generation path in each test, using
the existing test helpers and the analytics.ts symbols that manage the fallback
state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 00d36d6e-6d91-47b4-93d5-d71a31079f25
📒 Files selected for processing (2)
web/app/lib/analytics.tsweb/tests/analytics-client.test.ts
| const FALLBACK_DISTINCT_ID_KEY = "cmux.analytics.distinct_id"; | ||
| let memoryFallbackDistinctId: string | null = null; | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
getFallbackDistinctId lets storage silently override an already-cached memory id.
Storage is checked before memoryFallbackDistinctId on every call. If two tabs race before either persists (both see an empty store), each generates a different fallback id; whichever tab writes last "wins" in localStorage, and the other tab's next call will read that foreign value and overwrite its own in-memory id — splitting a single tab's session across two distinct ids mid-flight. Prefer the already-cached memory value once set, and only fall back to storage/generation when memory is empty.
🔧 Proposed fix
function getFallbackDistinctId(): string {
+ if (memoryFallbackDistinctId) {
+ return memoryFallbackDistinctId;
+ }
+
const storage = browserStorage();
const stored = readStoredDistinctId(storage);
if (stored) {
memoryFallbackDistinctId = stored;
return stored;
}
- if (memoryFallbackDistinctId) {
- writeStoredDistinctId(storage, memoryFallbackDistinctId);
- return memoryFallbackDistinctId;
- }
-
const generated = createFallbackDistinctId();
memoryFallbackDistinctId = generated;
writeStoredDistinctId(storage, generated);
return generated;
}Also consider exporting a reset hook for the module-level cache — see the companion comment in web/tests/analytics-client.test.ts.
As per coding guidelines, .github/review-bot-rules/reliability-single-source-of-truth.md flags "more than one disagreeing source of truth for the same fact."
Also applies to: 69-87
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/app/lib/analytics.ts` around lines 15 - 17, `getFallbackDistinctId`
currently prefers `localStorage` over the module-level
`memoryFallbackDistinctId`, which can let a later storage read overwrite an
already-cached in-memory id and split a tab’s session. Update the fallback
resolution in `web/app/lib/analytics.ts` so the function returns
`memoryFallbackDistinctId` immediately when it is already set, and only consults
`FALLBACK_DISTINCT_ID_KEY` or generate a new id when memory is empty. Keep
`memoryFallbackDistinctId` and storage in sync when a new value is chosen, and
add a reset hook for the module-level cache if needed for tests, matching the
companion `analytics-client.test.ts` expectations.
Sources: Coding guidelines, Path instructions
| beforeEach(() => { | ||
| postHogDistinctId.mockClear(); | ||
| currentPostHogDistinctId = () => "posthog-distinct-id"; | ||
| setLocalStorage(new MemoryStorage()); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
memoryFallbackDistinctId leaks across tests, making test 4 pass for the wrong reason.
beforeEach resets localStorage but not the module-level memoryFallbackDistinctId cache in analytics.ts. Tracing execution order: test 2 ("persists a generated fallback id...") leaves memoryFallbackDistinctId set to a generated id; test 3 masks the leak because storage is checked first and explicitly seeded. By test 4 ("keeps a page-lifetime fallback id when browser storage is unavailable"), memoryFallbackDistinctId still holds test 3's "cmux-web-existing-browser-id", so getFallbackDistinctId() returns that leaked value via the if (memoryFallbackDistinctId) branch instead of generating a fresh id. The assertions (toStartWith("cmux-web-"), second === first) pass coincidentally against the stale value, so this test currently does not exercise the "storage unavailable → generate new id" path at all.
Add a test-only reset hook and call it in beforeEach so each test starts from a clean module state.
🔧 Proposed fix
// web/app/lib/analytics.ts
+export function __resetAnalyticsDistinctIdCacheForTests(): void {
+ memoryFallbackDistinctId = null;
+}-const { getAnalyticsDistinctId } = await import("../app/lib/analytics");
+const { getAnalyticsDistinctId, __resetAnalyticsDistinctIdCacheForTests } = await import(
+ "../app/lib/analytics"
+);
beforeEach(() => {
postHogDistinctId.mockClear();
currentPostHogDistinctId = () => "posthog-distinct-id";
setLocalStorage(new MemoryStorage());
+ __resetAnalyticsDistinctIdCacheForTests();
});As per path instructions for web/tests/**, "Do not make tests order-dependent on shared static/global state... unless it is reset per test."
Also applies to: 54-64
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/tests/analytics-client.test.ts` around lines 23 - 27, The analytics tests
are order-dependent because the module-level memoryFallbackDistinctId cache in
analytics.ts is not reset between tests. Add a test-only reset hook on the
analytics module (or equivalent helper) and invoke it from the beforeEach in
analytics-client.test.ts alongside the existing localStorage setup. Make sure
the reset clears the cached fallback id so getFallbackDistinctId exercises the
fresh-generation path in each test, using the existing test helpers and the
analytics.ts symbols that manage the fallback state.
Source: Path instructions
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/app/api/analytics/browser-events/route.ts`:
- Around line 91-93: The cmuxterm_waitlist_signup branch in browserEventPolicy
currently returns event.properties unchanged, which allows reserved PostHog
control fields to pass through. Update this special-case path to filter or
reject reserved keys the same way as the existing $process_person_profile
handling, ideally by applying the same guard logic or an explicit allowlist in
browserEventPolicy before returning the properties.
- Around line 87-99: The waitlist event name is duplicated in postHogProperties,
so update it to reuse the exported constant from browserEventPolicy.ts instead
of hardcoding the string. Add or reuse a named export for the cmuxterm waitlist
event in browserEventPolicy and import it into the browser-events route so the
PostHog special case stays aligned with the allowlist if the event name changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6b2c068e-cf7b-4405-a360-9435f787a378
📒 Files selected for processing (2)
web/app/api/analytics/browser-events/route.tsweb/tests/browser-analytics-route.test.ts
| function postHogProperties(event: { | ||
| readonly event: string; | ||
| readonly properties: Record<string, unknown>; | ||
| }): Record<string, unknown> { | ||
| if (event.event === "cmuxterm_waitlist_signup") { | ||
| return event.properties; | ||
| } | ||
|
|
||
| return { | ||
| ...event.properties, | ||
| $process_person_profile: false, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "cmuxterm_waitlist_signup" web/services/analytics/browserEventPolicy.ts web/app/lib/analytics.ts -C 3Repository: manaflow-ai/cmux
Length of output: 690
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== browserEventPolicy exports =="
sed -n '1,220p' web/services/analytics/browserEventPolicy.ts
echo
echo "== route imports and postHogProperties =="
sed -n '1,180p' web/app/api/analytics/browser-events/route.ts
echo
echo "== repo-wide WAITLIST_SIGNUP_EVENT search =="
rg -n "WAITLIST_SIGNUP_EVENT" web . -S || trueRepository: manaflow-ai/cmux
Length of output: 7683
Export the waitlist event name from browserEventPolicy.ts and reuse it here. The special-case duplicates the same string literal as the policy allowlist, so a rename would leave PostHog forwarding behavior out of sync.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/app/api/analytics/browser-events/route.ts` around lines 87 - 99, The
waitlist event name is duplicated in postHogProperties, so update it to reuse
the exported constant from browserEventPolicy.ts instead of hardcoding the
string. Add or reuse a named export for the cmuxterm waitlist event in
browserEventPolicy and import it into the browser-events route so the PostHog
special case stays aligned with the allowlist if the event name changes.
Source: Coding guidelines
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/tests/browser-analytics-route.test.ts (1)
98-128: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winThis test locks in the reserved-key passthrough as expected behavior.
The assertion that
$set/$set_onceare forwarded verbatim for the waitlist event codifies the vulnerability flagged inroute.ts(lines 87-99). Once the route is fixed to strip reserved keys on this path, this test needs to be updated to assert they're filtered rather than preserved.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/tests/browser-analytics-route.test.ts` around lines 98 - 128, The waitlist enrollment test in browser-analytics-route.test.ts currently asserts that reserved properties are passed through unchanged, which conflicts with the intended fix in the route handler. Update the expectation for the POST path used by the waitlist event so that $set and $set_once are filtered or removed rather than preserved, and keep the assertion around the payload produced by POST/request to match the corrected behavior in route.ts.
♻️ Duplicate comments (2)
web/app/api/analytics/browser-events/route.ts (2)
91-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated event-name literal drifts from the allowlist.
"cmuxterm_waitlist_signup"is hardcoded here and separately inbrowserEventPolicy.ts's allowlist. A rename in one place silently breaks this special case. Export and reuse a shared constant.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/app/api/analytics/browser-events/route.ts` at line 91, The special-case event name is duplicated and can drift from the allowlist; extract the `"cmuxterm_waitlist_signup"` value into a shared constant and reuse it in both `route.ts` and `browserEventPolicy.ts`. Update the comparison in `browser-events/route.ts` to reference that constant, and make the allowlist in `browserEventPolicy.ts` import the same symbol so there is one source of truth.
87-99: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReserved PostHog keys still pass through unfiltered on the waitlist path.
For
cmuxterm_waitlist_signup,event.propertiesis forwarded to PostHog completely unchecked. Since this is a public, unauthenticated endpoint accepting client-supplieddistinctIdandproperties, a caller can inject reserved PostHog control fields ($set,$set_once,$ip, etc.) to overwrite person-profile properties or geolocation for an arbitrarydistinctId— including IDs belonging to other real visitors. This is the same gap flagged in a prior review round (no "Addressed" marker on that comment, and the behavior is unchanged in this diff). The companion test (web/tests/browser-analytics-route.test.tslines 98-128) now asserts this passthrough as expected behavior, effectively codifying the gap.Apply the same
$process_person_profileguard/allowlist to the waitlist branch instead of returningevent.propertiesverbatim.🔒 Proposed fix to strip reserved keys on the waitlist path
+const RESERVED_POSTHOG_PROPERTY_KEYS = new Set([ + "$set", + "$set_once", + "$unset", + "$ip", + "$process_person_profile", + "$group_0", + "$group_1", + "$group_2", + "$group_3", + "$group_4", +]); + function postHogProperties(event: { readonly event: string; readonly properties: Record<string, unknown>; }): Record<string, unknown> { if (event.event === "cmuxterm_waitlist_signup") { - return event.properties; + return Object.fromEntries( + Object.entries(event.properties).filter( + ([key]) => !RESERVED_POSTHOG_PROPERTY_KEYS.has(key), + ), + ); } return { ...event.properties, $process_person_profile: false, }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/app/api/analytics/browser-events/route.ts` around lines 87 - 99, The waitlist branch in postHogProperties is still returning client-supplied properties verbatim, which allows reserved PostHog fields to slip through. Update postHogProperties so cmuxterm_waitlist_signup applies the same protection as the other events: strip or allowlist properties and ensure $process_person_profile is set to false rather than forwarding event.properties directly. Make the change in the postHogProperties helper and adjust any browser-events tests that currently expect unrestricted passthrough so they verify reserved keys are removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@web/tests/browser-analytics-route.test.ts`:
- Around line 98-128: The waitlist enrollment test in
browser-analytics-route.test.ts currently asserts that reserved properties are
passed through unchanged, which conflicts with the intended fix in the route
handler. Update the expectation for the POST path used by the waitlist event so
that $set and $set_once are filtered or removed rather than preserved, and keep
the assertion around the payload produced by POST/request to match the corrected
behavior in route.ts.
---
Duplicate comments:
In `@web/app/api/analytics/browser-events/route.ts`:
- Line 91: The special-case event name is duplicated and can drift from the
allowlist; extract the `"cmuxterm_waitlist_signup"` value into a shared constant
and reuse it in both `route.ts` and `browserEventPolicy.ts`. Update the
comparison in `browser-events/route.ts` to reference that constant, and make the
allowlist in `browserEventPolicy.ts` import the same symbol so there is one
source of truth.
- Around line 87-99: The waitlist branch in postHogProperties is still returning
client-supplied properties verbatim, which allows reserved PostHog fields to
slip through. Update postHogProperties so cmuxterm_waitlist_signup applies the
same protection as the other events: strip or allowlist properties and ensure
$process_person_profile is set to false rather than forwarding event.properties
directly. Make the change in the postHogProperties helper and adjust any
browser-events tests that currently expect unrestricted passthrough so they
verify reserved keys are removed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 48967811-b803-4ad2-9371-e2b155b6f8c3
📒 Files selected for processing (4)
web/app/api/analytics/browser-events/route.tsweb/app/env.tsweb/tests/browser-analytics-route.test.tsweb/tests/client-config-env.test.ts
| value: { | ||
| ...event, | ||
| properties: { | ||
| ...event.properties, | ||
| $process_person_profile: false, |
There was a problem hiding this comment.
This generic event path still forwards caller-controlled PostHog control properties. A script can POST an allowlisted $pageview or click with any distinctId plus $set, $set_once, or $groups; sanitizeProperties only bounds those values, and this spread sends them to PostHog from the server. That lets the public endpoint mutate profile or group data for arbitrary analytics IDs instead of only forwarding inert click/pageview metadata. Strip PostHog person and group mutation keys here, and keep the server-generated $set fields limited to the waitlist path.
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
2 participants
Summary
/api/analytics/browser-events, an allowlisted first-party route that forwards to PostHog server-side.r.cmux.comor/e/browser requests.Testing
bun test tests/browser-analytics-route.test.tsbun run typecheckbunx eslint app/[locale]/posthog.tsx app/[locale]/(landing)/tracked-link.tsx app/[locale]/components/download-button.tsx app/[locale]/components/faq-platform-answer.tsx app/[locale]/components/github-button.tsx app/[locale]/components/github-stars.tsx app/[locale]/components/nav-links.tsx app/[locale]/components/pro-cta-link.tsx app/[locale]/components/waitlist-callout.tsx app/[locale]/components/waitlist-dialog.tsx app/lib/analytics.ts app/api/analytics/browser-events/route.ts services/analytics/browserEventPolicy.ts tests/browser-analytics-route.test.tsbun run lintcurrently fails on unrelated pre-existing lint errors in billing/vault/test files.Issues
https://r.cmux.com/i/v0/e/?...Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Touches all marketing analytics and waitlist person properties in PostHog; misconfiguration of the new rate limiter or forwarding could drop events or block signups in production until env is set.
Overview
Browser analytics no longer posts directly to
r.cmux.comfrom the client. Clicks, pageviews, and waitlist signups go through new helpers inapp/lib/analytics.tsthat POST to/api/analytics/browser-events, which allowlists events, sanitizes payloads, rate-limits on Vercel viaCMUX_BROWSER_ANALYTICS_RATE_LIMIT_ID, and forwards to PostHog/batch/server-side.posthog-jsis reconfigured so the SDK does not drive ingest:api_hostpoints at a disabled path, autocapture/pageleave/session recording/surveys/flags/decide are off, and$pageviewis sent via the first-party helper. The SDK remains mainly forget_distinct_id, with a localStorage/in-memory fallback when that is unavailable.Waitlist signup stops using a direct browser POST to PostHog’s
/e/endpoint; it usescaptureAnalyticsEventand the route re-validates email, platforms, anddistinctIdbefore applying server-controlled$set/ Early Access enrollment.Landing and marketing UI components swap
posthog.captureforcaptureAnalyticsClick(download, GitHub, pricing, pro CTA, tracked links, waitlist opens).Reviewed by Cursor Bugbot for commit be9c2bc. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Route all browser analytics through a first‑party API that forwards events to PostHog server‑side, with a dedicated Vercel rate limiter, a persistent fallback distinct id, anonymous‑profile behavior for non‑signup events, and strict server validation for waitlist signups. This avoids uBlock/extension blocks on
r.cmux.comwhile keeping attribution via the SDK or a local fallback.New Features
/api/analytics/browser-eventsto allowlist events, sanitize payloads, rate‑limit via Vercel Firewall usingCMUX_BROWSER_ANALYTICS_RATE_LIMIT_ID, and forward to PostHog/batch/(returns 429 when limited, 503 if misconfigured). Sets$process_person_profile: falsefor forwarded events.distinctId === email, allowlists platforms, and normalizes$set/$set_once(adds$feature_enrollment/*); rejects invalid payloads before forwarding while preserving enrollment for valid signups.app/lib/analytics.tswithcaptureAnalyticsEventandcaptureAnalyticsClick, plus a stored fallback distinct id (localStorage with in‑memory backup).$pageviewand clicks use this helper. Tests cover forwarding, event validation (incl. waitlist), rate limiting, distinct‑id fallback, and profile‑processing behavior.Refactors
posthog-jscaptures withcaptureAnalyticsClickacross links/buttons and routed waitlist signup through the first‑party API; UI waits for server acceptance.posthog-jsto not send network analytics (disabled autocapture, pageleave, session recording, surveys, flags/decide, and external dependency loading) and setapi_hostto a disabled path; SDK remains forget_distinct_idonly.Written for commit be9c2bc. Summary will update on new commits.
Summary by CodeRabbit