Conversation
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). |
jvsena42
left a comment
There was a problem hiding this comment.
Approved — no HIGH/MEDIUM findings at head 8e516f904. (Posted as a comment because GitHub does not allow approving your own PR.)
Checked: Traced the retry loop in SendCoinSelectionViewModel.listSpendableOutputsWithRetry (lines 99-108): exactly 3 listSpendableOutputs calls with 1s and 2s delays, the third result is returned as-is, and only NodeNotRunningError and ServiceError.NodeNotSetup trigger a retry, so the bound holds and NodeRunTimeoutError, LdkError and fee errors fall through on the first attempt. Cancellation is sound: loadUtxos cancels the previous job before launching a new one, every stale job is parked at a suspension point (executeWhenNodeRunning's withContext, delay, or calculateTotalFee's withContext) so it resumes with CancellationException, and runSuspendCatching rethrows it, so a cancelled attempt never runs the success or failure branch and cannot overwrite the newer job's state or clear isLoading; executeOperation and calculateTotalFee both rethrow CancellationException rather than folding it into a Result. On a first-load failure availableUtxos is empty and isSelectionValid stays false, so Continue is disabled while the inline error and Retry are shown; SecondaryButton disables itself while isLoading. The known limitation that a failed reload with coins already listed keeps the stale list, and that a selection can be made during the retry window, is stated in the PR body as out of scope and was not re-raised. Splitting the activity effect from the load effect removes the reload on activity change; tag loading via UtxoRow's LaunchedEffect(txid) behaves the same as before since items() had no key and re-loads did not re-run it. PR #1286 is this PR's base branch (fix/894-remove-coin-selection-auto), so there is no merge-semantic conflict, only a stacked dependency. The unit tests run on UnconfinedTestDispatcher with virtual time, so the times(2)/times(3) call counts and the mid-retry isLoading/loadError assertions exercise the new code rather than passing on master.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…to' into tmp-1288 # Conflicts: # journeys/README.md
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 a0fd13669. (Posted as a comment because GitHub does not allow approving your own PR.)
Inline: LOW — Journey omits the Received Bitcoin sheet that the self-deposit opens over coin selection; LOW — Sibling journey manual-coin-selection.xml still describes the activity-triggered reload this PR removes
Checked: Round 3 (a0fd136, load-state centering + journeys): Reviewed the delta since 8e516f9; content merged in from #1286 was not reviewed. In SendCoinSelectionScreen.kt (lines 125-146) the loading and error items cannot show at the same time: the spinner needs loadError == null and the error needs loadError != null, so the two fillParentMaxSize() items never stack and push each other off screen. During a retry (isLoading with loadError set) only the error item renders, the message stays, and Retry shows its loading state. Neither item renders once availableUtxos is non-empty, so the UTXO rows are never covered by a full-height placeholder. fillParentMaxSize() sizes each item to the LazyColumn viewport, which is bounded by weight(1f). The totals and Continue sit outside the LazyColumn, and Continue is still gated on isSelectionValid (line 190), which the delta does not touch. The CoinSelectionLoading, CoinSelectionLoadError, CoinSelectionRetry, coin_selection_screen and utxo_row_ tags are all present at head, and every other tag the new journey names exists at head (HeaderMenu, DrawerSettings, Tab-advanced via CustomTabRowWithSpacing, CoinSelectPreference, manual_button, SendNumberField, sync_node_view, ContinueAmount). The error string matches wallet__selection_load_error. No bitkit-ios twin PR exists, and the journey is recorded as not ported in the cross-platform table.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ion-load-retry # Conflicts: # journeys/README.md
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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 2d6d6053c. (Posted as a comment because GitHub does not allow approving your own PR.)
Checked: Follow-up on f7b31b3, 9fe4283, 378f6df and dd46737. Both journey LOWs are fixed: the load journey now closes the Received sheet with its OK button (new_transaction_sheet / ReceivedTransactionButton, both in NewTransactionSheet.kt) and warns against Back, and the stale "reloads on activity" note is gone from the manual coin selection journey. The only production change renames the private Content to a public SendCoinSelectionContent so SendCoinSelectionContentTest can drive it; the new tests pin the spinner-only, error-with-retry, retrying and list branches and the retry callback. The preview trailing-comma change follows the repo rule.
There was a problem hiding this comment.
Verdict: ✅ Approve
Reaudit: diff 4 files.
Findings:
N/A
Audit:
Audited - no findings.
QA: emu-1 redroid container on Android 15, dev build 2d6d605
-
✅ passed: Manual mode listed all UTXOs selected, with no toast or load error.
-
✅ passed: The deselected UTXO and 150 000 sat total survived activity sync without reloading.
-
✅ passed: A force-stop deeplink loaded all UTXOs with no toast or load error.
-
✅ passed: Forced failure centered loading and error with Continue disabled; Autopilot still skipped Coin Selection.
-
✅ passed: Retry kept the error visible while the button spinner ran.
-
✅ passed: Retry loaded all UTXOs and cleared the error after failures stopped.
Tip
Test 1 worth a journey:
- Open Settings → Advanced → Coin Selection
- Select Manual
- Return to the wallet home screen
- Open a bitcoin deeplink for 10 000 sats
- Verify the amount and tap Continue
- Verify every UTXO is listed and selected
- Verify no toast or load error appears
Test 2 worth a journey:
- Deselect the smallest UTXO
- Verify the selected total decreases
- Receive and confirm a new on-chain payment
- Wait for wallet activity to sync without leaving the screen
- Verify the same UTXO rows remain
- Verify the deselected row and selected total remain unchanged
Test 3 worth a journey:
- Force-stop the app
- Open a bitcoin deeplink for 10 000 sats
- Wait for the amount screen to become ready
- Tap Continue
- Verify the UTXOs load
- Verify no toast or load error appears
Coverage:
Unit tests: 90% - SendCoinSelectionContentTest.kt covers six load, error, Retry and list branches; centering remains preview-only.
QA: 8 of 8 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)
# Conflicts: # journeys/README.md
There was a problem hiding this comment.
Verdict: ✅ Approve
Reaudit: diff 0 files.
Findings:
N/A
Audit:
Already done in comment.
QA: carried
- ⏭️ skipped: tested on ef39fa1, recording
- ⏭️ skipped: tested on ef39fa1, recording
- ⏭️ skipped: tested on ef39fa1, recording
- ⏭️ skipped: tested on 2d6d605, recording
- ⏭️ skipped: tested on ef39fa1, recording
- ⏭️ skipped: tested on 2d6d605, recording
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 #616
Related to #1275
This PR adds a bounded retry and an inline error with a Retry button to the coin selection load, and stops activity updates from reloading the list.
Description
The race that originally caused the "Failed to load utxos" toast was fixed by c32193f and 27d4f0c; #616 is now about the error and retry handling, and could be retitled to match.
Out of Scope
SendCoinSelectionScreen.kt: going back from Send Confirm to Coin Selection still re-runs the load. A successful reload replaces a manual selection with all UTXOs, and a failed reload with coins already listed leaves the stale list without an error, because the error block only renders on an empty list. The same applies to a toggle made during the up-to-3s retry window. Deferred to a follow-up.SendCoinSelectionViewModel.kt:ServiceError.NodeNotStartedandNodeRunTimeoutErrorare not retried.LightningRepo: it already logs each failedlistSpendableOutputsattempt, so a persistent failure logs once per attempt plus the final view model error. Repo logging is unchanged.changelog.d/next/616.fixed.md: the fragment says coin selection "retries while the Lightning node is starting", but the retry covers the node not being set up or not running. A node that is still starting is already handled by the repo's wait. The wording can be tightened before release.journeys/coin-selection/manual-coin-selection-load.xmlis committed here (85757ed), with its rows injourneys/README.md. It is Android-only for now; iOS hasSendUtxoSelectionViewbut no load error or retry to assert on.SendCoinSelectionScreen.kt: theCoinSelectionLoadErrorandCoinSelectionRetrytest tags do not reach the accessibility tree, soandroid layoutcannot see them and a journey cannot assert on them; they still work with ComposeonNodeWithTag. This is pre-existing and not specific to this PR —SecondaryButtonapplies itsmodifierto a plainBoxwrapper, so everySecondaryButtontest tag in the app behaves the same way (verified againstResetToDefaulton the Electrum Server screen). Left as is.Design
Send (Paste) (On-chain) › Coin selection. The loading and error/retry states have no design.
Preview
All media below was captured from the PR head build, on a regtest emulator with Coin Selection set to Manual. The loaded-state media is from 2026-09-17; the loading and error/retry media was re-captured on 2026-09-18, after the states were centered.
The full flow, from the wallet home through a
bitcoin:deeplink to Coin Selection and a manual deselection:v01-manual-coin-selection-flow.mp4
The five below were captured with a temporary local change that made
listSpendableOutputsfail withNodeNotSetupfor the first 6 calls. That change is not part of this PR.The centered spinner, the centered load error, the Retry tap, the button's loading state, a second failure and the load that finally succeeds, on that same simulated-failure build:
retry-flow.mp4
On a cold start the Send sheet shows "Connecting to network" until the node runs, so the retry path cannot be reached on a healthy wallet.
QA Notes
Manual Tests
bitcoin:deeplink with an amount → Continue once the node runs → Coin Selection: UTXOs load with no toast.listSpendableOutputsforced to returnServiceError.NodeNotSetup→ Coin Selection: a spinner centered in the empty list area, then after 3 attempts "Bitkit could not load your coins. Please try again." with Retry, centered in that same area without moving; Continue disabled.regression:Settings → Advanced → Coin Selection → Autopilot → Send on-chain: Coin Selection is skipped and Send Confirm opens.Steps 1 and 2 were run on a regtest emulator and written up as a new journey, committed here at
journeys/coin-selection/manual-coin-selection-load.xml:manual-coin-selection-load.xml
Automated Checks
SendCoinSelectionViewModelTest.kt:NodeNotSetupfollowed by success loads UTXOs.NodeNotRunningErroris retried.NodeRunTimeoutErroris not retried and sets the error.NodeNotSetupsets the error after 3 attempts and sends no toast.SendCoinSelectionContentTest.kt, which rendersSendCoinSelectionContent(renamed fromContentand no longer private, so the test can reach it, the same waySendAmountContentTest.ktreachesSendAmountContent):CoinSelectionLoadingand neither error tag.CoinSelectionLoadErrorandCoinSelectionRetryand no spinner.CoinSelectionRetrycallsonRetry.PreviewLoadingandPreviewLoadErrorinSendCoinSelectionScreen.ktcover both centered states.just test file SendCoinSelectionViewModelTest(14 passed),just compile,just test(2608 passed, 0 failed) andjust lint.SendCoinSelectionContentTest.ktwas compiled only, throughjust build compileDevDebugAndroidTestKotlin; instrumented tests need a device and were not run here, they run in CI'sui-tests.yml.