feat(netlify): opt-in Split Testing via nf_ab query #207
Conversation
…t Testing; docs for opt-in/opt-out and limitations
✅ Deploy Preview for fireproof-ai-builder ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
- Cookie parsing uses a rigid regex that can miss the cookie if
document.cookielacks spaces after semicolons; make whitespace optional. - The script forces a reload even when nothing changes and leaves query params in the URL when values already match; prefer always cleaning the URL and only reloading when the cookie actually changes.
- Inline
dangerouslySetInnerHTMLmay violate strict CSP; consider externalizing the script or adding a nonce. - Optional: support a cookie
Domainattribute to improve cross-subdomain behavior and document this host-scoping caveat.
Additional notes (2)
-
Compatibility |
app/root.tsx:116-130
If your site is accessible via both apex and subdomains (e.g.,vibes.diyandwww.vibes.diy), the cookie as set (noDomainattribute) will be host-scoped and not shared across hosts. Consider supporting an optionalDomainattribute so teams can opt into apex-wide scoping when needed. -
Readability |
docs/split-testing.md:35-40
Consider documenting that thenf_abcookie is host-scoped by default and won’t apply across apex/subdomains unless you configure aDomainattribute. This can save debugging time if folks switch betweenwwwand apex URLs.
Summary of changes
- Added a new "Developer previews on the main domain (no redirects)" section to README linking to Split Testing docs.
- Inserted an inline, pre-mount script in
app/root.tsx<head>that reads?nf_ab/?abquery params to set/clear thenf_abcookie and reload once, then removes those params from the URL. - Added
docs/split-testing.mddescribing how to opt-in/out, limitations, and configuration notes for Netlify Split Testing.
app/root.tsx
Outdated
| var currentMatch = document.cookie.match(/(?:^|; )nf_ab=([^;]+)/); | ||
| var current = currentMatch ? decodeURIComponent(currentMatch[1]) : null; | ||
There was a problem hiding this comment.
The cookie parsing regex assumes a space after semicolons in document.cookie. Some environments omit that space, which can cause the current cookie to be missed and lead to unnecessary reloads or incorrect comparisons. Make the whitespace optional to be robust across browsers.
Suggestion
Update the regex to allow optional whitespace after semicolons:
- Current:
/(:?^|; )nf_ab=([^;]+)/ - Suggested:
/(:?^|;\s*)nf_ab=([^;]+)/
Example change:
- var currentMatch = document.cookie.match(/(?:^|; )nf_ab=([^;]+)/);
+ var currentMatch = document.cookie.match(/(?:^|;\s*)nf_ab=([^;]+)/);Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion
app/root.tsx
Outdated
| if (changed) { | ||
| // Remove our params to avoid reload loops and keep nice URLs | ||
| sp.delete('nf_ab'); | ||
| sp.delete('ab'); | ||
| var newUrl = u.origin + u.pathname + (sp.toString() ? ('?' + sp.toString()) : '') + u.hash; | ||
| window.location.replace(newUrl); | ||
| } |
There was a problem hiding this comment.
When clearing with ?nf_ab=clear but no cookie is set, the script still forces a full reload. Also, when the desired branch already matches the current cookie, the URL params are left in place unless a change occurred. You can avoid an unnecessary reload and still clean the URL by always removing the params and only reloading when the cookie actually changes.
Suggestion
Refactor to always remove the params and only reload when the cookie value changes (set-to-new or cleared-when-present):
- if (changed) {
- // Remove our params to avoid reload loops and keep nice URLs
- sp.delete('nf_ab');
- sp.delete('ab');
- var newUrl = u.origin + u.pathname + (sp.toString() ? ('?' + sp.toString()) : '') + u.hash;
- window.location.replace(newUrl);
- }
+ // Decide whether we need a reload
+ var shouldReload = false;
+ if (clear) {
+ if (current) shouldReload = true; // only reload if something actually changed
+ } else if (value && value !== current) {
+ shouldReload = true;
+ }
+
+ // Remove our params regardless to keep URLs clean
+ sp.delete('nf_ab');
+ sp.delete('ab');
+ var newUrl = u.origin + u.pathname + (sp.toString() ? ('?' + sp.toString()) : '') + u.hash;
+ if (shouldReload) {
+ window.location.replace(newUrl);
+ } else if (hasNf || hasAb) {
+ // Clean the URL without a reload if nothing changed
+ history.replaceState(null, '', newUrl);
+ }Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion
app/root.tsx
Outdated
| <script | ||
| // Intentionally inline and first in <head> to run before framework mount | ||
| dangerouslySetInnerHTML={{ | ||
| __html: `(() => { | ||
| try { | ||
| var u = new URL(window.location.href); | ||
| var sp = u.searchParams; | ||
| var hasNf = sp.has('nf_ab'); | ||
| var hasAb = sp.has('ab'); | ||
| if (!hasNf && !hasAb) return; | ||
| var value = hasNf ? sp.get('nf_ab') : sp.get('ab'); | ||
| var clear = value && /^(clear|off|reset|none)$/i.test(value); | ||
| var secure = window.location.protocol === 'https:' ? '; Secure' : ''; | ||
| var cookieBase = 'nf_ab='; | ||
| var currentMatch = document.cookie.match(/(?:^|; )nf_ab=([^;]+)/); | ||
| var current = currentMatch ? decodeURIComponent(currentMatch[1]) : null; | ||
| var changed = false; | ||
| if (clear) { | ||
| document.cookie = 'nf_ab=; Max-Age=0; Path=/; SameSite=Lax' + secure; | ||
| changed = true; | ||
| } else if (value && value !== current) { | ||
| // Long-ish expiration so the choice sticks for developers | ||
| var expires = new Date(Date.now() + 365*24*60*60*1000).toUTCString(); | ||
| document.cookie = cookieBase + encodeURIComponent(value) + '; Expires=' + expires + '; Path=/; SameSite=Lax' + secure; | ||
| changed = true; | ||
| } | ||
| if (changed) { | ||
| // Remove our params to avoid reload loops and keep nice URLs | ||
| sp.delete('nf_ab'); | ||
| sp.delete('ab'); | ||
| var newUrl = u.origin + u.pathname + (sp.toString() ? ('?' + sp.toString()) : '') + u.hash; | ||
| window.location.replace(newUrl); | ||
| } | ||
| } catch (e) { | ||
| // Swallow errors to avoid blocking page load in edge cases | ||
| } | ||
| })();`, | ||
| }} | ||
| /> |
There was a problem hiding this comment.
This uses an inline script via dangerouslySetInnerHTML. If a Content Security Policy is enforced without 'unsafe-inline', this will be blocked or force CSP weakening. Consider externalizing this script (served from the same origin) so CSP can remain strict, or threading a nonce through and adding it to the script tag.
Suggestion
Option A (preferred for CSP): move the IIFE into a small static file and reference it:
- Add
public/nf-ab.cookie.jswith the current IIFE contents. - Replace the inline tag with:
- <script
- // Intentionally inline and first in <head> to run before framework mount
- dangerouslySetInnerHTML={{ __html: `(/* IIFE */)` }}
- />
+ <script src="/nf-ab.cookie.js"></script>Option B: add a CSP nonce to the inline script tag and thread it from the server:
- <script dangerouslySetInnerHTML={{ __html: `(/* IIFE */)` }} />
+ <script nonce={cspNonce} dangerouslySetInnerHTML={{ __html: `(/* IIFE */)` }} />Reply with "@CharlieHelps yes please" if you'd like me to add a commit with Option A (extracting to public/nf-ab.cookie.js and wiring the <script src>).
|
@CharlieHelps apply your suggestions |
Expand this to see my work.
|
…t cookie parsing; refined reload + URL cleanup; document host-scoped cookie
|
Applied. I took your review suggestions and pushed the updates to this PR. Changes
Open question
Verification
Commits
Assigning and re-requesting review from @jchris. |
|
@CharlieHelps only document the |
Expand this to see my work.
|
|
Docs updated to document only the
Quick usage Shout if you want any additional wording tweaks. |
Configure client-side opt-in for Netlify Split Testing so devs can preview a branch deploy on the main domain (no redirects) while preserving local storage. Adds pre-mount cookie setter and docs.
Context
We need proxy-level routing between branch deploys without changing origin so browser storage persists. Netlify Split Testing uses the
nf_abcookie to bucket visitors and serves all content for that visitor from the selected branch deploy on the primary domain.Changes
app/root.tsxthat runs in<head>before the app mounts:?nf_ab=<branch>(or?ab=<branch>) to set thenf_abcookie and reload once.?nf_ab=clear(aliases:off,reset,none) and reloads once.docs/split-testing.mdwith usage, opt-out, and limitations.Notes / limitations
nf_ab./vibe/*and/firehose; confirm this trade-off is acceptable when Split Testing is active.Open questions (need confirmation to finalize)
mainand which experimental branch?)nf_abandab), or do you want a different alias?Verification
pnpm check # result: format ok; types ok; tests: 362 passed, 4 skipped