feat: introduce improved sync screen experience #619
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
EvanHahn
approved these changes
Aug 25, 2024
src/frontend/hooks/useSyncState.ts
Outdated
|
|
||
| // Remove devices from #perDeviceMaxSyncCount that are no longer found in the new sync state | ||
| for (const deviceId of this.#perDeviceMaxSyncCount.keys()) { | ||
| if (!connectedDevices.includes(deviceId)) { |
There was a problem hiding this comment.
Nit: this is O(N) but could be O(1).
Suggested change
| if (!connectedDevices.includes(deviceId)) { | |
| if (!Object.hasOwn(state.remoteDeviceSyncState, deviceId)) { |
(Is this the first time Big O notation has ever been relevant in my career? For this tiny nitpick?)
| WifiIcon, | ||
| } from '../../sharedComponents/icons'; | ||
|
|
||
| type SyncStage = |
Comment on lines
335
to
336
| // @ts-expect-error | ||
| throw new Error(`Invalid status: ${syncStage.status}`); |
There was a problem hiding this comment.
Nit: consider ExhaustivenessError. Same comment applies to the switch below.
| </Text> | ||
| )} | ||
| </View> | ||
| ); | ||
| } | ||
|
|
||
| function deriveSyncStage({ |
There was a problem hiding this comment.
Seems like this could be useful to test? Wrote tests in this gist.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #550
Closes #602
Closes #603
Closes #604
Towards #605
Closes #610
Important notes:
There is a bug where sync can occur without opt-in by connected peers. This is backend-related (see Sync can unexpectedly occur with devices that have not enabled it comapeo-core#762).
Upgrades core to latest alpha version, which introduces a new sync state api and also the ability to set/configure sync autostopping
This PR does not change the behavior related to sync being turned off when leaving the screen. I think that can/should be a follow-up PR that also introduces auto-stopping sync when leaving the screen after being fully synced.
Not an exhaustive showcase of all of the states (hard to do with just 2 devices)
01-sync-alone.mov
02-sync-no-data.mov
03-sync-with-data.mp4