fix: show profile recovery and deletion progress - #1394
ben-kaufman wants to merge 6 commits into
Conversation
|
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
jvsena42
left a comment
There was a problem hiding this comment.
Reviewed at 4a3b3cc (the latest push resolves both greptile threads).
Findings are LOW (inline). I also left two non-blocking suggestions on the delete path to cut its latency. Both are pre-existing and outside this diff; the code and tests are in the comments.
Non-blocking, pre-existing, outside this diff: createIdentity() still awaits loadProfile() + loadContacts() after the identity is created and _profile is already set, so Continue keeps spinning for those round trips before Pay Contacts opens. Launching them on the repo scope returns as soon as creation succeeds; failures there are already logged by the scope's handler:
val publicKey = result.getOrElse { return Result.failure(it) }
-return runSuspendCatching {
- if (_publicKey.value != publicKey) return@runSuspendCatching
+scope.launch {
+ if (_publicKey.value != publicKey) return@launch
loadProfile()
loadContacts()
}
+return Result.success(Unit)This changes two PubkyRepoTest expectations from 77239d258: "returns cache failure after profile loading" becomes success with the created profile kept, and "wipe completes while identity creation waits for contact loading" no longer asserts that creation is still pending.
Checked and clean: Back during delete is blocked by the modal; finally clears isSaving on failure and on cancellation; restore busy state resets in finally; the mutexes release via withLock; the keychain-failure default in identityExists fails closed, consistent with identity check fails closed when secure storage cannot be read.
The QA notes say delete-profile.xml is ported to iOS, but I found no open bitkit-ios PR carrying it.
Device gate: 4a3b3cc — journeys: 1 passed (delete-profile.xml, with one saved contact; progress modal shown, onboarding after ~15 s, profile stays deleted on reopen); Figma: Profile Edit compared, 1 delta; iOS: no twin found.
| } | ||
|
|
||
| if (uiState.isSaving) { | ||
| Dialog( |
There was a problem hiding this comment.
LOW (design): the spinner-only modal isn't in the Profile Edit frame.
Profile › Profile Edit on Handoff v62 has no saving/deleting state (the Delete section is only the Delete Profile button), and no other screen uses a bare progress Dialog, so this is a new pattern rather than the existing indicator. Could this stay within the frame's components, e.g. isLoading on the Delete Profile and Save PrimaryButtons (already supported, and it also disables them), with Back/Cancel disabled while isSaving? If design wants the modal, a linked frame settles it. delete-profile.xml step 5 would need its wording adjusted either way.
There was a problem hiding this comment.
Changed in 166d85a to the existing Save/Delete button loading states. Removed the bare modal and blocked Back, Cancel, drawer access and form edits while busy. The journey already asks for a visible progress indicator and blocked actions, so its wording remains valid. Added the mapped Profile Edit frame to the PR description.
There was a problem hiding this comment.
This stays within the frame. One refinement on my own wording: isSaving now drives isLoading on both buttons (ProfileEditForm.kt:241 and :291), and PrimaryButton draws its spinner whenever isLoading is set, even with enabled = false. On device, after Confirm on Delete Profile, the disabled Save button in the footer spins next to Delete for the whole ~19 s deletion. Disconnect from the failure dialog spins both with neither tapped, and Save with the form scrolled down spins Delete. iOS keeps isSaving and isDeleting separate in EditProfileView.swift, so only the tapped action shows progress there.
Non-blocking: keep one busy condition for the gating (fields, Cancel, Back, drawer), and pass which action is pending so each button gets isLoading only for its own action: isSaving for save(), isDeleting for attemptDeleteProfile() and disconnectProfile(). The EditProfileViewModelTest assertions on isSaving after deleteProfile() and in the disconnect tests would move to isDeleting.
There was a problem hiding this comment.
Save now spins only during saving, and Delete shows progress during deletion or Disconnect. Both paths still block edits, Cancel, Back and the drawer.
There was a problem hiding this comment.
Verdict: ♻️ Comment
Review: diff 17 files.
Parity with bitkit-ios master: diverges; open an iOS issue to track pending Profile taps and saved-identity recovery failures.
Findings:
1 inline (1 LOW)
QA:
Tests queued.
Reviewed by gpt-6-sol-xhigh 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 from merge base 483fd5a, at 4a3b3cc.
No new actionable code findings.
Home and the drawer wait for the saved-identity lookup before opening Profile, Profile intro, or Pubky Choice, and they drop that pending navigation when the origin screen has already changed. Profile keeps its spinner up through session restore and the profile fetch. Save, delete, and disconnect show a non-dismissible progress dialog and ignore a second action while that work is in progress. The earlier comments on dropped taps and a repeated profile fetch match the fixes in this revision (taps, fetch).
An open comment still matches this code: a saved session that cannot be imported, with no local secret key, stays on the empty Profile screen after Sign out fails to revoke it, and the next Profile open returns there (comment). Drawer Contacts still sends that signed-out saved identity to Pubky Choice (comment).
Validation: the new unit tests were read and not executed here. CI build, lint, and e2e passed on this revision. Device testing: not performed in this review. The added delete-profile.xml text matches bitkit-ios master. iOS profile-routing source was outside this pass.
Suggested additional test cases
- Android. Setup: Paykit enabled, keychain holds a session and no local secret key, and session import fails. Action: open Profile from the Home header, wait for the empty state, confirm Sign out while revocation fails, return Home, and open Profile again. Expected: the saved session is cleared, and the second open does not show that empty Profile.
|
Addressed the recovery, Contacts routing, progress UI and link-burst cleanup comments in 166d85a. All 3,035 unit tests, compile, lint and build pass. I left the optional creation-timing change out of this PR. Moving metadata loads into a background scope changes when createIdentity completes and reports errors, so that needs its own scoped change. The existing loading indicator stays visible through creation. The iOS journey parity point was already corrected above. |
There was a problem hiding this comment.
Advice: ✅ Approve
Reaudit: diff 16 files.
New findings: 1 inline (1 LOW); the rest is in the review.
Retest suggested: Tests 1, 3 (Test 1 now covers the Contacts routing change, and Test 3 covers the local disconnect change).
synonymdev/bitkit-ios#796 covers the matching deletion journey and Profile recovery. The saved-identity Contacts routing still needs an iOS follow-up.
QA:
Tests running: 0 of 4 passed.
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest (author)
| hasSeenProfileIntro -> navController.navigateTo(Routes.PubkyChoice) { popUpTo(Routes.Home) } | ||
| else -> navController.navigateTo(Routes.ProfileIntro) { popUpTo(Routes.Home) } | ||
| else -> scope.launch { | ||
| navController.navigateToProfile( |
There was a problem hiding this comment.
LOW: Saved-identity Contacts routing has no tracked iOS counterpart.
Contacts now sends a saved identity to Profile while authentication is pending or has failed. On iOS, ContactsIntroView and MainNavView still send users to onboarding, where they can create an identity during recovery. Neither synonymdev/bitkit-ios#796 nor an existing issue covers these routes. Could we open an iOS issue to apply saved-identity routing to both Contacts entry points?
There was a problem hiding this comment.
The iOS fix is open in synonymdev/bitkit-ios#864. It keeps saved identities on Profile recovery from both Contacts entry points and drops pending navigation if the user leaves.
jvsena42
left a comment
There was a problem hiding this comment.
Follow-up at 166d85a. Four of my five threads are resolved, and I withdrew the endpoint-sync one. One LOW refinement is open as a reply on the spinner thread: both Save and Delete spin during a deletion.
Checked and clean:
forgetUnrestoredIdentityruns the SDKforgetSessionAccess()before deleting keychain entries, so an SDK error leaves local state intact and surfaces as a toast. It re-checks_publicKeyunderinitializeMutex, so a Retry that recovers mid-tap keeps the identity.- Both disconnect bodies run under
NonCancellable, andfinallyresets the busy flag, so leaving the screen neither abandons cleanup nor leaves a spinner behind. - Partial failures: with a live session, a failed endpoint cleanup aborts before
signOut()and keeps the identity. A failed revocation keeps state and marks public cleanup pending. clearInitialLinkBurst()runs first inremovePublishedEndpointsForCleanup, so the burst cancel covers delete, both disconnect paths and wipe.- Contacts routing goes through
navigateToProfile, andidentityExistsfails closed totrue. The remainingRoutes.PubkyChoiceentries run after the identity is cleared or come from an explicit user choice.
Device gate: 166d85a — journeys: 1 passed (delete-profile.xml, 7/7, three runs with screen recordings; Back, system Back, Cancel, fields and Save stayed blocked while pending, and the top-bar Back shows a ripple but does not navigate); Contacts drawer manual check passed with and without a profile; Figma: Profile › Profile Edit, button loading states only, plus the double spinner above; iOS: not run on synonymdev/bitkit-ios#796.
There was a problem hiding this comment.
Advice: ✅ Approve
Reaudit: diff 5 files.
No new findings; the rest is in the review.
Retest suggested: Tests J1, 3 (The deletion journey checks progress and Save text; disconnecting on the edit screen shows the delete indicator.)
iOS already separates save and delete state and has the deletion journey in synonymdev/bitkit-ios#796. Android now shows separate indicators on each button; iOS uses a deletion overlay.
QA:
Tests queued.
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest (author)
Closes #1403
Refs: synonymdev/bitkit-ios#796
Description
Opening Profile or Contacts while a saved identity reconnects could offer identity creation or show an error before recovery finished. An unrestorable imported session could then get stuck because Disconnect required server access. Saving and deleting a profile also lacked visible progress.
Out of Scope
Contact import delays are handled separately. Creating a profile already shows loading; the reported missing creation indicator was not reproduced. No authentication protocol changes or background metadata-loading redesign.
Design
Profile › Profile Edit, Handoff v62. Uses existing button loading states.
Preview
Created and deleted a disposable profile on the Android emulator. The recording shows inline progress, disabled form controls, hidden drawer and return to onboarding. Reopening Profile stays on onboarding. Back/Cancel taps occurred after deletion completed, so their behavior during deletion remains a manual QA check. Recording and screenshots are saved locally for review.
QA Notes
Journeys
journeys/profile/delete-profile.xml— steps 1–4 and 6–7 passed on Android. Step 5 showed button progress and disabled controls, but Back/Cancel taps missed the pending window, so the full interaction check is still pending. Matching journey is already merged on iOS in LNURL-auth issue after migration (plunda.co) #796.Manual Tests
Automated Checks
PubkyRepoTest: restoration state, saved identity after failure, local disconnect and concurrent authentication.ProfileViewModelTest/EditProfileViewModelTest: loading, duplicate actions, cleanup failure and cancellation during disconnect.ContentViewTest: real navigation graph covers deferred/repeated taps, saved/absent identity, origin changes and Contacts intro stack cleanup.PrivatePaykitRepoTest: cleanup stops scheduled link publication before local state is cleared.SettingsViewModelTest: saved identity wiring.just compile, fulljust test(3,035 tests, no failures or skips),just lint, andjust buildpassed. Import-order corrections were checked by a final lint run. Remaining lint warnings are outside this change.Independent scoped review is complete. No existing tests were removed.