Repository navigation
fix: data-loss, sync and concurrency bugs from a QA pass - #81
Merged
Merged
Conversation
The four stat chips sat in a plain Row and overflowed by 4-45px on a 320-360px wide phone, which also made settings_overflow_test fail.
AppState inserted into and removed from whatever list the client handed back. With an unmodifiable list (as returned by the test backend stub) creating a note threw "Cannot add to an unmodifiable list", which made the local-to-backend app_state test fail.
Typing while a save was in flight was never saved: the finished save cleared the unsaved flag, so the next debounce returned early and the header showed "Saved". Saves also ran concurrently, so with a slow backend an older save could land last and put old text back. Text saves are now queued one at a time and only clear the flag when nothing was typed meanwhile. Ink saves are coalesced through a single writer for the same reason. Locally, title, text and ink saves each did read-modify-write on the whole note row, so overlapping saves wrote back stale copies of each other (text erased, rename reverted). They now update only their own column. The editor persistence test also never ran to completion: it awaited real sqflite I/O inside the widget test's fake clock and deadlocked. It now goes through tester.runAsync.
savePageText, savePageInk, deleteNote, deleteNotebook, restoreNote and configureSync ignored the response status, so a 404/413/500 from the backend (or an HTML error page from a reverse proxy) was reported as a successful save and the editor showed "Saved" for text that never reached the server. Every call now checks its expected status. Only ping had a timeout. A backend that accepts the connection and never answers left saves spinning forever and the sync button disabled. Normal requests now time out after 20s, /sync/trigger after 5 minutes. Settings now reports a rejected sync configuration instead of an unhandled exception, and does not start a sync with the old settings.
The app shell showed the startup splash whenever AppState.isLoading was true, and every notes reload (search keystroke, notebook switch, rename) sets it. HomeScreen was torn down each time: the search field lost its text and the open editor was disposed and rebuilt from the note as it was when first opened. The splash now only covers init(). On the wide layout the editor had no key, so selecting another note kept the old editor State: the panel kept showing the first note. Reloads and note opening also ignore stale results now, so slow search results for "gr" can't replace those for "groc", and a slow load can't override a newer selection. The active search query is kept across reloads, and a note that fails to open shows an error instead of the tap doing nothing.
The editor's trash button deleted the note on a single tap, while the list swipe asks first, and the app has no trash view to restore from. It now uses the same confirmation and reports a failed delete.
When a local-mode user connected a backend, SyncService.pushLocal only created empty notes from the titles: the text and drawing never reached the server, and in connected mode the app then showed those empty copies. It also sent the local notebook id, which the server rejects with a 404, pushed notes that had been deleted locally, and never marked anything as pushed, so every later sync duplicated notes and notebooks again. pushLocal now re-keys pushed notebooks to the server id, uploads text and ink after creating a note, skips notes deleted before they were pushed, and marks pushed notes synced with their remote id. A push interrupted after the create resumes the upload instead of creating the note again. The upload ran only on the first connect after local mode and swallowed failures, so one network error meant offline notes were never uploaded. It now retries on every connect while something is pending and reports failures through syncError.
The text, ink, update, delete and restore routes read the whole note, change one part and save it back. FastAPI runs them on a thread pool, so the app's text autosave and ink save for one note could overlap and the slower one wrote back a stale copy: strokes vanished after a text save, or a rename was reverted by an autosave. Those routes now take a per-note lock; writes to different notes still run in parallel.
Several ways a sync lost data, all reproduced against a live WsgiDAV server in test_webdav_sync_safety.py: - Remote pages were rebuilt with updated_at=now(), so the merge always preferred the server text and a newer offline edit was reverted. - The conflict resolver stamps its winner SYNCED, so a local edit that won (or was merged) was never pushed and later got overwritten. - A failed PROPFIND (401/5xx/HTML body/timeout) was read as an empty server: pull was skipped and push overwrote newer remote notes while the report said success. Listing failures now abort the sync, and a notebook that can't be listed is not pushed into. This also makes a network drop after ping fail in a few requests instead of retrying the whole chain for every note. - A failed page_N.ink download was read as "no strokes" and the pulled note replaced the local drawing. - note.json carries no notebook or archived flag, so every pull dropped them from the local note. - After an upload, the snapshot was re-saved as SYNCED, reverting and hiding an edit made meanwhile. - The WebDAV server stamped its own clock on every PUT, so an uploaded note always looked newer than an edit made right after the upload. It now keeps the timestamps the client sends. The reliability test fixture had a page edited after its own note; it only passed because remote pages used to be stamped "now".
POST /sync/trigger built a fresh engine per request with no lock, so two quick taps on Sync (or a manual sync during an automatic one) ran two sessions at once. They resolved the same conflicts twice and wrote duplicate conflict copies. The engine now takes a non-blocking lock per data directory and reports "sync already running" to the second caller.
When note.json arrived with an id different from the note behind the path, the provider assumed that note was an MKCOL placeholder and deleted it permanently. But the path can also resolve to a real note through the title-slug or id-prefix fallbacks, so another client writing to a colliding path destroyed it, drawings included. Only an empty placeholder minted from the same id prefix is replaced now; anything else gets a 409.
Only OSError was caught while listing notes and notebooks, so one file with invalid UTF-8, a malformed frontmatter value or an id-less plain sidecar made GET /notes, /notebooks, /stats and the WebDAV listing (and so every sync) fail with a 500. Those files are now logged and skipped, and left untouched on disk. Timestamps without a timezone (hand edits, YAML dates) are read as UTC instead of being skipped: mixing them with aware datetimes broke the sort of the whole notes list.
Single-page notes are written without page markers, but the reader split the body on any `<!-- nexanote:page N -->` line it found. A note whose text contained that line came back empty, in both stores. The body is only split now when the metadata lists more than one page.
The stem decoded from a synthetic md.<base64> id was joined into
notes_dir as-is, so an id built from "../../x" let GET and PUT on
/notes/{id} read and write .md files anywhere the backend can reach.
The API has no auth and allows any origin, so a web page could do it.
Such stems are now rejected and the id resolves to nothing (404).
POST /notes with an unknown note_type crashed in NoteType(...) with a 500. /sync/configure accepted any conflict_strategy, saved it to disk, and every later /sync/trigger then failed with a 500 until the user reconfigured sync. Both fields are typed as their enums now, so bad values get a 422 and nothing is saved.
The canvas fed every pointer into the same current stroke: - A palm or finger touching down while writing with the stylus started a new stroke over the one in progress, so the stylus stroke was lost and the palm's was saved instead. - A two-finger pinch to zoom always left a zig-zag stroke between the fingers. - Stylus plus palm counted as a two-pointer pinch and zoomed the page under the pen. - Right or middle mouse clicks drew. The canvas now tracks the one pointer that is drawing, drops the stroke when a second finger turns the touch into a pinch, and only zooms while nothing is drawing.
TheZupZup
changed the title
Text was only saved by the 2s debounce or when the editor was disposed. Switching apps right after typing, then having Android kill the backgrounded process (or closing the desktop window), lost the last edits since neither ran. The editor now saves as soon as the app stops being resumed.
On phones the notebook dialog accepted an empty or whitespace-only name and created a nameless notebook. Creating a note or notebook while the backend refused the write threw an unhandled exception and the tap silently did nothing. Names are trimmed and blank ones ignored on both layouts, and failures now show a snackbar.
"How to start the server" sat in a plain Row and overflowed by 19px on a 360px wide screen (more with a large system font).
The text save queue read the edit counter when a queued save started, not when its snapshot was taken. An edit made while the save waited behind a slow one was counted as saved when the queued save finished, so its own debounce and the dispose flush both skipped it. The counter is now captured with the snapshot.
The earlier listing fix only covered notebooks. When note.json or a page_N.ink failed to download (500, timeout, invalid body) the note was skipped by pull, then push still uploaded the local copy over a newer remote version. note.json failures were not even reported. Both reads now raise unless the file is really gone (404), and push skips any note whose pull failed. A note deleted locally was also left MODIFIED after conflict resolution, but push never sends deleted notes, so every later sync re-ran the conflict.
Since API writes started checking status codes, any 4xx went through the backend-failure path: deleting a note another device had already removed showed the offline banner and replaced the notes list with the local cache (empty for users who never synced). Only timeouts, network errors and 5xx switch to offline now, and a 404 on a delete counts as done since the item is already gone.
The upload now runs on every connect, and two quick Reconnect taps ran two uploads concurrently. Both saw the same pending notes and created each one twice on the server. Concurrent connects now share the in-flight upload.
If the create succeeded but the text upload failed, the next connect re-sent the local text to that note, overwriting anything the user had written in it online since (pull keeps pending rows, so the local copy was never refreshed). A resumed upload now checks the server copy first: still empty, the content goes in as before; already written in, the offline text is uploaded as a separate "(offline copy)" note.
A palm touching the screen before the pen became the drawing pointer, so the stylus was ignored and the palm's stroke was saved. A stylus now takes over from a touch and the touch stroke is dropped.
Notes reloads keep the active search query, but only the wide layout has a search field. After narrowing the window or rotating a tablet the list stayed filtered with no way to see or clear the filter, and a recreated search field started empty while still filtering. The phone layout now shows the active filter with a clear button, and the search field starts from the current query.
The 20s request timeout also applied to savePageInk and savePageText, which send the whole page. A long drawing on a slow mobile uplink could time out on every attempt and never be saved. Saves now get 2 minutes; other requests keep 20s.
Uploading offline notes re-keys their notebooks to the server ids, but the selected notebook kept its local id, so the next note created in it was sent with an unknown notebook_id and rejected. The selection is cleared when the notebook isn't on the backend.
When an offline note's create reached the server but its content upload failed, the next push treated an empty remote note as our placeholder and wrote the old local content into it. A note someone had edited online and left empty on purpose was overwritten that way. The server's updated_at at create time is now stored with the remote id (schema v3, remote_baseline). An upload is only resumed into the remote note if it is still empty and its updated_at is unchanged; every server write moves it. Otherwise, including when there is no usable baseline, the local version is uploaded as a separate "(offline copy)" note and the remote one is left alone.
The provider treated any note that was empty and shared the payload's 8-char id prefix as an MKCOL placeholder: it deleted it and took over the payload id. A real empty note with a colliding prefix was replaced that way. Placeholders are now recorded when MKCOL (or a PUT into a missing path) creates them, in .webdav_placeholders.json in the data directory so the claim survives a restart, and only a recorded id can be replaced. Everything else gets a 409. The claim no longer depends on the placeholder being empty either, so a drawing PUT before note.json is kept instead of blocking the note with a 409 forever. The placeholder is deleted only after the note is saved under its real id, and when the client's id already exists on the server (the note moved notebooks), that note is updated in place and keeps its drawing instead of being overwritten by the placeholder's empty pages.
The ink writer took the pending snapshot before saving it, so when the last save failed and nothing else was drawn, nothing retried it: the drawing stayed on screen for the session but was never stored. The failed snapshot now stays pending (unless a newer drawing replaced it meanwhile, which is tried instead and never overtaken by the older one) and is retried with backoff, when the app leaves the foreground, and when the editor is closed. After the editor is gone, retries stop once the backoff list is exhausted. Closing the desktop window now waits up to 5s for pending text and ink saves instead of exiting with them in flight.
flutter analyze exited 1 on main and on this branch (82 findings), so it could only run as `|| true`. This clears all of them so the new CI job can fail on any new finding: - Deprecated APIs replaced by their direct equivalents: withOpacity -> withValues(alpha:), Color.value -> toARGB32(), Matrix4 translate/scale -> translateByDouble/scaleByDouble, Switch activeColor -> activeThumbColor. - `path` was imported without being declared; it is now a direct dependency. pubspec.lock regenerated by `flutter pub get` (the other entries are the SDK-pinned packages of current stable). - Two Settings methods read context after an await without checking mounted, and the Reconnect snackbar checked the State instead of the builder context. - The rest is mechanical (const, unused imports, test local names). - analysis_options.yaml: the build/platform excludes flutter analyze adds on its own.
Only the Python backend was checked on pull requests; the Flutter client was analyzed and tested in the APK release job with `|| true`, so a failing test never turned anything red. A separate Flutter app workflow now runs flutter pub get, flutter analyze and flutter test on PRs and pushes to main, on the same stable channel as the release build, and fails on any analyzer finding or test failure.
A recorded MKCOL placeholder is a normal note in the app until its note.json arrives. If someone wrote in it meanwhile, the claim replaced that text with the payload's. A placeholder with text is no longer claimable (409); strokes are still fine since the claim keeps them.
Failed ink saves keep retrying after the editor is closed. Reopening the note started from the stored copy (without the unsaved stroke), and the old editor's retry could then land after the new editor's save, putting an older drawing back. AppState now tracks the newest unsaved drawing per note: a reopened editor shows it and takes over saving it, and a save whose snapshot has been superseded is skipped.
Follow-up to the placeholder registry, from a second review pass: - A drawing made in a recorded placeholder through the app was lost: the claim no longer required an empty placeholder, and the client's page ink PUT right after it replaced the strokes. The registry now records the strokes the client itself sent into each placeholder, and a placeholder holding any other stroke (or text) can't be claimed. - A page ink PUT addressed to another note (the client sends note_id) overwrote whatever note the path resolved to through the slug/prefix fallbacks, even after note.json had been refused. It now gets a 409, except for the client's own claimable placeholder. - Ink the client sent into the placeholder before note.json was dropped when the client's note already existed; it now replaces that page's strokes on the existing note, as the same PUT would have. - In the plain store a claimed note ended up as "Doc (2).md" because the placeholder's file was deleted after the save. Since the placeholder only holds what the client sent, it is deleted first again.
When the remote note had changed, pushLocal created an "(offline copy)" note but never recorded it. If the copy's content upload failed, every later push created another empty copy. The copy now becomes the row's remote note (with its own baseline) before the upload, so a retry resumes into it under the same rules.
PUT /notes/{id}/pages/{n}/text and /ink now include note_updated_at, the
note's version right after the write (taken under the per-note lock).
The app uses it to tell its own partial uploads apart from someone
else's edits when it resumes an interrupted upload. Older clients ignore
the extra field.
The note.json claim deleted the placeholder before the new note was saved, and parsed the payload while already mutating it. An invalid page entry or a storage error halfway through lost the placeholder and any ink the client had already sent into it. The body is now validated first (400 on bad types or page numbers), the claimed note is built on a copy, saved, and only then is the placeholder deleted and removed from the registry. A failed delete keeps the registry entry so the placeholder stays claimable. The plain store re-saves the claimed note afterwards so it gets the placeholder's file name back. The registry now stores a fingerprint of each placeholder's state (title, type, tags, flags, notebook, pages, strokes) instead of only checking text and strokes. Any edit made to a placeholder through the app makes it unclaimable (409). Ink the client sends into its own untouched placeholder updates the fingerprint, so that flow keeps working.
Page saves now return the note's updated_at, and every successful content write moves the row's remote_baseline to it. A resume only needs the remote to be exactly as our last write left it, so a text upload followed by a failed drawing upload finishes in place instead of creating an offline copy. A copy title never gets the suffix twice.
In connected mode the newest title, text and drawing only lived in the editor and in AppState memory until the server answered, so a killed process, a desktop close with a stuck backend or a crash lost them. Edits now go to the local store first in every mode. Each part (title, text, ink) has a revision; the backend push reads the newest local version and only marks that revision synced, so a late answer for an older save can't mark a newer edit as sent. Pending edits are shown when the note is reopened (also from a new process, also offline) and are pushed on connect and sync. Pulls decide against the row as it is when written, so they never revert a pending edit. The ink tickets stay as the guard against older snapshots. Without a usable local store (the database can't be opened), saves keep going straight to the backend as before.
A resume can now write into a note that already holds our earlier upload, so skipping empty parts left text or strokes the user had removed on the server while the row was marked synced.
Rows created by a pull hold a note's metadata but not its text or drawing (has_content = 0). Text or ink typed over such a row in local mode was pushed as the note's content and wiped the server copy. Those blind edits are now split off into an "(offline copy)" note, and such a row no longer opens offline as an empty page. Also from review: - a half-uploaded row is the local copy of its remote note, so opening that note no longer creates a second row and steals its strokes; stroke ids shared by two notes are stored per note - pre-v4 local edits kept as `modified` are marked pending by the migration instead of being overwritten by the next refresh - pending edits are pushed when the backend comes back and on Sync, and a part the server refuses doesn't hold back the others - a delete resolves the local row before the request, so a push failing on that note can't turn it into a note to upload again
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 65229d0735
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+1046
to
+1047
| if untouched_placeholder: | ||
| _placeholders(self.db).record(self.note) |
There was a problem hiding this comment.
When a placeholder is edited through page_N.ink, this unconditionally records a new fingerprint whenever it matched before the write, including for a generic WebDAV PUT with no note_id or with the placeholder's temporary ID. That makes the user's edited drawing look like trusted pre-uploaded client ink, so a later note.json PUT whose ID merely shares the placeholder prefix can still claim the placeholder and delete that drawing. Only refresh the registry when the ink payload identifies the actual note that will claim the placeholder; otherwise the edit must make the placeholder unclaimable.
Useful? React with 👍 / 👎.
Comment on lines
+787
to
+790
| if page_data.get("updated_at"): | ||
| page.updated_at = datetime.fromisoformat(page_data["updated_at"]) | ||
| else: | ||
| page.updated_at = note.updated_at |
There was a problem hiding this comment.
With the bundled WebDAV server, NoteMetaFile._serialize still omits updated_at from every page, so this fallback is always used and assigns the note-level timestamp to its text page. If device A edits text and device B later changes only the title or ink, B's note timestamp makes its older text appear newer than A's text; _merge_strokes then keeps that stale remote text and overwrites A's edit. Include each page's stored timestamp in note.json instead of collapsing it to the timestamp of an unrelated note-level change.
Useful? React with 👍 / 👎.
Comment on lines
105
to
+109
| if (_isAlreadySynced(note.syncStatus)) continue; | ||
| // Already adopted from a remote .md file — never re-create on the | ||
| // server. The non-idempotent createNote endpoint would mint a fresh | ||
| // UUID and a duplicate file alongside the existing one. | ||
| if (note.remoteId != null && note.remoteId!.isNotEmpty) continue; | ||
| await _api.createNote( | ||
| title: cleanRemoteTitle(note.title), | ||
| noteType: note.noteType, | ||
| notebookId: note.notebookId, | ||
| ); | ||
| notes++; | ||
| final pendingUpload = note.syncStatus == 'local_only'; | ||
| final remoteId = note.remoteId; | ||
| final hasRemote = remoteId != null && remoteId.isNotEmpty; | ||
| if (hasRemote && !pendingUpload) continue; |
There was a problem hiding this comment.
A note deleted in local mode is marked modified while retaining its remoteId, so this condition skips it on every reconnect because only local_only rows are considered resumable. The later pull also preserves the newer modified local row, leaving the note hidden on this device but permanently present on the server and other devices. Handle isDeleted linked rows by calling the remote delete endpoint (and then removing or acknowledging the local tombstone) before applying this skip.
Useful? React with 👍 / 👎.
Comment on lines
+441
to
445
| # Only a 404 means the note is gone; any other failure must not be | ||
| # mistaken for it, or push would overwrite the version we failed | ||
| # to read. | ||
| if resp is not None and resp.status_code == 404: | ||
| return None |
There was a problem hiding this comment.
A 404 here is treated as if the remote note disappeared, but _pull_note reached this call only after PROPFIND listed the note directory. This state occurs during an interrupted upload where the directory may already contain a newer page_N.ink; returning None prevents the prefix from being marked unreadable, so the push phase can write local note.json and ink into that same directory without ever comparing the remote content. Raise RemoteReadError for a missing note.json in an enumerated note so that note's push is skipped.
Useful? React with 👍 / 👎.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Fixes from a QA pass over the Flutter client and the Python backend, mostly bugs that lose or revert notes. Every commit comes with regression tests that fail before the fix.
Local-first saves (Flutter)
Every title, text and ink edit is now written to SQLite before anything goes to the network, in connected mode too (schema v4: per-part revisions). The push reads the newest local version and only marks the revision it sent as synced, so a late answer for an older save can't mark a newer edit as sent. Pending edits survive a killed process, an Android background kill and a desktop close with a stuck backend. They show when the note is reopened (also offline) and are pushed on connect, on Sync and when the backend comes back. Pulls never revert a pending edit. Text or ink typed in local mode over a note whose content was never downloaded goes to an "(offline copy)" note instead of wiping the server copy. Restart tests run a second AppState on the same database file. The ink tickets and the serialized text queue are kept.
Linux desktop can't open the local store today (#82). There, connected saves still go straight to the backend as before.
Other client fixes
updated_atis still the one our last write left (remote_baseline). Otherwise the offline version goes to one "(offline copy)" note, never several.Backend
note_updated_at..webdav_placeholders.jsonand still matching its fingerprint (any edit to title, tags, pin, type, notebook, text or strokes disqualifies it) can be claimed. The payload is validated and the note saved before the placeholder is removed./notesand sync. Page-marker lines in text are kept. Path traversal throughmd.ids is blocked.CI
New
Flutter appworkflow:flutter pub get,flutter analyze,flutter test, nothing soft-failing. 276 Flutter tests, 474 Python tests.Known limitations: edits pushed from connected mode are last-writer-wins against changes made on another device meanwhile (as before). Deletions don't propagate through WebDAV sync. The APK release job still runs analyze/test with
|| true.