GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Feature/offline sync conflict ux by priyanshuvishwakarma273403 · Pull Request #22 · docsgraph/docsgraph Conversation
Collaborator
OverviewThis PR designs and implements interactive connection status indicators, a dynamic offline editing flow, and a conflict resolution overlay modal that supports field-level Last-Write-Wins (LWW) and offline edit preservation without data loss. Key Changes1. Connection & Sync Status UI (
| |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| await loadData(); | ||
| } | ||
|
|
||
| store.clearConflicts(); |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Replacing the fake setTimeout conflict with a real one detected in applyRemoteOps is a solid step, and moving the Conflict type into the data package so the app stops redeclaring it inline is the right direction. The stray Tailwind classes that never existed (bg-slate-805, text-slate-405, hover:bg-slate-750) being cleaned up is a nice bonus.
A few things I would like addressed:
Blocking:
LocalStore, so reloading the page during a conflict loses it while the divergent local edit stays. For a feature about surviving offline, that is the case that matters most.getActiveConflicts hands back the internal array by reference and applyRemoteOps pushes into it, so React holds the same array it mutates and will not re-render for new conflicts.else branch treats "field was edited locally" and "field is not in the column allowlist" as the same thing, so an unrecognized remote field pops a conflict dialog at the user.window.fetch from a component patches every request the app makes, not just sync.Minor:
baseUrl is hardcoded to localhost.snakeKey, and substr is deprecated.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| } | ||
|
|
||
| export class LocalStore { | ||
| private activeConflicts: Conflict[] = []; |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
|
||
| constructor(private adapter: SqliteAdapter) {} | ||
|
|
||
| getActiveConflicts(): Conflict[] { |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| @@ -563,6 +585,34 @@ export class LocalStore { | |||
| if (!locallyModifiedFields.has(key) && tableAllowed.has(snakeKey)) { | |||
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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 */ }
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| // Intercept window.fetch to simulate offline behavior and direct sync calls to backend | ||
| useEffect(() => { | ||
| const originalFetch = window.fetch; | ||
| window.fetch = async (input: RequestInfo | URL, init?: RequestInit) => { |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| updateKeys.push(`${snakeKey} = ?`); | ||
| updateValues.push(remotePayload[key] as SqlParam); | ||
| } else { | ||
| const entityRows = await tx.query( |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| [op.entityId] | ||
| ); | ||
| const currentEntity = entityRows[0]; | ||
| const snakeKey = this.camelToSnake(key); |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| let syncStatusListener: ((status: SyncStatus) => void) | null = null; | ||
|
|
||
| const syncClient = new HttpSyncClient({ | ||
| baseUrl: 'http://localhost:8000', |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| const adapter = new InMemorySqliteAdapter(); | ||
| const store = new LocalStore(adapter); | ||
|
|
||
| let syncStatusListener: ((status: SyncStatus) => void) | null = null; |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| // Trigger Mock Sync Conflict button | ||
| const handleTriggerMockConflict = () => { | ||
| // Trigger Mock Sync Conflict by applying a simulated remote divergent operation | ||
| const handleTriggerMockConflict = async () => { |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Requested changes must be addressed to merge this pull request.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.