Basic account import #1347

robknight · 2023-12-11T17:28:53Z

artwyman

Review of the core PCD-handling logic, and some thoughts on the user experience. I didn't review the UI integration in detail.

This simple version is probably fine, though I raised some questions in comments to think about where it might cause confusing situations.

I think what I'll need for sync merge is going to be somewhat more complex than this. Depending on where things go from comments, this code converge with that need, or could be kept separate. I'm less concerned about duplicated effort than I was, because this code is less complex than I feared.

apps/passport-client/components/modals/AccountImportModal.tsx

artwyman · 2023-12-11T18:33:49Z

apps/passport-client/components/modals/AccountImportModal.tsx

+              setImportState({
+                valid: true,
+                imported: false,
+                pcds,


This is explicitly importing only PCDs, not subscriptions, and not other user data which is in the blob. I think that's a valid choice for this feature, though maybe different from what I want for sync merge.

Yes - I'm not sure whether restoring subscriptions makes sense or not, or whether it would be a desirable feature.

Depends on whether the user sees this as importing a set of PCDs, or rolling back their account to a known good state. Feeds are likely not hard to reconstruct, but they are technically user data which could be lost in a conflict.

apps/passport-client/components/modals/AccountImportModal.tsx

artwyman · 2023-12-11T18:39:46Z

apps/passport-client/components/modals/AccountImportModal.tsx

+    for (const pcd of importState.pcds) {
+      if (!pcdCollection.hasPCDWithId(pcd.id)) {
+        try {
+          pcdCollection.add(pcd);


This works fine for import. Given I recently realized that PCD ordering is a user-visible piece of state, let me think through that here. This logic doesn't attempt to preserve PCD ordering but puts imported ones at the end of the folder order as if they were new. Doing otherwise would be complicated, so I'm not recommending it, just pointing it out.

apps/passport-client/components/modals/AccountImportModal.tsx

artwyman · 2023-12-12T02:53:40Z

I reviewed the latest changes and resolved one conversation based on it, but others remain TBD.

artwyman

Looking great! A few more suggestions inline. I also think you planned to investigate single-folder import before calling this done

apps/passport-client/components/modals/AccountImportModal.tsx

apps/passport-client/src/dispatch.ts

packages/pcd-collection/src/PCDCollection.ts

artwyman · 2023-12-13T02:10:21Z

packages/pcd-collection/src/PCDCollection.ts

+
+    this.addAll(pcds, { upsert: true });
+
+    // If the caller wants folders to be merged too


Not your fault, but it's super weird to me that setting folders is a separate operation from adding the PCDs. Should probably be unified into add/addAll someday.

packages/pcd-collection/test/PCDCollection.spec.ts

artwyman

A few more things to consider or discuss.

apps/passport-client/components/modals/AccountImportModal.tsx

artwyman · 2023-12-13T19:45:49Z

apps/passport-client/src/dispatch.ts

+  collection: PCDCollection,
+  pcdsToMergeIds: Set<PCD["id"]>
+) {
+  const userHasExistingSemaphoreIdentityPCD =


It would be nice if this filtering logic (special-case for identity, email, and the decision never to replace the same ID) were in a helper function defined once rather than duplicated both here and in the UI.

apps/passport-client/src/dispatch.ts

artwyman · 2023-12-13T19:52:56Z

apps/passport-client/src/dispatch.ts

+        imported: pcds.getAll().length - pcdCountBeforeMerge
+      }
+    });
+  } catch (e) {


Log the error to console. Actually in general a few console log statements in this function in normal path would be welcome.

I'd still like to see some console logging in here, both for errors and normal cases.

Ping that this still remains to be done, though I'm not going to hold back the review just for logging.

apps/passport-client/src/state.ts

apps/passport-client/src/dispatch.ts

packages/pcd-collection/src/PCDCollection.ts

ichub

bug report

I exported pcds from one account, after having added a new eddsa signature pcd via consumer-client. then, I logged out, and logged in with a new account. attempting to import my pcds from the 1st account resulted in the following error:

However, after reloading the page, the eddsa signature PCD is indeed imported.

Opinions - everyone's got 'em, including me :3 Sorry to be so pedantic in my review, but it's what I think based on the information I have.

apps/passport-client/components/modals/AccountImportModal.tsx

apps/passport-client/src/dispatch.ts

apps/passport-client/src/state.ts

packages/pcd-collection/src/PCDCollection.ts

apps/passport-client/src/dispatch.ts

robknight · 2023-12-14T10:37:19Z

The modal is now a screen, which makes the UI nicer, and also removes the sync problem: sync only runs when you're on a screen with the useSyncE2EEStorage() hook, and by not including that here we can ensure that the sync process will not interfere with the import.

ichub

tested locally and it seemed to work.

ichub · 2023-12-14T19:03:31Z

apps/passport-client/components/modals/SettingsModal.tsx

+            <LinkButton $primary={true} to="/import" onClick={close}>
+              Import Account Data
+            </LinkButton>
+            <Spacer h={16} />


I'm not sure this spacer made a difference as the modal is still the same size for me. I wonder if its height is hard-coded somewhere.

apps/passport-client/pages/index.tsx

apps/passport-client/src/state.ts

apps/passport-client/components/screens/ImportBackupScreen.tsx

artwyman

I like the new functionality, but a few more requests to make on the latest state.

apps/passport-client/components/screens/ImportBackupScreen.tsx

apps/passport-client/src/dispatch.ts

artwyman · 2023-12-14T20:21:26Z

apps/passport-client/src/dispatch.ts

+        imported: pcds.getAll().length - pcdCountBeforeMerge
+      }
+    });
+  } catch (e) {


I'd still like to see some console logging in here, both for errors and normal cases.

apps/passport-client/components/screens/ImportBackupScreen.tsx

artwyman · 2023-12-15T03:57:53Z

packages/pcd-collection/src/PCDCollection.ts

+
+  /**
+   * Merges another PCD collection into this one.
+   * There are two options:


There aren't two of these anymore.

robknight requested a review from artwyman December 11, 2023 17:28

artwyman requested changes Dec 11, 2023

View reviewed changes

robknight force-pushed the rob/import-backup branch from 7184178 to 9989899 Compare December 12, 2023 15:42

artwyman requested changes Dec 13, 2023

View reviewed changes

robknight force-pushed the rob/import-backup branch 2 times, most recently from da05221 to d788104 Compare December 13, 2023 14:53

artwyman requested changes Dec 13, 2023

View reviewed changes

ichub reviewed Dec 13, 2023

View reviewed changes

robknight force-pushed the rob/import-backup branch from 5f15edb to f399ff6 Compare December 14, 2023 09:53

ichub approved these changes Dec 14, 2023

View reviewed changes

artwyman requested changes Dec 14, 2023

View reviewed changes

artwyman approved these changes Dec 15, 2023

View reviewed changes

artwyman reviewed Dec 15, 2023

View reviewed changes

artwyman added a commit that referenced this pull request Dec 15, 2023

PCDCollection merge extracted from #1347

66671ce

artwyman mentioned this pull request Dec 15, 2023

Implement merge for sync #1360

Merged

robknight force-pushed the rob/import-backup branch from 08dab73 to 148b7f5 Compare December 15, 2023 20:18

robknight added 12 commits December 15, 2023 20:18

Basic account import

d2dc138

Fix lint

5d1731a

Added more comments

36dd573

Move merge logic to PCDCollection and add tests

e0e073c

Review feedback

577ce04

Make import modal state machine more explicit

a2d67d9

Prevent import of secondary email PCDs

1ed80e1

Final (?) tidying

f548869

Review feedback

1a20729

Review feedback

db1f5c5

More review feedback

30de606

Change modal view to screen

b3aa823

robknight added 3 commits December 15, 2023 20:18

Allow user to select which folders to import

0419492

Review feedback

053c93d

More review feedback

f4b8dd9

robknight force-pushed the rob/import-backup branch from 148b7f5 to f4b8dd9 Compare December 15, 2023 20:18

robknight enabled auto-merge December 15, 2023 20:18

Fix yarn.lock

46f5bdb

robknight added this pull request to the merge queue Dec 15, 2023

Merged via the queue into main with commit 98d39d9 Dec 15, 2023
1 check passed

robknight mentioned this pull request Dec 15, 2023

Queue action dispatches #1363

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Basic account import #1347

Basic account import #1347

robknight commented Dec 11, 2023 •

edited

Loading

artwyman left a comment

artwyman Dec 11, 2023

robknight Dec 11, 2023

artwyman Dec 12, 2023

artwyman Dec 11, 2023

artwyman commented Dec 12, 2023

artwyman left a comment

artwyman Dec 13, 2023

artwyman left a comment

artwyman Dec 13, 2023

artwyman Dec 13, 2023

artwyman Dec 14, 2023

artwyman Dec 15, 2023

ichub left a comment

robknight commented Dec 14, 2023

ichub left a comment

ichub Dec 14, 2023

artwyman left a comment

artwyman Dec 14, 2023

artwyman Dec 15, 2023


		this.addAll(pcds, { upsert: true });

		// If the caller wants folders to be merged too

Basic account import #1347

Basic account import #1347

Conversation

robknight commented Dec 11, 2023 • edited Loading

artwyman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artwyman commented Dec 12, 2023

artwyman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artwyman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ichub left a comment

Choose a reason for hiding this comment

bug report

robknight commented Dec 14, 2023

ichub left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artwyman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robknight commented Dec 11, 2023 •

edited

Loading