Repository navigation
fix: keep pubky sign-in links working on a cold start - #1514
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 changes affect Android's Pubky sign-in flow and shared toast display. The PR appears safe to merge, with a non-blocking notification-order issue worth fixing. Findings
SummaryKeeps Pubky sign-in links working while a saved session restores.
One non-blocking issue remains: a second notification can get stuck behind the loading toast. Acknowledged scope: Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Open sign-in link] --> B{Saved session ready?}
B -->|No| C[Show loading toast]
C --> D[Wait for initialization and retry restore]
D -->|Unavailable| E[Wait 3 seconds and for network]
E --> D
D -->|60-second limit| F[Hide loading toast and show profile error]
D -->|Ready| G[Check restored identity key]
B -->|Yes| G
G -->|Key available| H[Open Authorize sheet]
G -->|No key| I[Show Ring guidance]
H --> J[Hide loading toast]
Reviews (1) · Last reviewed commit: "fix: bring the sign-in wait toast back a..." · Reviewed by Greptile |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8cc76f2415
ℹ️ 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".
|
Pushed f8bd4ec: a loading toast already waiting in the queue now moves behind each new toast, so a second toast arriving during the profile wait shows before the loader returns, instead of waiting until the wait ends. Answers the Greptile and Codex findings on |
|
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
talosmachina
left a comment
There was a problem hiding this comment.
No findings. Keeps the Pubky sign-in handler enabled while a saved session with a Bitkit-held key is still restoring, retries the restore for up to 60 s while the link waits, and shows a loading toast that survives interrupting toasts. Reviewed f8bd4ec, full tier.
What I checked, and 6 candidates I ruled out
Read: PubkyAuthHandlerRegistrar.kt, ToastQueueManager.kt, Toast.kt, the ToastView.kt hunk, and in AppViewModel.kt the scan scheduling job, awaitPubkyDeeplinkInitialization, the new wait-toast helpers, handlePubkyAuth and showPubkyIdentityUnavailableToast; in PubkyRepo.kt awaitIdentityReady, restoreSession, restoreSessionLocked, initializeSession, hasSecretKey, managedSecretKeyFor and the new hasLocalSecretKey.
CI: build, lint, detekt, build-local green; Run tests (testDevDebugUnitTest) green on this head; the e2e shards and e2e-status were still running when this was posted.
Not run here: the unit tests could not be run on this machine (the new ldk-node-android and bitkit-core-android artifacts were not in the offline Gradle cache), so CI's testDevDebugUnitTest is the evidence. The cold-start behaviour itself needs a device: the new journey and the manual network-cut test.
Ruled out
- Early-enabled handler lets a Ring identity approve:
hasLocalSecretKey()excludesSHARED_PUBKY_SOURCE, andhandlePubkyAuthstill requires a restoredpublicKeyplushasSecretKey()beforeSheet.PubkyAuthopens. - Wait toast left on screen:
hidePubkyAuthWaitToast()sits in the scan job'sfinally, so cancellation by a newer scan, the locked-app deferral and the timeout path all clear it; removal is by reference, so a toast that replaced it stays. - Wait toast evicted from a full queue:
enqueueputs loading toasts after every other toast beforetakeLast(MAX_QUEUE_SIZE), so overflow drops the oldest non-loading toast first. - Retry loop spinning without the network: each
Unavailablewaits 3 s and then forConnectivityState.CONNECTED, all inside the 60 swithTimeoutOrNull;Missingends the loop and falls through to the no-identity toast. - Grant signup with no identity waits 60 s:
awaitIdentityReady()returnsMissingwhenhasIdentity()is false, so the loop exits at once andcreatesIdentityopens the sheet. - Contact links affected: they keep
PubkyService.AUTHORIZATION_TIMEOUT, skip the wait toast and the retry loop, and keep their own error toast.
Merge confidence: 4/5, no findings and the change is unit-tested in CI, but the required e2e-status check had not finished and the cold-start path is proven on a device only by the journey and the manual test.
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Full review of the PR diff against its merge base, at f8bd4ecd.
No new actionable code findings.
A cold-start pubkyauth://signin_grant link keeps the auth handler enabled while a Bitkit-held secret key exists and the saved session is not restored yet, retries that restore for up to 60 seconds while the device is online, and shows a loading toast until the Authorize sheet opens or the wait fails. Approval still requires the restored identity's own key. A Ring-sourced identity does not keep the handler on. Contact links keep the previous 30-second timeout and their own error toast.
The earlier report that a second toast stays stuck behind the loader is fixed at this revision: a queued loading toast is placed behind each new toast (thread). Compared bitkit-ios eb3156ab: iOS does not register the raw pubkyauth scheme and opens sign-in through bitkit://, so this activity-alias failure has no iOS twin. During an offline wait, the existing "Your profile session has expired" toast can still appear; the loader is expected to return after it.
Validation: CI Run tests (testDevDebugUnitTest) passed on this revision. Those tests were not run locally. Device testing: not performed in this review.
Ready for device testing.
Suggested additional test cases
- Android, dev build, Paykit on, Pubky profile created in Bitkit. Force-stop, open a fresh
pubkyauth://signin_grantlink, and keep the device offline for more than 60 seconds. Bitkit stays in the foreground, the loading toast is replaced by "Couldn't Load Your Pubky Profile", and a later sign-in link still opens Bitkit. - Android, same build with a Ring-sourced identity and no Bitkit-held key. Force-stop and open a
pubkyauth://signin_grantlink. Bitkit does not present the Authorize sheet for that identity.
jvsena42
left a comment
There was a problem hiding this comment.
Code scan at f8bd4ec: one LOW inline. I found no HIGH or MEDIUM.
I reviewed it as an auth and deep-link change.
Checked and clean:
- The authorization gate is unchanged.
handlePubkyAuth(AppViewModel.kt:7652-7665) still requires a restoredpublicKeyand a matching managed secret key beforeSheet.PubkyAuth. Enabling the alias early only routes the link. - Registrar (
PubkyAuthHandlerRegistrar.kt:66,78-83): before restore it requiresPUBKY_SECRET_KEYpresent andSHARED_PUBKY_SOURCEabsent.adoptRingIdentitywrites the source first.signOut,wipeLocalStateandforgetUnrestoredIdentitygo throughclearLocalState. Keychain errors leave the alias disabled. - Lock screen:
_isAuthenticatedstarts false, so on a cold start the link waits asmustCompleteuntil unlock. The wait toast and the sheet never show over the PIN screen. A lock during the wait defers after it, andfinallyhides the toast. - Link replacement: a duplicate intent is skipped. A different link during a
mustCompletewait goes to the single deferred slot and runs after the sheet closes. Approval uses the displayedauthUrl, so there is no new TOCTOU. - Wait paths: newer scan, timeout, lock,
return falseand exceptions all pass throughfinally { hidePubkyAuthWaitToast() }, anddismissmatches by reference. The 60 s timeout shows the error toast and opens no sheet. ToastQueueManager: the loader is always placed last andtakeLast(5)drops the oldest non-loading toast first. Toast text comes from string resources only.- No persisted-state change, so no v2.5.1 angle.
- l10n: two new keys (
pubky_auth__loading_identity,_desc), English only per the Transifex flow.
Performance-relevant (user-facing wait raised from 30 s to 60 s, a restore retry loop, scanMutex held during the wait).
Device gate (partial): f8bd4ec — not run yet. Pending:
signin-cold-start.xml;- a 25 s and a >60 s network cut;
- a Ring identity;
- a payment QR during the wait, which must open only after the sign-in sheet closes;
- a PIN lock during the wait;
- time from
am startto the Authorize sheet, and the count of restore retries.
The device result will follow.
|
Pushed d790f34: the cold-start sign-in journey now names its iOS counterpart, bitkit-ios |
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Follow-up review of the changes since f8bd4ecd, at d790f344, inheriting the completed baseline review. Inherited coverage is unrestored-session handler gating for a Bitkit-held key versus a Ring source, the cold-start online retry, 60-second timeout and profile-unavailable error, loading-toast show, reference dismiss and requeue, unchanged contact-link and non-grant signup behavior, and the unit tests plus the cold-start journey actions as the unexecuted runtime plan. This pass covered the documentation delta and the iOS journey it names.
No new actionable code findings.
The only change since the baseline is documentation. The cross-platform row, journeys/pubky-auth/README.md, and the signin-cold-start.xml description now point at bitkit-ios pubky-auth/open-signin-link.xml (bitkit://pubky-auth/signin) as an adapted counterpart, and list the raw pubkyauth scheme, the "Loading Your Pubky Profile" toast, and Authorize through PubkyAuthOK as Android-only. That matches the requested wording in the review thread. Compared with bitkit-ios afdc534b, where that journey file was already present at eb3156ab: iOS registers bitkit for this handoff, and open-signin-link.xml launches a terminated app with a local bitkit://pubky-auth/signin grant, shows the Authorize sheet, and cancels. It does not include the loading toast or PubkyAuthOK. Its description still says Android needs no twin. Android journey actions are unchanged.
Validation: no tests were executed in this review. The delta does not change app code. Unit-test evidence for the unchanged code remains the baseline run CI Run tests on f8bd4ec. Device testing: not performed in this review.
Ready for device testing.
jvsena42
left a comment
There was a problem hiding this comment.
Approving at d790f34. My journey-parity LOW is fixed in d790f34 and confirmed on its thread. No HIGH or MEDIUM.
Device gate: d790f34 — on the CI APK with a Bitkit-held identity, against a requester built on @synonymdev/pubky 0.14.0 with a fresh signin_grant URL each run.
signin-cold-start.xmlpasses: loading toast with spinner until the sheet, the sheet showsgate.testwith/pub/gate.testREAD/WRITE, Authorize reachesPubkyAuthOK. The requester received{"status":"approved","pubky":"pubky3bx9…4fe9ujo","caps":["/pub/gate.test/:rw"]}.- 25 s airplane cut: Bitkit stays open and the loading toast returns after "Internet Connection Restored". One
Retrying Pubky session restoreline, and the sheet opens ~16 s after reconnect. -
60 s cut: "Couldn't Load Your Pubky Profile" at ~+62 s with no sheet and Bitkit on Home. A later link opens the sheet.
- PIN on: the PIN screen shows first, and 9 dumps over 27 s show no toast and no sheet over it. The sheet opens within 3 s of unlock, and Authorize asks for the PIN.
- Payment URI during the wait: it is queued (
Queuing 'deeplink' scan for deferred handling) and opens Bitcoin Amount 1 000 sats only after the sign-in sheet is cancelled. - No crash or ANR.
perf: am start → sheet 15.6 s online; 39–43.5 s with a 15–25 s network cut; 0–1 retry lines per run (3 in the failed run below). l10n: two new English-only keys in the toast, no overflow.
The first cold start right after profile creation reached the 60 s limit and showed "Couldn't Load Your Pubky Profile". A re-run passed. Details are in a separate comment, since the cause is outside this PR.
CI green at d790f34.
|
Outside this PR's scope, no change needed here. On a cold start, Paykit shared-state contention can outlast the 60 s sign-in wait. In 2 of 8 cold starts during the gate, the session restore was deferred with The identity republish and the public endpoint refresh on startup contend with the restore for the SDK's shared-state lock. The restore succeeded 6 s after the wait expired. The same pattern appeared in a later PIN run, where the restore took 65 s. On iOS, a second device restored from the same seed also hit Reproduction: create a profile, then Two pre-existing deferral behaviours seen in the same runs:
Tracked in #1541 (sign-in wait vs shared-state lock) and #1545 (a later link drops a pending sign-in). |
Closes #1513
This PR fixes Pubky sign-in links opened while Bitkit is not running. It fixes a 3.0.0 blocker, for RC3: when the Pubky session restore failed on a cold start, Bitkit closed itself, dropped the sign-in request, and later sign-in links did nothing until Bitkit was opened by hand (repro).
Description
Out of Scope
PubkyRepo: the "Your profile session has expired" toast that a restore failing while offline shows during the wait; it comes from the existing restore failure handling.PubkyRepo: the time the session restore takes on a cold start (12-19 s in the repro); this PR makes the wait visible and survivable, not shorter.pubkyauthscheme for OS link delivery.Design
N/A — no design available.
Preview
cold5-2x.mp4
netcut2-2x.mp4
Recorded at 2x on an Android 16 emulator against staging, with a Pubky profile created in Bitkit.
QA Notes
Journeys
signin-cold-start.xml— a cold-start sign-in link shows the loading toast until the Authorize sheet opensManual Tests
Automated Checks
ToastQueueManagerTest.kt— a loading toast is dismissed by reference, never another toast, and returns after one or several toasts that interrupt itPubkyAuthHandlerRegistrarTest.kt— a failed restore keeps the handler enabled for a stored local key, and still disables it without onePubkyRepoTest.kt— a stored key counts as local only without a Ring sourceAppViewModelSendFlowTest.kt— a cold sign-in link shows and clears the loading toast, retries a deferred restore until it succeeds, and shows the profile error after the wait times out