mirror of
https://github.com/johannesjo/super-productivity.git
synced 2026-07-18 08:56:41 +00:00
37 commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Johannes Millan
|
eff41c041d
|
feat(project): project completion experience (#8036)
* feat(project): add project completion with celebration + trophy view
Completing a project marks it done and archives it, with a celebration
dialog (confetti + live stats) and a prompt to resolve unfinished tasks
(move to Inbox / mark done). Completed projects show a trophy badge and
a Reopen action on the archived-projects page.
isDone stays distinct from isArchived; selectArchivedProjects is left
intact so completed projects' tasks stay filtered out of Today/Overdue.
Append/merge deferred to #8032. Plan: docs/plans/2026-06-05-project-completion.md
* refactor(project): drop Archive menu item; Complete is the retire path
Archive and Complete produced near-identical end states; collapse to one
user-facing action. Removes the 'Archive project' menu item and handler so
Complete is the single way to retire a project. The archiveProject action,
reducer and ProjectService.archive() stay (needed for op-log replay of
historical archive ops and the legacy unarchive/restore path). Wiki updated;
menu specs cover the Complete flow.
* fix(project): keep completion dialog open for the active project
MatDialog closeOnNavigation (default true) dismissed the celebration the
moment completing the currently-active project navigated to '/'. Navigate
first, then open the dialog.
* test(project): e2e for completion flow (complete, celebrate, reopen)
Covers the resolve-unfinished-tasks prompt, the celebration dialog with
stats, the trophy badge on the archived page, and reopening. Adds an
openProjectContextMenu page-object helper.
* feat(project): confirm project completion
* feat(project): show completion celebration fullscreen
* fix(project): harden completion celebration flow
* style(project): use spacing tokens in completion dialog
* fix(project): align completion screen with project context
* style(project): soften completion screen coloring
* style(project): reuse completion screen surfaces
* fix: refine project completion dialog actions
* fix: complete projects with atomic task resolution
* fix: restore archive path in project completion UI
* refactor(project): remove dead completion code
The project-level completeProject action (OpType.Update) was never
dispatched — completion goes exclusively through the atomic
TaskSharedActions.completeProject (OpType.Batch) meta-reducer. Drop the
dead action, its reducer case, the PROJECT_COMPLETE enum member and the
immutable 'PCO' op-log code (which would otherwise be permanently
reserved for an op that can never be produced); enum count 147->146.
Also remove the unused selectCompletedProjects / selectPlainArchivedProjects
selectors (no consumers) and the misspelled, unused 'angel' confetti
field, keeping the regression tests that guard selectArchivedProjects.
* fix(project): make project completion non-reversible
Completion resolves a project's open tasks (move-to-inbox / mark-done),
which reopen cannot truly restore, so the "Reopen"/"Undo" affordances on
the completion path were misleading. Drop the celebration dialog's Reopen
button and the post-complete undo snack; the fullscreen celebration is
the feedback and deliberate reactivation still lives on the
archived-projects page (Project.reopen kept for it).
Also restore the project title param on the archive confirm dialog (it
was rendering a raw {{title}} placeholder), remove the now-unused
moveTasksToInbox / markTasksDone resolution helpers (the meta-reducer
resolves tasks atomically), and drop the orphaned UNDO / S.COMPLETED
i18n keys. Updates the completion e2e to the close flow and asserts the
resolution props are forwarded to the atomic action.
* fix(tasks): cancel native reminders for project-completed tasks
Completing a project marks its unfinished tasks done inside the
meta-reducer (no per-task updateTask), so unscheduleDoneTask$'s
native-reminder cancellation is bypassed and an OS-scheduled Android
notification could still fire for a now-done task. Add a local-only
effect that cancels native reminders for the force-completed task ids.
Local-only by design: it dispatches no actions (the persistent
dismissReminderOnly/clearDeadlineReminder would each be an extra synced
op), and done tasks are already filtered from reminders$ on all
platforms — only the native Android notification needs explicit removal.
* test(project): drop TaskService spies orphaned by helper removal
getByIdWithSubTaskData$/moveToProject/setDone/setUnDone were only used
by the removed moveTasksToInbox/markTasksDone helpers and their deleted
tests.
* feat(sync): affectedEntities multi-entity conflict detection for atomic completion
WIP checkpoint of the atomic completeProject approach. The Batch op declares
every touched entity (PROJECT, INBOX, TASKs, TODAY_TAG) via a new
affectedEntities field threaded through op-log capture, conflict detection,
the sync server (+Prisma migration) and shared-schema. Per-effect
completeProject handlers (issue two-way-sync, time-block, repeat-cfg)
re-derive the task changes the atomic op bypasses.
* revert(sync): remove affectedEntities multi-entity conflict detection
Reverts
|
||
Johannes Millan
|
0d1869263f |
docs: remove outdated and implemented plan docs
Delete 29 plan/design docs whose work has shipped or been superseded (SuperSync slices, sync-core extraction, encryption-at-rest drafts, document-mode/Stage-A persistence, calendar/CalDAV concepts, focus-mode time-tracking sync, etc.). Kept the still-forward-looking docs (e.g. supersync-encryption-at-rest, sync-core-simplification-roadmap, calendar-two-way-sync-technical-analysis). Source comments that cited deleted docs are rewritten into self-contained inline rationale so no "see docs/..." reference dangles. |
||
|
Johannes Millan
|
c41c18f247
|
fix(sync): guard fresh-client first sync against data-loss trap & decrypt-race (#7980)
* fix(sync): guard fresh-client first sync against data-loss trap & decrypt-race A genuinely-fresh client seeds example tasks before its first sync, so on first connect to a populated, encrypted SuperSync dataset it hit a SYNC_IMPORT conflict dialog whose USE_LOCAL would overwrite the real remote, plus a red DecryptNoPasswordError. - A1: SyncImportConflictGateService flags isNeverSynced (\!hasSyncedOps) on the incoming-import dialog; the dialog guards the destructive USE_LOCAL with a confirm and focuses the scenario-appropriate safe button via cdkFocusInitial. - B: OperationLogDownloadService logs 'encrypted ops, no key' quietly only for a genuinely-fresh client (never synced AND no local encryption flag); it stays loud (dropped-credential signature) for already-synced clients and for a wiped op store whose config still flags encryption on, via the new optional provider method SuperSync.isEncryptionEnabled(). - D: SuperSync.isReady() returns false while isEncryptionEnabled && \!encryptKey, keeping a self-inconsistent encrypted config out of auto-sync; shares the _isEncryptionHalfConfigured predicate with getEncryptKey(). Tests: super-sync isReady/isEncryptionEnabled, conflict-gate isNeverSynced, dialog confirm-guard + focus-by-scenario, download severity branches, and a sync-service end-to-end first-sync-trap guard. Plan/rationale: docs/plans/2026-06-03-fresh-client-first-sync-data-loss-trap.md * docs(sync): add fresh-client first-sync data-loss-trap plan Documents the shared root cause (example tasks seeded pre-first-sync), the A1/B/D fixes, and the accepted Fix C residual (example-task pollution onto non-encrypted accounts) with the rationale for deferring an identity-based cleanup. * fix(sync): capture never-synced guard pre-download for piggyback SYNC_IMPORT The SYNC_IMPORT conflict gate's never-synced guard (which blocks a fresh client from force-overwriting a populated remote via USE_LOCAL) was computed from a live hasSyncedOps() read on the piggyback-upload path. By the time that gate runs, the same sync has already persisted downloaded ops with syncedAt and marked accepted uploads synced, so the flag flips to "has synced" mid-cycle and the guard disarms itself — re-opening the data-loss trap on the piggyback path. Capture the never-synced snapshot once at sync-cycle start, before download, and thread it through downloadRemoteOps()/uploadPendingOps() into the gate. The gate falls back to a live read only for standalone callers (download gate is safe live: nothing is persisted until processRemoteOps). Also stop the sync-wrapper catch from re-logging the expected DecryptNoPasswordError at error level — the download/upload service already logs it at the right severity, so re-logging undid the fresh-client onboarding-prompt quieting. Tests: gate honors a caller-provided isNeverSynced without consulting live history; piggyback path flags isNeverSynced=true even when the upload marks ops synced; wrapper threads the pre-download snapshot. Refresh the plan doc's stale test counts and document the fix (§11). |
||
|
Johannes Millan
|
5fa1383bc5
|
fix(ios): reconcile time tracking and focus mode on app resume (#7837)
* fix(ios): reconcile time tracking and focus mode on app resume iOS suspends the WKWebView WebContent process within seconds of backgrounding, freezing JS timers so tracked time and the focus-mode countdown stall. On resume we now credit the wall-clock gap (capped at MOBILE_BACKGROUND_IDLE_CAP_MS) via a wake-up tick, reset the tracking anchor, flush accumulated time, and nudge the focus reducer to recompute elapsed. On pause we flush accumulated time and drain the op-log write queue inside the existing BackgroundTask.beforeExit budget. Effects are gated by IS_IOS_NATIVE and registered only on iOS; handler bodies are exported as pure functions for unit coverage. Refs #7824, #7826 * fix(ios): persist tracked time in background budget, drop duplicate flush The flushOnPause$ effect duplicated the op-log drain that main.ts already runs inside BackgroundTask.beforeExit, but executed outside that budget and raced the main.ts listener — so accumulated time dispatched by the effect could be lost on suspension. flushPendingWrites also has a 30s timeout, well beyond the iOS budget, so the "drains within budget" premise was false. Move the only needed pause work — flushAccumulatedTimeSpent() — into the existing budgeted main.ts iOS handler before the drain, and remove the pause effect, its OperationWriteFlushService dependency, and onPause$. onResume$ becomes a plain Subject since the iOS producer is a JS listener registered at bootstrap (no cold-start replay race, unlike the native-fed Android shim). Refs #7824, #7826 * test(ios): cover resume effect wiring via extracted factory The reconcileOnResume$ effect field is false under Karma (IS_IOS_NATIVE gate), so the onResume$ -> withLatestFrom(selectTimer) -> handler pipe was untested. Extract the pipe into an exported reconcileOnResume factory and add specs that drive it against a mock store, covering the selector read and per-resume reconciliation that the pure-handler tests could not reach. Refs #7824, #7826 |
||
|
Johannes Millan
|
f10d0e9d48
|
Android soft-keyboard: fix dark-theme white flash on resize (#7839)
* test(e2e): raise timeouts for two CI-flaky waits Both timeouts repeatedly hit their limit on saturated scheduled runners without indicating a real product regression: - supersync.page.ts:781 — final sync-state icon waitFor went 10s → 30s. The race above it already consumed a 30s budget, so falling back to 10s here is too tight when the runner is hot (e.g. SuperSync 5/6 in run 26514574130, "Client A can migrate multiple times" failure). - repeat-task-day-change #6230 — post-midnight visibility went 30s → 60s. Even with the focus-event fallback added in |
||
|
Johannes Millan
|
6d81dde043
|
feat(plugins): adopt PERSISTED_DATA_CHANGED in document-mode (#7752) (#7812)
* feat(plugins): adopt PERSISTED_DATA_CHANGED in document-mode (#7752) Document-mode's iframe editor went stale when another device edited the same context's doc — the editor kept typing on in-memory `storedState` and the next throttled save merged-on-stale-base, partially clobbering the remote edit. Background's `enabledIds` set had the same gap for remote toggles. Adopts the host-side `PERSISTED_DATA_CHANGED` hook (shipped in #7805, made multi-handler-safe in #7811) in both surfaces: - `background.ts` — on fire: re-read `enabledIds`, diff, and call `showInWorkContext`/`closeWorkContextView` only when the active context's membership flipped. Idempotent on no-op re-fires; the hook also fires for `doc:` writes which background ignores via the set-unchanged short-circuit. - `ui/editor.ts` — on fire: load the current ctx's raw `doc:` bytes via the new `loadContextDoc` `{ raw, parsed }` shape, byte-compare to `lastSeenRemoteData`. Equal → noop (self-echo or another key's fire). Different → show a one-button "reload" banner with a distinct `doc-banner--remote-update` modifier class. Clicking Reload re-runs `setActiveContext` which already re-reads + setContent's. `isDocCorrupt === true` short-circuits to a direct reload (the corruption banner already promises saved data is untouched). The load-bearing piece is `lastSeenRemoteData` — captured as the raw `loadSyncedData()` string on both load (`setActiveContext`) and write (`flushSave` / `flushSaveSync`). The editor's own `JSON.stringify(getJSON())` is NOT byte-stable across the load path because `prepareStoredDoc` reshapes chip content against the local task cache, so comparing against editor output would mis-flag every load as remote. Raw-against-raw keeps the host's deterministic encoding the only thing that matters. Acceptance criteria from #7752: all met except the E2E append, which needs iframe-level state injection that the existing spec helpers don't cover; covered by manual verification + unit specs instead. What this doesn't fix: - No cross-context notification (editor catches up silently on switch). - Same-context concurrent edits still resolve whole-doc LWW. - No silent swap or selection preservation. - No "Keep mine" force-flush — dismiss-and-keep-typing already wins LWW. Tests: 84/84 plugin specs pass (+6 background hook membership branches, +1 corrupt-entry raw-bytes case, +1 saveContextDoc-returns-raw case). Plan v6 changelog documents the v5/v6 multi-review iterations and the cuts (pre-reload backup, pure-fn file, coalesce timer, "Keep mine"). * refactor(plugins): apply doc-mode review findings (#7752) Multi-review of the prior commit surfaced two real and one likely-real bugs in the reconciler and a few cleanup wins. Apply all that survived verification. W1 — wrap getActiveWorkContext in try/catch. The prior handler only caught loadEnabledCtxIds rejections; a getActiveWorkContext throw would escape via the void onPersistedDataChanged() registration as an unhandled rejection and leave enabledIds stale across every subsequent fire. W2 — snapshot enabledIds at entry. The prior handler read the closure variable across multiple awaits before assigning at the end; two interleaved fires could mis-compute wasActiveEnabled and drop a close/show call. Pass-by-parameter to reconcileEnabledIds eliminates the in-function race; concurrent fires now race only on the final caller assignment, which is at least eventual-consistent. W3 — extract reconcileEnabledIds to its own file and import the real function from the spec (no more local copy). reconcile-enabled.ts sidesteps the testability problem at its root: background.ts has top-level PluginAPI side-effects (registerWorkContextHeaderButton, registerHook) that crash in node, so the spec can't import from there. The new file holds only the pure reconciler + try/catch hardening. W4 — extract serializeContextDoc(doc) helper. flushSave and flushSaveSync now produce byte-identical output via the same function, so a future encoding change can't silently desync the sync path from the async one. W5 — fix the inline comment on showRemoteUpdateBanner re-entrancy to match what the code actually does (early-return because text is invariant, not "replace text in place" as the v6 plan implied). S1 — rename lastSeenRemoteData → lastSeenDocBytes. The variable also holds local-write bytes, not only "remote" ones; the new name follows the existing lastSeenTaskIds convention. S2 — drop the try/catch around loadSyncedData in onRemotePersistedDataChanged. The hook dispatcher already catches + logs handler rejections (PluginHooksService._invokeWithTimeout), and loadContextDoc elsewhere in this file follows the no-wrap convention. S3 — add primitive-JSON case to persistence.spec.ts. loadContextDoc's new {raw, parsed} shape silently forwards parsed=123/null/"hi" to callers; the editor's truthy guard is the safety net, but the persistence contract is now locked. S5 — explain why isDocCorrupt short-circuits to a direct reload (auto-recovery without user click; alternative would leave the user stuck on the corruption banner even when the remote already fixed the entry). Tests: 86/86 plugin specs pass (+7 reconciler error/membership cases, +1 persistence primitive-JSON). Bundle redeployed. * docs(plugins): update stale lastSeenRemoteData refs in JSDoc/comments Two comments referenced the pre-rename variable name. |
||
|
Johannes Millan
|
8f274582e2
|
feat(plugins): Stage A keyed persistence with LWW (#7749) (#7763)
* feat(plugins): keyed persistence API for per-context LWW (Stage A Phase 1+3) Add an optional `key` argument to `persistDataSynced` / `loadSyncedData`, composed at the bridge transport boundary into `pluginId:key` entity ids. Distinct keys now produce distinct ops that LWW-resolve per-entity, enabling document-mode-style plugins to avoid cross-context blob overwrites without changing existing keyless callers. Phase 3: `removePluginUserData(pluginId)` now sweeps the full prefix (legacy entry + every keyed entry), dispatching one delete per match with the rule-6 setTimeout(0) trailer so remote replicas don't keep keyed entries after uninstall. The reducer-only "smart prefix match" shortcut is wrong (one op for the prefix only, remote keyed entries leak) — see docs/plans/2026-05-23-stage-a-keyed-plugin-persistence.md Phase 3. Phase 4 (document-mode plugin-side migration of the legacy single-blob entry) is left as a separate follow-up so the host change can be reviewed in isolation. Issue #7749 * feat(document-mode): migrate to keyed persistence (Stage A Phase 4) Move from one synced blob under the bare plugin id to per-entity keyed entries: - meta — { enabledCtxIds: string[] }, owned by background.ts - doc:${ctxId} — one entry per context, owned by the editor iframe - __meta__ — migration stamp Each entry has its own LWW timestamp on the host, so a concurrent edit in project A on Device 1 and project B on Device 2 no longer whole-blob-collide. The migration runs idempotently from both background.ts and editor.ts (stamp-guarded), splits the legacy single blob into keyed entries, then tombstones the legacy entry with an empty payload — giving LWW a winning side against any offline device that still writes the old shape. flushSave / flushSaveSync no longer need to read+merge sibling state, since each context's entry stands alone. The future-version blob guard (isStorageUnreadable) is dropped — it referenced the wrapping blob's version, which no longer exists; per-doc corruption still falls back via isDocCorrupt. Issue #7749 * fix(plugins): tighten Stage A keyspace at the boundaries Multi-review surfaced three small gaps in the keyed-persistence rollout: - The synchronous composeId throw covers the bridge's iframe and direct-API entry points, but three in-process callers (plugin-config.service, plugin.service, plugin-config-dialog) bypass the bridge and route directly into the persistence service. A user-installed plugin with `id: "evil:plugin"` passed manifest validation and would have collided with the legitimate `evil` plugin's keyed namespace — `removePluginUserData('evil')` would have over-matched the sweep. Reject the colon at install time in `validatePluginManifest`; keep the bridge throw as defense-in-depth. - The new `key` arg at the bridge was typia-asserted on `data` but unchecked itself. A compromised iframe could pass a multi-megabyte string or a non-string value via postMessage. `data` is capped at 1 MB, but the entity id composed from `key` would be stored verbatim in NgRx state, IndexedDB, the op-log, and on the sync wire — bypassing the data cap. Add `assertPluginPersistenceKey` with a 256-char cap. - `_loadPersistedData` silently returned `null` when composeId threw, while `_persistDataSynced` rethrew. The asymmetry made a malformed pluginId look like "no data yet" on the load side, indistinguishable from a fresh install. Hoist composeId + key validation out of the load try/catch so it throws symmetrically. Issue #7749 * fix(plugins): lower per-write cap to 256 KB The pre-Stage-A 1 MB cap was sized for the old single-blob shape, where one entry held every context's data. With the keyed split, each entity gets its own write budget — 1 MB per write is wildly over-provisioned for the realistic upper bound of plugin payloads (heavy document-mode docs ~30–100 KB, configs and automations KB-scale). 256 KB keeps 2–5× headroom over realistic payloads while bounding the per-plugin storage growth more tightly. Document-mode's migration loop now skips oversized legacy docs instead of aborting the whole run: a user whose legacy blob holds one ~500 KB doc (legal under the old cap) keeps the other contexts migrated and the original bytes preserved in the legacy entry. The success stamp stays at migrated:0 in that case so a future build (or pruning of the doc) can complete the migration without data loss. Issue #7749 * test(plugins): e2e migration of legacy single-blob to keyed entries The migration logic in document-mode is unit-tested against a mock PluginAPI, which can't catch real-iframe quirks (postMessage handling of undefined second args, commit-chain timing under the host's per-entity rate limiter, hydration ordering against the op-log). Add two end-to-end scenarios: - Fresh install: enable the plugin, verify the __meta__ stamp lands at migrated:1 (the migration's final write — observing it implies every earlier step completed). - Legacy blob: seed a pre-Stage-A single-blob entry via the e2e helper store, enable the plugin, verify the legacy entry is tombstoned, meta carries the enabledCtxIds, and each doc landed under its own doc:${ctxId} key. Issue #7749 * chore(plugins): drop dead code and review-driven polish Four small follow-ups from the multi-review pass: - Don't log the plugin-supplied `key` value. Plugins may use user content (search queries, doc titles) as keys; the log history is exportable. Log `keyLen` instead, per CLAUDE.md rule 9. - Delete `detectStaleLegacyWrite` and its 3 specs. Exported and fully tested, but zero non-test callers — banner UI is forbidden by project convention for transient-only messaging. If the need resurfaces, the implementation is four lines. - Drop the `attemptedAt` field from `MigrationStamp`. It was written but never read; the success stamp is the only re-entry gate, and the resume path is just "re-run the loop" — re-writes are content- idempotent. Saves one rate-limited write per fresh migration. - Update `docs/plans/2026-05-23-stage-a-keyed-plugin-persistence.md` with an implementation-status table referencing the shipping commits, so future readers don't have to dig through git. Issue #7749 |
||
|
Johannes Millan
|
196e50b906
|
test: strengthen unit test assertions and revive disabled plugin specs (#7755)
* chore(plugins): re-bundle document-mode and document Stage A path Reverts the unbundling from |
||
|
Johannes Millan
|
0c2420eeb3 |
feat(plugin): improve sync data size for plugins
- docs(plugins): remove delta-sync plan; track Stage A as GH issue - fix(plugins): preserve teardown safety net + earlier base64 size gate - fix(plugins): address second-round multi-review findings - refactor(sync): drop unused isVirtualEntity host re-export - fix(plugins): address multi-review findings in plugin persistence - perf(plugins): delegate plugin-data codec to sync-core helpers, cap decompression - docs(sync): mark delta-sync plan implementation status - perf(plugins): gzip plugin user data at the persistence boundary - perf(document-mode): raise save throttle and add focus-loss flush triggers - fix(sync): resolve plugin-data LWW conflicts via array storagePattern |
||
|
Johannes Millan
|
7a93265281 |
docs(sync): add document-mode sync data model and delta-sync plans
Two design docs for slimming the document-mode plugin's sync footprint: the sync-data-model plan covers the immediate fix (bare-atom chips) plus deferred per-context entities; the delta-sync plan analyses why true deltas need finer entity granularity or a commutative CRDT (Yjs) given the partially-ordered op-log. Refs #7740. |
||
|
Johannes Millan
|
713245e6f0 |
docs(sync): add SUP_OPS versionchange handler plan (#7735)
Rescopes #7735 from the original three-connection consolidation down to its one genuine correctness fix: register versionchange handlers on the OperationLogStoreService and ArchiveStoreService SUP_OPS connections so a future schema bump cannot be stalled by a handler-less connection. The connection consolidation is documented as not planned (no behavioral benefit, net-additive LOC on a safety-critical path); the rejected "OperationLogStoreService as connection owner" alternative is kept for the record. Both decisions followed multi-agent review rounds. |
||
|
Johannes Millan
|
50e2b53d90 | Merge branch 'master' into feat/doc-mode4-2880bb | ||
|
Johannes Millan
|
508998c6a1
|
Improve on sync (#7736)
* fix(android): restore share title derivation and dedupe shared tasks Commit |
||
|
Johannes Millan
|
1c10ff67dd |
feat(document-mode): add TipTap-based document-mode plugin
A document-mode plugin under packages/plugin-dev/document-mode/ that renders the active work context's tasks as an editable TipTap document. Per-context doc state is persisted as a last-writer-wins JSON blob via PluginAPI.persistDataSynced; task identity (title, done state, hierarchy) stays in NgRx and is reached through PluginAPI.updateTask and the ANY_TASK_UPDATE hook. It registers a work-context header button and embeds itself into the work-view embed slot added in the preceding commit. See docs/plans/2026-05-21-document-mode-tiptap-plugin.md for the full design. Squashed from the feat/doc-mode-v4 work; full per-step history is preserved in the archive/doc-mode-v4-full-history branch and the doc-mode-v4-pre-squash tag. |
||
|
Johannes Millan
|
292f8b0e9a
|
refactor(sync): address multi-review findings on #7709 (#7712)
* docs(sync): plan for clean-slate upload data-loss prevention (#7709) Revised after a 6-reviewer parallel pass. Splits into 4 PRs; PR-A (atomicity + empty-snapshot dialog fix + pre-migration-backup implementation + dead-code cleanup) closes the reported precondition chain. Preflight gating deferred pending forensic log evidence. * test(sync): reproduce #7709 precondition — interrupted createCleanSlate Integration tests proving that an interrupt between `clearAllOperations()` and `append()` on a device that has never reached the compaction threshold leaves the device in `isWhollyFreshClient===true && hasMeaningfulStoreData===true` — the exact branch that throws `LocalDataConflictError(0, {})` and opens the issue #7709 conflict dialog. Also documents two adjacent findings: - Interrupt at `setVectorClock` corrupts state_cache but does NOT trigger the #7709 chain (lastSeq stays > 0). - A previously-compacted device is NOT vulnerable: state_cache survives the interrupt, so `isWhollyFreshClient` stays false. These tests will need to be flipped (or deleted) when PR-A lands the atomic destructive sequence: the bug post-condition should become impossible. * fix(sync): atomic destructive state replacement closes #7709 precondition `CleanSlateService.createCleanSlate` and `BackupService.importComplete` both ran a four-step destructive sequence as independent IndexedDB transactions: `clearAllOperations` → `append(syncImportOp)` → `setVectorClock` → `saveStateCache`. An interrupt between steps on a device that had never reached COMPACTION_THRESHOLD = 500 ops left OPS empty and state_cache still null — i.e. `isWhollyFreshClient()===true` with meaningful in-memory data, the precondition that routes through `operation-log-sync.service.ts:606` and throws `LocalDataConflictError(0, {})` to open the conflict dialog. From there a "Keep remote" click propagates the empty server snapshot to every other device. Replace both call sites with a new `OperationLogStoreService.runDestructiveStateReplacement` helper that uses snapshot-then-swap: 1. Stage the new state to STATE_CACHE under STATE_CACHE_STAGING_KEY (single-store write; large payload kept out of the destructive tx). 2. Round-trip verify the staged row. 3. One short multi-store readwrite transaction (OPS + STATE_CACHE + VECTOR_CLOCK) does small writes only: clear OPS, append the SYNC_IMPORT entry, write the vector clock, promote staging row to the singleton, delete the staging row. 4. On any error, explicitly `tx.abort()` so already-queued writes (notably the `clear()`) are rolled back. IDB does NOT auto-abort a transaction when JS throws between `await`ed requests — a mid-flight exception lets the tx commit partial state without the explicit abort. The integration test caught this before the helper landed. 5. `init()` sweeps any orphaned staging row left over from a previous interrupted destructive replacement. `BackupService` additionally bails out cleanly if the pre-import backup write failed — better to refuse the destructive import than overwrite local state without a recovery point. clean-slate-interrupt.integration.spec.ts inverts: it previously demonstrated the corrupt post-condition; now it proves the device's prior state is preserved through every injected failure mode (staging read, destructive tx abort, never-compacted device). * fix(sync): address review gaps in #7709 destructive replacement - BackupService now throws (not silently returns) when the pre-import backup fails, so the caller doesn't fall through into a hybrid state (NgRx + archives + sync seqs replaced, op-log still old). - Roll back clientId rotation in CleanSlate/Backup if the destructive tx aborts; pf and SUP_OPS now agree on the device's clientId after either success or failure. - runDestructiveStateReplacement: patch the singleton's lastAppliedOpSeq to the assigned seq (was hardcoded 0), require snapshotEntityKeys from callers (was undefined → triggered an "old snapshot format" recompaction after every destructive replacement), drop the unused Promise<number> return, drop the dead-defensive verify-read, switch boot probe to getKey to skip multi-MB structured clone of an orphan staging row, and sanitize the raw Error in the reconciliation log. * test(sync): cover clientId rollback-fails edge case (#7709) When both the destructive call and the clientId rollback throw, the caller must still see the ORIGINAL destructive failure (not the rollback error), and the rollback failure must be logged at critical level. Pins down behaviour the rollback path previously only reasoned about. * refactor(sync): tighten destructive replacement + log forensics (#7709) Follow-up to round-2 multi-review findings. - runDestructiveStateReplacement: write the new singleton row directly from opts.newState inside the destructive tx. Drops the in-tx stateCacheStore.get(STAGING_KEY) and the {...staged, ...} spread, which re-cloned the multi-MB payload while holding the multi-store tx open. Also restores the implicit "newState \!= null" precondition that the prior verify-read had enforced (the new "if (\!staged)" branch was weaker than the dropped check). Staging row remains as a crash-detection sentinel for boot-time reconciliation. - _cacheLastSeq = 0 after the destructive write to match the pattern used by every other write site in the file (the previous "= seq" was unreadable because _appliedOpIdsCache=null forces a rebuild anyway). - OpLog.critical on rollback failure now also carries originalError (name + message), so a forensic reader can correlate the pf/SUP_OPS divergence with the destructive failure that triggered it. - Sanitise the pre-existing raw Error in [CleanSlate] pre-migration backup warn log to { name, message }, matching the rest of the module. - Drop the brittle log-string regex in the rollback-fails tests; structural payload assertion (priorClientId + originalError) locks the contract without coupling to log wording. * refactor(sync): delete unused PreMigrationBackupService placeholder (#7709) The service was committed as a no-op placeholder before this branch and never implemented. Its intended purpose — provide a recovery path independent of IDB atomicity — is obsolete now that runDestructiveStateReplacement makes the destructive sequence atomic within SUP_OPS. The destructive tx either fully commits or fully rolls back, so there is no partial-write state to recover from. - Delete pre-migration-backup.service.ts and its placeholder spec. - Remove the DI wiring, injected field, and dead try/catch from CleanSlateService. - Rename PreMigrationReason → CleanSlateReason and define it locally; the type is internal to clean-slate.service.ts (single string-literal caller in encryption-password-change.service.ts). - Remove the PreMigrationBackupService mock from the unit + integration specs and drop the "should continue if pre-migration backup fails" test (the code path it covered no longer exists). - Update the plan doc: PR-A no longer ships pre-migration backup; Fix 3 and Fix 6 deferred to follow-up PRs. * refactor(sync): address multi-review findings on #7709 Multi-review of the #7709 destructive-replacement branch surfaced one privacy regression and several over-engineering smells that landed in the earlier commits. This commit cleans them up. - Drop priorClock from the "Starting clean slate" log payload — vector- clock keys are per-device clientIds and Log history is user-exportable. The branch's own plan explicitly forbids logging clock contents ("Security C2: size only, never the contents"); the implementation regressed against that and the test asserted the wrong shape. Spec now asserts the field must NOT appear. - Drop the snapshot-then-swap staging row from runDestructiveStateReplacement. The staging row was supposed to keep the multi-MB payload outside the destructive multi-store tx, but the in-tx singleton put already wrote the same payload — so staging cost one extra full-state structured clone, one boot-time getKey round-trip per cold start, a catch-block cleanup, two integration tests, and ~30 lines of JSDoc, in exchange for a crash-detection sentinel nothing acts on. Single multi-store readwrite tx provides the same atomicity guarantee with none of the machinery. (Comment on the retained tx.abort() in the catch path corrected — it is unreachable in production but load-bearing for the spy-based fault-injection seam used by the interrupt integration test.) - Extract ClientIdService.withRotation(logPrefix, fn) so the cross-DB clientId rotation+rollback dance is owned in one place. CleanSlateService and BackupService delegate; ~20 lines of byte-for-byte duplication removed. Rollback semantics (happy path, restore-on-failure, no-prior-id edge case, rollback-itself-fails) are now tested once against the helper instead of duplicated across the two consumer specs. Net diff: 249 insertions, 401 deletions across 9 files. Verified: full Karma suite passes in both Europe/Berlin and America/Los_Angeles timezones (9444 / 9430 pre-existing pass; the 10 failures are in immediate-upload.service.spec.ts and reproduce on master, unrelated to this change). tsc --noEmit, ng lint, stylelint, and the custom lint rules all clean. Public APIs of CleanSlateService and BackupService unchanged; their three production callers (encryption-password-change, user-profile import x2) require no updates. Closes the post-implementation review for #7709. * Fix destructive import race conditions * refactor(sync): address round-3 multi-review findings on #7709 Six-reviewer parallel pass on the destructive-replacement branch surfaced one privacy regression, one awkward control-flow pattern, and a stale plan-doc section. This commit clears them. - LockService.request: generic <T> return type. The Web Locks API and the fallback mutex both naturally return the callback's value; Promise<void> was just too narrow. Lets CleanSlateService.createCleanSlate() drop the `let result: ... | null = null` + null-check + cast + unreachable "completed without a replacement result" throw and use a single `const { syncImportId } = await lockService.request(...)`. Generic mock signatures threaded through 9 spec files (19 callFake sites). - ClientIdService: stop logging plaintext clientIds. Per CLAUDE.md sync rule 9 (log history is user-exportable) the plan's own Fix-4 contract says "never log vector-clock contents; size only" — clientIds are the keyspace of the vector clock, so the same rule applies. Four log sites (loadClientId, generateNewClientId, persistClientId, invalid-format critical log) now omit the id; CleanSlateService logs a 3-char suffix where correlation is useful but the full value is not. - BackupService: drop `message` from the pre-import-backup failure log. Matches the `name`-only pattern used by ClientIdService.withRotation rollback logging; future validator/IDB error types could otherwise interpolate user content into Error.message. - runDestructiveStateReplacement: JSDoc-document the payload duplication trade-off (full state lives in OPS for the uploader and in STATE_CACHE for `isWhollyFreshClient`; eliminating either is unsafe — STATE_CACHE can be advanced past the SYNC_IMPORT op's seq by compaction). - Plan doc: replace the snapshot-then-swap "Fix 2 Implementation" section with a description of the single multi-store readwrite tx that actually shipped. The plan's load-bearing premise ("every existing db.transaction() call is single-store") was wrong: appendWithVectorClockUpdate already runs a 2-store readwrite tx on every action. Revision-history bullet adjusted to record both the initial switch and the revert. Verified: 788 tests across the touched sync surface pass (clean-slate / backup / client-id / lock / write-flush / compaction / upload / download / remote-ops / capture-effects / store / repair / interrupt-integration / task-done-replay / migration-handling / focus-mode-reducer / bug-7707 / focus-mode-effects / operation-log-sync / superseded-operation-resolver). checkFile clean on every modified .ts file. * test(sync): interleave-race coverage + boot-time partial-write detector (#7709) Two additions raising end-to-end confidence on the #7709 fix: 1. Interleave race test (operation-log.effects.spec.ts): Asserts that a queued op acquiring the operation-log lock AFTER a concurrent destructive replacement reads the POST-rotation clientId. The existing test pinned the call ORDER (lock → clientId → append); this test pins the SEMANTICS: by the time the queued op's callback runs inside the lock, ClientIdService reflects the rotation, so the appended op carries 'newClient', not 'oldClient'. 2. Boot-time state_cache consistency check (operation-log-store.service.ts): Fire-and-forget on init: if state_cache.lastAppliedOpSeq references an op that doesn't exist in OPS, log a critical with counts-only forensics (referencedSeq, opsCount, vector-clock sizes). The atomic runDestructiveStateReplacement makes the invariant "state_cache.lastAppliedOpSeq always points to a real OPS entry" hold for any future write path that uses the helper. This check surfaces violations from either (a) pre-#7709 partial-state recoveries still on disk after upgrade, or (b) any future code path that bypasses the helper. Observability only — wrapped in try/catch, never blocks initialization. Counts-only payload (no entity IDs, no vector-clock contents) per CLAUDE.md sync rule 9. Both changes verified: 34 effects-spec tests + 166 store-spec tests pass. Full Karma run (9477 tests) has 10 pre-existing failures in immediate-upload.service.spec.ts that reproduce on upstream/master HEAD — unrelated to this branch. * refactor(sync): drop boot consistency detector, derive replacement state from the op Follow-up addressing multi-review findings on the #7709 atomicity work: - Remove _verifyStateCacheConsistencyOnBoot. The atomicity fix makes the partial-write signature unreachable via runDestructiveStateReplacement, and "OPS empty + state_cache present" is also the valid state left by a full-log compaction, so the two are indistinguishable at boot and a critical log there is a false alarm. Forensic logging is deferred to a later PR per the design doc's staged plan. - runDestructiveStateReplacement now derives newState / newVectorClock / schemaVersion from syncImportOp instead of taking them as separate opts. Nothing enforced that they agreed; a divergent value would silently desync OPS from state_cache, the exact bug class this work prevents. - Restore the null-tolerant archive guard (was undefined-only) to preserve the prior defensive handling of backups with null archive fields. |
||
|
Johannes Millan
|
e419fd6da5 | docs(supersync): design for generic CONCURRENTLY migration recovery | ||
|
Johannes Millan
|
b8be00d526 |
refactor(sync): decompose SuperSync server giants into cohesive modules
Split the three oversized SuperSync-server files into focused, single-responsibility modules behind thin SyncService / SnapshotService facades. Behavior, public API, HTTP/wire contract and DB schema are unchanged (verbatim moves). - sync.service.ts 2322 -> ~660, sync.routes.ts 1475 -> ~445, snapshot.service.ts 1215 -> ~390 LOC - src/sync/op-replay.ts: pure replay engine (no Prisma) - src/sync/conflict.ts: conflict detection + resolution - src/sync/sync.routes.payload.ts / .quota.ts: HTTP helper modules - src/sync/sync.routes.ops-handler.ts / .snapshot-handler.ts: POST handlers - services/operation-upload.service.ts: upload pipeline (runs inside the caller's prisma.$transaction; tx threaded, never opens its own) - services/snapshot-generation.service.ts: snapshot replay/generation - StorageQuotaService gains quota-driven eviction; DeviceService gains stale-device cleanup; shared upload/conflict types -> sync.types - EncryptedOpsNotSupportedError re-exported from snapshot.service for identity-stable instanceof in sync.routes - new op-replay.spec.ts / conflict.spec.ts; sanctioned private-spy re-points only (no behavioral spec changes) - includes the decomposition plan (docs/plans) and dead-weight cleanup Verified: tsc --noEmit clean; 717 tests pass / 5 skipped (35 files); checkFile + lint clean. Multi-reviewed (7 agents): behavior-faithful, all sync-correctness invariants preserved. Known gap: route-handler + multi-client integration specs are config-excluded (stale legacy / Postgres-only) and were not executed here — run integration in CI before merge. |
||
|
Johannes Millan
|
2d9988dd73
|
perf(sync): SuperSync server speed + correctness hardening (#7621)
* docs(sync): add super sync server perf plan * perf(sync): implement supersync server perf phases * fix(sync): bracket auth cache invalidation * fix(sync): avoid empty replay state stringify * fix(sync): harden supersync batch uploads * fix super sync review findings * fix(sync): guard payload bytes backfill rollout * perf(sync): speed up payload_bytes backfill and index its scan Raise the backfill batch size (DEFAULT 5->500, MAX 25->1000) so a 100M-row operations table backfills in minutes rather than tens of hours. Add a CONCURRENTLY partial index on (user_id, id) WHERE payload_bytes = 0: it drains to empty post-backfill so the boot-time backfill self-check and the BOOL_OR quota probe stop doing a full sequential scan to prove absence, and it makes the backfill's per-user keyset paging a true index seek. Wire the new concurrent-index migration into both deploy scripts' P3018 recovery path. Add migration-SQL guard tests for the ADD COLUMN (metadata-only fast path) and the new partial index. * fix(sync): bound auth cache invalidation map and bracket every delete The auth verification cache's invalidationVersions map grew one entry per lifetime-invalidated user with no eviction (unbounded heap on a long-lived single replica). Cap it at the same 10k LRU bound as the entries map, re-inserting the just-invalidated user at the MRU tail so the CAS race protection still holds for the only window that matters (one DB round trip). Bracket the passkey/magic-link registration cleanup deletes with pre+post invalidate to match the documented convention, and invalidate on verifyEmail so a freshly-verified user isn't denied for up to the cache TTL. * perf(sync): skip the redundant exact replay-state measurement The delta accounting is a proven over-estimate of the serialized state size, so when the running bound stays within the cap the true size is too and the final exact JSON.stringify is provably redundant. Skip it in that case (still measure-and-throw whenever the bound does not prove safety). This collapses the common small/incremental replay back to zero expensive full stringifications, matching the old per-op loop instead of regressing it. Name the entity-key JSON overhead constant and document that assertReplayStateSize's return value is load-bearing. * refactor(sync): split processOperationBatch into pipeline stages Extract the 297-line batch upload method into a thin orchestrator plus six named single-responsibility stage helpers (validate+clamp, intra- batch dedupe, classify existing duplicates, conflict-detect, reserve seq + insert, full-state clock). Behavior-preserving: every stage writes terminal rejections into the shared results array by index and the two empty-set guards short-circuit exactly as before. Also share the timestamp clamp, the duplicate-op SELECT, and the merged full-state clock persistence between the batch and legacy paths so they cannot silently diverge. * test(sync): pin batch error-code divergence and aggregate-once Strengthen the intra-batch duplicate test to assert same-id / different-content yields DUPLICATE_OPERATION (deliberate divergence from the legacy INVALID_OP_ID), and document the divergence in the plan. Replace the single-full-state aggregate test with two full-state ops + a spy asserting _aggregatePriorVectorClock runs exactly once and last-write-wins — the old test could not catch a per-op-aggregate regression. Add a makeOp fixture factory. Correct the plan's overstated replay-stringification numbers. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: johannesjo <1456265+johannesjo@users.noreply.github.com> |
||
|
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
|
4e1ace0786 | refactor(sync): remove encryption test mocks | ||
|
Johannes Millan
|
faa18c1638 | docs(sync): add plan to extract encryption primitives to @sp/sync-core | ||
|
johannesjo
|
c40feef6d2 |
refactor(sync-providers): move SuperSync provider into package
Move SuperSync's provider class, model, and spec from
`src/app/op-log/sync-providers/super-sync/` into
`packages/sync-providers/src/super-sync/` behind the ports
established by slices 5-6 (logger, platform-info, web-fetch,
native-http-executor, credential-store) plus two new ports for this
slice: `SuperSyncStorage` (narrow, three methods for `lastServerSeq`
persistence) and `SuperSyncResponseValidators` (host-side wrapper
that keeps `@sp/shared-schema` out of the package boundary).
Privacy fixes applied inline per multi-review consensus:
- `AuthFailSPError(reason, body)` now only takes the extracted
reason — body is no longer retained on `additionalLog`.
- `_handleNativeRequestError` drops the parenthesised
`(${errorMessage})` interpolation from the user-facing message
to avoid leaking hostname/URL on native-stack errors.
- Timeout `Error.message` drops the `path` interpolation
(`excludeClient` clientId was leaking into the thrown message).
- `_extractServerErrorReason` caps the extracted server reason at
80 chars to defend against future server contract drift.
- Thrown non-2xx errors use a fixed `HTTP <status> <statusText> —
<reason?>` form (web) or `HTTP <status> — <reason?>` (native)
instead of embedding the raw response body in `Error.message`.
JSDoc invariants pinned on `getEncryptKey`, `getWebSocketParams`,
`_cachedServerSeqKey`, and `deleteAllData` response shape.
Compression switched to direct `@sp/sync-core` import (no
`CompressError` wrapping at SuperSync's call sites; the existing
app-side compression-handler still wraps for
`EncryptAndCompressHandlerService`).
Spec converted Jasmine → Vitest. `TestableSuperSyncProvider`
subclass gone — native-platform routing now tested via injected
`platformInfo` + `nativeHttpExecutor` mocks. Adds a new
`privacy regression: no user content in error/log surface`
describe block driven by an HTTP-error path with a body containing
plausible user content (taskId/title/accessToken).
App-side `super-sync.ts` collapses to a 50-line
`createSuperSyncProvider()` factory (no `extraPath` arg —
SuperSync explicitly ignored it). The `SyncProviderId.SuperSync`
enum stays app-side; `AssertSuperSyncId` conditional pins
enum-vs-constant drift at compile time. `super-sync.model.ts`
becomes a re-export shim. `sync-providers.factory.ts` updated.
`super-sync-restore.service.ts` narrows the package's
`RestorePoint<string>` return through the consensus "narrow at the
app shim" decision (single cast at the call boundary).
Bundle: CJS 95.15 KB / ESM 92.34 KB (up from 76.78 / 74.18). Right
in the performance reviewer's 95-100 KB estimate; under the
original 110 KB projection. Tiered barrel split remains a
documented deferral.
Package spec count: 273 (was 197, added 76 SuperSync specs).
All targeted app specs green: sync-wrapper (107), op-log (2513
across the directory), imex/sync (381).
|
||
|
johannesjo
|
3d0ff2aec7 |
docs(sync-core-plan): record SuperSync slice multi-review consensus
Folds the six-Claude-lens (correctness, security/privacy, architecture, alternatives, performance, simplicity) review pass into the SuperSync slice design doc. Closes all 8 open questions in-doc, records new blockers the original draft undercounted, and trims the body per the simplicity reviewer's recommendations. Key revisions: - Storage port is narrow `SuperSyncStorage` (3 methods), not the generic `KeyValueStoragePort` originally proposed. Architecture and alternatives both pushed back. - Promoted helper renamed to `isRetryableUploadError` (intent- anchored) to avoid naming collision with package's existing `isTransientNetworkError`. - Factory drops `extraPath` — SuperSync explicitly ignores it. - Privacy A1 fix uses extracted-reason form (via `_extractServerErrorReason`, capped at 80 chars), not blanket body-drop — preserves 5xx debug context. Four new privacy blockers added that the original sweep missed: - `AuthFailSPError(reason, body)` retains body via `AdditionalLogErrorBase.additionalLog` (security + correctness both flagged independently — PR 5b did NOT scrub this for SuperSync's call site). - `_handleNativeRequestError` re-throw embeds raw `errorMessage` (can leak hostname/URL from native-stack `.message`). - Timeout `Error.message` embeds `path` with `excludeClient` query param (pseudonymous clientId). - `_extractServerErrorReason` returns server `error` field uncapped. Dissents recorded: alternatives reviewer preferred pre-split spec before move, moving CompressError/DecompressError into sync-core for strict behavior preservation, and barrel split as PR 7d. |
||
|
johannesjo
|
6a1f569a09 |
docs(sync-core-plan): draft SuperSync slice design for multi-review
Captures the SuperSync provider move (slice 7) design surface before implementation: which files move into @sp/sync-providers, which stay app-side, which ports get reused from prior slices, which new ports this slice introduces, the privacy sweep checklist, the suggested commit shape, and the open questions for parallel multi-review. Key design calls flagged for review: - response-validators.ts stays app-side (banned @sp/shared-schema import). New SuperSyncResponseValidators port. - localStorage access becomes a KeyValueStoragePort. - isTransientNetworkError promoted from app sync-error-utils to the package as isTransientErrorMessage (distinct from the package's existing native-error-code-aware version). - Compression imported directly from @sp/sync-core; CompressError wrapping dropped at SuperSync call sites (no instanceof catches exist). - Two privacy A1 sites identified: _doNativeFetch:646-648 and _doWebFetch:582 embed response body in thrown Error.message; fix via fixed status-only message. - Spec migration kept monolithic for the move (1553 lines), split deferred to a follow-up. Multi-review consensus block left blank for the reviewer pass. |
||
|
Johannes Millan
|
cf4fbd22e3 |
docs(sync-core-plan): fold Gemini review dissent into consensus
Gemini's review eventually completed after a workspace-sandbox retry and quota throttling. Its findings broadly affirm the Claude consensus, with two dissents — both rejected with reasoning: - Q1 (port reuse): Gemini wanted to keep the new WebDavNativeHttpExecutor port "for consistency with NativeHttpExecutor." Rejected — the architecture and simplicity reviewers verified by code reading that the existing NativeHttpExecutor already supports arbitrary methods, responseType: 'text', and maxRetries: 0. Naming consistency is a weak argument against actual port duplication. - Q6 (retry policy): Gemini wanted a 2-attempt + explicit 423 Locked retry. Rejected — alternatives and simplicity flagged this as a behavior change masquerading as a refactor. Adapter has zero retries today; preserving that keeps the slice scope a move. Per-call-site maxRetries remains trivially available once we reuse the existing port. Refresh the consensus header from "Claude-only consensus" to reflect that Gemini did contribute. |
||
|
Johannes Millan
|
9a00152c22 |
docs(sync-core-plan): record WebDAV slice multi-review consensus
Add a "Multi-review consensus (2026-05-12)" section between the goal
and the what-moves description, mirroring the Dropbox slice doc's
structure. Captures findings from four Claude reviewers (security,
architecture, alternatives, simplicity). Codex / Copilot / Gemini CLIs
were attempted but failed for environment reasons (sandbox, deny-tool
classifier, quota+workspace limits); noted in the consensus header.
Decisions revised:
- Open question 1: DROP the new WebDavNativeHttpExecutor port. Reuse
NativeHttpExecutor — the existing port already supports responseType:
'text', maxRetries: 0, and arbitrary methods (PROPFIND/MKCOL/MOVE).
Auto-JSON-parse is a property of CapacitorHttp.request, not the port.
The app injects a different adapter (wired to WebDavHttp plugin) of
the same port. Commit 2 collapses to "wire app-side adapter."
- Open question 4: Drop inline registerPlugin in this slice — the
canonical registration in capacitor-webdav-http/index.ts carries the
web: () => import('./web') fallback; the inline one in
webdav-http-adapter.ts:31 does not.
- Open question 5: Tighten CORS heuristic in this slice. Collapse the
string-matching to a ~3-line "TypeError && message.includes('cors')"
check and replace the ambiguous-error log with toSyncLogError +
urlPathOnly meta. Closes a privacy leak (Firefox NetworkError embeds
the URL) and removes 40 lines.
- Open question 6: Preserve no-retry behavior. Under open-question-1's
port reuse, becomes a per-call-site maxRetries: 0 argument.
- Open question 8: Keep webdav-api.spec.ts monolithic. Match Dropbox
precedent (~876 lines, single file move).
- Commit shape: Match Dropbox 5a/5b split. PR 6a = log helpers
promotion; PR 6b = bulk move + adapter wiring + privacy sweep +
Nextcloud generic widen + md5HashSync → hash-wasm + CORS tighten +
spec migration.
- Factory shape: createWebdavProvider(extraPath?: string), not (deps).
Deps are composed internally from app singletons, matching the
Dropbox precedent at app-side dropbox.ts:31-43.
Decisions affirmed: md5HashSync → hash-wasm async (with one-line
benchmark in PR description), Nextcloud generic widening to union,
delete TestableWebDavHttpAdapter spec harness.
New privacy blockers surfaced (must fix in PR 6b): URL/basePath leak
via _buildFullPath at four error-construction call sites, PROPFIND
response body fed into HttpNotOkAPIError (contains user filenames),
testConnection returning raw e.message, _buildFullPath throwing
generic Error with raw path, A3 sweep undercount (10+ raw-error log
sites), new B3.4 invariant (FileMeta never enters a Log call site),
and a documented package-boundary invariant that response headers
are not logged or attached to errors.
Action items: PR 6a (helpers promotion) → PR 6b (bulk move). The
original "Open questions" section is preserved below the consensus
as a decision-log for review history, in the same style as the
Dropbox slice doc.
|
||
|
Johannes Millan
|
bd6863ac84 |
docs(sync-core-plan): draft WebDAV slice design for multi-review
Pre-implementation design doc for PR 5's WebDAV + Nextcloud slice, mirroring the structure of the Dropbox slice design doc. Captures the file-move surface (9 source files + specs), the new WebDavNativeHttpExecutor port shape (callable type, divergent from NativeHttpExecutor because of Capacitor WebDavHttp plugin transport differences), the md5HashSync → hash-wasm migration choice, the errorMeta / urlPathOnly log-helper promotion as a precursor commit, the privacy A1/A3/B3.x sweep checklist, and the Nextcloud generic- parameter / cast cleanup option. Includes eight open questions framed for multi-review (port naming, md5 strategy, generic parameter, registerPlugin cleanup scope, CORS heuristic, retry policy, spec migration, file split). Suggested three-commit shape (helpers promotion → port introduction → bulk move) keeps each commit independently green. No code changes; design doc only. |
||
|
Johannes Millan
|
91e53bb488 |
refactor(sync-providers): move provider error classes
Lift 12 provider-shared error classes (AuthFailSPError, InvalidDataSPError,
HttpNotOkAPIError, NoRevAPIError, RemoteFileNotFoundAPIError,
MissingCredentialsSPError, MissingRefreshTokenAPIError,
TooManyRequestsAPIError, UploadRevToMatchMismatchAPIError,
PotentialCorsError, RemoteFileChangedUnexpectedly, EmptyRemoteBodySPError)
plus AdditionalLogErrorBase and extractErrorMessage into
@sp/sync-providers. App-side sync-errors.ts becomes a re-export shim
so existing call sites and instanceof checks keep working.
The moved AdditionalLogErrorBase drops its constructor-time
OP_LOG_SYNC_LOGGER.log side effect (Option A from the slice design):
privacy responsibility shifts entirely to catch-site logging via the
injected SyncLogger port. A new app-side identity spec asserts the
constructor identity is preserved across import paths so future bundler
or tsconfig drift can't silently break instanceof catches.
HttpNotOkAPIError splits its parsed body excerpt off .message onto a
new opt-in .detail field; getErrorTxt forwards .detail to UI surfaces
so user-visible toasts remain unchanged while privacy-aware logger
paths see only "HTTP <status> <statusText>".
TooManyRequestsAPIError's constructor is narrowed to accept only
{ status, retryAfter?, path? } — closing a latent bearer-token leak
where Dropbox's _handleErrorResponse passed the raw Authorization
header through additionalLog. Callers in dropbox-api and
webdav-http-adapter updated accordingly.
Package gains "sideEffects": false to unlock tree-shaking through the
barrel for consumers that import only error classes.
Slice design and round-2 multi-review findings documented in
docs/plans/2026-05-12-pr5-dropbox-slice.md.
|
||
|
Johannes Millan
|
b51bd2c9ca
|
New focus mode rework (#7411)
* fix(android): avoid false WebView version lockout * fix(android): add WebView block recovery paths Builds on the prior authoritative-vs-fallback fix with three layered recovery mechanisms so users hit by a false BLOCK are never locked out of their data: - Last-known-good auto-recovery: persist the highest WebView version that has ever loaded the app on this device. A later transient mis-read that drops below MIN_CHROMIUM_VERSION is downgraded to WARN. - "Try anyway" override: third button on the block screen opens an AlertDialog with an explicit risk acknowledgment (crashes, render failures, possible data loss). Confirming persists an override and relaunches the app. Hardened against tapjacking via filterTouchesWhenObscured on both the activity and dialog window. - Override auto-clears once a healthy version is detected, so a future genuine block is not silently bypassed. Also tightens the UA regex (drops the misleading Safari Version/X fallback that always reads "4.0" and would falsely block) and adds diagnostic logging gated by Log.isLoggable for field debugging. Tests: 12 unit tests covering statusForVersion branches, all applyOverrides paths, and parseMajorVersion edge cases. Refs #7229 * fix(android): recover tracking after WebView cold start (#7390) When the WebView is killed in the background (e.g. profile switch on GrapheneOS) the JS-side state is lost on the next cold start, but the native foreground tracking service keeps accumulating elapsed time. The app previously discarded that elapsed time on cold start, leading to silent data loss for the user. Recovery flow: - syncTrackingToService$ detects "no current task + native is tracking" on the first emission after hydration and emits a recovery request. - syncOnResume$ does the same on warm resume. - processRecovery$ drains requests with exhaustMap, coalescing concurrent triggers onto a single in-flight recovery. - _doRecover syncs the native elapsed time onto the task and dispatches setCurrentId, restoring the JS-side tracking state. - The null→task re-emission in syncTrackingToService$ then calls updateTrackingService instead of startTrackingService when native is already tracking the same task, preserving the just-reconciled native counter (Kotlin's startTracking otherwise resets accumulatedMs). Supporting changes: - onResume$ is now a ReplaySubject(1) so cold-start emissions delivered before the JS subscriber attaches are still received. The 4 existing consumers are idempotent native-queue drains and verified safe. - parseNativeTrackingData extracted as a top-level pure function with shape validation; warning logs use a length-only fingerprint to avoid burning user content into the exportable log if the native contract ever changes. - Diagnostic 'source' label ('cold-start' | 'resume') in the recovery log line for field triage of any future re-reports. Tests: 12 unit tests for parseNativeTrackingData against the real production code, plus 4 helper-level tests for the null→task transition logic. The pipeline-level tests follow the file's existing pattern of re-implementing logic due to the IS_ANDROID_WEB_VIEW gate. Not addressed (out of scope, separate Kotlin work): write-side flush reliability under aggressive OS kills (flushOnPause$ may not complete before WebView termination). The recovery covers most cases by reading the native counter as the source of truth. * fix(sync): warn before destructive SYNC_IMPORT actions Previously the 'Server Already Has Data' dialog described a destructive SYNC_IMPORT as a 'merge' with a primary-colored 'Upload Local Data' button — leading users to clobber syncing devices' data. The decrypt- error 'Overwrite Remote' button had similarly understated copy and no final confirmation gate. - Rewrite D_SERVER_MIGRATION_CONFIRM body to call out 'overwrite' / 'replace' / 'other devices'; affirmative button is now 'Replace Server Data' with color=warn. - Rewrite D_DECRYPT_ERROR P3 + button label to make cross-device blast radius explicit. - Gate updatePWAndForceUpload behind a confirmDialog with a stronger warning string. - Add component spec for the migration dialog as a regression guard. * fix(infra): close db-startup race in supersync e2e stack pg_isready -U supersync without -d returned OK as soon as postgres accepted connections to the default database, but during first-run initdb the server briefly bounced while POSTGRES_DB was created. supersync's prisma db push then race-failed with P1001. - Healthcheck now runs psql -d supersync_db -c 'SELECT 1' so it only passes once the app's db is queryable. - Dockerfile.test entrypoint retries prisma db push up to 15x before giving up — defense in depth if anything else ever races. * chore(sync): instrument destructive-recovery paths for next incident Adds read-only diagnostic logs at the four sites a sync-stuck incident flows through, so the next occurrence is debuggable from a single log file without forensic recovery: - clean-slate.service: snapshot prior vector clock, count + opType breakdown of unsynced ops, syncImportReason — captured before any mutation - sync-wrapper.service: forceUpload(triggerSource) typed union stamps which error class drove the user into destructive recovery - remote-ops-processing.service: incoming full-state op shape + receiver's prior clock and unsynced-op tally about to be wiped - credential-store.service: encryptKey state on every fresh disk load, length-redacted ([length=N] / [empty]) — surfaces the isEncryptionEnabled=true + empty-key smoking-gun signature No behaviour change. Hot sync paths are untouched (full-state branch is gated; load() short-circuits on cache). Existing redaction patterns preserved — keys never logged in plaintext. * 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
|
28daf519fb |
feat(schedule): add week number and date range to nav header
Show "Week N · start – end" in week view and "Month YYYY" in month view, right-aligned in the schedule nav bar. Move the week/month view toggle out of main-header into the left of the schedule bar. Replace the "Now" text button with a circle icon next to the chevrons, and disable prev navigation when today is already in the displayed range. - ISO 8601 week numbers via existing getWeekNumber util - Parse day strings via parseDbDateStr to avoid UTC-midnight TZ skew - Reuse existing T.F.WORKLOG.CMP.WEEK_NR / T.F.TODAY_TAG_TITLE keys - Drop the duplicate month-title inside schedule-month; the shared nav now owns the label for both views |
||
|
Johannes Millan
|
9eeded0845 |
feat(onboarding): add lightweight onboarding hints
Add onboarding hint UI with translations for all 27 languages. |
||
|
Johannes Millan
|
c699fde21d | chore: remove plan docs | ||
|
Johannes Millan
|
9af096a39b | docs: add deadline day banner implementation plan | ||
|
Johannes Millan
|
0fedfab722 | docs: add deadline day banner design | ||
|
Johannes Millan
|
da63550459 |
docs: add task deadlines feature design
Design document for GitHub issue #4328. Covers data model (deadlineDay, deadlineWithTime, deadlineRemindAt), UI placement in detail panel and task list rows, NgRx actions/reducers, and reminder integration. |
||
|
Johannes Millan
|
ec847ce897 |
fix(sync): use manual code entry for Dropbox auth on all platforms
- Force dialog to show input field instead of OAuth redirect spinner - Remove platform-specific OAuth redirect URI logic - Ensures reliable copy/paste auth flow on Android, web, and Electron |
||
|
Johannes Millan
|
8931759745 | docs(supersync): add CORS wildcard support design |