Repository navigation
refactor nostr backups to use chunks - #235
Conversation
WalkthroughRefactors Nostr backup/restore to a BackupProvider class (pubkey/seckey init), updates NostrStorage to pool-based publish/load and per-event decryption, integrates provider across lightning, config, and UI, adds Playwright e2e tests and NAK Docker service, and bumps Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Lightning as LightningSwapProvider
participant Backup as BackupProvider
participant Nostr as NostrStorage
participant Pool as SimplePool
User->>Lightning: createReverseSwap()/createSubmarineSwap()
Lightning->>Lightning: build pendingSwap
Lightning->>Backup: backup(pendingSwap)
alt reverse swap
Backup->>Backup: backupReverseSwap(pendingSwap)
else submarine swap
Backup->>Backup: backupSubmarineSwap(pendingSwap)
end
Backup->>Nostr: save(payload string)
Nostr->>Pool: publish event(s)
Pool-->>Nostr: publish success
Nostr-->>Backup: resolved
Backup-->>Lightning: resolved
Lightning-->>User: operation complete
sequenceDiagram
autonumber
participant User
participant UI as Restore Screen
participant Backup as BackupProvider
participant Nostr as NostrStorage
participant Pool as SimplePool
User->>UI: trigger restore with seckey
UI->>UI: set restoring = true (show Loading)
UI->>Backup: restore(updateConfig)
Backup->>Nostr: load()
Nostr->>Pool: subscribe for events
Pool-->>Nostr: events[]
Nostr->>Nostr: decrypt & merge events → Event[]
Nostr-->>Backup: return Event[]
Backup->>UI: call updateConfig(config)
UI->>UI: set restoring = false (navigate)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
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-signet with
|
| Latest commit: |
f9bd623
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://cd2f58d9.wallet-23u.pages.dev |
| Branch Preview URL: | https://nostr-backup-chunks.wallet-23u.pages.dev |
Deploying wallet-mutinynet with
|
| Latest commit: |
f9bd623
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://557875d7.arkade-wallet.pages.dev |
| Branch Preview URL: | https://nostr-backup-chunks.arkade-wallet.pages.dev |
Deploying wallet-bitcoin with
|
| Latest commit: |
0adaa60
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://0d663882.wallet-bitcoin.pages.dev |
| Branch Preview URL: | https://nostr-backup-chunks.wallet-bitcoin.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 (2)
src/providers/config.tsx (1)
21-41: Config context type forupdateConfigshould reflect async implementation
updateConfigis now declared asasync, so its actual type is(config: Config, backup: boolean) => Promise<void>, butConfigContextProps.updateConfig(and the default context value) still declare it as returningvoid. This is likely a TypeScript type error when passingupdateConfiginto the provider.Recommend updating the type and default to match:
interface ConfigContextProps { config: Config configLoaded: boolean resetConfig: () => void setConfig: (c: Config) => void showConfig: boolean toggleShowConfig: () => void - updateConfig: (config: Config, backup: boolean) => void + updateConfig: (config: Config, backup: boolean) => Promise<void> useFiat: boolean } export const ConfigContext = createContext<ConfigContextProps>({ config: defaultConfig, configLoaded: false, resetConfig: () => {}, setConfig: () => {}, showConfig: false, toggleShowConfig: () => {}, - updateConfig: () => {}, + updateConfig: async () => {}, useFiat: false, })Call sites that don’t need to await it can continue to ignore the returned promise.
src/lib/nostr.ts (1)
81-117: Close subscription on timeout to avoid leaking relay subscriptionsThe timeout handler resolves
[]without closing the active subscription. If relays never sendEOSE, repeated calls accumulate dangling subscriptions and open relay connections.The fix is valid: move
subto outer scope and callsub?.close()in the timeout handler (line 115), mirroring the cleanup inoneose()(line 103).(Note: The secondary reference "Also applies to: 142-150" is incorrect—those lines contain unrelated methods; only lines 81-117 have the subscription issue.)
🧹 Nitpick comments (5)
src/lib/backup.ts (2)
75-97: Consider factoring out size limit and guarding partial failures infullBackupThe
fullBackupimplementation is correct in terms of splitting large backups into multiple events, but two improvements would make it more robust:
- Extract the
65000threshold into a named constant (e.g.MAX_NOSTR_PAYLOAD_SIZE) shared with any other Nostr backup logic, so the limit is easy to tune and documented.- In the split path, any failure mid‑loop will stop backing up remaining swaps; depending on expectations, you may want to wrap each
backup*call in its own try/catch and log errors per‑swap while continuing, or abort early with a clear error message.Not strictly required for correctness, but would harden the backup process against transient failures.
103-117:restoreAPI shape is fine; type ofupdateConfigcould be widened
restorecorrectly callsupdateConfig(config, false)before replaying swap collections, and then writes swaps viacontractRepo. GivenupdateConfigis nowasyncin the config provider, you might want to widen this parameter’s type to acceptPromise<void>(e.g.(config: Config, backup: boolean) => void | Promise<void>) or at least document that callers may pass an async function whose promise will be ignored. Functionally fine as‑is becausebackupis alwaysfalsein restore flows so no async work is actually awaited.src/screens/Settings/Backup.tsx (1)
74-81: ErrorMessage integration is correct; consider clearing stale errors on successful backupRendering
<ErrorMessage error={Boolean(error)} text={error} />here correctly surfaces both password and backup failures. To avoid showing stale error messages after a subsequent successful backup, you might want to clearerror(e.g.setError('')) on success insidetoggleNostrBackup.src/screens/Init/Restore.tsx (1)
53-63: Restore flow always navigates to success screen, even on failureThe new restore logic:
new BackupProvider({ seckey: privateKey! }) .restore(updateConfig) .catch((err) => consoleError(err, 'Error restoring from nostr')) .finally(() => { setRestoring(false) navigate(Pages.InitSuccess) })will navigate to
InitSuccessregardless of whetherrestoresucceeds or throws. That means a failed Nostr restore still lands the user on a “success” screen, with only a log entry indicating failure.If prior behaviour was to only navigate on success, consider moving
navigate(Pages.InitSuccess)into the success path and surfacing a user‑visible error (e.g. viasetError) on failure. If always navigating is intentional, a small text change on the destination screen clarifying that restore may have partially succeeded could help avoid confusion.src/providers/config.tsx (1)
53-68: Backup integration inupdateConfigis correct; consider gating onnostrBackupand handling missing pubkeyThe async
updateConfigflow is generally good: it normalizesaspUrl, updates state and theme, persists to storage, and only then (optionally) performs a Nostr backup viaBackupProvider.backupConfig(config).Two potential refinements:
Respect the global
nostrBackupflag
Currently, backups run wheneverbackupistrue, regardless ofconfig.nostrBackup. If the intent is “backup only when the user has Nostr backups enabled”, you might want:if (backup && config.nostrBackup) { const backupProvider = new BackupProvider({ pubkey: config.pubkey }) await backupProvider.backupConfig(config).catch((error) => { consoleError(error, 'Backup to Nostr failed') }) }Guard against missing/invalid
pubkey
BackupProviderwill throw on an invalidpubkey(length not 64 or 66). If there’s any path whereupdateConfigcould be called withbackup === truebeforeconfig.pubkeyis populated, you may want to add a simple precondition check (e.g.if (!config.pubkey) return;) or log a clearer error.These aren’t correctness blockers, but they’ll help align behaviour with user expectations and make failures easier to reason about.
📜 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 (7)
package.json(1 hunks)src/lib/backup.ts(1 hunks)src/lib/lightning.ts(5 hunks)src/lib/nostr.ts(5 hunks)src/providers/config.tsx(3 hunks)src/screens/Init/Restore.tsx(3 hunks)src/screens/Settings/Backup.tsx(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-06T09:53:28.428Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 134
File: src/screens/Settings/Password.tsx:34-39
Timestamp: 2025-08-06T09:53:28.428Z
Learning: In src/screens/Settings/Password.tsx, the saveNewPassword function doesn't need error handling for getPrivateKey because it's only called after authentication is verified. The useEffect validates the oldPassword using isValidPassword() which internally calls getPrivateKey(), and only sets authenticated=true if successful. The saveNewPassword function has a guard clause that returns early if not authenticated, so getPrivateKey(oldPassword) is guaranteed to work when called.
Applied to files:
src/screens/Settings/Backup.tsxsrc/screens/Init/Restore.tsx
📚 Learning: 2025-07-21T18:25:36.391Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 121
File: src/screens/Init/Restore.tsx:58-58
Timestamp: 2025-07-21T18:25:36.391Z
Learning: In src/screens/Init/Restore.tsx, the Input component for private key restoration intentionally omits the `value` prop to make it uncontrolled. This is a security feature to prevent the private key from being displayed if users navigate away and return to the screen, avoiding potential exposure of sensitive data in the UI.
Applied to files:
src/screens/Settings/Backup.tsxsrc/screens/Init/Restore.tsx
📚 Learning: 2025-07-07T14:41:35.991Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 106
File: src/lib/bolt11.ts:153-155
Timestamp: 2025-07-07T14:41:35.991Z
Learning: In src/lib/bolt11.ts, the code is copied from the original bitcoinjs/bolt11 library (https://github.com/bitcoinjs/bolt11/blob/master/payreq.js) and should be kept unchanged to maintain compatibility with the upstream source, even if there are potential improvements or syntax issues.
Applied to files:
package.json
🧬 Code graph analysis (5)
src/screens/Settings/Backup.tsx (3)
src/lib/backup.ts (1)
BackupProvider(16-161)src/lib/logs.ts (1)
consoleError(40-44)src/components/Error.tsx (1)
ErrorMessage(10-21)
src/lib/lightning.ts (3)
src/lib/types.ts (1)
Config(8-23)src/lib/backup.ts (1)
BackupProvider(16-161)src/lib/logs.ts (1)
consoleError(40-44)
src/lib/nostr.ts (1)
src/lib/logs.ts (1)
consoleError(40-44)
src/lib/backup.ts (2)
src/lib/types.ts (1)
Config(8-23)src/lib/nostr.ts (1)
NostrStorage(9-151)
src/screens/Init/Restore.tsx (3)
src/lib/backup.ts (1)
BackupProvider(16-161)src/lib/logs.ts (1)
consoleError(40-44)src/components/Loading.tsx (1)
Loading(5-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (8)
package.json (1)
7-7: boltz-swap patch bump looks fine; confirm compatibility with new backup flowThe version bump to
@arkade-os/boltz-swap@0.2.9is reasonable for this PR’s backup refactor, assuming no breaking changes between0.2.8and0.2.9. Please double‑check the package’s changelog/release notes for any behavioural changes that might affectPendingReverseSwap/PendingSubmarineSwapshapes used insrc/lib/backup.ts.src/lib/backup.ts (2)
27-41: Constructor seckey/pubkey handling is consistent with NostrStorage usageThe constructor’s handling of
seckeyvspubkey, including 0x‑prefixed hex normalization and length checks, matches the expectations inNostrStorageand should behave correctly for both backup‑only (pubkey) and restore (seckey) flows.
47-68: Backup helpers are straightforward and type-safe
backupConfig,backupReverseSwap, andbackupSubmarineSwapeach wrap a minimalNostrStorageDataenvelope and delegate tonostrStorage.save, which keeps the payload shape simple and consistent withloadData. No issues here.src/screens/Init/Restore.tsx (2)
32-49: Newrestoringstate and key parsing flow look goodThe additional
restoringflag combined with the existing key parsing logic (nsecToPrivateKey/hex.decode/invalidPrivateKey) anddisabledbutton state keeps the restore UX coherent while preventing multiple submissions. Input remains uncontrolled, which preserves the intended security property from earlier work. Based on learnings
67-67: Loading indicator gating onrestoringis appropriateThe early return
if (restoring) return <Loading text='Restoring wallet...' />correctly blocks interaction during restore and avoids multiple submissions. No issues here.src/lib/nostr.ts (2)
5-5: Consistent app tagging for backup events looks goodUsing a single
nostrAppNameconstant for both publish (['t', nostrAppName]) and subscribe filters ('#t': [nostrAppName]) is a clean way to scope backup events to this app and avoid cross-app collisions. No issues here.Also applies to: 54-55, 92-93
13-13: Pool reuse and raw string payloads are reasonableInitializing a single
SimplePoolperNostrStorageinstance and reusing it forsaveavoids repeated connection setup and is a good direction. Taking a raw stringpayload(with callers handling JSON serialization) also keeps this class focused on transport concerns instead of data shape. The publish timeout guard is appropriate for preventing hangs.Also applies to: 23-23, 46-47, 57-58, 65-65
src/lib/lightning.ts (1)
9-11: Backup routing by swap type is sound and keeps failures non-fatalUsing
isPendingSubmarineSwap/isPendingReverseSwapand re-fetching pending swaps before backing up ensures you persist the freshest view of each swap. Callingbackupafter creation, claim, creation of submarine swaps, and refund covers the important lifecycle transitions, and thenostrBackupguard plus catch block keep backup failures from impacting swap flows.Once the constructor concern above is addressed, this backup integration looks solid.
Also applies to: 40-57, 100-101, 118-119, 155-156, 170-171
| import { BackupProvider } from './backup' | ||
| import * as Sentry from '@sentry/react' |
There was a problem hiding this comment.
🧩 Analysis chain
Guard BackupProvider construction to avoid hard failures when Nostr backup is disabled or misconfigured
LightningSwapProvider now always constructs a BackupProvider with config.pubkey. Because BackupProvider validates pubkey length and throws on invalid input, any config with an empty or malformed pubkey will now cause the Lightning provider constructor to fail, even if config.nostrBackup is false and backup would never be used.
To keep Lightning usable when Nostr backup is turned off (or before a pubkey is configured), consider instantiating BackupProvider only when backup is enabled and the pubkey is valid, e.g.:
- private readonly backupProvider: BackupProvider
+ private readonly backupProvider: BackupProvider | null
@@
- const indexerProvider = new RestIndexerProvider(aspInfo.url)
- this.backupProvider = new BackupProvider({ pubkey: config.pubkey })
+ const indexerProvider = new RestIndexerProvider(aspInfo.url)
+ this.backupProvider = config.nostrBackup ? new BackupProvider({ pubkey: config.pubkey }) : nulland then in backup:
- private backup = async (swap: PendingReverseSwap | PendingSubmarineSwap) => {
- if (!this.config.nostrBackup) return
+ private backup = async (swap: PendingReverseSwap | PendingSubmarineSwap) => {
+ if (!this.config.nostrBackup || !this.backupProvider) returnThis preserves the new behavior when backup is enabled but avoids introducing a new hard dependency on a valid Nostr pubkey for all Lightning usage.
Also applies to: 25-26, 32-32
🏁 Script executed:
#!/bin/bash
# Examine the current state of lightning.ts around the identified lines and constructor
fd -t f 'lightning\.ts$' | head -5Length of output: 20
🏁 Script executed:
#!/bin/bash
# Look at the constructor and backupProvider initialization
rg -n 'class LightningSwapProvider|backupProvider|BackupProvider' src/lib/lightning.ts -A 3 -B 1Length of output: 1294
🏁 Script executed:
#!/bin/bash
# Find the BackupProvider implementation to understand validation
fd -t f 'backup\.ts$' -o -name '*backup*' | grep -v node_modules | head -10Length of output: 230
🏁 Script executed:
#!/bin/bash
# Check the backup() method and its call sites
rg -n 'private backup|createReverseSwap|claimVHTLC|createSubmarineSwap|refundVHTLC' src/lib/lightning.ts -B 2 -A 5Length of output: 2574
🏁 Script executed:
#!/bin/bash
# Find backup file(s) more carefully
find src -type f -name '*backup*' 2>/dev/nullLength of output: 17
🏁 Script executed:
#!/bin/bash
# Search for BackupProvider class definition
rg -n 'class BackupProvider|export.*BackupProvider' -A 10Length of output: 716
🏁 Script executed:
#!/bin/bash
# Check the Config type definition
rg -n 'type Config|interface Config' src/lib/types.ts -A 15Length of output: 311
🏁 Script executed:
#!/bin/bash
# Look at the BackupProvider constructor to see validation logic
rg -n 'constructor|seckey|pubkey.*=' src/lib/backup.ts -A 5 -B 2 | head -50Length of output: 1255
🏁 Script executed:
#!/bin/bash
# Find where BackupProvider throws or validates
rg -n 'throw|if.*pubkey|pubkey.*length' src/lib/backup.ts -B 2 -A 2Length of output: 915
🏁 Script executed:
#!/bin/bash
# Check if config.pubkey is validated or checked before line 32 in lightning.ts
rg -n 'config\.pubkey' src/lib/lightning.ts -B 5 -A 2Length of output: 462
Fix BackupProvider instantiation to prevent constructor failure when Nostr backup is disabled or pubkey is invalid
BackupProvider validates pubkey length and throws on invalid input (lines 35, 39 of src/lib/backup.ts). Currently, LightningSwapProvider unconditionally constructs a BackupProvider at line 32 with config.pubkey, causing the entire Lightning provider to fail during construction if the pubkey is empty or malformed—even when config.nostrBackup is false.
The backup() method's guard at line 41 (if (!this.config.nostrBackup) return) only protects the backup operations, not the constructor. To keep Lightning usable when backup is disabled, gate BackupProvider instantiation:
- private readonly backupProvider: BackupProvider
+ private readonly backupProvider: BackupProvider | null
constructor(apiUrl: string, aspInfo: AspInfo, wallet: Wallet | ServiceWorkerWallet, config: Config) {
const indexerProvider = new RestIndexerProvider(aspInfo.url)
- this.backupProvider = new BackupProvider({ pubkey: config.pubkey })
+ this.backupProvider = config.nostrBackup ? new BackupProvider({ pubkey: config.pubkey }) : nulland update the backup method guard:
private backup = async (swap: PendingReverseSwap | PendingSubmarineSwap) => {
- if (!this.config.nostrBackup) return
+ if (!this.config.nostrBackup || !this.backupProvider) returnCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/lib/lightning.ts around lines 17–41, the code currently constructs
BackupProvider unconditionally which throws if config.nostrBackup is false or
config.pubkey is empty/invalid; change this so BackupProvider is only
instantiated when this.config.nostrBackup is true and the pubkey passes the
length/format check (e.g., non-empty and expected length) — make the
BackupProvider field optional/nullable, initialize it inside the constructor
only when nostrBackup is enabled and pubkey is valid, and update the backup()
method guard to first check that this.config.nostrBackup is true AND that
this.backupProvider (or equivalent) exists before calling into it.
| const toggleNostrBackup = async () => { | ||
| const newConfig = { ...config, nostrBackup: !config.nostrBackup } | ||
| if (newConfig.nostrBackup) { | ||
| const backupProvider = new BackupProvider({ pubkey: config.pubkey }) | ||
| await backupProvider.fullBackup(newConfig).catch((error) => { | ||
| consoleError(error, 'Backup to Nostr failed') | ||
| setError('Backup to Nostr failed') | ||
| return | ||
| }) | ||
| } else { | ||
| updateConfig(newConfig, true) | ||
| } | ||
| present(backupToNostr) |
There was a problem hiding this comment.
Enabling Nostr backup doesn’t persist config change; disabling still triggers backup
There are a couple of behavioural issues in toggleNostrBackup:
-
When enabling (
newConfig.nostrBackup === true), you callfullBackup(newConfig)but never callupdateConfig(newConfig, ...), so:config.nostrBackupin context remains unchanged;- the toggle will not stay “on” after the re-render or app restart.
-
When disabling (
newConfig.nostrBackup === false), you callupdateConfig(newConfig, true), which still triggers a backup viaBackupProvider.backupConfig, even though the user just turned Nostr backups off. -
Errors thrown synchronously by
new BackupProvider({ pubkey: config.pubkey })(e.g. invalid/missing pubkey) are not caught by the.catchattached tofullBackup, so they will surface as unhandled rejections and won’t set theerrorstate.
A more consistent flow would be:
- Always persist the new config (in both branches).
- Only perform a backup when enabling (or when explicitly desired).
- Wrap the whole block in try/catch so all errors are handled uniformly and the toast isn’t shown on failure.
For example:
- const toggleNostrBackup = async () => {
- const newConfig = { ...config, nostrBackup: !config.nostrBackup }
- if (newConfig.nostrBackup) {
- const backupProvider = new BackupProvider({ pubkey: config.pubkey })
- await backupProvider.fullBackup(newConfig).catch((error) => {
- consoleError(error, 'Backup to Nostr failed')
- setError('Backup to Nostr failed')
- return
- })
- } else {
- updateConfig(newConfig, true)
- }
- present(backupToNostr)
- }
+ const toggleNostrBackup = async () => {
+ const newConfig = { ...config, nostrBackup: !config.nostrBackup }
+ try {
+ if (newConfig.nostrBackup) {
+ const backupProvider = new BackupProvider({ pubkey: config.pubkey })
+ await backupProvider.fullBackup(newConfig)
+ }
+ await updateConfig(newConfig, false)
+ setError('')
+ present(backupToNostr)
+ } catch (error) {
+ consoleError(error, 'Backup to Nostr failed')
+ setError('Backup to Nostr failed')
+ }
+ }Adjust the backup flag in updateConfig according to the semantics you want, but the key is to ensure the config change is always persisted and backups match the user’s intent.
🤖 Prompt for AI Agents
In src/screens/Settings/Backup.tsx around lines 51 to 63, toggleNostrBackup
currently only persists config when disabling, triggers an unwanted backup when
disabling, and does not catch synchronous errors from BackupProvider; change it
to always persist the newConfig (call updateConfig(newConfig, <backupFlag>) in
both branches), set the updateConfig backup flag to true only when you intend to
perform a backup (false when disabling), run the actual Nostr backup only when
enabling (call BackupProvider.fullBackup after persisting), wrap the entire
enable/backup flow in try/catch so both constructor errors and async errors are
caught and setError/consoleError, and only call present(backupToNostr) after the
backup succeeds (or immediately after persisting when no backup is requested).
louisinger
left a comment
louisinger
left a comment
There was a problem hiding this comment.
Is it possible to add a "restore" e2e test using nak (nak serve starts a nostr relay "inmemory" so easy setup).
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/screens/Settings/Backup.tsx (1)
31-43: Clearerroron successful unlock/backup to avoid stale or misleading messagesWith the new
ErrorMessageat line 73 andNeedsPasswordstill consuming the sameerrorstate (line 99), stale errors can now leak into the main backup view:
- After a wrong password,
setError('Invalid password')is called in thecatchblock (lines 37‑41), buterroris never cleared on a subsequent successfulgetPrivateKeycall. Oncensecis set, the main view renders and still shows “Invalid password”.- Similarly, after a transient backup failure, “Backup to Nostr failed” will remain visible even after later successful backups unless you reset
error.Consider explicitly clearing
erroron success in both flows, e.g.:useEffect(() => { const pass = password ? password : defaultPassword getPrivateKey(pass) .then((privateKey) => { + setError('') setNsec(privateKeyToNsec(privateKey)) }) .catch((err) => { if (password) { consoleError(err, 'error unlocking wallet') setError('Invalid password') } }) }, [password])(and as suggested in
toggleNostrBackup, callsetError('')in the success path there as well).Also applies to: 73-73, 99-99
♻️ Duplicate comments (2)
src/lib/backup.ts (1)
127-166:loadData’s merge strategy and JSON.parse error handling look robustThe new
loadDataimplementation addresses the previous hard‑failure risk:
- Events are sorted by
created_atfor deterministic replay.JSON.parseis wrapped in atry/catchand logs viaconsoleError, so a single malformed event no longer aborts the whole restore.- Config uses “last write wins”, while swaps are de‑duplicated by
idusing maps.This is a solid approach for dealing with mixed legacy and chunked backup events on unreliable relays.
src/screens/Settings/Backup.tsx (1)
51-63: Fix backup flag inversion and wrap the entire Nostr backup flow in try/catchThe current
toggleNostrBackupstill has the behavioural problems from the earlier review:
- Line 53:
updateConfig(newConfig, !newConfig.nostrBackup)inverts the backup flag, so you request a backup when disabling and explicitly skip it when enabling.- Lines 55‑60: only
fullBackup’s promise rejection is caught; synchronous errors fromnew BackupProvider(...)will escape and won’t seterroror avoid the success toast.- Line 62:
present(backupToNostr)runs even if the backup failed.A safer, more consistent flow is:
- Always persist
newConfig.- Only run a Nostr backup when enabling.
- Treat the whole enable/backup + persist sequence as one
try/catch, clearingerroron success and setting it on any failure.- Only show the success toast when everything succeeds.
For example:
- const toggleNostrBackup = async () => { - const newConfig = { ...config, nostrBackup: !config.nostrBackup } - updateConfig(newConfig, !newConfig.nostrBackup) - if (newConfig.nostrBackup) { - const backupProvider = new BackupProvider({ pubkey: config.pubkey }) - await backupProvider.fullBackup(newConfig).catch((error) => { - consoleError(error, 'Backup to Nostr failed') - setError('Backup to Nostr failed') - return - }) - } - present(backupToNostr) - } + const toggleNostrBackup = async () => { + const newConfig = { ...config, nostrBackup: !config.nostrBackup } + try { + // Always persist config; do not trigger implicit backup from updateConfig here. + await updateConfig(newConfig, false) + + if (newConfig.nostrBackup) { + const backupProvider = new BackupProvider({ pubkey: config.pubkey }) + await backupProvider.fullBackup(newConfig) + } + + setError('') + present(backupToNostr) + } catch (error) { + consoleError(error, 'Backup to Nostr failed') + setError('Backup to Nostr failed') + } + }This ensures config persistence regardless of backup success, avoids unwanted backups on disable, and keeps user feedback consistent.
🧹 Nitpick comments (4)
nak.Dockerfile (1)
2-26: Prefer pinning Go/nak versions and a non-latestruntime image for reproducible buildsThe multi‑stage build looks fine functionally, but using
golang:1.25.3-alpine,alpine:latest, andgo install ...@latestmakes builds non‑deterministic and potentially brittle when upstreams change or tags disappear.Consider pinning explicit, known‑good versions instead (both for the base images and the
nakmodule tag), e.g.:-FROM golang:1.25.3-alpine AS builder +FROM golang:1.2x-alpine AS builder # pin to the Go version you actually support -# Install nak -RUN go install github.com/fiatjaf/nak@latest +# Install a specific nak release for reproducibility +RUN go install github.com/fiatjaf/nak@vX.Y.Z -# runtime stage -FROM alpine:latest +# runtime stage +FROM alpine:3.19Adjust versions to whatever you’re testing against in CI.
test.docker-compose.yml (1)
248-255: Consider adding a restart policy for thenaktest serviceThe
nakservice definition (build + port + command) is straightforward and matches the Dockerfile, but unlike most other services in this file it has norestartpolicy.If you want parity with the rest of the test stack and more resilient local runs, you might add:
nak: build: context: . dockerfile: nak.Dockerfile container_name: nak + restart: unless-stopped ports: - '10547:10547' command: ['nak', 'serve', '--hostname', '0.0.0.0', '--port', '10547']Optional, but can reduce flakiness in iterative testing.
src/lib/backup.ts (2)
28-42: Constructor key handling is sound but duplicates NostrStorage’s validationThe constructor correctly supports both
{ seckey }and{ pubkey }and enforces pubkey length, which matches howNostrStorageworks. Right now, though:
- The key‑validation logic is largely duplicated with
NostrStorage’s constructor.seckeyis stored onBackupProviderbut never used outside constructingNostrStorage.If you want to reduce duplication, you could consider delegating more of the key normalization/validation to
NostrStorageand either dropping theseckeyfield here or exposing it only if you plan to use it later (e.g. for additional Nostr features). Not urgent, but it would simplify this class a bit.
76-97: Chunking logic is correct; consider parallelizing per-swap saves for large backupsThe
fullBackupmethod correctly:
- Builds a single snapshot of config + swaps.
- Falls back to per‑piece events when the payload exceeds ~65 KB.
- Uses maps on restore to de‑duplicate by swap id.
In the chunked branch you send each swap sequentially:
for (const reverseSwap of data.reverseSwaps ?? []) { await this.backupReverseSwap(reverseSwap) } for (const submarineSwap of data.submarineSwaps ?? []) { await this.backupSubmarineSwap(submarineSwap) }For many swaps this can add noticeable latency. If
NostrStorage.saveand the relays can tolerate some parallelism, you could optionally batch withPromise.all:await Promise.all((data.reverseSwaps ?? []).map((swap) => this.backupReverseSwap(swap))) await Promise.all((data.submarineSwaps ?? []).map((swap) => this.backupSubmarineSwap(swap)))Not required, but it would make full backups scale better as history grows.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
nak.Dockerfile(1 hunks)src/lib/backup.ts(1 hunks)src/screens/Settings/Backup.tsx(3 hunks)test.docker-compose.yml(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-06T09:53:28.428Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 134
File: src/screens/Settings/Password.tsx:34-39
Timestamp: 2025-08-06T09:53:28.428Z
Learning: In src/screens/Settings/Password.tsx, the saveNewPassword function doesn't need error handling for getPrivateKey because it's only called after authentication is verified. The useEffect validates the oldPassword using isValidPassword() which internally calls getPrivateKey(), and only sets authenticated=true if successful. The saveNewPassword function has a guard clause that returns early if not authenticated, so getPrivateKey(oldPassword) is guaranteed to work when called.
Applied to files:
src/screens/Settings/Backup.tsx
📚 Learning: 2025-07-21T18:25:36.391Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 121
File: src/screens/Init/Restore.tsx:58-58
Timestamp: 2025-07-21T18:25:36.391Z
Learning: In src/screens/Init/Restore.tsx, the Input component for private key restoration intentionally omits the `value` prop to make it uncontrolled. This is a security feature to prevent the private key from being displayed if users navigate away and return to the screen, avoiding potential exposure of sensitive data in the UI.
Applied to files:
src/screens/Settings/Backup.tsx
🧬 Code graph analysis (2)
src/screens/Settings/Backup.tsx (3)
src/lib/backup.ts (1)
BackupProvider(17-168)src/lib/logs.ts (1)
consoleError(40-44)src/components/Error.tsx (1)
ErrorMessage(10-21)
src/lib/backup.ts (3)
src/lib/types.ts (1)
Config(8-23)src/lib/nostr.ts (1)
NostrStorage(9-151)src/lib/logs.ts (1)
consoleError(40-44)
🔇 Additional comments (3)
src/screens/Settings/Backup.tsx (2)
19-20: BackupProvider/ErrorMessage imports are consistent with the new backup flowThe new imports cleanly align this screen with the centralized
BackupProviderand sharedErrorMessagecomponent; no issues here.
73-73: Nice reuse ofErrorMessageto surface backup/password issues in the backup UIHooking
ErrorMessageinto this screen gives users immediate feedback when something goes wrong; once the stale‑error handling is addressed, this should improve UX significantly.test.docker-compose.yml (1)
45-47: Image pinning for core services looks consistent and improves reproducibilitySwitching
arkd-wallet,arkd,boltz, andboltz-fulmineto explicit tagged images makes the test stack more predictable and easier to reason about across environments; the wiring (ports/env/volumes) remains unchanged, which is good.Also applies to: 63-63, 124-124, 228-228
|
@louisinger it took me way longer to make the test than to fix the bug 🤣 Please double check, specially cae38eb |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/screens/Init/Restore.tsx (1)
29-49: ResetprivateKey/validation when the input is cleared or invalidIn the
useEffectonsomeKey, whensomeKeyis falsy youreturnearly without resetting any state:if (!someKey) return ... setPrivateKey(privateKey)This means:
- If the user enters a valid key (setting
privateKeyand clearingerror), then clears the field,privateKeyanderrorkeep their previous values.disabledis computed asBoolean(!privateKey || error), so with a cleared field but a staleprivateKeyand noerror, the “Continue” button stays enabled and will restore with the old key.Consider explicitly clearing state when the field is empty and only setting
privateKeyon valid parses, e.g.:useEffect(() => { if (!someKey) { setPrivateKey(undefined) setLabel(buttonLabel) setError('') return } let pk: Uint8Array | undefined try { pk = someKey.match(/^nsec/) ? nsecToPrivateKey(someKey) : hex.decode(someKey) const invalid = invalidPrivateKey(pk) setLabel(invalid ? 'Unable to validate private key format' : buttonLabel) setError(invalid) setPrivateKey(invalid ? undefined : pk) } catch (err) { setLabel('Unable to validate key format') setError(extractError(err)) setPrivateKey(undefined) } }, [someKey])Also, adding
name='private-key'while keeping theInputuncontrolled (novalueprop) preserves the earlier security decision to avoid re‑rendering the secret, which is good. Based on learnings.Also applies to: 65-66, 76-76
src/lib/nostr.ts (1)
81-117: Close the Nostr subscription whenload()times out to avoid resource leaksSimplePool.subscribeMany returns a SubCloser object with a .close() method you call to end that subscription, and without calling it, subscriptions persist and connections remain open.
In the current
load()implementation (lines 81–117), the success path closes the subscription viasub.close()in theoneosecallback (line 103), but the timeout promise (line 111–113) resolves without closing it. Sincesubis scoped to the first Promise, the timeout path cannot access it. This leaves dangling subscriptions and open WebSocket connections.Repeated
load()calls that hit the timeout will accumulate unclosed subscriptions in theSimplePool. Capture the subscription reference at a higher scope so both success and timeout paths can clean up:async load(): Promise<Event[]> { if (!this.seckey) throw new Error('Secret key is required for loading data') const events: Event[] = [] let timeoutHandler: ReturnType<typeof setTimeout> | undefined let sub: ReturnType<SimplePool['subscribeMany']> | undefined const loadPromise = new Promise<Event[]>((resolve) => { sub = this.pool.subscribeMany( this.relays, { kinds: [4], '#p': [this.pubkey], '#t': [nostrAppName] }, { onevent: (event: Event) => { try { const content = this.decryptEvent(event) events.push({ ...event, content }) } catch (error) { consoleError(error, 'Failed to decrypt event') } }, oneose: () => { if (sub) sub.close() if (timeoutHandler) clearTimeout(timeoutHandler) resolve(events) }, }, ) }) const timeoutPromise = new Promise<Event[]>((resolve) => { timeoutHandler = setTimeout(() => { consoleError(new Error('Load timeout'), 'Failed to load backup data') if (sub) sub.close() resolve([]) }, 10000) }) return Promise.race([loadPromise, timeoutPromise]) }
🧹 Nitpick comments (3)
playwright.config.ts (1)
16-22: Consider making thewebServer.commandenv setup cross‑platform
command: 'VITE_NOSTR_RELAY_URL=ws://localhost:10547 pnpm start'relies on POSIX shell syntax and will fail on Windows unless you always run tests in a POSIX environment. To make this portable, consider either:
- Using Playwright’s
webServer.env:webServer: { command: 'pnpm start', port: 3002, env: { VITE_NOSTR_RELAY_URL: 'ws://localhost:10547' }, }or
- Moving the env var into the
startscript via something likecross-env.src/test/e2e/nostr.test.ts (1)
3-90: Harden E2E selectors and waits to reduce flakinessThe flow this test covers is excellent, but a few selector patterns are quite brittle:
const nsec = await page.locator('p').nth(2).innerText()depends on the exact<p>order in “Backup and privacy”. Any content change can break this. Prefer a more specific locator (e.g., adata-testid, a label near the nsec, or a text pattern that clearly identifies the key).- Many steps use
getByText('Settings')and similar generic text; if multiple elements share the same label, Playwright may click the wrong one. Using roles or test IDs (e.g.,getByRole('button', { name: 'Settings' })) tends to be more stable.- The
page.waitForTimeout(1000)after resetting the wallet is a fixed delay. It’d be more robust to wait for a deterministic condition, such as the first onboarding screen becoming visible or a URL change.- For
await page.locator('ion-input[name="private-key"] input').fill(nsec), double‑check this works with Ionic’s Shadow DOM. In some setups you may need a>>combinator (e.g.,page.locator('ion-input[name="private-key"] >> input')) or a test ID on a higher‑level React component instead of reaching into the internal<input>.These changes should make the test less brittle as the UI evolves.
src/lib/nostr.ts (1)
5-8: Nostr relay/env wiring and nip44 usage look coherentUsing a single
nostrAppNametag andrelaysderived fromVITE_NOSTR_RELAY_URL(with a sensible default list) makes the storage layer predictable and easy to point at test relays. Likewise,save(payload: string)using:
- an ephemeral
sk/pkpair,nip44.getConversationKey(ephemeralSk, this.pubkey)on write, andnip44.getConversationKey(this.seckey, event.pubkey)on readis consistent with NIP‑44 DM semantics and matches the filters
{ kinds: [4], '#p': [this.pubkey], '#t': [nostrAppName] }.One minor nit: for log consistency with the rest of the codebase you might want to use
consoleErrorinstead of rawconsole.errorin the publish error path, but that’s purely cosmetic.Also applies to: 22-25, 50-58
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
playwright.config.ts(3 hunks)src/components/Input.tsx(2 hunks)src/components/InputAddress.tsx(3 hunks)src/components/InputWithScanner.tsx(3 hunks)src/lib/nostr.ts(5 hunks)src/screens/Init/Restore.tsx(3 hunks)src/screens/Wallet/Send/Form.tsx(1 hunks)src/test/e2e/nostr.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-21T18:25:36.391Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 121
File: src/screens/Init/Restore.tsx:58-58
Timestamp: 2025-07-21T18:25:36.391Z
Learning: In src/screens/Init/Restore.tsx, the Input component for private key restoration intentionally omits the `value` prop to make it uncontrolled. This is a security feature to prevent the private key from being displayed if users navigate away and return to the screen, avoiding potential exposure of sensitive data in the UI.
Applied to files:
src/components/Input.tsxsrc/screens/Wallet/Send/Form.tsxsrc/components/InputWithScanner.tsxsrc/components/InputAddress.tsxsrc/screens/Init/Restore.tsx
📚 Learning: 2025-08-06T09:53:28.428Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 134
File: src/screens/Settings/Password.tsx:34-39
Timestamp: 2025-08-06T09:53:28.428Z
Learning: In src/screens/Settings/Password.tsx, the saveNewPassword function doesn't need error handling for getPrivateKey because it's only called after authentication is verified. The useEffect validates the oldPassword using isValidPassword() which internally calls getPrivateKey(), and only sets authenticated=true if successful. The saveNewPassword function has a guard clause that returns early if not authenticated, so getPrivateKey(oldPassword) is guaranteed to work when called.
Applied to files:
src/screens/Init/Restore.tsx
🧬 Code graph analysis (2)
src/screens/Init/Restore.tsx (4)
src/lib/backup.ts (1)
BackupProvider(17-168)src/lib/logs.ts (1)
consoleError(40-44)src/components/Loading.tsx (1)
Loading(5-18)src/components/Input.tsx (1)
Input(13-40)
src/lib/nostr.ts (1)
src/lib/logs.ts (1)
consoleError(40-44)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (4)
src/screens/Wallet/Send/Form.tsx (1)
367-375: Addingname='send-address'toInputAddressis a safe enhancementWiring a
nameonto the recipientInputAddressimproves testability/identification and doesn’t change behavior, since the component just forwards this to the underlying input.src/components/Input.tsx (1)
8-8: LGTM! Clean prop threading for form accessibility.The addition of the optional
nameprop and its forwarding toIonInputis correct and maintains backward compatibility. This enhances form handling and accessibility without affecting existing behavior.Also applies to: 13-13, 32-32
src/components/InputWithScanner.tsx (1)
12-12: LGTM! Consistent prop forwarding.The
nameprop addition follows the same pattern asInput.tsxand is correctly threaded through to the underlyingIonInput. The changes integrate well with the existing props and functionality.Also applies to: 25-25, 51-51
src/components/InputAddress.tsx (1)
15-15: LGTM! Completes the prop propagation chain.The
nameprop is correctly added and forwarded toInputWithScanner, completing the propagation chain down to the native input element. The changes are consistent with the pattern established in the other input components.Also applies to: 27-27, 51-51
| const handleProceed = () => { | ||
| setInitInfo({ privateKey, password: defaultPassword, restoring: true }) | ||
| handleNostrRestore(privateKey!, updateConfig) | ||
| .then(() => navigate(Pages.InitSuccess)) | ||
| setRestoring(true) | ||
| new BackupProvider({ seckey: privateKey! }) | ||
| .restore(updateConfig) | ||
| .catch((err) => consoleError(err, 'Error restoring from nostr')) | ||
| .finally(() => { | ||
| setRestoring(false) | ||
| navigate(Pages.InitSuccess) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Avoid navigating to “success” when Nostr restore fails
BackupProvider({ seckey: privateKey! }).restore(updateConfig) can reject, but navigate(Pages.InitSuccess) is called in .finally, so you’ll show the success screen (and “Wallet restored successfully!”) even if restore failed or no backup was applied.
It would be safer to:
- Navigate to
Pages.InitSuccessonly on successful restore, and - On error, clear
restoring, surface an error message, and stay on this screen.
For example:
setRestoring(true)
new BackupProvider({ seckey: privateKey! })
.restore(updateConfig)
.then(() => {
setRestoring(false)
navigate(Pages.InitSuccess)
})
.catch((err) => {
consoleError(err, 'Error restoring from nostr')
setRestoring(false)
setError('Failed to restore from Nostr backup')
})You could also add a quick guard at the top of handleProceed:
if (restoring) returnto prevent double‑clicks from firing multiple restore requests.
Also applies to: 67-67
🤖 Prompt for AI Agents
In src/screens/Init/Restore.tsx around lines 53 to 63 (and also at line 67), the
current flow calls navigate(Pages.InitSuccess) in a .finally block which will
show success even when BackupProvider.restore rejects; change the promise
handling to call navigate only on successful .then, move setRestoring(false)
into both success and error branches (or use finally only after resolving
whether to navigate), and in the .catch log the error, clear restoring and call
setError('Failed to restore from Nostr backup') (or a suitable message) instead
of navigating; also add a guard at the top of handleProceed (if (restoring)
return) to prevent duplicate submissions.
| * @param options.pubkey - Optional public key (hex string). Required if seckey not provided. | ||
| * @throws Error if neither seckey nor pubkey is provided, or if pubkey format is invalid | ||
| */ | ||
| constructor(options: { pubkey?: string; seckey?: Uint8Array }) { |
There was a problem hiding this comment.
now I understand why we need arkade-os/ts-sdk#223
we need to think about it
|
|
||
| const dataSize = JSON.stringify(data).length | ||
|
|
||
| if (dataSize > 65000) { |
There was a problem hiding this comment.
Can't we backup 1 event / swap whatever the datasize ? easier to have only 1 unique format no ? also are we sure this 65000 is a constant, relay can't configure it ?
There was a problem hiding this comment.
I'm trying not to spam Nostr with a lot of events and also for fastest backup/restore.
For most cases 64KB should be enough.
We only control one relay, and we are saving the backup to 3 relays for redundancy, so we need to respect the limit.
| * @param event event to decrypt | ||
| * @returns string encrypted in the event | ||
| */ | ||
| private decryptEvent(event: Event): string { |
There was a problem hiding this comment.
it seems decryptData is only used here, it should become decryptEvent(Event) so we can remove this extra method
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/screens/Settings/Backup.tsx (1)
51-64: Still backing up when disabling; toast shown regardless of backup success.The previous review comment was only partially addressed. Issues remaining:
Line 62: When disabling Nostr backup,
backupConfig(newConfig)is called, which backs up the config (withnostrBackup=false) to Nostr. This is contradictory—if the user is disabling Nostr backups, we shouldn't be writing to Nostr at all.Line 64: The toast is shown unconditionally, even if the backup fails or is skipped. It should only be shown after a successful operation.
Line 55: The
BackupProviderconstructor can throw synchronously (e.g., if pubkey is invalid), but it's not wrapped in try/catch, so those errors won't be caught by the.catch()on Line 56 and won't set the error state.Apply this diff to fix the logic:
const toggleNostrBackup = async () => { const newConfig = { ...config, nostrBackup: !config.nostrBackup } - updateConfig(newConfig) - if (newConfig.nostrBackup) { - const backupProvider = new BackupProvider({ pubkey: config.pubkey }) - await backupProvider.fullBackup(newConfig).catch((error) => { - consoleError(error, 'Backup to Nostr failed') - setError('Backup to Nostr failed') - return - }) - } else { - backupConfig(newConfig) - } - present(backupToNostr) + try { + if (newConfig.nostrBackup) { + const backupProvider = new BackupProvider({ pubkey: config.pubkey }) + await backupProvider.fullBackup(newConfig) + } + updateConfig(newConfig) + setError('') + present(backupToNostr) + } catch (error) { + consoleError(error, 'Backup to Nostr failed') + setError('Backup to Nostr failed') + } }This ensures:
- No backup is performed when disabling
- Config is persisted only after successful backup (when enabling) or immediately (when disabling)
- All errors (constructor and async) are caught
- Toast is shown only on success
🧹 Nitpick comments (7)
test.docker-compose.yml (1)
255-262: Add missingrestartpolicy and consider health checks for consistency.The new
nakservice is missing therestart: unless-stoppedpolicy present in all other services, which could impact reliability during local test runs if the container unexpectedly exits. Additionally, depending on how Playwright e2e tests interact with this service, consider adding ahealthcheck(like other backend services) and/ordepends_ondeclarations in dependent services.Apply this diff to add the restart policy and align with service patterns:
nak: + restart: unless-stopped build: context: . dockerfile: nak.DockerfileIf other services in this compose file (such as UI tests or integration tests) require
nakto be ready before starting, declare that dependency explicitly in those services:some-test-service: depends_on: nak: condition: service_started # or service_healthy if you add a healthchecksrc/screens/Settings/Notifications.tsx (1)
14-28: Consider adding error handling for backup operations.The backup operations in both branches (lines 19 and 25) could fail, but there's no user-facing error handling. If
backupConfigrejects, the promise chain will continue and the config will still be updated. Consider wrapping the backup calls in try-catch blocks to provide user feedback if the backup fails.- const handleChange = async () => { + const handleChange = async () => { if (!notificationApiSupport) return if (!config.notifications) { requestPermission().then(async (notifications) => { const newConfig = { ...config, notifications } - if (config.nostrBackup) await backupConfig(newConfig) + if (config.nostrBackup) { + try { + await backupConfig(newConfig) + } catch (error) { + console.error('Backup failed:', error) + // Optionally show error to user + } + } if (notifications) sendTestNotification() updateConfig(newConfig) }) } else { const newConfig = { ...config, notifications: false } - if (config.nostrBackup) await backupConfig(newConfig) + if (config.nostrBackup) { + try { + await backupConfig(newConfig) + } catch (error) { + console.error('Backup failed:', error) + // Optionally show error to user + } + } updateConfig(newConfig) } }src/test/e2e/nostr.test.ts (1)
136-141: Potential race condition:execis not awaited.Line 137 uses
exec()without awaiting the command completion, while line 154 correctly usesexecAsync()with await. Although the test waits for the "Payment received!" message on line 140, which should handle the async nature of the payment, this inconsistency could lead to flaky tests if the payment processing is slow.Consider using
execAsyncconsistently for both invoice payments to ensure deterministic test execution.- // pay invoice with lnd - exec(`docker exec lnd lncli --network=regtest payinvoice ${receiveInvoice} --force`) + // pay invoice with lnd + await execAsync(`docker exec lnd lncli --network=regtest payinvoice ${receiveInvoice} --force`) // wait for payment received await page.waitForSelector('text=Payment received!')src/providers/config.tsx (1)
50-55: Consider validating pubkey before creating BackupProvider.Line 51 creates a
BackupProviderwithconfig.pubkey, but during initial wallet creation or reset, the pubkey might be empty or invalid. While the error is caught and logged (lines 52-54), the BackupProvider constructor will throw if the pubkey is invalid, which could cause confusion in error logs.Consider adding a guard to skip backup if pubkey is not set:
const backupConfig = async (config: Config) => { + if (!config.pubkey) { + console.warn('Backup skipped: pubkey not set') + return + } const backupProvider = new BackupProvider({ pubkey: config.pubkey }) await backupProvider.backupConfig(config).catch((error) => { consoleError(error, 'Backup to Nostr failed') }) }src/lib/backup.ts (3)
76-98: Remove redundant config check and clarify chunking strategy.Line 86 checks
if (config), butconfigis a required parameter of the function, making this check redundant. The chunking logic at line 85 uses a 65KB threshold, which was intentionally chosen as discussed in previous reviews.const dataSize = JSON.stringify(data).length if (dataSize > 65000) { - if (config) await this.backupConfig(config) + await this.backupConfig(config) for (const reverseSwap of data.reverseSwaps ?? []) { await this.backupReverseSwap(reverseSwap)
109-117: Use consistent logging utility instead of console.log.Lines 110 and 115 use
console.logdirectly, while the rest of the codebase uses theconsoleLogutility fromsrc/lib/logs.tsfor consistent log management. Consider updating these for consistency.+import { consoleError, consoleLog } from './logs' ... for (const swap of data?.reverseSwaps ?? []) { - console.log('restoring reverse swap:', swap.id) + consoleLog('restoring reverse swap:', swap.id) await contractRepo.saveToContractCollection('reverseSwaps', swap, 'id') } for (const swap of data?.submarineSwaps ?? []) { - console.log('restoring submarine swap:', swap.id) + consoleLog('restoring submarine swap:', swap.id) await contractRepo.saveToContractCollection('submarineSwaps', swap, 'id') }
136-160: Sort stability: events with identical timestamps may not have deterministic ordering.Line 137 sorts events by
created_at, but if multiple events share the same timestamp (e.g., during bulk backup operations), the sort order becomes non-deterministic. While the deduplication logic using Maps (lines 153-159) handles conflicts by keeping the last entry seen, the final result could vary across restores.Consider adding a secondary sort key (e.g., event.id) to ensure consistent ordering:
// Events are sorted by created_at to have a deterministic order. - const sorted = events.sort((a, b) => a.created_at - b.created_at) + const sorted = events.sort((a, b) => { + if (a.created_at !== b.created_at) return a.created_at - b.created_at + return a.id.localeCompare(b.id) + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
package.json(1 hunks)src/components/Balance.tsx(1 hunks)src/lib/backup.ts(1 hunks)src/lib/nostr.ts(5 hunks)src/providers/config.tsx(5 hunks)src/providers/lightning.tsx(2 hunks)src/providers/wallet.tsx(1 hunks)src/screens/Settings/Backup.tsx(3 hunks)src/screens/Settings/Display.tsx(1 hunks)src/screens/Settings/Fiat.tsx(1 hunks)src/screens/Settings/Notifications.tsx(1 hunks)src/screens/Settings/Server.tsx(2 hunks)src/screens/Settings/Theme.tsx(1 hunks)src/test/e2e/nostr.test.ts(1 hunks)src/test/setup.mjs(1 hunks)test.docker-compose.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/nostr.ts
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-08-06T09:53:28.428Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 134
File: src/screens/Settings/Password.tsx:34-39
Timestamp: 2025-08-06T09:53:28.428Z
Learning: In src/screens/Settings/Password.tsx, the saveNewPassword function doesn't need error handling for getPrivateKey because it's only called after authentication is verified. The useEffect validates the oldPassword using isValidPassword() which internally calls getPrivateKey(), and only sets authenticated=true if successful. The saveNewPassword function has a guard clause that returns early if not authenticated, so getPrivateKey(oldPassword) is guaranteed to work when called.
Applied to files:
src/screens/Settings/Backup.tsxsrc/screens/Settings/Server.tsx
📚 Learning: 2025-07-21T18:25:36.391Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 121
File: src/screens/Init/Restore.tsx:58-58
Timestamp: 2025-07-21T18:25:36.391Z
Learning: In src/screens/Init/Restore.tsx, the Input component for private key restoration intentionally omits the `value` prop to make it uncontrolled. This is a security feature to prevent the private key from being displayed if users navigate away and return to the screen, avoiding potential exposure of sensitive data in the UI.
Applied to files:
src/screens/Settings/Backup.tsxsrc/screens/Settings/Display.tsx
📚 Learning: 2025-06-30T18:33:29.839Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 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/providers/wallet.tsx
📚 Learning: 2025-07-07T14:41:35.991Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 106
File: src/lib/bolt11.ts:153-155
Timestamp: 2025-07-07T14:41:35.991Z
Learning: In src/lib/bolt11.ts, the code is copied from the original bitcoinjs/bolt11 library (https://github.com/bitcoinjs/bolt11/blob/master/payreq.js) and should be kept unchanged to maintain compatibility with the upstream source, even if there are potential improvements or syntax issues.
Applied to files:
package.json
📚 Learning: 2025-11-20T12:08:19.349Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 230
File: test.docker-compose.yml:46-46
Timestamp: 2025-11-20T12:08:19.349Z
Learning: The Docker image `boltz/boltz:ark` exists and is available for the arkade-os/wallet repository.
Applied to files:
package.jsontest.docker-compose.yml
🧬 Code graph analysis (10)
src/screens/Settings/Fiat.tsx (2)
src/providers/config.tsx (1)
ConfigContext(33-43)src/components/Select.tsx (1)
Select(12-43)
src/screens/Settings/Notifications.tsx (2)
src/providers/config.tsx (1)
ConfigContext(33-43)src/lib/notifications.ts (3)
notificationApiSupport(1-2)requestPermission(4-7)sendTestNotification(29-33)
src/providers/lightning.tsx (1)
src/providers/config.tsx (1)
ConfigContext(33-43)
src/screens/Settings/Backup.tsx (4)
src/providers/config.tsx (1)
ConfigContext(33-43)src/lib/backup.ts (1)
BackupProvider(17-168)src/lib/logs.ts (1)
consoleError(40-44)src/components/Error.tsx (1)
ErrorMessage(10-21)
src/lib/backup.ts (3)
src/lib/types.ts (1)
Config(8-23)src/lib/nostr.ts (1)
NostrStorage(9-141)src/lib/logs.ts (1)
consoleError(40-44)
src/test/e2e/nostr.test.ts (2)
src/test/e2e/utils.ts (1)
readClipboard(3-16)src/test/setup.mjs (1)
invoice(243-243)
src/screens/Settings/Server.tsx (1)
src/providers/config.tsx (1)
ConfigContext(33-43)
src/screens/Settings/Display.tsx (2)
src/providers/config.tsx (1)
ConfigContext(33-43)src/components/Select.tsx (1)
Select(12-43)
src/providers/config.tsx (3)
src/lib/types.ts (1)
Config(8-23)src/lib/backup.ts (1)
BackupProvider(17-168)src/lib/logs.ts (1)
consoleError(40-44)
src/screens/Settings/Theme.tsx (2)
src/providers/config.tsx (1)
ConfigContext(33-43)src/components/Select.tsx (1)
Select(12-43)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cloudflare Pages: wallet-bitcoin
- GitHub Check: test
🔇 Additional comments (14)
test.docker-compose.yml (1)
255-262: nak.Dockerfile verified as valid and properly formatted.The file exists at the repository root and contains valid Dockerfile syntax with no errors. The multi-stage build structure (golang:1.25.3-alpine → alpine:latest) is correct, all directives are syntactically valid, and the file is complete with proper ending. Security best practices are followed (non-root user, binary permissions).
src/components/Balance.tsx (1)
30-30: LGTM!The change correctly aligns with the refactored
updateConfigAPI that now accepts a single argument.src/screens/Settings/Display.tsx (1)
10-16: LGTM!The change correctly integrates backup handling with config updates. The pattern of conditionally backing up when
nostrBackupis enabled, then updating the config, is consistent with the new BackupProvider-based architecture.src/screens/Settings/Fiat.tsx (1)
10-16: LGTM!Consistent implementation of the backup-before-update pattern.
src/test/setup.mjs (1)
300-312: LGTM!The
setupNostrfunction follows the same pattern assetupBoltzand integrates cleanly into the test setup flow.src/screens/Settings/Theme.tsx (1)
10-16: LGTM!Consistent implementation matching the backup pattern in Display.tsx and Fiat.tsx.
src/providers/wallet.tsx (1)
256-256: LGTM!The change correctly updates to the new single-argument
updateConfigAPI.package.json (1)
7-7: No issues found. The package version is valid and has no known vulnerabilities.The verification confirms that
@arkade-os/boltz-swapversion 0.2.9 exists on npm and has no known security vulnerabilities.src/screens/Settings/Server.tsx (1)
54-68: LGTM! Backup timing is correct.The backup flow here is implemented correctly: backup is performed before
updateConfig, which ensures consistency. The try-catch block provides appropriate error handling for the entire operation.src/test/e2e/nostr.test.ts (1)
20-94: Well-structured E2E test for config backup/restore.This test thoroughly validates the config backup and restore flow by:
- Enabling backups and changing settings (EUR)
- Disabling backups and changing settings again (USD)
- Restoring from backup and verifying the EUR setting persists
The test design ensures that the restored config comes from Nostr backup and not from local persistence.
src/providers/config.tsx (1)
62-71: LGTM! Simplified updateConfig signature.Removing the backup parameter from
updateConfigand centralizing backup logic throughbackupConfigis a cleaner design. The function now has a single responsibility: updating and persisting the config to local storage.src/lib/backup.ts (2)
28-42: LGTM! Well-structured constructor with proper validation.The constructor correctly handles both seckey and pubkey initialization paths, validates inputs, and throws appropriate errors for invalid configurations. The pubkey normalization (removing '0x' prefix and length validation) is robust.
142-148: LGTM! Proper error handling for JSON parsing.The try-catch block around JSON parsing addresses the previous review concern about malformed events causing restore failures. The code now gracefully skips invalid events and logs errors using the appropriate utility.
src/providers/lightning.tsx (1)
73-78: Standardize backup timing and async handling across all settings screens.The review comment's observation is verified as accurate. The ordering and async handling of
backupConfigis inconsistent:
- lightning.tsx (line 77): calls
backupConfigwithoutawaitafterupdateConfig- Server.tsx (line 60), Notifications.tsx (lines 19, 25): call
backupConfigwithawaitbeforeupdateConfig- Display.tsx, Theme.tsx, Fiat.tsx: all follow the backup-first pattern with
awaitThe difference matters: lightning.tsx doesn't wait for the backup to complete before the function returns, whereas other screens wait for the backup operation to finish. This could lead to the backup running asynchronously in the background without error handling synchronization.
Decide whether the async-without-await pattern in lightning.tsx is intentional (fire-and-forget backup) or should be standardized with the rest of the codebase. If standardizing, align both the ordering (backup before update) and async handling (
awaitkeyword) across all files.
* Add ChatWoot integration for customer support - Added Support settings page under Settings menu - Implemented ChatWoot widget integration with secure chat functionality - Users can now report bugs and track support conversations from the wallet - Added useChatwoot hook for managing widget lifecycle - Support page displays when ChatWoot is configured via environment variables - Added VITE_CHATWOOT_WEBSITE_TOKEN and VITE_CHATWOOT_BASE_URL to configuration - Integration includes user identification and custom attributes (wallet pubkey, network) * Include wallet addresses in ChatWoot support integration - Added ARK address (offchainAddr) to ChatWoot custom attributes - Added BTC boarding address (boardingAddr) to custom attributes - Included explorer URL for the current network - Support operators can now search user addresses in the blockchain explorer - Addresses are automatically fetched when support page is opened * Add comprehensive service URLs and addresses to ChatWoot - Added location origin (window.location.origin) for deployment context - Added ARK server URL and indexer URL (both use aspInfo.url) - Added wallet ARK address (offchainAddr) and BTC boarding address - Added blockchain explorer URL for the user's network - Added Boltz Lightning swap provider URL - Added LendaSat and LendaSwap application URLs - Support operators can now see all service endpoints and addresses - Enables quick troubleshooting by showing user's complete configuration * add chatwoot support * deal with error loading chatwoot * Add safety check before using non-null assertion * Fix the timeout logic bug * console.log to debug * console.log for debug * increase menu row height * remove debug console logs * increase height of menus --------- Co-authored-by: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
src/test/e2e/swap.test.ts (1)
19-19: Consider using a more specific locator.The tests now use
getByText('Boltz')instead of the previousgetByRole('heading', { name: 'Boltz' }). While this works given the UI changes, text-based locators are less specific and don't follow accessibility testing best practices.Consider using a more specific locator if the Header component renders with identifiable attributes. For example:
-await expect(page.getByText('Boltz')).toBeVisible() +await expect(page.getByRole('banner').getByText('Boltz')).toBeVisible()Or add a test ID to the Header component for more robust testing:
-await expect(page.getByText('Boltz')).toBeVisible() +await expect(page.getByTestId('app-header')).toContainText('Boltz')Also applies to: 80-80, 143-143, 210-210
src/components/Select.tsx (1)
30-40: Consistent spacing; consider adding effect dependenciesThe padding change on the option row matches the rest of the settings UI and is fine. To avoid a stale closure if
optionsoronChangecan change over time, you might optionally include them in the effect deps:- }, [selected]) + }, [selected, options, onChange])src/icons/Support.tsx (1)
1-10: SupportIcon SVG component looks goodThe icon component is straightforward and consistent with typical inline SVG usage. If this icon is purely decorative and other icons follow suit, you could optionally add
aria-hidden='true'/focusable='false'to reduce noise for screen readers, but it’s not required.src/components/ChatWoot.tsx (1)
1-25: Chatwoot widget integration and cleanup look solidThe initialization flow (env vars → settings → script injection) and script cleanup on unmount are well-contained in this component. If there’s any chance this widget could mount more than once during an app session, you might optionally short‑circuit before
injectAndRunChatwootScriptwhen a matching SDK<script>tag already exists to avoid duplicate loads, but it’s not blocking.src/screens/Settings/Support.tsx (2)
60-60: Potential stale closure with timeout callback.The useEffect has an empty dependency array, but the timeout callback on line 48 references
supportChatLoaded. While the current logic likely works (since the timeout is cleared when the event fires), this is fragile—if the effect were refactored or the state update order changed, the stale closure could cause the error to be set incorrectly.Consider using a ref for the timeout flag or restructuring to avoid the stale closure:
useEffect(() => { if (!hasChatwootVars()) { setError('Support chat is not configured') return } + let loaded = false const loadTimeout = setTimeout(() => { - if (!supportChatLoaded) setError('Failed to load support chat') + if (!loaded) setError('Failed to load support chat') }, 5_000) const eventHandler = () => { + loaded = true clearTimeout(loadTimeout) setSupportChatLoaded(true) window.$chatwoot?.toggleBubbleVisibility('hide') } const event = 'chatwoot:ready' window.addEventListener(event, eventHandler) - return () => window.removeEventListener(event, eventHandler) + return () => { + window.removeEventListener(event, eventHandler) + clearTimeout(loadTimeout) + } }, [])
67-68: User identifier truncation could cause collisions (low risk).Using
wallet.pubkey.substring(0, 16)as the user identifier creates a theoretical collision risk if multiple users share the first 16 characters of their pubkey. While this is extremely unlikely with cryptographic keys, consider using the full pubkey or a hash if Chatwoot's identifier length allows it.src/lib/chatwoot.ts (2)
56-59: Incomplete JSDoc comment.The JSDoc comment block is present but empty. Either document the function (parameters, return value, behavior) or remove the empty comment block.
Example documentation:
/** - * - * @param settings - */ + * Dynamically injects the Chatwoot SDK script and initializes it with the provided configuration. + * Polls for SDK availability up to 5 seconds before timing out. + * @param vars - Chatwoot configuration containing websiteToken and baseUrl + */ export const injectAndRunChatwootScript = (vars: ChatwootVars) => {
67-70: Consider handling missing parent node more gracefully.The function logs an error and returns early if the parent node is missing, but this leaves the Chatwoot feature silently broken. Consider propagating the error or notifying the user that chat support is unavailable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
README.md(1 hunks)src/components/ChatWoot.tsx(1 hunks)src/components/Menu.tsx(1 hunks)src/components/Select.tsx(1 hunks)src/icons/Support.tsx(1 hunks)src/lib/chatwoot.ts(1 hunks)src/lib/types.ts(1 hunks)src/providers/options.tsx(2 hunks)src/screens/Apps/Boltz/Index.tsx(1 hunks)src/screens/Settings/General.tsx(1 hunks)src/screens/Settings/Index.tsx(2 hunks)src/screens/Settings/Support.tsx(1 hunks)src/test/e2e/swap.test.ts(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- README.md
🧰 Additional context used
🧬 Code graph analysis (7)
src/screens/Settings/General.tsx (1)
src/components/FlexRow.tsx (1)
FlexRow(17-48)
src/components/Select.tsx (1)
src/components/FlexRow.tsx (1)
FlexRow(17-48)
src/screens/Settings/Support.tsx (15)
src/providers/asp.tsx (1)
AspContext(15-20)src/providers/config.tsx (1)
ConfigContext(33-43)src/providers/lightning.tsx (1)
LightningContext(24-30)src/providers/wallet.tsx (1)
WalletContext(42-56)src/lib/types.ts (1)
Addresses(3-6)src/lib/asp.ts (1)
getReceivingAddresses(144-150)src/lib/chatwoot.ts (1)
hasChatwootVars(35-38)src/lib/explorers.ts (1)
getWebExplorerURL(38-40)src/components/FlexCol.tsx (1)
FlexCol(15-40)src/components/Text.tsx (2)
Text(25-79)TextSecondary(91-97)src/components/Content.tsx (1)
Content(9-16)src/components/Padded.tsx (1)
Padded(7-14)src/components/Error.tsx (1)
ErrorMessage(10-21)src/components/ButtonsOnBottom.tsx (1)
ButtonsOnBottom(10-26)src/components/Button.tsx (1)
Button(21-64)
src/providers/options.tsx (1)
src/icons/Support.tsx (1)
SupportIcon(1-10)
src/components/ChatWoot.tsx (1)
src/lib/chatwoot.ts (3)
getChatwootVars(28-33)getChatwootSettings(47-54)injectAndRunChatwootScript(60-89)
src/screens/Settings/Index.tsx (1)
src/screens/Settings/Support.tsx (1)
Support(21-136)
src/lib/chatwoot.ts (1)
src/lib/logs.ts (1)
consoleError(40-44)
🔇 Additional comments (7)
src/screens/Apps/Boltz/Index.tsx (1)
33-33: LGTM!The explicit header text consolidates the Boltz label into the Header component, improving consistency and eliminating duplicate headings.
src/components/Menu.tsx (1)
19-29: Menu row padding tweak looks goodThe increased vertical padding improves touch targets and keeps styling consistent with other settings rows; no behavioral impact.
src/screens/Settings/General.tsx (1)
17-28: General settings row spacing is consistentThe updated padding aligns this row with other settings components and improves click/tap area without changing behavior.
src/screens/Settings/Index.tsx (1)
1-44: Support screen wiring is correctImporting
Supportand rendering it whenSettingsOptions.Supportis selected matches the existing pattern for other settings screens and should integrate cleanly with the options provider.src/lib/types.ts (1)
45-63: New SettingsOptions.Support enum entry is consistentAdding
Support = 'support'follows the existing naming and string conventions for settings options and cleanly supports the new Support screen and menu entry.src/providers/options.tsx (1)
10-77: Support option registration is wired correctlyImporting
SupportIconand adding aSettingsOptions.Supportentry in the General section keeps the options list coherent and ensures the new Support screen is discoverable from the main settings menu.src/lib/chatwoot.ts (1)
72-87: Review timeout and polling logic.The polling interval is set to 100ms with a 5-second timeout (50 iterations). This is reasonable, but ensure this timeout aligns with the timeout in Support.tsx (line 49, also 5 seconds). If both timeouts race, the user might see inconsistent error states.
Verify that the timeout coordination between
injectAndRunChatwootScript(line 82) and the Support component's timeout (Support.tsx line 47-49) doesn't cause race conditions or duplicate error messages. Both use 5 seconds, but they may not start at exactly the same time.
| lendaswap_url: import.meta.env.VITE_LENDASWAP_IFRAME_URL || 'not available', | ||
| explorer_url: wallet.network ? getWebExplorerURL(wallet.network as NetworkName) : 'not available', | ||
| }) | ||
| }, [addresses, supportChatLoaded, wallet.pubkey, window.$chatwoot]) |
There was a problem hiding this comment.
Invalid dependency in useEffect array.
window.$chatwoot is not a reactive value that React can track. Including it in the dependency array serves no purpose and may trigger linter warnings. Since window.$chatwoot becomes available when supportChatLoaded is set to true, the existing dependency on supportChatLoaded is sufficient.
Apply this diff:
- }, [addresses, supportChatLoaded, wallet.pubkey, window.$chatwoot])
+ }, [addresses, supportChatLoaded, wallet.pubkey])📝 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.
| }, [addresses, supportChatLoaded, wallet.pubkey, window.$chatwoot]) | |
| }, [addresses, supportChatLoaded, wallet.pubkey]) |
🤖 Prompt for AI Agents
In src/screens/Settings/Support.tsx around line 85, the useEffect dependency
array incorrectly includes window.$chatwoot (a non-reactive global); remove
window.$chatwoot from the dependency array and keep addresses,
supportChatLoaded, and wallet.pubkey only so React and the linter track real
reactive values—update the array to [, addresses, supportChatLoaded,
wallet.pubkey] (or the correct ordering your code uses) and run the linter to
confirm no remaining warnings.
* refactor nostr backups to use chunks * Add error handling around JSON parsing * fix enabling Nostr backup doesn’t persist config change * use consoleError instead of console.error * new regtest service: nak * update docker images * adds nostr test * fixes * tidy code * Chatwoot (arkade-os#236) * Add ChatWoot integration for customer support - Added Support settings page under Settings menu - Implemented ChatWoot widget integration with secure chat functionality - Users can now report bugs and track support conversations from the wallet - Added useChatwoot hook for managing widget lifecycle - Support page displays when ChatWoot is configured via environment variables - Added VITE_CHATWOOT_WEBSITE_TOKEN and VITE_CHATWOOT_BASE_URL to configuration - Integration includes user identification and custom attributes (wallet pubkey, network) * Include wallet addresses in ChatWoot support integration - Added ARK address (offchainAddr) to ChatWoot custom attributes - Added BTC boarding address (boardingAddr) to custom attributes - Included explorer URL for the current network - Support operators can now search user addresses in the blockchain explorer - Addresses are automatically fetched when support page is opened * Add comprehensive service URLs and addresses to ChatWoot - Added location origin (window.location.origin) for deployment context - Added ARK server URL and indexer URL (both use aspInfo.url) - Added wallet ARK address (offchainAddr) and BTC boarding address - Added blockchain explorer URL for the user's network - Added Boltz Lightning swap provider URL - Added LendaSat and LendaSwap application URLs - Support operators can now see all service endpoints and addresses - Enables quick troubleshooting by showing user's complete configuration * add chatwoot support * deal with error loading chatwoot * Add safety check before using non-null assertion * Fix the timeout logic bug * console.log to debug * console.log for debug * increase menu row height * remove debug console logs * increase height of menus --------- Co-authored-by: Claude <noreply@anthropic.com> * Fix UI incoherence on Boltz app page --------- Co-authored-by: Claude <noreply@anthropic.com>
Summary by CodeRabbit
Dependencies
New Features
Tests
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.