do not merge: signet default #105
Conversation
* update to ts-sdk `0.2.1` (#121) * update to ts-sdk next-version * remove console.log * hotfix * don't show notImplemented on logs * lint * @arkade-os/sdk v0.2.0 from npm registry * fix constants.ts * disable service worker cache * build worker * update test server to signet * add pages.dev to test server domains * fix locked wallet after creation * add @noble/secp256k1 * fix css * update faucet * use en var VITE_ARK_SERVER to define default ark server * revert testServer back to mutinynet * fix notes * save asp url to config with protocol * lint * lint * remove "back to wallet" button after button redeem * remove "back to wallet" button, anti pattern * send all by clicking the balance * build worker * use input instead of textarea to get nsec * update ts-sdk to 0.2.1 --------- Co-authored-by: João Bordalo <bordalix@users.noreply.github.com> * fix: lnurl support * support OCP * Show valid urls in clipboard viewer Lock input amount if min == max Refactor dealing with lnurl limits --------- Co-authored-by: louisinger <41042567+louisinger@users.noreply.github.com> Co-authored-by: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com> Co-authored-by: lapatric <42653152+lapatric@users.noreply.github.com>
Signed-off-by: Marco Argentieri <3596602+tiero@users.noreply.github.com>
Co-authored-by: João Bordalo <bordalix@users.noreply.github.com>
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (1)
⛔ Files ignored due to path filters (1)
You can disable this status message by setting the WalkthroughLarge refactor adding Boltz/Lightning integration, new Boltz app screens, Nudge provider and UI, many component/API surface changes (Error→ErrorMessage, scanner API, BIP21/bolt11/address parsing), removal of Dexie DB, service-worker wallet adjustments, tooling/Docker/docs updates, and many tests/fixtures. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant App
participant LightningProvider
participant BoltzAPI as Boltz API
participant SW as ServiceWorkerWallet
Note over App,LightningProvider: App mounts and providers initialize
App->>LightningProvider: mount(network, wallet, config)
LightningProvider->>BoltzAPI: fetch fees & swap history
BoltzAPI-->>LightningProvider: fees, history
LightningProvider-->>App: context { connected, swapProvider, fees }
Note over User,App: Submarine swap (send invoice)
User->>App: submit invoice
App->>LightningProvider: swapProvider.createSubmarineSwap(invoice)
LightningProvider->>BoltzAPI: create submarine swap
BoltzAPI-->>LightningProvider: PendingSubmarineSwap
App->>LightningProvider: swapProvider.payInvoice(pendingSwap)
LightningProvider->>BoltzAPI: off-chain pay / wait settlement
BoltzAPI-->>LightningProvider: settlement result (txid, preimage)
LightningProvider-->>App: success -> UI
sequenceDiagram
autonumber
actor User
participant ReceiveScreen as App
participant Limits
participant Lightning
participant SW as ServiceWorkerWallet
participant Notif
ReceiveScreen->>Limits: utxoTxsAllowed / vtxoTxsAllowed
ReceiveScreen->>Lightning: swapProvider (create reverse swap if requested)
ReceiveScreen-->>User: show QR (encodeBip21)
SW-->>ReceiveScreen: VTXO_UPDATE / UTXO_UPDATE (payment detected)
ReceiveScreen->>Notif: sendNotification(...)
ReceiveScreen-->>User: navigate Success
sequenceDiagram
autonumber
participant WalletProv
participant NudgeProv
participant Options
participant Nav
actor User
WalletProv-->>NudgeProv: wallet, balance
NudgeProv->>WalletProv: noUserDefinedPassword()
alt balance >= minSatsToNudge and no password
NudgeProv-->>User: render CreatePasswordWarning
User->>NudgeProv: click Create password
NudgeProv->>Options: setOption(Password)
NudgeProv->>Nav: navigate(Settings)
else
NudgeProv-->>User: nothing
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying wallet-signet with
|
| Latest commit: |
23e5eed
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a9148ff4.wallet-23u.pages.dev |
| Branch Preview URL: | https://signet.wallet-23u.pages.dev |
Deploying wallet-mutinynet with
|
| Latest commit: |
23e5eed
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://9a62c760.arkade-wallet.pages.dev |
| Branch Preview URL: | https://signet.arkade-wallet.pages.dev |
Deploying wallet-bitcoin-v8 with
|
| Latest commit: |
23e5eed
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://2160144f.wallet-v8.pages.dev |
| Branch Preview URL: | https://signet.wallet-v8.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 25
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (12)
src/components/Button.tsx (1)
42-46: Disable the button during loading state.The button remains clickable while
loadingis true, which can lead to duplicate submissions or unintended repeated actions. Users may click multiple times during async operations, causing race conditions or repeated API calls.Apply this diff to disable the button when loading:
<IonButton className={red ? 'red' : secondary ? 'secondary' : clear ? 'clear' : 'dark'} - disabled={disabled} + disabled={disabled || loading} expand={short ? undefined : 'block'} fill={clear ? 'clear' : 'solid'} onClick={onClick} size={small ? 'small' : 'default'} >src/lib/privateKey.ts (2)
6-10: Empty private key not flagged as invalidinvalidPrivateKey returns '' for length 0, so empty keys pass validation.
export const invalidPrivateKey = (key: Uint8Array): string => { - if (key.length === 0) return '' + if (key.length === 0) return 'Private key cannot be empty' if (key.length !== 32) return 'Invalid length: private key must be 32 bytes' return '' }
116-123: Type bug: invalid cast in encryptPrivateKeyPassing privateKey as Uint8Array is correct; the current cast is invalid TS and unnecessary.
const encrypted = await crypto.subtle.encrypt( { name: 'AES-GCM', iv, }, key, - privateKey as Uint8Array<ArrayBuffer>, + privateKey, )src/screens/Wallet/Receive/QrCode.tsx (1)
60-103: Fix stalerecvInfowritesInside both the reverse-swap promise chain (Lines 60-68) and the service-worker listener (Lines 99-103) we call
setRecvInfo({ ...recvInfo, ... }). Because the surrounding effects depend only onsatoshis/svcWallet, those callbacks capture an outdated snapshot ofrecvInfo. As soon as a payment message arrives we overwrite the context with that stale snapshot, dropping newer fields like the freshly created invoice or updated addresses. This regresses the receive flow (e.g. the invoice disappears right after we emit it). Switch these setters to the functional form so they always merge onto the latest state. Example:- setRecvInfo({ ...recvInfo, invoice }) + setRecvInfo((prev) => ({ ...prev, invoice }))Do the same for the other
setRecvInfocalls in this component, including thesatoshisupdates inside the swapthenand the service-worker handler.src/screens/Wallet/Receive/Amount.tsx (1)
99-112: Add guard for missing offchain address before faucet callAvoid calling faucet if addresses failed to fetch.
const handleFaucet = async () => { try { if (!amount) throw 'Invalid amount' + if (!recvInfo?.offchainAddr) throw 'Address unavailable' setFauceting(true) const ok = await callFaucet(recvInfo.offchainAddr, satoshis, aspInfo)src/providers/limits.tsx (2)
53-66: UTXO/VTXO limits incorrectly gated by swapProviderThis effect sets on-chain limits but returns early when swapProvider is null. That prevents limits from initializing unless LN is connected.
- useEffect(() => { - if (!aspInfo.network || !svcWallet || !swapProvider) return + useEffect(() => { + if (!aspInfo.network || !svcWallet) returnAlso, the effect should re-run if the ASP limit fields change. Consider expanding deps:
- }, [aspInfo.network, svcWallet]) + }, [aspInfo.network, aspInfo.utxoMinAmount, aspInfo.utxoMaxAmount, aspInfo.vtxoMinAmount, aspInfo.vtxoMaxAmount, aspInfo.dust, svcWallet])
143-146: Minimum limit logic contradicts comment; use max of mins with -1 semanticsComment says “absolute min” should be the maximum of UTXO/VTXO mins, treating -1 as “no limit”. Current code returns the smaller value.
- 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 + if (utxo.min === BigInt(-1)) return vtxo.min + if (vtxo.min === BigInt(-1)) return utxo.min + return utxo.min > vtxo.min ? utxo.min : vtxo.min + }amountIsBelowMinLimit will then behave as intended with the existing -1 check.
Also applies to: 158-165
src/screens/Wallet/Send/Details.tsx (2)
99-116: Fix loading lock: set sending=false on early-return error paths.Currently returns with sending still true, leaving the UI stuck in Loading.
} else if (invoice) { const response = pendingSwap?.response - if (!response) return setError('Swap response not available') + if (!response) { + setError('Swap response not available') + setSending(false) + return + } const swapAddress = pendingSwap?.response.address - if (!swapAddress) return setError('Swap address not available') + if (!swapAddress) { + setError('Swap address not available') + setSending(false) + return + } const promise = swapProvider?.payInvoice(pendingSwap) if (!promise) { setError('Lightning swaps not enabled') setSending(false) return }
99-119: Respect limits when sending; gate by allowed routes.Proceeding without checking vtxo/utxo/LN allowances can trigger avoidable failures.
const handleContinue = async () => { if (!satoshis || !svcWallet) return setSending(true) - if (arkAddress) { + if (arkAddress) { + if (!vtxoTxsAllowed()) { + setError('Paying inside the Ark is currently disabled') + setSending(false) + return + } sendOffChain(svcWallet, satoshis, arkAddress).then(handleTxid).catch(handleError) } else if (invoice) { + if (!lnSwapsAllowed()) { + setError('Lightning swaps are currently disabled') + setSending(false) + return + } const response = pendingSwap?.response ... } else if (address) { + if (!utxoTxsAllowed()) { + setError('Mainnet transactions are currently disabled') + setSending(false) + return + } collaborativeExit(svcWallet, satoshis, address).then(handleTxid).catch(handleError) } }src/screens/Wallet/Send/Form.tsx (3)
70-71: Fix rules-of-hooks violation: remove early return before hooksReturning early before declaring hooks breaks hook ordering. Replace the early return with a loading flag and render conditionally after hooks.
Apply these changes:
- if (!svcWallet) return <Loading text='Loading...' /> + const isLoading = !svcWallet
73-79: Guard wallet usage in effect and re-run when wallet becomes availablePrevent calling getReceivingAddresses with undefined wallet and make the effect react to wallet availability.
-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])
326-333: Render Loading after hooks instead of early returnShow the loading UI here using the flag defined earlier to keep hook order intact.
- if (scan) + if (scan) return ( <Scanner close={() => setScan(false)} label='Recipient address' onData={setRecipient} onError={smartSetError} /> ) - if (keys) return <Keyboard back={() => setKeys(false)} onChange={setAmount} value={amount} /> + if (isLoading) return <Loading text='Loading...' /> + if (keys) return <Keyboard back={() => setKeys(false)} onChange={setAmount} value={amount} />
🧹 Nitpick comments (44)
src/icons/Settings.tsx (1)
17-26: Prefer themeable strokes over hard-coded whiteHard-coding
stroke="#FBFBFB"locks the icon to pure white and ignores any theme color overrides. Switching both paths tostroke="currentColor"keeps the light variant reusable across themes while still letting the caller supply white when needed.- stroke='#FBFBFB' + stroke='currentColor' ... - stroke='#FBFBFB' + stroke='currentColor'src/lib/biometrics.ts (1)
36-36: Remove unnecessaryas BufferSourcecasts
Uint8Array already implements ArrayBufferView (part of BufferSource); drop the explicit casts onchallenge,password, etc. (e.g. lines 36, 53, 79, 83).src/lib/storage.ts (2)
4-8: Remove unnecessaryasynckeyword.Since
clearStorageno longer contains any asynchronous operations (the vtxosRepository calls were removed), theasynckeyword andPromise<void>return type are unnecessary.Apply this diff:
-export async function clearStorage(): Promise<void> { +export function clearStorage(): void { const config = readConfigFromStorage() localStorage.clear() if (config) saveConfigToStorage(config) }
10-17: Consider logging errors for easier debugging.While the non-capturing catch clause is valid ES2019 syntax, silently swallowing errors (e.g., JSON parsing failures, localStorage issues) can make debugging harder. Consider logging the error for better developer experience.
Apply this diff if you want to add logging:
- } catch { + } catch (error) { + console.warn(`Failed to read storage item "${key}":`, error) return fallback }src/test/components/Button.test.tsx (1)
6-18: Add test coverage for the newloadingprop.The Button component now accepts a
loadingprop (added in this PR), but the tests don't exercise this new behavior. Consider adding tests to verify:
- The spinner is displayed when
loading={true}- The label is hidden when
loading={true}- The button is disabled during loading (assuming the earlier review comment is addressed)
Add a test case like this:
it('displays spinner and hides label when loading', () => { render(<Button label='Click Me' onClick={() => {}} loading={true} />) expect(screen.queryByText('Click Me')).not.toBeInTheDocument() expect(document.querySelector('.spinner')).toBeInTheDocument() })src/test/components/Details.test.tsx (1)
19-22: Expand test assertions to validate more fields.The test only checks for 'outgoing' text but provides many other props. Consider verifying more rendered fields to improve coverage, especially formatted amounts, icons, and conditional rendering logic.
Add assertions for other key fields:
it('renders the details with the correct information', () => { + const mockConfigValue = { + config: { showBalance: true, fiat: 'USD' }, + useFiat: false, + } + const mockFiatValue = { + toFiat: (amount: number) => amount * 0.0001, + } render( + <ConfigContext.Provider value={mockConfigValue}> + <FiatContext.Provider value={mockFiatValue}> - <Details details={props} /> + <Details details={props} /> + </FiatContext.Provider> + </ConfigContext.Provider> ) expect(screen.getByText('outgoing')).toBeInTheDocument() + expect(screen.getByText('123 Main St')).toBeInTheDocument() + expect(screen.getByText('Test Note')).toBeInTheDocument() + expect(screen.getByText('INV-123')).toBeInTheDocument() + expect(screen.getByText('payment')).toBeInTheDocument() })src/test/components/Checkbox.test.tsx (1)
7-10: Consider usinggetByRolefor more semantic queries.While
getByTextworks, querying by role is more aligned with accessibility and testing-library best practices. This would also verify that the checkbox is properly exposed as a checkbox role.Example refactor:
it('renders the checkbox with the correct text', () => { render(<Checkbox text='Click Me' onChange={() => {}} />) - expect(screen.getByText('Click Me')).toBeInTheDocument() + const checkbox = screen.getByRole('checkbox', { name: /Click Me/i }) + expect(checkbox).toBeInTheDocument() })src/lib/asp.ts (1)
32-55: Logic is correct; consider enhancing error messages.The collaborative exit implementation correctly:
- Selects vtxos using greedy selection
- Validates sufficient funds before attempting settlement
- Calculates and handles change amounts
- Uses wallet-based vtxo retrieval and settlement
Consider enhancing the error message at line 43 to include more context:
- if (selectedAmount < amount) throw new Error('Insufficient funds') + if (selectedAmount < amount) { + throw new Error(`Insufficient funds: need ${amount} sats, but only ${selectedAmount} sats available`) + }Based on learnings: This function correctly accepts IWallet type, which can include ServiceWorkerWallet per the learning note.
src/components/Header.tsx (1)
38-38: Consider improving accessibility for clickable elements.The clickable divs for the back button and auxiliary action lack proper accessibility attributes. Consider adding:
role="button"for semantic meaningtabIndex={0}for keyboard navigationonKeyDownhandlers for Enter/Space key supportaria-labelfor screen reader contextExample for the auxiliary action:
<div style={style} onClick={auxFunc || undefined} role="button" tabIndex={auxFunc ? 0 : -1} onKeyDown={(e) => { if (auxFunc && (e.key === 'Enter' || e.key === ' ')) auxFunc(); }} aria-label={auxText || 'Auxiliary action'} > {auxText ? SideButton(auxText, auxFunc || (() => {})) : auxIcon ? auxIcon : <p> </p>} </div>Also applies to: 46-48
src/screens/Wallet/Onboard.tsx (1)
83-111: Verify PWA support and consider improving installation guidance.Step 4 provides manual PWA installation instructions, but there are potential concerns:
- Inconsistency with automated install removal: Users might expect an automated install prompt but only see manual instructions, creating a suboptimal UX.
- Platform-specific instructions: The current instructions appear iOS-specific (Share icon, "Add to Home Screen"). Android users follow different steps (menu → "Install app" or "Add to Home screen").
- Functionality verification: Ensure the app actually supports PWA installation (manifest.json, service worker, display mode, etc.) before guiding users through manual installation.
Consider:
- Removing this step entirely if PWA support is deprecated.
- Making instructions platform-agnostic with conditional rendering based on user agent.
- Adding a fallback message if PWA installation isn't supported on the user's device/browser.
Verify PWA support with the script provided in the previous comment. If PWA is fully supported, consider this refactor for platform-agnostic instructions:
if (step === 4) { + const isIOS = /iPad|iPhone|iPod/.test(navigator.userAgent); + const isAndroid = /Android/.test(navigator.userAgent); + return ( <FlexCol gap='0.5rem'> <Title text='Install Arkade on Home' /> - <Text wrap>Adding Arkade to Home enable push notifications and better user experience.</Text> + <Text wrap>Adding Arkade to Home enables push notifications and a better user experience.</Text> <Shadow purple> - <FlexCol gap='1rem'> - <FlexRow> - <Bullet number={1} /> - <Text>Tap</Text> - <Shadow flex inverted slim> - <ShareIcon /> - </Shadow> - </FlexRow> - <FlexRow> - <Bullet number={2} /> - <Text>Then</Text> - <Shadow flex inverted slim> - <FlexRow> - <Text>Add to Home Screen</Text> - <AddIcon /> - </FlexRow> - </Shadow> - </FlexRow> - </FlexCol> + {isIOS && ( + <FlexCol gap='1rem'> + <FlexRow> + <Bullet number={1} /> + <Text>Tap</Text> + <Shadow flex inverted slim> + <ShareIcon /> + </Shadow> + </FlexRow> + <FlexRow> + <Bullet number={2} /> + <Text>Then</Text> + <Shadow flex inverted slim> + <FlexRow> + <Text>Add to Home Screen</Text> + <AddIcon /> + </FlexRow> + </Shadow> + </FlexRow> + </FlexCol> + )} + {isAndroid && ( + <FlexCol gap='1rem'> + <FlexRow> + <Bullet number={1} /> + <Text>Open the menu (three dots)</Text> + </FlexRow> + <FlexRow> + <Bullet number={2} /> + <Text>Tap "Install app" or "Add to Home screen"</Text> + </FlexRow> + </FlexCol> + )} + {!isIOS && !isAndroid && ( + <Text>Use your browser's menu to install this app on your device.</Text> + )} </Shadow> </FlexCol> ) }Also note the typo fix: "enable" → "enables".
src/components/Text.tsx (1)
43-43: Consider refactoring the fontSize calculation for clarity.The nested ternary chain now handles 7 size variants, which reduces readability and maintainability.
Apply this diff to use a more readable approach:
- const fontSize = tiny ? 12 : smaller ? 13 : small ? 14 : big ? 24 : bigger ? 32 : large ? 18 : 16 + const getFontSize = () => { + if (tiny) return 12 + if (smaller) return 13 + if (small) return 14 + if (big) return 24 + if (bigger) return 32 + if (large) return 18 + return 16 + } + const fontSize = getFontSize()Alternatively, use a mapping object if multiple size props shouldn't be set simultaneously.
src/lib/explorers.ts (2)
1-1: Import path: drop trailing slash to avoid resolution issuesUse the package entrypoint without trailing slash.
-import { NetworkName } from '@arkade-os/sdk/' +import { NetworkName } from '@arkade-os/sdk'Please confirm NetworkName includes 'mutinynet' and 'signet' in the current SDK build.
51-53: Harden window.openAdd noopener to prevent opener leaks.
-window.open(getTxIdURL(txid, wallet), '_blank', 'noreferrer') +window.open(getTxIdURL(txid, wallet), '_blank', 'noopener,noreferrer')src/test/fixtures.json (1)
189-198: Private key material in repo (even as fixtures)Ensure these are non-sensitive test keys and cannot fund anything. Consider masking or moving to a generated-at-test-time fixture to avoid accidental reuse.
I can provide a .gitleaks.toml allowlist snippet if you want.
src/components/NewPassword.tsx (1)
18-23: Effect dependencies and UX copy
- If onNewPassword/setLabel aren’t stable (useCallback), add them to deps or memoize upstream to avoid stale closures.
- Consider less casual copy than “No password, YOLO” for clarity.
src/lib/privateKey.ts (1)
12-22: npub length check is brittlenpub length can vary; relying on nip19.decode is sufficient. Consider removing the fixed-length check to avoid false negatives.
- if (npub.length !== 63) return 'Invalid length: npub must be 63 characters'src/lib/format.ts (1)
50-53: Avoid fractional seconds in prettyDeltaFloor secs to keep output consistent with other units.
- if (delta > 0) { - const secs = delta + if (delta > 0) { + const secs = Math.floor(delta) return `${secs}${long ? (secs === 1 ? ' second' : ' seconds') : 's'}` }src/test/components/AlertBox.test.tsx (1)
12-20: LGTM: Sanitization test covers critical XSS vectors.The test correctly validates that
DOMPurify.sanitizeremoves script elements and dangerous event handler attributes, preventing XSS attacks.Consider adding additional test cases to strengthen sanitization coverage:
it('sanitizes dangerous attributes and elements', () => { const { container } = render( <InfoBox html={` <a href="javascript:alert(1)">link</a> <iframe src="evil.com"></iframe> <form action="evil.com"><input name="x"></form> <div onclick="alert(1)">click</div> `} /> ) // javascript: protocol should be removed or sanitized const link = container.querySelector('a') expect(link?.getAttribute('href')).not.toMatch(/javascript:/) // iframes should be removed (depending on DOMPurify config) expect(container.querySelector('iframe')).toBeNull() // onclick and other event handlers should be removed expect(container.querySelector('[onclick]')).toBeNull() }) it('handles edge cases gracefully', () => { const { container } = render(<InfoBox html='' />) expect(container.textContent).toBe('') })These tests would provide additional confidence in the sanitization implementation, particularly against less common XSS vectors.
src/screens/Apps/Boltz/Index.tsx (1)
38-38: Use Text component instead of raw HTML heading for consistency.The raw
<h1>element breaks from the project's pattern of using theTextcomponent for typography. This may result in inconsistent styling that doesn't follow the design system (colors, fonts, spacing).Apply this diff to use the Text component:
- <h1>Boltz</h1> + <Text large bold>Boltz</Text>Alternatively, if a larger heading is needed:
- <h1>Boltz</h1> + <Text big bold>Boltz</Text>src/components/TransactionsList.tsx (1)
60-60: Rename component to avoid shadowing global Date.The component name
Dateshadows the global JavaScriptDateconstructor, which can cause confusion and potential bugs if developers attempt to usenew Date()within this scope.Apply this diff to rename the component:
- const Date = () => <TextSecondary>{date}</TextSecondary> + const DateDisplay = () => <TextSecondary>{date}</TextSecondary>And update the usage:
<div> <Kind /> - <Date /> + <DateDisplay /> </div>As per coding guidelines
src/lib/bolt11.ts (2)
10-17: Add documentation for extractNote's expected format.The
extractNotefunction handles a specific JSON array structure[[type, value]]but lacks documentation explaining:
- What format is expected (e.g., LNURL metadata, LUD-06, etc.)
- Why this transformation is necessary
- What happens with non-conforming data
Add JSDoc documentation:
+/** + * Extracts note from invoice description field. + * Handles both plain strings and LNURL/LUD-06 metadata format: [[type, value]] + * @param data - Description string from decoded invoice + * @returns Extracted note or empty string on error + */ const extractNote = (data: string): string => { if (!/^\[/.test(data)) return data try { return JSON.parse(data)[0][1] } catch { return '' } }
31-38: Consider logging validation errors for debugging.The
isValidInvoicefunction silently swallows all errors, which may make debugging difficult when invoices are rejected. While silent validation is appropriate for user-facing flows, consider adding optional debug logging.Add optional error logging:
export const isValidInvoice = (data: string): boolean => { try { decodeInvoice(data) return true } catch { + // Silent validation for user flows + // Uncomment for debugging: console.debug('Invalid invoice:', data) return false } }src/screens/Settings/Server.tsx (1)
30-35: Consider using native URL constructor for more robust validation.The custom regex-based URL validation may miss edge cases. JavaScript's native
URLconstructor provides more robust validation and handles various URL formats correctly.Replace the custom regex with native URL validation:
const isValidUrl = (url: string) => { - if (url.startsWith('localhost') || url.startsWith('http://localhost')) return true - if (url.startsWith('127.0.0.1') || url.startsWith('http://127.0.0.1')) return true - const urlPattern = /^(https?:\/\/)?([\w-]+(\.[\w-]+)+)([\w.,@?^=%&:/~+#-]*[\w@?^=%&/~+#-])?$/ - return urlPattern.test(url) + // Allow localhost and 127.0.0.1 for development + if (url.startsWith('localhost') || url.startsWith('127.0.0.1')) return true + + try { + const parsed = new URL(url.startsWith('http') ? url : `https://${url}`) + return parsed.protocol === 'http:' || parsed.protocol === 'https:' + } catch { + return false + } }This approach:
- Leverages browser's native URL parsing
- Handles edge cases automatically (IPv6, punycode, etc.)
- Ensures protocol validation
- Falls back gracefully on invalid URLs
src/test/lib/address.test.ts (1)
13-18: Align describe title with function name (optional)Use "decodeArkAddress" for clarity and grep-ability.
src/screens/Settings/Lock.tsx (1)
27-30: Handle promise rejection in password-state effectAdd a catch to avoid unhandled rejections and log context.
Apply:
useEffect(() => { - noUserDefinedPassword().then(setNoPassword) + noUserDefinedPassword() + .then(setNoPassword) + .catch((err) => consoleError(err, 'could not determine password state')) }, [])src/screens/Apps/Boltz/Settings.tsx (1)
21-26: Guard toggle with pending state and error feedback (optional)If
toggleConnectionis async/fallible, debounce/disable while pending and surface errors to users to avoid flip-flop UI.src/screens/Apps/Index.tsx (2)
54-57: Harden external link openingUse noopener/noreferrer to prevent tabnabbing.
Apply:
const handleClick = () => { if (typeof page !== 'undefined') return navigate(page) - if (link) window.open(link, '_blank') + if (link) window.open(link, '_blank', 'noopener,noreferrer') }
59-62: Make the clickable card accessible (keyboard/role) (optional)Use a semantic button/anchor or add role="button", tabIndex=0, and Enter/Space key handlers.
Apply:
- <div style={style} onClick={handleClick}> + <div + role='button' + tabIndex={0} + style={style} + onClick={handleClick} + onKeyDown={(e) => (e.key === 'Enter' || e.key === ' ') && handleClick()} + >src/App.tsx (2)
126-138: Mount content declaratively per tab (optional)Consider using IonTabs’
valueprop and routing/value binding instead of conditionally renderingcompin each IonTab. This reduces imperative refs and potential focus issues.
80-97: Use React Router to switch tabs instead of calling setActive()
In Ionic React v8, tabs are tied to the router. Replace imperative calls likewalletRef.current?.setActive()with navigation (e.g.,
navigate('/tabs/wallet')) or by controlling<IonTabs value={tab}>declaratively.src/icons/Status.tsx (1)
5-11: Add a11y attrs and align color theming
- Set aria-hidden on decorative SVGs to avoid noisy screen-reader output.
- Prefer CSS variables over hard-coded hex for consistent theming (you already use var(--green) for green; do similarly for red/orange if tokens exist).
Apply this minimal a11y diff:
- <svg width={width} height={height} viewBox='0 0 20 21' fill='none' xmlns='http://www.w3.org/2000/svg'> + <svg width={width} height={height} viewBox='0 0 20 21' fill='none' xmlns='http://www.w3.org/2000/svg' aria-hidden='true' focusable='false'> ... - <svg width={size} height={size} viewBox='0 0 18 18' fill='none' xmlns='http://www.w3.org/2000/svg'> + <svg width={size} height={size} viewBox='0 0 18 18' fill='none' xmlns='http://www.w3.org/2000/svg' aria-hidden='true' focusable='false'> ... - <svg width={size} height={size} viewBox='0 0 18 18' fill='none' xmlns='http://www.w3.org/2000/svg'> + <svg width={size} height={size} viewBox='0 0 18 18' fill='none' xmlns='http://www.w3.org/2000/svg' aria-hidden='true' focusable='false'>If your design system provides tokens (e.g., var(--red), var(--orange), var(--dark)), consider replacing hard-coded fills/strokes accordingly.
Also applies to: 18-31, 37-53
src/components/Empty.tsx (2)
12-24: Render secondary text conditionallyAvoid empty when secondaryText is missing.
- <TextSecondary>{secondaryText}</TextSecondary> + {secondaryText ? <TextSecondary>{secondaryText}</TextSecondary> : null}Also consider extracting strings for i18n and aligning punctuation across messages.
46-54: Optional: add aria-hidden on decorative Empty iconsThese SVGs are decorative; mark them hidden from assistive tech.
- <svg width='57' height='56' viewBox='0 0 57 56' fill='none' xmlns='http://www.w3.org/2000/svg'> + <svg width='57' height='56' viewBox='0 0 57 56' fill='none' xmlns='http://www.w3.org/2000/svg' aria-hidden='true' focusable='false'>Apply to EmptyCoinsIcon, EmptyListIcon, and EmptySwapListIcon.
Also applies to: 56-64, 92-101
src/screens/Wallet/Receive/Amount.tsx (1)
81-90: Unify button label derivation; avoid flicker
- The effect derives label; handleChange also sets it, causing flicker. Rely on the effect only.
- Include amountIsAboveMaxLimit in deps so label updates when limits change.
- useEffect(() => { + useEffect(() => { setButtonLabel( !satoshis ? defaultButtonLabel : satoshis < 1 ? 'Amount below 1 satoshi' : amountIsAboveMaxLimit(satoshis) ? 'Amount above max limit' : 'Continue', ) - }, [satoshis]) + }, [satoshis, amountIsAboveMaxLimit]) ... - const handleChange = (amount: number) => { - setAmount(amount) - setButtonLabel(amount ? 'Continue' : defaultButtonLabel) - } + const handleChange = (amount: number) => setAmount(amount)Also applies to: 94-97
src/providers/limits.tsx (1)
47-52: Ref state may not trigger consumer updates; prefer state or a version bumpUpdating limits.current in effects won’t trigger re-renders; consumers relying on derived values (e.g., labels) may not update unless they re-render for other reasons.
- Store limits in useState and setState on updates, or maintain a “version” state you bump when limits change and pass it through context to drive reactivity.
- Alternatively, expose an async getLimits() method in context returning fresh values. Based on learnings
Also applies to: 68-91, 93-101
src/screens/Apps/Boltz/Swap.tsx (1)
31-42: Clamp negative “Fees” and remove redundant coercion
totalandamountare already numbers—noNumber(...)calls needed.- Wrap
total - amountinMath.max(0, …)to avoid negative fees; omit clamp if negative values should be shown.- ['Fees', formatAmount(total - amount)], + ['Fees', formatAmount(Math.max(0, total - amount))],Confirm whether negative fees should ever be displayed.
src/components/AlertBox.tsx (1)
34-45: Add alert semantics for accessibility.Mark the container as an alert to improve screen reader announcement.
- <div style={style.alert}> + <div style={style.alert} role="alert" aria-live="polite">src/providers/lightning.tsx (1)
44-51: Avoid redundant config write on mount.Calling setConnected(config.apps.boltz.connected) immediately mirrors existing state and can cause unnecessary updates.
useEffect(() => { if (!aspInfo.network || !svcWallet) return const baseUrl = BoltzUrl ?? BASE_URLS[aspInfo.network as Network] if (!baseUrl) return // No boltz server for this network setSwapProvider(new LightningSwapProvider(baseUrl, aspInfo, svcWallet)) - setConnected(config.apps.boltz.connected) }, [aspInfo, svcWallet, config.apps.boltz.connected])src/providers/nudge.tsx (1)
23-37: Persist nudge dismissal across sessions.Prevent re-appearing on refresh by storing a dismissed flag (e.g., localStorage).
- const [dismissed, setDismissed] = useState(false) + const [dismissed, setDismissed] = useState<boolean>(() => { + return localStorage.getItem('nudge.password.dismissed') === '1' + }) ... const dismissNudge = () => { setDismissed(true) setNudge(null) + localStorage.setItem('nudge.password.dismissed', '1') }Also applies to: 39-47
src/screens/Wallet/Send/Form.tsx (2)
178-182: Harden Ark server key check with try/catchdecodeArkAddress() can throw; handle gracefully to avoid crashing validation.
- const { serverPubKey } = decodeArkAddress(arkAddress) - const { serverPubKey: expectedServerPubKey } = decodeArkAddress(offchainAddr) - if (serverPubKey !== expectedServerPubKey) setSendInfo({ ...sendInfo, arkAddress: '' }) + try { + const { serverPubKey } = decodeArkAddress(arkAddress) + const { serverPubKey: expectedServerPubKey } = decodeArkAddress(offchainAddr) + if (serverPubKey !== expectedServerPubKey) setSendInfo({ ...sendInfo, arkAddress: '' }) + } catch { + setSendInfo({ ...sendInfo, arkAddress: '' }) + }
293-295: Compute “send all” more accurately for LN swapsSwap fees are usually a function of the amount. Using balance to estimate may overshoot. Consider iterating to solve for x = balance − fee(x), or at least pass (balance − feeEstimate) to calcSubmarineSwapFee for a better estimate.
src/components/Scanner.tsx (2)
34-75: Persist camera/canvas/cancel with refs to avoid stale closurescamera, canvas, cancel are plain variables; re-renders can desync cleanup. Store them in useRef to ensure proper stop/clear on unmount/switch.
-function ScannerMills({ close, label, onData, onError, onSwitch }: ScannerProps) { - const [error, setError] = useState(false) - const videoRef = useRef<HTMLVideoElement>(null) - let camera: any - let canvas: QRCanvas - let cancel: () => void +function ScannerMills({ close, label, onData, onError, onSwitch }: ScannerProps) { + const [error, setError] = useState(false) + const videoRef = useRef<HTMLVideoElement>(null) + const cameraRef = useRef<any>(null) + const canvasRef = useRef<QRCanvas | null>(null) + const cancelRef = useRef<(() => void) | null>(null)Then update usages (camera -> cameraRef.current, etc.).
104-111: Throttle QrReader error propagationonResult emits frequent NotFound/Checksum errors while scanning. Forwarding every error will spam the parent. Consider filtering benign errors or debouncing.
src/lib/lightning.ts (1)
132-144: Prefer structured logging over console.logUse consoleError or a logger consistently; avoid console.log in library code.
- console.log('Refunding failed submarine swap', swap) + consoleError(undefined, 'Refunding failed submarine swap') ... - console.log('Successfully refunded swap:', swap.response.id) + consoleError(undefined, `Successfully refunded swap: ${swap.response.id}`)
| function faucet { | ||
| local ADDRESS="$1" | ||
| local AMOUNT="$2" | ||
| local ATTEMPTS=10 # Total number of attempts | ||
| local INTERVAL=1 # Seconds between retries | ||
|
|
||
| INITIAL_COUNT=$(curl -s http://localhost:3000/address/$ADDRESS | jq .chain_stats.tx_count) | ||
|
|
||
| TXID=$(nigiri faucet $ADDRESS $AMOUNT) | ||
|
|
||
| for ((i=1; i<=ATTEMPTS; i++)); do | ||
| NEW_COUNT=$(curl -s http://localhost:3000/address/$ADDRESS | jq .chain_stats.tx_count) | ||
| if [ "$NEW_COUNT" -gt "$INITIAL_COUNT" ]; then | ||
| echo $TXID | ||
| echo " ✔" | ||
| return 0 | ||
| else | ||
| echo "Attempt $i/$ATTEMPTS failed, retrying in $INTERVAL second..." | ||
| sleep $INTERVAL | ||
| fi | ||
| done | ||
| } |
There was a problem hiding this comment.
Ensure faucet fails when funding never arrives.
As written, if the loop exhausts all attempts, the function falls through and returns the exit status of the last successful command (sleep), i.e. zero. The script will happily proceed even though the address never received funds, so subsequent channel/opening steps will fail later in confusing ways. Please make the function return non‑zero (and log) when the timeout is hit.
for ((i=1; i<=ATTEMPTS; i++)); do
NEW_COUNT=$(curl -s http://localhost:3000/address/$ADDRESS | jq .chain_stats.tx_count)
if [ "$NEW_COUNT" -gt "$INITIAL_COUNT" ]; then
echo $TXID
echo " ✔"
return 0
else
echo "Attempt $i/$ATTEMPTS failed, retrying in $INTERVAL second..."
sleep $INTERVAL
fi
done
+
+ echo "Timed out waiting for funding after $((ATTEMPTS * INTERVAL)) seconds."
+ return 1
}📝 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.
| function faucet { | |
| local ADDRESS="$1" | |
| local AMOUNT="$2" | |
| local ATTEMPTS=10 # Total number of attempts | |
| local INTERVAL=1 # Seconds between retries | |
| INITIAL_COUNT=$(curl -s http://localhost:3000/address/$ADDRESS | jq .chain_stats.tx_count) | |
| TXID=$(nigiri faucet $ADDRESS $AMOUNT) | |
| for ((i=1; i<=ATTEMPTS; i++)); do | |
| NEW_COUNT=$(curl -s http://localhost:3000/address/$ADDRESS | jq .chain_stats.tx_count) | |
| if [ "$NEW_COUNT" -gt "$INITIAL_COUNT" ]; then | |
| echo $TXID | |
| echo " ✔" | |
| return 0 | |
| else | |
| echo "Attempt $i/$ATTEMPTS failed, retrying in $INTERVAL second..." | |
| sleep $INTERVAL | |
| fi | |
| done | |
| } | |
| function faucet { | |
| local ADDRESS="$1" | |
| local AMOUNT="$2" | |
| local ATTEMPTS=10 # Total number of attempts | |
| local INTERVAL=1 # Seconds between retries | |
| INITIAL_COUNT=$(curl -s http://localhost:3000/address/$ADDRESS | jq .chain_stats.tx_count) | |
| TXID=$(nigiri faucet $ADDRESS $AMOUNT) | |
| for ((i=1; i<=ATTEMPTS; i++)); do | |
| NEW_COUNT=$(curl -s http://localhost:3000/address/$ADDRESS | jq .chain_stats.tx_count) | |
| if [ "$NEW_COUNT" -gt "$INITIAL_COUNT" ]; then | |
| echo $TXID | |
| echo " ✔" | |
| return 0 | |
| else | |
| echo "Attempt $i/$ATTEMPTS failed, retrying in $INTERVAL second..." | |
| sleep $INTERVAL | |
| fi | |
| done | |
| echo "Timed out waiting for funding after $((ATTEMPTS * INTERVAL)) seconds." | |
| return 1 | |
| } |
🤖 Prompt for AI Agents
In scripts/setupRegtestEnv.sh around lines 34 to 55, the faucet function
currently falls through with a zero exit status when all retry attempts are
exhausted; change it so that after the for loop completes without detecting the
funding arrival the function prints an error message (to stderr or prefixed with
"ERROR:"), and returns a non‑zero status (e.g. return 1) to signal failure to
callers; ensure the success path still returns 0 and preserve existing logging
for successful attempts.
| export function InfoBox({ html }: { html: string }) { | ||
| const sanitizedHtml = DOMPurify.sanitize(html) | ||
| return ( | ||
| <AlertBox icon={<MegaphoneIcon animated />}> | ||
| <div dangerouslySetInnerHTML={{ __html: sanitizedHtml }} /> | ||
| </AlertBox> | ||
| ) |
There was a problem hiding this comment.
Harden HTML injection and suppress linter at the call site.
Sanitize with a stricter profile and explicitly suppress the Biome warning for this trusted, sanitized use.
Apply:
export function InfoBox({ html }: { html: string }) {
- const sanitizedHtml = DOMPurify.sanitize(html)
+ const sanitizedHtml = DOMPurify.sanitize(html, { USE_PROFILES: { html: true } })
return (
<AlertBox icon={<MegaphoneIcon animated />}>
- <div dangerouslySetInnerHTML={{ __html: sanitizedHtml }} />
+ {/* biome-ignore lint/security/noDangerouslySetInnerHtml: content is sanitized above */}
+ <div dangerouslySetInnerHTML={{ __html: sanitizedHtml }} />
</AlertBox>
)
}📝 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 function InfoBox({ html }: { html: string }) { | |
| const sanitizedHtml = DOMPurify.sanitize(html) | |
| return ( | |
| <AlertBox icon={<MegaphoneIcon animated />}> | |
| <div dangerouslySetInnerHTML={{ __html: sanitizedHtml }} /> | |
| </AlertBox> | |
| ) | |
| export function InfoBox({ html }: { html: string }) { | |
| const sanitizedHtml = DOMPurify.sanitize(html, { USE_PROFILES: { html: true } }) | |
| return ( | |
| <AlertBox icon={<MegaphoneIcon animated />}> | |
| {/* biome-ignore lint/security/noDangerouslySetInnerHtml: content is sanitized above */} | |
| <div dangerouslySetInnerHTML={{ __html: sanitizedHtml }} /> | |
| </AlertBox> | |
| ) | |
| } |
🧰 Tools
🪛 ast-grep (0.39.6)
[warning] 60-60: 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)
🪛 Biome (2.1.2)
[error] 61-61: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
🤖 Prompt for AI Agents
In src/components/AlertBox.tsx around lines 57 to 63, the call to
DOMPurify.sanitize uses default settings and the dangerouslySetInnerHTML line
triggers a Biome linter warning; update the sanitize call to use a stricter
profile (e.g., DOMPurify.sanitize(html, { USE_PROFILES: { html: true } }) or
equivalent stricter config) so only safe HTML is allowed, and add an inline
Biome suppression comment on the dangerouslySetInnerHTML line to silence the
linter for this trusted, sanitized usage.
| function CloseIcon({ onClick }: { onClick: () => void }) { | ||
| return ( | ||
| <svg xmlns='http://www.w3.org/2000/svg' width='20' height='20' viewBox='0 0 512 512' onClick={onClick}> | ||
| <path | ||
| fill='currentColor' | ||
| d='M256 48C141.31 48 48 141.31 48 256s93.31 208 208 208s208-93.31 208-208S370.69 48 256 48Zm75.31 260.69a16 16 0 1 1-22.62 22.62L256 278.63l-52.69 52.68a16 16 0 0 1-22.62-22.62L233.37 256l-52.68-52.69a16 16 0 0 1 22.62-22.62L256 233.37l52.69-52.68a16 16 0 0 1 22.62 22.62L278.63 256Z' | ||
| /> | ||
| </svg> | ||
| ) | ||
| } |
There was a problem hiding this comment.
Make CloseIcon accessible and prevent parent onClick from firing.
Use a button, add aria-label, and stop propagation so dismiss doesn’t also trigger the AlertBox onClick.
-function CloseIcon({ onClick }: { onClick: () => void }) {
- return (
- <svg xmlns='http://www.w3.org/2000/svg' width='20' height='20' viewBox='0 0 512 512' onClick={onClick}>
+function CloseIcon({ onClick }: { onClick: () => void }) {
+ return (
+ <button
+ type='button'
+ aria-label='Dismiss'
+ onClick={(e) => {
+ e.stopPropagation()
+ onClick()
+ }}
+ style={{ background: 'none', border: 'none', padding: 0, color: 'inherit', cursor: 'pointer' }}
+ >
+ <svg xmlns='http://www.w3.org/2000/svg' width='20' height='20' viewBox='0 0 512 512'>
<path
fill='currentColor'
d='M256 48C141.31 48 48 141.31 48 256s93.31 208 208 208s208-93.31 208-208S370.69 48 256 48Zm75.31 260.69a16 16 0 1 1-22.62 22.62L256 278.63l-52.69 52.68a16 16 0 0 1-22.62-22.62L233.37 256l-52.68-52.69a16 16 0 0 1 22.62-22.62L256 233.37l52.69-52.68a16 16 0 0 1 22.62 22.62L278.63 256Z'
/>
- </svg>
+ </svg>
+ </button>
)
}📝 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.
| function CloseIcon({ onClick }: { onClick: () => void }) { | |
| return ( | |
| <svg xmlns='http://www.w3.org/2000/svg' width='20' height='20' viewBox='0 0 512 512' onClick={onClick}> | |
| <path | |
| fill='currentColor' | |
| d='M256 48C141.31 48 48 141.31 48 256s93.31 208 208 208s208-93.31 208-208S370.69 48 256 48Zm75.31 260.69a16 16 0 1 1-22.62 22.62L256 278.63l-52.69 52.68a16 16 0 0 1-22.62-22.62L233.37 256l-52.68-52.69a16 16 0 0 1 22.62-22.62L256 233.37l52.69-52.68a16 16 0 0 1 22.62 22.62L278.63 256Z' | |
| /> | |
| </svg> | |
| ) | |
| } | |
| function CloseIcon({ onClick }: { onClick: () => void }) { | |
| return ( | |
| <button | |
| type='button' | |
| aria-label='Dismiss' | |
| onClick={(e) => { | |
| e.stopPropagation() | |
| onClick() | |
| }} | |
| style={{ background: 'none', border: 'none', padding: 0, color: 'inherit', cursor: 'pointer' }} | |
| > | |
| <svg xmlns='http://www.w3.org/2000/svg' width='20' height='20' viewBox='0 0 512 512'> | |
| <path | |
| fill='currentColor' | |
| d='M256 48C141.31 48 48 141.31 48 256s93.31 208 208 208s208-93.31 208-208S370.69 48 256 48Zm75.31 260.69a16 16 0 1 1-22.62 22.62L256 278.63l-52.69 52.68a16 16 0 0 1-22.62-22.62L233.37 256l-52.68-52.69a16 16 0 0 1 22.62-22.62L256 233.37l52.69-52.68a16 16 0 0 1 22.62 22.62L278.63 256Z' | |
| /> | |
| </svg> | |
| </button> | |
| ) | |
| } |
| {onMax && !disabled && !readOnly ? ( | ||
| <IonText | ||
| slot='end' | ||
| style={{ ...fontStyle, marginLeft: '0.5rem', color: 'var(--purpletext)', cursor: 'pointer' }} | ||
| onClick={onMax} | ||
| role='button' | ||
| tabIndex={0} | ||
| aria-label='Set maximum amount' | ||
| > | ||
| Max | ||
| </IonText> | ||
| ) : null} |
There was a problem hiding this comment.
Add keyboard event handlers for accessibility.
The Max button has role='button' and tabIndex={0} but lacks keyboard event handlers. Keyboard-only users cannot activate it with Enter or Space keys, violating WCAG 2.1.1 (Keyboard).
Apply this diff to add keyboard support:
{onMax && !disabled && !readOnly ? (
<IonText
slot='end'
style={{ ...fontStyle, marginLeft: '0.5rem', color: 'var(--purpletext)', cursor: 'pointer' }}
onClick={onMax}
+ onKeyDown={(ev) => {
+ if (ev.key === 'Enter' || ev.key === ' ') {
+ ev.preventDefault()
+ onMax()
+ }
+ }}
role='button'
tabIndex={0}
aria-label='Set maximum amount'
>
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 && !disabled && !readOnly ? ( | |
| <IonText | |
| slot='end' | |
| style={{ ...fontStyle, marginLeft: '0.5rem', color: 'var(--purpletext)', cursor: 'pointer' }} | |
| onClick={onMax} | |
| role='button' | |
| tabIndex={0} | |
| aria-label='Set maximum amount' | |
| > | |
| Max | |
| </IonText> | |
| ) : null} | |
| {onMax && !disabled && !readOnly ? ( | |
| <IonText | |
| slot='end' | |
| style={{ ...fontStyle, marginLeft: '0.5rem', color: 'var(--purpletext)', cursor: 'pointer' }} | |
| onClick={onMax} | |
| onKeyDown={(ev) => { | |
| if (ev.key === 'Enter' || ev.key === ' ') { | |
| ev.preventDefault() | |
| onMax() | |
| } | |
| }} | |
| role='button' | |
| tabIndex={0} | |
| aria-label='Set maximum amount' | |
| > | |
| Max | |
| </IonText> | |
| ) : null} |
🤖 Prompt for AI Agents
In src/components/InputAmount.tsx around lines 94 to 105, the "Max" element is
given role='button' and tabIndex={0} but lacks keyboard handlers; add an
onKeyDown handler that calls the existing onMax when the user presses Enter (key
=== 'Enter') or Space (key === ' ' or key === 'Spacebar') and preventDefault for
Space to avoid scrolling, and ensure the handler no-ops if disabled or readOnly;
keep the existing onClick, role, and tabIndex so the element is operable by both
mouse and keyboard.
| if (!swapProvider) return | ||
| swapProvider.getSwapHistory().then(setSwapHistory) | ||
| }, [swapProvider]) |
There was a problem hiding this comment.
Handle getSwapHistory rejections.
swapProvider.getSwapHistory() is invoked without a catch, so any network or provider failure triggers an unhandled rejection in React’s effect, spamming the console and leaving the list stuck empty. Wrap the call in try/catch (or append .catch(consoleError)) so we degrade gracefully.
- swapProvider.getSwapHistory().then(setSwapHistory)
+ ;(async () => {
+ try {
+ const history = await swapProvider.getSwapHistory()
+ setSwapHistory(history)
+ } catch (err) {
+ consoleError(err, 'Failed to load swap history')
+ }
+ })()📝 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 (!swapProvider) return | |
| swapProvider.getSwapHistory().then(setSwapHistory) | |
| }, [swapProvider]) | |
| if (!swapProvider) return | |
| ;(async () => { | |
| try { | |
| const history = await swapProvider.getSwapHistory() | |
| setSwapHistory(history) | |
| } catch (err) { | |
| consoleError(err, 'Failed to load swap history') | |
| } | |
| })() | |
| }, [swapProvider]) |
🤖 Prompt for AI Agents
In src/components/SwapsList.tsx around lines 121 to 123, the effect calls
swapProvider.getSwapHistory() without handling rejections; wrap the call in an
async function with try/catch (or append .catch(...)) to handle network/provider
errors, log the error (e.g., console.error or a logger) and ensure you set a
safe state (e.g., setSwapHistory([]) or an error flag) so the component degrades
gracefully instead of leaving an unhandled rejection and an empty list.
| const pass = password ? password : defaultPassword | ||
| getPrivateKey(pass) | ||
| .then(initWallet) | ||
| .then(() => navigate(Pages.Wallet)) | ||
| .catch((err) => { | ||
| consoleError(err, 'error unlocking wallet') | ||
| setError('Invalid password') | ||
| if (password) { | ||
| consoleError(err, 'error unlocking wallet') | ||
| setError('Invalid password') | ||
| } | ||
| }) |
There was a problem hiding this comment.
Critical: Silent auto-unlock with default password.
The unlock logic now automatically attempts to use defaultPassword when no password is entered, with several concerning behaviors:
- Silent fallback: Users are unaware that a default password is being tried
- Hidden failures: If the default password fails, no error is shown (Line 23 checks
if (password)before setting error) - Security bypass: Wallets initialized with the default password can be unlocked without any user input
This creates a confusing and insecure user experience:
- Users may not realize their wallet has a default password
- Failed unlock attempts with the default password provide no feedback
- The unlock screen becomes misleading (appears to require a password, but auto-succeeds without one)
Related to the Init.tsx changes that bypass password creation. If this is for testing only (per PR title "do not merge: signet default"), add explicit guards or feature flags. Otherwise, remove the default password fallback and require explicit user authentication.
Apply this diff to require explicit password entry:
useEffect(() => {
- const pass = password ? password : defaultPassword
- getPrivateKey(pass)
+ if (!password) return
+ getPrivateKey(password)
.then(initWallet)
.then(() => navigate(Pages.Wallet))
.catch((err) => {
- if (password) {
- consoleError(err, 'error unlocking wallet')
- setError('Invalid password')
- }
+ consoleError(err, 'error unlocking wallet')
+ setError('Invalid password')
})
}, [password])🤖 Prompt for AI Agents
In src/screens/Wallet/Unlock.tsx around lines 18 to 27, the current code
silently falls back to defaultPassword when no password is entered and
suppresses error feedback if that fallback fails; change the flow to require an
explicit user-entered password (do not use defaultPassword), call getPrivateKey
only when password is non-empty, and in the catch handler always log the error
and call setError('Invalid password') so failures are visible to users; if the
default-password behavior is truly needed for testing, gate it behind a clearly
named feature flag (e.g., ENABLE_DEFAULT_WALLET) and ensure the UI shows a
prominent warning when the flag is active.
| import userEvent from '@testing-library/user-event' | ||
| import { describe, expect, it, vi } from 'vitest' | ||
| import Button from '../../components/Button' | ||
| import ButtonsOnButtom from '../../components/ButtonsOnBottom' |
There was a problem hiding this comment.
Fix the typo in the component import.
The import statement has a typo: ButtonsOnButtom should be ButtonsOnBottom. This will cause a module resolution error if the actual exported component name is ButtonsOnBottom.
Apply this diff to correct the import:
-import ButtonsOnButtom from '../../components/ButtonsOnBottom'
+import ButtonsOnBottom from '../../components/ButtonsOnBottom'You'll also need to update the JSX usage on lines 10 and 20:
- <ButtonsOnButtom>
+ <ButtonsOnBottom>
<Button label='Click Me' onClick={() => {}} />
- </ButtonsOnButtom>,
+ </ButtonsOnBottom>,📝 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.
| import ButtonsOnButtom from '../../components/ButtonsOnBottom' | |
| // src/test/components/ButtonsOnBottom.test.tsx | |
| import React from 'react' | |
| import ButtonsOnBottom from '../../components/ButtonsOnBottom' | |
| import { render } from '@testing-library/react' | |
| import Button from '../../components/Button' | |
| describe('ButtonsOnBottom', () => { | |
| it('renders children inside ButtonsOnBottom', () => { | |
| const { getByText } = render( | |
| <ButtonsOnBottom> | |
| <Button label='Click Me' onClick={() => {}} /> | |
| </ButtonsOnBottom>, | |
| ) | |
| expect(getByText('Click Me')).toBeInTheDocument() | |
| }) | |
| }) |
🤖 Prompt for AI Agents
In src/test/components/ButtonsOnBottom.test.tsx around line 5, the import has a
typo: change the imported identifier from ButtonsOnButtom to ButtonsOnBottom to
match the actual component export; also update the JSX usage on lines 10 and 20
to use <ButtonsOnBottom /> instead of the misspelled name so imports and
component references are consistent.
| import { render, screen } from '@testing-library/react' | ||
| import { describe, expect, it } from 'vitest' | ||
| import Details from '../../components/Details' |
There was a problem hiding this comment.
Wrap test with required context providers.
The Details component uses ConfigContext and FiatContext (lines 29-30 in src/components/Details.tsx), but this test doesn't provide them. The test will fail when the component tries to call useContext.
Wrap the component in the necessary providers:
import { render, screen } from '@testing-library/react'
import { describe, expect, it } from 'vitest'
import Details from '../../components/Details'
+import { ConfigContext } from '../../context/ConfigContext'
+import { FiatContext } from '../../context/FiatContext'Then update the test:
it('renders the details with the correct information', () => {
- render(<Details details={props} />)
+ const mockConfigValue = {
+ config: { showBalance: true, fiat: 'USD' },
+ useFiat: false,
+ }
+ const mockFiatValue = {
+ toFiat: (amount: number) => amount * 0.0001, // example conversion
+ }
+ render(
+ <ConfigContext.Provider value={mockConfigValue}>
+ <FiatContext.Provider value={mockFiatValue}>
+ <Details details={props} />
+ </FiatContext.Provider>
+ </ConfigContext.Provider>
+ )
expect(screen.getByText('outgoing')).toBeInTheDocument()
})📝 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.
| import { render, screen } from '@testing-library/react' | |
| import { describe, expect, it } from 'vitest' | |
| import Details from '../../components/Details' | |
| import { render, screen } from '@testing-library/react' | |
| import { describe, expect, it } from 'vitest' | |
| import Details from '../../components/Details' | |
| import { ConfigContext } from '../../context/ConfigContext' | |
| import { FiatContext } from '../../context/FiatContext' | |
| describe('Details component', () => { | |
| const props = { | |
| /* existing test props */ | |
| } | |
| it('renders the details with the correct information', () => { | |
| const mockConfigValue = { | |
| config: { showBalance: true, fiat: 'USD' }, | |
| useFiat: false, | |
| } | |
| const mockFiatValue = { | |
| toFiat: (amount: number) => amount * 0.0001, // example conversion | |
| } | |
| render( | |
| <ConfigContext.Provider value={mockConfigValue}> | |
| <FiatContext.Provider value={mockFiatValue}> | |
| <Details details={props} /> | |
| </FiatContext.Provider> | |
| </ConfigContext.Provider> | |
| ) | |
| expect(screen.getByText('outgoing')).toBeInTheDocument() | |
| }) | |
| }) |
🤖 Prompt for AI Agents
In src/test/components/Details.test.tsx around lines 1 to 3, the test renders
<Details /> without providing ConfigContext and FiatContext which the component
consumes; update the test to import or create the appropriate provider
components/values and wrap the rendered <Details /> inside those providers
(supply minimal/mock context values required by Details), then call render with
the provider-wrapped tree so useContext calls succeed.
| const props = { | ||
| address: '123 Main St', | ||
| arknote: 'Test Note', | ||
| date: '2023-01-01', | ||
| destination: '456 Elm St', | ||
| direction: 'outgoing', | ||
| fees: 1000, | ||
| invoice: 'INV-123', | ||
| satoshis: 100000, | ||
| total: 1000, | ||
| type: 'payment', | ||
| when: '2023-01-01', | ||
| } |
There was a problem hiding this comment.
Fix inconsistent test data.
The total value (1000) doesn't match the sum of satoshis (100000) and fees (1000), which would be 101000. This inconsistency could mask bugs if the component's logic changes.
Apply this diff:
satoshis: 100000,
- total: 1000,
+ total: 101000,
type: 'payment',📝 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 props = { | |
| address: '123 Main St', | |
| arknote: 'Test Note', | |
| date: '2023-01-01', | |
| destination: '456 Elm St', | |
| direction: 'outgoing', | |
| fees: 1000, | |
| invoice: 'INV-123', | |
| satoshis: 100000, | |
| total: 1000, | |
| type: 'payment', | |
| when: '2023-01-01', | |
| } | |
| const props = { | |
| address: '123 Main St', | |
| arknote: 'Test Note', | |
| date: '2023-01-01', | |
| destination: '456 Elm St', | |
| direction: 'outgoing', | |
| fees: 1000, | |
| invoice: 'INV-123', | |
| satoshis: 100000, | |
| total: 101000, | |
| type: 'payment', | |
| when: '2023-01-01', | |
| } |
🤖 Prompt for AI Agents
In src/test/components/Details.test.tsx around lines 6 to 18, the test fixture
has inconsistent numeric data: total is 1000 but should equal satoshis + fees;
update the total value to the correct sum (satoshis 100000 + fees 1000 = 101000)
so the test data is consistent with component expectations.
| it('should throw an error for an invalid address', () => { | ||
| expect(() => decodeArkAddress('invalidAddress')).toThrow('Invalid address') | ||
| }) |
There was a problem hiding this comment.
Avoid brittle error message assertions in thrown-error test
Relying on exact/partial message text couples the test to library internals. Assert that an error is thrown instead.
Apply:
- it('should throw an error for an invalid address', () => {
- expect(() => decodeArkAddress('invalidAddress')).toThrow('Invalid address')
- })
+ it('should throw an error for an invalid address', () => {
+ expect(() => decodeArkAddress('invalidAddress')).toThrow()
+ })📝 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.
| it('should throw an error for an invalid address', () => { | |
| expect(() => decodeArkAddress('invalidAddress')).toThrow('Invalid address') | |
| }) | |
| it('should throw an error for an invalid address', () => { | |
| expect(() => decodeArkAddress('invalidAddress')).toThrow() | |
| }) |
🤖 Prompt for AI Agents
In src/test/lib/address.test.ts around lines 20 to 22, the test currently
asserts a specific error message when decoding an invalid address which makes
the test brittle; change the assertion to only expect that an error is thrown
(e.g., use expect(() => decodeArkAddress('invalidAddress')).toThrow() or
expect(() => decodeArkAddress('invalidAddress')).toThrowError() depending on
your test framework) so the test verifies failure without coupling to exact
error text.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
vite.worker.config.ts (1)
14-14: Remove unused external 'expo/fetch'.
In vite.worker.config.ts (line 14), 'expo/fetch' is externalized but not imported in src/wallet-service-worker.ts. You can safely remove it from theexternalarray.
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (3)
src/providers/wallet.tsx (2)
153-175: Persist and reuse SW message handler; current removeEventListener is ineffectiveYou create a new handler each init and attempt to remove it using a different function reference, leading to duplicate listeners.
Apply this diff here; add a ref, remove old handler, then attach the new one:
+ // keep a stable ref to the handler to properly remove it + // declare at top-level: const svcMsgHandlerRef = useRef<((e: MessageEvent) => void) | null>(null) // handle messages from the service worker - const handleServiceWorkerMessages = (event: MessageEvent) => { - const reloadWallet = () => { + const handleServiceWorkerMessages = (event: MessageEvent) => { + const doReload = () => { getTxHistory(svcWallet).then(setTxs).catch(consoleError) getBalance(svcWallet).then(setBalance).catch(consoleError) } if (event.data && ['VTXO_UPDATE', 'UTXO_UPDATE'].includes(event.data.type)) { - reloadWallet() + doReload() // reload again after a delay to give the indexer time to update its cache - setTimeout(reloadWallet, 5000) + setTimeout(doReload, 5000) } } // listen for messages from the service worker - if (listeningForServiceWorker.current) { - navigator.serviceWorker.removeEventListener('message', handleServiceWorkerMessages) - navigator.serviceWorker.addEventListener('message', handleServiceWorkerMessages) - } else { - navigator.serviceWorker.addEventListener('message', handleServiceWorkerMessages) - listeningForServiceWorker.current = true - } + if (svcMsgHandlerRef.current) { + navigator.serviceWorker.removeEventListener('message', svcMsgHandlerRef.current) + } + svcMsgHandlerRef.current = handleServiceWorkerMessages + navigator.serviceWorker.addEventListener('message', svcMsgHandlerRef.current) + listeningForServiceWorker.current = trueAdditionally, add once-per-component cleanup:
// near hooks const svcMsgHandlerRef = useRef<((e: MessageEvent) => void) | null>(null) useEffect(() => { return () => { if (svcMsgHandlerRef.current) { navigator.serviceWorker.removeEventListener('message', svcMsgHandlerRef.current) svcMsgHandlerRef.current = null } } }, [])
181-188: Clear the heartbeat interval to avoid timer leaksA new setInterval is created on each init without clearing the previous one.
- // ping the service worker wallet status every 1 second - setInterval(async () => { + // ping the service worker wallet status every 1 second + // declare at top-level: const statusIntervalRef = useRef<number | null>(null) + if (statusIntervalRef.current) { + clearInterval(statusIntervalRef.current) + statusIntervalRef.current = null + } + statusIntervalRef.current = window.setInterval(async () => { try { const { walletInitialized } = await svcWallet.getStatus() setInitialized(walletInitialized) } catch (err) { consoleError(err, 'Error pinging wallet status') } - }, 1_000) + }, 1_000)And add unmount cleanup:
const statusIntervalRef = useRef<number | null>(null) useEffect(() => { return () => { if (statusIntervalRef.current) { clearInterval(statusIntervalRef.current) statusIntervalRef.current = null } } }, [])src/screens/Wallet/Transaction.tsx (1)
167-171: Remove the hardcodedtruedisabling “Resend”.The button remains permanently disabled because of
disabled={resending || true}. Drop the literaltrueso the control can actually be used (or hide it until resend is implemented).- <Button onClick={handleResend} label='Resend' disabled={resending || true} /> + <Button onClick={handleResend} label='Resend' disabled={resending} />
🧹 Nitpick comments (4)
src/screens/Settings/Vtxos.tsx (2)
79-102: Extract inline component definitions to prevent recreation on every render.
BoxandVtxoLineare defined inside theVtxosfunction body, causing them to be recreated on every render. This breaks referential equality, can trigger unnecessary re-renders, and is generally considered an anti-pattern in React.Apply this diff to extract the components:
+const Box = ({ children }: { children: ReactNode }) => { + const style = { + backgroundColor: 'var(--dark10)', + border: '1px solid var(--dark20)', + borderRadius: '0.25rem', + padding: '0.5rem', + width: '100%', + } + return ( + <div style={style}> + <FlexRow between>{children}</FlexRow> + </div> + ) +} + +const VtxoLine = ({ vtxo, showBalance }: { vtxo: Vtxo; showBalance: boolean }) => { + const amount = showBalance ? prettyNumber(vtxo.value) : prettyHide(vtxo.value) + return ( + <Box> + <Text>{amount} SATS</Text> + <Text>{prettyAgo(vtxo.virtualStatus?.batchExpiry ?? 0)}</Text> + </Box> + ) +} + export default function Vtxos() { const { aspInfo, calcBestMarketHour } = useContext(AspContext) const { config } = useContext(ConfigContext) ... - const Box = ({ children }: { children: ReactNode }) => { - const style = { - backgroundColor: 'var(--dark10)', - border: '1px solid var(--dark20)', - borderRadius: '0.25rem', - padding: '0.5rem', - width: '100%', - } - return ( - <div style={style}> - <FlexRow between>{children}</FlexRow> - </div> - ) - } - - const VtxoLine = ({ vtxo }: { vtxo: Vtxo }) => { - const amount = config.showBalance ? prettyNumber(vtxo.value) : prettyHide(vtxo.value) - return ( - <Box> - <Text>{amount} SATS</Text> - <Text>{prettyAgo(vtxo.virtualStatus?.batchExpiry ?? 0)}</Text> - </Box> - ) - }And update the VtxoLine usage:
{vtxos.spendable?.map((v: Vtxo) => ( - <VtxoLine key={v.txid} vtxo={v} /> + <VtxoLine key={v.txid} vtxo={v} showBalance={config.showBalance} /> ))}
99-99: Consider clearer handling for missing expiry.When
batchExpiryis missing, the fallback to0causesprettyAgo(0)to display a date from the epoch (e.g., "55 years ago"), which may confuse users about the actual expiry status.Consider one of these approaches:
- Filter out vtxos without expiry:
{vtxos.spendable?.filter(v => v.virtualStatus?.batchExpiry).map((v: Vtxo) => ( <VtxoLine key={v.txid} vtxo={v} /> ))}
- Display explicit text for missing expiry:
<Text>{vtxo.virtualStatus?.batchExpiry ? prettyAgo(vtxo.virtualStatus.batchExpiry) : 'Unknown'}</Text>src/lib/asp.ts (1)
1-1: Allow ServiceWorkerWallet in collaborativeExit (others remain IWallet-only)Aligns with prior guidance: only collaborativeExit needs to accept both wallet types. Others stay IWallet-only. Based on learnings.
-import { IWallet, ArkNote, RestArkProvider } from '@arkade-os/sdk' +import { IWallet, ServiceWorkerWallet, ArkNote, RestArkProvider } from '@arkade-os/sdk' @@ -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> => {Also applies to: 41-64
src/providers/wallet.tsx (1)
227-230: Use functional state update to avoid stale merges in updateWalletPrevents losing concurrent fields when multiple effects update wallet.
-const updateWallet = (data: Wallet) => { - setWallet({ ...data }) - saveWalletToStorage(data) -} +const updateWallet = (data: Wallet) => { + setWallet((prev) => { + const merged = { ...prev, ...data } + saveWalletToStorage(merged) + return merged + }) +}
📜 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 (10)
package.json(2 hunks)src/lib/asp.ts(2 hunks)src/lib/wallet.ts(2 hunks)src/providers/asp.tsx(3 hunks)src/providers/wallet.tsx(6 hunks)src/screens/Settings/About.tsx(3 hunks)src/screens/Settings/Vtxos.tsx(5 hunks)src/screens/Wallet/Transaction.tsx(4 hunks)src/test/screens/mocks.ts(1 hunks)test.docker-compose.yml(6 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.tssrc/providers/wallet.tsx
🧬 Code graph analysis (7)
src/screens/Settings/Vtxos.tsx (7)
src/providers/asp.tsx (1)
AspContext(15-20)src/providers/config.tsx (1)
ConfigContext(29-37)src/providers/limits.tsx (1)
LimitsContext(29-40)src/providers/wallet.tsx (1)
WalletContext(40-54)src/lib/format.ts (3)
prettyNumber(85-88)prettyHide(69-74)prettyAgo(12-25)src/components/Error.tsx (1)
ErrorMessage(10-21)src/components/Empty.tsx (1)
EmptyCoinsList(26-34)
src/lib/asp.ts (1)
src/providers/asp.tsx (1)
AspInfo(6-6)
src/lib/wallet.ts (1)
src/lib/types.ts (1)
Vtxo(95-95)
src/providers/wallet.tsx (7)
src/providers/flow.tsx (1)
FlowContext(82-95)src/providers/notifications.tsx (1)
NotificationsContext(14-19)src/lib/types.ts (3)
Tx(70-80)Vtxo(95-95)Wallet(97-103)src/lib/storage.ts (2)
readWalletFromStorage(35-37)saveWalletToStorage(31-33)src/lib/wallet.ts (1)
calcNextRollover(17-25)src/lib/asp.ts (2)
getTxHistory(86-120)getBalance(80-84)src/lib/explorers.ts (1)
getRestApiExplorerURL(34-36)
src/screens/Wallet/Transaction.tsx (10)
src/providers/limits.tsx (1)
LimitsContext(29-40)src/providers/flow.tsx (1)
FlowContext(82-95)src/providers/asp.tsx (1)
AspContext(15-20)src/providers/wallet.tsx (1)
WalletContext(40-54)src/lib/format.ts (3)
prettyAgo(12-25)prettyDate(57-67)prettyDelta(36-55)src/components/Error.tsx (1)
ErrorMessage(10-21)src/components/ButtonsOnBottom.tsx (1)
ButtonsOnBottom(10-26)src/components/Button.tsx (1)
Button(20-62)src/components/Reminder.tsx (1)
Reminder(17-69)src/components/WaitingForRound.tsx (1)
WaitingForRound(6-35)
src/test/screens/mocks.ts (3)
src/providers/asp.tsx (1)
AspInfo(6-6)src/lib/asp.ts (1)
emptyAspInfo(11-39)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)
🪛 Gitleaks (8.28.0)
test.docker-compose.yml
[high] 60-60: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (7)
package.json (1)
7-8: Correction: @arkade-os/sdk@0.3.1-alpha.1 is published npm view output confirms 0.3.1-alpha.1 exists; no action required.Likely an incorrect or invalid review comment.
src/screens/Settings/About.tsx (3)
10-10: LGTM! Consistent with codebase-wide Error → ErrorMessage rename.The import correctly reflects the component rename. This change is consistent with the broader refactoring effort across the codebase.
39-39: LGTM! Component usage consistent with import change.The usage of
ErrorMessageis correct and maintains the same props (errorandtext) as the previousErrorcomponent.
27-29: Timing properties are defined on AspInfo and numeric; Number() conversion before prettyDelta is valid.src/test/screens/mocks.ts (1)
6-12: LGTM – useful, minimal mocksShapes align with providers, and defaults are safe for tests. No issues.
Also applies to: 27-32, 34-38, 40-56, 58-71, 73-84
src/lib/wallet.ts (1)
17-24: Guard optional access and normalize expiry in calcNextRollover
- Use optional chaining on
cur.virtualStatus?.batchExpiryto avoid a crash whenvirtualStatusis undefined.- Normalize
batchExpiryto a number before dividing (e.g.Number(expiry)) to prevent runtime errors if it’s a bigint or string.- Verify whether
batchExpiryis in milliseconds or seconds and adjust/removing the/ 1000accordingly.Apply this diff:
-export const calcNextRollover = (vtxos: Vtxo[]): number => { - return vtxos.length - ? vtxos.reduce((acc: number, cur) => { - if (!cur.virtualStatus.batchExpiry) return acc - const unixtimestamp = Math.floor(cur.virtualStatus.batchExpiry / 1000) - return unixtimestamp < acc || acc === 0 ? unixtimestamp : acc - }, 0) - : 0 -} +export const calcNextRollover = (vtxos: Vtxo[]): number => { + if (!vtxos.length) return 0 + return vtxos.reduce((acc: number, cur) => { + const expiry = cur.virtualStatus?.batchExpiry + if (!expiry) return acc + const seconds = Math.floor(Number(expiry) / 1000) + return acc === 0 || seconds < acc ? seconds : acc + }, 0) +}src/providers/asp.tsx (1)
42-56: Convert expiration to BigInt for session calculations
- Wrap
expirationinBigIntbefore all comparisons/arithmetic withscheduledSessionfields- Confirm whether session timestamps use seconds or milliseconds and adjust accordingly (e.g. divide by
1000n)
| txs.sort((a, b) => { | ||
| if (a.createdAt === b.createdAt) return a.type === 'sent' ? -1 : 1 | ||
| if (b.createdAt === 0) return 1 // tx with no date go to the top | ||
| if (a.createdAt === 0) return -1 // tx with no date go to the top | ||
| return a.createdAt > b.createdAt ? -1 : 1 | ||
| }) |
There was a problem hiding this comment.
Fix sort to match comment (“received” first on same timestamp)
Comparator currently prefers 'sent' first, contradicting the comment.
- txs.sort((a, b) => {
- if (a.createdAt === b.createdAt) return a.type === 'sent' ? -1 : 1
+ txs.sort((a, b) => {
+ if (a.createdAt === b.createdAt) return a.type === 'received' ? -1 : 1
if (b.createdAt === 0) return 1 // tx with no date go to the top
if (a.createdAt === 0) return -1 // tx with no date go to the top
return a.createdAt > b.createdAt ? -1 : 1
})📝 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.
| txs.sort((a, b) => { | |
| if (a.createdAt === b.createdAt) return a.type === 'sent' ? -1 : 1 | |
| if (b.createdAt === 0) return 1 // tx with no date go to the top | |
| if (a.createdAt === 0) return -1 // tx with no date go to the top | |
| return a.createdAt > b.createdAt ? -1 : 1 | |
| }) | |
| txs.sort((a, b) => { | |
| if (a.createdAt === b.createdAt) return a.type === 'received' ? -1 : 1 | |
| if (b.createdAt === 0) return 1 // tx with no date go to the top | |
| if (a.createdAt === 0) return -1 // tx with no date go to the top | |
| return a.createdAt > b.createdAt ? -1 : 1 | |
| }) |
🤖 Prompt for AI Agents
In src/lib/asp.ts around lines 113–118 the txs.sort comparator currently returns
a.type === 'sent' ? -1 : 1 when createdAt timestamps are equal, which places
'sent' before 'received' contrary to the comment; change the equality branch to
prefer 'received' first (e.g. return a.type === 'received' ? -1 : 1) and keep
the existing zero-timestamp handling and fallback comparison unchanged.
| // update next rollover when vtxos change | ||
| useEffect(() => { | ||
| let pingInterval: NodeJS.Timeout | ||
| async function initSvcWorkerWallet() { | ||
| try { | ||
| // read wallet from storage | ||
| const walletFromStorage = readWalletFromStorage() | ||
| if (walletFromStorage) setWallet(walletFromStorage) | ||
|
|
||
| // listen for messages from the service worker | ||
| navigator.serviceWorker.addEventListener('message', (event) => { | ||
| if (event.data && event.data.type === 'RELOAD_PAGE') { | ||
| window.location.reload() | ||
| } | ||
| }) | ||
|
|
||
| // connect to the service worker | ||
| const serviceWorker = await setupServiceWorker('/wallet-service-worker.mjs') | ||
| const svcWallet = new ServiceWorkerWallet(serviceWorker) | ||
| setSvcWallet(svcWallet) | ||
|
|
||
| // check if the service worker wallet is initialized | ||
| const { walletInitialized } = await svcWallet.getStatus() | ||
| setInitialized(walletInitialized) | ||
|
|
||
| // ping the service worker wallet status every 1 second | ||
| pingInterval = setInterval(async () => { | ||
| try { | ||
| const { walletInitialized } = await svcWallet.getStatus() | ||
| setInitialized(walletInitialized) | ||
| } catch (err) { | ||
| consoleError(err, 'Error pinging wallet status') | ||
| } | ||
| }, 1_000) | ||
| } catch (err) { | ||
| consoleError(err, 'Error initializing service worker wallet') | ||
| } | ||
| } | ||
| // call async function to initialize the service worker wallet | ||
| initSvcWorkerWallet().then(() => { | ||
| setWalletLoaded(true) | ||
| }) | ||
| return () => clearInterval(pingInterval) | ||
| }, []) | ||
| if (!vtxos?.spendable?.length) return | ||
| const nextRollover = calcNextRollover(vtxos.spendable) | ||
| updateWallet({ ...wallet, nextRollover }) | ||
| }, [vtxos]) |
There was a problem hiding this comment.
Reset nextRollover when no spendable vtxos
Currently it early-returns, leaving stale rollover values.
- if (!vtxos?.spendable?.length) return
- const nextRollover = calcNextRollover(vtxos.spendable)
- updateWallet({ ...wallet, nextRollover })
+ if (!vtxos?.spendable?.length) {
+ updateWallet({ ...wallet, nextRollover: 0 })
+ } else {
+ const nextRollover = calcNextRollover(vtxos.spendable)
+ updateWallet({ ...wallet, nextRollover })
+ }📝 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.
| // update next rollover when vtxos change | |
| useEffect(() => { | |
| let pingInterval: NodeJS.Timeout | |
| async function initSvcWorkerWallet() { | |
| try { | |
| // read wallet from storage | |
| const walletFromStorage = readWalletFromStorage() | |
| if (walletFromStorage) setWallet(walletFromStorage) | |
| // listen for messages from the service worker | |
| navigator.serviceWorker.addEventListener('message', (event) => { | |
| if (event.data && event.data.type === 'RELOAD_PAGE') { | |
| window.location.reload() | |
| } | |
| }) | |
| // connect to the service worker | |
| const serviceWorker = await setupServiceWorker('/wallet-service-worker.mjs') | |
| const svcWallet = new ServiceWorkerWallet(serviceWorker) | |
| setSvcWallet(svcWallet) | |
| // check if the service worker wallet is initialized | |
| const { walletInitialized } = await svcWallet.getStatus() | |
| setInitialized(walletInitialized) | |
| // ping the service worker wallet status every 1 second | |
| pingInterval = setInterval(async () => { | |
| try { | |
| const { walletInitialized } = await svcWallet.getStatus() | |
| setInitialized(walletInitialized) | |
| } catch (err) { | |
| consoleError(err, 'Error pinging wallet status') | |
| } | |
| }, 1_000) | |
| } catch (err) { | |
| consoleError(err, 'Error initializing service worker wallet') | |
| } | |
| } | |
| // call async function to initialize the service worker wallet | |
| initSvcWorkerWallet().then(() => { | |
| setWalletLoaded(true) | |
| }) | |
| return () => clearInterval(pingInterval) | |
| }, []) | |
| if (!vtxos?.spendable?.length) return | |
| const nextRollover = calcNextRollover(vtxos.spendable) | |
| updateWallet({ ...wallet, nextRollover }) | |
| }, [vtxos]) | |
| // update next rollover when vtxos change | |
| useEffect(() => { | |
| if (!vtxos?.spendable?.length) { | |
| updateWallet({ ...wallet, nextRollover: 0 }) | |
| } else { | |
| const nextRollover = calcNextRollover(vtxos.spendable) | |
| updateWallet({ ...wallet, nextRollover }) | |
| } | |
| }, [vtxos]) |
🤖 Prompt for AI Agents
In src/providers/wallet.tsx around lines 100 to 105, the useEffect early-returns
when there are no spendable vtxos which leaves a stale wallet.nextRollover
value; change the effect to explicitly clear or reset nextRollover (e.g., set to
null/undefined/0 per your app convention) when vtxos?.spendable is empty, and
otherwise compute nextRollover with calcNextRollover and call updateWallet({
...wallet, nextRollover }) so the wallet state is always updated both when
spendable vtxos appear and when they are removed.
| ['Server URL', aspInfo.url], | ||
| ['Server pubkey', aspInfo.signerPubkey], |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that the new aspInfo properties exist.
The new Server URL and Server pubkey rows access aspInfo.url and aspInfo.signerPubkey properties. Please verify that these properties are defined on the AspContext.aspInfo type.
Run the following script to verify these properties exist in the AspContext type definition:
🏁 Script executed:
#!/bin/bash
# Description: Verify aspInfo.url and aspInfo.signerPubkey exist in AspContext type
# Search for AspContext type/interface definition
ast-grep --pattern $'interface $_ {
$$$
aspInfo: $$$
$$$
}'
# Also search for the aspInfo type definition
rg -nP --type=ts --type=tsx -C5 'aspInfo.*[:=].*\{|type.*AspInfo.*=|interface.*AspInfo'Length of output: 416
🏁 Script executed:
#!/bin/bash
# Search for AspInfo type or interface definition
rg -nP "(export )?(type|interface)\s+AspInfo" -g '*.ts' -g '*.tsx' -C5Length of output: 1063
Add signerPubkey to AspInfo type
The AspInfo definition in src/providers/asp.tsx (and mirrored in src/lib/faucet.ts) only includes url and unreachable; add signerPubkey: string so aspInfo.signerPubkey compiles.
🤖 Prompt for AI Agents
In src/screens/Settings/About.tsx around lines 22 to 23, the code reads ['Server
URL', aspInfo.url], ['Server pubkey', aspInfo.signerPubkey]; update the AspInfo
interface/type definition in src/providers/asp.tsx (and mirror change in
src/lib/faucet.ts) to include signerPubkey: string so aspInfo.signerPubkey
compiles; add the new property with type string to both definitions and update
any constructors/initializers to supply a value (or default to an empty string)
to avoid runtime undefined values.
| useEffect(() => { | ||
| if (!vtxos?.spendable?.length) return | ||
| const totalAmount = vtxos.spendable.reduce((a, v) => a + v.value, 0) || 0 | ||
| setAboveDust(totalAmount > aspInfo.dust) | ||
| }, [vtxos]) |
There was a problem hiding this comment.
Add aspInfo.dust to the dependency array.
The effect computes aboveDust by comparing totalAmount with aspInfo.dust, but aspInfo.dust is not included in the dependency array. If the dust threshold changes, aboveDust will not be recalculated, potentially causing stale UI state.
Apply this diff to fix the dependency array:
useEffect(() => {
if (!vtxos?.spendable?.length) return
const totalAmount = vtxos.spendable.reduce((a, v) => a + v.value, 0) || 0
setAboveDust(totalAmount > aspInfo.dust)
- }, [vtxos])
+ }, [vtxos, aspInfo.dust])📝 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 (!vtxos?.spendable?.length) return | |
| const totalAmount = vtxos.spendable.reduce((a, v) => a + v.value, 0) || 0 | |
| setAboveDust(totalAmount > aspInfo.dust) | |
| }, [vtxos]) | |
| useEffect(() => { | |
| if (!vtxos?.spendable?.length) return | |
| const totalAmount = vtxos.spendable.reduce((a, v) => a + v.value, 0) || 0 | |
| setAboveDust(totalAmount > aspInfo.dust) | |
| }, [vtxos, aspInfo.dust]) |
🤖 Prompt for AI Agents
In src/screens/Settings/Vtxos.tsx around lines 60 to 64, the useEffect computes
aboveDust using aspInfo.dust but only lists [vtxos] as dependencies, so changes
to aspInfo.dust won't trigger recalculation; update the effect's dependency
array to include aspInfo.dust (e.g., [vtxos, aspInfo.dust]) so totalAmount is
recomputed whenever the dust threshold changes.
| useEffect(() => { | ||
| if (!tx) return | ||
| const expiration = tx.createdAt + Number(aspInfo.vtxoTreeExpiry) | ||
| const bestMarketHour = calcBestMarketHour(expiration) | ||
| const bestMarketHour = calcBestMarketHour(wallet.nextRollover) | ||
| if (bestMarketHour) { | ||
| // setCanSettleOnMarketHour(true) TODO remove after | ||
| setCanSettleOnMarketHour(false) | ||
| setStartTime(Number(bestMarketHour.nextStartTime)) | ||
| setDuration(bestMarketHour.duration) | ||
| setDuration(Number(bestMarketHour.duration)) | ||
| } else { | ||
| setCanSettleOnMarketHour(false) | ||
| setStartTime(wallet.nextRollover) | ||
| setDuration(0) | ||
| } | ||
| }, [wallet.nextRollover]) |
There was a problem hiding this comment.
Fix effect dependencies for reminder scheduling.
This effect bails out on the initial render when tx is still undefined and never re-runs once txInfo loads, because tx (and calcBestMarketHour) are missing from the dependency list. The reminder then keeps startTime = 0, so “Add reminder” opens a calendar entry for 1 Jan 1970. Include the relevant dependencies so the effect recalculates once the transaction arrives.
- }, [wallet.nextRollover])
+ }, [wallet.nextRollover, tx, calcBestMarketHour])📝 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 (!tx) return | |
| const expiration = tx.createdAt + Number(aspInfo.vtxoTreeExpiry) | |
| const bestMarketHour = calcBestMarketHour(expiration) | |
| const bestMarketHour = calcBestMarketHour(wallet.nextRollover) | |
| if (bestMarketHour) { | |
| // setCanSettleOnMarketHour(true) TODO remove after | |
| setCanSettleOnMarketHour(false) | |
| setStartTime(Number(bestMarketHour.nextStartTime)) | |
| setDuration(bestMarketHour.duration) | |
| setDuration(Number(bestMarketHour.duration)) | |
| } else { | |
| setCanSettleOnMarketHour(false) | |
| setStartTime(wallet.nextRollover) | |
| setDuration(0) | |
| } | |
| }, [wallet.nextRollover]) | |
| useEffect(() => { | |
| if (!tx) return | |
| const bestMarketHour = calcBestMarketHour(wallet.nextRollover) | |
| if (bestMarketHour) { | |
| // setCanSettleOnMarketHour(true) TODO remove after | |
| setCanSettleOnMarketHour(false) | |
| setStartTime(Number(bestMarketHour.nextStartTime)) | |
| setDuration(Number(bestMarketHour.duration)) | |
| } else { | |
| setCanSettleOnMarketHour(false) | |
| setStartTime(wallet.nextRollover) | |
| setDuration(0) | |
| } | |
| }, [wallet.nextRollover, tx, calcBestMarketHour]) |
🤖 Prompt for AI Agents
In src/screens/Wallet/Transaction.tsx around lines 55 to 68, the useEffect only
depends on wallet.nextRollover so it bails out on initial render when tx is
undefined and never re-runs when the transaction or helper changes; update the
dependency array to include tx and calcBestMarketHour (and any other values used
from scope that can change) so the effect re-executes when the transaction
arrives and recalculates startTime/duration accordingly.
| environment: | ||
| - POSTGRES_HOST_AUTH_METHOD=trust | ||
| volumes: | ||
| - type: tmpfs | ||
| target: /var/lib/postgresql/data | ||
| healthcheck: | ||
| test: ['CMD-SHELL', 'pg_isready -U postgres -d nbxplorer -h 127.0.0.1 -p 5432'] | ||
| interval: 2s | ||
| timeout: 2s | ||
| retries: 30 |
There was a problem hiding this comment.
Fix Postgres DB initialization for NBXplorer healthcheck
pg_isready -d nbxplorer will never succeed because the container never creates that database—the Postgres image defaults to the postgres database when POSTGRES_DB isn’t set. With this healthcheck failing, pgnbxplorer never becomes “healthy”, so nbxplorer is permanently stuck in depends_on wait. Please add POSTGRES_DB=nbxplorer (or adjust the healthcheck/database name) so the DB exists before readiness is probed.
environment:
- POSTGRES_HOST_AUTH_METHOD=trust
+ - POSTGRES_DB=nbxplorer📝 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.
| environment: | |
| - POSTGRES_HOST_AUTH_METHOD=trust | |
| volumes: | |
| - type: tmpfs | |
| target: /var/lib/postgresql/data | |
| healthcheck: | |
| test: ['CMD-SHELL', 'pg_isready -U postgres -d nbxplorer -h 127.0.0.1 -p 5432'] | |
| interval: 2s | |
| timeout: 2s | |
| retries: 30 | |
| environment: | |
| - POSTGRES_HOST_AUTH_METHOD=trust | |
| - POSTGRES_DB=nbxplorer | |
| volumes: | |
| - type: tmpfs | |
| target: /var/lib/postgresql/data | |
| healthcheck: | |
| test: ['CMD-SHELL', 'pg_isready -U postgres -d nbxplorer -h 127.0.0.1 -p 5432'] | |
| interval: 2s | |
| timeout: 2s | |
| retries: 30 |
🤖 Prompt for AI Agents
In test.docker-compose.yml around lines 8 to 17, the Postgres healthcheck is
using pg_isready -d nbxplorer but the container never creates that database
because POSTGRES_DB is not set; update the environment block to include
POSTGRES_DB=nbxplorer (or change the healthcheck to check the default postgres
DB) so the nbxplorer database exists before the healthcheck runs, ensuring the
pgnbxplorer service can become healthy and allow nbxplorer to proceed.
| - ARKD_WALLET_NBXPLORER_URL=http://nbxplorer:32838 | ||
| - ARKD_WALLET_DATADIR=./data/regtest | ||
| - ARKD_WALLET_NETWORK=regtest | ||
| - ARKD_WALLET_SIGNER_KEY=afcd3fa10f82a05fddc9574fdb13b3991b568e89cc39a72ba4401df8abef35f0 |
There was a problem hiding this comment.
Remove the hardcoded signer private key
Line [60] bakes a full private key into version control. Even in a test compose file this is high risk (and tripped gitleaks); anyone cloning the repo now owns that key forever. Instead, source the signer key from a .env/secrets file or placeholder (ARKD_WALLET_SIGNER_KEY=${ARKD_WALLET_SIGNER_KEY:?set me}), and rotate any real key that might already be exposed.
🧰 Tools
🪛 Gitleaks (8.28.0)
[high] 60-60: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
In test.docker-compose.yml around line 60, the file currently contains a
hardcoded private key assigned to ARKD_WALLET_SIGNER_KEY which must be removed;
replace the literal with an environment-sourced placeholder (e.g. use
ARKD_WALLET_SIGNER_KEY=${ARKD_WALLET_SIGNER_KEY:?set me} or similar) so the
value is injected from a .env or secrets manager, remove any committed real keys
and rotate any exposed key immediately, and update .gitignore/docs to ensure
.env or secret files are not committed.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/screens/Wallet/Receive/Amount.tsx (1)
75-78: Effect missing dependencies leads to stale satoshi conversion.This issue was previously flagged and remains unresolved. The effect depends only on
amountbut usesuseFiatandfromFiat. When fiat mode toggles or conversion rates change,satoshiswill not update untilamountchanges.Apply this diff to fix the dependency array:
- }, [amount]) + }, [amount, useFiat, fromFiat])If
fromFiatis not stable (e.g., recreated on every render), ensure it's memoized usinguseCallbackoruseMemoin the FiatContext provider to prevent unnecessary re-renders.
🧹 Nitpick comments (1)
src/screens/Wallet/Receive/Amount.tsx (1)
169-174: Consider optimizing redundant fee calculation.
calcReverseSwapFee(satoshis)is called twice within the same expression. Consider calculating it once and storing it in a variable for better performance and readability.Apply this diff:
- {showLightningFees ? ( - <Text color='dark50' smaller> - In Lightning you'll receive: {prettyAmount(satoshis)} - {calcReverseSwapFee(satoshis)} ={' '} - {prettyAmount(Math.max(0, satoshis - calcReverseSwapFee(satoshis)))} - </Text> - ) : null} + {showLightningFees ? (() => { + const fee = calcReverseSwapFee(satoshis) + const net = Math.max(0, satoshis - fee) + return ( + <Text color='dark50' smaller> + In Lightning you'll receive: {prettyAmount(satoshis)} - {prettyAmount(fee)} ={' '} + {prettyAmount(net)} + </Text> + ) + })() : null}Alternatively, compute the fee value in a
useMemohook to avoid recalculation on every render.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/screens/Wallet/Receive/Amount.tsx(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/screens/Wallet/Receive/Amount.tsx (6)
src/providers/lightning.tsx (1)
LightningContext(25-31)src/providers/limits.tsx (1)
LimitsContext(29-40)src/components/Loading.tsx (1)
Loading(5-18)src/components/Error.tsx (1)
ErrorMessage(10-21)src/components/Text.tsx (1)
Text(25-79)src/lib/format.ts (1)
prettyAmount(27-34)
🔇 Additional comments (2)
src/screens/Wallet/Receive/Amount.tsx (2)
60-73: Loading gating correctly added.The addition of the
if (!svcWallet) returnguard ensures that address loading is deferred until the wallet service is available, preventing potential race conditions.
92-92: Loading check correctly positioned.The loading guard is correctly placed after all hooks, following React's Rules of Hooks. This ensures hooks are called unconditionally on every render.
|
|
||
| const showFaucetButton = balance === 0 && faucetAvailable | ||
| const disabled = !satoshis ? false : amountIsBelowMinLimit(satoshis) || amountIsAboveMaxLimit(satoshis) | ||
| const showLightningFees = amount && validLnSwap(amount) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify validLnSwap argument when useFiat is enabled.
When useFiat is true, amount represents fiat currency, not satoshis. However, validLnSwap(amount) is called with the fiat value, which may not be the intended behavior. The function likely expects satoshis.
Consider this fix:
- const showLightningFees = amount && validLnSwap(amount)
+ const showLightningFees = satoshis > 0 && validLnSwap(satoshis)Verify whether validLnSwap expects satoshis or should handle both fiat and satoshi inputs. Run the following script to check the function signature:
🏁 Script executed:
#!/bin/bash
# Check validLnSwap implementation in LimitsContext
ast-grep --pattern $'validLnSwap: ($PARAM) => $_'Length of output: 0
🏁 Script executed:
#!/bin/bash
rg -n "validLnSwap" -C3Length of output: 5239
#!/bin/bash
sed -n '1,60p' src/screens/Wallet/Receive/Amount.tsx🏁 Script executed:
#!/bin/bash
sed -n '1,60p' src/screens/Wallet/Receive/Amount.tsxLength of output: 2688
🏁 Script executed:
#!/bin/bash
sed -n '80,140p' src/screens/Wallet/Receive/Amount.tsxLength of output: 1636
🏁 Script executed:
#!/bin/bash
sed -n '60,80p' src/screens/Wallet/Receive/Amount.tsx
rg -n "setSatoshis" -C3 src/screens/Wallet/Receive/Amount.tsxLength of output: 1099
Use satoshis instead of amount when calling validLnSwap
amount may be in fiat (when useFiat is true), but validLnSwap expects a satoshi value.
File src/screens/Wallet/Receive/Amount.tsx line 124:
- const showLightningFees = amount && validLnSwap(amount)
+ const showLightningFees = satoshis > 0 && validLnSwap(satoshis)📝 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 showLightningFees = amount && validLnSwap(amount) | |
| const showFaucetButton = balance === 0 && faucetAvailable | |
| const showLightningFees = satoshis > 0 && validLnSwap(satoshis) | |
| const disabled = !satoshis ? false : satoshis < 1 || amountIsAboveMaxLimit(satoshis) |
🤖 Prompt for AI Agents
In src/screens/Wallet/Receive/Amount.tsx around line 124, change the call to
validLnSwap to use the satoshis value rather than amount because amount can be
fiat when useFiat is true; replace validLnSwap(amount) with
validLnSwap(satoshis) and ensure satoshis is a numeric satoshi value
(parse/convert if needed) so the validation receives the correct units.
Summary by CodeRabbit
New Features
Improvements
Documentation
Chores
Tests