Repository navigation
fix boltz swaps scroll - #585
Conversation
Deploying wallet-mutinynet with
|
| Latest commit: |
428d748
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ca2ea316.arkade-wallet.pages.dev |
| Branch Preview URL: | https://fix-boltz-swaps-scroll.arkade-wallet.pages.dev |
Deploying wallet-bitcoin with
|
| Latest commit: |
428d748
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d7297981.wallet-bitcoin.pages.dev |
| Branch Preview URL: | https://fix-boltz-swaps-scroll.wallet-bitcoin.pages.dev |
Deploying tmp-boltz-upstream-mainnet-arkade-wallet with
|
| Latest commit: |
428d748
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://9cbbdca8.tmp-boltz-upstream-mainnet-arkade-wallet.pages.dev |
| Branch Preview URL: | https://fix-boltz-swaps-scroll.tmp-boltz-upstream-mainnet-arkade-wallet.pages.dev |
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Code Review — fix boltz swaps scroll
🔴 Bug: Virtualizer initialized but never used (dead code)
src/components/SwapsList.tsx:173-178 — useVirtualizer is set up, but the render loop at line 221 still uses swapHistory.map(...) instead of virtualizer.getVirtualItems().map(...). This means:
- All items are rendered — no virtualization benefit, defeats the purpose of adding
@tanstack/react-virtual - Items are not absolutely positioned — the
getTotalSize()wrapper div is there (line 218) but children flow normally instead of usingtransform: translateY(${virtualItem.start}px) - Layout will be broken — the wrapper div reserves
getTotalSize()pixels of height, but all items also render at their natural positions inside it, causing double-height or overlapping content
Reference: TransactionsList.tsx in this same repo implements virtualization correctly — use it as a template. Key differences needed:
// Instead of: swapHistory.map((swap) => ...)
// Use:
{virtualizer.getVirtualItems().map((virtualItem) => {
const swap = swapHistory[virtualItem.index]
return (
<div
key={key(swap)}
style={{
position: 'absolute',
top: 0,
left: 0,
width: '100%',
transform: `translateY(${virtualItem.start}px)`,
}}
>
<Focusable
id={key(swap)}
inactive={!focused}
ariaLabel={ariaLabel(swap)}
onEscape={focusOnOuterShell}
onEnter={() => handleClick(swap)}
>
<SwapLine onClick={() => handleClick(swap)} swap={swap} />
</Focusable>
</div>
)
})}🟡 Minor: Keyboard navigation not updated for virtualization
TransactionsList.tsx handles keyboard nav with virtualizer.scrollToIndex() + requestAnimationFrame focus. This PR's focusOnFirstRow uses document.getElementById directly, which will break when the virtualizer is actually wired up (off-screen items won't be in the DOM).
✅ What's good
- The scroll container approach (
height: calc(100dvh - 280px),overflowY: auto) is correct and matchesTransactionsList.tsx - Moving
scroll-fadeto the scroll container is the right call estimateSize: () => 61andoverscan: 5match the existing pattern
Summary
The scroll container fix is correct, but the virtualizer is dead code — items aren't virtualized. Please wire up getVirtualItems() following the TransactionsList.tsx pattern, or remove the virtualizer entirely if the goal is just scroll containment (in which case, drop the useVirtualizer import and @tanstack/react-virtual isn't needed since it's already a dep for TransactionsList).
Not protocol-critical (UI only), but shipping dead code with a broken layout is a bug.
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughSwapsList component adds list virtualization using TanStack's useVirtualizer hook. A scroll container reference is created and the virtualizer is configured with Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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. Review rate limit: 0/1 reviews remaining, refill in 23 minutes and 52 seconds.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: 1
🧹 Nitpick comments (1)
src/components/SwapsList.tsx (1)
182-188: Scroll virtualized index into view before focusing element.
focusOnFirstRowdirectly focuses by ID without scrolling the element into the virtual list. With virtualization, the element may not be mounted if the list is scrolled, causing focus to fail.TransactionsList.tsximplements the correct pattern: callvirtualizer.scrollToIndex()first, then focus inrequestAnimationFrameto ensure the DOM element exists.Proposed fix
const focusOnFirstRow = () => { setFocused(true) if (swapHistory.length === 0) return - const id = key(swapHistory[0]) - const first = document.getElementById(id) as HTMLElement - if (first) first.focus() + virtualizer.scrollToIndex(0) + requestAnimationFrame(() => { + const id = key(swapHistory[0]) + const first = document.getElementById(id) as HTMLElement | null + first?.focus() + }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/SwapsList.tsx` around lines 182 - 188, The focusOnFirstRow function focuses the first swap by ID but doesn't ensure a virtualized row is scrolled/mounted first; change it to call virtualizer.scrollToIndex(0) (or equivalent index for swapHistory[0]) before trying to focus, then perform the DOM focus inside a requestAnimationFrame callback to ensure the element is rendered; keep existing use of key(swapHistory[0]) to compute the element id and only call first.focus() after confirming document.getElementById(id) is non-null.
🤖 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/SwapsList.tsx`:
- Around line 222-234: The list currently sets container height via
virtualizer.getTotalSize() but still renders every item using swapHistory.map(),
so replace the swapHistory.map(...) block with an iteration over
virtualizer.getVirtualItems() and render only those items; for each virtual
item, resolve the swap with swapHistory[virtualItem.index], use key(swap) or
virtualItem.key for the key/id, wrap SwapLine in Focusable as before, and apply
absolute positioning using style={{ position: 'absolute', top:
`${virtualItem.start}px`, height: `${virtualItem.size}px`, width: '100%' }} so
the virtualizer controls visible rows (keep onEnter, onEscape, ariaLabel and
handleClick logic).
---
Nitpick comments:
In `@src/components/SwapsList.tsx`:
- Around line 182-188: The focusOnFirstRow function focuses the first swap by ID
but doesn't ensure a virtualized row is scrolled/mounted first; change it to
call virtualizer.scrollToIndex(0) (or equivalent index for swapHistory[0])
before trying to focus, then perform the DOM focus inside a
requestAnimationFrame callback to ensure the element is rendered; keep existing
use of key(swapHistory[0]) to compute the element id and only call first.focus()
after confirming document.getElementById(id) is non-null.
🪄 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: ab86ea74-1d55-46ea-baad-5b74900f41fd
📒 Files selected for processing (1)
src/components/SwapsList.tsx
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Follow-up Review — fix virtualization (428d748)
The new commit addresses the dead-code issue from my previous review. Virtualization is now correctly wired up:
- ✅
virtualizer.getVirtualItems().map(...)replacesswapHistory.map(...) - ✅ Items are absolutely positioned with
translateY(virtualItem.start) - ✅ Scroll container has
ref={parentRef},overflowY: auto, fixed height - ✅ Total size wrapper is correct
- ✅ Pattern matches existing
TransactionsList.tsximplementation
Minor nit (non-blocking): TransactionsList includes data-index={virtualItem.index} on each virtual item div. Not strictly required without measureElement, but worth adding for consistency and future-proofing.
No protocol-critical code touched — this is UI-only (swap history list rendering). LGTM.
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Follow-up Review — fix virtualization (428d748)
The new commit addresses the dead-code issue from my previous review. Virtualization is now correctly wired up:
- ✅
virtualizer.getVirtualItems().map(...)replacesswapHistory.map(...) - ✅ Items are absolutely positioned with
translateY(virtualItem.start) - ✅ Scroll container has
ref={parentRef},overflowY: auto, fixed height - ✅ Total size wrapper is correct
- ✅ Pattern matches existing
TransactionsList.tsximplementation
Minor nit (non-blocking): TransactionsList includes data-index={virtualItem.index} on each virtual item div. Not strictly required without measureElement, but worth adding for consistency and future-proofing.
No protocol-critical code touched — this is UI-only (swap history list rendering). LGTM.
Summary by CodeRabbit