feat(onboarding): add onboarding flow variant dispatch system#60951
Merged
Conversation
Add a multivariate `ONBOARDING_FLOW_VARIANT` flag and a small dispatch layer so the entire onboarding experience can be swapped per variant. The current flow becomes `control`; with no variants registered, everyone falls through to it unchanged. - New flag `onboarding-flow-variant` (multivariate, control only for now) - `onboardingLogic.onboardingFlowVariant` selector reads the flag, defaults to control - `onboardingVariantRegistry` maps variant -> component; control holds the existing flow - `Onboarding.tsx` scene shell dispatches to the registered component Generated-By: PostHog Code Task-Id: 83397e93-9f89-4f96-aa39-90cf8d83145b
Contributor
|
Hey @fercgomes! 👋 It looks like your git author email on this PR isn't your
You can fix it for this repo with: git config user.email "you@posthog.com"Or set it globally with |
Contributor
|
Size Change: 0 B Total Size: 80.9 MB ℹ️ View Unchanged
|
Generated-By: PostHog Code Task-Id: 83397e93-9f89-4f96-aa39-90cf8d83145b
Contributor
|
🎭 Playwright report · View test results →
These issues are not necessarily caused by your changes. |
Contributor
|
👋 Visual changes detected for this PR. Review and approve in PostHog Visual Review If these changes are unexpected, they may be caused by a flaky test or a broken snapshot on master. Don't approve — rerun the job or wait for a fix. |
Generated-By: PostHog Code Task-Id: 83397e93-9f89-4f96-aa39-90cf8d83145b
Contributor
MCP UI Apps size report
|
fercgomes
force-pushed
the
posthog-code/onboarding-flow-variant
branch
from
b437ca8 to
b6e809d
Compare
Add a multivariate `ONBOARDING_FLOW_VARIANT` flag and a dispatch layer so the entire onboarding experience can be swapped per variant. The current flow is `control`; with no variants registered everyone falls through to it unchanged. - New flag `onboarding-flow-variant` (multivariate, control only for now) - `onboardingLogic.onboardingFlowVariant` selector resolves the flag, defaulting to control for missing/non-string values - `onboardingVariantRegistry` maps a variant to its component; control holds the existing flow - `Onboarding.tsx` dispatches to the registered component - Per-variant chrome via `onboardingVariants.ts`: a variant can declare `chrome: 'none'` so `navigationLogic` renders no navbar and the variant owns the whole viewport - Tests cover variant resolution and chrome fallbacks Generated-By: PostHog Code Task-Id: 83397e93-9f89-4f96-aa39-90cf8d83145b
….com/PostHog/posthog into posthog-code/onboarding-flow-variant
assign-reviewers-posthog
Bot
requested review from
a team,
MattBro and
rafaeelaudibert
and removed request for
a team
Contributor
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
frontend/src/scenes/onboarding/onboardingVariants.test.ts:7-35
The three `resolveOnboardingFlowVariant` cases and the two `onboardingVariantChrome` cases are each testing the same function with different inputs — a classic fit for `it.each`. The project's conventions explicitly prefer parameterised tests, and the current structure forces a separate named test for every future flag value added to `ONBOARDING_FLOW_VARIANTS`.
```suggestion
describe('resolveOnboardingFlowVariant', () => {
it.each<[string, FeatureFlagsSet, string]>([
['flag not set', {} as FeatureFlagsSet, 'control'],
['boolean flag value', { [FEATURE_FLAGS.ONBOARDING_FLOW_VARIANT]: true } as FeatureFlagsSet, 'control'],
[
'named variant',
{ [FEATURE_FLAGS.ONBOARDING_FLOW_VARIANT]: 'some_future_variant' } as FeatureFlagsSet,
'some_future_variant',
],
])('%s → %s', (_label, flags, expected) => {
expect(resolveOnboardingFlowVariant(flags)).toBe(expected)
})
})
describe('onboardingVariantChrome', () => {
it.each<[string, string]>([
['control', 'minimal'],
['not_a_real_variant', 'minimal'],
])('%s → %s', (variant, expected) => {
expect(onboardingVariantChrome(variant)).toBe(expected)
})
})
```
### Issue 2 of 2
frontend/src/scenes/onboarding/onboardingLogic.test.ts:572-603
**Duplicate edge-case coverage** — the three `onboardingFlowVariant` tests (flag unset → `control`, boolean flag → `control`, string flag → passthrough) are identical to the three unit tests already in `onboardingVariants.test.ts`. The pure-function behaviour is covered at the unit level; at this integration level a single test verifying the selector is wired (i.e. sets the flag to a named string and reads it back through `logic.values.onboardingFlowVariant`) is enough to catch a mis-wiring without repeating all the edge cases.
Reviews (1): Last reviewed commit: "Merge branch 'posthog-code/onboarding-fl..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+7
to
+35
| describe('resolveOnboardingFlowVariant', () => { | ||
| it('falls back to control when the flag is not set', () => { | ||
| expect(resolveOnboardingFlowVariant({} as FeatureFlagsSet)).toBe('control') | ||
| }) | ||
|
|
||
| it('falls back to control when the flag resolves to a boolean', () => { | ||
| expect( | ||
| resolveOnboardingFlowVariant({ [FEATURE_FLAGS.ONBOARDING_FLOW_VARIANT]: true } as FeatureFlagsSet) | ||
| ).toBe('control') | ||
| }) | ||
|
|
||
| it('returns the variant string when set to a named variant', () => { | ||
| expect( | ||
| resolveOnboardingFlowVariant({ | ||
| [FEATURE_FLAGS.ONBOARDING_FLOW_VARIANT]: 'some_future_variant', | ||
| } as FeatureFlagsSet) | ||
| ).toBe('some_future_variant') | ||
| }) | ||
| }) | ||
|
|
||
| describe('onboardingVariantChrome', () => { | ||
| it('control keeps the minimal top bar', () => { | ||
| expect(onboardingVariantChrome('control')).toBe('minimal') | ||
| }) | ||
|
|
||
| it('defaults to minimal for an unregistered variant', () => { | ||
| expect(onboardingVariantChrome('not_a_real_variant')).toBe('minimal') | ||
| }) | ||
| }) |
Contributor
There was a problem hiding this comment.
resolveOnboardingFlowVariant cases and the two onboardingVariantChrome cases are each testing the same function with different inputs — a classic fit for it.each. The project's conventions explicitly prefer parameterised tests, and the current structure forces a separate named test for every future flag value added to ONBOARDING_FLOW_VARIANTS.
Suggested change
| describe('resolveOnboardingFlowVariant', () => { | |
| it('falls back to control when the flag is not set', () => { | |
| expect(resolveOnboardingFlowVariant({} as FeatureFlagsSet)).toBe('control') | |
| }) | |
| it('falls back to control when the flag resolves to a boolean', () => { | |
| expect( | |
| resolveOnboardingFlowVariant({ [FEATURE_FLAGS.ONBOARDING_FLOW_VARIANT]: true } as FeatureFlagsSet) | |
| ).toBe('control') | |
| }) | |
| it('returns the variant string when set to a named variant', () => { | |
| expect( | |
| resolveOnboardingFlowVariant({ | |
| [FEATURE_FLAGS.ONBOARDING_FLOW_VARIANT]: 'some_future_variant', | |
| } as FeatureFlagsSet) | |
| ).toBe('some_future_variant') | |
| }) | |
| }) | |
| describe('onboardingVariantChrome', () => { | |
| it('control keeps the minimal top bar', () => { | |
| expect(onboardingVariantChrome('control')).toBe('minimal') | |
| }) | |
| it('defaults to minimal for an unregistered variant', () => { | |
| expect(onboardingVariantChrome('not_a_real_variant')).toBe('minimal') | |
| }) | |
| }) | |
| describe('resolveOnboardingFlowVariant', () => { | |
| it.each<[string, FeatureFlagsSet, string]>([ | |
| ['flag not set', {} as FeatureFlagsSet, 'control'], | |
| ['boolean flag value', { [FEATURE_FLAGS.ONBOARDING_FLOW_VARIANT]: true } as FeatureFlagsSet, 'control'], | |
| [ | |
| 'named variant', | |
| { [FEATURE_FLAGS.ONBOARDING_FLOW_VARIANT]: 'some_future_variant' } as FeatureFlagsSet, | |
| 'some_future_variant', | |
| ], | |
| ])('%s → %s', (_label, flags, expected) => { | |
| expect(resolveOnboardingFlowVariant(flags)).toBe(expected) | |
| }) | |
| }) | |
| describe('onboardingVariantChrome', () => { | |
| it.each<[string, string]>([ | |
| ['control', 'minimal'], | |
| ['not_a_real_variant', 'minimal'], | |
| ])('%s → %s', (variant, expected) => { | |
| expect(onboardingVariantChrome(variant)).toBe(expected) | |
| }) | |
| }) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/scenes/onboarding/onboardingVariants.test.ts
Line: 7-35
Comment:
The three `resolveOnboardingFlowVariant` cases and the two `onboardingVariantChrome` cases are each testing the same function with different inputs — a classic fit for `it.each`. The project's conventions explicitly prefer parameterised tests, and the current structure forces a separate named test for every future flag value added to `ONBOARDING_FLOW_VARIANTS`.
```suggestion
describe('resolveOnboardingFlowVariant', () => {
it.each<[string, FeatureFlagsSet, string]>([
['flag not set', {} as FeatureFlagsSet, 'control'],
['boolean flag value', { [FEATURE_FLAGS.ONBOARDING_FLOW_VARIANT]: true } as FeatureFlagsSet, 'control'],
[
'named variant',
{ [FEATURE_FLAGS.ONBOARDING_FLOW_VARIANT]: 'some_future_variant' } as FeatureFlagsSet,
'some_future_variant',
],
])('%s → %s', (_label, flags, expected) => {
expect(resolveOnboardingFlowVariant(flags)).toBe(expected)
})
})
describe('onboardingVariantChrome', () => {
it.each<[string, string]>([
['control', 'minimal'],
['not_a_real_variant', 'minimal'],
])('%s → %s', (variant, expected) => {
expect(onboardingVariantChrome(variant)).toBe(expected)
})
})
```
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!
Comment on lines
572
to
603
| }) | ||
| }) | ||
|
|
||
| describe('onboardingFlowVariant', () => { | ||
| const setVariant = (value: string | boolean | undefined): void => { | ||
| featureFlagLogic | ||
| .findMounted() | ||
| ?.actions.setFeatureFlags( | ||
| value === undefined ? [] : [FEATURE_FLAGS.ONBOARDING_FLOW_VARIANT], | ||
| value === undefined ? {} : { [FEATURE_FLAGS.ONBOARDING_FLOW_VARIANT]: value } | ||
| ) | ||
| } | ||
|
|
||
| it('falls back to control when the flag is not set', () => { | ||
| // Default test env sets no feature flags — the missing-flag path must resolve to control. | ||
| expect(logic.values.onboardingFlowVariant).toBe('control') | ||
| }) | ||
|
|
||
| it('falls back to control when the flag resolves to a boolean (non-string) value', () => { | ||
| setVariant(true) | ||
| expect(logic.values.onboardingFlowVariant).toBe('control') | ||
| }) | ||
|
|
||
| it('returns the variant string when the flag is set to a named variant', () => { | ||
| setVariant('some_future_variant') | ||
| expect(logic.values.onboardingFlowVariant).toBe('some_future_variant') | ||
| }) | ||
| }) | ||
|
|
||
| describe('flow stays consistent across product changes', () => { | ||
| it('switching primary resets the secondary list and rebuilds the flow', () => { | ||
| logic.actions.setProductKey(ProductKey.PRODUCT_ANALYTICS) |
Contributor
There was a problem hiding this comment.
onboardingFlowVariant tests (flag unset → control, boolean flag → control, string flag → passthrough) are identical to the three unit tests already in onboardingVariants.test.ts. The pure-function behaviour is covered at the unit level; at this integration level a single test verifying the selector is wired (i.e. sets the flag to a named string and reads it back through logic.values.onboardingFlowVariant) is enough to catch a mis-wiring without repeating all the edge cases.
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/scenes/onboarding/onboardingLogic.test.ts
Line: 572-603
Comment:
**Duplicate edge-case coverage** — the three `onboardingFlowVariant` tests (flag unset → `control`, boolean flag → `control`, string flag → passthrough) are identical to the three unit tests already in `onboardingVariants.test.ts`. The pure-function behaviour is covered at the unit level; at this integration level a single test verifying the selector is wired (i.e. sets the flag to a named string and reads it back through `logic.values.onboardingFlowVariant`) is enough to catch a mis-wiring without repeating all the edge cases.
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!
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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
1 participant
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
We want to be able to A/B test entirely different onboarding experiences in the future, but we have no mechanism to do so today — the onboarding scene always renders one fixed flow. This ships the system to swap whole onboardings by feature flag, with no behavioral variants yet.
Changes
A small dispatch layer at the onboarding scene shell:
ONBOARDING_FLOW_VARIANT(onboarding-flow-variant), starting with justcontrol.onboardingLogic.onboardingFlowVariantselector reads the flag from the already-connectedfeatureFlags, defaulting to'control'.onboardingVariantRegistry— aRecord<string, Component>mapping a flag value to the onboarding component.controlholds today's flow (ProductSelection/OnboardingFlowHost) verbatim.Onboarding.tsxnow reads the variant and renders the registered component, falling back tocontrolfor any unset/unknown value.Because
controlis the default (and the fallback before flags load), the current experience is unchanged and flicker-free. Shipping a future onboarding is then: add a variant on the flag, build the component, register it.The user chose component-swap dispatch (no URL redirect / new routes) and the flag key
ONBOARDING_FLOW_VARIANT.How did you test this code?
I'm an agent. I could not run
typescript:check/formatlocally becausenode_modulesisn't installed in this environment (kea-typegen will regenerateonboardingLogicTypefor the new selector in CI). I verified the references are wired consistently across the four files by inspection. No automated tests were added; reviewers should run the frontend type check and, ideally, smoke-test/onboardingand/onboarding/product_analyticswith the flag unset to confirm the flow is unchanged.Automatic notifications
🤖 Agent context
Authored with Claude Code (Opus 4.8). The author explored the onboarding scene/logic and feature-flag patterns, then chose the smallest contained injection point: the
Onboarding.tsxscene shell, sinceonboardingLogicalready connectsfeatureFlagLogic. Considered and rejected a URL-redirect-per-variant approach (heavier — new scenes/routes) in favor of an in-place component registry. The control path intentionally defaults on before flags arrive to avoid flicker; gating onreceivedFeatureFlagsis noted as a future option once a real variant exists.