fix for ssr flags for posthog/next#3268
Conversation
|
@firstarcprime is attempting to deploy a commit to the PostHog Team on Vercel. A member of the Team first needs to authorize it. |
Prompt To Fix All With AIThis is a comment left during a code review.
Path: packages/next/src/client/ClientPostHogProvider.tsx
Line: 50-53
Comment:
**`updateFlags` called on every render — overwrites live flag updates**
`updateFlags` is invoked unconditionally during the render phase, so it runs on **every re-render** of `ClientPostHogProvider` (e.g. page navigation, parent state change). Because `updateFlags` defaults to *replace* mode (no `merge: true`), it will clobber any real-time flag values that PostHog received from the network after the initial load, reverting the user's flags back to the stale bootstrap snapshot each time.
It also runs during SSR (since the `typeof window` guard was removed), where the posthog-js singleton may not be fully initialised, making the call a potential no-op or source of unexpected behaviour server-side.
The call should be protected so it only fires once on the client, for example by tracking whether it has already been applied:
```tsx
// Track whether the bootstrap flags have been applied to avoid
// overwriting live flag updates on subsequent re-renders.
const bootstrapApplied = React.useRef(false)
if (!posthogJs.__loaded) {
posthogJs.init(apiKey, mergedOptions)
}
if (bootstrap?.featureFlags && !bootstrapApplied.current) {
posthogJs.updateFlags(bootstrap.featureFlags)
bootstrapApplied.current = true
}
```
(A `useEffect` could also be used, but the existing pattern deliberately initialises during render for the reasons explained in the JSDoc.)
**Rule Used:** Add inline comments to clarify the purpose of sign... ([source](https://app.greptile.com/review/custom-context?memory=4d5b48c5-045d-4693-9cd9-4081bb19508b))
**Learnt From**
[PostHog/posthog#32083](https://github.com/PostHog/posthog/pull/32083)
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: .changeset/shy-jeans-rescue.md
Line: 2
Comment:
**Changeset bump type should be `patch`, not `minor`**
This is a bug fix, which maps to a `patch` version bump in semver. A `minor` bump is reserved for new backwards-compatible features.
```suggestion
'@posthog/next': patch
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/next/tests/ClientPostHogProvider.test.tsx
Line: 127-149
Comment:
**Missing test: `updateFlags` called when already loaded**
The two new tests only cover the case where `__loaded = false` (the `beforeEach` default). There is no test verifying that `updateFlags` is still called when `__loaded = true` — i.e. when posthog was initialised in a previous render and `init()` is skipped. This is the production steady-state after the first mount. Adding a test for it would document and guard the intended behaviour, ensuring the bootstrap flags are applied on subsequent renders even after init has already run.
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: "fix for ssr flags" |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (bootstrap?.featureFlags) { | ||
| // If bootstrapping, update feature flags from the server | ||
| posthogJs.updateFlags(bootstrap.featureFlags) | ||
| } |
There was a problem hiding this comment.
updateFlags called on every render — overwrites live flag updates
updateFlags is invoked unconditionally during the render phase, so it runs on every re-render of ClientPostHogProvider (e.g. page navigation, parent state change). Because updateFlags defaults to replace mode (no merge: true), it will clobber any real-time flag values that PostHog received from the network after the initial load, reverting the user's flags back to the stale bootstrap snapshot each time.
It also runs during SSR (since the typeof window guard was removed), where the posthog-js singleton may not be fully initialised, making the call a potential no-op or source of unexpected behaviour server-side.
The call should be protected so it only fires once on the client, for example by tracking whether it has already been applied:
// Track whether the bootstrap flags have been applied to avoid
// overwriting live flag updates on subsequent re-renders.
const bootstrapApplied = React.useRef(false)
if (!posthogJs.__loaded) {
posthogJs.init(apiKey, mergedOptions)
}
if (bootstrap?.featureFlags && !bootstrapApplied.current) {
posthogJs.updateFlags(bootstrap.featureFlags)
bootstrapApplied.current = true
}(A useEffect could also be used, but the existing pattern deliberately initialises during render for the reasons explained in the JSDoc.)
Rule Used: Add inline comments to clarify the purpose of sign... (source)
Learnt From
PostHog/posthog#32083
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/next/src/client/ClientPostHogProvider.tsx
Line: 50-53
Comment:
**`updateFlags` called on every render — overwrites live flag updates**
`updateFlags` is invoked unconditionally during the render phase, so it runs on **every re-render** of `ClientPostHogProvider` (e.g. page navigation, parent state change). Because `updateFlags` defaults to *replace* mode (no `merge: true`), it will clobber any real-time flag values that PostHog received from the network after the initial load, reverting the user's flags back to the stale bootstrap snapshot each time.
It also runs during SSR (since the `typeof window` guard was removed), where the posthog-js singleton may not be fully initialised, making the call a potential no-op or source of unexpected behaviour server-side.
The call should be protected so it only fires once on the client, for example by tracking whether it has already been applied:
```tsx
// Track whether the bootstrap flags have been applied to avoid
// overwriting live flag updates on subsequent re-renders.
const bootstrapApplied = React.useRef(false)
if (!posthogJs.__loaded) {
posthogJs.init(apiKey, mergedOptions)
}
if (bootstrap?.featureFlags && !bootstrapApplied.current) {
posthogJs.updateFlags(bootstrap.featureFlags)
bootstrapApplied.current = true
}
```
(A `useEffect` could also be used, but the existing pattern deliberately initialises during render for the reasons explained in the JSDoc.)
**Rule Used:** Add inline comments to clarify the purpose of sign... ([source](https://app.greptile.com/review/custom-context?memory=4d5b48c5-045d-4693-9cd9-4081bb19508b))
**Learnt From**
[PostHog/posthog#32083](https://github.com/PostHog/posthog/pull/32083)
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
I don't think we need the useRef here, but we should only either init or updateFlags. if/else seems reasonable.
There was a problem hiding this comment.
Looking at the code, I need to point out that using if/else doesn't solve the re-render problem that was raised:
With if/else structure:
if (!posthogJs.__loaded) {
posthogJs.init(apiKey, mergedOptions) // Run once
} else if (bootstrap?.featureFlags) {
posthogJs.updateFlags(bootstrap.featureFlags) // Run on EVERY subsequent render
}What happens:
- Render 1:
__loaded = false→ callsinit()→__loadedbecomestrue - Render 2+:
__loaded = true→ skipsinit, executeselse if→ callsupdateFlags()each time
The component could re-render for various reasons (parent state change, navigation, React StrictMode, etc.), and each re-render would call updateFlags() with the bootstrap snapshot, overwriting any live flags fetched from PostHog's servers.
Question: Are you thinking that:
mergedOptionsalready contains the bootstrap data, soinit()handles it and we don't needupdateFlagsat all?- Or that this component won't re-render in practice, so the
else ifis effectively a one-time call?
If option 1, we should remove the updateFlags call entirely. If option 2, we should still guard against re-renders since they can happen unexpectedly.
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| '@posthog/next': minor | |||
There was a problem hiding this comment.
patch, not minor
This is a bug fix, which maps to a patch version bump in semver. A minor bump is reserved for new backwards-compatible features.
| '@posthog/next': minor | |
| '@posthog/next': patch |
Prompt To Fix With AI
This is a comment left during a code review.
Path: .changeset/shy-jeans-rescue.md
Line: 2
Comment:
**Changeset bump type should be `patch`, not `minor`**
This is a bug fix, which maps to a `patch` version bump in semver. A `minor` bump is reserved for new backwards-compatible features.
```suggestion
'@posthog/next': patch
```
How can I resolve this? If you propose a fix, please make it concise.| it('calls updateFlags with bootstrap featureFlags', () => { | ||
| const bootstrap = { | ||
| featureFlags: { 'flag-a': true, 'flag-b': 'variant-1' }, | ||
| } | ||
| render( | ||
| <ClientPostHogProvider apiKey="phc_test123" bootstrap={bootstrap}> | ||
| <div>Child</div> | ||
| </ClientPostHogProvider> | ||
| ) | ||
| expect(mockPostHogJs.updateFlags).toHaveBeenCalledWith(bootstrap.featureFlags) | ||
| }) | ||
|
|
||
| it('does not call updateFlags when bootstrap featureFlags are not provided', () => { | ||
| const bootstrap = { | ||
| distinctID: 'user_abc', | ||
| } | ||
| render( | ||
| <ClientPostHogProvider apiKey="phc_test123" bootstrap={bootstrap}> | ||
| <div>Child</div> | ||
| </ClientPostHogProvider> | ||
| ) | ||
| expect(mockPostHogJs.updateFlags).not.toHaveBeenCalled() | ||
| }) |
There was a problem hiding this comment.
updateFlags called when already loaded
The two new tests only cover the case where __loaded = false (the beforeEach default). There is no test verifying that updateFlags is still called when __loaded = true — i.e. when posthog was initialised in a previous render and init() is skipped. This is the production steady-state after the first mount. Adding a test for it would document and guard the intended behaviour, ensuring the bootstrap flags are applied on subsequent renders even after init has already run.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/next/tests/ClientPostHogProvider.test.tsx
Line: 127-149
Comment:
**Missing test: `updateFlags` called when already loaded**
The two new tests only cover the case where `__loaded = false` (the `beforeEach` default). There is no test verifying that `updateFlags` is still called when `__loaded = true` — i.e. when posthog was initialised in a previous render and `init()` is skipped. This is the production steady-state after the first mount. Adding a test for it would document and guard the intended behaviour, ensuring the bootstrap flags are applied on subsequent renders even after init has already run.
How can I resolve this? If you propose a fix, please make it concise.
dustinbyrne
left a comment
dustinbyrne
left a comment
There was a problem hiding this comment.
Good catch, @firstarcprime, and thanks for the submission!
I think this change is good as-is, but another option would be to automatically reload flags from the client when the pathname changes. E.g., something like:
const pathname = usePathname()
React.useEffect(() => {
if (posthogJs.__loaded) {
posthogJs.reloadFeatureFlags()
}
}, [pathname])So I'm curious, would you prefer that flags are always evaluated server-side, or just on the initial render? Both ways have pros and cons, so I could potentially see this being a configurable option as well:
- Always bootstrap flags via SSR: guaranteed never to flicker, longer response times (though, local evaluation could help in this case)
- Only bootstrap flags via SSR on first page load: guaranteed not to flicker on first page load, faster response times following the initial page load.
Interested to hear your thoughts here!
| if (!posthogJs.__loaded) { | ||
| posthogJs.init(apiKey, mergedOptions) | ||
| } | ||
|
|
||
| if (bootstrap?.featureFlags) { | ||
| // If bootstrapping, update feature flags from the server | ||
| posthogJs.updateFlags(bootstrap.featureFlags) | ||
| } |
There was a problem hiding this comment.
Also consider payloads
| if (!posthogJs.__loaded) { | |
| posthogJs.init(apiKey, mergedOptions) | |
| } | |
| if (bootstrap?.featureFlags) { | |
| // If bootstrapping, update feature flags from the server | |
| posthogJs.updateFlags(bootstrap.featureFlags) | |
| } | |
| if (!posthogJs.__loaded) { | |
| posthogJs.init(apiKey, mergedOptions) | |
| } else if (bootstrap?.featureFlags) { | |
| // If bootstrapping, update feature flags from the server | |
| posthogJs.updateFlags(bootstrap.featureFlags, bootstrap.featureFlagPayloads | |
| } |
There was a problem hiding this comment.
A configurable option sounds great; personally, I think always rendering on the server is a sensible default, as the flicker can be a massive deal for user experience. Users may not notice a small increase in load time, but they'll definitely notice the flicker, and it could make the website owners seem like they're running a visibly buggy site. Though, off the top of my head, I'm not sure how bootstrapping only on first page load could be achieved, and whether this could reintroduce the flicker.
In response to the useEffect you mentioned, I've actually tried testing this and I think it might cause another issue; I get a NextJs error saying that it can't update one of my components at the same time as ClientPosthogProvider.
Thanks for the correction on the "else if" and the flag payloads.
2 participants
Problem
@dustinbyrne
There is a hydration error with @posthog/next using SSR with Nextjs 16 App Router. The docs (USAGE.md and README.md) were followed exactly for setting up (minus how I had to use proxy.ts instead of middleware.ts as the latter is deprecated).
After investigating, I found that the flags that were fetched by the PosthogProvider were not being passed to the ClientPosthogProvider during SSR; they seem to only be passed along when posthogJS is initialised. From what I could debug, this leads to the feature flags being undefined on the server and then defined on the client, which flickers the UI.
I performed this change as a patch and it finally started working as I expected; the UI would load with the correct feature flag on both the server and the client. I'll admit that I'm not a good enough engineer to know exactly how hacky this change is, so I was hoping somebody with more experience could take a look. This is my first contributing PR, so apologies for mistakes/inconveniences.
Changes
I removed the window check and added a call to updateFlags if the user enables bootstrapping on the PosthogProvider. This seemed to resolve the issue, though I have no idea if there will be any hidden consequences.
Release info Sub-libraries affected
Libraries affected
Checklist
If releasing new changes
pnpm changesetto generate a changeset file