feat: add Sync screen #241

achou11 · 2024-04-09T21:05:08Z

Closes #118

Intentional omissions/departures:

no line of text displaying connection to cloud server
no mention of map sync in the sync progress title
no animation between different screen states
no wifi screen being primarily white instead of primarily blue
no wiggle animation for button when pressing during "up-to-date" state

Important changes:

completely refactored the useSyncState() hook because I was noticing stale closure issues with the implementation. using a class-based approach seemed easier to work with and easier to get working.
introduces useSyncProgress() hook, derived from the same sync store instance as useSyncState()

TODOs that may block merging:

~~use correct color for project icon in ProjectSyncDisplay~~
~~figure out what settings button in top right of ProjectSyncDisplay is supposed to do~~
~~decide if we want to suspend data sync when app is not focused~~
~~figure out how to calculate progress based on sync state~~
~~review changes to useSyncState hook~~

Pretty difficult to test the sync states right now, so previews are working with mocked situations:

No project joined/created:

Nothing to sync / sync done state:

Something to sync state:

Syncing state:

achou11 · 2024-04-15T21:53:18Z

There are some TODOs that I scattered in the PR in the earlier days of working on this. I haven't looked at them in a while but will revisit tomorrow to see if they're still relevant

ErikSin · 2024-04-17T21:45:49Z

Can you rebase/merge from main? It will be easier to test once I can invite someone to a project

achou11 · 2024-04-17T21:48:32Z

Can you rebase/merge from main? It will be easier to test once I can invite someone to a project

done!

ErikSin · 2024-04-17T22:16:36Z

Were you able to make 2 devices sync? I am testing with 2 devices that are both on the same project and network and they are not discovering each other. My phone has this problem with sync in mapeo-mobile, so it could be my device.

achou11 · 2024-04-17T22:19:22Z

Were you able to make 2 devices sync? I am testing with 2 devices that are both on the same project and network and they are not discovering each other. My phone has this problem with sync in mapeo-mobile, so it could be my device.

honestly haven't tried actually syncing yet (needed the invite stuff merged before I could test it). expecting there to be some issues that I need to resolve, so consider this PR "ready to review but probably needs lots of work based on real testing" 😅

as for your issue, are you talking about discovery when inviting or during sync?

ErikSin · 2024-04-17T22:23:17Z

as for your issue, are you talking about discovery when inviting or during sync?

I was able to discover the other device and invite them to a project. But on the sync screen, both devices are saying "0 devices nearby" even when they are on the same wifi network

achou11 · 2024-04-17T22:24:50Z

as for your issue, are you talking about discovery when inviting or during sync?

I was able to discover the other device and invite them to a project. But on the sync screen, both devices are saying "0 devices nearby" even when they are on the same wifi network

ah that might be an issue in my implementation then. will need to look into it but thanks for noting it 👍

achou11 · 2024-04-17T22:29:43Z

converting to a draft until I revisit this and test it out given the recent updates, which are relevant for this

ErikSin · 2024-04-17T22:37:34Z

I did a quick code review (not a functionality review) and no glaring problems. I didn't go to deep into it though because it seems like there will some code changes when trying to make it work

EvanHahn

LGTM with a small question and a few nitpicks.

EvanHahn · 2024-04-24T16:22:25Z

src/frontend/hooks/useSyncState.ts

-    isSubscribedInternal = true;
-    project.$sync
+  #onSyncState = (state: SyncState) => {
+    console.log({from: 'listener', ...state});


Nitpick: should we remove this?

Suggested change

console.log({from: 'listener', ...state});

src/frontend/hooks/server/projects.ts

EvanHahn · 2024-04-24T16:30:27Z

src/frontend/hooks/useSyncState.ts

+      .then(this.#onSyncState)
      .catch(e => {
-        error = e;
-        listeners.forEach(listener => listener());
+        this.#error = e;
+        this.#notifyListeners();


Should we do anything special if we've unsubscribed before this promise resolves? I imagine the following scenario:

#startSubscription is called, kicking off this.#project.$sync.getState().

#stopSubscription is called.

The getState() promise resolves.

i think it's okay because resolving will eventually call the subscribed listeners (in #onSyncState), but if #stopSubscription is called it's because there are no more listeners, so it should result in a no-op.

that's my initial thinking, anyways

achou11 · 2024-04-24T16:41:48Z

src/frontend/sharedComponents/ScreenContentWithDock.tsx

i'm hoping this is a useful shared component for us moving forward. basically creates a layout that includes a fixed position "dock" at the bottom and a scrollable content area above it. this is to account for allowing dynamic content area size without needing to scroll down to the bottom of the view to see the content in the dock. the overflow from the content area gets clipped by the fixed dock area. usually this dock area contains big, actionable buttons, and we don't want these to get hidden due to being pushed by an overflowing content area

achou11 · 2024-04-24T16:42:33Z

src/frontend/screens/Sync/index.tsx

+  // TODO: Replace with proper check of being a part of a shared project
+  if (data && data.length === 1) {


highlighting this, even though this needs to be addressed later due to core limitations

achou11 · 2024-04-24T16:43:12Z

src/frontend/screens/Sync/index.tsx

+}
+
+export const SyncScreen = ({navigation}: NativeRootNavigationProps<'Sync'>) => {
+  // TODO: Is this the right field to use?


can probably remove this?

achou11 · 2024-04-24T16:43:45Z

src/frontend/screens/Sync/index.tsx

+  // TODO: Is this the right field to use?
+  const wifiStatus = useLocalDiscoveryState(state => state.wifiStatus);
+
+  // TODO: Handle error case


is this something we can address in this PR, or should be leave it for later? not sure what the error state would look like (would it use the bottom sheet modal pattern?)

achou11 · 2024-04-24T16:59:38Z

src/frontend/screens/Sync/ProjectSyncDisplay.tsx

+  React.useEffect(() => {
+    if (isSyncDone) {
+      project.$sync.stop();
+      queryClient.invalidateQueries({queryKey: [OBSERVATION_KEY]});
+    }
+  }, [isSyncDone, project, queryClient]);


discussed elsewhere but this check should probably be done when leaving the screen e.g. navigation.addListener('beforeRemove', ...)

src/frontend/screens/Sync/ProjectSyncDisplay.tsx

achou11 force-pushed the 118/sync-screens branch 5 times, most recently from 1572d25 to d2f0952 Compare April 15, 2024 21:48

achou11 requested a review from ErikSin April 15, 2024 21:52

achou11 marked this pull request as ready for review April 15, 2024 21:52

achou11 force-pushed the 118/sync-screens branch from d2f0952 to 2cf73e9 Compare April 17, 2024 21:47

achou11 marked this pull request as draft April 17, 2024 22:28

ErikSin force-pushed the 118/sync-screens branch from 2cf73e9 to 2a086ec Compare April 23, 2024 21:46

ErikSin marked this pull request as ready for review April 24, 2024 16:19

EvanHahn approved these changes Apr 24, 2024

View reviewed changes

achou11 commented Apr 24, 2024

View reviewed changes

achou11 added 7 commits April 24, 2024 10:44

WIP

fbacc01

use svg image for project sync display

849abef

make header right button navigate to project settings

a1f5b09

remove old screen name

511064d

remove unused variable

e9f8428

do not stop sync when leaving screen

618ebc5

add useSyncProgress() hook

7565393

ErikSin added 4 commits April 24, 2024 10:44

chore: small typing fixes

136c7d7

chore: add isSyncDone

7197863

removes console.log

896b3f5

chore: cleanup sync on dismount

47e827b

ErikSin force-pushed the 118/sync-screens branch from 48f51fb to 47e827b Compare April 24, 2024 17:44

achou11 commented Apr 24, 2024

View reviewed changes

src/frontend/screens/Sync/ProjectSyncDisplay.tsx Outdated Show resolved Hide resolved

ErikSin added 2 commits April 24, 2024 14:09

chore: use navigation listener to stop sync

f485ca7

chore:translations

0793d94

ErikSin merged commit de78926 into main Apr 24, 2024
3 checks passed

ErikSin deleted the 118/sync-screens branch April 24, 2024 21:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add Sync screen #241

feat: add Sync screen #241

achou11 commented Apr 9, 2024 •

edited

Loading

achou11 commented Apr 15, 2024

ErikSin commented Apr 17, 2024

achou11 commented Apr 17, 2024

ErikSin commented Apr 17, 2024

achou11 commented Apr 17, 2024 •

edited

Loading

ErikSin commented Apr 17, 2024

achou11 commented Apr 17, 2024

achou11 commented Apr 17, 2024

ErikSin commented Apr 17, 2024

EvanHahn left a comment

EvanHahn Apr 24, 2024

EvanHahn Apr 24, 2024

achou11 Apr 24, 2024 •

edited

Loading

achou11 Apr 24, 2024 •

edited

Loading

achou11 Apr 24, 2024

achou11 Apr 24, 2024

achou11 Apr 24, 2024

achou11 Apr 24, 2024 •

edited

Loading

		// TODO: Replace with proper check of being a part of a shared project
		if (data && data.length === 1) {

feat: add Sync screen #241

feat: add Sync screen #241

Conversation

achou11 commented Apr 9, 2024 • edited Loading

achou11 commented Apr 15, 2024

ErikSin commented Apr 17, 2024

achou11 commented Apr 17, 2024

ErikSin commented Apr 17, 2024

achou11 commented Apr 17, 2024 • edited Loading

ErikSin commented Apr 17, 2024

achou11 commented Apr 17, 2024

achou11 commented Apr 17, 2024

ErikSin commented Apr 17, 2024

EvanHahn left a comment

Choose a reason for hiding this comment

EvanHahn Apr 24, 2024

Choose a reason for hiding this comment

EvanHahn Apr 24, 2024

Choose a reason for hiding this comment

achou11 Apr 24, 2024 • edited Loading

Choose a reason for hiding this comment

achou11 Apr 24, 2024 • edited Loading

Choose a reason for hiding this comment

achou11 Apr 24, 2024

Choose a reason for hiding this comment

achou11 Apr 24, 2024

Choose a reason for hiding this comment

achou11 Apr 24, 2024

Choose a reason for hiding this comment

achou11 Apr 24, 2024 • edited Loading

Choose a reason for hiding this comment

achou11 commented Apr 9, 2024 •

edited

Loading

achou11 commented Apr 17, 2024 •

edited

Loading

achou11 Apr 24, 2024 •

edited

Loading

achou11 Apr 24, 2024 •

edited

Loading

achou11 Apr 24, 2024 •

edited

Loading