Skip to content

fix for ssr flags for posthog/next #3268

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
firstarcprime wants to merge 1 commit into
base: main
Choose a base branch
from
+37 −1
Open

fix for ssr flags for posthog/next #3268

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/shy-jeans-rescue.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@posthog/next': minor
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot Mar 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Suggested change
'@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.

---

fix for feature flags not being used during SSR, leading to hydration error
7 changes: 6 additions & 1 deletion packages/next/src/client/ClientPostHogProvider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,14 @@ export function ClientPostHogProvider({ apiKey, options, bootstrap, children }:
// Initialize eagerly during render on the client so that child effects
// see a fully configured posthog instance. The `__loaded` guard prevents
// double-init (e.g. React StrictMode).
if (typeof window !== 'undefined' && !posthogJs.__loaded) {
if (!posthogJs.__loaded) {
posthogJs.init(apiKey, mergedOptions)
}

if (bootstrap?.featureFlags) {
// If bootstrapping, update feature flags from the server
posthogJs.updateFlags(bootstrap.featureFlags)
}
Comment on lines +50 to +53
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot Mar 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

@dustinbyrne dustinbyrne Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need the useRef here, but we should only either init or updateFlags. if/else seems reasonable.

Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 → calls init() → __loaded becomes true
  • Render 2+: __loaded = true → skips init, executes else if → calls updateFlags() 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:

  1. mergedOptions already contains the bootstrap data, so init() handles it and we don't need updateFlags at all?
  2. Or that this component won't re-render in practice, so the else if is 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.

Comment on lines +46 to +53
Copy link
Copy Markdown
Contributor

@dustinbyrne dustinbyrne Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also consider payloads

Suggested change
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
}

Copy link
Copy Markdown
Author

@firstarcprime firstarcprime Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


return <ReactPostHogProvider client={posthogJs}>{children}</ReactPostHogProvider>
}
26 changes: 26 additions & 0 deletions packages/next/tests/ClientPostHogProvider.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ jest.mock('posthog-js', () => ({
default: {
__loaded: false,
init: jest.fn(),
updateFlags: jest.fn()
},
}))

Expand All @@ -25,6 +26,7 @@ describe('ClientPostHogProvider', () => {
beforeEach(() => {
mockPostHogProvider.mockClear()
;(mockPostHogJs.init as jest.Mock).mockClear()
;(mockPostHogJs.updateFlags as jest.Mock).mockClear()
mockPostHogJs.__loaded = false
})

Expand Down Expand Up @@ -122,6 +124,30 @@ describe('ClientPostHogProvider', () => {
expect(mockPostHogJs.init).not.toHaveBeenCalled()
})

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()
})
Comment on lines +127 to +149
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot Mar 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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.


it('renders children and warns when apiKey is empty', () => {
const warnSpy = jest.spyOn(console, 'warn').mockImplementation()
render(
Expand Down
Loading