Repository navigation
feat: toast when a pending payment request expires - #1068
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: 95a6aba278
ℹ️ 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 reaches the in-app payment-request experience. The PR appears safe to merge, with a non-blocking case where a paid request can show an incorrect expiration toast. Findings
SummaryAdds a toast when a pending incoming payment request expires.
Reviews (1) · Last reviewed commit: "feat: toast when a pending payment reque..." · Reviewed by Greptile |
|
Pushed 6d6866f: the expiry report now comes only from |
talosmachina
left a comment
There was a problem hiding this comment.
No findings. Records unopened incoming one-time proposals that discardExpiredRequests drops and toasts once from AppScene. Reviewed 6d6866f (both commits), full tier.
What I checked, and 4 candidates I ruled out
Read in full: recordSilentExpirations, discardExpiredRequests and all its call sites (requestPresentation, deferPresentation, reconcileExpiredRequests, refresh, actions), scheduleExpiration, recordRequestedPresentationExpiration, the AppScene observer and IncomingPaykitPaymentRequestPresentationFeedback; both new tests in PaykitPaymentRequestServiceTests.swift, and the new journey.
CI: SwiftFormat and validate green; unit and integration tests still running on 6d6866f. Swift is not built on this box, so those jobs are the gate.
Ruled out
- Two toasts when an automatic presentation finds the request expired:
deferPresentationreturns.requestExpired(wasRequested: false), whose feedback carries no toast, so the silent path's toast is the only one. - Opened request toasted twice:
requestPresentationpassesopenedRequestId, the requested one is excluded byrequestedPresentationId, and its own expiry goes throughhandledRequestedExpirationId. - Paid or accepted request past its deadline shown as expired: the filter now requires
.proposed, which also matches whatscheduleExpirationschedules. - Toast over the request being paid:
AppScenedrops the id fromapp.contactPaymentContext.
Merge confidence: 4/5, the unit tests are still running on the new head and Swift is not built here.
jvsena42
left a comment
There was a problem hiding this comment.
Code scan at 6d6866f: one LOW, inline with a failing test. Nothing above LOW.
- No false toasts: paid, accepted and in-flight requests are excluded, and so is the opened request on the presentation path. Identity clear and activation don't emit.
- Strings: reuses existing keys.
- Journey: new
payment-requests/expiry-toast.xml, with the same text as Android. - Parity with synonymdev/bitkit-android#1643: iOS skips only the request being paid during a payment, where Android drops every expiry toast. iOS also counts payment-deadline expiry. Both behaviours are reasonable.
Device gate (partial): 6d6866f — not run yet. Pending: expiry-toast.xml, plus backgrounding the app across the expiry and resuming, to see whether the toast shows, shows late or is lost.
Device results will follow in a separate review.
|
Pushed 1466d65: fixes both LOWs (expired toast twice on dismiss/decline; silent expiry missed when a refresh drops the request first), each with a test. |
|
Pushed ca81e9e: parity with synonymdev/bitkit-android#1643 (raised there by @coreyphillips). A refresh no longer reports an expired proposal that the snapshot still keeps, so the expiry timer can't report the same request a second time. |
coreyphillips
left a comment
There was a problem hiding this comment.
Two independent reviews, nothing blocking a merge.
worth doing, does not block
- Expiry toast is not gated on unlock, scene phase or the Paykit UI flag (Bitkit/AppScene.swift:1830).
toastSilentlyExpiredPaymentRequestscallsapp.toastwith no unlock, scene-phase or Paykit flag check, so the toast can show over the PIN screen and is then never repeated. Toasts render in a window at.alert + 1(ToastWindowManager.swift:174), above theAuthCheckPIN screen. With PIN enabled, going to background setsisPinVerified = false. A request that expires while the app is suspended is reported when the expiry task resumes on foreground. The toast then shows over the PIN pad and hides after 4 seconds. BecausereportedSilentExpirationIdsalready holds the id, it never shows again after unlock. The same loss happens for an expiry in the few seconds after backgrounding, while the app still runs but is not visible. Automatic presentation already waits forisPinVerified || !settings.pinEnabledandscenePhase == .active. There is also noPaykitFeatureFlags.isUIEnabledcheck. Disabling Paykit UI from Dev Settings (ContactPaymentsService.disablePaykitUI) does not clear the manager, so a pending request still toasts after the bell is hidden. The Android twin returns early whenisPaykitEnabledis false. Found by reading the code. Not reproduced on a device. A fix is to hold the toast until the scene is active and unlocked, and to check the flag. - A request expiring mid-decline is reported as silently expired (Bitkit/Services/PaykitPaymentRequestService.swift:2813). If a request expires while its Decline call is in flight, the user sees "The payment request has expired." right after tapping Decline.
recordSilentExpirationsskipsrequestedPresentationIdand the opened id but notprocessingRequestIds. Decline from the Payment Requests list or details runsdismiss, thenreject, thenperform.performkeeps the id inprocessingRequestIdswhileservice.rejectruns. If the expiry timer fires during that call,reconcileExpiredRequestsreports the request. If the SDK then refuses the reject,app.toast(error)shows a second toast. The pay path avoids this only because AppScene compares againstcontactPaymentContext. SkippingprocessingRequestIdsin the filter would cover both cases inside the manager, where a unit test can reach it. This is a narrow race, found by reading the code. - Explicit expiry feedback can be followed by a duplicate toast (Bitkit/Services/PaykitPaymentRequestService.swift:2815). An in-flight refresh can restore a request after its explicit expiry feedback and trigger another toast. Hold a refresh after its snapshot is read, expire a request with
requestedPresentationIdset, then resume the refresh. The requested-expiry path never adds the ID toreportedSilentExpirationIds, so the finaldiscardExpiredRequestsreports it again. Running the extracted manager methods confirmed that both expiry triggers increment for the same request.
nits
- clear() leaves silentlyExpiredRequests holding the previous identity's requests (Bitkit/Services/PaykitPaymentRequestService.swift:2238).
clear()resetsreportedSilentExpirationIdsbut notsilentlyExpiredRequests. After sign-out or an identity switch, the manager keeps the last batch of expired requests from the previous identity, including notes and counterparty, until the next report. Nothing reads the array unlesssilentExpirationTriggerchanges, so this is cleanup only. Resetting it next toreportedSilentExpirationIdswould match the rest ofclear().
|
@coreyphillips thanks. Fixed all four in 6a17ec6:
|
coreyphillips
left a comment
There was a problem hiding this comment.
Rechecked after 1 new commit (6a17ec6). Nothing blocking remains.
addressed
- Expiry toast is not gated on unlock, scene phase or the Paykit UI flag (Bitkit/AppScene.swift:1830). Fixed:
showPendingExpiryToast(AppScene.swift:1841-1847) drops the toast whenPaykitFeatureFlags.isUIEnabledis false. Otherwise it holds it untilscenePhase == .activeandisPinVerified || !settings.pinEnabledhold. It retries from.onChange(of: isPinVerified)(544) and.onChange(of: scenePhase)(712). I traced background, then foreground with PIN: line 1275 clearsisPinVerifiedfirst, so the toast now waits for the PIN entry. Checked by reading only; no Xcode on this machine. - A request expiring mid-decline is reported as silently expired (Bitkit/Services/PaykitPaymentRequestService.swift:2813). Fixed:
recordSilentExpirationsnow skipsprocessingRequestIds(PaykitPaymentRequestService.swift:2817).performinserts the id beforeservice.rejectand removes it in adefer, after the catch path'srefresh(excludingProtectedRequestId:). So neither the timer nor that refresh reports a request mid-decline. Without the line, the new test'sreconcileExpiredRequestscall would bump the trigger to 1. I read the test, I did not run it. - clear() leaves silentlyExpiredRequests holding the previous identity's requests (Bitkit/Services/PaykitPaymentRequestService.swift:2238). Fixed:
clear()now resetssilentlyExpiredRequestsnext toreportedSilentExpirationIds(PaykitPaymentRequestService.swift:2238-2239).
still open
- Explicit expiry feedback can be followed by a duplicate toast (Bitkit/Services/PaykitPaymentRequestService.swift:2815). An in-flight refresh can restore a request after its explicit expiry feedback and trigger another toast. Hold a refresh after its snapshot is read, expire a request with
requestedPresentationIdset, then resume the refresh. The requested-expiry path never adds the ID toreportedSilentExpirationIds, so the finaldiscardExpiredRequestsreports it again. Running the extracted manager methods confirmed that both expiry triggers increment for the same request. Line 2840 marks the id whenrecordRequestedPresentationExpirationfires, which covers the timer and refresh paths. The new test covers exactly that.deferPresentationpasses the expired requested id ashandledRequestedExpirationId, so the guard at 2833 returns before the insert. ItsPaymentRequestExpiredToastfeedback (AppScene.swift:39) therefore leaves the id unmarked. So doesdiscardExpiredRequests(openedRequestId:)fromperform(2702), whoserequestExpirederror a Decline toasts. In both cases a refresh holding an older snapshot can restore the request and toast it again. Inserting the handled and opened ids indiscardExpiredRequestswould close both.
new since the last review
worth doing, does not block
- Held expiry toast survives a sign-out behind the PIN screen (Bitkit/AppScene.swift:628).
hasPendingExpiryToastis not reset when the Pubky profile is cleared, so a toast held behind the PIN screen still shows after a sign-out. Sequence: a request expires while the app is backgrounded, soqueueSilentExpiryToastsets the flag and waits. On return,handleScenePhaseChangerunspubkyProfile.checkAdoptedSource()andrestoreSessionIfNeeded()behind the PIN screen. Either can callclearAuthenticatedState, which setsauthState = .idle. AppScene.swift:628 then callspaykitPaymentRequestManager.clear()but leaveshasPendingExpiryToastset. After unlock the user sees "source lost" or "session expired", then "The payment request has expired" for a request of the profile that was just signed out. The request really did expire, so the effect is small. But the PR says sign-outs never lead to this toast. Resetting the flag next to theclear()call at line 628 would fix it. Found by reading; not reproduced. - Deferred expiry toast survives sign-out (Bitkit/AppScene.swift:1842). Expiry feedback queued behind the PIN screen survives sign-out. For an adopted identity, let a request expire in the background, remove the identity in its source app, then return to Bitkit.
checkAdoptedSource()clears authentication, but the session observers never resethasPendingExpiryToast. Unlocking then shows the previous session's expiry toast. Exercising the extracted callbacks confirmed that clearing the manager leaves this toast queued.
|
@coreyphillips thanks, merged. The sign-out reset is in #1093. |
Refs #1051
Twin: synonymdev/bitkit-android#1643
Description
Out of Scope
Design
N/A — reuses the existing expired-request toast.
Preview
A pending request expires on Home: the bell clears and the toast shows.
ios-toast-2x.mp4
QA Notes
Journeys
expiry-toast.xmlManual Tests
N/A
Automated Checks
PaykitPaymentRequestServiceTests.swift(an unopened request expiring is reported; an opened or requested one is not)PaykitPaymentRequestServiceTestson a simulator