Feature/offline sync conflict ux - #22

This clears the entire conflicts array unconditionally, but only resolves currentConflict (activeConflicts[0]). If getActiveConflicts() ever returns more than one (multiple diverged fields/entities), resolving the first one silently discards the rest without ever surfacing them to the user. Should remove just the resolved conflict from the array, not clear all of them.

Everything else in LocalStore goes through the SQLite adapter, but conflicts are a plain field on the instance. So a refresh, a crash, or closing the tab while the conflict modal is open drops every pending conflict, and the local edits that caused them stay in the database still diverged from the server. The user is left with no indication anything needs resolving.

This wants a conflicts table alongside the others, written inside the same transaction as applyRemoteOps so a conflict cannot be detected and then lost. getActiveConflicts becomes async, which the two call sites in App.tsx are already positioned to handle since they sit in async functions.

This returns the live internal array. App.tsx passes the result straight into setActiveConflicts, so React state and this.activeConflicts are the same object, and line 606 then pushes into it. React compares by reference, sees no change, and does not re-render, so a conflict detected during an active sync silently never reaches the UI even though the array grew.

return [...this.activeConflicts] fixes the immediate problem. resolveConflict already reassigns rather than mutating, which is why resolution appears to work and detection does not.

The condition mixes two unrelated checks, and the else catches both. !locallyModifiedFields.has(key) failing means a genuine conflict. tableAllowed.has(snakeKey) failing means the remote sent a column this table does not have, which is a schema or version mismatch, not something the user can resolve by picking a side.

As written, an older client receiving an op with a new field shows a conflict dialog offering a choice between two values for a field it cannot store either way. Please split them:

if (!tableAllowed.has(snakeKey)) continue; // or log it
if (!locallyModifiedFields.has(key)) { /* apply */ }
else { /* conflict */ }

This replaces the global fetch for the whole application, so every request, not only sync, throws when the simulated switch is off. Anything the app fetches for unrelated reasons fails in a way that looks like a network error, which will be confusing to debug.

It is also fragile as a component effect: the cleanup restores whatever originalFetch captured, so if anything else patches fetch in between, or this effect ever runs twice without its cleanup, the app is left with a stale or doubly wrapped implementation.

Since HttpSyncClient is constructed right here on line 35, it would be cleaner to give it an injectable fetch, or an offline flag the client checks, and leave the global alone. That keeps the simulation scoped to the thing being simulated.

This SELECT * sits inside the loop over payload keys, so an op touching five conflicting fields queries the same row five times, and it re-reads state that the surrounding transaction may already have. Hoisting it above the loop, or reusing the row the caller already loaded to build locallyModifiedFields, gives you one read per entity.

Minor: this shadows the snakeKey declared on line 584 with an identical value. Harmless today, but it is the kind of thing that turns into a real bug the moment someone changes one of the two. Just use the outer one.

Also on line 607, substr is deprecated; slice(2, 11) does the same job. If conflict ids need to be stable across reloads once they are persisted, a deterministic id built from entity, field and remote value would serve better than a timestamp and random suffix, and would make the duplicate check on line 599 unnecessary.

A hardcoded localhost URL means any build other than a developer's own points at nothing. import.meta.env.VITE_API_BASE_URL with this as the fallback keeps local development working and makes the deployed build configurable.

Minor: a module-level mutable listener plus module-level syncClient and syncManager means only one component can ever subscribe, and the listener is null whenever no component is mounted, so status changes during that window are dropped. If SyncManager exposed subscribe and unsubscribe returning an unsubscribe function, the effect could use it directly and this variable would go away.

Minor, and I realise a demo trigger is useful while the backend conflict path is still settling. Two things worth tidying if it stays: the store.updateDocument(doc.id, { title: doc.title }) on the next lines is a write of the value onto itself purely to mark the field dirty, which will confuse anyone reading it without a comment saying so. And sequence: 999 on line 184 is a magic number that will collide with real sequences once the server is producing them.

Ideally this button and its handler live behind a development-only flag so they do not ship in a production build.