Skip to content

fix: swap out CookieStorage with plain sync cookie methods - #370

Open
kwliou wants to merge 1 commit into
mainfrom
web/doccookie
Open

fix: swap out CookieStorage with plain sync cookie methods#370
kwliou wants to merge 1 commit into
mainfrom
web/doccookie

Conversation

@kwliou

@kwliou kwliou commented Aug 19, 2026

Copy link
Copy Markdown
Collaborator

Summary

Partial migration with less code changes compared to #369

Starting from SDK script first statement, it took ~5-10ms with document.cookie version to finish running applyVariants(localFlagKeys).
With previous async CookieStorage version it was ~240-280ms.
This was tested with proxying so chrome devtools was closed and didn't cause extra slowdown.

Checklist

  • Does your PR title have the correct title format?
  • Does your PR have a breaking change?:

Note

Medium Risk
Changes identity, redirect, and marketing cookie read/write paths and TLD probing behavior; wire-format compatibility is tested but document.cookie lacks CookieStorage’s duplicate-name-by-domain filtering.

Overview
Replaces @amplitude/analytics-core CookieStorage with synchronous document.cookie I/O for consent-gated cross-subdomain cookies (identity, redirect impressions, marketing), while keeping analytics-core’s base64 wire format via new readCookieStorageSync / writeCookieStorageSync helpers.

createCookieStorage moves to util/cookie.ts and wraps a documentCookieStore delegate with existing ConsentAwareCookieStorage; call sites pass a plain options object with domain: getTopLevelDomain(hostname) instead of async option factories. getTopLevelDomain is unified as a synchronous API (probe via throwaway cookies, per-hostname cache) and the async CookieStorage.isDomainWritable path and startup domain warmup in start() are removed.

Session and campaign code follow the same sync domain resolver; tests mock createCookieStorage or use real document.cookie helpers instead of mocking CookieStorage.

Reviewed by Cursor Bugbot for commit f8ef5f9. Bugbot is set up for automated code reviews on this repo. Configure here.

@github-actions

github-actions Bot commented Aug 19, 2026

Copy link
Copy Markdown

size-limit report 📦

Path Size
experiment-tag-min (gzipped) 54.69 KB (-2.47% 🔽)

@cursor cursor Bot left a comment

Copy link
Copy Markdown

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.

Autofix Details

Bugbot Autofix prepared a fix for the issue found in the latest run.

  • ✅ Fixed: Cookie reads can throw on blocked cookies
    • Moved the raw cookie read inside the existing try/catch so blocked document.cookie access degrades to an absent cookie.

Create PR

Or push these changes by commenting:

@cursor push ec11cb2e5e
Preview (ec11cb2e5e)
diff --git a/packages/experiment-tag/src/util/cookie.ts b/packages/experiment-tag/src/util/cookie.ts
--- a/packages/experiment-tag/src/util/cookie.ts
+++ b/packages/experiment-tag/src/util/cookie.ts
@@ -303,9 +303,9 @@
  * Returns `undefined` when the cookie is absent or undecodable.
  */
 export function readCookieStorageSync<T>(key: string): T | undefined {
-  const raw = readRawCookie(key);
-  if (raw === undefined) return undefined;
   try {
+    const raw = readRawCookie(key);
+    if (raw === undefined) return undefined;
     const decoded = decodeCookieValue(raw);
     if (decoded === undefined) return undefined;
     return JSON.parse(decoded) as T;

You can send follow-ups to the cloud agent here.

Comment thread packages/experiment-tag/src/util/cookie.ts

@stephen-choi-amplitude stephen-choi-amplitude left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

A couple of minor comments but overall LGTM!

Comment thread packages/experiment-tag/src/util/cookie.ts
*/
export function readCookieStorageSync<T>(key: string): T | undefined {
try {
const raw = readRawCookie(key);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Something claude pointed out:

getRaw picks the cookie whose domain matches options.domain (isDomainEqual over cookieStore.getAll). document.cookie can't do that, so this takes whatever's listed first. With a host-only EXP_ and a .example.com EXP_ cookie both present we can hand back a stale identity — analytics-core has a cookies.duplicate diagnostic for it, so it happens.

Chrome-only change since everyone else already fell through to getRawSync, but worth a line in the description.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for pointing out that difference, updated the comment.

Comment thread packages/experiment-tag/src/util/cookie.ts Outdated
Comment thread packages/experiment-tag/src/util/cookie.ts

@cursor cursor Bot left a comment

Copy link
Copy Markdown

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.

Autofix Details

Bugbot Autofix prepared a fix for the issue found in the latest run.

  • ✅ Fixed: Cookie write can throw past catch
    • Wrapped cross-subdomain cookie persistence in best-effort error handling so restricted cookie access cannot abort identity resolution.

Create PR

Or push these changes by commenting:

@cursor push 1498eb40ef
Preview (1498eb40ef)
diff --git a/packages/experiment-tag/src/util/cookie.ts b/packages/experiment-tag/src/util/cookie.ts
--- a/packages/experiment-tag/src/util/cookie.ts
+++ b/packages/experiment-tag/src/util/cookie.ts
@@ -235,7 +235,11 @@
   }
 
   if (changed) {
-    await cookieStorage.set(cookieKey, JSON.stringify(resolved));
+    try {
+      await cookieStorage.set(cookieKey, JSON.stringify(resolved));
+    } catch {
+      /* best-effort persistence */
+    }
   }
   return resolved;
 }

diff --git a/packages/experiment-tag/test/util/cookie.test.ts b/packages/experiment-tag/test/util/cookie.test.ts
--- a/packages/experiment-tag/test/util/cookie.test.ts
+++ b/packages/experiment-tag/test/util/cookie.test.ts
@@ -131,6 +131,24 @@
       first_seen: 'fresh-ts',
     });
   });
+
+  it('returns the resolved value when cookie persistence throws', async () => {
+    const storage = fakeCookieStorage();
+    storage.set.mockImplementation(() => {
+      throw new Error('cookie access blocked');
+    });
+    await expect(
+      resolveCrossSubdomainObject<Identity>(
+        storage as never,
+        'KEY',
+        {},
+        { web_exp_id_v2: () => 'fresh', first_seen: () => 'fresh-ts' },
+      ),
+    ).resolves.toEqual({
+      web_exp_id_v2: 'fresh',
+      first_seen: 'fresh-ts',
+    });
+  });
 });
 
 // jsdom shares document.cookie across tests in a file; clear between tests.

You can send follow-ups to the cloud agent here.

Comment thread packages/experiment-tag/src/util/cookie.ts
tyiuhc added a commit that referenced this pull request Aug 24, 2026
Resolve conflicts by keeping sync document.cookie paths from #370
while adopting main's consent withheld domain guessing and lazy
createCookieStorage factory for post-grant domain probing.

Co-authored-by: Cursor <cursoragent@cursor.com>

@cursor cursor Bot left a comment

Copy link
Copy Markdown

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 2 potential issues.

Fix All in Cursor

Bugbot Autofix prepared fixes for both issues found in the latest run.

  • ✅ Fixed: Local Codex MCP config committed
    • Removed the tracked local Atlassian MCP configuration from the repository.
  • ✅ Fixed: Pending domain guess frozen on client
    • Removed the startup assignment that froze an unprobed domain guess while retaining lazy domain resolution at cookie access time.

Create PR

Or push these changes by commenting:

@cursor push 029db099cd
Preview (029db099cd)
diff --git a/.codex/config.toml b/.codex/config.toml
deleted file mode 100644
--- a/.codex/config.toml
+++ /dev/null
@@ -1,2 +1,0 @@
-[mcp_servers.atlassian]
-url = "https://mcp.atlassian.com/v1/mcp"
\ No newline at end of file

diff --git a/packages/experiment-tag/src/experiment.ts b/packages/experiment-tag/src/experiment.ts
--- a/packages/experiment-tag/src/experiment.ts
+++ b/packages/experiment-tag/src/experiment.ts
@@ -586,13 +586,6 @@
     this.subscriptionManager.markUrlAsPublished(this.globalScope.location.href);
     this.messageBus.publish('url_change', { updateActivePages: true });
 
-    // Resolve the cross-subdomain cookie domain so identity below
-    // and the RTBT session (behavioral-targeting plugin, also via
-    // getTopLevelDomainSync()) resolve the same domain without an async probe on
-    // the startup critical path. While consent is withheld this returns an
-    // unprobed guess; cookie storages resolve the domain lazily at write time.
-    this.rootDomain = getTopLevelDomainSync(this.globalScope.location.hostname);
-
     const experimentStorageName = `EXP_${this.apiKey.slice(0, 10)}`;
     const user =
       getStorageItem<WebExperimentUser>(

You can send follow-ups to the cloud agent here.

Reviewed by Cursor Bugbot for commit 950fe12. Configure here.

Comment thread .codex/config.toml Outdated
Comment thread packages/experiment-tag/src/experiment.ts Outdated
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.

2 participants