feat(core): A/B test variant branch via proxy rewrite#3378
Conversation
Read `__variant` from the querystring in the Next.js proxy (proxy.ts) and rewrite to internal `/_variant/[branchId]/...` routes so getStaticProps receives the branchId via params and passes it to ContentService. ISR is preserved — each branchId is a distinct cache entry — and original pages stay untouched. - proxy.ts: compose resolveVariantRewrite before localization rewrites - utils/variant.ts: pure resolveVariantRewrite(url) + constants - server/content: branchId on ContentRequestContext, getVariantBranchId, map branchId to cmsOptions.versionId without enabling preview mode - pages/_variant/[branchId]/*: thin variant-aware home/PDP/PLP wrappers - components/ExperimentDataLayer: push experiment context to dataLayer Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
renatomaurovtex
left a comment
There was a problem hiding this comment.
Reviewed the diff against the @faststore/core boundaries and the ISR/proxy design. The ISR-preserving approach (path-encoded variant + fallback: 'blocking', untouched original pages) is sound, tests are well-placed, and the tooling churn was correctly kept out of the branch. Two things I'd resolve before merge — one security/authorization, one maintainability — plus a couple of nits.
🟠 [security] src/utils/variant.ts — __variant is fully attacker-controlled and unvalidated
resolveVariantRewrite reflects the raw querystring value straight into the rewrite path, the ISR cache key, and (downstream) the CP versionId. Since __variant is a public, user-supplied param, this opens three concerns:
- Unpublished/branch content disclosure.
branchId→cmsOptions.versionIdresolves CP content from an arbitrary branch without preview mode (by design, to keep ISR). But "no preview mode" also means no auth gate — any visitor who knows/guesses abranchIdcan render draft/version content that previously required authenticated preview. Is that an accepted trade-off for the PoC, or should the branch id be validated against an allowlist (e.g. the active experiment's variants) before it's honored? - Cache-fill / SSR amplification. Each distinct
__variantvalue mints a new/_variant/<x>/...ISR entry withfallback: 'blocking', so unbounded values let an attacker force unbounded on-demand renders. An allowlist/charset bound caps this. - Path normalization. Assigning to
URL.pathnamenormalizes..segments, so a crafted value can escape the/_variant/prefix.
Minimum fix — reject anything that isn't a plain branch token:
+const VALID_VARIANT = /^[a-zA-Z0-9_-]+$/
+
export function resolveVariantRewrite(url: URL): URL | null {
const variant = url.searchParams.get(VARIANT_QUERY_PARAM)
- if (!variant) {
+ if (!variant || !VALID_VARIANT.test(variant)) {
return null
}Flagging for human security review since it touches the authorization boundary for non-production content.
🟡 [code quality] _variant/[branchId]/[slug]/p.tsx & _variant/[branchId]/[...slug].tsx — duplicated GraphQL operations
ServerProductQuery and ServerCollectionPageQuery are copy-pasted byte-identical from the original pages, and the variant pages import the originals' generated types. Today the client-preset dedupes by document string so pnpm codegen is fine — but the moment either copy drifts, codegen breaks with Not all operations have an unique name (or the reused generated types silently go stale). Extract the query into a shared module and import it from both the original and variant page instead of maintaining two copies.
💬 [performance] 3 new _variant page entrypoints
The wrappers re-export the original components, but each is still a distinct route/bundle. Acceptable for a PoC; just confirm pnpm size shows no regression before merge (perf is NON-NEGOTIABLE here).
💬 [nit] src/server/content/types.ts — stale "middleware" wording
branchId doc says "Sourced from the __variant querystring via middleware." The entrypoint is now proxy.ts (Next 16). Quick wording fix to match the rest of the PR.
💬 [nit] ExperimentDataLayer/index.tsx — redundant typeof window guard
The check inside useEffect is dead code — effects only run client-side. Harmless, drop it if you want.
Verdict: Changes requested
Blocking (🔴/🟠):
- 🟠 Validate/allowlist
__variantinresolveVariantRewrite— unauthenticated branch-content access, cache-fill, and path normalization. Needs human security sign-off on the authorization trade-off.
Non-blocking (🟡/💬):
- 🟡 De-duplicate the copy-pasted
ServerProductQuery/ServerCollectionPageQueryoperations (latent codegen break on drift). - 💬 Confirm
pnpm sizefor the new variant routes. - 💬 Stale "middleware" wording in
types.ts; redundanttypeof windowguard inExperimentDataLayer.
Checks to confirm before merge: pnpm lint · build · pnpm codegen · pnpm turbo run test --filter=@faststore/core · pnpm size
@faststore/api
@faststore/cli
@faststore/components
@faststore/core
@faststore/diagnostics
@faststore/lighthouse
@faststore/sdk
@faststore/ui
commit: |

What
Adds A/B test support driven by a
__variantquerystring param. The Next.js proxy (proxy.ts— the Next 16 convention that replacesmiddleware.ts) reads__variantand rewrites the request to an internal/_variant/[branchId]/...route, sogetStaticPropsreceives thebranchIdviaparamsand forwards it toContentServicefor all CP calls.How
proxy.ts— composesresolveVariantRewrite(request.nextUrl)at the top ofproxy(), before the existing localization rewrites; reuses the existing matcher.utils/variant.ts— pure, unit-testedresolveVariantRewrite(url)+VARIANT_QUERY_PARAM/VARIANT_PATH_PREFIXconstants.server/content—branchId?onContentRequestContext,getVariantBranchId(params), andbuildCmsOptionsmapsbranchId→cmsOptions.versionIdwithout enabling preview mode (precedence:branchId>previewData.versionId).pages/_variant/[branchId]/*— thin variant-aware wrappers (home, PDP, PLP/landing) withfallback: 'blocking'that delegate to existing page logic.components/ExperimentDataLayer— client-side: reads thevtex_exp_variantcookie and pushes{ experiment_id, variant_id }towindow.dataLayer.Why this preserves ISR
getStaticPropshas no access to querystring/headers. Encoding the variant in the path (via proxy rewrite) keeps each/_variant/[branchId]/...as a distinct ISR entry — nogetServerSideProps, no preview mode. Original pages stay untouched, so non-experiment traffic is unaffected.Testing
resolveVariantRewrite,getVariantBranchId,buildCmsOptionswith branchId.ContentServicewith the correct branchId across content types.ExperimentDataLayercookie → dataLayer behavior.Scope note
PoC scope — single experiment, hardcoded
experiment_id. Unrelated working-tree tooling churn (tsconfig.json,pnpm-workspace.yaml,next-env.d.ts, build artifacts) was intentionally left out of this branch.🤖 Generated with Claude Code