Repository navigation
fix: exit lnurl screens whose payment data is gone - #1101
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc2fad4926
ℹ️ 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".
[Medium impact] The change is limited to Send's LNURL payment screens. Fix the exit from an LNURL amount root before merging. Findings
SummaryAdds an automatic exit when an LNURL amount or confirmation screen loses its payment data.
Reviews (1) · Last reviewed commit: "fix: exit lnurl screens whose payment da..." · Reviewed by Greptile |
|
Pushed
|
talosmachina
left a comment
There was a problem hiding this comment.
No findings. Sends an LNURL screen whose payment data is gone back to the screen before the LNURL route, or closes the sheet when an LNURL route is the root, skipping scans still being handled and payments being submitted. Reviewed a58030b1, full tier, stacked on #1099 (fix/lnurl-flow-state-1087), delta reviewed against that base.
What I checked, and 6 candidates I ruled out
Read in full: SendSheet.swift (body modifiers, cleanup, setUpSend, failure/retry routes, exitMissingLnurlPayment), LnurlPayAmount.swift, the LnurlPayConfirm.swift submit path, AppViewModel.handleScannedData / resetSendState, and the LNURL routing in PaymentNavigationHelper.swift and SubscriptionsView.swift.
Callers traced: every resetSendState() call site that can run while Send is up, and every producer of .lnurlPayAmount / .lnurlPayConfirm.
CI: SwiftFormat green; Run Tests still pending at review time. iOS does not build here, so this review is reasoned from the code and from CI.
Ruled out
- Variable-amount flow left on a dead amount screen: at
fc2fad49, a home-scanner variable LNURL (root.lnurlPayAmount, path[.lnurlPayConfirm]) losing its data popped to the root, which still has no data, andonChangedoes not fire again for true to true.a58030b1checkscurrentRootfirst and closes, andtestMissingLnurlDataClosesAmountRootedPresentationcovers it. - Exit during a rescan:
handleScannedDataclears send state before it sets the new data, butisHandlingScannedDataholds the exit off, and sincescannedDataHandlingIdis now@Publishedthe condition is re-evaluated when handling ends. - Exit while paying:
LnurlPayConfirmsetsisSubmittingPaymentfor the whole submit, so the data cannot disappear mid-payment and close the sheet. - Exit over a failure, pending or success screen: those routes are pushed on top (or replace the root in embedded Send), so
navigationPath.lastis not an LNURL route and nothing fires. - Embedded initial-subscription retry:
retryEmbeddedInitialSubscriptionPaymentresets send state across anawaitbefore the scan, but it runs from the failure route, which is not an LNURL route. - Stale sheet closing a newer one: the handler checks
isCurrentPresentation, and the exit goes throughhideSheetIfActive.
Cold reader: the run on the fc2fad49 candidate timed out with no verdict. The author's a58030b1 fixed that candidate anyway.
Merge confidence: 4/5, no findings, but Run Tests was still pending and the iOS change was not built here.
Reviewer model: claude-opus-5-5
Reasoning effort: Unknown (this session's run metadata does not expose the effort setting)
Dependency: #1099
This PR closes the remaining gap in #1087 on top of #1099. When LNURL payment data is really gone, Send now leaves the LNURL screen instead of showing a dead amount screen or a blank confirmation. Reported by @ben-kaufman. @piotr-iohk, this targets your branch; merge it there if it looks right.
Description
SendSheet: when the current presentation showslnurlPayAmountorlnurlPayConfirmwith nolnurlPayData, it closes the sheet when the root is an LNURL route (a home-scanner LNURL, including after continuing to confirmation). Otherwise it pops back to the screen before the LNURL route. The issue asks that an unavailable flow exit safely and notes that a nil guard alone is not enough. With fix: preserve active LNURL payment state #1099 alone, the amount screen stays with a max of 0 and the confirmation renders nothing.AppViewModel.isHandlingScannedData, sincehandleScannedDataresets send state before it sets the new data; the handling id is published, so the check re-runs once handling ends), a payment being submitted, and a presentation that is no longer current.SendSheetLifecycleTests: a new test removes the data from a live confirmation and expects the sheet to close. The embedded test now sets the LNURL data before mounting embedded Send, asSubscriptionSheetdoes: it routes to.lnurlPayConfirmonly once the data exists.Out of Scope
Bitkit/Views/Wallets/Send/SendSheet.swift: the remount concern in the Greptile thread on synonymdev/bitkit-ios#1099 (route and amount after recreated content) stays with that PR.Design
N/A — no UI changes.
Preview
N/A
QA Notes
Setup
Same as #1099.
Journeys
N/A — the exit fires only when the data is lost while an LNURL screen is shown, which no journey can trigger on demand. #1099's J1 still applies to the normal flow.
Manual Tests
N/A
Automated Checks
SendSheetLifecycleTests.swift— a confirmation or amount screen whose LNURL data disappears closes its sheet.SendSheetLifecycleTests.swift— embedded Send is mounted after its LNURL data is set, as in production.Evidence
On
fc2fad49(unit-workflow selection) anda58030b1(focused tests), iPhone 17 simulator, iOS 27:SendSheetLifecycleTests,LnurlPayConfirmTests,SheetViewModelTestsandScannerManagerTestspassed in 3 iterations ona58030b1.SendConfirmationLifecycleTests.testCustomFeeWalletSwitchSurvivesFeeScreenNavigation. That failure is known on master (bug: send confirmation lifecycle tests fail intermittently on master #1096), and its fix is open in fix: prepare the wallet switch once on send review #1098.Gaps
lnurl.e2e.tswas not run locally on this branch. CI's E2E runs on the PR.Models used
claude-opus-5-5(reasoning:low)claude-opus-5-5(reasoning:low)