refactor(billing): remove /payment page in favor of /billing#2846
Merged
refactor(billing): remove /payment page in favor of /billing#2846
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request consolidates payment functionality into the billing page by introducing a dependency-injection pattern to AccountOverview, removing the standalone payment page and related payment components, and updating navigation links throughout the app to redirect to the billing page with the openPayment query parameter. Changes
Sequence DiagramsequenceDiagram
participant User
participant Router
participant AccountOverview
participant SearchParams
participant PaymentPopup
participant Navigation
User->>Router: Click "Add Funds"
Router->>Router: Navigate to /billing?openPayment=true
Router->>AccountOverview: Render component
AccountOverview->>SearchParams: Read query params
SearchParams-->>AccountOverview: Return { openPayment: true }
AccountOverview->>PaymentPopup: Check default payment method exists
PaymentPopup-->>AccountOverview: Confirm method available
AccountOverview->>PaymentPopup: Trigger payment popup
User->>PaymentPopup: Complete payment
PaymentPopup->>Navigation: Redirect to /billing
Navigation-->>User: Payment flow complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2846 +/- ##
==========================================
- Coverage 55.01% 54.64% -0.38%
==========================================
Files 1025 985 -40
Lines 23779 22767 -1012
Branches 5837 5691 -146
==========================================
- Hits 13082 12441 -641
+ Misses 9323 8985 -338
+ Partials 1374 1341 -33
*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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/deploy-web/src/components/billing-usage/AccountOverview/AccountOverview.tsx (1)
29-38: Consider handling the case when no payment method exists.The effect requires
defaultPaymentMethodto be truthy before opening the popup. If a user navigates to/billing?openPayment=truewithout a payment method configured, nothing visibly happens—the popup silently doesn't open.While this is consistent with the "Add Funds" button being disabled without a payment method (line 128), external links (e.g., from onboarding flows or emails) may lead users here expecting an action. Consider either:
- Redirecting to the payment methods page, or
- Showing a toast/message indicating a payment method is needed first.
💡 Example: redirect to payment methods page when no default method exists
useEffect(() => { - if (!isLoading && searchParams.get("openPayment") === "true" && defaultPaymentMethod) { - setShowPaymentPopup(true); - router.replace(UrlService.billing(), { scroll: false }); + if (!isLoading && searchParams.get("openPayment") === "true") { + if (defaultPaymentMethod) { + setShowPaymentPopup(true); + router.replace(UrlService.billing(), { scroll: false }); + } else { + router.replace(UrlService.paymentMethods(), { scroll: false }); + } } }, [isLoading, searchParams, defaultPaymentMethod, router]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/deploy-web/src/components/billing-usage/AccountOverview/AccountOverview.tsx` around lines 29 - 38, The effect currently opens the payment popup only when defaultPaymentMethod is truthy; update the useEffect (the block referencing searchParams, defaultPaymentMethod, setShowPaymentPopup, router.replace) to handle the case where searchParams.get("openPayment") === "true" but defaultPaymentMethod is falsy and isLoading is false by redirecting the user to the payment methods page (e.g., router.replace(UrlService.paymentMethods(), { scroll: false })) or, alternatively, show a user-facing toast indicating a payment method is required before opening the popup; ensure the new branch runs when !isLoading && searchParams.get("openPayment")==="true" && !defaultPaymentMethod and does not attempt to open the popup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/deploy-web/src/components/billing-usage/AccountOverview/AccountOverview.tsx`:
- Around line 29-38: The effect currently opens the payment popup only when
defaultPaymentMethod is truthy; update the useEffect (the block referencing
searchParams, defaultPaymentMethod, setShowPaymentPopup, router.replace) to
handle the case where searchParams.get("openPayment") === "true" but
defaultPaymentMethod is falsy and isLoading is false by redirecting the user to
the payment methods page (e.g., router.replace(UrlService.paymentMethods(), {
scroll: false })) or, alternatively, show a user-facing toast indicating a
payment method is required before opening the popup; ensure the new branch runs
when !isLoading && searchParams.get("openPayment")==="true" &&
!defaultPaymentMethod and does not attempt to open the popup.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (16)
apps/deploy-web/src/components/billing-usage/AccountOverview/AccountOverview.tsxapps/deploy-web/src/components/billing-usage/AddPaymentMethodPopup/AddPaymentMethodPopup.tsxapps/deploy-web/src/components/billing-usage/PaymentMethodsView/PaymentMethodsView.tsxapps/deploy-web/src/components/billing-usage/PaymentSuccessAnimation/PaymentSuccessAnimation.tsxapps/deploy-web/src/components/deployments/DeploymentDepositModal.tsxapps/deploy-web/src/components/get-started/GetStartedStepper.tsxapps/deploy-web/src/components/home/AccountHeader.tsxapps/deploy-web/src/components/new-deployment/CreateLease/CreateLease.tsxapps/deploy-web/src/components/shared/TrialDeploymentTooltip.tsxapps/deploy-web/src/components/user/payment/DeletePaymentMethodPopup.tsxapps/deploy-web/src/components/user/payment/PaymentForm.tsxapps/deploy-web/src/components/user/payment/index.tsapps/deploy-web/src/components/wallet/ManagedWalletPopup.tsxapps/deploy-web/src/hooks/usePayingCustomerRequiredEventHandler.tsxapps/deploy-web/src/pages/payment.tsxapps/deploy-web/src/utils/urlUtils.ts
💤 Files with no reviewable changes (4)
- apps/deploy-web/src/components/user/payment/PaymentForm.tsx
- apps/deploy-web/src/components/user/payment/DeletePaymentMethodPopup.tsx
- apps/deploy-web/src/pages/payment.tsx
- apps/deploy-web/src/components/user/payment/index.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/deploy-web/src/components/billing-usage/AccountOverview/AccountOverview.spec.tsx`:
- Line 20: Replace the use of getByTestId in the failing expectation with
queryByTestId to match repository testing guidelines and the rest of
AccountOverview.spec.tsx; specifically update the assertion that uses
screen.getByTestId("payment-popup") to use screen.queryByTestId("payment-popup")
in the expect call (so the expectation reads that the queried element is in the
document), keeping behavior identical but following the required queryBy*
pattern.
- Around line 77-113: The dependency mocks in the DEPENDENCIES object use broad
"as any" casts which bypass type checking; replace those casts by typing each
mock to the real dependency return types (e.g., useSnackbar,
useDefaultPaymentMethodQuery, useWalletBalance, useWalletSettingsQuery,
useWeeklyDeploymentCostQuery, useWalletSettingsMutations, usePopup,
useSearchParams, useRouter) using their actual ReturnType or hook/prop types and
create vi.fn implementations that return objects matching those types, and
likewise type component mocks
(PaymentPopup/PaymentSuccessAnimation/FormattedNumber, Title, Link, Button,
Card, etc.) as the corresponding React ComponentType/props interfaces and
implement UrlService as the concrete UrlService type rather than "as any" so the
test will fail at compile time if the AccountOverview contract changes.
In `@apps/deploy-web/src/utils/urlUtils.spec.ts`:
- Line 5: The root test suite currently uses a hardcoded string
"UrlService.billing"; update the describe call to use the subject's name
property so renames are safe — replace the literal with UrlService.billing.name
in the root describe, locating the suite that references UrlService.billing and
changing its label to UrlService.billing.name so automated refactors will track
it.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/deploy-web/src/components/billing-usage/AccountOverview/AccountOverview.spec.tsxapps/deploy-web/src/components/billing-usage/AccountOverview/AccountOverview.tsxapps/deploy-web/src/utils/urlUtils.spec.ts
stalniy
reviewed
Mar 3, 2026
stalniy
approved these changes
Mar 3, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/api/test/seeders/auto-top-up-deployment.seeder.ts (1)
13-13: Prefer deterministic default forisWalletAutoTopUpEnabledin the seeder.Using a random default here can make behavior-driven tests flaky. Defaulting to
falseand overriding per test is safer.♻️ Suggested tweak
- isWalletAutoTopUpEnabled: faker.datatype.boolean(), + isWalletAutoTopUpEnabled: false,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/test/seeders/auto-top-up-deployment.seeder.ts` at line 13, Change the random default for isWalletAutoTopUpEnabled in the seeder to a deterministic value (set isWalletAutoTopUpEnabled: false instead of faker.datatype.boolean()) so tests are not flaky, and allow tests to explicitly override this field where needed (locate the isWalletAutoTopUpEnabled property in the auto-top-up deployment seeder object/export and replace the faker.datatype.boolean() call with false).apps/api/src/deployment/services/top-up-managed-deployments/top-up-managed-deployments-instrumentation.service.spec.ts (1)
89-107: Consider asserting counter existence before checking calls.The optional chaining on
countersByName["..."]?.addworks but could produce confusing error messages if the counter isn't created due to a regression. Asserting the counter exists first would provide clearer diagnostics.♻️ Suggested improvement
it("increments auto-reload counter when insufficient balance and wallet auto-reload is enabled", () => { const { service, countersByName } = setup(); service.start(100, { dryRun: false }); const deployment = createDrainingDeployment({ isWalletAutoTopUpEnabled: true }); service.recordMessagePreparationError({ deployment, error: new Error("Insufficient balance for address") }); - expect(countersByName["auto_top_up_insufficient_balance_with_auto_reload_total"]?.add).toHaveBeenCalledWith(1); + const counter = countersByName["auto_top_up_insufficient_balance_with_auto_reload_total"]; + expect(counter).toBeDefined(); + expect(counter.add).toHaveBeenCalledWith(1); }); it("does not increment auto-reload counter when insufficient balance and wallet auto-reload is disabled", () => { const { service, countersByName } = setup(); service.start(100, { dryRun: false }); const deployment = createDrainingDeployment({ isWalletAutoTopUpEnabled: false }); service.recordMessagePreparationError({ deployment, error: new Error("Insufficient balance for address") }); - expect(countersByName["auto_top_up_insufficient_balance_with_auto_reload_total"]?.add).not.toHaveBeenCalled(); + const counter = countersByName["auto_top_up_insufficient_balance_with_auto_reload_total"]; + expect(counter).toBeDefined(); + expect(counter.add).not.toHaveBeenCalled(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/deployment/services/top-up-managed-deployments/top-up-managed-deployments-instrumentation.service.spec.ts` around lines 89 - 107, The test uses optional chaining when asserting the counter was called which hides missing-counter regressions; update the two specs to first assert the counter exists (e.g., assert countersByName["auto_top_up_insufficient_balance_with_auto_reload_total"] is defined/truthy) before checking its .add call, keeping the rest of the flow (setup(), service.start(), createDrainingDeployment(), service.recordMessagePreparationError()) the same so failures clearly indicate a missing counter rather than a silent undefined method check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/api/src/deployment/repositories/deployment-setting/deployment-setting.repository.ts`:
- Line 77: The projection maps WalletSetting.autoReloadEnabled directly to
isWalletAutoTopUpEnabled but because WalletSetting is LEFT JOINed this value can
be null while the TypeScript type is boolean; update the projection to return a
non-null boolean (e.g., use a null-coalescing/defaulting expression such as
COALESCE or equivalent in your query builder) for
WalletSetting.autoReloadEnabled and the other nullable WalletSetting fields
referenced around the same block (the fields at lines 82–83) so the repository
(deployment-setting.repository.ts) always produces a boolean for
isWalletAutoTopUpEnabled.
In
`@apps/api/src/deployment/services/draining-deployment/draining-deployment.service.spec.ts`:
- Around line 412-415: Remove the redundant inline calculation comments in the
test that restate the math already expressed by the code: delete the three
comment lines above the expectedTotal assignment (the lines describing "Cost =
sum..." and the two per-deployment math lines) so only the computed const
expectedTotal = 5035000 + 7545000; remains; leave the test name, surrounding
assertions, and the expectedTotal variable untouched (look for expectedTotal in
draining-deployment.service.spec.ts to locate the spot).
---
Nitpick comments:
In
`@apps/api/src/deployment/services/top-up-managed-deployments/top-up-managed-deployments-instrumentation.service.spec.ts`:
- Around line 89-107: The test uses optional chaining when asserting the counter
was called which hides missing-counter regressions; update the two specs to
first assert the counter exists (e.g., assert
countersByName["auto_top_up_insufficient_balance_with_auto_reload_total"] is
defined/truthy) before checking its .add call, keeping the rest of the flow
(setup(), service.start(), createDrainingDeployment(),
service.recordMessagePreparationError()) the same so failures clearly indicate a
missing counter rather than a silent undefined method check.
In `@apps/api/test/seeders/auto-top-up-deployment.seeder.ts`:
- Line 13: Change the random default for isWalletAutoTopUpEnabled in the seeder
to a deterministic value (set isWalletAutoTopUpEnabled: false instead of
faker.datatype.boolean()) so tests are not flaky, and allow tests to explicitly
override this field where needed (locate the isWalletAutoTopUpEnabled property
in the auto-top-up deployment seeder object/export and replace the
faker.datatype.boolean() call with false).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: da1fab4b-79dc-49b3-ace2-6a0783de4c9c
📒 Files selected for processing (9)
apps/api/src/billing/services/balances/balances.service.tsapps/api/src/billing/services/wallet-balance-reload-check/wallet-balance-reload-check.handler.spec.tsapps/api/src/billing/services/wallet-balance-reload-check/wallet-balance-reload-check.handler.tsapps/api/src/deployment/repositories/deployment-setting/deployment-setting.repository.tsapps/api/src/deployment/services/draining-deployment/draining-deployment.service.spec.tsapps/api/src/deployment/services/draining-deployment/draining-deployment.service.tsapps/api/src/deployment/services/top-up-managed-deployments/top-up-managed-deployments-instrumentation.service.spec.tsapps/api/src/deployment/services/top-up-managed-deployments/top-up-managed-deployments-instrumentation.service.tsapps/api/test/seeders/auto-top-up-deployment.seeder.ts
The /payment page was a duplicate of billing functionality, causing users to miss features like auto credit reload and invoices. All "Add Funds" links now navigate to /billing with an openPayment query param that auto-opens the payment popup.
…ce.billing Refactors AccountOverview to use the DEPENDENCIES pattern and adds tests covering the openPayment query param auto-open behavior. Also adds unit tests for UrlService.billing() with the new openPayment param.
- Use queryByTestId instead of getByTestId per repo testing guidelines - Replace individual as any casts with single as unknown as typeof DEPENDENCIES - Use .name for describe suite label in urlUtils spec
…rvices Address review feedback: replace inline test stubs with MockComponents helper and inject UrlService through useServices hook instead of directly in DEPENDENCIES.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/deploy-web/src/components/billing-usage/AccountOverview/AccountOverview.spec.tsx (1)
50-60: Consider adding popup assertion for consistency.The other "does not open" tests verify both
mockReplaceandqueryByTestId("payment-popup"). This test only checksmockReplace. While the loading state renders skeleton UI (so the popup wouldn't be present anyway), adding the assertion would maintain consistency across all negative test cases.♻️ Proposed fix
expect(mockReplace).not.toHaveBeenCalled(); + expect(screen.queryByTestId("payment-popup")).not.toBeInTheDocument(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/deploy-web/src/components/billing-usage/AccountOverview/AccountOverview.spec.tsx` around lines 50 - 60, In the "does not open payment popup while still loading" test in AccountOverview.spec.tsx, add the same negative DOM assertion used in the other tests: after calling setup(...) assert that queryByTestId("payment-popup") is null (or not present). Locate the test block with the name string "does not open payment popup while still loading", keep the existing setup call (including isLoading: true and mockReplace), and add the queryByTestId("payment-popup") check to mirror the other "does not open" tests for consistency.apps/deploy-web/src/components/billing-usage/AccountOverview/AccountOverview.tsx (1)
95-99: Consider addingdto the useCallback dependency array.The callback references
d.Snackbarbutdis not in the dependency array. Whiledtypically defaults to the constantDEPENDENCIES, if a caller passes a differentdependenciesprop that changes between renders, the callback would use a stale reference.♻️ Proposed fix
[confirm, enqueueSnackbar, upsertWalletSettings] + [confirm, d, enqueueSnackbar, upsertWalletSettings]Also applies to: 102-102
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/deploy-web/src/components/billing-usage/AccountOverview/AccountOverview.tsx` around lines 95 - 99, The useCallback that calls enqueueSnackbar and references d.Snackbar should include d in its dependency array to avoid capturing a stale dependencies object; update the useCallback hook (the callback that enqueues success/failure Snackbars) to add d (or the prop named dependencies) to its dependency list so d.Snackbar is refreshed when the dependencies prop changes, ensuring both the success and onError handlers use the latest d value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/deploy-web/src/components/billing-usage/AccountOverview/AccountOverview.spec.tsx`:
- Around line 50-60: In the "does not open payment popup while still loading"
test in AccountOverview.spec.tsx, add the same negative DOM assertion used in
the other tests: after calling setup(...) assert that
queryByTestId("payment-popup") is null (or not present). Locate the test block
with the name string "does not open payment popup while still loading", keep the
existing setup call (including isLoading: true and mockReplace), and add the
queryByTestId("payment-popup") check to mirror the other "does not open" tests
for consistency.
In
`@apps/deploy-web/src/components/billing-usage/AccountOverview/AccountOverview.tsx`:
- Around line 95-99: The useCallback that calls enqueueSnackbar and references
d.Snackbar should include d in its dependency array to avoid capturing a stale
dependencies object; update the useCallback hook (the callback that enqueues
success/failure Snackbars) to add d (or the prop named dependencies) to its
dependency list so d.Snackbar is refreshed when the dependencies prop changes,
ensuring both the success and onError handlers use the latest d value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a6ce6852-0ed8-4042-af76-acb898a923b3
⛔ Files ignored due to path filters (1)
packages/net/src/generated/netConfigData.tsis excluded by!**/generated/**
📒 Files selected for processing (18)
apps/deploy-web/src/components/billing-usage/AccountOverview/AccountOverview.spec.tsxapps/deploy-web/src/components/billing-usage/AccountOverview/AccountOverview.tsxapps/deploy-web/src/components/billing-usage/AddPaymentMethodPopup/AddPaymentMethodPopup.tsxapps/deploy-web/src/components/billing-usage/PaymentMethodsView/PaymentMethodsView.tsxapps/deploy-web/src/components/billing-usage/PaymentSuccessAnimation/PaymentSuccessAnimation.tsxapps/deploy-web/src/components/deployments/DeploymentDepositModal.tsxapps/deploy-web/src/components/get-started/GetStartedStepper.tsxapps/deploy-web/src/components/home/AccountHeader.tsxapps/deploy-web/src/components/new-deployment/CreateLease/CreateLease.tsxapps/deploy-web/src/components/shared/TrialDeploymentTooltip.tsxapps/deploy-web/src/components/user/payment/DeletePaymentMethodPopup.tsxapps/deploy-web/src/components/user/payment/PaymentForm.tsxapps/deploy-web/src/components/user/payment/index.tsapps/deploy-web/src/components/wallet/ManagedWalletPopup.tsxapps/deploy-web/src/hooks/usePayingCustomerRequiredEventHandler.tsxapps/deploy-web/src/pages/payment.tsxapps/deploy-web/src/utils/urlUtils.spec.tsapps/deploy-web/src/utils/urlUtils.ts
💤 Files with no reviewable changes (4)
- apps/deploy-web/src/components/user/payment/index.ts
- apps/deploy-web/src/components/user/payment/PaymentForm.tsx
- apps/deploy-web/src/components/user/payment/DeletePaymentMethodPopup.tsx
- apps/deploy-web/src/pages/payment.tsx
✅ Files skipped from review due to trivial changes (1)
- apps/deploy-web/src/components/billing-usage/PaymentMethodsView/PaymentMethodsView.tsx
…opening without default method Remove defaultPaymentMethod requirement from AccountOverview useEffect so the popup opens via openPayment query param even without a pre-selected method. Add a payment method dropdown inside PaymentPopup using usePaymentMethodsQuery. Export getPaymentMethodDisplay from PaymentMethodCard for reuse. Inject urlService via useServices in ManagedWalletPopup.
…ment=true Replace deprecated /payment route with /billing?openPayment=true query param across all env files and test fixtures.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The
/paymentpage was a duplicate of the billing functionality. Some users never noticed features like auto credit reload and invoices because they only used the/paymentpage. This consolidates everything into the/billingpage.CON-42
What
/paymentpage and its payment-only components (PaymentForm,DeletePaymentMethodPopup)AddPaymentMethodPopup,PaymentSuccessAnimation) tobilling-usage//billing?openPayment=trueopenPaymentquery param auto-opens the payment popup on the billing page, so the UX from the old/paymentpage is preservedUrlService.payment()methodSummary by CodeRabbit
New Features
Refactor
Screen.Recording.2026-03-06.at.10.24.00.AM.mov