Skip to content

feat(onboarding): auto-start trial and route by login choice#3222

Merged
ygrishajev merged 1 commit into
mainfrom
feat/onboarding-auto-start-trial-on-entry
May 27, 2026
Merged

feat(onboarding): auto-start trial and route by login choice#3222
ygrishajev merged 1 commit into
mainfrom
feat/onboarding-auto-start-trial-on-entry

Conversation

@ygrishajev
Copy link
Copy Markdown
Contributor

@ygrishajev ygrishajev commented May 26, 2026

Why

Fixes CON-365 — when a user enters the onboarding redesign at /onboarding, the trial should start automatically (no explicit "Start Trial" click) and the deploy should wait silently for the wallet to be ready.

What

Backend

  • WalletInitializerService.startTrial(userId) orchestrates the trial-start flow. The controller is a thin pass-through.
  • Stripe payment-method validation is skipped when console_onboarding_redesign is on; email-verified + fingerprint anti-abuse checks still run on both branches.

Frontend

  • New onboardingStore atom records which login flow the user took: /login marks legacy, /login-v2 marks redesign (only when the FF is on).
  • OnboardingRedirectEffect picks /onboarding only when atom is redesign AND the FF is on; falls back to legacy /signup otherwise. /onboarding is excluded from the redirect loop.
  • /onboarding and /login-v2 are SSR-gated on the FF.
  • useEnsureTrialStarted (single mount on the picker) fires the trial mutation. PhasedDeploymentContainer receives isWalletReady + trialError as props so usePhasedDeploymentFlow gates createDeployment until the wallet is ready and fails fast on a terminal trial error.
  • useCreateManagedWalletMutation retries up to 5 times with exponential backoff.
  • WalletProvider syncs to the managed-wallet network on first load via reload() so the switch never happens mid-deploy.

E2E

  • onboarding-quick-deploy now uses passwordless login with a fresh mailsac user and cleans the Auth0 user up afterEach.

Test plan

  • apps/api unit/integration specs green (wallet controller + wallet-initializer)
  • apps/deploy-web unit specs green (useEnsureTrialStarted, usePhasedDeploymentFlow, PhasedDeploymentContainer, OnboardingPickerPage, OnboardingRedirectEffect)
  • apps/deploy-web e2e: onboarding-quick-deploy.spec.ts
  • Manual smoke with FF off → legacy /signup flow still works
  • Manual smoke with FF on → fresh passwordless user lands on /onboarding, deploys hello world, no mid-flow reloads

Summary by CodeRabbit

  • New Features

    • Onboarding redesign available alongside legacy flow, with client/server gating and a persisted picker.
    • Background trial wallet setup auto-starts and surfaces a destructive error alert with refresh/support guidance when it fails.
    • Deployment flow now respects wallet readiness and trial errors to pause or show errors during phased deploy.
  • Bug Fixes

    • Enforced managed wallet network selection and reload to ensure correct network on mismatch.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 26, 2026

📝 Walkthrough

Walkthrough

This PR centralizes trial-start validation in WalletInitializerService (with optional 3DS), simplifies WalletController to delegate startTrial and shape responses, and adds frontend wallet-readiness/trial-error gating plus a feature-flag driven onboarding redesign route.

Changes

Trial Creation Refactor and Onboarding Redesign

Layer / File(s) Summary
Feature flags and result types
apps/api/src/core/services/feature-flags/feature-flags.ts, apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts
CONSOLE_ONBOARDING_REDESIGN flag constant and Requires3DSResult/StartTrialResult types establish the foundation for conditional payment validation and 3DS handling.
WalletInitializerService trial startup with payment validation
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts, apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts
startTrial(userId) enforces verified email, optional fingerprint de-duplication in production, and Stripe payment-method validation when the redesign flag is OFF; returns a Requires3DSResult when 3DS is required or initializes/grants trial limits otherwise; tests cover ON/OFF branches and error cases.
WalletController delegates trial creation to service
apps/api/src/billing/controllers/wallet/wallet.controller.ts, apps/api/src/billing/controllers/wallet/wallet.controller.spec.ts
Controller's create() now calls walletInitializer.startTrial(userId), then augments the returned payload with denom (DEPLOYMENT_GRANT_DENOM) and topUpMinAmountUsd from TrialValidationService; specs assert delegation, augmentation for both normal and 3DS results, and error propagation.
useEnsureTrialStarted hook for wallet initialization
apps/deploy-web/src/hooks/useEnsureTrialStarted.ts, apps/deploy-web/src/hooks/useEnsureTrialStarted.spec.ts
New hook computes isWalletReady from wallet?.address, triggers create() when appropriate, exposes isWalletReady, isLoading, and error, and stops retrying after a terminal create error; tests validate firing conditions and state.
Phased deployment flow: wallet-ready and trial-error gating
apps/deploy-web/src/hooks/usePhasedDeploymentFlow/usePhasedDeploymentFlow.ts, apps/deploy-web/src/hooks/usePhasedDeploymentFlow/usePhasedDeploymentFlow.spec.ts, apps/deploy-web/src/components/deployments/PhasedDeploymentContainer/PhasedDeploymentContainer.tsx, apps/deploy-web/src/components/deployments/PhasedDeploymentContainer/PhasedDeploymentContainer.spec.tsx
usePhasedDeploymentFlow now accepts isWalletReady and trialError, pauses/blocks createDeployment when wallet not ready, and transitions to error when a terminal trialError exists; container forwards props.
OnboardingPickerPage: trial initialization and error display
apps/deploy-web/src/components/onboarding-picker/OnboardingPickerPage.tsx, apps/deploy-web/src/components/onboarding-picker/OnboardingPickerPage.spec.tsx
Page consumes useEnsureTrialStarted, passes isWalletReady/trialError into PhasedDeploymentContainer, and renders a destructive Alert when trial setup failed; tests added for error/no-error.
Onboarding flow state and routing infrastructure
apps/deploy-web/src/store/onboardingStore.ts, apps/deploy-web/src/utils/urlUtils.ts
Adds persisted Jotai selectedOnboardingFlow atom ("legacy"
OnboardingRedirectEffect: conditional redesign routing
apps/deploy-web/src/components/onboarding/OnboardingRedirectEffect/OnboardingRedirectEffect.tsx, apps/deploy-web/src/components/onboarding/OnboardingRedirectEffect/OnboardingRedirectEffect.spec.tsx
Reads selectedOnboardingFlow and console_onboarding_redesign flag; redirects to onboarding picker (/onboarding) when redesign is selected and enabled, otherwise falls back to legacy /signup?return-to=...; excludes /onboarding prefix.
Login pages set onboarding flow state
apps/deploy-web/src/pages/login/index.tsx, apps/deploy-web/src/pages/login-v2/index.tsx
Login sets flow to "legacy"; LoginV2 sets flow to "redesign" when flag enabled and gates server access to LoginV2 based on the feature flag.
Onboarding page feature flag gate
apps/deploy-web/src/pages/onboarding/index.tsx
getServerSideProps now requires both authentication and console_onboarding_redesign feature flag; redirects to / when either check fails.
WalletProvider network enforcement and mutation retry
apps/deploy-web/src/context/WalletProvider/WalletProvider.tsx, apps/deploy-web/src/queries/useManagedWalletQuery.ts
WalletProvider now enforces the managed network id and reloads when changed; create-managed-wallet mutation uses conditional retries with exponential backoff (capped).
E2E test: passwordless onboarding redesign flow
apps/deploy-web/tests/ui/onboarding-quick-deploy.spec.ts
E2E spec switched to passwordless onboarding, verifies OTP/auth0 user creation and cleanup, checks background trial provisioning shows no error alert, and deploys template through onboarding picker.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

  • akash-network/console#3204: The main PR’s additions of isWalletReady/trialError into usePhasedDeploymentFlow and downstream PhasedDeploymentContainer are code-level extensions to the phased deploy flow introduced in the retrieved PR.
  • akash-network/console#3211: Enforces topUpMinAmountUsd trial-minimum via StripeController and TrialValidationService; this PR propagates the same logic into WalletController responses and wallet initialization flow.

Suggested reviewers

  • stalniy
  • baktun14
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/onboarding-auto-start-trial-on-entry

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 26, 2026

Codecov Report

❌ Patch coverage is 71.76471% with 24 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.17%. Comparing base (6d702d9) to head (c466754).
⚠️ Report is 5 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
apps/deploy-web/src/pages/login-v2/index.tsx 0.00% 6 Missing and 2 partials ⚠️
apps/deploy-web/src/pages/login/index.tsx 0.00% 4 Missing ⚠️
...s/wallet-initializer/wallet-initializer.service.ts 91.89% 2 Missing and 1 partial ⚠️
...-web/src/context/WalletProvider/WalletProvider.tsx 0.00% 2 Missing and 1 partial ⚠️
apps/deploy-web/src/pages/onboarding/index.tsx 0.00% 2 Missing ⚠️
...ps/deploy-web/src/queries/useManagedWalletQuery.ts 0.00% 2 Missing ⚠️
...oardingRedirectEffect/OnboardingRedirectEffect.tsx 85.71% 1 Missing ⚠️
apps/deploy-web/src/utils/urlUtils.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3222      +/-   ##
==========================================
- Coverage   66.41%   65.17%   -1.25%     
==========================================
  Files        1061      979      -82     
  Lines       26013    24080    -1933     
  Branches     6253     5873     -380     
==========================================
- Hits        17277    15694    -1583     
+ Misses       7622     7303     -319     
+ Partials     1114     1083      -31     
Flag Coverage Δ *Carryforward flag
api 84.70% <92.10%> (+<0.01%) ⬆️
deploy-web 50.57% <55.31%> (+<0.01%) ⬆️
log-collector ?
notifications 91.06% <ø> (ø) Carriedforward from 6d702d9
provider-console 81.48% <ø> (ø) Carriedforward from 6d702d9
provider-inventory ?
provider-proxy 86.08% <ø> (ø) Carriedforward from 6d702d9
tx-signer ?

*This pull request uses carry forward flags. Click here to find out more.

Files with missing lines Coverage Δ
...rc/billing/controllers/wallet/wallet.controller.ts 100.00% <100.00%> (+1.72%) ⬆️
...i/src/core/services/feature-flags/feature-flags.ts 100.00% <ø> (ø)
...dDeploymentContainer/PhasedDeploymentContainer.tsx 100.00% <ø> (ø)
...ponents/onboarding-picker/OnboardingPickerPage.tsx 100.00% <100.00%> (ø)
apps/deploy-web/src/hooks/useEnsureTrialStarted.ts 100.00% <100.00%> (ø)
...usePhasedDeploymentFlow/usePhasedDeploymentFlow.ts 98.97% <100.00%> (+0.05%) ⬆️
apps/deploy-web/src/store/onboardingStore.ts 100.00% <100.00%> (ø)
...oardingRedirectEffect/OnboardingRedirectEffect.tsx 94.44% <85.71%> (-5.56%) ⬇️
apps/deploy-web/src/utils/urlUtils.ts 56.41% <0.00%> (-0.74%) ⬇️
apps/deploy-web/src/pages/onboarding/index.tsx 0.00% <0.00%> (ø)
... and 5 more

... and 88 files with indirect coverage changes

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 5

🧹 Nitpick comments (2)
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts (1)

80-82: 💤 Low value

Mixed test framework APIs: jest.fn() in a Vitest test file.

The test uses jest.fn() instead of Vitest's native vi.fn(). While this may work due to Vitest's Jest compatibility, it's inconsistent with the Vitest conventions used elsewhere in the codebase.

♻️ Suggested fix
-          getOrCreateWallet: jest.fn().mockResolvedValue({ wallet: newWallet, isNew: true }),
-          updateWalletById: jest.fn().mockResolvedValue(newWallet)
+          getOrCreateWallet: vi.fn().mockResolvedValue({ wallet: newWallet, isNew: true }),
+          updateWalletById: vi.fn().mockResolvedValue(newWallet)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts`
around lines 80 - 82, Replace the Jest mock helper with Vitest's API: in the
test where the mocked service object defines getOrCreateWallet and
updateWalletById (the object with getOrCreateWallet:
jest.fn().mockResolvedValue(...) and updateWalletById:
jest.fn().mockResolvedValue(...)), change both jest.fn() calls to vi.fn() so the
test uses Vitest's native mocking (keep the mockResolvedValue calls intact).
apps/api/src/billing/controllers/wallet/wallet.controller.spec.ts (1)

114-154: ⚡ Quick win

Using rootContainer directly without cleanup.

The setup function registers mocks on rootContainer without calling rootContainer.clearInstances() between tests. This could cause test pollution if tests run sequentially in the same process. Consider either using container.createChildContainer() for isolation or adding cleanup in afterEach.

♻️ Suggested fix using child container
  function setup(input?: {
    user?: UserOutput;
    wallets?: UserWalletPublicOutput[];
    startTrialResult?: Awaited<ReturnType<WalletInitializerService["startTrial"]>>;
    startTrialError?: Error;
  }) {
+   const container = rootContainer.createChildContainer();
    const startTrial = jest.fn();
    if (input?.startTrialError) {
      startTrial.mockRejectedValue(input.startTrialError);
    } else if (input?.startTrialResult) {
      startTrial.mockResolvedValue(input.startTrialResult);
    }

-   rootContainer.register(AuthService, {
+   container.register(AuthService, {
      useValue: mock<AuthService>({
        ability: createMongoAbility<MongoAbility>([{ action: "create", subject: "UserWallet" }]),
        currentUser: input?.user ?? createUser()
      })
    });
    // ... apply same change to all other register calls ...

-   return rootContainer;
+   return container;
  }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/api/src/billing/controllers/wallet/wallet.controller.spec.ts` around
lines 114 - 154, The tests call setup() and register mocks directly on
rootContainer which can leak between tests; modify setup to create and return a
child container (e.g., const container = rootContainer.createChildContainer())
and register all services (AuthService, WalletInitializerService,
BillingConfigService, ManagedSignerService, RefillService, WalletReaderService,
BalancesService, UserWalletRepository, TrialValidationService) on that child,
then have the test suite use/return that child container; alternatively, add an
afterEach that calls rootContainer.clearInstances() to remove registrations—pick
one approach and apply it consistently so each test runs with an isolated
container.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@apps/deploy-web/src/components/onboarding-picker/OnboardingPickerPage.spec.tsx`:
- Around line 63-67: Replace the unsafe "as unknown as" double-casts in
OnboardingPickerPage.spec.tsx by providing correctly typed mock factories:
change the mock for useEnsureTrialStarted to use vi.fn<typeof
DEPENDENCIES.useEnsureTrialStarted>(...) and similarly replace the casts for
useSnackbar and useRouter with vi.fn<typeof DEPENDENCIES.useSnackbar>(...) and
vi.fn<typeof DEPENDENCIES.useRouter>(...) respectively (locations referencing
useEnsureTrialStarted, useSnackbar, useRouter in the spec). Ensure the vi.fn
generic wraps the existing mock implementation so the mocks are strongly typed
instead of being cast.

In
`@apps/deploy-web/src/hooks/usePhasedDeploymentFlow/usePhasedDeploymentFlow.ts`:
- Line 74: The hook usePhasedDeploymentFlow currently treats an omitted optional
isWalletReady as false, causing the flow to get stuck at "creating"; update the
function signature or parameter destructuring for usePhasedDeploymentFlow
(Options -> isWalletReady) to default isWalletReady to true when undefined
(e.g., set isWalletReady = true in the parameter defaults) so the existing guard
if (!isWalletReady) return; only blocks when explicitly false rather than when
omitted.

In `@apps/deploy-web/src/queries/useManagedWalletQuery.ts`:
- Around line 27-28: The retry predicate in useManagedWalletQuery (the options
lines with retry: failureCount => failureCount < 5) currently allows only 4
retries (5 total attempts); if you intended "retries up to 5 times" change the
predicate to allow 5 retries by updating the expression to retry: failureCount
=> failureCount <= 5 (or failureCount < 6) so the query will attempt the initial
call plus five retries; leave retryDelay as-is.

In `@apps/deploy-web/tests/ui/onboarding-quick-deploy.spec.ts`:
- Around line 47-49: The current assertion using test.step "trial provisions in
background — no error alert" checks page.getByText(/We couldn't set up your
trial/i) immediately and can miss late alerts; instead, wait for a concrete
readiness milestone (for example await the deploy-start signal or a UI element
that indicates trial initialization finished such as the deploy button or a
success banner) before asserting absence of the error text. Update the test.step
to first await that readiness marker (e.g., await page.waitForSelector or await
page.getByRole/getByText for the known readiness element), then assert
page.getByText(/We couldn't set up your trial/i).toHaveCount(0) so the check
observes a real completion window rather than an instantaneous snapshot.
- Around line 14-20: The teardown currently uses the shared variable testUserId
when calling auth0.deleteUser so if delete throws a non-404 the shared
testUserId is not cleared and leaks into subsequent tests; change the cleanup in
test.afterEach to copy testUserId to a local const (e.g., const idToDelete =
testUserId), then immediately set testUserId = undefined before awaiting
auth0.deleteUser(idToDelete). Keep the existing catch that ignores
ManagementApiError 404 (ManagementApiError and error.statusCode checks) but
ensure the shared testUserId is cleared regardless of delete outcome.

---

Nitpick comments:
In `@apps/api/src/billing/controllers/wallet/wallet.controller.spec.ts`:
- Around line 114-154: The tests call setup() and register mocks directly on
rootContainer which can leak between tests; modify setup to create and return a
child container (e.g., const container = rootContainer.createChildContainer())
and register all services (AuthService, WalletInitializerService,
BillingConfigService, ManagedSignerService, RefillService, WalletReaderService,
BalancesService, UserWalletRepository, TrialValidationService) on that child,
then have the test suite use/return that child container; alternatively, add an
afterEach that calls rootContainer.clearInstances() to remove registrations—pick
one approach and apply it consistently so each test runs with an isolated
container.

In
`@apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts`:
- Around line 80-82: Replace the Jest mock helper with Vitest's API: in the test
where the mocked service object defines getOrCreateWallet and updateWalletById
(the object with getOrCreateWallet: jest.fn().mockResolvedValue(...) and
updateWalletById: jest.fn().mockResolvedValue(...)), change both jest.fn() calls
to vi.fn() so the test uses Vitest's native mocking (keep the mockResolvedValue
calls intact).
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: aad49133-db5d-4eef-b6cd-2eb5e1836665

📥 Commits

Reviewing files that changed from the base of the PR and between 6d702d9 and 5870fe0.

📒 Files selected for processing (23)
  • apps/api/src/billing/controllers/wallet/wallet.controller.spec.ts
  • apps/api/src/billing/controllers/wallet/wallet.controller.ts
  • apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts
  • apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts
  • apps/api/src/core/services/feature-flags/feature-flags.ts
  • apps/deploy-web/src/components/deployments/PhasedDeploymentContainer/PhasedDeploymentContainer.spec.tsx
  • apps/deploy-web/src/components/deployments/PhasedDeploymentContainer/PhasedDeploymentContainer.tsx
  • apps/deploy-web/src/components/onboarding-picker/OnboardingPickerPage.spec.tsx
  • apps/deploy-web/src/components/onboarding-picker/OnboardingPickerPage.tsx
  • apps/deploy-web/src/components/onboarding/OnboardingRedirectEffect/OnboardingRedirectEffect.spec.tsx
  • apps/deploy-web/src/components/onboarding/OnboardingRedirectEffect/OnboardingRedirectEffect.tsx
  • apps/deploy-web/src/context/WalletProvider/WalletProvider.tsx
  • apps/deploy-web/src/hooks/useEnsureTrialStarted.spec.ts
  • apps/deploy-web/src/hooks/useEnsureTrialStarted.ts
  • apps/deploy-web/src/hooks/usePhasedDeploymentFlow/usePhasedDeploymentFlow.spec.ts
  • apps/deploy-web/src/hooks/usePhasedDeploymentFlow/usePhasedDeploymentFlow.ts
  • apps/deploy-web/src/pages/login-v2/index.tsx
  • apps/deploy-web/src/pages/login/index.tsx
  • apps/deploy-web/src/pages/onboarding/index.tsx
  • apps/deploy-web/src/queries/useManagedWalletQuery.ts
  • apps/deploy-web/src/store/onboardingStore.ts
  • apps/deploy-web/src/utils/urlUtils.ts
  • apps/deploy-web/tests/ui/onboarding-quick-deploy.spec.ts

Comment thread apps/deploy-web/src/components/onboarding-picker/OnboardingPickerPage.spec.tsx Outdated
Comment thread apps/deploy-web/src/queries/useManagedWalletQuery.ts Outdated
Comment thread apps/deploy-web/tests/ui/onboarding-quick-deploy.spec.ts Outdated
Comment thread apps/deploy-web/tests/ui/onboarding-quick-deploy.spec.ts
…login choice

Backend
- Move trial-start orchestration into WalletInitializerService.startTrial; the
  controller becomes a thin pass-through that wraps the result with denom +
  topUpMinAmountUsd.
- Skip stripe payment-method validation when the console_onboarding_redesign FF
  is on; keep email-verified and fingerprint anti-abuse checks on both branches.

Frontend
- Add an onboardingStore atom recording which login flow the user took. /login
  marks legacy, /login-v2 marks redesign only when the FF is enabled.
- OnboardingRedirectEffect picks /onboarding only when the atom is "redesign"
  AND the FF is on; falls back to legacy /signup otherwise. /onboarding is now
  excluded from the redirect loop.
- /onboarding and /login-v2 are SSR-gated on the FF.
- useEnsureTrialStarted lives at the picker page only and fires the create
  mutation; PhasedDeploymentContainer receives isWalletReady + trialError as
  props so usePhasedDeploymentFlow gates createDeployment until the wallet has
  an address and fails fast on a terminal trial error.
- useCreateManagedWalletMutation retries up to 5 times with exponential backoff.
- WalletProvider syncs to the managed-wallet network on first load (reload
  instead of nav-to-home) so the switch never happens mid-deploy.

E2E
- onboarding-quick-deploy now uses passwordless login with a fresh mailsac user
  and cleans the Auth0 user up afterEach.
@ygrishajev ygrishajev force-pushed the feat/onboarding-auto-start-trial-on-entry branch from 5870fe0 to c466754 Compare May 26, 2026 12:16
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
apps/api/src/billing/controllers/wallet/wallet.controller.spec.ts (1)

114-153: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Test isolation: rootContainer modifications persist between tests.

The setup function registers mocks directly on rootContainer without cleanup, causing state to leak across tests. This can lead to flaky tests when run together. The service spec uses container.createChildContainer() for isolation.

Proposed fix using child container
  function setup(input?: {
    user?: UserOutput;
    wallets?: UserWalletPublicOutput[];
    startTrialResult?: Awaited<ReturnType<WalletInitializerService["startTrial"]>>;
    startTrialError?: Error;
  }) {
+   const container = rootContainer.createChildContainer();
    const startTrial = jest.fn();
    if (input?.startTrialError) {
      startTrial.mockRejectedValue(input.startTrialError);
    } else if (input?.startTrialResult) {
      startTrial.mockResolvedValue(input.startTrialResult);
    }

-   rootContainer.register(AuthService, {
+   container.register(AuthService, {
      useValue: mock<AuthService>({
        ability: createMongoAbility<MongoAbility>([{ action: "create", subject: "UserWallet" }]),
        currentUser: input?.user ?? createUser()
      })
    });
-   rootContainer.register(WalletInitializerService, {
+   container.register(WalletInitializerService, {
      useValue: mock<WalletInitializerService>({ startTrial })
    });
    // ... apply same pattern to remaining registrations ...

-   return rootContainer;
+   return container;
  }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/api/src/billing/controllers/wallet/wallet.controller.spec.ts` around
lines 114 - 153, The setup function currently registers mocks directly on
rootContainer causing test state leakage; change setup to create an isolated
child container (e.g., const container = rootContainer.createChildContainer())
and register all mocks (AuthService, WalletInitializerService,
BillingConfigService, ManagedSignerService, RefillService, WalletReaderService,
BalancesService, UserWalletRepository, TrialValidationService) on that child
container instead of rootContainer, return the child container so each test gets
its own isolated container; ensure any test code that calls setup uses the
returned child container for resolving controllers/services.
♻️ Duplicate comments (1)
apps/deploy-web/src/queries/useManagedWalletQuery.ts (1)

27-28: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Retry budget is lower than intended for trial wallet creation.

At Line 27, failureCount < 3 allows only 2 retries (3 total attempts). That conflicts with the PR objective of retrying up to 5 times and weakens resilience on transient failures.

Suggested fix
-    retry: failureCount => failureCount < 3,
+    retry: failureCount => failureCount <= 5,
     retryDelay: attempt => Math.min(1000 * 2 ** attempt, 30_000),
In `@tanstack/react-query` v5, for useMutation retry callback `retry: (failureCount, error) => ...`, does `failureCount` start at 1 on the first failed attempt, and therefore does `failureCount < 3` mean only 2 retries after the initial attempt?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/deploy-web/src/queries/useManagedWalletQuery.ts` around lines 27 - 28,
The retry budget is too low in the useManagedWalletQuery mutation config: update
the retry callback used in useManagedWalletQuery so it permits up to 5 failed
attempts (i.e., 5 retries/attempts total) by changing the retry predicate to
check for failureCount < 5 (or use the v5 signature retry: (failureCount, error)
=> failureCount < 5) while leaving retryDelay as-is; this ensures the mutation
will retry up to the intended count on transient failures.
🧹 Nitpick comments (1)
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts (1)

37-42: 💤 Low value

Type casting violates test guidelines.

Per coding guidelines, use mock<T>() instead of as unknown as <Type>. For nullable fields, consider adjusting the seeder or using a partial type.

Suggested approach
- const user = createUser({ emailVerified: true, stripeCustomerId: null as unknown as string });
+ const user = createUser({ emailVerified: true, stripeCustomerId: undefined });

Or update the createUser seeder to accept null for optional fields if the type allows it.

As per coding guidelines: "Use mock<T>() instead of as unknown as <Type> for type casting in tests"

Also applies to: 121-123

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts`
around lines 37 - 42, Replace the unsafe type cast in the test that passes
stripeCustomerId with a proper test mock or seeder change: instead of using
"stripeCustomerId: null as unknown as string" in the createUser call, use the
test helper mock<T>() to create a User stub with stripeCustomerId set to null
(or update the createUser seeder to accept null for optional fields), then call
WalletInitializerService.startTrial(user.id) as before; locate this in the spec
around the createUser usage and also fix the similar casts at the other
occurrences (lines referenced in the review).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts`:
- Around line 45-57: The startTrial method validates authService.currentUser
(emailVerified, fingerprint, payment) but then calls
initializeAndGrantTrialLimits(userId), which can create a trial for a different
user; either assert userId matches currentUser.id at the start of startTrial
(e.g., throw/ assert if userId !== currentUser.id) or stop using the userId
parameter and call
initializeAndGrantTrialLimits(this.authService.currentUser.id) directly; update
the startTrial signature/ call sites accordingly and keep references to
startTrial, initializeAndGrantTrialLimits, and authService.currentUser when
making the change.

In `@apps/deploy-web/src/pages/login/index.tsx`:
- Around line 22-27: The client-only useEffect markOnboardingFlowAsLegacy
calling setSelectedOnboardingFlow("legacy") can be skipped when
getServerSideProps redirects (console_embedded_login off), leaving a persisted
atomWithStorage value ("redesign") that misroutes via OnboardingRedirectEffect;
fix by setting or clearing the onboardingStore.selectedOnboardingFlow marker
during the guaranteed server-to-client handoff — e.g., write a cookie or add a
query flag in getServerSideProps when redirecting from the login page and then
read that flag in the login page entry point to call
setSelectedOnboardingFlow("legacy") (or clear the atom) so the legacy marker is
applied even when the client useEffect never runs.

---

Outside diff comments:
In `@apps/api/src/billing/controllers/wallet/wallet.controller.spec.ts`:
- Around line 114-153: The setup function currently registers mocks directly on
rootContainer causing test state leakage; change setup to create an isolated
child container (e.g., const container = rootContainer.createChildContainer())
and register all mocks (AuthService, WalletInitializerService,
BillingConfigService, ManagedSignerService, RefillService, WalletReaderService,
BalancesService, UserWalletRepository, TrialValidationService) on that child
container instead of rootContainer, return the child container so each test gets
its own isolated container; ensure any test code that calls setup uses the
returned child container for resolving controllers/services.

---

Duplicate comments:
In `@apps/deploy-web/src/queries/useManagedWalletQuery.ts`:
- Around line 27-28: The retry budget is too low in the useManagedWalletQuery
mutation config: update the retry callback used in useManagedWalletQuery so it
permits up to 5 failed attempts (i.e., 5 retries/attempts total) by changing the
retry predicate to check for failureCount < 5 (or use the v5 signature retry:
(failureCount, error) => failureCount < 5) while leaving retryDelay as-is; this
ensures the mutation will retry up to the intended count on transient failures.

---

Nitpick comments:
In
`@apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts`:
- Around line 37-42: Replace the unsafe type cast in the test that passes
stripeCustomerId with a proper test mock or seeder change: instead of using
"stripeCustomerId: null as unknown as string" in the createUser call, use the
test helper mock<T>() to create a User stub with stripeCustomerId set to null
(or update the createUser seeder to accept null for optional fields), then call
WalletInitializerService.startTrial(user.id) as before; locate this in the spec
around the createUser usage and also fix the similar casts at the other
occurrences (lines referenced in the review).
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 61cd6c23-4670-44f9-bfdd-7acbfd33db6e

📥 Commits

Reviewing files that changed from the base of the PR and between 5870fe0 and c466754.

📒 Files selected for processing (23)
  • apps/api/src/billing/controllers/wallet/wallet.controller.spec.ts
  • apps/api/src/billing/controllers/wallet/wallet.controller.ts
  • apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts
  • apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts
  • apps/api/src/core/services/feature-flags/feature-flags.ts
  • apps/deploy-web/src/components/deployments/PhasedDeploymentContainer/PhasedDeploymentContainer.spec.tsx
  • apps/deploy-web/src/components/deployments/PhasedDeploymentContainer/PhasedDeploymentContainer.tsx
  • apps/deploy-web/src/components/onboarding-picker/OnboardingPickerPage.spec.tsx
  • apps/deploy-web/src/components/onboarding-picker/OnboardingPickerPage.tsx
  • apps/deploy-web/src/components/onboarding/OnboardingRedirectEffect/OnboardingRedirectEffect.spec.tsx
  • apps/deploy-web/src/components/onboarding/OnboardingRedirectEffect/OnboardingRedirectEffect.tsx
  • apps/deploy-web/src/context/WalletProvider/WalletProvider.tsx
  • apps/deploy-web/src/hooks/useEnsureTrialStarted.spec.ts
  • apps/deploy-web/src/hooks/useEnsureTrialStarted.ts
  • apps/deploy-web/src/hooks/usePhasedDeploymentFlow/usePhasedDeploymentFlow.spec.ts
  • apps/deploy-web/src/hooks/usePhasedDeploymentFlow/usePhasedDeploymentFlow.ts
  • apps/deploy-web/src/pages/login-v2/index.tsx
  • apps/deploy-web/src/pages/login/index.tsx
  • apps/deploy-web/src/pages/onboarding/index.tsx
  • apps/deploy-web/src/queries/useManagedWalletQuery.ts
  • apps/deploy-web/src/store/onboardingStore.ts
  • apps/deploy-web/src/utils/urlUtils.ts
  • apps/deploy-web/tests/ui/onboarding-quick-deploy.spec.ts
✅ Files skipped from review due to trivial changes (1)
  • apps/api/src/core/services/feature-flags/feature-flags.ts

Comment thread apps/deploy-web/src/pages/login/index.tsx
Comment thread apps/deploy-web/src/pages/login-v2/index.tsx
@ygrishajev ygrishajev enabled auto-merge May 27, 2026 07:31
@ygrishajev ygrishajev added this pull request to the merge queue May 27, 2026
Merged via the queue into main with commit 97ef273 May 27, 2026
56 of 59 checks passed
@ygrishajev ygrishajev deleted the feat/onboarding-auto-start-trial-on-entry branch May 27, 2026 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants