From 14fb89dd0cd642683842498595a6c8500cc5086e Mon Sep 17 00:00:00 2001 From: Talisson Costa Date: Thu, 13 Aug 2026 09:07:05 -0300 Subject: [PATCH] fix(onboarding): record the exposure for the variant we actually show getExperimentFlag records an exposure when it reads the flag, and the caller races that read against a 2s timeout. So a user whose decision lost the race, or came back without a targeting key, or whose storage write failed, was logged as exposed to single_page and then shown the legacy page. Those users sit in the test arm as non-converters and drag its conversion rate down. Read the flag off the SDK state instead, and record the exposure in App.js once we know which variant we are routing to. No identifier means nothing was assigned, so no exposure. Co-Authored-By: Claude Opus 5 (1M context) --- .../utils/__tests__/onboardingEntry.test.ts | 67 +++++++++++++++++-- frontend/common/utils/onboardingEntry.ts | 36 ++++++++-- frontend/web/components/App.js | 7 ++ 3 files changed, 98 insertions(+), 12 deletions(-) diff --git a/frontend/common/utils/__tests__/onboardingEntry.test.ts b/frontend/common/utils/__tests__/onboardingEntry.test.ts index cf2ff8c3d5f6..43b649836459 100644 --- a/frontend/common/utils/__tests__/onboardingEntry.test.ts +++ b/frontend/common/utils/__tests__/onboardingEntry.test.ts @@ -4,12 +4,14 @@ import { getStoredOnboardingTargetingKey, getStoredOnboardingVariant, persistOnboardingEntry, + trackOnboardingExposure, } from 'common/utils/onboardingEntry' jest.mock('@flagsmith/flagsmith', () => ({ getContext: jest.fn(), - getExperimentFlag: jest.fn(), + getState: jest.fn(), identify: jest.fn(), + trackExposureEvent: jest.fn(), })) const storage = new Map() @@ -31,11 +33,29 @@ describe('decideOnboardingEntry', () => { } as any) }) + it('reads the flag without recording an exposure', async () => { + // Given + mockFlagsmith.getState.mockReturnValue({ + flags: { + onboarding_quickstart_flow: { enabled: true, variant: 'single_page' }, + }, + } as any) + + // When + await decideOnboardingEntry() + + // Then + // Being asked the question is not being shown the answer: the caller races + // this against a timeout, so exposure is recorded once the variant lands. + expect(mockFlagsmith.trackExposureEvent).not.toHaveBeenCalled() + }) + it('returns the decision without persisting anything', async () => { // Given - mockFlagsmith.getExperimentFlag.mockReturnValue({ - enabled: true, - variant: 'single_page', + mockFlagsmith.getState.mockReturnValue({ + flags: { + onboarding_quickstart_flow: { enabled: true, variant: 'single_page' }, + }, } as any) // When @@ -53,8 +73,8 @@ describe('decideOnboardingEntry', () => { it('maps a disabled flag to control', async () => { // Given - mockFlagsmith.getExperimentFlag.mockReturnValue({ - enabled: false, + mockFlagsmith.getState.mockReturnValue({ + flags: { onboarding_quickstart_flow: { enabled: false } }, } as any) // When @@ -96,3 +116,38 @@ describe('persistOnboardingEntry', () => { expect(getStoredOnboardingTargetingKey()).toBeNull() }) }) + +describe('trackOnboardingExposure', () => { + beforeEach(() => { + jest.resetAllMocks() + }) + + it('records the variant the user was routed to, not the one served', () => { + // Given + const decision = { + targetingKey: 'anon-123', + variant: 'single_page', + } as const + + // When + // The flag said single_page, but persistOnboardingEntry downgraded it. + trackOnboardingExposure(decision, 'control') + + // Then + expect(mockFlagsmith.trackExposureEvent).toHaveBeenCalledWith( + 'onboarding_quickstart_flow', + { identifier: 'anon-123', value: 'control' }, + ) + }) + + it('records nothing without an identifier, since nothing was assigned', () => { + // When + trackOnboardingExposure( + { targetingKey: null, variant: 'single_page' }, + 'control', + ) + + // Then + expect(mockFlagsmith.trackExposureEvent).not.toHaveBeenCalled() + }) +}) diff --git a/frontend/common/utils/onboardingEntry.ts b/frontend/common/utils/onboardingEntry.ts index df45626196e1..0a7f4f9bb5a2 100644 --- a/frontend/common/utils/onboardingEntry.ts +++ b/frontend/common/utils/onboardingEntry.ts @@ -5,6 +5,8 @@ import { storageGet, storageRemove, storageSet } from 'common/safeLocalStorage' const TARGETING_KEY_STORAGE_KEY = 'onboarding_targeting_key' const VARIANT_STORAGE_KEY = 'onboarding_variant' +export const ONBOARDING_FLAG_NAME = 'onboarding_quickstart_flow' + export type OnboardingEntryDecision = { variant: OnboardingVariant targetingKey: string | null @@ -13,23 +15,45 @@ export type OnboardingEntryDecision = { /** * Decide which onboarding flow a new user enters, before their organisation * exists. Identifies with an empty identifier so the API assigns a - * pseudorandom one and reads the flag under it (recording the exposure). + * pseudorandom one, then reads the flag under it. + * + * Reads the flag without recording an exposure: the caller races this against + * a timeout, so being asked the question is not the same as being shown the + * answer. `trackOnboardingExposure` records it once the variant is applied. * - * Persists nothing: the caller races this against a timeout, and a late - * decision must not be stored — by then the SDK identity may already be - * the logged-in user, and the routing it should have driven has happened. - * Call `persistOnboardingEntry` with an accepted decision. + * Persists nothing: a late decision must not be stored — by then the SDK + * identity may already be the logged-in user, and the routing it should have + * driven has happened. Call `persistOnboardingEntry` with an accepted decision. */ export async function decideOnboardingEntry(): Promise { // @ts-expect-error transient is missing from the SDK's identify type await flagsmith.identify('', {}, true) - const flag = flagsmith.getExperimentFlag('onboarding_quickstart_flow') + const flag = flagsmith.getState().flags?.[ONBOARDING_FLAG_NAME] const identifier = flagsmith.getContext().identity?.identifier const variant: OnboardingVariant = flag?.enabled && flag.variant !== 'control' ? 'single_page' : 'control' return { targetingKey: identifier ? String(identifier) : null, variant } } +/** + * Record the exposure for the variant the user was actually routed to, against + * the identifier that becomes the organisation's targeting key. Skipped when + * there is no identifier: without one nothing was assigned, so the user was + * never in the experiment. + */ +export function trackOnboardingExposure( + decision: OnboardingEntryDecision, + appliedVariant: OnboardingVariant, +): void { + if (!decision.targetingKey) { + return + } + flagsmith.trackExposureEvent(ONBOARDING_FLAG_NAME, { + identifier: decision.targetingKey, + value: appliedVariant, + }) +} + /** * Store an accepted entry decision. The identifier becomes the * organisation's `targeting_key` at creation, pinning its bucketing to diff --git a/frontend/web/components/App.js b/frontend/web/components/App.js index 4faaf15696a6..29fdcaaa7f80 100644 --- a/frontend/web/components/App.js +++ b/frontend/web/components/App.js @@ -16,6 +16,7 @@ import { decideOnboardingEntry, getStoredOnboardingVariant, persistOnboardingEntry, + trackOnboardingExposure, } from 'common/utils/onboardingEntry' import { Provider } from 'react-redux' import { getStore } from 'common/store' @@ -158,6 +159,12 @@ const App = class extends Component { // Only an accepted decision is persisted: a decision losing the // race must not store its assignment after routing has happened. const variant = decision ? persistOnboardingEntry(decision) : 'control' + // Exposure records the variant we are about to route to, not the one + // the flag returned: a decision that lost the race, or that came back + // without a targeting key, was never shown to anyone. + if (decision) { + trackOnboardingExposure(decision, variant) + } // Restore the logged-in identity for the rest of the app. Promise.resolve(API.flagsmithIdentify()).catch(() => {}) if (variant === 'single_page') {