refactor(sync): tighten SYNC_IMPORT gate naming and inline single-use helper

Inline _hasAnyMeaningfulData at its remaining caller (the snapshot/provider-
switch path) and rename _hasMeaningfulLocalData to _hasMeaningfulStoreData
for parallel naming with _hasMeaningfulPendingOps. Strengthen the silent-
accept piggyback test to assert kind === 'completed' rather than
\!== 'cancelled', and clarify the originating-device cross-reference in the
piggyback (D.6) doc and the IMPORT_CONFLICT diamond in the flowchart.
This commit is contained in:
Johannes Millan 2026-04-29 12:38:21 +02:00
parent 82815cf9b2
commit 3fc1bb6e94
4 changed files with 12 additions and 20 deletions

View file

@ -43,7 +43,7 @@ flowchart TD
IS_IMPORT -->|No| CONFLICT_CHK
IS_IMPORT -->|Yes| ENC_ONLY{Encryption-only change<br/>+ no pending ops?}
ENC_ONLY -->|Yes| APPLY
ENC_ONLY -->|No| IMPORT_CONFLICT{Meaningful pending<br/>ops?<br/>TASK/PROJECT/TAG/NOTE<br/>C/U/D or full-state}
ENC_ONLY -->|No| IMPORT_CONFLICT{Meaningful<br/>pending ops?}
IMPORT_CONFLICT -->|Yes| IMPORT_DLG[ImportConflictDialog:<br/>import reason shown,<br/>Use Server Data recommended]
IMPORT_CONFLICT -->|No| APPLY_IMPORT[Apply full state replacement<br/>silently — already-synced<br/>store data is not a conflict]
IMPORT_DLG -->|Use Server| FORCE_DL
@ -108,6 +108,6 @@ flowchart TD
- The `Enter Password` and `Decrypt Error` dialogs correspond to `DecryptNoPasswordError` and `DecryptError` respectively — they are distinct components with different options.
- `Encryption-only change` bypass: when an incoming SYNC_IMPORT has `syncImportReason === 'PASSWORD_CHANGED'` and there are no meaningful pending ops, the dialog is skipped (data is identical, only encryption changed).
- `IMPORT_CONFLICT` gate uses pending ops only, not store contents (`_hasMeaningfulPendingOps()`). Already-synced store data is not a conflict with the incoming SYNC_IMPORT — the user-facing warning happens on the originating device. Including store contents in the gate would let an old client pick `USE_LOCAL` and force-upload its stale pre-import state, rolling back the remote import for everyone.
- `IMPORT_CONFLICT` gate uses pending ops only, not store contents (`_hasMeaningfulPendingOps()`). "Meaningful" = TASK/PROJECT/TAG/NOTE create/update/delete or full-state ops — config-only ops don't count. Already-synced store data is not a conflict with the incoming SYNC_IMPORT — the user-facing warning happens on the originating device. Including store contents in the gate would let an old client pick `USE_LOCAL` and force-upload its stale pre-import state, rolling back the remote import for everyone.
- LWW tie-breaking: on equal timestamps, remote wins (server-authoritative). `moveToArchive` operations always win regardless of timestamp.
- Re-download retry limit: max 3 resolution attempts per entity (`MAX_CONCURRENT_RESOLUTION_ATTEMPTS`); if exceeded, ops are permanently rejected.

View file

@ -263,7 +263,7 @@ Comprehensive spec of all scenarios that can occur during SuperSync synchronizat
**Mirrors the download path (D.1 / D.2):** the gate is unsynced pending changes, not store contents. Prompting on already-synced store data would let an old client roll back the remote import via USE_LOCAL.
**User sees:** Nothing when there are no pending changes. Conflict dialog only when actual unsynced work is at risk.
**User sees:** Nothing when there are no pending changes — the user-facing warning happened on the originating device (`D_SERVER_MIGRATION_CONFIRM` / encryption flow), see D.1. Conflict dialog only when actual unsynced work is at risk.
---

View file

@ -1757,7 +1757,7 @@ describe('OperationLogSyncService', () => {
});
});
describe('_hasMeaningfulLocalData detection for first-time sync', () => {
describe('_hasMeaningfulStoreData detection for first-time sync', () => {
let downloadServiceSpy: jasmine.SpyObj<OperationLogDownloadService>;
beforeEach(() => {
@ -2454,7 +2454,7 @@ describe('OperationLogSyncService', () => {
expect(remoteOpsProcessingServiceSpy.processRemoteOps).toHaveBeenCalledWith([
piggybackedSyncImport,
]);
expect(result.kind).not.toBe('cancelled');
expect(result.kind).toBe('completed');
});
it('should process piggybacked SYNC_IMPORT silently when no meaningful local data', async () => {

View file

@ -154,12 +154,12 @@ export class OperationLogSyncService {
*
* @returns true if user has created any tasks, projects (besides INBOX), tags (besides system tags), or notes
*/
private _hasMeaningfulLocalData(): boolean {
private _hasMeaningfulStoreData(): boolean {
const snapshot = this.stateSnapshotService.getStateSnapshot();
if (!snapshot) {
OpLog.warn(
'OperationLogSyncService._hasMeaningfulLocalData: Unable to get state snapshot',
'OperationLogSyncService._hasMeaningfulStoreData: Unable to get state snapshot',
);
return false; // Assume no data rather than blocking sync
}
@ -193,15 +193,6 @@ export class OperationLogSyncService {
return false;
}
/**
* Checks if there is any meaningful user data either in the pending ops
* or already in the NgRx store. This combines both checks that are always
* used together to decide whether a conflict dialog is needed.
*/
private _hasAnyMeaningfulData(pendingOps: OperationLogEntry[]): boolean {
return this._hasMeaningfulPendingOps(pendingOps) || this._hasMeaningfulLocalData();
}
/**
* Checks if any of the given ops represent meaningful user data.
* Meaningful = TASK/PROJECT/TAG/NOTE creates/updates/deletes, or full-state ops.
@ -527,7 +518,8 @@ export class OperationLogSyncService {
// The store check catches provider-switch scenarios: user switches from
// SuperSync→Dropbox, only has a config-change op (not "meaningful"), but the
// store is full of real data that would be overwritten by old Dropbox state.
const hasMeaningfulUserData = this._hasAnyMeaningfulData(unsyncedOps);
const hasMeaningfulUserData =
this._hasMeaningfulPendingOps(unsyncedOps) || this._hasMeaningfulStoreData();
if (hasMeaningfulUserData) {
// Client has meaningful user data - show conflict dialog
@ -557,7 +549,7 @@ export class OperationLogSyncService {
// CRITICAL FIX: Even if op-log is empty, check if NgRx store has meaningful data.
// This catches data that existed before the operation-log feature was added.
if (isFreshClient && this._hasMeaningfulLocalData()) {
if (isFreshClient && this._hasMeaningfulStoreData()) {
OpLog.warn(
'OperationLogSyncService: Fresh client detected with meaningful local data in store. ' +
'Throwing LocalDataConflictError for conflict resolution dialog.',
@ -646,7 +638,7 @@ export class OperationLogSyncService {
const isEmptyServer = result.latestServerSeq === 0;
if (isEmptyServer) {
const isFresh = await this.isWhollyFreshClient();
if (isFresh && this._hasMeaningfulLocalData()) {
if (isFresh && this._hasMeaningfulStoreData()) {
OpLog.warn(
'OperationLogSyncService: Pre-op-log client with meaningful local data on empty server. ' +
'Creating SYNC_IMPORT via server migration to seed the server.',
@ -681,7 +673,7 @@ export class OperationLogSyncService {
// check if there's meaningful local data that would be overwritten.
const isFreshClient = await this.isWhollyFreshClient();
if (isFreshClient && result.newOps.length > 0) {
if (this._hasMeaningfulLocalData()) {
if (this._hasMeaningfulStoreData()) {
// Local data exists — throw conflict error so the full conflict dialog is shown,
// letting the user choose between keeping local data or using remote data.
OpLog.warn(