Repository navigation
fix: keep contact previews independent of Paykit - #1683
ben-kaufman wants to merge 2 commits into
Conversation
|
Triage: 3.1.0. It fixes shipped contact previews, and 3.1.0 is due 28 Oct. |
[Medium impact] The PR appears safe to merge; no actionable issues were found. SummaryShows the fetched contact profile and enables Save before optional payment discovery finishes.
Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Fetch profile] --> B[Show profile and enable Save]
B --> C[Discover public payment endpoint]
B --> D[Save contact]
C --> E{Read still active?}
E -->|Yes, endpoint confirmed| F[Show Pay]
E -->|No| G[Ignore result]
D --> H[Block repeated Save calls]
H --> I[Write contact and schedule link preparation]
I --> J[Open saved contact]
Reviews (1) · Last reviewed commit: "fix: keep contact previews independent o..." · Reviewed by Greptile |
talosmachina
left a comment
There was a problem hiding this comment.
No findings. Shows the fetched contact profile and enables Save before the optional public payment-endpoint lookup finishes, with stale-result and duplicate-save guards in AddContactViewModel. Reviewed 7e56c3c, full tier, reasoned from the code and CI (this branch was not built here).
What I checked, and 5 candidates I ruled out
Read in full: AddContactViewModel.kt, the AddContactScreen.kt state switch and LoadedContent, the new tests in AddContactViewModelTest.kt
Call sites traced: fetchProfile (init and onRetry), saveContact (onSave), hasPublicPaymentEndpoint consumers
CI: detect-changes and e2e determine green; build, build-local and lint still pending at review time
Ruled out
- Endpoint result clearing an in-flight save: the late update copies only
hasPublicPaymentEndpoint, soisLoading = trueset bysaveContactsurvives; covered byendpoint completion cannot clear an in flight save. - Retry cancelling a running save:
onRetryis wired only fromErrorContent, which renders only whilefetchedProfile == null, and Save needs a profile, so the two cannot overlap. ensureActive()inside theonSuccesslambda:Result.onSuccessis inline, so the suspend call runs in the launched coroutine and throws on a cancelledprofileJobas intended.- Duplicate save: the
isLoadingcheck and the state update now run synchronously on the main thread beforelaunch, so a second tap returns early. - New
isLoadinggate blocking Save during the first fetch:fetchedProfileis null then, so Save was already unreachable.
Merge confidence: 4/5, CI build, unit tests and lint were still pending when this was read and the branch was not built here; the change itself is covered by the new view-model tests.
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
jvsena42
left a comment
There was a problem hiding this comment.
Code scan: no findings.
Checked and clean:
- The preview can't be stale or belong to another identity:
profileJobis cancelled on a new key, andensureActiveruns before each write (AddContactViewModel.kt:81/89/93). - No payment path reads the preview:
payContactre-fetches endpoints throughbeginPayment, so Pay still depends on a confirmed endpoint. - The post-save link helpers only schedule work, as the PR body says; Save doesn't await them.
Performance-relevant (removes a wait on Add Contact). The gate times key entered → Save shown against → Pay shown, head vs master, and counts endpoint lookups per screen open (expected 1).
Device gate (partial): 7e56c3c — not run yet; pending that measurement. contacts/save-during-endpoint-discovery.xml itself needs a homeserver fault fixture to hold the endpoint lookup, so it will be reported unrun, as its description says. Device results will follow in a separate review.
There was a problem hiding this comment.
Approving at 7e56c3c. My code scan stands with no findings, and the device gate found none.
Device gate: 7e56c3c (base dc5a47d) — Pixel_Tablet, staging Pubky, a readable peer with public endpoints. perf: key entered → Save shown 2.8 s warm / 3.1 s cold, the same as base (2.8 s / 3.1 s); no ANR. Three Save taps within ~100 ms produce one Added contact and open the contact once. Figma: no design; l10n: no UI text changed.
- On staging the endpoint lookup finishes in the same frame as the profile, so both builds show Save and Pay together. One base sample took 8.8 s for both buttons, which is the wait this PR removes, but the slow lookup doesn't reproduce reliably on staging, so the improvement isn't measured here.
contacts/save-during-endpoint-discovery.xml and the perf improvement need the endpoint lookup delayed on its own. A local proxy that holds only the endpoint reads now exists: it drove this journey on the iOS twin synonymdev/bitkit-ios#1119, where Save showed in ~3 s against ~18 s on base. The Android run follows next pass. Endpoint lookups per open can't be counted from Android logs (publicRead logs nothing); the proxy log can count them.
Skipped — covered by CI at 7e56c3c: unit tests, lint, detekt, e2e (all green).
There was a problem hiding this comment.
Verdict: ✅ Approve
Findings: None.
QA for 7e56c3c:
Pixel 7 emulator, Android 15, disposable staging identities, committed dependencies and an unmodified app build.
Test 1: J1 — Save during endpoint discovery ✅ The profile and Save appeared with Pay absent; Save opened Contact while endpoint discovery remained held.
A TLS passthrough relay selected the peer's endpoint-discovery traffic by TLS record size. Device logs confirmed list payment endpoints was pending; relay timestamps verified that Save finished before the held traffic was released. Profile reads and authenticated contact writes remained available.
Contacts showed the peer once. After restoring access and reopening the contact, Pay resolved its regtest address and stopped at “Insufficient Savings.” No payment was sent. Saving did not establish private-link readiness.
J1-2x.2x.mp4
J1 at 2× speed; the key is already entered at the start.
Check 1: Build and contact regressions ✅ The pinned head built and the focused contact view-model tests passed.
Ran assembleDevDebug and testDevDebugUnitTest --tests to.bitkit.ui.screens.contacts.AddContactViewModelTest. The installed APK hash matched the build. Full lint and broader suites were not repeated; the current-head CI checks and lint workflow passed.
I relied on the existing Android device review for normal discovery and rapid repeated Save taps at this head.
Twin: bitkit-ios#1119 fix: keep contact previews independent of Paykit matches the early profile/Save behavior, guarded Pay availability, cancellation handling and duplicate-save prevention.
Full review of dc5a47d..7e56c3c, all four changed files and affected callers. Cancellation, superseded reads and endpoint completion during an active save were checked with unit tests rather than separate device cases.
Reviewer model: gpt-6.1-sol
Reasoning effort: high
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
Twin: synonymdev/bitkit-ios#1119
Dependency: #1681
Description
A contact profile already fetched from Pubky stayed on the loading screen while optional payment-endpoint discovery ran. Show the profile and allow Save immediately; expose Pay only after a payable endpoint is confirmed. Ignore cancelled or superseded reads and prevent repeated Save calls from creating overlapping writes.
The confirmed contact write and existing background private-link preparation are unchanged. Those post-save helpers already schedule the handshake rather than waiting for it; this PR repairs the separate preview wait.
Out of Scope
Full-wallet backup separation, ordinary receive/startup/payment-result waits and historical contact-failure attribution are separate investigations. This PR does not report a saved contact as synced or payment-ready.
Design
N/A — no design available. Existing layout, updated loading behavior.
Preview
Existing contact preview with Save visible while optional payment discovery remains pending. Native test artifacts retain rendered iOS screenshots.
QA Notes
Setup
Use the committed Debug dependencies: Core 0.8.0-rc11, Paykit 0.1.1-rc1, VSS 0.5.23. Component tests inject held/failed public-endpoint reads and do not require funds. J1 requires a disposable authenticated identity, an unsaved peer with a readable profile, and a compatible homeserver fault fixture that delays only public payment-endpoint lookup while allowing contact writes. That complete service fixture is not currently provisioned; do not treat the component tests as its execution. No payment is needed.
Journeys
save-during-endpoint-discovery.xml— save a fetched contact while optional endpoint discovery is delayed, without claiming payment readiness.Manual Tests
N/A — the controlled service prerequisite and journey are described above.
Automated Checks
AddContactViewModelTest.kt— held endpoint discovery, failure and cancellation; Android also covers superseded reads, save completion and duplicate prevention.Evidence
Controlled held-read tests prove the profile can be saved before endpoint discovery completes, stale/non-cooperative results are ignored, endpoint completion preserves an active save, and repeated Save calls create one write. Three regression cases fail unchanged production.
Required just compile, just test and just lint passed. All 254 JUnit reports: 4,609 tests, zero failures/errors/skips. Detekt SARIF and Android Lint XML/SARIF: zero findings. Structured Gradle reports exactly match the unchanged Detekt plugin deprecation. Two consecutive final independent reviews found no actionable issues after correcting test isolation and the platform-specific journey title. Tested source exactly matches the signed implementation commit; later changes, if any, are documentation only.
Gaps
J1 has not run against an authenticated homeserver fault fixture. iOS hosting tests inspect rendered UI and cancellation without tapping Save through real persistence; Android save tests mock persistence/link preparation. No claim that this caused the historical contact incident.
Models used
gpt-6-astra(reasoning:xhigh)gpt-6-astra(reasoning:xhigh)gpt-6-astra(reasoning:xhigh)