feat: soil change tracking #2364
Conversation
There was a problem hiding this comment.
LGTM!
a few take it or leave it comments mostly about documentation/code organization. i feel most strongly about the one asking for a write up how the data flow is supposed to work, but i might be misremembering what scenarios this code is supposed to handle in which case it's less relevant
dev-client/src/store/persistence.ts
Outdated
| export const patchPersistedReduxState = (state?: Partial<AppState>) => { | ||
| if (state) { | ||
| merge(state, {soilId: {soilChanges: {}}}); | ||
| } | ||
| }; |
There was a problem hiding this comment.
suggestions:
- should we use this as part of the implementation of
loadPersistedReduxStaterather than exporting and relying on clients to remember to call it? - should we use immutable logic here instead of mutating the state object? (i think if you import
mergefrom'lodash/fp'instead of'lodash'you will get this behavior)
There was a problem hiding this comment.
- I don't think that's necessary. I'd rather be able to test the load/save functionality in isolation and also test this in isolation. I don't think the issue of clients "remembering" to call this method is substantial since there is only one client (the app at startup).
- Good idea, I'll make that change.
| export const markAllChanged = <T>( | ||
| records: ChangeRecords<T>, |
There was a problem hiding this comment.
question: thoughts on using Draft<ChangeRecords<T>>? implicitly that is what is happening when this method gets called via the soil ID slice, so it seems nice to be explicit about it (since it would probably be a bug if we were using this method on something that is not a draft). i'm not sure whether that would make the test code more awkward though, and don't feel too strongly about it
There was a problem hiding this comment.
I understand the principle of being more explicit about when we're operating on intermediate Redux state vs. immutable data. I'm hesitant to engineer a potentially-complex approach to that right now (as you note it could have consequences for testing) but in a subsequent PR I'd be open to it, once we fully understand how this code is used in a well-tested push implementation.
dev-client/src/model/sync/sync.ts
Outdated
| export const markChanged = <T>( | ||
| records: ChangeRecords<T>, | ||
| id: string, | ||
| at: ChangeTimestamp, | ||
| ) => { | ||
| const prevRecord = getChange(records, id); | ||
| const prevRevisionId = getRevisionId(prevRecord); | ||
| const revisionId = nextRevisionId(prevRevisionId); | ||
| records[id] = { | ||
| revisionId: revisionId, | ||
| lastModifiedAt: at, | ||
|
|
||
| lastSyncedRevisionId: prevRecord?.lastSyncedRevisionId, | ||
| lastSyncedData: prevRecord?.lastSyncedData, | ||
| lastSyncedAt: prevRecord?.lastSyncedAt, | ||
| }; | ||
| }; |
There was a problem hiding this comment.
question: am i remembering right that this version is meant to handle cases where the client makes further changes while a push or pull is in flight? i'm having trouble remembering the logic of how/in what order these methods are going to be called to handle that scenario (or am maybe misremembering whether this version is supposed to handle that scenario) and am wondering if you could write it up in a comment somewhere in this file
There was a problem hiding this comment.
(Assuming that by "this version" you mean the revision ID)
So there is already a comment but I can rewrite it for better clarity. Yes, the intended use of revision IDs is to handle situations where data gets changed while a sync is in progress so that we do not overwrite user changes with the results of a stale operation.
…s; improve comment explaining revision ID
Description
Add a data model to track changes relevant to the sync system. Entities have change records containing a revision ID and information about the last change and last successful sync. The Soil ID slice tracks changes for soil data by updating the change records when mutations occur and marking them as synced when user data is loaded from the server. The number of un-synced sites is exposed through the sync button component for diagnostic purposes.
Also adds a basic system for patching data loaded from disk during startup -- ultimately we will want this to work by merging the slices' default initial states with the states saved to mmkv, however circular app dependencies prevented this and so we should revisit it as follow-on work once the main offline feature is done.
Checklist
Related Issues
Fixes #2278
Verification steps
Make changes to soil data and observe that the number of unsynced sites changes appropriately. Reload the user's data and observe that changes are marked as synced.