Riga #131
Riga #131
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/components/Scanner.tsx (1)
71-76: “Cancel” may not work if pressed quickly
handleClosebails out whencancelis stillundefined(camera not started yet), leaving the scanner open and the UI unresponsive for the user who immediately taps Cancel.
Drop the guard and always execute the shutdown logic.- if (!cancel && !error) return if (cancel) cancel() camera?.stop() close()
🧹 Nitpick comments (2)
src/components/Scanner.tsx (2)
27-28: Remove leftover debug logging
console.log('currentImplementation', currentImplementation)ships dev noise to production consoles.
Delete before merge.
51-53: Consider picking the rear camera by default
await camera.setDevice(devices[0].deviceId)often selects the front camera on phones, making QR scanning awkward.
Prefer the first environment-facing device or the last device in the list for better UX.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
package.json(1 hunks)src/components/Scanner.tsx(4 hunks)src/screens/Settings/Nostr.tsx(1 hunks)src/screens/Settings/Server.tsx(1 hunks)src/screens/Wallet/Notes/Form.tsx(1 hunks)src/screens/Wallet/Send/Form.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- package.json
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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.
📚 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:
src/screens/Wallet/Send/Form.tsxsrc/screens/Wallet/Notes/Form.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/screens/Wallet/Send/Form.tsxsrc/screens/Settings/Nostr.tsxsrc/screens/Wallet/Notes/Form.tsx
📚 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/screens/Wallet/Send/Form.tsxsrc/screens/Settings/Nostr.tsxsrc/screens/Wallet/Notes/Form.tsx
🧬 Code Graph Analysis (5)
src/screens/Wallet/Send/Form.tsx (1)
src/components/Scanner.tsx (1)
Scanner(20-34)
src/screens/Settings/Server.tsx (1)
src/components/Scanner.tsx (1)
Scanner(20-34)
src/screens/Settings/Nostr.tsx (1)
src/components/Scanner.tsx (1)
Scanner(20-34)
src/screens/Wallet/Notes/Form.tsx (1)
src/components/Scanner.tsx (1)
Scanner(20-34)
src/components/Scanner.tsx (6)
src/lib/error.ts (1)
extractError(1-10)src/components/Content.tsx (1)
Content(9-16)src/components/Padded.tsx (1)
Padded(7-14)src/components/Error.tsx (1)
Error(10-21)src/components/ButtonsOnBottom.tsx (1)
ButtonsOnBottom(10-26)src/components/Button.tsx (1)
Button(20-62)
🔇 Additional comments (4)
src/screens/Settings/Server.tsx (1)
66-66: Change looks goodProp rename aligns with new
ScannerAPI; no further issues.src/screens/Wallet/Send/Form.tsx (1)
314-317: Good migration to new Scanner props
smartSetErroralready matches(msg:string)=>void; update is correct.src/screens/Settings/Nostr.tsx (1)
53-53: LGTMProp names updated consistently.
src/screens/Wallet/Notes/Form.tsx (1)
56-56: No issues foundCallback rename matches new interface.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
src/components/Scanner.tsx (2)
34-41: Instance state re-initialises on every render
camera,canvas, andcancelare ordinaryletvariables – each render shadows the previous instances, sohandleClosein previous renders may reference stale objects. Use stable refs instead:- let camera: any - let canvas: QRCanvas - let cancel: () => void + const camera = useRef<any>() + const canvas = useRef<QRCanvas>() + const cancel = useRef<() => void>()Update usages accordingly.
69-74: User cannot cancel before camera initialisesEarly-return guards prevent
handleClosefrom firing ifcancelis undefined and no error yet – exactly the state right after opening. Result: the “Cancel” button does nothing for 1-2 s.- if (!cancel && !error) return + // allow immediate cancelJust drop the condition.
🧹 Nitpick comments (3)
src/components/Scanner.tsx (3)
1-1: Consider lazy-loading the QR scanner lib
react-qr-barcode-scanneradds ~70-90 KB to the bundle. Wrapping it in a dynamicimport()(e.g.React.lazy) will keep the initial load lighter, especially for users that never open the scanner.
98-109: ReuseextractErrorfor consistency
ScannerReact.handleErrorcrafts its own message. Align with the Mills impl for consistent UX:- const handleError = (error: any) => { - onError(error.message || 'An error occurred while scanning') + const handleError = (error: any) => { + onError(extractError(error))
111-117: Clearing errors afterhandleCloseis redundant
onError('')executes afterclose()unmounts the scanner, so the cleared state is never displayed. Remove the call to avoid pointless renders.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Scanner.tsx(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
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.
🧬 Code Graph Analysis (1)
src/components/Scanner.tsx (6)
src/lib/error.ts (1)
extractError(1-10)src/components/Content.tsx (1)
Content(9-16)src/components/Padded.tsx (1)
Padded(7-14)src/components/Error.tsx (1)
Error(10-21)src/components/ButtonsOnBottom.tsx (1)
ButtonsOnBottom(10-26)src/components/Button.tsx (1)
Button(20-62)
🔇 Additional comments (1)
src/components/Scanner.tsx (1)
42-67: IncompleteuseEffectdependency list
useEffect(..., [videoRef])omitsonData,onError, andclose. This can trigger eslint-hook warnings and stale closures when the parent supplies new callbacks.-}, [videoRef]) +}, [videoRef, onData, onError, close])If you convert camera/canvas/cancel to refs, they don’t belong in the array.
| const handleSwitch = () => { | ||
| setCurrentImplementation(currentImplementation === 'mills' ? 'react' : 'mills') | ||
| } | ||
|
|
||
| return currentImplementation === 'mills' ? ( | ||
| <ScannerMills close={close} label={label} onData={onData} onError={onError} onSwitch={handleSwitch} /> | ||
| ) : ( | ||
| <ScannerReact close={close} label={label} onData={onData} onError={onError} onSwitch={handleSwitch} /> | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Switch handler may leak camera/stream resources
Toggling implementations only flips state; the active scanner (video track / WebRTC stream) stays alive because handleClose() isn’t called first. When switching many times, multiple streams can accumulate and keep the camera LED on.
-const handleSwitch = () => {
- setCurrentImplementation(currentImplementation === 'mills' ? 'react' : 'mills')
-}
+const handleSwitch = () => {
+ closeActiveScanner?.(); // call through ref supplied by child
+ setCurrentImplementation(cur => (cur === 'mills' ? 'react' : 'mills'));
+}A small callback prop from child → parent that exposes its own handleClose would solve this.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/components/Scanner.tsx around lines 23 to 31, the handleSwitch function
switches scanner implementations without calling handleClose, causing camera
streams to remain active and leak resources. Modify the child scanner components
to expose a handleClose callback prop that the parent can call before switching.
Update handleSwitch to invoke this handleClose callback to properly release
camera/stream resources before toggling the implementation state.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/screens/Settings/Backup.tsx (1)
24-34: Clear error on retry and guard against stale updates in the effect.Reset the error when a new attempt starts and ignore late resolves/rejects after password changes or unmount. This prevents flicker and state updates on unmounted components.
useEffect(() => { - if (!password) return - getPrivateKey(password) - .then((privateKey) => { - setNsec(privateKeyToNsec(privateKey)) - }) - .catch((err) => { - consoleError(err, 'error unlocking wallet') - setError('Invalid password') - }) + if (!password) return + let cancelled = false + setError('') // clear previous error before a new attempt + getPrivateKey(password) + .then((privateKey) => { + if (cancelled) return + setNsec(privateKeyToNsec(privateKey)) + }) + .catch((err) => { + consoleError(err, 'error unlocking wallet') + if (cancelled) return + setError('Invalid password') + }) + return () => { + cancelled = true + } }, [password])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/screens/Settings/Backup.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
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.
📚 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/screens/Settings/Backup.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/screens/Settings/Backup.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:
src/screens/Settings/Backup.tsx
📚 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/screens/Settings/Backup.tsx
⏰ 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: arkade-wallet
🔇 Additional comments (1)
src/screens/Settings/Backup.tsx (1)
32-33: Good change: fixed “Invalid password” message avoids leaking error details.This aligns with prior guidance around not surfacing internals of getPrivateKey failures. For this screen (no pre-auth), a generic message is appropriate.
| .catch((err) => { | ||
| consoleError(err, 'error unlocking wallet') | ||
| setError(extractError(err)) | ||
| setError('Invalid password') | ||
| }) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm getPrivateKey only throws auth-related errors here.
If getPrivateKey can fail for non-auth reasons (e.g., missing/corrupt vault), a different user message may be warranted. Otherwise, the generic “Invalid password” is fine.
🏁 Script executed:
#!/bin/bash
# Locate getPrivateKey implementation and inspect thrown errors to confirm they are auth-related.
set -euo pipefail
echo "Searching for getPrivateKey implementation..."
rg -n -A 5 -B 2 $'export (async )?function getPrivateKey|export const getPrivateKey' | sed -n '1,120p'
echo
echo "Where is getPrivateKey used?"
rg -n -A 2 -B 2 "getPrivateKey\\("
echo
echo "Scan for throws in privateKey module(s)..."
fd privateKey | xargs -I{} sh -c 'echo ":: {} ::"; rg -n -A 2 -B 2 "throw|new Error\\(" "{}"'Length of output: 2926
Handle missing encrypted private key separately in Backup screen
The call to getPrivateKey(password) can throw a non–authentication error when the vault is missing or corrupt (new Error('No encrypted private key found')). Right now all errors are reported as “Invalid password,” which could mislead users if their vault is actually missing.
Please update the catch block in src/screens/Settings/Backup.tsx (around lines 26–33) to distinguish between these cases. For example:
.catch((err) => {
consoleError(err, 'error unlocking wallet')
- setError('Invalid password')
+ if (err.message === 'No encrypted private key found') {
+ setError('Unable to backup: vault not found or corrupted')
+ } else {
+ setError('Invalid password')
+ }
})This way, users get an accurate message when their vault is missing or corrupted, while still showing “Invalid password” for bad credentials.
[src/lib/privateKey.ts:39] [src/screens/Settings/Backup.tsx:26–33]
📝 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.
| .catch((err) => { | |
| consoleError(err, 'error unlocking wallet') | |
| setError(extractError(err)) | |
| setError('Invalid password') | |
| }) | |
| .catch((err) => { | |
| consoleError(err, 'error unlocking wallet') | |
| if (err.message === 'No encrypted private key found') { | |
| setError('Unable to backup: vault not found or corrupted') | |
| } else { | |
| setError('Invalid password') | |
| } | |
| }) |
🤖 Prompt for AI Agents
In src/screens/Settings/Backup.tsx around lines 26 to 33, the catch block for
getPrivateKey(password) treats all errors as "Invalid password," which is
misleading if the vault is missing or corrupt. Update the catch block to check
if the error message is 'No encrypted private key found' and set a specific
error message like "Vault missing or corrupted" for that case; otherwise, keep
"Invalid password" for authentication errors. This will provide users with
accurate feedback based on the error type.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
package.json(2 hunks)src/components/InputAmount.tsx(3 hunks)src/ionic.css(2 hunks)src/screens/Wallet/Send/Form.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/screens/Wallet/Send/Form.tsx
- src/ionic.css
- package.json
⏰ 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: arkade-wallet
🔇 Additional comments (2)
src/components/InputAmount.tsx (2)
18-18: Prop addition looks goodOptional onMax prop is well-scoped and backward compatible.
33-33: Plumbing prop into component is correctonMax is properly added to the props destructure.
| {onMax ? ( | ||
| <IonText | ||
| slot='end' | ||
| style={{ ...fontStyle, marginLeft: '0.5rem', color: 'var(--purpletext)', cursor: 'pointer' }} | ||
| onClick={onMax} | ||
| > | ||
| Max | ||
| </IonText> | ||
| ) : null} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make “Max” control accessible and state-aware
Use semantic/accessible interactions and hide when input is disabled/readOnly. Also support keyboard activation.
Apply this diff:
- {onMax ? (
- <IonText
- slot='end'
- style={{ ...fontStyle, marginLeft: '0.5rem', color: 'var(--purpletext)', cursor: 'pointer' }}
- onClick={onMax}
- >
- Max
- </IonText>
- ) : null}
+ {onMax && !disabled && !readOnly ? (
+ <IonText
+ slot='end'
+ role='button'
+ aria-label='Use maximum amount'
+ tabIndex={0}
+ style={{ ...fontStyle, marginLeft: '0.5rem', color: 'var(--purpletext)', cursor: 'pointer' }}
+ onClick={onMax}
+ onKeyDown={(e) => {
+ if (e.key === 'Enter' || e.key === ' ') {
+ e.preventDefault()
+ onMax()
+ }
+ }}
+ >
+ Max
+ </IonText>
+ ) : null}📝 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.
| {onMax ? ( | |
| <IonText | |
| slot='end' | |
| style={{ ...fontStyle, marginLeft: '0.5rem', color: 'var(--purpletext)', cursor: 'pointer' }} | |
| onClick={onMax} | |
| > | |
| Max | |
| </IonText> | |
| ) : null} | |
| {onMax && !disabled && !readOnly ? ( | |
| <IonText | |
| slot='end' | |
| role='button' | |
| aria-label='Use maximum amount' | |
| tabIndex={0} | |
| style={{ ...fontStyle, marginLeft: '0.5rem', color: 'var(--purpletext)', cursor: 'pointer' }} | |
| onClick={onMax} | |
| onKeyDown={(e) => { | |
| if (e.key === 'Enter' || e.key === ' ') { | |
| e.preventDefault() | |
| onMax() | |
| } | |
| }} | |
| > | |
| Max | |
| </IonText> | |
| ) : null} |
🤖 Prompt for AI Agents
In src/components/InputAmount.tsx around lines 91 to 99, replace the clickable
IonText "Max" with an accessible, keyboard-focusable control (preferably a
semantic <button> or IonButton with type="button") that preserves the existing
styles; ensure it is not rendered (or is hidden with aria-hidden) when onMax is
missing or when the input is disabled or readOnly, add aria-label (e.g., "Set
max amount"), wire onClick to onMax and also call onMax from key activation
(Enter/Space) or let the native button handle it, and ensure the control
reflects disabled state (disabled attribute) so it is skipped from keyboard
focus when input is non-editable.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/screens/Wallet/Index.tsx (2)
32-32: Nit: avoid shadowing globalalertand clarify intentConsider renaming to motd or remoteAlert for clarity and to avoid shadowing window.alert in dev tools and logs.
-const [alert, setAlert] = useState<string | undefined>() +const [motd, setMotd] = useState<string | undefined>()Follow-through: update call sites accordingly.
39-41: Harden alert fetching: trim, ignore empties, and avoid setState after unmountCurrent code can render empty/placeholder text and may set state after unmount. Add an unmounted guard and ignore blank messages.
-useEffect(() => { - getAlert().then(setAlert) -}, []) +useEffect(() => { + let cancelled = false + ;(async () => { + try { + const msg = await getAlert() + const cleaned = msg?.trim() + if (!cancelled && cleaned) setAlert(cleaned) + } catch { + // no-op; getAlert already logs + } + })() + return () => { + cancelled = true + } +}, [])Additionally, consider improving getAlert to check response.ok and ignore non-200s:
// src/lib/alerts.ts export const getAlert = async (): Promise<string | undefined> => { try { const response = await fetch('https://raw.githubusercontent.com/arkade-os/wallet/riga/motd.html', { cache: 'no-store' }) if (!response.ok) return undefined const message = (await response.text()).trim() return message.length > 0 ? message : undefined } catch (error) { console.error('Error fetching alert:', error) return undefined } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/screens/Wallet/Index.tsx(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/screens/Wallet/Index.tsx (3)
src/lib/alerts.ts (1)
getAlert(1-9)src/lib/constants.ts (1)
isRiga(7-7)src/components/AlertBox.tsx (1)
InfoBox(44-46)
⏰ 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: arkade-wallet
🔇 Additional comments (2)
src/screens/Wallet/Index.tsx (2)
21-23: Imports LGTMNew imports are cohesive with the MOTD/alert and Riga flag features. No issues.
73-73: UI spacing tweak is finePadding change reads fine and matches the new InfoBox below.
| <FlexRow padding='0 0 0.5rem 0'> | ||
| <Button icon={<SendIcon />} label='Send' onClick={handleSend} /> | ||
| <Button icon={<ReceiveIcon />} label='Receive' onClick={handleReceive} /> | ||
| <Button icon={<ReceiveIcon />} label='Receive' onClick={handleReceive} disabled={isRiga} /> |
There was a problem hiding this comment.
Critical: disabled={isRiga} may be wrong due to env parsing bug
isRiga is produced by casting an env var to boolean:
src/lib/constants.ts: export const isRiga = (import.meta.env.VITE_IS_RIGA as boolean) ?? false
At runtime VITE_IS_RIGA is a string ("true"/"false"). Casting doesn't coerce values; "false" is a non-empty string, which React treats as truthy when used for a boolean attribute. Result: Receive may be disabled even when VITE_IS_RIGA="false".
Fix the source of truth to parse strings safely:
// src/lib/constants.ts
// robust boolean coercion with sensible defaults
const raw = String(import.meta.env.VITE_IS_RIGA ?? '').trim().toLowerCase()
export const isRiga = raw === '1' || raw === 'true' || raw === 'yes'Optionally, type it explicitly to avoid accidental misuse:
export const isRiga: boolean = raw === '1' || raw === 'true' || raw === 'yes'If you prefer to avoid code changes outside this file immediately, guard locally until constants.ts is fixed:
-<Button ... disabled={isRiga} />
+<Button ... disabled={isRiga === true} />But please prioritize fixing constants.ts to keep usage consistent across the app.
🤖 Prompt for AI Agents
In src/screens/Wallet/Index.tsx around line 75, the Receive button is using
disabled={isRiga} but isRiga is derived from an env var cast that treats the
string "false" as truthy; update the source of truth in src/lib/constants.ts to
robustly parse the env string (e.g. normalize with
String(import.meta.env.VITE_IS_RIGA ?? '').trim().toLowerCase() and set isRiga =
raw === '1' || raw === 'true' || raw === 'yes' and type it as boolean) so
components receive a proper boolean, or temporarily guard here by converting
isRiga to a boolean (e.g. compare normalized string or use Boolean parsing)
until constants.ts is fixed.
src/screens/Wallet/Index.tsx
Outdated
| <Button icon={<ReceiveIcon />} label='Receive' onClick={handleReceive} /> | ||
| <Button icon={<ReceiveIcon />} label='Receive' onClick={handleReceive} disabled={isRiga} /> | ||
| </FlexRow> | ||
| {alert ? <InfoBox text={alert} /> : null} |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Remote content rendering: verify sanitization or switch to plain text
You’re sourcing arbitrary HTML (motd.html) from GitHub. If AlertBox renders text via dangerouslySetInnerHTML, sanitize first (e.g., DOMPurify). If it renders as escaped text, the HTML will show as raw markup—either sanitize-and-render HTML or switch to a .txt source and continue rendering as plain text.
Action items:
- Decide whether MOTD should support HTML. If yes, sanitize; if no, change to plain text and ensure AlertBox does not use dangerous HTML rendering.
Run this to check for dangerous HTML usage:
If HTML is intended, a safe pattern inside AlertBox:
import DOMPurify from 'dompurify'
// ...
<div
className={...}
dangerouslySetInnerHTML={{ __html: DOMPurify.sanitize(text, { USE_PROFILES: { html: true } }) }}
/>🏁 Script executed:
#!/bin/bash
# Find any usage of dangerouslySetInnerHTML (especially in AlertBox)
rg -n -A2 -B2 'dangerouslySetInnerHTML' src
# Inspect AlertBox implementation for how "text" is rendered
rg -n -A20 -B5 'export function AlertBox|export function InfoBox|text:' src/componentsLength of output: 16341
Remote MOTD content is rendered safely as escaped text (no XSS risk)
InfoBox (and its underlying AlertBox/Text components) render the text prop as children—there’s no use of dangerouslySetInnerHTML, so HTML tags in your MOTD will be displayed literally rather than executed.
Action items:
- Decide whether the MOTD should include HTML formatting:
- If yes, update InfoBox (or AlertBox) to sanitize and render HTML. For example, import DOMPurify and replace the
{text}child with:<div className={…} dangerouslySetInnerHTML={{ __html: DOMPurify.sanitize(text, { USE_PROFILES: { html: true } }) }} />
- If no, switch to a plain-text source (e.g. fetch
motd.txtinstead ofmotd.html) so that users aren’t shown raw HTML tags.
- If yes, update InfoBox (or AlertBox) to sanitize and render HTML. For example, import DOMPurify and replace the
- Apply these changes in:
src/screens/Wallet/Index.tsx(where you fetch/render the MOTD)src/components/AlertBox.tsxor around<InfoBox>if you opt into HTML rendering
🤖 Prompt for AI Agents
In src/screens/Wallet/Index.tsx around line 77 the MOTD is passed into InfoBox
as raw text which currently renders HTML tags literally; decide whether the MOTD
should support HTML formatting or remain plain text and implement accordingly:
if you want HTML, update InfoBox (or its AlertBox/Text children) to sanitize and
render HTML using a library like DOMPurify and replace the plain child rendering
with a sanitized dangerouslySetInnerHTML wrapper; if you want plain text, ensure
the source is plain (fetch motd.txt or strip HTML on fetch) and keep passing
escaped text to InfoBox; apply the chosen change in both
src/screens/Wallet/Index.tsx (fetch/render) and src/components/AlertBox.tsx (or
InfoBox) to keep behavior consistent.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (3)
src/lib/asp.ts (3)
29-35: Add insufficient-funds guard before building outputsCurrently, if selectedAmount < amount, we proceed and rely on wallet.settle to fail later. This yields a less actionable error and wastes a network call.
Add an explicit guard:
for (const vtxo of vtxos) { if (selectedAmount >= amount) break selectedVtxos.push(vtxo) selectedAmount += vtxo.value } + if (selectedAmount < amount) { + throw new Error('Insufficient VTXO balance to perform collaborative exit') + } const changeAmount = selectedAmount - amount
27-27: Annotate selectedVtxos to preserve type safetyselectedVtxos is inferred as any[] when initialized empty. Explicitly annotate to keep the inputs compatible with wallet.settle and catch mistakes at compile time.
- const selectedVtxos = [] + const selectedVtxos: ExtendedVirtualCoin[] = []Optional: If ExtendedVirtualCoin.value is bigint, migrate selectedAmount to bigint to avoid accidental number/bigint mixing.
107-113: Ensure async DB errors are caught: await getSpendableVtxos() in try/catchAs written, try/catch won’t intercept Promise rejections because the promise isn’t awaited. This can leak rejections upstream.
async function getVtxos(): Promise<ExtendedVirtualCoin[]> { try { - return vtxosRepository.getSpendableVtxos() + return await vtxosRepository.getSpendableVtxos() } catch (err) { consoleError(err, 'error getting vtxos from DB') return [] } }
🧹 Nitpick comments (2)
src/components/TransactionsList.tsx (1)
58-58: Confirm intent to drop txid/identifier; consider a compact ID for discriminability.Only showing “Sent/Received” may make adjacent transactions with same amount/date harder to tell apart. If this simplification was for privacy or to reduce clutter, ignore. Otherwise, consider adding a compact, derived ID (no extra imports) while keeping the minimal look.
Proposed minimal diff (no new imports; uses existing tx fields seen in the key):
- const Kind = () => <Text thin>{tx.type === 'sent' ? 'Sent' : 'Received'}</Text> + const Kind = () => { + const label = tx.type === 'sent' ? 'Sent' : 'Received' + const id = + tx.boardingTxid || tx.roundTxid || tx.redeemTxid || '' + const shortId = id ? `${id.slice(0, 6)}...${id.slice(-6)}` : '' + return <Text thin>{shortId ? `${label} · ${shortId}` : label}</Text> + }Alternative (adds copy-on-tap for power users without changing visuals much):
- const Kind = () => <Text thin>{tx.type === 'sent' ? 'Sent' : 'Received'}</Text> + const Kind = () => { + const label = tx.type === 'sent' ? 'Sent' : 'Received' + const id = + tx.boardingTxid || tx.roundTxid || tx.redeemTxid || '' + const shortId = id ? `${id.slice(0, 6)}...${id.slice(-6)}` : '' + return <Text thin copy={id || undefined}>{shortId ? `${label} · ${shortId}` : label}</Text> + }src/lib/asp.ts (1)
128-134: Deduplicate sendOffChain/sendOnChain or make semantic difference explicitBoth functions call wallet.sendBitcoin with identical parameters. Either:
- Consolidate to a single function to avoid duplication, or
- Call distinct SDK methods if offchain vs onchain is meant to differ.
Example consolidation:
-export const sendOffChain = async (wallet: IWallet, sats: number, address: string): Promise<string> => { - return wallet.sendBitcoin({ address, amount: sats }) -} +export const sendOffChain = async (wallet: IWallet, sats: number, address: string): Promise<string> => + wallet.sendBitcoin({ address, amount: sats }) export const sendOnChain = async (wallet: IWallet, sats: number, address: string): Promise<string> => { - return wallet.sendBitcoin({ address, amount: sats }) + return sendOffChain(wallet, sats, address) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/TransactionsList.tsx(2 hunks)src/lib/asp.ts(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:
src/lib/asp.ts
🧬 Code Graph Analysis (1)
src/components/TransactionsList.tsx (1)
src/components/Text.tsx (1)
Text(24-77)
⏰ 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: arkade-wallet
🔇 Additional comments (3)
src/components/TransactionsList.tsx (1)
5-5: Import cleanup aligns with removal of txid display. LGTM.All imported format helpers are used;
prettyLongTextremoval is consistent with the simplified Kind.src/lib/asp.ts (2)
91-95: Clarify same‐timestamp ordering intxs.sortThe comment says “put 'received' txs first,” but the comparator currently does the opposite (it returns –1 when
a.type === 'sent', placing sent transactions first). Please confirm the intended behavior and either:
- Update the comparator to match the comment:
- if (a.createdAt === b.createdAt) return a.type === 'sent' ? -1 : 1 + if (a.createdAt === b.createdAt) return a.type === 'received' ? -1 : 1- Or revise the comment to accurately describe the existing logic.
File: src/lib/asp.ts @ lines 91–95
25-25: BroadencollaborativeExitto accept both wallet types
Per our team learning, onlycollaborativeExitshould acceptServiceWorkerWalletin addition toIWallet.• Update import:
-import { ExtendedVirtualCoin, IWallet, ArkNote, RestArkProvider } from '@arkade-os/sdk' +import { ExtendedVirtualCoin, IWallet, ArkNote, RestArkProvider, ServiceWorkerWallet } from '@arkade-os/sdk'• Change signature:
-export const collaborativeExit = async (wallet: IWallet, amount: number, address: string): Promise<string> => { +export const collaborativeExit = async ( + wallet: IWallet | ServiceWorkerWallet, + amount: number, + address: string +): Promise<string> => {• Please verify that
ServiceWorkerWalletis exported under this name/path in@arkade-os/sdk.
| } | ||
|
|
||
| return wallet.settle({ inputs: selectedVtxos, outputs }, consoleLog) | ||
| return wallet.settle({ inputs: selectedVtxos, outputs }) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Settle signature change: ensure consistency across the file
You’ve removed the logger argument in collaborativeExit (Line 42), but settleVtxos (Line 137) still passes a second consoleLog argument. This is an inconsistency if the SDK signature changed globally; or intentional if both overloads are supported.
- If the SDK dropped the logger param entirely, update settleVtxos to match.
- If both are valid, consider keeping behavior consistent (either both log or both silent).
Run this to list usages and count args passed into settle across the codebase:
Option A: make both silent (if logger was removed):
- await wallet.settle(undefined, consoleLog)
+ await wallet.settle()Option B: keep logging consistent (if both overloads exist):
- return wallet.settle({ inputs: selectedVtxos, outputs })
+ return wallet.settle({ inputs: selectedVtxos, outputs }, consoleLog)Also applies to: 137-137
🏁 Script executed:
#!/bin/bash
# Show settle() call sites and following line for quick arg count inspection
rg -n --no-heading "settle\\(" -A 1
# Optional: check if we have any local type declarations shadowing IWallet
rg -n "interface\\s+IWallet|type\\s+IWallet"Length of output: 632
Remove deprecated logger argument from all wallet.settle calls
The SDK has dropped the logger parameter for wallet.settle. You’ve already removed it in collaborativeExit (Line 42), but it remains in settleVtxos (Line 137):
- src/lib/asp.ts:137
- await wallet.settle(undefined, consoleLog) + await wallet.settle()
Please remove the extra consoleLog argument to keep both calls consistent and avoid runtime errors.
📝 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.
| return wallet.settle({ inputs: selectedVtxos, outputs }) | |
| // In settleVtxos (around line 137), remove deprecated logger argument | |
| - await wallet.settle(undefined, consoleLog) | |
| + await wallet.settle() |
🤖 Prompt for AI Agents
In src/lib/asp.ts around lines 42 and 137, calls to wallet.settle must be
consistent: the SDK removed the logger argument so remove the extra consoleLog
argument in the wallet.settle call at line 137 (leave only the object with
inputs and outputs), matching the already-updated call at line 42; ensure no
other wallet.settle calls pass consoleLog and run type-checks/tests to confirm
no remaining references.
Signed-off-by: Andrew Camilleri <evilkukka@gmail.com>
Signed-off-by: Andrew Camilleri <evilkukka@gmail.com>
Signed-off-by: Marco Argentieri <3596602+tiero@users.noreply.github.com>
Signed-off-by: Marco Argentieri <3596602+tiero@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/components/AlertBox.tsx (1)
17-19: Fix color inconsistency: container sets white, Text forces black.The container sets color: white, but Text overrides to black. Remove the explicit color on Text to inherit the container color.
Apply this diff:
- <Text color='black' bold tiny wrap> + <Text bold tiny wrap>Also applies to: 26-26
🧹 Nitpick comments (3)
src/components/AlertBox.tsx (3)
6-9: DRY up AlertBox usage and make it more flexible (accept children, ReactNode icon).InfoBox duplicates the AlertBox styling. Generalize AlertBox to accept children and a ReactNode icon, then reuse it in InfoBox/WaitBox. This removes duplication and simplifies future content variations.
Apply these diffs:
-interface AlertProps { - icon: JSX.Element - text: string -} +interface AlertProps { + icon: React.ReactNode + children: React.ReactNode +} -function AlertBox({ icon, text }: AlertProps) { +function AlertBox({ icon, children }: AlertProps) { const style = { backgroundColor: 'var(--purple20)', border: '1px solid var(--dark20)', borderRadius: '0.5rem', padding: '0.5rem', color: 'white', width: '100%', } return ( <div style={{ padding: '2px', width: '100%' }}> <div style={style}> <FlexRow gap='0.5rem'> <AlertBoxIcon>{icon}</AlertBoxIcon> - <Text color='black' bold tiny wrap> - {text} - </Text> + {children} </FlexRow> </div> </div> ) } -export function InfoBox({ html }: { html: string }) { - const style = { - backgroundColor: 'var(--purple20)', - border: '1px solid var(--dark20)', - borderRadius: '0.5rem', - padding: '0.5rem', - color: 'white', - width: '100%', - } - return ( - <div style={{ padding: '2px', width: '100%' }}> - <div style={style}> - <FlexRow gap='0.5rem'> - <AlertBoxIcon> - <MegaphoneIcon animated /> - </AlertBoxIcon> - <div dangerouslySetInnerHTML={{ __html: html }} /> - </FlexRow> - </div> - </div> - ) -} +export function InfoBox({ html }: { html: string }) { + // (Sanitization as per previous comment) + DOMPurify.addHook('afterSanitizeAttributes', (node) => { + if (node.tagName === 'A' && node.getAttribute('target') === '_blank') { + node.setAttribute('rel', 'noopener noreferrer') + } + }) + const safeHtml = DOMPurify.sanitize(html, { + ALLOWED_TAGS: ['b', 'i', 'em', 'strong', 'a', 'p', 'ul', 'ol', 'li', 'br'], + ALLOWED_ATTR: ['href', 'target', 'rel'], + }) + return ( + <AlertBox icon={<MegaphoneIcon animated />}> + <div dangerouslySetInnerHTML={{ __html: safeHtml }} /> + </AlertBox> + ) +} export function WaitBox({ text }: { text: string }) { return ( <div style={{ width: '304px' }}> - <AlertBox icon={<LogoIconAnimated />} text={text} /> + <AlertBox icon={<LogoIconAnimated />}> + <Text bold tiny wrap>{text}</Text> + </AlertBox> </div> ) }Add this import to support the new types (if not already present):
import type { ReactNode } from 'react'Also applies to: 11-33, 44-65, 67-73
35-42: Hide decorative icon from screen readers.The icon is purely visual. Mark it aria-hidden to avoid noisy announcements.
Apply this diff:
- return <div style={style}>{children}</div> + return ( + <div style={style} aria-hidden='true'> + {children} + </div> + )
67-73: Make WaitBox width responsive.Use maxWidth with 100% width for better responsiveness on small screens.
Apply this diff:
- <div style={{ width: '304px' }}> + <div style={{ width: '100%', maxWidth: '304px' }}>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/AlertBox.tsx(1 hunks)src/screens/Wallet/Index.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/screens/Wallet/Index.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/components/AlertBox.tsx (4)
src/components/FlexRow.tsx (1)
FlexRow(16-45)src/components/Text.tsx (1)
Text(24-77)src/icons/Megaphone.tsx (1)
MegaphoneIcon(5-19)src/icons/Logo.tsx (1)
LogoIconAnimated(14-26)
🪛 Biome (2.1.2)
src/components/AlertBox.tsx
[error] 60-60: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
🪛 ast-grep (0.38.6)
src/components/AlertBox.tsx
[warning] 59-59: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
⏰ 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: arkade-wallet
| export function InfoBox({ html }: { html: string }) { | ||
| const style = { | ||
| backgroundColor: 'var(--purple20)', | ||
| border: '1px solid var(--dark20)', | ||
| borderRadius: '0.5rem', | ||
| padding: '0.5rem', | ||
| color: 'white', | ||
| width: '100%', | ||
| } | ||
| return ( | ||
| <div style={{ padding: '2px', width: '100%' }}> | ||
| <div style={style}> | ||
| <FlexRow gap='0.5rem'> | ||
| <AlertBoxIcon> | ||
| <MegaphoneIcon animated /> | ||
| </AlertBoxIcon> | ||
| <div dangerouslySetInnerHTML={{ __html: html }} /> | ||
| </FlexRow> | ||
| </div> | ||
| </div> | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Sanitize HTML before using dangerouslySetInnerHTML (XSS risk).
Raw HTML injection is unsafe. Both Biome and ast-grep flagged this (lint/security/noDangerouslySetInnerHtml and react-unsafe-html-injection). Sanitize HTML before injecting, and enforce noopener/noreferrer on outbound links.
Apply this diff to sanitize the HTML and use the sanitized version:
export function InfoBox({ html }: { html: string }) {
- const style = {
- backgroundColor: 'var(--purple20)',
- border: '1px solid var(--dark20)',
- borderRadius: '0.5rem',
- padding: '0.5rem',
- color: 'white',
- width: '100%',
- }
+ const style = {
+ backgroundColor: 'var(--purple20)',
+ border: '1px solid var(--dark20)',
+ borderRadius: '0.5rem',
+ padding: '0.5rem',
+ color: 'white',
+ width: '100%',
+ }
+ // Ensure external links are safe when opening in a new tab
+ DOMPurify.addHook('afterSanitizeAttributes', (node) => {
+ if (node.tagName === 'A' && node.getAttribute('target') === '_blank') {
+ node.setAttribute('rel', 'noopener noreferrer')
+ }
+ })
+ const safeHtml = DOMPurify.sanitize(html, {
+ ALLOWED_TAGS: ['b', 'i', 'em', 'strong', 'a', 'p', 'ul', 'ol', 'li', 'br'],
+ ALLOWED_ATTR: ['href', 'target', 'rel'],
+ })
return (
<div style={{ padding: '2px', width: '100%' }}>
<div style={style}>
<FlexRow gap='0.5rem'>
<AlertBoxIcon>
<MegaphoneIcon animated />
</AlertBoxIcon>
- <div dangerouslySetInnerHTML={{ __html: html }} />
+ <div dangerouslySetInnerHTML={{ __html: safeHtml }} />
</FlexRow>
</div>
</div>
)
}Add this import at the top of the file:
import DOMPurify from 'dompurify'I can wire this up and add a security unit test that asserts links get rel="noopener noreferrer" and that scripts/styles are stripped. Do you want me to push that?
🧰 Tools
🪛 Biome (2.1.2)
[error] 60-60: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
🪛 ast-grep (0.38.6)
[warning] 59-59: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
🤖 Prompt for AI Agents
In src/components/AlertBox.tsx around lines 44 to 65, the component uses
dangerouslySetInnerHTML with raw html which creates an XSS risk; import
DOMPurify at the top of the file, run DOMPurify.sanitize(html, {ADD_ATTR:
['target'], FORBID_TAGS: ['style','script']}) (or equivalent safe config) before
rendering, store the sanitized string and pass that to dangerouslySetInnerHTML,
and then post-process the sanitized HTML to enforce rel="noopener noreferrer"
(and target="_blank" if you allow opening in new tab) on outbound anchor tags so
external links are safe; ensure the import is added and tests/assertions cover
scripts/styles removal and rel attribute enforcement.
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
src/providers/limits.tsx (1)
80-83: Effect dependencies are incomplete; limits can go stale when aspInfo fields changeThe effect uses aspInfo.url, utxo/vtxo min/max and dust, but only depends on network and svcWallet. If any of those fields change, limits won’t refresh.
- }, [aspInfo.network, svcWallet]) + }, [ + aspInfo.network, + aspInfo.url, + aspInfo.utxoMinAmount, + aspInfo.utxoMaxAmount, + aspInfo.vtxoMinAmount, + aspInfo.vtxoMaxAmount, + aspInfo.dust, + svcWallet, + ])
🧹 Nitpick comments (1)
src/providers/limits.tsx (1)
144-147: Guard against falsy/undefined sats in amountIsAboveMaxLimitIf sats is undefined/null, BigInt(sats) will throw. Other helpers guard for falsy sats; mirror that here.
- const amountIsAboveMaxLimit = (sats: Satoshis): boolean => { - const maxAllowed = getMaxSatsAllowed() - return maxAllowed === BigInt(-1) ? false : BigInt(sats) > maxAllowed - } + const amountIsAboveMaxLimit = (sats: Satoshis): boolean => { + if (!sats) return false + const maxAllowed = getMaxSatsAllowed() + return maxAllowed === BigInt(-1) ? false : BigInt(sats) > maxAllowed + }
📜 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 settings in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/providers/limits.tsx(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/providers/limits.tsx (3)
src/lib/lightning.ts (1)
LightningSwap(13-66)src/lib/logs.ts (1)
consoleError(40-44)src/lib/constants.ts (1)
isRiga(7-7)
🔇 Additional comments (1)
src/providers/limits.tsx (1)
123-137: Comment vs implementation mismatch for getMinSatsAllowedThe comment states “it should be the maximum between utxo and vtxo min amounts,” but the implementation returns the minimum of both (consistent with the matrix). Please reconcile intent vs implementation.
- If the goal is “global minimum amount allowed by at least one mechanism (UTXO or VTXO)”, the current code is correct, and the comment should say “minimum.”
- If the goal is “minimum amount guaranteed regardless of mechanism”, then the function should return the maximum.
Two options:
Option A (keep logic, fix comment)
- // it should be the maximum between utxo and vtxo min amounts, + // it should be the minimum between utxo and vtxo min amounts,Option B (change logic to maximum)
- const getMinSatsAllowed = (): bigint => { - const { utxo, vtxo } = limits.current - return utxo.min < vtxo.min ? utxo.min : vtxo.min - } + const getMinSatsAllowed = (): bigint => { + const { utxo, vtxo } = limits.current + // treat -1 (no limit) as lowest, so max() must respect sentinel: + if (utxo.min < BigInt(0)) return vtxo.min + if (vtxo.min < BigInt(0)) return utxo.min + return utxo.min > vtxo.min ? utxo.min : vtxo.min + }
| import { consoleError } from '../lib/logs' | ||
| import { LightningSwap } from '../lib/lightning' | ||
| import { WalletContext } from './wallet' | ||
| import { isRiga } from '../lib/constants' |
There was a problem hiding this comment.
Environment boolean parsing bug for isRiga will mis-gate UTXO flows
import.meta.env values are strings at runtime. Casting to boolean in constants.ts does not coerce "false"/"true" correctly. If VITE_IS_RIGA="false", isRiga will be the string "false" (truthy), and !isRiga will be false, unintentionally disabling UTXO everywhere.
Proposed fix in src/lib/constants.ts (outside this file):
// src/lib/constants.ts
export const isRiga: boolean = ['1', 'true', 'yes', 'on'].includes(
String(import.meta.env.VITE_IS_RIGA ?? '').trim().toLowerCase()
)🤖 Prompt for AI Agents
In src/providers/limits.tsx around line 7, the imported isRiga from
src/lib/constants.ts can be a truthy string (e.g. "false") because
import.meta.env values are strings; update src/lib/constants.ts to parse the env
var into a proper boolean by normalizing the string (trim, toLowerCase) and
checking it against an explicit list of truthy values (e.g.
'1','true','yes','on'), export isRiga as a boolean, and ensure any code using
!isRiga operates on the boolean result so UTXO flows are gated correctly.
| swap: { min: BigInt(0), max: BigInt(-1) }, | ||
| utxo: { min: BigInt(0), max: BigInt(-1) }, | ||
| vtxo: { min: BigInt(0), max: BigInt(-1) }, | ||
| }) |
There was a problem hiding this comment.
Fail-open default for LN swap limits (max = -1) can expose LN features when not available
Setting the default swap.max to -1 (unlimited) enables lnSwapsAllowed() before limits are fetched (and even if fetching fails), potentially surfacing LN flows on unsupported envs.
Prefer fail-closed defaults so features are gated until we positively know they’re allowed:
- swap: { min: BigInt(0), max: BigInt(-1) },
+ swap: { min: BigInt(0), max: BigInt(0) },📝 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.
| swap: { min: BigInt(0), max: BigInt(-1) }, | |
| utxo: { min: BigInt(0), max: BigInt(-1) }, | |
| vtxo: { min: BigInt(0), max: BigInt(-1) }, | |
| }) | |
| swap: { min: BigInt(0), max: BigInt(0) }, | |
| utxo: { min: BigInt(0), max: BigInt(-1) }, | |
| vtxo: { min: BigInt(0), max: BigInt(-1) }, | |
| }) |
🤖 Prompt for AI Agents
In src/providers/limits.tsx around lines 47 to 50, the default max values are
set to BigInt(-1) which permits LN swap flows before real limits are fetched;
change the fail-open -1 defaults to a fail-closed value (e.g. BigInt(0) or null)
for swap, utxo and vtxo so features stay gated until positive limits are
retrieved, and verify any lnSwapsAllowed() checks treat the new default as “not
allowed” until updated from the remote config.
| if (aspInfo.network && aspInfo.network !== 'signet') { | ||
| // TODO: quick fix | ||
| const swapProvider = new LightningSwap(aspInfo, svcWallet) | ||
| swapProvider | ||
| .getLimits() | ||
| .then(({ min, max }) => { | ||
| if (cancelled) return | ||
| limits.current.swap = { ...limits.current.swap, min: BigInt(min), max: BigInt(max) } | ||
| }) | ||
| .catch(consoleError) | ||
| } |
There was a problem hiding this comment.
Lightning limits fetch should guard for aspInfo.url, disable LN on signet/unknown, and improve error logging
- Currently gated only by network !== 'signet', but not by aspInfo.url presence.
- If the condition fails, we leave prior swap limits as-is; better to explicitly disable LN (fail-closed).
- Add context to error logs.
- if (aspInfo.network && aspInfo.network !== 'signet') {
- // TODO: quick fix
- const swapProvider = new LightningSwap(aspInfo, svcWallet)
- swapProvider
- .getLimits()
- .then(({ min, max }) => {
- if (cancelled) return
- limits.current.swap = { ...limits.current.swap, min: BigInt(min), max: BigInt(max) }
- })
- .catch(consoleError)
- }
+ if (aspInfo.network && aspInfo.network !== 'signet' && aspInfo.url) {
+ // TODO: quick fix
+ const swapProvider = new LightningSwap(aspInfo, svcWallet)
+ swapProvider
+ .getLimits()
+ .then(({ min, max }) => {
+ if (cancelled) return
+ limits.current.swap = { ...limits.current.swap, min: BigInt(min), max: BigInt(max) }
+ })
+ .catch((err) => consoleError(err, 'Failed to fetch Lightning swap limits'))
+ } else {
+ // Fail-closed: ensure LN is disabled when not supported/unknown.
+ limits.current.swap = { ...limits.current.swap, min: BigInt(0), max: BigInt(0) }
+ }📝 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.
| if (aspInfo.network && aspInfo.network !== 'signet') { | |
| // TODO: quick fix | |
| const swapProvider = new LightningSwap(aspInfo, svcWallet) | |
| swapProvider | |
| .getLimits() | |
| .then(({ min, max }) => { | |
| if (cancelled) return | |
| limits.current.swap = { ...limits.current.swap, min: BigInt(min), max: BigInt(max) } | |
| }) | |
| .catch(consoleError) | |
| } | |
| if (aspInfo.network && aspInfo.network !== 'signet' && aspInfo.url) { | |
| // TODO: quick fix | |
| const swapProvider = new LightningSwap(aspInfo, svcWallet) | |
| swapProvider | |
| .getLimits() | |
| .then(({ min, max }) => { | |
| if (cancelled) return | |
| limits.current.swap = { ...limits.current.swap, min: BigInt(min), max: BigInt(max) } | |
| }) | |
| .catch((err) => consoleError(err, 'Failed to fetch Lightning swap limits')) | |
| } else { | |
| // Fail-closed: ensure LN is disabled when not supported/unknown. | |
| limits.current.swap = { ...limits.current.swap, min: BigInt(0), max: BigInt(0) } | |
| } |
🤖 Prompt for AI Agents
In src/providers/limits.tsx around lines 66 to 76, guard the Lightning limits
fetch by checking that aspInfo.url exists and that aspInfo.network is a
supported mainnet (not 'signet' or missing); if the guard fails, set
limits.current.swap to a disabled/fail-closed value (e.g. null or an object
indicating LN is disabled) so prior limits are not reused. When calling
LightningSwap.getLimits(), keep the cancelled check before mutating state and
improve error handling by catching the error with a named param and logging
contextual information (include aspInfo.network and aspInfo.url) and then
disable swap limits on error. Ensure the code path always either sets valid
BigInt limits on success or explicitly disables swap on guard-fail or on error.
| const lnSwapsAllowed = () => limits.current.swap.max !== BigInt(0) | ||
| const utxoTxsAllowed = () => limits.current.utxo.max !== BigInt(0) | ||
| const utxoTxsAllowed = () => limits.current.utxo.max !== BigInt(0) && !isRiga | ||
| const vtxoTxsAllowed = () => limits.current.vtxo.max !== BigInt(0) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Gate LN swaps by network to reflect provider availability (signet should be disabled)
At the moment lnSwapsAllowed only checks swap.max. If defaults are fail-open or fetch fails, LN may appear allowed on signet.
- const lnSwapsAllowed = () => limits.current.swap.max !== BigInt(0)
+ const lnSwapsAllowed = () =>
+ aspInfo.network !== 'signet' && limits.current.swap.max !== BigInt(0)📝 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 lnSwapsAllowed = () => limits.current.swap.max !== BigInt(0) | |
| const utxoTxsAllowed = () => limits.current.utxo.max !== BigInt(0) | |
| const utxoTxsAllowed = () => limits.current.utxo.max !== BigInt(0) && !isRiga | |
| const vtxoTxsAllowed = () => limits.current.vtxo.max !== BigInt(0) | |
| const lnSwapsAllowed = () => | |
| aspInfo.network !== 'signet' && limits.current.swap.max !== BigInt(0) | |
| const utxoTxsAllowed = () => limits.current.utxo.max !== BigInt(0) && !isRiga | |
| const vtxoTxsAllowed = () => limits.current.vtxo.max !== BigInt(0) |
🤖 Prompt for AI Agents
In src/providers/limits.tsx around lines 158 to 161, lnSwapsAllowed currently
only checks swap.max which can permit LN swaps on signet if defaults/fetch fail;
update lnSwapsAllowed to also gate by network (disable for signet) by adding the
existing network flag (e.g., !isRiga or a dedicated isSignet) to the condition
so it returns true only when swap.max !== BigInt(0) AND the network is not
signet.
Let's have a deployed wallet to make tests
Summary by CodeRabbit
New Features
Bug Fixes
Other