From 3e58b6d8d2ccc357bfb20e8d9c3d7dac58622f73 Mon Sep 17 00:00:00 2001 From: Johannes Millan Date: Wed, 3 Jun 2026 20:14:47 +0200 Subject: [PATCH] fix(sync): skip file-based conflict dialog for example-only stores (#7985) (#7995) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(sync): skip file-based conflict dialog for example-only stores (#7985) A fresh file-based client (Dropbox/WebDAV) that seeded onboarding example tasks before its first sync hit a LocalDataConflictError dialog when adopting a populated remote, because hasMeaningfulStateData() counts any task and the isExampleTask marker lives only on the op-log op, not the NgRx Task entity. Derive the pending example-task ids (via the now-shared isExampleTaskCreateOp predicate) and let the file-based store-meaning check ignore them, so an example-only store adopts remote silently — the same smoothing #7976/#7980 gave the SuperSync path (plan §7 "Option 2" for the file-based path). A real task / non-INBOX project / non-system tag / note still reads as meaningful and shows the dialog. hasMeaningfulStateData() gains an optional ignoreTaskIds arg that only narrows the result; omitting it preserves behavior for the #7892 empty-overwrite guard, snapshot/compaction, and first-time-sync callers. Note: the SuperSync upload-pollution residual (Fix C in the #7980 plan) is NOT addressed here — a status-based upload guard is ineffective (hasSyncedOps flips during the pre-upload download) and the correct fix is identity-based cleanup, left as a scoped follow-up. * fix(sync): discard example-task ops on first adoption of populated remote (#7985) The deferred "Fix C" from #7980: a never-synced client that adopts a populated, non-encrypted SuperSync account (seeded by normal upload, so no SYNC_IMPORT fires the incoming-import gate's discard) would upload its 4 onboarding example-task ops onto that remote, propagating throwaway scaffolding to every device. In OperationLogSyncService.uploadPendingOps, when the pre-download-captured isNeverSynced is true AND hasSyncedOps() is now true (this cycle's download just adopted remote ops), reject the pending example-create ops so the upload never sends them. The pre-download capture is what makes this reliable — a live hasSyncedOps() read is useless here because the download flips it mid-cycle (#7980 §10). Stranding safety: discard only from a PRISTINE post-boot batch — example creates plus default GLOBAL_CONFIG writes, nothing else. hasMeaningfulPendingOps() is not sufficient: it ignores Move/Batch (and planner/board/section) ops, which can reference an example task id without being "meaningful". Unlike the incoming- SYNC_IMPORT discard — where SyncImportFilterService drops any concurrent dependent op against the import — this adoption path has no import to filter them, so a surviving reorder Move would strand a dangling reference once its create is rejected. Requiring an all-examples+config batch also preserves "an edited example syncs as real data": any user action (edit, reorder, real task) adds a non-config op and skips the cleanup, so everything uploads together. Scope: stops cross-device propagation (the documented harm). On SuperSync the example tasks remain in this device's local NgRx state (op-based adoption merges rather than replaces); removing them from local state is a separate increment. The file-based path already clears them via snapshot hydration. * test(sync): real-store integration proof for example-task adoption cleanup (#7985) The sync-service unit specs mock opLogStore.hasSyncedOps(), so they only prove the Fix C hook's truth-table — not the load-bearing #7980 §10 claim that the pre-upload download phase persists adopted remote ops with syncedAt (flipping hasSyncedOps() to true) while the local example creates stay unsynced. This integration test exercises the REAL OperationLogStoreService against real IndexedDB: appending a remote-sourced op genuinely flips hasSyncedOps() to true while getUnsynced() still returns the local example creates, the production "pristine post-boot batch" predicate evaluates correctly on real ops, and the real markRejected() removes only the example creates (config survives). Also covers the reorder-Move stranding guard and the empty-server-seeding no-fire case. No docker / SuperSync server needed; complements the (still-recommended) e2e. --- .../sync/operation-log-sync.service.spec.ts | 290 ++++++++++++++++++ .../op-log/sync/operation-log-sync.service.ts | 64 +++- .../sync/sync-import-conflict-gate.service.ts | 24 +- .../op-log/sync/sync-local-state.service.ts | 10 +- ...-task-adoption-cleanup.integration.spec.ts | 150 +++++++++ .../has-meaningful-state-data.util.spec.ts | 31 ++ .../has-meaningful-state-data.util.ts | 20 +- .../is-example-task-op.util.spec.ts | 65 ++++ .../validation/is-example-task-op.util.ts | 31 ++ 9 files changed, 651 insertions(+), 34 deletions(-) create mode 100644 src/app/op-log/testing/integration/example-task-adoption-cleanup.integration.spec.ts create mode 100644 src/app/op-log/validation/is-example-task-op.util.spec.ts create mode 100644 src/app/op-log/validation/is-example-task-op.util.ts diff --git a/src/app/op-log/sync/operation-log-sync.service.spec.ts b/src/app/op-log/sync/operation-log-sync.service.spec.ts index a27c52bc49..e7e6538e3e 100644 --- a/src/app/op-log/sync/operation-log-sync.service.spec.ts +++ b/src/app/op-log/sync/operation-log-sync.service.spec.ts @@ -260,6 +260,202 @@ describe('OperationLogSyncService', () => { .and.returnValue(Promise.resolve(1)); // Not fresh (has seq) }); + describe('example-task cleanup on first adoption of a populated remote (#7985)', () => { + const exampleCreate = (taskId: string): OperationLogEntry => ({ + seq: 1, + op: { + id: `ex-op-${taskId}`, + clientId: 'client-A', + actionType: ActionType.TASK_SHARED_ADD, + opType: OpType.Create, + entityType: 'TASK', + entityId: taskId, + payload: { + actionPayload: { task: { id: taskId }, isExampleTask: true }, + entityChanges: [], + }, + vectorClock: { clientA: 1 }, + timestamp: Date.now(), + schemaVersion: 1, + }, + appliedAt: Date.now(), + source: 'local', + }); + + const realTaskCreate = (taskId: string): OperationLogEntry => ({ + seq: 2, + op: { + id: `real-op-${taskId}`, + clientId: 'client-A', + actionType: '[Task] Add Task' as ActionType, + opType: OpType.Create, + entityType: 'TASK', + entityId: taskId, + payload: { actionPayload: { task: { id: taskId } }, entityChanges: [] }, + vectorClock: { clientA: 1 }, + timestamp: Date.now(), + schemaVersion: 1, + }, + appliedAt: Date.now(), + source: 'local', + }); + + const configOp = (): OperationLogEntry => ({ + seq: 3, + op: { + id: 'config-op-1', + clientId: 'client-A', + actionType: '[Global Config] Update Global Config Section' as ActionType, + opType: OpType.Update, + entityType: 'GLOBAL_CONFIG', + entityId: 'sync', + payload: { sectionKey: 'sync' }, + vectorClock: { clientA: 1 }, + timestamp: Date.now(), + schemaVersion: 1, + }, + appliedAt: Date.now(), + source: 'local', + }); + + // A reorder of an example task: a Move op (entityType TASK, entityId = the example id) + // that hasMeaningfulPendingOps() does NOT flag — would strand a dangling reference if + // the cleanup rejected the create but let this Move upload (cross-validated by review). + const exampleMove = (taskId: string): OperationLogEntry => ({ + seq: 4, + op: { + id: `mov-op-${taskId}`, + clientId: 'client-A', + actionType: '[Project] Move Task In Backlog/Today' as ActionType, + opType: OpType.Move, + entityType: 'TASK', + entityId: taskId, + payload: {}, + vectorClock: { clientA: 1 }, + timestamp: Date.now(), + schemaVersion: 1, + }, + appliedAt: Date.now(), + source: 'local', + }); + + const exampleUpdate = (taskId: string): OperationLogEntry => ({ + seq: 5, + op: { + id: `upd-op-${taskId}`, + clientId: 'client-A', + actionType: '[Task] Update Task' as ActionType, + opType: OpType.Update, + entityType: 'TASK', + entityId: taskId, + payload: { actionPayload: { task: { id: taskId } }, entityChanges: [] }, + vectorClock: { clientA: 1 }, + timestamp: Date.now(), + schemaVersion: 1, + }, + appliedAt: Date.now(), + source: 'local', + }); + + const mockProvider = { isReady: () => Promise.resolve(true) } as any; + + beforeEach(() => { + uploadServiceSpy.uploadPendingOps.and.resolveTo({ + uploadedCount: 0, + piggybackedOps: [], + rejectedCount: 0, + rejectedOps: [], + }); + opLogStoreSpy.markRejected.and.resolveTo(); + }); + + it('discards untouched example-task ops when a never-synced client adopted a populated remote', async () => { + // isNeverSynced captured pre-download = true; hasSyncedOps now true = adopted this cycle. + opLogStoreSpy.hasSyncedOps.and.resolveTo(true); + opLogStoreSpy.getUnsynced.and.resolveTo([ + exampleCreate('ex-1'), + exampleCreate('ex-2'), + ]); + + await service.uploadPendingOps(mockProvider, { isNeverSynced: true }); + + expect(opLogStoreSpy.markRejected).toHaveBeenCalledWith([ + 'ex-op-ex-1', + 'ex-op-ex-2', + ]); + }); + + it('does NOT discard when the client had already synced before this cycle (isNeverSynced false)', async () => { + opLogStoreSpy.hasSyncedOps.and.resolveTo(true); + opLogStoreSpy.getUnsynced.and.resolveTo([exampleCreate('ex-1')]); + + await service.uploadPendingOps(mockProvider, { isNeverSynced: false }); + + expect(opLogStoreSpy.markRejected).not.toHaveBeenCalled(); + }); + + it('does NOT discard when nothing was adopted this cycle (empty-server seeding, hasSyncedOps false)', async () => { + opLogStoreSpy.hasSyncedOps.and.resolveTo(false); + opLogStoreSpy.getUnsynced.and.resolveTo([exampleCreate('ex-1')]); + + await service.uploadPendingOps(mockProvider, { isNeverSynced: true }); + + expect(opLogStoreSpy.markRejected).not.toHaveBeenCalled(); + }); + + it('does NOT discard when meaningful user work is also pending (a real task) — keeps everything', async () => { + opLogStoreSpy.hasSyncedOps.and.resolveTo(true); + opLogStoreSpy.getUnsynced.and.resolveTo([ + exampleCreate('ex-1'), + realTaskCreate('real-1'), + ]); + + await service.uploadPendingOps(mockProvider, { isNeverSynced: true }); + + expect(opLogStoreSpy.markRejected).not.toHaveBeenCalled(); + }); + + it('still discards when only default config ops coexist with the example tasks', async () => { + opLogStoreSpy.hasSyncedOps.and.resolveTo(true); + opLogStoreSpy.getUnsynced.and.resolveTo([ + exampleCreate('ex-1'), + configOp(), + exampleCreate('ex-2'), + ]); + + await service.uploadPendingOps(mockProvider, { isNeverSynced: true }); + + expect(opLogStoreSpy.markRejected).toHaveBeenCalledWith([ + 'ex-op-ex-1', + 'ex-op-ex-2', + ]); + }); + + it('does NOT discard when a reorder Move of an example task is pending (would otherwise strand a dangling ref)', async () => { + opLogStoreSpy.hasSyncedOps.and.resolveTo(true); + opLogStoreSpy.getUnsynced.and.resolveTo([ + exampleCreate('ex-1'), + exampleMove('ex-1'), + ]); + + await service.uploadPendingOps(mockProvider, { isNeverSynced: true }); + + expect(opLogStoreSpy.markRejected).not.toHaveBeenCalled(); + }); + + it('does NOT discard when an example task was edited (its Update keeps the whole batch)', async () => { + opLogStoreSpy.hasSyncedOps.and.resolveTo(true); + opLogStoreSpy.getUnsynced.and.resolveTo([ + exampleCreate('ex-1'), + exampleUpdate('ex-1'), + ]); + + await service.uploadPendingOps(mockProvider, { isNeverSynced: true }); + + expect(opLogStoreSpy.markRejected).not.toHaveBeenCalled(); + }); + }); + describe('uploadPendingOps', () => { it('should return localWinOpsCreated: 0 when no piggybacked ops', async () => { opLogStoreSpy.getUnsynced.and.returnValue(Promise.resolve([])); @@ -1109,6 +1305,100 @@ describe('OperationLogSyncService', () => { ); }); + // #7985: a never-synced file-based client whose store + pending ops contain only + // onboarding example tasks must not see the spurious conflict dialog. + const exampleCreateEntry = (taskId: string): OperationLogEntry => ({ + seq: 1, + op: { + id: `ex-op-${taskId}`, + clientId: 'client-A', + actionType: ActionType.TASK_SHARED_ADD, + opType: OpType.Create, + entityType: 'TASK', + entityId: taskId, + payload: { + actionPayload: { task: { id: taskId }, isExampleTask: true }, + entityChanges: [], + }, + vectorClock: { clientA: 1 }, + timestamp: Date.now(), + schemaVersion: 1, + }, + appliedAt: Date.now(), + source: 'local', + }); + + const fileSnapshotDownloadResult = { + newOps: [], + hasMore: false, + latestSeq: 0, + needsFullStateUpload: false, + success: true, + providerMode: 'fileSnapshotOps', + failedFileCount: 0, + snapshotState: { tasks: [{ id: 'remote-task' }] }, + snapshotVectorClock: { clientB: 5 }, + latestServerSeq: 1, + }; + + it('does NOT throw LocalDataConflictError when store + pending ops contain only example tasks (#7985)', async () => { + opLogStoreSpy.getUnsynced.and.returnValue( + Promise.resolve([exampleCreateEntry('ex-task-1')]), + ); + // Store holds ONLY that example task (the marker lives on the op, not the entity). + stateSnapshotServiceSpy.getStateSnapshot.and.returnValue({ + task: { ids: ['ex-task-1'] }, + project: { ids: [INBOX_PROJECT.id] }, + tag: { ids: [TODAY_TAG.id] }, + note: { ids: [] }, + } as any); + + const syncHydrationServiceSpy = TestBed.inject( + SyncHydrationService, + ) as jasmine.SpyObj; + syncHydrationServiceSpy.hydrateFromRemoteSync.and.resolveTo(); + + downloadServiceSpy.downloadRemoteOps.and.returnValue( + Promise.resolve(fileSnapshotDownloadResult as any), + ); + + const mockProvider = { + isReady: () => Promise.resolve(true), + supportsOperationSync: true, + setLastServerSeq: jasmine.createSpy('setLastServerSeq').and.resolveTo(), + } as any; + + await expectAsync(service.downloadRemoteOps(mockProvider)).toBeResolved(); + expect(syncHydrationServiceSpy.hydrateFromRemoteSync).toHaveBeenCalled(); + }); + + it('still throws LocalDataConflictError when a real task exists alongside example tasks (#7985)', async () => { + opLogStoreSpy.getUnsynced.and.returnValue( + Promise.resolve([exampleCreateEntry('ex-task-1')]), + ); + // Store has the example task AND a real one — the real task must still be protected. + stateSnapshotServiceSpy.getStateSnapshot.and.returnValue({ + task: { ids: ['ex-task-1', 'real-task-1'] }, + project: { ids: [INBOX_PROJECT.id] }, + tag: { ids: [TODAY_TAG.id] }, + note: { ids: [] }, + } as any); + + downloadServiceSpy.downloadRemoteOps.and.returnValue( + Promise.resolve(fileSnapshotDownloadResult as any), + ); + + const mockProvider = { + isReady: () => Promise.resolve(true), + supportsOperationSync: true, + setLastServerSeq: jasmine.createSpy('setLastServerSeq').and.resolveTo(), + } as any; + + await expectAsync(service.downloadRemoteOps(mockProvider)).toBeRejectedWith( + jasmine.any(LocalDataConflictError), + ); + }); + it('should include correct context in LocalDataConflictError', async () => { const unsyncedEntries: OperationLogEntry[] = [ { diff --git a/src/app/op-log/sync/operation-log-sync.service.ts b/src/app/op-log/sync/operation-log-sync.service.ts index 49e1cbf05d..227094dd14 100644 --- a/src/app/op-log/sync/operation-log-sync.service.ts +++ b/src/app/op-log/sync/operation-log-sync.service.ts @@ -35,6 +35,7 @@ import { getDefaultMainModelData } from '../model/model-config'; import { loadAllData } from '../../root-store/meta/load-all-data.action'; import { SyncLocalStateService } from './sync-local-state.service'; import { SyncImportConflictCoordinatorService } from './sync-import-conflict-coordinator.service'; +import { isExampleTaskCreateOp } from '../validation/is-example-task-op.util'; /** * Orchestrates synchronization of the Operation Log with remote storage. @@ -197,6 +198,47 @@ export class OperationLogSyncService { return { kind: 'blocked_fresh_client' }; } + // #7985: identity-based example-task cleanup (the deferred "Fix C" of #7980). + // `isNeverSyncedAtSyncStart` is captured PRE-download; if hasSyncedOps() is now true, + // this cycle's download just ADOPTED a populated remote. A genuinely-fresh client's + // onboarding example tasks are throwaway scaffolding every install regenerates; + // uploading them onto the adopted remote pollutes it and propagates to every device + // (the residual #7980 documented for non-encrypted accounts seeded by normal upload, + // where no SYNC_IMPORT fires the incoming-import gate's discard). Reject the example + // create ops so the upload never sends them. + // + // A status read alone is unsafe (download flips hasSyncedOps mid-cycle — #7980 §10); + // the PRE-download `isNeverSynced` is what makes this reliable. + // + // Stranding safety: we discard only from a PRISTINE post-boot batch — example creates + // plus the default GLOBAL_CONFIG writes, nothing else. hasMeaningfulPendingOps() is NOT + // enough here: it ignores Move/Batch (and planner/board/section) ops, which can reference + // an example task id without being "meaningful". Unlike the incoming-SYNC_IMPORT discard + // — where SyncImportFilterService drops any concurrent dependent op against the import — + // this adoption path has no import to filter them, so a surviving Move would strand a + // dangling reference once its create is rejected. Requiring an all-examples+config batch + // also preserves the "edited example syncs as real data" property: any user action (edit, + // reorder, real task) adds a non-config op and skips the cleanup, so everything uploads. + if (isNeverSyncedAtSyncStart && (await this.opLogStore.hasSyncedOps())) { + const pendingOps = await this.opLogStore.getUnsynced(); + const isPristinePostBootBatch = pendingOps.every( + (entry) => + isExampleTaskCreateOp(entry) || entry.op.entityType === 'GLOBAL_CONFIG', + ); + if (isPristinePostBootBatch) { + const exampleOpIds = pendingOps + .filter(isExampleTaskCreateOp) + .map((entry) => entry.op.id); + if (exampleOpIds.length > 0) { + await this._discardExampleTaskOps(exampleOpIds); + OpLog.normal( + `OperationLogSyncService: Discarded ${exampleOpIds.length} untouched example-task ` + + 'op(s) — never-synced client adopted a populated remote this sync (#7985).', + ); + } + } + } + // SERVER MIGRATION CHECK: Passed as callback to execute INSIDE the upload lock. // This prevents race conditions where multiple tabs could both detect migration // and create duplicate SYNC_IMPORT operations. @@ -474,14 +516,24 @@ export class OperationLogSyncService { // SuperSync→Dropbox, only has a config-change op (not "meaningful"), but the // store is full of real data that would be overwritten by old Dropbox state. // - // Known limitation (#7985): hasMeaningfulStoreData() counts ANY task, including onboarding - // example tasks (they carry the isExampleTask marker only on their op-log ops, not - // in NgRx state). So a fresh file-based client (Dropbox/WebDAV) that created example - // tasks while sync was disabled — or before a slow first sync completed — can still - // hit this dialog. afterInitialSyncDoneStrict$ shrinks but does not close that window. + // #7985: hasMeaningfulStoreData() counts ANY task, including onboarding example + // tasks (they carry the isExampleTask marker only on their op-log ops, not in NgRx + // state). Derive the example task ids from the pending example-create ops and let + // the store check ignore them, so a fresh file-based client (Dropbox/WebDAV) that + // only has example tasks adopts remote silently instead of hitting the spurious + // conflict dialog #7976/#7980 removed for the SuperSync path. Scope: this fires only + // while the example create ops are still pending (a never-synced file client) — + // exactly the reachable scenario. A real (non-example) task / non-INBOX project / + // non-system tag / note still reads as meaningful and shows the dialog. + const exampleTaskIds = new Set( + unsyncedOps + .filter(isExampleTaskCreateOp) + .map((entry) => entry.op.entityId) + .filter((id): id is string => id !== undefined), + ); const hasMeaningfulUserData = this.syncImportConflictGateService.hasMeaningfulPendingOps(unsyncedOps) || - this.syncLocalStateService.hasMeaningfulStoreData(); + this.syncLocalStateService.hasMeaningfulStoreData(exampleTaskIds); if (hasMeaningfulUserData) { // Client has meaningful user data - show conflict dialog diff --git a/src/app/op-log/sync/sync-import-conflict-gate.service.ts b/src/app/op-log/sync/sync-import-conflict-gate.service.ts index b20db27daf..b8013aee3a 100644 --- a/src/app/op-log/sync/sync-import-conflict-gate.service.ts +++ b/src/app/op-log/sync/sync-import-conflict-gate.service.ts @@ -1,8 +1,6 @@ import { inject, Injectable } from '@angular/core'; import { OperationLogStoreService } from '../persistence/operation-log-store.service'; import { - ActionType, - extractActionPayload, FULL_STATE_OP_TYPES, Operation, OperationLogEntry, @@ -10,30 +8,10 @@ import { } from '../core/operation.types'; import { OperationWriteFlushService } from './operation-write-flush.service'; import { SyncImportConflictData } from './dialog-sync-import-conflict/dialog-sync-import-conflict.component'; +import { isExampleTaskCreateOp } from '../validation/is-example-task-op.util'; const USER_ENTITY_TYPES = new Set(['TASK', 'PROJECT', 'TAG', 'NOTE']); -/** - * Startup example/onboarding tasks are generated locally on first run (see - * ExampleTasksService) and must not count as "meaningful local work" that would block - * an incoming SYNC_IMPORT. This only ever runs against local pending ops from - * getUnsynced() — never against incoming remote ops — so a remote-supplied - * `isExampleTask` flag cannot be used to bypass the conflict dialog. - */ -const isExampleTaskCreateOp = (entry: OperationLogEntry): boolean => { - const { op } = entry; - if ( - op.actionType !== ActionType.TASK_SHARED_ADD || - op.opType !== OpType.Create || - op.entityType !== 'TASK' - ) { - return false; - } - - const actionPayload = extractActionPayload(op.payload); - return actionPayload['isExampleTask'] === true; -}; - export interface IncomingFullStateConflictGateResult { fullStateOp?: Operation; pendingOps: OperationLogEntry[]; diff --git a/src/app/op-log/sync/sync-local-state.service.ts b/src/app/op-log/sync/sync-local-state.service.ts index 7e4afd4205..7057e3c81f 100644 --- a/src/app/op-log/sync/sync-local-state.service.ts +++ b/src/app/op-log/sync/sync-local-state.service.ts @@ -22,7 +22,13 @@ export class SyncLocalStateService { return !snapshot && lastSeq === 0; } - hasMeaningfulStoreData(): boolean { + /** + * @param ignoreTaskIds Optional task ids to exclude from the "has a task?" check. + * The file-based conflict gate passes the ids of pending onboarding example tasks so + * an example-only store is not treated as meaningful (#7985). Omitting it preserves + * the original behavior for every other caller. + */ + hasMeaningfulStoreData(ignoreTaskIds?: ReadonlySet): boolean { const snapshot = this.stateSnapshotService.getStateSnapshot(); if (!snapshot) { @@ -32,7 +38,7 @@ export class SyncLocalStateService { return false; } - return hasMeaningfulStateData(snapshot); + return hasMeaningfulStateData(snapshot, ignoreTaskIds); } confirmFreshClientSync(opCount: number): boolean { diff --git a/src/app/op-log/testing/integration/example-task-adoption-cleanup.integration.spec.ts b/src/app/op-log/testing/integration/example-task-adoption-cleanup.integration.spec.ts new file mode 100644 index 0000000000..4bf7531294 --- /dev/null +++ b/src/app/op-log/testing/integration/example-task-adoption-cleanup.integration.spec.ts @@ -0,0 +1,150 @@ +/* eslint-disable @typescript-eslint/naming-convention */ +import { TestBed } from '@angular/core/testing'; +import { OperationLogStoreService } from '../../persistence/operation-log-store.service'; +import { ActionType, OperationLogEntry, OpType } from '../../core/operation.types'; +import { isExampleTaskCreateOp } from '../../validation/is-example-task-op.util'; +import { resetTestUuidCounter, TestClient } from './helpers/test-client.helper'; + +/** + * Integration proof for the #7985 "Fix C" cleanup (the hook in + * OperationLogSyncService.uploadPendingOps that rejects example-task ops when a + * never-synced client adopts a populated remote). + * + * The sync-service unit specs MOCK opLogStore.hasSyncedOps(), so they can only prove the + * hook's truth-table — not the load-bearing #7980 §10 claim that the (pre-upload) download + * phase persists adopted remote ops with `syncedAt`, flipping hasSyncedOps() to true BEFORE + * the upload runs while the local example creates stay unsynced. These tests exercise the + * REAL store so that exact sequencing — and the real getUnsynced()/markRejected() the hook + * composes, plus the production "pristine post-boot batch" predicate against real ops — is + * verified end-to-end (no docker / SuperSync server needed). + */ +describe('Example-task adoption cleanup mechanic (integration)', () => { + let store: OperationLogStoreService; + const local = new TestClient('client-local'); + const remote = new TestClient('client-remote'); + + // Mirrors the production gate in operation-log-sync.service.ts uploadPendingOps. + const isPristinePostBootBatch = (ops: OperationLogEntry[]): boolean => + ops.every( + (entry) => isExampleTaskCreateOp(entry) || entry.op.entityType === 'GLOBAL_CONFIG', + ); + + const exampleCreate = (taskId: string): ReturnType => + local.createOperation({ + actionType: ActionType.TASK_SHARED_ADD, + opType: OpType.Create, + entityType: 'TASK', + entityId: taskId, + payload: { + actionPayload: { task: { id: taskId }, isExampleTask: true }, + entityChanges: [], + }, + }); + + const configOp = (): ReturnType => + local.createOperation({ + actionType: ActionType.GLOBAL_CONFIG_UPDATE_SECTION, + opType: OpType.Update, + entityType: 'GLOBAL_CONFIG', + entityId: 'sync', + payload: { sectionKey: 'sync' }, + }); + + // A real op adopted FROM a populated remote (e.g. a task from the account being joined). + const adoptedRemoteOp = (): ReturnType => + remote.createOperation({ + actionType: '[Task] Update Task' as ActionType, + opType: OpType.Update, + entityType: 'TASK', + entityId: 'remote-task-1', + payload: { actionPayload: { task: { id: 'remote-task-1' } }, entityChanges: [] }, + }); + + beforeEach(async () => { + TestBed.configureTestingModule({ providers: [OperationLogStoreService] }); + store = TestBed.inject(OperationLogStoreService); + await store.init(); + await store._clearAllDataForTesting(); + resetTestUuidCounter(); + }); + + it('a never-synced client: hasSyncedOps() is false with only local example+config ops pending', async () => { + await store.append(exampleCreate('ex-1'), 'local'); + await store.append(exampleCreate('ex-2'), 'local'); + await store.append(configOp(), 'local'); + + // Pre-download: nothing synced. (This is what sync-wrapper captures as isNeverSynced.) + expect(await store.hasSyncedOps()).toBe(false); + }); + + it('adopting a populated remote really flips hasSyncedOps() while example creates stay unsynced, and the cleanup rejects only the examples', async () => { + const ex1 = exampleCreate('ex-1'); + const ex2 = exampleCreate('ex-2'); + const cfg = configOp(); + await store.append(ex1, 'local'); + await store.append(ex2, 'local'); + await store.append(cfg, 'local'); + + // Simulate the download phase adopting the populated remote: a remote op is persisted + // with syncedAt set. THIS is the #7980 §10 sequencing the unit tests mock. + await store.append(adoptedRemoteOp(), 'remote'); + + // The live read the hook makes at upload time is now true — purely from the download. + expect(await store.hasSyncedOps()).toBe(true); + + // The example creates are still pending (local, unsynced); the remote op is not pending. + const pending = await store.getUnsynced(); + const pendingIds = pending.map((e) => e.op.id); + expect(pendingIds).toContain(ex1.id); + expect(pendingIds).toContain(ex2.id); + expect(pendingIds).toContain(cfg.id); + expect(pendingIds.length).toBe(3); + + // Production gate evaluates true against the real pending batch. + expect(isPristinePostBootBatch(pending)).toBe(true); + + // The hook's discard (real markRejected) removes ONLY the example creates. + const exampleOpIds = pending.filter(isExampleTaskCreateOp).map((e) => e.op.id); + await store.markRejected(exampleOpIds); + + const after = await store.getUnsynced(); + const afterIds = after.map((e) => e.op.id); + expect(afterIds).not.toContain(ex1.id); + expect(afterIds).not.toContain(ex2.id); + // The config op survives and would still upload (not throwaway scaffolding). + expect(afterIds).toContain(cfg.id); + }); + + it('a pending reorder Move of an example task breaks the pristine-batch gate (cleanup correctly skips → no stranding)', async () => { + await store.append(exampleCreate('ex-1'), 'local'); + // A reorder Move (entityType TASK, OpType.Move) — NOT flagged by hasMeaningfulPendingOps, + // but it references the example task id, so rejecting the create would strand it. + await store.append( + local.createOperation({ + actionType: '[Project] Move Task in Today' as ActionType, + opType: OpType.Move, + entityType: 'TASK', + entityId: 'ex-1', + payload: {}, + }), + 'local', + ); + await store.append(adoptedRemoteOp(), 'remote'); + + expect(await store.hasSyncedOps()).toBe(true); + const pending = await store.getUnsynced(); + // The gate is false → the production hook skips the discard, so nothing is rejected and + // the example create uploads alongside its Move (no dangling reference on the server). + expect(isPristinePostBootBatch(pending)).toBe(false); + }); + + it('empty-server seeding: with nothing adopted, hasSyncedOps() stays false (cleanup does not fire)', async () => { + await store.append(exampleCreate('ex-1'), 'local'); + await store.append(configOp(), 'local'); + + // No remote op appended (download returned no_new_ops on an empty server). + expect(await store.hasSyncedOps()).toBe(false); + // isNeverSynced && hasSyncedOps() === true is the hook's trigger; the second term is + // false here, so a first device seeding an empty server keeps its example ops. + }); +}); diff --git a/src/app/op-log/validation/has-meaningful-state-data.util.spec.ts b/src/app/op-log/validation/has-meaningful-state-data.util.spec.ts index 30df3e834f..71d5d583db 100644 --- a/src/app/op-log/validation/has-meaningful-state-data.util.spec.ts +++ b/src/app/op-log/validation/has-meaningful-state-data.util.spec.ts @@ -66,4 +66,35 @@ describe('hasMeaningfulStateData', () => { false, ); }); + + describe('with ignoreTaskIds (#7985)', () => { + it('returns false when the only tasks are in the ignore set (example-only store)', () => { + const s = initialState(); + s.task = { ids: ['ex1', 'ex2'], entities: {} }; + expect(hasMeaningfulStateData(s, new Set(['ex1', 'ex2']))).toBe(false); + }); + + it('returns true when an unignored real task remains', () => { + const s = initialState(); + s.task = { ids: ['ex1', 'real1'], entities: {} }; + expect(hasMeaningfulStateData(s, new Set(['ex1']))).toBe(true); + }); + + it('still returns true for a non-INBOX project even if all tasks are ignored', () => { + const s = initialState(); + s.task = { ids: ['ex1'], entities: {} }; + s.project = { ids: [INBOX_PROJECT.id, 'p1'], entities: {} }; + expect(hasMeaningfulStateData(s, new Set(['ex1']))).toBe(true); + }); + + // Locks the #7892 empty-overwrite guard / snapshot / compaction callers: passing no + // ignore set (or an empty one) must behave exactly as before. + it('behaves identically to the no-arg call when ignoreTaskIds is undefined or empty', () => { + const s = initialState(); + s.task = { ids: ['t1'], entities: {} }; + expect(hasMeaningfulStateData(s)).toBe(true); + expect(hasMeaningfulStateData(s, undefined)).toBe(true); + expect(hasMeaningfulStateData(s, new Set())).toBe(true); + }); + }); }); diff --git a/src/app/op-log/validation/has-meaningful-state-data.util.ts b/src/app/op-log/validation/has-meaningful-state-data.util.ts index 46c614ed92..2ff58eb128 100644 --- a/src/app/op-log/validation/has-meaningful-state-data.util.ts +++ b/src/app/op-log/validation/has-meaningful-state-data.util.ts @@ -27,15 +27,29 @@ const isEntityState = (obj: unknown): obj is { ids: string[] } => * * Accepts an arbitrary object so callers can pass an NgRx snapshot, a loaded * state cache, or a remote payload without type juggling. + * + * `ignoreTaskIds` (optional) lets a caller exclude specific task ids from the "has a + * task?" check — used by the file-based sync conflict gate to treat a store containing + * only onboarding example tasks as non-meaningful (#7985). It only ever NARROWS the + * result; omitting it preserves the original behavior for every other caller (the + * #7892 empty-overwrite guard, snapshot/compaction, first-time-sync detection). */ -export const hasMeaningfulStateData = (state: unknown): boolean => { +export const hasMeaningfulStateData = ( + state: unknown, + ignoreTaskIds?: ReadonlySet, +): boolean => { if (!state || typeof state !== 'object') { return false; } const s = state as Record; - if (isEntityState(s.task) && s.task.ids.length > 0) { - return true; + if (isEntityState(s.task)) { + const meaningfulTaskIds = ignoreTaskIds + ? s.task.ids.filter((id) => !ignoreTaskIds.has(id)) + : s.task.ids; + if (meaningfulTaskIds.length > 0) { + return true; + } } if (isEntityState(s.project) && s.project.ids.some((id) => id !== INBOX_PROJECT.id)) { diff --git a/src/app/op-log/validation/is-example-task-op.util.spec.ts b/src/app/op-log/validation/is-example-task-op.util.spec.ts new file mode 100644 index 0000000000..56cddc018e --- /dev/null +++ b/src/app/op-log/validation/is-example-task-op.util.spec.ts @@ -0,0 +1,65 @@ +import { isExampleTaskCreateOp } from './is-example-task-op.util'; +import { ActionType, OperationLogEntry, OpType } from '../core/operation.types'; + +const entry = (over: { + actionType?: ActionType | string; + opType?: OpType; + entityType?: string; + payload?: unknown; +}): OperationLogEntry => + ({ + seq: 1, + op: { + id: 'op-1', + clientId: 'client-1', + actionType: (over.actionType ?? ActionType.TASK_SHARED_ADD) as ActionType, + opType: over.opType ?? OpType.Create, + entityType: over.entityType ?? 'TASK', + entityId: 'task-1', + payload: over.payload ?? { + actionPayload: { task: { id: 'task-1' }, isExampleTask: true }, + entityChanges: [], + }, + vectorClock: { client1: 1 }, + timestamp: 1, + schemaVersion: 1, + }, + appliedAt: 1, + source: 'local', + }) as OperationLogEntry; + +describe('isExampleTaskCreateOp', () => { + it('returns true for a TASK_SHARED_ADD Create op carrying the isExampleTask marker', () => { + expect(isExampleTaskCreateOp(entry({}))).toBe(true); + }); + + it('returns false for a task create without the marker', () => { + expect( + isExampleTaskCreateOp( + entry({ + payload: { actionPayload: { task: { id: 'task-1' } }, entityChanges: [] }, + }), + ), + ).toBe(false); + }); + + it('returns false for a non-TASK_SHARED_ADD action even with the marker', () => { + expect(isExampleTaskCreateOp(entry({ actionType: '[Task] Add Task' }))).toBe(false); + }); + + it('returns false for an Update op even with the marker', () => { + expect(isExampleTaskCreateOp(entry({ opType: OpType.Update }))).toBe(false); + }); + + it('returns false for a Delete op even with the marker', () => { + expect(isExampleTaskCreateOp(entry({ opType: OpType.Delete }))).toBe(false); + }); + + it('returns false for a non-TASK entity type', () => { + expect(isExampleTaskCreateOp(entry({ entityType: 'PROJECT' }))).toBe(false); + }); + + it('does not throw on an empty payload (treats it as non-example)', () => { + expect(isExampleTaskCreateOp(entry({ payload: {} }))).toBe(false); + }); +}); diff --git a/src/app/op-log/validation/is-example-task-op.util.ts b/src/app/op-log/validation/is-example-task-op.util.ts new file mode 100644 index 0000000000..54188957b2 --- /dev/null +++ b/src/app/op-log/validation/is-example-task-op.util.ts @@ -0,0 +1,31 @@ +import { + ActionType, + extractActionPayload, + OperationLogEntry, + OpType, +} from '../core/operation.types'; + +/** + * Startup example/onboarding tasks are generated locally on first run (see + * ExampleTasksService) and carry an `isExampleTask: true` marker on their op-log op + * payload only — never on the NgRx Task entity. They must not count as "meaningful + * local work" that would block an incoming SYNC_IMPORT or a file-based snapshot adopt + * (see #7985 / #7976 / #7980). + * + * This predicate is only ever evaluated against LOCAL pending ops from getUnsynced() — + * never against incoming remote ops — so a remote-supplied `isExampleTask` flag can + * never be used to bypass a conflict dialog. + */ +export const isExampleTaskCreateOp = (entry: OperationLogEntry): boolean => { + const { op } = entry; + if ( + op.actionType !== ActionType.TASK_SHARED_ADD || + op.opType !== OpType.Create || + op.entityType !== 'TASK' + ) { + return false; + } + + const actionPayload = extractActionPayload(op.payload); + return actionPayload['isExampleTask'] === true; +};