feat: Implement two-tier consent model for PostHog analytics#160
Merged
Conversation
- Introduced a two-tier consent model in useAnalyticsConsent composable. - Updated usePostHogIdentity to handle PII based on consent. - Enhanced useTrack to capture events without consent gating. - Added demo account isolation with is_demo super property for analytics. - Implemented URL property sanitization in posthog-identity plugin. - Updated nuxt.config for cookieless tracking and GDPR compliance. - Created unit tests to verify consent model and identity handling.
|
🚅 Deployed to the reqcore-pr-160 environment in applirank
|
📝 WalkthroughWalkthroughThis PR refactors PostHog analytics consent and privacy handling by removing consent-gated event buffering, simplifying the consent model to a two-tier approach (cookieless or localStorage+cookie), restricting PII to consent-granted users only, and introducing demo account isolation via a new shared utility. Changes
Sequence DiagramsequenceDiagram
participant User
participant App as App Instance
participant Consent as useAnalyticsConsent
participant Identity as usePostHogIdentity
participant PostHog as PostHog Client
participant Server as Server (trackEvent)
participant DB as Organization DB
User->>App: Load page / Login
App->>Consent: Check consent state
alt Consent Declined or Not Decided
Consent->>PostHog: Keep memory-only persistence
PostHog->>PostHog: person_profiles: 'identified_only'
else Consent Granted
Consent->>PostHog: Upgrade to localStorage+cookie
PostHog->>PostHog: Enable cross_subdomain_cookie
end
User->>App: User logs in
App->>Identity: Log in with user.id + email
Identity->>PostHog: identify(user.id)
alt Email forwarding (Consent Granted)
Identity->>PostHog: identify(user.id, {email, name})
end
User->>App: Active organization set
App->>Identity: Set organization context
Identity->>DB: Check if demo org
DB-->>Identity: Organization details
alt Is Demo Organization
Identity->>PostHog: Set is_demo super property
Identity->>PostHog: Set org group with name
else Regular Organization
Identity->>PostHog: Set org group
end
User->>App: Trigger tracked event
App->>Server: POST /api/event
Server->>Server: Compute is_demo flag
Server->>DB: Query org from activeOrganizationId
DB-->>Server: Organization ID resolved
Server->>PostHog: ph.capture(event, {is_demo: true/false})
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
nuxt.config.ts (1)
118-133: LGTM — privacy posture is materially improved.
persistence: "memory"+person_profiles: "identified_only"+$ip/$initial_ipdenylisting gives you an ePrivacy-compliant default that still produces useful funnel/retention data once a user identifies.Optional nit: the comment "no consent banner needed" is technically true for storage-based consent only (Art. 5(3) ePrivacy). The repo still ships a
ConsentBanner.vueto gate the persistence upgrade, so consider rewording to "no consent banner needed for storage" to avoid future confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nuxt.config.ts` around lines 118 - 133, Update the inline comment near the PostHog config (references: persistence, person_profiles, property_denylist) to clarify that "no consent banner needed" only applies to storage-based consent (Art. 5(3) ePrivacy); reword it to something like "no consent banner needed for storage" or similar to avoid implying the app never requires any consent, and add a short note referencing the existing ConsentBanner.vue which still gates the persistence upgrade.server/utils/demoOrg.ts (1)
50-75: Minor: brief thundering-herd on cold cache.On boot, the first burst of authenticated requests will each issue the same
select … where slug = $1lookup before any of them populatesdemoOrgIdBySlug. Result is correct (idempotent reads) but you're paying N redundant DB hits for the first wave. If demo-org volume matters, dedupe in-flight lookups via aMap<string, Promise<string|undefined>>. Otherwise, ignore — once warm it's a Map hit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/utils/demoOrg.ts` around lines 50 - 75, getDemoOrgIds suffers a thundering-herd on cold cache because concurrent callers all query the DB for the same slug before demoOrgIdBySlug is populated; fix by adding an in-flight lookup map (e.g., inFlightLookups: Map<string, Promise<string|undefined>>) checked at the start of the slug loop, store the promise before issuing the DB select (the promise resolves to the id or undefined), await the in-flight promise instead of duplicating the query, on resolution populate demoOrgIdBySlug and demoOrgIdSet and remove the entry from inFlightLookups; reference getDemoOrgIds, demoOrgIdBySlug, demoOrgIdSet and ensure the in-flight map is used and cleaned up on success/failure to dedupe concurrent DB lookups.tests/unit/posthog-cookieless.test.ts (1)
17-227: Recommended: replace source-text regex assertions with behavior-level tests.The entire suite reads source files with
fs.readFileSyncand asserts viatoMatch(/regex/). This has two well-known weaknesses:
- False positives — assertions pass when the matched line exists but is unreachable, dead, or semantically wrong. For example, the suite passes today even though the org-watcher in
usePostHogIdentity.tsnever clearsis_demowhen leaving the demo org (see the major issue raised on that file). Behavior-level tests with a mockedposthog-jswould have caught it.- False negatives — Prettier/format changes, identifier renames, or moving
set_config({...})to a helper all break tests without changing behavior. Several patterns are particularly brittle:\/\^\[\\w-\]\{10,100\}\$\/(line 179) and the multi-propposthog.group(...)literal (line 119).Suggested direction (incremental — keep existing tests as a safety net while you migrate):
- Mock
posthog-jswithvi.mockand stubcapture/identify/group/register/set_config/unregister.- For
useAnalyticsConsent, driveacceptAnalytics()/declineAnalytics()and assert the resulting calls + cookie state.- For
usePostHogIdentity, mount a Vue component using@vue/test-utils, simulate session/org/consent transitions, and assertposthog.identifyis called with/without PII, plus theis_demoflag flow (which would surface the bug above).- For
useTrack, simply assertcaptureis invoked even withconsent === null.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/posthog-cookieless.test.ts` around lines 17 - 227, Replace brittle source-text regex assertions with behavior-level tests by mocking posthog-js via vi.mock and asserting calls to posthog.capture/identify/group/register/set_config/unregister; for useAnalyticsConsent, write tests that call acceptAnalytics() and declineAnalytics() and assert cookie state and that set_config/opt_out/identify are (not) invoked; for usePostHogIdentity, mount a test component with `@vue/test-utils`, simulate session/org/consent transitions and assert posthog.identify is called with/without PII, posthog.group calls include org id/name appropriately, and the is_demo flag is toggled/cleared across org changes; for useTrack and server trackEvent, assert posthog.capture is invoked with stable distinctId and proper groups/demos; keep the existing file-text regex tests as a safety net while migrating.app/composables/usePostHogIdentity.ts (1)
78-78: Nit:asyncon the org watcher is unused.The callback never
awaits anything; droppingasyncremoves a needless microtask hop and keeps the intent explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/composables/usePostHogIdentity.ts` at line 78, Remove the unnecessary async keyword from the watcher callback signature — change the callback from "async ([org, consented]) => { ... }" to a plain "([org, consented]) => { ... }" in usePostHogIdentity; the callback never awaits anything, so dropping async avoids an extra microtask hop and makes intent explicit (no other behavior changes needed).app/plugins/posthog-identity.client.ts (1)
133-140: Consider collapsing the identify branches.
posthog.identifyacceptsundefinedfor the second argument and behaves identically to the single-arg form, making the if/else purely stylistic. Not a blocker — feel free to keep it for readability if you prefer the explicit shape.♻️ Optional simplification
- posthogIdentifyUser: (userId: string, properties?: Record<string, string | undefined>) => { - if (properties) { - posthog.identify(userId, properties) - } - else { - posthog.identify(userId) - } - }, + posthogIdentifyUser: (userId: string, properties?: Record<string, string | undefined>) => { + posthog.identify(userId, properties) + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/plugins/posthog-identity.client.ts` around lines 133 - 140, The posthogIdentifyUser function has an unnecessary if/else because posthog.identify(userId, properties) accepts undefined for the second argument; simplify by always calling posthog.identify with properties (i.e., replace the conditional in posthogIdentifyUser so it just calls posthog.identify(userId, properties)), leaving behavior unchanged and reducing boilerplate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/plugins/posthog-identity.client.ts`:
- Around line 161-183: The JSDoc for posthogSetDemoFlag claims person properties
are forwarded "(when consented)" but the implementation always calls
posthog.setPersonProperties; either remove that qualifier from the comment to
match current behavior, or change posthogSetDemoFlag signature to accept a
consent boolean (e.g., posthogSetDemoFlag(isDemo: boolean, consented: boolean))
and wrap calls to posthog.setPersonProperties({ is_demo: … }) in an
if(consented) guard; if you add the parameter, update all call sites in
usePostHogIdentity (the setDemoFlag invocations) to pass the consented value.
- Around line 89-98: The ph_did query param must be removed from the URL
regardless of consent: keep the existing validation (marketingDistinctId and
isValidDistinctId) and the consent check that calls
posthog.alias(marketingDistinctId) when consentCookie.value === 'granted', but
move the url.searchParams.delete('ph_did') and urlModified = true out of the
consent-gated branch so they always run when isValidDistinctId is true (after
any alias call); reference marketingDistinctId, isValidDistinctId,
consentCookie.value, posthog.alias, url.searchParams.delete and urlModified when
making the change.
In `@tests/unit/posthog-cookieless.test.ts`:
- Around line 67-73: The test's regex that extracts the declineAnalytics body is
brittle and will break on nested braces; update the test for declineAnalytics to
use a robust extraction (either parse balanced braces with a small
brace-counting routine or anchor to the top-level closing token such as matching
until "\n}\n") instead of the current /function
declineAnalytics\(\)\s*\{([\s\S]*?)\n\s*\}/ pattern so the full function body is
captured reliably and the subsequent expect(...).not.toMatch checks run against
the true body.
---
Nitpick comments:
In `@app/composables/usePostHogIdentity.ts`:
- Line 78: Remove the unnecessary async keyword from the watcher callback
signature — change the callback from "async ([org, consented]) => { ... }" to a
plain "([org, consented]) => { ... }" in usePostHogIdentity; the callback never
awaits anything, so dropping async avoids an extra microtask hop and makes
intent explicit (no other behavior changes needed).
In `@app/plugins/posthog-identity.client.ts`:
- Around line 133-140: The posthogIdentifyUser function has an unnecessary
if/else because posthog.identify(userId, properties) accepts undefined for the
second argument; simplify by always calling posthog.identify with properties
(i.e., replace the conditional in posthogIdentifyUser so it just calls
posthog.identify(userId, properties)), leaving behavior unchanged and reducing
boilerplate.
In `@nuxt.config.ts`:
- Around line 118-133: Update the inline comment near the PostHog config
(references: persistence, person_profiles, property_denylist) to clarify that
"no consent banner needed" only applies to storage-based consent (Art. 5(3)
ePrivacy); reword it to something like "no consent banner needed for storage" or
similar to avoid implying the app never requires any consent, and add a short
note referencing the existing ConsentBanner.vue which still gates the
persistence upgrade.
In `@server/utils/demoOrg.ts`:
- Around line 50-75: getDemoOrgIds suffers a thundering-herd on cold cache
because concurrent callers all query the DB for the same slug before
demoOrgIdBySlug is populated; fix by adding an in-flight lookup map (e.g.,
inFlightLookups: Map<string, Promise<string|undefined>>) checked at the start of
the slug loop, store the promise before issuing the DB select (the promise
resolves to the id or undefined), await the in-flight promise instead of
duplicating the query, on resolution populate demoOrgIdBySlug and demoOrgIdSet
and remove the entry from inFlightLookups; reference getDemoOrgIds,
demoOrgIdBySlug, demoOrgIdSet and ensure the in-flight map is used and cleaned
up on success/failure to dedupe concurrent DB lookups.
In `@tests/unit/posthog-cookieless.test.ts`:
- Around line 17-227: Replace brittle source-text regex assertions with
behavior-level tests by mocking posthog-js via vi.mock and asserting calls to
posthog.capture/identify/group/register/set_config/unregister; for
useAnalyticsConsent, write tests that call acceptAnalytics() and
declineAnalytics() and assert cookie state and that set_config/opt_out/identify
are (not) invoked; for usePostHogIdentity, mount a test component with
`@vue/test-utils`, simulate session/org/consent transitions and assert
posthog.identify is called with/without PII, posthog.group calls include org
id/name appropriately, and the is_demo flag is toggled/cleared across org
changes; for useTrack and server trackEvent, assert posthog.capture is invoked
with stable distinctId and proper groups/demos; keep the existing file-text
regex tests as a safety net while migrating.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: c27cff00-f93f-4ecd-ba49-11653034dbd5
📒 Files selected for processing (9)
app/composables/useAnalyticsConsent.tsapp/composables/usePostHogIdentity.tsapp/composables/useTrack.tsapp/plugins/posthog-identity.client.tsnuxt.config.tsserver/middleware/demo-guard.tsserver/utils/demoOrg.tsserver/utils/trackEvent.tstests/unit/posthog-cookieless.test.ts
Comment on lines
+90
to
+98
| // Demo tagging by org slug — covers self-hosted deployments | ||
| // where the demo org may be owned by a non-demo email account. | ||
| // We only set true here; the email-based watcher above handles | ||
| // clearing the flag when the user is not the demo user. | ||
| const setDemoFlag = $posthogSetDemoFlag as ((isDemo: boolean) => void) | undefined | ||
| if (setDemoFlag && demoOrgSlug && org.slug === demoOrgSlug) { | ||
| setDemoFlag(true) | ||
| } | ||
| } |
There was a problem hiding this comment.
Demo flag is never cleared when the active org changes from demo → non-demo.
The org watcher only sets is_demo to true; it relies on the email watcher to clear it. But the email watcher keys on session.value and hasConsented — switching the active org does not change session.value (it lives in activeOrgState), so the email watcher won't re-fire.
Concrete failure mode:
- User with a non-demo email is currently active in the demo org → org watcher sets
is_demo=true. - User switches to a non-demo org → org watcher sees
org.slug !== demoOrgSlugand does nothing. Email watcher doesn't fire either. is_demostaystruefor events from a real user in a real org → those events get filtered out of "real user" funnels/dashboards, silently corrupting metrics.
Set the flag from the org context unconditionally (or from the OR of both signals):
🐛 Proposed fix
- // Demo tagging by org slug — covers self-hosted deployments
- // where the demo org may be owned by a non-demo email account.
- // We only set true here; the email-based watcher above handles
- // clearing the flag when the user is not the demo user.
- const setDemoFlag = $posthogSetDemoFlag as ((isDemo: boolean) => void) | undefined
- if (setDemoFlag && demoOrgSlug && org.slug === demoOrgSlug) {
- setDemoFlag(true)
- }
+ // Demo tagging by org slug — covers self-hosted deployments where
+ // the demo org may be owned by a non-demo email account. We must
+ // set both true AND false here, because the email-based watcher
+ // does not re-fire when only the active org changes.
+ const setDemoFlag = $posthogSetDemoFlag as ((isDemo: boolean) => void) | undefined
+ if (setDemoFlag && demoOrgSlug) {
+ const isDemoUser = !!user?.email && user.email === liveDemoEmail
+ setDemoFlag(isDemoUser || org.slug === demoOrgSlug)
+ }You'll need access to the current user inside the org watcher (e.g. read it from session.value?.user at the top of the callback) to combine both signals safely.
Comment on lines
89
to
98
| const marketingDistinctId = url.searchParams.get('ph_did') | ||
| // Validate format: PostHog distinct IDs are typically UUIDs or short | ||
| // alphanumeric strings. Reject anything outside that to prevent | ||
| // passing arbitrary untrusted input to posthog.alias(). | ||
| const isValidDistinctId = marketingDistinctId && /^[\w-]{10,100}$/.test(marketingDistinctId) | ||
| if (isValidDistinctId && storedConsent === 'granted') { | ||
| // Create an alias so both IDs resolve to the same person in PostHog. | ||
| if (isValidDistinctId && consentCookie.value === 'granted') { | ||
| posthog.alias(marketingDistinctId) | ||
| // Strip the param from the URL to prevent it leaking into pageview props. | ||
| url.searchParams.delete('ph_did') | ||
| urlModified = true | ||
| } |
There was a problem hiding this comment.
ph_did is left in the address bar when consent is not granted.
The cleanup (url.searchParams.delete('ph_did')) is now nested inside the consent-gated branch, so a non-consented visitor who follows a link with ?ph_did=… keeps that distinct id visible in the URL bar across the rest of their session. While the SENSITIVE_URL_PROPS sanitiser scrubs query strings from captured $current_url/$referrer, the raw location.href is still exposed to other client scripts, browser history, copy-paste, and any non-PostHog telemetry. This contradicts the AI summary's statement that aliasing now only happens with consent "while still deleting ph_did from the URL after processing".
Suggest cleaning the parameter independently of the alias call.
🛡️ Proposed fix
- const isValidDistinctId = marketingDistinctId && /^[\w-]{10,100}$/.test(marketingDistinctId)
- if (isValidDistinctId && consentCookie.value === 'granted') {
- posthog.alias(marketingDistinctId)
- url.searchParams.delete('ph_did')
- urlModified = true
- }
+ const isValidDistinctId = marketingDistinctId && /^[\w-]{10,100}$/.test(marketingDistinctId)
+ if (isValidDistinctId && consentCookie.value === 'granted') {
+ posthog.alias(marketingDistinctId)
+ }
+ // Always strip ph_did from the visible URL — it's a tracking identifier
+ // that should not linger in the address bar, history, or referrer chain
+ // regardless of whether we were able to alias it.
+ if (marketingDistinctId !== null) {
+ url.searchParams.delete('ph_did')
+ urlModified = true
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const marketingDistinctId = url.searchParams.get('ph_did') | |
| // Validate format: PostHog distinct IDs are typically UUIDs or short | |
| // alphanumeric strings. Reject anything outside that to prevent | |
| // passing arbitrary untrusted input to posthog.alias(). | |
| const isValidDistinctId = marketingDistinctId && /^[\w-]{10,100}$/.test(marketingDistinctId) | |
| if (isValidDistinctId && storedConsent === 'granted') { | |
| // Create an alias so both IDs resolve to the same person in PostHog. | |
| if (isValidDistinctId && consentCookie.value === 'granted') { | |
| posthog.alias(marketingDistinctId) | |
| // Strip the param from the URL to prevent it leaking into pageview props. | |
| url.searchParams.delete('ph_did') | |
| urlModified = true | |
| } | |
| const marketingDistinctId = url.searchParams.get('ph_did') | |
| // Validate format: PostHog distinct IDs are typically UUIDs or short | |
| // alphanumeric strings. Reject anything outside that to prevent | |
| // passing arbitrary untrusted input to posthog.alias(). | |
| const isValidDistinctId = marketingDistinctId && /^[\w-]{10,100}$/.test(marketingDistinctId) | |
| if (isValidDistinctId && consentCookie.value === 'granted') { | |
| posthog.alias(marketingDistinctId) | |
| } | |
| // Always strip ph_did from the visible URL — it's a tracking identifier | |
| // that should not linger in the address bar, history, or referrer chain | |
| // regardless of whether we were able to alias it. | |
| if (marketingDistinctId !== null) { | |
| url.searchParams.delete('ph_did') | |
| urlModified = true | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/plugins/posthog-identity.client.ts` around lines 89 - 98, The ph_did
query param must be removed from the URL regardless of consent: keep the
existing validation (marketingDistinctId and isValidDistinctId) and the consent
check that calls posthog.alias(marketingDistinctId) when consentCookie.value ===
'granted', but move the url.searchParams.delete('ph_did') and urlModified = true
out of the consent-gated branch so they always run when isValidDistinctId is
true (after any alias call); reference marketingDistinctId, isValidDistinctId,
consentCookie.value, posthog.alias, url.searchParams.delete and urlModified when
making the change.
Comment on lines
+161
to
+183
| /** | ||
| * Tag the current PostHog session as a demo session. | ||
| * | ||
| * Registers `is_demo` as a super property so EVERY subsequent | ||
| * event carries it — funnels and dashboards can then filter | ||
| * `is_demo != true` to exclude the public demo from real-user | ||
| * metrics. Also forwards the flag as a person property (when | ||
| * consented) so the demo person profile is permanently tagged. | ||
| * | ||
| * Pass `false` to clear the flag (e.g. when a real user signs | ||
| * in after a demo session in the same tab). | ||
| */ | ||
| posthogSetDemoFlag: (isDemo: boolean) => { | ||
| if (isDemo) { | ||
| posthog.register({ is_demo: true }) | ||
| // Person-level tag — survives across sessions for this user. | ||
| posthog.setPersonProperties({ is_demo: true }) | ||
| } | ||
| else { | ||
| posthog.unregister('is_demo') | ||
| posthog.setPersonProperties({ is_demo: false }) | ||
| } | ||
| }, |
There was a problem hiding this comment.
Doc comment claims consent gating that the implementation does not perform.
The JSDoc states the person-property tag is forwarded "(when consented)", but the body unconditionally calls posthog.setPersonProperties({ is_demo: … }) in both branches. Looking at the call sites in app/composables/usePostHogIdentity.ts (lines 63 and 96 of the relevant snippets), setDemoFlag is invoked outside the consented conditional, so without an internal guard the person property is always written.
This is functionally fine (is_demo isn't PII and the design captures events anonymously without consent), but the comment is misleading. Either drop the "(when consented)" qualifier or actually gate the setPersonProperties call on a consent check passed in by the caller.
📝 Proposed doc fix (matches current behaviour)
- * events carries it — funnels and dashboards can then filter
- * `is_demo != true` to exclude the public demo from real-user
- * metrics. Also forwards the flag as a person property (when
- * consented) so the demo person profile is permanently tagged.
+ * events carries it — funnels and dashboards can then filter
+ * `is_demo != true` to exclude the public demo from real-user
+ * metrics. Also forwards the flag as a person property so the
+ * demo person profile is permanently tagged (the flag itself
+ * is not PII, so this runs regardless of analytics consent).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** | |
| * Tag the current PostHog session as a demo session. | |
| * | |
| * Registers `is_demo` as a super property so EVERY subsequent | |
| * event carries it — funnels and dashboards can then filter | |
| * `is_demo != true` to exclude the public demo from real-user | |
| * metrics. Also forwards the flag as a person property (when | |
| * consented) so the demo person profile is permanently tagged. | |
| * | |
| * Pass `false` to clear the flag (e.g. when a real user signs | |
| * in after a demo session in the same tab). | |
| */ | |
| posthogSetDemoFlag: (isDemo: boolean) => { | |
| if (isDemo) { | |
| posthog.register({ is_demo: true }) | |
| // Person-level tag — survives across sessions for this user. | |
| posthog.setPersonProperties({ is_demo: true }) | |
| } | |
| else { | |
| posthog.unregister('is_demo') | |
| posthog.setPersonProperties({ is_demo: false }) | |
| } | |
| }, | |
| /** | |
| * Tag the current PostHog session as a demo session. | |
| * | |
| * Registers `is_demo` as a super property so EVERY subsequent | |
| * event carries it — funnels and dashboards can then filter | |
| * `is_demo != true` to exclude the public demo from real-user | |
| * metrics. Also forwards the flag as a person property so the | |
| * demo person profile is permanently tagged (the flag itself | |
| * is not PII, so this runs regardless of analytics consent). | |
| * | |
| * Pass `false` to clear the flag (e.g. when a real user signs | |
| * in after a demo session in the same tab). | |
| */ | |
| posthogSetDemoFlag: (isDemo: boolean) => { | |
| if (isDemo) { | |
| posthog.register({ is_demo: true }) | |
| // Person-level tag — survives across sessions for this user. | |
| posthog.setPersonProperties({ is_demo: true }) | |
| } | |
| else { | |
| posthog.unregister('is_demo') | |
| posthog.setPersonProperties({ is_demo: false }) | |
| } | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/plugins/posthog-identity.client.ts` around lines 161 - 183, The JSDoc for
posthogSetDemoFlag claims person properties are forwarded "(when consented)" but
the implementation always calls posthog.setPersonProperties; either remove that
qualifier from the comment to match current behavior, or change
posthogSetDemoFlag signature to accept a consent boolean (e.g.,
posthogSetDemoFlag(isDemo: boolean, consented: boolean)) and wrap calls to
posthog.setPersonProperties({ is_demo: … }) in an if(consented) guard; if you
add the parameter, update all call sites in usePostHogIdentity (the setDemoFlag
invocations) to pass the consented value.
Comment on lines
+17
to
+18
| const demoOrgIdBySlug = new Map<string, string>() | ||
| const demoOrgIdSet = new Set<string>() |
There was a problem hiding this comment.
Dead code: demoOrgIdSet is written but never read.
The Set at line 18 is populated on every cache miss (line 70) but no caller reads it. Two reasonable resolutions:
- Delete it if not needed.
- Wire it into
isDemoOrgIdfor an O(1) hot-path check that avoids re-iterating configured slugs on everytrackEventcall once the cache is warm.
♻️ Option 2: short-circuit `isDemoOrgId` via the id Set
export async function isDemoOrgId(orgId: string | null | undefined): Promise<boolean> {
if (!orgId) return false
+ // Hot path: once the cache is warmed, answer without iterating slugs.
+ if (demoOrgIdSet.has(orgId)) return true
const ids = await getDemoOrgIds()
return ids.has(orgId)
}Note this only short-circuits positive matches; negatives still pay the lookup cost (correct, since new demo orgs may be created at runtime).
Also applies to: 69-71, 84-88
Comment on lines
+67
to
+73
| const match = consent.match(/function declineAnalytics\(\)\s*\{([\s\S]*?)\n\s*\}/) | ||
| expect(match, 'declineAnalytics body found').toBeTruthy() | ||
| const body = match![1]! | ||
| expect(body).not.toMatch(/set_config/) | ||
| expect(body).not.toMatch(/opt_out/) | ||
| expect(body).not.toMatch(/identify/) | ||
| }) |
There was a problem hiding this comment.
Brittle regex: declineAnalytics body extraction will break on nested braces.
/function declineAnalytics\(\)\s*\{([\s\S]*?)\n\s*\}/ non-greedily stops at the first \n\s*\}, which today happens to be the function's closing brace, but will silently truncate the captured body if anyone adds an if/for block (or even an object literal) inside declineAnalytics. The subsequent not.toMatch(/set_config/) would then pass against a partial body and miss real regressions.
If you keep this style at all, anchor on the next top-level token instead, e.g. extract until the next \n\}\n or use a balanced-brace parser. Better yet, fold this into the behavior-level rewrite suggested above.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/posthog-cookieless.test.ts` around lines 67 - 73, The test's regex
that extracts the declineAnalytics body is brittle and will break on nested
braces; update the test for declineAnalytics to use a robust extraction (either
parse balanced braces with a small brace-counting routine or anchor to the
top-level closing token such as matching until "\n}\n") instead of the current
/function declineAnalytics\(\)\s*\{([\s\S]*?)\n\s*\}/ pattern so the full
function body is captured reliably and the subsequent expect(...).not.toMatch
checks run against the true body.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Type of change
Validation
DCO
Signed-off-by) viagit commit -sSummary by CodeRabbit
Release Notes
New Features
Privacy & Security
Bug Fixes