Skip to content
29 changes: 28 additions & 1 deletion browser_tests/tests/billingFacadeConsumers.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,8 @@ async function mockCloudBoot(
const billingRequests = {
legacyStatus: 0,
legacyBalance: 0,
workspaceStatus: 0
workspaceStatus: 0,
workspaceBalance: 0
}

await page.route('**/api/features', (r) => r.fulfill(jsonRoute(remoteConfig)))
Expand Down Expand Up @@ -125,6 +126,7 @@ async function mockCloudBoot(
)
})
await page.route('**/api/billing/balance', (r) => {
billingRequests.workspaceBalance++
return r.fulfill(jsonRoute(mockWorkspaceBalance))
})
await page.route('**/api/billing/plans', (r) =>
Expand Down Expand Up @@ -200,6 +202,31 @@ test.describe('Billing facade consumers (FE-933)', { tag: '@cloud' }, () => {
expect(billingRequests.legacyBalance).toBeGreaterThan(0)
})

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

suggestion (non-blocking): the flag-off baseline asserts legacyBalance > 0 but never asserts workspaceBalance === 0. Without that, the flag-on test (workspaceBalance > 0) and this test do not form a true mutual-exclusivity pair -- cross-contamination of /api/billing/balance in the flag-off path goes undetected. Consider adding expect(billingRequests.workspaceBalance).toBe(0) here.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I tested this assertion in the cloud Playwright spec, and it fails because bootstrap legitimately makes one workspace-balance request before /api/billing/status returns legacy_stripe and the facade switches to legacy billing. I therefore did not retain workspaceBalance === 0; the stable post-selection behavior remains covered by legacyBalance > 0.


test('rollout flag migrates a legacy Stripe workspace to workspace billing', async ({
page
}) => {
test.setTimeout(60_000)

const billingRequests = await mockCloudBoot(
page,
{
is_active: true,
subscription_tier: 'PRO',
subscription_duration: 'MONTHLY',
has_funds: true
},
{ legacy_billing_migration_enabled: true },
'legacy_stripe'
)
await bootApp(page)

await expect
.poll(() => billingRequests.workspaceBalance, { timeout: 30_000 })
.toBeGreaterThan(0)
expect(billingRequests.legacyStatus).toBe(0)
expect(billingRequests.legacyBalance).toBe(0)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nitpick (non-blocking): expect.poll() here uses the default 5 s Playwright timeout while test.setTimeout(60_000) is set. If the workspace balance fetch is delayed behind async flag resolution in a slow CI runner, the poll could expire before the request fires. Making the timeout explicit matches the test's declared budget: await expect.poll(() => billingRequests.workspaceBalance, { timeout: 30_000 }).toBeGreaterThan(0).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Addressed in 91b27ec by giving the workspace-balance poll an explicit 30-second timeout.

})

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

suggestion (non-blocking): the new flag-on test checks workspaceBalance > 0 and legacyBalance === 0, but the flag-off test at line 201 also asserts legacyStatus === 0. A migrated workspace that still calls **/customers/cloud-subscription-status for subscription state before switching rails would go undetected here. Consider adding expect(billingRequests.legacyStatus).toBe(0).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Addressed in 91b27ec. The migrated browser test now asserts legacyStatus === 0 in addition to workspace balance activity and no legacy balance request.


test('subscribe-to-run routes an inactive FREE user to the pricing table', async ({
page
}) => {
Expand Down
25 changes: 24 additions & 1 deletion src/composables/billing/useBillingContext.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ import type {
BillingStatusResponse,
Plan
} from '@/platform/workspace/api/workspaceApi'
import {
remoteConfig,
remoteConfigState
} from '@/platform/remoteConfig/remoteConfig'

import { useBillingContext } from './useBillingContext'

Expand Down Expand Up @@ -166,12 +170,15 @@ vi.mock('@/platform/workspace/api/workspaceApi', () => ({
currency: 'usd'
})),
subscribe: vi.fn(async () => ({ status: 'subscribed' })),
previewSubscribe: vi.fn(async () => ({ allowed: true }))
previewSubscribe: vi.fn(async () => ({ allowed: true })),
createTopup: vi.fn(async () => undefined)
}
}))

describe('useBillingContext', () => {
beforeEach(() => {
remoteConfig.value = {}
remoteConfigState.value = 'unloaded'
mockIsPersonal.value = true
mockBillingRail.value = undefined
mockSetWorkspaceBillingRail.mockImplementation(
Expand Down Expand Up @@ -340,6 +347,22 @@ describe('useBillingContext', () => {
expect(mockPurchaseCredits).toHaveBeenCalledWith(5)
})

it('routes migrated legacy Stripe topups through workspace billing', async () => {
remoteConfig.value = { legacy_billing_migration_enabled: true }
remoteConfigState.value = 'authenticated'
mockBillingRail.value = 'legacy_stripe'

const context = useBillingContext()
await nextTick()
vi.clearAllMocks()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nitpick (non-blocking): vi.clearAllMocks() is called before asserting workspaceApi.createTopup. If the useBillingContext() construction triggers an async initialize watch (the shared composable's watch on workspace changes) that settles after the clear, mocks reset before the auto-init call lands could mask unexpected side effects. The surrounding tests in this file await nextTick after construction before clearing -- consider doing the same here.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Addressed in 91b27ec by awaiting nextTick() after constructing the billing context and before clearing initialization calls.

expect(context.type.value).toBe('workspace')
await context.topup(500)

expect(workspaceApi.createTopup).toHaveBeenCalledWith(500)
expect(mockPurchaseCredits).not.toHaveBeenCalled()
})

it('switches billing adapters before refreshing a migrated balance', async () => {
mockBillingRail.value = 'legacy_stripe'
mockBillingStatus.value = {
Expand Down
44 changes: 35 additions & 9 deletions src/composables/billing/useBillingRouting.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,31 @@ import type { BillingRail } from '@/platform/workspace/api/workspaceApi'

import { useBillingRouting } from './useBillingRouting'

const { mockIsCloud, mockActiveWorkspace, mockActiveWorkspaceBillingRail } =
vi.hoisted(() => ({
mockIsCloud: { value: true },
mockActiveWorkspace: {
value: null as { id: string; type: 'personal' | 'team' } | null
},
mockActiveWorkspaceBillingRail: {
value: null as BillingRail | null
const {
mockIsCloud,
mockLegacyBillingMigrationEnabled,
mockActiveWorkspace,
mockActiveWorkspaceBillingRail
} = vi.hoisted(() => ({
mockIsCloud: { value: true },
mockLegacyBillingMigrationEnabled: { value: false },
mockActiveWorkspace: {
value: null as { id: string; type: 'personal' | 'team' } | null
},
mockActiveWorkspaceBillingRail: {
value: null as BillingRail | null
}
}))

vi.mock('@/composables/useFeatureFlags', () => ({
useFeatureFlags: () => ({
flags: {
get legacyBillingMigrationEnabled() {
return mockLegacyBillingMigrationEnabled.value
}
}
}))
})
}))

vi.mock('@/platform/distribution/types', () => ({
get isCloud() {
Expand All @@ -38,6 +53,7 @@ const team = { id: 'w-team', type: 'team' as const }
describe('useBillingRouting', () => {
beforeEach(() => {
mockIsCloud.value = true
mockLegacyBillingMigrationEnabled.value = false
mockActiveWorkspace.value = personal
mockActiveWorkspaceBillingRail.value = null
})
Expand Down Expand Up @@ -71,6 +87,16 @@ describe('useBillingRouting', () => {
expect(shouldUseUnifiedPricing.value).toBe(true)
})

it('migrates legacy Stripe personal workspaces behind the rollout flag', () => {
mockLegacyBillingMigrationEnabled.value = true
mockActiveWorkspaceBillingRail.value = 'legacy_stripe'

const { type, shouldUseWorkspaceBilling } = useBillingRouting()

expect(type.value).toBe('workspace')
expect(shouldUseWorkspaceBilling.value).toBe(true)
})

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

suggestion (non-blocking): no test covers flag=true + team workspace + legacy_stripe rail. The workspaceType === 'personal' guard in useBillingRouting.ts:33 should make the migration flag a no-op for team workspaces, but there is no assertion confirming it. The existing team workspace test runs with the default flag=false, so a future accidental removal of the personal-workspace check would go undetected.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I left this unchanged after verifying the branch. The existing team + legacy_stripe test uses flag=false and expects workspace billing; removing the workspaceType === personal guard would make that condition select legacy billing, so the current test already fails on the stated regression. A flag=true variant would still return workspace with or without the personal guard and would not add regression sensitivity.


it('uses workspace billing for migrated Stripe personal workspaces', () => {
mockActiveWorkspace.value = personal
mockActiveWorkspaceBillingRail.value = 'stripe'
Expand Down
9 changes: 6 additions & 3 deletions src/composables/billing/useBillingRouting.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { computed } from 'vue'

import { useFeatureFlags } from '@/composables/useFeatureFlags'
import { isCloud } from '@/platform/distribution/types'
import { useTeamWorkspaceStore } from '@/platform/workspace/stores/teamWorkspaceStore'

Expand All @@ -9,10 +10,11 @@ import type { BillingType } from './types'
* Selects the billing backend for the active workspace: legacy user-scoped
* (`/customers/*`) or workspace-scoped (`/api/billing/*`). Personal workspaces
* use workspace billing unless an explicit legacy Stripe rail selects legacy
* account operations. An unloaded workspace remains legacy during bootstrap,
* and OSS always uses legacy billing.
* account operations and its migration flag is off. An unloaded workspace
* remains legacy during bootstrap, and OSS always uses legacy billing.
*/
export function useBillingRouting() {
const { flags } = useFeatureFlags()
const workspaceStore = useTeamWorkspaceStore()

const shouldUseUnifiedPricing = computed(() => {
Expand All @@ -29,7 +31,8 @@ export function useBillingRouting() {

if (
workspaceType === 'personal' &&
workspaceStore.activeWorkspaceBillingRail === 'legacy_stripe'
workspaceStore.activeWorkspaceBillingRail === 'legacy_stripe' &&
!flags.legacyBillingMigrationEnabled

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

issue (observability): this adds a second dimension to rail selection — a legacy_stripe user can now be on either API family — and nothing emits which one was chosen.

The design doc's own Risks and observability section asks for exactly this: "Instrument billing_rail, chosen API family, recovery trigger, retry count, time-to-reconcile, and terminal outcome." The only reads of billing_rail in src/ are useSubscription.ts:385 and useWorkspaceBilling.ts:245, and both just write it into teamWorkspaceStore. Nothing forwards it to telemetry.

The practical consequence for the rollout: with no dimension separating migrated from unmigrated users, a cohort enable cannot be evaluated. If migrated top-ups start failing, the failures land in the same undifferentiated bucket as everyone else's, and the flag's effect is invisible until someone correlates it by hand. That is the same shape as the rail-asymmetry blindness we already have on the billing dashboards, and this PR is the moment it gets cheap to fix — one dimension on the billing actions that already emit, tagged with the resolved type from this composable.

Not asking for it in this PR if you'd rather keep the diff to the routing decision. But I'd want it landed before the first cohort, not after — otherwise the rollout has no success signal, only a failure signal that arrives via support.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Agreed. I am keeping observability out of this routing-only diff as suggested, but treating it as a rollout gate: the flag will remain at 0% and no cohort will be enabled until billing events expose both the backend billing_rail and selected API family so migrated and unmigrated outcomes can be evaluated separately.

) {
return 'legacy'
}
Expand Down
64 changes: 64 additions & 0 deletions src/composables/useFeatureFlags.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
import * as distributionTypes from '@/platform/distribution/types'
import {
cachedBillingControlEnabled,
cachedLegacyBillingMigrationEnabled,
cachedV1PaymentRecovery,
remoteConfig,
remoteConfigState
Expand Down Expand Up @@ -207,6 +208,64 @@ describe('useFeatureFlags', () => {
})
})

describe('legacyBillingMigrationEnabled', () => {
beforeEach(() => {
vi.mocked(distributionTypes).isCloud = true
remoteConfigState.value = 'authenticated'
})

afterEach(() => {
vi.mocked(distributionTypes).isCloud = false
remoteConfigState.value = 'unloaded'
remoteConfig.value = {}
cachedLegacyBillingMigrationEnabled.value = undefined
})

it('migrates legacy billing when enabled by remote config', () => {
remoteConfig.value = { legacy_billing_migration_enabled: true }

const { flags } = useFeatureFlags()

expect(flags.legacyBillingMigrationEnabled).toBe(true)
})

it('keeps migration off when remote config explicitly disables it', () => {
remoteConfig.value = { legacy_billing_migration_enabled: false }
vi.mocked(api.getServerFeature).mockReturnValue(true)

const { flags } = useFeatureFlags()

expect(flags.legacyBillingMigrationEnabled).toBe(false)
})
Comment thread
dante01yoon marked this conversation as resolved.

it('uses the server feature when authenticated config leaves it unset', () => {
vi.mocked(api.getServerFeature).mockImplementation(
(path, defaultValue) =>
path === ServerFeatureFlag.LEGACY_BILLING_MIGRATION_ENABLED
? true
: defaultValue
)

const { flags } = useFeatureFlags()

expect(flags.legacyBillingMigrationEnabled).toBe(true)
expect(api.getServerFeature).toHaveBeenCalledWith(
ServerFeatureFlag.LEGACY_BILLING_MIGRATION_ENABLED,
false
)
})

it('keeps legacy billing when the rollout flag is unset', () => {
vi.mocked(api.getServerFeature).mockImplementation(
(_path, defaultValue) => defaultValue
)

const { flags } = useFeatureFlags()

expect(flags.legacyBillingMigrationEnabled).toBe(false)
})
})

describe('onboardingTourEnabled', () => {
afterEach(() => {
remoteConfig.value = {}
Expand Down Expand Up @@ -296,6 +355,7 @@ describe('useFeatureFlags', () => {
remoteConfigState.value = 'unloaded'
remoteConfig.value = {}
cachedBillingControlEnabled.value = undefined
cachedLegacyBillingMigrationEnabled.value = undefined
cachedV1PaymentRecovery.value = undefined
})

Expand All @@ -304,21 +364,25 @@ describe('useFeatureFlags', () => {
remoteConfigState.value = 'unloaded'
remoteConfig.value = {}
cachedBillingControlEnabled.value = undefined
cachedLegacyBillingMigrationEnabled.value = undefined
cachedV1PaymentRecovery.value = undefined
})

it('returns the cached session value during the auth window', () => {
cachedBillingControlEnabled.value = true
cachedLegacyBillingMigrationEnabled.value = true
cachedV1PaymentRecovery.value = true

const { flags } = useFeatureFlags()
expect(flags.billingControlEnabled).toBe(true)
expect(flags.legacyBillingMigrationEnabled).toBe(true)
expect(flags.v1PaymentRecovery).toBe(true)
})

it('defaults to false during the auth window when nothing is cached', () => {
const { flags } = useFeatureFlags()
expect(flags.billingControlEnabled).toBe(false)
expect(flags.legacyBillingMigrationEnabled).toBe(false)
expect(flags.v1PaymentRecovery).toBe(false)
})

Expand Down
9 changes: 9 additions & 0 deletions src/composables/useFeatureFlags.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import type { Ref } from 'vue'
import { isCloud, isNightly } from '@/platform/distribution/types'
import {
cachedBillingControlEnabled,
cachedLegacyBillingMigrationEnabled,
cachedV1PaymentRecovery,
isAuthenticatedConfigLoaded,
remoteConfig
Expand Down Expand Up @@ -34,6 +35,7 @@ export enum ServerFeatureFlag {
SHOW_SIGNIN_BUTTON = 'show_signin_button',
UNIFIED_CLOUD_AUTH = 'unified_cloud_auth',
BILLING_CONTROL_ENABLED = 'billing_control_enabled',
LEGACY_BILLING_MIGRATION_ENABLED = 'legacy_billing_migration_enabled',
V1_PAYMENT_RECOVERY = 'v1_payment_recovery',
FREE_TIER_JOB_ALLOWANCE_ENABLED = 'free_tier_job_allowance_enabled',
CHURNKEY_APP_ID = 'churnkey_app_id',
Expand Down Expand Up @@ -204,6 +206,13 @@ export function useFeatureFlags() {
cachedBillingControlEnabled
)
},
get legacyBillingMigrationEnabled() {
return resolveAuthGatedFlag(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

issue (rollout blocker, not a merge blocker): moving to resolveAuthGatedFlag was my suggestion and it does fix the anonymous-response hazard I raised — but it brings a localStorage cache that is a per-browser answer to a per-user question, and this is the first flag where that selects a money rail rather than a UI affordance.

resolveAuthGatedFlag returns cachedValue.value ?? false whenever isAuthenticatedConfigLoaded is false. That computed is remoteConfigState === 'authenticated', so the cache is consulted in three states, not just the auth window: unloaded, anonymous, and error. The error case is the one that matters — it is not a brief startup race, it is a stable state that persists for the whole session once /features fails, times out, or returns 401/403.

That gives two reachable paths once a cohort is enabled:

  1. User switch on a shared browser. Nothing clears these keys. Grepping src/ for billing_control_enabled, v1_payment_recovery, and legacy_billing_migration_enabled outside the enum definitions and the Storybook mock returns only the writes in refreshRemoteConfig.ts and the reads here — no logout or session-teardown path clears them. An enrolled user's true outlives their session and is the value the next user resolves against until authenticated config lands.
  2. A /features outage. The 401/403 branch of refreshRemoteConfig deliberately wipes remoteConfig.value = {} — an explicit "do not trust config we could not authenticate" gesture — and then this resolver prefers a localStorage value written by whoever last authenticated in that browser. Those two behaviors point in opposite directions.

Why it matters specifically here: the design doc records that POST /api/billing/topup still rejects the legacy_stripe rail. So resolving true for a user the backend has not enrolled routes their top-up into an endpoint that rejects it. The failure is in the direction of a broken payment, not a cosmetic one.

Today the blast radius is genuinely zero — at 0% rollout the cached value is false for everyone, so false ?? false is correct by accident. It stops being zero on the first cohort enable.

Worth noting refreshRemoteConfig.test.ts now asserts the cache survives an anonymous refresh, so this behavior is pinned as intended rather than incidental. Changing it is a deliberate decision, not a bugfix.

Cheapest option that keeps your fix: clear the three cached flags on sign-out/user change. Alternative: scope the key by user id. Either is fine; I'd just like the decision recorded before a cohort is enabled rather than discovered from a chargeback.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Addressed in 758925f. The migration eligibility is no longer persisted in localStorage: it is now an in-memory value populated only by an authenticated /features response. It is cleared when the authenticated identity changes and whenever an authenticated config refresh fails, so another account or an outage cannot inherit a prior user’s true routing decision. Added coverage for account switching, 401 responses, and fetch failures; the focused 144 tests, typecheck, lint, and formatting all pass.

ServerFeatureFlag.LEGACY_BILLING_MIGRATION_ENABLED,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

issue: legacyBillingMigrationEnabled uses resolveFlag but its two immediate neighbors billingControlEnabled (line 202) and v1PaymentRecovery (line 216) both use resolveAuthGatedFlag. The auth-gated variant guards against the anonymous-to-authenticated config window: while !isAuthenticatedConfigLoaded, it returns the cached localStorage value rather than the bootstrap response. Without this, if the anonymous /features endpoint ever delivers this key as true, a personal/legacy_stripe workspace is silently routed to workspace billing before the per-user authenticated config confirms the flag. The guard also enforces if (!isCloud) return false at the resolver level, so OSS isolation does not depend on the if (!isCloud) return 'legacy' call-site guard in useBillingRouting.ts -- which PR #15051 (open, comfydesigner) removes.

Suggested fix: add export const cachedLegacyBillingMigrationEnabled = useStorage<boolean | undefined>('legacy_billing_migration_enabled', undefined) to remoteConfig.ts, then call resolveAuthGatedFlag(ServerFeatureFlag.LEGACY_BILLING_MIGRATION_ENABLED, remoteConfig.value.legacy_billing_migration_enabled, cachedLegacyBillingMigrationEnabled) here.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Addressed in 91b27ec. The migration flag now uses resolveAuthGatedFlag with a dedicated cached value. Authenticated refreshes persist the cache, anonymous refreshes leave it untouched, and tests cover cached bootstrap, authenticated remote config, server fallback, and the false default.

remoteConfig.value.legacy_billing_migration_enabled,
cachedLegacyBillingMigrationEnabled
)
},
get v1PaymentRecovery() {
return resolveAuthGatedFlag(
ServerFeatureFlag.V1_PAYMENT_RECOVERY,
Expand Down
Loading
Loading