fix(sync): don't report lifetime op totals as "changes since last sync" in conflict dialog (#8785)

* fix(sync): thread real lastSyncedVectorClock through LocalDataConflictError

The sync conflict dialog reported thousands of changes because getChangeCount() summed the entire (lifetime) vector clock whenever lastSyncedVectorClock was falsy, and the file-based (Dropbox) conflict path hardcoded that field to null - so it always hit the summing branch.

LocalDataConflictError now carries the client's last-synced vector clock (and timestamp). operation-log-sync populates it from the local snapshot clock at the seq-0-download-with-local-ops throw site, and passes null explicitly for genuinely-fresh clients. _handleLocalDataConflict reads it from the error instead of hardcoding null. getChangeCount() drops the sum-the-whole-clock fallback and returns null when no last-synced baseline exists; the dialog renders 'unknown' and still shows the overwrite confirmation, worded without a count.

SPAP-7

* refactor(sync): clarify conflict-count baseline and drop dead lastSyncedUpdate

Addresses non-blocking review feedback on the conflict-dialog change counts:

1. Soften the "last-synced baseline" wording. The snapshot vector clock is an
   APPROXIMATE baseline, not an exact last-synced one: compaction folds
   still-unsynced local ops into the snapshot clock (while only deleting synced
   ops), so a localClock - snapshotClock delta can under-count real local changes.
   Documented on getSnapshotVectorClock() and LocalDataConflictError so the count
   reads as the display heuristic it is.

   Left the count on the vector-clock delta rather than switching the local side to
   error.unsyncedCount: the wholly-fresh-client conflict throws with unsyncedCount=0
   despite meaningful store data, so unsyncedCount would misreport that path as
   "0 local changes" - worse than the current "unknown".

2. Drop the dead lastSyncedUpdate parameter from LocalDataConflictError. All three
   throw sites passed null and nothing ever populated it, so it was pure plumbing.
   The shared ConflictData.local.lastSyncedUpdate field and dialog row are left
   intact; the op-log handler now sets null explicitly (the dialog shows "Never"/"-",
   correct for a no-last-sync conflict).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
aakhter 2026-07-07 05:51:19 -04:00 committed by GitHub
parent 0c07053032
commit 2fd9acbe60
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
10 changed files with 307 additions and 30 deletions

View file

@ -23,7 +23,9 @@
</td>
<td [attr.data-label]="T.F.SYNC.D_CONFLICT.CHANGES | translate">
<span [class.isHighlight]="isHighlightRemoteChanges">{{
remoteChangeCount
remoteChangeCount === null
? (T.F.SYNC.D_CONFLICT.CHANGES_UNKNOWN | translate)
: remoteChangeCount
}}</span>
</td>
</tr>
@ -36,7 +38,11 @@
{{ local.lastUpdate | shortTime }}
</td>
<td [attr.data-label]="T.F.SYNC.D_CONFLICT.CHANGES | translate">
<span [class.isHighlight]="isHighlightLocalChanges">{{ localChangeCount }}</span>
<span [class.isHighlight]="isHighlightLocalChanges">{{
localChangeCount === null
? (T.F.SYNC.D_CONFLICT.CHANGES_UNKNOWN | translate)
: localChangeCount
}}</span>
</td>
</tr>
<tr>

View file

@ -0,0 +1,154 @@
import { TestBed } from '@angular/core/testing';
import { MAT_DIALOG_DATA, MatDialog, MatDialogRef } from '@angular/material/dialog';
import { NoopAnimationsModule } from '@angular/platform-browser/animations';
import { TranslateModule } from '@ngx-translate/core';
import { provideMockStore } from '@ngrx/store/testing';
import { DialogSyncConflictComponent } from './dialog-sync-conflict.component';
import { ConflictData, ConflictReason, VectorClock } from '../../../op-log/sync-exports';
import { T } from '../../../t.const';
const buildConflictData = (overrides: {
localVectorClock?: VectorClock;
remoteVectorClock?: VectorClock;
lastSyncedVectorClock?: VectorClock | null;
}): ConflictData => ({
reason: ConflictReason.NoLastSync,
remote: {
lastUpdate: 1000,
lastUpdateAction: 'Remote data',
revMap: {},
crossModelVersion: 1,
mainModelData: {},
isFullData: true,
vectorClock: overrides.remoteVectorClock,
},
local: {
lastUpdate: 2000,
lastUpdateAction: 'local changes',
revMap: {},
crossModelVersion: 1,
lastSyncedUpdate: null,
metaRev: null,
vectorClock: overrides.localVectorClock,
lastSyncedVectorClock: overrides.lastSyncedVectorClock,
},
});
describe('DialogSyncConflictComponent', () => {
const createComponent = (data: ConflictData): DialogSyncConflictComponent => {
TestBed.overrideProvider(MAT_DIALOG_DATA, { useValue: data });
const fixture = TestBed.createComponent(DialogSyncConflictComponent);
return fixture.componentInstance;
};
beforeEach(async () => {
const mockDialogRef = jasmine.createSpyObj('MatDialogRef', ['close']);
const mockDialog = jasmine.createSpyObj('MatDialog', ['open']);
await TestBed.configureTestingModule({
imports: [
DialogSyncConflictComponent,
NoopAnimationsModule,
TranslateModule.forRoot(),
],
providers: [
provideMockStore(),
{ provide: MatDialogRef, useValue: mockDialogRef },
{ provide: MatDialog, useValue: mockDialog },
{ provide: MAT_DIALOG_DATA, useValue: buildConflictData({}) },
],
}).compileComponents();
});
describe('getChangeCount()', () => {
it('(a) returns correct per-client delta when lastSyncedVectorClock is present', () => {
const component = createComponent(
buildConflictData({
localVectorClock: { clientA: 10, clientB: 5 },
remoteVectorClock: { clientA: 3, clientB: 12 },
lastSyncedVectorClock: { clientA: 3, clientB: 5 },
}),
);
// local: max(0,10-3) + max(0,5-5) = 7
expect(component.localChangeCount).toBe(7);
// remote: max(0,3-3) + max(0,12-5) = 7
expect(component.remoteChangeCount).toBe(7);
});
it('(b) returns null (no whole-clock summing) when lastSyncedVectorClock is null', () => {
const component = createComponent(
buildConflictData({
localVectorClock: { clientA: 1000, clientB: 2000 },
remoteVectorClock: { clientA: 500, clientB: 900 },
lastSyncedVectorClock: null,
}),
);
// Bug SPAP-7: previously summed the whole clock (3000 / 1400). Must be null now.
expect(component.localChangeCount).toBeNull();
expect(component.remoteChangeCount).toBeNull();
});
it('returns null when vector clocks are entirely absent', () => {
const component = createComponent(
buildConflictData({
localVectorClock: undefined,
remoteVectorClock: undefined,
lastSyncedVectorClock: null,
}),
);
expect(component.localChangeCount).toBeNull();
expect(component.remoteChangeCount).toBeNull();
});
});
describe('getConfirmationMessage() / shouldConfirmOverwrite()', () => {
it('(c) renders the counted warning key when deltas are known', () => {
const component = createComponent(
buildConflictData({
localVectorClock: { clientA: 30 },
remoteVectorClock: { clientA: 3 },
lastSyncedVectorClock: { clientA: 3 },
}),
);
// local delta = 27, remote delta = 0 -> USE_REMOTE should confirm (>= 20)
expect(
(
component as unknown as { shouldConfirmOverwrite(r: string): boolean }
).shouldConfirmOverwrite('USE_REMOTE'),
).toBe(true);
const msg = (
component as unknown as { getConfirmationMessage(r: string): string }
).getConfirmationMessage('USE_REMOTE');
expect(msg).toContain(T.F.SYNC.D_CONFLICT.OVERWRITE_WARNING);
expect(msg).not.toContain('OVERWRITE_WARNING_UNKNOWN');
});
it('(c) renders the count-free warning key and still confirms when deltas are unknown', () => {
const component = createComponent(
buildConflictData({
localVectorClock: { clientA: 1000 },
remoteVectorClock: { clientA: 500 },
lastSyncedVectorClock: null,
}),
);
// Null counts must still allow the confirmation dialog to appear.
expect(
(
component as unknown as { shouldConfirmOverwrite(r: string): boolean }
).shouldConfirmOverwrite('USE_REMOTE'),
).toBe(true);
const msg = (
component as unknown as { getConfirmationMessage(r: string): string }
).getConfirmationMessage('USE_REMOTE');
expect(msg).toContain(T.F.SYNC.D_CONFLICT.OVERWRITE_WARNING_UNKNOWN);
});
});
});

View file

@ -59,7 +59,7 @@ export class DialogSyncConflictComponent {
remoteChangeCount = this.getChangeCount('remote');
localChangeCount = this.getChangeCount('local');
isHighlightRemoteChanges = this.remoteChangeCount > this.localChangeCount;
isHighlightRemoteChanges = (this.remoteChangeCount ?? 0) > (this.localChangeCount ?? 0);
isHighlightLocalChanges = !this.isHighlightRemoteChanges;
constructor() {
@ -124,36 +124,49 @@ export class DialogSyncConflictComponent {
}
}
private getChangeCount(side: 'remote' | 'local'): number {
// First try vector clock, fall back to Lamport if not available
if (this.remote.vectorClock && this.local.vectorClock) {
const clock = side === 'remote' ? this.remote.vectorClock : this.local.vectorClock;
const lastSyncedClock = this.local.lastSyncedVectorClock;
if (!clock) return 0;
// If no last synced clock, return total of all values
if (!lastSyncedClock) {
return Object.values(clock).reduce((sum, value) => sum + value, 0);
}
// Calculate changes since last sync
let changeCount = 0;
for (const [clientId, value] of Object.entries(clock)) {
const lastSyncedValue = lastSyncedClock[clientId] || 0;
changeCount += Math.max(0, value - lastSyncedValue);
}
return changeCount;
/**
* Number of changes on the given side since the last successful sync,
* computed as a per-client vector-clock delta.
*
* Returns `null` when no last-synced baseline is available (a
* never-synced/fresh client). We deliberately do NOT sum the whole clock as
* a fallback: vector-clock counters are per-client LIFETIME totals, so
* summing reports total ops ever performed (thousands), not changes since
* last sync (SPAP-7). A null result is rendered as "unknown" in the UI.
*/
private getChangeCount(side: 'remote' | 'local'): number | null {
if (!this.remote.vectorClock || !this.local.vectorClock) {
return null;
}
// No vector clock available
return 0;
const clock = side === 'remote' ? this.remote.vectorClock : this.local.vectorClock;
const lastSyncedClock = this.local.lastSyncedVectorClock;
// No last-synced baseline → changes-since-sync is genuinely unknown.
if (!lastSyncedClock) {
return null;
}
// Calculate changes since last sync (per-client delta).
let changeCount = 0;
for (const [clientId, value] of Object.entries(clock)) {
const lastSyncedValue = lastSyncedClock[clientId] || 0;
changeCount += Math.max(0, value - lastSyncedValue);
}
return changeCount;
}
private shouldConfirmOverwrite(resolution: DialogConflictResolutionResult): boolean {
const remoteChanges = this.getChangeCount('remote');
const localChanges = this.getChangeCount('local');
// If we cannot quantify the changes (fresh client, no last-synced
// baseline), still show the confirmation — overwriting could discard real
// data. The message is worded without a count (see getConfirmationMessage).
if (remoteChanges === null || localChanges === null) {
return resolution === 'USE_REMOTE' || resolution === 'USE_LOCAL';
}
const MIN_CHANGES_DIFFERENCE = 20;
if (resolution === 'USE_REMOTE') {
@ -170,10 +183,22 @@ export class DialogSyncConflictComponent {
private getConfirmationMessage(resolution: DialogConflictResolutionResult): string {
const remoteChanges = this.getChangeCount('remote');
const localChanges = this.getChangeCount('local');
const [sourceChanges, targetChanges, sourceName, targetName] =
const [sourceName, targetName] =
resolution === 'USE_REMOTE' ? ['remote', 'local'] : ['local', 'remote'];
// Without a known change count, use the count-free warning wording.
if (remoteChanges === null || localChanges === null) {
return this._translateService.instant(
T.F.SYNC.D_CONFLICT.OVERWRITE_WARNING_UNKNOWN,
{ targetName, sourceName },
);
}
const [sourceChanges, targetChanges] =
resolution === 'USE_REMOTE'
? [remoteChanges, localChanges, 'remote', 'local']
: [localChanges, remoteChanges, 'local', 'remote'];
? [remoteChanges, localChanges]
: [localChanges, remoteChanges];
return this._translateService.instant(T.F.SYNC.D_CONFLICT.OVERWRITE_WARNING, {
targetName,

View file

@ -1242,10 +1242,12 @@ export class SyncWrapperService {
lastUpdateAction: `${error.unsyncedCount} local changes pending`,
revMap: {},
crossModelVersion: 1,
// Op-log (NoLastSync) conflicts do not carry a last-synced timestamp, so
// this is always null here; the dialog renders it as "Never"/"-".
lastSyncedUpdate: null,
metaRev: null,
vectorClock: localClock,
lastSyncedVectorClock: null,
lastSyncedVectorClock: error.lastSyncedVectorClock ?? null,
},
};

View file

@ -74,6 +74,13 @@ export class LocalDataConflictError extends Error {
public readonly unsyncedCount: number,
public readonly remoteSnapshotState: Record<string, unknown>,
public readonly remoteVectorClock?: Record<string, number>,
// The client's vector clock as of its last successful sync. Used by the
// conflict dialog as an APPROXIMATE baseline for the per-client
// changes-since-last-sync delta. Note: compaction can fold still-unsynced ops
// into this clock, so the delta can under-count actual local changes — it is a
// display heuristic, not an exact "unsynced" figure. `null` for genuinely-fresh
// clients that have never synced (SPAP-7).
public readonly lastSyncedVectorClock?: Record<string, number> | null,
) {
super(`Local data conflict: ${unsyncedCount} unsynced changes would be lost`);
}

View file

@ -1510,6 +1510,64 @@ describe('OperationLogSyncService', () => {
}
});
it('carries the last-synced vector clock (from snapshot) on the seq-0-download-with-local-ops path', async () => {
const unsyncedEntries: OperationLogEntry[] = [
{
seq: 1,
op: {
id: 'local-op-1',
clientId: 'client-A',
actionType: 'test' as ActionType,
opType: OpType.Update,
entityType: 'TASK',
entityId: 'task-1',
payload: {},
vectorClock: { clientA: 6 },
timestamp: Date.now(),
schemaVersion: 1,
},
appliedAt: Date.now(),
source: 'local',
},
];
opLogStoreSpy.getUnsynced.and.returnValue(Promise.resolve(unsyncedEntries));
// The snapshot's clock is the last-synced baseline this client had.
const lastSyncedClock = { clientA: 3, clientB: 5 };
const vectorClockService = TestBed.inject(
VectorClockService,
) as jasmine.SpyObj<VectorClockService>;
vectorClockService.getSnapshotVectorClock.and.resolveTo(lastSyncedClock);
downloadServiceSpy.downloadRemoteOps.and.returnValue(
Promise.resolve({
newOps: [],
hasMore: false,
latestSeq: 0,
needsFullStateUpload: false,
success: true,
providerMode: 'fileSnapshotOps',
failedFileCount: 0,
snapshotState: { tasks: [{ id: 'remote-task' }] },
snapshotVectorClock: { clientB: 5 },
}),
);
const mockProvider = {
isReady: () => Promise.resolve(true),
supportsOperationSync: true,
} as any;
try {
await service.downloadRemoteOps(mockProvider);
fail('Expected LocalDataConflictError to be thrown');
} catch (error) {
expect(error).toBeInstanceOf(LocalDataConflictError);
const conflictError = error as LocalDataConflictError;
expect(conflictError.lastSyncedVectorClock).toEqual(lastSyncedClock);
}
});
it('should NOT throw LocalDataConflictError when client has no unsynced ops', async () => {
// No unsynced ops
opLogStoreSpy.getUnsynced.and.returnValue(Promise.resolve([]));

View file

@ -20,6 +20,7 @@ import { SuperSyncStatusService } from './super-sync-status.service';
import { ServerMigrationService } from './server-migration.service';
import { OperationWriteFlushService } from './operation-write-flush.service';
import { RemoteOpsProcessingService } from './remote-ops-processing.service';
import { VectorClockService } from './vector-clock.service';
import {
DownloadResultForRejection,
RejectedOpsHandlerService,
@ -117,6 +118,7 @@ export class OperationLogSyncService {
// Extracted services
private remoteOpsProcessingService = inject(RemoteOpsProcessingService);
private vectorClockService = inject(VectorClockService);
private rejectedOpsHandlerService = inject(RejectedOpsHandlerService);
private syncHydrationService = inject(SyncHydrationService);
private syncImportConflictGateService = inject(SyncImportConflictGateService);
@ -525,10 +527,18 @@ export class OperationLogSyncService {
'Throwing LocalDataConflictError for conflict resolution dialog.',
);
// The local snapshot's vector clock is this client's last-synced
// baseline: unsynced ops sit on top of it, so the dialog can compute
// changes-since-last-sync as a per-client delta instead of summing
// the whole (lifetime) clock (SPAP-7). Undefined snapshot → null.
const lastSyncedVectorClock =
(await this.vectorClockService.getSnapshotVectorClock()) ?? null;
throw new LocalDataConflictError(
unsyncedOps.length,
result.snapshotState as Record<string, unknown>,
result.snapshotVectorClock,
lastSyncedVectorClock,
);
} else {
// Defer the markRejected call until hydration has succeeded — see
@ -554,10 +564,13 @@ export class OperationLogSyncService {
'Throwing LocalDataConflictError for conflict resolution dialog.',
);
// Fresh client (no unsynced ops, no prior sync) — there is no
// last-synced clock, so pass null explicitly (SPAP-7).
throw new LocalDataConflictError(
0, // No unsynced ops, but we have meaningful store data
result.snapshotState as Record<string, unknown>,
result.snapshotVectorClock,
null,
);
}
@ -684,7 +697,8 @@ export class OperationLogSyncService {
OpLog.warn(
`OperationLogSyncService: Fresh client has local data and ${result.newOps.length} remote ops. Showing conflict dialog.`,
);
throw new LocalDataConflictError(0, {});
// Wholly fresh client — no prior sync, so no last-synced clock (SPAP-7).
throw new LocalDataConflictError(0, {}, undefined, null);
}
OpLog.warn(

View file

@ -115,6 +115,13 @@ export class VectorClockService {
* Use this as a fallback when no per-entity frontier exists to prevent
* false conflicts for entities modified before compaction.
*
* NOTE: this is an APPROXIMATE "last-synced" baseline, not an exact one.
* Compaction snapshots the current vector clock which includes local ops that
* were created but not yet synced while only deleting already-synced ops. So a
* `localClock snapshotClock` delta can UNDER-count real unsynced local changes
* (down to zero if compaction ran with unsynced ops pending). Callers should
* treat the resulting count as a display heuristic, not an exact figure.
*
* @returns The snapshot vector clock, or undefined if no snapshot exists
*/
async getSnapshotVectorClock(): Promise<VectorClock | undefined> {

View file

@ -1250,6 +1250,7 @@ const T = {
ADDITIONAL_INFO: 'F.SYNC.D_CONFLICT.ADDITIONAL_INFO',
CHANGES: 'F.SYNC.D_CONFLICT.CHANGES',
CHANGES_SINCE_LAST_SYNC: 'F.SYNC.D_CONFLICT.CHANGES_SINCE_LAST_SYNC',
CHANGES_UNKNOWN: 'F.SYNC.D_CONFLICT.CHANGES_UNKNOWN',
COMPARISON_RESULT: 'F.SYNC.D_CONFLICT.COMPARISON_RESULT',
DATE: 'F.SYNC.D_CONFLICT.DATE',
LAST_SYNC: 'F.SYNC.D_CONFLICT.LAST_SYNC',
@ -1259,6 +1260,7 @@ const T = {
LOCAL_REMOTE: 'F.SYNC.D_CONFLICT.LOCAL_REMOTE',
NEVER: 'F.SYNC.D_CONFLICT.NEVER',
OVERWRITE_WARNING: 'F.SYNC.D_CONFLICT.OVERWRITE_WARNING',
OVERWRITE_WARNING_UNKNOWN: 'F.SYNC.D_CONFLICT.OVERWRITE_WARNING_UNKNOWN',
REMOTE: 'F.SYNC.D_CONFLICT.REMOTE',
RESULT: 'F.SYNC.D_CONFLICT.RESULT',
TEXT: 'F.SYNC.D_CONFLICT.TEXT',

View file

@ -1227,6 +1227,7 @@
"ADDITIONAL_INFO": "Additional Info",
"CHANGES": "Changes",
"CHANGES_SINCE_LAST_SYNC": "Changes since last sync",
"CHANGES_UNKNOWN": "unknown",
"COMPARISON_RESULT": "Comparison Result",
"DATE": "Date",
"LAST_SYNC": "Last Sync:",
@ -1236,6 +1237,7 @@
"LOCAL_REMOTE": "Local -> Remote",
"NEVER": "Never",
"OVERWRITE_WARNING": "WARNING: The {{targetName}} data contains approximately {{targetChanges}} changes while the {{sourceName}} data only has {{sourceChanges}} changes. Are you sure you want to overwrite the {{targetName}} data with the {{sourceName}} version? This action cannot be undone.",
"OVERWRITE_WARNING_UNKNOWN": "WARNING: This device cannot determine how many unsynced changes the {{targetName}} data has (it has no record of a previous sync). Overwriting the {{targetName}} data with the {{sourceName}} version may discard changes. Are you sure? This action cannot be undone.",
"REMOTE": "Remote",
"RESULT": "Result",
"TEXT": "<p>Update from remote. Both local and remote data seem to be modified.</p>",