Repository navigation
Conversation
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: João Bordalo <bordalix@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/screens/Wallet/Receive/QrCode.tsx (2)
58-87: Prevent setState after unmount; add missing deps; use functional updates.Guard async chain and include dependencies to avoid stale closures.
-useEffect(() => { +useEffect(() => { + let cancelled = false // if boltz is available and amount is between limits, let's create a swap invoice - if (validLnSwap(satoshis) && wallet && svcWallet) { - swapProvider + if (validLnSwap(satoshis) && wallet && svcWallet && swapProvider) { + swapProvider ?.createReverseSwap(satoshis) .then((pendingSwap) => { if (!pendingSwap) throw new Error('Failed to create reverse swap') const invoice = pendingSwap.response.invoice - setRecvInfo({ ...recvInfo, invoice }) + if (cancelled) return + setRecvInfo((prev) => ({ ...prev, invoice })) setInvoice(invoice) consoleLog('Reverse swap invoice created:', invoice) - swapProvider + return swapProvider .waitAndClaim(pendingSwap) - .then(() => { - setRecvInfo({ ...recvInfo, satoshis: pendingSwap.response.onchainAmount }) - navigate(Pages.ReceiveSuccess) - }) + .then(() => { + if (cancelled) return + setRecvInfo((prev) => ({ ...prev, satoshis: pendingSwap.response.onchainAmount })) + navigate(Pages.ReceiveSuccess) + }) .catch((error) => { - setShowQrCode(true) - consoleError(error, 'Error claiming reverse swap:') + if (cancelled) return + setShowQrCode(true) + consoleError(error, 'Error claiming reverse swap') }) }) .catch((error) => { - setShowQrCode(true) - consoleError(error, 'Error creating reverse swap:') + if (cancelled) return + setShowQrCode(true) + consoleError(error, 'Error creating reverse swap') }) } else { setShowQrCode(true) } -}, [satoshis]) + return () => { + cancelled = true + } +}, [satoshis, wallet, svcWallet, swapProvider, validLnSwap, navigate, setRecvInfo])
95-101: Avoid in-place sort; use functional update..sort() mutates vtxos.spendable; also avoid stale recvInfo.
- const lastVtxo = vtxos.spendable.sort((a, b) => b.createdAt.getTime() - a.createdAt.getTime())[0] + const lastVtxo = [...vtxos.spendable].sort((a, b) => b.createdAt.getTime() - a.createdAt.getTime())[0] @@ - setRecvInfo({ ...recvInfo, satoshis: value }) + setRecvInfo((prev) => ({ ...prev, satoshis: value }))
♻️ Duplicate comments (5)
src/lib/format.ts (1)
38-48: Boundary and shadowing fixes in prettyDelta look good.Using >= at unit boundaries and renaming the shadowed seconds variable resolves prior feedback.
package.json (3)
88-88: Engines: Node >=20 — good.Align CI/Docker/.nvmrc to Node 20+ to avoid mismatch.
21-21: DOMPurify: enforce strict sanitize config at call sites.Default sanitize() is permissive. Harden config wherever used (e.g., AlertBox).
If you want, I can supply a tiny wrapper like sanitizeStrict(html, optsOverride).
27-27: react-qr-barcode-scanner: add secure-context guards and lazy-load.Mount only when isSecureContext && navigator.mediaDevices?.getUserMedia exist; lazy-load to avoid SSR/import-time issues.
src/screens/Wallet/Receive/QrCode.tsx (1)
35-37: Avoid emitting invalid BIP21 when on-chain address is absent; guard encodeBip21 and add deps.Currently, address='' yields
bitcoin:?...(invalid). Only build BIP21 when address exists; otherwise fall back to invoice for QR.- const address = validUtxoTx(satoshis) && utxoTxsAllowed() ? boardingAddr : '' - const arkAddress = validVtxoTx(satoshis) && vtxoTxsAllowed() ? offchainAddr : '' + const address = validUtxoTx(satoshis) && utxoTxsAllowed() ? boardingAddr : undefined + const arkAddress = validVtxoTx(satoshis) && vtxoTxsAllowed() ? offchainAddr : undefined @@ - const defaultBip21uri = encodeBip21(address, arkAddress, '', satoshis) + const defaultBip21uri = address ? encodeBip21(address, arkAddress ?? '', '', satoshis) : '' @@ -useEffect(() => { - const bip21uri = encodeBip21(address, arkAddress, invoice, satoshis) - setBip21uri(bip21uri) - setQrValue(bip21uri) - if (invoice) setShowQrCode(true) -}, [invoice]) +useEffect(() => { + if (address) { + const uri = encodeBip21(address, arkAddress ?? '', invoice, satoshis) + setBip21uri(uri) + setQrValue(uri) + } else if (invoice) { + setBip21uri(invoice) + setQrValue(invoice) + } + if (invoice) setShowQrCode(true) +}, [address, arkAddress, invoice, satoshis])Also applies to: 43-43, 50-56
🧹 Nitpick comments (14)
src/lib/format.ts (2)
51-52: Optional: floor seconds to avoid fractional output.If callers ever pass fractional seconds, we’ll print decimals. Floor for consistency with other branches.
- const secs = delta + const secs = Math.floor(delta)
76-83: Coerce before falsy check and avoid param reassignment in prettyLongText.Move coercion ahead to handle numeric inputs (e.g., 0) correctly and use a local const.
-export const prettyLongText = (str?: string, showChars = 11): string => { - if (!str) return '' - str = String(str) - if (str.length <= showChars * 2 + 4) return str - const left = str.substring(0, showChars) - const right = str.substring(str.length - showChars, str.length) - return `${left}...${right}` +export const prettyLongText = (str?: string, showChars = 11): string => { + const s = String(str ?? '') + if (s.length === 0) return '' + if (s.length <= showChars * 2 + 4) return s + const left = s.substring(0, showChars) + const right = s.substring(s.length - showChars, s.length) + return `${left}...${right}`src/test/fixtures.json (4)
10-11: Network mix (bcrt1 vs 1BgGZ…) may confuse future address validation.You mix regtest Bech32 (bcrt1…) with legacy mainnet vectors (1BgGZ…). That’s fine for parsing tests, but if validation becomes network-aware, this will create false negatives. Consider tagging vectors with
networkor splitting per network.Also applies to: 17-19
160-165: Canonical BIP21 string is order- and format-sensitive; keep fixture synced with encoder.This fixture asserts exact order and decimal formatting. Any change in
encodeBip21(param ordering, prettyNumber behavior) will break tests. Add a note that the canonical order isark,lightning,amountand decimals must be fixed-point (no scientific notation).
15-113: Upstream vector provenance; add attribution to avoid licensing ambiguity.These cases resemble upstream bitcoinjs bip21 vectors. Add attribution in a NOTICE/README near tests identifying the source and license.
I can draft the NOTICE snippet if you confirm the source.
Also applies to: 114-158
13-13: Unusedarknotefixture—keep only if covered to avoid drift.If no test uses
arknoteyet, either add a test or move it to a dedicated arknote fixture to reduce noise.src/test/lib/bip21.test.ts (5)
13-15: Error path covered.Covers non-
bitcoin:scheme. Consider addingbitcoin://to assert non-compliance still throws if you choose strict-only behavior.
18-22: String-equality on encoder is acceptable but brittle.If encoder formatting changes (decimals, param order), this test will fail. Consider asserting decoded equivalence instead of exact string match.
Proposed tweak:
- expect(encodeBip21(address, arkAddress, invoice, satoshis)).toEqual(bip21) + const encoded = encodeBip21(address, arkAddress, invoice, satoshis) + expect(decodeBip21(encoded)).toEqual({ arkAddress, address, invoice, satoshis, lnurl: undefined })
46-52: Invalid-case coverage skips entries withouturi.If you want full parity with fixtures, generate URIs for those entries (e.g.,
bitcoin:${address}?amount=${options.amount}) and assert they throw too.I can provide a helper to build these URIs safely if you want to expand coverage.
6-16: Add lnurl coverage.Decode supports
lightning=lnurl…but tests don’t. Add a case to ensurelnurlis set andinvoiceis undefined.Example:
describe('decodeBip21', () => { + it('parses lnurl when present', () => { + const uri = 'bitcoin:1BgGZ9tcN4rm9KBzDn7KprQz87SZ26SAMH?amount=1&lightning=lnurl1dp68gurn8ghj7mrfde' + const out = decodeBip21(uri) + expect(out).toMatchObject({ + address: '1BgGZ9tcN4rm9KBzDn7KprQz87SZ26SAMH', + satoshis: toSatoshis(1), + lnurl: expect.stringMatching(/^lnurl/i), + invoice: undefined, + }) + })
25-31: Naming nit: “bip21.js tests”.Consider renaming to “upstream bip21 vectors” to reflect provenance and TypeScript context.
package.json (1)
64-64: Remove @types/qs from package.json
@types/qs isn’t needed—qs isn’t listed as a dependency and isn’t used in the codebase.src/screens/Wallet/Receive/QrCode.tsx (2)
111-126: Polling: stabilize UTXO detection and harden error handling.includes() on objects relies on reference equality; add try/catch for reloadWallet.
- const interval = setInterval(async () => { + const interval = setInterval(async () => { const utxos = await svcWallet!.getBoardingUtxos() if (utxos.length < currentUtxos.length) { currentUtxos = utxos } if (utxos.length > currentUtxos.length) { - const newUtxo = utxos.find((utxo) => !currentUtxos.includes(utxo)) + // TODO: replace reference check with stable key comparison (e.g., txid:index) + const newUtxo = utxos.find((u) => !currentUtxos.includes(u)) if (newUtxo) { currentUtxos = utxos - setRecvInfo({ ...recvInfo, satoshis: newUtxo.value }) - await reloadWallet() + setRecvInfo((prev) => ({ ...prev, satoshis: newUtxo.value })) + try { await reloadWallet() } catch (e) { consoleError(e, 'Error reloading wallet') } notifyPaymentReceived(newUtxo.value) navigate(Pages.ReceiveSuccess) } } }, 5_000)To confirm a stable key you can use (txid, index/vout) from ExtendedCoin; please verify field names in the SDK.
130-138: Minor: define share payload before handler to avoid temporal coupling.Small readability improvement; no behavior change.
- const handleShare = () => { - setSharing(true) - shareData(data) - .catch(consoleError) - .finally(() => setSharing(false)) - } - - const data = { title: 'Receive', text: qrValue } + const data = { title: 'Receive', text: qrValue } + + const handleShare = () => { + setSharing(true) + shareData(data).catch(consoleError).finally(() => setSharing(false)) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
package.json(4 hunks)src/lib/address.ts(2 hunks)src/lib/bip21.ts(2 hunks)src/lib/format.ts(2 hunks)src/screens/Settings/Logs.tsx(2 hunks)src/screens/Wallet/Receive/QrCode.tsx(4 hunks)src/test/components/CheckList.test.tsx(1 hunks)src/test/fixtures.json(1 hunks)src/test/lib/bip21.test.ts(1 hunks)src/test/lib/format.test.ts(1 hunks)src/test/setup.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/screens/Settings/Logs.tsx
- src/test/setup.ts
- src/lib/address.ts
- src/lib/bip21.ts
- src/test/lib/format.test.ts
- src/test/components/CheckList.test.tsx
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-28T15:58:17.212Z
Learnt from: bordalix
PR: arkade-os/wallet#142
File: src/lib/bip21.ts:7-13
Timestamp: 2025-08-28T15:58:17.212Z
Learning: In src/lib/bip21.ts, the encodeBip21 function is used by the wallet which always has an arkAddress available to pass to the function, while decodeBip21 is used to parse received possible BIP21 URIs which may not contain arkAddress. This explains why arkAddress is required in encodeBip21 but optional in the Bip21Decoded interface.
Applied to files:
src/screens/Wallet/Receive/QrCode.tsxsrc/test/lib/bip21.test.ts
📚 Learning: 2025-06-30T18:33:29.839Z
Learnt from: bordalix
PR: arkade-os/wallet#114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
Applied to files:
package.json
📚 Learning: 2025-07-07T14:41:35.991Z
Learnt from: bordalix
PR: arkade-os/wallet#106
File: src/lib/bolt11.ts:153-155
Timestamp: 2025-07-07T14:41:35.991Z
Learning: In src/lib/bolt11.ts, the code is copied from the original bitcoinjs/bolt11 library (https://github.com/bitcoinjs/bolt11/blob/master/payreq.js) and should be kept unchanged to maintain compatibility with the upstream source, even if there are potential improvements or syntax issues.
Applied to files:
src/test/fixtures.json
🧬 Code graph analysis (2)
src/screens/Wallet/Receive/QrCode.tsx (6)
src/providers/flow.tsx (1)
FlowContext(82-95)src/providers/lightning.tsx (1)
LightningContext(21-25)src/providers/wallet.tsx (1)
WalletContext(42-56)src/providers/limits.tsx (1)
LimitsContext(29-40)src/lib/bip21.ts (1)
encodeBip21(65-72)src/lib/logs.ts (1)
consoleError(40-44)
src/test/lib/bip21.test.ts (2)
src/lib/bip21.ts (2)
decodeBip21(16-63)encodeBip21(65-72)src/lib/format.ts (1)
toSatoshis(8-10)
🔇 Additional comments (5)
src/test/lib/bip21.test.ts (2)
9-11: Good golden-path assertion.Directly validates all key fields including absence of
lnurl. Solid baseline.
2-2: No action needed:resolveJsonModuleis already enabled
The tsconfig.json includes"resolveJsonModule": trueon line 14, so importing JSON modules will work as expected.package.json (2)
35-36: Nice add: UI test/coverage scripts.Works with Vitest UI/coverage. No changes requested.
7-8: Approve SDK and boltz-swap bumps. Ensure all updated APIs (createReverseSwap, createSubmarineSwap, waitAndClaim, getLimits, getSwapHistory, ArkNote, ServiceWorkerWallet, etc.) still compile and behave correctly—run integration tests and review each package’s changelog for any breaking changes.src/screens/Wallet/Receive/QrCode.tsx (1)
19-21: Imports: LightningContext + encodeBip21 — OK.Good modularization.
| "@testing-library/jest-dom": "^6.8.0", | ||
| "@testing-library/react": "^16.3.0", | ||
| "@testing-library/user-event": "^14.6.1", |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
ESLint 7 + ts-eslint v4 are incompatible with TypeScript 5.8 — upgrade lint stack.
With typescript ^5.8.3, keep ESLint ≥8 and @typescript-eslint ≥7; also bump airbnb config accordingly.
Apply this diff:
"devDependencies": {
- "@typescript-eslint/eslint-plugin": "^4.33.0",
- "@typescript-eslint/parser": "^4.33.0",
+ "@typescript-eslint/eslint-plugin": "^7.18.0",
+ "@typescript-eslint/parser": "^7.18.0",
- "eslint": "^7.32.0",
- "eslint-config-airbnb": "^18.2.1",
+ "eslint": "^8.57.0",
+ "eslint-config-airbnb": "^19.0.4",
"eslint-config-prettier": "^8.10.0",
"eslint-plugin-import": "^2.31.0",
"eslint-plugin-jsx-a11y": "^6.10.2",
"eslint-plugin-prettier": "^4.2.1",
"eslint-plugin-react": "^7.37.5",
"eslint-plugin-react-hooks": "^4.6.2",
"jsdom": "^26.1.0",
"typescript": "^5.8.3",
"vite": "^7.1.3",
"vitest": "^3.2.4"
}Quick check:
Also applies to: 79-79, 81-84
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
jq -r '.devDependencies | {typescript, eslint, "@typescript-eslint/eslint-plugin", "@typescript-eslint/parser", "eslint-config-airbnb"}' package.jsonLength of output: 351
Upgrade lint stack for TS 5.8 compatibility
TS 5.8 support was added in typescript-eslint v8.26.0; bump ESLint to ^8.57.0, @typescript-eslint/parser & plugin to ^8.26.0, and eslint-config-airbnb to ^19.0.4. (classic.yarnpkg.com, typescript-eslint.io, unpkg.com)
🤖 Prompt for AI Agents
In package.json around lines 60 to 62, the lint toolchain is out of date for
TypeScript 5.8; update devDependencies: set "eslint" to ^8.57.0,
"@typescript-eslint/parser" and "@typescript-eslint/eslint-plugin" to ^8.26.0,
and "eslint-config-airbnb" to ^19.0.4 (adjust any peer deps if required), then
run your package manager to regenerate the lockfile (yarn install or npm
install) and run the linter to resolve any new rule issues introduced by the
upgraded packages.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (10)
.eslintrc.json (1)
56-63: Avoid style-rule overlap with Prettier; remove quotes rule and enable Prettier integrationHaving ESLint’s
quotesalongside Prettier typically creates redundant/contradictory reports. Prefer enforcing via Prettier.Apply within this range:
- "quotes": [ - "error", - "single", - { - "avoidEscape": true, - "allowTemplateLiterals": true - } - ]Additionally, enable Prettier via extends (outside this range, top-level of the config):
{ "extends": ["plugin:prettier/recommended"], "plugins": ["react", "react-hooks", "@typescript-eslint", "prettier"], "rules": { "prettier/prettier": "error", ... } }src/lib/lnurl.ts (2)
74-76: Bug: UTF-8 conversion direction is reversed; returns bytes instead of stringbech32.decodeToBytes gives URL bytes; you must decode bytes → string. Current code likely returns Uint8Array, breaking callers expecting string.
Apply:
- const { bytes } = bech32.decodeToBytes(lnurl) - return utf8.encode(bytes) + const { bytes } = bech32.decodeToBytes(lnurl) + return utf8.decode(bytes)
41-46: Build callback URL safely and encode noteAvoid manual string concatenation and ensure proper encoding of comment and coexistence with existing query params.
Apply:
-const fetchLnUrlInvoice = async (amount: number, note: string, data: LnUrlResponse) => { - let url = `${data.callback}?amount=${amount}` - if (note) url += `&comment=${note}` - const res = await fetch(url).then(checkResponse<LnUrlCallbackResponse>) - return res.pr -} +const fetchLnUrlInvoice = async (amount: number, note: string, data: LnUrlResponse) => { + const u = new URL(data.callback) + u.searchParams.set('amount', String(amount)) + if (note) u.searchParams.set('comment', note) // URL API handles encoding + const res = await fetch(u.toString()).then(checkResponse<LnUrlCallbackResponse>) + return res.pr +}src/lib/format.ts (1)
12-19: prettyAgo: fix epoch unit detection (ms/µs) and align with summaryCurrent logic only handles ms (threshold-based) and misses µs; also inconsistent with the summary claiming truthiness-based parsing. Prefer magnitude-based detection for s/ms/µs.
Apply:
const unixTimestamp = typeof timestamp === 'string' ? Math.floor(new Date(timestamp).getTime() / 1000) - : timestamp > 200_000_000_000 - ? Math.floor(timestamp / 1000) - : timestamp + : (() => { + const t = Math.abs(timestamp) + if (t >= 1_000_000_000_000_000) return Math.floor(timestamp / 1_000_000) // microseconds → seconds + if (t >= 1_000_000_000_000) return Math.floor(timestamp / 1_000) // milliseconds → seconds + return Math.floor(timestamp) // seconds + })()src/screens/Wallet/Receive/Amount.tsx (3)
57-58: Fix rules-of-hooks violation: early return precedes later hooksMove the svcWallet loading early-return below all hooks so hooks are always called in the same order.
- if (!svcWallet) return <Loading text='Loading...' /> @@ + // place this after the last hook + if (!svcWallet) return <Loading text='Loading...' />Also applies to: 88-88
59-71: Guard wallet-dependent effect, add deps, avoid string throws, and prevent setState after unmountWithout guarding, this effect runs on mount (svcWallet may be undefined); it also throws strings and lacks unmount protection.
-useEffect(() => { - getReceivingAddresses(svcWallet) - .then(({ offchainAddr, boardingAddr }) => { - if (!offchainAddr) throw 'Unable to get offchain address' - if (!boardingAddr) throw 'Unable to get boarding address' - setRecvInfo({ boardingAddr, offchainAddr, satoshis: 0 }) - }) - .catch((err) => { - const error = extractError(err) - consoleError(error, 'error getting addresses') - setError(error) - }) -}, []) +useEffect(() => { + if (!svcWallet) return + let cancelled = false + getReceivingAddresses(svcWallet) + .then(({ offchainAddr, boardingAddr }) => { + if (cancelled) return + if (!offchainAddr) throw new Error('Unable to get offchain address') + if (!boardingAddr) throw new Error('Unable to get boarding address') + setRecvInfo({ boardingAddr, offchainAddr, satoshis: 0 }) + }) + .catch((err) => { + const errorMsg = extractError(err) + consoleError(errorMsg, 'error getting addresses') + setError(errorMsg) + }) + return () => { + cancelled = true + } +}, [svcWallet])
94-107: Harden faucet flow (validate prerequisites, use Error, and prevent double submits)Guard missing address, validate satoshis, and disable the Faucet button while running.
const handleFaucet = async () => { try { - if (!amount) throw 'Invalid amount' + if (!satoshis) throw new Error('Invalid amount') + if (!recvInfo?.offchainAddr) throw new Error('Missing offchain address') setFauceting(true) const ok = await callFaucet(recvInfo.offchainAddr, satoshis, aspInfo) - if (!ok) throw 'Faucet failed' + if (!ok) throw new Error('Faucet failed') setFauceting(false) setFaucetSuccess(true) } catch (err) { consoleError(err, 'error fauceting') setError(extractError(err)) setFauceting(false) } } @@ - {showFaucetButton ? <Button disabled={!amount} label='Faucet' onClick={handleFaucet} secondary /> : null} + {showFaucetButton ? ( + <Button disabled={!amount || fauceting} label='Faucet' onClick={handleFaucet} secondary /> + ) : null}Also applies to: 168-169
src/screens/Wallet/Send/Details.tsx (1)
106-115: Stop leaving UI in a loading state on early swap errorsYou set sending=true at Line 102 but return early on missing response/address without resetting it, leaving the spinner stuck.
Apply:
- const response = pendingSwap?.response - if (!response) return setError('Swap response not available') - const swapAddress = pendingSwap?.response.address - if (!swapAddress) return setError('Swap address not available') + const response = pendingSwap?.response + if (!response) { + setError('Swap response not available') + setSending(false) + return + } + const swapAddress = response.address + if (!swapAddress) { + setError('Swap address not available') + setSending(false) + return + }src/screens/Wallet/Send/Form.tsx (2)
71-71: Fix rules-of-hooks violation: early return before hooksReturning at Line 71 skips subsequent hooks on some renders. Move the loading state into the main render and guard effects that depend on svcWallet.
- if (!svcWallet) return <Loading text='Loading...' /> + // Do not early-return before hooks; render guarded later. @@ - useEffect(() => { - const { recipient, satoshis } = sendInfo - setRecipient(recipient ?? '') - setAmount(satoshis ? satoshis : undefined) - getReceivingAddresses(svcWallet).then(setReceivingAddresses) - }, []) + useEffect(() => { + const { recipient, satoshis } = sendInfo + setRecipient(recipient ?? '') + setAmount(satoshis ? satoshis : undefined) + if (!svcWallet) return + getReceivingAddresses(svcWallet).then(setReceivingAddresses) + }, [svcWallet]) @@ - if (scan) + if (!svcWallet) return <Loading text='Loading...' /> + if (scan) return ( <Scanner close={() => setScan(false)} label='Recipient address' onData={setRecipient} onError={smartSetError} /> )Also applies to: 73-79
85-101: Do not lowercase recipient data; breaks Base58/BIP21 addressesLowercasing corrupts Bitcoin Base58 addresses and BIP21 payloads. Normalize only for detection; pass original-case data to decoders/state.
- setNudgeBoltz(false) - if (!recipient) return - const lowerCaseData = recipient.toLowerCase().replace(/^lightning:/, '') + setNudgeBoltz(false) + if (!recipient) return + const raw = recipient.trim() + const normalized = raw.replace(/^lightning:/i, '') + const lower = normalized.toLowerCase() @@ - if (isBip21(lowerCaseData)) { - const { address, arkAddress, invoice, satoshis } = decodeBip21(lowerCaseData) + if (isBip21(normalized)) { + const { address, arkAddress, invoice, satoshis } = decodeBip21(normalized) if (!address && !arkAddress && !invoice) return setError('Unable to parse bip21') setAmount(useFiat ? toFiat(satoshis) : satoshis ? satoshis : undefined) - return setState({ address, arkAddress, invoice, recipient, satoshis }) + return setState({ address, arkAddress, invoice, recipient: raw, satoshis }) } - if (isArkAddress(lowerCaseData)) { - return setState({ ...sendInfo, address: '', arkAddress: lowerCaseData }) + if (isArkAddress(normalized)) { + return setState({ ...sendInfo, address: '', arkAddress: normalized }) } - if (isLightningInvoice(lowerCaseData)) { + if (isLightningInvoice(lower)) { if (!connected) { setError('Lightning swaps not enabled') return setNudgeBoltz(true) } - const satoshis = getInvoiceSatoshis(lowerCaseData) + const satoshis = getInvoiceSatoshis(normalized) if (!satoshis) return setError('Invoice must have amount defined') setAmount(useFiat ? toFiat(satoshis) : satoshis ? satoshis : undefined) - return setState({ ...sendInfo, address: '', arkAddress: '', invoice: lowerCaseData }) + return setState({ ...sendInfo, address: '', arkAddress: '', invoice: normalized }) } - if (isBTCAddress(lowerCaseData)) { - return setState({ ...sendInfo, address: lowerCaseData, arkAddress: '' }) + if (isBTCAddress(normalized)) { + return setState({ ...sendInfo, address: normalized, arkAddress: '' }) } @@ - if (isValidLnUrl(lowerCaseData)) { - return setState({ ...sendInfo, lnUrl: lowerCaseData }) + if (isValidLnUrl(lower)) { + return setState({ ...sendInfo, lnUrl: normalized }) }Also applies to: 92-97, 101-113, 123-127
♻️ Duplicate comments (6)
package.json (3)
7-8: SDK and Boltz bump noted — ensure all API call sites remain compatiblePrevious verification covered settle/signature usage; if unchanged since then, no action needed.
21-21: DOMPurify added — enforce hardened sanitize config at call sitesReiterate: pass strict allowlists to
sanitize(); avoid defaults.
27-27: Scanner dep added — mount guard + lazy-load required for secure contextsEnsure feature-detect + client-only lazy import to avoid SSR/permission pitfalls.
src/lib/storage.ts (1)
5-11: Ensure storage is cleared and config re-saved even if DB cleanup fails (repeat)On deleteAll/close error, localStorage.clear and config persistence are skipped. Use try/finally and optionally rethrow.
Apply:
export async function clearStorage(): Promise<void> { - const config = readConfigFromStorage() - await vtxosRepository.deleteAll() - await vtxosRepository.close() - localStorage.clear() - if (config) saveConfigToStorage(config) + const config = readConfigFromStorage() + let opError: unknown + try { + await vtxosRepository.deleteAll() + await vtxosRepository.close() + } catch (err) { + opError = err + console.error('Failed to cleanup vtxos repository:', err) + } finally { + localStorage.clear() + if (config) saveConfigToStorage(config) + } + if (opError) throw opError }src/screens/Wallet/Send/Form.tsx (2)
219-233: Handle null swapProvider; otherwise Proceed stallsIf swapProvider is null, promise is undefined and nothing happens after setting proceed=true. Show error and nudge.
- if (!sendInfo.arkAddress && sendInfo.invoice && !sendInfo.pendingSwap) { - const promise = swapProvider?.createSubmarineSwap(sendInfo.invoice) - if (promise) { - promise - .then((pendingSwap) => { - if (!pendingSwap) return setError('Unable to create swap') - setState({ ...sendInfo, pendingSwap }) - }) - .catch(handleError) - } - } else navigate(Pages.SendDetails) + if (!sendInfo.arkAddress && sendInfo.invoice && !sendInfo.pendingSwap) { + const promise = swapProvider?.createSubmarineSwap(sendInfo.invoice) + if (!promise) { + setError('Lightning swaps not enabled') + setNudgeBoltz(true) + setProceed(false) + return + } + promise + .then((pendingSwap) => { + if (!pendingSwap) return setError('Unable to create swap') + setState({ ...sendInfo, pendingSwap }) + }) + .catch(handleError) + } else { + navigate(Pages.SendDetails) + }
362-368: Ensure isRiga parses env var correctlyThis gating depends on isRiga, but isRiga is currently read as a boolean from a string env var. Normalize it to a real boolean in src/lib/constants.ts to avoid inverted gating.
Suggested (constants.ts):
export const isRiga = String(import.meta.env.VITE_IS_RIGA ?? '').toLowerCase() === 'true'
🧹 Nitpick comments (6)
.eslintrc.json (1)
28-28: Tighten no-unused-vars to reduce noise and align with common TS patternsEmpty options add no value. Recommend ignoring underscore-prefixed args/vars and rest siblings.
- "@typescript-eslint/no-unused-vars": ["warn", {}], + "@typescript-eslint/no-unused-vars": [ + "warn", + { + "args": "after-used", + "argsIgnorePattern": "^_", + "varsIgnorePattern": "^_", + "ignoreRestSiblings": true + } + ],src/lib/format.ts (1)
28-33: Nit: rename local for clarity when suffix is provided‘sats’ can be misleading when suffix switches units. Rename to ‘value’.
-export const prettyAmount = (amount: string | number, suffix?: string): string => { - const sats = typeof amount === 'string' ? Number(amount) : amount - if (suffix) return `${prettyNumber(sats, 2)} ${suffix}` - if (sats >= 100_000_000_000) return `${prettyNumber(fromSatoshis(sats), 0)} BTC` - if (sats >= 100_000_000) return `${prettyNumber(fromSatoshis(sats), 3)} BTC` - if (sats >= 1_000_000) return `${prettyNumber(sats / 1_000_000, 3)}M SATS` - return `${prettyNumber(sats)} ${sats === 1 ? 'SAT' : 'SATS'}` +export const prettyAmount = (amount: string | number, suffix?: string): string => { + const value = typeof amount === 'string' ? Number(amount) : amount + if (suffix) return `${prettyNumber(value, 2)} ${suffix}` + if (value >= 100_000_000_000) return `${prettyNumber(fromSatoshis(value), 0)} BTC` + if (value >= 100_000_000) return `${prettyNumber(fromSatoshis(value), 3)} BTC` + if (value >= 1_000_000) return `${prettyNumber(value / 1_000_000, 3)}M SATS` + return `${prettyNumber(value)} ${value === 1 ? 'SAT' : 'SATS'}`src/screens/Wallet/Receive/Amount.tsx (3)
89-92: Remove duplicate buttonLabel updates; the effect already controls itAvoid flicker and inconsistent states by letting the effect be the single source of truth.
const handleChange = (amount: number) => { setAmount(amount) - setButtonLabel(amount ? 'Continue' : defaultButtonLabel) }
137-137: Coalesce amount before toFiat to avoid NaNEnsure toFiat always receives a number.
- const displayAmount = useFiat ? prettyAmount(toFiat(amount), config.fiat) : prettyAmount(amount ?? 0) + const displayAmount = useFiat ? prettyAmount(toFiat(amount ?? 0), config.fiat) : prettyAmount(amount ?? 0)
51-55: Consider re-pinging faucet when ASP changesIf users switch servers, faucet availability may change. Re-run when aspInfo changes.
-}, []) +}, [aspInfo])src/screens/Wallet/Send/Form.tsx (1)
193-196: Keep derived-amount effect stableThe effect reads useFiat/fromFiat but doesn’t depend on them; add to deps to avoid stale conversions.
- }, [amount]) + }, [amount, useFiat, fromFiat])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
.eslintrc.json(2 hunks)package.json(3 hunks)src/components/Shadow.tsx(3 hunks)src/components/TransactionsList.tsx(3 hunks)src/index.css(3 hunks)src/ionic.css(2 hunks)src/lib/address.ts(3 hunks)src/lib/format.ts(3 hunks)src/lib/lnurl.ts(1 hunks)src/lib/storage.ts(1 hunks)src/providers/options.tsx(2 hunks)src/screens/Settings/Password.tsx(1 hunks)src/screens/Wallet/Receive/Amount.tsx(3 hunks)src/screens/Wallet/Send/Details.tsx(6 hunks)src/screens/Wallet/Send/Form.tsx(11 hunks)src/test/components/AlertBox.test.tsx(1 hunks)src/test/lib/bip21.test.ts(1 hunks)src/test/lib/format.test.ts(1 hunks)src/test/setup.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- src/test/components/AlertBox.test.tsx
- src/components/Shadow.tsx
- src/test/lib/format.test.ts
- src/screens/Settings/Password.tsx
- src/test/lib/bip21.test.ts
- src/components/TransactionsList.tsx
- src/ionic.css
- src/lib/address.ts
- src/providers/options.tsx
- src/index.css
- src/test/setup.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-28T15:58:17.212Z
Learnt from: bordalix
PR: arkade-os/wallet#142
File: src/lib/bip21.ts:7-13
Timestamp: 2025-08-28T15:58:17.212Z
Learning: In src/lib/bip21.ts, the encodeBip21 function is used by the wallet which always has an arkAddress available to pass to the function, while decodeBip21 is used to parse received possible BIP21 URIs which may not contain arkAddress. This explains why arkAddress is required in encodeBip21 but optional in the Bip21Decoded interface.
Applied to files:
src/screens/Wallet/Send/Form.tsx
📚 Learning: 2025-06-30T18:33:29.839Z
Learnt from: bordalix
PR: arkade-os/wallet#114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
Applied to files:
package.json
🧬 Code graph analysis (3)
src/screens/Wallet/Send/Details.tsx (4)
src/providers/lightning.tsx (1)
LightningContext(21-25)src/lib/lightning.tsx (1)
calcSwapFee(130-132)src/lib/constants.ts (1)
defaultFee(2-2)src/components/Error.tsx (1)
ErrorMessage(10-21)
src/screens/Wallet/Send/Form.tsx (7)
src/providers/lightning.tsx (1)
LightningContext(21-25)src/lib/bip21.ts (2)
isBip21(74-81)decodeBip21(16-63)src/lib/logs.ts (1)
consoleError(40-44)src/lib/error.ts (1)
extractError(1-10)src/components/Scanner.tsx (1)
Scanner(20-32)src/components/Error.tsx (1)
ErrorMessage(10-21)src/lib/constants.ts (1)
isRiga(7-7)
src/screens/Wallet/Receive/Amount.tsx (1)
src/components/Error.tsx (1)
ErrorMessage(10-21)
🪛 Biome (2.1.2)
src/screens/Wallet/Send/Form.tsx
[error] 193-193: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
src/screens/Wallet/Receive/Amount.tsx
[error] 77-77: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🔇 Additional comments (15)
.eslintrc.json (1)
55-55: Good call on enforcing canonical array syntax
@typescript-eslint/array-typewith{ default: 'array' }improves consistency and readability.package.json (2)
35-36: Nice addition of UI/coverage test scriptsThis aids local dev and CI visibility.
89-89: Engines: Node ≥20 is correct (Node 18 EOL passed)This matches modern Vite/Vitest stacks.
src/lib/lnurl.ts (1)
49-53: Catch binding removal is fineNo behavior change; eliminates unused var.
src/lib/storage.ts (1)
17-19: Catch binding removal is fineKeeps behavior; avoids unused var.
src/lib/format.ts (5)
30-33: BTC/SATS thresholds and pluralization LGTM= boundaries and singular “SAT” at 1 are correct.
38-41: Days boundary/pluralization fix LGTM
42-45: Hours boundary/pluralization fix LGTM
46-49: Minutes boundary/pluralization fix LGTM
51-52: Seconds branch rename avoids shadowing; LGTMsrc/screens/Wallet/Receive/Amount.tsx (2)
7-7: Import rename looks correctDefault export is ErrorMessage from components/Error. Alignment with repo-wide rename.
154-154: Component rename usage LGTM<ErrorMessage ... /> correctly matches the new import.
src/screens/Wallet/Send/Details.tsx (2)
110-116: Good guard against undefined promise from optional chainingThe added guard avoids calling .then on undefined when swapProvider is null.
148-148: Consistent error UISwitch to ErrorMessage is consistent with the rest of the app.
src/screens/Wallet/Send/Form.tsx (1)
329-331: Scanner/InputAmount prop updates look goodProp renames (onData/onError and onMax) are consistent with the new component APIs.
Also applies to: 350-358, 369-375
| export const prettyLongText = (str?: string, showChars = 11): string => { | ||
| if (!str) return '' | ||
| str = String(str) | ||
| if (str.length <= showChars * 2 + 4) return str |
There was a problem hiding this comment.
prettyLongText: coercion happens too late; 0 gets dropped
With the current order, numeric 0 returns '' due to the falsy check before coercion. Move coercion ahead or narrow the falsy check.
-export const prettyLongText = (str?: string, showChars = 11): string => {
- if (!str) return ''
- str = String(str)
+export const prettyLongText = (str?: string, showChars = 11): string => {
+ if (str === undefined || str === null) return ''
+ str = String(str)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const prettyLongText = (str?: string, showChars = 11): string => { | |
| if (!str) return '' | |
| str = String(str) | |
| if (str.length <= showChars * 2 + 4) return str | |
| export const prettyLongText = (str?: string, showChars = 11): string => { | |
| if (str === undefined || str === null) return '' | |
| str = String(str) | |
| if (str.length <= showChars * 2 + 4) return str | |
| // …rest of implementation unchanged | |
| } |
🤖 Prompt for AI Agents
In src/lib/format.ts around lines 76 to 79, the current falsy check runs before
coercing the input to a string so numeric 0 (and other falsy-but-valid values)
become '' — move the coercion up or change the guard to explicitly check for
null/undefined; specifically, first coerce with str = str === undefined || str
=== null ? undefined : String(str) (or simply String(str) before the emptiness
check), then use a check like if (str === undefined || str === '') return '' (or
if (str == null) return ''), and keep the existing length comparison and
truncation logic unchanged.
| useEffect(() => { | ||
| setSatoshis(useFiat ? fromFiat(amount) : amount ?? 0) | ||
| setSatoshis(useFiat ? fromFiat(amount) : (amount ?? 0)) | ||
| }, [amount]) | ||
|
|
There was a problem hiding this comment.
Prevent NaN when amount is undefined and include missing deps
fromFiat(amount) can produce NaN when amount is undefined; also missing useFiat/fromFiat in deps.
-useEffect(() => {
- setSatoshis(useFiat ? fromFiat(amount) : (amount ?? 0))
-}, [amount])
+useEffect(() => {
+ setSatoshis(useFiat ? fromFiat(amount ?? 0) : (amount ?? 0))
+}, [amount, useFiat, fromFiat])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| setSatoshis(useFiat ? fromFiat(amount) : amount ?? 0) | |
| setSatoshis(useFiat ? fromFiat(amount) : (amount ?? 0)) | |
| }, [amount]) | |
| useEffect(() => { | |
| setSatoshis(useFiat ? fromFiat(amount ?? 0) : (amount ?? 0)) | |
| }, [amount, useFiat, fromFiat]) |
🧰 Tools
🪛 Biome (2.1.2)
[error] 73-73: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🤖 Prompt for AI Agents
In src/screens/Wallet/Receive/Amount.tsx around lines 73 to 76, the effect calls
fromFiat(amount) which can return NaN when amount is undefined and the
dependency array is missing useFiat and fromFiat; fix by computing a safe
numeric value first (e.g. const safeAmount = amount ?? 0; const satoshis =
useFiat ? fromFiat(safeAmount) : safeAmount) and ensure you coerce NaN to 0
(e.g. Number.isFinite(satoshis) ? satoshis : 0) before calling setSatoshis, and
add useFiat and fromFiat to the effect dependency array.
| useEffect(() => { | ||
| setButtonLabel( | ||
| !satoshis | ||
| ? defaultButtonLabel | ||
| : amountIsBelowMinLimit(satoshis) | ||
| ? 'Amount below min limit' | ||
| : amountIsAboveMaxLimit(satoshis) | ||
| ? 'Amount above max limit' | ||
| : 'Continue', | ||
| ? 'Amount below min limit' | ||
| : amountIsAboveMaxLimit(satoshis) | ||
| ? 'Amount above max limit' | ||
| : 'Continue', | ||
| ) | ||
| }, [satoshis]) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add missing dependencies to label effect
These functions can change; include them to keep label accurate.
-}, [satoshis])
+}, [satoshis, amountIsBelowMinLimit, amountIsAboveMaxLimit])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| setButtonLabel( | |
| !satoshis | |
| ? defaultButtonLabel | |
| : amountIsBelowMinLimit(satoshis) | |
| ? 'Amount below min limit' | |
| : amountIsAboveMaxLimit(satoshis) | |
| ? 'Amount above max limit' | |
| : 'Continue', | |
| ? 'Amount below min limit' | |
| : amountIsAboveMaxLimit(satoshis) | |
| ? 'Amount above max limit' | |
| : 'Continue', | |
| ) | |
| }, [satoshis]) | |
| useEffect(() => { | |
| setButtonLabel( | |
| !satoshis | |
| ? defaultButtonLabel | |
| : amountIsBelowMinLimit(satoshis) | |
| ? 'Amount below min limit' | |
| : amountIsAboveMaxLimit(satoshis) | |
| ? 'Amount above max limit' | |
| : 'Continue', | |
| ) | |
| }, [satoshis, amountIsBelowMinLimit, amountIsAboveMaxLimit]) |
🧰 Tools
🪛 Biome (2.1.2)
[error] 77-77: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🤖 Prompt for AI Agents
In src/screens/Wallet/Receive/Amount.tsx around lines 77 to 88, the useEffect
that sets the button label only depends on satoshis but also reads
defaultButtonLabel, amountIsBelowMinLimit and amountIsAboveMaxLimit (which can
change); update the dependency array to include those values
(defaultButtonLabel, amountIsBelowMinLimit, amountIsAboveMaxLimit) so the effect
re-runs when any of them change and the label stays accurate.
| const handleTxid = (txid: string) => { | ||
| if (!txid) return setError('Error sending transaction') | ||
| setSendInfo({ ...sendInfo, total: (satoshis ?? 0) + feeInSats, txid }) | ||
| setSendInfo({ ...sendInfo, total: details?.total, txid }) | ||
| navigate(Pages.SendSuccess) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t trust derived state for totals; compute a safe fallback
details?.total can be undefined/stale. Use a deterministic fallback based on the current inputs.
- setSendInfo({ ...sendInfo, total: details?.total, txid })
+ const fallbackTotal =
+ details?.total ??
+ (satoshis + (invoice ? calcSwapFee(satoshis) : 0))
+ setSendInfo({ ...sendInfo, total: fallbackTotal, txid })Optionally make the fee detection explicit instead of coupling to destination equality:
- const feeInSats = destination === invoice ? calcSwapFee(satoshis) : defaultFee
+ const feeInSats = invoice && lnSwapsAllowed() ? calcSwapFee(satoshis) : defaultFeeAlso applies to: 62-64
🤖 Prompt for AI Agents
In src/screens/Wallet/Send/Details.tsx around lines 83-86 (also apply same
change to lines 62-64), do not use details?.total directly as it can be
undefined or stale; instead compute a deterministic fallback total from current
inputs (e.g., derive total from sendInfo amounts and explicit fee) before
setting state. Modify handleTxid to compute total = details?.total ??
computeTotalFrom(sendInfo, explicitFeeDetection) and use that total when calling
setSendInfo and navigate; make fee detection explicit rather than inferring from
destination equality (derive fee from state/inputs or a clear helper).
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/Scanner.tsx (2)
38-41: Use refs for camera/canvas/cancel; fix early-return close.Plain lets are lost across renders, breaking cleanup and leaking the camera. Also, handleClose no-ops before init, preventing users from closing quickly.
- let camera: any - let canvas: QRCanvas - let cancel: () => void + const cameraRef = useRef<any | null>(null) + const canvasRef = useRef<QRCanvas | null>(null) + const cancelRef = useRef<null | (() => void)>(null) … - const handleClose = () => { - if (!cancel && !error) return - if (cancel) cancel() - camera?.stop() - close() - } + const handleClose = () => { + cancelRef.current?.() + cancelRef.current = null + cameraRef.current?.stop?.() + cameraRef.current = null + close() + }Also applies to: 69-74
42-67: Effect cleanup, device selection, and handleClose behavior
- Move
camera,canvas, andcancelintouseRefhooks to avoid stale closures inside the effect.- In
handleClose, remove the earlyif (!cancel && !error) returnso thatclose()always runs—even before camera init.- When choosing a device, match
/back|environment/iagainstdevice.label, falling back to the last entry.- In the effect’s cleanup, call
cancelRef.current(), stop the camera, then invokeclose().
♻️ Duplicate comments (2)
package.json (2)
7-8: SDK/boltz-swap bumps — confirm API compatibility (settle signature, etc.)Past reports indicated break risk on a 2nd arg to settle; ensure all call sites are aligned.
21-21: DOMPurify added — enforce strict sanitize config at call sitesUse a hardened config (empty ALLOWED_TAGS/ATTR, forbid on* attrs, disallow unknown protocols) instead of defaults in Alert/Info components.
🧹 Nitpick comments (9)
package.json (2)
27-27: react-qr-reader (beta) — guard secure contexts and lazy-load the scannerEnsure scanner mounts only when
isSecureContext && navigator.mediaDevices?.getUserMediaare available; prefer lazy/dynamic import to avoid SSR/worker import issues and shrink initial bundle. Also confirm iOS rear-camera selection fallback via deviceId when facingMode is ignored.
60-60: @eslint/compat added — align on config strategy (flat vs. legacy)You still have a package.json "eslintConfig". If moving to flat config with @eslint/compat, drop the legacy block and add an eslint.config.(js|mjs); otherwise remove @eslint/compat.
src/test/screens/wallet/index.test.tsx (3)
7-7: Wrap Wallet with Router/providers to avoid context crashes.If Wallet uses routing/contexts, wrap with
MemoryRouter(or project renderWithProviders).-import { render, screen } from '@testing-library/react' +import { render, screen } from '@testing-library/react' +import { MemoryRouter } from 'react-router-dom' @@ - render(<Wallet />) + render( + <MemoryRouter> + <Wallet /> + </MemoryRouter> + )
6-13: Use role-based, case-insensitive, and async queries for stability (i18n/async UI).Texts may change casing/locale or render after effects. Prefer roles and
/.../iwithfindBy*.-describe('Wallet screen', () => { - it('renders the wallet screen with the correct elements', () => { - render(<Wallet />) - expect(screen.getByText('SATS')).toBeInTheDocument() - expect(screen.getByText('Send')).toBeInTheDocument() - expect(screen.getByText('Receive')).toBeInTheDocument() - expect(screen.getByText('My balance')).toBeInTheDocument() - expect(screen.getByText('No transactions yet')).toBeInTheDocument() - }) -}) +describe('Wallet screen', () => { + it('renders primary CTAs and empty state', async () => { + render( + <MemoryRouter> + <Wallet /> + </MemoryRouter> + ) + expect(await screen.findByText(/sats/i)).toBeInTheDocument() + expect(await screen.findByRole('button', { name: /send/i })).toBeInTheDocument() + expect(await screen.findByRole('button', { name: /receive/i })).toBeInTheDocument() + expect(await screen.findByText(/my balance/i)).toBeInTheDocument() + expect(await screen.findByText(/no transactions yet/i)).toBeInTheDocument() + }) +})
5-14: Consider isolating states and providers for determinism.If Wallet conditionally shows Riga alerts/Nudge/default-password banners, mock those providers (or expose feature flags) so this test stays deterministic.
I can add a
renderWithProviderstest util with default mocked contexts and router. Want a follow-up PR?src/components/Scanner.tsx (4)
88-88: Ensure camera autoplay on iOS Safari.Add muted/playsInline/autoplay to avoid fullscreen blocking and permission quirks.
- <video style={videoStyle} ref={videoRef} /> + <video style={videoStyle} ref={videoRef} autoPlay playsInline muted />
84-84: Aux label “M/R” may be cryptic.Consider an icon or “Switch” text for clarity/accessibility.
Also applies to: 101-101
87-87: Message copy nit.“Camera not available” → “Camera not available. Check permissions in your browser settings.” for clearer guidance.
12-18: Split public/internal props: removeonSwitchfrom the exportedScanner’s props and extract it into an internalScannerInnerPropsforScannerMills/ScannerReact. No external callers ofonSwitchfound.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
package.json(3 hunks)src/components/Scanner.tsx(4 hunks)src/test/screens/wallet/index.test.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-30T18:33:29.839Z
Learnt from: bordalix
PR: arkade-os/wallet#114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
Applied to files:
package.json
🧬 Code graph analysis (1)
src/components/Scanner.tsx (2)
src/lib/error.ts (1)
extractError(1-10)src/components/Error.tsx (1)
ErrorMessage(10-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cloudflare Pages: wallet-bitcoin-v8
🔇 Additional comments (7)
package.json (4)
89-90: Engines Node ≥20 — good call; update CI/tooling pinsLGTM. Ensure GitHub Actions, Dockerfiles, and .nvmrc/asdf match Node 20.
35-36: Vitest UI/coverage scripts — nice additionsHelpful for local dev and CI coverage workflows.
83-85: Peer dependency verification complete
All required peer ranges are satisfied: Vitest 3.2.4 has no Vite peer, and @vitejs/plugin-react 5.0.2 accepts Vite ^7.0.0 (including 7.1.3). No further changes needed.
93-94: Ensure Rollup resolution doesn’t break plugins
pnpm ls rollupshows Rollup 2.79.2 (vite-plugin-eslint) and 4.44.2 (Vite). Since you’ve pinned Rollup to 4.44.2 via resolutions, confirm vite-plugin-eslint is compatible with Rollup 4.x or adjust the resolution.src/test/screens/wallet/index.test.tsx (1)
3-3: Verified default export and correct path casingThe
src/screens/Wallet/Index.tsxfile exports a defaultWalletcomponent (line 26), and the import path in the test matches the directory and filename casing.src/components/Scanner.tsx (2)
4-11: Imports look consistent with recent renames and new deps.ErrorMessage/extractError wiring and QrReader import are correct.
98-118: Remove invalid refactor suggestion
ScannerInnerPropsisn’t defined—keep usingScannerProps—andreact-qr-reader@3.0.0-beta-1’sonResultdelivers a{ text: string }result (not agetText()method).Likely an incorrect or invalid review comment.
| import { render, screen } from '@testing-library/react' | ||
| import { describe, expect, it } from 'vitest' | ||
| import Wallet from '../../../screens/Wallet/Index' |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure jest-dom matchers are loaded in Vitest setup.
toBeInTheDocument() requires @testing-library/jest-dom. Verify setup includes it.
Run:
🏁 Script executed:
#!/bin/bash
# Check Vitest setup file is registered
rg -nP 'setupFiles\s*:\s*\[.*src/test/setup' -g 'vitest.config.*'
# Check jest-dom is imported in setup
rg -n "@testing-library/jest-dom/(vitest)?'" src/test/setup.*Length of output: 179
Add the setup file to Vitest’s config to load jest-dom matchers
Vitest isn’t currently loading your setup file, so even though src/test/setup.ts imports @testing-library/jest-dom/vitest, those matchers won’t be applied. In vitest.config.ts, under the test section, add:
import { defineConfig } from 'vitest/config'
export default defineConfig({
test: {
setupFiles: ['src/test/setup.ts'],
// …other options
},
})This ensures toBeInTheDocument() and other jest-dom matchers are available in all your tests.
🤖 Prompt for AI Agents
In src/test/screens/wallet/index.test.tsx lines 1-3: Vitest isn't loading the
jest-dom matchers because the test setup file isn't registered; update
vitest.config.ts (the project's root vitest config) and under the test section
add setupFiles: ['src/test/setup.ts'] (or the correct relative path) so that
src/test/setup.ts (which imports @testing-library/jest-dom/vitest) is executed
before tests, ensuring matchers like toBeInTheDocument() are available globally.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/screens/Wallet/Onboard.tsx (1)
86-87: Fix user-facing grammar.
“enable” → “enables”; add article; optionally clarify “Home Screen”.- <Title text='Install Arkade on Home' /> - <Text wrap>Adding Arkade to Home enable push notifications and better user experience.</Text> + <Title text='Install Arkade on Home Screen' /> + <Text wrap>Adding Arkade to the Home Screen enables push notifications and a better user experience.</Text>
🧹 Nitpick comments (9)
src/screens/Settings/About.tsx (4)
22-23: Harden URL/pubkey rendering; consider copyability/overflow handling.Add a safe fallback to avoid empty cells when values are missing. Optionally make these cells copyable and truncate long strings to prevent layout breakage.
- ['Server URL', aspInfo.url], - ['Server pubkey', aspInfo.signerPubkey], + ['Server URL', aspInfo.url || '—'], + ['Server pubkey', aspInfo.signerPubkey || '—'],
26-26: Unit casing nit — prefer “sats”.Consistent lowercase “sats” is more common across UIs.
- ['Dust', `${aspInfo.dust} SATS`], + ['Dust', `${aspInfo.dust} sats`],
27-31: Avoid blank cells when values are 0/NaN; prettyDelta returns '' in those cases.Add a simple fallback so the table communicates “no delay/unknown” rather than rendering empty.
- ['Round interval', prettyDelta(Number(aspInfo.roundInterval), true)], + ['Round interval', prettyDelta(Number(aspInfo.roundInterval), true) || '—'], - ['VTXO tree expiry', prettyDelta(Number(aspInfo.vtxoTreeExpiry), true)], + ['VTXO tree expiry', prettyDelta(Number(aspInfo.vtxoTreeExpiry), true) || '—'], - ['Boarding exit delay', prettyDelta(Number(aspInfo.boardingExitDelay), true)], + ['Boarding exit delay', prettyDelta(Number(aspInfo.boardingExitDelay), true) || '—'], - ['Unilateral exit delay', prettyDelta(Number(aspInfo.unilateralExitDelay), true)], + ['Unilateral exit delay', prettyDelta(Number(aspInfo.unilateralExitDelay), true) || '—'],
40-40: Drop derived state for error; compute inline to avoid staleness.No need for useState/useEffect just to mirror aspInfo.unreachable.
// imports -import { useContext, useEffect, useState } from 'react' +import { useContext } from 'react' // inside component -const [error, setError] = useState(false) -useEffect(() => { - setError(aspInfo.unreachable) -}, [aspInfo.unreachable]) +const error = Boolean(aspInfo?.unreachable)Rendering line stays the same:
<ErrorMessage error={error} text='Ark server unreachable' />src/screens/Wallet/Onboard.tsx (2)
29-30: Avoid stale state; use functional updater and clamp.
Prevents lost increments on rapid double-clicks and guards upper bound.- const handleContinue = () => setStep(step + 1) + const handleContinue = () => setStep((s) => Math.min(s + 1, steps))
41-46: Type the inline style.
Use a proper React CSS type instead of any.- const style: any = { + const style: React.CSSProperties = {And add:
- import { ReactNode, useContext, useState } from 'react' + import type { CSSProperties } from 'react' + import { ReactNode, useContext, useState } from 'react'src/test/screens/wallet/onboard.test.tsx (1)
17-25: Prefer userEvent for realistic interactions and add test isolation.
Improves reliability and avoids brittle DOM .click().-import { render, screen } from '@testing-library/react' +import { render, screen } from '@testing-library/react' +import userEvent from '@testing-library/user-event' @@ describe('Onboard screen', () => { + beforeEach(() => { + pwaCanInstall = false + vi.clearAllMocks() + }) @@ - screen.getByText('Continue').click() + const user = userEvent.setup() + await user.click(screen.getByText('Continue')) @@ - screen.getByText('Continue').click() + await user.click(screen.getByText('Continue'))Repeat similarly for the 4-step test.
src/test/screens/mocks.ts (2)
15-28: Freeze or clone to prevent accidental mutation in tests.
The shared objects are currently mutable and get modified in tests; shallow-freeze them to catch mutations early.-export const mockFlowContextValue = { +export const mockFlowContextValue = Object.freeze({ txInfo: mockTxInfo, swapInfo: undefined, initInfo: emptyInitInfo, noteInfo: emptyNoteInfo, recvInfo: emptyRecvInfo, sendInfo: emptySendInfo, setInitInfo: () => {}, setNoteInfo: () => {}, setRecvInfo: () => {}, setSendInfo: () => {}, setSwapInfo: () => {}, setTxInfo: () => {}, -} +})Optionally also freeze mockTxInfo.
3-13: Consider typing and const-asserting mocks.
Improves DX and catches shape drift when provider contracts change.-export const mockTxInfo = { +export const mockTxInfo = { amount: 100000, @@ - type: 'received', + type: 'received', -} +} as const
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
package.json(3 hunks)src/screens/Settings/About.tsx(3 hunks)src/screens/Wallet/Onboard.tsx(3 hunks)src/test/screens/mocks.ts(1 hunks)src/test/screens/wallet/onboard.test.tsx(1 hunks)src/test/screens/wallet/transaction.test.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🧬 Code graph analysis (4)
src/test/screens/wallet/onboard.test.tsx (2)
src/lib/pwa.ts (1)
pwaCanInstall(5-5)src/screens/Wallet/Onboard.tsx (1)
Onboard(22-134)
src/test/screens/wallet/transaction.test.tsx (4)
src/providers/flow.tsx (1)
FlowContext(82-95)src/test/screens/mocks.ts (2)
mockFlowContextValue(15-28)mockLimitsContextValue(30-41)src/providers/limits.tsx (1)
LimitsContext(29-40)src/screens/Wallet/Transaction.tsx (1)
Transaction(26-164)
src/test/screens/mocks.ts (1)
src/providers/flow.tsx (4)
emptyInitInfo(57-60)emptyNoteInfo(62-65)emptyRecvInfo(67-71)emptySendInfo(73-80)
src/screens/Settings/About.tsx (2)
src/lib/format.ts (1)
prettyDelta(36-55)src/components/Error.tsx (1)
ErrorMessage(10-21)
🔇 Additional comments (4)
src/screens/Settings/About.tsx (2)
10-10: LGTM — clearer import alias.The default import binding to ErrorMessage improves readability and matches the component name.
22-31: AspInfo properties verified All referenced keys (url,signerPubkey,forfeitAddress,network,dust,roundInterval,vtxoTreeExpiry,boardingExitDelay,unilateralExitDelay) are defined on the AspInfo type (seeemptyAspInfoin src/lib/asp.ts), so runtime accesses will never be undefined.src/test/screens/wallet/transaction.test.tsx (1)
55-56: Exact match is correct — the component passesname='Settle transaction'(lowercase “t”), so your test’s case-sensitive assertion matches the rendered text and doesn’t need changing.src/test/screens/wallet/onboard.test.tsx (1)
5-9: Good dynamic module mock for PWA capability.
This pattern keeps tests deterministic while covering 3- and 4-step paths.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
91-93: resolutions won’t apply under pnpm; switch to pnpm.overridesWith pnpm 10, use overrides to pin rollup.
- "resolutions": { - "rollup": "4.44.2" - } + "pnpm": { + "overrides": { + "rollup": "4.44.2" + } + }
🧹 Nitpick comments (4)
src/providers/lightning.tsx (2)
48-59: Avoid redundant config writesShort-circuit when the value is unchanged to prevent unnecessary re-renders across the app.
- const setConnected = (value: boolean) => { - updateConfig({ + const setConnected = (value: boolean) => { + if (config.apps.boltz.connected === value) return + updateConfig({ ...config, apps: { ...config.apps, boltz: { ...config.apps.boltz, connected: value, }, }, }) }
38-39: Treat empty VITE_BOLTZ_URL as unsetAn empty string currently disables baseUrl via the nullish coalescing semantics. The revised effect above explicitly trims and ignores empty values to avoid confusion. If you don’t adopt that change, at least normalize here.
- const baseUrl = BoltzUrl ?? BASE_URLS[aspInfo.network as Network] + const baseUrl = (typeof BoltzUrl === 'string' && BoltzUrl.trim()) || BASE_URLS[aspInfo.network as Network]package.json (2)
26-26: New scanner dep: verify secure-context and feature-detect guardsEnsure components using react-qr-reader only mount in HTTPS/localhost and after checking navigator.mediaDevices/getUserMedia. If you want, I can supply a guarded, lazy-loaded wrapper.
64-64: Remove unused @types/qsqs was removed; drop its types to avoid stale deps.
- "@types/qs": "^6.9.18",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
package.json(3 hunks)src/lib/constants.ts(2 hunks)src/providers/lightning.tsx(1 hunks)src/test/screens/wallet/transaction.test.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/test/screens/wallet/transaction.test.tsx
- src/lib/constants.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-30T18:33:29.839Z
Learnt from: bordalix
PR: arkade-os/wallet#114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
Applied to files:
package.json
🧬 Code graph analysis (1)
src/providers/lightning.tsx (5)
src/lib/lightning.tsx (1)
LightningSwapProvider(16-128)src/providers/asp.tsx (1)
AspContext(15-20)src/providers/wallet.tsx (1)
WalletContext(42-56)src/providers/config.tsx (1)
ConfigContext(29-37)src/lib/constants.ts (1)
BoltzUrl(24-24)
🔇 Additional comments (4)
package.json (4)
7-8: Boltz/Sdk bumps look goodMatches provider usage introduced in this PR.
34-35: Nice: added vitest UI/coverage scriptsGood additions for DX.
59-85: Lint config consistency checkYou upgraded ESLint/ts-eslint, but package.json still has "eslintConfig": { "extends": ["react-app"] }. If you’re not using eslint-config-react-app, remove that block or add the missing config to devDependencies. Otherwise pnpm lint may fail.
88-88: Engine to Node ≥20: goodAligns with Vite/Vitest ecosystem and long-term support.
| useEffect(() => { | ||
| if (!aspInfo.network || !svcWallet) return | ||
| const baseUrl = BoltzUrl ?? BASE_URLS[aspInfo.network as Network] | ||
| if (!baseUrl) return // No boltz server for this network | ||
| StorageProvider.create().then((storageProvider) => { | ||
| setSwapProvider(new LightningSwapProvider(baseUrl, aspInfo, svcWallet, storageProvider)) | ||
| setConnected(config.apps.boltz.connected) | ||
| }) | ||
| }, [aspInfo, svcWallet, config.apps.boltz.connected]) | ||
|
|
There was a problem hiding this comment.
Fix stale provider and race on reconnect; gate by connected + add cleanup/error-handling
If aspInfo/svcWallet/connected changes or becomes unavailable, the old swapProvider remains set. This can lead to using a provider bound to the wrong network/wallet. Also, the async init has no cancellation or error handling.
Apply:
- useEffect(() => {
- if (!aspInfo.network || !svcWallet) return
- const baseUrl = BoltzUrl ?? BASE_URLS[aspInfo.network as Network]
- if (!baseUrl) return // No boltz server for this network
- StorageProvider.create().then((storageProvider) => {
- setSwapProvider(new LightningSwapProvider(baseUrl, aspInfo, svcWallet, storageProvider))
- setConnected(config.apps.boltz.connected)
- })
- }, [aspInfo, svcWallet, config.apps.boltz.connected])
+ useEffect(() => {
+ let cancelled = false
+
+ // Respect user toggle; clear provider when disconnected
+ if (!config.apps.boltz.connected) {
+ setSwapProvider(null)
+ return
+ }
+ // Clear when prerequisites are missing
+ if (!aspInfo?.network || !svcWallet) {
+ setSwapProvider(null)
+ return
+ }
+ // Prefer non-empty override; otherwise derive by network
+ const override = typeof BoltzUrl === 'string' && BoltzUrl.trim().length > 0 ? BoltzUrl : undefined
+ const baseUrl = override ?? BASE_URLS[aspInfo.network as Network]
+ if (!baseUrl) {
+ setSwapProvider(null)
+ return
+ }
+
+ StorageProvider.create()
+ .then((storageProvider) => {
+ if (cancelled) return
+ setSwapProvider(new LightningSwapProvider(baseUrl, aspInfo, svcWallet, storageProvider))
+ })
+ .catch((err) => {
+ console.error('Lightning: failed to init swap provider', err)
+ if (!cancelled) setSwapProvider(null)
+ })
+
+ return () => {
+ cancelled = true
+ setSwapProvider(null)
+ }
+ }, [aspInfo.network, aspInfo.url, svcWallet, config.apps.boltz.connected])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| if (!aspInfo.network || !svcWallet) return | |
| const baseUrl = BoltzUrl ?? BASE_URLS[aspInfo.network as Network] | |
| if (!baseUrl) return // No boltz server for this network | |
| StorageProvider.create().then((storageProvider) => { | |
| setSwapProvider(new LightningSwapProvider(baseUrl, aspInfo, svcWallet, storageProvider)) | |
| setConnected(config.apps.boltz.connected) | |
| }) | |
| }, [aspInfo, svcWallet, config.apps.boltz.connected]) | |
| useEffect(() => { | |
| let cancelled = false | |
| // Respect user toggle; clear provider when disconnected | |
| if (!config.apps.boltz.connected) { | |
| setSwapProvider(null) | |
| return | |
| } | |
| // Clear when prerequisites are missing | |
| if (!aspInfo?.network || !svcWallet) { | |
| setSwapProvider(null) | |
| return | |
| } | |
| // Prefer non-empty override; otherwise derive by network | |
| const override = | |
| typeof BoltzUrl === 'string' && BoltzUrl.trim().length > 0 | |
| ? BoltzUrl | |
| : undefined | |
| const baseUrl = override ?? BASE_URLS[aspInfo.network as Network] | |
| if (!baseUrl) { | |
| setSwapProvider(null) | |
| return | |
| } | |
| StorageProvider.create() | |
| .then((storageProvider) => { | |
| if (cancelled) return | |
| setSwapProvider( | |
| new LightningSwapProvider( | |
| baseUrl, | |
| aspInfo, | |
| svcWallet, | |
| storageProvider, | |
| ), | |
| ) | |
| }) | |
| .catch((err) => { | |
| console.error('Lightning: failed to init swap provider', err) | |
| if (!cancelled) setSwapProvider(null) | |
| }) | |
| return () => { | |
| cancelled = true | |
| setSwapProvider(null) | |
| } | |
| }, [aspInfo.network, aspInfo.url, svcWallet, config.apps.boltz.connected]) |
🤖 Prompt for AI Agents
In src/providers/lightning.tsx around lines 36 to 45, the effect currently
initializes a LightningSwapProvider asynchronously but does not gate by the
boltz connected flag, lacks cancellation/cleanup, and leaves stale providers
when aspInfo/svcWallet/connected changes; update the effect to: 1) include
config.apps.boltz.connected in the early-return guard (only proceed when
connected is true and aspInfo and svcWallet are present), 2) create a
cancellable init (use a mounted boolean or AbortController) and ignore/set aside
results if cancelled, 3) add try/catch around async StorageProvider.create and
provider construction to log errors and avoid leaving state inconsistent, 4) on
cleanup clear setSwapProvider(null) and dispose the created provider if it
exposes a close/dispose method, and 5) ensure setConnected is only set based on
actual success or left unchanged on failure.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
package.json (2)
41-45: Remove CRA “react-app” ESLint preset — not installed and conflicts with ViteVite projects typically don’t use eslint-config-react-app. Keep a .eslintrc/flat config instead; this block will cause resolution errors.
Apply:
- "eslintConfig": { - "extends": [ - "react-app" - ] - },
92-94: Using “resolutions” with pnpm won’t take effect — use pnpm.overridespnpm ignores Yarn’s “resolutions”. Move the Rollup pin to pnpm.overrides (or top-level overrides).
Apply:
- "resolutions": { - "rollup": "4.44.2" - } + "pnpm": { + "overrides": { + "rollup": "4.44.2" + } + }
♻️ Duplicate comments (1)
src/test/fixtures.json (1)
15-113: Avoid reintroducing “compliant” flag coupling; confirm tests treat absence as true.Past reviews noted valid vectors lacking
compliantwere skipped when tests usedcompliant === true. If tests were updated to interpret absence as compliant (i.e.,compliant !== false), we’re good. Otherwise, add"compliant": trueto the intended-valid cases.
🧹 Nitpick comments (11)
src/test/lib/fiat.test.ts (4)
5-8: Reset mocks between tests to prevent cross-test leakage.Add a beforeEach to reset the mocked fetch and clear spies.
describe('fiat utilities', () => { + beforeEach(() => { + fetchMocker.resetMocks() + vi.clearAllMocks() + }) it('should fetch fiat values', async () => {
19-22: Also assert the endpoint and call count.This ensures the implementation hits the expected URL exactly once.
- const result = await getPriceFeed() - expect(result).toEqual(expected) + const result = await getPriceFeed() + expect(globalThis.fetch).toHaveBeenCalledTimes(1) + expect(globalThis.fetch).toHaveBeenCalledWith('https://blockchain.info/ticker') + expect(result).toEqual(expected)
9-23: Add a failure-path test.Guard against regressions: fetch rejection should yield undefined.
describe('fiat utilities', () => { it('should fetch fiat values', async () => { @@ expect(result).toEqual(expected) }) + + it('returns undefined when fetch fails', async () => { + fetchMocker.mockRejectOnce(new Error('network')) + await expect(getPriceFeed()).resolves.toBeUndefined() + }) })
5-8: Consider moving enableMocks to a global test setup.Centralizing fetchMocker.enableMocks() in a Vitest setup file avoids accidental partial mocking across suites.
If you want, I can draft a minimal vitest.setup.ts and config snippet to wire this globally.
package.json (3)
26-26: react-qr-reader (beta) — guard for secure context + lazy-loadThis lib accesses camera APIs. Only mount in HTTPS/localhost, feature-detect mediaDevices/getUserMedia, and consider lazy-loading to avoid SSR/import-time issues.
59-86: ESLint/TS stack versions look compatible; remove stale @types/qsTooling versions are coherent with TS 5.8. If runtime "qs" was removed, drop @types/qs to avoid dead types.
Apply:
- "@types/qs": "^6.9.18",
89-89: Engines: Node >=20 is correct; align pnpm to >=10You pin packageManager to pnpm 10.13.1 but engines.pnpm is >=8. Tighten to avoid toolchain drift.
Apply:
"engines": { "node": ">=20", - "pnpm": ">=8" + "pnpm": ">=10" },src/test/lib/lnurl.test.ts (4)
1-4: Import beforeEach to reset fetch mocks per test.-import { describe, expect, it, vi } from 'vitest' +import { beforeEach, describe, expect, it, vi } from 'vitest'
6-9: Reset fetch mocks between tests to avoid cross-test leakage.fetchMocker.enableMocks() +beforeEach(() => { + fetchMocker.resetMocks() + vi.clearAllMocks() +})
34-41: Assert the invoice request includes the correct msat amount and note.Strengthen this test by verifying the second fetch call hits
callbackwithamount = 21000(for 21 sats) and the expected note/comment, preventing regressions in query construction.I can follow up with an assertion snippet tailored to your
fetchLnUrlInvoiceimplementation if you expose the URL build logic.
10-15: Use spec-shaped LNURL metadata to increase fixture fidelity.LNURL
metadatais a JSON-encoded string (e.g.,[[\"text/plain\",\"…\"]]). Using that shape better matches real servers.- metadata: 'mock-metadata', + metadata: '[[\"text/plain\",\"mock-metadata\"]]',
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
package.json(3 hunks)src/lib/fiat.ts(0 hunks)src/test/fixtures.json(1 hunks)src/test/lib/fiat.test.ts(1 hunks)src/test/lib/lnurl.test.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/lib/fiat.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-07T14:41:35.991Z
Learnt from: bordalix
PR: arkade-os/wallet#106
File: src/lib/bolt11.ts:153-155
Timestamp: 2025-07-07T14:41:35.991Z
Learning: In src/lib/bolt11.ts, the code is copied from the original bitcoinjs/bolt11 library (https://github.com/bitcoinjs/bolt11/blob/master/payreq.js) and should be kept unchanged to maintain compatibility with the upstream source, even if there are potential improvements or syntax issues.
Applied to files:
src/test/fixtures.json
📚 Learning: 2025-06-30T18:33:29.839Z
Learnt from: bordalix
PR: arkade-os/wallet#114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
Applied to files:
package.json
🧬 Code graph analysis (2)
src/test/lib/fiat.test.ts (1)
src/lib/fiat.ts (1)
getPriceFeed(8-19)
src/test/lib/lnurl.test.ts (1)
src/lib/lnurl.ts (4)
isValidLnUrl(65-65)getCallbackUrl(67-76)checkLnUrlConditions(78-86)fetchInvoice(88-99)
🔇 Additional comments (4)
src/test/lib/fiat.test.ts (1)
11-18: Good: Correctly maps API payload to expected shape.The test verifies EUR/USD last → lowercase keys, matching src/lib/fiat.ts.
package.json (2)
7-8: SDK/boltz-swap bumps: confirm API compatibility across call sitesThese minor bumps look fine. Please re-check any settle/swap flows for signature/behavior changes before release.
34-35: Nice: added vitest UI and coverage scriptsGood quality-of-life additions.
src/test/fixtures.json (1)
127-133: Mixed schema in invalid vectors; ensure the test harness accepts address+options without uri.This case omits
uribut providesaddressandoptions. Verify the test runner builds the URI from these fields for negative tests; if not, add the expliciturito match the options.Apply if needed:
{ "exception": "Invalid amount", "address": "1BgGZ9tcN4rm9KBzDn7KprQz87SZ26SAMH", + "uri": "bitcoin:1BgGZ9tcN4rm9KBzDn7KprQz87SZ26SAMH?amount=-1", "options": { "amount": "-1" } },
| "decimal.js": "^10.5.0", | ||
| "dexie": "^4.0.11", | ||
| "dexie-react-hooks": "^1.1.7", | ||
| "dompurify": "^3.2.6", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
DOMPurify added — enforce strict sanitization config at call sites
Ensure sanitize() is called with a restrictive config (empty ALLOWED_TAGS/ATTR, forbid on* attrs, disallow unknown protocols).
🤖 Prompt for AI Agents
In package.json around line 20, DOMPurify was added but call sites must enforce
a strict sanitize config; update every DOMPurify.sanitize(...) invocation to
pass a restrictive options object such as ALLOWED_TAGS: [], ALLOWED_ATTR: [],
FORBID_ATTR: ['on*'] and set ALLOW_UNKNOWN_PROTOCOLS: false (or use
ALLOWED_URI_REGEXP to whitelist protocols) so event handler attributes and
unknown protocols are disallowed; ensure all sanitize calls use this config (or
a shared wrapper utility) rather than the default permissive behavior.
| "privatekey": { | ||
| "secret": { | ||
| "nsec": "nsec13374vlgh58p4sw9nrdc72rmf40x62qgvk6jac8rekwm872p0esesxqx6t5", | ||
| "hex": "8c7d567d17a1c35838b31b71e50f69abcda5010cb6a5dc1c79b3b67f282fcc33" | ||
| }, | ||
| "public": { | ||
| "npub": "npub1lvnwe70kgqpta7jctmugtrfy8gk9ntc787449833a09356w4f0us7w9wej", | ||
| "hex": "fb26ecf9f64002befa585ef8858d243a2c59af1e3fab529e31ebcb1a69d54bf9" | ||
| } | ||
| } |
There was a problem hiding this comment.
Real-looking Nostr keys in repo; replace with generated test keys or allowlist with justification.
Committed nsec…/npub… pairs can trip secret scanners and cause policy friction, even if test-only. Prefer generating deterministic throwaway keys at test runtime and removing them from fixtures; alternatively, add a scoped secret-scan allowlist with clear rationale.
Option A (recommended): generate per-test keys and reference them instead of fixtures.
// src/test/utils/nostrKeys.ts
import { sha256 } from '@noble/hashes/sha256'
import { bytesToHex } from '@noble/hashes/utils'
import { getPublicKey } from 'nostr-tools' // or noble-secp256k1 if preferred
export const testNostrKeys = (seed = 'arkade-wallet-test'): { seckeyHex: string; pubkeyHex: string } => {
const seckeyHex = bytesToHex(sha256(seed))
const pubkeyHex = getPublicKey(seckeyHex)
return { seckeyHex, pubkeyHex }
}Option B (interim): add a scanner allowlist limited to this file/keys with a comment explaining they are inert test vectors. I can provide a minimal gitleaks/secretlint rule if you want.
🤖 Prompt for AI Agents
In src/test/fixtures.json around lines 182-191, the file contains real-looking
Nostr nsec/npub key pairs that can trigger secret scanners; replace them by
either (A) removing these hardcoded keys and updating tests to generate
deterministic throwaway keys at runtime (e.g., derive a seckey via a fixed test
seed and compute its pubkey) and reference those generated values in tests, or
(B) if immediate change is infeasible, add a repository scanner allowlist entry
scoped to this file/these exact values with a brief comment stating they are
inert test vectors and why, then commit that allowlist rule; update tests/docs
accordingly.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
src/components/NeedsPassword.tsx (3)
40-46: Nit: improve copy.“Insert password” reads odd in English; “Enter password” is more idiomatic.
Apply this diff:
- label='Insert password' + label='Enter password'
47-47: Show errors for both flows (biometrics and password).
ErrorMessageis only rendered in the password branch; biometric failures won’t surface to users.Apply this diff to render the error below the conditional:
- <ErrorMessage text={error} error={Boolean(error)} /> - </FlexCol> - )} + </FlexCol> + )} + <ErrorMessage text={error} error={Boolean(error)} />
25-25: Biometric UX: avoid double-trigger and surface failure inline.Consider guarding against rapid re-clicks and showing a user-visible message on catch (e.g., local error state rendered via
ErrorMessage). Right now failures only log to console.src/test/screens/wallet/unlock.test.tsx (2)
22-22: Prefer accessible queries for the buttonUse role+name to reduce brittleness against copy changes and ensure a11y alignment.
Apply this diff:
- const button = screen.getByText('Unlock wallet') + const button = screen.getByRole('button', { name: /unlock wallet/i })
7-10: Remove unused PWA mock in unlock.test.tsx
src/test/screens/wallet/unlock.test.tsx (lines 7–10) defines and mockspwaCanInstall, but Unlock.tsx never importslib/pwa—delete these lines or relocate the mock to a shared setup if other wallet tests need it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/components/NeedsPassword.tsx(3 hunks)src/test/screens/wallet/unlock.test.tsx(1 hunks)src/test/setup.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/setup.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-06T09:53:28.428Z
Learnt from: bordalix
PR: arkade-os/wallet#134
File: src/screens/Settings/Password.tsx:34-39
Timestamp: 2025-08-06T09:53:28.428Z
Learning: In src/screens/Settings/Password.tsx, the saveNewPassword function doesn't need error handling for getPrivateKey because it's only called after authentication is verified. The useEffect validates the oldPassword using isValidPassword() which internally calls getPrivateKey(), and only sets authenticated=true if successful. The saveNewPassword function has a guard clause that returns early if not authenticated, so getPrivateKey(oldPassword) is guaranteed to work when called.
Applied to files:
src/components/NeedsPassword.tsx
📚 Learning: 2025-07-21T18:25:36.391Z
Learnt from: bordalix
PR: arkade-os/wallet#121
File: src/screens/Init/Restore.tsx:58-58
Timestamp: 2025-07-21T18:25:36.391Z
Learning: In src/screens/Init/Restore.tsx, the Input component for private key restoration intentionally omits the `value` prop to make it uncontrolled. This is a security feature to prevent the private key from being displayed if users navigate away and return to the screen, avoiding potential exposure of sensitive data in the UI.
Applied to files:
src/components/NeedsPassword.tsx
🧬 Code graph analysis (2)
src/test/screens/wallet/unlock.test.tsx (2)
src/lib/pwa.ts (1)
pwaCanInstall(5-5)src/screens/Wallet/Unlock.tsx (1)
Unlock(10-36)
src/components/NeedsPassword.tsx (8)
src/lib/biometrics.ts (1)
authenticateUser(71-98)src/lib/logs.ts (1)
consoleError(40-44)src/icons/Lock.tsx (1)
LockIcon(1-11)src/components/Text.tsx (1)
Text(25-79)src/components/CenterScreen.tsx (1)
CenterScreen(9-28)src/components/FlexCol.tsx (1)
FlexCol(15-40)src/components/InputPassword.tsx (1)
InputPassword(15-41)src/components/Error.tsx (1)
ErrorMessage(10-21)
🔇 Additional comments (4)
src/components/NeedsPassword.tsx (3)
35-36: LGTM: icon/text tweaks for biometrics.Lock icon + centered text looks consistent with the new pattern.
3-3: LGTM:ErrorMessageimport.Matches the component’s default export from
./Error.
14-14: LGTM:LockIconimport.Icon component and usage align with
src/icons/Lock.tsx.src/test/screens/wallet/unlock.test.tsx (1)
21-27: Baseline render assertions look goodThe test validates the screen title, primary action, and password field presence — solid smoke coverage for initial render.
| } | ||
|
|
||
| const handleBiometrics = () => authenticateUser(wallet.passkeyId).then(onPassword).catch(consoleError) | ||
| const handleChange = (ev: any) => setPassword(ev.target.value) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix Ionic event usage: use ev.detail.value, not ev.target.value.
With IonInput, onIonInput emits a CustomEvent; target.value will be undefined, breaking password capture.
Apply this diff:
- const handleChange = (ev: any) => setPassword(ev.target.value)
+ const handleChange = (ev: any) => setPassword(ev.detail?.value ?? '')Run to find similar issues across the repo:
🏁 Script executed:
#!/bin/bash
# Look for likely misuses of Ionic input values
rg -nP --type=tsx -C2 '\bonIonInput\b|\bev\.(target|currentTarget)\.value\b' srcLength of output: 110
🏁 Script executed:
#!/bin/bash
# Search for direct ev.target.value or ev.currentTarget.value usage in TSX files
rg -nP 'ev\.(target|currentTarget)\.value' -g '*.tsx' src
# Search for onIonInput usage in TSX files
rg -nP 'onIonInput' -g '*.tsx' srcLength of output: 465
Fix Ionic input event handling in NeedsPassword.tsx
Change to use ev.detail?.value instead of ev.target.value to correctly capture the input value.
--- a/src/components/NeedsPassword.tsx
+++ b/src/components/NeedsPassword.tsx
@@ 26
- const handleChange = (ev: any) => setPassword(ev.target.value)
+ const handleChange = (ev: any) => setPassword(ev.detail?.value ?? '')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleChange = (ev: any) => setPassword(ev.target.value) | |
| const handleChange = (ev: any) => setPassword(ev.detail?.value ?? '') |
🤖 Prompt for AI Agents
In src/components/NeedsPassword.tsx around line 26, the input handler currently
reads the value from ev.target.value which doesn't work with Ionic's IonInput;
update the handler to read the value from ev.detail?.value (and adjust the event
parameter type to the appropriate Ionic input event type if desired) and pass
that value into setPassword so the component captures user input correctly.
| describe('Unlock screen', () => { | ||
| const user = userEvent.setup() | ||
| it('renders the unlock screen with the correct elements', async () => { |
There was a problem hiding this comment.
Don’t mutate DOM directly; simulate real typing with user-event
Assigning input.value bypasses React’s onChange pipeline and can yield false positives. Use user.type you already set up.
Apply this diff:
- const user = userEvent.setup()
+ const user = userEvent.setup()
@@
- input.value = 'testpassword'
- expect(input).toHaveValue('testpassword')
+ await user.type(input, 'testpassword')
+ expect(input).toHaveValue('testpassword')Also applies to: 28-29
🤖 Prompt for AI Agents
In src/test/screens/wallet/unlock.test.tsx around lines 12-14 (and similarly
lines 28-29), the test mutates the input by assigning to input.value which
bypasses React's onChange handling; replace direct DOM mutation with the
user-event API by calling await user.type(targetInputElement, 'yourPassword')
(and remove the input.value assignment), ensuring you await the typing and
trigger any submit or blur needed so React handlers run as in real user
interaction.
| render( | ||
| <IonApp> | ||
| <Unlock /> | ||
| </IonApp>, | ||
| ) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Stabilize Unlock side effects (initWallet/navigate/getPrivateKey) to avoid flakiness
Unlock triggers async work on mount via useEffect (default password → getPrivateKey → initWallet → navigate). Without explicit mocks/providers, the test can navigate away or throw depending on defaults. Recommend mocking these dependencies or wrapping with providers that no-op.
Run this script to locate the exact import paths to mock and check whether the contexts provide safe defaults:
Once you have the paths, add top-level mocks like:
- mock
getPrivateKeytovi.fn().mockRejectedValueOnce(new Error("test"))(so the screen stays put), or - provide
NavigationContext.Providerwith{ navigate: vi.fn() }andWalletContext.Providerwith{ initWallet: vi.fn() }.
I can wire a minimal provider wrapper once we confirm the context module paths.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Imports in Unlock.tsx for side-effectful deps:"
rg -nP 'import.+(getPrivateKey|defaultPassword|NavigationContext|WalletContext)' src/screens/Wallet/Unlock.tsx -C2 || true
echo
echo "Context defaults (look for createContext initial values):"
rg -nP 'createContext\\s*\\(' -n src -C2 | rg -nP 'Wallet|Navigation' -C2 || trueLength of output: 1023
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "WalletContext createContext call:"
rg -nP 'createContext' -n src/providers/wallet
echo
echo "NavigationContext createContext call:"
rg -nP 'createContext' -n src/providers/navigationLength of output: 204
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "List src/providers directory structure:"
fd -t d -d 1 src/providers || true
echo
echo "Find files under src/providers containing WalletContext or NavigationContext:"
rg -lP 'WalletContext|NavigationContext' -n src/providers || true
echo
echo "Show createContext lines in those files:"
for f in $(rg -lP 'WalletContext|NavigationContext' -n src/providers); do
echo "$f:"
rg -nP 'createContext' -n "$f" || true
doneLength of output: 3061
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "WalletContext default value:"
sed -n '42,60p' src/providers/wallet.tsx
echo
echo "NavigationContext default value:"
sed -n '154,180p' src/providers/navigation.tsxLength of output: 1422
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,200p' src/screens/Wallet/Unlock.tsx || trueLength of output: 1202
Mock getPrivateKey in unlock.test.tsx
Stub out the real getPrivateKey call to avoid async side effects—e.g.:
vi.mock('../../lib/privateKey', () => ({
getPrivateKey: vi.fn().mockResolvedValue('stub-key'),
}));Both WalletContext and NavigationContext use no-op defaults, so no provider wrappers are required.
🤖 Prompt for AI Agents
In src/test/screens/wallet/unlock.test.tsx around lines 15 to 19, the test
renders Unlock without stubbing the real getPrivateKey which can cause async
side-effects; add a vi.mock for the '../../lib/privateKey' module at the top of
the test file that exports getPrivateKey as a
vi.fn().mockResolvedValue('stub-key') so the component receives a predictable
key and the test remains synchronous; no WalletContext or NavigationContext
providers are required since they have no-op defaults.
|
Substituted by #148 |
Several PR merged here for tests:
Summary by CodeRabbit
New Features
Improvements
Tests
Chores