Skip to content

fix: use cookie/urlParam-based fallback by default with redirect action - #328

Open
tyiuhc wants to merge 113 commits into
mainfrom
web/subdomain-redirect-default
Open

fix: use cookie/urlParam-based fallback by default with redirect action#328
tyiuhc wants to merge 113 commits into
mainfrom
web/subdomain-redirect-default

Conversation

@tyiuhc

@tyiuhc tyiuhc commented May 26, 2026

Copy link
Copy Markdown
Collaborator

Summary

Branch script to use cookie+urlParam-based fallback for redirect actions by default.

Checklist

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

Note

Medium Risk
Changing a public default alters redirect URLs and impression recovery for anyone relying on implicit config, though teams can still set encodeRedirectInUrl: false.

Overview
Default redirect tracking now turns on encodeRedirectInUrl in Defaults.redirectConfig (was false, now true). Integrators who do not override redirectConfig get cookie + AMP_REDIRECT query-param fallback for redirect variant actions, matching the PR’s cookie/urlParam default.

Cross-subdomain redirects will embed impression data in the destination URL when encodeRedirectInUrl applies; destination pages will consume and strip that param when the flag is enabled. encodeRedirectInCookie stays true by default—only the URL leg of the fallback changes for out-of-the-box config.

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

@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: Unconditional console.error calls pollute customer browser consoles
    • Removed all three console.error calls in cookie operations (storeRedirectImpressions, fireStoredRedirectImpressions, and cleanup) and replaced them with silent failure as the cookie storage is a non-critical fallback mechanism.

Create PR

Or push these changes by commenting:

@cursor push 9ead4e2377
Preview (9ead4e2377)
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
@@ -1179,11 +1179,8 @@
       const redirects = storedRedirects || {};
       redirects[flagKey] = impression;
       await storage.set(storageKey, redirects);
-    } catch (error) {
-      console.error(
-        `Failed to store redirect impression in cookie for ${flagKey}:`,
-        error,
-      );
+    } catch {
+      // Fail silently - cookie storage is a fallback mechanism
     }
   }
 
@@ -1225,11 +1222,8 @@
     });
     try {
       cookieImpressions = (await cookieStorage.get(storageKey)) || {};
-    } catch (error) {
-      console.error(
-        `Failed to retrieve redirect impressions from cookie ${storageKey}:`,
-        error,
-      );
+    } catch {
+      // Fail silently - cookie storage is a fallback mechanism
     }
 
     // Merge with priority: url > session > cookie
@@ -1261,11 +1255,8 @@
 
     const cleanup = async () => {
       removeStorageItem('sessionStorage', storageKey);
-      await cookieStorage.remove(storageKey).catch((error) => {
-        console.error(
-          `Failed to remove redirect impressions from cookie ${storageKey}:`,
-          error,
-        );
+      await cookieStorage.remove(storageKey).catch(() => {
+        // Fail silently - cookie storage is a fallback mechanism
       });
     };

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

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

@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.

There are 2 total unresolved issues (including 1 from previous review).

Autofix Details

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

  • ✅ Fixed: Redirect config flags ignored
    • Added guards checking this.config.redirectConfig?.encodeRedirectInUrl !== false and this.config.redirectConfig?.encodeRedirectInCookie !== false to respect explicit opt-out while maintaining default-enabled behavior.

Create PR

Or push these changes by commenting:

@cursor push 9f8b10e1b1
Preview (9f8b10e1b1)
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
@@ -909,29 +909,33 @@
     // Embed impression data in redirect URL for cross-domain and
     // cookie-blocked environments. Merge with any existing param in case
     // multiple redirect experiments fire in sequence.
-    const targetUrlObj = new URL(targetUrl);
-    let urlPayload: Record<string, StoredRedirectImpression> = {};
-    const existingEncoded = targetUrlObj.searchParams.get(
-      REDIRECT_IMPRESSION_PARAM,
-    );
-    if (existingEncoded) {
-      try {
-        urlPayload = JSON.parse(atob(existingEncoded));
-      } catch (error) {
-        console.error('Failed to decode existing AMP_REDIRECT param:', error);
+    if (this.config.redirectConfig?.encodeRedirectInUrl !== false) {
+      const targetUrlObj = new URL(targetUrl);
+      let urlPayload: Record<string, StoredRedirectImpression> = {};
+      const existingEncoded = targetUrlObj.searchParams.get(
+        REDIRECT_IMPRESSION_PARAM,
+      );
+      if (existingEncoded) {
+        try {
+          urlPayload = JSON.parse(atob(existingEncoded));
+        } catch (error) {
+          console.error('Failed to decode existing AMP_REDIRECT param:', error);
+        }
       }
+      urlPayload[flagKey] = {
+        redirectUrl,
+        variantKey: variant.key || '',
+        ...(variant.expKey !== undefined ? { expKey: variant.expKey } : {}),
+        ...(variant.metadata !== undefined
+          ? { metadata: variant.metadata }
+          : {}),
+      };
+      targetUrlObj.searchParams.set(
+        REDIRECT_IMPRESSION_PARAM,
+        btoa(JSON.stringify(urlPayload)),
+      );
+      targetUrl = targetUrlObj.toString();
     }
-    urlPayload[flagKey] = {
-      redirectUrl,
-      variantKey: variant.key || '',
-      ...(variant.expKey !== undefined ? { expKey: variant.expKey } : {}),
-      ...(variant.metadata !== undefined ? { metadata: variant.metadata } : {}),
-    };
-    targetUrlObj.searchParams.set(
-      REDIRECT_IMPRESSION_PARAM,
-      btoa(JSON.stringify(urlPayload)),
-    );
-    targetUrl = targetUrlObj.toString();
 
     // set previous url - relevant for SPA if redirect happens before push/replaceState is complete
     this.previousUrl = this.globalScope.location.href;
@@ -1169,25 +1173,29 @@
     setStorageItem('sessionStorage', storageKey, stored);
 
     // Also write to cookie to enable cross-subdomain tracking
-    const domain = await getTopLevelDomain(this.globalScope.location.hostname);
-    const storage = new CookieStorage<Record<string, StoredRedirectImpression>>(
-      {
+    if (this.config.redirectConfig?.encodeRedirectInCookie !== false) {
+      const domain = await getTopLevelDomain(
+        this.globalScope.location.hostname,
+      );
+      const storage = new CookieStorage<
+        Record<string, StoredRedirectImpression>
+      >({
         ...(domain && { domain }),
         sameSite: 'Lax',
         expirationDays: 1 / 1440, // 1 minute
-      },
-    );
+      });
 
-    try {
-      const storedRedirects = await storage.get(storageKey);
-      const redirects = storedRedirects || {};
-      redirects[flagKey] = impression;
-      await storage.set(storageKey, redirects);
-    } catch (error) {
-      console.error(
-        `Failed to store redirect impression in cookie for ${flagKey}:`,
-        error,
-      );
+      try {
+        const storedRedirects = await storage.get(storageKey);
+        const redirects = storedRedirects || {};
+        redirects[flagKey] = impression;
+        await storage.set(storageKey, redirects);
+      } catch (error) {
+        console.error(
+          `Failed to store redirect impression in cookie for ${flagKey}:`,
+          error,
+        );
+      }
     }
   }

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

Comment thread packages/experiment-tag/src/experiment.ts Outdated
@tyiuhc
tyiuhc force-pushed the web/subdomain-redirect-default branch from 528f239 to ce31466 Compare June 4, 2026 21:46
Base automatically changed from web/subdomain-redirect-poc to main June 10, 2026 22:23

@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: Redirect stall hangs startup
    • Removed the never-resolving promise after location.replace() and replaced it with a simple return statement, allowing startup to complete normally when navigation doesn't occur immediately.

Create PR

Or push these changes by commenting:

@cursor push cc22a41767
Preview (cc22a41767)
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
@@ -999,10 +999,7 @@
       return;
     }
     this.globalScope.location.replace(targetUrl);
-    // location.replace() queues navigation but JS continues — stall so applyVariants
-    // never resolves and removeAntiFlickerCss() is never called during the unload window.
-    // eslint-disable-next-line @typescript-eslint/no-empty-function
-    await new Promise(() => {});
+    return;
   }
 
   private handleMutate(action, flagKey: string, variant: Variant) {

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

Comment thread packages/experiment-tag/src/experiment.ts Outdated
…g promise

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.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 1 potential issue.

Fix All in Cursor

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

  • ✅ Fixed: Custom redirect skips remote flags
    • Moved isRedirecting = true to only execute after the custom handler check, ensuring SPAs using custom redirect handlers will continue to fetch remote flags after client-side navigation.

Create PR

Or push these changes by commenting:

@cursor push ba7cf0cad9
Preview (ba7cf0cad9)
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
@@ -995,14 +995,18 @@
     // set previous url - relevant for SPA if redirect happens before push/replaceState is complete
     this.previousUrl = this.globalScope.location.href;
     await setMarketingCookie(this.apiKey, this.globalScope.location.hostname);
-    // Mark redirect as in-flight so start() skips removeAntiFlickerCss and
-    // further processing after applyVariants returns.
-    this.isRedirecting = true;
     // perform redirection
     if (this.customRedirectHandler) {
+      // Custom redirect handlers may perform client-side routing without a
+      // page reload, so we should NOT set isRedirecting. This allows start()
+      // to continue and fetch remote flags after the SPA navigation.
       this.customRedirectHandler(targetUrl);
       return;
     }
+    // Mark redirect as in-flight so start() skips removeAntiFlickerCss and
+    // further processing after applyVariants returns. Only set for full-page
+    // navigation since the page will unload anyway.
+    this.isRedirecting = true;
     this.globalScope.location.replace(targetUrl);
   }

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

Reviewed by Cursor Bugbot for commit 27bd6d0. Configure here.

Comment thread packages/experiment-tag/src/experiment.ts
tyiuhc and others added 3 commits June 18, 2026 14:03
start() resolves immediately after location.replace() fires, but the
browser keeps painting the current page until the destination commits.
The index.ts bootstrap unconditionally called removeAntiFlickerCss() in
.finally(), tearing down the overlay (including a customer's #amp-exp-css
snippet) during the redirect's network wait and flashing the source page.

Skip removeAntiFlickerCss() when isRedirecting is set, mirroring the guard
already added inside start(). Expose isRedirecting publicly so the
bootstrap can read it.

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

The preview/extension bootstrap path removed anti-flicker unconditionally in a
trailing .finally that also ran before start() completed (startClient was not
awaited), so a redirect experiment surfaced in preview mode flashed the source
page during navigation. Route teardown through startClient's single
isRedirecting-gated finally by returning its promise, matching the normal path.

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

github-actions Bot commented Jul 13, 2026

Copy link
Copy Markdown

size-limit report 📦

Path Size
experiment-tag-min (gzipped) 58.44 KB (+0.02% 🔺)

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.

3 participants