feat(billing): add fixed-threshold auto top-up UI behind a feature flag - #3535
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds feature-flagged fixed-threshold auto top-up settings and tests. Wallet reload scheduling now cleans up existing user jobs before enqueueing replacement jobs. ChangesAuto top-up flow
Estimated code review effort: 4 (Complex) | ~45 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/types/feature-flags.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Addresses review on #3535: - Gate the Auto Top-Up edit button on autoReloadEnabled and the in-flight mutation state, so editing from the off state can no longer silently re-enable auto-charging (saveSettings always submits autoReloadEnabled: true) and a save can't race the switch's disable mutation. - Mirror the backend WalletSettingsInputSchema $10,000 max on the client threshold/amount schema so over-limit values fail inline instead of as a generic 400. Refs CON-717
There was a problem hiding this comment.
Beyond the inline finding, I also checked whether the client should validate multipleOf(0.01) on the threshold/amount fields to preempt a generic 400 for values like 20.005 — the backend's zod schema does the same coercion/rounding without a multipleOf constraint, so this isn't a real mismatch and no fix is needed there.
Extended reasoning...
Verifier agents this run also examined whether the AutoTopUpSettingsPopup form should add a multipleOf(0.01) check to mirror stricter backend validation and avoid a generic 400 on fractional-cent inputs; this was ruled out because the backend schema doesn't enforce that constraint either, so there's no client/server mismatch to fix.
Addresses review on #3535. The client used useWallet().topUpMinAmountUsd (a trial-aware value for the one-time Add-Funds flow) as the recurring auto-top-up amount minimum, but the backend floors every recurring charge at the fixed STANDARD_TOP_UP_MIN_AMOUNT_USD ($20) in both WalletSettingsInputSchema and the reload-check clamp. Under a non-default MANAGED_WALLET_TRIAL_MIN_TOP_UP_AMOUNT a trialing user could be blocked client-side from saving an amount the API would accept. Mirror the fixed $20 floor with a client constant and drop the now-unused useWallet dependency (the schema no longer needs a runtime factory). Refs CON-717
There was a problem hiding this comment.
LGTM. The three substantive issues from the prior review round (edit-button gating on autoReloadEnabled, edit-button vs. in-flight mutation race, and the trial-aware min-amount mismatch) have all been fixed in 7e5802f and bd699ae, each with a matching test. The only remaining finding this round is a copy-only nit (label vs. description wording), which doesn't block.
Extended reasoning...
Overview
This PR adds the frontend for a new fixed-threshold auto top-up feature, fully gated behind the auto_reload_fixed_threshold feature flag (off by default). It touches AccountOverview.tsx (reworked card UI) and a new AutoTopUpSettingsPopup component, plus corresponding spec files and a feature-flag type addition. No backend/API code is touched here (this is a stacked frontend-only PR).
Security risks
None of note. There's no auth, crypto, or permission logic here — this is UI/UX plumbing around an existing wallet-settings mutation endpoint that already enforces its own bounds server-side. Client-side zod validation mirrors (and is stricter than, per the fixed min-amount bug found and fixed in bd699ae) the backend's bounds, so no invalid values can reach the API from this surface.
Level of scrutiny
Moderate. The change is entirely behind a default-off feature flag, so there's zero production exposure until explicitly enabled, which lowers the bar. That said, it's a real behavior change to a billing-adjacent flow (auto-charging a payment method), so the prior round's substantive findings (silent re-enable via Edit button, concurrent mutation race, trial-aware min-amount divergence) were worth catching and fixing, which the author did with matching test coverage for each fix.
Other factors
The PR has strong test coverage added alongside each fix (e.g., a dedicated test asserting the edit button is disabled in the off state, and one covering the in-flight-mutation case). The one open item flagged this round (label says 'goes below' while its own description says 'at or below') is a copy-only inconsistency with no functional or validation impact, so it doesn't warrant blocking. Given the flag-gated exposure and the demonstrated responsiveness to prior feedback, this is safe to approve.
53725b0 to
e8f0db4
Compare
Addresses review on #3535: - Gate the Auto Top-Up edit button on autoReloadEnabled and the in-flight mutation state, so editing from the off state can no longer silently re-enable auto-charging (saveSettings always submits autoReloadEnabled: true) and a save can't race the switch's disable mutation. - Mirror the backend WalletSettingsInputSchema $10,000 max on the client threshold/amount schema so over-limit values fail inline instead of as a generic 400. Refs CON-717
Addresses review on #3535. The client used useWallet().topUpMinAmountUsd (a trial-aware value for the one-time Add-Funds flow) as the recurring auto-top-up amount minimum, but the backend floors every recurring charge at the fixed STANDARD_TOP_UP_MIN_AMOUNT_USD ($20) in both WalletSettingsInputSchema and the reload-check clamp. Under a non-default MANAGED_WALLET_TRIAL_MIN_TOP_UP_AMOUNT a trialing user could be blocked client-side from saving an amount the API would accept. Mirror the fixed $20 floor with a client constant and drop the now-unused useWallet dependency (the schema no longer needs a runtime factory). Refs CON-717
bd699ae to
897928c
Compare
Addresses review on #3535: - Gate the Auto Top-Up edit button on autoReloadEnabled and the in-flight mutation state, so editing from the off state can no longer silently re-enable auto-charging (saveSettings always submits autoReloadEnabled: true) and a save can't race the switch's disable mutation. - Mirror the backend WalletSettingsInputSchema $10,000 max on the client threshold/amount schema so over-limit values fail inline instead of as a generic 400. Refs CON-717
Addresses review on #3535. The client used useWallet().topUpMinAmountUsd (a trial-aware value for the one-time Add-Funds flow) as the recurring auto-top-up amount minimum, but the backend floors every recurring charge at the fixed STANDARD_TOP_UP_MIN_AMOUNT_USD ($20) in both WalletSettingsInputSchema and the reload-check clamp. Under a non-default MANAGED_WALLET_TRIAL_MIN_TOP_UP_AMOUNT a trialing user could be blocked client-side from saving an amount the API would accept. Mirror the fixed $20 floor with a client constant and drop the now-unused useWallet dependency (the schema no longer needs a runtime factory). Refs CON-717
897928c to
2d709b7
Compare
There was a problem hiding this comment.
Beyond the inline history on this PR (all previously flagged issues are now fixed), this run's bug hunt also checked whether the new non-transactional job scheduling in wallet-settings.service.ts (scheduleForWalletSetting(next, { withCleanup: true })) could break rollback atomicity if reload values change — verified it does not (job cleanup/scheduling is idempotent and outside the DB transaction boundary by design, so a rollback doesn't leave a stale job).
Extended reasoning...
This run's bug hunting system found no new bugs and flagged one candidate issue (non-transactional job cancellation in wallet-settings.service.ts) that its verifier agent examined and ruled out as not a real bug. All bugs I previously raised on this PR (edit button gating on autoReloadEnabled and isReloadChangeDisabled, the trial-aware min-amount mismatch, and the threshold label wording) have since been fixed by the author in commits 7e5802f, bd699ae, and 3c652c3. The remaining disagreement (as unknown as vs mock() for overloaded call signatures in the new spec file) was addressed with a reasonable technical rationale from the author and does not block approval on its own.
There was a problem hiding this comment.
No new bugs found in this pass. Given this is a billing feature that automates real payment charges and this PR has already gone through several rounds of substantive fixes (edit-button gating, an in-flight mutation race, and copy/validation mismatches), I'd like a human to take a final look before merge.
Extended reasoning...
Overview
This PR adds the frontend half of a fixed-threshold auto top-up feature (CON-717), gated behind the auto_reload_fixed_threshold flag (off by default). It introduces a new AutoTopUpSettingsPopup dialog, reworks the AccountOverview card's enable/disable/edit flows, and makes small supporting changes on the API side (WalletReloadJobService.scheduleForWalletSetting now optionally cancels prior singleton jobs before re-enqueuing, used from both the wallet-settings service and scheduleImmediate).
Security risks
No injection/auth-bypass vectors — this is UI + a job-scheduling cleanup, and all mutations go through existing, already-authorized useWalletSettingsMutations / WalletSettingService paths with server-side validation (payment-method presence, min/max bounds) unchanged by this PR. The main risk class here is logic/UX correctness around real payment charging (wrong amount/threshold submitted, or a stale mutation clobbering a newer one), not a classic security vulnerability.
Level of scrutiny
This sits in a gray zone: it's flagged off by default (zero production exposure until flipped), but the feature it gates is auto-charging a user's payment method, and the diff touches the enable/disable/edit state machine plus a job re-scheduling change on the backend. That combination — payment automation + non-trivial state/race handling — argues for higher scrutiny than a typical UI PR, even though blast radius is currently zero.
Other factors
This run's finder surfaced three candidates that verifiers ruled out: cancelCreatedBy running outside the transaction while re-enqueue runs inside it, the Auto Top-Up card header layout change, and the switch staying enabled during an in-flight popup save — none held up as real issues. Three earlier rounds of review on this same PR did find and get real fixes merged (edit button not gated on enabled/pending state, and a client/server validation-minimum mismatch), which is a track record worth a human sanity-check on the cumulative diff rather than trusting the flag-off safety net alone.
There was a problem hiding this comment.
No new bugs found this run. This PR touches billing charge-scheduling logic (auto top-up job dedup, wallet settings) behind a feature flag that's off by default, so I'd still like a human to take a look given the payment-related blast radius.
Extended reasoning...
Overview
This PR adds the frontend half of a fixed-threshold auto top-up feature (AutoTopUpSettingsPopup, reworked AccountOverview card) plus a small backend companion change: wallet-reload-job scheduling now cancels prior pending reload-check jobs (via singletonKey) before enqueuing a new one, whenever wallet settings change.
Security risks
No auth/crypto changes. The main risk surface is billing correctness: a user's default payment method gets auto-charged when balance drops to/below a threshold. All new UI is gated behind auto_reload_fixed_threshold, off by default, so there's no production exposure yet.
Level of scrutiny
This run found zero new bugs, and a finder-raised concern about duplicate reload-check jobs double-charging a payment method was examined and refuted (verifier confirmed the singleton-key based enqueue/cancel logic prevents duplicates). However, over the PR's review history, four real issues were found and fixed by the author across separate commits (edit-button gating on auto-reload-off/in-flight state, a mismatched validation minimum vs. backend, and a self-contradictory label). Given that history, the payment-charging code path, and the overall size (L), I think this still warrants a human pass rather than an automatic approval, even though no new issues surfaced this run.
Other factors
Test coverage looks solid (AccountOverview and AutoTopUpSettingsPopup specs cover enable/edit/disable flows, validation bounds, and race conditions), and the author has been responsive to every prior finding with targeted fixes and added tests.
Addresses review on #3535: - Gate the Auto Top-Up edit button on autoReloadEnabled and the in-flight mutation state, so editing from the off state can no longer silently re-enable auto-charging (saveSettings always submits autoReloadEnabled: true) and a save can't race the switch's disable mutation. - Mirror the backend WalletSettingsInputSchema $10,000 max on the client threshold/amount schema so over-limit values fail inline instead of as a generic 400. Refs CON-717
Addresses review on #3535. The client used useWallet().topUpMinAmountUsd (a trial-aware value for the one-time Add-Funds flow) as the recurring auto-top-up amount minimum, but the backend floors every recurring charge at the fixed STANDARD_TOP_UP_MIN_AMOUNT_USD ($20) in both WalletSettingsInputSchema and the reload-check clamp. Under a non-default MANAGED_WALLET_TRIAL_MIN_TOP_UP_AMOUNT a trialing user could be blocked client-side from saving an amount the API would accept. Mirror the fixed $20 floor with a client constant and drop the now-unused useWallet dependency (the schema no longer needs a runtime factory). Refs CON-717
f9aadd8 to
cbb50d1
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3535 +/- ##
==========================================
- Coverage 75.85% 75.08% -0.77%
==========================================
Files 1160 1071 -89
Lines 30245 27960 -2285
Branches 7521 7083 -438
==========================================
- Hits 22942 20994 -1948
+ Misses 6442 6133 -309
+ Partials 861 833 -28
*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: 2
🧹 Nitpick comments (1)
apps/deploy-web/src/components/billing-usage/AutoTopUpSettingsPopup/AutoTopUpSettingsPopup.spec.tsx (1)
163-175: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse typed mocks instead of double assertions.
These tests use
as unknown asto force mock types. This bypasses the dependency contract.
apps/deploy-web/src/components/billing-usage/AutoTopUpSettingsPopup/AutoTopUpSettingsPopup.spec.tsx#L163-L175: Create typed hook-return mocks withmock<T>().apps/deploy-web/src/components/billing-usage/AccountOverview/AccountOverview.spec.tsx#L317-L347: Construct a typed dependency mock instead of casting the object throughunknown.As per coding guidelines, “In tests, use
mock<T>()instead ofas unknown as <Type>for creating mocks.”🤖 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/billing-usage/AutoTopUpSettingsPopup/AutoTopUpSettingsPopup.spec.tsx` around lines 163 - 175, Replace the double assertions in AutoTopUpSettingsPopup.spec.tsx lines 163-175 with typed mock<T>() hook-return mocks for useSnackbar, useDefaultPaymentMethodQuery, and useWalletSettingsMutations while preserving their current values and behavior. In AccountOverview.spec.tsx lines 317-347, construct the dependency mock with mock<T>() instead of casting through unknown; update both sites to use the established typed-mock pattern without changing 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/api/src/billing/services/wallet-settings/wallet-settings.service.ts`:
- Line 145: Update the wallet-setting job cleanup flow around
upsertWalletSetting and scheduleForWalletSetting to pass the ambient transaction
connection into cancelCreatedBy, rather than having it obtain pgBoss.getDb()
independently. Ensure both cancellation and enqueue use the WithTransaction
connection so they commit or roll back atomically with the wallet setting
update.
In
`@apps/deploy-web/src/components/billing-usage/AutoTopUpSettingsPopup/AutoTopUpSettingsPopup.tsx`:
- Around line 40-45: Remove zodResolver from the DEPENDENCIES fixture and its
corresponding test dependency fixture, while retaining the direct import and
invoking it directly in useForm. Keep the other hook and mutation dependencies
unchanged.
---
Nitpick comments:
In
`@apps/deploy-web/src/components/billing-usage/AutoTopUpSettingsPopup/AutoTopUpSettingsPopup.spec.tsx`:
- Around line 163-175: Replace the double assertions in
AutoTopUpSettingsPopup.spec.tsx lines 163-175 with typed mock<T>() hook-return
mocks for useSnackbar, useDefaultPaymentMethodQuery, and
useWalletSettingsMutations while preserving their current values and behavior.
In AccountOverview.spec.tsx lines 317-347, construct the dependency mock with
mock<T>() instead of casting through unknown; update both sites to use the
established typed-mock pattern without changing 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: 0fb2759f-b3cf-4285-9691-7fba7c1fce6f
📒 Files selected for processing (10)
apps/api/src/billing/services/wallet-reload-job/wallet-reload-job.service.spec.tsapps/api/src/billing/services/wallet-reload-job/wallet-reload-job.service.tsapps/api/src/billing/services/wallet-settings/wallet-settings.service.integration.tsapps/api/src/billing/services/wallet-settings/wallet-settings.service.tsapps/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/AutoTopUpSettingsPopup/AutoTopUpSettingsPopup.spec.tsxapps/deploy-web/src/components/billing-usage/AutoTopUpSettingsPopup/AutoTopUpSettingsPopup.tsxapps/deploy-web/src/queries/useWalletSettingsQueries.spec.tsxapps/deploy-web/src/types/feature-flags.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/api/src/core/services/job-queue/job-queue.service.spec.ts (1)
195-197: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a typed
Sqlmock.
as unknown as Sqlbypasses type checking and can hide an incompatible mock shape. Create the connection withmock<Sql>()and configureconnection.unsafedirectly.As per coding guidelines, tests must use
mock<T>()instead ofas unknown as <Type>for creating mocks.🤖 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/core/services/job-queue/job-queue.service.spec.ts` around lines 195 - 197, Replace the unsafe connection cast in the “cancels created jobs on the ambient transaction connection when one is active” test with a typed mock created via mock<Sql>(). Configure its unsafe method directly with the existing resolved job result, preserving the test’s current 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/api/src/core/services/job-queue/job-queue.service.spec.ts`:
- Around line 191-192: Strengthen the executeSql assertion in the cancellation
spec to verify the generated SQL includes all cancellation predicates: name
filtering, state = 'created', and singleton_key filtering, while retaining the
existing bind-value assertion and JOBS_CANCELLED logger assertion.
In `@apps/api/src/core/services/job-queue/job-queue.service.ts`:
- Around line 137-138: Add `@WithTransaction`() to the scheduleForWalletSetting()
method so its cleanup and replacement-job enqueue operations execute within one
transaction, including calls made without an ambient transaction. Preserve the
existing connection selection and transaction database handling.
---
Nitpick comments:
In `@apps/api/src/core/services/job-queue/job-queue.service.spec.ts`:
- Around line 195-197: Replace the unsafe connection cast in the “cancels
created jobs on the ambient transaction connection when one is active” test with
a typed mock created via mock<Sql>(). Configure its unsafe method directly with
the existing resolved job result, preserving the test’s current 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: 66771909-ee38-42ef-bd26-acd8f321dc6a
📒 Files selected for processing (2)
apps/api/src/core/services/job-queue/job-queue.service.spec.tsapps/api/src/core/services/job-queue/job-queue.service.ts
There was a problem hiding this comment.
The atomicity gap I flagged in cancelCreatedBy (job cleanup running outside the ambient transaction) is fixed in f0d7c2f — it now uses txService.getConnection() when active, mirroring enqueue, with tests covering both the transactional and non-transactional paths. No new issues found in this pass.
Extended reasoning...
Follow-up to my prior inline comment on this PR: the reload-job cleanup / ambient-transaction atomicity gap in JobQueueService.cancelCreatedBy has been fixed in commit f0d7c2f, which routes the cancellation UPDATE through the ambient transaction connection when one is active, falling back to pgBoss's own connection otherwise — matching the pattern already used by enqueue. The fix includes two new unit tests explicitly covering both branches (with and without an active transaction). This directly closes the gap described in my earlier finding, so I'm not asking for anything further on this line. This is an informational note only, not a fresh review of the rest of the PR.
f0d7c2f to
33d4200
Compare
There was a problem hiding this comment.
No new bugs found in this run, but this PR touches financial/billing-critical code (wallet auto top-up scheduling and its job-queue transaction handling) and has already gone through several rounds of substantive fixes (a transaction-atomicity bug in the reschedule path was found and fixed just before this run). Given the scope and sensitivity, I'd like a human to give it a final look before merge.
What was reviewed: the wallet-settings upsert/reschedule flow and its transactional job-queue cleanup, the new AutoTopUpSettingsPopup's client-side validation bounds against the backend schema limits, and the feature-flagged AccountOverview card behavior (switch on/off, edit button, summary text, disabled states). Also checked the candidate issue about the card header tooltip being detached from the title — in the current markup the tooltip sits directly beside the title as a sibling, so it's not actually detached; ruled out.
Extended reasoning...
Overview
This PR is the frontend half of CON-717, adding a fixed-threshold auto top-up UI (AutoTopUpSettingsPopup, reworked AccountOverview card) fully gated behind the auto_reload_fixed_threshold feature flag (off by default), plus backend changes to WalletSettingService/WalletReloadJobService that make the reschedule-on-change path also clean up (cancel) the previously-scheduled reload-check job before re-enqueuing.
Security risks
No auth/crypto changes. The main risk surface is financial correctness: an auto top-up misconfiguration or a lost/duplicated scheduled job could either overcharge, undercharge, or silently stop protecting a user's deployments from running out of credit. A transaction-atomicity gap in this exact area (cancel running outside the ambient @WithTransaction() while enqueue ran inside it) was identified and fixed in this PR's own history (commit f0d7c2f), which is exactly the kind of subtle correctness issue that benefits from a second pair of eyes given how easy it is for a similar gap to be reintroduced elsewhere in the same code paths.
Level of scrutiny
Warrants more than mechanical scrutiny: new user-facing behavior is flag-gated and low blast-radius today, but the backend scheduling/transaction changes affect the existing (already-enabled) predicted-spend auto-reload path too, since scheduleForWalletSetting is shared. That shared surface, plus the financial nature of the feature, pushes this above the bar for silent auto-approval even with zero new findings this run.
Other factors
Test coverage is thorough (unit tests for both the popup and card in all major states, an integration test for the upsert/reschedule flow, and updated JobQueueService tests for the transactional cancel path) and Codecov reports full coverage of changed lines with no failed tests. The PR has already been through multiple review rounds (CodeRabbit, and my own prior inline comments on min-amount validation, label wording, and the transaction atomicity gap) with all issues addressed in subsequent commits, which increases confidence but doesn't change the underlying risk profile enough to skip a human check on a billing-critical, multi-round PR.
Frontend half of CON-717, split out from the combined PR and stacked on the
API branch (it consumes the regenerated wallet-settings types). Adds the
AutoTopUpSettingsPopup dialog and reworks the AccountOverview card under
useFlag("auto_reload_fixed_threshold"). Flag off leaves the current card
unchanged.
Refs CON-717
Addresses review on #3535: - Gate the Auto Top-Up edit button on autoReloadEnabled and the in-flight mutation state, so editing from the off state can no longer silently re-enable auto-charging (saveSettings always submits autoReloadEnabled: true) and a save can't race the switch's disable mutation. - Mirror the backend WalletSettingsInputSchema $10,000 max on the client threshold/amount schema so over-limit values fail inline instead of as a generic 400. Refs CON-717
Addresses review on #3535. The client used useWallet().topUpMinAmountUsd (a trial-aware value for the one-time Add-Funds flow) as the recurring auto-top-up amount minimum, but the backend floors every recurring charge at the fixed STANDARD_TOP_UP_MIN_AMOUNT_USD ($20) in both WalletSettingsInputSchema and the reload-check clamp. Under a non-default MANAGED_WALLET_TRIAL_MIN_TOP_UP_AMOUNT a trialing user could be blocked client-side from saving an amount the API would accept. Mirror the fixed $20 floor with a client constant and drop the now-unused useWallet dependency (the schema no longer needs a runtime factory). Refs CON-717
Label read 'goes below' (strict <) but its description, the AccountOverview card, and the backend reload check all fire when balance <= threshold. Match the label to inclusive semantics.
Editing the threshold/amount on an already-enabled wallet re-enqueued the balance-reload check without cancelling the pending job first. The singleton queue deduped the send and returned null, throwing a 500. Pass withCleanup so the existing created job is cancelled before enqueuing, matching the enable and next-check paths.
…e when unavailable Relocate the edit button from the card header to next to the top-up summary line, and hide it (instead of disabling) when auto top-up is off or no payment method is set.
scheduleImmediate (post-deposit / deployment hooks) re-enqueued the balance-reload check without cancelling the pending daily job first. With the singleton queue this deduped the send, returned null, and threw a 500 on add-funds even though the deposit succeeded on-chain. Pass withCleanup so it cancels the pending job before enqueuing, matching every other schedule path.
Use justify-start so the pencil sits directly after the top-up summary line instead of being pushed to the card's right edge.
The new akash/dependencies-component-or-hook rule (added on main in #3556) restricts the injectable DEPENDENCIES map to React components and hooks. zodResolver is a plain utility, so call it directly instead of injecting it; no test overrides it, so behavior is unchanged.
33d4200 to
7f43f65
Compare
There was a problem hiding this comment.
I don't see a first top-level review from me on this PR yet (my prior activity here was inline findings only), so here's my verdict: no bugs found this run, but I'd like a human to take a pass before merge given the scope and that this wires up real payment auto-charging.
What was reviewed: the AccountOverview card's feature-flagged Auto Top-Up UI (switch on/off, edit button gating, dialog open/close state), the new AutoTopUpSettingsPopup (prefill, validation bounds, enable-on-save vs edit payloads, success/error handling), and the backend wallet-settings/job-scheduling changes (withCleanup on reschedule, wallet-reload-job tests). The header layout and the frontend schema's missing multipleOf(0.01) were both looked at and ruled out as non-issues. The transaction-atomicity question raised earlier in the thread was discussed and resolved (fix reverted from this branch, tracked as a separate pre-existing follow-up).
Extended reasoning...
Overview
This PR adds a feature-flagged (auto_reload_fixed_threshold, off by default) fixed-threshold Auto Top-Up UI: a new AutoTopUpSettingsPopup dialog, a reworked AccountOverview card, and small backend changes to WalletSettingService/WalletReloadJobService to add cleanup-before-reschedule semantics when reload values change.
Security risks
No auth/crypto/permission changes. The main risk surface is financial: this UI enables automatic charges to a user's stored payment method. Client-side validation bounds mirror backend bounds (min/max on threshold and amount), and the backend is the actual source of truth/enforcement, so a client bug here wouldn't bypass server-side charge limits. No injection or data-exposure concerns in the diff.
Level of scrutiny
This should get a human look before merge. It's a size-L PR, gated behind a flag that's off by default (so blast radius is currently zero), but it touches real money-charging logic (auto top-up) and job scheduling, which is a category more sensitive than average feature is UI even though flagged off. There's also a long review history in this thread (CodeRabbit findings, my own earlier inline findings) that were all addressed across multiple follow-up commits — worth a final human pass to confirm the sum of those fixes still hangs together, particularly the wallet-reload-job/job-queue interaction.
Other factors
All bugs raised earlier in this thread (edit-button gating while disabled/in-flight, trial-aware vs fixed minimum amount, label/description wording mismatch, mock-convention nits) were fixed by the author in follow-up commits and verified in the code as currently modified. The one item still under discussion — the cancelCreatedBy transaction-atomicity gap — was explained by the author as pre-existing (present since PR #3533) and explicitly out of scope for this PR (the attempted fix was reverted to keep this PR UI-scoped, with a follow-up tracked separately); CodeRabbit withdrew its finding on that basis. Test coverage looks solid (dedicated specs for both new components plus updated wallet-settings/job service specs), and lint/tsc are reported clean per the PR description.
2 participants
Why
CON-717 replaces the predicted-spend auto-reload with a fixed, user-configurable rule: when the credit balance is at or below threshold X, charge the default payment method exactly amount Y.
This is the frontend half of CON-717, split out from the original combined PR and stacked on the backend PR #3533 — it consumes the wallet-settings types regenerated there, so #3533 must merge first. GitHub will retarget this PR's base to
mainautomatically once #3533 lands.Closes CON-717
What
All new behavior is gated behind
useFlag("auto_reload_fixed_threshold")(off by default). Flag off → the current Auto Top-Up card is unchanged.AutoTopUpSettingsPopup(AkashML-style dialog): threshold + fixed-amount fields with client validation, a read-only default-payment-method row, and enable-on-save vs. edit modes.AccountOverviewcard reworked under the flag: "Auto Top-Up" title, threshold-semantics tooltip, edit button, and a summary line ("Top up $100 when balance ≤ $20"). Toggle-on opens the dialog (no optimistic flip); toggle-off keeps the confirm popup.Testing
AutoTopUpSettingsPopup(prefill, default-card row, validation, enable vs. edit payloads, success/error),AccountOverview(summary variants, switch-on opens dialog without mutating, switch-off confirm, edit button, disabled without payment method), and the wallet-settings query fixtures.npm run lint -- --quietandnpx tsc --noEmitclean for the changed files.Summary by CodeRabbit
New Features
Bug Fixes