Skip to content

feat: support Pubky signup - #1224

Open
ben-kaufman wants to merge 19 commits into
masterfrom
codex/pubky-ring-signup
Open

feat: support Pubky signup#1224
ben-kaufman wants to merge 19 commits into
masterfrom
codex/pubky-ring-signup

Conversation

@ben-kaufman

@ben-kaufman ben-kaufman commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Description

  • Accept app-authorized pubkyring://signup and auth-bearing pubkyauth://signup requests, plus direct pubkyauth://direct_signup and parameter-only legacy pubkyauth://signup, through the normal scanner and deep-link flow.
  • Register the wallet-derived Pubky identity with the requested Homeserver and optional signup token.
  • Require explicit approval and the existing PIN/biometric check for every signup format; show the requested homeserver before registration.
  • Restore locally owned identities by signing in on their existing homeserver, keeping credentials retryable after failures instead of registering at Homegate.
  • Activate the returned Paykit session and continue into the existing profile setup flow.
  • Reuse the approval sheet’s loading state while signup completes, report Already signed in when a local identity exists, and keep payment-only scanner state intact when rejecting Pubky requests.

This PR is stacked on #1200 and uses its Paykit rc51 authorization model. Ordinary Pubky App sign-in must use that grant-auth model; compatibility with the older sign-in request is intentionally outside this signup PR.

Preview

Not included; this reuses the existing scanner, authorization approval sheet, loading treatment, and profile setup UI.

QA Notes

  1. With no Pubky identity, scan a signup QR from staging Pubky App; approve it, confirm the website continues, and complete profile setup.
  2. With no Pubky identity, scan either direct signup format; confirm the homeserver is visible, Cancel performs no registration, and Authorize requires local authentication before profile setup opens.
  3. Scan a signup request while signed in and confirm the Already signed in state.
  4. Confirm payment-only scanners reject Pubky requests without clearing the current payment flow.
  5. With the wallet locked, open a signup deep link; confirm nothing registers or opens until unlocking, then the approval sheet appears.
  6. After signup on a non-default homeserver, make session recovery fail, restore connectivity, and retry Create/Restore Profile. Confirm it keeps the same homeserver. Repeat with a failed profile save and confirm retry never calls signup.

Validation:

  • Full testDevDebugUnitTest: 2,346 tests passed on the restacked branch.
  • compileDevDebugKotlin and detekt passed.
  • Regression coverage includes locked signup deep links, consent/local-auth ordering, real SDK activation and partial-persistence cleanup, stale pending state, and one-time profile navigation.
  • PubkyRepoTest.kt: 97 tests passed, including existing-key recovery, secure-storage errors, sign-in/profile failure retries, cancellation, and no-key signup. Detekt reports no changed-file findings.

@ben-kaufman
ben-kaufman force-pushed the codex/pubky-ring-signup branch from d02c926 to 09dff52 Compare September 2, 2026 22:23
@greptile-apps

greptile-apps Bot commented Sep 2, 2026

Copy link
Copy Markdown

Greptile Summary

This PR adds Pubky Ring signup URL parsing and scanner routing, registers the wallet-derived identity with the requested Homeserver, activates Paykit, and resumes profile setup through durable local state.

  • Adds parsing and validation for pubkyring://signup requests.
  • Adds registration, requester authorization, session activation, and profile-setup continuation.
  • Preserves payment scanner state when Pubky requests are rejected and reports existing local identities.

Confidence Score: 3/5

This PR should not merge until signup activation can recover from the second network operation failing and pending profile setup can be exited without an immediate navigation loop.

The new signup sequence can complete remote registration and authorization while leaving Bitkit without a local session, and the successful path's durable pending marker makes the CreateProfile back action ineffective.

Files Needing Attention: app/src/main/java/to/bitkit/services/PaykitSdkService.kt, app/src/main/java/to/bitkit/repositories/PubkyRepo.kt, app/src/main/java/to/bitkit/ui/ContentView.kt

Important Files Changed

Filename Overview
app/src/main/java/to/bitkit/models/PubkyAuthRequest.kt Adds strict Ring signup parsing, query validation, and conversion into the existing Pubky authorization request model.
app/src/main/java/to/bitkit/repositories/PubkyRepo.kt Coordinates signup registration, authorization, activation, and pending profile state, but the multi-step flow can strand remotely completed signup without a local session.
app/src/main/java/to/bitkit/services/PaykitSdkService.kt Adds registration without activation, discarding an activatable signup result and requiring a failure-prone second sign-in.
app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt Routes Ring signup through the normal scanner while explicitly rejecting Pubky requests in payment-only contexts.
app/src/main/java/to/bitkit/ui/ContentView.kt Resumes pending profile setup automatically, but conflicts with the unchanged dismissible CreateProfile back action.
app/src/main/java/to/bitkit/ui/screens/profile/PubkyAuthApprovalViewModel.kt Dispatches Ring signup approval, handles existing identities, and dismisses the approval sheet before profile setup.

Sequence Diagram

sequenceDiagram
    participant Scanner
    participant Approval as Approval UI
    participant Repo as PubkyRepo
    participant Server as Homeserver
    participant App as Requesting app
    participant Paykit
    Scanner->>Approval: Pubky Ring signup request
    Approval->>Repo: Approve signup
    Repo->>Server: Register derived identity
    Repo->>App: Approve authorization
    Repo->>Paykit: Sign in and activate session
    Paykit-->>Repo: Active local session
    Repo-->>Approval: Dismiss approval
    Repo->>Repo: Mark profile setup pending
    Repo-->>Scanner: Navigate to profile setup
Loading

Reviews (1): Last reviewed commit: d02c926 | Re-trigger Greptile

Comment thread app/src/main/java/to/bitkit/services/PaykitSdkService.kt
Comment thread app/src/main/java/to/bitkit/ui/ContentView.kt
@ben-kaufman ben-kaufman changed the title feat: support Pubky Ring signup feat: support Pubky signup Sep 2, 2026
@piotr-iohk

piotr-iohk commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Regtest device QA, home Scan, QR from staging.pubky.app.

Staging e2e doesn’t finish — is that expected?
Signup QR does create a Bitkit identity and Create Profile (ring2 / pubky9m…). Staging stays on the Ring / QR step after Authorize. Sign-in QR from the same flow is rejected (only Pubky grant auth URLs are supported). So this isn’t e2e with the Pubky staging app — Bitkit gets a local profile, the website never continues. If approveRingAuth is supposed to complete that session, it’s a bug. If the site only listens for Ring, say so; Ben’s “QR → sheet → profile” case is then only the Bitkit half.

No spinner after scan.
Scanner closes, home looks unchanged for ~18s while register/activate runs. Feels like the scan did nothing, worse when staging also doesn’t move. “Deriving your keys…” on Create Profile is too late. Need a loader from scan until the profile screen.

Already signed in toast works and the scanner closes (iOS leaves it up — noted on #724).

bitkit_logs_2026-09-03_09-44-43-android.zip

Screen_Recording_20260903_113712_Bitkit.Regtest-android.mp4

@ben-kaufman

Copy link
Copy Markdown
Contributor Author

@piotr-iohk Thanks for the device QA and logs. You found a real signup interop bug: Bitkit treated every pubkyauth://signup request as direct signup, even when Pubky App included relay, secret, and caps. I pushed a fix so auth-bearing signup requests now register the identity and then complete the relay authorization; parameter-only/direct signup still skips app authorization. Direct signup also shows progress while registration and activation run.

The ordinary sign-in QR rejection is separate. These PRs use Paykit rc50’s app-scoped grant auth model, while staging Pubky App is still generating the older auth request format. Pubky App needs to update its sign-in flow to the new grant model for ordinary sign-in to work with Bitkit. Could you please recheck the staging signup path on this head?

@piotr-iohk

piotr-iohk commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Retested the rebased head on a physical Samsung S22 using the regtest build and a signup QR from staging.pubky.app.

Signup now completes end to end: authorization progress is visible, Bitkit creates the Pubky identity and opens profile setup, and the staging website continues successfully. The logs confirm the authorization completed and the local identity/session were created.

Ordinary sign-in still returns Authorization failed because staging currently generates the older non-grant authorization request. As clarified, that is outside the scope of this signup PR.

Non-blocking UI parity note: the approval screen differs between platforms. iOS (left) always shows the placeholder profile card, while Android (right) omits it because no profile exists yet. It would be good to align the intended design, but I don’t think this should block the signup fix.

Screenshot 2026-09-03 at 16 50 53

bitkit_logs_2026-09-03_14-48-43.zip

Screen_Recording_20260903_164721_Bitkit.Regtest.mp4

piotr-iohk
piotr-iohk previously approved these changes Sep 3, 2026
@ovitrif

ovitrif commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

needs conflict resolution @ben-kaufman

@ben-kaufman
ben-kaufman force-pushed the codex/pubky-ring-signup branch from 504ae04 to 94bc950 Compare September 3, 2026 19:03
ovitrif

This comment was marked as resolved.

@ben-kaufman
ben-kaufman force-pushed the codex/pubky-ring-signup branch from ea18746 to f331ad8 Compare September 3, 2026 22:16
@ben-kaufman
ben-kaufman requested a review from ovitrif September 3, 2026 22:18
@ben-kaufman
ben-kaufman force-pushed the codex/paykit-rc50-auth branch from 21d5a06 to a681dfc Compare September 4, 2026 12:24
@ben-kaufman
ben-kaufman force-pushed the codex/pubky-ring-signup branch from f331ad8 to ef49aa1 Compare September 4, 2026 13:19

Copy link
Copy Markdown
Contributor Author

I checked the failing pubky_paykit CI shard. It repeatedly fails the profile/contact-edit scenario with concatenated old and new text (ALICE WALLET AALICE WALLET A, Wallet A original notesEdited from wallet B), rather than failing Ring signup. The shared E2E helper calls clearValue() then setValue() without checking the clear took effect; the app's edit handlers replace the name/bio state directly and do not append it. This points to the Android input-clearing/automation path, but I have not reproduced the cause locally yet, so I am not calling this CI failure resolved or a proven harmless flake. The previously discussed signup-session preservation fix and its regression coverage are present; I resolved that answered review thread.

@ben-kaufman
ben-kaufman force-pushed the codex/pubky-ring-signup branch from ef49aa1 to ff24be9 Compare September 4, 2026 22:15

Copy link
Copy Markdown
Contributor Author

Restacked onto the updated #1200 head (580b042fd) and pushed as ff24be924. All five signup commits are signed; range-diff confirms their patches are unchanged. The only resulting tree changes are the parent PR's SDK-version/changelog update. The previously reported profile-edit E2E issue is not being marked fixed by this restack; the new head still needs CI verification.

ovitrif

This comment was marked as resolved.

ovitrif

This comment was marked as resolved.

@ben-kaufman
ben-kaufman force-pushed the codex/pubky-ring-signup branch from ff24be9 to f54f1f3 Compare September 6, 2026 15:04
@ben-kaufman
ben-kaufman force-pushed the codex/pubky-ring-signup branch from 73328c7 to 6df1707 Compare September 8, 2026 13:30

@ovi-reviewer ovi-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK, left a few inline nits.

Coverage

Total: 54%

  • Journeys: 83% - Five of the six QA journeys are directly exercised by scoped regression tests; website continuation is supported only by prior device QA
  • Unit tests: 80% - Nine changed behavior surfaces have focused tests; the approval-sheet composition and PubkyService delegates lack direct changed tests
  • QA: 0% - Manual Tests not run

Reviewed by Codex (gpt-5.6-sol-high) via gh-pr-review-loop skill

Comment thread app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt Outdated
Comment thread app/src/test/java/to/bitkit/ui/screens/profile/PubkyAuthApprovalViewModelTest.kt Outdated
@ovi-reviewer

This comment was marked as resolved.

@ovi-reviewer

This comment was marked as resolved.

@ovitrif
ovitrif dismissed stale reviews from ovi-reviewer[bot], ovi-reviewer[bot], and themself September 8, 2026 18:16

addressed - reaudit confirmed

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One LOW inline, diagnostics-only. Not blocking.

Gating: every entry point needs isPaykitEnabled (DataStore default false, written only from Dev Settings) and PaykitFeatureFlags.isUiAvailable. Nothing in .github or the justfile sets PAYKIT_UI_DISABLED, so the UI is available in a mainnet release build but off until the Dev Settings toggle. Both manifest aliases ship android:enabled="false" and are flipped only by PubkyAuthHandlerRegistrar. So: reachable only by someone who has opened Dev Settings and enabled Paykit.

Delta since my last pass: 0ff1a0129 merges master with no conflict-resolution edits, and 6df1707e1 "separate pubky signup and authorization routes" answers the thread on PubkyAuthHandlerRegistrar.kt:95. Over-correction check passed — the auth-alias predicate is now byte-identical to the pre-PR merge-base (isPaykitUiEnabled && hasIdentity && hasSecretKey), so session-only Ring users are advertised exactly as before, and the new signup alias claims only pubkyauth://signup, pubkyauth://direct_signup and pubkyring://signup. Nothing else was introduced. None of my threads were pending.

Checked and clean:

  • Manifest split. MainActivityPubkySignup is enabled="false", exported="true", with filter 1 = scheme pubkyauth x hosts {signup, direct_signup} and filter 2 = pubkyring+signup. Since Android merges <data> attributes per filter, that cross-product is exactly what's intended. PubkyAuthManifestTest resolves against the merged manifest via Robolectric in all three states and pins that with the auth alias enabled, pubkyauth://signup* goes to the auth alias (where handlePubkyAuth answers "Already signed in") and pubkyring://signup goes nowhere.
  • Key material. The identity is seed-derived, never generated. The hex secret is passed in-process to bootstrap().signUp and to bitkitcore approvePubkyAuth, and persisted only by persistSessionAccess into the AndroidKeyStore-backed Keychain. On logging: scanLogId hashes any non-SamRock QR, sanitizedDeeplinkLogValue drops query and fragment, and Logger.error renders only [Class='message']. Every new parse error message in PubkyAuthRequest.kt is value-free, and a URISyntaxException — whose message embeds the whole input — can't reach the signup path because isSignupUrl already requires URI(rawUrl) to succeed. One limitation worth stating: my local bitkit-core checkout predates the pubky module, so I could not read approvePubkyAuth's Rust error strings. That sink is shared with the pre-existing non-signup approveAuth path, so it isn't new exposure, but it is unverified rather than cleared.
  • Authorization / TOCTOU. Display state is built from parseAuthUrl(authUrl); approval re-parses the same immutable string with pure functions, so the homeserverPublicKey rendered on the sheet is the same field handed to registerIdentity. requestAuthorize pins state.authUrl == authUrl, transitionToAuthorizing and inFlightAuthorization are both CAS, and PubkyAuthorizationLocalAuth confirms only the pendingAuthUrl it was asked to authenticate.
  • Caps shown equal caps signed. Kotlin parseCapabilities drops only segments pubky-common's Capability::try_from also rejects, and any non-r/w action makes Rust reject the whole capability — so displayed is always a superset of granted. validateSignupRequest runs bitkitcore's parser on the constructed URL first.
  • Trust boundary. hs is format-checked only, with no allowlist — you declined that after my earlier thread and mitigated by rendering the homeserver plus the trust warning, so I'm not reopening it. relay/secret are never rendered and only reach the network via the constructed pubkyauth:///?relay=… handed to bitkitcore, which is pre-existing for ordinary auth URLs. x-bitkit-claim on a signup is rejected, and pubkyauth://signup?hs=X with a relay but no secret fails requiredSingle("secret") rather than silently downgrading to direct signup.
  • Manifest exposure. Both aliases are exported, so any installed app can fire the intent while an alias is enabled — but the most it gets is the sheet. processDeeplink requires Paykit on and a wallet to exist, routes through launchScan(allowPubkyAuth = true) which defers while locked, and approveSignupAuth is reachable only via Authorize -> RequestLocalAuth -> PIN/biometric (or the tap alone when neither is configured, consistent with the rest of the app). Both are pinned by tests.
  • Lifecycle and partial failure. approveRingAuth failing writes nothing locally and the remote account is retried via rc51's 409 path; activation failure runs clearRegisteredIdentityActivationLocked under NonCancellable; a failed pending-flag write runs forgetSessionAccess + clearLocalState; process death between activation and the pending write leaves the session restorable on next launch with the profile creatable from the Profile tab. createIdentity with a stored key goes straight to signIn, never Homegate.
  • Cancellation. The ViewModel is hiltViewModel() in the sheet host, a sibling of RootNavHost under setContent, so it's Activity-scoped — dismissing the sheet cannot cancel approveSignupAuth. All rollbacks are NonCancellable. _effects is a SharedFlow with extraBufferCapacity = 1, so a Dismiss with no collector is dropped, which is harmless because CreateProfile navigation is driven by the persisted pending flag rather than the effect.
  • Cross-identity. hasIdentity() checks in-memory key plus both keychain entries, so a session-only Ring identity blocks a second seed-derived signup. activateBootstrapResult deletes PAYKIT_SDK_STATE when the key changes, and sharesPrivatePaykitEndpoints is reset before registration.
  • Scanner containment. Contact-payment, shop and webview scans pass allowPubkyAuth = false and are rejected before touching payment state.
  • Migration limit clause. The createIdentity restructure moved shouldRevokeSessionOnFailure = true from the shared path into the Homegate branch only. I checked what else that assignment carried — nothing but revoke-on-failure for a freshly created Homegate session, which is exactly what it was meant to confine. No migration finding.

One observation, not a finding: the registrar keys on the in-memory _publicKey while the runtime check uses the keychain, so after a RestorationFailed the signup alias is advertised and the app answers "Already signed in". The outcome is correct and it predates this delta.

Cross-repo: the alias split is Android-only by construction — iOS registers URL schemes statically in Info.plist and can't runtime-gate them, so the "advertised for links it can't handle" concern is structural there rather than a bug. Worth a note on synonymdev/bitkit-ios#722 rather than a fix. The LOW below should also be checked against synonymdev/bitkit-ios#724 approval-failure handler, which I haven't re-reviewed this round.

@ben-kaufman

Copy link
Copy Markdown
Contributor Author

This reuses the existing authorization sheet and Create Profile screen, with signup details and a homeserver card added to the sheet. ContentView adds navigation after signup. I could not find a Figma frame linked in the PR or repository, so I do not have a verified design reference to add.

@ovi-reviewer ovi-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

Coverage

Total: 67%

  • Journeys: 100% - The changed behavior has a direct regression for rejection followed by a later incoming request
  • Unit tests: 100% - Both prior findings are covered by focused unit-test changes in the scoped delta
  • QA: 0% - Manual Tests not run

Reviewed by Codex (gpt-5.6-sol-high) via gh-pr-review-loop skill

@ovi-reviewer

ovi-reviewer Bot commented Sep 8, 2026

Copy link
Copy Markdown

ben-kaufman: This reuses the existing authorization sheet and Create Profile screen, with signup details and a homeserver card added to the sheet. ContentView adds… (comment)

Confirmed: no Figma source exists for these reused screens, and this delta changes no UI layout, copy, or resource. The design-reference note is settled.

@ovi-reviewer ovi-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK, left a few inline nits.

Coverage

Total: 0%

  • Journeys: 0% - The diagnostics-only superseded-request path is not exercised by a test in this delta
  • Unit tests: 0% - No unit test observes the relocated error log for a superseded approval failure
  • QA: 0% - Manual Tests not run

Reviewed by Codex (gpt-5.6-sol-high) via gh-pr-review-loop skill

ovi-reviewer[bot]

This comment was marked as resolved.

ovitrif

This comment was marked as resolved.

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The consent bypass I raised last round is fixed. Verified independently at head rather than taking the diff's word for it:

  • handleDirectPubkySignup, isDirectSignupUrl and isCompletingPubkySignup no longer exist anywhere in app/src/main.
  • approveSignupAuth has exactly one production caller — PubkyAuthApprovalViewModel:189 inside approveRequest, reachable only from authorize():173confirmAuthorize():159. registerIdentity, approveRingAuth and activateRegisteredIdentity live inside it with no other callers. There is no remaining path from a scanned, pasted or deeplinked URL to registration that skips the sheet or local auth.
  • The wallet-exists guard is now inside the isProtocolUrl branch (AppViewModel.kt:4668-4677), so the old ordering bug is gone, and launchScan:1929-1932 enqueues while !_isAuthenticated with flushDeferredScan:2048 refusing until authenticated — the PIN bypass is closed.
  • handlePubkyAuth:4696-4717 has no approval branch left; pubkyauth://signup?hs= with no relay/secret/caps now takes the same sheet path, since authorizesApp only affects parsing.
  • The consent chain is pinned end to end: requestAuthorize CASes on state.authUrl == authUrl && state == Authorize, local auth routes through AuthCheckView/BiometricsView, confirmAuthorize CASes on inFlightAuthorization, and authorize() re-parses the same immutable URL. PubkyAuthApprovalViewModelTest:193-229 runs all three formats and asserts never { approveSignupAuth } before consent and after cancelling local auth.
  • homeserverPublicKey is rendered (PubkyAuthApprovalSheet.kt:411-418, testTag("PubkySignupHomeserver")).
  • The signup hosts moved to a MainActivityPubkySignup alias with android:enabled="false", flipped only by PubkyAuthHandlerRegistrar.kt:67 (isPaykitUiEnabled && !hasIdentity).

One new LOW inline, dev/QA-facing.

if (!approveRequest(request, authUrl)) return

Logger.info("Auth approved for '${request.serviceNames.firstOrNull().orEmpty()}'", context = TAG)
if (request.isSignup) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Low, and dev/QA-facing today (isPaykitEnabled defaults false) — but this early return leaves the VM parked in Authorizing for that URL.

The ordinary path two lines below sets ApprovalState.Success; the signup branch returns without touching _uiState, so it stays {authUrl = url, state = Authorizing} from transitionToAuthorizing:255-265. The PubkyAlreadySignedInError exit at :304 does the same, returning before the _uiState.update { Authorize } at :307.

That matters because resetForLoad:267-278 returns false when the URL matches and the state is Authenticating/Authorizing, so load:63 returns without parsing. inFlightAuthorization was already cleared in the finally at :161, so the in-flight restore at :55-62 doesn't apply either. Result: AuthorizingContent:360-374 renders a buttonless spinner. Not a lockout — back still dismisses via approvalBackAction:264dismiss():321-323 — but dismiss() never resets state, so every re-open of that URL is the same spinner for the life of the Activity. The VM is Activity-scoped (bare hiltViewModel() in ContentView.kt:571-575, a sibling of RootNavHost), so it survives sheet close/reopen and rotation.

Reachable because direct-signup URLs carry no nonce: parseSignup only requires hs, and relay/secret/caps are needed only when authorizesApp is true. So pubkyauth://direct_signup?hs=<key> from a printed or static QR is byte-identical on every scan — the VM's own test at :194-200 uses exactly those static strings. The gate re-opens after Sign out (ProfileScreen.kt:82clearLocalState:1397-1404 clears both keychain keys, so hasIdentity() is false and the already-signed-in toast no longer intercepts). pubkyring://signup is unaffected — it carries a per-request secret.

Settle the state before dismissing on both paths. Emit Dismiss first and update after, so the signup path doesn't flash SuccessContent for a frame on its way into Create Profile. Alternatively — and this covers any future terminal path — scope resetForLoad's Authorizing refusal to a genuinely live authorization: inFlightAuthorization.get()?.authUrl == authUrl. load:55-62 already handles the truly in-flight case.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed the reload guard so a completed signup URL can return to consent instead of reopening the spinner. Local authentication and an authorization that is still in flight remain protected from duplicate loads.

@ovitrif ovitrif added this to the 2.6.0 milestone Sep 9, 2026
ovitrif
ovitrif previously approved these changes Sep 9, 2026

@ovitrif ovitrif left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

manual review of delta, re-approved

@ovi-reviewer ovi-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QA Notes

QA_1 and QA_2 Pixel 10 Pro emulators on Android 16/API 36; exact-head dev build 2.4.1 (188).

  1. passed: Scanning the live staging Pubky App QR registered the identity, advanced the website to profile setup, and completed the Bitkit profile.

    Test 1:
    With no Pubky identity, scan a signup QR from staging Pubky App; approve it, confirm the website continues,…
    1.mp4
  2. passed: Scanning a direct-signup QR showed the homeserver; Cancel left the wallet unsigned in, and Authorize required the local PIN before profile setup.

    Test 2:
    With no Pubky identity, scan either direct signup format; confirm the homeserver is visible, Cancel performs…
    2.mp4
  3. passed: A signup request while signed in showed “Already signed in” and preserved the existing profile.

    Test 3:
    Scan a signup request while signed in and confirm the Already signed in state.
    3.mp4
  4. passed: The payment-only scanner rejected the Pubky QR and retained the Bitcoin send flow.

    Test 4:
    Confirm payment-only scanners reject Pubky requests without clearing the current payment flow.
    4.mp4
  5. passed: A locked wallet queued the signup deep link until unlock, then showed the approval sheet without prior registration.

    Test 5:
    With the wallet locked, open a signup deep link; confirm nothing registers or opens until unlocking, then the…
    5.mp4
  6. ⛔️ failed: Offline cold-start session recovery reproducibly terminated Bitkit with a NullPointerException in AppViewModel.toast at AppViewModel.kt:4519. Restoring connectivity recovered the same Pubky and non-default homeserver, and the profile-save retry completed without another signup call.

    Test 6:
    After signup on a non-default homeserver, make session recovery fail, restore connectivity, and retry…
    6a.mp4
    6b.mp4

Tip

The app must remain running after a recovery failure so the user can restore connectivity and retry.

Coverage

Total: 94% (delta since 3dac138, 5 files)

  • Journeys: 100% - PubkyAuthApprovalViewModelTest.kt covers terminal success, already-signed-in dismissal, duplicate local authentication, and active authorization reload.
  • Unit tests: 100% - The changed reload predicate has focused assertions for both terminal branches and its active-operation guard.
  • QA: 83% - 5 of 6 Manual Tests passed on Pixel 10 Pro Android 16 emulators.

Reviewed by Codex (gpt-5.6-sol-high) via gh-pr-review-loop skill

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

15aca5075 closes it. You took the second option — scoping the Authorizing refusal to a live inFlightAuthorization rather than settling the state on the terminal paths — and it works for both branches at once, since the guard keys on the in-flight record instead of on which branch was taken.

Trace: after either terminal path returns, the finally at :161 CASes inFlightAuthorization back to null. Re-opening the same URL then hits load:55 (in-flight restore skipped) → resetForLoad:63 → state is Authorizing but inFlightAuthorization.get()?.authUrl == authUrl is null == authUrl, so isAuthorizing is false → CAS to {authUrl, Loading} at :278parseAuthUrlAuthorize with real buttons. No SuccessContent flash either, since neither path touches _uiState.

No new door opened: resetForLoad(A) is unreachable while A is genuinely in flight because load:55-62 returns early on the same condition, and a second approveSignupAuth is independently blocked by the untouched inFlightAuthorization.compareAndSet(null, …) at :149 plus initializeMutex and the hasIdentity() re-check in PubkyRepo.approveSignupAuth:1006-1013. A UI-state reset can't bypass those.

terminal signup outcomes allow the same URL to reload for consent (PubkyAuthApprovalViewModelTest.kt:240-270) is a real regression test — revert the :270-271/:274 hunk and it fails on both the assertEquals(Authorize, …) at :261 and the parseAuthUrl verification at :262, and it loops both Result.success and PubkyAlreadySignedInError.

One sibling case inline.

if (
currentState.authUrl == authUrl &&
currentState.state in setOf(ApprovalState.Authenticating, ApprovalState.Authorizing)
(currentState.state == ApprovalState.Authenticating || isAuthorizing)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Low, dev/QA-facing — the Authorizing half is fixed, but Authenticating can strand a URL the same way, via a dismissal the VM never hears about.

With PIN enabled: Authorize → requestAuthorize:104-111 sets AuthenticatingAuthCheckView overlays inside the sheet's Box (PubkyAuthApprovalSheet.kt:80-98, :149-164). If the user then taps the scrim, swipes down, or presses system back, none of PubkyAuthApprovalSheet.kt, AuthCheckView.kt or BiometricsView.kt declares a BackHandler or DisposableEffect, so the event goes straight to SheetHost.kt:181-195onDismissContentView.kt:499 appViewModel.hideSheet(), bypassing the VM entirely. cancelLocalAuth is only wired to AuthCheckView.onBack / BiometricsView.onFailure (:160, :175), neither of which fires on host-driven dismissal.

So the VM stays {authUrl, Authenticating} with no pending prompt, and unlike the Authorizing branch there is no in-flight record to invalidate it — this guard returns false on the flag alone. Re-scanning the same static direct-signup URL renders AuthorizingContent: spinner, no PIN pad, no buttons, for the life of the Activity. Same symptom you just fixed, reached from the sibling state. Biometrics is mostly immune, since the system prompt's cancel drives onFailurecancelLocalAuth.

The new test at :217-222 pins the Authenticating refusal as intended but doesn't cover host-driven dismissal, so it passes either way.

Smallest fix keeps the guard and releases the state when the overlay goes away — in PubkyAuthorizationLocalAuth:

DisposableEffect(Unit) { onDispose { pendingAuthUrl?.let(viewModel::cancelLocalAuth) } }

Alternatively collapse AuthenticatingAuthorize from the sheet host's onDismiss.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dismissing the approval sheet now clears its pending authentication state. Reopening the same URL returns to consent and requires local authentication again.

@ben-kaufman

Copy link
Copy Markdown
Contributor Author

Moved toast initialization before the startup collectors, so an immediate session-recovery error can show its toast without crashing the app.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants