fix(flags): reload feature flags on bfcache (pageshow) restore#3881
Draft
posthog[bot] wants to merge 1 commit into
Draft
fix(flags): reload feature flags on bfcache (pageshow) restore#3881posthog[bot] wants to merge 1 commit into
posthog[bot] wants to merge 1 commit into
Conversation
On a back-forward cache restore the SDK is not re-initialized and no /flags request is made. When feature_flag_cache_ttl_ms is set, the restored cached flags can be past their TTL, so getFeatureFlag() returns undefined (read as false by app guards) until an unrelated action triggers a reload — silently disabling gated features. Arc restores from bfcache aggressively even on a plain refresh. Listen for pageshow and reload flags when event.persisted is true so a restored page self-heals within one network round-trip. Generated-By: PostHog Code Task-Id: e7211b94-b820-4fb8-8c86-5f67e8962931
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
packages/browser/src/__tests__/posthog-core-also.test.ts:860-876
The two tests are structurally identical apart from the `persisted` flag and the expected call count. The repo's stated preference is parameterised tests, and the pattern is already used elsewhere in this file (e.g. `configRenames` at line 65). Collapsing them removes duplication without losing any coverage.
```suggestion
it.each([
['reloads feature flags when the page is restored from the back-forward cache', true, 1],
['does not reload feature flags on a normal (non-persisted) page show', false, 0],
] as [string, boolean, number][])('%s', (_description, persisted, expectedCallCount) => {
const posthog = posthogWith(defaultConfig, defaultOverrides)
const reloadSpy = jest.spyOn(posthog.featureFlags, 'reloadFeatureFlags').mockImplementation(() => {})
dispatchPageShow(persisted)
expect(reloadSpy).toHaveBeenCalledTimes(expectedCallCount)
})
```
Reviews (1): Last reviewed commit: "fix(flags): reload feature flags on bfca..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+860
to
+876
| it('reloads feature flags when the page is restored from the back-forward cache', () => { | ||
| const posthog = posthogWith(defaultConfig, defaultOverrides) | ||
| const reloadSpy = jest.spyOn(posthog.featureFlags, 'reloadFeatureFlags').mockImplementation(() => {}) | ||
|
|
||
| dispatchPageShow(true) | ||
|
|
||
| expect(reloadSpy).toHaveBeenCalledTimes(1) | ||
| }) | ||
|
|
||
| it('does not reload feature flags on a normal (non-persisted) page show', () => { | ||
| const posthog = posthogWith(defaultConfig, defaultOverrides) | ||
| const reloadSpy = jest.spyOn(posthog.featureFlags, 'reloadFeatureFlags').mockImplementation(() => {}) | ||
|
|
||
| dispatchPageShow(false) | ||
|
|
||
| expect(reloadSpy).not.toHaveBeenCalled() | ||
| }) |
Contributor
There was a problem hiding this comment.
persisted flag and the expected call count. The repo's stated preference is parameterised tests, and the pattern is already used elsewhere in this file (e.g. configRenames at line 65). Collapsing them removes duplication without losing any coverage.
Suggested change
| it('reloads feature flags when the page is restored from the back-forward cache', () => { | |
| const posthog = posthogWith(defaultConfig, defaultOverrides) | |
| const reloadSpy = jest.spyOn(posthog.featureFlags, 'reloadFeatureFlags').mockImplementation(() => {}) | |
| dispatchPageShow(true) | |
| expect(reloadSpy).toHaveBeenCalledTimes(1) | |
| }) | |
| it('does not reload feature flags on a normal (non-persisted) page show', () => { | |
| const posthog = posthogWith(defaultConfig, defaultOverrides) | |
| const reloadSpy = jest.spyOn(posthog.featureFlags, 'reloadFeatureFlags').mockImplementation(() => {}) | |
| dispatchPageShow(false) | |
| expect(reloadSpy).not.toHaveBeenCalled() | |
| }) | |
| it.each([ | |
| ['reloads feature flags when the page is restored from the back-forward cache', true, 1], | |
| ['does not reload feature flags on a normal (non-persisted) page show', false, 0], | |
| ] as [string, boolean, number][])('%s', (_description, persisted, expectedCallCount) => { | |
| const posthog = posthogWith(defaultConfig, defaultOverrides) | |
| const reloadSpy = jest.spyOn(posthog.featureFlags, 'reloadFeatureFlags').mockImplementation(() => {}) | |
| dispatchPageShow(persisted) | |
| expect(reloadSpy).toHaveBeenCalledTimes(expectedCallCount) | |
| }) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browser/src/__tests__/posthog-core-also.test.ts
Line: 860-876
Comment:
The two tests are structurally identical apart from the `persisted` flag and the expected call count. The repo's stated preference is parameterised tests, and the pattern is already used elsewhere in this file (e.g. `configRenames` at line 65). Collapsing them removes duplication without losing any coverage.
```suggestion
it.each([
['reloads feature flags when the page is restored from the back-forward cache', true, 1],
['does not reload feature flags on a normal (non-persisted) page show', false, 0],
] as [string, boolean, number][])('%s', (_description, persisted, expectedCallCount) => {
const posthog = posthogWith(defaultConfig, defaultOverrides)
const reloadSpy = jest.spyOn(posthog.featureFlags, 'reloadFeatureFlags').mockImplementation(() => {})
dispatchPageShow(persisted)
expect(reloadSpy).toHaveBeenCalledTimes(expectedCallCount)
})
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Contributor
Contributor
|
Size Change: +1.27 kB (+0.01%) Total Size: 17 MB
ℹ️ View Unchanged
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
An enterprise customer reported that feature flags load correctly after sign-in but flip to
falseafter a page refresh in the Arc browser, silently disabling gated features for end users.The trigger is the back-forward cache (bfcache). On a bfcache restore the SDK is not re-initialized — the whole JS heap, including cached feature flags, is restored as-is and no
/flagsrequest is made. Whenfeature_flag_cache_ttl_msis configured (an opt-in that intentionally makesgetFeatureFlag()returnundefinedonce the cache is stale, so blocked/flagsrequests don't serve indefinitely-stale values), the restored flags can already be past their TTL. App guards likeif (posthog.getFeatureFlag('x'))then readundefined/falseand stay that way until some unrelated action triggers a reload — potentially never. Arc (and some other Chromium-based browsers) restore from bfcache aggressively, even on a plain refresh, which is why this surfaced there.Why: Reported by an enterprise customer as a real, user-facing regression — gated features silently turning off after a refresh.
Changes
pageshowlistener (gated onevent.persisted) that callsreloadFeatureFlags()when a page is restored from bfcache, so a restored page re-evaluates flags and self-heals within one network round-trip.I deliberately did not change the
undefined-on-stale behaviour ofgetFeatureFlag(): that is the documented, tested contract of the opt-infeature_flag_cache_ttl_msfeature, built specifically to stop serving stale flags when/flagsis blocked. Reverting it to "return cached while refreshing" would reintroduce the indefinitely-stale-flags bug that feature was created to fix. The missing piece was rehydration on bfcache restore, which is what this PR adds.Note: this fix is only observable when
feature_flag_cache_ttl_msis enabled in the SDK init — worth confirming the affected customer has it set.Release info Sub-libraries affected
Libraries affected
Checklist
If releasing new changes
pnpm changesetto generate a changeset file🤖 Agent context
Autonomy: Fully autonomous
Authored by PostHog Code (Claude Opus 4.8) from an inbox report. I traced the symptom to the interaction between the opt-in
feature_flag_cache_ttl_msstaleness check (getFeatureFlag()returningundefinedpast TTL, by design) and the absence of anypageshow/bfcache rehydration path insrc/— so a bfcache-restored page never refreshes its stale cache. I considered the alternative of returning the cached value while a refresh is in flight, but rejected it because that reverts the deliberate contract offeature_flag_cache_ttl_ms(commit ec54fd8) and would reintroduce the indefinitely-stale-flag bug. Verified via the new unit tests and the existing 172-test feature-flag suite (all passing); lint clean on the changed files. Not manually reproduced in Arc.Created with PostHog Code from an inbox report.