Repository navigation
fix: keep a pending pubky sign-in link when another link opens - #1547
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. |
 [Medium impact] The change reaches Bitkit’s incoming-link and scan flow. Fix the stale payment replay before merging; a newer payment link can leave an older payment waiting to reopen. FindingsSummaryThis PR keeps a pending Pubky sign-in ahead of a payment link and delays payment links while Authorize is open.
Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Locked: sign-in plus payment A"] --> B["Unlock drains sign-in"]
B --> C["Authorize opens; payment A remains"]
C --> D["Payment B arrives"]
D --> E["B enters deferredScan; A remains in scanAfterPubkyAuth"]
E --> F["Close Authorize"]
F --> G["Payment B opens"]
G --> H["Close Send"]
H --> I["Older payment A opens"]
Reviews (1) · Last reviewed commit: "fix: keep a pending pubky sign-in link w..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: aff356cd97
ℹ️ 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".
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
|
Pushed 475f44b, answering the Greptile and Codex findings:
|
talosmachina
left a comment
talosmachina
left a comment
There was a problem hiding this comment.
No findings. Keeps a pending Pubky sign-in link when another link opens: the sign-in holds the deferred slot, a non-sign-in link waits in a single follow-up slot (scanAfterPubkyAuth), and links that arrive while the Authorize sheet is showing or transitioning in are deferred until it closes. Reviewed 475f44b, full tier, reasoned from the code and CI (not built here).
What I checked, and 5 candidates I ruled out
Read in full: launchScan, deferLockedScan, enqueueDeferredScan, flushDeferredScan, markDeferredScanUnlocked, invalidatePaymentRequestPresentation, showSheet/hideSheet, handlePubkyAuth, processDeeplink, PubkyAuthApprovalSheet dismiss paths, and the four new or changed tests in AppViewModelSendFlowTest.kt.
Diffed aff356c..475f44b against the five Greptile/Codex threads: older payment replay, newer sign-in on the PIN screen, the sheet-transition window and the PIN-screen test gap are all addressed by that commit and covered by the added tests.
Ruled out
- Follow-up slot orphaned and replayed after an unrelated scan: it can only outlive its sign-in if the sign-in job is cancelled, and a sign-in started from
flushDeferredScanispreserveUntilComplete, so later scans queue behind it instead of cancelling it. Every normal completion (sheet shown, identity toast, init timeout) goes throughinvokeOnCompletiontoflushDeferredScan. - Stale
transitionSheetdeferring links forever:hideSheetnullssheetTransitionJobso the completion handler skips the reset, butisPubkyAuthSheetShownOrPendingalso requiressheetTransitionJob?.isActive, so the stale value is never read. - QuickPay auto-paying a link deferred behind an open Authorize sheet: an unlocked link already QuickPays when handled immediately, and the existing deferral behind a running scan does not suppress it either. Links deferred across the lock still get
suppressQuickPaythroughmarkDeferredScanUnlocked, which now covers both slots. - Duplicate check ignores the follow-up slot: a repeat of the same link only replaces the queued one with itself; the only difference is QuickPay on a link the user tapped again while unlocked, the same as a fresh tap.
- Incoming Paykit request jumping the queue:
hasDeferredScancounts both slots, soisPaymentRequestPresentationBlockedholds while a follow-up is waiting.
CI: lint, detekt, build-local and build (which runs testDevDebugUnitTest) green on 475f44b; the local e2e shards were still running at review time.
Merge confidence: 4/5, the queue paths of the 7,800-line AppViewModel.kt were read by function rather than whole, and the e2e shards had not finished.
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Code scan at 475f44b: no code findings. I reviewed it as a deep-link and auth change.
Both cases in #1545 are fixed:
- Locked, sign-in link then a
bitcoin:link.enqueueDeferredScanputs the payment inscanAfterPubkyAuth(AppViewModel.kt:3239), and the reverse order swaps them (:3244). Unlock marks both assuppressQuickPay, andflushDeferredScanruns the sign-in first. Every way out of the Authorize sheet (Authorize, Cancel, swipe, Back, close after success) ends inhideSheet()→ flush → the payment, which goes throughhandleScanto review with no QuickPay. A failed sign-in also completes and flushes. - Unlocked with Authorize shown or in transition. A non-auth scan is deferred (
:3062;isPubkyAuthSheetShownOrPending()covers the transition viatransitionSheet,:6655-6656). A newer sign-in link still replaces the sheet, as the body states.
Checked and clean:
- No new silent-drop path. Each slot keeps the newest link. Dismissing a payment request clears both slots (
:1895-1900). - No queue growth: there are two fields, both replaced, and the invariant "non-auth
deferredScan⇒scanAfterPubkyAuth == null" holds. - The kept link lives in memory only and needs the Authorize sheet, which re-checks the requester and asks for local auth.
transitionSheetis cleared with its job, andhideSheet()nulls the job, so a stale transition cannot keep links deferred.- The 5 new tests cover sheet-open deferral, both PIN orders, newer-payment replacement, newer-sign-in precedence, and a locked payment behind a sign-in awaiting identity.
- Not performance-relevant (two nullable fields under the existing lock).
LOW, journeys: this changes the order users see sheets in, but #### Journeys adds none, and the body says the manual tests need capabilities the table lacks. journeys/README.md:138 lists am start -a android.intent.action.VIEW -d "<uri>". security/pin-lock-on-resume.xml drives the PIN pad, and pubky-auth/signin-cold-start.xml already does force-stop, sign-in link, unlock and PubkyAuthAuthorize. Extending signin-cold-start.xml with "open a bitcoin: link on the PIN screen → Authorize first, then Send", plus one case with Authorize already open, would make both repros a journey. Port it to iOS, or record it in the cross-platform table.
Device gate (partial): 475f44b — not run yet. Pending, with PIN on and a Bitkit-held identity:
- A sign-in link, then a
bitcoin:link on the PIN screen, in both orders. Expect Authorize, then Send after close, with theQueuing 'deeplink' scan after a sign-in linklog line and noReplacing deferred scan. - A
bitcoin:link while Authorize is open. Expect the sheet to stay and Send to open after Cancel.
The device result will follow.
|
Pushed 747075f: a link reopened while a sign-in is pending now merges with the queued duplicate, keeping its QuickPay suppression and payment context, as @ben-kaufman asked. |
@jvsena42 added in 685c9d4: |
|
Pushed 45abb40: the iOS check reproduced the drop on the PIN screen, so the cross-platform row and the journey now point at synonymdev/bitkit-ios#985 instead of a pending check. Docs only. |
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Approving at 45abb40. My journeys LOW is addressed: journeys/pubky-auth/signin-with-second-link.xml is added and listed.
Delta since my code scan at 475f44b: 747075f moves the duplicate merge into mergeDuplicate and applies it to the follow-up slot too. A link re-delivered after crossing the lock now keeps suppressQuickPay, which can only become true through the merge, and keeps its contact payment context. Clean.
Device gate: 45abb40 — journeys: the PR's two repro cases pass on Android (CI APK, fresh profile, 200k funded, PIN on). The new journey file itself was not driven step by step. perf: not relevant. l10n: no UI text changed. No crash or ANR.
- Sign-in link, then
bitcoin:on the PIN pad. Authorize shows first and Send stays hidden. After Cancel, Send opens with 10 000 sats, and there is noReplacing deferred scan:18:53:11.246 INFO [AppViewModel.kt:3258] Queuing 'deeplink' scan for deferred handling: 'redacted#11…' 18:53:17.473 INFO [AppViewModel.kt:3240] Queuing 'deeplink' scan after a sign-in link: 'redacted#e7…' 18:53:24.332 DEBUG [AppViewModel.kt:3096] Starting scan from 'deeplink': 'redacted#11…' 18:53:45.674 DEBUG [AppViewModel.kt:3096] Starting scan from 'deeplink': 'redacted#e7…' - Reverse order. It passes after a re-run:
Queuing 'deeplink' sign-in link first(:3246), Authorize first, then Send. The first attempt kept the queue order right, but the restore stayed deferred withshared_state_busyfor the whole 60 s wait, so no sheet opened and Send followed. That is #1541, which #1543 fixes. This branch does not include it, and the two were not run together. - Authorize open, then
bitcoin:. The sheet stays, withSendNumberFieldabsent 5 s later, and Send opens after Cancel.
lightning: invoice (needs a usable channel), and the new journey file driven step by step.
CI: 15 checks green at 45abb40.
|
@ovitrif I am fine with sign-in first, then opening the queued payment after sign-in is approved or cancelled. The queued payment should retain its normal confirmation and QuickPay safeguards, including any QuickPay suppression already applied. |
@ben-kaufman it does: the queued payment goes through the normal scan path to confirmation once the sign-in sheet closes. Since 747075f a link queued across the lock keeps its QuickPay suppression, also when it is reopened and merged as a duplicate, and keeps its payment context. |
ben-kaufman
left a comment
ben-kaufman
left a comment
There was a problem hiding this comment.
Reviewed 45abb40 and the fixes for the existing link-ordering findings. No blocking findings. All 449 focused send-flow tests pass, including ordering, replacement, transition, and duplicate-context cases. Current hosted checks are green. Device journeys and combined integration with the other sign-in PR were not rerun as part of this review.
Device check on masterRan the Pubky sign-in journeys on
Recorded at 2x. |
4 participants
Closes #1542
Refs:
This PR keeps a pending Pubky sign-in link when another link opens Bitkit before the sign-in is approved or cancelled. The later link now opens after the sign-in instead of dropping it (device logs).
Description
Out of Scope
AppViewModel: two non-sign-in links opened while locked still keep only the latest, as before.Design
N/A — no UI changes.
Preview
N/A — no new UI; the sheets open in a different order.
QA Notes
Journeys
signin-with-second-link.xml— a payment link opened on the PIN screen in either order, or over the open Authorize sheet, opens only after the sign-in sheet closesManual Tests
N/A
Automated Checks
AppViewModelSendFlowTest.kt— a payment link waits until the open Authorize sheet closes; links opened on the PIN screen in either order open the sign-in first and the payment once; a newer payment replaces the queued one; a newer sign-in link wins over an older waiting one; a payment deferred by the lock and reopened over the Authorize sheet still skips QuickPayAppViewModelSendFlowTest.kt— a payment scan made while locked runs after the sign-in link waiting for the Pubky identity, instead of replacing it