Repository navigation
refactor(exchange): use core-react sync hooks for the server connection - #2213
gmaclennan wants to merge 5 commits into
Conversation
| }); | ||
|
|
||
| // Cutting the server websocket mid-sync leaves it with partial data. | ||
| const shouldDisconnectServersWhenLeavingScreen = |
There was a problem hiding this comment.
Do we need this variable, or could we just use shouldDisconnectServersRef.current, and avoid the useEffect to update it?
There was a problem hiding this comment.
Half of it. The variable is gone (afac571), but the effect has to stay: writing ref.current during render trips the react-hooks/refs rule in this repo's eslint config ("Cannot update ref during render"), so the only lint-clean way to keep the latest value readable from the focus cleanup is an effect. It now runs without a deps array and assigns directly, which is as small as I could make it.
…212) When syncing with more than one device at once, the data sync progress reported by `useDataSyncProgress` can jump to 1 (complete) while another peer, typically the cloud archive, still has blocks to exchange. It looks intermittent because it only shows when the device that finished first happens to be the last one enumerated. The cause is in `src/lib/sync.ts`: inside the loop over enabled devices, `totalMaxSyncCount` is accumulated with `+=` but `currentSyncCount` is assigned with `=`, so only the last device's remaining count survives. The change makes it `+=` so progress reflects the remaining blocks across all devices. The new test in `test/lib/sync.test.ts` drives `SyncStore` directly with a fake `$sync` rather than real IPC peers, since the existing progress test in `projects.test.tsx` has no multi-peer setup. It can move there if the hook-level route is preferred. Related changes. A premature "complete" could be one of the reasons people leave the Exchange screen or press Stop while the cloud still wants blocks. digidem/comapeo-mobile#2213 keeps the server connection when the user leaves anyway, and digidem/comapeo-core#1353 keeps serving after Stop. This fix is independent of both and the cheapest of the three.
Replace the direct projectApi.$sync calls in ExchangeScreenContent with the mutation hooks from @comapeo/core-react, and drop the ref that kept the server connection open on blur while sync was in progress. Disconnecting gracefully is moving into core's disconnectServers(), so the screen goes back to an unconditional disconnect on blur. The component no longer needs projectApi from useActiveProject.
…n blur The previous tests asserted the conditional disconnect that has been removed. The remaining test mocks the core-react hooks and checks that the screen clears the autostop and connects servers once on focus, and disconnects once when another screen is pushed.
gmaclennan
changed the title
|
For anyone who read the earlier version: this PR no longer keeps the server websocket open on blur while sync is in progress. Review decided that decision belongs in core, so |
|
Since this can be fixed in core, I don't think this refactor is a priority, and I think we can do it later because we could be changing the sync api anyway. |
Replaces the direct
projectApi.$synccalls inExchangeScreenContentwith the mutation hooks that@comapeo/core-reactalready exports:useConnectSyncServers,useDisconnectSyncServers,useSetAutostopDataSyncTimeoutanduseStopSync. The screen no longer needsprojectApifromuseActiveProject. The focus effects depend on the hooks'mutatefunctions, which react-query keeps referentially stable, so they run exactly as before: clear the autostop and connect servers on focus, disconnect on blur.This PR started as a fix for partial uploads to the remote archive when the user leaves the Exchange screen mid-sync, by keeping the websocket open from the screen. Review concluded the screen is the wrong place for that decision. The fix moves to core: digidem/comapeo-core#1353 covers Stop and autostop, and a change making
$sync.disconnectServers()itself graceful (waiting, bounded by an idle timer, for sync with the server to finish before closing the socket, withconnectServers()cancelling a pending disconnect) is stacked on it as digidem/comapeo-core#1360. With that in core, the screen's unconditional disconnect on blur is correct as it is.This PR is therefore a refactor only. On its own it changes no behaviour. The test file checks that the screen connects servers once on focus and disconnects once on blur through the hooks.