Repository navigation
fix: slide receive tabs on wraparound - #1608
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
 [Medium impact] The PR reaches Receive tab navigation and shared QR image drawing. The PR appears safe to merge, with a non-blocking correction needed in the journey’s swipe instructions. Findings
SummaryThe PR adds duplicate pages at both ends of the Receive pager so wraparound swipes slide before moving to the matching real page.
Acknowledged scope: ovitrif explicitly deferred the loading QR shown while sliding onto USDT because it predates this change. Diagram%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[First real page] -->|Swipe toward start| B[Copy of last page]
B -->|Scroll settles| C[Last real page]
C -->|Swipe toward end| D[Copy of first page]
D -->|Scroll settles| A
Reviews (1) · Last reviewed commit: "fix: slide receive tabs on wraparound" · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
talosmachina
left a comment
talosmachina
left a comment
There was a problem hiding this comment.
No findings. Replaces the swipePastEdges jump with wrap copies of the end tabs in the Receive pager, swapped for the real page once the scroll settles, and caches generated QR bitmaps so the swapped-in page draws at once. Reviewed dafe004, full tier.
What I checked, and 6 candidates I ruled out
Read in full: ReceiveQrScreen.kt (pager and effects, lines 120-600), QrCodeImage.kt, SwipeToChangeTabModifier.kt, the onSelectTab path in UsdtDepositReceiveScreen.kt
Call sites traced: rememberQrBitmap (QR view and the 400 dp share bitmap), selectReceiveTab, every scrollToItem/animateScrollToItem now going through pageIndexOf; no other user of swipePastEdges remains
Not built: Bitkit Android reviews here are reasoned from the code and CI, the branch is not built on this box
CI: build-local green; build, lint and the e2e matrix still running at review time
Ruled out
- Index collector picking a tab off a wrap page:
getOrNull(index - 1)returns null for page 0 and pagesize + 1, so only the settle effect sets the tab there. - Settle effect firing on the initial emission: the first
falsearrives atpageIndexOf(defaultTab), never 0 orsize + 1. - Swap onto USDT leaving the pager mid-scroll: the settle effect sits above the early return, so
scrollToItem(pageIndexOf(USDT))completes and the pager comes back at the USDT page, which is whereselectReceiveTabslides from. - Position jump when
visibleTabschanges (AUTO appearing): real pages keeptab.namekeys, so the list stays anchored on the same tab. - Shared cached bitmap being mutated or recycled: nothing recycles QR bitmaps and
shareQrCodeonly compresses it;LruCacheis synchronized for theDispatchers.Defaultput. - Cache growth: bounded at 16 MB by
byteCount, a few full-width codes.
Merge confidence: 4/5, no automated test covers the wrap swap (the updated journey is the only check) and build/lint had not finished.
|
Pushed 0624d16: the journey's two pager steps now swipe in 1200 ms, matching its description (Greptile's note). Journey only, no code change. |
|
Pushed 8488077: a tap on the USDT tab now lands the pager on the USDT page at once, since the USDT screen replaces the pager immediately. Before, the pager stayed on the tab it left, so the next swipe on USDT slid across every tab. Recording added to the Preview; |
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Code scan at 0624d16: no HIGH/MEDIUM. One LOW is inline, verified on Pixel_9.
Checked and clean:
- Paging: the index collector ignores the wrap pages (
getOrNull(index - 1)). The settle effect swaps a settled wrap page for the real one, and its first emission never lands on a wrap page. - Page keys stay unique with two tabs (USDT off, or CJIT pending). Real pages keep
tab.namekeys, so Auto appearing doesn't move the pager. - Hardware wrap copy: shows the loading view, and the address loads once the swipe settles.
- Resets:
showDetailsresets on the swap, and the CJIT and Auto auto-switches usepageIndexOf. - Gestures:
- QR tap-to-copy still works, and
swipePastEdgesis gone, so no extra pointer handler remains. - The edge swipe is still system back.
- USDT Back still calls
onClose.
- QR tap-to-copy still works, and
- QR cache: the key includes size and content, bitmaps are never recycled or mutated, and the 400 dp share bitmap has its own key.
- RTL: the wrap is index-based.
- CI green.
Perf-relevant: a process-wide 16 MB LruCache of QR bitmaps and two extra pager pages.
Device gate (partial): 0624d16 — not run yet. Pending:
receive/receive-tabs-wraparound.xml;- Receive open and wrap timing with USDT off against master.
Device results will follow in a separate review.
|
Device gate (partial): The wallet has a channel, so the tabs are Savings, Auto, Spending and USDT, and Receive opens on Auto. Savings → Spending therefore passes through Auto (one extra swipe).
Step 7 looks like journey timing rather than an app bug: the journey already says the Savings/Auto/Spending pager snaps back from short synthetic swipes on the emulator, and this step uses 300 ms on that pager. Consider 1200 ms there, as in steps 4-5. Still running: steps 9-14 (USDT off), the re-check of |
|
Device gate (partial 2): The channel adds an Auto tab, so the tabs are Savings, Auto and Spending rather than the journey's two. Steps 11, 12 and 14 therefore pass via Auto as a neighbour; the wrap steps themselves are unchanged.
Perf on the PR build, measured from screenrecord scene changes:
Still running: the master baseline for the same numbers, and the tap-USDT-then-leave check at |
|
Device gate (partial 3): master baseline (
On master the wrap is one large jump (peak scene score about 0.30); on the PR it is spread over frames (0.09-0.15), which is the slide this PR adds. The PR renders more frames because the slide animates. The new-style janky count is higher, but the percentiles are equal or better. No regression and no memory growth from the QR cache. Still running: the two actions |
|
Pushed f71aeb8: the two USDT-exit journey checks use the wording that holds on both platforms, kept identical to the iOS twin. Journey only. |
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Approving. My LOW is fixed in 8488077 and verified on the device.
Device gate: f71aeb8 (app code 8488077) — journeys: 16 passed, 0 failed (receive/receive-tabs-wraparound.xml, including the two actions from bdbb8b3 and the slower above-sheet swipe from 14d81fa, 3/3); perf: Receive open 0.98-1.15 s vs master 1.15-1.43 s; wrap 1.68 s (a 1.2 s drag that now slides, where master snapped); 10 wraps p99 21 ms vs master 25 ms; PSS 317.6 → 310.1 MB with no growth from the QR cache; Figma: n/a (no new UI); l10n: no UI text changed; iOS: same journey on synonymdev/bitkit-ios#1027, 23/23 passed.
Ran on Pixel_9 (emulator). The wallet has a channel, so the tabs are Savings, Auto, Spending and USDT; the two-tab USDT-off steps passed via the Auto neighbour.
Non-blocking: on iOS the change out of USDT is a crossfade, not a slide. I asked on synonymdev/bitkit-ios#1027 to reword actions 24-25 as "goes straight from the USDT QR to the Savings QR" in both repos so the prose stays identical. The 50-70 ms USDT stand-in page at the start of the slide is the loading page you already deferred.
3 participants
Refs: #1590
This PR makes a swipe past the first or last Receive tab slide to the other end, the way a swipe between neighbouring tabs does. Before, the wrap from #1593 jumped there with no animation.
Description
ReceiveQrScreen: the tab pager holds a copy of the last tab before the first page and a copy of the first tab after the last page. A swipe past either end follows the finger onto the copy, and once the scroll settles the pager swaps to the matching real page, which shows the same content, so the wrap reads as one slide.QrCodeImage: generated QR bitmaps are kept in a 16 MB cache by size and content, so a page composed again, such as the real page after a wrap, shows its code at once instead of the loading state.swipePastEdgesis removed; the pager's own scrolling handles the wrap, in left-to-right and right-to-left layouts alike.Out of Scope
ReceiveQrScreen: the pager's USDT page shows a loading QR while the swipe slides onto it, before the USDT screen replaces the pager; this predates the wrap.Design
N/A — no UI changes.
Preview
Pixel 9 emulator (API 36), dev wallet, animations at 1x, recorded at 30 fps and played at 2x. Tabs Savings, Spending and USDT, and Savings and Spending with USDT off.
A tap on the Savings QR still copies.
Savings, swipe right: slides onto USDT
s1-savings-right-wrap2x.mp4
USDT, swipe left: slides on to Savings
s2-usdt-left-wrap2x.mp4
USDT off, Savings, swipe right: slides onto Spending
s8-nousdt-savings-right-wrap2x.mp4
USDT off, Spending, swipe left: slides onto Savings
s9-nousdt-spending-left-wrap2x.mp4
Savings, swipe left: Spending, as before
s3-savings-left2x.mp4
Spending, swipe left: USDT, as before
s4-spending-left2x.mp4
USDT, swipe right: slides back to Spending
s5-usdt-right2x.mp4
USDT, tap the Savings tab: slides to Savings
s6-usdt-tap-savings-tab2x.mp4
Tap the USDT tab, then swipe left on USDT: slides one page on to Savings
t1-tap-usdt-then-swipe-left2x.mp4
QA Notes
Journeys
receive-tabs-wraparound.xml— the wrap slides to the other end; the pager steps take a 1200 ms synthetic swipe on the emulatorManual Tests
N/A
Automated Checks
N/A