diff --git a/e2e/pages/supersync.page.ts b/e2e/pages/supersync.page.ts index c10665a07f..5ec78b99d9 100644 --- a/e2e/pages/supersync.page.ts +++ b/e2e/pages/supersync.page.ts @@ -2031,15 +2031,6 @@ export class SuperSyncPage extends BasePage { * @param newPassword - The new encryption password */ async changeEncryptionPassword(newPassword: string): Promise { - // Capture the sync check icon state BEFORE we start. If a previous - // syncAndWait() left the check icon visible, the final wait at the end of - // this method would see a stale icon and return before the server wipe + - // re-upload completes — causing a race where the next client starts - // syncing against partially-uploaded server state. - const checkVisibleBeforeOperation = await this.syncCheckIcon - .isVisible() - .catch(() => false); - // Open sync settings via right-click // Use noWaitAfter to prevent blocking on Angular hash navigation await this.syncBtn.click({ button: 'right', noWaitAfter: true }); @@ -2134,10 +2125,27 @@ export class SuperSyncPage extends BasePage { await expect(confirmBtn).toBeEnabled({ timeout: 1000 }); }).toPass({ timeout: 10000 }); + // The password-change service performs a clean-slate snapshot upload rather + // than a regular sync cycle, so the toolbar sync icon is not a reliable + // completion signal. Observe the authoritative server response instead. + const snapshotUploadResponse = this.page.waitForResponse( + (response) => + response.request().method() === 'POST' && + response.url().includes('/api/sync/snapshot'), + { timeout: 60000 }, + ); await confirmBtn.click(); - // Wait for the dialog to close (password change complete) - await changePasswordDialog.waitFor({ state: 'detached', timeout: 60000 }); + const [snapshotResponse] = await Promise.all([ + snapshotUploadResponse, + changePasswordDialog.waitFor({ state: 'detached', timeout: 60000 }), + ]); + if (!snapshotResponse.ok()) { + throw new Error( + `Password-change snapshot upload failed with ${snapshotResponse.status()}`, + ); + } + this._encryptionPassword = newPassword; // Wait for the config dialog to close as well await this.page.waitForTimeout(500); @@ -2153,33 +2161,6 @@ export class SuperSyncPage extends BasePage { await configDialog.waitFor({ state: 'hidden', timeout: 5000 }); } } - - // Wait for password change operation to complete (server wipe + re-upload). - // - // If the check icon was visible BEFORE we opened the settings dialog, it's - // stale from a previous sync — we must first wait for it to disappear (new - // sync cycle started) or the spinner to appear, before waiting for the - // check icon to reappear (new sync completed). Without this, we'd return - // immediately against a stale icon and race the server re-upload. - if (checkVisibleBeforeOperation) { - await Promise.race([ - this.syncCheckIcon.waitFor({ state: 'hidden', timeout: 5000 }), - this.syncSpinner.waitFor({ state: 'visible', timeout: 5000 }), - ]).catch(() => { - // Neither happened within 5s — the password change may not have - // triggered a re-sync (rare). Fall through and rely on the final - // check-icon wait below. - }); - } - - const spinnerVisible = await this.syncSpinner - .waitFor({ state: 'visible', timeout: 5000 }) - .then(() => true) - .catch(() => false); - if (spinnerVisible) { - await this.syncSpinner.waitFor({ state: 'hidden', timeout: 30000 }); - } - await this.syncCheckIcon.waitFor({ state: 'visible', timeout: 10000 }); } /** diff --git a/e2e/tests/sync/supersync-time-tracking-advanced.spec.ts b/e2e/tests/sync/supersync-time-tracking-advanced.spec.ts index 4b8b99447c..a45571219c 100644 --- a/e2e/tests/sync/supersync-time-tracking-advanced.spec.ts +++ b/e2e/tests/sync/supersync-time-tracking-advanced.spec.ts @@ -8,12 +8,121 @@ import { startTimeTracking, stopTimeTracking, waitForTaskTimeSpent, + getTaskTimeSpentFromState, markTaskDone, type SimulatedE2EClient, } from '../../utils/supersync-helpers'; import { expectTaskVisible } from '../../utils/supersync-assertions'; import { waitForAppReady } from '../../utils/waits'; +const isRecord = (value: unknown): value is Record => + typeof value === 'object' && value !== null; + +/** + * Record an exact local time delta through the same two actions used by the + * production timer: one updates local state, the other writes the replayable op. + */ +const recordTaskTimeDelta = async ( + client: SimulatedE2EClient, + taskName: string, + date: string, + duration: number, +): Promise => { + await client.page.evaluate( + async ({ name, taskDate, delta }) => { + const isRecordInPage = (value: unknown): value is Record => + typeof value === 'object' && value !== null; + + type StoreSubscription = { unsubscribe: () => void }; + type StoreLike = { + subscribe: (next: (state: unknown) => void) => StoreSubscription; + dispatch: (action: unknown) => void; + }; + + const store = ( + window as unknown as { + __e2eTestHelpers?: { store?: StoreLike }; + } + ).__e2eTestHelpers?.store; + + if (!store) { + throw new Error('E2E store helper is unavailable'); + } + + const rootState = await new Promise>((resolve, reject) => { + const subscriptionRef: { current?: StoreSubscription } = {}; + let isDone = false; + const timeoutId = window.setTimeout(() => { + if (!isDone) { + isDone = true; + subscriptionRef.current?.unsubscribe(); + reject(new Error('Timed out reading the NgRx state')); + } + }, 1000); + + subscriptionRef.current = store.subscribe((state) => { + if (isDone || !isRecordInPage(state)) { + return; + } + isDone = true; + window.clearTimeout(timeoutId); + window.setTimeout(() => subscriptionRef.current?.unsubscribe()); + resolve(state); + }); + }); + + const taskState = rootState.tasks ?? rootState.task; + if (!isRecordInPage(taskState) || !isRecordInPage(taskState.entities)) { + throw new Error('Task state is unavailable'); + } + + const task = Object.values(taskState.entities).find( + (value) => + isRecordInPage(value) && + typeof value.title === 'string' && + value.title.includes(name), + ); + if (!isRecordInPage(task) || typeof task.id !== 'string') { + throw new Error(`Task not found: ${name}`); + } + + store.dispatch({ + type: '[TimeTracking] Add time spent', + task, + date: taskDate, + duration: delta, + isFromTrackingReminder: false, + }); + store.dispatch({ + type: '[TimeTracking] Sync time spent', + taskId: task.id, + date: taskDate, + duration: delta, + meta: { + isPersistent: true, + entityType: 'TASK', + entityId: task.id, + opType: 'UPD', + }, + }); + }, + { name: taskName, taskDate: date, delta: duration }, + ); +}; + +const expectExactTaskTime = async ( + client: SimulatedE2EClient, + taskName: string, + expectedTimeSpent: number, +): Promise => { + await expect + .poll(() => getTaskTimeSpentFromState(client, taskName), { + timeout: 30000, + intervals: [250, 500, 1000], + }) + .toBe(expectedTimeSpent); +}; + /** * SuperSync Time Tracking Advanced E2E Tests * @@ -200,97 +309,107 @@ test.describe('@supersync Time Tracking Advanced Sync', () => { } }); - /** - * Test: Concurrent time tracking resolves via LWW - * - * Actions: - * 1. Both clients have same task - * 2. Client A tracks 3 seconds, stops, syncs - * 3. Client B tracks 5 seconds concurrently (started before A sync), stops, syncs - * 4. Verify final time is consistent (LWW - later sync wins) - */ - test('Concurrent time tracking resolves consistently', async ({ + test('Concurrent task-time deltas survive snapshot hydration and restart', async ({ browser, baseURL, testRunId, }) => { + test.setTimeout(240000); + + const initialTime = 10000; + const clientADelta = 3000; + const clientBDelta = 5000; + const expectedTime = initialTime + clientADelta + clientBDelta; + const snapshotPassword = 'e2e-time-snapshot-pw'; + const taskDate = '2026-07-13'; const uniqueId = Date.now(); let clientA: SimulatedE2EClient | null = null; let clientB: SimulatedE2EClient | null = null; + let clientC: SimulatedE2EClient | null = null; try { const user = await createTestUser(testRunId); const syncConfig = getSuperSyncConfig(user); - // ============ PHASE 1: Setup Both Clients ============ clientA = await createSimulatedClient(browser, baseURL!, 'A', testRunId); await clientA.sync.setupSuperSync(syncConfig); const taskName = `ConcurrentTime-${uniqueId}`; await clientA.workView.addTask(taskName); - await clientA.sync.syncAndWait(); + await waitForTask(clientA.page, taskName); + await recordTaskTimeDelta(clientA, taskName, taskDate, initialTime); + await expectExactTaskTime(clientA, taskName, initialTime); + + // Password change uses the production clean-slate path: it replaces the + // server with a full-state snapshot. The initial time is therefore in the + // snapshot, while the two concurrent contributions below remain tail ops. + await clientA.sync.changeEncryptionPassword(snapshotPassword); + await expectExactTaskTime(clientA, taskName, initialTime); + const snapshotSyncConfig = { ...syncConfig, password: snapshotPassword }; clientB = await createSimulatedClient(browser, baseURL!, 'B', testRunId); - await clientB.sync.setupSuperSync(syncConfig); + await clientB.sync.setupSuperSync(snapshotSyncConfig); await clientB.sync.syncAndWait(); - await waitForTask(clientA.page, taskName); await waitForTask(clientB.page, taskName); - console.log('[Concurrent Time Test] Both clients have task'); + await expectExactTaskTime(clientB, taskName, initialTime); - // ============ PHASE 2: Client A Tracks Time ============ - await startTimeTracking(clientA, taskName); - console.log('[Concurrent Time Test] Client A started tracking'); + // Both clients record against the same base before either sees the other delta. + await recordTaskTimeDelta(clientA, taskName, taskDate, clientADelta); + await recordTaskTimeDelta(clientB, taskName, taskDate, clientBDelta); + await expectExactTaskTime(clientA, taskName, initialTime + clientADelta); + await expectExactTaskTime(clientB, taskName, initialTime + clientBDelta); - await clientA.page.waitForTimeout(3000); - - await stopTimeTracking(clientA, taskName); - console.log('[Concurrent Time Test] Client A stopped after 3s'); - - // ============ PHASE 3: Client B Tracks Time (Concurrent) ============ - await startTimeTracking(clientB, taskName); - console.log('[Concurrent Time Test] Client B started tracking'); - - await clientB.page.waitForTimeout(2000); // Reduced from 5000ms - - await stopTimeTracking(clientB, taskName); - console.log('[Concurrent Time Test] Client B stopped after 2s'); - - // ============ PHASE 4: Sync Both ============ await clientA.sync.syncAndWait(); await clientB.sync.syncAndWait(); - await clientA.sync.syncAndWait(); // Converge - console.log('[Concurrent Time Test] All synced'); + await clientA.sync.syncAndWait(); + await clientB.sync.syncAndWait(); + await expectExactTaskTime(clientA, taskName, expectedTime); + await expectExactTaskTime(clientB, taskName, expectedTime); - // ============ PHASE 5: Verify Consistent State ============ - // Reload to ensure UI reflects final state (use goto instead of reload for reliability) - await clientA.page.goto(clientA.page.url(), { - waitUntil: 'domcontentloaded', - timeout: 30000, - }); - await waitForAppReady(clientA.page); - await waitForTask(clientA.page, taskName); + // C is a fresh database. Requiring the snapshot vector clock proves its + // first hydration used the server's latest snapshot boundary plus tail ops. + clientC = await createSimulatedClient(browser, baseURL!, 'C', testRunId); + const snapshotHydrationResponse = clientC.page.waitForResponse( + async (response) => { + if ( + response.request().method() !== 'GET' || + !response.url().includes('/api/sync/ops') + ) { + return false; + } - await clientB.page.goto(clientB.page.url(), { - waitUntil: 'domcontentloaded', - timeout: 30000, - }); - await waitForAppReady(clientB.page); - await waitForTask(clientB.page, taskName); + try { + const responseBody: unknown = await response.json(); + return isRecord(responseBody) && isRecord(responseBody.snapshotVectorClock); + } catch { + return false; + } + }, + { timeout: 60000 }, + ); + await clientC.sync.setupSuperSync(snapshotSyncConfig); + await clientC.sync.syncAndWait(); + await snapshotHydrationResponse; + await waitForTask(clientC.page, taskName); + await expectExactTaskTime(clientC, taskName, expectedTime); - const timeA = await waitForTaskTimeSpent(clientA, taskName, 10000); - const timeB = await waitForTaskTimeSpent(clientB, taskName, 10000); + const clients = [clientA, clientB, clientC]; + for (const client of clients) { + await client.page.reload({ waitUntil: 'domcontentloaded', timeout: 30000 }); + await waitForAppReady(client.page); + await waitForTask(client.page, taskName); + await expectExactTaskTime(client, taskName, expectedTime); + } - console.log(`[Concurrent Time Test] Client A final time: ${timeA}ms`); - console.log(`[Concurrent Time Test] Client B final time: ${timeB}ms`); - - // Times should match (LWW resolution) - expect(timeA).toBe(timeB); - - console.log('[Concurrent Time Test] Time tracking resolved consistently'); + for (const client of clients) { + await client.sync.syncAndWait(); + await expectExactTaskTime(client, taskName, expectedTime); + } } finally { if (clientA) await closeClient(clientA); if (clientB) await closeClient(clientB); + if (clientC) await closeClient(clientC); } }); }); diff --git a/packages/super-sync-server/src/sync/conflict.ts b/packages/super-sync-server/src/sync/conflict.ts index 488837f8e2..53b692c048 100644 --- a/packages/super-sync-server/src/sync/conflict.ts +++ b/packages/super-sync-server/src/sync/conflict.ts @@ -15,6 +15,8 @@ import { limitVectorClockSize, } from './sync.types'; +const TASK_TIME_DELTA_ACTION_TYPE = '[TimeTracking] Sync time spent'; + /** * Check if an incoming operation conflicts with existing operations. * Returns conflict info if a concurrent modification is detected. @@ -94,6 +96,7 @@ export const detectConflictForEntities = async ( SELECT DISTINCT ON (eid) eid AS "entityId", o.client_id AS "clientId", + o.action_type AS "actionType", o.vector_clock AS "vectorClock" FROM operations o CROSS JOIN LATERAL unnest( @@ -128,7 +131,7 @@ export const detectConflictForEntities = async ( export const resolveConflictForExistingOp = ( op: Operation, entityId: string, - existingOp: { clientId: string; vectorClock: unknown }, + existingOp: { actionType?: string; clientId: string; vectorClock: unknown }, ): ConflictResult => { // Stored JSON/vector_clock values arrive as unknown from both Prisma model // reads and raw SQL rows; cast only at the vector-clock comparison boundary. @@ -137,6 +140,19 @@ export const resolveConflictForExistingOp = ( // Compare vector clocks const comparison = compareVectorClocks(op.vectorClock, existingClock); + // Timer batches are additive and uniquely identified operations. Concurrent + // deltas commute, so entity-level LWW must not discard either contribution. + // Keep the causal checks below for EQUAL/LESS_THAN clocks: those operations + // may already be represented by the stored state and replaying them could + // double-count time. + if ( + comparison === 'CONCURRENT' && + op.actionType === TASK_TIME_DELTA_ACTION_TYPE && + existingOp.actionType === TASK_TIME_DELTA_ACTION_TYPE + ) { + return { hasConflict: false }; + } + // If the incoming op's clock is GREATER_THAN existing, it's a valid successor if (comparison === 'GREATER_THAN') { return { hasConflict: false }; @@ -219,7 +235,7 @@ export const detectConflictForEntity = async ( entityType: op.entityType, OR: [{ entityId }, { entityIds: { has: entityId } }], }, - select: { clientId: true, vectorClock: true, serverSeq: true }, + select: { actionType: true, clientId: true, vectorClock: true, serverSeq: true }, orderBy: { serverSeq: 'desc' }, }); @@ -486,6 +502,7 @@ export const prefetchLatestEntityOpsForBatch = async ( o.entity_type AS "entityType", eid AS "entityId", o.client_id AS "clientId", + o.action_type AS "actionType", o.vector_clock AS "vectorClock", o.server_seq AS "serverSeq" FROM operations o @@ -521,7 +538,7 @@ export const prefetchLatestEntityOpsForBatch = async ( entityId: 'misc', schemaVersion: { lt: CURRENT_SCHEMA_VERSION }, }, - select: { clientId: true, vectorClock: true, serverSeq: true }, + select: { actionType: true, clientId: true, vectorClock: true, serverSeq: true }, orderBy: { serverSeq: 'desc' }, }); const tasksKey = getEntityConflictKey('GLOBAL_CONFIG', 'tasks'); diff --git a/packages/super-sync-server/src/sync/services/operation-upload.service.ts b/packages/super-sync-server/src/sync/services/operation-upload.service.ts index 5d923b1784..34838c5540 100644 --- a/packages/super-sync-server/src/sync/services/operation-upload.service.ts +++ b/packages/super-sync-server/src/sync/services/operation-upload.service.ts @@ -558,6 +558,7 @@ export class OperationUploadService { entityType: op.entityType, entityId, clientId: op.clientId, + actionType: op.actionType, vectorClock: op.vectorClock, }); } diff --git a/packages/super-sync-server/src/sync/services/validation.service.ts b/packages/super-sync-server/src/sync/services/validation.service.ts index c100623f82..8bf4761c9e 100644 --- a/packages/super-sync-server/src/sync/services/validation.service.ts +++ b/packages/super-sync-server/src/sync/services/validation.service.ts @@ -23,6 +23,33 @@ import { Logger } from '../../logger'; * Typed as Set since we're validating unknown input strings. */ export const ALLOWED_ENTITY_TYPES: Set = new Set(ENTITY_TYPES); +const TASK_TIME_DELTA_ACTION_TYPE = '[TimeTracking] Sync time spent'; + +const isValidCalendarDate = (value: unknown): value is string => { + if (typeof value !== 'string') return false; + const match = /^(\d{4})-(\d{2})-(\d{2})$/.exec(value); + if (!match) return false; + const year = Number(match[1]); + const month = Number(match[2]); + const day = Number(match[3]); + if (month < 1 || month > 12 || day < 1) return false; + const isLeapYear = year % 4 === 0 && (year % 100 !== 0 || year % 400 === 0); + const daysInMonth = [31, isLeapYear ? 29 : 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31]; + return day <= daysInMonth[month - 1]; +}; + +const extractActionPayload = (payload: unknown): Record | null => { + if (!payload || typeof payload !== 'object' || Array.isArray(payload)) { + return null; + } + const payloadObject = payload as Record; + const actionPayload = payloadObject['actionPayload']; + return actionPayload && + typeof actionPayload === 'object' && + !Array.isArray(actionPayload) + ? (actionPayload as Record) + : payloadObject; +}; export interface ValidationResult { valid: boolean; @@ -157,6 +184,26 @@ export class ValidationService { errorCode: SYNC_ERROR_CODES.INVALID_PAYLOAD, }; } + + // Validate the visible form of additive task-time operations at the server + // boundary. Encrypted payloads are validated by the client after decryption. + if (op.actionType === TASK_TIME_DELTA_ACTION_TYPE && !op.isPayloadEncrypted) { + const actionPayload = extractActionPayload(op.payload); + if ( + !actionPayload || + actionPayload['taskId'] !== op.entityId || + !isValidCalendarDate(actionPayload['date']) || + typeof actionPayload['duration'] !== 'number' || + !Number.isFinite(actionPayload['duration']) || + actionPayload['duration'] < 0 + ) { + return { + valid: false, + error: 'Invalid task-time sync payload', + errorCode: SYNC_ERROR_CODES.INVALID_PAYLOAD, + }; + } + } if (op.schemaVersion !== undefined) { if ( !Number.isInteger(op.schemaVersion) || diff --git a/packages/super-sync-server/src/sync/sync.types.ts b/packages/super-sync-server/src/sync/sync.types.ts index 836b397337..4186879aaa 100644 --- a/packages/super-sync-server/src/sync/sync.types.ts +++ b/packages/super-sync-server/src/sync/sync.types.ts @@ -221,6 +221,7 @@ export const DUPLICATE_OP_SELECT = { export interface LatestEntityOperationRow { entityId: string; clientId: string; + actionType: string; vectorClock: unknown; serverSeq?: number; } diff --git a/packages/super-sync-server/tests/conflict.spec.ts b/packages/super-sync-server/tests/conflict.spec.ts index dca33fff32..7b4bf4cea0 100644 --- a/packages/super-sync-server/tests/conflict.spec.ts +++ b/packages/super-sync-server/tests/conflict.spec.ts @@ -218,6 +218,41 @@ describe('conflict helpers', () => { }); }); + it('accepts concurrent additive task-time deltas for the same task', () => { + const result = resolveConflictForExistingOp( + op({ + actionType: '[TimeTracking] Sync time spent', + vectorClock: { 'client-a': 1 }, + }), + 'task-1', + { + actionType: '[TimeTracking] Sync time spent', + clientId: 'client-b', + vectorClock: { 'client-b': 1 }, + }, + ); + + expect(result).toEqual({ hasConflict: false }); + }); + + it('still rejects a causally stale additive task-time delta', () => { + const result = resolveConflictForExistingOp( + op({ + actionType: '[TimeTracking] Sync time spent', + vectorClock: { 'client-a': 1 }, + }), + 'task-1', + { + actionType: '[TimeTracking] Sync time spent', + clientId: 'client-a', + vectorClock: { 'client-a': 2 }, + }, + ); + + expect(result.hasConflict).toBe(true); + expect(result.conflictType).toBe('superseded'); + }); + it('classifies less-than vector clocks as superseded', () => { const result = resolveConflictForExistingOp( op({ vectorClock: { 'client-a': 1 } }), diff --git a/packages/super-sync-server/tests/sync.service.spec.ts b/packages/super-sync-server/tests/sync.service.spec.ts index 6a5f2a5b52..916b7e8926 100644 --- a/packages/super-sync-server/tests/sync.service.spec.ts +++ b/packages/super-sync-server/tests/sync.service.spec.ts @@ -1150,6 +1150,41 @@ describe('SyncService', () => { }, ); + it('should preserve concurrent additive task-time deltas within one batch', async () => { + const service = new SyncService({ batchUpload: true }); + const makeTaskTimeOp = ( + id: string, + vectorClock: Record, + duration: number, + ): Operation => ({ + id, + clientId, + actionType: '[TimeTracking] Sync time spent', + opType: 'UPD', + entityType: 'TASK', + entityId: 'task-1', + payload: { + actionPayload: { + taskId: 'task-1', + date: '2026-07-13', + duration, + }, + entityChanges: [], + }, + vectorClock, + timestamp: Date.now(), + schemaVersion: 1, + }); + + const results = await service.uploadOps(userId, clientId, [ + makeTaskTimeOp(uuidv7(), { [clientId]: 1 }, 5000), + makeTaskTimeOp(uuidv7(), { 'other-client': 1 }, 7000), + ]); + + expect(results.every((result) => result.accepted)).toBe(true); + expect(testState.operations.size).toBe(2); + }); + it('should use entityIds when prefetching batch conflicts', async () => { const service = new SyncService({ batchUpload: true }); testState.userSyncStates.set(userId, { userId, lastSeq: 1 }); diff --git a/packages/super-sync-server/tests/validation.service.spec.ts b/packages/super-sync-server/tests/validation.service.spec.ts index 734e2a110c..3824d84771 100644 --- a/packages/super-sync-server/tests/validation.service.spec.ts +++ b/packages/super-sync-server/tests/validation.service.spec.ts @@ -16,11 +16,13 @@ describe('ValidationService', () => { const createValidOp = (overrides: Record = {}) => ({ id: 'op-1', clientId, + actionType: '[Task] Add Task', opType: 'CRT' as const, entityType: 'TASK', entityId: 'entity-1', payload: { name: 'Test' }, timestamp: Date.now(), + schemaVersion: 1, vectorClock: { [clientId]: 1 }, ...overrides, }); @@ -301,6 +303,61 @@ describe('ValidationService', () => { expect(result.errorCode).toBe(SYNC_ERROR_CODES.INVALID_PAYLOAD); }); + it('should validate additive task-time payload identity and arithmetic', () => { + const validPayload = { + actionPayload: { + taskId: 'entity-1', + date: '2024-02-29', + duration: 5000, + }, + entityChanges: [], + }; + expect( + validationService.validateOp( + createValidOp({ + actionType: '[TimeTracking] Sync time spent', + opType: 'UPD', + payload: validPayload, + }), + clientId, + ).valid, + ).toBe(true); + expect( + validationService.validateOp( + createValidOp({ + actionType: '[TimeTracking] Sync time spent', + opType: 'UPD', + payload: { + actionPayload: { + taskId: 'entity-1', + date: '0099-12-31', + duration: 5000, + }, + entityChanges: [], + }, + }), + clientId, + ).valid, + ).toBe(true); + + for (const actionPayload of [ + { taskId: 'other-task', date: '2024-02-29', duration: 5000 }, + { taskId: 'entity-1', date: '2024-02-30', duration: 5000 }, + { taskId: 'entity-1', date: '2024-02-29', duration: -1 }, + ]) { + const result = validationService.validateOp( + createValidOp({ + actionType: '[TimeTracking] Sync time spent', + opType: 'UPD', + payload: { actionPayload, entityChanges: [] }, + }), + clientId, + ); + expect(result.valid).toBe(false); + expect(result.errorCode).toBe(SYNC_ERROR_CODES.INVALID_PAYLOAD); + } + }); + it('should reject schema version less than 1', () => { const op = createValidOp({ schemaVersion: 0 }); const result = validationService.validateOp(op, clientId); diff --git a/packages/sync-providers/src/provider-types.ts b/packages/sync-providers/src/provider-types.ts index d746fdd096..1914815e71 100644 --- a/packages/sync-providers/src/provider-types.ts +++ b/packages/sync-providers/src/provider-types.ts @@ -159,6 +159,11 @@ export interface OperationSyncCapable< ops: SyncOperation[], clientId: string, lastKnownServerSeq?: number, + /** + * Optional host snapshot captured atomically with `ops`. File-backed + * providers embed it beside their recent-op window; API providers ignore it. + */ + localStateSnapshot?: unknown, ): Promise; /** * @param limit Best-effort page-size hint. Cursor-based providers (SuperSync) diff --git a/src/app/core/util/batched-time-sync-accumulator.spec.ts b/src/app/core/util/batched-time-sync-accumulator.spec.ts index 6d9a700981..1c516ee6e9 100644 --- a/src/app/core/util/batched-time-sync-accumulator.spec.ts +++ b/src/app/core/util/batched-time-sync-accumulator.spec.ts @@ -121,6 +121,54 @@ describe('BatchedTimeSyncAccumulator', () => { jasmine.clock().uninstall(); }); + + it('should restore an entry when dispatch throws so it can be retried', () => { + dispatchSpy.and.throwError('dispatch failed'); + accumulator.accumulate('entity1', 1000, '2024-01-15'); + + accumulator.flush(); + + expect(accumulator.getPendingEntries()).toEqual([ + { id: 'entity1', date: '2024-01-15', duration: 1000 }, + ]); + expect(accumulator.shouldFlush()).toBe(true); + }); + }); + + describe('clear', () => { + it('should remove all entries without dispatching them', () => { + accumulator.accumulate('entity1', 1000, '2024-01-15'); + accumulator.accumulate('entity2', 2000, '2024-01-15'); + + accumulator.clear(); + accumulator.flush(); + + expect(accumulator.getPendingEntries()).toEqual([]); + expect(dispatchSpy).not.toHaveBeenCalled(); + }); + }); + + describe('getPendingEntries', () => { + it('should return a detached snapshot of pending durations', () => { + accumulator.accumulate('entity1', 1000, '2024-01-15'); + + const entries = accumulator.getPendingEntries(); + entries[0].duration = 9999; + + expect(accumulator.getPendingEntries()).toEqual([ + { id: 'entity1', date: '2024-01-15', duration: 1000 }, + ]); + }); + + it('should not return entries that were flushed or cleared', () => { + accumulator.accumulate('entity1', 1000, '2024-01-15'); + accumulator.accumulate('entity2', 2000, '2024-01-15'); + + accumulator.flushOne('entity1'); + accumulator.clearOne('entity2'); + + expect(accumulator.getPendingEntries()).toEqual([]); + }); }); describe('flushOne', () => { @@ -158,6 +206,17 @@ describe('BatchedTimeSyncAccumulator', () => { expect(dispatchSpy).not.toHaveBeenCalled(); }); + + it('should restore the entity when dispatch throws', () => { + dispatchSpy.and.throwError('dispatch failed'); + accumulator.accumulate('entity1', 1000, '2024-01-15'); + + accumulator.flushOne('entity1'); + + expect(accumulator.getPendingEntries()).toEqual([ + { id: 'entity1', date: '2024-01-15', duration: 1000 }, + ]); + }); }); describe('clearOne', () => { diff --git a/src/app/core/util/batched-time-sync-accumulator.ts b/src/app/core/util/batched-time-sync-accumulator.ts index c6351d8918..8ee7935a4c 100644 --- a/src/app/core/util/batched-time-sync-accumulator.ts +++ b/src/app/core/util/batched-time-sync-accumulator.ts @@ -1,5 +1,11 @@ import { Log } from '../log'; +export interface BatchedTimeSyncEntry { + id: string; + duration: number; + date: string; +} + /** * Handles batched accumulation and flushing of time tracking data. * Used by TaskService and SimpleCounterService to reduce sync frequency. @@ -35,6 +41,17 @@ export class BatchedTimeSyncAccumulator { return Date.now() - this._lastSyncTime >= this._syncIntervalMs; } + /** + * Returns a detached snapshot for replay-safe state projection. + */ + getPendingEntries(): BatchedTimeSyncEntry[] { + return Array.from(this._unsyncedDuration, ([id, { duration, date }]) => ({ + id, + duration, + date, + })); + } + /** * Flushes all accumulated time and resets. * Uses defensive approach: clears internal state first to prevent data loss @@ -46,13 +63,16 @@ export class BatchedTimeSyncAccumulator { this._unsyncedDuration.clear(); this._lastSyncTime = Date.now(); - // Dispatch each entry, catching errors individually + // Dispatch each entry, catching errors individually. A failed dispatch is + // restored to the accumulator so snapshot projection keeps excluding it and + // the next timer tick/explicit flush can retry it. for (const [id, { duration, date }] of entries) { if (duration > 0) { try { this._dispatchSync(id, date, duration); } catch (e) { Log.error('[BatchedTimeSyncAccumulator] Error dispatching sync for', id, e); + this._restoreAfterDispatchFailure(id, date, duration); } } } @@ -71,6 +91,7 @@ export class BatchedTimeSyncAccumulator { this._dispatchSync(id, accumulated.date, accumulated.duration); } catch (e) { Log.error('[BatchedTimeSyncAccumulator] Error dispatching sync for', id, e); + this._restoreAfterDispatchFailure(id, accumulated.date, accumulated.duration); } } } @@ -82,10 +103,34 @@ export class BatchedTimeSyncAccumulator { this._unsyncedDuration.delete(id); } + /** Clears all accumulated data without dispatching it. */ + clear(): void { + this._unsyncedDuration.clear(); + this._lastSyncTime = Date.now(); + } + /** * Resets last sync time (call after flushing secondary data). */ resetSyncTime(): void { this._lastSyncTime = Date.now(); } + + private _restoreAfterDispatchFailure(id: string, date: string, duration: number): void { + const current = this._unsyncedDuration.get(id); + if (!current) { + this._unsyncedDuration.set(id, { date, duration }); + } else if (current.date === date) { + current.duration += duration; + } else { + // JavaScript dispatch is synchronous, so a different-date entry cannot + // normally appear here. Preserve the newer entry and surface the anomaly. + Log.error( + '[BatchedTimeSyncAccumulator] Could not restore failed sync for older date', + id, + ); + } + // Make the next shouldFlush() check retry immediately. + this._lastSyncTime = 0; + } } diff --git a/src/app/features/android/store/android-foreground-tracking.effects.spec.ts b/src/app/features/android/store/android-foreground-tracking.effects.spec.ts index 3d3e77b3b1..3790e48c4a 100644 --- a/src/app/features/android/store/android-foreground-tracking.effects.spec.ts +++ b/src/app/features/android/store/android-foreground-tracking.effects.spec.ts @@ -1396,7 +1396,11 @@ describe('AndroidForegroundTrackingEffects - syncTimeSpent dispatch (issue #6207 const syncElapsedTimeForTaskWithOpLog = async ( taskId: string, elapsedJson: string | null, - getTask: (id: string) => Promise<{ id: string; timeSpent: number } | null>, + getTask: (id: string) => Promise<{ + id: string; + timeSpent: number; + timeSpentOnDay?: Record; + } | null>, addTimeSpent: (task: unknown, duration: number, date: string) => void, dispatch: (action: { taskId: string; date: string; duration: number }) => void, resetTrackingStart: () => void, @@ -1439,7 +1443,11 @@ describe('AndroidForegroundTrackingEffects - syncTimeSpent dispatch (issue #6207 if (duration > 0) { addTimeSpent(task, duration, todayStr); // Also dispatch syncTimeSpent to capture in operation log - dispatch({ taskId: task.id, date: todayStr, duration }); + dispatch({ + taskId: task.id, + date: todayStr, + duration, + }); resetTrackingStart(); } } catch (e) { @@ -1463,9 +1471,14 @@ describe('AndroidForegroundTrackingEffects - syncTimeSpent dispatch (issue #6207 const expectedDuration = nativeElapsed - appTimeSpent; // 14 minutes const elapsedJson = JSON.stringify({ taskId: 'task-1', elapsedMs: nativeElapsed }); - const getTask = async (): Promise<{ id: string; timeSpent: number }> => ({ + const getTask = async (): Promise<{ + id: string; + timeSpent: number; + timeSpentOnDay: Record; + }> => ({ id: 'task-1', timeSpent: appTimeSpent, + timeSpentOnDay: { ['2024-01-01']: appTimeSpent }, }); await syncElapsedTimeForTaskWithOpLog( @@ -1480,7 +1493,11 @@ describe('AndroidForegroundTrackingEffects - syncTimeSpent dispatch (issue #6207 ); expect(addTimeSpentSpy).toHaveBeenCalledWith( - { id: 'task-1', timeSpent: appTimeSpent }, + { + id: 'task-1', + timeSpent: appTimeSpent, + timeSpentOnDay: { ['2024-01-01']: appTimeSpent }, + }, expectedDuration, '2024-01-01', ); @@ -1579,9 +1596,14 @@ describe('AndroidForegroundTrackingEffects - syncTimeSpent dispatch (issue #6207 elapsedMs: existingTimeMs + backgroundTimeMs, // Total native elapsed }); - const getTask = async (): Promise<{ id: string; timeSpent: number }> => ({ + const getTask = async (): Promise<{ + id: string; + timeSpent: number; + timeSpentOnDay: Record; + }> => ({ id: 'task-1', timeSpent: existingTimeMs, // App only knows about pre-background time + timeSpentOnDay: { ['2024-01-01']: existingTimeMs }, }); await syncElapsedTimeForTaskWithOpLog( @@ -1597,7 +1619,11 @@ describe('AndroidForegroundTrackingEffects - syncTimeSpent dispatch (issue #6207 // addTimeSpent updates local NgRx state immediately expect(addTimeSpentSpy).toHaveBeenCalledWith( - { id: 'task-1', timeSpent: existingTimeMs }, + { + id: 'task-1', + timeSpent: existingTimeMs, + timeSpentOnDay: { ['2024-01-01']: existingTimeMs }, + }, syncDuration, '2024-01-01', ); diff --git a/src/app/features/android/store/android-foreground-tracking.effects.ts b/src/app/features/android/store/android-foreground-tracking.effects.ts index a3f758ae99..af86e6643a 100644 --- a/src/app/features/android/store/android-foreground-tracking.effects.ts +++ b/src/app/features/android/store/android-foreground-tracking.effects.ts @@ -19,7 +19,6 @@ import { selectIsTaskDataLoaded, } from '../../tasks/store/task.selectors'; import { DroidLog } from '../../../core/log'; -import { DateService } from '../../../core/date/date.service'; import { Task } from '../../tasks/task.model'; import { selectTimer } from '../../focus-mode/store/focus-mode.selectors'; import { combineLatest, firstValueFrom, Subject } from 'rxjs'; @@ -28,7 +27,6 @@ import { HydrationStateService } from '../../../op-log/apply/hydration-state.ser import { SnackService } from '../../../core/snack/snack.service'; import { GlobalTrackingIntervalService } from '../../../core/global-tracking-interval/global-tracking-interval.service'; import { OperationWriteFlushService } from '../../../op-log/sync/operation-write-flush.service'; -import { syncTimeSpent } from '../../time-tracking/store/time-tracking.actions'; export type NativeTrackingData = { taskId: string; @@ -174,7 +172,6 @@ export const handleAndroidResume = async ( export class AndroidForegroundTrackingEffects { private _store = inject(Store); private _taskService = inject(TaskService); - private _dateService = inject(DateService); private _hydrationState = inject(HydrationStateService); private _snackService = inject(SnackService); private _globalTrackingIntervalService = inject(GlobalTrackingIntervalService); @@ -646,16 +643,7 @@ export class AndroidForegroundTrackingEffects { } if (duration > 0) { - this._taskService.addTimeSpent(task, duration, this._dateService.todayStr()); - // Also dispatch syncTimeSpent to capture in operation log - // addTimeSpent only updates local state, syncTimeSpent creates the operation - this._store.dispatch( - syncTimeSpent({ - taskId: task.id, - date: this._dateService.todayStr(), - duration, - }), - ); + this._taskService.addTimeSpentAndSync(task, duration); // Reset the tracking interval to prevent double-counting // The native service has the authoritative time, so we reset the app's // interval timer to avoid adding the same time again from tick$ diff --git a/src/app/features/project/project.service.spec.ts b/src/app/features/project/project.service.spec.ts index ce0c566c30..c85daaa517 100644 --- a/src/app/features/project/project.service.spec.ts +++ b/src/app/features/project/project.service.spec.ts @@ -29,6 +29,7 @@ import { import { selectMenuTreeProjectTree } from '../menu-tree/store/menu-tree.selectors'; import { MenuTreeKind } from '../menu-tree/store/menu-tree.model'; import { menuTreeFeatureKey } from '../menu-tree/store/menu-tree.reducer'; +import { TaskTimeSyncService } from '../tasks/task-time-sync.service'; describe('ProjectService', () => { let service: ProjectService; @@ -37,6 +38,7 @@ describe('ProjectService', () => { let snackService: jasmine.SpyObj; let workContextService: jasmine.SpyObj; let timeTrackingService: jasmine.SpyObj; + let taskTimeSync: jasmine.SpyObj; /* eslint-disable @typescript-eslint/naming-convention */ const initialTaskState: TaskState = { @@ -142,6 +144,7 @@ describe('ProjectService', () => { 'setUnDone', 'getAllTasksForProject', ]); + taskTimeSync = jasmine.createSpyObj('TaskTimeSyncService', ['clearOne']); taskService.createNewTaskWithDefaults.and.callFake(() => { taskCounter++; return createTask({ @@ -187,6 +190,7 @@ describe('ProjectService', () => { }), provideMockActions(() => EMPTY), { provide: TaskService, useValue: taskService }, + { provide: TaskTimeSyncService, useValue: taskTimeSync }, { provide: TranslateService, useValue: { @@ -240,6 +244,25 @@ describe('ProjectService', () => { store.resetSelectors(); }); + describe('remove', () => { + it('should clear pending time for every task removed with the project', async () => { + const project = createProject({ + id: 'project-1', + taskIds: ['task-1', 'task-2'], + backlogTaskIds: [], + noteIds: [], + }); + + await service.remove(project); + + expect(taskTimeSync.clearOne.calls.allArgs()).toEqual([ + ['task-1'], + ['task-2'], + ['sub-task-1'], + ]); + }); + }); + describe('tree order lists', () => { it('should expose visible projects in menu tree order', (done) => { const projects = [ diff --git a/src/app/features/project/project.service.ts b/src/app/features/project/project.service.ts index 05a955261d..84a839e986 100644 --- a/src/app/features/project/project.service.ts +++ b/src/app/features/project/project.service.ts @@ -57,6 +57,7 @@ import { DateService } from '../../core/date/date.service'; import { getDeadlineAutoPlanFields } from '../tasks/util/get-deadline-auto-plan-fields'; import { MenuTreeService } from '../menu-tree/menu-tree.service'; import { selectMenuTreeProjectTree } from '../menu-tree/store/menu-tree.selectors'; +import { TaskTimeSyncService } from '../tasks/task-time-sync.service'; export interface ProjectCompletionInfo { topLevelTasks: Task[]; @@ -114,6 +115,7 @@ export class ProjectService { private readonly _actions$ = inject(LOCAL_ACTIONS); private readonly _timeTrackingService = inject(TimeTrackingService); private readonly _taskService = inject(TaskService); + private readonly _taskTimeSync = inject(TaskTimeSyncService); private readonly _translate = inject(TranslateService); private readonly _matDialog = inject(MatDialog); private readonly _dateService = inject(DateService); @@ -398,6 +400,7 @@ export class ProjectService { } }); const allTaskIds = [...allParentTaskIds, ...subTaskIdsForProject]; + allTaskIds.forEach((taskId) => this._taskTimeSync.clearOne(taskId)); this._store$.dispatch( TaskSharedActions.deleteProject({ projectId: project.id, diff --git a/src/app/features/schedule/schedule-event/schedule-event.component.spec.ts b/src/app/features/schedule/schedule-event/schedule-event.component.spec.ts index f584562b87..b02d75f1a4 100644 --- a/src/app/features/schedule/schedule-event/schedule-event.component.spec.ts +++ b/src/app/features/schedule/schedule-event/schedule-event.component.spec.ts @@ -1,6 +1,6 @@ -import { ComponentFixture, TestBed } from '@angular/core/testing'; +import { ComponentFixture, fakeAsync, TestBed, tick } from '@angular/core/testing'; import { NO_ERRORS_SCHEMA } from '@angular/core'; -import { provideMockStore } from '@ngrx/store/testing'; +import { MockStore, provideMockStore } from '@ngrx/store/testing'; import { DragDropModule } from '@angular/cdk/drag-drop'; import { TranslateModule } from '@ngx-translate/core'; import { ScheduleEventComponent } from './schedule-event.component'; @@ -10,6 +10,7 @@ import { MatDialog } from '@angular/material/dialog'; import { TaskService } from '../../tasks/task.service'; import { CalendarEventActionsService } from '../../calendar-integration/calendar-event-actions.service'; import { DateTimeFormatService } from '../../../core/date-time-format/date-time-format.service'; +import { selectTaskByIdWithSubTaskData } from '../../tasks/store/task.selectors'; const makeCalendarScheduleEvent = (isReferenceCalendar: boolean): ScheduleEvent => ({ id: 'cal-1', @@ -50,7 +51,10 @@ describe('ScheduleEventComponent – isReferenceCalendar', () => { { provide: MatDialog, useValue: { open: jasmine.createSpy('open') } }, { provide: TaskService, - useValue: { setSelectedId: jasmine.createSpy('setSelectedId') }, + useValue: { + setSelectedId: jasmine.createSpy('setSelectedId'), + remove: jasmine.createSpy('remove'), + }, }, { provide: CalendarEventActionsService, @@ -170,6 +174,26 @@ describe('ScheduleEventComponent – isReferenceCalendar', () => { }); }); + it('should delete scheduled tasks through TaskService cleanup', fakeAsync(() => { + const task = { + id: 'task-1', + title: 'Task', + timeEstimate: 3600000, + subTaskIds: [], + subTasks: [], + } as any; + const store = TestBed.inject(MockStore); + const taskService = TestBed.inject(TaskService) as jasmine.SpyObj; + store.overrideSelector(selectTaskByIdWithSubTaskData, task); + fixture.componentRef.setInput('event', makeTaskScheduleEvent()); + fixture.detectChanges(); + + component.deleteTask(); + tick(51); + + expect(taskService.remove).toHaveBeenCalledOnceWith(task); + })); + describe('style', () => { it('should render overlapping events in equal-width lanes', () => { fixture.componentRef.setInput( @@ -210,7 +234,10 @@ describe('ScheduleEventComponent – isReferenceCalendar', () => { { provide: MatDialog, useValue: { open: jasmine.createSpy('open') } }, { provide: TaskService, - useValue: { setSelectedId: jasmine.createSpy('setSelectedId') }, + useValue: { + setSelectedId: jasmine.createSpy('setSelectedId'), + remove: jasmine.createSpy('remove'), + }, }, { provide: CalendarEventActionsService, diff --git a/src/app/features/schedule/schedule-event/schedule-event.component.ts b/src/app/features/schedule/schedule-event/schedule-event.component.ts index bc66bada5d..e0ed59d029 100644 --- a/src/app/features/schedule/schedule-event/schedule-event.component.ts +++ b/src/app/features/schedule/schedule-event/schedule-event.component.ts @@ -471,7 +471,7 @@ export class ScheduleEventComponent implements AfterViewInit, OnDestroy { delay(50), ) .subscribe((task) => { - this._store.dispatch(TaskSharedActions.deleteTask({ task })); + this._taskService.remove(task); }); } diff --git a/src/app/features/schedule/schedule-week/schedule-week-drag.service.spec.ts b/src/app/features/schedule/schedule-week/schedule-week-drag.service.spec.ts index 7b2f5609b6..7ffaf51265 100644 --- a/src/app/features/schedule/schedule-week/schedule-week-drag.service.spec.ts +++ b/src/app/features/schedule/schedule-week/schedule-week-drag.service.spec.ts @@ -3,7 +3,7 @@ import { CdkDragRelease } from '@angular/cdk/drag-drop'; import { provideMockStore, MockStore } from '@ngrx/store/testing'; import { ScheduleWeekDragService } from './schedule-week-drag.service'; import { GlobalConfigService } from '../../config/global-config.service'; -import { TaskReminderOptionId } from '../../tasks/task.model'; +import { TaskCopy, TaskReminderOptionId } from '../../tasks/task.model'; import { DEFAULT_GLOBAL_CONFIG } from '../../config/default-global-config.const'; import { TaskSharedActions } from '../../../root-store/meta/task-shared.actions'; import { signal } from '@angular/core'; @@ -12,6 +12,7 @@ import { ScheduleEvent } from '../schedule.model'; import { FH, SVEType, T_ID_PREFIX } from '../schedule.const'; import { PlannerActions } from '../../planner/store/planner.actions'; import { CalendarEventActionsService } from '../../calendar-integration/calendar-event-actions.service'; +import { DateService } from '../../../core/date/date.service'; const ONE_HOUR_MS = 60 * 60 * 1000; const TWO_HOURS_MS = 2 * 60 * 60 * 1000; @@ -58,6 +59,10 @@ describe('ScheduleWeekDragService', () => { provide: GlobalConfigService, useValue: createMockGlobalConfigService(defaultTaskRemindOption), }, + { + provide: DateService, + useValue: { todayStr: () => '2026-03-20' }, + }, ], }); @@ -75,6 +80,25 @@ describe('ScheduleWeekDragService', () => { TestBed.resetTestingModule(); }); + it('captures today when unscheduling a timed task but leaving it in Today', () => { + setupTestBed(); + const sourceEvent = createTaskEvent({ id: 'task-1', dueWithTime: Date.now() }); + + ( + service as unknown as { + _handleUnschedule: (task: TaskCopy, sourceEvent: ScheduleEvent) => void; + } + )._handleUnschedule(sourceEvent.data as TaskCopy, sourceEvent); + + expect(dispatchSpy).toHaveBeenCalledWith( + TaskSharedActions.unscheduleTask({ + id: 'task-1', + isLeaveInToday: true, + today: '2026-03-20', + }), + ); + }); + const createTaskEvent = ( task: Partial<{ id: string; title: string; dueWithTime: number }> = {}, ): ScheduleEvent => diff --git a/src/app/features/schedule/schedule-week/schedule-week-drag.service.ts b/src/app/features/schedule/schedule-week/schedule-week-drag.service.ts index 74ca0a0276..66016beb4d 100644 --- a/src/app/features/schedule/schedule-week/schedule-week-drag.service.ts +++ b/src/app/features/schedule/schedule-week/schedule-week-drag.service.ts @@ -25,6 +25,7 @@ import { selectTodayTaskIds } from '../../work-context/store/work-context.select import { first } from 'rxjs/operators'; import { getTimeLeftForTask } from '../../../util/get-time-left-for-task'; import { CalendarEventActionsService } from '../../calendar-integration/calendar-event-actions.service'; +import { DateService } from '../../../core/date/date.service'; interface PointerPosition { x: number; @@ -49,6 +50,7 @@ export class ScheduleWeekDragService { private readonly _store = inject(Store); private readonly _globalConfigService = inject(GlobalConfigService); private readonly _calendarEventActions = inject(CalendarEventActionsService); + private readonly _dateService = inject(DateService); private readonly _isShiftMode = signal(false); readonly isShiftMode: Signal = this._isShiftMode.asReadonly(); @@ -874,6 +876,7 @@ export class ScheduleWeekDragService { TaskSharedActions.unscheduleTask({ id: task.id, isLeaveInToday: true, + today: this._dateService.todayStr(), }), ); } diff --git a/src/app/features/task-repeat-cfg/store/task-repeat-cleanup.effects.spec.ts b/src/app/features/task-repeat-cfg/store/task-repeat-cleanup.effects.spec.ts index 3f7c29c013..c2153ab469 100644 --- a/src/app/features/task-repeat-cfg/store/task-repeat-cleanup.effects.spec.ts +++ b/src/app/features/task-repeat-cfg/store/task-repeat-cleanup.effects.spec.ts @@ -21,12 +21,14 @@ import { DateService } from '../../../core/date/date.service'; import { getDateTimeFromClockString } from '../../../util/get-date-time-from-clock-string'; import { remindOptionToMilliseconds } from '../../tasks/util/remind-option-to-milliseconds'; import { TODAY_TAG } from '../../tag/tag.const'; +import { TaskTimeSyncService } from '../../tasks/task-time-sync.service'; describe('TaskRepeatCleanupEffects', () => { let effects: TaskRepeatCleanupEffects; let store: jasmine.SpyObj; let repeatableTasks$: BehaviorSubject; let repeatCfgs$: BehaviorSubject; + let taskTimeSync: jasmine.SpyObj; const DAY_MS = 24 * 60 * 60 * 1000; const todayMs = new Date().setHours(12, 0, 0, 0); @@ -83,6 +85,7 @@ describe('TaskRepeatCleanupEffects', () => { 'DeletedTaskIssueSidecarService', ['set'], ); + taskTimeSync = jasmine.createSpyObj('TaskTimeSyncService', ['clearOne']); TestBed.configureTestingModule({ providers: [ @@ -93,6 +96,7 @@ describe('TaskRepeatCleanupEffects', () => { { provide: SyncWrapperService, useValue: syncWrapperSpy }, { provide: HydrationStateService, useValue: hydrationStateSpy }, { provide: DeletedTaskIssueSidecarService, useValue: sidecarSpy }, + { provide: TaskTimeSyncService, useValue: taskTimeSync }, { provide: DateService, useValue: { todayStr: () => getDbDateStr(todayMs) } }, ], }); @@ -215,6 +219,7 @@ describe('TaskRepeatCleanupEffects', () => { // The older instance is removed; the newer one survives. expect(getDispatchedDeleteIds()).toEqual(['today-stale']); + expect(taskTimeSync.clearOne).toHaveBeenCalledOnceWith('today-stale'); sub.unsubscribe(); })); diff --git a/src/app/features/task-repeat-cfg/store/task-repeat-cleanup.effects.ts b/src/app/features/task-repeat-cfg/store/task-repeat-cleanup.effects.ts index a507458743..7b9ffd8e23 100644 --- a/src/app/features/task-repeat-cfg/store/task-repeat-cleanup.effects.ts +++ b/src/app/features/task-repeat-cfg/store/task-repeat-cleanup.effects.ts @@ -22,6 +22,7 @@ import { isValidSplitTime } from '../../../util/is-valid-split-time'; import { getDateTimeFromClockString } from '../../../util/get-date-time-from-clock-string'; import { dateStrToUtcDate } from '../../../util/date-str-to-utc-date'; import { remindOptionToMilliseconds } from '../../tasks/util/remind-option-to-milliseconds'; +import { TaskTimeSyncService } from '../../tasks/task-time-sync.service'; const _sameStringSet = (a: readonly string[], b: readonly string[]): boolean => { if (a.length !== b.length) { @@ -119,6 +120,7 @@ export class TaskRepeatCleanupEffects { private _hydrationState = inject(HydrationStateService); private _deletedTaskIssueSidecar = inject(DeletedTaskIssueSidecarService); private _dateService = inject(DateService); + private _taskTimeSync = inject(TaskTimeSyncService); /** * After initial sync + date change, detect and remove stale duplicate @@ -278,6 +280,12 @@ export class TaskRepeatCleanupEffects { issueProviderId: t.issueProviderId!, })), ); + for (const task of deleteTasks) { + this._taskTimeSync.clearOne(task.id); + task.subTasks.forEach((subTask) => + this._taskTimeSync.clearOne(subTask.id), + ); + } this._store.dispatch( TaskSharedActions.deleteTasks({ taskIds: deleteIds, diff --git a/src/app/features/tasks/store/short-syntax.effects.spec.ts b/src/app/features/tasks/store/short-syntax.effects.spec.ts index 84037f3415..5b58081c2b 100644 --- a/src/app/features/tasks/store/short-syntax.effects.spec.ts +++ b/src/app/features/tasks/store/short-syntax.effects.spec.ts @@ -17,6 +17,7 @@ import { LOCAL_ACTIONS } from '../../../util/local-actions.token'; import { DEFAULT_GLOBAL_CONFIG } from '../../config/default-global-config.const'; import { DEFAULT_TASK, Task } from '../task.model'; import { WorkContextType } from '../../work-context/work-context.model'; +import { TaskTimeSyncService } from '../task-time-sync.service'; describe('ShortSyntaxEffects', () => { let effects: ShortSyntaxEffects; @@ -44,6 +45,7 @@ describe('ShortSyntaxEffects', () => { let workContextServiceMock: { activeWorkContextId: string; }; + let taskTimeSyncServiceSpy: jasmine.SpyObj; const createTask = (id: string, partial: Partial = {}): Task => ({ ...DEFAULT_TASK, @@ -112,6 +114,7 @@ describe('ShortSyntaxEffects', () => { workContextServiceMock = { activeWorkContextId: 'project-1', }; + taskTimeSyncServiceSpy = jasmine.createSpyObj('TaskTimeSyncService', ['flushOne']); TestBed.configureTestingModule({ providers: [ @@ -127,6 +130,7 @@ describe('ShortSyntaxEffects', () => { { provide: LayoutService, useValue: layoutServiceSpy }, { provide: WorkContextService, useValue: workContextServiceMock }, { provide: LOCAL_ACTIONS, useValue: actions$ }, + { provide: TaskTimeSyncService, useValue: taskTimeSyncServiceSpy }, ], }); @@ -248,6 +252,21 @@ describe('ShortSyntaxEffects', () => { expect(emitted).toBe(false); })); + it('should flush pending timer deltas before an absolute short-syntax time edit', fakeAsync(() => { + const task = createTask('task-1', { title: 'Task 10m/1h' }); + taskServiceMock.getByIdOnce$.and.returnValue(of(task)); + effects.shortSyntax$.subscribe(); + + actions$.next( + TaskSharedActions.updateTask({ + task: { id: 'task-1', changes: { title: task.title } }, + }), + ); + tick(100); + + expect(taskTimeSyncServiceSpy.flushOne).toHaveBeenCalledOnceWith('task-1'); + })); + it('should add URL as attachment when updating task title with a URL', fakeAsync(() => { const task = createTask('task-1', { title: 'Check out www.example.com for more info', diff --git a/src/app/features/tasks/store/short-syntax.effects.ts b/src/app/features/tasks/store/short-syntax.effects.ts index ebda655baa..769525f4cf 100644 --- a/src/app/features/tasks/store/short-syntax.effects.ts +++ b/src/app/features/tasks/store/short-syntax.effects.ts @@ -37,6 +37,7 @@ import { DateService } from '../../../core/date/date.service'; import { INBOX_PROJECT } from '../../project/project.const'; import { devError } from '../../../util/dev-error'; import { TaskLog } from '../../../core/log'; +import { TaskTimeSyncService } from '../task-time-sync.service'; @Injectable() export class ShortSyntaxEffects { @@ -50,6 +51,7 @@ export class ShortSyntaxEffects { private _layoutService = inject(LayoutService); private _workContextService = inject(WorkContextService); private _dateService = inject(DateService); + private _taskTimeSyncService = inject(TaskTimeSyncService); shortSyntax$ = createEffect(() => this._actions$.pipe( @@ -232,6 +234,13 @@ export class ShortSyntaxEffects { delete finalTaskChanges.hasDeadlineTime; + // The parser writes an absolute per-day total. Persist any earlier + // additive timer batch first so replay observes the same order as + // the live store: delta, then absolute replacement. + if (finalTaskChanges.timeSpentOnDay) { + this._taskTimeSyncService.flushOne(task.id); + } + actions.push( TaskSharedActions.applyShortSyntax({ task, diff --git a/src/app/features/tasks/store/task.reducer.spec.ts b/src/app/features/tasks/store/task.reducer.spec.ts index d66e73d3d5..e1937d01ea 100644 --- a/src/app/features/tasks/store/task.reducer.spec.ts +++ b/src/app/features/tasks/store/task.reducer.spec.ts @@ -960,6 +960,104 @@ describe('Task Reducer', () => { expect(state.entities['task-r']!.timeSpent).toBe(8000); }); + it('should add consecutive durations from stale task action snapshots', () => { + const staleTask = createTask('task-r', { + timeSpentOnDay: { '2024-01-01': 100 }, + timeSpent: 100, + }); + const stateWithTime: TaskState = { + ...initialTaskState, + ids: ['task-r'], + entities: { 'task-r': staleTask }, + }; + + const afterFirstCredit = taskReducer( + stateWithTime, + TimeTrackingActions.addTimeSpent({ + task: staleTask, + date: '2024-01-01', + duration: 20, + isFromTrackingReminder: false, + }), + ); + const afterSecondCredit = taskReducer( + afterFirstCredit, + TimeTrackingActions.addTimeSpent({ + task: staleTask, + date: '2024-01-01', + duration: 30, + isFromTrackingReminder: false, + }), + ); + + expect(afterSecondCredit.entities['task-r']!.timeSpentOnDay['2024-01-01']).toBe( + 150, + ); + expect(afterSecondCredit.entities['task-r']!.timeSpent).toBe(150); + }); + + it('should keep own time sync additive when client identity is unavailable', () => { + const taskWithLocalTime = createTask('task-r', { + timeSpentOnDay: { '2024-01-01': 3000 }, + timeSpent: 3000, + }); + const stateWithLocalTime: TaskState = { + ...initialTaskState, + ids: ['task-r'], + entities: { 'task-r': taskWithLocalTime }, + }; + const action = { + ...syncTimeSpent({ + taskId: 'task-r', + date: '2024-01-01', + duration: 5000, + }), + timeSpentForDay: 5000, + }; + const ownReplayAction = { + ...action, + meta: { ...action.meta, isRemote: true }, + }; + + const state = taskReducer(stateWithLocalTime, ownReplayAction); + + expect(state.entities['task-r']!.timeSpentOnDay['2024-01-01']).toBe(8000); + expect(state.entities['task-r']!.timeSpent).toBe(8000); + }); + + it('should keep foreign time sync additive to preserve concurrent tracking', () => { + const taskWithLocalTime = createTask('task-r', { + timeSpentOnDay: { '2024-01-01': 3000 }, + timeSpent: 3000, + }); + const stateWithLocalTime: TaskState = { + ...initialTaskState, + ids: ['task-r'], + entities: { 'task-r': taskWithLocalTime }, + }; + const action = { + ...syncTimeSpent({ + taskId: 'task-r', + date: '2024-01-01', + duration: 5000, + }), + timeSpentForDay: 5000, + }; + const foreignAction = { + ...action, + meta: { + ...action.meta, + isRemote: true, + isApplyingFromOtherClient: true, + }, + }; + + const state = taskReducer(stateWithLocalTime, foreignAction); + + expect(state.entities['task-r']!.timeSpentOnDay['2024-01-01']).toBe(8000); + expect(state.entities['task-r']!.timeSpent).toBe(8000); + }); + it('should handle remote dispatch for missing task gracefully', () => { const action = syncTimeSpent({ taskId: 'nonexistent', diff --git a/src/app/features/tasks/store/task.reducer.ts b/src/app/features/tasks/store/task.reducer.ts index cede160553..e92ce7658c 100644 --- a/src/app/features/tasks/store/task.reducer.ts +++ b/src/app/features/tasks/store/task.reducer.ts @@ -219,21 +219,27 @@ export const taskReducer = createReducer( ); return state; } + const currentTask = state.entities[task.id]; + if (!currentTask) { + return state; + } const currentTimeSpentForTickDay = - (task.timeSpentOnDay && +task.timeSpentOnDay[date]) || 0; + (currentTask.timeSpentOnDay && +currentTask.timeSpentOnDay[date]) || 0; return updateTimeSpentForTask( task.id, { - ...task.timeSpentOnDay, + ...currentTask.timeSpentOnDay, [date]: currentTimeSpentForTickDay + duration, }, state, ); }), - // Sync time spent from remote clients - // Local: no-op (state already updated by addTimeSpent ticks) - // Remote: apply the batched duration + // Sync time spent from operation replay. + // Local: no-op (state already updated by addTimeSpent ticks). + // Replay is additive for every client so concurrent tracking contributions are + // preserved. Op-log/file snapshots project pending local batches out before they + // are persisted, preventing a snapshot from overlapping a later delta operation. on(syncTimeSpent, (state, action) => { // Only apply for remote actions - local state is already up-to-date if (!(action.meta as PersistentActionMeta).isRemote) { diff --git a/src/app/features/tasks/task-list/task-list.component.spec.ts b/src/app/features/tasks/task-list/task-list.component.spec.ts index 1581e2e9e7..9a5a7f9a4a 100644 --- a/src/app/features/tasks/task-list/task-list.component.spec.ts +++ b/src/app/features/tasks/task-list/task-list.component.spec.ts @@ -833,11 +833,14 @@ describe('TaskListComponent', () => { ); expect(store.dispatch).toHaveBeenCalledWith( - TaskSharedActions.convertToMainTask({ + jasmine.objectContaining({ + type: TaskSharedActions.convertToMainTask.type, task: dragged as unknown as TaskWithSubTasks, isPlanForToday: false, afterTaskId: 't1', isDone: false, + today: jasmine.any(String), + modified: jasmine.any(Number), }), ); }); @@ -854,11 +857,15 @@ describe('TaskListComponent', () => { ); expect(store.dispatch).toHaveBeenCalledWith( - TaskSharedActions.convertToMainTask({ + jasmine.objectContaining({ + type: TaskSharedActions.convertToMainTask.type, task: dragged as unknown as TaskWithSubTasks, isPlanForToday: false, afterTaskId: null, isDone: true, + today: jasmine.any(String), + doneOn: jasmine.any(Number), + modified: jasmine.any(Number), }), ); }); diff --git a/src/app/features/tasks/task-list/task-list.component.ts b/src/app/features/tasks/task-list/task-list.component.ts index b02edbd6ca..0afa599c0d 100644 --- a/src/app/features/tasks/task-list/task-list.component.ts +++ b/src/app/features/tasks/task-list/task-list.component.ts @@ -572,12 +572,17 @@ export class TaskListComponent implements OnDestroy, AfterViewInit { (target === 'DONE' || target === 'UNDONE') ) { const afterTaskId = getAnchorFromDragDrop(taskId, newOrderedIds); + const now = Date.now(); + const isDone = target === 'DONE'; this._store.dispatch( TaskSharedActions.convertToMainTask({ task: draggedTask ?? ({ id: taskId, parentId: src } as TaskWithSubTasks), isPlanForToday: this._workContextService.activeWorkContextId === TODAY_TAG.id, afterTaskId, - isDone: target === 'DONE', + isDone, + today: this._dateService.todayStr(), + modified: now, + ...(isDone ? { doneOn: now } : {}), }), ); return; diff --git a/src/app/features/tasks/task-time-sync.service.spec.ts b/src/app/features/tasks/task-time-sync.service.spec.ts new file mode 100644 index 0000000000..5e8dcd3f74 --- /dev/null +++ b/src/app/features/tasks/task-time-sync.service.spec.ts @@ -0,0 +1,93 @@ +import { TestBed } from '@angular/core/testing'; +import { MockStore, provideMockStore } from '@ngrx/store/testing'; +import { AppStateSnapshot } from '../../op-log/core/types/backup.types'; +import { initialTaskState, taskReducer } from './store/task.reducer'; +import { DEFAULT_TASK, Task, TaskState } from './task.model'; +import { TaskTimeSyncService } from './task-time-sync.service'; + +const createTask = (id: string, overrides: Partial = {}): Task => + ({ + ...DEFAULT_TASK, + id, + title: id, + created: 1, + ...overrides, + }) as Task; + +describe('TaskTimeSyncService', () => { + let service: TaskTimeSyncService; + let store: MockStore; + let dispatchSpy: jasmine.Spy; + + beforeEach(() => { + TestBed.configureTestingModule({ + providers: [TaskTimeSyncService, provideMockStore()], + }); + service = TestBed.inject(TaskTimeSyncService); + store = TestBed.inject(MockStore); + dispatchSpy = spyOn(store, 'dispatch'); + }); + + it('flushes accumulated time as a delta-only persistent action', () => { + service.accumulate('task-1', 3000, '2024-01-15'); + service.accumulate('task-1', 2000, '2024-01-15'); + + service.flush(); + + const action = dispatchSpy.calls.mostRecent().args[0] as Record; + expect(action['type']).toBe('[TimeTracking] Sync time spent'); + expect(action['taskId']).toBe('task-1'); + expect(action['date']).toBe('2024-01-15'); + expect(action['duration']).toBe(5000); + expect(action['timeSpentForDay']).toBeUndefined(); + }); + + it('projects pending task time out of an op-log snapshot', () => { + const task = createTask('task-1', { + timeSpentOnDay: { ['2024-01-15']: 5000 }, + timeSpent: 5000, + }); + const taskState: TaskState = { + ...initialTaskState, + ids: ['task-1'], + entities: { ['task-1']: task }, + }; + const snapshot = { task: taskState } as AppStateSnapshot; + service.accumulate('task-1', 5000, '2024-01-15'); + + const projected = service.projectSnapshot(snapshot); + + expect((projected.task as TaskState).entities['task-1']!.timeSpent).toBe(0); + expect((snapshot.task as TaskState).entities['task-1']!.timeSpent).toBe(5000); + }); + + it('reconstructs the live total from a projected snapshot plus the flushed tail op', () => { + const task = createTask('task-1', { + timeSpentOnDay: { ['2024-01-15']: 5000 }, + timeSpent: 5000, + }); + const taskState: TaskState = { + ...initialTaskState, + ids: ['task-1'], + entities: { ['task-1']: task }, + }; + service.accumulate('task-1', 5000, '2024-01-15'); + const projected = service.projectSnapshot({ task: taskState } as AppStateSnapshot); + + service.flush(); + const tailAction = dispatchSpy.calls.mostRecent().args[0]; + const replayedState = taskReducer(projected.task as TaskState, { + ...tailAction, + meta: { ...tailAction.meta, isRemote: true }, + }); + + expect(replayedState.entities['task-1']!.timeSpentOnDay['2024-01-15']).toBe(5000); + expect(replayedState.entities['task-1']!.timeSpent).toBe(5000); + }); + + it('returns the original snapshot when no task time is pending', () => { + const snapshot = { task: initialTaskState } as AppStateSnapshot; + + expect(service.projectSnapshot(snapshot)).toBe(snapshot); + }); +}); diff --git a/src/app/features/tasks/task-time-sync.service.ts b/src/app/features/tasks/task-time-sync.service.ts new file mode 100644 index 0000000000..c2d3d23a02 --- /dev/null +++ b/src/app/features/tasks/task-time-sync.service.ts @@ -0,0 +1,69 @@ +import { inject, Injectable } from '@angular/core'; +import { Store } from '@ngrx/store'; +import { + BatchedTimeSyncAccumulator, + BatchedTimeSyncEntry, +} from '../../core/util/batched-time-sync-accumulator'; +import { AppStateSnapshot } from '../../op-log/core/types/backup.types'; +import { RootState } from '../../root-store/root-state'; +import { syncTimeSpent } from '../time-tracking/store/time-tracking.actions'; +import { TaskState } from './task.model'; +import { projectPendingTimeFromTaskState } from './util/project-pending-time-from-task-state'; + +/** + * Owns the task-time batching state shared by live tracking and replay-safe snapshots. + */ +@Injectable({ providedIn: 'root' }) +export class TaskTimeSyncService { + private static readonly SYNC_INTERVAL_MS = 5 * 60 * 1000; + private readonly _store = inject>(Store); + private readonly _accumulator = new BatchedTimeSyncAccumulator( + TaskTimeSyncService.SYNC_INTERVAL_MS, + (taskId, date, duration) => { + this._store.dispatch(syncTimeSpent({ taskId, date, duration })); + }, + ); + + accumulate(taskId: string, duration: number, date: string): void { + this._accumulator.accumulate(taskId, duration, date); + } + + shouldFlush(): boolean { + return this._accumulator.shouldFlush(); + } + + flush(): void { + this._accumulator.flush(); + } + + flushOne(taskId: string): void { + this._accumulator.flushOne(taskId); + } + + clearOne(taskId: string): void { + this._accumulator.clearOne(taskId); + } + + clear(): void { + this._accumulator.clear(); + } + + projectSnapshot( + snapshot: AppStateSnapshot, + additionalPendingEntries: BatchedTimeSyncEntry[] = [], + ): AppStateSnapshot { + const pendingEntries = [ + ...this._accumulator.getPendingEntries(), + ...additionalPendingEntries, + ]; + if (pendingEntries.length === 0) { + return snapshot; + } + + const taskState = snapshot.task as TaskState; + const projectedTaskState = projectPendingTimeFromTaskState(taskState, pendingEntries); + return projectedTaskState === taskState + ? snapshot + : { ...snapshot, task: projectedTaskState }; + } +} diff --git a/src/app/features/tasks/task.service.spec.ts b/src/app/features/tasks/task.service.spec.ts index e0193caad9..dd4d8b8e7e 100644 --- a/src/app/features/tasks/task.service.spec.ts +++ b/src/app/features/tasks/task.service.spec.ts @@ -27,12 +27,14 @@ import { TODAY_TAG } from '../tag/tag.const'; import { INBOX_PROJECT } from '../project/project.const'; import { signal } from '@angular/core'; import { DeletedTaskIssueSidecarService } from '../issue/two-way-sync/deleted-task-issue-sidecar.service'; +import { TaskTimeSyncService } from './task-time-sync.service'; describe('TaskService', () => { let service: TaskService; let store: MockStore; let archiveService: jasmine.SpyObj; let deletedTaskIssueSidecar: DeletedTaskIssueSidecarService; + let taskTimeSync: TaskTimeSyncService; let tickSubject: Subject<{ duration: number; date: string }>; const createMockTask = (id: string, overrides: Partial = {}): Task => @@ -166,6 +168,7 @@ describe('TaskService', () => { store = TestBed.inject(MockStore); archiveService = TestBed.inject(ArchiveService) as jasmine.SpyObj; deletedTaskIssueSidecar = TestBed.inject(DeletedTaskIssueSidecarService); + taskTimeSync = TestBed.inject(TaskTimeSyncService); spyOn(store, 'dispatch').and.callThrough(); }); @@ -315,6 +318,20 @@ describe('TaskService', () => { expect(store.dispatch).toHaveBeenCalledWith(TaskSharedActions.deleteTask({ task })); }); + + it('should clear pending time for the task and its subtasks', () => { + const clearOneSpy = spyOn(taskTimeSync, 'clearOne'); + const subTask = createMockTask('subtask-1', { parentId: 'task-1' }); + const task = createMockTaskWithSubTasks( + createMockTask('task-1', { subTaskIds: ['subtask-1'] }), + [subTask], + ); + + service.remove(task); + + expect(clearOneSpy).toHaveBeenCalledWith('task-1'); + expect(clearOneSpy).toHaveBeenCalledWith('subtask-1'); + }); }); describe('removeMultipleTasks', () => { @@ -344,6 +361,24 @@ describe('TaskService', () => { }), ); }); + + it('should flush pending time before replacing timeSpentOnDay', () => { + const flushOneSpy = spyOn(taskTimeSync, 'flushOne'); + + service.update('task-1', { + timeSpentOnDay: { ['2026-01-05']: 30000 }, + }); + + expect(flushOneSpy).toHaveBeenCalledWith('task-1'); + }); + + it('should not flush pending time for unrelated task updates', () => { + const flushOneSpy = spyOn(taskTimeSync, 'flushOne'); + + service.update('task-1', { title: 'Updated Title' }); + + expect(flushOneSpy).not.toHaveBeenCalled(); + }); }); describe('updateTags', () => { @@ -864,12 +899,57 @@ describe('TaskService', () => { }); }); + describe('batched time sync', () => { + it('captures a delta-only operation when the accumulator flushes', () => { + const task = createMockTask('task-1', { + timeSpentOnDay: { ['2026-01-05']: 300000 }, + timeSpent: 300000, + }); + store.setState({ + tasks: { + ids: ['task-1'], + entities: { ['task-1']: task }, + currentTaskId: 'task-1', + selectedTaskId: null, + taskDetailTargetPanel: null, + isDataLoaded: true, + }, + }); + store.refreshState(); + TestBed.inject(TaskTimeSyncService).accumulate('task-1', 60000, '2026-01-05'); + + service.flushAccumulatedTimeSpent(); + + expect(store.dispatch).toHaveBeenCalledWith( + jasmine.objectContaining({ + type: '[TimeTracking] Sync time spent', + taskId: 'task-1', + date: '2026-01-05', + duration: 60000, + }), + ); + const action = (store.dispatch as jasmine.Spy).calls + .allArgs() + .map(([dispatchedAction]) => dispatchedAction) + .find( + (dispatchedAction) => + dispatchedAction.type === '[TimeTracking] Sync time spent', + ) as Record; + expect(action['timeSpentForDay']).toBeUndefined(); + }); + }); + describe('addTimeSpentAndSync', () => { it('should dispatch both addTimeSpent and syncTimeSpent', () => { - const task = createMockTask('task-1'); + const task = createMockTask('task-1', { + timeSpentOnDay: { ['2026-01-05']: 120000 }, + timeSpent: 120000, + }); + const flushOneSpy = spyOn(taskTimeSync, 'flushOne'); service.addTimeSpentAndSync(task, 60000); + expect(flushOneSpy).toHaveBeenCalledWith('task-1'); expect(store.dispatch).toHaveBeenCalledWith( jasmine.objectContaining({ type: '[TimeTracking] Add time spent', @@ -927,8 +1007,11 @@ describe('TaskService', () => { describe('removeTimeSpent', () => { it('should dispatch removeTimeSpent action', () => { + const flushOneSpy = spyOn(taskTimeSync, 'flushOne'); + service.removeTimeSpent('task-1', 30000); + expect(flushOneSpy).toHaveBeenCalledWith('task-1'); expect(store.dispatch).toHaveBeenCalledWith( jasmine.objectContaining({ type: '[Task] Remove time spent', diff --git a/src/app/features/tasks/task.service.ts b/src/app/features/tasks/task.service.ts index 67fe38b707..0d04471e1d 100644 --- a/src/app/features/tasks/task.service.ts +++ b/src/app/features/tasks/task.service.ts @@ -36,7 +36,6 @@ import { import { getNextHideSubTasksMode } from './util/get-next-hide-sub-tasks-mode'; import { IssueProviderKey } from '../issue/issue.model'; import { GlobalTrackingIntervalService } from '../../core/global-tracking-interval/global-tracking-interval.service'; -import { BatchedTimeSyncAccumulator } from '../../core/util/batched-time-sync-accumulator'; import { selectAllTasks, selectCurrentTask, @@ -106,6 +105,7 @@ import { TaskFocusService } from './task-focus.service'; import { DeletedTaskIssueSidecarService } from '../issue/two-way-sync/deleted-task-issue-sidecar.service'; import { TimeBlockDeleteSidecarService } from '../calendar-integration/time-block/time-block-delete-sidecar.service'; import { getDeadlineAutoPlanFields } from './util/get-deadline-auto-plan-fields'; +import { TaskTimeSyncService } from './task-time-sync.service'; @Injectable({ providedIn: 'root', @@ -124,6 +124,7 @@ export class TaskService { private readonly _deletedTaskIssueSidecar = inject(DeletedTaskIssueSidecarService); private readonly _timeBlockDeleteSidecar = inject(TimeBlockDeleteSidecarService); private readonly _archiveTaskPromisesById = new Map>(); + private readonly _taskTimeSync = inject(TaskTimeSyncService); currentTaskId$: Observable = this._store.pipe( select(selectCurrentTaskId), @@ -200,13 +201,6 @@ export class TaskService { private _allTasks$: Observable = this._store.pipe(select(selectAllTasks)); private _taskEntities = this._store.selectSignal(selectTaskEntities); - // Batch sync for time tracking: accumulates duration per task, syncs every 5 minutes - private static readonly SYNC_INTERVAL_MS = 5 * 60 * 1000; // 5 minutes - private _timeAccumulator = new BatchedTimeSyncAccumulator( - TaskService.SYNC_INTERVAL_MS, - (taskId, date, duration) => - this._store.dispatch(syncTimeSpent({ taskId, date, duration })), - ); private _unsyncedContexts: Map< string, { contextType: 'TAG' | 'PROJECT'; contextId: string; date: string } @@ -238,13 +232,13 @@ export class TaskService { this.addTimeSpent(currentTask, tick.duration, tick.date); // Accumulate for batch sync - this._timeAccumulator.accumulate(currentTask.id, tick.duration, tick.date); + this._taskTimeSync.accumulate(currentTask.id, tick.duration, tick.date); // Track contexts for TIME_TRACKING sync this._trackContextsForSync(currentTask, tick.date); // Check if it's time to sync (every 5 minutes) - if (this._timeAccumulator.shouldFlush()) { + if (this._taskTimeSync.shouldFlush()) { this._flushAccumulatedTimeSpent(); } } @@ -292,7 +286,7 @@ export class TaskService { */ private _flushAccumulatedTimeSpent(): void { // Sync task.timeSpent totals - this._timeAccumulator.flush(); + this._taskTimeSync.flush(); // Sync TIME_TRACKING session data (start/end times) if (this._unsyncedContexts.size > 0) { @@ -482,6 +476,8 @@ export class TaskService { } remove(task: TaskWithSubTasks): void { + this._taskTimeSync.clearOne(task.id); + task.subTasks.forEach((subTask) => this._taskTimeSync.clearOne(subTask.id)); this._store.dispatch(TaskSharedActions.deleteTask({ task })); } @@ -493,6 +489,10 @@ export class TaskService { const tasks = taskIds .map((id) => entities[id]) .filter((task): task is Task => !!task); + for (const task of tasks) { + this._taskTimeSync.clearOne(task.id); + task.subTaskIds.forEach((subTaskId) => this._taskTimeSync.clearOne(subTaskId)); + } this._deletedTaskIssueSidecar.set( tasks .filter((t) => !!t.issueId && !!t.issueType && !!t.issueProviderId) @@ -509,6 +509,9 @@ export class TaskService { } update(id: string, changedFields: Partial): void { + if (Object.prototype.hasOwnProperty.call(changedFields, 'timeSpentOnDay')) { + this._taskTimeSync.flushOne(id); + } this._store.dispatch( TaskSharedActions.updateTask({ task: { id, changes: changedFields }, @@ -870,9 +873,16 @@ export class TaskService { if (duration <= 0) { return; } + this._taskTimeSync.flushOne(task.id); const date = this._dateService.todayStr(); this.addTimeSpent(task, duration, date); - this._store.dispatch(syncTimeSpent({ taskId: task.id, date, duration })); + this._store.dispatch( + syncTimeSpent({ + taskId: task.id, + date, + duration, + }), + ); } removeTimeSpent( @@ -880,6 +890,7 @@ export class TaskService { duration: number, date: string = this._dateService.todayStr(), ): void { + this._taskTimeSync.flushOne(id); this._store.dispatch(removeTimeSpent({ id, date, duration })); } @@ -1092,6 +1103,7 @@ export class TaskService { }); // today + todayIds.forEach((taskId) => this._taskTimeSync.flushOne(taskId)); this._store.dispatch( roundTimeSpentForDay({ day, taskIds: todayIds, roundTo, isRoundUp, projectId }), ); @@ -1256,11 +1268,14 @@ export class TaskService { async convertToMainTask(task: Task): Promise { const parent = await this.getByIdOnce$(task.parentId as string).toPromise(); + const now = Date.now(); this._store.dispatch( TaskSharedActions.convertToMainTask({ task, parentTagIds: parent.tagIds, isPlanForToday: this._workContextService.activeWorkContextId === TODAY_TAG.id, + today: this._dateService.todayStr(), + modified: now, }), ); } diff --git a/src/app/features/tasks/util/project-pending-time-from-task-state.spec.ts b/src/app/features/tasks/util/project-pending-time-from-task-state.spec.ts new file mode 100644 index 0000000000..81c4b12a82 --- /dev/null +++ b/src/app/features/tasks/util/project-pending-time-from-task-state.spec.ts @@ -0,0 +1,70 @@ +import { DEFAULT_TASK, Task, TaskState } from '../task.model'; +import { initialTaskState } from '../store/task.reducer'; +import { projectPendingTimeFromTaskState } from './project-pending-time-from-task-state'; + +const createTask = (id: string, overrides: Partial = {}): Task => + ({ + ...DEFAULT_TASK, + id, + title: id, + created: 1, + ...overrides, + }) as Task; + +describe('projectPendingTimeFromTaskState', () => { + it('subtracts pending deltas without mutating live task state', () => { + const task = createTask('task-1', { + timeSpentOnDay: { ['2024-01-15']: 5000 }, + timeSpent: 5000, + }); + const state: TaskState = { + ...initialTaskState, + ids: ['task-1'], + entities: { ['task-1']: task }, + currentTaskId: 'task-1', + }; + + const projected = projectPendingTimeFromTaskState(state, [ + { id: 'task-1', date: '2024-01-15', duration: 5000 }, + ]); + + expect(projected.entities['task-1']!.timeSpentOnDay['2024-01-15']).toBeUndefined(); + expect(projected.entities['task-1']!.timeSpent).toBe(0); + expect(state.entities['task-1']!.timeSpentOnDay['2024-01-15']).toBe(5000); + expect(state.entities['task-1']!.timeSpent).toBe(5000); + }); + + it('subtracts a pending subtask delta from its parent total', () => { + const parent = createTask('parent', { + subTaskIds: ['subtask'], + timeSpentOnDay: { ['2024-01-15']: 5000 }, + timeSpent: 5000, + }); + const subtask = createTask('subtask', { + parentId: 'parent', + timeSpentOnDay: { ['2024-01-15']: 5000 }, + timeSpent: 5000, + }); + const state: TaskState = { + ...initialTaskState, + ids: ['parent', 'subtask'], + entities: { parent, subtask }, + currentTaskId: 'subtask', + }; + + const projected = projectPendingTimeFromTaskState(state, [ + { id: 'subtask', date: '2024-01-15', duration: 5000 }, + ]); + + expect(projected.entities['subtask']!.timeSpent).toBe(0); + expect(projected.entities['parent']!.timeSpent).toBe(0); + }); + + it('leaves missing tasks unchanged', () => { + expect( + projectPendingTimeFromTaskState(initialTaskState, [ + { id: 'missing', date: '2024-01-15', duration: 5000 }, + ]), + ).toBe(initialTaskState); + }); +}); diff --git a/src/app/features/tasks/util/project-pending-time-from-task-state.ts b/src/app/features/tasks/util/project-pending-time-from-task-state.ts new file mode 100644 index 0000000000..31369b7932 --- /dev/null +++ b/src/app/features/tasks/util/project-pending-time-from-task-state.ts @@ -0,0 +1,37 @@ +import { BatchedTimeSyncEntry } from '../../../core/util/batched-time-sync-accumulator'; +import { TaskState } from '../task.model'; +import { updateTimeSpentForTask } from '../store/task.reducer.util'; + +/** + * Removes locally accumulated task-time deltas from an op-log snapshot. + * + * The live store is updated on every timer tick, while the corresponding persistent + * operation is intentionally batched. Persisting the live total during that window + * would make the later delta overlap the snapshot and double-count on replay. + */ +export const projectPendingTimeFromTaskState = ( + state: TaskState, + pendingEntries: readonly BatchedTimeSyncEntry[], +): TaskState => { + let projectedState = state; + + for (const { id, date, duration } of pendingEntries) { + const task = projectedState.entities[id]; + if (!task || !Number.isFinite(duration) || duration <= 0) { + continue; + } + + const currentForDay = task.timeSpentOnDay?.[date] || 0; + const projectedForDay = Math.max(0, currentForDay - duration); + const timeSpentOnDay = { ...task.timeSpentOnDay }; + if (projectedForDay > 0) { + timeSpentOnDay[date] = projectedForDay; + } else { + delete timeSpentOnDay[date]; + } + + projectedState = updateTimeSpentForTask(id, timeSpentOnDay, projectedState); + } + + return projectedState; +}; diff --git a/src/app/features/time-tracking/store/time-tracking.actions.ts b/src/app/features/time-tracking/store/time-tracking.actions.ts index 87db66b10e..206e8dfad6 100644 --- a/src/app/features/time-tracking/store/time-tracking.actions.ts +++ b/src/app/features/time-tracking/store/time-tracking.actions.ts @@ -68,7 +68,8 @@ export const TimeTrackingActions = createActionGroup({ * Dispatched every 5 minutes during active tracking and when tracking stops. * * Local dispatch: Ignored by reducer (state already updated by addTimeSpent ticks) - * Remote dispatch: Applied to update timeSpentOnDay and timeTracking state + * Replay: Adds the duration. Replay-safe snapshots exclude still-pending batches, + * so the delta cannot overlap the state from which replay starts. */ export const syncTimeSpent = createAction( '[TimeTracking] Sync time spent', diff --git a/src/app/imex/sync/snapshot-upload.service.spec.ts b/src/app/imex/sync/snapshot-upload.service.spec.ts index 2e2507a124..c4dba2c187 100644 --- a/src/app/imex/sync/snapshot-upload.service.spec.ts +++ b/src/app/imex/sync/snapshot-upload.service.spec.ts @@ -14,6 +14,7 @@ import { OperationLogStoreService } from '../../op-log/persistence/operation-log import type { SuperSyncPrivateCfg } from '@sp/sync-providers/super-sync'; import { WebCryptoNotAvailableError } from '../../op-log/core/errors/sync-errors'; import { DEFAULT_GLOBAL_CONFIG } from '../../features/config/default-global-config.const'; +import { LockService } from '../../op-log/sync/lock.service'; describe('SnapshotUploadService', () => { let service: SnapshotUploadService; @@ -26,6 +27,7 @@ describe('SnapshotUploadService', () => { }; let mockEncryptionService: jasmine.SpyObj; let mockOpLogStore: jasmine.SpyObj; + let mockLockService: jasmine.SpyObj; let mockSyncProvider: jasmine.SpyObj< SyncProviderBase & OperationSyncCapable >; @@ -75,9 +77,11 @@ describe('SnapshotUploadService', () => { mockProviderManager.setProviderConfig.and.resolveTo(); mockStateSnapshotService = jasmine.createSpyObj('StateSnapshotService', [ - 'getStateSnapshotAsync', + 'getStateSnapshotForOperationLogAsync', ]); - mockStateSnapshotService.getStateSnapshotAsync.and.resolveTo({} as any); + mockStateSnapshotService.getStateSnapshotForOperationLogAsync.and.resolveTo( + {} as any, + ); mockVectorClockService = jasmine.createSpyObj('VectorClockService', [ 'getCurrentVectorClock', @@ -102,6 +106,10 @@ describe('SnapshotUploadService', () => { ]); mockOpLogStore.getUnsynced.and.resolveTo([]); mockOpLogStore.markSynced.and.resolveTo(undefined); + mockLockService = jasmine.createSpyObj('LockService', ['request']); + mockLockService.request.and.callFake( + async (_name: string, callback: () => Promise) => callback(), + ); TestBed.configureTestingModule({ providers: [ @@ -112,6 +120,7 @@ describe('SnapshotUploadService', () => { { provide: CLIENT_ID_PROVIDER, useValue: mockClientIdProvider }, { provide: OperationEncryptionService, useValue: mockEncryptionService }, { provide: OperationLogStoreService, useValue: mockOpLogStore }, + { provide: LockService, useValue: mockLockService }, ], }); @@ -178,7 +187,9 @@ describe('SnapshotUploadService', () => { it('should gather all required data', async () => { const mockState = { tasks: [] }; const mockVectorClock = { clientA: 1 }; - mockStateSnapshotService.getStateSnapshotAsync.and.resolveTo(mockState as any); + mockStateSnapshotService.getStateSnapshotForOperationLogAsync.and.resolveTo( + mockState as any, + ); mockVectorClockService.getCurrentVectorClock.and.resolveTo(mockVectorClock); mockSyncProvider.privateCfg.load = jasmine .createSpy('load') @@ -206,7 +217,9 @@ describe('SnapshotUploadService', () => { }, }, }; - mockStateSnapshotService.getStateSnapshotAsync.and.resolveTo(mockState as any); + mockStateSnapshotService.getStateSnapshotForOperationLogAsync.and.resolveTo( + mockState as any, + ); const result = await service.gatherSnapshotData(); const globalConfig = result.state.globalConfig as Record; @@ -286,7 +299,9 @@ describe('SnapshotUploadService', () => { describe('deleteAndReuploadWithNewEncryption', () => { it('should gather data, delete, update config, and upload when disabling encryption', async () => { const mockState = { task: [] }; - mockStateSnapshotService.getStateSnapshotAsync.and.resolveTo(mockState as any); + mockStateSnapshotService.getStateSnapshotForOperationLogAsync.and.resolveTo( + mockState as any, + ); mockVectorClockService.getCurrentVectorClock.and.resolveTo({ c1: 1 }); const result = await service.deleteAndReuploadWithNewEncryption({ @@ -364,6 +379,10 @@ describe('SnapshotUploadService', () => { }); expect(callOrder).toEqual(['getUnsynced', 'deleteAllData']); + expect(mockLockService.request).toHaveBeenCalledWith( + 'sp_op_log', + jasmine.any(Function), + ); }); }); @@ -406,7 +425,9 @@ describe('SnapshotUploadService', () => { it('still succeeds when enabling with a usable key', async () => { mockCryptoSubtleAvailable(); - mockStateSnapshotService.getStateSnapshotAsync.and.resolveTo({ task: [] } as any); + mockStateSnapshotService.getStateSnapshotForOperationLogAsync.and.resolveTo({ + task: [], + } as any); await service.deleteAndReuploadWithNewEncryption({ encryptKey: 'my-key', @@ -422,7 +443,9 @@ describe('SnapshotUploadService', () => { it('should encrypt payload when enabling encryption', async () => { mockCryptoSubtleAvailable(); const mockState = { task: [] }; - mockStateSnapshotService.getStateSnapshotAsync.and.resolveTo(mockState as any); + mockStateSnapshotService.getStateSnapshotForOperationLogAsync.and.resolveTo( + mockState as any, + ); await service.deleteAndReuploadWithNewEncryption({ encryptKey: 'my-key', @@ -580,10 +603,12 @@ describe('SnapshotUploadService', () => { mockCryptoSubtleAvailable(); const callOrder: string[] = []; - mockStateSnapshotService.getStateSnapshotAsync.and.callFake(async () => { - callOrder.push('getStateSnapshotAsync'); - return {} as any; - }); + mockStateSnapshotService.getStateSnapshotForOperationLogAsync.and.callFake( + async () => { + callOrder.push('getStateSnapshotForOperationLogAsync'); + return {} as any; + }, + ); mockEncryptionService.encryptPayload.and.callFake(async () => { callOrder.push('encryptPayload'); @@ -615,7 +640,7 @@ describe('SnapshotUploadService', () => { }); expect(callOrder).toEqual([ - 'getStateSnapshotAsync', + 'getStateSnapshotForOperationLogAsync', 'encryptPayload', 'deleteAllData', 'setProviderConfig', @@ -635,7 +660,9 @@ describe('SnapshotUploadService', () => { }), ).toBeRejectedWithError(WebCryptoNotAvailableError); - expect(mockStateSnapshotService.getStateSnapshotAsync).not.toHaveBeenCalled(); + expect( + mockStateSnapshotService.getStateSnapshotForOperationLogAsync, + ).not.toHaveBeenCalled(); expect(mockSyncProvider.deleteAllData).not.toHaveBeenCalled(); expect(mockProviderManager.setProviderConfig).not.toHaveBeenCalled(); expect(mockSyncProvider.uploadSnapshot).not.toHaveBeenCalled(); diff --git a/src/app/imex/sync/snapshot-upload.service.ts b/src/app/imex/sync/snapshot-upload.service.ts index 160967fc50..ed8a417686 100644 --- a/src/app/imex/sync/snapshot-upload.service.ts +++ b/src/app/imex/sync/snapshot-upload.service.ts @@ -25,6 +25,8 @@ import { OperationEncryptionService } from '../../op-log/sync/operation-encrypti import { isCryptoSubtleAvailable } from '@sp/sync-core'; import { WebCryptoNotAvailableError } from '../../op-log/core/errors/sync-errors'; import { stripLocalOnlySyncSettingsFromAppData } from '../../features/config/local-only-sync-settings.util'; +import { LockService } from '../../op-log/sync/lock.service'; +import { LOCK_NAMES } from '../../op-log/core/operation-log.const'; /** * Data gathered for a snapshot upload operation. @@ -71,6 +73,7 @@ export class SnapshotUploadService { private _clientIdProvider: ClientIdProvider = inject(CLIENT_ID_PROVIDER); private _encryptionService = inject(OperationEncryptionService); private _opLogStore = inject(OperationLogStoreService); + private _lockService = inject(LockService); /** * Validates that the active provider is SuperSync and operation-sync capable. @@ -122,7 +125,7 @@ export class SnapshotUploadService { // The sync getStateSnapshot() returns DEFAULT_ARCHIVE (empty) which causes data loss SyncLog.normal(`${prefix}Getting current state...`); const state = stripLocalOnlySyncSettingsFromAppData( - await this._stateSnapshotService.getStateSnapshotAsync(), + await this._stateSnapshotService.getStateSnapshotForOperationLogAsync(), ) as AppStateSnapshot; const vectorClock = await this._vectorClockService.getCurrentVectorClock(); const clientId = await this._clientIdProvider.getOrGenerateClientId(); @@ -235,10 +238,14 @@ export class SnapshotUploadService { // otherwise re-push the entire local history on top of the snapshot). Mirrors // planRegularOpsAfterFullStateUpload in the op-log upload path, which this // direct snapshot upload bypasses. - const opsSubsumedBySnapshot = await this._opLogStore.getUnsynced(); - - const { syncProvider, existingCfg, state, vectorClock, clientId } = - await this.gatherSnapshotData(logPrefix); + const { opsSubsumedBySnapshot, snapshotData } = await this._lockService.request( + LOCK_NAMES.OPERATION_LOG, + async () => ({ + opsSubsumedBySnapshot: await this._opLogStore.getUnsynced(), + snapshotData: await this.gatherSnapshotData(logPrefix), + }), + ); + const { syncProvider, existingCfg, state, vectorClock, clientId } = snapshotData; // GHSA-9v8x-68pf-p5x7 defense-in-depth: a provider that mandates E2E // encryption (SuperSync) must never have a plaintext snapshot pushed. Fail diff --git a/src/app/op-log/apply/operation-converter.util.spec.ts b/src/app/op-log/apply/operation-converter.util.spec.ts index a55ee633ec..13cbc6fd58 100644 --- a/src/app/op-log/apply/operation-converter.util.spec.ts +++ b/src/app/op-log/apply/operation-converter.util.spec.ts @@ -81,6 +81,63 @@ describe('operation-converter utility', () => { expect(action.meta.opType).toBe(OpType.Create); }); + describe('task-time sync payload validation', () => { + const createTaskTimeOp = ( + payload: Record, + entityId = 'task-1', + ): Operation => + createMockOperation({ + actionType: ActionType.TIME_TRACKING_SYNC_TIME_SPENT, + entityId, + payload, + }); + + it('should accept a valid task-time delta', () => { + const action = convertOpToAction( + createTaskTimeOp({ + taskId: 'task-1', + date: '2024-02-29', + duration: 5000, + }), + ); + + expect((action as any).duration).toBe(5000); + }); + + it('should reject a taskId that differs from the canonical entityId', () => { + expect(() => + convertOpToAction( + createTaskTimeOp({ + taskId: 'task-2', + date: '2024-01-15', + duration: 5000, + }), + ), + ).toThrowError(/Invalid task-time sync payload/); + }); + + it('should reject impossible dates and negative durations', () => { + expect(() => + convertOpToAction( + createTaskTimeOp({ + taskId: 'task-1', + date: '2024-02-30', + duration: 5000, + }), + ), + ).toThrowError(/Invalid task-time sync payload/); + expect(() => + convertOpToAction( + createTaskTimeOp({ + taskId: 'task-1', + date: '2024-01-15', + duration: -1, + }), + ), + ).toThrowError(/Invalid task-time sync payload/); + }); + }); + it('should handle Create operation', () => { const op = createMockOperation({ opType: OpType.Create, @@ -399,6 +456,145 @@ describe('operation-converter utility', () => { expect((action as any).today).toBe('2024-06-14'); }); + + it('uses a timezone-independent UTC day near midnight', () => { + const op = createMockOperation({ + actionType: ActionType.TASK_SHARED_PLAN_FOR_TODAY, + timestamp: Date.UTC(2024, 5, 14, 23, 30), + payload: { taskIds: ['task-1'] }, + }); + + expect((convertOpToAction(op) as any).today).toBe('2024-06-14'); + }); + + it('replaces an invalid captured day with the deterministic fallback', () => { + const op = createMockOperation({ + actionType: ActionType.TASK_SHARED_PLAN_FOR_TODAY, + timestamp: Date.UTC(2024, 5, 14, 12), + payload: { taskIds: ['task-1'], today: '' }, + }); + + expect((convertOpToAction(op) as any).today).toBe('2024-06-14'); + }); + + it('replaces an impossible captured calendar day', () => { + const op = createMockOperation({ + actionType: ActionType.TASK_SHARED_PLAN_FOR_TODAY, + timestamp: Date.UTC(2024, 5, 14, 12), + payload: { taskIds: ['task-1'], today: '2024-99-99' }, + }); + + expect((convertOpToAction(op) as any).today).toBe('2024-06-14'); + }); + + it('uses a stable epoch fallback for a malformed legacy timestamp', () => { + const op = createMockOperation({ + actionType: ActionType.TASK_SHARED_PLAN_FOR_TODAY, + timestamp: Number.NaN, + payload: { taskIds: ['task-1'] }, + }); + + expect(() => convertOpToAction(op)).not.toThrow(); + expect((convertOpToAction(op) as any).today).toBe('1970-01-01'); + }); + }); + + describe('legacy convertToMainTask date backfill', () => { + it('injects replay-safe dates from the originating operation timestamp', () => { + const timestamp = new Date(2024, 5, 14, 12, 0, 0, 0).getTime(); + const op = createMockOperation({ + actionType: ActionType.TASK_SHARED_CONVERT_TO_MAIN, + timestamp, + payload: { + task: { id: 'task-1', parentId: 'parent-1' }, + isPlanForToday: true, + isDone: true, + }, + }); + + const action = convertOpToAction(op) as any; + + expect(action.today).toBe('2024-06-14'); + expect(action.doneOn).toBe(timestamp); + expect(action.modified).toBe(timestamp); + }); + + it('preserves dates already captured by the originating action', () => { + const op = createMockOperation({ + actionType: ActionType.TASK_SHARED_CONVERT_TO_MAIN, + timestamp: new Date(2024, 5, 15, 12, 0, 0, 0).getTime(), + payload: { + task: { id: 'task-1', parentId: 'parent-1' }, + isDone: true, + today: '2024-06-14', + doneOn: 1718352000000, + modified: 1718352000000, + }, + }); + + const action = convertOpToAction(op) as any; + + expect(action.today).toBe('2024-06-14'); + expect(action.doneOn).toBe(1718352000000); + expect(action.modified).toBe(1718352000000); + }); + + it('replaces invalid captured dates and timestamps', () => { + const timestamp = Date.UTC(2024, 5, 14, 12); + const op = createMockOperation({ + actionType: ActionType.TASK_SHARED_CONVERT_TO_MAIN, + timestamp, + payload: { + task: { id: 'task-1', parentId: 'parent-1' }, + isDone: true, + today: 'invalid', + doneOn: Number.POSITIVE_INFINITY, + modified: Number.NaN, + }, + }); + + const action = convertOpToAction(op) as any; + + expect(action.today).toBe('2024-06-14'); + expect(action.doneOn).toBe(timestamp); + expect(action.modified).toBe(timestamp); + }); + }); + + describe('legacy unscheduleTask date backfill', () => { + it('injects today when leaving the task in Today', () => { + const timestamp = new Date(2024, 5, 14, 12, 0, 0, 0).getTime(); + const op = createMockOperation({ + actionType: ActionType.TASK_SHARED_UNSCHEDULE, + timestamp, + payload: { id: 'task-1', isLeaveInToday: true }, + }); + + const action = convertOpToAction(op) as any; + + expect(action.today).toBe('2024-06-14'); + }); + + it('does not inject today for a plain unschedule', () => { + const op = createMockOperation({ + actionType: ActionType.TASK_SHARED_UNSCHEDULE, + payload: { id: 'task-1' }, + }); + + const action = convertOpToAction(op) as any; + + expect(action.today).toBeUndefined(); + }); + + it('replaces an invalid captured day when leaving the task in Today', () => { + const op = createMockOperation({ + actionType: ActionType.TASK_SHARED_UNSCHEDULE, + timestamp: Date.UTC(2024, 5, 14, 12), + payload: { id: 'task-1', isLeaveInToday: true, today: '' }, + }); + + expect((convertOpToAction(op) as any).today).toBe('2024-06-14'); + }); }); describe('updateTask done replay date backfill', () => { @@ -463,6 +659,25 @@ describe('operation-converter utility', () => { expect(action.task.changes.dueDay).toBe('2024-06-14'); }); + it('replaces a non-finite doneOn timestamp', () => { + const timestamp = Date.UTC(2024, 5, 14, 12); + const op = createMockOperation({ + actionType: ActionType.TASK_SHARED_UPDATE, + timestamp, + payload: { + actionPayload: { + task: { + id: 'task-1', + changes: { isDone: true, doneOn: Number.NaN }, + }, + }, + entityChanges: [], + }, + }); + + expect((convertOpToAction(op) as any).task.changes.doneOn).toBe(timestamp); + }); + it('does not inject done fields for undone updates', () => { const op = createMockOperation({ actionType: ActionType.TASK_SHARED_UPDATE, diff --git a/src/app/op-log/apply/operation-converter.util.ts b/src/app/op-log/apply/operation-converter.util.ts index f65d20d970..d610b638ff 100644 --- a/src/app/op-log/apply/operation-converter.util.ts +++ b/src/app/op-log/apply/operation-converter.util.ts @@ -9,7 +9,24 @@ import { isLwwUpdateActionType } from '../core/lww-update-action-types'; import { isSingletonEntityId } from '../core/entity-registry'; import { PersistentAction } from '../core/persistent-action.interface'; import { SyncLog } from '../../core/log'; -import { getDbDateStr } from '../../util/get-db-date-str'; +import { isValidDBDateStr } from '../../util/get-db-date-str'; + +const isValidDbDate = (value: unknown): value is string => + typeof value === 'string' && isValidDBDateStr(value); + +const isFiniteNumber = (value: unknown): value is number => + typeof value === 'number' && Number.isFinite(value); + +/** + * Legacy operations did not capture the originating logical day or timezone. + * UTC cannot recover that lost context, but it gives every replaying client the + * same deterministic best-effort fallback. + */ +const getDeterministicLegacyTimestamp = (timestamp: number): number => + Number.isFinite(timestamp) ? timestamp : 0; + +const getDeterministicLegacyDay = (timestamp: number): string => + new Date(getDeterministicLegacyTimestamp(timestamp)).toISOString().slice(0, 10); /** * Maps old/renamed action types to their current names. @@ -51,20 +68,62 @@ const addLegacyPlanForTodayDate = ( ): Record => { if ( actionType === ActionType.TASK_SHARED_PLAN_FOR_TODAY && - typeof actionPayload['today'] !== 'string' + !isValidDbDate(actionPayload['today']) ) { - // Legacy operations did not store the logical day, timezone, or start-of-next-day - // offset. The timestamp is the best available fallback, but it is interpreted in - // the replaying device's local timezone and can still be off near midnight or for - // dueWithTime values around a different original day-start offset. return { ...actionPayload, - today: getDbDateStr(op.timestamp), + today: getDeterministicLegacyDay(op.timestamp), }; } return actionPayload; }; +const addLegacyConvertToMainTaskDates = ( + actionType: string, + actionPayload: Record, + op: Operation, +): Record => { + if (actionType !== ActionType.TASK_SHARED_CONVERT_TO_MAIN) { + return actionPayload; + } + + return { + ...actionPayload, + today: isValidDbDate(actionPayload['today']) + ? actionPayload['today'] + : getDeterministicLegacyDay(op.timestamp), + modified: isFiniteNumber(actionPayload['modified']) + ? actionPayload['modified'] + : getDeterministicLegacyTimestamp(op.timestamp), + ...(actionPayload['isDone'] === true + ? { + doneOn: isFiniteNumber(actionPayload['doneOn']) + ? actionPayload['doneOn'] + : getDeterministicLegacyTimestamp(op.timestamp), + } + : {}), + }; +}; + +const addLegacyUnscheduleDate = ( + actionType: string, + actionPayload: Record, + op: Operation, +): Record => { + if ( + actionType !== ActionType.TASK_SHARED_UNSCHEDULE || + actionPayload['isLeaveInToday'] !== true || + isValidDbDate(actionPayload['today']) + ) { + return actionPayload; + } + + return { + ...actionPayload, + today: getDeterministicLegacyDay(op.timestamp), + }; +}; + const addReplaySafeDoneFields = ( actionType: string, actionPayload: Record, @@ -90,7 +149,7 @@ const addReplaySafeDoneFields = ( return actionPayload; } - const hasDoneOn = typeof taskChanges['doneOn'] === 'number'; + const hasDoneOn = isFiniteNumber(taskChanges['doneOn']); const replaySafeChanges = { ...taskChanges, @@ -99,7 +158,9 @@ const addReplaySafeDoneFields = ( // synthesize a schedule, so local apply and replay both yield no `dueDay` for // an unscheduled completion (replay determinism). Any `dueDay` the op already // carries (an explicit schedule) is preserved via the spread above. - doneOn: hasDoneOn ? taskChanges['doneOn'] : op.timestamp, + doneOn: hasDoneOn + ? taskChanges['doneOn'] + : getDeterministicLegacyTimestamp(op.timestamp), }; return { @@ -145,6 +206,40 @@ const stripMalformedConvertToMainTaskParentTagIds = ( return rest; }; +/** + * Task-time deltas are additive and therefore must be rejected rather than + * coerced when their identity or arithmetic fields are malformed. Otherwise an + * operation indexed for one task could mutate another, or a negative/invalid + * duration could silently corrupt replayed totals. + */ +const assertValidTaskTimeSyncPayload = ( + actionType: string, + actionPayload: Record, + op: Operation, +): void => { + if (actionType !== ActionType.TIME_TRACKING_SYNC_TIME_SPENT) { + return; + } + + const taskId = actionPayload['taskId']; + const date = actionPayload['date']; + const duration = actionPayload['duration']; + if ( + typeof taskId !== 'string' || + taskId.length === 0 || + taskId !== op.entityId || + typeof date !== 'string' || + !isValidDBDateStr(date) || + typeof duration !== 'number' || + !Number.isFinite(duration) || + duration < 0 + ) { + throw new Error( + `[convertOpToAction] Invalid task-time sync payload for operation ${op.id}`, + ); + } +}; + /** * Converts an Operation from the operation log back into a PersistentAction. * Used during sync replay and recovery to re-dispatch operations. @@ -169,12 +264,15 @@ export const convertOpToAction = (op: Operation): PersistentAction => { : (extractActionPayload(op.payload) as Record); actionPayload = addLegacyPlanForTodayDate(actionType, actionPayload, op); + actionPayload = addLegacyConvertToMainTaskDates(actionType, actionPayload, op); + actionPayload = addLegacyUnscheduleDate(actionType, actionPayload, op); actionPayload = addReplaySafeDoneFields(actionType, actionPayload, op); actionPayload = stripMalformedConvertToMainTaskParentTagIds( actionType, actionPayload, op, ); + assertValidTaskTimeSyncPayload(actionType, actionPayload, op); // Force `payload.id = op.entityId` for non-singleton LWW Update ops. The // op's `entityId` is the canonical identifier — producers also enforce diff --git a/src/app/op-log/backup/backup.service.spec.ts b/src/app/op-log/backup/backup.service.spec.ts index 6cd8081448..6589b9bd04 100644 --- a/src/app/op-log/backup/backup.service.spec.ts +++ b/src/app/op-log/backup/backup.service.spec.ts @@ -11,6 +11,7 @@ import { OperationWriteFlushService } from '../sync/operation-write-flush.servic import { LockService } from '../sync/lock.service'; import { ConflictJournalService } from '../sync/conflict-journal.service'; import { LOCK_NAMES } from '../core/operation-log.const'; +import { TaskTimeSyncService } from '../../features/tasks/task-time-sync.service'; describe('BackupService', () => { let service: BackupService; @@ -22,6 +23,7 @@ describe('BackupService', () => { let mockLockService: jasmine.SpyObj; let mockConflictJournal: jasmine.SpyObj; const backupRef = { backupId: 'backup-123', savedAt: 123 }; + let mockTaskTimeSyncService: jasmine.SpyObj; // eslint-disable-next-line @typescript-eslint/explicit-function-return-type const createMinimalValidBackup = () => ({ @@ -116,6 +118,7 @@ describe('BackupService', () => { mockLockService = jasmine.createSpyObj('LockService', ['request']); mockConflictJournal = jasmine.createSpyObj('ConflictJournalService', ['clearAll']); mockConflictJournal.clearAll.and.resolveTo(); + mockTaskTimeSyncService = jasmine.createSpyObj('TaskTimeSyncService', ['clear']); // Default mock returns mockStateSnapshotService.getStateSnapshotAsync.and.resolveTo( @@ -141,12 +144,19 @@ describe('BackupService', () => { }, { provide: LockService, useValue: mockLockService }, { provide: ConflictJournalService, useValue: mockConflictJournal }, + { provide: TaskTimeSyncService, useValue: mockTaskTimeSyncService }, ], }); service = TestBed.inject(BackupService); }); + it('should discard task-time accumulated against the replaced pre-import state', async () => { + await service.importCompleteBackup(createMinimalValidBackup() as any, true, true); + + expect(mockTaskTimeSyncService.clear).toHaveBeenCalledBefore(mockStore.dispatch); + }); + describe('captureImportBackup (#8107)', () => { it('should snapshot current state into the import backup store', async () => { const snapshot = createMinimalValidBackup(); diff --git a/src/app/op-log/backup/backup.service.ts b/src/app/op-log/backup/backup.service.ts index 61aee092e2..827b6e7db0 100644 --- a/src/app/op-log/backup/backup.service.ts +++ b/src/app/op-log/backup/backup.service.ts @@ -26,6 +26,7 @@ import { OperationWriteFlushService } from '../sync/operation-write-flush.servic import { LockService } from '../sync/lock.service'; import { ConflictJournalService } from '../sync/conflict-journal.service'; import { LOCK_NAMES } from '../core/operation-log.const'; +import { TaskTimeSyncService } from '../../features/tasks/task-time-sync.service'; /** * Service for handling backup import and export operations. @@ -47,6 +48,7 @@ export class BackupService { private _operationWriteFlushService = inject(OperationWriteFlushService); private _lockService = inject(LockService); private _conflictJournalService = inject(ConflictJournalService); + private _taskTimeSyncService = inject(TaskTimeSyncService); /** * Loads a complete backup of all application data. @@ -179,6 +181,10 @@ export class BackupService { // fresh post-import journal entries cannot land before the clear and // be wiped. clearAll swallows its own errors (must not fail the import). await this._conflictJournalService.clearAll(); + + // The imported full state replaces the live task totals. Any batch from + // the pre-import state must not flush later onto that new baseline. + this._taskTimeSyncService.clear(); }); // 4c. Reset all sync providers' lastServerSeq to 0. diff --git a/src/app/op-log/backup/state-snapshot.service.spec.ts b/src/app/op-log/backup/state-snapshot.service.spec.ts index 0421722ee5..5de0964e78 100644 --- a/src/app/op-log/backup/state-snapshot.service.spec.ts +++ b/src/app/op-log/backup/state-snapshot.service.spec.ts @@ -20,7 +20,12 @@ import { selectPluginMetadataFeatureState } from '../../plugins/store/plugin-met import { selectReminderFeatureState } from '../../features/reminder/store/reminder.reducer'; import { ArchiveModel } from '../../features/time-tracking/time-tracking.model'; import { initialTimeTrackingState } from '../../features/time-tracking/store/time-tracking.reducer'; -import { TaskState } from '../../features/tasks/task.model'; +import { TaskTimeSyncService } from '../../features/tasks/task-time-sync.service'; +import { DEFAULT_TASK, Task, TaskState } from '../../features/tasks/task.model'; +import { initialTaskState } from '../../features/tasks/store/task.reducer'; +import { OperationCaptureService } from '../capture/operation-capture.service'; +import { OpType } from '../core/operation.types'; +import { PersistentAction } from '../core/persistent-action.interface'; describe('StateSnapshotService', () => { let service: StateSnapshotService; @@ -172,6 +177,72 @@ describe('StateSnapshotService', () => { expect((snapshot.task as any).ids).toEqual(['task1']); expect((snapshot.task as any).entities).toBeDefined(); }); + + it('should exclude pending task time from an operation-log snapshot', () => { + const task = { + ...DEFAULT_TASK, + id: 'task1', + title: 'Test Task', + created: 1, + projectId: 'project-1', + timeSpentOnDay: { ['2024-01-15']: 5000 }, + timeSpent: 5000, + } as Task; + store.overrideSelector(selectTaskFeatureState, { + ...initialTaskState, + ids: ['task1'], + entities: { task1: task }, + }); + store.refreshState(); + TestBed.inject(TaskTimeSyncService).accumulate('task1', 5000, '2024-01-15'); + + const liveSnapshot = service.getStateSnapshot(); + const opLogSnapshot = service.getStateSnapshotForOperationLog(); + + expect((liveSnapshot.task as TaskState).entities['task1']!.timeSpent).toBe(5000); + expect((opLogSnapshot.task as TaskState).entities['task1']!.timeSpent).toBe(0); + }); + + it('should exclude a task-time delta whose operation write is still pending', () => { + const task = { + ...DEFAULT_TASK, + id: 'task1', + title: 'Test Task', + created: 1, + projectId: 'project-1', + timeSpentOnDay: { ['2024-01-15']: 5000 }, + timeSpent: 5000, + } as Task; + store.overrideSelector(selectTaskFeatureState, { + ...initialTaskState, + ids: ['task1'], + entities: { task1: task }, + }); + store.refreshState(); + const captureService = TestBed.inject(OperationCaptureService); + const action = { + type: '[TimeTracking] Sync time spent', + taskId: 'task1', + date: '2024-01-15', + duration: 5000, + meta: { + isPersistent: true, + entityType: 'TASK', + entityId: 'task1', + opType: OpType.Update, + }, + } as PersistentAction; + captureService.incrementPending(action); + + const opLogSnapshot = service.getStateSnapshotForOperationLog(); + + expect((opLogSnapshot.task as TaskState).entities['task1']!.timeSpent).toBe(0); + captureService.decrementPending(action); + expect( + (service.getStateSnapshotForOperationLog().task as TaskState).entities['task1']! + .timeSpent, + ).toBe(5000); + }); }); describe('getStateSnapshotAsync', () => { @@ -254,6 +325,30 @@ describe('StateSnapshotService', () => { }); }); + describe('getStateSnapshotForOperationLogAsync', () => { + it('should capture NgRx state before awaiting archive reads', async () => { + let resolveArchiveYoung!: (archive: ArchiveModel) => void; + archiveDbAdapterSpy.loadArchiveYoung.and.returnValue( + new Promise((resolve) => { + resolveArchiveYoung = resolve; + }), + ); + + const snapshotPromise = service.getStateSnapshotForOperationLogAsync(); + store.overrideSelector(selectTaskFeatureState, { + ...mockTaskState, + ids: ['task2'], + entities: { task2: { id: 'task2', title: 'Later Task' } }, + } as any); + store.refreshState(); + resolveArchiveYoung(mockArchiveYoung); + + const snapshot = await snapshotPromise; + + expect((snapshot.task as TaskState).ids).toEqual(['task1']); + }); + }); + describe('backward compatibility aliases', () => { it('getAllSyncModelDataFromStore should call getStateSnapshot', () => { spyOn(service, 'getStateSnapshot').and.callThrough(); diff --git a/src/app/op-log/backup/state-snapshot.service.ts b/src/app/op-log/backup/state-snapshot.service.ts index ce3f125622..8bd0027ecb 100644 --- a/src/app/op-log/backup/state-snapshot.service.ts +++ b/src/app/op-log/backup/state-snapshot.service.ts @@ -23,6 +23,8 @@ import { environment } from '../../../environments/environment'; import { ArchiveModel } from '../../features/time-tracking/time-tracking.model'; import { initialTimeTrackingState } from '../../features/time-tracking/store/time-tracking.reducer'; import { ArchiveDbAdapter } from '../../core/persistence/archive-db-adapter.service'; +import { TaskTimeSyncService } from '../../features/tasks/task-time-sync.service'; +import { OperationCaptureService } from '../capture/operation-capture.service'; import { AppStateSnapshot } from '../core/types/backup.types'; @@ -103,6 +105,8 @@ const SNAPSHOT_SELECTORS: readonly { export class StateSnapshotService { private _store = inject(Store); private _archiveDbAdapter = inject(ArchiveDbAdapter); + private _taskTimeSync = inject(TaskTimeSyncService); + private _operationCapture = inject(OperationCaptureService); /** * Gets all sync model data from NgRx store. @@ -120,6 +124,17 @@ export class StateSnapshotService { }; } + /** + * Gets a snapshot aligned with the operation log by excluding task-time deltas + * that are still waiting in the local batch accumulator. + */ + getStateSnapshotForOperationLog(): AppStateSnapshot { + return this._taskTimeSync.projectSnapshot( + this.getStateSnapshot(), + this._operationCapture.getPendingTaskTimeEntries(), + ); + } + /** * Alias for getStateSnapshot() for backward compatibility * @deprecated Use getStateSnapshot() instead @@ -151,6 +166,19 @@ export class StateSnapshotService { }; } + /** Async archive-inclusive counterpart of getStateSnapshotForOperationLog(). */ + async getStateSnapshotForOperationLogAsync(): Promise { + // Capture immutable NgRx references synchronously at the operation boundary. + // Archive reads can await IndexedDB without allowing later reducer updates to + // drift into a snapshot whose operation sequence was already fixed. + const snapshot = this.getStateSnapshotForOperationLog(); + const [archiveYoung, archiveOld] = await Promise.all([ + this._loadArchive('archiveYoung'), + this._loadArchive('archiveOld'), + ]); + return { ...snapshot, archiveYoung, archiveOld }; + } + /** * Alias for getStateSnapshotAsync() for backward compatibility * @deprecated Use getStateSnapshotAsync() instead diff --git a/src/app/op-log/capture/operation-capture.service.spec.ts b/src/app/op-log/capture/operation-capture.service.spec.ts index 747282dff2..707883cf45 100644 --- a/src/app/op-log/capture/operation-capture.service.spec.ts +++ b/src/app/op-log/capture/operation-capture.service.spec.ts @@ -84,6 +84,26 @@ describe('OperationCaptureService', () => { service.clear(); expect(service.getPendingCount()).toBe(0); }); + + it('should expose pending task-time deltas until their write attempt completes', () => { + const action = createPersistentAction( + '[TimeTracking] Sync time spent', + 'TASK', + 'task-1', + OpType.Update, + { taskId: 'task-1', date: '2024-01-15', duration: 5000 }, + ); + + service.incrementPending(action); + + expect(service.getPendingTaskTimeEntries()).toEqual([ + { id: 'task-1', date: '2024-01-15', duration: 5000 }, + ]); + + service.decrementPending(action); + + expect(service.getPendingTaskTimeEntries()).toEqual([]); + }); }); describe('extractEntityChanges', () => { diff --git a/src/app/op-log/capture/operation-capture.service.ts b/src/app/op-log/capture/operation-capture.service.ts index c4ff05963f..4e8df96d80 100644 --- a/src/app/op-log/capture/operation-capture.service.ts +++ b/src/app/op-log/capture/operation-capture.service.ts @@ -2,6 +2,7 @@ import { Injectable } from '@angular/core'; import { EntityChange, OpType } from '../core/operation.types'; import { PersistentAction } from '../core/persistent-action.interface'; import { OpLog } from '../../core/log'; +import { BatchedTimeSyncEntry } from '../../core/util/batched-time-sync-accumulator'; /** * Tracks how many local actions have been captured but not yet written to the @@ -46,6 +47,13 @@ export class OperationCaptureService { */ private pendingCount = 0; + /** + * Task-time actions update live task state before their operation is durable. + * Keep those deltas visible to snapshot projection until the matching write + * attempt completes, otherwise a snapshot can overlap its later tail op. + */ + private pendingTaskTimeEntries = new Map(); + /** * Tracks if we've already warned about the pending counter growing large, * to avoid log spam. @@ -58,6 +66,12 @@ export class OperationCaptureService { */ incrementPending(action: PersistentAction): void { this.pendingCount++; + const taskTimeEntry = this._getTaskTimeEntry(action); + if (taskTimeEntry) { + const entries = this.pendingTaskTimeEntries.get(action) ?? []; + entries.push(taskTimeEntry); + this.pendingTaskTimeEntries.set(action, entries); + } // Warn if the counter is growing large (indicates the effect is not keeping up) if ( @@ -82,7 +96,7 @@ export class OperationCaptureService { * failure). Called from the persist effect's `finally`, so a thrown write — * e.g. a lock-acquisition timeout — can never leak a pending entry (#8306). */ - decrementPending(): void { + decrementPending(action?: PersistentAction): void { if (this.pendingCount <= 0) { // Underflow guard: a decrement with no matching increment. This is only // reachable in the degenerate window before the meta-reducer service is @@ -96,6 +110,14 @@ export class OperationCaptureService { } this.pendingCount--; + if (action) { + const entries = this.pendingTaskTimeEntries.get(action); + if (entries?.length === 1) { + this.pendingTaskTimeEntries.delete(action); + } else if (entries && entries.length > 1) { + entries.shift(); + } + } // Reset warning flag once the backlog drains so we can warn again if it refills if ( @@ -118,12 +140,53 @@ export class OperationCaptureService { return this.pendingCount; } + /** Returns task-time deltas that are captured but not yet durably written. */ + getPendingTaskTimeEntries(): BatchedTimeSyncEntry[] { + const totals = new Map(); + for (const entries of this.pendingTaskTimeEntries.values()) { + for (const entry of entries) { + const key = `${entry.id}\u0000${entry.date}`; + const existing = totals.get(key); + if (existing) { + existing.duration += entry.duration; + } else { + totals.set(key, { ...entry }); + } + } + } + return [...totals.values()]; + } + /** * Resets the pending counter (for testing and error recovery). */ clear(): void { this.pendingCount = 0; this.hasWarnedAboutPending = false; + this.pendingTaskTimeEntries.clear(); + } + + private _getTaskTimeEntry(action: PersistentAction): BatchedTimeSyncEntry | undefined { + if ( + action.type !== '[TimeTracking] Sync time spent' || + action.meta.entityType !== 'TASK' + ) { + return undefined; + } + + const actionPayload = action as unknown as Record; + const taskId = actionPayload['taskId']; + const date = actionPayload['date']; + const duration = actionPayload['duration']; + if ( + typeof taskId !== 'string' || + typeof date !== 'string' || + typeof duration !== 'number' || + !Number.isFinite(duration) + ) { + return undefined; + } + return { id: taskId, date, duration }; } /** @@ -231,7 +294,11 @@ export class OperationCaptureService { entityType: 'TASK', entityId: taskId, opType: OpType.Update, - changes: { taskId, date, duration }, + changes: { + taskId, + date, + duration, + }, }, ]; } diff --git a/src/app/op-log/capture/operation-log.effects.ts b/src/app/op-log/capture/operation-log.effects.ts index a7c30065ef..a3ddc5d4ae 100644 --- a/src/app/op-log/capture/operation-log.effects.ts +++ b/src/app/op-log/capture/operation-log.effects.ts @@ -153,7 +153,7 @@ export class OperationLogEffects implements DeferredLocalActionsPort { // shared effect stream is never torn down by a single failed write. OpLog.err('OperationLogEffects: persist failed (handled; stream preserved)', e); } finally { - this.operationCaptureService.decrementPending(); + this.operationCaptureService.decrementPending(action); } } diff --git a/src/app/op-log/clean-slate/clean-slate.service.spec.ts b/src/app/op-log/clean-slate/clean-slate.service.spec.ts index 011f8a5f20..03a7aa5065 100644 --- a/src/app/op-log/clean-slate/clean-slate.service.spec.ts +++ b/src/app/op-log/clean-slate/clean-slate.service.spec.ts @@ -9,6 +9,7 @@ import { OpLog } from '../../core/log'; import { OperationWriteFlushService } from '../sync/operation-write-flush.service'; import { LockService } from '../sync/lock.service'; import { LOCK_NAMES } from '../core/operation-log.const'; +import { TaskTimeSyncService } from '../../features/tasks/task-time-sync.service'; describe('CleanSlateService', () => { let service: CleanSlateService; @@ -16,6 +17,7 @@ describe('CleanSlateService', () => { let mockOpLogStore: jasmine.SpyObj; let mockOperationWriteFlushService: jasmine.SpyObj; let mockLockService: jasmine.SpyObj; + let mockTaskTimeSyncService: jasmine.SpyObj; const mockState = { task: { ids: [], entities: {} }, @@ -26,7 +28,7 @@ describe('CleanSlateService', () => { beforeEach(() => { mockStateSnapshotService = jasmine.createSpyObj('StateSnapshotService', [ - 'getStateSnapshotAsync', + 'getStateSnapshotForOperationLogAsync', ]); mockOpLogStore = jasmine.createSpyObj('OperationLogStoreService', [ 'runDestructiveStateReplacement', @@ -37,6 +39,7 @@ describe('CleanSlateService', () => { 'flushPendingWrites', ]); mockLockService = jasmine.createSpyObj('LockService', ['request']); + mockTaskTimeSyncService = jasmine.createSpyObj('TaskTimeSyncService', ['flush']); TestBed.configureTestingModule({ providers: [ @@ -48,13 +51,16 @@ describe('CleanSlateService', () => { useValue: mockOperationWriteFlushService, }, { provide: LockService, useValue: mockLockService }, + { provide: TaskTimeSyncService, useValue: mockTaskTimeSyncService }, ], }); service = TestBed.inject(CleanSlateService); // Setup default mock responses - mockStateSnapshotService.getStateSnapshotAsync.and.resolveTo(mockState as any); + mockStateSnapshotService.getStateSnapshotForOperationLogAsync.and.resolveTo( + mockState as any, + ); mockOpLogStore.runDestructiveStateReplacement.and.resolveTo(); mockOpLogStore.getVectorClock.and.resolveTo(null); mockOpLogStore.getUnsynced.and.resolveTo([]); @@ -67,7 +73,9 @@ describe('CleanSlateService', () => { await service.createCleanSlate('ENCRYPTION_CHANGE', 'PASSWORD_CHANGED'); // Should get current state (async version to include archives) - expect(mockStateSnapshotService.getStateSnapshotAsync).toHaveBeenCalled(); + expect( + mockStateSnapshotService.getStateSnapshotForOperationLogAsync, + ).toHaveBeenCalled(); // Should route through the atomic helper (issues #7709, #7732) expect(mockOpLogStore.runDestructiveStateReplacement).toHaveBeenCalledTimes(1); @@ -91,16 +99,21 @@ describe('CleanSlateService', () => { mockOperationWriteFlushService.flushPendingWrites.and.callFake(async () => { callOrder.push('flush'); }); + mockTaskTimeSyncService.flush.and.callFake(() => { + callOrder.push('flush-task-time'); + }); mockLockService.request.and.callFake(async (lockName, fn) => { callOrder.push(`lock:${lockName}`); const r = await fn(); callOrder.push('unlock'); return r; }); - mockStateSnapshotService.getStateSnapshotAsync.and.callFake(async () => { - callOrder.push('snapshot'); - return mockState as any; - }); + mockStateSnapshotService.getStateSnapshotForOperationLogAsync.and.callFake( + async () => { + callOrder.push('snapshot'); + return mockState as any; + }, + ); mockOpLogStore.runDestructiveStateReplacement.and.callFake(async () => { callOrder.push('replace'); }); @@ -108,6 +121,7 @@ describe('CleanSlateService', () => { await service.createCleanSlate('ENCRYPTION_CHANGE', 'PASSWORD_CHANGED'); expect(callOrder).toEqual([ + 'flush-task-time', 'flush', `lock:${LOCK_NAMES.OPERATION_LOG}`, 'snapshot', @@ -200,7 +214,7 @@ describe('CleanSlateService', () => { }); it('should throw if state snapshot fails', async () => { - mockStateSnapshotService.getStateSnapshotAsync.and.rejectWith( + mockStateSnapshotService.getStateSnapshotForOperationLogAsync.and.rejectWith( new Error('State error'), ); diff --git a/src/app/op-log/clean-slate/clean-slate.service.ts b/src/app/op-log/clean-slate/clean-slate.service.ts index 363d3fa9b5..3e41c36174 100644 --- a/src/app/op-log/clean-slate/clean-slate.service.ts +++ b/src/app/op-log/clean-slate/clean-slate.service.ts @@ -11,6 +11,7 @@ import { extractEntityKeysFromState } from '../persistence/extract-entity-keys'; import { OperationWriteFlushService } from '../sync/operation-write-flush.service'; import { LockService } from '../sync/lock.service'; import { LOCK_NAMES } from '../core/operation-log.const'; +import { TaskTimeSyncService } from '../../features/tasks/task-time-sync.service'; /** * Reason a clean-slate was triggered. Logged for diagnostic correlation @@ -47,6 +48,7 @@ export class CleanSlateService { private opLogStore = inject(OperationLogStoreService); private operationWriteFlushService = inject(OperationWriteFlushService); private lockService = inject(LockService); + private taskTimeSyncService = inject(TaskTimeSyncService); /** * Creates a clean slate by resetting local operation log and preparing @@ -67,6 +69,10 @@ export class CleanSlateService { reason: CleanSlateReason, syncImportReason: SyncImportReason, ): Promise { + // Move the current timer batch into the op log before fixing the new + // baseline. Deltas that arrive after this flush remain projected out and + // are persisted as tail operations after the replacement. + this.taskTimeSyncService.flush(); await this.operationWriteFlushService.flushPendingWrites(); const { syncImportId } = await this.lockService.request( @@ -98,7 +104,8 @@ export class CleanSlateService { // IMPORTANT: must use the async version to load real archives from // IndexedDB. The sync getStateSnapshot() returns DEFAULT_ARCHIVE (empty) // which causes data loss. - const currentState = await this.stateSnapshotService.getStateSnapshotAsync(); + const currentState = + await this.stateSnapshotService.getStateSnapshotForOperationLogAsync(); // Mint a fresh clientId for the new sync baseline. It is pure here — // persisted only inside runDestructiveStateReplacement's atomic diff --git a/src/app/op-log/persistence/operation-log-compaction.service.spec.ts b/src/app/op-log/persistence/operation-log-compaction.service.spec.ts index 57506d68c8..140493cb41 100644 --- a/src/app/op-log/persistence/operation-log-compaction.service.spec.ts +++ b/src/app/op-log/persistence/operation-log-compaction.service.spec.ts @@ -46,6 +46,7 @@ describe('OperationLogCompactionService', () => { mockLockService = jasmine.createSpyObj('LockService', ['request']); mockStateSnapshot = jasmine.createSpyObj('StateSnapshotService', [ 'getStateSnapshot', + 'getStateSnapshotForOperationLog', ]); mockVectorClockService = jasmine.createSpyObj('VectorClockService', [ 'getCurrentVectorClock', @@ -67,6 +68,9 @@ describe('OperationLogCompactionService', () => { mockOpLogStore.deleteOpsWhere.and.returnValue(Promise.resolve()); mockOpLogStore.getPendingRemoteOps.and.resolveTo([]); mockStateSnapshot.getStateSnapshot.and.returnValue(mockState); + mockStateSnapshot.getStateSnapshotForOperationLog.and.callFake(() => + mockStateSnapshot.getStateSnapshot(), + ); mockVectorClockService.getCurrentVectorClock.and.returnValue( Promise.resolve(mockVectorClock), ); @@ -98,7 +102,7 @@ describe('OperationLogCompactionService', () => { it('should get current state from store delegate', async () => { await service.compact(); - expect(mockStateSnapshot.getStateSnapshot).toHaveBeenCalled(); + expect(mockStateSnapshot.getStateSnapshotForOperationLog).toHaveBeenCalled(); }); it('should log metrics if compaction is slow', async () => { diff --git a/src/app/op-log/persistence/operation-log-compaction.service.ts b/src/app/op-log/persistence/operation-log-compaction.service.ts index 0f9f80714c..218d22ebb9 100644 --- a/src/app/op-log/persistence/operation-log-compaction.service.ts +++ b/src/app/op-log/persistence/operation-log-compaction.service.ts @@ -74,7 +74,7 @@ export class OperationLogCompactionService { } // 1. Get current state from NgRx store - const currentState = this.stateSnapshot.getStateSnapshot(); + const currentState = this.stateSnapshot.getStateSnapshotForOperationLog(); this.checkCompactionTimeout(startTime, `${label}state snapshot`); // GUARD (#7892): never compact against an empty/degraded state. Compaction diff --git a/src/app/op-log/persistence/operation-log-snapshot.service.spec.ts b/src/app/op-log/persistence/operation-log-snapshot.service.spec.ts index 521b6fe2cc..cb550556ef 100644 --- a/src/app/op-log/persistence/operation-log-snapshot.service.spec.ts +++ b/src/app/op-log/persistence/operation-log-snapshot.service.spec.ts @@ -45,7 +45,11 @@ describe('OperationLogSnapshotService', () => { ]); mockStateSnapshotService = jasmine.createSpyObj('StateSnapshotService', [ 'getStateSnapshot', + 'getStateSnapshotForOperationLog', ]); + mockStateSnapshotService.getStateSnapshotForOperationLog.and.callFake(() => + mockStateSnapshotService.getStateSnapshot(), + ); mockSchemaMigrationService = jasmine.createSpyObj('SchemaMigrationService', [ 'migrateStateIfNeeded', ]); diff --git a/src/app/op-log/persistence/operation-log-snapshot.service.ts b/src/app/op-log/persistence/operation-log-snapshot.service.ts index 63a074a0bc..4dac9eb706 100644 --- a/src/app/op-log/persistence/operation-log-snapshot.service.ts +++ b/src/app/op-log/persistence/operation-log-snapshot.service.ts @@ -104,7 +104,7 @@ export class OperationLogSnapshotService { // NOTE: compaction reads in the opposite order (state, then lastSeq); // its failure mode if the lock is bypassed is missed-op data loss. const lastSeq = await this.opLogStore.getLastSeq(); - const currentState = this.stateSnapshotService.getStateSnapshot(); + const currentState = this.stateSnapshotService.getStateSnapshotForOperationLog(); // GUARD (#7892): never cache an empty/degraded state over a good one. // The snapshot is only a load-time cache — the op-log is the source of diff --git a/src/app/op-log/sync-providers/file-based/file-based-sync-adapter.service.spec.ts b/src/app/op-log/sync-providers/file-based/file-based-sync-adapter.service.spec.ts index afd2831209..9fa6d590e3 100644 --- a/src/app/op-log/sync-providers/file-based/file-based-sync-adapter.service.spec.ts +++ b/src/app/op-log/sync-providers/file-based/file-based-sync-adapter.service.spec.ts @@ -149,6 +149,7 @@ describe('FileBasedSyncAdapterService', () => { mockStateSnapshotService = jasmine.createSpyObj('StateSnapshotService', [ 'getStateSnapshot', + 'getStateSnapshotForOperationLog', ]); mockStateSnapshotService.getStateSnapshot.and.returnValue({ tasks: [], @@ -164,6 +165,9 @@ describe('FileBasedSyncAdapterService', () => { }, }, } as any); + mockStateSnapshotService.getStateSnapshotForOperationLog.and.callFake(() => + mockStateSnapshotService.getStateSnapshot(), + ); mockSnackService = jasmine.createSpyObj('SnackService', ['open']); @@ -816,7 +820,7 @@ describe('FileBasedSyncAdapterService', () => { }); describe('uploadSnapshot', () => { - it('should create new sync file with state from getStateSnapshot (not the passed parameter)', async () => { + it('should create new sync file from the state captured by the upload boundary', async () => { mockProvider.downloadFile.and.throwError( new RemoteFileNotFoundAPIError('sync-data.json'), ); @@ -827,9 +831,17 @@ describe('FileBasedSyncAdapterService', () => { return Promise.resolve({ rev: 'rev-1' }); }); - // The passed state should be IGNORED — file adapter uses getStateSnapshot() instead - // to prevent double-encryption when the upload service encrypts the payload. - const passedState = { tasks: [{ id: 't1', title: 'Test' }] }; + const passedState = { + tasks: [{ id: 't1', title: 'Test' }], + globalConfig: { + sync: { + syncProvider: SyncProviderId.WebDAV, + syncInterval: 300000, + isManualSyncOnly: true, + isCompressionEnabled: true, + }, + }, + }; const vectorClock = { client1: 5 }; const result = await adapter.uploadSnapshot( @@ -843,10 +855,12 @@ describe('FileBasedSyncAdapterService', () => { ); expect(result.accepted).toBe(true); + expect( + mockStateSnapshotService.getStateSnapshotForOperationLog, + ).not.toHaveBeenCalled(); const uploadedData = parseWithPrefix(uploadedDataStr); - // State should come from getStateSnapshot(), not the passed parameter expect(uploadedData.state).toEqual( - jasmine.objectContaining({ tasks: [], projects: [] }) as any, + jasmine.objectContaining({ tasks: passedState.tasks }) as any, ); const uploadedState = uploadedData.state as Record; const globalConfig = uploadedState['globalConfig'] as Record; diff --git a/src/app/op-log/sync-providers/file-based/file-based-sync-adapter.service.ts b/src/app/op-log/sync-providers/file-based/file-based-sync-adapter.service.ts index ed6ddbc816..227c7599f3 100644 --- a/src/app/op-log/sync-providers/file-based/file-based-sync-adapter.service.ts +++ b/src/app/op-log/sync-providers/file-based/file-based-sync-adapter.service.ts @@ -341,6 +341,7 @@ export class FileBasedSyncAdapterService { ops: SyncOperation[], clientId: string, lastKnownServerSeq?: number, + localStateSnapshot?: unknown, ): Promise => { return this._uploadOps( provider, @@ -349,6 +350,7 @@ export class FileBasedSyncAdapterService { ops, clientId, lastKnownServerSeq, + localStateSnapshot, ); }, @@ -495,6 +497,7 @@ export class FileBasedSyncAdapterService { ops: SyncOperation[], clientId: string, currentSyncVersion: number, + localStateSnapshot?: unknown, ): Promise { const newSyncVersion = currentSyncVersion + 1; @@ -527,7 +530,7 @@ export class FileBasedSyncAdapterService { // Get current state from NgRx store - this keeps the snapshot up-to-date const currentState = stripLocalOnlySyncSettingsFromAppData( - await this._stateSnapshotService.getStateSnapshot(), + localStateSnapshot ?? this._stateSnapshotService.getStateSnapshotForOperationLog(), ); // Compute oldestOpSyncVersion from the first (oldest) op in mergedOps. @@ -655,6 +658,7 @@ export class FileBasedSyncAdapterService { ops: SyncOperation[], clientId: string, lastKnownServerSeq?: number, + localStateSnapshot?: unknown, ): Promise { const providerKey = this._getProviderKey(provider); @@ -662,7 +666,15 @@ export class FileBasedSyncAdapterService { // reached when the opt-in setting is ON. The single-file path below is // byte-for-byte unchanged when the setting is OFF (the default). if (this._isSplitSyncEnabled()) { - return this._uploadOpsSplit(provider, cfg, encryptKey, ops, clientId, providerKey); + return this._uploadOpsSplit( + provider, + cfg, + encryptKey, + ops, + clientId, + providerKey, + localStateSnapshot, + ); } // Step 1: Get current sync state @@ -696,6 +708,7 @@ export class FileBasedSyncAdapterService { ops, clientId, currentSyncVersion, + localStateSnapshot, ); // Step 2.5: Backup-before-overwrite (two-phase write). Copy the CURRENT remote @@ -1114,7 +1127,7 @@ export class FileBasedSyncAdapterService { provider: FileSyncProvider, cfg: EncryptAndCompressCfg, encryptKey: string | undefined, - _state: unknown, + state: unknown, clientId: string, reason: 'initial' | 'recovery' | 'migration', vectorClock: Record, @@ -1136,21 +1149,17 @@ export class FileBasedSyncAdapterService { vectorClock, schemaVersion, providerKey, + state, ); } // For snapshots, we start fresh with syncVersion = 1 const newSyncVersion = 1; - // Always use fresh state from the NgRx store instead of the passed `_state` parameter. - // 1. Consistency: _buildMergedSyncData also uses getStateSnapshot() for its state. - // 2. Freshness: the passed state may come from an op payload created earlier in the - // sync cycle, so fetching directly ensures we capture the latest store state. - // Note: double-encryption is not a concern here — file-based providers don't expose - // getEncryptKey, so the upload service never applies payload-level encryption for them. - const currentState = stripLocalOnlySyncSettingsFromAppData( - await this._stateSnapshotService.getStateSnapshot(), - ); + // The caller captures this state at the same boundary as the full-state op. + // Re-reading the live store here would let later timer deltas leak into a + // snapshot whose vector clock does not cover them. + const currentState = stripLocalOnlySyncSettingsFromAppData(state); // Load archive data from IndexedDB to include in snapshot const archiveYoung = await this._archiveDbAdapter.loadArchiveYoung(); @@ -1481,15 +1490,16 @@ export class FileBasedSyncAdapterService { return null; } - /** Builds a full snapshot (`sync-state.json` payload) from the live store. */ + /** Builds a full snapshot (`sync-state.json` payload). */ private async _buildStateFileData( clientId: string, syncVersion: number, vectorClock: VectorClock, schemaVersion: number, + localStateSnapshot?: unknown, ): Promise { const state = stripLocalOnlySyncSettingsFromAppData( - await this._stateSnapshotService.getStateSnapshot(), + localStateSnapshot ?? this._stateSnapshotService.getStateSnapshotForOperationLog(), ); const archiveYoung = await this._archiveDbAdapter.loadArchiveYoung(); const archiveOld = await this._archiveDbAdapter.loadArchiveOld(); @@ -1708,6 +1718,7 @@ export class FileBasedSyncAdapterService { ops: SyncOperation[], clientId: string, providerKey: string, + localStateSnapshot?: unknown, ): Promise { let opsFile: FileBasedOpsFile | null = null; let opsRev: string | null = null; @@ -1783,6 +1794,7 @@ export class FileBasedSyncAdapterService { newSyncVersion, mergedClock, schemaVersion, + localStateSnapshot, ); // Preserve the pre-compaction snapshot for snapshotRef-mismatch recovery. await this._backupStateFile(provider, cfg, encryptKey); @@ -2099,6 +2111,7 @@ export class FileBasedSyncAdapterService { vectorClock: Record, schemaVersion: number, providerKey: string, + state: unknown, ): Promise { const newSyncVersion = 1; const clock = vectorClock as VectorClock; @@ -2108,6 +2121,7 @@ export class FileBasedSyncAdapterService { newSyncVersion, clock, schemaVersion, + state, ); // sync-state.json.bak is deliberately NOT refreshed here: its adoption is // ref-validated (_loadValidatedSnapshot requires an EQUAL clock against the diff --git a/src/app/op-log/sync/conflict-resolution.service.spec.ts b/src/app/op-log/sync/conflict-resolution.service.spec.ts index c576f2f4e8..7cc6a3e389 100644 --- a/src/app/op-log/sync/conflict-resolution.service.spec.ts +++ b/src/app/op-log/sync/conflict-resolution.service.spec.ts @@ -3579,6 +3579,33 @@ describe('ConflictResolutionService', () => { expect(mockStore.select).not.toHaveBeenCalled(); }); + it('should keep concurrent additive task-time deltas non-conflicting', async () => { + const remoteOp: Operation = { + ...createMockOp('remote-time', 'clientB'), + actionType: ActionType.TIME_TRACKING_SYNC_TIME_SPENT, + entityId: 'task-1', + payload: { taskId: 'task-1', date: '2024-01-15', duration: 3000 }, + vectorClock: { clientB: 1 }, + }; + const localOp: Operation = { + ...createMockOp('local-time', 'clientA'), + actionType: ActionType.TIME_TRACKING_SYNC_TIME_SPENT, + entityId: 'task-1', + payload: { taskId: 'task-1', date: '2024-01-15', duration: 2000 }, + vectorClock: { clientA: 1 }, + }; + const localPendingOpsByEntity = new Map([ + ['TASK:task-1', [localOp]], + ]); + + const result = await service.checkOpForConflicts( + remoteOp, + buildCtx({ localPendingOpsByEntity }), + ); + + expect(result).toEqual({ isSupersededOrDuplicate: false, conflict: null }); + }); + it('should use entityId fallback when entityIds is an empty array', async () => { // Regression test: entityIds: [] is truthy in JS, so the old || fallback // would use the empty array instead of falling back to entityId. diff --git a/src/app/op-log/sync/conflict-resolution.service.ts b/src/app/op-log/sync/conflict-resolution.service.ts index d262f56d1f..a31bd7a94b 100644 --- a/src/app/op-log/sync/conflict-resolution.service.ts +++ b/src/app/op-log/sync/conflict-resolution.service.ts @@ -1974,6 +1974,18 @@ export class ConflictResolutionService { // CONCURRENT = true conflict if (vcComparison === VectorClockComparison.CONCURRENT) { + // Task-time sync operations are positive deltas, so two concurrent timer + // batches commute. Sending them through entity-level LWW would discard one + // user's tracked time even though both can be applied safely. + if ( + remoteOp.actionType === ActionType.TIME_TRACKING_SYNC_TIME_SPENT && + ctx.localOpsForEntity.every( + (op) => op.actionType === ActionType.TIME_TRACKING_SYNC_TIME_SPENT, + ) + ) { + return { isSupersededOrDuplicate: false, conflict: null }; + } + const conflict: EntityConflict = { entityType: remoteOp.entityType, entityId, diff --git a/src/app/op-log/sync/operation-log-upload.service.spec.ts b/src/app/op-log/sync/operation-log-upload.service.spec.ts index ea4a810525..c7bb616106 100644 --- a/src/app/op-log/sync/operation-log-upload.service.spec.ts +++ b/src/app/op-log/sync/operation-log-upload.service.spec.ts @@ -11,11 +11,13 @@ import { EncryptNoPasswordError } from '../core/errors/sync-errors'; import { ActionType, OpType, OperationLogEntry } from '../core/operation.types'; import { SnackService } from '../../core/snack/snack.service'; import { provideMockStore } from '@ngrx/store/testing'; +import { StateSnapshotService } from '../backup/state-snapshot.service'; describe('OperationLogUploadService', () => { let service: OperationLogUploadService; let mockOpLogStore: jasmine.SpyObj; let mockLockService: jasmine.SpyObj; + let mockStateSnapshotService: jasmine.SpyObj; const createMockEntry = ( seq: number, @@ -48,6 +50,12 @@ describe('OperationLogUploadService', () => { 'deleteOpsWhere', ]); mockLockService = jasmine.createSpyObj('LockService', ['request']); + mockStateSnapshotService = jasmine.createSpyObj('StateSnapshotService', [ + 'getStateSnapshotForOperationLog', + ]); + mockStateSnapshotService.getStateSnapshotForOperationLog.and.returnValue({ + task: { ids: [], entities: {} }, + } as any); // Default mock implementations mockLockService.request.and.callFake(async (_name: string, fn: () => Promise) => @@ -63,6 +71,7 @@ describe('OperationLogUploadService', () => { provideMockStore(), { provide: OperationLogStoreService, useValue: mockOpLogStore }, { provide: LockService, useValue: mockLockService }, + { provide: StateSnapshotService, useValue: mockStateSnapshotService }, { provide: SnackService, useValue: jasmine.createSpyObj('SnackService', ['open']), @@ -284,6 +293,33 @@ describe('OperationLogUploadService', () => { ); }); + it('should capture and pass file state under the operation-log lock', async () => { + mockApiProvider.providerMode = 'fileSnapshotOps'; + mockOpLogStore.getUnsynced.and.returnValue( + Promise.resolve([createMockEntry(1, 'op-1', 'client-1')]), + ); + mockApiProvider.uploadOps.and.returnValue( + Promise.resolve({ + results: [{ opId: 'op-1', accepted: true }], + latestSeq: 1, + newOps: [], + }), + ); + + await service.uploadPendingOps(mockApiProvider); + + const operationLockCall = mockLockService.request.calls + .allArgs() + .find(([name]) => name === 'sp_op_log'); + expect(operationLockCall).toBeDefined(); + expect( + mockStateSnapshotService.getStateSnapshotForOperationLog, + ).toHaveBeenCalled(); + expect(mockApiProvider.uploadOps.calls.mostRecent().args[3]).toEqual({ + task: { ids: [], entities: {} }, + } as any); + }); + it('should return empty result when no pending ops', async () => { mockOpLogStore.getUnsynced.and.returnValue(Promise.resolve([])); @@ -1687,10 +1723,10 @@ describe('OperationLogUploadService', () => { const callOrder: string[] = []; mockLockService.request.and.callFake( - async (_name: string, fn: () => Promise) => { - callOrder.push('lock-acquired'); + async (name: string, fn: () => Promise) => { + callOrder.push(`${name}-acquired`); const r = await fn(); - callOrder.push('lock-released'); + callOrder.push(`${name}-released`); return r; }, ); @@ -1702,11 +1738,14 @@ describe('OperationLogUploadService', () => { await service.uploadPendingOps(mockApiProvider, { preUploadCallback: callback }); expect(callback).toHaveBeenCalled(); - // Verify callback was called INSIDE the lock + // The callback owns its operation-log transaction; this service keeps + // it inside upload serialization and then captures the upload boundary. expect(callOrder).toEqual([ - 'lock-acquired', + 'sp_op_log_upload-acquired', 'callback-executed', - 'lock-released', + 'sp_op_log-acquired', + 'sp_op_log-released', + 'sp_op_log_upload-released', ]); }); @@ -1775,6 +1814,7 @@ describe('OperationLogUploadService', () => { [jasmine.objectContaining(callbackCreatedEntry.op)], 'client-1', jasmine.any(Number), + undefined, ); }); }); diff --git a/src/app/op-log/sync/operation-log-upload.service.ts b/src/app/op-log/sync/operation-log-upload.service.ts index e6281604fb..31d71fbbb3 100644 --- a/src/app/op-log/sync/operation-log-upload.service.ts +++ b/src/app/op-log/sync/operation-log-upload.service.ts @@ -40,6 +40,7 @@ import { stripLocalOnlySyncScheduleSettings, stripLocalOnlySyncSettingsFromAppData, } from '../../features/config/local-only-sync-settings.util'; +import { StateSnapshotService } from '../backup/state-snapshot.service'; // Re-export for consumers that import from this service export type { @@ -63,6 +64,7 @@ export class OperationLogUploadService { private opLogStore = inject(OperationLogStoreService); private lockService = inject(LockService); private encryptionService = inject(OperationEncryptionService); + private stateSnapshotService = inject(StateSnapshotService); async uploadPendingOps( syncProvider: OperationSyncCapable, @@ -119,13 +121,28 @@ export class OperationLogUploadService { let encryptionRequiredKeyMissing = false; await this.lockService.request(LOCK_NAMES.UPLOAD, async () => { - // Execute pre-upload callback INSIDE the lock, BEFORE checking for pending ops. - // This ensures operations like server migration checks are atomic with the upload. + // Execute migration/recovery preparation while upload serialization is + // held. The migration service owns its own operation-log transaction. if (options?.preUploadCallback) { await options.preUploadCallback(); } - const pendingOps = await this.opLogStore.getUnsynced(); + // Fix the pending-op set and the file snapshot under the same operation-log + // boundary. A task-time action applied by reducers while waiting for this + // lock remains visible to snapshot projection as an in-flight delta. + const { pendingOps, localStateSnapshot } = await this.lockService.request( + LOCK_NAMES.OPERATION_LOG, + async () => { + const capturedPendingOps = await this.opLogStore.getUnsynced(); + return { + pendingOps: capturedPendingOps, + localStateSnapshot: + syncProvider.providerMode === 'fileSnapshotOps' + ? this.stateSnapshotService.getStateSnapshotForOperationLog() + : undefined, + }; + }, + ); selectedPendingOps = pendingOps; if (pendingOps.length === 0) { @@ -330,8 +347,17 @@ export class OperationLogUploadService { } // Upload in batches to avoid 413 Payload Too Large errors - const chunks = chunkArray(syncOps, MAX_OPS_PER_UPLOAD_REQUEST); - const correspondingEntries = chunkArray(uploadEntries, MAX_OPS_PER_UPLOAD_REQUEST); + // A file upload embeds one full snapshot. Keep its atomically captured op + // set in one request so a partial chunk failure cannot publish state that + // already contains operations left for a later retry. + const chunks = + syncProvider.providerMode === 'fileSnapshotOps' + ? [syncOps] + : chunkArray(syncOps, MAX_OPS_PER_UPLOAD_REQUEST); + const correspondingEntries = + syncProvider.providerMode === 'fileSnapshotOps' + ? [uploadEntries] + : chunkArray(uploadEntries, MAX_OPS_PER_UPLOAD_REQUEST); for (let i = 0; i < chunks.length; i++) { const chunk = chunks[i]; @@ -343,7 +369,12 @@ export class OperationLogUploadService { let response; try { - response = await syncProvider.uploadOps(chunk, clientId, lastKnownServerSeq); + response = await syncProvider.uploadOps( + chunk, + clientId, + lastKnownServerSeq, + localStateSnapshot, + ); } catch (err) { const message = err instanceof Error ? err.message : 'Unknown error'; OpLog.error(`OperationLogUploadService: Upload failed: ${message}`); diff --git a/src/app/op-log/sync/server-migration.service.spec.ts b/src/app/op-log/sync/server-migration.service.spec.ts index 03bbaa7adf..d3d496500b 100644 --- a/src/app/op-log/sync/server-migration.service.spec.ts +++ b/src/app/op-log/sync/server-migration.service.spec.ts @@ -85,7 +85,11 @@ describe('ServerMigrationService', () => { stateSnapshotServiceSpy = jasmine.createSpyObj('StateSnapshotService', [ 'getStateSnapshot', 'getStateSnapshotAsync', + 'getStateSnapshotForOperationLogAsync', ]); + stateSnapshotServiceSpy.getStateSnapshotForOperationLogAsync.and.callFake(() => + stateSnapshotServiceSpy.getStateSnapshotAsync(), + ); snackServiceSpy = jasmine.createSpyObj('SnackService', ['open']); clientIdProviderSpy = jasmine.createSpyObj('ClientIdProvider', ['loadClientId']); matDialogSpy = jasmine.createSpyObj('MatDialog', ['open']); @@ -261,6 +265,15 @@ describe('ServerMigrationService', () => { }); describe('handleServerMigration', () => { + it('should create the snapshot and import under the operation-log lock', async () => { + await service.handleServerMigration(defaultProvider); + + expect(lockServiceSpy.request).toHaveBeenCalledWith( + 'sp_op_log', + jasmine.any(Function), + ); + }); + it('should skip if state is empty (no tasks/projects/tags)', async () => { stateSnapshotServiceSpy.getStateSnapshotAsync.and.returnValue( Promise.resolve({ diff --git a/src/app/op-log/sync/server-migration.service.ts b/src/app/op-log/sync/server-migration.service.ts index b58985fdd1..2465c74055 100644 --- a/src/app/op-log/sync/server-migration.service.ts +++ b/src/app/op-log/sync/server-migration.service.ts @@ -211,7 +211,7 @@ export class ServerMigrationService { // Get current full state from NgRx store (async to include archives from IndexedDB) // Cast to Record for validation compatibility let currentState: Record = - (await this.stateSnapshotService.getStateSnapshotAsync()) as unknown as Record< + (await this.stateSnapshotService.getStateSnapshotForOperationLogAsync()) as unknown as Record< string, unknown >; diff --git a/src/app/op-log/testing/integration/clean-slate-interrupt.integration.spec.ts b/src/app/op-log/testing/integration/clean-slate-interrupt.integration.spec.ts index 9b82175cc4..a9ad363b93 100644 --- a/src/app/op-log/testing/integration/clean-slate-interrupt.integration.spec.ts +++ b/src/app/op-log/testing/integration/clean-slate-interrupt.integration.spec.ts @@ -7,6 +7,7 @@ import { TranslateService } from '@ngx-translate/core'; import { CURRENT_SCHEMA_VERSION } from '../../persistence/schema-migration.service'; import { Operation } from '../../core/operation.types'; import { SINGLETON_KEY, STORE_NAMES } from '../../persistence/db-keys.const'; +import { TaskTimeSyncService } from '../../../features/tasks/task-time-sync.service'; /** * Integration tests for issue #7709 — `createCleanSlate` / `BackupService` import @@ -30,6 +31,7 @@ describe('CleanSlate / Backup interrupt (issue #7709 regression)', () => { let cleanSlate: CleanSlateService; let mockStateSnapshot: jasmine.SpyObj; let mockTranslate: jasmine.SpyObj; + let mockTaskTimeSync: jasmine.SpyObj; const meaningfulState = { task: { @@ -46,10 +48,13 @@ describe('CleanSlate / Backup interrupt (issue #7709 regression)', () => { beforeEach(async () => { mockStateSnapshot = jasmine.createSpyObj('StateSnapshotService', [ 'getStateSnapshot', - 'getStateSnapshotAsync', + 'getStateSnapshotForOperationLogAsync', ]); mockStateSnapshot.getStateSnapshot.and.returnValue(meaningfulState as any); - mockStateSnapshot.getStateSnapshotAsync.and.resolveTo(meaningfulState as any); + mockStateSnapshot.getStateSnapshotForOperationLogAsync.and.resolveTo( + meaningfulState as any, + ); + mockTaskTimeSync = jasmine.createSpyObj('TaskTimeSyncService', ['flush']); mockTranslate = jasmine.createSpyObj('TranslateService', ['instant']); mockTranslate.instant.and.callFake((k: string) => k); @@ -60,6 +65,7 @@ describe('CleanSlate / Backup interrupt (issue #7709 regression)', () => { SyncLocalStateService, CleanSlateService, { provide: StateSnapshotService, useValue: mockStateSnapshot }, + { provide: TaskTimeSyncService, useValue: mockTaskTimeSync }, { provide: TranslateService, useValue: mockTranslate }, ], }); diff --git a/src/app/op-log/testing/integration/compaction.integration.spec.ts b/src/app/op-log/testing/integration/compaction.integration.spec.ts index ab0a99a82a..462d70c979 100644 --- a/src/app/op-log/testing/integration/compaction.integration.spec.ts +++ b/src/app/op-log/testing/integration/compaction.integration.spec.ts @@ -39,6 +39,7 @@ describe('Compaction Integration', () => { // Create mock for StateSnapshotService mockStateSnapshot = jasmine.createSpyObj('StateSnapshotService', [ 'getStateSnapshot', + 'getStateSnapshotForOperationLog', ]); // Default mock return value - cast to any since we only need partial data for tests @@ -49,6 +50,9 @@ describe('Compaction Integration', () => { note: { ids: [], entities: {} }, globalConfig: {}, } as any); + mockStateSnapshot.getStateSnapshotForOperationLog.and.callFake(() => + mockStateSnapshot.getStateSnapshot(), + ); TestBed.configureTestingModule({ providers: [ diff --git a/src/app/op-log/testing/integration/helpers/file-based-sync-test-harness.ts b/src/app/op-log/testing/integration/helpers/file-based-sync-test-harness.ts index 34af6570f0..006a950188 100644 --- a/src/app/op-log/testing/integration/helpers/file-based-sync-test-harness.ts +++ b/src/app/op-log/testing/integration/helpers/file-based-sync-test-harness.ts @@ -114,6 +114,10 @@ class MockStateSnapshotService { return this._state; } + getStateSnapshotForOperationLog(): unknown { + return this._state; + } + setState(state: unknown): void { this._state = state; } diff --git a/src/app/op-log/testing/integration/performance.integration.spec.ts b/src/app/op-log/testing/integration/performance.integration.spec.ts index 8ba26fb193..dcb89a0855 100644 --- a/src/app/op-log/testing/integration/performance.integration.spec.ts +++ b/src/app/op-log/testing/integration/performance.integration.spec.ts @@ -35,12 +35,16 @@ describe('Performance Integration', () => { beforeEach(async () => { mockStateSnapshot = jasmine.createSpyObj('StateSnapshotService', [ 'getStateSnapshot', + 'getStateSnapshotForOperationLog', 'getAllSyncModelDataFromStore', ]); mockStateSnapshot.getStateSnapshot.and.returnValue({ task: { ids: [], entities: {} }, project: { ids: [], entities: {} }, } as any); + mockStateSnapshot.getStateSnapshotForOperationLog.and.callFake(() => + mockStateSnapshot.getStateSnapshot(), + ); mockStateSnapshot.getAllSyncModelDataFromStore.and.returnValue({ task: { ids: [], entities: {} }, project: { ids: [], entities: {} }, diff --git a/src/app/op-log/testing/integration/task-time-replay-state-machine.integration.spec.ts b/src/app/op-log/testing/integration/task-time-replay-state-machine.integration.spec.ts new file mode 100644 index 0000000000..1ba174a15f --- /dev/null +++ b/src/app/op-log/testing/integration/task-time-replay-state-machine.integration.spec.ts @@ -0,0 +1,313 @@ +import { Action } from '@ngrx/store'; +import { TestBed } from '@angular/core/testing'; +import { MockStore, provideMockStore } from '@ngrx/store/testing'; +import { BatchedTimeSyncEntry } from '../../../core/util/batched-time-sync-accumulator'; +import { TimeTrackingActions } from '../../../features/time-tracking/store/time-tracking.actions'; +import { DEFAULT_TASK, Task, TaskState } from '../../../features/tasks/task.model'; +import { TaskTimeSyncService } from '../../../features/tasks/task-time-sync.service'; +import { + initialTaskState, + taskAdapter, + taskReducer, +} from '../../../features/tasks/store/task.reducer'; +import { updateTimeSpentForTask } from '../../../features/tasks/store/task.reducer.util'; +import { AppStateSnapshot } from '../../core/types/backup.types'; + +type ReplayEvent = + | { kind: 'delta'; entry: BatchedTimeSyncEntry } + | { kind: 'replace'; taskId: string; date: string; duration: number } + | { kind: 'delete'; taskId: string } + | { kind: 'create'; task: Task }; + +interface Checkpoint { + state: TaskState; + durableEventCount: number; +} + +const DATES = ['2026-07-13', '2026-07-14', '2026-07-15'] as const; + +const createTask = (id: string, timeSpentOnDay: Record = {}): Task => + ({ + ...DEFAULT_TASK, + id, + title: id, + created: 1, + projectId: 'INBOX_PROJECT', + timeSpentOnDay, + timeSpent: Object.values(timeSpentOnDay).reduce((total, value) => total + value, 0), + }) as Task; + +const createState = (tasks: Task[]): TaskState => ({ + ...initialTaskState, + ids: tasks.map((task) => task.id), + entities: Object.fromEntries(tasks.map((task) => [task.id, task])), +}); + +const applyReplayEvent = (state: TaskState, event: ReplayEvent): TaskState => { + switch (event.kind) { + case 'delta': { + const action = { + type: '[TimeTracking] Sync time spent', + taskId: event.entry.id, + date: event.entry.date, + duration: event.entry.duration, + meta: { + isPersistent: true, + isRemote: true, + entityType: 'TASK', + entityId: event.entry.id, + opType: 'UPD', + }, + }; + return taskReducer(state, action); + } + case 'replace': + return state.entities[event.taskId] + ? updateTimeSpentForTask(event.taskId, { [event.date]: event.duration }, state) + : state; + case 'delete': + return taskAdapter.removeOne(event.taskId, state); + case 'create': + return taskAdapter.addOne(event.task, state); + } +}; + +const replayFromCheckpoint = ( + checkpoint: Checkpoint, + durableEvents: ReplayEvent[], +): TaskState => + durableEvents + .slice(checkpoint.durableEventCount) + .reduce(applyReplayEvent, checkpoint.state); + +const comparableTaskTimes = ( + state: TaskState, +): Record }> => + Object.fromEntries( + (state.ids as string[]).map((id) => { + const task = state.entities[id]!; + return [ + id, + { + timeSpent: task.timeSpent, + timeSpentOnDay: { ...task.timeSpentOnDay }, + }, + ]; + }), + ); + +const createSeededRandom = (seed: number): (() => number) => { + let state = seed >>> 0; + return () => { + state += 0x6d2b79f5; + let value = state; + value = Math.imul(value ^ (value >>> 15), value | 1); + value ^= value + Math.imul(value ^ (value >>> 7), value | 61); + return (value ^ (value >>> 14)) >>> 0; + }; +}; + +describe('Task-time replay state machine integration', () => { + let taskTimeSync: TaskTimeSyncService; + let store: MockStore; + let dispatchedEntries: BatchedTimeSyncEntry[]; + + beforeEach(() => { + TestBed.configureTestingModule({ + providers: [TaskTimeSyncService, provideMockStore()], + }); + taskTimeSync = TestBed.inject(TaskTimeSyncService); + store = TestBed.inject(MockStore); + dispatchedEntries = []; + + spyOn(store, 'dispatch').and.callFake(((action: Action) => { + const candidate = action as Action & { + taskId?: unknown; + date?: unknown; + duration?: unknown; + }; + if ( + candidate.type === '[TimeTracking] Sync time spent' && + typeof candidate.taskId === 'string' && + typeof candidate.date === 'string' && + typeof candidate.duration === 'number' + ) { + dispatchedEntries.push({ + id: candidate.taskId, + date: candidate.date, + duration: candidate.duration, + }); + } + }) as typeof store.dispatch); + }); + + afterEach(() => taskTimeSync.clear()); + + it('keeps every durable delta exactly once across seeded snapshots, imports, and replay', () => { + const seeds = [0x8957, 0x1badb002, 0x5eed1234, 0xc0ffee, 0xdeadbeef]; + + for (const seed of seeds) { + taskTimeSync.clear(); + dispatchedEntries = []; + + let nextTaskNumber = 3; + let liveState = createState([ + createTask('task-1', { [DATES[0]]: 1000 }), + createTask('task-2', { [DATES[1]]: 2000 }), + ]); + let acceptedState = liveState; + let durableEvents: ReplayEvent[] = []; + let checkpoint: Checkpoint = { state: liveState, durableEventCount: 0 }; + const random = createSeededRandom(seed); + + const persistOne = (): void => { + const entry = dispatchedEntries.shift(); + if (!entry) return; + const event: ReplayEvent = { kind: 'delta', entry }; + durableEvents.push(event); + acceptedState = applyReplayEvent(acceptedState, event); + }; + + const persistAll = (): void => { + while (dispatchedEntries.length > 0) persistOne(); + }; + + const takeSnapshot = (): void => { + const projected = taskTimeSync.projectSnapshot( + { task: liveState } as AppStateSnapshot, + dispatchedEntries, + ); + checkpoint = { + state: projected.task as TaskState, + durableEventCount: durableEvents.length, + }; + }; + + const assertInvariant = (step: number): void => { + const context = `seed=${seed.toString(16)}, step=${step}`; + const projected = taskTimeSync.projectSnapshot( + { task: liveState } as AppStateSnapshot, + dispatchedEntries, + ).task as TaskState; + expect(comparableTaskTimes(projected)) + .withContext(`projected state diverged: ${context}`) + .toEqual(comparableTaskTimes(acceptedState)); + expect(comparableTaskTimes(replayFromCheckpoint(checkpoint, durableEvents))) + .withContext(`checkpoint replay diverged: ${context}`) + .toEqual(comparableTaskTimes(acceptedState)); + }; + + for (let step = 0; step < 120; step++) { + const activeTaskIds = liveState.ids as string[]; + const taskId = activeTaskIds[random() % activeTaskIds.length]; + const date = DATES[random() % DATES.length]; + const eventType = random() % 10; + + switch (eventType) { + case 0: + case 1: + case 2: { + const duration = (random() % 5000) + 1; + const task = liveState.entities[taskId]!; + liveState = taskReducer( + liveState, + TimeTrackingActions.addTimeSpent({ + task, + date, + duration, + isFromTrackingReminder: false, + }), + ); + taskTimeSync.accumulate(taskId, duration, date); + break; + } + case 3: + if (random() % 2 === 0) { + taskTimeSync.flushOne(taskId); + } else { + taskTimeSync.flush(); + } + break; + case 4: + persistOne(); + break; + case 5: + takeSnapshot(); + break; + case 6: { + const event: ReplayEvent = { + kind: 'delta', + entry: { id: taskId, date, duration: (random() % 5000) + 1 }, + }; + durableEvents.push(event); + acceptedState = applyReplayEvent(acceptedState, event); + liveState = applyReplayEvent(liveState, event); + break; + } + case 7: { + taskTimeSync.flushOne(taskId); + persistAll(); + const event: ReplayEvent = { + kind: 'replace', + taskId, + date, + duration: random() % 20000, + }; + durableEvents.push(event); + acceptedState = applyReplayEvent(acceptedState, event); + liveState = applyReplayEvent(liveState, event); + break; + } + case 8: { + if (random() % 2 === 0) { + taskTimeSync.clearOne(taskId); + persistAll(); + const deleteEvent: ReplayEvent = { kind: 'delete', taskId }; + durableEvents.push(deleteEvent); + acceptedState = applyReplayEvent(acceptedState, deleteEvent); + liveState = applyReplayEvent(liveState, deleteEvent); + + const newTask = createTask(`task-${nextTaskNumber++}`); + const createEvent: ReplayEvent = { kind: 'create', task: newTask }; + durableEvents.push(createEvent); + acceptedState = applyReplayEvent(acceptedState, createEvent); + liveState = applyReplayEvent(liveState, createEvent); + } else { + taskTimeSync.flush(); + persistAll(); + checkpoint = { state: liveState, durableEventCount: 0 }; + durableEvents = []; + } + break; + } + case 9: + if (random() % 3 === 0) { + taskTimeSync.clear(); + dispatchedEntries = []; + liveState = createState([ + createTask(`task-${nextTaskNumber++}`, { [date]: random() % 10000 }), + ]); + acceptedState = liveState; + durableEvents = []; + checkpoint = { state: liveState, durableEventCount: 0 }; + } else { + liveState = replayFromCheckpoint(checkpoint, durableEvents); + acceptedState = liveState; + taskTimeSync.clear(); + dispatchedEntries = []; + } + break; + } + + assertInvariant(step); + } + + taskTimeSync.flush(); + persistAll(); + assertInvariant(120); + expect(comparableTaskTimes(liveState)) + .withContext(`live state did not converge: seed=${seed.toString(16)}`) + .toEqual(comparableTaskTimes(acceptedState)); + } + }); +}); diff --git a/src/app/op-log/validation/validate-operation-payload.spec.ts b/src/app/op-log/validation/validate-operation-payload.spec.ts index 52b0e0fdac..3b8441fbf2 100644 --- a/src/app/op-log/validation/validate-operation-payload.spec.ts +++ b/src/app/op-log/validation/validate-operation-payload.spec.ts @@ -230,6 +230,7 @@ describe('validateOperationPayload', () => { it('should validate TASK UPDATE with syncTimeSpent shape (taskId, date, duration)', () => { const op = createTestOperation({ + actionType: ActionType.TIME_TRACKING_SYNC_TIME_SPENT, opType: OpType.Update, entityType: 'TASK' as EntityType, entityId: 'task-123', @@ -246,6 +247,7 @@ describe('validateOperationPayload', () => { it('should validate TASK UPDATE with syncTimeSpent shape with zero duration', () => { const op = createTestOperation({ + actionType: ActionType.TIME_TRACKING_SYNC_TIME_SPENT, opType: OpType.Update, entityType: 'TASK' as EntityType, entityId: 'task-456', @@ -260,6 +262,45 @@ describe('validateOperationPayload', () => { expect(result.warnings).toBeUndefined(); }); + it('should reject syncTimeSpent when taskId does not match the operation entity', () => { + const result = validateOperationPayload( + createTestOperation({ + actionType: ActionType.TIME_TRACKING_SYNC_TIME_SPENT, + entityId: 'task-123', + payload: { taskId: 'task-456', date: '2024-01-15', duration: 1000 }, + }), + ); + + expect(result.success).toBeFalse(); + expect(result.error).toContain('taskId'); + }); + + it('should reject syncTimeSpent with an impossible calendar date', () => { + const result = validateOperationPayload( + createTestOperation({ + actionType: ActionType.TIME_TRACKING_SYNC_TIME_SPENT, + entityId: 'task-123', + payload: { taskId: 'task-123', date: '2024-99-99', duration: 1000 }, + }), + ); + + expect(result.success).toBeFalse(); + expect(result.error).toContain('date'); + }); + + it('should reject syncTimeSpent with negative or non-finite duration', () => { + for (const duration of [-1, Number.POSITIVE_INFINITY, Number.NaN]) { + const result = validateOperationPayload( + createTestOperation({ + actionType: ActionType.TIME_TRACKING_SYNC_TIME_SPENT, + entityId: 'task-123', + payload: { taskId: 'task-123', date: '2024-01-15', duration }, + }), + ); + expect(result.success).withContext(`duration=${duration}`).toBeFalse(); + } + }); + it('should validate TIME_TRACKING UPDATE with syncTimeTracking shape', () => { const op = createTestOperation({ opType: OpType.Update, diff --git a/src/app/op-log/validation/validate-operation-payload.ts b/src/app/op-log/validation/validate-operation-payload.ts index f9d48dd9df..114cc72c4f 100644 --- a/src/app/op-log/validation/validate-operation-payload.ts +++ b/src/app/op-log/validation/validate-operation-payload.ts @@ -5,6 +5,7 @@ import { OpType, isMultiEntityPayload, EntityChange, + ActionType, } from '../core/operation.types'; import { getPayloadKey, @@ -14,6 +15,7 @@ import { import { isValidEntityId } from './is-valid-entity-id'; import type { SyncLogMeta } from '@sp/sync-core'; import { OP_LOG_SYNC_LOGGER } from '../core/sync-logger.adapter'; +import { isValidDBDateStr } from '../../util/get-db-date-str'; /** * Result of validating an operation payload. @@ -201,12 +203,35 @@ const validateCreatePayload = ( * Expects payload to contain entity ID and changes, or a task/entity with changes. */ const validateUpdatePayload = ( - entityType: EntityType, + op: Operation, payload: unknown, ): PayloadValidationResult => { + const entityType = op.entityType; const p = payload as Record; const warnings: string[] = []; + if (op.actionType === ActionType.TIME_TRACKING_SYNC_TIME_SPENT) { + const taskId = p['taskId']; + const date = p['date']; + const duration = p['duration']; + if (!isValidEntityId(taskId) || taskId !== op.entityId) { + return { + success: false, + error: 'syncTimeSpent taskId must match the operation entityId', + }; + } + if (typeof date !== 'string' || !isValidDBDateStr(date)) { + return { success: false, error: 'syncTimeSpent date must be a valid YYYY-MM-DD' }; + } + if (typeof duration !== 'number' || !Number.isFinite(duration) || duration < 0) { + return { + success: false, + error: 'syncTimeSpent duration must be a finite non-negative number', + }; + } + return { success: true }; + } + // TIME_TRACKING uses action-payload capture with specific shapes // that don't match standard entity update patterns (see operation-capture.service.ts) if (entityType === 'TIME_TRACKING') { @@ -221,12 +246,6 @@ const validateUpdatePayload = ( // Fall through to standard validation if unknown TIME_TRACKING shape } - // TASK time tracking updates use a special shape: { taskId, date, duration } - // This comes from syncTimeSpent action (see time-tracking.actions.ts) - if (entityType === 'TASK' && 'taskId' in p && 'date' in p && 'duration' in p) { - return { success: true }; - } - // convertToSubTask uses a compact intent shape rather than { task: { changes } }. // afterTaskId is the placement anchor: a task id or null (prepend). if ( @@ -609,7 +628,7 @@ export const validateOperationPayload = (op: Operation): PayloadValidationResult break; case OpType.Update: - result = validateUpdatePayload(op.entityType, actionPayload); + result = validateUpdatePayload(op, actionPayload); break; case OpType.Delete: diff --git a/src/app/op-log/validation/validate-state.service.spec.ts b/src/app/op-log/validation/validate-state.service.spec.ts index 9e0d249072..7df0357eb3 100644 --- a/src/app/op-log/validation/validate-state.service.spec.ts +++ b/src/app/op-log/validation/validate-state.service.spec.ts @@ -65,7 +65,7 @@ describe('ValidateStateService', () => { mockStateSnapshotService = jasmine.createSpyObj('StateSnapshotService', [ 'getStateSnapshot', - 'getStateSnapshotAsync', + 'getStateSnapshotForOperationLogAsync', ]); mockClientIdProvider = { loadClientId: jasmine @@ -233,7 +233,9 @@ describe('ValidateStateService', () => { const result = await service.validateAndRepairCurrentState('test-context'); expect(result).toBeTrue(); - expect(mockStateSnapshotService.getStateSnapshotAsync).not.toHaveBeenCalled(); + expect( + mockStateSnapshotService.getStateSnapshotForOperationLogAsync, + ).not.toHaveBeenCalled(); expect(mockRepairService.createRepairOperation).not.toHaveBeenCalled(); expect(store.dispatch).not.toHaveBeenCalled(); }); @@ -250,7 +252,9 @@ describe('ValidateStateService', () => { projectTree: [{ id: 'ORPHAN', k: MenuTreeKind.PROJECT }], }; mockStateSnapshotService.getStateSnapshot.and.returnValue(invalidState as any); - mockStateSnapshotService.getStateSnapshotAsync.and.resolveTo(invalidState as any); + mockStateSnapshotService.getStateSnapshotForOperationLogAsync.and.resolveTo( + invalidState as any, + ); mockClientIdProvider.loadClientId.and.returnValue(Promise.resolve(null)); const result = await service.validateAndRepairCurrentState('sync'); @@ -265,7 +269,9 @@ describe('ValidateStateService', () => { it('should pass skipLock option to repair service when callerHoldsLock is true', async () => { const state = createEmptyState(); mockStateSnapshotService.getStateSnapshot.and.returnValue(state as any); - mockStateSnapshotService.getStateSnapshotAsync.and.resolveTo(state as any); + mockStateSnapshotService.getStateSnapshotForOperationLogAsync.and.resolveTo( + state as any, + ); // Mock validateAndRepair to return repaired state spyOn(service, 'validateAndRepair').and.resolveTo({ @@ -288,7 +294,9 @@ describe('ValidateStateService', () => { it('should start/end hydration state for sync contexts', async () => { const state = createEmptyState(); mockStateSnapshotService.getStateSnapshot.and.returnValue(state as any); - mockStateSnapshotService.getStateSnapshotAsync.and.resolveTo(state as any); + mockStateSnapshotService.getStateSnapshotForOperationLogAsync.and.resolveTo( + state as any, + ); // Mock validateAndRepair to return repaired state spyOn(service, 'validateAndRepair').and.resolveTo({ @@ -316,7 +324,9 @@ describe('ValidateStateService', () => { projectTree: [{ id: 'ORPHAN', k: MenuTreeKind.PROJECT }], }; mockStateSnapshotService.getStateSnapshot.and.returnValue(invalidState as any); - mockStateSnapshotService.getStateSnapshotAsync.and.resolveTo(invalidState as any); + mockStateSnapshotService.getStateSnapshotForOperationLogAsync.and.resolveTo( + invalidState as any, + ); await service.validateAndRepairCurrentState('other-context'); @@ -338,7 +348,9 @@ describe('ValidateStateService', () => { projectTree: [{ id: 'ORPHAN', k: MenuTreeKind.PROJECT }], }; mockStateSnapshotService.getStateSnapshot.and.returnValue(invalidState as any); - mockStateSnapshotService.getStateSnapshotAsync.and.resolveTo(invalidState as any); + mockStateSnapshotService.getStateSnapshotForOperationLogAsync.and.resolveTo( + invalidState as any, + ); mockHydrationStateService.isApplyingRemoteOps.and.returnValue(true); await service.validateAndRepairCurrentState('sync'); @@ -354,7 +366,9 @@ describe('ValidateStateService', () => { it('should dispatch loadAllData with isRemote flag for sync contexts', async () => { const state = createEmptyState(); mockStateSnapshotService.getStateSnapshot.and.returnValue(state as any); - mockStateSnapshotService.getStateSnapshotAsync.and.resolveTo(state as any); + mockStateSnapshotService.getStateSnapshotForOperationLogAsync.and.resolveTo( + state as any, + ); // Mock validateAndRepair to return repaired state spyOn(service, 'validateAndRepair').and.resolveTo({ @@ -374,7 +388,9 @@ describe('ValidateStateService', () => { it('should dispatch loadAllData without isRemote flag for non-sync contexts', async () => { const state = createEmptyState(); mockStateSnapshotService.getStateSnapshot.and.returnValue(state as any); - mockStateSnapshotService.getStateSnapshotAsync.and.resolveTo(state as any); + mockStateSnapshotService.getStateSnapshotForOperationLogAsync.and.resolveTo( + state as any, + ); // Mock validateAndRepair to return repaired state spyOn(service, 'validateAndRepair').and.resolveTo({ @@ -410,7 +426,7 @@ describe('ValidateStateService', () => { mockStateSnapshotService.getStateSnapshot.and.returnValue( createEmptyState() as any, ); - mockStateSnapshotService.getStateSnapshotAsync.and.resolveTo( + mockStateSnapshotService.getStateSnapshotForOperationLogAsync.and.resolveTo( stateWithArchive as any, ); @@ -424,7 +440,9 @@ describe('ValidateStateService', () => { await service.validateAndRepairCurrentState('sync'); // The repair path must load the async snapshot (archives from IndexedDB). - expect(mockStateSnapshotService.getStateSnapshotAsync).toHaveBeenCalled(); + expect( + mockStateSnapshotService.getStateSnapshotForOperationLogAsync, + ).toHaveBeenCalled(); // The state fed into validation/repair must include the archive... const validatedState = validateAndRepairSpy.calls.mostRecent().args[0]; diff --git a/src/app/op-log/validation/validate-state.service.ts b/src/app/op-log/validation/validate-state.service.ts index e50d104b99..968f3d4f40 100644 --- a/src/app/op-log/validation/validate-state.service.ts +++ b/src/app/op-log/validation/validate-state.service.ts @@ -118,7 +118,8 @@ export class ValidateStateService { // State is invalid — load the full snapshot including archives so the REPAIR // operation carries archive data. A REPAIR op built from the sync snapshot // would ship empty archives and wipe them on every client that applies it. - const currentState = await this.stateSnapshotService.getStateSnapshotAsync(); + const currentState = + await this.stateSnapshotService.getStateSnapshotForOperationLogAsync(); const result = await this.validateAndRepair( currentState as unknown as Record, diff --git a/src/app/root-store/meta/task-shared-meta-reducers/task-shared-crud.reducer.spec.ts b/src/app/root-store/meta/task-shared-meta-reducers/task-shared-crud.reducer.spec.ts index ea5356ac72..8504efe808 100644 --- a/src/app/root-store/meta/task-shared-meta-reducers/task-shared-crud.reducer.spec.ts +++ b/src/app/root-store/meta/task-shared-meta-reducers/task-shared-crud.reducer.spec.ts @@ -12,6 +12,7 @@ import { WorkContextType } from '../../../features/work-context/work-context.mod import { Action, ActionReducer } from '@ngrx/store'; import { getDbDateStr } from '../../../util/get-db-date-str'; import { IN_PROGRESS_TAG } from '../../../features/tag/tag.const'; +import { appStateFeatureKey } from '../../app-state/app-state.reducer'; import { createBaseState, createMockTag, @@ -408,6 +409,49 @@ describe('taskSharedCrudMetaReducer', () => { ); }); + it('should use captured dates when replaying on a different day', () => { + const capturedToday = '2024-06-14'; + const capturedTimestamp = new Date(2024, 5, 14, 12, 0, 0, 0).getTime(); + const { action, testState: baseTestState } = createConvertAction( + {}, + { + isPlanForToday: true, + isDone: true, + today: capturedToday, + doneOn: capturedTimestamp, + modified: capturedTimestamp, + }, + ); + const testState = { + ...baseTestState, + [TASK_FEATURE_NAME]: { + ...baseTestState[TASK_FEATURE_NAME], + entities: { + ...baseTestState[TASK_FEATURE_NAME].entities, + task1: action.task, + }, + ids: [...baseTestState[TASK_FEATURE_NAME].ids, 'task1'], + }, + [appStateFeatureKey]: { + ...baseTestState[appStateFeatureKey], + todayStr: '2024-06-15', + }, + }; + + metaReducer(testState, action); + + expectStateUpdate( + expectTaskUpdate('task1', { + dueDay: capturedToday, + doneOn: capturedTimestamp, + modified: capturedTimestamp, + }), + action, + mockReducer, + testState, + ); + }); + it('should add task at the beginning of existing taskIds', () => { const { action, testState: baseTestState } = createConvertAction(); diff --git a/src/app/root-store/meta/task-shared-meta-reducers/task-shared-crud.reducer.ts b/src/app/root-store/meta/task-shared-meta-reducers/task-shared-crud.reducer.ts index 7ab1eb23ca..dc29386bb4 100644 --- a/src/app/root-store/meta/task-shared-meta-reducers/task-shared-crud.reducer.ts +++ b/src/app/root-store/meta/task-shared-meta-reducers/task-shared-crud.reducer.ts @@ -148,6 +148,9 @@ const handleConvertToMainTask = ( isPlanForToday?: boolean, afterTaskId?: string | null, isDone?: boolean, + capturedToday?: string, + capturedDoneOn?: number, + capturedModified?: number, ): RootState => { // First, get the parent task to copy its properties const parentTask = state[TASK_FEATURE_NAME].entities[task.parentId as string] as Task; @@ -155,7 +158,7 @@ const handleConvertToMainTask = ( throw new Error('No parent for sub task'); } - const todayStr = state[appStateFeatureKey]?.todayStr ?? getDbDateStr(); + const todayStr = capturedToday ?? state[appStateFeatureKey]?.todayStr ?? getDbDateStr(); // `Array.isArray` guard (not `??`): a truthy non-array `parentTagIds` // from a captured op (seen on long-running SuperSync clients) bypasses // `??` and crashes the spread below. Same for `parentTask.tagIds`. @@ -190,7 +193,7 @@ const handleConvertToMainTask = ( tagIds: (Array.isArray(parentTask.tagIds) ? parentTask.tagIds : []).filter( (id) => id !== TODAY_TAG.id, ), - modified: Date.now(), + modified: capturedModified ?? Date.now(), ...(isPlanForToday && !task.dueWithTime ? { dueDay: todayStr, @@ -206,7 +209,7 @@ const handleConvertToMainTask = ( isDone, ...(isDone ? { - doneOn: Date.now(), + doneOn: capturedDoneOn ?? Date.now(), dueDay: todayStr, dueWithTime: undefined, } @@ -814,8 +817,16 @@ const createActionHandlers = (state: RootState, action: Action): ActionHandlerMa return handleAddTask(state, task, isAddToBottom, isAddToBacklog); }, [TaskSharedActions.convertToMainTask.type]: () => { - const { task, parentTagIds, isPlanForToday, afterTaskId, isDone } = - action as ReturnType; + const { + task, + parentTagIds, + isPlanForToday, + afterTaskId, + isDone, + today, + doneOn, + modified, + } = action as ReturnType; return handleConvertToMainTask( state, task, @@ -823,6 +834,9 @@ const createActionHandlers = (state: RootState, action: Action): ActionHandlerMa isPlanForToday, afterTaskId, isDone, + today, + doneOn, + modified, ); }, [TaskSharedActions.convertToSubTask.type]: () => { diff --git a/src/app/root-store/meta/task-shared-meta-reducers/task-shared-scheduling.reducer.spec.ts b/src/app/root-store/meta/task-shared-meta-reducers/task-shared-scheduling.reducer.spec.ts index bc846c3aa7..a9d6af905d 100644 --- a/src/app/root-store/meta/task-shared-meta-reducers/task-shared-scheduling.reducer.spec.ts +++ b/src/app/root-store/meta/task-shared-meta-reducers/task-shared-scheduling.reducer.spec.ts @@ -183,6 +183,33 @@ describe('taskSharedSchedulingMetaReducer', () => { testState, ); }); + + it('should use the captured day when replaying on a different day', () => { + const testState = createStateWithExistingTasks([], [], [], ['task1']); + testState[appStateFeatureKey] = { + ...testState[appStateFeatureKey], + todayStr: '2024-06-15', + }; + const action = { + ...TaskSharedActions.unscheduleTask({ + id: 'task1', + isLeaveInToday: true, + }), + today: '2024-06-14', + }; + + metaReducer(testState, action); + + expectStateUpdate( + expectTaskUpdate('task1', { + dueDay: '2024-06-14', + dueWithTime: undefined, + }), + action, + mockReducer, + testState, + ); + }); }); describe('dismissReminderOnly action', () => { diff --git a/src/app/root-store/meta/task-shared-meta-reducers/task-shared-scheduling.reducer.ts b/src/app/root-store/meta/task-shared-meta-reducers/task-shared-scheduling.reducer.ts index befa896077..a0eab586dc 100644 --- a/src/app/root-store/meta/task-shared-meta-reducers/task-shared-scheduling.reducer.ts +++ b/src/app/root-store/meta/task-shared-meta-reducers/task-shared-scheduling.reducer.ts @@ -124,9 +124,11 @@ const handleUnScheduleTask = ( state: RootState, taskId: string, isLeaveInToday = false, + capturedToday?: string, ): RootState => { // First, update the task entity to clear scheduling data - const todayStrForUnschedule = state[appStateFeatureKey]?.todayStr ?? getDbDateStr(); + const todayStrForUnschedule = + capturedToday ?? state[appStateFeatureKey]?.todayStr ?? getDbDateStr(); const updatedState = { ...state, [TASK_FEATURE_NAME]: taskAdapter.updateOne( @@ -332,10 +334,10 @@ const createActionHandlers = (state: RootState, action: Action): ActionHandlerMa ); }, [TaskSharedActions.unscheduleTask.type]: () => { - const { id, isLeaveInToday } = action as ReturnType< + const { id, isLeaveInToday, today } = action as ReturnType< typeof TaskSharedActions.unscheduleTask >; - return handleUnScheduleTask(state, id, isLeaveInToday); + return handleUnScheduleTask(state, id, isLeaveInToday, today); }, [TaskSharedActions.dismissReminderOnly.type]: () => { const { id } = action as ReturnType; diff --git a/src/app/root-store/meta/task-shared.actions.ts b/src/app/root-store/meta/task-shared.actions.ts index 3e2a3bd280..be53b088d9 100644 --- a/src/app/root-store/meta/task-shared.actions.ts +++ b/src/app/root-store/meta/task-shared.actions.ts @@ -42,6 +42,9 @@ export const TaskSharedActions = createActionGroup({ isPlanForToday?: boolean; afterTaskId?: string | null; isDone?: boolean; + today?: string; + doneOn?: number; + modified?: number; }) => ({ ...taskProps, meta: { @@ -176,6 +179,7 @@ export const TaskSharedActions = createActionGroup({ id: string; isSkipToast?: boolean; isLeaveInToday?: boolean; + today?: string; }) => ({ ...taskProps, meta: { diff --git a/src/app/util/get-db-date-str.spec.ts b/src/app/util/get-db-date-str.spec.ts index 6137ffaaa8..b9a31b6a2b 100644 --- a/src/app/util/get-db-date-str.spec.ts +++ b/src/app/util/get-db-date-str.spec.ts @@ -1,4 +1,4 @@ -import { getDbDateStr, isDBDateStr } from './get-db-date-str'; +import { getDbDateStr, isDBDateStr, isValidDBDateStr } from './get-db-date-str'; describe('getDbDateStr', () => { it('should return YYYY-MM-DD for a given date', () => { @@ -88,3 +88,17 @@ describe('isDBDateStr', () => { }); }); }); + +describe('isValidDBDateStr', () => { + it('should accept real calendar dates including leap days', () => { + expect(isValidDBDateStr('2024-02-29')).toBe(true); + expect(isValidDBDateStr('2026-12-31')).toBe(true); + }); + + it('should reject structurally valid but impossible dates', () => { + expect(isValidDBDateStr('2023-02-29')).toBe(false); + expect(isValidDBDateStr('2024-02-30')).toBe(false); + expect(isValidDBDateStr('2024-13-01')).toBe(false); + expect(isValidDBDateStr('2024-00-10')).toBe(false); + }); +}); diff --git a/src/app/util/get-db-date-str.ts b/src/app/util/get-db-date-str.ts index 96a97ffd12..65d694d43f 100644 --- a/src/app/util/get-db-date-str.ts +++ b/src/app/util/get-db-date-str.ts @@ -23,3 +23,17 @@ export const isDBDateStr = (str: string): boolean => { } return true; }; + +/** Validates both the YYYY-MM-DD shape and the actual Gregorian calendar day. */ +export const isValidDBDateStr = (str: string): boolean => { + if (!isDBDateStr(str)) return false; + + const year = Number(str.slice(0, 4)); + const month = Number(str.slice(5, 7)); + const day = Number(str.slice(8, 10)); + if (month < 1 || month > 12 || day < 1) return false; + + const isLeapYear = year % 4 === 0 && (year % 100 !== 0 || year % 400 === 0); + const daysInMonth = [31, isLeapYear ? 29 : 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31]; + return day <= daysInMonth[month - 1]; +};