Skip to content

fix: save contacts without repeated remote lookups - #851

Merged
ovitrif merged 5 commits into
masterfrom
fix/contact-import-progress
Oct 1, 2026
Merged

ovitrif merged 5 commits into
masterfrom
fix/contact-import-progress

Conversation

@ben-kaufman

@ben-kaufman ben-kaufman commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Closes #853
Twin: synonymdev/bitkit-android#1395

Importing an already prepared list of contacts repeated profile lookups and receiver discovery for every contact. Those requests were serialized by the SDK adapter, so large lists could remain busy for minutes. Partial failures were also allowed to advance past import.

Description

  • Save the profiles already shown in the preview using local SDK contact storage. The import operation no longer performs profile or receiver network lookups.
  • Preserve existing receiver paths; new contacts start with the wallet path. Additional receivers are discovered through the existing contact refresh/private-link flow.
  • Preserve successful saves and keep the remaining import retryable when a local save fails. Retry skips contacts already saved.
  • Prevent repeated import actions while a save is pending.

Out of Scope

  • SDK lock and network timeout redesign; profile discovery before the preview still requires connectivity.
  • The exact cause of the later payment-sharing Continue failure is not established: the supplied log export ends before that attempt.
  • Profile restoration/deletion feedback is covered by separate changes.

Design

N/A — no design available. Existing import screens and loading controls are reused.

Preview

The reported recordings and logs were supplied privately and are not attached here. No post-fix device recording is claimed; the import journeys below remain for device QA.

QA Notes

Journeys

  • new import-all-contacts.xml — imports a prepared list, including Pubky-only profiles; repeat with 62 contacts.
  • new import-selected-contacts.xml — saves selected contacts and leaves the deselected contact absent.

Both journeys are ported between Android and iOS.

Manual Tests

  • Disable connectivity after the preview is populated → prepared contacts can still be saved without remote discovery — network fault injection is outside journey capabilities.
  • Fail a local contact save → remain on import, preserve successful saves and retry missing contacts; do not report complete success — storage fault injection is outside journey capabilities.

Automated Checks

  • A storage-backed regression exercises the default SDK import path without a live session. It verifies canonical key/label persistence, the default wallet receiver, preservation of existing receiver paths, and reloading after the SDK runtime is discarded.

  • updated ContactsManagerTests.swift — a 62-contact prepared batch saves without network resolution, duplicates are skipped, partial failure preserves successful saves, retry saves missing contacts, and cancellation stops further writes/results.

  • ran 45 tests across ContactsManagerTests and ContactPaymentsServiceTests: all passed, including existing publication/rollback behavior.

  • ran XcodeBuildMCP simulator build/test with DEBUG E2E_BUILD UNIT_TESTING, SwiftFormat and git diff --check.

Four new regression tests; no existing tests removed.

Independent final correctness and repository-fit review: clean.

@greptile-apps

greptile-apps Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 3/5

[Medium risk] Refactors contact import to skip remote lookups on prepared data.

The PR should not merge until canonical-key deduplication and cancellation reconciliation are addressed.

Findings

  1. P1 Mixed-case keys bypass duplicate checks ▶
  2. P1 Cancellation hides completed saves ▶
  3. P2 SDK save path remains untested ▶

Summary

The PR saves prepared preview contacts locally instead of repeating profile and receiver lookups, retains successful saves on ordinary partial failure, and adds import journeys and regression tests.

  • The new import path needs canonical-key handling and reconciliation of completed saves on cancellation.
  • The new tests do not exercise the default SDK persistence path.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Prepared preview contacts] --> B[Import: skip keys in memory]
  B --> C[Save contact locally]
  C --> D[Publish completed contacts]
  D --> E[Contacts change triggers link burst]
  E --> F[Discover and persist receiver paths]
Loading

Reviews (1) · Last reviewed commit: "fix: save prepared contacts without remo..."

Comment thread Bitkit/Managers/ContactsManager.swift
Comment thread Bitkit/Managers/ContactsManager.swift
Comment thread BitkitTests/ContactsManagerTests.swift

@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.

Advice: ✅ Approve

Review: diff 8 files.
Pair PR synonymdev/bitkit-android#1395: equivalent.

Findings:
2 inline (2 LOW)

QA:
Tests running: 2 of 4 passed.


Reviewed by gpt-6-sol-xhigh via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest (author)

Comment thread journeys/contacts/README.md Outdated
Comment thread Bitkit/Views/Contacts/ContactImportOverviewView.swift

@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 (own key in the follow list).

Merge note: #830 adds a restorePrivateConnection || existing != nil guard to saveContact, and the two PRs conflict in ContactsManager.swift. Whichever lands second must make the new default saveContact closure in importContacts pass restorePrivateConnection: true, or every import of a new contact fails with profileNotFound.

Checked and clean:

  • Receiver paths are re-discovered on every link/publication pass, and endpoints are fetched from the contact key at pay time.
  • The label is only a fallback.
  • The saved key is the preview key.
  • Partial success is kept, and a retry saves only the missing contacts.
  • The progress guards hold.
  • The journey files are identical to the Android twin.

Comment thread Bitkit/Managers/ContactsManager.swift Outdated

@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.

Advice: ✅ Approve

Reaudit: diff 9 files.
New findings below; the rest is in the review.
Retest suggested: Tests 1-2, J1, J2 (Both journeys changed, as did ContactsManager and the self-follow expectations.)
Pair PR synonymdev/bitkit-android#1395: not compared; the prepared-save, retry and pending-control behavior remains aligned with Android PR #1395.

Findings: 2 fixed. No new findings.

The README guidance is wrapped, and both import views now have tests for pending-state UI. Self-follows are excluded before profile resolution, with tests for bare and prefixed keys and self-only lists.

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)

@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.

Advice: ✅ Approve

Reaudit: diff 3 files.
New findings below; the rest is in the review.
Retest suggested: Tests 1-2, J1, J2 (No QA item finished.)
The prepared-save, retry, and pending-control behavior remains aligned with Android PR #1395; that PR was not compared here.

Findings: 2 previously fixed. No new findings.

Prepared contact saving now restores local private connections as required by the updated SDK. The DEBUG fixture initializer works with the updated manager, and the import regression tests remain intact.

QA:
Tests running: 4 of 4 passed.


Reviewed by gpt-6.1-sol-medium via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest (author)

@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.

Verdict: ✅ Approve

Tests for the review: Tests 1 and 2 and journeys J1 and J2 pass.

QA:
Tested on iOS 26.5 simulator (iPhone 17 Pro), regtest.

🟢 Tests 1, 2, J1, J2
Test 1

Import completed offline and all three contacts remained after reconnecting.

1.mp4

Test 2

The failed save kept import open and preserved two contacts; retry saved only the missing contact.

2.mp4

Test J1

Imported all 62 Pubky-only contacts; self-follow variants excluded the owner from saved contacts.

J1small.mp4
J1large.mp4
J1self.mp4
J1selfonly.mp4

Test J2

Selected contacts were saved and the deselected contact stayed absent, including with self-follow.

J2.mp4
J2self.mp4

Note

I ticked Tests 1–2 and J1–J2 in the PR checklist.

Tip

Tests 1, 2 worth a journey
Test 1
  • Open the populated contact import preview
  • Disable connectivity after the preview is populated
  • Verify the prepared contact list remains visible
  • Tap Import All
  • Verify Let Your Contacts Pay You appears
  • Restore connectivity and open Contacts to verify the saved names

Test 2

  • Open the profile import preview with three contacts
  • Make one contact save fail in the debug build
  • Tap Import All
  • Verify the import screen stays open and two contacts were saved
  • Clear the save fault and tap Import All again
  • Verify only the missing contact was written and all three contacts appear


Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest (author)

@piotr-iohk piotr-iohk 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.

QA review

Scope: Full review of the complete pull request diff against its merge base, at b7ce984.

No new actionable code findings.

Prepared import saves the contacts already shown in the preview, keeps successful saves when a later local save fails, and skips the signed-in profile before profile resolution. That matches #853. New contacts start on the wallet receiver path; the existing contact-sync link burst discovers further paths. The same prepared-save, retry, and self-follow behavior is in bitkit-android#1395 at faae845. Follow keys are re-encoded by Paykit before they reach this filter, so a padding-bit alias of the same key arrives as the canonical spelling.

Validation: inspected ContactsManagerTests and ContactImportUITests at this revision and did not run them here. Run Tests passed on this head.

Device testing: not performed in this review. ovi-reviewer reports manual tests 1–2 and journeys J1–J2 passed on an iOS 26.5 simulator at this revision.

Ready for device testing.

Suggested additional test cases

  • Platform: iOS simulator, regtest, disposable Ring identity.
  • Setup: the following list includes that identity under a z-base32 spelling that changes only the final character's padding bits, plus at least one other follow.
  • Action: open the import preview and tap Import All.
  • Expected result: the preview count excludes that identity, Import All finishes on Let your contacts pay you, and Contacts does not contain that profile.

@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.

No new findings. The self-follow thread is addressed at b7ce984.

Checked and clean:

  • ContactsManager.swift:548 skips the signed-in key before profile resolution, so it never reaches pendingImportContacts or importContacts.
  • Padding-alias self-follows, which Android filters with canonicalized in synonymdev/bitkit-android#1395, cannot reach matches here. Follow keys come back from fetchPubkyFollows re-encoded by PubkyPublicKey::new, and the session key is derived from the secret via pubkyPublicKeyFromSecret, so both inputs are canonical.
  • After the merge, the default importContacts closure passes restorePrivateConnection: true. New contacts no longer throw profileNotFound, and blocked peers are re-blocked if the save fails.
  • Partial failure keeps the saved subset. contacts drives prepareSavedContacts and the link burst. A retry saves only the missing keys and rethrows the first error so the preview stays.
  • Both views guard against re-entry with isImporting, and Select is disabled while an import runs.
  • ContactImportUITestFixture is only reachable under #if DEBUG with -contact-import-ui-test, and the release path is unchanged.

Device gate: not run — the contact journeys need a disposable Ring identity with a known follow list including its own key, which the journey Capabilities table does not provide. The iOS run waits on the Android gate.

@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.

reAck

@ovitrif
ovitrif merged commit 0cdb704 into master Oct 1, 2026
18 checks passed
@ovitrif
ovitrif deleted the fix/contact-import-progress branch October 1, 2026 14:10
Jasonvdb added a commit that referenced this pull request Oct 1, 2026
…bky-profile-load-lag

Adopts #851's contact import: it saves the profiles the preview already
shows to local SDK contact storage, without profile or receiver lookups,
and the import screens guard against repeated taps. This drops the
branch's own import save path, its unresolved follow tracking, the
import running flag and the route check after an import, which #851
replaces. Follow lookups while preparing an import stay on the
interactive read lane.

# Conflicts:
#	Bitkit/Managers/ContactsManager.swift
#	Bitkit/Views/Contacts/ContactImportOverviewView.swift
#	Bitkit/Views/Contacts/ContactImportSelectView.swift
Jasonvdb added a commit that referenced this pull request Oct 1, 2026
#851 adds journeys for importing all and selected contacts, which cover
saving the prepared follows. contact-import.xml becomes
contact-import-after-leaving.xml and keeps only what those do not: an
import keeps saving after you leave the overview and, when it finishes,
does not take you to Pay Contacts. The Contacts list journey now points
at import-all-contacts.xml for its precondition.
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.

bug: importing contacts stays busy for minutes on large lists

4 participants