Repository navigation
Alerts are back - #178
Alerts are back#178
Conversation
WalkthroughThis PR updates the boltz-swap dependency to a stable release, refactors internal cryptographic identifiers throughout the wallet service worker, implements a new remote alert-fetching system with conditional rendering, increases the nudge transaction threshold, and updates alert display styling. Changes
Sequence DiagramsequenceDiagram
participant Wallet as Wallet Screen
participant Alert as Alert System
participant Remote as Remote Server
Wallet->>Wallet: Mount component
Wallet->>Alert: getAlert()
Alert->>Remote: fetch(alertUrl)
alt Success
Remote-->>Alert: HTML content
Alert-->>Wallet: HTML string
Wallet->>Wallet: Display AlertBox with HTML
else Error
Remote-->>Alert: Error
Alert-->>Wallet: undefined
Wallet->>Wallet: Show nudge or nothing
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The service worker file undergoes extensive identifier renaming across cryptographic and encoding routines, requiring careful verification that renamed references remain functionally correct. While largely mechanical renaming, the density of changes across multiple interrelated modules (public keys, signatures, PSBT handling, curve operations) necessitates careful tracing. The alert system additions are straightforward but add new remote-fetching logic requiring validation of error handling and rendering integration. Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying wallet-bitcoin-v8 with
|
| Latest commit: |
1a89ef8
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://090d5f79.wallet-v8.pages.dev |
| Branch Preview URL: | https://alerts-are-back.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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
public/wallet-service-worker.mjs (1)
5553-5581: Avoid mutating event payload in aggregatedNonces; clone input map.
aggregatedNonces(t, n)writes inton(the event-provided map). This is brittle and may break callers relying on immutability.Apply this diff:
- async aggregatedNonces(t, n) { + async aggregatedNonces(t, n) { if (!this.graph) throw Pn; if (this.aggregateNonces || (this.aggregateNonces = new Map()), this.myNonces || await this.getNonces(), this.aggregateNonces.has(t)) return { hasAllNonces: this.aggregateNonces.size === this.myNonces?.size }; - const r = this.myNonces.get(t); + const r = this.myNonces.get(t); if (!r) throw new Error(`missing nonce for txid ${t}`); - const o = await this.getPublicKey(); - n.set(S.encode(o.subarray(1)), r); + const o = await this.getPublicKey(); + // clone input map to avoid side-effects on event payload + const nonces = new Map(n); + nonces.set(S.encode(o.subarray(1)), r); const s = this.graph.find(t); if (!s) throw new Error(`missing tx for txid ${t}`); const i = wo(s.root, 0, yo).map( (u) => S.encode(u.key.subarray(1)) ), c = []; for (const u of i) { - const f = n.get(u); + const f = nonces.get(u); if (!f) throw new Error(`missing nonce for cosigner ${u}`); c.push(f.pubNonce); } - const a = gf(c); + const a = gf(c); return this.aggregateNonces.set(t, { pubNonce: a }), { hasAllNonces: this.aggregateNonces.size === this.myNonces?.size }; }Optionally document that
hasAllNoncesmeans “all tree txs aggregated,” to avoid confusion.
🧹 Nitpick comments (2)
public/wallet-service-worker.mjs (1)
8976-8992: De-duplicate history computation; delegate to wallet to avoid drift.Service worker reimplements transaction history logic already present in
wallet.getTransactionHistory(). Prefer a single source of truth to prevent future inconsistencies.Example change:
- async getTransactionHistory() { - if (!this.wallet) return []; - let t = []; - try { - const { boardingTxs: n, commitmentsToIgnore: r } = await this.wallet.getBoardingTxs(), { spendable: o, spent: s } = await this.getAllVtxos(); - const i = Jc(o, s, r); - t = [...n, ...i]; - t.sort((c, a) => c.createdAt === 0 ? -1 : a.createdAt === 0 ? 1 : a.createdAt - c.createdAt); - } catch (n) { - console.error("Error getting transaction history:", n); - } - return t; - } + async getTransactionHistory() { + if (!this.wallet) return []; + try { + return await this.wallet.getTransactionHistory(); + } catch (err) { + console.error("Error getting transaction history:", err); + return []; + } + }src/screens/Wallet/Index.tsx (1)
36-38: Consider adding cleanup for the fetch operation.The fetch operation is not canceled if the component unmounts before completion. While React 18+ handles this gracefully, consider adding an AbortController for cleaner resource management.
Optional enhancement:
useEffect(() => { - getAlert().then(setAlert) + const controller = new AbortController() + getAlert(controller.signal).then(setAlert).catch(() => {}) + return () => controller.abort() }, [])Note: This would require updating
getAlertto accept an optionalAbortSignalparameter.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
package.json(1 hunks)public/wallet-service-worker.mjs(119 hunks)src/components/AlertBox.tsx(1 hunks)src/lib/alerts.ts(1 hunks)src/lib/constants.ts(1 hunks)src/screens/Wallet/Index.tsx(3 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:
public/wallet-service-worker.mjs
🧬 Code graph analysis (2)
src/screens/Wallet/Index.tsx (2)
src/lib/alerts.ts (1)
getAlert(1-9)src/components/AlertBox.tsx (1)
InfoBox(57-66)
src/components/AlertBox.tsx (1)
src/components/Text.tsx (1)
Text(25-79)
🪛 ast-grep (0.39.6)
src/components/AlertBox.tsx
[warning] 61-61: 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)
src/components/AlertBox.tsx
[error] 62-62: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
🔇 Additional comments (5)
src/lib/constants.ts (1)
8-8: LGTM: Threshold increased 10x.The increase from 10,000 to 100,000 sats significantly raises the bar for nudge notifications, which aligns with the new alert system introduced in this PR.
src/components/AlertBox.tsx (1)
58-58: Sanitization is properly implemented.DOMPurify.sanitize() correctly mitigates the XSS concerns flagged by static analysis tools.
package.json (1)
7-7: Verify breaking changes before deploying — package released today.The @arkade-os/boltz-swap upgrade from 0.2.1-alpha.7 (Oct 16) to 0.2.1 (Oct 20) is active in critical code paths:
src/lib/lightning.ts: imports swap status and refund functionssrc/providers/lightning.tsx: manages swap provider and feessrc/screens/Wallet/Send/Form.tsx: uses invoice parsingsrc/components/SwapsList.tsx: displays swap status typesWith only a 4-day alpha-to-stable cycle and no publicly available release notes, confirm there are no breaking changes in the
FeesResponse,Network,PendingReverseSwap,PendingSubmarineSwap, orBoltzSwapStatusexports. Run your test suite to validate compatibility before merging.src/screens/Wallet/Index.tsx (2)
19-20: LGTM! Clean integration of alert functionality.The imports and state declaration are properly structured for the new alert feature.
Also applies to: 29-29
62-62: LGTM! Correct conditional rendering logic.The priority order (nudge → alert → null) is logical, and the InfoBox integration properly handles HTML sanitization via DOMPurify.
| async getTransactionHistory() { | ||
| if (!this.wallet) | ||
| return []; | ||
| let t = []; | ||
| try { | ||
| const { boardingTxs: n, commitmentsToIgnore: r } = await this.wallet.getBoardingTxs(), { spendable: o, spent: s } = await this.getAllVtxos(); | ||
| console.log("getTransactionHistory - vtxosToTxs:", o); | ||
| const i = Jc(o, s, r); | ||
| t = [...n, ...i], t.sort( | ||
| // place createdAt = 0 (unconfirmed txs) first, then descending | ||
| (c, a) => c.createdAt === 0 ? -1 : a.createdAt === 0 ? 1 : a.createdAt - c.createdAt | ||
| ); | ||
| } catch (n) { | ||
| console.error("Error getting transaction history:", n); | ||
| } | ||
| return t; | ||
| } |
There was a problem hiding this comment.
Remove debug logging from getTransactionHistory (leaks and noise).
Avoid logging VTXO data in the service worker. It can expose sensitive info and degrade performance.
Apply this diff:
- try {
- const { boardingTxs: n, commitmentsToIgnore: r } = await this.wallet.getBoardingTxs(), { spendable: o, spent: s } = await this.getAllVtxos();
- console.log("getTransactionHistory - vtxosToTxs:", o);
- const i = Jc(o, s, r);
+ try {
+ const { boardingTxs: n, commitmentsToIgnore: r } = await this.wallet.getBoardingTxs(), { spendable: o, spent: s } = await this.getAllVtxos();
+ const i = Jc(o, s, r);📝 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.
| async getTransactionHistory() { | |
| if (!this.wallet) | |
| return []; | |
| let t = []; | |
| try { | |
| const { boardingTxs: n, commitmentsToIgnore: r } = await this.wallet.getBoardingTxs(), { spendable: o, spent: s } = await this.getAllVtxos(); | |
| console.log("getTransactionHistory - vtxosToTxs:", o); | |
| const i = Jc(o, s, r); | |
| t = [...n, ...i], t.sort( | |
| // place createdAt = 0 (unconfirmed txs) first, then descending | |
| (c, a) => c.createdAt === 0 ? -1 : a.createdAt === 0 ? 1 : a.createdAt - c.createdAt | |
| ); | |
| } catch (n) { | |
| console.error("Error getting transaction history:", n); | |
| } | |
| return t; | |
| } | |
| async getTransactionHistory() { | |
| if (!this.wallet) | |
| return []; | |
| let t = []; | |
| try { | |
| const { boardingTxs: n, commitmentsToIgnore: r } = await this.wallet.getBoardingTxs(), { spendable: o, spent: s } = await this.getAllVtxos(); | |
| const i = Jc(o, s, r); | |
| t = [...n, ...i], t.sort( | |
| // place createdAt = 0 (unconfirmed txs) first, then descending | |
| (c, a) => c.createdAt === 0 ? -1 : a.createdAt === 0 ? 1 : a.createdAt - c.createdAt | |
| ); | |
| } catch (n) { | |
| console.error("Error getting transaction history:", n); | |
| } | |
| return t; | |
| } |
🤖 Prompt for AI Agents
In public/wallet-service-worker.mjs around lines 8976 to 8992, remove the debug
console.log that prints VTXO data to avoid leaking sensitive information and
noisy logs; delete the line console.log("getTransactionHistory - vtxosToTxs:",
o); and, if logging is required for diagnostics, replace it with a
non-sensitive, rate-limited message (e.g., a simple status string) rather than
printing the actual vtxo contents.
| <Text color='black' bold smaller wrap> | ||
| <div dangerouslySetInnerHTML={{ __html: sanitizedHtml }} /> | ||
| </Text> |
There was a problem hiding this comment.
Potential HTML structure violation when nesting arbitrary HTML in <p> tags.
The Text component wraps content in <p> tags (see src/components/Text.tsx). Injecting sanitized HTML that contains block-level elements (div, p, h1, ul, etc.) inside a <p> tag violates HTML spec and will cause browsers to break the paragraph tag, potentially disrupting styling and layout.
Consider one of these solutions:
Solution 1: Render sanitized HTML directly without Text wrapper
- <Text color='black' bold smaller wrap>
- <div dangerouslySetInnerHTML={{ __html: sanitizedHtml }} />
- </Text>
+ <div
+ style={{ color: 'black', fontWeight: '600', fontSize: '13px', whiteSpace: 'normal', wordBreak: 'break-word' }}
+ dangerouslySetInnerHTML={{ __html: sanitizedHtml }}
+ />Solution 2: Use a span wrapper in Text component for inline content only
If alert HTML is guaranteed to contain only inline elements, wrap in a span instead:
- <Text color='black' bold smaller wrap>
- <div dangerouslySetInnerHTML={{ __html: sanitizedHtml }} />
- </Text>
+ <Text color='black' bold smaller wrap>
+ <span dangerouslySetInnerHTML={{ __html: sanitizedHtml }} />
+ </Text>📝 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.
| <Text color='black' bold smaller wrap> | |
| <div dangerouslySetInnerHTML={{ __html: sanitizedHtml }} /> | |
| </Text> | |
| <div | |
| style={{ color: 'black', fontWeight: '600', fontSize: '13px', whiteSpace: 'normal', wordBreak: 'break-word' }} | |
| dangerouslySetInnerHTML={{ __html: sanitizedHtml }} | |
| /> |
| <Text color='black' bold smaller wrap> | |
| <div dangerouslySetInnerHTML={{ __html: sanitizedHtml }} /> | |
| </Text> | |
| <Text color='black' bold smaller wrap> | |
| <span dangerouslySetInnerHTML={{ __html: sanitizedHtml }} /> | |
| </Text> |
🧰 Tools
🪛 ast-grep (0.39.6)
[warning] 61-61: 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] 62-62: 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 61 to 63, the Text component renders
sanitizedHtml inside a <p> (Text wraps content in a paragraph), which can
violate HTML structure if sanitizedHtml contains block-level elements (div, p,
h1, ul, etc.); fix by either rendering the sanitized HTML directly without the
Text wrapper (move the dangerouslySetInnerHTML node out of Text) or ensure Text
uses an inline wrapper (e.g., span) when rendering arbitrary HTML; pick one: if
alerts may contain blocks, remove the Text wrapper and render sanitizedHtml
directly, otherwise change Text usage to an inline wrapper for inline-only
content.
| export const getAlert = async (): Promise<string | undefined> => { | ||
| try { | ||
| const response = await fetch('https://raw.githubusercontent.com/arkade-os/wallet/motd/motd.html') | ||
| const message = await response.text() | ||
| return message | ||
| } catch (error) { | ||
| console.error('Error fetching alert:', error) | ||
| } | ||
| } |
There was a problem hiding this comment.
Add timeout, response validation, and consider privacy implications.
This implementation has several concerns for a wallet application:
- Security/Reliability: No timeout (fetch can hang), no HTTP status validation, and no size limits on the response.
- Privacy: Every wallet screen load triggers a network request to GitHub, potentially enabling usage tracking via IP addresses and timestamps.
- Performance: No caching mechanism, causing repeated fetches and network latency on every visit.
Apply these improvements:
-export const getAlert = async (): Promise<string | undefined> => {
+export const getAlert = async (signal?: AbortSignal): Promise<string | undefined> => {
try {
- const response = await fetch('https://raw.githubusercontent.com/arkade-os/wallet/motd/motd.html')
+ const controller = new AbortController()
+ const timeoutId = setTimeout(() => controller.abort(), 5000)
+
+ const response = await fetch(
+ 'https://raw.githubusercontent.com/arkade-os/wallet/motd/motd.html',
+ {
+ signal: signal || controller.signal,
+ cache: 'default' // Leverage HTTP caching
+ }
+ )
+ clearTimeout(timeoutId)
+
+ if (!response.ok) {
+ throw new Error(`HTTP ${response.status}`)
+ }
+
+ // Limit response size to prevent memory issues
+ const contentLength = response.headers.get('content-length')
+ if (contentLength && parseInt(contentLength) > 50000) {
+ throw new Error('Response too large')
+ }
+
const message = await response.text()
+
+ // Additional size check after reading
+ if (message.length > 50000) {
+ throw new Error('Content too large')
+ }
+
return message
} catch (error) {
console.error('Error fetching alert:', error)
}
}Additionally, consider implementing client-side caching (e.g., localStorage with TTL) to reduce network requests and improve privacy.
🤖 Prompt for AI Agents
In src/lib/alerts.ts around lines 1-9, the fetch of the remote motd lacks
timeout, status/size validation, caching, and privacy controls; change the
function to use an AbortController with a short timeout (e.g., 3–5s) and cancel
the fetch on timeout, check response.ok and Content-Length (or guard the
streamed response to enforce a max size like 50KB) before reading text, and on
any failure return undefined (and log minimally). Add a client-side cache using
localStorage with a TTL and store the fetched HTML plus a fetchedAt timestamp so
repeated calls return cached content until TTL expires; only fetch from network
when cache is stale and avoid adding any identifying headers or analytics, and
ensure errors are swallowed/handled to avoid UI hangs. Optionally make network
fetch conditional on a user preference or a low-frequency background update to
reduce privacy exposure.
@tiero please review
Summary by CodeRabbit
New Features
Bug Fixes
Chores