refactor(wallet): managed-only context with wallet boot gate - #3567
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe deploy-web wallet model now uses ChangesWallet model and application migration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
apps/deploy-web/src/components/deployments/DeploymentListRow.tsxESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. apps/deploy-web/src/components/get-started/GetStartedStepper.spec.tsxESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. apps/deploy-web/src/components/get-started/GetStartedStepper.tsxESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox.
Comment |
77b2e3f to
b33b72d
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is a large, cross-cutting refactor (72 files) that changes core wallet-boot behavior — the app now blocks behind a boot overlay on every page until the wallet lookup settles, and the WalletProvider context API is reshaped for ~20 consumers. No bugs were found by the automated review, but the scope and behavioral change to app boot warrant a human look.
Extended reasoning...
Overview
The PR removes the custodial-era wallet surface (isManaged, isWalletLoaded, isWalletLoading, isWalletInitializing, walletName, logout, isOnboarding, useIsManagedWalletUser) from WalletProvider and replaces it with a slimmer hasWallet/isWalletCreating/createWallet API. It also moves wallet-loading logic out of roughly 20 individual consumers and into a single boot gate in WalletProvider that renders BootLoading instead of children until the initial wallet-existence lookup settles. Dead custodial code (ACT auto-mint path, useMintACT, unused apiUtils builders, the Guard/composeGuards HOC, etc.) is deleted, and one visible copy fix (literal double-dollar-sign in the escrow alert) is included.
Security risks
No injection, auth-bypass, or data-exposure concerns — this is UI/context wiring, not auth or crypto logic. The removed Guard HOC on /billing, /usage, /payment-methods was a no-op (useIsManagedWalletUser always resolved true for a managed-only app), so removing it doesn't change access control.
Level of scrutiny
This warrants a human pass despite finding no bugs: it changes the loading/boot sequence for the entire app (every registered user now waits on a new full-screen gate on every page load), and it touches a large number of call sites where subtle behavioral differences (dropped isManaged branches, merged isWalletConnected/hasManagedWallet into hasWallet) could regress edge cases not fully exercised by unit tests. This is large-scale design-decision territory rather than a mechanical rename.
Other factors
Test coverage looks substantial (a new WalletProvider.spec.tsx with 13 cases covering the gate, plus updated specs across nearly every touched consumer), and the PR description documents manual verification (unit tests, lint, type-check, and a headless boot-overlay smoke test). The PR itself notes it is stacked on #3563 and needs retargeting/rebasing to main, which is worth confirming before merge.
b33b72d to
963b86c
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3567 +/- ##
==========================================
- Coverage 75.90% 75.35% -0.55%
==========================================
Files 1159 1067 -92
Lines 30270 27708 -2562
Branches 7529 6988 -541
==========================================
- Hits 22976 20879 -2097
+ Misses 6432 6020 -412
+ Partials 862 809 -53
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/deploy-web/src/components/onboarding/steps/PaymentMethodsDisplay/PaymentMethodsDisplay.test.tsx (1)
314-321: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winConvert this test to Vitest.
Replace
jest.Mockand other Jest APIs withMock,vi.fn, andviAPIs fromvitest.🤖 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/components/onboarding/steps/PaymentMethodsDisplay/PaymentMethodsDisplay.test.tsx` around lines 314 - 321, Convert the PaymentMethodsDisplay test setup and all related test code from Jest to Vitest: replace jest.Mock types with Vitest’s Mock type, jest.fn with vi.fn, and any other Jest APIs with their vi equivalents, importing the required symbols from vitest while preserving existing test behavior.Source: Coding guidelines
🤖 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/deployments/DeploymentListRow.tsx`:
- Around line 226-232: Update the spent amount conversion in the
DeploymentListRow rendering to call udenomToDenom with precision 6, matching the
escrowBalanceInDenom conversion used for Balance. Keep the existing amountSpent
fallback and PriceValue rendering unchanged.
In `@apps/deploy-web/src/components/get-started/GetStartedStepper.tsx`:
- Around line 132-134: Remove the legacy ACT readiness guidance around the
GetStartedStepper balance message, including the ACT-related condition and
acquire-ACT warning. Ensure readiness is derived only from the managed-wallet
funding balance (such as USDC), so a funded wallet without ACT is treated as
ready; otherwise remove the obsolete block entirely.
In
`@apps/deploy-web/src/components/home/AccountStatsCards/AccountStatsCards.spec.tsx`:
- Around line 19-23: Update the test case “renders deployment escrow USD” to
assert the complete formatted value “$42.00 used in deployments” in the rendered
output, ensuring totalDeploymentEscrowUSD is actually reflected rather than
checking only the static suffix.
In `@apps/deploy-web/src/hooks/useIsOnboarded.ts`:
- Around line 16-21: The useIsOnboarded hook currently converts an unresolved or
failed useLeaseExistenceQuery into false, potentially triggering premature
onboarding redirects. Update useIsOnboarded and its routing consumers to
preserve loading and error states, ensuring redirects wait for a settled query
and lease lookup errors follow useOnboardingChrome’s fail-open behavior.
In `@apps/deploy-web/src/utils/walletUtils.ts`:
- Line 52: Remove the duplicate ManagedWalletUpdate type alias declaration in
walletUtils.ts, keeping exactly one definition of the existing type so
TypeScript compilation succeeds.
---
Outside diff comments:
In
`@apps/deploy-web/src/components/onboarding/steps/PaymentMethodsDisplay/PaymentMethodsDisplay.test.tsx`:
- Around line 314-321: Convert the PaymentMethodsDisplay test setup and all
related test code from Jest to Vitest: replace jest.Mock types with Vitest’s
Mock type, jest.fn with vi.fn, and any other Jest APIs with their vi
equivalents, importing the required symbols from vitest while preserving
existing test behavior.
🪄 Autofix
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: 3f665a90-c34a-42ab-9bf5-0e4c6cfd5078
📒 Files selected for processing (72)
apps/deploy-web/src/components/api-keys/ApiKeyList.tsxapps/deploy-web/src/components/deployments/ConfigureDeployment/ConfigurationPane/HardwareSection/useTrialGate/useTrialGate.spec.tsapps/deploy-web/src/components/deployments/ConfigureDeployment/ConfigurationPane/HardwareSection/useTrialGate/useTrialGate.tsapps/deploy-web/src/components/deployments/DeploymentDepositModal/DeploymentDepositModal.spec.tsxapps/deploy-web/src/components/deployments/DeploymentDepositModal/DeploymentDepositModal.tsxapps/deploy-web/src/components/deployments/DeploymentDetail.tsxapps/deploy-web/src/components/deployments/DeploymentDetailTopBar/DeploymentDetailTopBar.spec.tsxapps/deploy-web/src/components/deployments/DeploymentList.tsxapps/deploy-web/src/components/deployments/DeploymentListRow.tsxapps/deploy-web/src/components/deployments/ManifestUpdate/ManifestUpdate.spec.tsxapps/deploy-web/src/components/get-started/GetStartedStepper.spec.tsxapps/deploy-web/src/components/get-started/GetStartedStepper.tsxapps/deploy-web/src/components/home/AccountHeader.tsxapps/deploy-web/src/components/home/AccountStatsCards/AccountStatsCards.spec.tsxapps/deploy-web/src/components/home/AccountStatsCards/AccountStatsCards.tsxapps/deploy-web/src/components/home/HomeContainer.tsxapps/deploy-web/src/components/home/YourAccount/YourAccount.spec.tsxapps/deploy-web/src/components/home/YourAccount/YourAccount.tsxapps/deploy-web/src/components/layout/Layout.spec.tsxapps/deploy-web/src/components/layout/Layout.tsxapps/deploy-web/src/components/layout/TransactionModal.tsxapps/deploy-web/src/components/layout/WalletStatus.spec.tsxapps/deploy-web/src/components/layout/WalletStatus.tsxapps/deploy-web/src/components/new-deployment/CreateLease/CreateLease.spec.tsxapps/deploy-web/src/components/new-deployment/CreateLease/CreateLease.tsxapps/deploy-web/src/components/new-deployment/ManifestEdit/ManifestEdit.spec.tsxapps/deploy-web/src/components/new-deployment/ManifestEdit/ManifestEdit.tsxapps/deploy-web/src/components/new-deployment/NewDeploymentContainer/NewDeploymentContainer.tsxapps/deploy-web/src/components/new-deployment/SdlBuilder.spec.tsxapps/deploy-web/src/components/onboarding/OnboardingContainer/OnboardingContainer.spec.tsxapps/deploy-web/src/components/onboarding/OnboardingContainer/OnboardingContainer.tsxapps/deploy-web/src/components/onboarding/RequireOnboarding/RequireOnboarding.spec.tsxapps/deploy-web/src/components/onboarding/RequireOnboarding/RequireOnboarding.tsxapps/deploy-web/src/components/onboarding/steps/PaymentMethodContainer/PaymentMethodContainer.spec.tsxapps/deploy-web/src/components/onboarding/steps/PaymentMethodContainer/PaymentMethodContainer.tsxapps/deploy-web/src/components/onboarding/steps/PaymentMethodStep/PaymentMethodStep.tsxapps/deploy-web/src/components/onboarding/steps/PaymentMethodsDisplay/PaymentMethodsDisplay.test.tsxapps/deploy-web/src/components/onboarding/steps/PaymentMethodsDisplay/PaymentMethodsDisplay.tsxapps/deploy-web/src/components/sdl/DeploymentMinimumEscrowAlertText.spec.tsxapps/deploy-web/src/components/sdl/DeploymentMinimumEscrowAlertText.tsxapps/deploy-web/src/context/PaymentPollingProvider/PaymentPollingProvider.spec.tsxapps/deploy-web/src/context/WalletProvider/WalletProvider.spec.tsxapps/deploy-web/src/context/WalletProvider/WalletProvider.tsxapps/deploy-web/src/context/WalletProvider/index.tsapps/deploy-web/src/hoc/guard/guard.hoc.tsxapps/deploy-web/src/hooks/useHasFundingBanner.spec.tsapps/deploy-web/src/hooks/useHasFundingBanner.tsapps/deploy-web/src/hooks/useIsOnboarded.spec.tsapps/deploy-web/src/hooks/useIsOnboarded.tsapps/deploy-web/src/hooks/useManagedDeploymentConfirm.tsxapps/deploy-web/src/hooks/useManagedWallet.tsapps/deploy-web/src/hooks/useMintACT/useMintACT.spec.tsapps/deploy-web/src/hooks/useMintACT/useMintACT.tsapps/deploy-web/src/hooks/useOnboardingChrome.spec.tsapps/deploy-web/src/hooks/useOnboardingChrome.tsapps/deploy-web/src/hooks/useProviderCredentials/useProviderCredentials.spec.tsapps/deploy-web/src/hooks/useProviderCredentials/useProviderCredentials.tsapps/deploy-web/src/hooks/useProviderJwt/useProviderJwt.spec.tsxapps/deploy-web/src/hooks/useProviderJwt/useProviderJwt.tsapps/deploy-web/src/hooks/useWalletBalance.tsapps/deploy-web/src/pages/billing/index.tsxapps/deploy-web/src/pages/payment-methods/index.tsxapps/deploy-web/src/pages/usage/index.tsxapps/deploy-web/src/services/analytics/analytics.service.spec.tsapps/deploy-web/src/services/analytics/analytics.service.tsapps/deploy-web/src/store/walletStore.tsapps/deploy-web/src/utils/TransactionMessageData.tsapps/deploy-web/src/utils/apiUtils.tsapps/deploy-web/src/utils/walletUtils.spec.tsapps/deploy-web/src/utils/walletUtils.tsapps/deploy-web/tests/seeders/localWallet.tsapps/deploy-web/tests/seeders/wallet.ts
💤 Files with no reviewable changes (8)
- apps/deploy-web/src/utils/apiUtils.ts
- apps/deploy-web/src/hoc/guard/guard.hoc.tsx
- apps/deploy-web/tests/seeders/localWallet.ts
- apps/deploy-web/src/hooks/useMintACT/useMintACT.ts
- apps/deploy-web/src/store/walletStore.ts
- apps/deploy-web/src/hooks/useMintACT/useMintACT.spec.ts
- apps/deploy-web/src/utils/walletUtils.spec.ts
- apps/deploy-web/src/components/new-deployment/SdlBuilder.spec.tsx
There was a problem hiding this comment.
No bugs found, but this is an XL refactor of the shared wallet context (72 files) that changes app-wide boot-gating behavior (the boot overlay now blocks all pages, including public ones, for registered users until the wallet lookup settles) and removes several fields consumers relied on. Given the blast radius across billing/deployment flows and the behavioral change to boot sequencing, this is worth a human look.
Extended reasoning...
Overview
This PR completes a managed-wallet-only refactor: it slims the WalletProvider context API (removing isManaged, isWalletLoaded, isWalletLoading, isWalletInitializing, walletName, logout, isOnboarding, useIsManagedWalletUser), introduces a new boot gate that renders <BootLoading /> in place of children until the initial wallet-existence lookup settles, and deletes dead custodial-era code (ACT auto-mint path, useMintACT, Guard HOC, unused apiUtils builders, etc.). It touches 72 files across billing, deployment, and onboarding flows.
Security risks
No injection/auth/crypto surface is introduced. The gate change affects rendering/loading sequencing, not authorization — RequireAuth/route guards are unchanged. Wallet creation and signing paths are untouched aside from renaming (connectManagedWallet → createWallet).
Level of scrutiny
This warrants more than a cursory pass: the WalletProvider boot gate now wraps the entire app in _app.tsx, including public pages, so its loading/settling behavior affects every page load for registered users. The PR also changes the public context shape consumed by ~15+ components, several of which had non-trivial conditional logic collapsed (e.g. AccountStatsCards, GetStartedStepper, DeploymentDepositModal's auto-mint removal). The bug-hunting system found no issues, and a candidate concern about the boot gate flashing/unmounting public pages for late-resolving registered users was investigated and refuted (the gate only engages when hasAuthenticatedUserId is true, so anonymous visitors on public pages are never blocked, and the check is against the current codebase which confirms WalletProvider wraps everything in _app.tsx).
Other factors
The PR includes solid test coverage (new WalletProvider.spec.tsx with 13 cases, and updates across ~20 other spec files), and the author's own verification notes (test/lint/type-check clean, manual smoke test) are thorough. However, the size (XL, 72 files), the fact it's stacked on another unmerged PR (#3563), and the app-wide behavioral change to boot sequencing are exactly the kind of large-scale, cross-cutting change where a second human reviewer's judgment adds value beyond automated checks.
…ltip The Spent line pre-rounded to 2 decimals before PriceValue converts to USD, so small spent amounts collapsed to $0.00 while the Balance line in the same tooltip converts with full 6-decimal precision.
The balance step still keyed its readiness check on AKT/ACT minimum deposits and pointed users at the Discord faucet, while the displayed amount only read USDC. Managed credit is USD-pegged and can sit in USDC or ACT, so legacy USDC-funded wallets saw the obsolete ACT warning and ACT-funded wallets saw $0. Readiness and the displayed amount now both derive from the combined USDC+ACT balance.
The suite was named .test.tsx, which vitest's include pattern never picks up, and it still used Jest APIs, so it never ran and drifted from the component: it asserted copy and button semantics that no longer exist. Renamed to .spec.tsx, converted to vitest, and rewrote the drifted expectations against the current component tree.
|
Addressed CodeRabbit's outside-diff note about Jest APIs in PaymentMethodsDisplay.test.tsx in eda0e0c, one step further than the suggestion: the .test.tsx suffix is not matched by vitest's include pattern (src/**/*.spec.{tsx,ts}), so the suite never ran and had drifted from the component (it asserted copy like "Payment Method Added" that no longer exists anywhere in src). Renamed it to .spec.tsx, converted it to vitest, and rewrote the drifted expectations against the current component tree; the 15 tests now run and pass. |
There was a problem hiding this comment.
No bugs were found, but this PR is large in scope (73 files) and changes core wallet/session behavior — most notably adding a boot gate that shows a full-screen loading overlay on every page (including public ones) for registered users until the wallet lookup resolves. That's an app-wide UX and architectural change worth a human look before merging.
What was reviewed: the WalletProvider boot gate and its isInitializing/isWalletCreating derivation, the RequireOnboarding lease-based gate interaction with the new hasWallet field, the deposit modal's ACT/USD conversion after removing auto-mint, and the ~12 consumer sites that dropped isManaged/custodial branches.
Extended reasoning...
Overview
This PR completes a managed-wallet-only refactor across apps/deploy-web: it collapses the WalletProvider context (dropping isManaged, isWalletLoaded, isWalletLoading, walletName, logout, isOnboarding, etc. in favor of hasWallet/isWalletCreating/createWallet), introduces a boot gate that blocks rendering behind BootLoading until the initial wallet-existence lookup settles, removes the ACT auto-mint deposit flow and useMintACT, and strips now-dead custodial branches from ~12 consumers (GetStartedStepper, DeploymentListRow, ApiKeyList, AccountStatsCards/AccountHeader, CreateLease, useWalletBalance, etc.). It also removes the no-op Guard/useIsManagedWalletUser HOC from the billing/usage/payment-methods pages and deletes dead API URL builders and custodial analytics types.
Security risks
No injection, auth-bypass, or data-exposure concerns stood out — this is UI/state-management refactoring, not new data access or permission logic. The wallet-signing path (signAndBroadcastTx) and JWT generation (useProviderJwt) are unchanged in their security properties, just re-gated on hasWallet instead of isWalletConnected.
Level of scrutiny
This warrants a higher level of scrutiny than typical: it is an XL PR (73 files) that changes a cross-cutting app boot sequence — the new gate shows a full-screen loading overlay on every page, including public ones, for any registered user until v1/wallets resolves. That is an intentional, documented behavioral change, but it is exactly the kind of app-wide UX/architecture decision that benefits from a human sign-off rather than an automated approval, even though the bug-hunting pass came back clean.
Other factors
The PR has a new WalletProvider.spec.tsx (13 cases) covering the gate directly, and the author has already responded to and fixed all of CodeRabbit's findings (precision mismatch, duplicate type alias, obsolete ACT copy, incomplete test assertion) plus explained why the tri-state loading/error concern on useIsOnboarded does not apply. No outstanding unaddressed reviewer comments remain. Given the size and the app-wide gating change, a human review is still the safer path here.
2 participants
Why
Console is managed-wallets-only (the server holds and signs the wallet), but the
WalletProvidercontext still carried the custodial-era surface: a hardcodedisManaged: truefeeding statically dead!isManagedbranches across consumers, dead fields (isWalletLoadedwas constanttrue,walletName/logout/isOnboardinghad zero consumers,isWalletConnected≡hasManagedWallet), and vestiges like the deposit-modal ACT/auto-mint path anduseMintACT. On top of that, every consumer had to handleisWalletLoading/isWalletInitializingindividually.This PR finishes the managed-only refactor and moves the wallet-loading wait to a single boot gate: the app now waits (behind the existing shared boot overlay) until the wallet record is resolved from the backend, so below the provider the wallet state is always settled and consumers carry no wallet-loading logic at all.
Stacked on #3563 (
refactor/wallet-remove-signed-in-with-trial-flag) — retarget/rebase tomainonce that merges.What
Boot gate (
WalletProvider)<BootLoading />(shared overlay, same idiom asRequireAuth) instead of children until the initial wallet-existence lookup settles.userId). The 5s address poll cannot re-engage the gate (a no-wallet response is a settled null success).Slimmed context API (drop the now-redundant "managed" qualifier)
Removed:
isManaged,isWalletLoaded,isWalletLoading,isWalletInitializing,walletName,logout,isOnboarding,useIsManagedWalletUser.hasWallet≠ has-address: sites needing a provisioned address keep thehasWallet && !!addressconjunct as a local (RequireOnboarding,useOnboardingChrome,useIsOnboarded).Dead custodial-era code deleted
useMintACT,getMintACTMsg,"mintingACT"tx-modal state.!isManagedbranches in ~12 consumers (GetStartedStepper,DeploymentListRow,ApiKeyList,useWalletBalanceincl.TX_FEE_BUFFER,AccountStatsCards/AccountHeadercustodial cards/props,CreateLeasemanifest snackbar,useManagedDeploymentConfirm, escrow alert, …).Guard(useIsManagedWalletUser)on/billing,/usage,/payment-methods(+ the now-unusedGuard/composeGuardsHOC),Layout'sisUsingWalletwallet-loading branch,isWalletModalOpenatom, deadapiUtilsbuilders (certificatesList,allowancesIssued,allowancesGranted,granterGrants), custodial analytics types,name/isManagedconstants in theManagedLocalWalletstorage shape.useManagedWalletstops decorating the wallet withusername/isWalletConnected/isWalletLoaded/selected(nothing read them) and exposesisCreating(cross-instance viauseIsMutating).Behavioral notes
v1/walletsresolves — one query, replacing the per-widget spinners/Loading wallet...states.$$(fix(deployment)).WalletProvider.spec.tsx(13 cases) covers the gate (blocks registered users mid-lookup, anonymous pass-through, creation never blanks the app, transition mounts children) and the new context shape.Verification
npm run test:unit: 3000 passed; the only failures are 3 pre-existing on the base branch (useScreenedProviders,provider-proxy.service, oneuseProviderJwtcase) — verified failing on a clean checkout too.npm run lint -- --quietclean;npm run type-checkhas zero new errors vs the base branch (baseline has long-standing spec-typing errors; this PR fixes one of them).next devwith mocked/api/auth/me+ heldv1/wallets: boot overlay covers the app while the lookup is in flight, releases the moment it settles, app renders (script: dev-server + Playwright route interception).Follow-ups (out of scope)
addWalletEssentialsdecorations inpackages/http-sdkare now unused by deploy-web.PaymentMethodsDisplay.test.tsx→.spec.tsxrename (currently not picked up by vitest's*.specinclude; typechecked only).GetStartedStepper's crypto-flavored min-balance tooltip.Summary by CodeRabbit