Repository navigation
refactor(receive): extract the solver-RFQ receive into its own rail module - #1075
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe Lightning receive flow moves from ChangesLightning receive flow
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Refactor Suggested reviewers: Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 4 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: moving the solver-based Lightning receive flow into a separate rail module.
- Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
Deploying wallet-bitcoin with
|
| Latest commit: |
0c366c5
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ab846427.wallet-bitcoin.pages.dev |
| Branch Preview URL: | https://split-p03-swaprail-refactor.wallet-bitcoin.pages.dev |
Deploying wallet-mutinynet with
|
| Latest commit: |
0c366c5
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://9a77bde3.arkade-wallet.pages.dev |
| Branch Preview URL: | https://split-p03-swaprail-refactor.arkade-wallet.pages.dev |
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Code Review — #1075 refactor(receive): extract the solver-RFQ receive into its own rail module
Pure UI refactor; no VTXO handling, signing, forfeit, or round lifecycle logic is touched. No protocol-critical findings.
✅ What looks good
Extraction is mechanically faithful.
The effect body in swapRail.ts is byte-identical to what was removed from QrCode.tsx. The state reset ordering (clearing flags before the early-return guards, not beside negotiate) is preserved — the comment explaining why is retained too.
abandoned teardown is correct.
The cleanup closure sets abandoned = true; the outstanding cancelSwap call in the resolved-but-abandoned branch is wired to consoleError, not silently swallowed (swapRail.ts:69–71).
lapsed vs refunded mapping preserved.
swapRail.ts:115–118 retains the vocabulary migration note and the correct receiveOutcome === 'lapsed' check. Good — that distinction is real money.
enabled guard placement is right.
!enabled is the first clause in the effect, after the state reset but before any async work. Dep array includes enabled (swapRail.ts:128), so toggling it reruns the effect cleanly.
⚠️ Findings
1. No unit test for useSwapRail itself — specifically the enabled=false path.
src/lib/receive/swapRail.ts (new file, 147 lines)
Danger flagged this: "Source changed with no test changes." The existing integration tests (receive-lightning-error, receive-qrcode, receive-clear-amount) exercise the full component and still pass, but they test enabled=true only (the default). The enabled=false guard is the only net-new behaviour in this PR — it's precisely what the LNURL rail PR will rely on — and it has zero coverage. A useSwapRail.test.ts with a renderHook test verifying that calling receiveLightning is suppressed when enabled=false would close this before the LNURL PR ships. Not a blocker for a pure-extract, but worth tracking.
2. Lint suppression in swapRail.ts doesn't name what it is suppressing.
swapRail.ts:128: // eslint-disable-next-line react-hooks/exhaustive-deps
The same suppression existed in QrCode.tsx, but a standalone hook file is the permanent home for this effect — the reason recvInfo.pendingLnReceive and recvInfo.invoice are intentionally absent from the dep array should be stated here. The existing comment above the negotiate() call explains the semantics, but not why those two specific reads are safe as stale closures. Worth a one-liner before the suppression.
3. Naming proximity to SDK's SwapRailClient / onchainSwapRail.
The new export SwapRail (UI interface, swapRail.ts:11) sits in the same codebase search space as SwapRailClient and onchainSwapRail in ts-sdk/packages/swap. They're in different packages and the concepts don't overlap, but a future reader (or grep) will hit both. Not actionable now — just flagging so the LNURL PR naming stays consistent.
Minor
swapRail.ts:5:extractErroris imported but the same import was deleted fromQrCode.tsx— correct, no dead import.SwapRail.tsx:8–31: the<> … </>fragment wrapping is fine; nothing to render when all three conditions are false costs nothing.QrCode.tsx:SwapsContextimport removed cleanly;LnReceiveContext(if still present for other logic) is unaffected.
Summary: Clean extraction. The only material gap is a test for the enabled=false path before the LNURL rail PR lands on top of this. Everything else is cosmetic.
|
@arkana-ai-bot thanks.
|
|
All three findings verified against 1. 2. Lint suppression — confirmed.
3. Naming — noted. No outstanding findings. Good to go. |
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Follow-up pass on 9965cdd — 1 commit since 069f2fe. All 3 prior findings fixed; 0 remain open.
Prior findings closure
Finding 1 — No test for enabled=false path.
Fixed in 9965cdda (src/test/lib/receive/swapRail.test.tsx, +49 lines).
Both cases are correct: the enabled path uses waitFor to confirm receiveLightning is called with the satoshis amount; the disabled path asserts synchronously (no waitFor needed — the early return fires before any async work starts, and @Kukks confirmed the test fails when the guard is removed). Coverage is sufficient to protect the LNURL rail PR.
Finding 2 — Lint suppression without explanation.
Fixed in 9965cdda (swapRail.ts:121, just above the eslint-disable-next-line):
// invoice and pendingLnReceive are only the guard above: listing them would re-run this on its own result.
Placement (after the cleanup closure, immediately before the suppress comment) is correct. Content is accurate.
Finding 3 — Naming proximity to SDK's SwapRailClient.
No longer applicable as an open action item — acknowledged, no change needed in this PR. #1081 uses useLnurlRail, which keeps the namespace clean.
Incremental diff assessment
No new findings. The two-file diff is entirely additive (test) and cosmetic (comment). No logic, API surface, or protocol path changed.
This PR is clean.
|
@coderabbitai review |
✅ Action performedReview finished.
|
…odule The Lightning receive the Receive screen negotiates through a solver moves out of QrCode.tsx into lib/receive/swapRail.ts (useSwapRail) and its status lines into Receive/SwapRail.tsx. The negotiation effect, its retry and no-driver handling and the lost/claim-failed outcome read are moved verbatim; the screen renders <SwapRailStatus> where it rendered them. useSwapRail takes an `enabled` flag, on by default, so a second receive rail can switch it off; nothing passes it yet. No behaviour change: the existing receive screen tests are the gate. Split out of #1000.
useSwapRail(false) is what the LNURL rail relies on to keep the solver swap off while an lnurl-server is configured, and it had no direct test. The hook test checks an enabled rail negotiates for the amount and a disabled one never calls receiveLightning. The lint suppression now says why the invoice and pendingLnReceive reads stay out of the deps.
9965cdd to
0c366c5
Compare
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Pass 3 of N — rebase sync (9965cdd → 0c366c5): branch rebased onto SDK 0.5 base; 2 prior issues closed, 0 remain, 0 new findings.
Prior-finding closure
1. coverage — Fixed (commit )
covers both paths. The enabled test uses waitFor for the async call; the disabled test asserts synchronously (correct, since the early return is synchronous). Nothing changed here in this push.
2. Lint suppression naming — Fixed (commit )
swapRail.ts line now reads:
// invoice and pendingLnReceive are only the guard above: listing them would re-run this on its own result.
Still present and unchanged in0c366c5.
3. useLnurlRail naming note for PR #1081 — No longer applicable
That was a forward suggestion for a different PR, not a finding on this one.
Incremental diff
The 4 focus files are functionally identical after the rebase. The commit graph shows the branch was rebased onto the SDK 0.5 base (PRs #936, #1087 merged to main), giving the PR commits new SHAs. One cosmetic diff: a trailing {' '} space removed after a JSX element in QrCode.tsx — no behaviour change.
Cloudflare Pages deploy succeeded at 0c366c5, confirming the build is clean against SDK 0.5.
No new findings. No blockers.
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Pass 3 of N — rebase sync (9965cdd → 0c366c5): branch rebased onto SDK 0.5 base; 2 prior issues closed, 0 remain, 0 new findings.
Prior-finding closure
1. enabled=false coverage — Fixed (commit 9965cdd)
src/test/lib/receive/swapRail.test.tsx covers both paths. The enabled test uses waitFor for the async call; the disabled test asserts synchronously (correct, since the early return is synchronous). Nothing changed here in this push.
2. Lint suppression naming — Fixed (commit 9965cdd)
swapRail.ts dep-array line now reads:
// invoice and pendingLnReceive are only the guard above: listing them would re-run this on its own result.
Still present and unchanged in0c366c5.
3. useLnurlRail naming note for PR #1081 — No longer applicable
That was a forward suggestion for a different PR, not a finding on this one.
Incremental diff 9965cdd → 0c366c5
The 4 focus files are functionally identical after the rebase. The commit graph shows the branch was rebased onto the SDK 0.5 base (PRs #936, #1087 merged to main), giving the PR commits new SHAs. One cosmetic diff: a trailing {' '} space removed after a JSX element in QrCode.tsx — no behaviour change.
Cloudflare Pages deploy succeeded at 0c366c5, confirming the build is clean against SDK 0.5.
No new findings. No blockers.
A pure move. The Lightning receive the Receive screen negotiates through a solver leaves
QrCode.tsx:lib/receive/swapRail.ts(useSwapRail);Receive/SwapRail.tsx(<SwapRailStatus>).The moved effect is byte-identical to the base's, apart from the new
enabledguard [ran: diff].useSwapRail(enabled = true). The flag defaults on; nothing passes it until theLNURL rail PR, which turns the swap rail off while an lnurl-server is configured.
Test plan
receive-qrcode,receive-lightning-error,receive-clear-amount(26 tests).useSwapRailhook test: an enabled rail negotiates for the amount; a disabled one never callsreceiveLightning(2 tests).pnpm test:unit: 101 files / 971 tests (CI at 9965cdd).Dependencies: none. The LNURL receive rail PR merges this in.
Split out of #1000.
Split series — supersedes #1000 and #1068; each PR is one feature. Base
masterunless noted:Summary by CodeRabbit