feat(wallet): redesign home and settings entry points#627
Conversation
Port the scoped home redesign foundation and bitcoin-only wallet home from the draft branch. Adds shared motion and haptics helpers, redesigned home header/hero/actions/assets/upsells/activity preview, hides bottom navigation for home-launched wallet flows, and wires scan/swap interactions without pulling in passkeys or prototype asset accounts.
Remove the bottom nav from the redesigned wallet flow and align the settings screens with the draft redesign, including the Arkade Mint entry under General. Refresh the full Activity page before showing an empty state, keep the home activity preview hidden when empty, and use the shared secondary/drawer styling for the swap coming-soon sheet.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRefactors the wallet home and portfolio UI, adds privacy-masked balance components and portfolio hooks, migrates haptics to web-haptics, introduces token logos and icon updates, adds replace()-based navigation and dev auto-init boot/watchdog plumbing, updates animations and styles, and centralizes E2E navigation helpers and tests. ChangesWallet Home & Portfolio Redesign
Estimated code review effort: 🎯 4 (Complex) | ⏱️ ~75 minutes
✨ Finishing Touches🧪 Generate unit tests (beta)
|
|
@bordalix heads up for QA: we need a manual pass on Boltz swaps to make sure receives still work, and on Arkade Mint now that it lives in Settings. Please verify Arkade Mint works properly and that assets show up in the wallet after they are minted. |
Deploying tmp-boltz-upstream-mainnet-arkade-wallet with
|
| Latest commit: |
3e22f56
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://0bdc8e04.tmp-boltz-upstream-mainnet-arkade-wallet.pages.dev |
| Branch Preview URL: | https://wt-wallet-redesign-followups.tmp-boltz-upstream-mainnet-arkade-wallet.pages.dev |
Deploying wallet-bitcoin with
|
| Latest commit: |
3e22f56
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://8c7e1b73.wallet-bitcoin.pages.dev |
| Branch Preview URL: | https://wt-wallet-redesign-followups.wallet-bitcoin.pages.dev |
Deploying wallet-mutinynet with
|
| Latest commit: |
3e22f56
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e9861269.arkade-wallet.pages.dev |
| Branch Preview URL: | https://wt-wallet-redesign-followups.arkade-wallet.pages.dev |
![arkanaai[bot]](https://avatars.githubusercontent.com/u/170820085?s=60&v=4){kind=small}
There was a problem hiding this comment.
Arkana Code Review — PR #627
Verdict: Approve — This is a UI-only redesign (home screen, settings, nav). No protocol-critical code (VTXO handling, signing, forfeit, round lifecycle, exit paths) is modified. SDK bumps (0.4.24→0.4.27, boltz 0.3.28→0.3.32) are backward-compatible (HD receive rotation, WS resilience, VTXO reconciliation fixes).
Issues (non-blocking)
1. Stale closure risk in Send/Form.tsx scan effect
src/screens/Wallet/Send/Form.tsx:122-129 — The useEffect depends on [sendInfo.scan] but spreads the full sendInfo object. If other sendInfo fields change between renders without scan changing, the spread captures stale state. In practice this fires once on mount then deletes the flag, so it's low-risk, but the ESLint exhaustive-deps rule will flag it. Consider destructuring or adding sendInfo to deps.
2. SVG clipPath ID collision in TokenLogo.tsx
src/components/TokenLogo.tsx:63,80 — UnitedStatesFlagLogo and BrazilFlagLogo use hardcoded clipPath IDs (us-flag-circle, br-flag-circle). If multiple instances of the same flag render on screen (e.g. two USD asset rows), the duplicate IDs will cause the second SVG to reference the first's clip region. Fix: use useId() or inline the clip path.
3. BigInt conversion fragility in AssetCard.tsx
src/components/AssetCard.tsx:31 — BigInt(balance) will throw for non-integer numbers. The balance prop accepts bigint | number but BigInt() rejects floats. If any consumer passes a fractional number, this crashes. Consider BigInt(Math.round(balance)) or tightening the type.
4. DOM query for scroll tracking is fragile
src/screens/Wallet/Index.tsx:82 — document.querySelector('.wallet-home-content') couples scroll tracking to a CSS class name. A ref passed to <Content> would be more resilient to refactors.
5. web-haptics is v0.0.6 (pre-1.0)
src/lib/haptics.ts — The library is very new. The .catch(() => {}) on trigger() is good defensive coding, but pin the version in package.json (currently ^0.0.6 allows any 0.0.x bump). Pre-1.0 semver means even patches can break.
What looks good
- Clean removal of PillNavbarOverlay and radix-ui dependency tree — good dependency hygiene.
PrivacyAmountcomponent is well-structured with properaria-label/aria-pressedfor accessibility.- Dev auto-init watchdog with session-storage reload guard is solid DX improvement.
matchesAssetFilterin TransactionsList handles edge cases correctly.- Tests updated to match the new nav behavior (navbar removal assertions flipped).
- Vtxos.tsx restructuring with
WaitingForRoundis a cleaner separation of concerns.
SDK bumps — no action needed
@arkade-os/sdk0.4.27: HD receive rotation via contract repo, VTXO reconciliation by outpoint, pre-flight VTXO validation. All backward-compatible.@arkade-os/boltz-swap0.3.32: WS fallback polling, SW restart recovery, default referral ID (wallet overrides witharkade-money). No breaking changes.
LGTM for merge after human sign-off.
Update Playwright helpers and specs to navigate through the redesigned home header, settings entry points, and Boltz deep link instead of the removed bottom nav.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/providers/wallet.tsx (1)
263-302:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd missing dependencies to effect dependency array.
The effect at line 263 uses
isDevAutoInit,devAutoInitFailed, andinitializedin its conditional logic but only includeswallet.pubkeyin the dependency array. IfdevAutoInitFailedchanges totruewithout a pubkey change, the effect won't re-run andauthStatecan become stale.🔧 Proposed fix
- }, [wallet.pubkey]) + }, [wallet.pubkey, isDevAutoInit, devAutoInitFailed, initialized])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/providers/wallet.tsx` around lines 263 - 302, The effect in the hook uses isDevAutoInit, devAutoInitFailed and initialized (and also the helper functions like hasMnemonic/getMnemonic/noUserDefinedPassword/defaultPassword) but only lists wallet.pubkey in the dependency array, so update the useEffect dependency list to include isDevAutoInit, devAutoInitFailed, initialized (and any stable helper refs if they are not stable) so the effect re-runs when those values change; locate the effect surrounding detectPasswordState in the wallet provider (the useEffect block that sets authState) and add the missing dependencies to the dependency array accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/AssetCard.tsx`:
- Around line 38-39: The component is calling BigInt(balance) without validating
balance which can throw on non-integer/NaN/Infinity values; update the logic
around rawBalance and prettyBalance in AssetCard (symbols: balance, rawBalance,
prettyBalance, prettyCurrencyAssetAmount, decimals, tokenTick) to first validate
balance (e.g., allow bigint, numeric finite integers, or integer-like numeric
strings), perform safe coercion inside a guarded block or try/catch, and fall
back to a safe value (such as BigInt(0) or null/placeholder) when validation
fails so rendering won't crash; ensure the chosen fallback is propagated into
prettyCurrencyAssetAmount or handled so the component renders gracefully.
In `@src/components/PrivacyAmount.tsx`:
- Around line 51-53: The aria-pressed value on the visibility toggle is
inverted: in the PrivacyAmount component set aria-pressed to reflect the current
"balances visible" state (visible) instead of its negation; update the toggle
rendering where aria-pressed is used so it reads aria-pressed={visible} (keep
the existing aria-label logic and className/cn usage intact).
In `@src/components/TokenLogo.tsx`:
- Line 4: The current conditional in TokenLogo.tsx returns UnitedStatesFlagLogo
for USD, USDT, and USDC; change it so only 'USD' returns UnitedStatesFlagLogo
and add explicit branches for 'USDT' and 'USDC' that return their dedicated
token logo components (e.g., USDTLogo/TetherLogo and USDCLogo/UsdcLogo) instead
of the flag; update the conditional or switch in the TokenLogo component to
check ticker === 'USD' -> UnitedStatesFlagLogo, ticker === 'USDT' -> USDT token
logo component, and ticker === 'USDC' -> USDC token logo component so each asset
displays the correct logo.
In `@src/components/TransactionsList.tsx`:
- Line 185: The current assignment for txs uses a truthy check so limit=0 is
treated as "no limit"; change the condition to explicitly test for a numeric
limit (e.g., typeof limit === 'number' or Number.isFinite(limit') and optionally
limit >= 0) so that when normalizedMode === 'static' and limit is 0 you return
visibleTxs.slice(0, 0) (empty) instead of the full list; update the expression
that defines txs (currently using normalizedMode, limit, visibleTxs)
accordingly.
- Around line 247-261: In the static-mode branch (where normalizedMode ===
'static') the TransactionLine entries are clickable but not keyboard-focusable;
update the rendered TransactionLine (or its wrapper div) so each row is
focusable (tabIndex=0), has an appropriate role (e.g., role="button"), and
handles keyboard activation by wiring onKeyDown to call handleClick(tx) when
Enter or Space is pressed; ensure the key generation (key(tx, index)), txs.map,
TransactionLine, handleClick, and normalizedMode are the identifiers you modify
so keyboard users can focus and activate rows the same way as mouse users.
In `@src/components/WaitingForRound.tsx`:
- Around line 23-32: The current useEffect uses sleep(2000).then(...) so if the
component unmounts before the 2s elapses the interval can still start and leak;
replace the sleep promise with a cancellable timer: declare both timeout and
interval handles (e.g. let timeoutId: NodeJS.Timeout, intervalId:
NodeJS.Timeout), use timeoutId = setTimeout(() => { intervalId = setInterval(()
=> setLogLength(getInfoLogsLength()), 500) }, 2000), and in the cleanup call
clearTimeout(timeoutId) and clearInterval(intervalId) to ensure setInterval
never starts after unmount; keep references to the same identifiers used in this
file (useEffect, setLogLength, getInfoLogsLength).
In `@src/index.css`:
- Around line 607-620: Keyboard focus is currently invisible for the new
interactive controls because .home-section-action (and the other interactive
control selectors added in this diff) have no :focus-visible styles and global
outline reset hides focus; add a clear, accessible focus style by defining
.home-section-action:focus-visible (and the same :focus-visible rule for the
other interactive selectors in this change) to restore a visible
outline/box-shadow (e.g., a 2px solid or ring-like box-shadow using the existing
theme color variable like var(--purple-700) or a high-contrast color), include a
border-radius/padding-safe offset so it doesn't shift layout, and add a fallback
.home-section-action:focus for browsers without :focus-visible support so
keyboard users always see a visible focus indicator.
- Around line 1645-1647: Update the .settings-page padding shorthand to use the
CSS variable --pill-navbar-footer-clearance with a fallback so the declaration
doesn't become invalid when the variable is unset; replace the current third
padding value with a var() expression that provides a safe fallback (e.g., 0)
for --pill-navbar-footer-clearance so the top/right/bottom/left padding remains
valid.
In `@src/lib/format.ts`:
- Around line 84-87: The code converts Decimal/BigInt to JS number (e.g.
Decimal.div(...).toNumber(), Number(amount), Number(centsToUnits(...))) which
loses precision for large values; instead preserve the Decimal/BigInt domain and
format as strings: keep unitAmount as a Decimal (remove .toNumber()), and update
calls into prettyNumber to accept a Decimal/string (or pass
Decimal.toFixed(fiatDecimals) to produce a precise string), and replace
Number(amount)/Number(centsToUnits(...)) with BigInt/Decimal-safe string
conversions; modify prettyNumber (or add a wrapper) to use Intl.NumberFormat on
the string or format grouping via string manipulation so no intermediate number
is created. Ensure you update references to fiatDecimalsFor, useGrouping and
centsToUnits accordingly so formatting consumes precise string/Decimal inputs
rather than JS numbers.
In `@src/screens/Settings/General.tsx`:
- Around line 17-21: The button in General.tsx is overriding the accessible name
via aria-label which hides the visible row text/value from screen readers;
remove the aria-label on the settings-row button (the JSX button with className
'settings-row settings-row--value') so the browser uses the visible content as
the accessible name, or alternatively implement aria-labelledby that references
the label and value element IDs so both label and current value are announced
(ensure those elements have stable ids and update the button to use
aria-labelledby if choosing that approach).
---
Outside diff comments:
In `@src/providers/wallet.tsx`:
- Around line 263-302: The effect in the hook uses isDevAutoInit,
devAutoInitFailed and initialized (and also the helper functions like
hasMnemonic/getMnemonic/noUserDefinedPassword/defaultPassword) but only lists
wallet.pubkey in the dependency array, so update the useEffect dependency list
to include isDevAutoInit, devAutoInitFailed, initialized (and any stable helper
refs if they are not stable) so the effect re-runs when those values change;
locate the effect surrounding detectPasswordState in the wallet provider (the
useEffect block that sets authState) and add the missing dependencies to the
dependency array accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e690bbcb-efb3-4203-b2e5-84d917f6a2b6
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (72)
package.jsonsrc/App.tsxsrc/app.csssrc/components/AssetCard.tsxsrc/components/Content.tsxsrc/components/Menu.tsxsrc/components/PrivacyAmount.tsxsrc/components/Select.tsxsrc/components/Toggle.tsxsrc/components/TokenLogo.tsxsrc/components/TransactionsList.tsxsrc/components/WaitingForRound.tsxsrc/components/WalletLoadIn.tsxsrc/components/ui/drawer.tsxsrc/hooks/usePortfolioBalanceDisplay.tssrc/hooks/usePortfolioFiat.tssrc/icons/Coins.tsxsrc/icons/History.tsxsrc/icons/Landmark.tsxsrc/icons/Logo.tsxsrc/icons/Preconfirmed.tsxsrc/icons/Receive.tsxsrc/icons/Received.tsxsrc/icons/Scan.tsxsrc/icons/Send.tsxsrc/icons/Settings.tsxsrc/index.csssrc/index.tsxsrc/lib/animations.tssrc/lib/format.tssrc/lib/haptics.tssrc/lib/types.tssrc/providers/flow.tsxsrc/providers/navigation.tsxsrc/providers/options.tsxsrc/providers/wallet.tsxsrc/screens/Apps/Assets/Index.tsxsrc/screens/Apps/Dfx/Index.tsxsrc/screens/Apps/Lendasat/Index.tsxsrc/screens/Settings/Advanced.tsxsrc/screens/Settings/Display.tsxsrc/screens/Settings/Fiat.tsxsrc/screens/Settings/General.tsxsrc/screens/Settings/Haptics.tsxsrc/screens/Settings/Index.tsxsrc/screens/Settings/Menu.tsxsrc/screens/Settings/Theme.tsxsrc/screens/Settings/Vtxos.tsxsrc/screens/Wallet/Activity.tsxsrc/screens/Wallet/AssetsSection.tsxsrc/screens/Wallet/HomeHeader.tsxsrc/screens/Wallet/HomeQuickActions.tsxsrc/screens/Wallet/Index.tsxsrc/screens/Wallet/PortfolioHero.tsxsrc/screens/Wallet/RecentActivitySection.tsxsrc/screens/Wallet/Send/Form.tsxsrc/screens/Wallet/UpsellsSection.tsxsrc/test/App.test.tsxsrc/test/e2e/backup.test.tssrc/test/e2e/bip21.test.tssrc/test/e2e/delegate.test.tssrc/test/e2e/init.test.tssrc/test/e2e/keyboard.test.tssrc/test/e2e/lnurl.test.tssrc/test/e2e/lock.test.tssrc/test/e2e/nostr.test.tssrc/test/e2e/receive.test.tssrc/test/e2e/restore.test.tssrc/test/e2e/send.test.tssrc/test/e2e/swap.test.tssrc/test/e2e/utils.tssrc/test/screens/wallet/index.test.tsx
💤 Files with no reviewable changes (1)
- src/icons/Scan.tsx
Use the settings header text rendered by the shared Header component and assert the redesigned zero-balance home through stable test ids.
There was a problem hiding this comment.
Arkana Code Review — PR #627 (incremental, post-push)
Scope of new commits (9fb989d1, 8df89a2c): E2E test refactoring only — adapts all Playwright specs to the redesigned navigation (bottom tab bar removed, replaced with header icon buttons).
Reviewed:
-
navigateHome()— Bounded back-button loop (max 8 iterations) withpage.goto('/')fallback. Sentinel:home-action-receivetestid. Clean and robust. -
navigateToSettings()— Early-return if "Settings" heading already visible, otherwise navigates home first then clickstop-right-settings. Correct. -
navigateToBoltz()— Direct hash navigationpage.goto('/#app+boltz'). Clean workaround for removed Apps tab. -
navigateToAssets()— Now routes through Settings → "Arkade Mint" to match the UI relocation. Consistent with the main PR changes. -
init.test.ts— Assertions correctly flipped:tab-wallet/tab-apps/tab-settingsare nownot.toBeVisible(), replaced bytop-right-activity/top-right-settings.No transactions yetcorrectly hidden (activity preview absent when empty). -
All other test files — Mechanical
tab-*→ helper replacements. No logic changes.
No protocol-critical code. No security concerns. LGTM.
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com
There was a problem hiding this comment.
Arkana Code Review — PR #627 (incremental, commit 5811a172)
Scope: Adds a dev-only __ARKADE_E2E_NAVIGATE__ window hook so Playwright tests can navigate to app screens (e.g. Boltz) without page.goto() hash hacks, plus timeout bumps for mintAsset.
Reviewed:
-
navigatewrapped inuseCallback(…, [])— All dependencies are refs (previousPage,backStack,screenRef,skipNextPopstate), module-level constants (ROOT_PAGES,pageTab,subNavHandler), or stable state setters (setDirection,setScreen,setTab). Empty dep array is correct. -
Dev-only
useEffect— Guarded byimport.meta.env.DEV, tree-shaken from production builds. Cleanup deletes the window property.[navigate]dep is correct (stable due touseCallback). -
navigateToBoltz()test helper — Replaces brittlepage.goto('/#app+boltz')hash navigation withpage.evaluate()calling the new hook. Throws explicitly if hook is unavailable. Clean. -
mintAssettimeout bumps (3s→10s, 30s→60s) — Reduces flaky test failures. No-op in production.
No protocol-critical code. No security concerns. LGTM.
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com
There was a problem hiding this comment.
Arkana Code Review — PR #627 (incremental, commit 5811a172)
Scope: Adds a dev-only __ARKADE_E2E_NAVIGATE__ window hook so Playwright tests can navigate to app screens (e.g. Boltz) without page.goto() hash hacks, plus timeout bumps for mintAsset.
Reviewed:
-
navigatewrapped inuseCallback(…, [])— All dependencies are refs (previousPage,backStack,screenRef,skipNextPopstate), module-level constants (ROOT_PAGES,pageTab,subNavHandler), or stable state setters (setDirection,setScreen,setTab). Empty dep array is correct. -
Dev-only
useEffect— Guarded byimport.meta.env.DEV, tree-shaken from production builds. Cleanup deletes the window property.[navigate]dep is correct (stable due touseCallback). -
navigateToBoltz()test helper — Replaces brittlepage.goto('/#app+boltz')hash navigation withpage.evaluate()calling the new hook. Throws explicitly if hook is unavailable. Clean. -
mintAssettimeout bumps (3s→10s, 30s→60s) — Reduces flaky test failures. No-op in production.
No protocol-critical code. No security concerns. LGTM.
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/test/e2e/utils.ts (1)
69-72:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReuse
waitForWalletPage()after the hard-reload fallback.Line 70 reloads the app, but this path skips the BootError / “Continue anyway” handling that already lives in
waitForWalletPage(). If the fallback hits that overlay,navigateHome()just waits forhome-action-receiveuntil it times out.Proposed fix
if (!(await homeReceive.isVisible().catch(() => false))) { await page.goto('/') + await waitForWalletPage(page) } await homeReceive.waitFor({ state: 'visible', timeout: 30000 })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/e2e/utils.ts` around lines 69 - 72, The hard-reload fallback in navigateHome() currently does page.goto('/') then waits directly for homeReceive which skips BootError / “Continue anyway” handling; after calling page.goto('/') replace the direct waitFor({ state: 'visible' ... }) on homeReceive with a call to waitForWalletPage() so the existing overlay handling runs (i.e., invoke await waitForWalletPage() after the reload and remove or replace the homeReceive.waitFor(...) line).
🧹 Nitpick comments (1)
src/providers/navigation.tsx (1)
341-348: ⚡ Quick winDon’t rely on
import.meta.env.DEVfor the E2E navigation hook.With the current Playwright setup (
webServer.commandrunspnpm start, i.e. Vite dev server),import.meta.env.DEVshould be true sowindow.__ARKADE_E2E_NAVIGATE__exists during normaltest:e2eruns. Still, the setup is brittle: any switch to a production-like server (vite build+vite preview, etc.) would makeDEVfalse and causenavigateToBoltz()to hard-fail when the hook is missing. Gate the hook behind an explicit E2E flag/env var (or add a UI fallback when the hook isn’t present).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/providers/navigation.tsx` around lines 341 - 348, The effect currently registers window.__ARKADE_E2E_NAVIGATE__ only when import.meta.env.DEV is true which is brittle; change the guard to rely on an explicit E2E flag (e.g. import.meta.env.VITE_E2E === 'true' or a runtime window flag) and/or always register a no-op safe hook so callers like navigateToBoltz() never hard-fail; specifically update the useEffect that defines __ARKADE_E2E_NAVIGATE__ to check import.meta.env.VITE_E2E (or a runtime window.__ARKADE_E2E_ENABLED__) instead of import.meta.env.DEV and ensure navigate(Pages[page]) is only invoked when navigate and Pages are available (or provide a fallback no-op implementation) so tests work regardless of dev vs preview server.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/test/e2e/utils.ts`:
- Around line 69-72: The hard-reload fallback in navigateHome() currently does
page.goto('/') then waits directly for homeReceive which skips BootError /
“Continue anyway” handling; after calling page.goto('/') replace the direct
waitFor({ state: 'visible' ... }) on homeReceive with a call to
waitForWalletPage() so the existing overlay handling runs (i.e., invoke await
waitForWalletPage() after the reload and remove or replace the
homeReceive.waitFor(...) line).
---
Nitpick comments:
In `@src/providers/navigation.tsx`:
- Around line 341-348: The effect currently registers
window.__ARKADE_E2E_NAVIGATE__ only when import.meta.env.DEV is true which is
brittle; change the guard to rely on an explicit E2E flag (e.g.
import.meta.env.VITE_E2E === 'true' or a runtime window flag) and/or always
register a no-op safe hook so callers like navigateToBoltz() never hard-fail;
specifically update the useEffect that defines __ARKADE_E2E_NAVIGATE__ to check
import.meta.env.VITE_E2E (or a runtime window.__ARKADE_E2E_ENABLED__) instead of
import.meta.env.DEV and ensure navigate(Pages[page]) is only invoked when
navigate and Pages are available (or provide a fallback no-op implementation) so
tests work regardless of dev vs preview server.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c7716bfb-24ba-4920-bcbc-b71a5d02dfae
📒 Files selected for processing (3)
src/providers/navigation.tsxsrc/test/e2e/init.test.tssrc/test/e2e/utils.ts
|
@sahilc0 homepage balance doesn't obey to "sats only" option, i.e. balance is always in fiat, even when the user selects to see sats only values. |
|
Eternal loop between asset mint and asset. Screen.Recording.2026-05-21.at.14.52.49.mov |
|
@sahilc0 I think we should use the asset ticker instead of name here 
|
|
@sahilc0 WaitingForRound.tsx is back? |
|
@bordalix pushed a follow-up for these.
Validation on the rebased branch: lint, format:check, focused unit tests, and production build all pass locally. |
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 (1)
src/components/AssetCard.tsx (1)
47-47:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve the selected fiat unit when masking.
maskedFiatTextonly keeps a currency marker for$-prefixed values. For other fiat formats, masked mode degrades to plain••••, so the row loses the user's selected currency context. Please derive the visible unit fromfiatTextinstead of hardcoding USD here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/AssetCard.tsx` at line 47, maskedFiatText currently hardcodes checking only for '$' and falls back to '••••', losing the user's selected currency; update the logic that computes maskedFiatText to extract the visible currency unit from fiatText (e.g., leading symbol like '$' or '€' or a trailing currency code) and preserve that unit when masking instead of hardcoding USD. Locate the maskedFiatText variable and the maskedFiat helper and change the call so the unit is derived from fiatText and passed/used to render a masked value (for example unit + masked payload) so any non-USD currency context remains visible while amounts are masked.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/screens/Apps/Assets/Mint.tsx`:
- Line 167: The current pendingNav callback uses
replace(Pages.AppAssetMintSuccess) without a backTo, leaving the previous mint
form on the back stack; change the callback to call replace with an explicit
backTo target (e.g. replace(Pages.AppAssetMintSuccess, Pages.AppAssets)) so
system-back from AppAssetMintSuccess goes to the desired page instead of the
half-completed mint form; update the assignment to pendingNav.current
accordingly.
---
Outside diff comments:
In `@src/components/AssetCard.tsx`:
- Line 47: maskedFiatText currently hardcodes checking only for '$' and falls
back to '••••', losing the user's selected currency; update the logic that
computes maskedFiatText to extract the visible currency unit from fiatText
(e.g., leading symbol like '$' or '€' or a trailing currency code) and preserve
that unit when masking instead of hardcoding USD. Locate the maskedFiatText
variable and the maskedFiat helper and change the call so the unit is derived
from fiatText and passed/used to render a masked value (for example unit +
masked payload) so any non-USD currency context remains visible while amounts
are masked.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bcdc3335-30d6-4a4a-8310-474acd4d3e51
📒 Files selected for processing (14)
src/components/AssetCard.tsxsrc/components/Menu.tsxsrc/providers/navigation.tsxsrc/screens/Apps/Assets/Burn.tsxsrc/screens/Apps/Assets/Detail.tsxsrc/screens/Apps/Assets/Import.tsxsrc/screens/Apps/Assets/Index.tsxsrc/screens/Apps/Assets/Mint.tsxsrc/screens/Apps/Assets/MintSuccess.tsxsrc/screens/Apps/Assets/Reissue.tsxsrc/screens/Apps/Assets/Settings.tsxsrc/screens/Settings/Index.tsxsrc/test/e2e/lock.test.tssrc/test/screens/mocks.ts
There was a problem hiding this comment.
Arkana Code Review — PR #627 (incremental, commits a92ef222, ae7063f0)
Reviewing changes since 635a80ae: Arkade Mint navigation refactor and test fixes.
Summary
Good changes. The Arkade Mint is now navigated to directly from the settings menu via navigate(Pages.AppAssets) instead of being rendered inline as a settings sub-view. The new replace function in the navigation provider properly trims the back stack, and WaitingForRound deletion is clean (zero remaining consumers). Test changes in send.test.ts are a real improvement — capturing actual fees from the UI instead of hardcoding makes assertions less brittle.
Findings
Minor — dead import send.test.ts:1
import { wait } from '@testing-library/user-event/dist/cjs/utils/index.js'wait is never called anywhere in the file. Remove to keep the import list honest.
Nit — replace without backTo leaves back stack unchanged navigation.tsx:343-358
When replace(page) is called without backTo, the current screen is swapped but nothing is pushed or popped on backStack.current. This means "back" navigates to whatever the previous back-stack top was, not the page you replaced from. Current call sites all pass backTo (except replace(Pages.AppAssetMintSuccess) in Mint.tsx:167), so verify that case — pressing back from the mint-success screen should go where you intend.
Verdict
No protocol-critical paths touched. UI/navigation only. Approved — fix the dead import at your convenience.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/test/e2e/send.test.ts`:
- Line 1: The import of wait from
'`@testing-library/user-event/dist/cjs/utils/index.js`' is unused; remove the
unused symbol by deleting the `wait` import from the top-level import statement
(the import line that currently references `wait`) in send.test.ts so the file
no longer imports `wait`.
- Line 327: The assertion in send.test.ts is missing the USD dollar sign: update
the expectation that currently checks page.getByText(`${balance.toFixed(2)} sent
successfully`) to include the `$` prefix for USD (e.g., use
`$${balance.toFixed(2)} sent successfully` or the existing currency-format
helper used elsewhere) so the test matches the actual USD success copy; change
the assertion that references the balance variable in this test to the
dollar-prefixed string.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1c25e058-cf1c-4019-8356-79577e5b6317
📒 Files selected for processing (3)
src/components/AssetCard.tsxsrc/components/TransactionsList.tsxsrc/test/e2e/send.test.ts
There was a problem hiding this comment.
Arkana Code Review — PR #627 (incremental, commit 6e9972ff)
Reviewing the single new commit since last review (ae7063f0).
Commit 6e9972ff — "fix test"
Test-only changes to src/test/e2e/send.test.ts:
- Removed unused
waitimport — cleanup, good. - Fixed float comparison (
balance.toFixed(2)vs raw float equality) — correct fix for flaky JS float arithmetic in USDS balance assertions. - Fixed success message prefix (added
$to match actual UI text$X.XX sent successfully) — aligns test with the redesigned formatting from this PR.
No protocol-critical code. No security concerns. LGTM.
* feat(wallet): add bitcoin detail page Add a bitcoin-only wallet detail screen opened from the home asset row, with the Liveline price chart, current price, percentage badge, holdings, wallet actions, and bitcoin-filtered recent activity. Share the existing swap coming-soon drawer between home and the detail page, and cover the bitcoin row navigation with a wallet screen regression test. * fix(wallet): harden bitcoin chart loading Remove the invalid fiat conversion path from the bitcoin detail chart, use CoinGecko max data for non-USD all-time charts, cache chart responses per fiat/range, fetch Coinbase all-time chunks sequentially, and guard the holdings balance before BigInt conversion. * fix(wallet): pause bitcoin chart while hovering * fix tests * fix test * use graph worker * default chart period one week * remove console.log * new marketData file * fixes --------- Co-authored-by: Sahil Chaturvedi <sahilc0@users.noreply.github.com> Co-authored-by: João Bordalo <bordalix@users.noreply.github.com>
There was a problem hiding this comment.
Arkana Code Review — PR #627 (incremental, commit 8941b6ef)
Reviewing the new commit since last review (6e9972ff): feat(wallet): add bitcoin detail page (#630).
Not protocol-critical (UI/display only — no VTXO, signing, forfeit, round, or exit path changes).
🔴 Bug: AbortSignal not passed to fetch
src/screens/Wallet/BitcoinDetail.tsx:393 — fetchHistoricalMarketData(windowSecs, fiat) creates an AbortController (line 384) but never passes controller.signal to the fetch call. The function signature in src/lib/marketData.ts:29 accepts signal?: AbortSignal as the 3rd argument.
Current behavior: on unmount/re-render, the abort only prevents the .then() state update (line 395 checks controller.signal.aborted), but the network request itself is never cancelled. On rapid tab-switching between chart windows, this fires up to 6 concurrent in-flight requests that can't be aborted.
Fix:
fetchHistoricalMarketData(windowSecs, fiat, controller.signal)🟡 Stale comment
src/components/AssetCard.tsx:86 — Comment says "Non-interactive row (e.g. BTC which has no detail screen)" but BTC now has a detail screen. Minor, but misleading for the next reader.
🟡 Module-level cache never evicts
src/screens/Wallet/BitcoinDetail.tsx:38 — bitcoinChartCache is a module-scope Map that grows but never clears. In practice this is bounded (~6 windows × N fiat currencies), so it won't OOM, but stale price data persists for the entire session lifetime. Consider adding a TTL or clearing on fiat change. Not blocking.
ℹ️ Notes (non-blocking)
liveline@0.0.7— very early-stage dependency (v0.0.7). Acceptable for a chart widget, just be aware upstream API may churn.src/lib/marketData.ts:4— TODO comment (// TODO: move to final worker URL) — track this so it doesn't ship to prod pointing at a dev/personal worker.- Test coverage for the new screen is minimal (1 test: pointer pause/unpause). Navigation, action buttons, delta badge edge cases (0%, negative), and error states (fetch failure) are untested. Not blocking this PR but worth a follow-up.
Verdict: One real bug (missing abort signal). The rest are minor. Fix the signal pass-through and this is good to go. 👍
There was a problem hiding this comment.
Arkana Code Review — PR #627 (incremental, commit 8941b6ef)
Reviewing the new commit since last review (6e9972ff): feat(wallet): add bitcoin detail page (#630).
Not protocol-critical (UI/display only — no VTXO, signing, forfeit, round, or exit path changes).
🔴 Bug: AbortSignal not passed to fetch
src/screens/Wallet/BitcoinDetail.tsx:393 — fetchHistoricalMarketData(windowSecs, fiat) creates an AbortController (line 384) but never passes controller.signal to the fetch call. The function signature in src/lib/marketData.ts:29 accepts signal?: AbortSignal as the 3rd argument.
Current behavior: on unmount/re-render, the abort only prevents the .then() state update (line 395 checks controller.signal.aborted), but the network request itself is never cancelled. On rapid tab-switching between chart windows, this fires up to 6 concurrent in-flight requests that can't be aborted.
Fix:
fetchHistoricalMarketData(windowSecs, fiat, controller.signal)🟡 Stale comment
src/components/AssetCard.tsx:86 — Comment says "Non-interactive row (e.g. BTC which has no detail screen)" but BTC now has a detail screen. Minor, but misleading for the next reader.
🟡 Module-level cache never evicts
src/screens/Wallet/BitcoinDetail.tsx:38 — bitcoinChartCache is a module-scope Map that grows but never clears. In practice this is bounded (~6 windows × N fiat currencies), so it won't OOM, but stale price data persists for the entire session lifetime. Consider adding a TTL or clearing on fiat change. Not blocking.
ℹ️ Notes (non-blocking)
liveline@0.0.7— very early-stage dependency (v0.0.7). Acceptable for a chart widget, just be aware upstream API may churn.src/lib/marketData.ts:4— TODO comment (// TODO: move to final worker URL) — track this so it doesn't ship to prod pointing at a dev/personal worker.- Test coverage for the new screen is minimal (1 test: pointer pause/unpause). Navigation, action buttons, delta badge edge cases (0%, negative), and error states (fetch failure) are untested. Not blocking this PR but worth a follow-up.
Verdict: One real bug (missing abort signal). The rest are minor. Fix the signal pass-through and this is good to go. 👍
* chore(dev): support mnemonic wallet auto-init Allow local dev auto-init to use VITE_DEV_MNEMONIC in addition to VITE_DEV_NSEC, so demo wallets can be restored from the current seed phrase without using the broken legacy default key. * feat(wallet): align recent activity with prototype Update the home recent activity list, full activity view, and transaction receipt details to match the prototype visuals while keeping the shared Activity header from PR 627. Add swap/account asset rendering, receipt-style detail rows, and measured virtual rows for taller activity entries. * fix(wallet): address activity review feedback Guard the round waiting screen when no info logs exist, cancel delayed polling on unmount, preserve the safer settlement status flow, remove incomplete BRL fiat support, and share token ticker mapping helpers across activity and asset views. * fix send max with fiat ' + tests --------- Co-authored-by: Sahil Chaturvedi <sahilc0@users.noreply.github.com> Co-authored-by: João Bordalo <bordalix@users.noreply.github.com>
Port the draft send-flow field shell, asset selector, amount max control, and button safety polish on top of PR 627. Add dev mnemonic auto-init support so local wallets can boot from mnemonic-based seeds without falling back to stale nsec state. Co-authored-by: Sahil Chaturvedi <sahilc0@users.noreply.github.com> Co-authored-by: João Bordalo <bordalix@users.noreply.github.com>
* chore(dev): support mnemonic wallet auto-init Allow local dev auto-init to use VITE_DEV_MNEMONIC in addition to VITE_DEV_NSEC, so demo wallets can be restored from the current seed phrase without using the broken legacy default key. * feat(wallet): align recent activity with prototype Update the home recent activity list, full activity view, and transaction receipt details to match the prototype visuals while keeping the shared Activity header from PR 627. Add swap/account asset rendering, receipt-style detail rows, and measured virtual rows for taller activity entries. * fix(wallet): address activity review feedback Guard the round waiting screen when no info logs exist, cancel delayed polling on unmount, preserve the safer settlement status flow, remove incomplete BRL fiat support, and share token ticker mapping helpers across activity and asset views. * fix send max with fiat ' + tests --------- Co-authored-by: Sahil Chaturvedi <sahilc0@users.noreply.github.com> Co-authored-by: João Bordalo <bordalix@users.noreply.github.com>
2 participants
Summary
QA notes
Testing
Build passed with the existing Vite warnings about publicDir, mnemonic import chunking, and bundle size.
Summary by CodeRabbit
New Features
Improvements