remove ionic/normalize and ion-button#534
Conversation
|
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:
WalkthroughRemoves Ionic tab infrastructure and many Ion components in favor of native HTML/CSS, adds fiat formatting helpers/symbols, introduces a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ReceiveUI as "ReceiveQRCode"
participant FlowProv as "FlowProvider (RecvInfo)"
participant SwapSvc as "SwapService/Backend"
participant SW as "ServiceWorker"
User->>ReceiveUI: open / request invoice
ReceiveUI->>FlowProv: read recvInfo (received?)
alt recvInfo.received == true
ReceiveUI-->>User: no-op (short-circuit)
else
ReceiveUI->>SwapSvc: create invoice / initiate swap
SwapSvc-->>ReceiveUI: invoice / swap response
ReceiveUI->>User: show copy sheet
User->>ReceiveUI: copy invoice
ReceiveUI->>ReceiveUI: setShowCopySheet(false)
par swap claimed via UI
SwapSvc->>FlowProv: mark recvInfo.received = true
and/or service worker detects onchain
SW->>FlowProv: payment update -> recvInfo.received = true
end
FlowProv-->>ReceiveUI: recvInfo updated (received: true)
ReceiveUI-->>User: stop further QR creation
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: |
9c0f4df
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c26f34bf.arkade-wallet.pages.dev |
| Branch Preview URL: | https://remove-ion-button.arkade-wallet.pages.dev |
Deploying tmp-boltz-upstream-mainnet-arkade-wallet with
|
| Latest commit: |
9c0f4df
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ad55e7a9.tmp-boltz-upstream-mainnet-arkade-wallet.pages.dev |
| Branch Preview URL: | https://remove-ion-button.tmp-boltz-upstream-mainnet-arkade-wallet.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: 2
🧹 Nitpick comments (1)
src/App.tsx (1)
2-4: Consider removing the commented import instead of leaving it.Dead/commented code adds noise. If this removal is confirmed working, the line should be deleted rather than commented out.
Proposed fix
import '@ionic/react/css/core.css' /* Basic CSS for apps built with Ionic */ -// import '@ionic/react/css/normalize.css' import '@ionic/react/css/structure.css'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/App.tsx` around lines 2 - 4, Remove the commented-out import statement for '@ionic/react/css/normalize.css' in App.tsx to eliminate dead code; locate the commented line "// import '@ionic/react/css/normalize.css'" near the top of the file and delete it so only active imports (e.g., "import '@ionic/react/css/structure.css'") remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/index.css`:
- Around line 127-135: The .clear modifier (.clear represented as "&.clear" in
this stylesheet) uses an invalid value "border-color: none"; update the
declaration inside the "&.clear" block to remove the invalid value by either
deleting the border-color line entirely (if you want no border styling) or
replace it with a valid value such as "border-color: transparent" so the rule is
valid and behaves as intended.
- Around line 101-117: The :root does not define --btn-shadow-color while button
rules (button selector) use var(--btn-shadow-color); open the global CSS :root
(in ionic.css) and add a --btn-shadow-color declaration (choose the intended
shadow color value, e.g., a semi-opaque dark/primary color) so the box-shadow
for the button selector resolves; ensure the variable is placed alongside other
root variables so all components inherit it.
---
Nitpick comments:
In `@src/App.tsx`:
- Around line 2-4: Remove the commented-out import statement for
'@ionic/react/css/normalize.css' in App.tsx to eliminate dead code; locate the
commented line "// import '@ionic/react/css/normalize.css'" near the top of the
file and delete it so only active imports (e.g., "import
'@ionic/react/css/structure.css'") remain.
🪄 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: 2c8d0917-e312-4282-88a9-a013a1248adb
📒 Files selected for processing (9)
src/App.tsxsrc/components/Button.tsxsrc/components/ButtonsOnBottom.tsxsrc/components/Select.tsxsrc/index.csssrc/ionic.csssrc/screens/Settings/Delegates.tsxsrc/screens/Settings/General.tsxsrc/screens/Wallet/Receive/QrCode.tsx
|
hmm seems like some regressions here, the styling of the buttons is off, and typography of the navbar seems to be wrong too somehow |
Deploying wallet-bitcoin with
|
| Latest commit: |
9c0f4df
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://bc3308af.wallet-bitcoin.pages.dev |
| Branch Preview URL: | https://remove-ion-button.wallet-bitcoin.pages.dev |
Amounts previously rendered with a trailing ISO code (`100.00 USD`, `50.00 EUR`) now use the common Unicode currency symbol as a prefix: `$100.00`, `€50.00`, `£25.00`, `¥1,000`. Currencies without a widely-used single-char symbol (CHF, CNY) keep the trailing-code form, so CNY does not collide with JPY's `¥`. The new formatters live alongside the existing generic ones: - `prettyFiatAmount(amount, currency)` and `prettyFiatHide(value, currency)` in `src/lib/format.ts` take the `Fiats` enum as a typed parameter. - The generic `prettyAmount` / `prettyHide` are intentionally left untouched so asset tickers (e.g. a stablecoin named `USD`) that flow through those helpers keep their trailing-code format. - Decimals are derived from the currency via `fiatDecimalsFor` in `src/lib/fiat.ts` (JPY = 0, others = 2) — shared between `FiatContext` and `prettyFiatAmount` so call sites no longer need to pass `fiatDecimals()`. Touched displays: hero balance (`Balance.tsx`), transaction list rows (`TransactionsList.tsx`), amount picker (`Keyboard.tsx`), transaction details (`Details.tsx`), Send/Receive/Notes success screens, and Send form available caption. Amount input labels (`InputAmount.tsx`) also use the symbol when available. Side fix: `Details.tsx` previously called `prettyAmount` without passing `fiatDecimals()`, hardcoding 2 decimals. JPY now renders as `¥174` / `¥0` in the transaction details instead of `174.00 JPY` / `0.00 JPY`. Hero alignment: the eye-toggle button wrapper now omits an empty unit `<Text>` when the symbol is inlined into the balance, and the button sits inside a `alignItems: center` flex container next to the unit text — keeping the icon centered with the smaller ISO code in the CHF case and snug against the balance in the `$7.30` case. Tests: - New coverage for `prettyFiatAmount` and `prettyFiatHide` in `src/test/lib/format.test.ts` (symbol + trailing-code + JPY 0-decimals). - Existing `prettyAmount` / `prettyHide` tests unchanged — the generic helpers still behave the same. - UI assertions updated to match the new rendered text: `src/test/screens/wallet/index.test.tsx` → `$0.00` `src/test/e2e/init.test.ts` → `$0.00` `src/test/e2e/keyboard.test.ts` → symbol-prefix regex Co-authored-by: Sahil Chaturvedi <sahilc0@users.noreply.github.com> Co-authored-by: João Bordalo <bordalix@users.noreply.github.com>
* reverse logs in csv export * fix * avoid mutating the React state array
* close copy sheet after copy * lint * test swaps in receive
* Prevent swap after receival * fix
![arkanaai[bot]](https://avatars.githubusercontent.com/u/170820085?s=60&v=4){kind=small}
There was a problem hiding this comment.
Arkana Code Review — wallet#534
Reviewed the full diff (32 files). This is mostly a UI refactor (Ionic → native HTML) plus fiat symbol formatting and bug fixes. The logic changes are sound overall, but there are two issues that need fixing before merge — one will break mobile layouts, the other will silently corrupt non-Button <button> elements across the app.
🔴 MUST FIX
1. Hardcoded width: '640px' breaks mobile — src/App.tsx:235
<div className='page-transition-container' style={{ width: '640px' }}>This is a hard width, not max-width. On any mobile viewport (typically 360–430px), this forces horizontal scrolling or overflow. The old Ionic layout handled this via ion-page's max-width: 640px; margin: auto (defined in ionic.css:203). Fix: change to max-width: '640px' or remove the inline style entirely and let the existing ion-page CSS handle it.
2. Global button { ... } selector nukes all non-Button buttons — src/index.css:100-183
The new global button rule applies width: 100%, min-height: 40px, text-transform: uppercase, font-family: 'Geist Mono', and box-shadow to every <button> in the app, not just the Button.tsx component. This will break at minimum:
PillNavbar.tsx:39,52,65— nav tab buttons become full-width, uppercase, monospaceBalance.tsx:49— eye toggle icon button becomes full-width with 40px min-heightDismissibleBanner.tsx:29— dismiss/action buttons stretch to 100% width, uppercaseInAppBrowser.tsx:265— 44×44px copy icon button becomes full-widthQrCode.tsx:409— QR code wrapper button gets unwanted min-height/box-shadowButton.tsx:114(ButtonOnInput) — pill buttons get width:100%, uppercase, wrong min-height
The .copy and .pill-nav-btn overrides partially help but don't cover all cases (missing: Balance eye button, DismissibleBanner, InAppBrowser, ButtonOnInput/pill-base, QR button).
Fix: Scope the global styles to a class (e.g., button.btn or .btn) and add that class in Button.tsx, rather than styling the bare button element. Or add explicit reset rules for every non-Button button in the app — but that's fragile.
🟡 SHOULD FIX
3. Missing type='button' on copy button — src/screens/Wallet/Receive/QrCode.tsx:619
The PR removes type='button' from the copy button in AddressLine. Without it, <button> defaults to type='submit', which can trigger unintended form submissions if the button is ever placed inside a <form>. The old code had it — keep it.
4. Stale closure in received flag callbacks — src/screens/Wallet/Receive/QrCode.tsx:125-130,213-216,227-230
The received flag fix is correct in intent (prevents re-triggering swaps — nice fix for #547). However, the setRecvInfo({ ...recvInfo, received: true, ... }) calls capture recvInfo from the effect's closure, which may be stale by the time the async waitAndClaim resolves. Consider using the functional updater form:
setRecvInfo(prev => ({ ...prev, received: true, satoshis: ... }))This is especially important because multiple swap callbacks could race. The current code works most of the time but has a subtle overwrite risk.
🟢 LOOKS GOOD
- Fiat symbol formatting (
format.ts,fiat.ts,Balance.tsx,Keyboard.tsx, etc.) — clean implementation. JPY zero-decimal handling is correct. CNY/CHF trailing-code avoidance is sensible. Tests cover the key cases well. - SDK bumps (0.4.15→0.4.16, 0.3.16→0.3.17) — the SDK fix (
set proof tx lockTime to 0) is important, good to pick up. - CSV log reversal (
Logs.tsx:108) —[...logs].reverse()correctly avoids mutating React state. ✅ onchainAmount ?? 0fallback (SwapsList.tsx:69) — defensive, fine.StrengthBarsexport change (Strength.tsx) — no other consumer imports the default, safe.receivedfield onRecvInfo(flow.tsx) — properly added to interface,emptyRecvInfo, and all set sites. The guard at QrCode.tsx:185 (if (received) return) prevents re-creating swaps after payment lands.- New e2e tests (
receive.test.ts) — good coverage for invoice update and no-swap-on-amountless-receive. setShowCopySheet(false)after copy (QrCode.tsx:327) — nice UX fix.
Not protocol-critical
This PR doesn't touch VTXOs, transaction signing, forfeit paths, round lifecycle, or exit paths. The swap-related changes (received flag, onchainAmount ?? 0) are defensive UI guards, not protocol logic. No human sign-off required for protocol reasons — just fix the two 🔴 items above.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/screens/Wallet/Receive/QrCode.tsx (2)
93-112:⚠️ Potential issue | 🔴 Critical
receivedflag is never reset when entering the receive flow — risk of permanent "Loading..." on second receive.
recvInfolives inFlowContextand persists across screens. ThisuseEffectspreads...recvInfowhen refreshing addresses but does not resetreceived. After a successful receive (which setsreceived: true) the user goes toReceiveSuccess→Wallet. If they start another receive and land onQrCodewhilerecvInfostill carriesreceived: trueand a prior non-zerosatoshis, the main QR effect short-circuits on line 186 (if (received) return), sosetShowQrCode(true)is never called and the screen stays stuck onLoadingLogo.Either reset it here, or in
emptyRecvInfo-style teardown when leavingReceiveSuccess.🛠️ Suggested reset at address-fetch time
.then(({ offchainAddr, boardingAddr }) => { if (!offchainAddr) throw 'Unable to get offchain address' if (!boardingAddr) throw 'Unable to get boarding address' - setRecvInfo({ ...recvInfo, boardingAddr, offchainAddr, satoshis: 0, addressError: undefined }) + setRecvInfo({ + ...recvInfo, + boardingAddr, + offchainAddr, + satoshis: 0, + addressError: undefined, + received: false, + receivedAssets: undefined, + }) setAddressesLoaded(true) })And mirror the reset on the fast path when
boardingAddr && offchainAddrare already populated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/screens/Wallet/Receive/QrCode.tsx` around lines 93 - 112, The effect that fetches addresses should also reset the persistent recvInfo's received and satoshis so the QR flow can run again; update the fast path (the if (boardingAddr && offchainAddr) branch) to call setRecvInfo({...recvInfo, received: false, satoshis: 0}) and the success .then branch (after getReceivingAddresses) to setRecvInfo({...recvInfo, boardingAddr, offchainAddr, received: false, satoshis: 0, addressError: undefined}); keep the existing error handling and setAddressesLoaded calls as-is.
208-239:⚠️ Potential issue | 🟠 MajorStale
recvInfoin swap-claim resolutions can clobber listener updates.Both
.waitAndClaimArkand.waitAndClaimresolve withsetRecvInfo({ ...recvInfo, received: true, satoshis: ... })using therecvInfocaptured when the effect ran. The service-worker listener (line 306) can fire for the same claim and writereceivedAssets/ updatedsatoshisontorecvInfo; whicheversetRecvInforuns last spreads its stale snapshot and overwrites the other's fields (notablyreceivedAssets, which these branches don't set).Consider using a functional updater to avoid the race:
♻️ Use functional setState
- .then(() => { - setRecvInfo({ ...recvInfo, received: true, satoshis: pendingSwap.response.claimDetails.amount }) + .then(() => { + setRecvInfo((prev) => ({ + ...prev, + received: true, + satoshis: pendingSwap.response.claimDetails.amount, + })) navigate(Pages.ReceiveSuccess) })Same pattern at the reverse-swap branch (line 230) and the listener at line 306. Note: this requires widening
setRecvInfoinFlowContextPropsto accept an updater function.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/screens/Wallet/Receive/QrCode.tsx` around lines 208 - 239, The issue is that setRecvInfo is called with a stale recvInfo snapshot inside the async resolution of waitAndClaimArk and waitAndClaim (and the service-worker listener), which can overwrite concurrent updates; change all setRecvInfo(...) calls in the Boltz swap resolution branches (the callbacks after arkadeSwaps.waitAndClaimArk and arkadeSwaps.waitAndClaim) and in the service-worker listener to use the functional updater form setRecvInfo(prev => ({ ...prev, received: true, satoshis: ..., /* preserve other fields */ })); and widen the FlowContextProps/type for setRecvInfo to accept a React.SetStateAction-style updater (value or updater) so the functional form is allowed. Ensure you preserve other fields (e.g., receivedAssets) by spreading prev, not a captured recvInfo.src/components/Button.tsx (1)
62-75:⚠️ Potential issue | 🟠 MajorAdd explicit
type='button'to the native<button>.Native
<button>defaults totype='submit'when rendered inside a<form>, whereas the replacedIonButtondefaulted totype='button'. AnyButtonplaced inside a form will now submit the form on click, which is a subtle regression that can cause unexpected navigation/validation runs. Set it explicitly (and consider exposing it as a prop if you need submit elsewhere).🛡️ Proposed fix
<button + type='button' className={className} disabled={disabled} onClick={handleClick}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Button.tsx` around lines 62 - 75, The rendered native <button> in the Button component currently lacks an explicit type and will default to type="submit" inside forms; update the JSX in the Button component (the element that uses className, disabled, onClick={handleClick}, onMouseDown={handlePressStart}, etc.) to include type="button" to preserve previous non-submitting behavior, and optionally add a prop (e.g., prop name "type" on the Button component) that defaults to "button" so callers can override it when a submit button is needed.
🧹 Nitpick comments (6)
src/ionic.css (1)
106-113: Broadbuttonselector in reduced-motion block.This now applies to every
<button>in the app (including native UA buttons inside third-party widgets), not just the styled ones. That's generally fine for accessibility, but if any component relies on atransitionfor state feedback, it will be silently disabled underprefers-reduced-motion. Consider scoping it to the app's button class (e.g., the one defined insrc/index.css) if you want tighter control.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ionic.css` around lines 106 - 113, The broad selector "button" inside the "@media (prefers-reduced-motion: reduce)" block disables transitions for every native and third-party button; narrow the scope to the app's styled button class (replace "button" and "&.pressed" with your app-specific selector used in src/index.css, e.g., ".app-button" and ".app-button.pressed") so only your components lose transitions while third-party/native buttons keep their behavior; update the media-query block in src/ionic.css to use that class selector instead of the global "button".src/screens/Wallet/Receive/QrCode.tsx (1)
619-629: Minor: default<button>type issubmit.Dropping
type='button'is fine today (no enclosing form), but it's a subtle foot-gun if this list ever gets wrapped in a<form>later. Keeping the explicit attribute alongsideclassName='copy'is cheap insurance.<button aria-label={`Copy ${title}`} className='copy' + type='button' data-testid={testId + '-address-copy'}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/screens/Wallet/Receive/QrCode.tsx` around lines 619 - 629, The <button> in QrCode.tsx currently lacks an explicit type which defaults to "submit" and can cause unexpected form submissions later; update the button element (the one with aria-label={`Copy ${title}`} and data-testid={testId + '-address-copy'} that calls onCopy(value) and shows {copied === value ? <CheckMarkIcon /> : <CopyIcon />}) to include type="button" to prevent accidental submission.src/App.tsx (1)
3-3: Prefer deleting the dead import over commenting it out.Leaving
// import '@ionic/react/css/normalize.css'in place adds noise and can confuse future readers about intent. Since the PR goal is to removeionic/normalize, drop the line entirely (git history preserves the removal).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/App.tsx` at line 3, Remove the dead commented import line "// import '@ionic/react/css/normalize.css'" from src/App.tsx so the file no longer contains the commented-out Ionic normalize import; locate the commented import in App.tsx (the line that begins with // import '@ionic/react/css/normalize.css') and delete it entirely, relying on git history if you need to recover it.src/components/Button.tsx (1)
54-60:loadingdoes not gateonClick.
handlePressStartearly-returns whileloading, buthandleClickstill fireshapticTap()andonClick(event). Double-tapping the spinner can trigger duplicate submissions (e.g. re-broadcast, re-settle).IonButtonhad no such guard either so this may be preexisting, but it's easy to add while you're here.♻️ Proposed fix
const handleClick = useCallback( (event: any) => { + if (disabled || loading) return hapticTap() onClick(event) }, - [onClick], + [onClick, disabled, loading], )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Button.tsx` around lines 54 - 60, handleClick currently always calls hapticTap() and onClick(event) even when the component is in loading state; add the same loading guard used in handlePressStart so clicks are ignored while loading. Inside the handleClick callback (referencing handleClick, hapticTap, onClick and the loading prop/state), early-return when loading is true before calling hapticTap or onClick to prevent duplicate submissions (same pattern used for handlePressStart and consistent with IonButton behavior).src/components/Keyboard.tsx (1)
207-217: Consider making keypad keys keyboard-accessible.The numeric keys are rendered as
<div onClick>with no role, tabIndex, or key-press handler. They're not reachable or activatable via keyboard or screen readers. If you want to improve this while you're already touching the layout, switch to<button type='button' class='clear'>(which you now have global styling for) or addrole='button',tabIndex={0}, and anonKeyDownhandler. This looks like preexisting behavior so it's optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Keyboard.tsx` around lines 207 - 217, The keypad items rendered in the map (keys.map(...) in Keyboard.tsx) use <div onClick> and lack keyboard accessibility; update the key rendering to use an actual interactive element or add ARIA/keyboard handlers: replace the div with a <button type="button" className="clear"> for keys that need the global styling (or add role="button", tabIndex={0}, and an onKeyDown that invokes handleKeyPress(key) for Enter/Space) and ensure handleKeyPress is reused for both click and key activation so screen readers and keyboard users can activate keys.src/test/e2e/receive.test.ts (1)
85-89: Clarify the+ 1in the delete loop.
for (let i = 0; i < sats.toString().length + 1; i++)presses backspace one extra time beyond the known digit count. If this is to clear a stray decimal/leading character, a short comment would help; otherwise, if the input starts exactly atsats, the+1may leave state inconsistent across runs (e.g. delete beyond empty is a no-op today but could change).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/e2e/receive.test.ts` around lines 85 - 89, The delete loop in receive.test.ts currently uses for (let i = 0; i < sats.toString().length + 1; i++) which presses backspace one extra time; either remove the +1 or, better, replace this brittle loop with a deterministic clear (e.g., use the input element's clear/fill('') or delete exactly the current value length) and/or add a comment explaining why an extra delete is needed; update the test to use the input-clearing approach before calling handleKeyboardInput so the test is robust across runs and reference the test helper handleKeyboardInput and the keyboard-x test id when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/App.tsx`:
- Around line 236-242: The inline style on the div with class
page-transition-container currently fixes width to '640px', causing overflow on
small viewports; update the styling for that container (where PageAnimWrapper
and PageTransition are wrapped) to use responsive constraints — e.g., change the
inline style to width: '100%' and maxWidth: '640px' or remove the inline style
and add a CSS rule for .page-transition-container that sets width:100% and
max-width:640px so it caps at 640px on large screens but fits mobile viewports.
In `@src/components/Strength.tsx`:
- Around line 34-48: The StrengthBars component currently fills bars using col <
strength and drops native progress semantics; change the fill logic to use
Math.floor(strength) so fractional values (e.g. 3.25) only fill full bars for
the integer portion (use col < Math.floor(strength) in the style predicate), and
restore accessibility by adding progress semantics on the wrapper (e.g., set
role="progressbar" with aria-valuenow={strength}, aria-valuemin={0} and
aria-valuemax={4}); keep getColor(strength) and FlexRow as-is and avoid using an
empty string for backgroundColor (use undefined or omit the property when not
filled).
In `@src/index.css`:
- Around line 101-180: The global button rule is overwriting navbar styling
(making .pill-nav-btn uppercase, monospace, 40px tall and full-width); update
the styles so navbar buttons are not affected by the global rule: either scope
the global rule by converting the top-level selector from button to a component
class used by Button.tsx (e.g. .btn) and update Button.tsx to add that class, or
add explicit resets inside the .pill-nav-btn block for text-transform,
font-family, font-size, width, min-height, padding and box-shadow to restore the
navbar's intended lowercase, app font and sizing.
- Around line 209-223: The .has-pill-navbar .content rule sets --padding-bottom
but the base .content never uses it; update the .content rule to consume that
custom property (e.g. add padding-bottom: var(--padding-bottom, 0)) so the
--pill-navbar-clearance value is applied, or alternatively remove the
.has-pill-navbar .content override if you choose not to support that clearance;
target the .content selector and the .has-pill-navbar .content override and
adjust the CSS to either reference --padding-bottom in .content or delete the
unused override.
---
Outside diff comments:
In `@src/components/Button.tsx`:
- Around line 62-75: The rendered native <button> in the Button component
currently lacks an explicit type and will default to type="submit" inside forms;
update the JSX in the Button component (the element that uses className,
disabled, onClick={handleClick}, onMouseDown={handlePressStart}, etc.) to
include type="button" to preserve previous non-submitting behavior, and
optionally add a prop (e.g., prop name "type" on the Button component) that
defaults to "button" so callers can override it when a submit button is needed.
In `@src/screens/Wallet/Receive/QrCode.tsx`:
- Around line 93-112: The effect that fetches addresses should also reset the
persistent recvInfo's received and satoshis so the QR flow can run again; update
the fast path (the if (boardingAddr && offchainAddr) branch) to call
setRecvInfo({...recvInfo, received: false, satoshis: 0}) and the success .then
branch (after getReceivingAddresses) to setRecvInfo({...recvInfo, boardingAddr,
offchainAddr, received: false, satoshis: 0, addressError: undefined}); keep the
existing error handling and setAddressesLoaded calls as-is.
- Around line 208-239: The issue is that setRecvInfo is called with a stale
recvInfo snapshot inside the async resolution of waitAndClaimArk and
waitAndClaim (and the service-worker listener), which can overwrite concurrent
updates; change all setRecvInfo(...) calls in the Boltz swap resolution branches
(the callbacks after arkadeSwaps.waitAndClaimArk and arkadeSwaps.waitAndClaim)
and in the service-worker listener to use the functional updater form
setRecvInfo(prev => ({ ...prev, received: true, satoshis: ..., /* preserve other
fields */ })); and widen the FlowContextProps/type for setRecvInfo to accept a
React.SetStateAction-style updater (value or updater) so the functional form is
allowed. Ensure you preserve other fields (e.g., receivedAssets) by spreading
prev, not a captured recvInfo.
---
Nitpick comments:
In `@src/App.tsx`:
- Line 3: Remove the dead commented import line "// import
'@ionic/react/css/normalize.css'" from src/App.tsx so the file no longer
contains the commented-out Ionic normalize import; locate the commented import
in App.tsx (the line that begins with // import
'@ionic/react/css/normalize.css') and delete it entirely, relying on git history
if you need to recover it.
In `@src/components/Button.tsx`:
- Around line 54-60: handleClick currently always calls hapticTap() and
onClick(event) even when the component is in loading state; add the same loading
guard used in handlePressStart so clicks are ignored while loading. Inside the
handleClick callback (referencing handleClick, hapticTap, onClick and the
loading prop/state), early-return when loading is true before calling hapticTap
or onClick to prevent duplicate submissions (same pattern used for
handlePressStart and consistent with IonButton behavior).
In `@src/components/Keyboard.tsx`:
- Around line 207-217: The keypad items rendered in the map (keys.map(...) in
Keyboard.tsx) use <div onClick> and lack keyboard accessibility; update the key
rendering to use an actual interactive element or add ARIA/keyboard handlers:
replace the div with a <button type="button" className="clear"> for keys that
need the global styling (or add role="button", tabIndex={0}, and an onKeyDown
that invokes handleKeyPress(key) for Enter/Space) and ensure handleKeyPress is
reused for both click and key activation so screen readers and keyboard users
can activate keys.
In `@src/ionic.css`:
- Around line 106-113: The broad selector "button" inside the "@media
(prefers-reduced-motion: reduce)" block disables transitions for every native
and third-party button; narrow the scope to the app's styled button class
(replace "button" and "&.pressed" with your app-specific selector used in
src/index.css, e.g., ".app-button" and ".app-button.pressed") so only your
components lose transitions while third-party/native buttons keep their
behavior; update the media-query block in src/ionic.css to use that class
selector instead of the global "button".
In `@src/screens/Wallet/Receive/QrCode.tsx`:
- Around line 619-629: The <button> in QrCode.tsx currently lacks an explicit
type which defaults to "submit" and can cause unexpected form submissions later;
update the button element (the one with aria-label={`Copy ${title}`} and
data-testid={testId + '-address-copy'} that calls onCopy(value) and shows
{copied === value ? <CheckMarkIcon /> : <CopyIcon />}) to include type="button"
to prevent accidental submission.
In `@src/test/e2e/receive.test.ts`:
- Around line 85-89: The delete loop in receive.test.ts currently uses for (let
i = 0; i < sats.toString().length + 1; i++) which presses backspace one extra
time; either remove the +1 or, better, replace this brittle loop with a
deterministic clear (e.g., use the input element's clear/fill('') or delete
exactly the current value length) and/or add a comment explaining why an extra
delete is needed; update the test to use the input-clearing approach before
calling handleKeyboardInput so the test is robust across runs and reference the
test helper handleKeyboardInput and the keyboard-x test id when making the
change.
🪄 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: ac00a6e2-84b2-4649-a42c-fc8588e869ce
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (32)
package.jsonsrc/App.tsxsrc/components/Balance.tsxsrc/components/Button.tsxsrc/components/ButtonsOnBottom.tsxsrc/components/Content.tsxsrc/components/Details.tsxsrc/components/Empty.tsxsrc/components/Header.tsxsrc/components/InputAmount.tsxsrc/components/Keyboard.tsxsrc/components/NewPassword.tsxsrc/components/Strength.tsxsrc/components/SwapsList.tsxsrc/components/TransactionsList.tsxsrc/index.csssrc/ionic.csssrc/lib/fiat.tssrc/lib/format.tssrc/providers/fiat.tsxsrc/providers/flow.tsxsrc/screens/Settings/Logs.tsxsrc/screens/Wallet/Notes/Success.tsxsrc/screens/Wallet/Receive/QrCode.tsxsrc/screens/Wallet/Receive/Success.tsxsrc/screens/Wallet/Send/Form.tsxsrc/screens/Wallet/Send/Success.tsxsrc/test/e2e/init.test.tssrc/test/e2e/keyboard.test.tssrc/test/e2e/receive.test.tssrc/test/lib/format.test.tssrc/test/screens/wallet/index.test.tsx
✅ Files skipped from review due to trivial changes (6)
- src/test/screens/wallet/index.test.tsx
- src/components/NewPassword.tsx
- src/test/e2e/init.test.ts
- package.json
- src/providers/fiat.tsx
- src/test/e2e/keyboard.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/ButtonsOnBottom.tsx
| export function StrengthBars({ strength }: { strength: number }) { | ||
| const style = (col: number): React.CSSProperties => ({ | ||
| backgroundColor: col < strength ? `var(--${getColor(strength)})` : '', | ||
| border: '1px solid var(--dark20)', | ||
| height: '0.5rem', | ||
| height: '4px', | ||
| width: '100%', | ||
| }) | ||
|
|
||
| return ( | ||
| <IonGrid style={{ width: '100%' }}> | ||
| <IonRow> | ||
| <IonCol size='3'> | ||
| <div style={style(0)} /> | ||
| </IonCol> | ||
| <IonCol size='3'> | ||
| <div style={style(1)} /> | ||
| </IonCol> | ||
| <IonCol size='3'> | ||
| <div style={style(2)} /> | ||
| </IonCol> | ||
| <IonCol size='3'> | ||
| <div style={style(3)} /> | ||
| </IonCol> | ||
| </IonRow> | ||
| </IonGrid> | ||
| <div style={{ width: '100%' }}> | ||
| <FlexRow gap='0'> | ||
| {[0, 1, 2, 3].map((col) => ( | ||
| <div key={col} style={style(col)} /> | ||
| ))} | ||
| </FlexRow> | ||
| </div> |
There was a problem hiding this comment.
Preserve progress semantics and avoid full bars for medium strength.
With col < strength, a value like 3.25 renders all four bars even though getWord(3.25) is still medium. The native div replacement also drops progress semantics, so consumers rendering StrengthBars alone won’t announce the strength state.
Suggested adjustment
export function StrengthBars({ strength }: { strength: number }) {
+ const normalizedStrength = Math.max(0, Math.min(4, strength))
+ const filledBars = Math.floor(normalizedStrength)
+
const style = (col: number): React.CSSProperties => ({
- backgroundColor: col < strength ? `var(--${getColor(strength)})` : '',
+ backgroundColor: col < filledBars ? `var(--${getColor(normalizedStrength)})` : '',
height: '4px',
width: '100%',
})
return (
- <div style={{ width: '100%' }}>
+ <div
+ aria-label='Password strength'
+ aria-valuemax={4}
+ aria-valuemin={0}
+ aria-valuenow={normalizedStrength}
+ aria-valuetext={getWord(normalizedStrength)}
+ role='progressbar'
+ style={{ width: '100%' }}
+ >
<FlexRow gap='0'>
{[0, 1, 2, 3].map((col) => (
<div key={col} style={style(col)} />
))}
</FlexRow>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function StrengthBars({ strength }: { strength: number }) { | |
| const style = (col: number): React.CSSProperties => ({ | |
| backgroundColor: col < strength ? `var(--${getColor(strength)})` : '', | |
| border: '1px solid var(--dark20)', | |
| height: '0.5rem', | |
| height: '4px', | |
| width: '100%', | |
| }) | |
| return ( | |
| <IonGrid style={{ width: '100%' }}> | |
| <IonRow> | |
| <IonCol size='3'> | |
| <div style={style(0)} /> | |
| </IonCol> | |
| <IonCol size='3'> | |
| <div style={style(1)} /> | |
| </IonCol> | |
| <IonCol size='3'> | |
| <div style={style(2)} /> | |
| </IonCol> | |
| <IonCol size='3'> | |
| <div style={style(3)} /> | |
| </IonCol> | |
| </IonRow> | |
| </IonGrid> | |
| <div style={{ width: '100%' }}> | |
| <FlexRow gap='0'> | |
| {[0, 1, 2, 3].map((col) => ( | |
| <div key={col} style={style(col)} /> | |
| ))} | |
| </FlexRow> | |
| </div> | |
| export function StrengthBars({ strength }: { strength: number }) { | |
| const normalizedStrength = Math.max(0, Math.min(4, strength)) | |
| const filledBars = Math.floor(normalizedStrength) | |
| const style = (col: number): React.CSSProperties => ({ | |
| backgroundColor: col < filledBars ? `var(--${getColor(normalizedStrength)})` : '', | |
| height: '4px', | |
| width: '100%', | |
| }) | |
| return ( | |
| <div | |
| aria-label='Password strength' | |
| aria-valuemax={4} | |
| aria-valuemin={0} | |
| aria-valuenow={normalizedStrength} | |
| aria-valuetext={getWord(normalizedStrength)} | |
| role='progressbar' | |
| style={{ width: '100%' }} | |
| > | |
| <FlexRow gap='0'> | |
| {[0, 1, 2, 3].map((col) => ( | |
| <div key={col} style={style(col)} /> | |
| ))} | |
| </FlexRow> | |
| </div> | |
| ) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Strength.tsx` around lines 34 - 48, The StrengthBars component
currently fills bars using col < strength and drops native progress semantics;
change the fill logic to use Math.floor(strength) so fractional values (e.g.
3.25) only fill full bars for the integer portion (use col <
Math.floor(strength) in the style predicate), and restore accessibility by
adding progress semantics on the wrapper (e.g., set role="progressbar" with
aria-valuenow={strength}, aria-valuemin={0} and aria-valuemax={4}); keep
getColor(strength) and FlexRow as-is and avoid using an empty string for
backgroundColor (use undefined or omit the property when not filled).
| .content { | ||
| display: block; | ||
| position: relative; | ||
| flex: 1 1 0%; | ||
| width: 100%; | ||
| height: 100%; | ||
| font-family: var(--ion-font-family, inherit); | ||
| contain: size style; | ||
| margin: 0px; | ||
| padding: 0px; | ||
| } | ||
|
|
||
| .has-pill-navbar .content { | ||
| --padding-bottom: var(--pill-navbar-clearance); | ||
| } |
There was a problem hiding this comment.
--padding-bottom on .has-pill-navbar .content has no consumer.
The new .content rule (lines 209–219) sets padding: 0px directly and never references var(--padding-bottom). The .has-pill-navbar .content { --padding-bottom: var(--pill-navbar-clearance); } override is a carryover from Ionic's IonContent (which used that custom property internally) and now does nothing, so the pill navbar clearance is not actually applied to the content area. Either reference the variable in the base .content rule (e.g. padding-bottom: var(--padding-bottom, 0)) or drop the override.
🛠️ Proposed fix
.content {
display: block;
position: relative;
flex: 1 1 0%;
width: 100%;
height: 100%;
font-family: var(--ion-font-family, inherit);
contain: size style;
margin: 0px;
- padding: 0px;
+ padding: 0 0 var(--padding-bottom, 0) 0;
}📝 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.
| .content { | |
| display: block; | |
| position: relative; | |
| flex: 1 1 0%; | |
| width: 100%; | |
| height: 100%; | |
| font-family: var(--ion-font-family, inherit); | |
| contain: size style; | |
| margin: 0px; | |
| padding: 0px; | |
| } | |
| .has-pill-navbar .content { | |
| --padding-bottom: var(--pill-navbar-clearance); | |
| } | |
| .content { | |
| display: block; | |
| position: relative; | |
| flex: 1 1 0%; | |
| width: 100%; | |
| height: 100%; | |
| font-family: var(--ion-font-family, inherit); | |
| contain: size style; | |
| margin: 0px; | |
| padding: 0 0 var(--padding-bottom, 0) 0; | |
| } | |
| .has-pill-navbar .content { | |
| --padding-bottom: var(--pill-navbar-clearance); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/index.css` around lines 209 - 223, The .has-pill-navbar .content rule
sets --padding-bottom but the base .content never uses it; update the .content
rule to consume that custom property (e.g. add padding-bottom:
var(--padding-bottom, 0)) so the --pill-navbar-clearance value is applied, or
alternatively remove the .has-pill-navbar .content override if you choose not to
support that clearance; target the .content selector and the .has-pill-navbar
.content override and adjust the CSS to either reference --padding-bottom in
.content or delete the unused override.
There was a problem hiding this comment.
Arkana Code Review — wallet#534 (incremental)
Reviewed the new commit 6a4b1a39 ("increase viewport size in tests").
New commit: ✅ Fine
playwright.config.ts:32 — Adding viewport: { width: 1920, height: 1080 } to the Chrome test config is harmless. No concerns.
Previous findings still open
Neither 🔴 from my prior review has been addressed:
src/App.tsx:235— hardcodedwidth: '640px'still breaks mobile viewports. Needsmax-widthinstead.src/index.css:100-183— globalbutton { }selector still nukes every non-Button.tsxbutton in the app (PillNavbar, Balance eye toggle, DismissibleBanner, InAppBrowser copy, QR wrapper, ButtonOnInput). Scope it to a class.
Please fix those two before merge.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@playwright.config.ts`:
- Line 32: The current desktop project overrides the baseline viewport by
setting use: { ...devices['Desktop Chrome'], channel: 'chrome', viewport: {
width: 1920, height: 1080 } }, which hides regressions at the intended baseline
size; restore that project's viewport to the baseline 1280x800 (keep the same
project and its use object referencing devices['Desktop Chrome']) and add a new
separate project (e.g., name it "Desktop Chrome Large") that copies the same use
settings but sets viewport: { width: 1920, height: 1080 } so you preserve
baseline coverage while still testing a large-desktop variant.
🪄 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: 7030166d-ecb2-45e2-b724-75bc85f18b9c
📒 Files selected for processing (1)
playwright.config.ts
| { | ||
| name: 'Google Chrome', | ||
| use: { ...devices['Desktop Chrome'], channel: 'chrome' }, | ||
| use: { ...devices['Desktop Chrome'], channel: 'chrome', viewport: { width: 1920, height: 1080 } }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect Playwright viewport coverage and viewport-dependent tests.
# Expected: Either a desktop project remains at 1280x800, or tests explicitly cover that size.
rg -n -C3 'viewport|setViewportSize|innerWidth|1280|800|1920|1080' playwright.config.ts src/test/e2eRepository: arkade-os/wallet
Length of output: 2973
🏁 Script executed:
cat -n playwright.config.tsRepository: arkade-os/wallet
Length of output: 1252
Restore baseline desktop viewport coverage.
Line 32 overrides the global 1280x800 desktop viewport to 1920x1080 for the only desktop project, removing test coverage at the baseline width. Since the PR refactors button styling and navbar typography with regressions already reported, this masks exactly the kind of CSS issues that break at narrower desktop widths. Preserve the baseline project and add a separate large-desktop variant if needed.
Suggested adjustment
{
name: 'Google Chrome',
- use: { ...devices['Desktop Chrome'], channel: 'chrome', viewport: { width: 1920, height: 1080 } },
+ use: { ...devices['Desktop Chrome'], channel: 'chrome' },
+ },
+ {
+ name: 'Google Chrome Large',
+ use: { ...devices['Desktop Chrome'], channel: 'chrome', viewport: { width: 1920, height: 1080 } },
},📝 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.
| use: { ...devices['Desktop Chrome'], channel: 'chrome', viewport: { width: 1920, height: 1080 } }, | |
| { | |
| name: 'Google Chrome', | |
| use: { ...devices['Desktop Chrome'], channel: 'chrome' }, | |
| }, | |
| { | |
| name: 'Google Chrome Large', | |
| use: { ...devices['Desktop Chrome'], channel: 'chrome', viewport: { width: 1920, height: 1080 } }, | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@playwright.config.ts` at line 32, The current desktop project overrides the
baseline viewport by setting use: { ...devices['Desktop Chrome'], channel:
'chrome', viewport: { width: 1920, height: 1080 } }, which hides regressions at
the intended baseline size; restore that project's viewport to the baseline
1280x800 (keep the same project and its use object referencing devices['Desktop
Chrome']) and add a new separate project (e.g., name it "Desktop Chrome Large")
that copies the same use settings but sets viewport: { width: 1920, height: 1080
} so you preserve baseline coverage while still testing a large-desktop variant.
There was a problem hiding this comment.
Arkana Code Review — wallet#534 (incremental #2)
Reviewed new commit 4397770 ("more removals"). Four files changed: Text.tsx, Toggle.tsx, index.css, Vtxos.tsx.
🔴 MUST FIX
1. Debug console.log left in — src/components/Toggle.tsx:18
console.log('Toggle clicked, new value:', !checked)Remove before merge. This will spam the console on every toggle interaction across 8+ settings screens (Delegates, Backup, Haptics, Notifications, Assets/Settings, Boltz/Settings, etc).
2. Stale IonToggle import — src/components/Toggle.tsx:2
import { IonToggle } from '@ionic/react'IonToggle is no longer used — the component now renders a native checkbox. This is a dead import that will cause lint warnings and keeps Ionic in the bundle for this module.
3. .grid CSS uses descendant selectors, not direct children — src/index.css:219-231
.grid {
& div { /* matches ALL descendant divs, not just direct children */
display: flex;
& div { /* matches ALL div descendants of descendants */
flex: 1;
&:last-child { flex: 0; }
}
}
}In CSS nesting, & div is equivalent to .grid div (descendant), not .grid > div (direct child). This means the FlexCol div inside the first column of Vtxos.tsx:228 — and any div nested inside it — will also get display: flex; width: 100% forced on it. The flex: 1 / flex: 0 rules will also bleed into nested structures.
Fix: Use & > div for direct child selectors:
.grid {
& > div {
display: flex;
width: 100%;
& > div {
flex: 1;
&:last-child { flex: 0; }
}
}
}4. Row click removed from Toggle — UX regression — src/components/Toggle.tsx:27
The old code had <FlexRow between onClick={handleClick}> so users could tap anywhere on the row (label + toggle) to toggle. The new code removed onClick from FlexRow — now only clicking the tiny checkbox input toggles it. This is a significant UX regression on mobile where the toggle switch is a small target.
Fix: Restore onClick={handleClick} on FlexRow, or wrap the whole row in a <label> that associates with the input.
🟡 SHOULD FIX
5. readOnly doesn't work on checkboxes — src/components/Toggle.tsx:32
<input type='checkbox' checked={checked} readOnly onClick={handleClick} />HTML readOnly has no effect on checkboxes — it only applies to text inputs. Since this is a controlled React component, use onChange={handleClick} instead of readOnly onClick={handleClick} to properly handle the controlled input pattern. React will warn about a controlled checkbox without onChange in dev mode.
🟢 LOOKS GOOD
Text.tsx— Clean removal ofIonTextwrapper. Movingdata-testidto the<p>is correct. ✅Vtxos.tsx—IonGrid/IonRow/IonCol→ plain divs is fine in concept (pending.gridCSS fix above). ✅- Toggle CSS (
index.css:577-702) — The custom toggle/switch CSS implementation is thorough with proper states (hover, focus, active, disabled, checked). ✅
⚠️ Previous 🔴 findings STILL OPEN (from reviews #1 and #2)
src/App.tsx:236— hardcodedwidth: '640px'— still breaks mobile. Needsmax-width.src/index.css:100-183— globalbutton { }selector — still nukes every non-Button.tsxbutton. Scope to a class.
These have been flagged across two prior reviews. Please address them.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/Text.tsx (1)
53-66:⚠️ Potential issue | 🟠 MajorReset native paragraph margins after removing Ionic normalize.
Rendering a raw
<p>now reintroduces browser default margins, which can shift header/navbar text and any otherTextusage. Add an explicit margin reset to preserve the previous normalized layout.🎨 Proposed fix
const pStyle: any = { color: color ? `var(--${color})` : undefined, cursor: copy ? 'pointer' : undefined, fontFamily: heading ? 'var(--heading-font)' : undefined, fontSize, fontWeight: thin ? '400' : medium ? '500' : bold ? (heading ? '700' : '600') : undefined, letterSpacing: heading ? '-0.5px' : undefined, lineHeight: heading ? (bigger || big ? '1.2' : large ? '1.4' : '1.5') : tiny ? '1' : '1.5', + margin: 0, overflow: wrap ? undefined : 'hidden', textAlign: centered ? 'center' : right ? 'right' : undefined, textOverflow: wrap ? undefined : 'ellipsis', whiteSpace: wrap ? undefined : 'nowrap', wordBreak: 'break-word', }Also applies to: 82-84
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Text.tsx` around lines 53 - 66, The pStyle object in Text.tsx is missing an explicit margin reset, so add margin: 0 to pStyle (and the other similar style block referenced later) to override browser default <p> margins; update the style objects named pStyle (and the analogous style used around the second block at lines noted in the review) to include margin: 0 so raw <p> elements do not reintroduce native spacing.
♻️ Duplicate comments (2)
src/index.css (2)
101-180:⚠️ Potential issue | 🟠 MajorNavbar regression still not fully addressed.
The global
button { … }rule (uppercase, Geist Mono,font-size: 0.875rem,width: 100%,min-height: 40px,box-shadow: 0 4px …) still bleeds into.pill-nav-btn. The only override added at line 161 isbox-shadow: none; neither this block nor the.pill-nav-btnrule at lines 413–432 resetstext-transform,font-family,font-size,width, ormin-height, so nav labels will continue to render uppercase monospace at 0.875rem, with 100% width and 40px min-height — exactly the navbar typography regression@sahilc0reported.Either scope the global rule behind a dedicated class (e.g.
.btn) thatButton.tsxopts into, or reset the inherited properties inside.pill-nav-btn(not justbox-shadow).🛠️ Suggested reset on the existing
.pill-nav-btnrule (lines 413–432).pill-nav-btn { display: flex; flex-direction: column; align-items: center; justify-content: center; gap: 3px; min-width: 72px; min-height: 44px; + width: auto; padding: 6px 12px; border: none; border-radius: 999px; background: none; + text-transform: none; + font-family: inherit; + font-size: inherit; color: `#808080`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.css` around lines 101 - 180, The global button rule is leaking styles into the navbar; fix by either scoping the global rule to a class (e.g., replace the global selector button with .btn and update Button.tsx to use that class) or by fully resetting all inherited button properties inside .pill-nav-btn (reset text-transform, font-family, font-size, min-height, width, letter-spacing, padding, and any other layout/typography properties coming from the global button rule) so the nav labels stop rendering uppercase monospace 0.875rem full-width buttons.
209-238:⚠️ Potential issue | 🟡 Minor
.has-pill-navbar .content { --padding-bottom: … }still has no consumer.Same finding as the earlier review:
.contenthard-codespadding: 0pxand never referencesvar(--padding-bottom), so the pill-navbar clearance is not applied and content can sit under the floating pill navbar. Either consume the variable in.content's padding declaration, or drop the override.🛠️ Suggested fix
.content { display: block; position: relative; flex: 1 1 0%; width: 100%; height: 100%; font-family: var(--ion-font-family, inherit); contain: size style; margin: 0px; - padding: 0px; + padding: 0 0 var(--padding-bottom, 0) 0; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.css` around lines 209 - 238, The .has-pill-navbar .content rule sets --padding-bottom: var(--pill-navbar-clearance) but .content currently hard-codes padding: 0px so the clearance is never applied; update the .content rule (the .content selector) to consume the variable in its padding declaration (e.g. incorporate var(--padding-bottom) as the bottom padding) or remove the .has-pill-navbar .content override entirely; ensure you reference the CSS custom property names --padding-bottom and --pill-navbar-clearance when making the change so the pill-navbar clearance is actually applied.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/Toggle.tsx`:
- Line 18: Remove the debug console.log in the Toggle component: delete the line
that logs "Toggle clicked, new value:" and the computed !checked value (the
statement inside the onClick/handler that references checked in Toggle.tsx) so
no debug output remains in production; ensure the handler still updates
state/props as intended and run tests to confirm behavior unchanged.
- Around line 28-33: The native checkbox breaks string-attribute assertions; add
an explicit data attribute on the input and update tests to use it: in
Toggle.tsx modify the <input ... /> (the element using checked,
data-testid={testId} and onClick={handleClick}) to include data-checked={checked
? 'true' : 'false'} so tests can assert data-checked==='true'|'false' (then
update the listed e2e tests to check data-checked instead of the checked
attribute).
In `@src/index.css`:
- Around line 154-174: The modifier classes .outline, .red and .secondary
currently only set border-color but inherit a parent rule that set border: none,
so re-enable visible borders by explicitly restoring border-style and width
(e.g., set border: 1px solid <color> or add border-style: solid and
border-width: 1px) inside the .outline, .red and .secondary blocks; reference
the existing selectors .outline, .red and .secondary and use their current
border-color values (var(--dark50), var(--red), var(--black) respectively) when
restoring the border so the visual outline appears as intended.
---
Outside diff comments:
In `@src/components/Text.tsx`:
- Around line 53-66: The pStyle object in Text.tsx is missing an explicit margin
reset, so add margin: 0 to pStyle (and the other similar style block referenced
later) to override browser default <p> margins; update the style objects named
pStyle (and the analogous style used around the second block at lines noted in
the review) to include margin: 0 so raw <p> elements do not reintroduce native
spacing.
---
Duplicate comments:
In `@src/index.css`:
- Around line 101-180: The global button rule is leaking styles into the navbar;
fix by either scoping the global rule to a class (e.g., replace the global
selector button with .btn and update Button.tsx to use that class) or by fully
resetting all inherited button properties inside .pill-nav-btn (reset
text-transform, font-family, font-size, min-height, width, letter-spacing,
padding, and any other layout/typography properties coming from the global
button rule) so the nav labels stop rendering uppercase monospace 0.875rem
full-width buttons.
- Around line 209-238: The .has-pill-navbar .content rule sets --padding-bottom:
var(--pill-navbar-clearance) but .content currently hard-codes padding: 0px so
the clearance is never applied; update the .content rule (the .content selector)
to consume the variable in its padding declaration (e.g. incorporate
var(--padding-bottom) as the bottom padding) or remove the .has-pill-navbar
.content override entirely; ensure you reference the CSS custom property names
--padding-bottom and --pill-navbar-clearance when making the change so the
pill-navbar clearance is actually applied.
🪄 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: 057058e2-2a0b-4411-8e61-ec9026d99545
📒 Files selected for processing (4)
src/components/Text.tsxsrc/components/Toggle.tsxsrc/index.csssrc/screens/Settings/Vtxos.tsx
There was a problem hiding this comment.
Arkana Code Review — wallet#534 (incremental #3)
Reviewed new commits 13586bf and b9080e0 ("more removals", "fixes"). 12 files changed.
Previous feedback addressed ✅
All issues from my last two reviews are fixed:
Toggle.tsx:console.logremoved,data-checkedattribute added on label,onClick→onChangeon checkbox input — all correct.index.css: Globalbuttonselector scoped to.buttonclass — fixes the navbar style leakage I flagged. Grid selectors tightened with> div— prevents unintended nesting matches.- Tests: All updated to assert against
data-checkedinstead of nativecheckedattribute.
New changes look clean
App.tsx:width: '640px'→maxWidth: '640px'— correct fix for narrow viewports.Button.tsx: Newcopyvariant +.buttonclass applied —.copyrule (line 136) properly stripsbox-shadowand setsaspect-ratio: 1, so copy buttons render correctly.Checkbox.tsx:IonCheckboxreplaced with uncontrolled local-state component. Only consumer isReset.tsx(confirmation gate: "I have backed up my wallet") — uncontrolled pattern is appropriate here.QrCode.tsx: Raw<button className='copy'>→<Button copy>— consistent with the.buttonclass migration.Grid.tsx: Trivial wrapper, used inVtxos.tsx. Fine.
No protocol-critical changes
No VTXO handling, transaction signing, forfeit paths, or exit logic touched. All changes are UI/CSS refactoring.
LGTM. Ship it. 🚢
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com
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 (3)
src/components/Button.tsx (3)
76-76:⚠️ Potential issue | 🟠 MajorHardcoded
margin: '4px 0'on everyButton— likely contributes to the reported button-styling regression.Baking margin into the component forces vertical spacing on every instance, including icon-only
copybuttons used inline inside rows (e.g.AddressLineinQrCode.tsx), and prevents consumers from overriding layout. Move this to the caller that actually needs it (e.g.ButtonsOnBottom) or to a CSS class so it can be composed/overridden. This is a plausible cause of the "button styling incorrect" regression sahilc0 flagged on the PR.🛠️ Proposed fix
- style={{ margin: '4px 0' }}and apply spacing via the parent layout (e.g. flex
gapalready used inButtonsOnBottom/FlexCol).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Button.tsx` at line 76, The Button component currently hardcodes style={{ margin: '4px 0' }}, forcing vertical spacing everywhere; remove this inline margin from the Button component (the one defined as Button in src/components/Button.tsx) and instead apply spacing at the caller or via a composable CSS class — e.g., remove the margin from Button and add layout spacing in ButtonsOnBottom (or use flex gap in FlexCol) or add a utility CSS class for buttons used inline (such as the copy button inside AddressLine in QrCode.tsx) so consumers can control/override spacing.
9-24:⚠️ Potential issue | 🔴 Critical
Buttondrops unknown props likearia-labelanddata-testid— accessibility and test-id regressions.
ButtonPropsonly lists named props and the component destructures an explicit set; unknown props (e.g.aria-label,data-testid) are not forwarded to the underlying<button>.AddressLineinsrc/screens/Wallet/Receive/QrCode.tsx(lines 619–629) passes botharia-label={...}anddata-testid={testId + '-address-copy'}to<Button>; with the current shape those attributes silently disappear from the DOM, breaking screen-reader labels and any tests that querybip21-address-copy,invoice-address-copy, etc. (The newtestIdprop is the intended replacement fordata-testid, but callers aren't using it.)Either accept/forward these props on
Button, or fix all call sites. Forwarding is safer:🛠️ Proposed fix (forward aria-* and data-* via rest props)
-interface ButtonProps { +interface ButtonProps extends React.ButtonHTMLAttributes<HTMLButtonElement> { children?: ReactNode clear?: boolean copy?: boolean - disabled?: boolean fancy?: boolean icon?: ReactElement label?: string loading?: boolean main?: boolean onClick: (event: any) => void outline?: boolean red?: boolean secondary?: boolean testId?: string } export default function Button({ children, clear, copy, disabled, fancy, icon, label, loading, main, onClick, outline, red, secondary, testId, + ...rest }: ButtonProps) {and spread
{...rest}on the<button>element.Also applies to: 26-45
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Button.tsx` around lines 9 - 24, Button currently declares ButtonProps and destructures only named props, so unknown attributes like aria-label and data-testid passed from callers (e.g., AddressLine) are dropped; change ButtonProps to extend React.ButtonHTMLAttributes<HTMLButtonElement> (or include an index signature) so it accepts aria-* and data-* attributes, keep the explicit testId prop for compatibility, and in the Button component collect the rest via ...rest and spread {...rest} onto the rendered <button> element (while still preserving any explicit mapping from testId to data-testid if desired).
65-77:⚠️ Potential issue | 🟠 MajorMissing explicit
type='button'— risks unintended form submissions.The previous
IonButtondefaulted totype="button", but a native<button>defaults totype="submit"when rendered inside a<form>. AnyButtonplaced in a form context will now submit the form on click. Settype='button'explicitly (callers can still override via a prop if needed for submit buttons).🛠️ Proposed fix
<button + type='button' className={className} disabled={disabled} onClick={handleClick}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Button.tsx` around lines 65 - 77, The native <button> in the Button component is missing an explicit type and will default to "submit" inside forms; update the rendered <button> element to include type={type ?? 'button'} (or type={props.type || 'button'}) and add a 'type' prop to the component's props with a default of 'button' (or optional prop passed through) so callers can override when they need a submit button; ensure the change is applied where the JSX button is defined alongside existing attributes (className, disabled, onClick={handleClick}, onMouseDown={handlePressStart}, onMouseUp={handlePressEnd}, data-testid={testId}, etc.).
🧹 Nitpick comments (2)
src/components/Checkbox.tsx (2)
37-57: Use CSS variables and fix misleading style props.
'#f24c58'/'#7c7c7c'are hardcoded; the rest of the codebase themes via CSS vars (e.g.var(--dark50)). Using vars keeps light/dark theming consistent with the broader de-Ionic migration.svgStyle.borderColorhas no effect without abordershorthand/borderWidth; either remove it or set an actual border.backgroundon the<svg>does render the filled box, but namingsvgStyleafter a “box” and usingbackgroundplus a deadborderColoris confusing.- Minor:
strokeDashoffset: 0andstrokeDasharray: 30are no-ops without a transition; if no animation is intended you can drop them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Checkbox.tsx` around lines 37 - 57, The BoxIcon component uses hardcoded colors and misleading style props; switch the literal colors ('#f24c58'/'#7c7c7c') to theme CSS variables (e.g. var(--accent) / var(--muted) or existing vars used across the codebase) and apply them via style values or className so light/dark theming follows the rest of the app; remove the unused svgStyle.borderColor (or replace it with an actual border like border: '2px solid var(...)' if a border is needed) and rename svgStyle to reflect that it styles the box container if you keep it, and drop strokeDasharray/strokeDashoffset from pathStyle unless you add an animation—update the BoxIcon function and its svg/path style objects accordingly.
11-18: Checkbox is uncontrolled-only and drops the checked value fromonChange.
checkedis tracked purely in local state with nochecked/defaultCheckedprop, andonChange: () => voiddoesn't forward the new value. This makes it impossible for the parent to (a) set an initial value, (b) reset/override the state, or (c) know the new checked state without shadowing it. Consider:♻️ Proposed API
interface CheckboxProps { - onChange: () => void + checked?: boolean + defaultChecked?: boolean + onChange: (checked: boolean) => void text: string } -export default function Checkbox({ onChange, text }: CheckboxProps) { - const [checked, setChecked] = useState(false) +export default function Checkbox({ checked: controlled, defaultChecked = false, onChange, text }: CheckboxProps) { + const [internal, setInternal] = useState(defaultChecked) + const checked = controlled ?? internal const handleChange = () => { - setChecked(!checked) + const next = !checked + if (controlled === undefined) setInternal(next) hapticLight() - onChange() + onChange(next) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Checkbox.tsx` around lines 11 - 18, The Checkbox component currently manages checked state internally and calls onChange() with no value; update the CheckboxProps and component so it supports both controlled and uncontrolled usage: add optional props "checked?: boolean" and "defaultChecked?: boolean" to CheckboxProps, change onChange to accept the new boolean (onChange: (checked: boolean) => void), initialize internal state from defaultChecked, sync internal state with the "checked" prop when provided (e.g., useEffect to update local state when props.checked changes), and modify handleChange in the Checkbox function to compute the newChecked value, set local state, call hapticLight(), and invoke onChange(newChecked) so parents can control or be notified of the new value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/App.tsx`:
- Line 3: Remove the commented import line "// import
'@ionic/react/css/normalize.css'" from src/App.tsx so the file no longer
contains the disabled import; simply delete that line to avoid noise and
accidental re-enabling.
In `@src/components/Checkbox.tsx`:
- Around line 27-35: The custom checkbox lost native semantics and keyboard
support; update the component around FlexRow/BoxIcon/Text and handleChange to
restore accessibility by either adding a visually-hidden native <input
type="checkbox"> synchronized with checked and onChange, or make the clickable
wrapper keyboard-operable by adding role="checkbox", aria-checked={checked},
tabIndex={0}, and a keyDown handler that calls handleChange for Space/Enter;
also ensure the Text label is associated (wrap FlexRow and Text in a <label> or
use htmlFor) so assistive tech announces the label and checked state correctly.
In `@src/screens/Wallet/Receive/QrCode.tsx`:
- Around line 619-629: Replace the dropped attribute by using the Button
component's testId prop instead of data-testid and keep the aria-label prop so
it can be forwarded; specifically update the instance that renders Button (with
props aria-label={`Copy ${title}`}, copy, onClick stopping propagation and
children showing {copied === value ? <CheckMarkIcon /> : <CopyIcon />}) to pass
testId={testId + '-address-copy'} (not data-testid) and retain aria-label so the
Button can forward it. Also confirm the Button component forwards aria-label
(see the Button component implementation) so accessibility is preserved.
---
Outside diff comments:
In `@src/components/Button.tsx`:
- Line 76: The Button component currently hardcodes style={{ margin: '4px 0' }},
forcing vertical spacing everywhere; remove this inline margin from the Button
component (the one defined as Button in src/components/Button.tsx) and instead
apply spacing at the caller or via a composable CSS class — e.g., remove the
margin from Button and add layout spacing in ButtonsOnBottom (or use flex gap in
FlexCol) or add a utility CSS class for buttons used inline (such as the copy
button inside AddressLine in QrCode.tsx) so consumers can control/override
spacing.
- Around line 9-24: Button currently declares ButtonProps and destructures only
named props, so unknown attributes like aria-label and data-testid passed from
callers (e.g., AddressLine) are dropped; change ButtonProps to extend
React.ButtonHTMLAttributes<HTMLButtonElement> (or include an index signature) so
it accepts aria-* and data-* attributes, keep the explicit testId prop for
compatibility, and in the Button component collect the rest via ...rest and
spread {...rest} onto the rendered <button> element (while still preserving any
explicit mapping from testId to data-testid if desired).
- Around line 65-77: The native <button> in the Button component is missing an
explicit type and will default to "submit" inside forms; update the rendered
<button> element to include type={type ?? 'button'} (or type={props.type ||
'button'}) and add a 'type' prop to the component's props with a default of
'button' (or optional prop passed through) so callers can override when they
need a submit button; ensure the change is applied where the JSX button is
defined alongside existing attributes (className, disabled,
onClick={handleClick}, onMouseDown={handlePressStart},
onMouseUp={handlePressEnd}, data-testid={testId}, etc.).
---
Nitpick comments:
In `@src/components/Checkbox.tsx`:
- Around line 37-57: The BoxIcon component uses hardcoded colors and misleading
style props; switch the literal colors ('#f24c58'/'#7c7c7c') to theme CSS
variables (e.g. var(--accent) / var(--muted) or existing vars used across the
codebase) and apply them via style values or className so light/dark theming
follows the rest of the app; remove the unused svgStyle.borderColor (or replace
it with an actual border like border: '2px solid var(...)' if a border is
needed) and rename svgStyle to reflect that it styles the box container if you
keep it, and drop strokeDasharray/strokeDashoffset from pathStyle unless you add
an animation—update the BoxIcon function and its svg/path style objects
accordingly.
- Around line 11-18: The Checkbox component currently manages checked state
internally and calls onChange() with no value; update the CheckboxProps and
component so it supports both controlled and uncontrolled usage: add optional
props "checked?: boolean" and "defaultChecked?: boolean" to CheckboxProps,
change onChange to accept the new boolean (onChange: (checked: boolean) =>
void), initialize internal state from defaultChecked, sync internal state with
the "checked" prop when provided (e.g., useEffect to update local state when
props.checked changes), and modify handleChange in the Checkbox function to
compute the newChecked value, set local state, call hapticLight(), and invoke
onChange(newChecked) so parents can control or be notified of the new value.
🪄 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: 3a4a8574-7dec-4879-8e63-818392e3313b
📒 Files selected for processing (12)
src/App.tsxsrc/components/Button.tsxsrc/components/Checkbox.tsxsrc/components/Grid.tsxsrc/components/Toggle.tsxsrc/index.csssrc/screens/Settings/Vtxos.tsxsrc/screens/Wallet/Receive/QrCode.tsxsrc/test/e2e/delegate.test.tssrc/test/screens/settings/backup.test.tsxsrc/test/screens/settings/delegates.test.tsxsrc/test/screens/settings/notifications.test.tsx
✅ Files skipped from review due to trivial changes (3)
- src/test/screens/settings/notifications.test.tsx
- src/screens/Settings/Vtxos.tsx
- src/components/Grid.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/Toggle.tsx
- src/index.css
| return ( | ||
| <div style={style}> | ||
| <FlexRow> | ||
| <IonCheckbox labelPlacement='end' onIonChange={handleChange}> | ||
| {text} | ||
| </IonCheckbox> | ||
| <FlexRow onClick={handleChange} gap='0.5rem'> | ||
| <BoxIcon checked={checked} /> | ||
| <Text small>{text}</Text> | ||
| </FlexRow> | ||
| </div> | ||
| ) | ||
| } |
There was a problem hiding this comment.
Accessibility regression: custom checkbox is not keyboard-operable and exposes no semantics.
Replacing IonCheckbox with a clickable <div> + SVG drops the native checkbox semantics and keyboard support. The FlexRow wrapper renders a non-focusable <div> with an onClick, so users can't tab to the control, toggle it with Space, and assistive tech won't announce it as a checkbox or report its checked state. Please restore accessibility by either using a visually-hidden native <input type="checkbox"> overlaying the SVG, or adding role="checkbox", aria-checked={checked}, tabIndex={0}, and a keydown handler for Space/Enter. Also consider associating the Text label with the control (e.g. wrapping in a <label>).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Checkbox.tsx` around lines 27 - 35, The custom checkbox lost
native semantics and keyboard support; update the component around
FlexRow/BoxIcon/Text and handleChange to restore accessibility by either adding
a visually-hidden native <input type="checkbox"> synchronized with checked and
onChange, or make the clickable wrapper keyboard-operable by adding
role="checkbox", aria-checked={checked}, tabIndex={0}, and a keyDown handler
that calls handleChange for Space/Enter; also ensure the Text label is
associated (wrap FlexRow and Text in a <label> or use htmlFor) so assistive tech
announces the label and checked state correctly.
* fix swap page * lint * add tests
The vertical gap between the Add amount/Copy buttons and the Share button was too tight. Changed FlexCol gap from 0 to 0.75rem. Co-authored-by: Sahil Chaturvedi <sahilc0@users.noreply.github.com>
There was a problem hiding this comment.
Arkana Code Review — wallet#534 (incremental #4)
Reviewed 14 new commits since b9080e0. Focus: Refresher rewrite, Input migration from Ionic, Swap.tsx fixes, new tests.
🔴 Bug — Refresher.tsx: stale closure + race condition
src/components/Refresher.tsx:14-49
The useEffect has an empty dependency array [], but handleTouchEnd calls handleRefresh which is defined outside the effect and captures svcWallet/reloadWallet from the component's render scope. Since event listeners are registered once on mount, the handleRefresh reference inside handleTouchEnd is frozen to the initial render's version. If the wallet context updates after mount (which it will — wallet loads asynchronously), the refresh will call stale svcWallet?.reload() / reloadWallet().
Fix: move handleRefresh inside the effect, or use a ref to hold the latest function:
const handleRefreshRef = useRef(handleRefresh)
handleRefreshRef.current = handleRefresh
// inside useEffect:
const handleTouchEnd = () => {
if (triggered) handleRefreshRef.current()
}Additionally:
let xxx = false— rename this to something meaningful liketriggeredorshouldRefresh.xxxis never reset — after a refresh completes, if the user does another pull-down gesture,xxxis stilltruefrom the previous gesture (the variable lives in the effect closure, not re-created per gesture). Thetouchstarthandler should reset it:xxx = false.- No minimum pull distance — any tiny downward movement at
scrollY === 0triggers the refresh. Add a threshold (e.g.,touchDiff > 50). - No guard against concurrent refreshes — rapid pulls can fire multiple
svcWallet?.reload()concurrently. Add anisRefreshingguard. { passive: false }missing —e.preventDefault()intouchmovewill throw a console warning on Chrome because touch events are passive by default. Usedocument.addEventListener('touchmove', handleTouchMove, { passive: false }).
🟡 Typo — alignItems: 'strech'
src/components/InputPassword.tsx:28 and src/components/InputWithScanner.tsx:61
alignItems: 'strech' // should be 'stretch'This silently fails (CSS ignores invalid values), so the flex items won't align as intended.
✅ Swap.tsx fixes — correct
src/screens/Apps/Boltz/Swap.tsx:114-131
The submarine swap sent/received inversion is correct: expectedAmount is what the user pays (sent), and the decoded invoice amount is what the recipient gets (received). The onchainAmount ?? 0 null coalescing on reverse swaps is good defensive coding. The kind label fix from 'Chain Swap' to 'Reverse Swap' is also correct. New e2e tests at src/test/e2e/swap.test.ts:50-156 validate these values end-to-end — good coverage.
✅ Input migration — clean
The Ionic → native HTML input migration across Input.tsx, InputAmount.tsx, InputPassword.tsx, InputWithScanner.tsx is straightforward. onIonInput → onChange, setFocus() → focus(), IonInput → <input> — all correct. The type='button' addition on Button.tsx:68 prevents accidental form submissions — good.
Test selectors updated consistently from ion-input[name="..."] input to input[name="..."] across all e2e tests.
Summary
The Refresher rewrite has real bugs (stale closure, no debounce, missing passive flag). The rest of the changes are solid. Requesting changes for the Refresher issues — the rest can ship as-is (fix the typos while you're at it).
* lint * add lnurl tests Co-authored-by: Copilot <copilot@github.com> --------- Co-authored-by: Copilot <copilot@github.com>
* fix(ui): top-align tx history rows when assets are present When a transaction has assets, the right column (sats + fiat + asset info) is taller than the left column (icon + label + date). The outer FlexRow used alignItems 'center' by default, pushing the sats amount above the label text. Switch to alignItems 'start' so both columns begin at the same vertical position. Also adds explicit 'between' to the outer FlexRow and removes a dead alignItems on the non-flex wrapper div. * show only 2 coins in the rigth side of a tx in txs list (#550) --------- Co-authored-by: João Bordalo <bordalix@users.noreply.github.com>
Co-authored-by: Copilot <copilot@github.com>
|
tracking some bugs, ios PWA here:
|
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
|
woohoooo fixed! @bordalix thank you! |
4 participants
First PR in the refactor to eliminate ionic.
Removes ion-button and ionic/normalize.
Summary by CodeRabbit
Refactor
New Features
Bug Fixes
Tests