feat(browser): add __preview_cookie_wins_on_conflict for multi-subdomain identify#3719
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Contributor
|
Size Change: +2.4 kB (+0.01%) Total Size: 16.5 MB
ℹ️ View Unchanged
|
Contributor
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
packages/browser/src/__tests__/utils.test.ts:484-519
**Prefer parameterised tests for `extend` value semantics**
The first six tests in this block all have the same shape (`extend(base, override)` → expected result) and are better expressed as a single `it.each` table. The current form repeats setup/assertion boilerplate for each value variant, violating the "OnceAndOnlyOnce" simplicity rule and the team's explicit preference for parameterised tests.
The mutation/identity test (`result === target`) is legitimately separate since it asserts reference equality rather than value equality.
### Issue 2 of 2
packages/browser/src/__tests__/posthog-persistence.test.ts:575-603
**Prefer parameterised tests for the two defensive-filter cases**
The null-cookie and empty-string-cookie tests are structurally identical — same setup, same assertion, only the bad cookie value differs. They can be collapsed into a single `it.each([['null', null], ['empty string', '']])` test, which is more concise and consistent with the team's preference for parameterised tests.
Reviews (1): Last reviewed commit: "docs: trim cookie_wins_on_conflict chang..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+484
to
+519
|
|
||
| describe('extend', () => { | ||
| // Pins the merge semantics that the localStorage+cookie persistence merge depends on. | ||
| // Later args override earlier args for any defined value (undefined is skipped, but | ||
| // null, '', 0, false ARE applied). Code paths that need stricter behaviour (e.g. | ||
| // createLocalPlusCookieStore with __preview_cookie_wins_on_conflict) must filter | ||
| // before calling extend rather than relying on it to skip falsy values. | ||
|
|
||
| it('later args override earlier args for defined values', () => { | ||
| expect(extend({ a: 1 }, { a: 2 })).toEqual({ a: 2 }) | ||
| }) | ||
|
|
||
| it('skips undefined source values', () => { | ||
| expect(extend({ a: 1 }, { a: undefined })).toEqual({ a: 1 }) | ||
| }) | ||
|
|
||
| it('applies null source values (does not skip)', () => { | ||
| expect(extend({ a: 1 }, { a: null })).toEqual({ a: null }) | ||
| }) | ||
|
|
||
| it('applies empty-string source values (does not skip)', () => { | ||
| expect(extend({ a: 'valid' }, { a: '' })).toEqual({ a: '' }) | ||
| }) | ||
|
|
||
| it('applies 0 and false (does not skip falsy)', () => { | ||
| expect(extend({ a: 1, b: true }, { a: 0, b: false })).toEqual({ a: 0, b: false }) | ||
| }) | ||
|
|
||
| it('preserves keys that are absent from later args', () => { | ||
| expect(extend({ a: 1, b: 2 }, { b: 3 })).toEqual({ a: 1, b: 3 }) | ||
| }) | ||
|
|
||
| it('mutates the first argument and returns it', () => { | ||
| const target: Record<string, any> = { a: 1 } | ||
| const result = extend(target, { b: 2 }) | ||
| expect(result).toBe(target) |
Contributor
There was a problem hiding this comment.
extend value semantics
The first six tests in this block all have the same shape (extend(base, override) → expected result) and are better expressed as a single it.each table. The current form repeats setup/assertion boilerplate for each value variant, violating the "OnceAndOnlyOnce" simplicity rule and the team's explicit preference for parameterised tests.
The mutation/identity test (result === target) is legitimately separate since it asserts reference equality rather than value equality.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browser/src/__tests__/utils.test.ts
Line: 484-519
Comment:
**Prefer parameterised tests for `extend` value semantics**
The first six tests in this block all have the same shape (`extend(base, override)` → expected result) and are better expressed as a single `it.each` table. The current form repeats setup/assertion boilerplate for each value variant, violating the "OnceAndOnlyOnce" simplicity rule and the team's explicit preference for parameterised tests.
The mutation/identity test (`result === target`) is legitimately separate since it asserts reference equality rather than value equality.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
+575
to
+603
| JSON.stringify({ | ||
| distinct_id: 'from_localstorage', | ||
| // keys NOT in COOKIE_PERSISTED_PROPERTIES — they live only in localStorage | ||
| $surveys: ['s1', 's2'], | ||
| super_prop: 'value', | ||
| }) | ||
| ) | ||
|
|
||
| const lib = new PostHogPersistence(makeConfig('localStorage+cookie', true)) | ||
|
|
||
| expect(lib.props.distinct_id).toBe('from_cookie') | ||
| expect(lib.props.$surveys).toEqual(['s1', 's2']) | ||
| expect(lib.props.super_prop).toBe('value') | ||
| }) | ||
|
|
||
| it('flag on: empty cookie is a no-op, localStorage round-trips intact', () => { | ||
| expect(document.cookie).toEqual('') | ||
| localStorage.setItem(persistenceName, JSON.stringify({ distinct_id: 'ls_only', super_prop: 'value' })) | ||
|
|
||
| const lib = new PostHogPersistence(makeConfig('localStorage+cookie', true)) | ||
|
|
||
| expect(lib.props.distinct_id).toBe('ls_only') | ||
| expect(lib.props.super_prop).toBe('value') | ||
| }) | ||
|
|
||
| it('flag on: empty localStorage, cookie-only data populates props', () => { | ||
| document.cookie = encodeCookie({ distinct_id: 'cookie_only', $device_id: 'd1' }) | ||
| expect(localStorage.getItem(persistenceName)).toBe(null) | ||
|
|
Contributor
There was a problem hiding this comment.
The null-cookie and empty-string-cookie tests are structurally identical — same setup, same assertion, only the bad cookie value differs. They can be collapsed into a single it.each([['null', null], ['empty string', '']]) test, which is more concise and consistent with the team's preference for parameterised tests.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browser/src/__tests__/posthog-persistence.test.ts
Line: 575-603
Comment:
**Prefer parameterised tests for the two defensive-filter cases**
The null-cookie and empty-string-cookie tests are structurally identical — same setup, same assertion, only the bad cookie value differs. They can be collapsed into a single `it.each([['null', null], ['empty string', '']])` test, which is more concise and consistent with the team's preference for parameterised tests.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
dustinbyrne
approved these changes
Jun 5, 2026
dustinbyrne
approved these changes
Jun 5, 2026
dustinbyrne
approved these changes
Jun 5, 2026
Contributor
dustinbyrne
left a comment
dustinbyrne
left a comment
There was a problem hiding this comment.
third time's the charm
| defaults: defaults ?? 'unset', | ||
| __preview_deferred_init_extensions: false, // Opt-in only for now | ||
| __preview_external_dependency_versioned_paths: false, | ||
| __preview_cookie_wins_on_conflict: false, // Opt-in: fixes cross-subdomain stale-localStorage bug |
Contributor
There was a problem hiding this comment.
fwiw, I think it's likely safe to enable this by default if the persistence mode is localStorage+cookie and cross_subdomain_cookie is true
It's possible some users rely on the broken behavior, but I can't think of a legitimate use case that's not improved by this
turnipdabeets
approved these changes
Jun 5, 2026
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
3 participants
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
Adds an opt-in
__preview_cookie_wins_on_conflictconfig that fixes a latent multi-subdomain identify / session-disconnect bug in'localStorage+cookie'persistence.Bug: in the default
'localStorage+cookie'mode, identity keys (distinct_id,$device_id,$session_id,$user_state,$initial_person_info, etc.) are stored both in the cross-subdomain cookie and in the per-subdomain localStorage. On_parsethe two are merged withextend(cookieProperties, localStorageData)(storage.ts) andextend()lets later args win, so localStorage wins on conflict. For sites running posthog on more than one subdomain, a stale per-subdomain localStorage can clobber a fresh cookie written by anidentify()on another subdomain — surfacing as identified users looking anonymous after a subdomain transition, plus spurious new$session_idmints when the merged stale activity timestamp trips the idle-timeout check.Fix: opt-in flag that flips the merge order. When
true, cookie values win for the keys it holds; null/empty cookie values are filtered before the merge so a malformed legacy cookie cannot wipe out valid localStorage data. The existing self-heal write at the end of_parsemeans stale localStorage gets overwritten with the merged value on the first load with the flag enabled. localStorage-only keys (flag caches, surveys, super properties, registered URL params) are unaffected.Why not just flip the default: localStorage-wins has shipped for years; some users may rely on it. Opt-in unblocks the multi-subdomain use case immediately with zero blast radius. The changeset telegraphs this may become the default in a future major.
Why not
persistence: 'cookie': that mode dumps the entire props object (including$surveys,$feature_flag_details, payloads, super properties) into a single 4KB-capped cookie, which is too small for many real configs.Scope
packages/types/src/posthog-config.ts— new__preview_cookie_wins_on_conflict: booleanwith full JSDoc, defaultfalse, init-time only, no effect for other persistence modespackages/browser/src/posthog-core.ts— defaultfalsepackages/browser/src/storage.ts— second parampreferCookieOnConflictoncreateLocalPlusCookieStore; conditional merge order; defensive filter for null/empty cookie valuespackages/browser/src/posthog-persistence.ts— plumbs the config throughpackages/types/src/__tests__/__snapshots__/config-snapshot.spec.ts.snap— auto-regeneratedTests
posthog-persistence.test.tsunder a newmerge precedencedescribe block:utils.test.tspinningextend()semantics so a future refactor cannot silently change them (later wins for defined values, skipsundefined, appliesnull/''/0/false, mutates first arg)Test plan
pnpm test:unit(browser) — 4073/4073 pass, 102/102 suitespnpm test:unit(types) — 2/2 pass, snapshot cleanpnpm lint— clean (both packages)pnpm typecheck— clean (production; pre-existingtest:typecheckfailure on missing dompurify/dotenv type defs is unrelated to this change)Notes for reviewers
_buildStorageruns; toggling viaset_configafter init has no effect. Documented in JSDoc.'cookie','localStorage','memory','sessionStorage'persistence modes.createLocalPlusCookieStoreis not part of the public export surface — the signature change is internal.