Repository navigation
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughRefactors swap/wallet persistence to use injected repository abstractions, replaces ArkadeLightning with a ServiceWorker-based lightning client, converts swap subscription APIs to async unsubscribe promises, updates package dependency sources and pnpm settings, and wires repositories into service worker MessageBus initialization. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Client UI
participant SW as ServiceWorker
participant MB as MessageBus
participant WR as IndexedDbWalletRepository
participant SR as IndexedDbSwapRepository
participant LH as ArkadeLightningHandler
rect rgba(200,220,255,0.5)
App->>SW: register / init ServiceWorker
SW->>MB: create MessageBus(tickIntervalMs=5000)
MB->>WR: wire wallet repo
MB->>SR: wire swap repo
MB->>LH: attach lightning message handler
SW-->>App: ready
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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-mutinynet with
|
| Latest commit: |
afc84f1
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a0a5ab1f.arkade-wallet.pages.dev |
| Branch Preview URL: | https://use-sw-2.arkade-wallet.pages.dev |
Deploying wallet-bitcoin with
|
| Latest commit: |
afc84f1
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e44f1ede.wallet-bitcoin.pages.dev |
| Branch Preview URL: | https://use-sw-2.wallet-bitcoin.pages.dev |
|
@coderabbitai please review - it's still a work in progress where we test the latest updates from |
|
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/backup.ts (1)
74-97:⚠️ Potential issue | 🟠 Major
fullBackupno longer backs up swaps — functional regression.With the swap collections commented out (lines 77-79),
fullBackuponly backs up config. The iteration overdata.reverseSwapsanddata.submarineSwaps(lines 87-93) is dead code since those fields are alwaysundefined. Users enabling Nostr backups will silently lose swap backup data.Since
swapRepositoryis now available as an injected dependency, you can resolve this by retrieving swaps from the repository before storing them. Verify the actual method signatures onSwapRepository(from@arkade-os/boltz-swap) to wire up the appropriate retrieval methods—the exact API names need to be confirmed against the package.src/providers/wallet.tsx (1)
238-245:⚠️ Potential issue | 🟠 Major
setIntervalis never cleared — intervals accumulate on retries and across re-mounts.Each invocation of
initSvcWorkerWalletregisters a new 1-second polling interval that is never cleared. On service-worker activation retries (Lines 261-267), this stacks up additional intervals. There's also no cleanup on component unmount.Store the interval ID and clear it on cleanup/retry:
Sketch of a fix
One approach: store the interval ID in a ref and clear it during cleanup or before re-init. For example, return the interval ID from
initSvcWorkerWalletso the calling effect can clear it, or manage it via a ref:+ const statusIntervalRef = useRef<ReturnType<typeof setInterval> | null>(null) + // inside initSvcWorkerWallet, before setting the interval: + if (statusIntervalRef.current) clearInterval(statusIntervalRef.current) - setInterval(async () => { + statusIntervalRef.current = setInterval(async () => { try { const { walletInitialized } = await svcWallet.getStatus() setInitialized(walletInitialized) } catch (err) { consoleError(err, 'Error pinging wallet status') } }, 1_000)And clean up in the component's unmount effect.
🤖 Fix all issues with AI agents
In `@src/components/SwapsList.tsx`:
- Around line 122-144: The subscription can leak if the component unmounts
before swapManager.onSwapUpdate resolves; add a cancelled flag to the effect:
set let cancelled = false before calling swapManager.onSwapUpdate, and in the
.then((unsubscribe) => { if (cancelled) { call unsubscribe immediately } else {
unsub = unsubscribe } }); update the cleanup to set cancelled = true and call
unsub?.() so any late-resolved unsubscribe is either stored for cleanup or
invoked right away; reference swapManager.onSwapUpdate, unsub, and
setSwapHistory to locate the code to change.
In `@src/lib/indexer.ts`:
- Around line 10-15: The constructor currently accepts a walletRepository but
never uses it; either remove the unused constructor parameter or switch the
class to use WalletRepository instead of localStorage for persistence. If
choosing to use the repository, replace direct localStorage access in methods
that read/write wallet data (references to localStorage at lines around where
wallets are loaded/saved) with calls to the injected walletRepository (e.g., add
or use methods on WalletRepository such as getWallets/saveWallets or similar),
and remove any direct localStorage references; if choosing to remove it, delete
the walletRepository parameter from the constructor signature and any related
private field to avoid unused injection.
- Line 3: The import of WalletRepository from '../../../ts-sdk/src/repositories'
is a fragile local filesystem reference and the constructor parameter
walletRepository in the class is unused while methods directly use localStorage
(which also fails in Service Worker/CI). Replace the relative import with the
published package import for the SDK's repositories (use the package name used
in package.json), then either remove the unused constructor parameter
walletRepository or refactor the class methods (e.g., wherever localStorage is
accessed) to call walletRepository methods (save/load/delete) so persistence
goes through the repository API and not localStorage; ensure code no longer
depends on browser localStorage so it works in Service Worker and CI.
- Around line 27-38: The Indexer class currently injects walletRepository but
persists commitment IDs using localStorage in getCommitmentTxIds and
setCommitmentTxIds (using STORAGE_KEY); either remove the unused
walletRepository parameter from the Indexer constructor and any related fields
if you intend to keep main-thread-only localStorage persistence, or replace the
localStorage usage in getCommitmentTxIds and setCommitmentTxIds with the
walletRepository's get/set methods (use the repository's equivalent persistence
API to read/write the STORAGE_KEY-backed map) so the injected walletRepository
is actually used and consistent with Service Worker-compatible storage.
In `@src/providers/lightning.tsx`:
- Around line 90-109: The effect currently captures a stale arkadeLightning
value so the cleanup never disposes the created ServiceWorkerArkadeLightning
instance; fix it by tracking the instance in a ref (e.g., arkadeRef via useRef)
and use that ref for lifecycle ops: when
ServiceWorkerArkadeLightning.create(...) resolves, assign the instance to
arkadeRef.current and still call setArkadeLightning(instance); in the effect
cleanup (and before creating a new instance) check arkadeRef.current and call
arkadeRef.current.dispose().catch(...) and then null out arkadeRef.current to
ensure the created instance is always disposed on unmount or deps change (also
handle disposing any previous instance before assigning a new one).
In `@src/screens/Apps/Boltz/Swap.tsx`:
- Around line 37-52: The effect currently leaks subscriptions if the component
unmounts before subscribeToSwapUpdates resolves; update the useEffect around
swapManager.subscribeToSwapUpdates (and the local unsub variable) to use a
cancelled flag: set let cancelled = false before calling subscribeToSwapUpdates,
and in the .then((unsubscribe) => { ... }) handler, if cancelled call
unsubscribe() immediately, else assign unsub = unsubscribe; in the cleanup set
cancelled = true and call unsub?.() so any resolved promise either gets cleaned
up immediately or assigns a safe unsub reference for cleanup. Ensure you
reference swapManager, subscribeToSwapUpdates, swapInfo.id and unsub in the
change.
In `@src/wallet-service-worker.ts`:
- Around line 13-17: The worker instantiation currently hardcodes debug: true
which will produce noisy logs in production; change the MessageBus options to
derive debug from an environment/config flag instead (e.g., use
process.env.WORKER_DEBUG or check process.env.NODE_ENV !== 'production'), parse
the value to a boolean, and pass that boolean to the MessageBus constructor
where debug is currently set (the object passed to new MessageBus in
src/wallet-service-worker.ts that also sets messageHandlers, tickIntervalMs).
Ensure the new behavior defaults to false in production when the env var is
unset or explicitly set to "false".
🧹 Nitpick comments (6)
package.json (1)
7-8: Non-reproducible dependency references pinned to branch tips.Both
@arkade-os/boltz-swapand@arkade-os/sdkpoint to#nextbranches, so every install can resolve a different commit. This is fine for WIP testing (as noted), but before merging tonext, consider pinning to a specific commit SHA or a published version to ensure reproducible builds.src/lib/backup.ts (1)
108-109: TODO left in restore path — ensure parity with backup.The comment "restore as contracts via ad-hoc utils in boltz-swap" suggests the restore path is also incomplete. While
saveSwapcalls on Lines 111/115 do persist individual swaps, confirm this is sufficient for the full restore flow (e.g., are contract-level semantics needed beyond saving raw swap objects?).Would you like me to open an issue to track the remaining TODO items in this file?
src/providers/config.tsx (1)
53-53: Each call site creates a newIndexedDbSwapRepositoryinstance.
backupConfigis called on every config change when Nostr backup is enabled, and each invocation creates a freshIndexedDbSwapRepository(plus a freshBackupProvider). The repository isn't even used in thebackupConfigcode path — it's only needed for restore.Consider sharing a single repository instance (e.g., via context or a module-level singleton) across
config.tsx,Backup.tsx, andRestore.tsxto avoid redundant IndexedDB setup and improve consistency.src/providers/wallet.tsx (2)
197-210: Migration errors are silently swallowed — consider surfacing them to the user.The migration
try/catchlogs the error but the user has no indication that their data may not have been fully migrated. For a WIP this is acceptable, but before merging you may want to track migration status (e.g., a flag/toast) so users can retry or be informed.
104-104: A newIndexeris created on every effect execution.
new Indexer(aspInfo, svcWallet.walletRepository)is re-instantiated each time the dependencies[initialized, vtxos, svcWallet, aspInfo]change. The constructor is cheap (stores references), so this is not a performance problem today, but memoizing theIndexerinstance (e.g., viauseMemo) would avoid unnecessary object creation and would make the code clearer about instance identity.src/providers/lightning.tsx (1)
99-99: Inconsistent error handler:.catch(console.error)bypasses the app's logging system.Line 99 uses
console.errordirectly, while the rest of the file (and codebase) usesconsoleErrorfrom../lib/logsfor structured logging.Proposed fix
- .catch(console.error) + .catch((err) => consoleError(err, 'Failed to create ServiceWorkerArkadeLightning'))
|
@coderabbitai review again, I've addressed your comments in the last commit |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/lib/indexer.ts`:
- Around line 23-27: getCommitmentTxIds currently calls JSON.parse on
localStorage data which will throw if the blob is corrupted; wrap the parse in a
try/catch and fall back to an empty object (and optionally log the parse error)
so callers like getAndUpdateCommitmentTxCreatedAt don't crash; do the same
defensive parsing in setCommitmentTxIds (and validate that the parsed value is
an object and that map[txid] values are numbers before returning, otherwise
treat as empty) and ensure you always call JSON.stringify on the safe object
when writing back to localStorage.
In `@src/providers/lightning.tsx`:
- Around line 90-119: The effect never flips the cancelled flag, causing
ServiceWorkerArkadeLightning.create() to set state after teardown; in the
cleanup (the returned function) set cancelled = true before attempting disposal
and then, if disposeArkadeLightning is non-null, call it; this ensures the
.then() branch checks cancelled and disposes the instance instead of calling
setArkadeLightning on an unmounted/stale component (refer to cancelled,
disposeArkadeLightning, ServiceWorkerArkadeLightning.create,
setArkadeLightning).
🧹 Nitpick comments (3)
src/lib/indexer.ts (1)
13-21: Naming mismatch: method name says "Ids" but it deals with a singlecreatedAttimestamp.
getCommitmentTxIds/setCommitmentTxIdsactually get/set acreatedAtvalue for a given txid, not a collection of IDs. Consider renaming to something likegetCachedCreatedAt/setCachedCreatedAtto avoid confusion.src/providers/lightning.tsx (2)
109-109: Inconsistent error handler:console.errorvsconsoleError.Line 109 uses
console.errordirectly, while the rest of the file (lines 103, 118, 128, etc.) uses the project'sconsoleErrorwrapper which also writes to the structured log. This means creation failures won't appear in the app's log.Proposed fix
- .catch(console.error) + .catch((err) => consoleError(err, 'Failed to create ServiceWorkerArkadeLightning'))
103-105: Minor: double error catch on dispose.Line 105 wraps
instance.dispose()with.catch(consoleError), and then line 118 callsdisposeArkadeLightning().catch(consoleError)again. The outer catch is redundant since the inner one already swallows errors. Not a bug, but worth simplifying for clarity.Proposed fix — let only the call-site handle the error
- disposeArkadeLightning = () => instance.dispose().catch(consoleError) + disposeArkadeLightning = () => instance.dispose()
* feat(haptics): add cross-platform haptic feedback system Add haptics utility with three intensity levels (tap/light/subtle) that works across Android (vibrate API) and iOS 18+ (checkbox switch hack). Integrate haptic feedback into 12 interactive components: buttons, toggles, checkboxes, list items, copy actions, settings rows, and navigation. Add haptics toggle in Settings > General so users can disable vibration. Wire haptics enabled state through config provider. Also adds @vitejs/plugin-basic-ssl and host:true for mobile HTTPS testing. * style: fix prettier formatting * fix: harden iOS detection and add aria-label to settings rows Add iPadOS detection (MacIntel + maxTouchPoints > 1) and guard navigator access for non-browser contexts in haptics utility. Add descriptive aria-label to settings row Focusable wrappers. Addresses CodeRabbit review feedback on #325. * fix: make basicSsl conditional on HTTPS env var Playwright e2e tests in CI connect via HTTP but the dev server was unconditionally using HTTPS via basicSsl(), causing connection failures. Only enable basicSsl() when HTTPS=true is set in the environment. * feat(button): add 3D press effect with per-variant shadow colors (#326) Replace flat box-shadow with depth-accurate per-variant shadow colors: dark purple (#1a0b4a) under purple buttons, dark red (#6b0e0e) under red, subtle rgba(0,0,0,0.10) under secondary/outline. Add border-radius to outer ion-button so shadow follows rounded corners. Add pressed state via touch/mouse events (not :active — unreliable on mobile): translateY(4px) with shadow collapse for solid buttons, scale(0.97) for clear buttons. Suppress Ionic's internal ripple, focus ring, and activated state overrides. Includes prefers-reduced-motion media query to disable transitions. Co-authored-by: Sahil Chaturvedi <sahilc0@users.noreply.github.com> * style: fix prettier formatting in ionic.css * chore: add .claude and CLAUDE.md to .gitignore * fix: address coderabbit review feedback - Add blank lines between custom props and declarations (stylelint) - Memoize handleClick with useCallback in Logo --------- Co-authored-by: Sahil Chaturvedi <sahilc0@users.noreply.github.com>
* feat(ui): tab icon pop animation and logo tap target Add pop animation to tab bar icons on tap using CSS keyframes. AnimatedTabIcon component triggers scale animation on tab selection with spring easing and prefers-reduced-motion support. Increase logo tap target from ~47px to 71px using invisible padding (18px around the 35px icon) so the easter egg is easier to activate. Files: src/App.tsx, src/index.css, src/icons/Logo.tsx * style: fix prettier formatting in tab animation keyframes * style: run prettier formatting * fix(ui): add comment explaining reflow trick in tab animation * nit from rabbit --------- Co-authored-by: Sahil Chaturvedi <sahilc0@users.noreply.github.com> Co-authored-by: Marco Argentieri <3596602+tiero@users.noreply.github.com> Co-authored-by: João Bordalo <bordalix@users.noreply.github.com>
* Set a maxheight restriction on announcement modal and display adjustments to ensure responsiveness * Adjusted file formatting + fixed comment * Changed 'margin' to 'marginTop' and removed redundant styling
* feat(ui): animate loading bar with traveling wave Replace the static pixel-art battery SVG with a smooth traveling wave animation. Each of the 6 segments oscillates between 30-100% fill height using eased sine waves with phase offsets, creating a fluid wave effect. - Use requestAnimationFrame + refs for 60fps without React re-renders - Apply ease-in-out-cubic to raw sine for natural motion - Respect prefers-reduced-motion via useReducedMotion hook - Add cloudflare tunnel host to vite allowedHosts for mobile testing Files: src/icons/LoadingBar.tsx, vite.config.ts * style: run prettier formatting * fix(build): replace framer-motion import with inline useReducedMotion hook framer-motion is not a project dependency. Replace the import with a lightweight inline hook using window.matchMedia to detect the prefers-reduced-motion preference. --------- Co-authored-by: Sahil Chaturvedi <sahilc0@users.noreply.github.com>
Summary by CodeRabbit
Chores
Refactor
Bug Fixes