diff --git a/docs/plans/2026-05-22-document-mode-delta-sync.md b/docs/plans/2026-05-22-document-mode-delta-sync.md deleted file mode 100644 index 44644fd064..0000000000 --- a/docs/plans/2026-05-22-document-mode-delta-sync.md +++ /dev/null @@ -1,216 +0,0 @@ -# Document Mode — delta sync architecture - -**Status:** proposal -**Date:** 2026-05-22 -**Branch:** `feat/how-fat-is-data-model-for-sync-for-new-fbd044` -**Supersedes:** the "Future work — per-context sync entities" section of -[`2026-05-22-document-mode-sync-data-model.md`](./2026-05-22-document-mode-sync-data-model.md) -(that doc's Phase 1 — bare-atom chips — remains the immediate, separately-tracked fix) - -## Problem - -The document-mode plugin (`packages/plugin-dev/document-mode/`) is a TipTap -editor that persists **all** its data as a single opaque JSON blob via -`PluginAPI.persistDataSynced(string)`. The blob is: - -```jsonc -{ "version": 1, - "docs": { "": , ... }, // one doc per project/tag/TODAY - "enabledCtxIds": ["..."] } -``` - -Host-side this becomes **one** `PluginUserData` entry with `entityId = pluginId`. -Every save is one op (`upsertPluginUserData`, `entityType: PLUGIN_USER_DATA`, -`opType: Update`) whose payload embeds the **entire blob string**. So the data -sent per change is the whole multi-context corpus (~26–48 KB encrypted for a -typical 5-context user, measured) — **regardless of how tiny the edit was**. -Saves are throttled ~1 op / 2 s while typing (`SAVE_THROTTLE_MS` in -`src/ui/editor.ts`; the host additionally coalesces via -`MIN_PLUGIN_PERSIST_INTERVAL_MS`). - -This is **not a delta**. Typing one character re-transmits, re-encrypts, and -re-stores every project and tag document. Phase 1 of the sibling doc shrinks the -blob ~46% by stripping redundant chip content, but the unit transmitted is still -"the whole corpus". The deeper question — *can a change send only the change?* — -is what this doc addresses. - -## Goals - -1. Lay out the design space for transmitting **less than the whole corpus** per - edit, with honest effort/payoff for each option. -2. Identify which options are compatible with the op-log's partial ordering and - conflict model (a hard constraint — see below). -3. Recommend a staged path: what to do now, mid-term, and long-term. - -## Non-goals - -- Re-deriving the Phase 1 (bare-atom chip) work — already planned and tracked in - the sibling doc; it is orthogonal and complementary to everything here. -- Committing to an implementation in this branch. This is an architecture - proposal for discussion. -- Removing the in-tree `src/app/features/document-mode/` feature. - -## Why naive deltas do not work here - -The op-log is **partially ordered and conflict-resolved**. Vector clocks reorder -ops; `SYNC_IMPORT` / `BACKUP_IMPORT` deliberately drop concurrent ops -(`CONCURRENT` / `LESS_THAN` by vector clock — by design, per the sync model). -A position-dependent patch — "insert these 3 chars at offset 4012" — is only -valid against the exact base state it was computed from. Once another op is -interleaved ahead of it, the offset is wrong and the patch corrupts the -document. - -This is precisely why the op-log replays **semantic action payloads**, never -text diffs: an action like "set task title to X" is replayable against any -state. So a *valid* delta for this system must be **either**: - -- a **semantic operation** that is replayable against any document state, **or** -- a **commutative CRDT update** that composes correctly under any order. - -A line/character/JSON-position patch is neither. This rules out the "obvious" -delta — diff the old and new blob — outright. - -## The granularity spectrum - -| Granularity | Delta unit | ~Per change sent | Effort | -| --- | --- | --- | --- | -| Whole blob (today) | entire corpus | ~26–48 KB | — | -| Per-context entity | one document | ~6–9 KB | moderate (host API change) | -| Per-block entity | one paragraph | ~hundreds of B | high (stable block ids) | -| ProseMirror steps / Yjs CRDT | the actual edit | tens of B | major (new sync channel) | - -Each row below the first is a real option. They are not mutually exclusive — -per-context is a stepping stone, not a dead end. - -### Option A — per-context sync entities (document-level "delta") - -Give each `(plugin, context)` pair its own sync entity, so editing project X's -document transmits only project X's document, not the whole corpus. - -**What it needs:** - -1. **Keyed plugin-persistence API.** `persistDataSynced` is currently - single-arg (`persistDataSynced(dataStr: string)` in - `packages/plugin-api/src/types.ts`, line 555; `loadSyncedData()` line 557 — - verified). Add an optional `key`: `persistDataSynced(data, key?)` / - `loadSyncedData(key?)`, threaded through the whole chain — `plugin-api/types.ts`, - `plugin-bridge.service.ts`, the iframe wrapper (`plugin-api.ts`), and the - iframe postMessage util (`plugin-iframe.util.ts`, which currently drops a - second argument). -2. **Composite entity id.** `PluginUserData.id` becomes `pluginId:key`, so - concurrent edits to *different* contexts stop colliding on one entity. -3. **Virtual-entity LWW support — required, not optional.** `PLUGIN_USER_DATA` - is registered as a **`virtual`** entity in - `src/app/op-log/core/entity-registry.ts` (lines 318–322, verified), and - `ConflictResolutionService.getCurrentEntityState` - (`src/app/op-log/sync/conflict-resolution.service.ts`, lines 805–873) has - branches for adapter / singleton / map / array entities but **no `virtual` - branch** — it falls through and returns `undefined`. So the LWW local-win - path (`_createLocalWinUpdateOp`) cannot read the entity and produces no - replacement op. **LWW conflict resolution cannot resolve plugin-data - conflicts at all today.** Per-context entity ids alone do not fix this; a - same-context concurrent edit still mis-resolves. Conflict resolution must - learn to read a virtual entity from `selectPluginUserDataFeatureState`. - -**Payoff:** ~5x smaller payload per typical edit (~26–48 KB → ~6–9 KB). This is -a document-level "delta" — only the *edited document* is transmitted, never -sub-document. Concurrent edits to different contexts stop conflicting entirely. - -**Limits:** a single document is still sent whole on every keystroke-batch. -Concurrent edits to the *same* document still resolve whole-doc (LWW, once the -virtual branch exists). This is the realistic, moderate-effort win. - -### Option B — per-block sync entities - -Make each top-level block (paragraph, heading, list) its own keyed entity, so a -one-paragraph edit transmits one paragraph. - -**What it needs:** stable per-node ids — a TipTap unique-id extension that -assigns and preserves an id across edits. Split, merge, and reorder of blocks -each touch multiple entities (a paragraph split = one update + one create; a -merge = one update + one delete), and the ordering of blocks becomes its own -synced structure. - -**Assessment:** this is **essentially reinventing a block-level CRDT** — stable -identity, structural ops, ordering — but without the convergence guarantees a -real CRDT gives for free. The split/merge/reorder bookkeeping is exactly the -hard part of CRDTs, done by hand. **Not recommended.** If sub-document -granularity is wanted, go straight to Option C, which solves identity and -ordering correctly and gives finer granularity for the same conceptual cost. - -### Option C — Yjs CRDT (true edit-level deltas) - -Integrate `y-prosemirror`. Yjs models the document as a CRDT and emits a small -**binary update** per edit. These updates are **commutative** — they compose -correctly under *any* order — which is exactly the property the op-log's partial -ordering requires and that text/JSON patches lack. - -**How it maps onto the op-log:** - -- Each Yjs update becomes an **append-only** op (`opType: Create`) — never an - Update-that-replaces. The op-log already handles append-only creates well. -- **Conflict resolution becomes a no-op** for these ops: a CRDT converges by - construction, so there is no conflict to resolve. The partial-order / - `SYNC_IMPORT`-drops-concurrent problem disappears — Yjs updates are designed - to be applied in any order, including after gaps. -- **Op-log compaction** snapshots the document via `Y.encodeStateAsUpdate(doc)` - — a single binary blob that supersedes all prior update ops, the CRDT - equivalent of a state snapshot. - -**Cost:** - -- A new dependency (`yjs` + `y-prosemirror`). -- `persistDataSynced` is **replace-only**; Yjs needs an **append** primitive — - a new plugin API such as `appendSyncedDelta(bytes)`. This is a genuinely new - persistence channel, not a parameter addition. -- An op-log path that treats these ops as commutative create-only ops exempt - from conflict detection. - -**Bonus:** Yjs also enables **real-time concurrent editing of the same -document** — currently an explicit Non-goal of the sibling doc. It is the *only* -option here that does. - -**Assessment:** the correct long-term answer and the only path to true -edit-level deltas. But a CRDT sync channel is a significant architecture -initiative — disproportionate for an opt-in POC plugin *today*. It becomes the -right investment if/when real-time co-editing becomes a product goal, or the -plugin ships bundled-by-default and document sync volume matters at scale. - -## Recommendation - -A staged path, each stage independently shippable: - -1. **Now — Phase 1 (bare-atom chips).** Already planned and being implemented - separately (sibling doc). ~46% smaller blob, no schema break, no host change. -2. **Mid-term — Option A (per-context entities).** The realistic document-level - "delta": moderate effort, ~5x payload reduction, and it *also* fixes the - currently-broken plugin-data conflict resolution (virtual-entity LWW). Pick - this up when document mode ships more widely or cross-context conflicts are - observed. -3. **Long-term / conditional — Option C (Yjs).** The only path to true - edit-level deltas; subsumes the conflict problem entirely and unlocks - real-time co-editing. Justified once co-editing is a goal or sync volume at - scale demands it. **Skip Option B** — it is Option C's hard parts without its - guarantees. - -## Risks - -| Risk | Mitigation | -| --- | --- | -| Option A's per-key rate-limit / size-cap weakens the per-plugin flood guard (`MIN_PLUGIN_PERSIST_INTERVAL_MS`, `MAX_PLUGIN_DATA_SIZE` become per-key) | Keep an additional per-*plugin* aggregate cap; see sibling doc's Future-work item 4. | -| Option A uninstall cleanup — `removePluginUserData(pluginId)` deletes only the exact id; `pluginId:*` entries leak | Make deletion prefix-aware (sibling doc Future-work item 5). | -| Option A migration — splitting the legacy single blob into per-key entities | One-time, idempotent, guarded on a meta key's existence (sibling doc Future-work item 7). | -| Option C dependency size / bundle weight | `yjs` + `y-prosemirror` are compact and tree-shakeable; the plugin is opt-in so it does not affect the core bundle for non-users. | -| Option C — exempting CRDT ops from conflict detection could mask a real bug if the wrong ops are routed there | Gate strictly on `entityType` + a dedicated `opType`/actionType; never a general "skip conflict detection" flag. | -| Option C op-log growth before compaction (one op per edit) | Compaction snapshots `Y.encodeStateAsUpdate`; tune `COMPACTION_THRESHOLD` for the higher op rate. | - -## Open questions - -1. Does Option A need to land before Option C, or can Option C replace the - blob entirely in one step? (Likely A first — it is lower-risk and the keyed - API it adds is reusable; but C does not strictly depend on A.) -2. Should the keyed plugin-persistence API (Option A item 1) be designed up - front to also accommodate Option C's `appendSyncedDelta`, so plugins get one - coherent persistence surface rather than two bolt-ons? -3. Is real-time co-editing a desired product direction at all? The answer - decides whether Option C is "long-term" or "never". diff --git a/docs/plans/2026-05-22-document-mode-sync-data-model.md b/docs/plans/2026-05-22-document-mode-sync-data-model.md index 68503fa77b..4fff27f704 100644 --- a/docs/plans/2026-05-22-document-mode-sync-data-model.md +++ b/docs/plans/2026-05-22-document-mode-sync-data-model.md @@ -18,7 +18,7 @@ stored host-side as one `PluginUserData` entry keyed by **plugin id**: ``` Each save → `upsertPluginUserData` → **one op** (`entityType: PLUGIN_USER_DATA`, -`entityId: pluginId`, `opType: Update`) whose payload embeds the *entire* `data` +`entityId: pluginId`, `opType: Update`) whose payload embeds the _entire_ `data` string. Three problems compound: 1. **Every op carries every context.** Typing one character in TODAY's doc @@ -42,10 +42,10 @@ string. Three problems compound: TODAY/TAG). So chips are reconstructable; only the **prose between them** is plugin-owned. -3. **Concurrent edits do not resolve cleanly.** `entityId` is the *plugin id*, +3. **Concurrent edits do not resolve cleanly.** `entityId` is the _plugin id_, so all N documents collapse into one sync entity: device A editing project X and device B editing project Y produce `CONCURRENT` vector clocks on the - *same entity* → a conflict, even though they touched different documents. + _same entity_ → a conflict, even though they touched different documents. Worse, `PLUGIN_USER_DATA` is registered as a **`virtual`** entity (`entity-registry.ts`), and `ConflictResolutionService.getCurrentEntityState` has **no `virtual` branch** — it returns `undefined`. So the LWW local-win @@ -53,7 +53,7 @@ string. Three problems compound: replacement op. LWW does not function correctly for `PLUGIN_USER_DATA`; concurrent edits lose data, and not by a predictable last-writer-wins rule. - *Note:* even today a conflict that drops the blob only loses **prose** — on + _Note:_ even today a conflict that drops the blob only loses **prose** — on reload chips are rebuilt from the host regardless. Problem 3 is therefore a correctness gap, separate from problems 1–2 (size). @@ -66,10 +66,10 @@ string. Three problems compound: ## Non-goals -- **Fixing problem 3 now.** It needs host-side work (per-context entities *and* +- **Fixing problem 3 now.** It needs host-side work (per-context entities _and_ virtual-entity LWW support) and is deferred — see Future work. - **Fine-grained concurrent editing of the same doc.** Two devices editing the - *same* doc's prose will always resolve whole-doc; character-level merge needs + _same_ doc's prose will always resolve whole-doc; character-level merge needs a CRDT (Yjs) and is out of scope. - Removing the in-tree `src/app/features/document-mode/` feature. @@ -79,7 +79,7 @@ The smallest change that fixes problems 1 & 2: stop persisting the title text and `isDone` attr on chips. Store each chip as a **bare identity atom**: ```jsonc -{ "type": "taskRef", "attrs": { "taskId": "" } } // no content, no isDone +{ "type": "taskRef", "attrs": { "taskId": "" } } // no content, no isDone ``` The persisted doc stays an ordinary ProseMirror doc (`type: "doc"`, chips + @@ -87,7 +87,7 @@ prose interleaved) — only the chip nodes get lighter. ### Why this needs no schema bump and no migration -`migrateStoredDoc` was *built* to load atom-shaped chips ("Older docs stored +`migrateStoredDoc` was _built_ to load atom-shaped chips ("Older docs stored taskRef as an atom node (no `content` array)") — it backfills `content` from the task cache and defaults `isDone`. `refreshChipContentFromCache` then overwrites both unconditionally. So a bare-atom chip flows through the **existing, @@ -106,17 +106,17 @@ no `background.ts` change. `stripChipContent` operates on a **copy** — `editor.getJSON()` returns a fresh object each call. The live `editor` document keeps its inline chip content, so -the title-editing path (`reconcileTitlesFromDoc`, which reads the *live* doc) is +the title-editing path (`reconcileTitlesFromDoc`, which reads the _live_ doc) is untouched. Only the bytes written to storage shrink. ### Files -| File | Change | -| --- | --- | -| `src/doc-transform.ts` | Add pure `stripChipContent(doc): unknown` — walk content, replace each `taskRef`/`subTaskRef` node with `{ type, attrs: { taskId } }`, leave every other node (paragraphs, headings, lists — their text!) untouched. | -| `src/ui/editor.ts` | `flushSave` **and** `flushSaveSync` persist `stripChipContent(editor.getJSON())` instead of `editor.getJSON()`. No other change. | -| `src/doc-transform.spec.ts` | Test `stripChipContent` (chips emptied, prose intact); round-trip test (`stripChipContent` → `prepareStoredDoc` rebuilds full chips with titles); legacy content-bearing doc still loads. | -| `src/background.ts` | No change — it treats `docs` as opaque and only writes `enabledCtxIds`. | +| File | Change | +| --------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `src/doc-transform.ts` | Add pure `stripChipContent(doc): unknown` — walk content, replace each `taskRef`/`subTaskRef` node with `{ type, attrs: { taskId } }`, leave every other node (paragraphs, headings, lists — their text!) untouched. | +| `src/ui/editor.ts` | `flushSave` **and** `flushSaveSync` persist `stripChipContent(editor.getJSON())` instead of `editor.getJSON()`. No other change. | +| `src/doc-transform.spec.ts` | Test `stripChipContent` (chips emptied, prose intact); round-trip test (`stripChipContent` → `prepareStoredDoc` rebuilds full chips with titles); legacy content-bearing doc still loads. | +| `src/background.ts` | No change — it treats `docs` as opaque and only writes `enabledCtxIds`. | ### Expected reduction @@ -125,8 +125,8 @@ bare-atom chip is ~60–65 bytes (`taskId` is a 21-char nanoid). For a task-heav context (~60 chips) that is ~5 KB saved per doc; for a typical 5-context user the per-op blob drops roughly 20 KB → ~12 KB. Multiplied across the op-log retention window (up to 500 ops), that is a meaningful cut to IndexedDB volume -and sync transfer. Op *count* is unchanged (the throttles are untouched) — this -is purely a per-op *size* reduction. +and sync transfer. Op _count_ is unchanged (the throttles are untouched) — this +is purely a per-op _size_ reduction. ### Alternative considered — prose-only storage (rejected for now) @@ -143,10 +143,10 @@ if telemetry shows the blob is still too fat. ## Future work — per-context sync entities (host change, deferred) -> Expanded into a dedicated architecture proposal: -> [`2026-05-22-document-mode-delta-sync.md`](./2026-05-22-document-mode-delta-sync.md) -> — covers the full delta-sync granularity spectrum (per-context, per-block, -> Yjs CRDT). The summary below remains accurate for problem 3's host-side scope. +> Tracked as +> [super-productivity/super-productivity#7749](https://github.com/super-productivity/super-productivity/issues/7749) — +> Stage A (keyed plugin-persistence API for per-context sync entities). +> The summary below remains accurate for problem 3's host-side scope. This is the fix for **problem 3**. Deferred, not scheduled: document mode is an opt-in POC plugin, not bundled by default, so cross-context concurrent edits are @@ -156,14 +156,14 @@ It is **larger than "add a `key` parameter"** — the multi-review surfaced the full scope: 1. **Plugin API** — add an optional `key` to `persistDataSynced` / - `loadSyncedData`, threaded through the *entire* chain: `plugin-api/types.ts`, + `loadSyncedData`, threaded through the _entire_ chain: `plugin-api/types.ts`, `plugin-bridge.service.ts`, the iframe wrapper (`plugin-api.ts`), and the iframe postMessage util (`plugin-iframe.util.ts`) — which currently drops a second argument. 2. **Composite entity id** — `PluginUserData.id` becomes `pluginId:key` so each `(plugin, context)` is its own sync entity; concurrent edits to different contexts stop conflicting. -3. **Virtual-entity LWW** — *required, not optional.* Per-context entity ids do +3. **Virtual-entity LWW** — _required, not optional._ Per-context entity ids do **not** by themselves fix problem 3: `getCurrentEntityState` still has no `virtual` branch, so same-context conflicts still mis-resolve. Conflict resolution must learn to read a virtual entity (`PLUGIN_USER_DATA`) from @@ -171,7 +171,7 @@ full scope: 4. **Rate-limit & size-cap semantics** — `PluginUserPersistenceService` keys its coalesce/throttle Maps by id; per-key keys weaken the per-plugin flood guard (`MIN_PLUGIN_PERSIST_INTERVAL_MS`) and make `MAX_PLUGIN_DATA_SIZE` per-key. - Keep an additional per-*plugin* aggregate cap so a many-keyed plugin cannot + Keep an additional per-_plugin_ aggregate cap so a many-keyed plugin cannot bypass the limits. 5. **Uninstall cleanup** — `removePluginUserData(pluginId)` deletes only the exact id; keyed entries `pluginId:*` would leak. Make deletion prefix-aware. @@ -187,11 +187,11 @@ whole-doc — acceptable per Non-goals. ## Risks -| Risk | Mitigation | -| --- | --- | -| `migrateStoredDoc` / `refreshChipContentFromCache` stop backfilling → stripped chips render empty | Both are existing load-pipeline invariants with spec coverage; add a round-trip test that a stripped doc rebuilds full chips. | -| Strip accidentally mutates the live editor doc or non-chip text | `stripChipContent` is pure and runs on the `getJSON()` copy; only `taskRef`/`subTaskRef` nodes are altered. Unit-test both. | -| A chip stripped to empty content is written back to the host as a title erasure | Cannot happen — write-back (`reconcileTitlesFromDoc`) reads the *live* editor doc, which always has refreshed content; only the storage copy is stripped. | +| Risk | Mitigation | +| ------------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `migrateStoredDoc` / `refreshChipContentFromCache` stop backfilling → stripped chips render empty | Both are existing load-pipeline invariants with spec coverage; add a round-trip test that a stripped doc rebuilds full chips. | +| Strip accidentally mutates the live editor doc or non-chip text | `stripChipContent` is pure and runs on the `getJSON()` copy; only `taskRef`/`subTaskRef` nodes are altered. Unit-test both. | +| A chip stripped to empty content is written back to the host as a title erasure | Cannot happen — write-back (`reconcileTitlesFromDoc`) reads the _live_ editor doc, which always has refreshed content; only the storage copy is stripped. | ## Testing diff --git a/packages/plugin-dev/document-mode/src/ui/editor.ts b/packages/plugin-dev/document-mode/src/ui/editor.ts index e7aae5070d..0d4a668ca7 100644 --- a/packages/plugin-dev/document-mode/src/ui/editor.ts +++ b/packages/plugin-dev/document-mode/src/ui/editor.ts @@ -34,14 +34,17 @@ import { createTaskRefNode, type TaskRefNodeDeps } from './task-ref-node'; declare const PluginAPI: PluginAPI; -// Save cadence. This is a *throttle*, not a debounce: the host tears the -// embed iframe down on every work-context switch, so a save deferred until -// the user goes idle (or until teardown) routinely never runs. Committing -// every SAVE_THROTTLE_MS while editing keeps the doc blob fresh in host -// storage while the iframe is still alive. Kept above the host's 1 s -// persist rate limit (MIN_PLUGIN_PERSIST_INTERVAL_MS) so saves aren't -// rejected. -const SAVE_THROTTLE_MS = 2_000; +// Save cadence. This is a *throttle*, not a debounce: a debounce would never +// fire for a continuous typist, and the host tears the embed iframe down on +// every work-context switch. The throttle bounds op-rate during typing; the +// real safety net against data loss is the set of flush triggers wired up in +// mount(): `visibilitychange`, `blur`, `pagehide`, `unload`, plus the +// explicit `flushSaveSync` on work-context change. Together those cover +// every non-crash "user is done editing for now" moment, so the throttle +// ceiling is only paid on actual process termination without any of those +// firing. Kept above the host's 1 s persist rate limit +// (MIN_PLUGIN_PERSIST_INTERVAL_MS) so saves aren't rejected. +const SAVE_THROTTLE_MS = 30_000; const STORAGE_VERSION = 1; // Action type the host emits for an in-place single-task update (NgRx @@ -67,6 +70,12 @@ let taskCache = new Map(); */ const lookupTask: TaskLookup = (id) => taskCache.get(id); let saveTimer: ReturnType | null = null; +// True from the moment the throttled `flushSave` setTimeout fires until its +// async readBlob+persist round-trip completes. The dirty signal moves from +// `saveTimer` to this flag for the duration, so a teardown that arrives +// mid-flight (pagehide/unload while `await readBlob()` is suspended) still +// triggers the sync safety-net write in `flushSaveSync`. +let saveInFlight = false; let editor: Editor | null = null; let isLoadingDoc = false; // Set when the stored doc for the current ctx failed to parse and we fell @@ -272,6 +281,7 @@ const flushSave = async (): Promise => { saveTimer = null; } if (!currentCtx || !editor) return; + saveInFlight = true; try { const latest = await readBlob(); const merged: StoredState = { @@ -282,6 +292,8 @@ const flushSave = async (): Promise => { await PluginAPI.persistDataSynced(JSON.stringify(merged)); } catch (err) { logErr('persistDataSynced failed', err); + } finally { + saveInFlight = false; } }; @@ -297,7 +309,9 @@ const flushSave = async (): Promise => { * * This variant skips the round-trip: it builds the blob from the in-memory * `storedState` and dispatches `persistDataSynced` synchronously, so the - * postMessage leaves the iframe before it dies. + * postMessage leaves the iframe before it dies. The host transparently + * compresses the payload on its end (see `plugin-data-codec.ts`), so this + * path still benefits from the size win. * * Trade-off: an `enabledCtxIds` change made by background.ts since our last * `readBlob` would be written back stale. That field only changes on an @@ -305,6 +319,12 @@ const flushSave = async (): Promise => { * window is effectively nil, and losing the whole doc is the worse outcome. */ const flushSaveSync = (): void => { + // Dirty signal: a timer is pending (edits queued for the throttle window) + // OR an async flushSave is mid-flight (its readBlob round-trip is awaiting + // a response that may never arrive if teardown is now). Skipping when + // neither is true avoids a full stringify + gzip on every blur (which + // fires on any focus shift inside the page). + if (saveTimer === null && !saveInFlight) return; if (saveTimer !== null) { clearTimeout(saveTimer); saveTimer = null; @@ -2004,13 +2024,20 @@ const mount = async (): Promise => { onAnyTaskUpdate(payload as AnyTaskUpdatePayload); }); - // Best-effort teardown flush. The throttled save above is the real - // safety net (it commits while the iframe is unquestionably alive); this - // only catches edits made in the last < SAVE_THROTTLE_MS before the - // iframe is discarded. `pagehide` and `unload` are both wired up because - // browsers are inconsistent about which fires when an iframe element is - // removed from the DOM — flushSaveSync is idempotent, so double-firing - // is harmless. + // Flush triggers. `flushSaveSync` is idempotent, so overlap is harmless. + // + // - `visibilitychange` (on 'hidden') covers tab-switch, window-minimize, + // mobile-background, and screen-lock. The iframe's visibilityState + // mirrors the top-level document. + // - `blur` covers focus moving between iframes within the same page — + // which `visibilitychange` does not catch. + // - `pagehide` / `unload` cover iframe teardown; browsers are inconsistent + // about which fires when an iframe element is removed from the DOM, so + // both are wired. + document.addEventListener('visibilitychange', () => { + if (document.visibilityState === 'hidden') flushSaveSync(); + }); + window.addEventListener('blur', () => flushSaveSync()); window.addEventListener('pagehide', () => flushSaveSync()); window.addEventListener('unload', () => flushSaveSync()); }; diff --git a/src/app/op-log/core/entity-registry.spec.ts b/src/app/op-log/core/entity-registry.spec.ts index 86565d9ddb..f648d60ea9 100644 --- a/src/app/op-log/core/entity-registry.spec.ts +++ b/src/app/op-log/core/entity-registry.spec.ts @@ -8,7 +8,6 @@ import { isSingletonEntity, isMapEntity, isArrayEntity, - isVirtualEntity, EntityConfig, } from './entity-registry'; @@ -60,9 +59,12 @@ describe('entity-registry', () => { const MAP_ENTITIES: EntityType[] = ['PLANNER']; - const ARRAY_ENTITIES: EntityType[] = ['BOARD', 'REMINDER']; - - const VIRTUAL_ENTITIES: EntityType[] = ['PLUGIN_USER_DATA', 'PLUGIN_METADATA']; + const ARRAY_ENTITIES: EntityType[] = [ + 'BOARD', + 'REMINDER', + 'PLUGIN_USER_DATA', + 'PLUGIN_METADATA', + ]; describe('ENTITY_CONFIGS completeness', () => { it('should have config for all regular entity types', () => { @@ -267,33 +269,10 @@ describe('entity-registry', () => { }); }); - describe('virtual entities', () => { - it('should have correct storagePattern', () => { - for (const entityType of VIRTUAL_ENTITIES) { - const config = ENTITY_CONFIGS[entityType]; - expect(config?.storagePattern).toBe( - 'virtual', - `${entityType} should have storagePattern 'virtual'`, - ); - } - }); - - it('should have payloadKey', () => { - for (const entityType of VIRTUAL_ENTITIES) { - const config = ENTITY_CONFIGS[entityType]; - expect(config?.payloadKey).toBeDefined(`${entityType} missing payloadKey`); - } - }); - - it('should NOT have featureName (not stored in NgRx)', () => { - for (const entityType of VIRTUAL_ENTITIES) { - const config = ENTITY_CONFIGS[entityType]; - expect(config?.featureName).toBeUndefined( - `${entityType} should NOT have featureName (virtual)`, - ); - } - }); - }); + // (No 'virtual entities' describe block — no entity in this registry uses + // the 'virtual' storagePattern. PLUGIN_USER_DATA / PLUGIN_METADATA + // migrated to 'array'. The pattern remains in @sp/sync-core for external + // consumers.) describe('getEntityConfig', () => { it('should return config for known entity types', () => { @@ -388,12 +367,14 @@ describe('entity-registry', () => { expect(isArrayEntity(taskConfig)).toBe(false); }); - it('isVirtualEntity should correctly identify virtual entities', () => { - const pluginConfig = getEntityConfig('PLUGIN_USER_DATA') as EntityConfig; - const taskConfig = getEntityConfig('TASK') as EntityConfig; - - expect(isVirtualEntity(pluginConfig)).toBe(true); - expect(isVirtualEntity(taskConfig)).toBe(false); + it('no currently-configured entity uses the virtual storagePattern', () => { + // The 'virtual' pattern remains in @sp/sync-core for external + // consumers, but no entity in this registry uses it after + // PLUGIN_USER_DATA / PLUGIN_METADATA migrated to 'array'. + for (const entityType of REGULAR_ENTITY_TYPES) { + const config = getEntityConfig(entityType) as EntityConfig; + expect(config.storagePattern).not.toBe('virtual'); + } }); }); @@ -403,8 +384,7 @@ describe('entity-registry', () => { ADAPTER_ENTITIES.length + SINGLETON_ENTITIES.length + MAP_ENTITIES.length + - ARRAY_ENTITIES.length + - VIRTUAL_ENTITIES.length; + ARRAY_ENTITIES.length; expect(totalConfigured).toBe(REGULAR_ENTITY_TYPES.length); }); diff --git a/src/app/op-log/core/entity-registry.ts b/src/app/op-log/core/entity-registry.ts index d244b39950..99a812ac14 100644 --- a/src/app/op-log/core/entity-registry.ts +++ b/src/app/op-log/core/entity-registry.ts @@ -19,7 +19,6 @@ import { isArrayEntity as isArrayEntityFromCore, isMapEntity as isMapEntityFromCore, isSingletonEntity as isSingletonEntityFromCore, - isVirtualEntity as isVirtualEntityFromCore, } from '@sp/sync-core'; import type { EntityRegistry as CoreEntityRegistry } from '@sp/sync-core'; import type { @@ -81,6 +80,14 @@ import { plannerFeatureKey } from '../../features/planner/store/planner.reducer' import { BOARDS_FEATURE_NAME } from '../../features/boards/store/boards.reducer'; import { menuTreeFeatureKey } from '../../features/menu-tree/store/menu-tree.reducer'; import { REMINDER_FEATURE_NAME } from '../../features/reminder/store/reminder.reducer'; +import { + PLUGIN_USER_DATA_FEATURE_NAME, + selectPluginUserDataFeatureState, +} from '../../plugins/store/plugin-user-data.reducer'; +import { + PLUGIN_METADATA_FEATURE_NAME, + selectPluginMetadataFeatureState, +} from '../../plugins/store/plugin-metadata.reducer'; import { SECTION_FEATURE_NAME, adapter as sectionAdapter, @@ -315,15 +322,26 @@ export const buildEntityRegistry = (): EntityRegistry => arrayKey: null, // State IS the array }, - // ── VIRTUAL ENTITIES ─────────────────────────────────────────────────────── + // PLUGIN_USER_DATA / PLUGIN_METADATA were registered as `'virtual'` before + // ConflictResolutionService grew a virtual branch. Their state shape is + // already Array<{ id, ... }> — identical to REMINDER's arrayKey:null — + // so the existing array branch in getCurrentEntityState (and elsewhere) + // resolves them correctly. Keeping them virtual silently disabled LWW + // conflict resolution for all plugin data. PLUGIN_USER_DATA: { - storagePattern: 'virtual', + storagePattern: 'array', + featureName: PLUGIN_USER_DATA_FEATURE_NAME, payloadKey: 'pluginUserData', + selectState: selectPluginUserDataFeatureState, + arrayKey: null, }, PLUGIN_METADATA: { - storagePattern: 'virtual', + storagePattern: 'array', + featureName: PLUGIN_METADATA_FEATURE_NAME, payloadKey: 'pluginMetadata', + selectState: selectPluginMetadataFeatureState, + arrayKey: null, }, // Note: ALL, RECOVERY, MIGRATION are not configured - they're special operation types @@ -370,7 +388,10 @@ export const isMapEntity = isMapEntityFromCore; export const isArrayEntity = isArrayEntityFromCore; -export const isVirtualEntity = isVirtualEntityFromCore; +// `isVirtualEntity` removed: no entity in this registry uses the `'virtual'` +// storagePattern after PLUGIN_USER_DATA / PLUGIN_METADATA migrated to +// `'array'`. The pattern still exists in `@sp/sync-core` for external +// consumers; re-export it here once we have a real user again. /** * Returns all payload keys from configured entities. diff --git a/src/app/plugins/plugin-user-persistence.service.spec.ts b/src/app/plugins/plugin-user-persistence.service.spec.ts index e9e6698147..7bc2b1e4db 100644 --- a/src/app/plugins/plugin-user-persistence.service.spec.ts +++ b/src/app/plugins/plugin-user-persistence.service.spec.ts @@ -7,6 +7,17 @@ import { } from './plugin-persistence.model'; import { upsertPluginUserData, deletePluginUserData } from './store/plugin.actions'; import { selectPluginUserDataFeatureState } from './store/plugin-user-data.reducer'; +import { COMPRESS_THRESHOLD, SENTINEL, encodeForPersist } from './util/plugin-data-codec'; + +/** + * Drain enough microtasks for the per-plugin commit chain to settle. The + * chain is `Promise.resolve().catch().then(() => _encodeAndDispatch())` plus + * one `await` inside _encodeAndDispatch — three turns max — but a few extra + * yields are free and survive any future chain tweak. + */ +const drainAsync = async (): Promise => { + for (let i = 0; i < 8; i++) await Promise.resolve(); +}; describe('PluginUserPersistenceService', () => { let service: PluginUserPersistenceService; @@ -33,37 +44,57 @@ describe('PluginUserPersistenceService', () => { }); describe('persistPluginUserData', () => { - it('should dispatch upsertPluginUserData action with valid data', () => { + it('should dispatch upsertPluginUserData with the raw input (below codec threshold)', async () => { const pluginId = 'test-plugin'; const data = 'test data'; - - service.persistPluginUserData(pluginId, data); + await service.persistPluginUserData(pluginId, data); expect(dispatchSpy).toHaveBeenCalledWith( upsertPluginUserData({ pluginUserData: { id: pluginId, data } }), ); }); + it('should dispatch compressed data when the payload is above the codec threshold', async () => { + const pluginId = 'test-plugin'; + // ~2 KB of repetitive JSON — well above the 1 KB threshold, gzips small. + const data = JSON.stringify({ + items: Array.from({ length: 40 }, (_, i) => ({ + id: i, + name: `repeating-value-${i % 3}`, + })), + }); + expect(data.length).toBeGreaterThan(COMPRESS_THRESHOLD); + + await service.persistPluginUserData(pluginId, data); + + const dispatched = dispatchSpy.calls.mostRecent().args[0] as ReturnType< + typeof upsertPluginUserData + >; + expect(dispatched.type).toBe(upsertPluginUserData.type); + const stored = dispatched.pluginUserData.data; + expect(stored.startsWith(SENTINEL)).toBe(true); + expect(stored.length).toBeLessThan(data.length); + }); + it('should throw error when data exceeds MAX_PLUGIN_DATA_SIZE', () => { const pluginId = 'test-plugin'; - // Create a string larger than MAX_PLUGIN_DATA_SIZE (1MB) const largeData = 'x'.repeat(MAX_PLUGIN_DATA_SIZE + 1000); + // Size check is synchronous: throws before any async work begins. expect(() => service.persistPluginUserData(pluginId, largeData)).toThrowError( /Plugin data exceeds maximum size/, ); expect(dispatchSpy).not.toHaveBeenCalled(); }); - it('should coalesce a rapid second call instead of dropping it', () => { + it('should coalesce a rapid second call instead of dropping it', async () => { const baseTime = Date.now(); jasmine.clock().install(); jasmine.clock().mockDate(new Date(baseTime)); try { const pluginId = 'test-plugin'; - // First call commits immediately. - service.persistPluginUserData(pluginId, 'first'); + await service.persistPluginUserData(pluginId, 'first'); expect(dispatchSpy).toHaveBeenCalledTimes(1); // A call inside the rate-limit window must not throw and must not be @@ -71,8 +102,10 @@ describe('PluginUserPersistenceService', () => { expect(() => service.persistPluginUserData(pluginId, 'second')).not.toThrow(); expect(dispatchSpy).toHaveBeenCalledTimes(1); - // Once the interval elapses the held write is committed. jasmine.clock().tick(MIN_PLUGIN_PERSIST_INTERVAL_MS); + // setTimeout fires → _flushPendingData → _commit kicks off async + // compress+dispatch. Drain microtasks to let it settle. + await drainAsync(); expect(dispatchSpy).toHaveBeenCalledTimes(2); expect(dispatchSpy).toHaveBeenCalledWith( upsertPluginUserData({ @@ -84,19 +117,20 @@ describe('PluginUserPersistenceService', () => { } }); - it('should keep only the most recent of several coalesced calls', () => { + it('should keep only the most recent of several coalesced calls', async () => { const baseTime = Date.now(); jasmine.clock().install(); jasmine.clock().mockDate(new Date(baseTime)); try { const pluginId = 'test-plugin'; - service.persistPluginUserData(pluginId, 'v1'); // committed + await service.persistPluginUserData(pluginId, 'v1'); service.persistPluginUserData(pluginId, 'v2'); // coalesced service.persistPluginUserData(pluginId, 'v3'); // coalesced, replaces v2 expect(dispatchSpy).toHaveBeenCalledTimes(1); jasmine.clock().tick(MIN_PLUGIN_PERSIST_INTERVAL_MS); + await drainAsync(); expect(dispatchSpy).toHaveBeenCalledTimes(2); expect(dispatchSpy).toHaveBeenCalledWith( upsertPluginUserData({ pluginUserData: { id: pluginId, data: 'v3' } }), @@ -106,40 +140,59 @@ describe('PluginUserPersistenceService', () => { } }); - it('should allow different plugins to persist data without rate limiting each other', () => { + it('should allow different plugins to persist data without rate limiting each other', async () => { const plugin1 = 'plugin-1'; const plugin2 = 'plugin-2'; const data = 'test data'; - // Both plugins should be able to persist immediately - service.persistPluginUserData(plugin1, data); - service.persistPluginUserData(plugin2, data); + await Promise.all([ + service.persistPluginUserData(plugin1, data), + service.persistPluginUserData(plugin2, data), + ]); expect(dispatchSpy).toHaveBeenCalledTimes(2); }); it('should accept data at exactly MAX_PLUGIN_DATA_SIZE', () => { const pluginId = 'test-plugin'; - // Create a string exactly at the limit - const exactLimitData = 'x'.repeat(MAX_PLUGIN_DATA_SIZE - 10); // Slightly under to account for Blob overhead + const exactLimitData = 'x'.repeat(MAX_PLUGIN_DATA_SIZE - 10); - // This should not throw + // The sync size check is what we care about here; the async commit + // races with test teardown but is harmless. expect(() => service.persistPluginUserData(pluginId, exactLimitData)).not.toThrow(); }); }); describe('loadPluginUserData', () => { - it('should return data for existing plugin', async () => { + it('should decompress and return data for an existing plugin', async () => { const pluginId = 'test-plugin'; - const testData = 'stored data'; + const original = JSON.stringify({ + items: Array.from({ length: 40 }, (_, i) => ({ + id: i, + name: `repeating-value-${i % 3}`, + })), + }); + const stored = await encodeForPersist(original); + expect(stored.startsWith(SENTINEL)).toBe(true); store.overrideSelector(selectPluginUserDataFeatureState, [ - { id: pluginId, data: testData }, + { id: pluginId, data: stored }, ]); const result = await service.loadPluginUserData(pluginId); + expect(result).toBe(original); + }); - expect(result).toBe(testData); + it('should pass through legacy uncompressed data', async () => { + const pluginId = 'test-plugin'; + const legacy = 'stored data'; + + store.overrideSelector(selectPluginUserDataFeatureState, [ + { id: pluginId, data: legacy }, + ]); + + const result = await service.loadPluginUserData(pluginId); + expect(result).toBe(legacy); }); it('should return null for non-existent plugin', async () => { @@ -160,10 +213,9 @@ describe('PluginUserPersistenceService', () => { { id: pluginId, data: 'committed' }, ]); - service.persistPluginUserData(pluginId, 'committed'); // commits + await service.persistPluginUserData(pluginId, 'committed'); service.persistPluginUserData(pluginId, 'pending'); // coalesced - // A read-modify-write cycle must see its own not-yet-committed write. const result = await service.loadPluginUserData(pluginId); expect(result).toBe('pending'); } finally { @@ -181,39 +233,49 @@ describe('PluginUserPersistenceService', () => { expect(dispatchSpy).toHaveBeenCalledWith(deletePluginUserData({ pluginId })); }); - it('should cancel a pending coalesced write', () => { + it('should cancel a pending coalesced write', async () => { const baseTime = Date.now(); jasmine.clock().install(); jasmine.clock().mockDate(new Date(baseTime)); try { const pluginId = 'test-plugin'; - service.persistPluginUserData(pluginId, 'first'); // commits + await service.persistPluginUserData(pluginId, 'first'); service.persistPluginUserData(pluginId, 'pending'); // coalesced service.removePluginUserData(pluginId); dispatchSpy.calls.reset(); - // The cancelled flush must not resurrect the removed data. jasmine.clock().tick(MIN_PLUGIN_PERSIST_INTERVAL_MS * 2); + await drainAsync(); expect(dispatchSpy).not.toHaveBeenCalled(); } finally { jasmine.clock().uninstall(); } }); - }); - describe('getAllPluginUserData', () => { - it('should return all plugin user data', async () => { - const testData = [ - { id: 'plugin-1', data: 'data-1' }, - { id: 'plugin-2', data: 'data-2' }, - ]; + it('should not resurrect data when removed during an in-flight commit', async () => { + const pluginId = 'test-plugin'; + // A payload above the codec threshold forces async compression, so + // the commit's dispatch happens after at least one microtask turn — + // wide enough for a synchronous remove to land in between. + const data = 'x'.repeat(COMPRESS_THRESHOLD + 100); - store.overrideSelector(selectPluginUserDataFeatureState, testData); + const persistPromise = service.persistPluginUserData(pluginId, data); - const result = await service.getAllPluginUserData(); + // remove() runs synchronously while compression is still pending. + service.removePluginUserData(pluginId); - expect(result).toEqual(testData); + await persistPromise; + await drainAsync(); + + const upsertCalls = dispatchSpy.calls + .allArgs() + .filter(([action]) => action.type === upsertPluginUserData.type); + const deleteCalls = dispatchSpy.calls + .allArgs() + .filter(([action]) => action.type === deletePluginUserData.type); + expect(upsertCalls.length).toBe(0); + expect(deleteCalls.length).toBe(1); }); }); }); diff --git a/src/app/plugins/plugin-user-persistence.service.ts b/src/app/plugins/plugin-user-persistence.service.ts index c65b4ca47a..63fb99ef1a 100644 --- a/src/app/plugins/plugin-user-persistence.service.ts +++ b/src/app/plugins/plugin-user-persistence.service.ts @@ -8,11 +8,18 @@ import { } from './plugin-persistence.model'; import { upsertPluginUserData, deletePluginUserData } from './store/plugin.actions'; import { selectPluginUserDataFeatureState } from './store/plugin-user-data.reducer'; +import { decodeFromPersist, encodeForPersist } from './util/plugin-data-codec'; +import { PluginLog } from '../core/log'; /** * Service for persisting plugin user data using NgRx actions. * Handles data that plugins store and retrieve via persistDataSynced/loadSyncedData. * Includes rate limiting and size validation to prevent abuse. + * + * Data is transparently gzip-compressed at the persistence boundary (see + * `plugin-data-codec.ts`). Plugins only see their own raw strings; the + * compressed form lives in NgRx state, IndexedDB, the op-log, and on the + * sync server, shrinking per-op payloads ~4–5× for typical JSON. */ @Injectable({ providedIn: 'root', @@ -28,6 +35,7 @@ export class PluginUserPersistenceService { /** * Data that arrived inside the rate-limit window and is waiting to be * committed. Coalesced — only the most recent value per plugin is kept. + * Holds *uncompressed* input (compression happens at commit time). */ private _pendingData = new Map(); @@ -36,6 +44,32 @@ export class PluginUserPersistenceService { */ private _flushTimers = new Map>(); + /** + * The latest uncompressed input for a plugin whose `_commit` has started + * but whose async compression has not yet dispatched. A read in this + * window must see the latest write (read-your-writes), so we hold the + * raw input here until the dispatch lands. + */ + private _committing = new Map(); + + /** + * Per-plugin commit chain. Compression is async (`CompressionStream`), + * so two `_commit` calls in quick succession could finish out of order + * and dispatch a stale write last. Chaining serializes + * compress-then-dispatch per plugin while leaving different plugins + * concurrent. + */ + private _commitChain = new Map>(); + + /** + * Per-plugin generation counter. Bumped by `_cancelPending` so any + * in-flight `_commit` whose compression started under an older + * generation aborts before dispatching. Without this, a + * `removePluginUserData` issued during an async compress would be + * silently undone by the resurrecting upsert that lands afterwards. + */ + private _commitGeneration = new Map(); + /** * Persist user data for a specific plugin (called by plugin via persistDataSynced). * @@ -46,10 +80,27 @@ export class PluginUserPersistenceService { * the caller's most recent data. Dropping it silently lost edits — e.g. a * plugin's final save on teardown landing just after a periodic save. * + * Returns a Promise that resolves once the data has been compressed and + * dispatched (or queued for a future commit). The size-cap check is + * synchronous: callers that hit the limit get a thrown Error, not a + * rejected Promise, so the existing `try { persist(...) } catch` pattern + * keeps working. + * + * Read-modify-write contract: a `loadPluginUserData` whose result is then + * mutated and passed back via `persistPluginUserData` must not span a + * `removePluginUserData` for the same plugin. The remove invalidates any + * in-flight commit (via the generation counter), but a fresh `persist` + * issued from a stale `load` result *after* the remove is a new write and + * will resurrect the entry — the codec cannot distinguish "user wants + * this back" from "stale read". Callers performing RMW after a delete + * must `loadPluginUserData` again first. + * * @throws Error if data exceeds MAX_PLUGIN_DATA_SIZE */ - persistPluginUserData(pluginId: string, data: string): void { + persistPluginUserData(pluginId: string, data: string): Promise { // Validate data size — applies whether the write commits now or later. + // Cap is on the uncompressed user input, so a plugin can't bypass by + // sending pre-compressed bytes. const dataSize = new Blob([data]).size; if (dataSize > MAX_PLUGIN_DATA_SIZE) { throw new Error( @@ -71,37 +122,75 @@ export class PluginUserPersistenceService { const delay = MIN_PLUGIN_PERSIST_INTERVAL_MS - timeSinceLastPersist; this._flushTimers.set( pluginId, - setTimeout(() => this._flushPendingData(pluginId), delay), + setTimeout(() => { + void this._flushPendingData(pluginId); + }, delay), ); } - return; + return Promise.resolve(); } - this._commit(pluginId, data, now); + return this._commit(pluginId, data, now); } /** * Commit a coalesced write once its rate-limit window has elapsed. */ - private _flushPendingData(pluginId: string): void { + private async _flushPendingData(pluginId: string): Promise { this._flushTimers.delete(pluginId); const data = this._pendingData.get(pluginId); if (data === undefined) { return; } this._pendingData.delete(pluginId); - this._commit(pluginId, data, Date.now()); + await this._commit(pluginId, data, Date.now()); } /** - * Dispatch the persist action and record the commit time for rate limiting. + * Compress and dispatch. Serialized per plugin via `_commitChain` to + * preserve write order even if compression times vary across calls. */ - private _commit(pluginId: string, data: string, at: number): void { + private _commit(pluginId: string, data: string, at: number): Promise { this._lastPersistTime.set(pluginId, at); - const pluginUserData: PluginUserData = { - id: pluginId, - data, - }; + this._committing.set(pluginId, data); + // Capture the generation at the moment we schedule. If a remove bumps + // it before this commit's compression finishes, _encodeAndDispatch + // bails and the upsert is suppressed — preventing post-delete + // resurrection. + const myGeneration = this._commitGeneration.get(pluginId) ?? 0; + + const prev = this._commitChain.get(pluginId) ?? Promise.resolve(); + // Swallow prior errors so the chain doesn't poison subsequent writes. + // The original failed call has already seen and surfaced its own error. + const next: Promise = prev + .catch(() => undefined) + .then(() => this._encodeAndDispatch(pluginId, data, myGeneration)); + this._commitChain.set(pluginId, next); + + next.finally(() => { + if (this._committing.get(pluginId) === data) { + this._committing.delete(pluginId); + } + if (this._commitChain.get(pluginId) === next) { + this._commitChain.delete(pluginId); + } + }); + + return next; + } + + private async _encodeAndDispatch( + pluginId: string, + data: string, + generation: number, + ): Promise { + const encoded = await encodeForPersist(data); + if ((this._commitGeneration.get(pluginId) ?? 0) !== generation) { + // A removePluginUserData / clearAllPluginUserData ran between + // schedule and dispatch. Dropping the upsert preserves the delete. + return; + } + const pluginUserData: PluginUserData = { id: pluginId, data: encoded }; this._store.dispatch(upsertPluginUserData({ pluginUserData })); } @@ -109,18 +198,38 @@ export class PluginUserPersistenceService { * Load user data for a specific plugin (called by plugin via loadSyncedData). * * Returns a coalesced-but-not-yet-committed write if one is pending, so a - * plugin's read-modify-write cycle always sees its own latest data. + * plugin's read-modify-write cycle always sees its own latest data. Same + * applies to a commit that has started compression but not yet dispatched. + * + * Otherwise reads from NgRx state and decompresses transparently. */ async loadPluginUserData(pluginId: string): Promise { const pending = this._pendingData.get(pluginId); if (pending !== undefined) { return pending; } + const committing = this._committing.get(pluginId); + if (committing !== undefined) { + return committing; + } const currentState = await firstValueFrom( this._store.select(selectPluginUserDataFeatureState), ); const pluginData = currentState.find((item) => item.id === pluginId); - return pluginData?.data || null; + if (!pluginData?.data) { + return null; + } + try { + return await decodeFromPersist(pluginData.data); + } catch (err) { + // Don't log `err` directly — gzip/atob messages can contain partial + // payload bytes from user content. Surface only the error class. + PluginLog.err('PluginUserPersistenceService: failed to decode stored data', { + pluginId, + errName: (err as Error)?.name, + }); + return null; + } } /** @@ -133,7 +242,10 @@ export class PluginUserPersistenceService { /** * Drop any pending coalesced write (and its timer) for a plugin, so it - * cannot resurrect data that is being removed. + * cannot resurrect data that is being removed. Also bumps the commit + * generation so an *in-flight* `_commit` (already past its rate-limit + * check, currently awaiting compression) detects the cancel and skips + * its dispatch — the upsert would otherwise undo this delete. */ private _cancelPending(pluginId: string): void { const timer = this._flushTimers.get(pluginId); @@ -142,17 +254,16 @@ export class PluginUserPersistenceService { this._flushTimers.delete(pluginId); } this._pendingData.delete(pluginId); - } - - /** - * Get all plugin user data - */ - async getAllPluginUserData(): Promise { - return firstValueFrom(this._store.select(selectPluginUserDataFeatureState)); + this._committing.delete(pluginId); + this._commitGeneration.set(pluginId, (this._commitGeneration.get(pluginId) ?? 0) + 1); } /** * Clear all plugin user data (removes each one individually to create operations) + * + * Yields the event loop after the dispatch loop — CLAUDE.md sync rule 6: + * rapid in-loop dispatches against an `array`-pattern entity can lose + * state without a microtask break. */ async clearAllPluginUserData(): Promise { const currentState = await firstValueFrom( @@ -162,5 +273,6 @@ export class PluginUserPersistenceService { this._cancelPending(item.id); this._store.dispatch(deletePluginUserData({ pluginId: item.id })); } + await new Promise((r) => setTimeout(r, 0)); } } diff --git a/src/app/plugins/util/plugin-data-codec.spec.ts b/src/app/plugins/util/plugin-data-codec.spec.ts new file mode 100644 index 0000000000..1ede1a88d8 --- /dev/null +++ b/src/app/plugins/util/plugin-data-codec.spec.ts @@ -0,0 +1,65 @@ +import { + COMPRESS_THRESHOLD, + SENTINEL, + decodeFromPersist, + encodeForPersist, +} from './plugin-data-codec'; +import { compressWithGzipToString } from '../../op-log/encryption/compression-handler'; +import { MAX_PLUGIN_DATA_SIZE } from '../plugin-persistence.model'; + +describe('plugin-data-codec', () => { + it('returns short input unchanged (below threshold)', async () => { + const input = JSON.stringify({ hello: 'world' }); + expect(input.length).toBeLessThan(COMPRESS_THRESHOLD); + const encoded = await encodeForPersist(input); + expect(encoded).toBe(input); + expect(await decodeFromPersist(encoded)).toBe(input); + }); + + it('compresses and round-trips a large redundant JSON blob', async () => { + const blob = JSON.stringify({ + docs: Object.fromEntries( + Array.from({ length: 5 }, (_outer, i) => [ + `ctx-${i}`, + { + type: 'doc', + content: Array.from({ length: 40 }, (_inner, j) => ({ + type: 'paragraph', + content: [{ type: 'text', text: `Paragraph ${j} in context ${i}.` }], + })), + }, + ]), + ), + }); + expect(blob.length).toBeGreaterThan(COMPRESS_THRESHOLD); + + const encoded = await encodeForPersist(blob); + expect(encoded.startsWith(SENTINEL)).toBe(true); + expect(encoded.length).toBeLessThan(blob.length / 2); + + expect(await decodeFromPersist(encoded)).toBe(blob); + }); + + it('passes through legacy uncompressed data (no sentinel)', async () => { + const legacy = JSON.stringify({ version: 1, payload: 'whatever' }); + expect(await decodeFromPersist(legacy)).toBe(legacy); + }); + + it('rejects a malformed sentinel-prefixed blob', async () => { + await expectAsync(decodeFromPersist(SENTINEL + '!!!not-base64!!!')).toBeRejected(); + }); + + it('aborts when the decompressed payload would exceed the size cap', async () => { + // A highly redundant string of 4× MAX_PLUGIN_DATA_SIZE compresses to a + // small base64 blob but decompresses past the codec's MAX_DECOMPRESSED_SIZE + // ceiling (2× MAX_PLUGIN_DATA_SIZE). The decode must throw before the full + // expansion is realised — a gzip-bomb defence. + const massive = 'A'.repeat(MAX_PLUGIN_DATA_SIZE * 4); + const compressed = await compressWithGzipToString(massive); + expect(compressed.length).toBeLessThan(massive.length / 10); + + await expectAsync(decodeFromPersist(SENTINEL + compressed)).toBeRejectedWithError( + /decompressed size exceeded/, + ); + }); +}); diff --git a/src/app/plugins/util/plugin-data-codec.ts b/src/app/plugins/util/plugin-data-codec.ts new file mode 100644 index 0000000000..57138a714c --- /dev/null +++ b/src/app/plugins/util/plugin-data-codec.ts @@ -0,0 +1,150 @@ +import { compressWithGzipToString } from '../../op-log/encryption/compression-handler'; +import { MAX_PLUGIN_DATA_SIZE } from '../plugin-persistence.model'; + +/** + * Transparent compression for plugin user data on the way to/from the + * op-log. Plugins call `persistDataSynced(string)` / `loadSyncedData(): string` + * and only ever see the string they themselves wrote; the host wraps the + * payload with gzip+base64 between the plugin API and `PluginUserData.data`, + * so every op (and every sync upload) carries the compressed form. + * + * Format: `GZ1:`. The `GZ1:` sentinel doubles as a version + * tag — future format changes get a new prefix and stay decodable alongside. + * + * Below `COMPRESS_THRESHOLD` we emit the input unchanged. Gzip's framing + * (~18 bytes) plus base64's 4/3 overhead actively hurts very small payloads, + * and most plugin-metadata-style writes fall under the threshold. + * + * Backward compat on read: a string without the sentinel is treated as + * legacy uncompressed data and passed through. + * + * Compression itself delegates to `@sp/sync-core` via the host's + * `compression-handler` wrapper so we share the chunked base64 encoder, + * env-shim detection, and error reporting with the op-log path. + * + * Decompression is intentionally **bounded** via three cheap-to-expensive + * caps — `MAX_RAW_LENGTH` (pre-`atob`), `MAX_COMPRESSED_SIZE` (post-`atob`, + * pre-stream), and `MAX_DECOMPRESSED_SIZE` (per-chunk in `gunzipBounded`) — + * so a malicious gzip blob from a compromised sync server can't expand + * into gigabytes and OOM the renderer. The write-side `MAX_PLUGIN_DATA_SIZE` + * cap only constrains local input; remote data needs its own ceiling. + */ + +export const SENTINEL = 'GZ1:'; +export const COMPRESS_THRESHOLD = 1024; + +/** + * Decompression output cap. Plugins are write-bounded by + * `MAX_PLUGIN_DATA_SIZE` (1 MB); allowing 2× on read covers any reasonable + * round-trip while still bounding a gzip-bomb to a few MB of allocation. + */ +const MAX_DECOMPRESSED_SIZE = MAX_PLUGIN_DATA_SIZE * 2; + +/** + * Compressed input cap. Bounded at the same ceiling as the decompressed + * output (2× write cap, ~2 MB) — generous enough to tolerate gzip framing + * on near-incompressible payloads, tight enough to reject multi-MB attacks + * before we hand bytes to `DecompressionStream`. Cheap defense-in-depth on + * top of the per-chunk size check inside `gunzipBounded`. + */ +const MAX_COMPRESSED_SIZE = MAX_DECOMPRESSED_SIZE; + +/** + * Pre-decode cap on the raw (base64-encoded) string. base64 is 4 chars per + * 3 source bytes, so a string longer than `MAX_COMPRESSED_SIZE * 2` chars + * cannot fit under the byte cap. Checking before `atob` avoids allocating + * the decoded Uint8Array for oversized attacker payloads. + */ +const MAX_RAW_LENGTH = MAX_COMPRESSED_SIZE * 2; + +const TEXT_DECODER = new TextDecoder(); + +const base64ToBytes = (b64: string): Uint8Array => { + const binary = atob(b64); + const bytes = new Uint8Array(binary.length); + for (let i = 0; i < binary.length; i++) bytes[i] = binary.charCodeAt(i); + return bytes; +}; + +/** + * gzip-decompress a `Uint8Array` while enforcing an output-byte ceiling. + * Reads the `DecompressionStream` chunk-by-chunk and aborts if the running + * total exceeds `MAX_DECOMPRESSED_SIZE`, avoiding the gigabyte allocation + * an unbounded `Response.arrayBuffer()` would produce. + */ +const gunzipBounded = async (bytes: Uint8Array): Promise => { + const stream = new Blob([bytes as BlobPart]) + .stream() + .pipeThrough(new DecompressionStream('gzip')); + const reader = stream.getReader(); + const chunks: Uint8Array[] = []; + let total = 0; + try { + while (true) { + const { done, value } = await reader.read(); + if (done) break; + total += value.length; + if (total > MAX_DECOMPRESSED_SIZE) { + try { + await reader.cancel(); + } catch { + /* cancel is best-effort */ + } + throw new Error( + `Plugin data decompressed size exceeded ${MAX_DECOMPRESSED_SIZE} bytes`, + ); + } + chunks.push(value); + } + } finally { + try { + reader.releaseLock(); + } catch { + /* releaseLock is best-effort after cancel */ + } + } + const out = new Uint8Array(total); + let offset = 0; + for (const chunk of chunks) { + out.set(chunk, offset); + offset += chunk.length; + } + return TEXT_DECODER.decode(out); +}; + +/** + * Encode a plugin's persisted string for storage. Returns the input + * unchanged below the compression threshold; otherwise gzip+base64 with + * sentinel. Falls back to the raw input if compression throws — persistence + * must never fail in this codec. + */ +export const encodeForPersist = async (input: string): Promise => { + if (input.length < COMPRESS_THRESHOLD) return input; + try { + return SENTINEL + (await compressWithGzipToString(input)); + } catch { + return input; + } +}; + +/** + * Decode a stored value back to the original plugin string. Falls back to + * pass-through for legacy uncompressed data (no sentinel). + * + * Throws on a malformed sentinel-prefixed blob *or* if the decompressed + * output would exceed the size ceiling. Callers (the persistence service) + * wrap and surface as a load failure. + */ +export const decodeFromPersist = async (raw: string): Promise => { + if (!raw.startsWith(SENTINEL)) return raw; + const encoded = raw.slice(SENTINEL.length); + // Reject before `atob` allocates a ~0.75 × encoded.length byte array. + if (encoded.length > MAX_RAW_LENGTH) { + throw new Error(`Plugin data raw size exceeded ${MAX_RAW_LENGTH} chars`); + } + const compressed = base64ToBytes(encoded); + if (compressed.length > MAX_COMPRESSED_SIZE) { + throw new Error(`Plugin data compressed size exceeded ${MAX_COMPRESSED_SIZE} bytes`); + } + return gunzipBounded(compressed); +};