[WIP] Lightning top-up flow#4162
Draft
bznein wants to merge 5 commits into
Draft
Conversation
bznein
force-pushed
the
top-up-flow
branch
3 times, most recently
from
22a4b36 to
ede1800
Compare
Collaborator
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis PR adds a Lightning wallet top-up feature: a new /lightning/top-up route with a TopUp form (account selection, fiat/BTC conversion, fee proposal via useTxProposal), confirmation flow, SendResult integration, styling, and routing; minor Lightning page cleanup and Send component refactor to use the new hook. ChangesLightning top-up feature
Sequence Diagram(s) sequenceDiagram
participant User
participant TopUp as TopUp Component
participant Hook as useTxProposal
participant Propose as accountApi.proposeTx
participant Confirm as ConfirmSend
participant Send as accountApi.sendTx
participant Result as SendResult
User->>TopUp: select source account, enter amount, choose fee
TopUp->>Hook: validateAndDisplayFee(getValidTxInputData)
Hook->>Propose: accountApi.proposeTx(request)
Propose-->>Hook: proposal {fee, amount, total, recipient}
Hook-->>TopUp: proposedFee/proposedAmount/proposedTotal, valid
User->>TopUp: submit -> TopUp calls Confirm
Confirm->>Send: accountApi.sendTx(tx)
Send-->>Result: send result
Result-->>User: show success/error
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🤖 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 `@frontends/web/src/routes/lightning/components/action-buttons.tsx`:
- Around line 20-30: The handleTopUpClick handler currently performs a local
keystore connection flow using connectAnyKeystore; replace this by delegating to
the shared keystore flow: either simply navigate to '/lightning/top-up' (so the
page-level KeystoreConnectPrompt handles prompting) or call the shared
connectKeystore() API and let the KeystoreConnectPrompt handle UI, removing
local connection logic in handleTopUpClick and any use of connectAnyKeystore;
update references in handleTopUpClick (and the similar code at lines ~46-47) to
use navigate('/lightning/top-up') or connectKeystore() + KeystoreConnectPrompt
instead of the local connectAnyKeystore flow.
In `@frontends/web/src/routes/lightning/top-up.tsx`:
- Around line 255-276: When a proposal fails the component never clears the
loading flag, so add calls to setIsUpdatingProposal(false) when handling failed
or completed proposals: after assigning proposePromise from
accountApi.proposeTx(...) keep the existing comparison checks against
lastProposal.current and ensure that in the catch block you call
setIsUpdatingProposal(false) when (proposePromise === lastProposal.current), and
likewise in the finally block when clearing lastProposal.current also call
setIsUpdatingProposal(false) if (proposePromise === lastProposal.current);
reference setIsUpdatingProposal, proposeTimeout, lastProposal,
accountApi.proposeTx, and the existing try/catch/finally around proposePromise.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ac5fcff1-5d08-4ac9-9ada-2441b527fa1e
📒 Files selected for processing (8)
frontends/web/src/locales/en/app.jsonfrontends/web/src/routes/account/send/components/result.tsxfrontends/web/src/routes/lightning/components/action-buttons.module.cssfrontends/web/src/routes/lightning/components/action-buttons.tsxfrontends/web/src/routes/lightning/lightning.tsxfrontends/web/src/routes/lightning/top-up.module.cssfrontends/web/src/routes/lightning/top-up.tsxfrontends/web/src/routes/router.tsx
bznein
force-pushed
the
top-up-flow
branch
2 times, most recently
from
9c54add to
3cfe0e4
Compare
Collaborator
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@frontends/web/src/routes/account/send/use-tx-proposal.ts`:
- Around line 97-108: The hook currently captures txInput before the debounce
window, causing stale values to be proposed; fix by moving the call to
getValidTxInputData() inside the proposeTimeout callback (and re-check
accountCode there), ensure you clear any existing proposeTimeout before
scheduling a new setTimeout, and only call accountApi.proposeTx(accountCode,
txInput) with the freshly read txInput; keep existing behavior around
clearOnInvalidInput -> clearProposal() and setIsUpdatingProposal(true) but base
those actions on the validated input read inside the timeout.
- Around line 114-123: The catch block for proposePromise (variables:
proposePromise, lastProposal.current) currently clears validity and logs but
doesn't set a user-visible error after setErrorHandling({}) was previously
called; update the catch to (1) set an explicit user-facing error via
setErrorHandling({ message: String(error) }) or a structured error object so the
UI explains why send is disabled, (2) clear or remove the stale proposal data
(e.g., clear lastProposal.current and any proposal state tied to it) so the UI
doesn't keep showing the old proposal, and (3) keep the existing setValid(false)
and setIsUpdatingProposal(false) only when proposePromise ===
lastProposal.current to avoid race conditions; ensure the finally block still
resets lastProposal.current and setIsUpdatingProposal(false) conditionally to
avoid leaving inconsistent state.
In `@frontends/web/src/routes/lightning/top-up.tsx`:
- Around line 125-153: The effect that calls
connectKeystore(accountToConnect.keystore.rootFingerprint) doesn't handle
promise rejection, so connectPromptPending can remain true; update the effect to
catch errors from connectKeystore and in both then and catch ensure
setConnectPromptPending(false) is called (guarded by mounted.current), and on
rejection clear connectPromptedFingerprint if appropriate; apply the same
pattern to the other call site around lines 348-350 so neither path leaves the
spinner stuck.
- Around line 125-153: Replace the page-local keystore connection state machine
(remove connectPromptPending, connectPromptedFingerprint, accountNeedsConnect
and the second useEffect that calls connectKeystore) and instead invoke the
shared keystore flow: call the existing connectKeystore(rootFingerprint) only
via the shared KeystoreConnectPrompt component used elsewhere; render
KeystoreConnectPrompt when accountToConnect (from sourceAccount ||
preferredBitcoinAccount) exists and !accountToConnect.keystore.connected,
passing accountToConnect.keystore.rootFingerprint and any required
callbacks/props, and rely on that component to handle prompting/pending/errors
rather than managing local state in this file.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1619ade3-0345-42ac-8bec-8a893909f79c
📒 Files selected for processing (10)
frontends/web/src/locales/en/app.jsonfrontends/web/src/routes/account/send/components/result.tsxfrontends/web/src/routes/account/send/send.tsxfrontends/web/src/routes/account/send/use-tx-proposal.tsfrontends/web/src/routes/lightning/components/action-buttons.module.cssfrontends/web/src/routes/lightning/components/action-buttons.tsxfrontends/web/src/routes/lightning/lightning.tsxfrontends/web/src/routes/lightning/top-up.module.cssfrontends/web/src/routes/lightning/top-up.tsxfrontends/web/src/routes/router.tsx
💤 Files with no reviewable changes (1)
- frontends/web/src/routes/lightning/lightning.tsx
✅ Files skipped from review due to trivial changes (2)
- frontends/web/src/routes/lightning/components/action-buttons.module.css
- frontends/web/src/routes/lightning/components/action-buttons.tsx
Collaborator
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@frontends/web/src/routes/account/send/components/result.tsx`:
- Around line 124-133: The secondary "New transaction" button is rendered even
when onContinue is undefined, producing a no-op action; change the rendering so
the secondary Button inside ViewButtons is only rendered when onContinue is
provided (e.g., wrap the secondary Button with a conditional check on
onContinue) so that ViewButtons only contains actionable controls when
showSuccessActions is true and onContinue exists; reference the existing symbols
showSuccessActions, ViewButtons, Button, onContinue, navigate, and donePath to
locate and update the JSX accordingly.
In `@frontends/web/src/routes/lightning/top-up-form.tsx`:
- Around line 141-143: The current JSX renders "-" while lightningBalance is
undefined; change the conditional in the TopUp form so that lightningBalance ===
undefined returns nothing (or a Skeleton component) to represent loading, and
only render AmountWithUnit when lightningBalance is a defined object (and keep
"-" only for an explicit null/empty state if needed). Update the conditional
around lightningBalance and the AmountWithUnit usage so undefined maps to a
loading render (e.g., null or <Skeleton />) and a defined value maps to
<AmountWithUnit maxDecimals={9} amount={lightningBalance.available} />.
In `@frontends/web/src/routes/lightning/top-up-hooks.ts`:
- Around line 110-115: The current call to connectKeystore(...) only clears
setConnectPromptPending(true) on success, so if connectKeystore rejects the UI
stays stuck; update the logic around
connectKeystore(accountToConnect.keystore.rootFingerprint) (or the function that
triggers it) to use a finally path (either .catch/.finally chain or async/await
with try { await connectKeystore(...) } finally { if (mounted.current)
setConnectPromptPending(false) }) so setConnectPromptPending(false) always runs
regardless of success or failure and still guards with mounted.current before
updating state.
- Around line 105-116: The useEffect currently calls connectKeystore(...)
automatically when accountNeedsConnect/accountToConnect change; instead remove
the connectKeystore call from this state-only hook (keep only setting
setConnectPromptedFingerprint and setConnectPromptPending based on
accountNeedsConnect/accountToConnect/mounted) and wire the actual
connectKeystore(...) invocation to an explicit user action handler (e.g., the
send/review click flow) so the device connection happens only in response to
user intent; update callers to invoke
connectKeystore(accountToConnect.keystore.rootFingerprint) from the action path
and follow the explicit-action patterns used in the sign-message, widget, and
pocket flows.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 54e981c2-f3dd-47b2-97a3-a4714f8c0573
📒 Files selected for processing (13)
frontends/web/src/locales/en/app.jsonfrontends/web/src/routes/account/send/components/result.tsxfrontends/web/src/routes/account/send/send.tsxfrontends/web/src/routes/account/send/use-tx-proposal.tsfrontends/web/src/routes/lightning/components/action-buttons.module.cssfrontends/web/src/routes/lightning/components/action-buttons.tsxfrontends/web/src/routes/lightning/lightning.tsxfrontends/web/src/routes/lightning/top-up-draft.tsfrontends/web/src/routes/lightning/top-up-form.tsxfrontends/web/src/routes/lightning/top-up-hooks.tsfrontends/web/src/routes/lightning/top-up.module.cssfrontends/web/src/routes/lightning/top-up.tsxfrontends/web/src/routes/router.tsx
💤 Files with no reviewable changes (1)
- frontends/web/src/routes/lightning/lightning.tsx
Comment on lines
+124
to
+133
| {showSuccessActions && ( | ||
| <ViewButtons> | ||
| <Button primary onClick={() => navigate(donePath)}> | ||
| {t('button.done')} | ||
| </Button> | ||
| <Button secondary onClick={() => onContinue?.()}> | ||
| {t('send.newTransaction')} | ||
| </Button> | ||
| </ViewButtons> | ||
| )} |
There was a problem hiding this comment.
Avoid rendering a no-op “New transaction” button.
When showSuccessActions is true but onContinue is not provided, the secondary button does nothing. Guard rendering of that button on onContinue (or make it required in that case) to prevent a dead UI action.
🤖 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 `@frontends/web/src/routes/account/send/components/result.tsx` around lines 124
- 133, The secondary "New transaction" button is rendered even when onContinue
is undefined, producing a no-op action; change the rendering so the secondary
Button inside ViewButtons is only rendered when onContinue is provided (e.g.,
wrap the secondary Button with a conditional check on onContinue) so that
ViewButtons only contains actionable controls when showSuccessActions is true
and onContinue exists; reference the existing symbols showSuccessActions,
ViewButtons, Button, onContinue, navigate, and donePath to locate and update the
JSX accordingly.
Comment on lines
+141
to
+143
| {lightningBalance ? ( | ||
| <AmountWithUnit maxDecimals={9} amount={lightningBalance.available} /> | ||
| ) : '-'} |
There was a problem hiding this comment.
Use skeleton or empty render for undefined balance loading state.
While lightningBalance is undefined, this currently renders "-". Render nothing (or a skeleton) for the loading state to match the route loading-state convention.
As per coding guidelines: "Use undefined to represent 'loading' state in data loading hooks; never use a separate isLoading boolean. Render a skeleton or nothing while the value is undefined."
🤖 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 `@frontends/web/src/routes/lightning/top-up-form.tsx` around lines 141 - 143,
The current JSX renders "-" while lightningBalance is undefined; change the
conditional in the TopUp form so that lightningBalance === undefined returns
nothing (or a Skeleton component) to represent loading, and only render
AmountWithUnit when lightningBalance is a defined object (and keep "-" only for
an explicit null/empty state if needed). Update the conditional around
lightningBalance and the AmountWithUnit usage so undefined maps to a loading
render (e.g., null or <Skeleton />) and a defined value maps to <AmountWithUnit
maxDecimals={9} amount={lightningBalance.available} />.
Comment on lines
+105
to
+116
| useEffect(() => { | ||
| if (!accountNeedsConnect || !accountToConnect) { | ||
| return; | ||
| } | ||
| setConnectPromptedFingerprint(accountToConnect.keystore.rootFingerprint); | ||
| setConnectPromptPending(true); | ||
| connectKeystore(accountToConnect.keystore.rootFingerprint).then(() => { | ||
| if (mounted.current) { | ||
| setConnectPromptPending(false); | ||
| } | ||
| }); | ||
| }, [accountNeedsConnect, accountToConnect, mounted]); |
There was a problem hiding this comment.
Trigger keystore connection from an explicit user action, not from effect-driven state.
This effect starts connectKeystore() automatically when state conditions match, which creates an implicit connection flow in a route hook. Move the trigger to a user action path (e.g., send/review action) and keep this hook state-only.
As per coding guidelines: "Trigger connectKeystore() from explicit user actions that need the device, following existing simple flows in src/routes/account/sign-message/use-sign-message.ts, src/routes/bitsurance/widget.tsx, and src/routes/market/pocket.tsx."
🤖 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 `@frontends/web/src/routes/lightning/top-up-hooks.ts` around lines 105 - 116,
The useEffect currently calls connectKeystore(...) automatically when
accountNeedsConnect/accountToConnect change; instead remove the connectKeystore
call from this state-only hook (keep only setting setConnectPromptedFingerprint
and setConnectPromptPending based on
accountNeedsConnect/accountToConnect/mounted) and wire the actual
connectKeystore(...) invocation to an explicit user action handler (e.g., the
send/review click flow) so the device connection happens only in response to
user intent; update callers to invoke
connectKeystore(accountToConnect.keystore.rootFingerprint) from the action path
and follow the explicit-action patterns used in the sign-message, widget, and
pocket flows.
Comment on lines
+110
to
+115
| setConnectPromptPending(true); | ||
| connectKeystore(accountToConnect.keystore.rootFingerprint).then(() => { | ||
| if (mounted.current) { | ||
| setConnectPromptPending(false); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Clear connectPromptPending on both success and failure.
If connectKeystore() rejects, connectPromptPending stays true indefinitely. Reset it in a finally path so the UI cannot get stuck in a pending state.
🤖 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 `@frontends/web/src/routes/lightning/top-up-hooks.ts` around lines 110 - 115,
The current call to connectKeystore(...) only clears
setConnectPromptPending(true) on success, so if connectKeystore rejects the UI
stays stuck; update the logic around
connectKeystore(accountToConnect.keystore.rootFingerprint) (or the function that
triggers it) to use a finally path (either .catch/.finally chain or async/await
with try { await connectKeystore(...) } finally { if (mounted.current)
setConnectPromptPending(false) }) so setConnectPromptPending(false) always runs
regardless of success or failure and still guards with mounted.current before
updating state.
583a556 to
657da26
Compare
Regular send and Lightning top up both need the same transaction proposal lifecycle: debounce user edits, ignore stale proposals, expose fee and amount details, and convert proposed amounts to fiat. Keep that behavior in one hook so future fixes to proposal loading or error handling apply to both flows instead of being copied between screens.
Lightning top up reuses the send result component but does not offer a follow-up transaction action. Rendering the secondary button without an onContinue handler creates a visible no-op control. Only render the secondary action when a caller provides onContinue, while leaving the normal send result behavior unchanged.
The top up screen needs Lightning-specific data and Bitcoin account selection before it can build a transaction proposal. Keeping this in the route mixed data loading with screen flow. Move boarding address loading, Lightning balance loading, source account selection, and draft state into focused hooks. The hooks keep the route small while still using the existing send proposal API.
The top up UI is separate from route orchestration so the flow can be reviewed in smaller pieces. It still needs the familiar send controls because top up is an on-chain Bitcoin send to a Spark boarding address. Build the form from existing send components for fiat input, fee targets, confirmation, and send result. This keeps the approval path consistent with regular Bitcoin sends.
Users need a way to fund their Lightning wallet from an active Bitcoin account in the app. The flow should not send them to add an account when a BitBox is disconnected or an existing account only needs to be enabled. Add the route and action entry point, connect the BitBox from the explicit top up action, prefer the Bitcoin account tied to Lightning, and send users to account management when no active Bitcoin account is available.
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
1 participant
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.
Before asking for reviews, here is a check list of the most common things you might need to consider: