fix: present incoming requests automatically - #858
Conversation
|
Comments Outside DiffThese findings could not be posted inline.
|
|
Fixed in 7d67d34. Backgrounding or locking during preparation now exits without deferring the request or setting a retry deadline. The existing ownership checks still run before cleanup, and foregrounding or unlocking can retry immediately. The final branch builds and all 177 selected request, private-payment, and confirmation tests pass. |
There was a problem hiding this comment.
Advice: ✅ Approve
Review: diff 5 files.
Open synonymdev/bitkit-android#1405 presents the request when it arrives and has matching automatic-presentation and request-summary journeys. Android master still waits for refresh to finish. The confirmation layout fix applies only to Android; this iOS change keeps the current layout.
Findings:
1 inline (1 MEDIUM)
QA:
Tests running: 0 of 2 passed.
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest (author)
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Full review of the complete PR diff against its merge base, at e3d857a.
No new actionable code findings.
Newly pending payment-request ids dispatch confirmation before the rest of refresh finishes. Reviewed ids stay filtered by requestsForPresentation(), and an open sheet still owns the screen until it closes. If the scene leaves the foreground or the PIN locks during preparation, send state is reset and the request stays eligible, with no 120-second automatic retry. Unlock calls presentation on its own; an already unlocked return to the foreground presents after the existing foreground refresh.
The background-during-preparation note on this comment matches that reset-and-remain-eligible path at this revision. Shared automatic-presentation and request-summary steps match bitkit-android#1405 at 5985d70, aside from id versus testTag. The Android confirmation-layout journey stays out of scope here.
Validation: This review inspected testPendingRequestArrivalDispatchesPresentationBeforeRefreshFinishes and did not run it. Run Integration Tests passed on this head. Run Tests was still in progress. The author reports the selected request tests passed locally; that run was not repeated here.
Recommended before device testing: Wait for Run Tests on e3d857a.
Suggested additional test cases
-
Platform: iOS, regtest, two linked Bitkit wallets, payer funded for at least 21,000 sats, PIN enabled.
-
Setup: payer unlocked on Home.
-
Action: send a 21,000-sat request and, before confirmation is visible, background the payer so the PIN locks; then foreground and unlock.
-
Expected:
PaymentRequestConfirmappears after unlock without a 120-second wait. Dismiss it and background and unlock again; that same request stays dismissed. -
Platform: iOS, same wallets, PIN disabled.
-
Setup: payer on Home.
-
Action: background the payer during preparation, then return to the foreground.
-
Expected:
PaymentRequestConfirmappears with the foreground refresh, without a 120-second deferral.
Device testing: not performed in this review.
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Follow-up review of the changes since e3d857a, at 8284ac0. The inherited baseline is the previous full review of the complete PR diff against merge base 0cdb704. This pass inspected the interruption-reset delta and rechecked the presentation, unlock, and foreground paths those tests rely on.
No new actionable code findings.
canPresentPreparedRequest still clears send state and leaves the request immediately eligible when the scene is inactive or the PIN is locked, and it skips that reset when the prepared context was already replaced. Returning from that guard does not call deferPresentation, so the 120-second automatic retry is not scheduled. Greptile's background-delay note does not apply at this revision. The coverage thread is addressed by testInterruptedPreparationRemainsImmediatelyPresentableAfterForegroundOrUnlock and testInterruptedPreparationPreservesReplacementPaymentContext.
automatic-presentation.xml and request-summary.xml still match bitkit-android#1405 at 5985d70, aside from id versus testTag. Android was not reviewed beyond those journey files.
Validation: Run Tests and Run Integration Tests passed on 8284ac0. This review inspected the new tests and did not run them. e2e-tests-local was still in progress and is not evidence for this change.
Ready for device testing of the journeys listed on the PR.
Suggested additional test cases
-
Platform: iOS, regtest, two linked Bitkit wallets, payer funded for at least 21,000 sats, PIN enabled.
-
Setup: payer unlocked on Home.
-
Action: send a 21,000-sat request and, before confirmation is visible, background the payer so the PIN locks; then foreground and unlock.
-
Expected:
PaymentRequestConfirmappears after unlock without a 120-second wait. Dismiss it, then background and unlock again; that same request stays dismissed. -
Platform: iOS, same wallets, PIN disabled.
-
Setup: payer on Home.
-
Action: background the payer during preparation, then return to the foreground.
-
Expected:
PaymentRequestConfirmappears with the foreground refresh, without a 120-second deferral.
Device testing: not performed in this review.
Closes #863
Refs: synonymdev/bitkit-android#1404
This PR opens the existing Payment Request confirmation when a new request enters the pending queue.
Description
Out of Scope
Design
Uses the existing Payment Request confirmation sheet in Bitkit handoff. No layout changes; the direct frame link has not been verified.
Preview
QA Notes
Journeys
automatic-presentation.xml- Arrival opens confirmation, another sheet defers it, and reviewed requests stay dismissed.request-summary.xml- Confirmation opens without the bell and preserves requester and note details.Two-wallet regtest checks verified automatic presentation from Home, Receive remaining open while a second request arrived, presentation after Receive closed, and no repeated presentation after dismissing reviewed requests. Android also paid a 1,753-sat request successfully. Device QA applied #854 and Android #1399 in separate scratch builds; this PR does not claim delivery or execution latency is resolved.
Manual Tests
N/A
Automated Checks
PaykitPaymentRequestServiceTests.swift- Pauses refresh after publishing pending requests and verifies immediate dispatch; unchanged IDs do not retrigger presentation.The final branch builds and passes all 177 selected request, private-payment, and confirmation tests on an iPhone 16 simulator. SwiftFormat lint passes for the two changed Swift files.