Keyboard navigation #283
Keyboard navigation #283
Conversation
WalkthroughAdds a new Focusable component and applies keyboard focus/activation across many UI elements; refactors input-focus effects to respond to a focus prop; unifies swap types into Changes
Sequence Diagram(s)(None — changes are pervasive UI accessibility additions and typing updates without a single new multi-component sequential flow that requires visualization.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (5)
src/screens/Wallet/Send/Form.tsx (1)
366-367: Smart focus management for keyboard navigation looks good!The context-aware focus logic effectively guides users to missing fields based on their current position, which aligns well with the PR's keyboard accessibility goals.
💡 Optional: Make the amount check more explicit
The
!amountcheck uses truthiness, which treats0as missing. While this works given that amounts < 1 are invalid anyway, being more explicit improves code clarity:- if (!amount && focus === 'recipient') setFocus('amount') + if ((amount === undefined || amount === 0) && focus === 'recipient') setFocus('amount')Alternatively, if you want to keep it concise and only check for undefined:
- if (!amount && focus === 'recipient') setFocus('amount') + if (amount === undefined && focus === 'recipient') setFocus('amount')src/providers/navigation.tsx (1)
234-235: Consider using a more descriptive variable name.The variable
cis not very descriptive. Consider renaming it tomainElementormainContentfor better code readability.🔎 Proposed refactor
- const c = document.querySelector('[role="main"]') as HTMLElement | null - if (c) c.focus() + const mainElement = document.querySelector('[role="main"]') as HTMLElement | null + if (mainElement) mainElement.focus()src/screens/Settings/General.tsx (1)
22-34: Consider consolidating keyboard and mouse handlers on a single element.The current structure creates nested interactive elements: the outer
divhandles keyboard events while the innerFlexRowhandles click events. This works but is not ideal for accessibility.Consider consolidating both handlers on the same element to avoid nested interactivity and provide a simpler accessibility tree. You could add the
onClickandonKeyDownhandlers to the same element (either the div or the FlexRow).🔎 Proposed refactor
const Row = ({ option, value }: { option: SettingsOptions; value: string }) => ( - <div className='focusable' tabIndex={0} onKeyDown={(e) => handleKeyDown(e, option)}> - <FlexRow between padding='0.8rem 0' onClick={() => setOption(option)}> + <div + className='focusable' + tabIndex={0} + onKeyDown={(e) => handleKeyDown(e, option)} + onClick={() => setOption(option)} + > + <FlexRow between padding='0.8rem 0'> <Text capitalize thin> {option} </Text> <FlexRow end> <Text small thin color='dark50'> {value} </Text> <ArrowIcon /> </FlexRow> </FlexRow> </div> )src/components/ExpandAddresses.tsx (1)
37-56: Consider scoping the keyboard listener to avoid conflicts.The global window keyboard listener for ArrowDown/ArrowUp may interfere with other components' keyboard navigation (e.g., scrolling lists, other arrow key handlers). Consider adding the listener only when the component is focused or checking if the target is within this component's DOM tree.
🔎 Suggested approach
Instead of a global window listener, consider adding the keyboard handler to a focused container:
- useEffect(() => { - const handleArrowDown = (event: KeyboardEvent) => { - if (event.key === 'ArrowDown') { - if (!expand) { - setExpand(true) - bip21ref.current?.focus() - } - } - if (event.key === 'ArrowUp') { - if (expand) { - setExpand(false) - } - } - } - window.addEventListener('keydown', handleArrowDown) - return () => window.removeEventListener('keydown', handleArrowDown) - }, [expand]) + const handleKeyDown = (event: React.KeyboardEvent) => { + if (event.key === 'ArrowDown' && !expand) { + setExpand(true) + bip21ref.current?.focus() + } + if (event.key === 'ArrowUp' && expand) { + setExpand(false) + } + }Then attach
onKeyDown={handleKeyDown}to the main container div.src/components/SwapsList.tsx (1)
72-72: Consider renamingtabindexvariable totabIndexfor consistency.While functionally correct, the lowercase
tabindexvariable name (line 72) is inconsistent with React'stabIndexprop naming convention and may cause confusion.🔎 Suggested fix
- const tabindex = focusable ? 0 : -1 + const tabIndexValue = focusable ? 0 : -1And update the usage:
- tabIndex={tabindex} + tabIndex={tabIndexValue}Also applies to: 119-119
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
src/App.tsxsrc/components/ExpandAddresses.tsxsrc/components/FlexCol.tsxsrc/components/FlexRow.tsxsrc/components/InputAmount.tsxsrc/components/InputPassword.tsxsrc/components/InputWithScanner.tsxsrc/components/Menu.tsxsrc/components/SwapsList.tsxsrc/components/TransactionsList.tsxsrc/index.csssrc/ionic.csssrc/providers/navigation.tsxsrc/screens/Apps/Index.tsxsrc/screens/Settings/General.tsxsrc/screens/Wallet/Receive/Amount.tsxsrc/screens/Wallet/Send/Form.tsx
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-07-21T18:25:36.391Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 121
File: src/screens/Init/Restore.tsx:58-58
Timestamp: 2025-07-21T18:25:36.391Z
Learning: In src/screens/Init/Restore.tsx, the Input component for private key restoration intentionally omits the `value` prop to make it uncontrolled. This is a security feature to prevent the private key from being displayed if users navigate away and return to the screen, avoiding potential exposure of sensitive data in the UI.
Applied to files:
src/components/InputPassword.tsxsrc/components/InputAmount.tsxsrc/screens/Wallet/Send/Form.tsxsrc/components/InputWithScanner.tsx
📚 Learning: 2025-08-06T09:53:28.428Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 134
File: src/screens/Settings/Password.tsx:34-39
Timestamp: 2025-08-06T09:53:28.428Z
Learning: In src/screens/Settings/Password.tsx, the saveNewPassword function doesn't need error handling for getPrivateKey because it's only called after authentication is verified. The useEffect validates the oldPassword using isValidPassword() which internally calls getPrivateKey(), and only sets authenticated=true if successful. The saveNewPassword function has a guard clause that returns early if not authenticated, so getPrivateKey(oldPassword) is guaranteed to work when called.
Applied to files:
src/components/InputPassword.tsx
📚 Learning: 2025-12-20T10:09:41.030Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 278
File: src/providers/annoucements.tsx:32-50
Timestamp: 2025-12-20T10:09:41.030Z
Learning: In src/providers/annoucements.tsx, the useEffect hook depends on [config] (not just [configLoaded]) because it needs to react to config changes after nostr restoration. The seen.current ref guard prevents showing multiple announcements per session, even though the effect may re-run on config updates.
Applied to files:
src/components/InputPassword.tsxsrc/components/InputAmount.tsxsrc/components/ExpandAddresses.tsxsrc/screens/Apps/Index.tsx
📚 Learning: 2025-06-30T18:33:29.839Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
Applied to files:
src/App.tsxsrc/screens/Wallet/Send/Form.tsx
🧬 Code graph analysis (6)
src/App.tsx (3)
src/components/FlexCol.tsx (1)
FlexCol(17-48)src/icons/Wallet.tsx (1)
WalletIcon(1-12)src/icons/Apps.tsx (1)
AppsIcon(1-12)
src/components/ExpandAddresses.tsx (7)
src/components/FlexRow.tsx (1)
FlexRow(19-52)src/components/FlexCol.tsx (1)
FlexCol(17-48)src/components/Text.tsx (2)
TextSecondary(91-97)Text(25-79)src/lib/format.ts (1)
prettyLongText(76-83)src/components/Shadow.tsx (1)
Shadow(18-55)src/icons/CheckMark.tsx (1)
CheckMarkIcon(1-11)src/icons/Copy.tsx (1)
CopyIcon(1-13)
src/components/SwapsList.tsx (2)
src/components/Empty.tsx (1)
EmptySwapList(46-54)src/components/Text.tsx (1)
TextLabel(81-89)
src/components/TransactionsList.tsx (2)
src/lib/types.ts (1)
Tx(71-81)src/components/Text.tsx (3)
Text(25-79)TextSecondary(91-97)TextLabel(81-89)
src/components/Menu.tsx (4)
src/components/FlexCol.tsx (1)
FlexCol(17-48)src/components/FlexRow.tsx (1)
FlexRow(19-52)src/components/Text.tsx (1)
Text(25-79)src/icons/Arrow.tsx (1)
ArrowIcon(1-7)
src/screens/Settings/General.tsx (3)
src/components/FlexRow.tsx (1)
FlexRow(19-52)src/components/Text.tsx (1)
Text(25-79)src/icons/Arrow.tsx (1)
ArrowIcon(1-7)
🪛 Biome (2.1.2)
src/components/TransactionsList.tsx
[error] 58-58: Do not shadow the global "Date" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: Cloudflare Pages: wallet-mutinynet
🔇 Additional comments (14)
src/components/TransactionsList.tsx (2)
1-1: LGTM!The additional React hooks are correctly imported for the focus management implementation.
17-17: LGTM!The signature change and tabindex computation correctly implement the focusable behavior for keyboard navigation.
Also applies to: 26-26
src/screens/Wallet/Receive/Amount.tsx (1)
119-119: LGTM! Good defensive programming.The early return guard prevents proceeding without a valid amount, providing defense-in-depth alongside the existing disabled button logic.
src/index.css (2)
157-159: Verify that all focusable elements have visible focus indicators.Removing the default
outlinefor:focus-visiblecan harm keyboard accessibility if not all interactive elements provide alternative focus indicators. Ensure that all focusable elements (buttons, links, inputs, custom interactive elements) have visible focus states, either through the.focusableclass or other styling mechanisms.
161-166: LGTM! Custom focus styling aligns with the design system.The
.focusableclass provides consistent focus styling using the theme's focus background variable, supporting keyboard accessibility across the application.src/screens/Settings/General.tsx (1)
17-19: LGTM! Good keyboard accessibility pattern.The
handleKeyDownfunction correctly handles Enter key activation, providing keyboard accessibility for the settings rows.src/ionic.css (1)
30-30: LGTM! Focused background variables support keyboard navigation.The new
--ion-background-focusedCSS variables provide consistent focus styling across light and dark themes, supporting the keyboard accessibility enhancements throughout the application.Also applies to: 55-55, 146-146
src/components/FlexCol.tsx (1)
13-13: LGTM! Clean tabIndex prop forwarding.The tabIndex prop is correctly added to the interface, destructured, and forwarded to the root div, enabling focus management for FlexCol components.
Also applies to: 27-27, 44-44
src/components/ExpandAddresses.tsx (1)
70-80: Good keyboard accessibility for ExpandLine.The focusable wrapper with tabIndex={0} and onKeyDown handler enables proper keyboard interaction for copying addresses.
src/components/FlexRow.tsx (1)
15-15: LGTM! Clean tabIndex prop forwarding.The tabIndex prop is correctly added and forwarded to the root div, enabling keyboard focus management for FlexRow components. This mirrors the FlexCol implementation.
Also applies to: 31-31, 48-48
src/components/Menu.tsx (1)
31-33: LGTM! Proper keyboard navigation implementation.The focusable wrapper with Enter key handling enables keyboard navigation while preserving the existing click behavior. The pattern correctly separates focus management from the visual/interactive elements.
Also applies to: 38-48
src/components/SwapsList.tsx (1)
51-59: Excellent focus management implementation!The implementation properly handles:
- Focusable state toggle via Enter/Space/ArrowDown
- Escape key to unfocus and return focus to the header
- Conditional tabIndex based on focusable state
- Ref management to restore focus
This aligns well with the PR objectives for keyboard navigation in transaction lists.
Also applies to: 109-112, 134-134, 137-137, 173-182, 188-193
src/components/InputWithScanner.tsx (1)
49-49: LGTM! Proper ref and value binding.The ref and value props are correctly applied to the IonInput, enabling focus management and controlled input behavior.
Also applies to: 51-51
src/screens/Apps/Index.tsx (1)
53-53: Excellent keyboard navigation and focus management for app cards!The implementation provides:
- Keyboard activation via Enter key
- Conditional tabIndex based on live status (preventing focus on non-live apps)
- Auto-focus on the first app card on mount
- Proper ref forwarding through the first prop
This aligns well with the PR's accessibility goals.
Also applies to: 68-70, 73-81, 106-110, 124-124
Deploying wallet-bitcoin with
|
| Latest commit: |
b0b761f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://58a30ce5.wallet-bitcoin.pages.dev |
| Branch Preview URL: | https://keyboard-navigation.wallet-bitcoin.pages.dev |
Deploying wallet-mutinynet with
|
| Latest commit: |
b0b761f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d015efdf.arkade-wallet.pages.dev |
| Branch Preview URL: | https://keyboard-navigation.arkade-wallet.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/components/Focusable.tsx (1)
1-36: LGTM! Well-implemented keyboard accessibility wrapper.The Focusable component correctly implements keyboard activation for interactive elements:
- Properly handles Enter and Space keys
- Uses appropriate ARIA attributes and semantic roles
- Includes stopPropagation and preventDefault to prevent conflicts
- Sets tabIndex={0} for natural keyboard navigation
💡 Optional: Consider more descriptive default aria-label
The default aria-label of "Focusable element" is quite generic. Consider making the
ariaLabelprop required, or provide a more context-specific default. This would improve screen reader experience.export default function Focusable({ ariaLabel, role, children, fit, onKeyDown }: FocusableProps) { // ... return ( <div tabIndex={0} style={style} className='focusable' role={role ?? 'button'} onKeyDown={handleKeyDown} - aria-label={ariaLabel ?? 'Focusable element'} + aria-label={ariaLabel ?? 'Interactive element'} > {children} </div> ) }Alternatively, make
ariaLabelrequired to ensure each usage provides meaningful context.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/App.tsxsrc/components/ExpandAddresses.tsxsrc/components/Focusable.tsxsrc/components/Header.tsxsrc/components/InputAmount.tsxsrc/components/Keyboard.tsxsrc/components/Menu.tsxsrc/components/SwapsList.tsxsrc/components/Toggle.tsxsrc/components/TransactionsList.tsxsrc/index.csssrc/ionic.csssrc/screens/Apps/Index.tsxsrc/screens/Settings/General.tsxsrc/screens/Wallet/Receive/Amount.tsx
💤 Files with no reviewable changes (2)
- src/components/Keyboard.tsx
- src/screens/Wallet/Receive/Amount.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- src/index.css
- src/ionic.css
- src/components/TransactionsList.tsx
- src/screens/Apps/Index.tsx
- src/App.tsx
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-21T18:25:36.391Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 121
File: src/screens/Init/Restore.tsx:58-58
Timestamp: 2025-07-21T18:25:36.391Z
Learning: In src/screens/Init/Restore.tsx, the Input component for private key restoration intentionally omits the `value` prop to make it uncontrolled. This is a security feature to prevent the private key from being displayed if users navigate away and return to the screen, avoiding potential exposure of sensitive data in the UI.
Applied to files:
src/components/InputAmount.tsx
📚 Learning: 2025-12-20T10:09:41.030Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 278
File: src/providers/annoucements.tsx:32-50
Timestamp: 2025-12-20T10:09:41.030Z
Learning: In src/providers/annoucements.tsx, the useEffect hook depends on [config] (not just [configLoaded]) because it needs to react to config changes after nostr restoration. The seen.current ref guard prevents showing multiple announcements per session, even though the effect may re-run on config updates.
Applied to files:
src/components/InputAmount.tsxsrc/components/ExpandAddresses.tsx
🧬 Code graph analysis (7)
src/components/InputAmount.tsx (1)
src/components/Focusable.tsx (1)
Focusable(9-36)
src/components/Toggle.tsx (1)
src/components/Focusable.tsx (1)
Focusable(9-36)
src/components/ExpandAddresses.tsx (5)
src/components/Focusable.tsx (1)
Focusable(9-36)src/components/FlexRow.tsx (1)
FlexRow(18-50)src/components/Text.tsx (2)
TextSecondary(91-97)Text(25-79)src/lib/format.ts (1)
prettyLongText(76-83)src/components/Shadow.tsx (1)
Shadow(18-55)
src/screens/Settings/General.tsx (6)
src/providers/config.tsx (1)
ConfigContext(33-43)src/providers/options.tsx (1)
OptionsContext(125-131)src/components/Focusable.tsx (1)
Focusable(9-36)src/components/FlexRow.tsx (1)
FlexRow(18-50)src/components/Text.tsx (1)
Text(25-79)src/icons/Arrow.tsx (1)
ArrowIcon(1-7)
src/components/Menu.tsx (4)
src/components/Focusable.tsx (1)
Focusable(9-36)src/components/FlexRow.tsx (1)
FlexRow(18-50)src/components/Text.tsx (1)
Text(25-79)src/icons/Arrow.tsx (1)
ArrowIcon(1-7)
src/components/SwapsList.tsx (5)
src/components/FlexRow.tsx (1)
FlexRow(18-50)src/components/FlexCol.tsx (1)
FlexCol(16-46)src/components/Focusable.tsx (1)
Focusable(9-36)src/providers/lightning.tsx (1)
LightningContext(45-62)src/components/Text.tsx (1)
TextLabel(81-89)
src/components/Header.tsx (2)
src/components/Focusable.tsx (1)
Focusable(9-36)src/icons/Back.tsx (1)
BackIcon(1-13)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (7)
src/screens/Settings/General.tsx (1)
19-31: LGTM! Keyboard accessibility properly implemented.The Row component correctly wraps the settings option in a Focusable component, enabling both click and keyboard navigation while maintaining the existing visual structure and behavior.
src/components/Toggle.tsx (1)
20-22: LGTM! Toggle is now keyboard accessible.The IonToggle is correctly wrapped in a Focusable component with the
fitprop, enabling keyboard interaction via Enter/Space keys while preserving the existing click behavior.src/components/InputAmount.tsx (2)
52-55: LGTM! Dependency array correctly updated.The useEffect now only depends on
[focus], which is correct sinceinput.currentchanges don't trigger re-renders. The guardif (focus && input.current)safely handles null refs.
99-109: LGTM! Max button is now keyboard accessible.The Max button is correctly wrapped in a Focusable component, enabling keyboard interaction while preserving the existing click behavior and styling.
src/components/Header.tsx (1)
40-44: LGTM! Back button is now keyboard accessible.The back navigation control is correctly wrapped in a Focusable component with the
fitprop, enabling keyboard interaction via Enter/Space keys.src/components/Menu.tsx (1)
35-45: LGTM! Menu items are now keyboard accessible.The menu rows are correctly wrapped in Focusable components, enabling keyboard navigation while preserving the existing click behavior and styling.
src/components/SwapsList.tsx (1)
155-157: Both SwapsList and TransactionsList already implement the same focusable pattern consistently.Both components use identical Enter-to-focus behavior: the header Focusable sets
focusable=true, which enables keyboard navigation on individual list items. However, neither component currently implements Escape-to-unfocus handlers. If unfocusing is required, add Escape key handlers to both components to complete the pattern.Likely an incorrect or invalid review comment.
| useEffect(() => { | ||
| const handleArrowDown = (event: KeyboardEvent) => { | ||
| if (event.key === 'ArrowDown') { | ||
| if (!expand) setExpand(true) | ||
| } | ||
| if (event.key === 'ArrowUp') { | ||
| if (expand) setExpand(false) | ||
| } | ||
| } | ||
| window.addEventListener('keydown', handleArrowDown) | ||
| return () => window.removeEventListener('keydown', handleArrowDown) | ||
| }, [expand]) |
There was a problem hiding this comment.
Global keyboard listener may cause conflicts.
The window-level keyboard listener for ArrowDown/ArrowUp will be active whenever this component is mounted, regardless of whether the component is focused or visible. This could interfere with other parts of the application that use these keys for navigation.
Consider one of these approaches:
- Attach the listener to the component's container ref instead of window
- Check if the component or one of its descendants has focus before handling the event
- Add an event.target check to ensure the event originated from within this component
🔎 Proposed fix using a container ref
+import { useEffect, useRef, useState } from 'react'
-import { useEffect, useState } from 'react'
export default function ExpandAddresses({
bip21uri,
boardingAddr,
offchainAddr,
invoice,
onClick,
}: ExpandAddressesProps) {
const [copied, setCopied] = useState('')
const [expand, setExpand] = useState(false)
+ const containerRef = useRef<HTMLDivElement>(null)
const [present] = useIonToast()
useEffect(() => {
const handleArrowDown = (event: KeyboardEvent) => {
+ // Only handle if the event target is within this component
+ if (!containerRef.current?.contains(event.target as Node)) return
+
if (event.key === 'ArrowDown') {
if (!expand) setExpand(true)
}
if (event.key === 'ArrowUp') {
if (expand) setExpand(false)
}
}
window.addEventListener('keydown', handleArrowDown)
return () => window.removeEventListener('keydown', handleArrowDown)
}, [expand])
// ... rest of component
return (
- <div style={{ margin: '0 auto', maxWidth: '100%', width: '300px' }}>
+ <div ref={containerRef} style={{ margin: '0 auto', maxWidth: '100%', width: '300px' }}>
{/* ... */}
</div>
)
}🤖 Prompt for AI Agents
In src/components/ExpandAddresses.tsx around lines 36-47, the current
window-level keydown listener for ArrowDown/ArrowUp runs whenever the component
is mounted and can conflict with other parts of the app; change the
implementation to use a container ref (create a ref for the component wrapper
and attach the keydown listener to ref.current instead of window), or—if you
prefer keeping a global listener—first check that the event originated inside
the component by verifying event.target is contained within the container ref
(or that document.activeElement is a descendant of the container) before
toggling expand, and ensure you add/remove the listener from the ref element on
mount/unmount and include the ref in the effect dependencies for proper cleanup.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/components/Focusable.tsx (1)
1-8: Consider requiringariaLabelor improving the default.The default
aria-labelof "Focusable element" is generic and doesn't provide meaningful context for screen reader users. Since this component wraps diverse interactive elements (transaction rows, swap items, headers), each usage should provide a descriptive label.🔎 Options to improve accessibility
Option 1: Make
ariaLabelrequiredinterface FocusableProps { children: React.ReactNode onEscape?: () => void onEnter?: () => void - ariaLabel?: string + ariaLabel: string fit?: boolean role?: string }Option 2: Add a console warning when ariaLabel is not provided (development only)
export default function Focusable({ ariaLabel, role, children, fit, onEnter, onEscape }: FocusableProps) { + if (process.env.NODE_ENV === 'development' && !ariaLabel) { + console.warn('Focusable: ariaLabel should be provided for accessibility') + } + const style: React.CSSProperties = {Also applies to: 36-36
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/components/ExpandAddresses.tsxsrc/components/Focusable.tsxsrc/components/Header.tsxsrc/components/InputAmount.tsxsrc/components/Menu.tsxsrc/components/SwapsList.tsxsrc/components/Toggle.tsxsrc/components/TransactionsList.tsxsrc/lib/types.tssrc/providers/lightning.tsxsrc/screens/Apps/Index.tsxsrc/screens/Settings/General.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- src/screens/Settings/General.tsx
- src/components/ExpandAddresses.tsx
- src/components/Menu.tsx
- src/screens/Apps/Index.tsx
- src/components/InputAmount.tsx
- src/components/Header.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-21T18:25:36.391Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 121
File: src/screens/Init/Restore.tsx:58-58
Timestamp: 2025-07-21T18:25:36.391Z
Learning: In src/screens/Init/Restore.tsx, the Input component for private key restoration intentionally omits the `value` prop to make it uncontrolled. This is a security feature to prevent the private key from being displayed if users navigate away and return to the screen, avoiding potential exposure of sensitive data in the UI.
Applied to files:
src/components/TransactionsList.tsx
🧬 Code graph analysis (3)
src/components/TransactionsList.tsx (5)
src/lib/types.ts (1)
Tx(74-84)src/components/Text.tsx (3)
Text(25-79)TextSecondary(91-97)TextLabel(81-89)src/components/FlexRow.tsx (1)
FlexRow(18-50)src/components/Focusable.tsx (1)
Focusable(10-41)src/providers/wallet.tsx (1)
WalletContext(42-56)
src/components/SwapsList.tsx (6)
src/lib/types.ts (1)
PendingSwap(38-38)src/components/FlexRow.tsx (1)
FlexRow(18-50)src/components/Focusable.tsx (1)
Focusable(10-41)src/providers/lightning.tsx (1)
LightningContext(46-63)src/components/Empty.tsx (1)
EmptySwapList(46-54)src/components/Text.tsx (1)
TextLabel(81-89)
src/providers/lightning.tsx (1)
src/lib/types.ts (1)
PendingSwap(38-38)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (6)
src/lib/types.ts (1)
1-1: LGTM!The new
PendingSwaptype alias cleanly consolidates the union type for reuse across the codebase, improving type consistency.Also applies to: 38-39
src/components/Toggle.tsx (1)
20-22: LGTM!The Focusable wrapper correctly enables keyboard activation (Enter/Space) for the toggle, aligning with the broader keyboard navigation pattern introduced in this PR.
src/providers/lightning.tsx (1)
18-18: LGTM!The return type update to
Promise<PendingSwap[]>is consistent with the new type alias defined insrc/lib/types.ts, improving type clarity.Also applies to: 41-41, 191-194
src/components/Focusable.tsx (1)
16-27: LGTM!The keyboard event handling is correct:
- Uses
' '(space character) instead of the string'Space'- Properly calls
preventDefault()andstopPropagation()- Handles both Enter and Escape keys appropriately
src/components/TransactionsList.tsx (1)
1-1: LGTM on the core keyboard navigation implementation!The changes successfully address the previous review concerns:
- Component renamed from
DatetoWhen(avoids shadowing globalDate)- Keyboard event handling delegated to the
Focusablecomponent- Focus management properly implemented with state and callbacks
Also applies to: 14-14, 58-58, 124-124, 128-128
src/components/SwapsList.tsx (1)
14-15: LGTM on the type updates and focus management!The changes properly integrate the new
PendingSwaptype and implement keyboard navigation with state management and callbacks, consistent with the pattern used inTransactionsList.tsx.Also applies to: 53-53, 115-116, 150-150
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/test/e2e/nostr.test.ts (1)
91-91: Consider investigating the need for the fixed timeout.The 1000ms wait after wallet restoration might be masking an async timing issue. While this pattern exists elsewhere in the test suite (line 205), consider whether a more reliable wait condition (e.g.,
waitForSelectoron a specific element) could replace the fixed timeout.src/components/Focusable.tsx (2)
3-17: Consider adding ref forwarding and a disabled prop.The component would benefit from:
- Ref forwarding to enable programmatic focus management (e.g., auto-focusing after navigation)
- A disabled prop to properly handle disabled states with
aria-disabledand prevent keyboard activation🔎 Example implementation
+import { ReactNode, forwardRef } from 'react' -import { ReactNode } from 'react' -export default function Focusable({ +const Focusable = forwardRef<HTMLDivElement, { ariaLabel?: string children: ReactNode onEscape?: () => void onEnter?: () => void role?: string fit?: boolean -}: { + disabled?: boolean +}>(({ ariaLabel, children, onEscape, onEnter, role, fit, -}: { - ariaLabel?: string - children: ReactNode - onEscape?: () => void - onEnter?: () => void - role?: string - fit?: boolean -}) { + disabled = false, +}, ref) => { // ... rest of implementation const handleKeyDown = (e: React.KeyboardEvent) => { + if (disabled) return // ... existing key handling } return ( <div + ref={ref} tabIndex={0} // ... other props + aria-disabled={disabled} > {children} </div> ) -} +}) + +Focusable.displayName = 'Focusable' +export default Focusable
43-43: Consider the generic default aria-label.The default
'Focusable element'provides a fallback but isn't very descriptive for screen reader users. Consider documenting that consumers should provide meaningfulariaLabelvalues for better accessibility, especially when the role is 'button'.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/Focusable.tsxsrc/screens/Apps/Index.tsxsrc/test/e2e/nostr.test.tssrc/test/e2e/swap.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/screens/Apps/Index.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-07T14:41:35.991Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 106
File: src/lib/bolt11.ts:153-155
Timestamp: 2025-07-07T14:41:35.991Z
Learning: In src/lib/bolt11.ts, the code is copied from the original bitcoinjs/bolt11 library (https://github.com/bitcoinjs/bolt11/blob/master/payreq.js) and should be kept unchanged to maintain compatibility with the upstream source, even if there are potential improvements or syntax issues.
Applied to files:
src/test/e2e/nostr.test.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: Cloudflare Pages: wallet-mutinynet
🔇 Additional comments (3)
src/test/e2e/swap.test.ts (1)
18-18: LGTM! Test selector updates are consistent.The test-id updates from
app-Boltztoapp-boltzare consistent across all test cases and align with the UI component changes for keyboard navigation support.Also applies to: 79-79, 142-142, 209-209
src/test/e2e/nostr.test.ts (1)
149-150: LGTM! Test assertions updated to match new UI structure.The test-id updates and text-based assertions align with the Focusable component integration. The exact text matches (
+ 1,992 SATS,- 1,001 SATS) correctly verify the swap amounts are displayed.Also applies to: 180-181, 222-224
src/components/Focusable.tsx (1)
23-34: LGTM! Keyboard handler follows ARIA best practices.The implementation correctly handles Enter and Space for activation (consistent with button role) and Escape for dismissal. The use of
stopPropagation()andpreventDefault()appropriately prevents default behaviors and event bubbling.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/components/ExpandAddresses.tsx (1)
36-47: Global keyboard listener may conflict with other UI elements.The window-level
keydownlistener for ArrowDown/ArrowUp will be active whenever this component is mounted, regardless of whether it has focus. This could interfere with other parts of the application that use arrow keys for navigation (e.g., scrolling lists, dropdown menus).🔎 Recommended fix: scope the listener to the component
Add a container ref and check if the component (or one of its descendants) has focus before handling the event:
+import { useEffect, useRef, useState } from 'react' -import { useEffect, useState } from 'react' export default function ExpandAddresses({ bip21uri, boardingAddr, offchainAddr, invoice, onClick, }: ExpandAddressesProps) { const [copied, setCopied] = useState('') const [expand, setExpand] = useState(false) + const containerRef = useRef<HTMLDivElement>(null) const [present] = useIonToast() useEffect(() => { const handleArrowDown = (event: KeyboardEvent) => { + // Only handle if focus is within this component + if (!containerRef.current?.contains(document.activeElement)) return + if (event.key === 'ArrowDown') { if (!expand) setExpand(true) } if (event.key === 'ArrowUp') { if (expand) setExpand(false) } } window.addEventListener('keydown', handleArrowDown) return () => window.removeEventListener('keydown', handleArrowDown) }, [expand]) // ... rest of component return ( - <div style={{ margin: '0 auto', maxWidth: '100%', width: '300px' }}> + <div ref={containerRef} style={{ margin: '0 auto', maxWidth: '100%', width: '300px' }}> {/* ... */} </div> ) }
🧹 Nitpick comments (1)
src/test/e2e/nostr.test.ts (1)
91-91: Verify that the timeout addresses the underlying race condition.The 1000ms wait after wallet restoration may be masking a timing issue. Consider whether the test could wait for a specific UI state (e.g., a loading indicator to disappear or a specific element to become visible) instead of using an arbitrary timeout.
If there's a specific element that signals the wallet is fully loaded, consider replacing the timeout with:
await page.waitForSelector('[data-testid="some-loaded-indicator"]', { state: 'visible' })
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/ExpandAddresses.tsxsrc/components/Shadow.tsxsrc/test/e2e/nostr.test.tssrc/test/e2e/swap.test.ts
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-07-07T14:41:35.991Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 106
File: src/lib/bolt11.ts:153-155
Timestamp: 2025-07-07T14:41:35.991Z
Learning: In src/lib/bolt11.ts, the code is copied from the original bitcoinjs/bolt11 library (https://github.com/bitcoinjs/bolt11/blob/master/payreq.js) and should be kept unchanged to maintain compatibility with the upstream source, even if there are potential improvements or syntax issues.
Applied to files:
src/test/e2e/swap.test.tssrc/test/e2e/nostr.test.ts
📚 Learning: 2025-07-21T18:25:36.391Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 121
File: src/screens/Init/Restore.tsx:58-58
Timestamp: 2025-07-21T18:25:36.391Z
Learning: In src/screens/Init/Restore.tsx, the Input component for private key restoration intentionally omits the `value` prop to make it uncontrolled. This is a security feature to prevent the private key from being displayed if users navigate away and return to the screen, avoiding potential exposure of sensitive data in the UI.
Applied to files:
src/components/ExpandAddresses.tsx
📚 Learning: 2025-12-20T10:09:41.030Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 278
File: src/providers/annoucements.tsx:32-50
Timestamp: 2025-12-20T10:09:41.030Z
Learning: In src/providers/annoucements.tsx, the useEffect hook depends on [config] (not just [configLoaded]) because it needs to react to config changes after nostr restoration. The seen.current ref guard prevents showing multiple announcements per session, even though the effect may re-run on config updates.
Applied to files:
src/components/ExpandAddresses.tsx
📚 Learning: 2025-06-30T18:33:29.839Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
Applied to files:
src/test/e2e/nostr.test.ts
🧬 Code graph analysis (2)
src/test/e2e/swap.test.ts (1)
src/test/e2e/utils.ts (1)
readClipboard(3-16)
src/components/ExpandAddresses.tsx (6)
src/components/Focusable.tsx (1)
Focusable(3-48)src/components/FlexRow.tsx (1)
FlexRow(18-50)src/lib/format.ts (1)
prettyLongText(76-83)src/components/Shadow.tsx (1)
Shadow(19-57)src/icons/CheckMark.tsx (1)
CheckMarkIcon(1-11)src/icons/Copy.tsx (1)
CopyIcon(1-13)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: Cloudflare Pages: wallet-mutinynet
🔇 Additional comments (4)
src/components/Shadow.tsx (1)
16-16: LGTM! Test ID support added cleanly.The addition of the
testIdprop enables test hooks for the Shadow component, which aligns with the broader keyboard accessibility and testability improvements in this PR.Also applies to: 32-32, 53-53
src/test/e2e/swap.test.ts (1)
18-18: LGTM! Test selectors updated consistently.The updates to use lowercase
app-boltzand explicit test IDs (ark-address-copy,invoice-address-copy) align well with the new UI structure and improve test reliability by replacing complex locator chains with direct test ID references.Also applies to: 52-54, 75-75, 94-96, 136-136, 155-157, 200-200
src/test/e2e/nostr.test.ts (1)
127-129: LGTM! Consistent test selector updates.The updates to use
app-boltz, explicit test IDs, and exact SATS text matches improve test specificity and align with the broader keyboard accessibility changes in this PR.Also applies to: 145-146, 176-177, 218-220
src/components/ExpandAddresses.tsx (1)
65-65: LGTM! Test IDs enable better test reliability.The addition of
testIdprops toExpandLineand the propagation to Shadow components (with the-address-copysuffix) aligns perfectly with the test updates in the e2e files, enabling more reliable and maintainable tests.Also applies to: 72-72, 92-95
src/components/ExpandAddresses.tsx
Outdated
| const handleKeyDown = (value: string) => { | ||
| handleCopy(value) | ||
| onClick(value) | ||
| } |
There was a problem hiding this comment.
Inconsistent behavior: keyboard Enter differs from mouse click.
The keyboard interaction creates different behavior than mouse clicks:
- Pressing Enter on a Focusable line (line 66): calls
handleKeyDown→ copies and callsonClick - Clicking the FlexRow (line 67): only calls
onClick - Clicking the Shadow copy icon (line 72): only calls
handleCopy
This means keyboard users get a combined copy+click action, while mouse users can choose either action separately.
🔎 Recommended fix: align keyboard behavior with primary click action
Make the keyboard Enter key match the row click behavior (call onClick only), and keep the copy icon separate:
- const handleKeyDown = (value: string) => {
- handleCopy(value)
- onClick(value)
- }
-
- const ExpandLine = ({ testId, title, value }: { testId: string; title: string; value: string }) => (
- <Focusable onEnter={() => handleKeyDown(value)}>
+ const ExpandLine = ({ testId, title, value }: { testId: string; title: string; value: string }) => (
+ <Focusable onEnter={() => onClick(value)}>
<FlexRow between onClick={() => onClick(value)}>
<FlexCol gap='0'>
<TextSecondary>{title}</TextSecondary>
<Text>{prettyLongText(value, 12)}</Text>
</FlexCol>
- <Shadow flex onClick={() => handleCopy(value)} testId={testId + '-address-copy'}>
+ <Shadow
+ flex
+ onClick={(e) => {
+ e.stopPropagation()
+ handleCopy(value)
+ }}
+ testId={testId + '-address-copy'}
+ >
{copied === value ? <CheckMarkIcon /> : <CopyIcon />}
</Shadow>
</FlexRow>
</Focusable>
)Note: Add e.stopPropagation() to prevent the copy icon click from also triggering the row's onClick.
Also applies to: 66-72
🤖 Prompt for AI Agents
In src/components/ExpandAddresses.tsx around lines 60 to 72, the Enter key
handler currently both copies and triggers onClick, causing inconsistent
behavior vs mouse interactions; change handleKeyDown to only call onClick
(remove the call to handleCopy) so keyboard Enter matches the row click
behavior, and ensure the Shadow copy icon's click handler calls handleCopy with
e.stopPropagation() so clicking the copy icon does not also trigger the row's
onClick; apply the same adjustment to the other instance mentioned (lines
66–72).
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/components/ExpandAddresses.tsx (2)
60-76: Inconsistent behavior between keyboard and mouse activation.The
onEnterhandler (line 60-63) performs bothhandleCopy(value)ANDonClick(value), while:
- Clicking the FlexRow (line 67) only calls
onClick(value)- Clicking the Shadow copy icon (line 72) only calls
handleCopy(value)This means keyboard users get a combined action while mouse users can choose either action separately.
🔎 Proposed fix: align keyboard Enter with row click behavior
- const onEnter = (value: string) => { - handleCopy(value) - onClick(value) - } - const ExpandLine = ({ testId, title, value }: { testId: string; title: string; value: string }) => ( - <Focusable onEnter={() => onEnter(value)}> + <Focusable onEnter={() => onClick(value)}> <FlexRow between onClick={() => onClick(value)}>
36-47: Global keyboard listener may cause conflicts with other components.The
window.addEventListener('keydown', ...)captures ArrowDown/ArrowUp globally regardless of whether this component has focus. This could interfere with other parts of the application.Consider scoping the listener to only fire when the component (or a descendant) has focus:
🔎 Proposed fix using focus check
useEffect(() => { + const containerRef = document.getElementById('expand-addresses-container') const handleArrowDown = (event: KeyboardEvent) => { + // Only handle if focus is within this component + if (!containerRef?.contains(document.activeElement)) return + if (event.key === 'ArrowDown') { if (!expand) setExpand(true) } if (event.key === 'ArrowUp') { if (expand) setExpand(false) } } window.addEventListener('keydown', handleArrowDown) return () => window.removeEventListener('keydown', handleArrowDown) }, [expand])And add an id to the container:
- <div style={{ margin: '0 auto', maxWidth: '100%', width: '300px' }}> + <div id='expand-addresses-container' style={{ margin: '0 auto', maxWidth: '100%', width: '300px' }}>
🧹 Nitpick comments (5)
src/components/Header.tsx (1)
50-58: RedundantonClickon outer div may cause confusion.The outer
divat line 50 hasonClick={auxFunc}, but the innerFocusablewrapper handles keyboard activation viaonEnter={auxFunc}. This creates an inconsistent structure where mouse clicks are handled at the outer level while keyboard activation is handled at the inner level.Consider moving the click handling entirely to the
Focusablewrapper or its children for consistency:🔎 Proposed fix
- <div style={style} onClick={auxFunc} aria-label={auxAriaLabel}> + <div style={style} aria-label={auxAriaLabel}> {auxText || auxIcon ? ( - <Focusable onEnter={auxFunc} fit round> - {auxText ? SideButton(auxText) : <div style={{ padding: '0.5rem' }}>{auxIcon}</div>} + <Focusable onEnter={auxFunc} fit round> + <div onClick={auxFunc} style={{ cursor: 'pointer' }}> + {auxText ? SideButton(auxText) : <div style={{ padding: '0.5rem' }}>{auxIcon}</div>} + </div> </Focusable> ) : ( '\u00A0' )} </div>src/components/Table.tsx (1)
40-49: Hardcodedid='outer'may cause conflicts.Using a hardcoded
id='outer'could cause issues if:
- Multiple
Tablecomponents are rendered on the same page- Other components use the same id
Consider generating a unique id per table instance.
🔎 Proposed fix using useId
+import { useState, useId } from 'react' export default function Table({ data }: { data: any[][] }) { + const tableId = useId() const [focused, setFocused] = useState(false) // ... const focusOnOuterShell = () => { setFocused(false) - const outer = document.getElementById('outer') as HTMLElement + const outer = document.getElementById(`outer-${tableId}`) as HTMLElement if (outer) outer.focus() } // ... return ( - <Focusable id='outer' inactive={focused} onEnter={focusOnFirstRow} ariaLabel={ariaLabel()}> + <Focusable id={`outer-${tableId}`} inactive={focused} onEnter={focusOnFirstRow} ariaLabel={ariaLabel()}>src/screens/Settings/Logs.tsx (1)
48-52: Hardcodedid='outer'may cause conflicts with other components.Same issue as in
Table.tsx- ifLogsTableis used alongside other components withid='outer', focus management will break.🔎 Proposed fix using useId
+import { useEffect, useState, useId } from 'react' function LogsTable({ logs }: { logs: LogLine[] }) { + const tableId = useId() const [focused, setFocused] = useState(false) // ... const focusOnOuterShell = () => { setFocused(false) - const outer = document.getElementById('outer') as HTMLElement + const outer = document.getElementById(`outer-${tableId}`) as HTMLElement if (outer) outer.focus() }src/components/TransactionsList.tsx (1)
140-155: Hardcodedid='outer'may conflict with other components.This is the third component using
id='outer'. IfTransactionsList,Table, orLogsTableare rendered on the same page, focus management will break.Consider using React's
useId()hook to generate unique identifiers, or use component-specific prefixes likeid='transactions-outer'.src/components/SwapsList.tsx (1)
164-179: Hardcodedid='outer'may conflict with other list components.Same issue as
TransactionsList- consider using a unique identifier.🔎 Proposed fix
+import { useContext, useEffect, useState, useId } from 'react' export default function SwapsList() { + const listId = useId() // ... const focusOnOuterShell = () => { setFocused(false) - const outer = document.getElementById('outer') as HTMLElement + const outer = document.getElementById(`swaps-outer-${listId}`) as HTMLElement if (outer) outer.focus() } // ... return ( <div style={{ width: 'calc(100% + 2rem)', margin: '0 -1rem' }}> <TextLabel>Swap history</TextLabel> - <Focusable id='outer' inactive={focused} onEnter={focusOnFirstRow} ariaLabel={ariaLabel()}> + <Focusable id={`swaps-outer-${listId}`} inactive={focused} onEnter={focusOnFirstRow} ariaLabel={ariaLabel()}>
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/components/ExpandAddresses.tsxsrc/components/Focusable.tsxsrc/components/Header.tsxsrc/components/SwapsList.tsxsrc/components/Table.tsxsrc/components/Toggle.tsxsrc/components/TransactionsList.tsxsrc/icons/Settings.tsxsrc/screens/Settings/Logs.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Toggle.tsx
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-21T18:25:36.391Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 121
File: src/screens/Init/Restore.tsx:58-58
Timestamp: 2025-07-21T18:25:36.391Z
Learning: In src/screens/Init/Restore.tsx, the Input component for private key restoration intentionally omits the `value` prop to make it uncontrolled. This is a security feature to prevent the private key from being displayed if users navigate away and return to the screen, avoiding potential exposure of sensitive data in the UI.
Applied to files:
src/components/ExpandAddresses.tsxsrc/components/TransactionsList.tsx
📚 Learning: 2025-12-20T10:09:41.030Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 278
File: src/providers/annoucements.tsx:32-50
Timestamp: 2025-12-20T10:09:41.030Z
Learning: In src/providers/annoucements.tsx, the useEffect hook depends on [config] (not just [configLoaded]) because it needs to react to config changes after nostr restoration. The seen.current ref guard prevents showing multiple announcements per session, even though the effect may re-run on config updates.
Applied to files:
src/components/ExpandAddresses.tsx
🧬 Code graph analysis (4)
src/components/Header.tsx (3)
src/components/Shadow.tsx (1)
Shadow(19-57)src/components/Focusable.tsx (1)
Focusable(3-55)src/icons/Back.tsx (1)
BackIcon(1-13)
src/components/ExpandAddresses.tsx (9)
src/components/Focusable.tsx (1)
Focusable(3-55)src/components/FlexRow.tsx (1)
FlexRow(18-50)src/components/FlexCol.tsx (1)
FlexCol(16-46)src/components/Text.tsx (1)
Text(25-79)src/components/Shadow.tsx (1)
Shadow(19-57)src/icons/CheckMark.tsx (1)
CheckMarkIcon(1-11)src/icons/Copy.tsx (1)
CopyIcon(1-13)src/icons/ChevronUp.tsx (1)
ChevronUpIcon(1-13)src/icons/ChevronDown.tsx (1)
ChevronDownIcon(1-13)
src/components/SwapsList.tsx (4)
src/lib/types.ts (1)
PendingSwap(38-38)src/providers/flow.tsx (1)
FlowContext(89-104)src/providers/navigation.tsx (1)
NavigationContext(169-173)src/components/Focusable.tsx (1)
Focusable(3-55)
src/components/TransactionsList.tsx (5)
src/lib/types.ts (1)
Tx(74-84)src/providers/flow.tsx (1)
FlowContext(89-104)src/providers/navigation.tsx (1)
NavigationContext(169-173)src/providers/wallet.tsx (1)
WalletContext(42-56)src/components/Focusable.tsx (1)
Focusable(3-55)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: Cloudflare Pages: wallet-mutinynet
🔇 Additional comments (7)
src/icons/Settings.tsx (1)
19-27: LGTM! Good accessibility improvement.Using
currentColorinstead of hardcoded colors allows the icon to inherit styling from parent elements, which is essential for proper focus state visibility in the keyboard navigation context.src/components/Header.tsx (1)
40-44: LGTM!The back button correctly uses
Focusablefor keyboard activation while maintainingonClickon the inner element for mouse interaction.src/components/Focusable.tsx (1)
1-55: Well-designed reusable accessibility component.The implementation correctly:
- Handles both Enter and Space keys for activation (standard button behavior)
- Uses
stopPropagation()andpreventDefault()to prevent event bubbling- Controls focusability via
tabIndexbased on theinactiveprop- Defaults to
role="button"for proper semantic meaningsrc/screens/Settings/Logs.tsx (1)
60-82: LGTM! Good keyboard navigation implementation for logs.The focus management correctly:
- Makes the outer shell focusable when not in navigation mode
- Makes individual rows focusable when in navigation mode
- Provides descriptive aria labels for screen readers
- Handles Enter to copy and Escape to exit
src/components/TransactionsList.tsx (1)
18-103: Good refactoring of TransactionLine.The component now accepts an
onClickprop, cleanly separating the line rendering from navigation logic. This improves reusability and testability.src/components/SwapsList.tsx (2)
53-94: Good consolidation of swap line rendering.The
SwapLinecomponent is now cleaner with inline component definitions and properly acceptsonClickprop for navigation delegation.
137-143: Early return guards against empty array access.The check at line 137
if (swapHistory.length === 0) return <EmptySwapList />ensuresfocusOnFirstRowis only callable when the list is non-empty, preventing the crash scenario seen in other components.
| const focusOnFirstRow = () => { | ||
| setFocused(true) | ||
| const title = data[0][0] | ||
| const first = document.getElementById(title) as HTMLElement | ||
| if (first) first.focus() | ||
| } |
There was a problem hiding this comment.
Potential crash when data array is empty.
focusOnFirstRow accesses data[0][0] without checking if data is empty. If called when data.length === 0, this will throw a runtime error.
🔎 Proposed fix
const focusOnFirstRow = () => {
+ if (data.length === 0) return
setFocused(true)
const title = data[0][0]
const first = document.getElementById(title) as HTMLElement
if (first) first.focus()
}📝 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.
| const focusOnFirstRow = () => { | |
| setFocused(true) | |
| const title = data[0][0] | |
| const first = document.getElementById(title) as HTMLElement | |
| if (first) first.focus() | |
| } | |
| const focusOnFirstRow = () => { | |
| if (data.length === 0) return | |
| setFocused(true) | |
| const title = data[0][0] | |
| const first = document.getElementById(title) as HTMLElement | |
| if (first) first.focus() | |
| } |
🤖 Prompt for AI Agents
In src/components/Table.tsx around lines 21 to 26, focusOnFirstRow
unconditionally reads data[0][0] which will throw if data is empty; add a guard
that checks data exists and data.length > 0 and that data[0] has at least one
cell before accessing [0][0]; if the guard fails, still call setFocused(true) if
desired (or return early) and do not attempt to query the DOM; after confirming
a non-empty title, query by id and focus as before.
|
Found a minor UI issue when sending funds: in the "amount" the "max" selector is now expanded. 
|
Nice catch. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/screens/Settings/Logs.tsx (1)
66-81: Consider extracting the key to avoid duplicate computation.The
key()function is called twice for each log entry (lines 70 and 71). While this is not a performance issue for typical log sizes, extracting it to a variable would follow DRY principles.🔎 Proposed refactor
{[...logs].reverse().map(({ time, msg, level }) => { + const rowKey = key({ time, msg, level }) return ( <Focusable inactive={!focused} onEnter={() => copy(msg)} onEscape={focusOnOuterShell} - id={key({ time, msg, level })} - key={key({ time, msg, level })} - ariaLabel={ariaLabel({ time, msg, level })} + id={rowKey} + key={rowKey} + ariaLabel={ariaLabel({ time, msg, level })} >src/components/Table.tsx (2)
35-37: Clarify the ariaLabel semantics.The ariaLabel describes the value as "status" (line 37), but the Table component is generic and the
valuefield may represent various types of data (not just status). Consider using a more generic description like "value" instead.🔎 Proposed refactor
const ariaLabel = (title?: string, value?: string) => { if (!title || !value) return 'Pressing Enter enables keyboard navigation of the table' - return `Title ${title} with status ${value}. Press Escape to exit keyboard navigation.` + return `Row with title ${title} and value ${value}. Press Escape to exit keyboard navigation.` }
44-51: Component design relies on caller to ensure unique titles; current usages are safe but consider refactoring to reduce fragility.While the Table component uses
titlefor bothidandkeyattributes, all current usages (Details.tsx, About.tsx, Swap.tsx) construct tables with hardcoded static labels that are unique within each table. However, the component design doesn't enforce or validate title uniqueness, making it fragile for future usage. Consider either:
- Using an index-based key and deriving IDs from a stable identifier, or
- Requiring a unique
idfield separate from the display title.src/components/TransactionsList.tsx (1)
111-132: Consider extracting the focus management pattern into a custom hook.The focus management logic (focused state, focusOnFirstRow, focusOnOuterShell, ariaLabel) is repeated across TransactionsList, SwapsList, Logs, and Table components. Extracting this into a reusable
useFocusableListhook would reduce duplication and improve maintainability.Example hook structure
function useFocusableList<T>( items: T[], keyFn: (item: T) => string, ariaLabelFn: (item?: T) => string ) { const [focused, setFocused] = useState(false) const focusOnFirstRow = () => { setFocused(true) if (items.length === 0) return const id = keyFn(items[0]) const first = document.getElementById(id) as HTMLElement if (first) first.focus() } const focusOnOuterShell = () => { setFocused(false) const outer = document.getElementById('outer') as HTMLElement if (outer) outer.focus() } return { focused, focusOnFirstRow, focusOnOuterShell, ariaLabel: ariaLabelFn } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/components/InputAmount.tsxsrc/components/SwapsList.tsxsrc/components/Table.tsxsrc/components/TransactionsList.tsxsrc/screens/Settings/Logs.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/InputAmount.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-21T18:25:36.391Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 121
File: src/screens/Init/Restore.tsx:58-58
Timestamp: 2025-07-21T18:25:36.391Z
Learning: In src/screens/Init/Restore.tsx, the Input component for private key restoration intentionally omits the `value` prop to make it uncontrolled. This is a security feature to prevent the private key from being displayed if users navigate away and return to the screen, avoiding potential exposure of sensitive data in the UI.
Applied to files:
src/components/TransactionsList.tsx
🧬 Code graph analysis (3)
src/components/Table.tsx (5)
src/lib/clipboard.ts (1)
copyToClipboard(3-11)src/lib/toast.ts (1)
copiedToClipboard(13-16)src/components/Focusable.tsx (1)
Focusable(3-55)src/components/Text.tsx (1)
Text(25-79)src/lib/format.ts (1)
prettyLongText(76-83)
src/components/TransactionsList.tsx (6)
src/lib/types.ts (1)
Tx(74-84)src/components/Text.tsx (3)
Text(25-79)TextSecondary(91-97)TextLabel(81-89)src/providers/flow.tsx (1)
FlowContext(89-104)src/providers/navigation.tsx (1)
NavigationContext(169-173)src/providers/wallet.tsx (1)
WalletContext(42-56)src/components/Focusable.tsx (1)
Focusable(3-55)
src/components/SwapsList.tsx (6)
src/lib/types.ts (1)
PendingSwap(38-38)src/providers/flow.tsx (1)
FlowContext(89-104)src/providers/navigation.tsx (1)
NavigationContext(169-173)src/providers/lightning.tsx (1)
LightningContext(46-63)src/components/Text.tsx (1)
TextLabel(81-89)src/components/Focusable.tsx (1)
Focusable(3-55)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: Cloudflare Pages: wallet-mutinynet
Allow for keyboard navigation.
This is the first PR of a series of ARIA related PRs
TAB = go to next focusable element
SHIFT+TAB = go to previous focusable element
To enter the transaction list, focus on "Transaction history" and press Enter.
Press Esc to make transactions in list unfocusable again.
Same strategy for swap list in apps boltz
Summary by CodeRabbit
New Features
Bug Fixes
Style
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.