feat: layout v2 ssr - #6455
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
capJavert
left a comment
There was a problem hiding this comment.
Review: layout v2 SSR selection
On the ISR-cache-poisoning concern (raised internally): the design holds up. Middleware runs before Vercel's edge/ISR cache lookup, and the rewrite targets a distinct path, so /posts/x and /layout-v2/posts/x are separate ISR entries. The layout of each entry is a per-route constant (layoutProps.layoutVariant), not derived from whoever triggered generation — the cache cannot bake the "wrong" shell into a path. Verified against production: Cloudflare passes HTML through (cf-cache-status: DYNAMIC) and Vercel serves the ISR entry (x-vercel-cache: HIT) after middleware.
Two residual caveats worth recording in the PR description or a comment near the middleware:
- Correctness depends on no CDN in front of Vercel ever caching HTML by URL — Cloudflare currently doesn't (
DYNAMIC), but a future CF cache rule for HTML would cache pre-middleware, keyed only by URL with no cookie segmentation, and serve one variant to everyone. There is noVaryprotection here (and CF ignoresVaryfor HTML anyway). - A first-ever visit has no
da2cookie yet, so the first pageview is always the v1 shell even for a would-be v2 user; the shell can swap after boot. Probably acceptable, but it means the SSR guarantee only holds from the second request on.
Bots/crawlers fail closed to v1 (no cookie), so SEO output stays consistent — good.
Inline comments cover the remaining findings: reserved-slug drift, server/client allocation consistency, the viewport gate that forcing bypasses, and middleware latency.
Reviewed by AI.
| 'volunteering', | ||
| 'work', | ||
| ]); | ||
| const RESERVED_PROFILE_SLUGS = new Set([ |
There was a problem hiding this comment.
Blocking (drift risk): this denylist is a hand-maintained copy of the top-level pages/ directory, and the middleware matcher now captures every single-segment path (/:userId). Today the list is a superset of the pages dir (checked), but the failure mode for drift is bad: the next top-level route someone adds (say /leaderboard) is not in this set, so for every enrolled desktop user it's treated as a profile and rewritten to /layout-v2/leaderboard, which has no page — a 404 shipped only to the experiment cohort, invisible in control and to logged-out testing.
Can we add a jest test that reads the top-level entries of packages/webapp/pages/ and asserts each one is present in RESERVED_PROFILE_SLUGS (and similarly that PROFILE_SECTIONS matches pages/[userId]/*)? That turns silent drift into a CI failure. Reviewed by AI.
| } | ||
|
|
||
| return getServerFeatureValue({ | ||
| attributes: { |
There was a problem hiding this comment.
Question (allocation consistency): the client never evaluates against the GrowthBook CDN — GrowthBookProvider gets feature definitions from the boot payload (experimentation.features) and sets userId: user.id, deviceId = an IndexedDB UUID, plus API-injected experimentation.a attributes. This middleware evaluates raw CDN definitions with userId/deviceId both set to the da2 cookie.
So consistency between the server-selected shell and the client's useLayoutVariant evaluation holds only if layout_v2 (a) hashes on userId, (b) da2 equals the boot user id for both logged-in and anonymous users, and (c) uses no targeting attributes that exist only in the boot pipeline (registrationDate, firstVisit, experimentation.a, sticky buckets). If it hashes on deviceId, server and client diverge systematically: the v1-forced pages then pin client-enrolled-v2 users to v1 while the client still logs a v2 enrollment — contaminated experiment data both ways.
Can we assert/document that constraint (e.g. a comment here naming the required hash attribute, or ideally evaluating through the same daily-api boot path instead of the CDN)? Reviewed by AI.
| @@ -221,7 +232,11 @@ function MainLayoutComponent({ | |||
| // floating-card treatment, and the global feedback widget is suppressed | |||
| // because the rail provides its own. | |||
| const sidebarOwnsHeader = | |||
There was a problem hiding this comment.
Question (viewport gate bypassed): useLayoutVariant deliberately only enables v2 at laptop+ ("v2 chrome only renders at laptop+"), but the middleware can only see UA/client hints, not viewport. A desktop-UA user with a narrow window, or an iPad in desktop mode (Macintosh UA, Safari sends no sec-ch-ua-mobile), lands on a layout-v2 route below the laptop breakpoint. Here isForcedV2 overrides both sidebarRendered and the isLaptop gate, so sidebarOwnsHeader becomes true and the header is suppressed — what navigation chrome renders at tablet width on a forced-v2 route? Previously the client eval would have fallen back to v1 chrome in exactly this case. Worth checking the preview at ~700–1000px width on a v2-enrolled session. Reviewed by AI.
| }); | ||
|
|
||
| try { | ||
| await growthbook.loadFeatures({ timeout: DEFAULT_TIMEOUT }); |
There was a problem hiding this comment.
Non-blocking (hot-path latency): this runs in edge middleware ahead of what is otherwise an instant ISR HIT for the highest-traffic SEO pages. The SDK's in-memory feature repository cache helps warm isolates, but a cold isolate pays a network fetch to the GB CDN with up to a 2s timeout before the page can be served, and a GB outage degrades every eligible request's TTFB to +2s (it does fail closed to v1, which is right). Consider a tighter timeout (e.g. 300–500ms) and/or pointing GROWTHBOOK_API_HOST at a nearby proxy — a missed evaluation just means one v1-rendered pageview. Reviewed by AI.
Changes
Support layout SSR through gb feature flag
Events
Did you introduce any new tracking events?
Experiment
Did you introduce any new experiments?
Manual Testing
Caution
Please make sure existing components are not breaking/affected by this PR
Preview domain
https://layout-v2-ssr.preview.app.daily.dev