Repository navigation
Conversation
…sume After a stay in the background long enough for the Electrum server to drop the connection, show the send sheet's connecting screen for on-chain payments until a wallet sync succeeds again, then continue on its own; after 20 s it shows the existing connection-issues screen.
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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 59a2863a24
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| return false | ||
| // After a long stay in the background, wait until a sync proves the on-chain connection works again. | ||
| return OnchainConnectionMonitor.shared.isReconnecting |
There was a problem hiding this comment.
When a payment URI is handled during this reconnect window, AppViewModel.handleScannedData still treats a running node as fresh and validates the request against cached on-chain balances before SendSheet is created. If funds arrived during the long background stay and the old balance is insufficient, the scan is rejected immediately, so this overlay is never displayed and the successful retry cannot revalidate the payment. The reconnect state must also defer that pre-sheet balance check.
Useful? React with 👍 / 👎.
 [Medium impact] The change reaches wallet reconnect and on-chain payment flows. The PR appears safe to merge, with a non-blocking connection-warning issue for already-validated payments. Findings
SummaryAdds an on-chain reconnect wait after at least 30 seconds in the background.
Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Return after 30 seconds] --> B[Show connecting screen]
B --> C[Try wallet sync]
C -->|Failure| D[Wait three seconds]
D --> C
C -->|Success| E[Continue payment]
B --> F{20 seconds passed}
F -->|Not yet validated| G[Show connection issues]
F -->|Already validated| H[Keep connecting screen]
Reviews (1) · Last reviewed commit: "chore: name the changelog fragment after..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // After a long stay in the background, wait until a sync proves the on-chain connection works again. | ||
| return OnchainConnectionMonitor.shared.isReconnecting |
There was a problem hiding this comment.
An already-validated on-chain sheet now shows the reconnect screen after a long background stay, but its 20-second timer exits because hasValidatedAfterSync is still true. If the device has Internet access but Electrum stays unreachable, users keep seeing “Connecting to network” instead of “Connection issues.” Let the reconnect timeout run even when payment validation has finished, and cover this resume path in a test.
Knowledge Base Used: Payments and transfers
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
talosmachina
left a comment
talosmachina
left a comment
There was a problem hiding this comment.
1 finding (1 P1). Adds OnchainConnectionMonitor: after 30 s or more in the background, on-chain sends wait on the existing connecting screen until a wallet sync succeeds (retrying every 3 s), then resume validation on their own. Reviewed 59a2863, full tier, reasoned from code and CI: iOS does not build on this machine.
What I checked, and 5 candidates I ruled out
Read in full: OnchainConnectionMonitor.swift, OnchainConnectionMonitorTests.swift, SendSheet.swift (shouldShowSyncOverlay, setUpSend, validatePaymentAfterSync, handleSyncTimeout, the syncTimeoutPhase task), AppScene.handleScenePhaseChange, LightningService.sync, TabBar.onSendPress, SheetViewModel.sendSheetItem.
CI at review time: validate, detect-changes, Greptile green; Run Tests, Run Integration Tests and build-local still pending.
Ruled out
- Retry loop surviving backgrounding:
appDidEnterBackgroundcancels the task;Task.sleepthrows on cancel and the loop checksisCancelled, and the third test pins the resume on the next activation. - A late success after cancel clearing
isReconnecting: theguard !Task.isCancelledaftersync()covers it. - Validation running twice when the overlay ends: the new
onChangebranch requires!hasValidatedAfterSync, which every exit ofperformPaymentValidationAfterSyncsets except.waiting. - Concurrent
syncWalletswith the foregroundwallet.sync(): both go throughServiceQueue.background(.ldk), which serialises them. - USDT and Paykit asset-selection sends caught by the wait: the early return in
shouldShowSyncOverlayexcludespaykitUsesUsdtand the asset-selection root before the new line. - Cold reader: the finding stood; it rated it P2 since the scan button still reaches Lightning. Kept at P1 because Send itself is blocked on every long resume until a sync lands.
Merge confidence: 3/5, the one P1 on the empty Send sheet; the test jobs had not finished and the branch was not built here.
Only a send with an on-chain target waits for the network after resume; the options screen stays reachable for scan, paste and Lightning.
|
Combined into #971 so RC3 needs one iOS e2e run and one review instead of three. |
2 participants
Refs #976
Refs:
Description
Out of Scope
Design
N/A — no design available; reuses the existing send sheet connecting screen.
Preview
Regtest staging, iPhone 17 Pro simulator (iOS 26.5), 2x, built with #971. Bitkit goes to the background for 40 s while a local Electrum bridge drops its connection, then a
bitcoin:link opens the payment like a Shop deep link. "Connecting to network" shows while the first sync fails on the dropped connection, the amount screen opens on its own once a sync succeeds, and the payment is sent.c2x.mp4
QA Notes
Journeys
N/A
Manual Tests
Automated Checks
OnchainConnectionMonitorTests.swift— a short background stay never waits; a long one waits until a sync succeeds, retrying failures; an unfinished wait resumes after a short background stay.