mirror of
https://github.com/johannesjo/super-productivity.git
synced 2026-07-17 16:37:43 +00:00
85 commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Johannes Millan
|
99f923ad6c
|
docs(sync): fix stale schema-compat docs, add bump policy, rescope Task 6 (#9119)
* docs(sync): fix stale schema docs, add bump policy, rescope Task 6 * docs(sync): add severity-triage and schema-bump rules to AGENTS.md * docs(sync): fix review findings in schema compat docs |
||
|
Johannes Millan
|
fdbc2c1a86
|
refactor(sync): centralize clock pruning in store, make merge atomic (#9107)
Follow-up to
|
||
|
Johannes Millan
|
fcbb789746
|
docs(sync): record #9105 staleness-eviction plan, improve prune snack (#9106) | ||
|
Johannes Millan
|
a224eb5fa3
|
fix(sync): keep import author in client-side clock pruning (#9096) (#9102)
The client pruned its durable vector clock with uploader-only protection, so once 21+ client ids accumulated after a full-state import, the import author's low-counter entry was evicted. Every subsequent local op then permanently failed the sync-import filter's knows-import-counter rescue and was dropped as CONCURRENT on every peer — the client-side ceiling of the server fix in #9089. - client limitVectorClockSize wrapper now takes a preserve list, matching the shared implementation - calculateRemoteClockMerge (remote merge + reducer checkpoint) preserves the latest full-state author; an in-batch full-state op supersedes the stored one, and the checkpoint resolves the author inside its transaction so a just-rejected import cannot name the protected author - snapshot save, compaction, hydrator restore, and the sync-hydration file-snapshot bootstrap protect the author on their durable-clock paths - docs: add the missing calculateRemoteClockMerge prune site, correct the stale RepairOperationService rows (repair ships the full clock), and reword the sync-core pruning comment to the real invariant |
||
|
Johannes Millan
|
b50d5f6d96
|
fix(sync): dedupe surgical-sync retries and keep the import author through pruning (#9089)
* fix(sync): deduplicate surgical sync retries Persist split-file configuration and acknowledge operation IDs already committed remotely after a lost upload response. * fix(sync): preserve import author during clock pruning Keep the active causal full-state author in oversized stored clocks and reuse the stored protected IDs when classifying response-loss retries. * test(sync): harden supersync failure coverage Exercise real upload endpoints and exact operation IDs across response loss, validation failures, schema blockers, full-state boundaries, vector pruning, and concurrent edits. * fix(sync): heal a corrupt primary on duplicate-only uploads The .bak recovery path caches the CORRUPT primary's rev precisely so this cycle's conditional overwrite repairs sync-ops.json. The duplicate-retry short-circuit read that cache and returned before the write, leaving the primary corrupt whenever the recovered buffer already held every pending op. Flag the recovered entry and let those uploads fall through. Also stop synthesising a serverSeq for ops already in the buffer: the field is optional, the upload and download paths number ops differently, and a mixed batch could hand two ops the same value. * perf(sync): look the full-state author up once per upload Batch upload is off by default, so the guarded batch path was not the one serving production: the serial path queries the causal full-state author per op inside a single transaction, and a clock of 21-50 entries passes validation and trips the guard on every one of them. Memoize the author per transaction and resolve it lazily, so only an op whose clock actually overflows pays, and only once. This also retires the batch pre-scan and its loop-carried author, leaving both paths on one mechanism. Report the lookup through ProcessOperationResult so the upload summary stops under-reporting round-trips, and record why reconstructing the stored protected set loosens id-collision detection. * test(sync): wait for the committed title in renameTask renameTask blurred the textarea, slept 300ms and returned without ever checking the rename landed. Blur -> dispatch -> re-render outruns that delay on a loaded machine, so a following sync uploads without the rename op and the caller asserts against a task that was never renamed — which is what supersync 3.1 hits on CI but never locally. Wait for the new title instead, mirroring markTaskDone's done-state wait and the e2e no-waitForTimeout rule. * test(sync): dispatch focus so renameTask actually commits renameTask relied on el.focus() to emit a focus event, but these tests drive two clients as separate pages and only one page can hold focus, so on CI the event often never fires. TaskTitleComponent then keeps _isFocused=false, and resetToLastExternalValueTrigger resets tmpValue to the stored title on the next task-object emission. Blur therefore computes wasChanged=false, task.component skips update(), and the rename is silently dropped without ever becoming an op. That is supersync 3.1: client A's rename lives only in tmpValue, A syncs and uploads nothing, B uploads its done op, A downloads it, the task ref changes and the title reverts to the original — exactly the state the CI artifact captured. A real user always has real focus, so the app itself is unaffected. Dispatch focus explicitly, mirroring the synthetic input/blur already used here. Also correct the previous commit's claim: the toBeVisible wait matches tmpValue, a component-local signal rendered in both template branches, so it never observed the committed title and could not have fixed this. * docs: revert incidental prettier reformat of unrelated docs The master merge ran prettier across files it pulled in, reformatting three documents this branch has no business touching: markdown table cell padding plus *emphasis* -> _emphasis_, with no content change. handover.md documents two unrelated branches entirely. Restores them to master. vector-clocks.md keeps its edits — those are this branch's own and describe the pruning protection. * refactor(sync): drop the full-state author lookup's roundtrip accounting resolveFullStateAuthor memoizes per transaction, so the lookup it counts fires at most once per upload — the plumbing existed to report a number that is always 0 or 1, on a log line already counting dozens. The memo and its own accounting cancelled out. Removing didQuery lets resolveFullStateAuthor return string | undefined and getPruneProtectedIds return string[], instead of both carrying a tuple purely to feed the counter. uploadDbRoundtrips and the batch path's own counter are untouched. * test(sync): assert the committed store title in renameTask The focus dispatch did not fix supersync 3.1 — the shard failed again with an identical snapshot (original title, done, rename gone), so that diagnosis was wrong. Stop guessing at the trigger and make the helper able to observe the thing in question. task-title renders tmpValue, a component-local signal, in BOTH its editing and idle branches. Every DOM assertion here therefore matches as soon as the synthetic input event fires, whether or not an op was ever captured — which is why two rounds of "wait for the title" changed nothing. Read the store instead, via the __e2eTestHelpers.store hook the timeSpent helper already uses. This is a diagnostic as much as a fix: it splits the two remaining explanations. If renameTask now fails, the rename never becomes an op and the bug is in how the test drives the edit. If it passes and 3.1 still fails at the merge assertion, the op is captured and lost during sync — a real defect, and the test is right to fail. * test(sync): move the 3.1 disjoint-merge rewrite out to #9095 3.1 was the last red shard, and it turned out to be right: the store-backed renameTask passes, so the rename IS captured as an op, and the test still fails at the merge assertion — the op is committed and then lost during sync. Filed as #9095. That bug is pre-existing and cannot be reached by anything in this PR: the file-based adapter is not used by SuperSync, and the server-side author memo only engages for clocks over 20 entries where this test carries about three. 3.1 is also the only test here that exercises neither of this PR's fixes — it races a title change against a move-to-done, which is conflict resolution, not retry dedup or clock pruning. So it moves to #9095 rather than holding verified sync fixes red. The rest of the hardening stays: the fault injections whose globs never matched a real endpoint, the schema-mismatch test that asserted nothing, and the compaction suite that called an endpoint which never existed are what actually cover the fixes here. Restoring the old 3.1 puts a misleading test back, so it now carries a comment saying why it proves little and where the real one lives. The strengthened version is kept on test/issue-9095-disjoint-merge-repro. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> |
||
|
Johannes Millan
|
0238b5a729
|
fix(sync): fence in-flight sync cycles across destructive config changes (#9088)
* fix(sync): fence in-flight sync cycles across config changes (#9074) Destructive config changes (encryption enable/disable/password change, provider/account switch) blocked NEW sync cycles but neither drained nor cancelled a cycle already mid-await, which could then apply remote ops, upload, acknowledge, or advance the cursor against the new epoch/target (cross-epoch/cross-provider contamination, mixed-key server history). Fix, per the issue's KISS sketch: - Monotonic sync epoch on SyncProviderManager, bumped AFTER each provider switch / target-moving config write / bypass ingress, and at runWithSyncBlocked entry. Content-only saves do not bump. - Every cycle entry point (main sync, immediate upload, WS download, force upload) captures the epoch with its cycle-guard claim and threads it as fenceEpoch. - Provider I/O is fenced in one choke point: getOperationSyncCapable() returns a per-cycle delegate that re-asserts the epoch before every provider call (uploads, downloads, all setLastServerSeq cursor writes). Local writes (apply-lock closures incl. the full-state path, deferred acks, hydration, migration appends, rejected-ops handling, raw-rebuild resume) re-assert at the call site. - Stale completions throw SyncEpochChangedError, handled everywhere as a benign abort (no error snack; UNKNOWN_OR_CHANGED) — each abort point lands in a crash-equivalent state by design. - runWithSyncBlocked is serialized, sets the block flag FIRST, bumps the epoch, then drains the main sync AND the cycle-guard side channels (bounded, throws on timeout) — the fence cannot recall request bytes already on the wire, so destructive remote writes wait for the stale cycle to settle. ImmediateUploadService now routes through WrappedProviderService instead of a raw cast. Closes the audit findings C1-1 (ORCH-1) and C1-2 (SWITCH-2). * fix(sync): don't bump the sync epoch on first-time setup (#9074) Every conflict-dialog E2E timed out on `SyncEpochChangedError (1 → 2)`: the first-ever provider activation and the first-ever config save (no previous privateCfg, so isSyncTargetChanged reports a target change) both bumped the epoch, racing the fresh config's first sync into a spurious abort — the dialog-producing cycle itself was fenced. First-time setup has no OLD target an in-flight cycle could be running against (a pre-activation cycle sees getActiveProvider() === null and exits; a pre-first-save cycle is not ready), so these bumps fence nothing and only cause false positives. Gate them: bump on a config save only when a previous config existed, and on activation only when a previous provider was active. Real switches (X→Y, X→null) and real target moves still bump; the cache-invalidation emission keeps its true-on-first-save semantics untouched. * fix(sync): read the (provider, epoch) fence pair in one sync block (#9074) The SuperSync provider-switch E2E still aborted its first post-switch sync: the cycle captured the fence epoch at its guard claim but fetched the provider object several awaits later, so a switch completing in between handed the cycle the NEW provider with a STALE epoch — the fence then aborted a cycle that was actually running against the new target. The consistency rule is pair atomicity, not claim atomicity: a switch swaps the provider object and bumps the epoch in one synchronous block, so reading getActiveProvider() and syncEpoch in one synchronous block always yields a consistent pair (old+old aborts on the later bump; new+new proceeds). Move the capture to the provider read in the main sync and WS-download cycles (immediate upload aligned for uniformity — it was same-block already only by accident of having no await between). |
||
|
Johannes Millan
|
d1ff7963d0
|
fix(sync): rebase repair op clocks on the durable clock (#8939) (#9080)
The REPAIR paths built their vector clock from the per-tab in-memory clock cache and then REPLACED the durable clock with it. A stale cache (another tab advanced the clock) regressed the durable clock, letting subsequent captures reuse counters already shipped — silently corrupting cross-device dominance comparisons. - createRepairOperation: route through appendMixedSourceBatchSkipDuplicates; the in-transaction rebase makes regression unrepresentable. State cache stores the clock actually written. - replaceRejectedRepair: rebase the replacement clock onto the durable clock inside its transaction (shared rebaseLocalClockOnDurable helper). - Drop client-side clock pruning from repair op building: inert under the rebase, and it dropped client IDs the server still tracks (false CONCURRENT). Server prunes after conflict detection. - Rename appendWithVectorClockUpdate -> appendWithVectorClockOverwrite and document the derivation invariant; the capture path (its only remaining production caller) already satisfies it. |
||
|
Johannes Millan
|
51bf689bd5
|
fix(sync): preserve multi-entity conflict recovery (#8990)
* chore: add project-scoped Angular MCP * chore: update npm for release-age policy * fix(sync): preserve LWW outcomes across clients Distinguish replacement snapshots from partial merge operations, protect device-local sync configuration, recreate winning deletes, and resolve every conflicted entity in bulk operations. Fixes #8956 * fix(sync): harden mixed LWW conflict replay Preserve unaffected remote and local bulk intents, keep device-local sync settings during replacement replay, and gate replacement semantics behind schema v3. * fix(sync): recover subtask subtree and harden LWW replay follow-ups Follow-up hardening for #8956 after multi-agent review: - Recreate a locally-winning parent's subtasks when a remote bulk delete is a mixed winner. The full remote delete is applied (cascade-deleting the parent's subtasks via handleDeleteTasks) but only the parent had a compensation op, so the subtree was silently lost across devices. - extractUpdateChanges: scan array-valued payload props instead of guessing `${payloadKey}s`, so irregular bulk keys (e.g. taskUpdates) no longer return {} and drop a remote winner's changes. - Degrade gracefully instead of throwing when a remote update wins over a local delete with no reconstructable base entity, matching the single-entity path (a permanent sync wedge is worse than the bounded divergence it already accepts). - Remove dead code (unused `deleting` set + zero-caller wrapper), use the Set-based scoped-bulk-delete filter, type meta via LwwUpdateMode, and restore the withLocalOnlySyncSettings rationale comment. Adds regression tests for the subtree-recovery and irregular-bulk-key paths. * fix(sync): preserve conflict outcomes during replay Persist replacement LWW operations and bulk-delete snapshots so reconstructed state matches the result applied live. Bump the op-log DB version to prevent older clients from opening the incompatible schema. * fix(sync): persist conflict outcomes atomically Write remote losers, local compensations, and final remote winners in one IndexedDB transaction so crashes cannot expose a partial replay order. Add real-store coverage for live/replay equivalence and transaction rollback. * fix(sync): preserve multi-entity conflict recovery * fix(sync): close review gaps in multi-entity conflict recovery - recreate a winning parent's subtasks when a remote DELETE loses outright (single-entity or all-local-win bulk), so clients that applied the delete and status-blind hydration replay converge - apply the combined resolution batch in durable seq order so a pending row reused from a prior failed attempt replays identically live and after a crash - restamp converted remote updates carrying the v3 replacement envelope to the current schema version - strip the virtual TODAY tag from LWW task payloads and shallow-merge patch-mode singleton payloads instead of replacing feature state - pin the server snapshot fast-path spec to CURRENT_SCHEMA_VERSION (fixes the CI failure from the v2-to-v3 bump) * test(sync): pin outright-losing delete convergence across clients Three-way real-reducer/real-store convergence for the pure-loser path (live == restart replay == originating client), covering both the same-batch recreate exemption and the cross-batch recreate path. Verified to fail against the pre-fix service. |
||
aakhter
|
5b4200df95
|
fix(sync): enforce conflict-journal retention mid-session, not only at start (#8948)
* fix(sync): enforce conflict-journal retention mid-session, not only at start pruneOnStart (14 days / newest 200) ran only at startup, so a long-lived session could grow SUP_CONFLICT_JOURNAL unboundedly. record() now checks the store count (cheap) and, when it exceeds JOURNAL_MAX_ENTRIES plus a slack of JOURNAL_PRUNE_SLACK, runs the same age+count prune — amortized to once every JOURNAL_PRUNE_SLACK records. The prune core is extracted and shared with pruneOnStart; the observe-only never-throw contract of record() is unchanged. SPAP-36 (deferred LOW from PR #8874 review) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(sync): await tx.done with the deletes in the conflict-journal prune An aborted prune delete transaction rejects both the delete requests and tx.done. Awaiting them separately (Promise.all(deletes), then await tx.done) leaves tx.done's rejection unhandled once the delete aggregate rejects first, so it escapes as a global unhandled rejection during active conflict resolution. Await both in one Promise.all so tx.done always has a handler. Adds an aborted-transaction regression test (fails on the old sequencing) and updates the retention model comment + doc to reflect mid-session pruning. Addresses maintainer review on #8948. SPAP-36 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> |
||
|
Johannes Millan
|
329da9b9f3
|
fix(sync): harden full-state operation recovery (#8973)
* fix(sync): prevent destructive full-state sync races Use causal ordering and server-sequence preconditions for full-state operations, surface snapshot rejection, deduplicate migrations, and retain queued WebSocket downloads.\n\nFixes #8959 * fix(sync): block uploads after rejected imports Keep incremental operations behind a durable barrier when a local import or restore is rejected. Release the barrier only after a newer full-state snapshot is accepted, and surface the blocked state instead of reporting sync success. * fix(sync): harden causal repair recovery Persist repair base cursors across the client, provider, and server so stale snapshots are rejected before quota or history mutation and legacy repairs cannot poison restore state. Atomically rebase stale repairs after downloading the missing suffix, preserve rejected-import barriers and WebSocket watermarks, and cover PostgreSQL serialization. * fix(sync): harden repair recovery edge cases Block dependent uploads after any rejected full-state operation, reset negotiated capabilities across provider config changes, and bound WebSocket download retries. Update sync test doubles for causal repairs and the expanded duplicate-operation identity. |
||
|
Johannes Millan
|
24318d11cc
|
fix(sync): reject forged encrypted full-state operations (#8984)
* fix(sync): reject forged encrypted full-state operations Validate decrypted import and repair payloads against the full-state schema so authenticated ordinary operations cannot be promoted into destructive full-state operations. Closes #8905 * fix(sync): preserve compatible encrypted full-state payloads Normalize only known wire and legacy omissions on the validation copy so stripped device-local settings and pre-section backups are not mistaken for metadata tampering. * test(sync): cover encrypted full-state round trips |
||
|
Johannes Millan
|
5e754d3552
|
fix(sync): prevent multi-entity conflict corruption (#8980)
* fix(sync): reject disjoint merges for multi-entity ops Scope conflict field extraction and journal titles to the actual entity. Fall back to whole-op LWW whenever either side is multi-entity so sibling updates are never falsely reported as preserved.\n\nFixes #8944. * fix(sync): harden multi-entity conflict resolution |
||
|
Johannes Millan
|
5624f6891d
|
fix(sync): harden op-log replay and recovery (#8978)
* fix(sync): harden operation-log failure recovery Keep compaction, archive replacement, legacy recovery, and reducer replay checkpointing deterministic across crashes and concurrent clients. Refs #8958 * fix(sync): harden remote operation recovery Persist reducer failures across hydration and fail closed for full-state operations. Serialize recovery and archive mutations, and report skipped emergency compaction accurately. * fix(sync): preserve operations across replay failures |
||
Johannes Millan
|
291995f343 |
Merge remote-tracking branch 'origin/master' into feat/sync-55222e
* origin/master: (25 commits) refactor(tasks): extract shared task ordering helpers (#8926) feat(sync): conflict journal + disjoint-field auto-merge + review UI (#8874) refactor(config): reduce duplicated form and selector boilerplate (#8928) feat(work-view): show break time today (#8909) feat(tasks): navigate from empty add-subtask input (#8916) docs(development): add instructions for setting up local tests with Chromium (#8887) chore(lint): remove unused eslint-disable directives (#8913) fix(accessibility): add ARIA roles, live regions, and alt attributes to banner component (#8888) chore(ui): removes dead utilities and op-log leftovers [#8260 - Tier A] (#8892) fix(app): hide donation page on macOS (#8915) chore(scripts): remove one-off codemod scripts [#8260 - Tier A] (#8893) fix(sync): harden SuperSync E2EE against metadata tampering (GHSA-8pxh-mgc7-gp3g) (#8904) feat: add Home Assistant Bridge to community plugins (#8891) docs(sync): note local-file rev-check/write is non-atomic (#8898) (#8902) 18.14.0 fix(tasks): remove postal-mime dep and harden eml import (#8901) style(tasks): elevate add-subtask input fix(config): restore day-start offset after operation replay (#8899) fix(task-repeat): allow selecting day-of-month recurrence (#8896) feat(plugins): add Todoist import plugin with Import/Export launcher (#8882) ... # Conflicts: # docs/wiki/3.06-User-Data.md # src/app/op-log/backup/backup.service.spec.ts # src/app/op-log/backup/backup.service.ts # src/app/op-log/sync/conflict-resolution.service.ts |
||
|
Johannes Millan
|
582929e375 |
docs(sync): document atomic checkpoint, db v8 barrier and rebuild undo
Update the archive-operation lifecycle docs to the atomic reducer-checkpoint + clock-merge design, record the IndexedDB v8 downgrade barrier, drop the removed PENDING_OPERATION_EXPIRY_MS constant, and describe the durable Use-Server-Data Undo across reloads. |
||
|
aakhter
|
962c5bbeb1
|
feat(sync): conflict journal + disjoint-field auto-merge + review UI (#8874)
* feat(sync): conflict-journal foundation (observe-only)
Add a device-local IndexedDB conflict journal that records every sync-
conflict auto-resolution so the discarded ("losing") side is preserved
and reviewable later. Foundation subtask for the conflict-review epic;
no UI here, verifiable purely by unit tests.
- New SUP_CONFLICT_JOURNAL IndexedDB store (own DB; never touches the
op-log SUP_OPS schema) + ConflictJournalService with record/query API
(unreviewedCount$, list, markKept, markFlipped, getEntry) and 14-day /
200-entry retention pruning, wired to run on app start via APP_INITIALIZER.
- Pure classifier buildConflictJournalEntry maps each resolution to the
agreed taxonomy (newer/tie/delete-wins/noise/clock-corruption-
suspected; disjoint-merge reserved for the next subtask), capturing the
loser's field values verbatim. NOISE_FIELDS is limited to metadata
timestamps (modified/lastModified/created): the list-ordering arrays
carry membership as well as order, so an overlap on them is surfaced as
a reviewable conflict rather than silently classified as noise.
- Emission is strictly observe-only: journaling runs after the LWW plan is
built, wrapped in try/catch (record() swallows its own errors); clock-
corruption attribution uses a WeakSet side-channel tagged at detection.
The existing conflict-resolution suite stays 138/138 green, proving LWW
picks are unchanged. One-sided / sequential / EQUAL updates never become
conflicts and produce zero journal entries.
SPAP-13
* feat(sync): disjoint-field auto-merge for concurrent edits
When two clients concurrently edit the same entity but different fields
(A changes title, B changes notes), whole-entity LWW previously discarded
one side. Keep both when the non-noise changed-field sets are disjoint.
In _resolveConflictsWithLWW, before LWW picks a winner, each CONCURRENT
conflict is tested for merge eligibility (neither side deleting/archiving;
both changed >=1 real field; non-noise field sets disjoint). If eligible,
synthesize a single merged UPDATE op — the current entity overlaid with
the other side's non-noise fields, noise fields resolved by the greater
(timestamp, clientId) — carrying a vector clock that dominates both sides,
so it propagates through normal sync. The resolution is winner 'merged'
and is journaled reason 'disjoint-merge' / status 'info' (not counted as
unreviewed). Any overlap on a non-noise field, or a delete/archive on
either side, falls through to the existing LWW path unchanged.
Convergence: both clients compute the byte-identical merged entity
(disjoint real fields each owned by one side; noise resolved by the same
global tiebreak) with clocks dominating both originals, so the two
independently-synthesized merged ops carry identical full-entity payloads
and re-resolve to the same state via ordinary LWW without re-merging.
No sync-core/protocol change. Existing conflict-resolution suite stays
138/138; SPAP-13 journal specs stay green.
SPAP-14
* feat(sync): sync conflicts review UI (banner, badge, page, flip)
Builds the conflict-review UI on top of the device-local conflict journal.
- Post-sync summary banner "N conflicts auto-resolved (X remote, Y local
won)" with REVIEW / DISMISS, replacing the bare LWW_CONFLICTS_AUTO_
RESOLVED snack at its emission sites; a persistent badge on the sync
icon bound to unreviewedCount$ (survives banner dismiss).
- New /sync-conflicts page: Unreviewed | History tabs, rows grouped by
entity type with winner + reason chips, expandable per-field diff
(LOCAL vs REMOTE, device name + wall-clock time, winner marked),
per-row KEEP / FLIP and bulk KEEP ALL / FLIP ALL → LOCAL / → REMOTE.
History renders merged auto-merges as per-field chips.
- Flip dispatches a normal entity update with the loser's journaled field
values (syncs like a user edit, no history rewind) and marks the entry
flipped; a stale-flip confirm appears (with the current value shown)
when the entity changed since resolution.
- i18n under F.SYNC.CONFLICT_REVIEW.
Delete-restore and archived-entity flip are surfaced as unsupported for
now (an update op can't recreate an absent entity) — follow-up.
SPAP-15
* fix(sync): count disjoint-merge ops in localWinOpsCreated
autoResolveConflictsLWW returned only newLocalWinOps.length, excluding the
synthesized merged ops appended in STEP 3b. A sync whose only conflicts were
disjoint-field merges returned 0, so the caller (immediate-upload.service.ts)
skipped the immediate re-upload and reported IN_SYNC while the merged op sat
unsynced until a later cycle.
Count mergedResolutions.length too — each merge appends exactly one pending
local op. Mirrors the rejection-handler path (operation-log-sync.service.ts).
Add a regression spec asserting a merge-only conflict returns
localWinOpsCreated: 1 (fails on the old return, passes now).
Also harden the _corruptionSuspectedConflicts WeakSet doc against a future
refactor that clones EntityConflict between detect and resolve.
Addresses review feedback on PR #8874.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(sync): address second-pass review (delete-lost, archive guard test, polish)
Second-pass review follow-ups (johannesjo). The MEDIUM localWinOpsCreated
item was already fixed in
|
||
Lane Sawyer
|
c437dfc35a
|
chore(ui): removes dead utilities and op-log leftovers [#8260 - Tier A] (#8892)
* chore(ui): removes dead utilities and op-log leftovers [#8260 - Tier A] * update readme and remove-unused-log-imports.ts * restore benchmark test and make runnable |
||
|
Johannes Millan
|
eaa15fe575 | fix(sync): handle initial provider setup safely | ||
|
Lane Sawyer
|
6f1cacc553
|
chore(scripts): remove one-off codemod scripts [#8260 - Tier A] (#8893)
* chore(scripts): remove one-off codemod scripts [#8260 - Tier A] * Address PR feedback |
||
|
Johannes Millan
|
eabfd84436 | fix(sync): migrate legacy remote failures once | ||
|
Johannes Millan
|
62e9e34b29 | fix(sync): preserve incomplete recovery work | ||
|
Johannes Millan
|
7866e16b56 | docs(sync): document recovery checkpoints | ||
|
Johannes Millan
|
c6480d1cae
|
fix(sync): harden SuperSync E2EE against metadata tampering (GHSA-8pxh-mgc7-gp3g) (#8904)
* fix(sync): defense-in-depth vs entityId retarget on encrypted ops SuperSync E2EE (AES-256-GCM) covers only op.payload; metadata fields (entityId, opType, actionType, vectorClock, isPayloadEncrypted, ...) travel as plaintext and are not bound by the auth tag. A malicious/ compromised sync server or MITM could retag an encrypted LWW-update op with a different entityId, redirecting the authenticated changes onto an attacker-chosen entity — convertOpToAction() previously trusted the tampered entityId over the authenticated payload.id (coercing even a missing payload.id) and only warned. At the decrypt boundary (where encryption origin is known) verify that an in-scope LWW op's authenticated payload carries a string id equal to op.entityId; otherwise fail closed via a new OperationIntegrityError, distinct from DecryptError so it does not trigger the enter-password dialog. The gate mirrors convertOpToAction's predicate (alias resolution + singleton exclusion) so the two boundaries cannot drift. Scoped defense-in-depth for GHSA-8pxh-mgc7-gp3g, NOT full integrity. Still open (durable AAD-envelope fix): plaintext-injection downgrade via isPayloadEncrypted=false (needs a download-side mandatory-encryption guard), opType promotion, entityType swap, vectorClock replay. Correct the overstated integrity claim in the encryption architecture doc. * fix(sync): reject plaintext ops when SuperSync encryption is mandatory The isPayloadEncrypted flag is unauthenticated plaintext metadata, so a compromised SuperSync server or MITM could set it to false and inject a fully attacker-authored plaintext op — it would skip decryption AND the payload/metadata integrity check and be applied verbatim (arbitrary op forgery). This is a strictly more powerful bypass than the ciphertext entityId retarget closed previously. assertOpsEncryptedWhenExpected rejects any inbound plaintext op (download + piggyback paths) when encryption is enabled. It gates on config INTENT (isEncryptionMandatory && isEncryptionEnabled()), not key presence, so it also fails closed in the dropped-credential state (a !!encryptKey gate would fail open there). Safe with no legacy-data loss: enabling encryption deletes the server copy and re-uploads everything encrypted, so no legitimate plaintext op remains; a never-encrypted account (isEncryptionEnabled()===false) still accepts plaintext. The SuperSync op-level twin of the file-based GHSA-vrc7 download guard and the GHSA-9544 upload guard. Also give OperationIntegrityError a dedicated sync-wrapper branch: fail closed with a calm translated message instead of the raw GHSA/technical string. Follows up the review of GHSA-8pxh-mgc7-gp3g. |
||
|
Johannes Millan
|
05f6bd27d1
|
fix(op-log): serialize SQLite adapter transactions on the shared connection (#8849)
* fix(op-log): serialize SQLite adapter transactions on shared connection The SQLite op-log adapter issued raw BEGIN/COMMIT on the shared connection with no serialization. Once both stores share one SqliteDb (the staged native rollout), concurrent operations (capture append, archive write, compaction) would interleave BEGINs: SQLite has no nested transactions, so a second BEGIN throws and a bare statement issued mid-transaction silently joins — and rolls back with — the foreign transaction, corrupting op-log state. Funnel every adapter entry point through an internal FIFO queue so a transaction is exclusive on the connection for its whole BEGIN/COMMIT and no bare operation interleaves. Transaction-internal work runs directly on the connection (already holding the slot), so there is no re-entrancy. Document the mutual-exclusion invariant in the port contract and add a concurrent-transactions contract test that runs on both the in-memory fake and real sql.js. * docs: add complete architecture review report (2026-07-07) Whole-app architecture review synthesized from eight parallel subsystem reviews and an adversarial verification pass; findings filed as issues #8832-#8843, with duplicates cross-referenced rather than re-filed. * fix(op-log): key the SQLite transaction serializer to the connection Multi-agent review found the serializer was keyed to the adapter instance. The native rollout hands the op-log store and archive store two separate SqliteOpLogAdapter instances over one shared SqliteDb, so per-instance queues left an op-log BEGIN free to interleave with a concurrent archive BEGIN on the shared connection — the exact hazard the serializer targets. Key the FIFO queue to the connection (WeakMap<SqliteDb>) so every adapter over one SqliteDb shares one queue. Add a contract test that drives two adapters over one connection concurrently (verified red with per-instance keying, green with per-connection). Also: document the re-entrancy precondition as unenforced (a lint rule, not a runtime flag, is the right guard — a flag cannot distinguish a re-entrant call from a legal concurrent one) and correct init/getLastSeq/port-contract doc drift. |
||
Harbinger
|
8171bb05d0
|
refactor(sync): remove unused error classes, dialog, and constructor-time logging (phase 1, #8325) (#8510)
* fix(op-log): lock snapshot save to prevent lost-update window saveCurrentStateAsSnapshot() read NgRx state then lastSeq without holding OPERATION_LOG lock. An op appended between the two reads would get seq <= lastAppliedOpSeq but its effect would be absent from the snapshot. On next hydration the tail replay would start after that seq, silently skipping the op forever. Fix: wrap in lockService.request(LOCK_NAMES.OPERATION_LOG, ...) and read lastSeq BEFORE state snapshot so the worst interleaving degrades to harmless re-replay (idempotent) rather than a missed op. Fixes #8308 * fix(op-log): address review feedback on snapshot lock PR - Amend JSDoc idempotency claim: syncTimeSpent is additive on re-replay - Add inline note about compaction's opposite read order and worse failure mode - Add lock regression tests (#8308): lock acquired, read order, error handling Co-Authored-By: Claude <noreply@anthropic.com> * refactor(sync): remove unused error classes, dialog, and constructor-time logging Phase 1 of #8325: clean up orphaned sync error types and their UI. Removed error classes that are no longer thrown anywhere: - NoEtagAPIError, FileExistsAPIError (unused API errors) - RevMismatchForModelError, SyncInvalidTimeValuesError (superseded by file-based flow) - RevMapModelMismatchErrorOnDownload/Upload, NoRemoteModelFile, NoRemoteMetaFile - LockPresentError, LockFromLocalClientPresentError, MetaNotReadyError, InvalidRevMapError Removed DialogSyncErrorComponent and all references in SyncWrapperService (_forceDownload, _handleIncoherentTimestampsDialog, _handleIncompleteSyncDialog, _openSyncErrorDialog, _extractModelIdFromError). Removed constructor-time logging from JsonParseError, ModelValidationError, DataValidationFailedError — these errors are logged at the catch site; redundant construction-time logs risk leaking user data. Cleaned up dead translation keys (D_INCOMPLETE_SYNC block, DIALOG_RESULT_ERROR, ERROR_DATA_IS_CURRENTLY_WRITTEN) from en.json and t.const.ts. Updated file-based-sync-flowchart.md to reflect the removed error types. --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
Mohamed Abdeltawab
|
0b4bc79354
|
refactor: retire GET /api/sync/snapshot, re-scope GET /api/sync/status as diagnostic (#8496)
Retire GET /api/sync/snapshot (attack-surface reduction): - Remove route handler from sync.routes.ts (no production client caller) - Keep internal generateSnapshot() and all downstream code - Update tests: delete GET /snapshot describe block, rework isolation test (sync.routes.spec.ts), remove GET assertion (sync-fixes.spec.ts), remove SimulatedClient.getSnapshot() helper + rework 2 integration tests to use GET /api/sync/ops instead (multi-client-sync.integration.spec.ts) Re-scope GET /api/sync/status as diagnostic: - Add doc comment to route handler marking it diagnostic - Update all documentation (README, API wiki, architecture diagrams) Documentation updates across 5 files remove GET /snapshot references and label GET /status as diagnostic. Breaking: self-hosters with external tooling relying on GET /snapshot must migrate — no shipped client version ever called this endpoint. |
||
|
Mohamed Abdeltawab
|
c2bf7b26a0
|
refactor(sync-core): drop shared-schema vector-clock compat re-export anda app enum (#8441)
* feat(sync-core): drop shared-schema vector-clock compat re-export and app enum - Remove the shared-schema → sync-core compatibility re-export of vector-clock types/functions; retarget app files (vector-clock.ts,operation-log.const.ts) and the server (sync.types.ts) to import from @sp/sync-core directly - Add @sp/sync-core to super-sync-server/package.json deps (it was load-bearing through the re-export) - Convert VectorClockComparison from a bare type to an as const object + derived type in sync-core,drop the app-side enum copy and the as cast - Update spec imports and pa ckage-boundaries.md * docs: remove stale shared-schema vector-clock references - Update comments in vector-clocks.md, client vector-clock.ts, and server sync.types.ts to reference @sp/sync-core directly - Remove @sp/sync-core dependency from shared-schema/package.json - Regenerate package-lock.json to reflect the removed edge * docs(sync): fix orphaned VectorClockComparison comment The comment block describing VectorClockComparison was left dangling above no declaration after the app-side enum was removed, and still claimed 'Uses enum for client-side ergonomics'. Move it above the re-export it documents and correct the wording. --------- Co-authored-by: Johannes Millan <johannes.millan@gmail.com> |
||
|
Mohamed Abdeltawab
|
a4c0dc7d46
|
refactor: remove dead SyncStateCorruptedError, compat exports, and 0 byte sync-providers barrel #8328 (#8396)
* refactor: remove dead SyncStateCorruptedError, compat exports, and 0-byte sync-providers barrel #8328 * docs(sync): drop stale SyncStateCorruptedError/fail-fast references (#8328) The fail-fast dependency-resolution subsystem (DependencyResolverService + SyncStateCorruptedError throw) was removed earlier; OperationApplierService now bulk-dispatches ops in causal arrival order and returns a failedOp for the caller to re-validate/retry. Update the sync architecture docs, archive-operations diagram, and user-data wiki to match. Completes the dead-code cleanup for #8396. --------- Co-authored-by: Johannes Millan <johannes.millan@gmail.com> |
||
|
Johannes Millan
|
59e2a1791c
|
fix(sync): prevent lock-timeout from wedging op capture (#8306, #8318) (#8383)
A LockAcquisitionTimeoutError during op capture errored the whole persistOperation$ stream: concatMap tore down and silently dropped every buffered action, the positional capture FIFO leaked an entry so flushPendingWrites() could never reach 0 (every sync then failed after 30s), and after NgRx's 10-resubscribe cap the effect died until reload. Fix, bundled with the #8318 cleanup: - Replace the positional FIFO queue with a pending counter. The meta-reducer increments it; the effect decrements it in a `finally` (writeOperationFromEffect), so a thrown write can never leak the flush signal. The decrement runs after the write commits + lock releases, preserving the flush commit-ordering invariant. - The effect catches per write so one failure never tears down the shared stream (the resubscribe-death and silent-drop fixes). - entityChanges is now computed in the write path via the pure extractEntityChanges(); the `[]` field is still emitted (Android reads it; isMultiEntityPayload requires it). - writeOperation keeps its throw for the #7700 deferred retry loop (that path bypasses the wrapper and is not counted). This also structurally removes the #8307 double-dequeue. Adds operation-log-effect-stream-survival.regression.spec.ts (stream survives lock timeout, counter drains on always-fail, survives >10 failures) and updates the capture/flush/integration specs + sync docs. |
||
|
Johannes Millan
|
d30fd5f434
|
fix(sync): conflict-check all entities of multi-entity ops (#8334) (#8377)
* fix(sync): conflict-check all entities of multi-entity ops #8334 Multi-entity ops (deleteTasks, moveToArchive, __updateMultipleTaskSimple, round-time-spent, batch board/issue-provider actions) carry entityIds[], but the server operations table only persisted the scalar entity_id (= entityIds[0]). Once such an op was stored, only its first entity took part in future conflict lookups, so a later stale write to a non-first entity found no prior writer and was wrongly accepted instead of rejected as CONFLICT_SUPERSEDED/CONCURRENT. - Add an entity_ids text[] column (populated for multi-entity ops only via getStoredEntityIds; single-entity ops store [] and use the scalar) + a GIN index. Migration is metadata-only with no backfill: pre-migration rows fall back to entity_id, so the fix is forward-only (entities 2..n of already-stored ops were never persisted and stay unrecoverable). - detectConflictForEntity now runs two ordered LIMIT-1 lookups (scalar btree + entity_ids GIN) and takes the higher server_seq, preserving the fast ordered hot path instead of an OR's BitmapOr+sort. - detectConflictForEntities / prefetchLatestEntityOpsForBatch match an entity as the scalar entity_id OR a member of entity_ids (unnest CASE + && / = ANY). - Harden validateOp to bound entityIds (length + per-element), mirroring entityId. Raw SQL validated against Postgres (PGlite) incl. GIN usage and two-query correctness; single path + validation + migrations covered by unit tests. The full conflict-detection.spec needs a generated Prisma client (CI), and the hot-path round-trip tradeoff should be confirmed with a real-PG EXPLAIN. * fix(sync): store entity_ids when a batch op dedups off the scalar #8334 Multi-review follow-up. getStoredEntityIds gated on `length > 1`, so a batch op whose entityIds dedup to a single value that differs from entityId (the server does not enforce entity_id === entityIds[0]) stored [] and that entity became invisible to conflict lookups — reintroducing #8334 for it. Gate on "is the set exactly [entity_id]?" instead, and cover it with unit tests. Also: correct the array-branch comment/doc — a GIN(entity_ids) lookup has no server_seq so it match-all-then-sorts (cheap only because multi-entity ops are rare), it is not an ordered walk; drop a stale "OR filter" test docstring; add a counter-note on getConflictEntityIds vs getStoredEntityIds to prevent swapping. * refactor(sync): single OR lookup for entity conflict detection #8334 Third multi-review follow-up. Revert detectConflictForEntity from the two ordered findFirst lookups back to a single Prisma OR [{entityId}, {entityIds:{has}}]. The two-query split optimized the OR's BitmapOr+sort, but that is bounded by op-log pruning (sub-ms in practice) while the split added a guaranteed extra round-trip on the common single-entity path (doubled by the FIX-1.5 re-check) — a net-negative for the median. The OR is simpler, fully typed/testable, and likely faster in aggregate; a code comment documents the split as the escalation if a real-PG EXPLAIN ever shows the OR is a problem. Review polish (no behaviour change): correct the array-branch/getStoredEntityIds comments (a GIN @> is match-all-then-sort, not an ordered walk; multi-entity-only storage's win is GIN size + keeping single-entity inserts off the GIN, not sort depth); note the batch unnest paths as the first EXPLAIN candidates under load; keep the two batch queries' CASE/prefilter SQL inline (a shared fragment would shift the positional params the conflict-detection.spec mock relies on) with a keep-in-sync note; replace a non-ASCII <= in a client-facing validation error string. * test(sync): update db mocks for OR + prefetch entity_ids lookups #8334 Running the full super-sync-server suite (with a generated Prisma client) surfaced two specs whose inline db mocks hadn't tracked the new conflict-detection SQL: - time-tracking-operations.spec: findFirst now models the single-entity lookup's OR: [{entityId}, {entityIds:{has}}] shape (it previously only matched the scalar entityId, so a concurrent single-entity update was wrongly "accepted"). - sync.service.spec: the prefetch $queryRaw mock parsed userId as the last param and flattened all params into the touched pairs; the #8334 prefilter adds idArray params after userId, so it now finds userId by type and reads the touched pairs from the VALUES join fragment only. Production code unchanged; these are test-mock fidelity fixes. Full suite: 800 passing. |
||
Miklos
|
67d1e32ffc
|
feat(focus-mode): focus screen UX overhaul (#7586)
* feat(focus-mode): focus screen UX overhaul Major rework of the focus mode timer screens (#7349): - Shared <focus-clock-face> drives Pomodoro / Flowtime / Countdown / Break with a single visual chrome; size tokens scale fluidly via clamp() + vmin/vh, no discrete breakpoints. - Single source of truth: --clock-time-size and --control-offset derive from --clock-face-size. - Countdown: click-to-edit duration, draggable handle on the ring, hybrid 5-min/15-min snap with 6h-per-rotation above 1h ([useFlexibleIncrement] on input-duration-slider, opt-in for other consumers). - Pomodoro prep inherits the click-to-type input; drag handle hidden so dragging the ring can't silently shift the value. - Pause keeps the selected task on screen (displayedTask falls back to the paused task while currentTask is null). - Notes panel acts as a modal: clock stays put, backdrop dims and closes on outside-click. - Session controls row below the circle (pause / complete / reset cycles); buttons fade via shared --revealed-opacity gated on host hover + document.hasFocus(). - Break screen mirrors focus-mode-main layout; cycle counter inside the circle on both focus and break; back-to-planning unified. - Flowtime settings dialog: all fields render with proper disable/enable, stable dialog width when switching modes. - arrow_backward -> arrow_back: fixes glitched glyph in repeat-type context menus (boards, simple counters, take-a-break, flowtime). * fix(focus-mode): silence naming-convention lint on formly 'props.disabled' Formly's expressionProperties path-string keys ('props.disabled') aren't camelCase and the rule has no requiresQuotes exemption; matches the existing pattern in src/app/features/issue/common-issue-form-stuff.const.ts. * test(focus-mode): mock pomodoroConfig signal on FocusModeService The component's initialization effect now reads focusModeService.pomodoroConfig() in Pomodoro+Preparation mode, but the three mocked FocusModeService instances in the spec didn't provide it, causing TypeError in 47 tests on CI (somehow not surfaced locally). * test(focus-mode): align specs with unified back-to-planning flow - focus-mode-break.spec: exitBreakToPlanning -> cancelFocusSession - focus-mode-session-done.spec: drop obsolete hideFocusOverlay assertion (cancelFocusSession now handles both clearing tracking and hiding the overlay, matching the production component) - focus-mode-main.spec: storeSpy gains selectSignal returning a signal, needed by the displayedTask paused-task fallback * fix(focus-mode): restore E2E selector hooks on refactored controls The shared <focus-clock-face> refactor moved pause/complete buttons out of the clock face into a new .circle-controls row but didn't carry the class names forward; 23 E2E specs key off them. Also re-add .task-title-placeholder on the no-task FAB so prep-state checks find it. - .pause-resume-btn on pause/resume in focus-mode-main + focus-mode-break - .complete-session-btn on the done_all button in focus-mode-main - .task-title-placeholder added to .select-task-cta FAB * fix(focus-mode): aria-label icon buttons; align break E2Es with new flow - focus-mode-break: pause/resume/skip/reset icon buttons now carry [attr.aria-label] in addition to matTooltip — icon ligature alone isn't an accessible name and breaks getByRole locators. - pomodoro-break-timing-bug-6044.spec: skipButton uses getByRole with accessible name rather than hasText (mat-icon ligature is "skip_next", not "skip break"). - focus-mode-break.spec: "exit break to planning and change timer mode" and "Back to Planning should NOT auto-start next session" now match the unified back-to-planning flow — overlay closes on click, user re-opens focus mode to change settings or verify prep state. * fix(focus-mode): address PR #7586 review feedback Maintainer review (johannesjo): - Debounce the Pomodoro work-duration write so editing it emits one synced config op instead of one per keystroke; flush on session start so a value typed inside the debounce window is not lost. (A1) - Replace the local ::ng-deep restyling of the shared input-duration-slider with opt-in [bareRing] and [hideHandle] inputs that own the chrome overrides in the slider's own styles; the four other consumers keep the default look. (A2) - Add unit tests for the flexible drag math (_setValueFromRotationFlex): the A<->B boundary anchoring at 55/60 min and both +/-180 degree wrap branches. (A3) - Delete the orphaned exitBreakToPlanning action, its stopTrackingOnExitBreakToPlanning$ effect, reducer case and specs; cancelFocusSession already unsets the current task. (B1) - Drop the unreferenced CONTINUE_TO_NEXT_SESSION and BREAK_RELAX_MSG i18n keys. (B2) - Collapse the duplicated clock-size clamp() into a single --clock-face-size-default token. (B4) Smaller focus-screen fixes (beerkumquatpome): - Hide the break task title when "pause tracking during breaks" is on. (C7) - Commit and close the duration editor on Enter. (C9) - Rename "Back to Planning" to "Exit focus session". (C11) - Show Flowtime breaks as a neutral "Break". (C13) Break-circle vertical alignment (C1) is only partially addressed here (matched the top reservation); exact alignment is a follow-up. * refactor(focus-mode): share a layout shell across timer screens and auto-start Flowtime breaks Extract a presentational focus-mode-layout component (4-row content-projection skeleton: [fmTop]/[fmTask]/[fmClock]/[fmBottom]) shared by the focus-session and break screens, so both keep a stable clock baseline across the focus<->break and prep<->in-progress transitions. focus-mode-main and focus-mode-break now consume the shell instead of each maintaining their own absolute layout. Replace the Flowtime "break offer" step with an auto-started break, mirroring Pomodoro: - Remove the BreakOffer UI state and the offerFlowtimeBreak action/reducer. - endFlowtimeSession now dispatches completeFocusSession(isManual:false) + startBreak (unsetting the task first when tracking-pause-on-break is on), so the break starts automatically and the session is logged exactly once via logFocusSession$. Also reorder the bottom controls (Back to Planning leftmost), restore the BACK_TO_PLANNING label to "Back to Planning", and drop the now-orphaned FLOWTIME_BREAK_TITLE / START_BREAK i18n keys. * test(layout): restore document.activeElement after focus-restoration specs The LayoutService "Focus restoration" tests override document.activeElement with Object.defineProperty, which shadows the native (inherited) getter with an own property on document. The afterEach only removed the mock DOM node, so the override leaked into later specs: once it ran, document.activeElement was frozen at the mock element and subsequent .focus() calls could no longer move it. Depending on Karma's spec order this broke the task.service focusTaskById tests (#7120), which then saw the stale activeElement instead of the element they focused. Delete the shadowing own property in afterEach to restore native behavior. Repro: ng test --include layout.service.spec.ts --include task.service.spec.ts * refactor(focus-mode): add interactive tracking widget and polish timer-screen layout - Replace the read-only task-tracking-info with focus-mode-task-tracking (vertical time stack + play/pause), wired through the shared layout shell - Center the task title and floor the task-row height so the clock baseline stays aligned across focus <-> break - Spacing polish: task-title-row and layout gaps to --s2, segmented-button-group padding to 0 - Drop redundant safe-area-bottom padding on the action row (the overlay already reserves it for the fixed shell) - Add "Take a moment to relax" break message and a clock-digit edit affordance * refactor(focus-mode): share timer/break layout, drop dead tracking toggle - Extract a shared <focus-mode-layout> skeleton and <focus-mode-task-row> used by both the focus session and the break. - Remove the in-view tracking play/pause toggle (start/stop stays on the global header button); strip focus-mode-task-tracking to read-only and drop RESUME_TRACKING. - Pin the mode selector out of flow and center the task·clock·bottom group; equal reserved task/bottom rows keep the clock vertically centered, with the selector kept on top. - Tighten sizing: horizontal selector segments, settings cog matched to the in-session controls, and a fluid clock clamp. |
||
|
Johannes Millan
|
63d46f7a0b
|
feat(op-log): validate SQLite backend + IDB→SQLite migration + backend-aware init (#7931) (#7954)
* test(op-log): validate SqliteOpLogAdapter against a real sql.js engine The 23 adapter specs ran only against an in-memory regex stand-in that models the SQL shapes the adapter emits — it validates the translation layer, not SQLite itself. Add sql.js (dev-only; never in the app bundle) served into Karma, and run the behavioral contract against BOTH the fake and a real SQLite engine. This exercises genuine-engine behavior the stand-in could only model: the real UNIQUE-constraint message -> ConstraintError mapping, AUTOINCREMENT never reusing seq after clear(), compound-index + NULL range handling, and real BEGIN IMMEDIATE rollback. 51/51 green. B2 (translation-layer pass) per docs/sync-and-op-log/sqlite-migration.md. The integration-harness second pass and the on-device real-engine run remain. * test(op-log): run store-port integration against real sql.js (B2 stage 2) Parameterize the RemoteOperationApplyStorePort integration scenarios to run against BOTH the default IndexedDB backend and a sql.js-backed SqliteOpLogAdapter, exercising the store's COMPOSED flows (apply/mark/ merge-clock, partial-failure persistence, full-state import clearing, vector-clock persistence) on a real SQL engine — not just the adapter in isolation. 6/6 green. Surfaced a real B3 wiring gap: OperationLogStoreService.init() is IDB-shaped (opens+adopts an IndexedDB connection, never calls the adapter's own init()). For a self-managing backend like SQLite the tables would not exist. The sql.js setup creates them once on the shared db to mirror the store-init change B3 must make on native (call adapter.init() / skip the IDB open when the backend is SQLite). * feat(op-log): add verified IDB->SQLite backend migration (C1) One-time copy of the entire op-log from a source adapter (legacy IndexedDB) to a dest adapter (SQLite) in a single dest transaction with verify-before-commit: a mismatch in op count, last seq, or vector clock throws and rolls the dest back, leaving it empty and the source untouched. Adapter-agnostic (talks only to the OpLogDbAdapter port), so it is validated in CI with a real Chrome IndexedDB source + a sql.js SQLite dest; the native @capacitor-community/sqlite dest behaves identically through the same port. The generic iterate->put copy preserves ops seq (incl. gaps) via the put-honors-seq path and writes singletons at their out-of-line key uniformly, with no per-store special-casing. Not wired into startup — Phase B3/C2 decide WHEN to run it (SQLite empty + legacy SUP_OPS present) and retain the IDB copy >= 1 release. 5/5 green, incl. seq-fidelity, AUTOINCREMENT-continues-past-migrated, empty-source, non-empty-dest guard, and verify-rollback. * docs(sync): record sql.js validation, C1, and the B1/B3 findings Update the SQLite migration plan + follow-up backlog to reflect what landed this pass and hand off the device-gated remainder: - B2: real-engine (sql.js) adapter contract + store-port second pass are done in CI; only the on-device run remains. - C1: the backend-migration algorithm + verify-before-commit are done and tested (real IDB -> sql.js); only the startup wiring remains. - B3 finding: OperationLogStoreService.init() is IDB-shaped (opens+adopts IDB, never calls adapter.init()); native must call adapter.init() and skip the IDB open on SQLite. - B1 perf note: bridge round-trips dominate on native; return lastId from the plugin's run response and add a runBatch/executeSet bulk path so appendBatch is one crossing, with RETURNING-seq for per-op seq. * feat(op-log): make store init backend-aware for self-managing backends (B3) OperationLogStoreService.init() and ArchiveStoreService._init() were IDB-shaped: they unconditionally opened+adopted a WebView IndexedDB connection and never called the adapter's own init(). For a self-managing backend (SQLite) that meant (a) the adapter's tables were never created and (b) it still touched the evictable WebView store this migration exists to escape. Now: when the adapter exposes no adoptConnection (i.e. it self-manages, like SQLite), call adapter.init() and skip the IndexedDB open. The adopt-connection (IndexedDB) path is unchanged. The new branch is dead in production until B3 flips the native token, so this is risk-free now and unblocks that flip. Tested: two unit tests cover both branches (self-managing -> adapter.init, no IDB open; IDB -> open+adopt, no adapter.init). The store-port integration spec now drives the store fully on SQLite with _db undefined, so its earlier pre-init workaround is removed. 521 persistence + 6 integration green. * docs(sync): mark the B3 backend-aware init fix as landed The store-init half of B3 (call adapter.init() / skip the IDB open for self-managing backends) is implemented + CI-tested; only the device-gated native token flip + SqliteDb wrapper remain. |
||
|
Johannes Millan
|
64d3219d3a
|
feat(local-backup): Track A safeguards for #7925 (#7932)
* fix(android): escape JS bridge args via JSONObject.quote (#7925)
`loadFromDb` interpolated the stored value into a single-quoted JS string
literal passed to `evaluateJavascript`. Beyond the security smell, this is
a real data-loss bug: `JSON.stringify` does not escape apostrophes, so a
backup blob containing one (e.g. a task titled "don't…") terminated the JS
literal and the load returned garbage — silently corrupting any restore
from `KeyValStore`.
Use `JSONObject.quote()` (already established in the file for
`emitForegroundServiceStartFailed`) for all three callback args, so values
containing `'`, `\`, newlines or `</script>` round-trip cleanly.
* feat(startup): log storage-persistence outcome on all branches (#7925)
`_requestPersistence()` was silent on native and on the `false` resolution
of `persist()`, so #7892-style "woke up blank" reports carried no signal
about whether the WebView store was actually persistent.
Always log `{persisted, granted, isNative, isElectron}` — including the
already-persisted branch, the persist-resolved branch (both true and false),
the error branch, and the no-`navigator.storage` branch. User-facing snack
gating is unchanged (still web-only, non-onboarding). Logging-only — no
behavioral change.
This is Track A1 in `docs/sync-and-op-log/sqlite-migration-followup.md`:
the diagnostics that decide whether the deeper protective steps
(near-empty write guard, SQLite migration) are worth the added complexity.
* docs(sync): refresh sqlite-migration followup after #7924 (#7925)
The followup backlog described the local-backup ring as TODO under A2,
but #7924 already shipped the periodic + app-private backup, two-generation
ring, empty-state write guard, and informed restore prompt. Bring the doc
in sync:
- Add the shipped local-backup work to "Where we are now".
- Replace the old A2 ("Periodic local auto-backup") with the narrower
remaining gap: a debounced data-change backup trigger to complement the
5-min timer.
- Add A3 (near-empty write-time overwrite guard) with a concrete starting
threshold and a fail-safe rationale, sequenced after A1 so the
diagnostics tune the threshold before it lands.
- New "Cross-cutting / hardening" section consolidating the items
surfaced by the #7924 review (Kotlin JS-bridge escaping — now done;
backup-date reader bridge for the restore prompt; robust restore on
degraded boot; last-backup visibility; onboarding nudge for no-sync
users).
* feat(local-backup): debounced on-data-change backup trigger (#7925)
The 5-min `interval()` was the only thing that drove `LocalBackupService._backup()`,
so a destructive event in the minutes before a WebView eviction could be lost
from the backup ring even though the live store had it.
Merge a `LOCAL_ACTIONS`-driven trigger into `_triggerBackupSave$` that fires
once after a 30s quiet period. Catches the typical "user made changes then
put phone down" pattern before the next periodic tick. `LOCAL_ACTIONS`
already filters out remote/hydration replays, and the existing empty-state
guard prevents writing a degraded post-eviction snapshot over a good
backup, so this strictly adds backup frequency — never spam.
Logic-level only — `_backup()` continues to early-return on non-target
platforms (web/PWA), so the trigger is safe to subscribe everywhere.
Closes A2 (remaining gap) in
`docs/sync-and-op-log/sqlite-migration-followup.md`.
* docs(sync): mark A1 + A2 shipped in sqlite-migration followup (#7925)
A1 (storage-persistence diagnostics) and A2 (debounced data-change backup
trigger) both landed this round. Update the suggested order and the A2
section so the doc accurately reflects what's left in Track A (just A3,
the near-empty write-time overwrite guard).
* feat(local-backup): near-empty write-time overwrite guard (A3, #7925)
The exact-empty guard in `_backup()` only catches a fully-degraded store.
The residual gap is a post-eviction boot that leaves the store near-empty,
the user adds 1-2 tasks before the 5-min timer fires, and the degraded
state then overwrites the good primary slot. The prev slot is still safe,
but the informed restore prompt only fires on a wholly fresh launch — so
without this guard the user has lost direct access to the better backup
until they uninstall/reinstall.
Add a per-platform near-empty guard in `_backupAndroid` / `_backupIOS`:
read the existing primary, compare task counts (active + young-archived +
old-archived via the new shared `countAllTasks` helper), and bail when a
< 3-task snapshot would clobber a >= 10-task existing backup. Electron is
unchanged — its rotated, timestamped backup chain isn't a single-slot
overwrite.
Threshold rationale: `summarizeBackupStr` counts archived tasks too, so
"near-empty" means the same thing on the read side (the restore prompt)
and the write side (this guard). Fail-safe — skipping a write only delays
capturing a real wipe, never loses data; the guard self-clears once the
store grows back past 3 tasks, so a legitimate bulk-delete is captured
on the next tick.
Marks Track A (#7925 / sqlite-migration-followup.md) complete.
* fix(android): JSONObject.quote() the sibling JS bridge callbacks (#7925)
`saveToDbCallback` / `removeFromDbCallback` / `clearDbCallback` still raw-
interpolated `requestId` into single-quoted JS string literals. The args are
nanoid strings today so it works — but only by caller hygiene. Mirror the
`loadFromDb` fix: quote all three so the bridge contract no longer depends
on what the caller happens to pass.
Compress the `loadFromDb` rationale comment in the process — the file-level
intent now lives on one line near the cluster.
* refactor(local-backup, startup): trim Track A code per multi-agent review
Two independent reviewers flagged the same set of cleanups on the Track A
commits (#7925). Applying the high-confidence ones:
- Drop `_escapeAndroidNewlines` + its two call sites. The Kotlin bridge fix
(#7925,
|
||
|
Johannes Millan
|
4c239e5691
|
refactor(op-log): extract a swappable persistence port + SQLite backend (groundwork for #7892) (#7902)
* docs(sync): add SQLite migration plan + Phase A adapter port skeleton Documents the op-log persistence migration off WebView IndexedDB into app-private SQLite on native (Capacitor), addressing the data-loss class where Android can evict WebView storage when no sync is configured. Phase A skeleton (no behavior change, not yet wired in): - OpLogDbAdapter / OpLogTx: backend-agnostic persistence port with a callback-based transaction() as the atomicity seam both IndexedDB and SQLite map onto. - OP_LOG_DB_SCHEMA: declarative SUP_OPS schema descriptor (mirrors db-upgrade.ts v6) that both backends can consume. https://claude.ai/code/session_011wcqZgubKqoT6wxt1L1KBT * feat(op-log): add IndexedDbOpLogAdapter implementing the persistence port Phase A continuation of the SQLite migration (docs/sync-and-op-log/ sqlite-migration.md). Implements the IndexedDB backend behind the OpLogDbAdapter port: open-retry with the existing budgets, versionchange/ close re-open handling, IndexedDBOpenError wrapping, index/range queries, and a callback-based transaction() that commits on resolve and aborts on throw — the atomicity seam both backends share. Extends the port with cursor-style iterate() (continue/stop/delete/ delete-stop) to cover the latest-entry lookups and predicate pruning the store does today, plus a close() teardown hook. Spec exercises CRUD, the unique byId index, range queries, cursor direction/stop/delete, and — critically — multi-store transaction commit and rollback against fake-indexeddb. 10/10 pass. Not yet wired into OperationLogStoreService; additive scaffolding only. https://claude.ai/code/session_011wcqZgubKqoT6wxt1L1KBT * refactor(op-log): harden persistence port after multi-agent review Addresses blocking fidelity gaps found reviewing the adapter against the real store's usage, so the upcoming store refactor can be behavior- preserving: - iterate() visitor is now synchronous and receives the primary key. An async visitor could await real I/O mid-cursor, letting the IDB transaction auto-commit and the next continue() throw TransactionInactiveError. Synchronous-only also lets a buffered SQLite backend honor it without materializing the whole result set. - DbIterateOptions.query positions an index cursor at an exact key (clearFullStateOpsExcept's keyed delete). - getAll()/count() take an optional primary-key range (getOpsAfterSeq and the getUnsynced/getAppliedOpIds incremental caches use getAll(OPS, lowerBound(seq))). - getKeyFromIndex() for cheap existence probes (appendBatchSkipDuplicates' getKey, avoids deserializing the value). Tests expanded 10 -> 26: destructive clear()+delete() rollback, abort on inner-op rejection, transactional reads/index/cursor, readonly mode, keyed index iteration, compound-index match, getAll/count ranges, the close() re-open cliff, and the open-retry budgets via the _openDbOnce seam. https://claude.ai/code/session_011wcqZgubKqoT6wxt1L1KBT * refactor(op-log): route import-backup methods through the persistence adapter First method group of the Phase A store migration. Adds an adoptConnection() seam so IndexedDbOpLogAdapter operates on the store's existing connection rather than opening a second one to SUP_OPS (avoiding versionchange deadlocks and doubled close/upgrade handling during the transition). The store adopts/releases the connection alongside its own _db in init()/close/versionchange. saveImportBackup / loadImportBackup / clearImportBackup / hasImportBackup now go through the adapter. Behavior is identical — same connection, same store, same keys. Verified: 170 store unit specs, 26 adapter specs, 3 archive specs, and the import-sync integration spec all green. https://claude.ai/code/session_011wcqZgubKqoT6wxt1L1KBT * refactor(op-log): route state_cache + compaction methods through the adapter Second method group of the Phase A store migration. saveStateCache, loadStateCache, the migration-safety backup methods (save/load/clear/has/ restore), and the compaction counter (get/increment/reset) now go through the shared adapter. The two atomic read-modify-write methods (incrementCompactionCounter, resetCompactionCounter) use the adapter's callback transaction(), preserving their single-transaction semantics. Introduces a StateCacheEntry type; `id` is optional so the read-side return types stay assignable from the looser snapshot shapes callers construct (the pre-migration return types didn't surface `id`). Verified: 170 store unit, 53 compaction unit, 27 vector-clock, 20 compaction integration specs all green; full tsc clean. https://claude.ai/code/session_011wcqZgubKqoT6wxt1L1KBT * docs(sync): track Phase A migration progress in sqlite-migration.md https://claude.ai/code/session_011wcqZgubKqoT6wxt1L1KBT * refactor(op-log): route ops-table append + markApplied through the adapter Third method group of the Phase A store migration — the higher-risk write path. append, appendBatch, appendBatchSkipDuplicates and markApplied now go through the shared adapter. The batch methods use the adapter's callback transaction() (one atomic unit, same as before); the TOCTOU-free duplicate guard uses tx.getKeyFromIndex (the byId unique index probe, issue #6343). ConstraintError->DUPLICATE and QuotaExceededError-> StorageQuotaExceededError mappings are preserved — the adapter rethrows the original DOMException so the store's catch blocks still fire. Verified: 170 store unit + 367 op-log integration specs green; tsc clean. https://claude.ai/code/session_011wcqZgubKqoT6wxt1L1KBT * refactor(op-log): route ops-table reads + full-state clears through adapter Fourth method group. getPendingRemoteOps (compound-index match expressed as a degenerate [k,k] range, with the pre-v3 fallback scan preserved), hasOp, getOpById, getOpsAfterSeq (primary-key range), the two reverse- cursor latest-full-state lookups, and clearFullStateOps/ clearFullStateOpsExcept now go through the adapter's iterate()/getAll()/ getAllFromIndex(). The keyed-index-cursor delete is factored into a _deleteOpsByIds() helper using iterate({index, query}) + delete-stop in a single atomic transaction, matching the prior behavior (no-op + no cache invalidation on empty list). Verified: 170 store unit + 367 op-log integration specs green; tsc clean. https://claude.ai/code/session_011wcqZgubKqoT6wxt1L1KBT * refactor(op-log): route unsynced/applied caches + mark methods through adapter Fifth method group. The getUnsynced/getAppliedOpIds incremental cache builds (getAll with a primary-key range), getFailedRemoteOps (compound index), markSynced/markRejected/clearUnsyncedOps/markFailed (transactional get+put loops), deleteOpsWhere (predicate cursor delete) and getLastSeq (reverse cursor reading the primary key via the iterate visitor's key arg) now go through the adapter. markFailed keeps its original behavior of NOT invalidating the unsynced cache. Verified: 170 store unit + 367 op-log integration specs green; tsc clean. https://claude.ai/code/session_011wcqZgubKqoT6wxt1L1KBT * refactor(op-log): route remaining store methods through adapter Final OperationLogStoreService group — every method now goes through the persistence adapter; no direct `this.db` calls remain. Covers hasSyncedOps (bySyncedAt index cursor), clearAllOperations, _clearAllDataForTesting (multi-store clear in one transaction), the vector-clock accessors, and the two flagship atomic flows: - appendWithVectorClockUpdate (OPS + VECTOR_CLOCK in one transaction) - runDestructiveStateReplacement (OPS + STATE_CACHE + VECTOR_CLOCK + CLIENT_ID + archive). The hand-rolled try/abort is replaced by the adapter's commit-on-resolve / abort-on-throw transaction(); success-only cache + clientId-cache invalidation now runs after the resolved transaction. The #7709 interrupt atomicity tests still pass — the adapter operates on the same adopted connection the tests spy on, so a poisoned opsStore.add still aborts and unwinds the queued clientId rotation. Verified: 170 store unit + 367 op-log integration specs (incl. the 3 clean-slate-interrupt atomicity tests) green; tsc clean. https://claude.ai/code/session_011wcqZgubKqoT6wxt1L1KBT * refactor(op-log): route ArchiveStoreService through the persistence adapter Completes the Phase A store/archive migration. ArchiveStoreService gets its own IndexedDbOpLogAdapter that adopts its independent SUP_OPS connection (released on close/versionchange and on the iOS connection-closing retry path in _withRetryOnClose). All six accessors plus saveArchivesAtomic/_clearAllDataForTesting now go through the adapter; the dead `db` getter and its unused error constant are removed. No direct `this.db` calls remain in either persistence service. Verified: 3 archive unit + 170 store unit + 367 op-log integration specs green; tsc clean. https://claude.ai/code/session_011wcqZgubKqoT6wxt1L1KBT * refactor(sync): fix readonly cursor regression + multi-review findings Multi-agent review of the Phase A op-log adapter (post full store/cursor migration). Addresses one live regression plus hardening; no functional behavior change. W1 (live regression fix): the migrated read-only cursor methods — getLastSeq, hasSyncedOps, getLatestFullStateOp(Entry) — ran through iterate(), which always opened a 'readwrite' transaction, so pure reads on the hot ops store took an exclusive write lock and serialized against appends (pre-migration they were 'readonly'). Add `mode` to DbIterateOptions (default 'readwrite' so delete-walks keep working) and pass `mode:'readonly'` from those four readers; clearFullStateOps* delete-walks stay readwrite. W2: op-log-db-schema reuses DB_NAME/DB_VERSION from db-keys.const instead of re-literaling 'SUP_OPS'/6 (no third source of truth), and a new op-log-db-schema.spec.ts asserts the descriptor matches both DB_VERSION and the stores/indexes runDbUpgrade actually creates (the contract Phase B builds on). W3: test the adoptConnection seam (both branches) — ops route onto an adopted external connection, and adoptConnection(undefined) returns to the not-initialized cliff (the store's close/versionchange path). W4: convert the two open-retry specs from real ~8s backoff sleeps to fakeAsync + tick (adapter spec ~0.04s vs ~8s) and assert exact attempt budgets; add a full-lock-budget case. Gates: adapter 30 + schema 2 + store 170 unit, and 59 op-log integration specs (race-conditions, multi-entity-atomicity, compaction, server-migration, clean-slate-interrupt, indexeddb-error-recovery) green; checkFile clean. * refactor(op-log): inject the persistence adapter via DI token Phase B step 1. Both persistence services now obtain their OpLogDbAdapter from OP_LOG_DB_ADAPTER_FACTORY instead of constructing IndexedDbOpLogAdapter directly. The token vends a factory (not a singleton) because each service adopts its own connection into its own adapter instance. Defaults to IndexedDB on all platforms; Phase B step 2 will override it to return a SqliteOpLogAdapter when running native, with the stores untouched. adoptConnection() becomes an optional bridge method on the OpLogDbAdapter interface — documented as IDB-transition-only; a self-managing backend (SQLite) leaves it undefined and callers guard with `?.()`. Verified: 170 store unit + 3 archive unit + 367 op-log integration specs green; tsc clean. https://claude.ai/code/session_011wcqZgubKqoT6wxt1L1KBT * feat(op-log): add SqliteOpLogAdapter skeleton (Phase B, no native dep) Dependency-free skeleton of the SQLite backend behind the OpLogDbAdapter port. Solves the hard schema-mapping question the reviewers flagged without pulling in a native plugin or anything untestable in CI: - planTables()/buildDdl(): derive the physical SQL layout from the shared OP_LOG_DB_SCHEMA. Each store -> a table with a JSON `value` column plus one extracted column per IDB index. ops gets `seq INTEGER PRIMARY KEY AUTOINCREMENT` (monotonic, never-reused — matches IDB + getLastSeq), `op_id TEXT UNIQUE` (byId), `synced_at` (bySyncedAt) and a composite (source, application_status) index. keyPath stores -> TEXT PK from the keyPath; keyless singletons -> caller-supplied TEXT key. - A minimal SqliteDb port (run/query) the adapter talks to instead of importing @capacitor-community/sqlite, so this file has no native dependency and is unit-testable with a fake. - init() applies the DDL (idempotent); query/tx methods throw a loud not-implemented error (fail loudly rather than silently lose data) with the intended SQL documented per method. adoptConnection is intentionally absent — SQLite self-manages its handle. 12 specs cover the plan/DDL derivation and that init() emits the expected DDL. Doc updated with status + the deferred native-dependency decision. https://claude.ai/code/session_011wcqZgubKqoT6wxt1L1KBT * feat(op-log): fully implement SqliteOpLogAdapter (still no native dep) Completes the SQLite backend behind the OpLogDbAdapter port. All query, index, range, count, cursor-iterate and transaction methods are now implemented against the minimal SqliteDb port: - value→column extraction: each store row stores the JSON object in a `value` column plus extracted columns for the indexed paths (op_id/synced_at/source/application_status); writes populate them. - transactions map to BEGIN IMMEDIATE/COMMIT/ROLLBACK with rollback-on- throw; readonly iterate/transaction use no write lock. - SQLite errors map to the SAME DOMException names the store's existing catch blocks expect: UNIQUE→ConstraintError (→DUPLICATE_OPERATION_ERROR), disk-full→QuotaExceededError (→StorageQuotaExceededError). - ops uses AUTOINCREMENT so seq is monotonic and never reused across clear() — matching IDB + getLastSeq. Still imports no native plugin: a thin wrapper over @capacitor-community/sqlite's SQLiteDBConnection will satisfy SqliteDb on device. 23 specs validate the translation layer + transaction semantics (commit/rollback/abort-on-unique) against an in-memory SQLite stand-in; a real-engine on-device run is the remaining Phase B step (documented). https://claude.ai/code/session_011wcqZgubKqoT6wxt1L1KBT * docs(sync): add SQLite migration follow-up backlog Actionable, ordered backlog companion to sqlite-migration.md: - Track A: ship the #7892 safeguards now (persist() diagnostics + native filesystem auto-backup) — independent of SQLite, recommended near-term fix. - Track B: finish the native SQLite backend (plugin + SqliteDb wrapper, real-engine validation, DI flip behind a flag). - Track C: one-time IDB→SQLite data migration, staged rollout. - Track D: cleanup once SQLite is the native default. https://claude.ai/code/session_011wcqZgubKqoT6wxt1L1KBT * fix(op-log): scan full-state ops read-only to drop the write lock clearFullStateOps / clearFullStateOpsExcept iterate the ops store only to collect ids (the delete runs in a separate transaction), but the migrated iterate() defaulted to 'readwrite' — so these pure-read scans took an exclusive write lock on the hot ops store and serialized against appends. Pre-adapter (master) these scans used a readonly cursor. Pass mode:'readonly' to restore parity. Same regression class the earlier W1 fix addressed for getLastSeq/hasSyncedOps/getLatestFullStateOp(Entry); these two scans were missed because they are no longer delete-walks. Verified: 170 store unit + server-migration/import-sync/remote-apply/ vector-clock-import integration specs green; checkFile + tsc clean. * fix(op-log): correct SQLite seq round-trip + enforce tx scope and readonly Hardens the dormant SQLiteOpLogAdapter against three multi-review findings (translation-layer only; the backend is still wired to nothing): - C1 (data duplication): the autoinc `ops` PK (`seq`) lived only in its own column, never the JSON value, and was never re-injected on read. So reads returned seq===undefined and put() emitted INSERT…ON CONFLICT(seq) with no seq bound — the conflict never fired and every mark*/clearUnsynced re-put inserted a duplicate row. Now buildInsert binds seq when the value carries one (re-put / explicit-seq add) and decodeRow injects the PK back from a `__pk` alias on every read, matching IDB's keyPath+autoIncrement store. ON CONFLICT no longer overwrites the PK column. - W2 (atomicity scope): transaction() discarded its `stores` argument, so the OpLogTx could touch any store — silently passing where IDB throws. The tx now enforces the declared scope (and inherits the tx mode for iterate). - W3 (readonly contract): a delete action under a readonly iterate executed the DELETE outside any transaction; it now rejects with ReadOnlyError, matching IDB. Also makes the in-memory FakeSqliteDb faithfully model AUTOINCREMENT (honor an explicit seq, upsert on PK conflict, advance the high-water mark) so the spec actually catches C1-class bugs — verified: reverting the seq fix makes the new "updates in place" test fail with the real UNIQUE violation. Verified: 27 SQLite adapter specs (4 new) green; checkFile + tsc clean. * refactor(op-log): strip the autoinc keyPath prefix via extractPath idiom Follow-up review nit: decodeRow stripped the `$.` from the autoinc keyPath with slice(2); use the same `.replace(/^\$\./, '')` idiom as extractPath for consistency, and note that the autoinc keyPath is a top-level field. No behavior change (keyJsonPath is always `$.seq` for the only autoinc store). --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Johannes Millan
|
508998c6a1
|
Improve on sync (#7736)
* fix(android): restore share title derivation and dedupe shared tasks Commit |
||
|
Johannes Millan
|
42e6626b76 |
docs(sync): consolidate sync docs + enforce the contributor model
Collapse the sprawling, partly-stale docs/sync-and-op-log/ tree into a
small authoritative set and make the sync-correctness invariant
partly lint-enforced instead of convention-only.
Docs:
- Delete superseded/duplicate/provably-stale design, plan, and
background-research docs (quick-reference, the architecture-diagrams
monolith, the "Hybrid Manifest" docs describing code that does not
exist, completed long-term plans, LLM-synthesis analyses).
- Salvage load-bearing decision history into the surviving docs before
deletion: rejected-alternatives rationale -> operation-log-architecture
("Why this architecture"); vector-clock pruning incident history ->
vector-clocks.md; archive-payload optimization -> architecture E.7.
- Add contributor-sync-model.md as the single-invariant entry point
(one user intent = one op; replayed/remote ops must not re-trigger
effects), with a decision table mapping to the enforcing linters.
- Repoint external/internal cross-refs; add CONTRIBUTING.md + CLAUDE.md
pointers; record the migration in a dated docs/plans/ design doc.
Enforcement (new eslint-local-rules):
- no-actions-in-effects (error): effects must inject LOCAL_ACTIONS /
ALL_ACTIONS, never the raw @ngrx/effects Actions stream.
- no-multi-entity-effect (warn, heuristic): flags a literal returned
array of >=2 action-creator calls; docstring + valid-case specs pin
exactly which shapes are and are not detected.
- run-specs.js runner wired into `npm run lint` via test:lint-rules;
refuses to run under test-framework globals and counts RuleTester.run
invocations so a spec that asserts nothing fails instead of passing.
- Correct the ALL_ACTIONS JSDoc in local-actions.token.ts to match
reality (archive-operation-handler uses LOCAL_ACTIONS).
Reviewed via parallel multi-agent review; findings W1/W2/W4 and a
dangling doc anchor addressed.
|
||
|
Johannes Millan
|
087b9dd43f
|
refactor(sync): post-extraction review cleanup of @sp/sync-core and @sp/sync-providers (#7595)
* refactor(sync): tighten extracted package surfaces
Combined polish from the post-extraction review:
- sync-core: strip NgRx-shaped types from EntityConfig/EntityRegistry;
expose host extensions via generic param. Move StateSelector,
PropsStateSelector, SelectByIdFactory, SelectById, EntityUpdateLike,
EntityAdapterLike to a new app-side entity-registry-host.types.ts.
- sync-core: mark OpType.SyncImport/BackupImport/Repair as @deprecated;
hosts should use createFullStateOpTypeHelpers().
- sync-providers: resolve provider.types.ts vs provider-types.ts
duplication; inline implementation into the dashed canonical name.
- sync-providers: drop unused root barrel and "." export; consumers
already use focused subpath barrels (/dropbox, /webdav, etc.).
- sync-providers: replace wildcard "@sp/sync-providers/*" tsconfig path
alias with 11 explicit subpath entries matching package.json exports;
deep-internal imports now fail at typecheck.
- sync-providers: move @sp/sync-core from dependencies to
peerDependencies (kept in devDependencies for tests).
- both packages: add composite: true to enable project references;
introduce tsconfig.build.json overlay so tsup DTS bundler still works.
- gitignore: ignore **/*.tsbuildinfo composite outputs.
* refactor(sync-core): prune 47 unused barrel exports
Removes exports with zero consumers outside the package. Source files
are unchanged; only the public barrel is trimmed. Covers compression
helper classes, sync-file-prefix error/config types, replay coordinator
internals, remote-apply result types, upload/download planning option
and plan types, ports misc, conflict-resolution helper types, and
sync-import-filter decision types.
* refactor(sync-core): drop unused encryption migration path
decryptWithMigration and DecryptResult had no host consumer; they
exposed a structural-migration entry point ("here is your ciphertext
re-encrypted under Argon2id") that nothing in the codebase reads. The
side-channel setLegacyKdfWarningHandler — which IS used — stays.
encryptWithDerivedKey/decryptWithDerivedKey lose their export keyword
and remain as module-internal helpers; encrypt/decrypt/encryptBatch/
decryptBatch still call them. Wire format and legacy-fallback semantics
are unchanged, so existing ciphertext continues to decrypt.
Test imports for compression and sync-file-prefix specs now go via
their source files instead of the trimmed barrel.
* fix(sync-providers): bound dropbox token refresh to single retry; share md5 rev helper
The five hand-rolled token-refresh blocks in Dropbox.{getFileRev,
downloadFile, uploadFile, removeFile, listFiles} recursed on themselves
after refresh. If the post-refresh call still saw a token error (real
case: the refresh token itself was revoked), the recursion would not
terminate. Consolidated into a single _withTokenRefresh helper that
attempts the call, refreshes once on a token error, retries once, then
lets the outer 401 classifier surface AuthFailSPError.
Same log message, same _isTokenError discriminator, same refresh call.
Same five sites still apply their post-call non-token error mapping
(NoRev, InvalidData, RemoteFileNotFound, path-not-found swallow, etc.).
Also extracts md5 content-rev computation duplicated between
LocalFileSyncBase._getLocalRev and WebdavApi._computeContentHash into a
shared file-based/content-rev.ts; both call sites preserve their own
error wrapping at the boundary.
* refactor(sync): split oversized super-sync and conflict-resolution
sync-providers: extract request-ID hashing from super-sync.ts (1017 ->
918 lines) into a new request-id.ts. The helpers were free functions
already in disguise (none referenced this), so the move is mechanical.
HTTP plumbing (_doWebFetch/_doNativeFetch/_fetchApi*) stays as private
methods — it transitively touches 12 instance members and would need
either a wide context object or a separate http-client collaborator
class to extract cleanly. Left as a follow-up.
sync-core: split conflict-resolution.ts into three cohesive files:
- entity-frontier.ts now owns buildEntityFrontier and
adjustForClockCorruption (per-entity vector-clock domain).
- extractEntityFromPayload and extractUpdateChanges move to
operation.types.ts next to the existing extractActionPayload.
- conflict-resolution.ts keeps deep-equality, LWW planning,
partitioning, and identical-conflict detection.
Public barrel exports unchanged; tests now import the moved symbols
from their new homes.
* refactor(sync-core): drop redundant OperationStorePort
OperationStorePort overlapped with RemoteOperationApplyStorePort on the
two state-transition methods (markSynced/markApplied,
markRejected/markFailed) and had zero non-structural consumers — the
only implementer was OperationLogStoreService, which already exposes
the three methods as its own public surface. Removing the port leaves
the service contract intact and removes the verb-pair confusion noted
in the post-extraction review.
Spec contract test still drives the same state transitions; only the
local typing of the test fixture changes from the deleted interface to
Pick<OperationLogStoreService, ...>.
* refactor(sync-providers): decouple SuperSync provider from SP-specific host
Two coupling leaks the package shouldn't carry:
1. SUPER_SYNC_DEFAULT_BASE_URL was an implicit fallback inside
SuperSyncProvider — an SP-specific URL baked into a "framework-
agnostic" package. Make defaultBaseUrl a required SuperSyncDeps
field; the host factory supplies the SP default. The constant stays
exported as a suggested default for hosts targeting the SP-hosted
server.
2. Consumers that wanted the WebSocket path had to do
`provider as unknown as SuperSyncProvider` to call
getWebSocketParams. Introduce SuperSyncWebSocketAccess interface +
isSuperSyncWebSocketAccess structural guard; SuperSyncProvider
implements it. sync-wrapper.service drops its cast in favor of the
guard.
super-sync-restore.service still casts to SuperSyncProvider for the
restore path — same pattern would solve it, but out of scope here.
* test(sync-providers): extract shared test helpers and prefer barrels
Adds tests/helpers/sync-logger.ts and tests/helpers/credential-store.ts
to centralize the noopLogger and CredentialStore mocks that were copy-
pasted across 8 spec files. createStatefulCredentialStore covers the
"load/upsert/clear with state" cases; createMockCredentialStore covers
bare vi.fn() ports. Spec sites that needed a unique mockResolvedValue
chain it after the helper, preserving behavior 1:1.
Also migrates 5 spec files from deep ../src/<file> paths to the
matching sub-barrel (../src/webdav, /http, /super-sync, /platform) for
symbols already exported there. No new barrel exports added — internal
types (WebDavHttpAdapter, WebdavApi, DropboxApi, etc.) stay on deep
paths because they are intentionally not part of the public surface.
super-sync.spec.ts keeps its own credential/logger mocks (special
__asPort wrapper and vi.spyOn against the live NOOP_SYNC_LOGGER) that
the generic helpers cannot reproduce without bloat.
* test(sync): pin vector-clock pruning, error-meta privacy, and sync-import edges
Fills three test gaps surfaced by the post-extraction review:
- vector-clock pruning correctness across clocks: 4 cases pinning that
pruning legitimately flips GREATER_THAN to CONCURRENT/LESS_THAN when
the dropped keys are still present in the comparison clock. This is
the documented behavior (compareVectorClocks is intentionally not
pruning-aware); the protocol handles flips server-side via the
rejected-ops retry loop. preserveClientIds case also covered.
- error-meta privacy boundary: 22 new cases covering urlPathOnly (strip
query/fragment/userinfo, preserve host+path+port, leave non-URLs
intact) and errorMeta (no leakage of headers, response bodies, OAuth
tokens, signed-URL params, user emails, or attached error fields).
Real negative assertions (.not.toContain), not shape checks.
- sync-import-filter edge cases: 8 cases covering empty clocks on
either side, op clock listing the import client at 0, same-client
with equal counter (pinning the strict-greater-than boundary), and
different-client knowledge above the import counter.
sync-core 195 -> 207 tests, sync-providers 319 -> 341 tests; no
production code changed.
* style(sync-core): format sync-file-prefix.spec import line
* fix(sync): address package review feedback
|
||
|
Johannes Millan
|
00098f52fb | refactor(sync-providers): add tiered package exports | ||
|
Johannes Millan
|
5fa8260bb8 | refactor(sync): finish package extraction polish | ||
|
Johannes Millan
|
8005b4ec52 |
feat(supersync): retry transient web fetch failures and surface them as warnings
Adds a 2-retry / 1s+2s backoff loop to the browser/Electron SuperSync
request path, mirroring the existing native retry. A new typed
NetworkUnavailableSPError replaces the previous string-shape contract
between provider and wrapper: the provider throws the typed error from
both web-retry exhaustion and native-failure handler, and the wrapper
matches via instanceof to show a transient WARNING snackbar
(F.SYNC.S.NETWORK_ERROR) without flipping into a hard ERROR state.
Drops the wrapper-side regex classifier (and its defensive
^HTTP\s+\d{3}\b filter), the now-unused isTransientNetworkError alias,
and dedupes the predicate call inside the web-retry catch. The single
remaining call site for the broad regex (operation-log-upload) now
imports isRetryableUploadError directly under its honest name.
Tests cover the 1s/2s cadence, retry-then-success, retry-exhaustion via
the typed error, and negative paths (AbortError, HTTP-status 5xx,
AuthFailSPError must NOT retry). The error class is added to the
cross-module identity safety net.
|
||
|
Johannes Millan
|
9fd9d386a8 | refactor(sync): move vector clocks to sync-core | ||
|
Johannes Millan
|
9eaf266e3d |
docs(sync): align SYNC_IMPORT scenarios with current gate semantics
Rename stale _hasMeaningfulLocalData() refs to _hasMeaningfulStoreData() and remove the dead Encryption-only flowchart node — PASSWORD_CHANGED SYNC_IMPORTs without pending ops now fall through the standard gate. |
||
|
Johannes Millan
|
3fc1bb6e94 |
refactor(sync): tighten SYNC_IMPORT gate naming and inline single-use helper
Inline _hasAnyMeaningfulData at its remaining caller (the snapshot/provider- switch path) and rename _hasMeaningfulLocalData to _hasMeaningfulStoreData for parallel naming with _hasMeaningfulPendingOps. Strengthen the silent- accept piggyback test to assert kind === 'completed' rather than \!== 'cancelled', and clarify the originating-device cross-reference in the piggyback (D.6) doc and the IMPORT_CONFLICT diamond in the flowchart. |
||
|
Johannes Millan
|
9d3cf64986 |
fix(sync): apply incoming SYNC_IMPORT silently with no pending ops
Receiving clients with only already-synced data (no unsynced pending
changes) used to see a conflict dialog when an incoming SYNC_IMPORT
arrived. If the user picked USE_LOCAL — a natural reaction to "your
data may be lost" — forceUploadLocalState() re-uploaded the pre-import
state as a new SYNC_IMPORT, rolling back the import (e.g. encryption
change) for every device.
The originating device already gates the SYNC_IMPORT behind a strong
warning (D_SERVER_MIGRATION_CONFIRM,
|
||
|
Johannes Millan
|
b6b51707d3 |
docs: clean up and organize project documentation
- Remove outdated feature requests from .github/CONTRIBUTING.md (GitLab support already exists) and add commit message format section - Improve PR template with type-of-change checkboxes and checklist - Update commit guideline links in README and CONTRIBUTING.md to reference the project's own format instead of external angular.js docs - Add "only edit en.json" rule to TRANSLATING.md and clarify workflow - Update add-new-integration.md provider list to match codebase (add Trello, ClickUp, Linear, Azure DevOps, Nextcloud Deck; note GitHub plugin migration; fix type name to BuiltInIssueProviderKey) - Add cross-references between mac certificate docs and remove 240-line duplicate section from update-mac-certificates.md - Clean up update-android-app.md (specify npm version args, collapse deprecated workflow, translate German UI labels to English) - Add context to howto-refresh-snap-credentials.md - Fix fine-grained token note in github-access-token-instructions.md - Fix absolute URL to relative path in gitlab-access-token-instructions.md - Fix grammar in i18n-script-usage.md - Add status headers to all 19 long-term plan files (Planned, Completed, Archived with reason, Investigation Complete) - Fix broken relative link in hybrid-manifest-architecture.md - Delete supersync-scenarios-simplified.md (duplicate of supersync-scenarios.md; known issues already covered there) - Rename vector-clock-pruning-research.md to vector-clock-history-and-alternatives.md for clarity |
||
|
Johannes Millan
|
ab8b577c10 |
docs(sync): add file-based sync flowchart for Dropbox/WebDAV/LocalFile
Parallel to the SuperSync flowchart, covering the file-based sync decision tree: gap detection, snapshot hydration, rev-based upload retry, and error handling. Verified against source code with matching abstraction level to the SuperSync chart. |
||
|
Johannes Millan
|
058c26594b |
docs(sync): fix structural inaccuracies in supersync scenarios flowchart
Correct the flowchart to match actual codebase behavior: - Move fresh-client dialogs under the "has remote ops" branch (was incorrectly under "no remote ops") - Split single password dialog into two distinct decrypt error dialogs (DecryptNoPasswordError vs DecryptError) - Route SYNC_IMPORT conflicts to ImportConflictDialog (was incorrectly using SyncConflictDialog) - Add encryption-only change bypass for password-change SYNC_IMPORTs - Add LWW tie-breaking details (remote wins on tie, archive ops always win) - Add retry limit note on re-download, correct "Cancel" to "Disable SuperSync" - Show silent server migration path for fresh clients with local data on empty server |
||
|
Johannes Millan
|
b820055b3e |
docs(sync): distinguish dialog types and highlight actions in flowchart
Rename generic "Conflict dialog" labels to SyncConflictDialog and ImportConflictDialog to reflect the two distinct components. Add orange action styling for key state-changing nodes (apply, force upload/download, enable encryption, upload). |
||
|
Johannes Millan
|
dafee46f7a |
docs(sync): add encryption architecture and scenario documentation
Add comprehensive documentation for the encryption and sync features: - Add SuperSync encryption architecture document - Add SuperSync scenario spec with flowcharts - Add simplified scenario reference - Add SuperSync client simplification plan |
||
|
Johannes Millan
|
9328156ca1 |
fix(sync): add vector clock pruning to compaction and update docs
Add limitVectorClockSize to OperationLogCompactionService._doCompact() which was the remaining saveStateCache caller that persisted unpruned clocks. Update vector-clocks.md exhaustive pruning table with the three new client-side pruning locations. Add boundary tests at exactly MAX_VECTOR_CLOCK_SIZE for snapshot and hydrator services. |