Conversation
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jvsena42
left a comment
There was a problem hiding this comment.
Approved — no HIGH/MEDIUM findings at head fbc1dc281. (Posted as a comment because GitHub does not allow approving your own PR.)
Checked: Traced the tab state machine in ReceiveQrScreen.kt at the PR head (effects at lines 194, 213, 221, 232, 239, 255) for cold open (no Auto tab yet), warm open (Auto from the first frame), tap and drag before Auto appears, Auto appearing with a Trezor tab in front (LazyRow keys by tab name so the Savings index is preserved and the jump targets the right index), channel drop and return, CJIT confirm, and return from Edit Invoice or the Paykit contact picker (fresh composition, initialTab set by ReceiveInvoiceEditState so the auto-switch is skipped exactly as on master). hasUserSelectedTab is only set by the tab row click (CustomTabRowWithSpacing invokes onTabChange solely from clickableAlpha) and by DragInteraction.Start on the LazyRow interaction source, which programmatic scrollToItem/animateScrollToItem never emit, so the guard cannot be tripped by the screen's own scrolls. The early return on firstVisibleItemIndex == autoIndex cannot suppress a needed jump: the only way to sit on a non-Auto page equal to the new Auto index without a user pick is a CJIT invoice, which hides the Auto tab. invoicesByTab is unchanged, so every page still renders the URI for its own tab; the transient on-chain-only URI under Auto while the old bip21 string waits for refreshBip21 (behind syncState and the geoblock check in WalletViewModel.refreshReceiveState) is the limitation the author already listed as out of scope and existed on master on the Savings page. The showSheet change in AppViewModel.kt:4727 is an in-memory StateFlow update after the transition delay and before the sheet is published; no caller sets bip21AmountSats before showSheet(Sheet.Receive) (all writers are inside the open sheet), cancel by a competing showSheet, hideSheet, or process death leaves nothing half-applied, and refreshBip21 clears the same field on open anyway, so Paykit request routes and the ReceiveRoute.Amount funding entry are unaffected.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…o-tab-switch # Conflicts: # journeys/README.md
…o-tab-switch # Conflicts: # journeys/README.md
jvsena42
left a comment
There was a problem hiding this comment.
Approved — no blocking HIGH/MEDIUM findings at head 007e933a6. (Posted as a comment because GitHub does not allow approving your own PR.)
Inline: MEDIUM (journey, non-blocking) — Instant-jump step waits for the node first, so Savings never shows and the jump cannot be observed; LOW (journey, non-blocking) — First-frame checks rely on one screenshot and pass if the leftover-amount fix is reverted
Checked: Round 4 (7ae8457, new journey): Reviewed the journey commit 7ae8457 only. Every testTag it names exists at head: Receive (TabBar.kt:156), Tab-auto/Tab-savings/Tab-trezor (CustomTabRowWithSpacing.kt:62, Tab-${tab.name.lowercase()}), SpecifyInvoiceButton, ReceiveNumberPadTextField, ReceiveNumberPadSubmit, ShowQrReceive and the N keys. The Edit Invoice route in the last two steps matches the code: source AUTO always yields ReceiveAdditionalLiquidityAction.None (ReceiveLiquidityDecision.kt:42), so Show QR updates the invoice and pops back to the QR screen with no Auto tab, and reopening hits the new bip21AmountSats clear in AppViewModel.showSheet. The cold-start window with no Auto tab is real: channels are only populated by syncState() after lightningService.start returns (LightningRepo.kt:390-395). The README index row and the cross-platform not-ported row are in place.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jvsena42
left a comment
There was a problem hiding this comment.
Approved — no blocking HIGH/MEDIUM findings at head 049c57fdf. (Posted as a comment because GitHub does not allow approving your own PR.)
Checked: Follow-up on 049c57f: the journey MEDIUM and the first-frame LOW are fixed. One remaining LOW (nothing asserts the recording actually contains the jump) is a reply on the existing thread. Reviewed 049c57f only. The cold-start step no longer waits for the node: it taps Receive as soon as home shows and asserts Tab-auto absent, which is the reachable window before LightningRepo.start sets Running and runs syncState(). AUTO is inserted after SAVINGS in visibleTabs (ReceiveQrScreen.kt:132-143), so Savings stays index 0 and the jump targets index 1; on master animateScrollToItem leaves intermediate pager frames that the ffmpeg frame check catches. The reopen step now records and requires the first visible sheet frame to already show Auto selected and no Savings frame, which fails if the bip21AmountSats clear in showSheet is reverted; the underline has no initial crossfade on a first composition, so that check is sound.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jvsena42
left a comment
There was a problem hiding this comment.
Approved — no blocking HIGH/MEDIUM findings at head c5b492353. (Posted as a comment because GitHub does not allow approving your own PR.)
Checked: Follow-up: Follow-up on c5b4923, which answers the LOW that nothing required the recording to contain the jump. The cold-start step is now split into two. The first requires Tab-auto to appear while the recording is still running, and tells the runner to rerun with a longer --time-limit otherwise. The second requires the extracted frames to hold a Savings-centred frame followed by a later Auto-centred frame before it runs the part-way check, and treats a missing frame as a rerun, not a pass. This is the positive requirement asked for. It matches the code: AUTO is inserted after SAVINGS, and the scrollToItem(autoIndex) jump in ReceiveQrScreen.kt:239-251 fires only after the node is ready. The reopen step now also rejects a recording with no sheet frames. Journey-only commit.
|
@jvsena42 conflicts, I am approving |
# Conflicts: # journeys/README.md
There was a problem hiding this comment.
Verdict: ✅ Approve
Reaudit: diff 1 file.
Findings:
N/A
Audit:
Already done in comment.
QA: emu-1 redroid container on Android 15, dev build 2.4.1 (188)
-
✅ passed: Auto was selected on the first Receive frame; no pager slide appeared.
-
✅ passed: Savings remained selected five seconds after the swipe.
-
✅ passed: Savings remained selected when Auto appeared after a user selection.
-
✅ passed: The untouched flow jumped from Savings to Auto in one frame, without an intermediate pager position.
-
✅ passed: An above-capacity invoice reopened Receive directly on Auto, never Savings.
-
✅ passed: Zero-channel CJIT returned to Receive with Spending selected and a Lightning invoice.
Coverage:
QA: 6 of 6 manual tests passed
Reviewed by gpt-5.6-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest · audit (author or owner)









Fixes #876
Related to #1275
This PR:
Description
On a warm open with the node running, master already starts on Auto from the first frame on the emulator; the slide on open did not reproduce in that case. On a cold start, master opens on Savings while the node starts, then moves to Auto and overrides a tab the user picked in the meantime. That override is the part reproduced and fixed here.
Out of Scope
ReceiveQrScreen.kt: when the sheet is opened while the Lightning node is still starting, it still opens on Savings (no Auto tab yet) and then jumps to Auto once Lightning is ready. The jump is now instant, but the Auto QR needs a bolt11 from the running node, so it cannot be shown from the first frame.ReceiveQrScreen.kt: the user-selection flag records that the tabs were touched, not which tab was picked. If the user stays on Auto, Lightning drops (node restart) and comes back, the sheet stays on Savings instead of returning to Auto.AppViewModel.kt: only the leftover amount is cleared on open; the old bip21/bolt11 strings are replaced when the receive state refresh finishes, as before.ReceiveQrScreen.ktand fix: polish wallet ui #1270 edits the tab row there; expect a small text conflict with whichever lands first.Design
Preview
Recorded on a Pixel 9 emulator with a paired hardware wallet (hence the extra Trezor tab). Differences from Figma that already exist on master: the Trezor tab, no contacts icon in the top bar, and no "Scan or tap to pay" hint.
Before (master): cold start, tap Trezor then Savings before the Auto tab exists; when Auto appears the sheet moves to it.
876-master-coldstart-userpick.mp4
After: same flow; Savings stays selected after the Auto tab appears.
876-after-coldstart-userpick.mp4
Before (master): cold start, Receive opened without touching the tabs; opens on Savings, then moves to Auto once Lightning is ready.
876-master-coldstart.mp4
After: same flow; the move to Auto is an instant jump with no slide frames.
876-after-coldstart.mp4
After: five warm opens, each on Auto from the first frame.
876-after-opens.mp4
After: warm open on Auto, swipe to Savings; Savings stays selected.
876-after-swipe.mp4
After: reopening Receive after a $3,000 Edit Invoice amount (above inbound capacity) opens directly on Auto.
876-after-stale-amount.mp4
QA Notes
Manual Tests
Needs a wallet with a usable spending channel.
regression:No channels → Receive → Edit → amount → CJIT flow: still switches to Spending with the CJIT invoice.A journey covering tests 1 to 4 is committed at
journeys/receive/receive-auto-tab-selection.xml;the same patch inline:
receive-auto-tab-selection.xml
Automated Checks
ReceiveAutoTabSwitchTest.ktcovers when the sheet should switch to Auto (initial tab, user selection, Auto already selected, Lightning availability, CJIT invoice).AppViewModelSendFlowTest.ktchecks that opening Receive clears the leftover amount before the sheet is set, and that opening another sheet keeps it.ReceiveAutoTabSelectionTest.ktrendersReceiveQrScreen, flips Lightning to ready, and checks the tab row wiring: a tab picked by tap survives the Auto tab appearing, and with no tap the sheet lands on Auto.just compile,just test,just lintandcompileDevDebugAndroidTestKotlinpassed; the instrumented test itself runs in theUI Testsworkflow.