refactor(sync): address extraction review findings

This commit is contained in:
Johannes Millan 2026-05-12 12:52:39 +02:00
parent 4758464bd7
commit d4d3395c54
8 changed files with 279 additions and 63 deletions

View file

@ -473,6 +473,10 @@ Initial candidate-file audit:
through `SyncLogger` with counts, paths, expected types, and data shape
summaries only; raw validation result data and invalid values stay out of
exportable logs.
- `op-log/validation/is-related-model-data-valid.ts` and the invalid-date
repair branch in `data-repair.ts`: cross-model validation and date repair
diagnostics now keep raw app state, titles, and corrupted date strings out of
exportable logs.
- `op-log/util/sync-file-prefix.ts`: now delegates to the package helper with
app-supplied prefix and error construction. The app-facing shim should remain
until consumers are deliberately switched to injected/configured helpers.

View file

@ -33,8 +33,8 @@ describe('sync-core ports', () => {
conflictType: 'sync-import',
scenario: 'LOCAL_IMPORT_FILTERS_REMOTE',
reason: 'BACKUP_RESTORE',
counts: { filteredOps: 3 },
timestamps: { localImport: 123 },
counts: { filteredOpCount: 3 },
timestamps: { localImportTimestamp: 123 },
meta: { providerId: 'super-sync' },
}),
).resolves.toBe('USE_LOCAL');

View file

@ -0,0 +1,47 @@
import { TestBed } from '@angular/core/testing';
import { provideMockStore } from '@ngrx/store/testing';
import { SyncProviderId } from '../../op-log/sync-providers/provider.const';
import { DEFAULT_GLOBAL_CONFIG } from './default-global-config.const';
import { GlobalConfigService } from './global-config.service';
import { CONFIG_FEATURE_NAME } from './store/global-config.reducer';
import type { SyncConfig } from './global-config.model';
describe('GlobalConfigService', () => {
it('should expose a sync-core config snapshot without private provider fields', async () => {
const syncConfig = {
...DEFAULT_GLOBAL_CONFIG.sync,
isEnabled: true,
syncProvider: SyncProviderId.SuperSync,
isEncryptionEnabled: true,
isCompressionEnabled: true,
isManualSyncOnly: true,
syncInterval: 15,
encryptKey: 'private-key',
} as SyncConfig;
TestBed.configureTestingModule({
providers: [
provideMockStore({
initialState: {
[CONFIG_FEATURE_NAME]: {
...DEFAULT_GLOBAL_CONFIG,
sync: syncConfig,
},
},
}),
],
});
const snapshot = await TestBed.inject(GlobalConfigService).getSyncConfig();
expect(snapshot).toEqual({
isEnabled: true,
syncProvider: SyncProviderId.SuperSync,
isEncryptionEnabled: true,
isCompressionEnabled: true,
isManualSyncOnly: true,
syncInterval: 15,
});
expect('encryptKey' in snapshot).toBeFalse();
});
});

View file

@ -1,6 +1,6 @@
import { Injectable, inject, Signal } from '@angular/core';
import { select, Store } from '@ngrx/store';
import type { SyncConfigPort } from '@sp/sync-core';
import type { SyncConfigPort, SyncConfigSnapshot } from '@sp/sync-core';
import { toSignal } from '@angular/core/rxjs-interop';
import { updateGlobalConfigSection } from './store/global-config.actions';
import { firstValueFrom, Observable } from 'rxjs';
@ -196,7 +196,25 @@ export class GlobalConfigService implements SyncConfigPort<
);
}
getSyncConfig(): Promise<SyncConfig> {
return firstValueFrom(this.sync$);
async getSyncConfig(): Promise<
SyncConfigSnapshot<NonNullable<SyncConfig['syncProvider']>>
> {
const {
isEnabled,
syncProvider,
isEncryptionEnabled,
isCompressionEnabled,
isManualSyncOnly,
syncInterval,
} = await firstValueFrom(this.sync$);
return {
isEnabled,
syncProvider,
isEncryptionEnabled,
isCompressionEnabled,
isManualSyncOnly,
syncInterval,
};
}
}

View file

@ -14,6 +14,7 @@ import {
import { IssueProvider } from '../../features/issue/issue.model';
import { AppDataComplete } from '../model/model-config';
import { dirtyDeepCopy } from '../../util/dirtyDeepCopy';
import { OP_LOG_SYNC_LOGGER } from '../core/sync-logger.adapter';
const FAKE_PROJECT_ID = 'FAKE_PROJECT_ID';
describe('dataRepair()', () => {
@ -2267,6 +2268,8 @@ describe('dataRepair()', () => {
describe('invalid dueDay/deadlineDay sanitization (#6908)', () => {
it('should clear invalid dueDay from task', () => {
const invalidDueDay = '-/-/2026';
const warnSpy = spyOn(OP_LOG_SYNC_LOGGER, 'warn').and.stub();
const taskState = {
...mock.task,
...fakeEntityStateFromArray<Task>([
@ -2275,7 +2278,7 @@ describe('dataRepair()', () => {
id: 'INVALID_DUE',
title: 'INVALID_DUE',
projectId: FAKE_PROJECT_ID,
dueDay: '-/-/2026' as any,
dueDay: invalidDueDay as any,
},
]),
} as any;
@ -2287,6 +2290,15 @@ describe('dataRepair()', () => {
});
expect(result.data.task.entities['INVALID_DUE']!.dueDay).toBeUndefined();
expect(warnSpy).toHaveBeenCalledWith(
'[data-repair] Clearing invalid task date field',
jasmine.objectContaining({
taskId: 'INVALID_DUE',
field: 'dueDay',
valueStringLength: invalidDueDay.length,
}),
);
expect(JSON.stringify(warnSpy.calls.allArgs())).not.toContain(invalidDueDay);
});
it('should clear invalid deadlineDay from task', () => {

View file

@ -16,6 +16,7 @@ import { INBOX_PROJECT } from '../../features/project/project.const';
import { autoFixTypiaErrors } from './auto-fix-typia-errors';
import { IValidation } from 'typia';
import { OpLog } from '../../core/log';
import { OP_LOG_SYNC_LOGGER } from '../core/sync-logger.adapter';
import { repairMenuTree } from './repair-menu-tree';
import { initialTimeTrackingState } from '../../features/time-tracking/store/time-tracking.reducer';
import { RepairSummary } from '../core/operation.types';
@ -193,14 +194,22 @@ const _fixInvalidDueDateStrings = (
const t = taskState.entities[id] as TaskCopy;
if (!t) continue;
if (typeof t.dueDay === 'string' && !isDBDateStr(t.dueDay)) {
OpLog.warn(`[data-repair] Clearing invalid dueDay "${t.dueDay}" on task ${id}`);
OP_LOG_SYNC_LOGGER.warn('[data-repair] Clearing invalid task date field', {
taskId: id,
field: 'dueDay',
valueType: 'string',
valueStringLength: t.dueDay.length,
});
t.dueDay = undefined;
fixedCount++;
}
if (typeof t.deadlineDay === 'string' && !isDBDateStr(t.deadlineDay)) {
OpLog.warn(
`[data-repair] Clearing invalid deadlineDay "${t.deadlineDay}" on task ${id}`,
);
OP_LOG_SYNC_LOGGER.warn('[data-repair] Clearing invalid task date field', {
taskId: id,
field: 'deadlineDay',
valueType: 'string',
valueStringLength: t.deadlineDay.length,
});
t.deadlineDay = undefined;
fixedCount++;
}

View file

@ -1,6 +1,7 @@
import { AppDataComplete } from '../model/model-config';
import { OpLog } from '../../core/log';
import { TODAY_TAG } from '../../features/tag/tag.const';
import { OP_LOG_SYNC_LOGGER } from '../core/sync-logger.adapter';
describe('isRelatedModelDataValid', () => {
let isRelatedModelDataValid: any;
@ -13,6 +14,7 @@ describe('isRelatedModelDataValid', () => {
spyOn(OpLog, 'warn');
spyOn(OpLog, 'err');
spyOn(OpLog, 'critical');
spyOn(OP_LOG_SYNC_LOGGER, 'log');
/* eslint-disable @typescript-eslint/no-require-imports */
// Reset modules to allow re-importing with mocks
// @ts-ignore
@ -58,6 +60,48 @@ describe('isRelatedModelDataValid', () => {
expect(result).toBe(false);
});
it('should log validity error metadata without raw app data', () => {
const privateProjectTitle = 'Private Project Title';
const data: any = {
project: {
ids: ['p1'],
entities: {
p1: {
id: 'p1',
title: privateProjectTitle,
taskIds: ['missing-task'],
backlogTaskIds: [],
noteIds: [],
},
},
},
tag: { ids: [], entities: {} },
task: { ids: [], entities: {} },
taskRepeatCfg: { ids: [], entities: {} },
archiveYoung: { task: { ids: [], entities: {} } },
archiveOld: { task: { ids: [], entities: {} } },
note: { ids: [], entities: {}, todayOrder: [] },
issueProvider: { ids: [], entities: {} },
reminders: [],
menuTree: { projectTree: [], tagTree: [] },
};
const result = isRelatedModelDataValid(data as AppDataComplete);
expect(result).toBe(false);
expect(OP_LOG_SYNC_LOGGER.log).toHaveBeenCalledWith(
'[is-related-model-data-valid] Validity error info',
jasmine.objectContaining({
error: 'Missing task data for project',
tid: 'missing-task',
projectId: 'p1',
}),
);
expect(
JSON.stringify((OP_LOG_SYNC_LOGGER.log as jasmine.Spy).calls.allArgs()),
).not.toContain(privateProjectTitle);
});
it('should fail validation when task has non-existent repeatCfgId', () => {
const dataWithOrphanedRepeatCfgId: any = {
project: {

View file

@ -1,7 +1,8 @@
import { devError } from '../../util/dev-error';
import { environment } from '../../../environments/environment';
import { AppDataComplete } from '../model/model-config';
import { OpLog } from '../../core/log';
import type { SyncLogMeta } from '@sp/sync-core';
import { OP_LOG_SYNC_LOGGER } from '../core/sync-logger.adapter';
import {
MenuTreeKind,
MenuTreeTreeNode,
@ -19,12 +20,71 @@ import { TaskArchive } from '../../features/tasks/task.model';
let errorCount = 0;
let lastValidityError: string | undefined;
const SAFE_VALIDITY_INFO_KEYS = new Set([
'archiveLabel',
'defaultProjectId',
'id',
'ipId',
'nid',
'nodeKind',
'parentId',
'pid',
'projectId',
'repeatCfgId',
'subId',
'tagId',
'taskId',
'tid',
'treeType',
]);
const getValueType = (value: unknown): string => {
if (value === null) return 'null';
if (Array.isArray(value)) return 'array';
return typeof value;
};
const getValidityInfoMeta = (additionalInfo: unknown): SyncLogMeta => {
if (
additionalInfo === null ||
typeof additionalInfo !== 'object' ||
Array.isArray(additionalInfo)
) {
return {
additionalInfoType: getValueType(additionalInfo),
additionalInfoArrayLength: Array.isArray(additionalInfo)
? additionalInfo.length
: undefined,
};
}
const info = additionalInfo as Record<string, unknown>;
const meta: SyncLogMeta = {
additionalInfoType: 'object',
additionalInfoKeyCount: Object.keys(info).length,
};
for (const key of SAFE_VALIDITY_INFO_KEYS) {
const value = info[key];
if (
typeof value === 'string' ||
typeof value === 'number' ||
typeof value === 'boolean' ||
value === null
) {
meta[key] = value;
}
}
return meta;
};
export const isRelatedModelDataValid = (d: AppDataComplete): boolean => {
errorCount = 0;
lastValidityError = undefined; // Reset at start of each validation
if (!d) {
_validityError('Data is null or undefined', { d });
_validityError('Data is null or undefined');
return false;
}
@ -40,7 +100,9 @@ export const isRelatedModelDataValid = (d: AppDataComplete): boolean => {
!d.issueProvider ||
!d.reminders
) {
_validityError('Missing required model data in AppDataComplete', { d });
_validityError('Missing required model data in AppDataComplete', {
additionalInfoType: getValueType(d),
});
return false;
}
@ -95,14 +157,10 @@ export const getLastValidityError = (): string | undefined => lastValidityError;
const _validityError = (errTxt: string, additionalInfo?: unknown): void => {
if (additionalInfo) {
OpLog.log('Validity Error Info: ', additionalInfo);
if (environment.production) {
try {
OpLog.log('Validity Error Info string: ', JSON.stringify(additionalInfo));
} catch (e) {
OpLog.warn('Failed to stringify validity error info:', e);
}
}
OP_LOG_SYNC_LOGGER.log('[is-related-model-data-valid] Validity error info', {
error: errTxt,
...getValidityInfoMeta(additionalInfo),
});
}
if (errorCount <= 3) {
devError(errTxt);
@ -199,7 +257,7 @@ const validateTasksToProjectsAndTags = (
for (const pid of d.project.ids) {
const project = d.project.entities[pid];
if (!project) {
_validityError('No project', { pid, d });
_validityError('No project', { pid });
return false;
}
@ -214,15 +272,18 @@ const validateTasksToProjectsAndTags = (
for (const tid of projectTaskSet) {
const task = d.task.entities[tid];
if (!task) {
_validityError(`Missing task data (tid: ${tid}) for Project ${project.title}`, {
project,
d,
_validityError('Missing task data for project', {
tid,
projectId: project.id,
});
return false;
}
if (task.projectId !== project.id) {
_validityError('Inconsistent task projectId', { task, project, d });
_validityError('Inconsistent task projectId', {
taskId: task.id,
projectId: project.id,
});
return false;
}
}
@ -232,20 +293,26 @@ const validateTasksToProjectsAndTags = (
for (const tid of d.task.ids) {
const task = d.task.entities[tid];
if (!task) {
_validityError('Orphaned task ID in task.ids (no matching entity)', { tid, d });
_validityError('Orphaned task ID in task.ids (no matching entity)', { tid });
return false;
}
// Check project reference
if (task.projectId && !projectIds.has(task.projectId)) {
_validityError(`projectId ${task.projectId} from task not existing`, { task, d });
_validityError('projectId from task not existing', {
taskId: task.id,
projectId: task.projectId,
});
return false;
}
// Check tag references
for (const tagId of task.tagIds) {
if (!tagIds.has(tagId)) {
_validityError(`tagId "${tagId}" from task not existing`, { task, d });
_validityError('tagId from task not existing', {
taskId: task.id,
tagId,
});
return false;
}
}
@ -255,8 +322,8 @@ const validateTasksToProjectsAndTags = (
_validityError(
`repeatCfgId "${task.repeatCfgId}" from task "${task.id}" not existing`,
{
task,
d,
taskId: task.id,
repeatCfgId: task.repeatCfgId,
},
);
return false;
@ -264,7 +331,7 @@ const validateTasksToProjectsAndTags = (
// Check if task has project or tag
if (!task.parentId && !task.projectId && task.tagIds.length === 0) {
_validityError(`Task without project or tag`, { task, d });
_validityError(`Task without project or tag`, { taskId: task.id });
return false;
}
}
@ -299,10 +366,10 @@ const validateTasksToProjectsAndTags = (
for (const tid of tag.taskIds) {
if (!taskIds.has(tid)) {
_validityError(
`Inconsistent Task State: Missing task id ${tid} for Tag ${tag.title}`,
{ tag, d },
);
_validityError(`Inconsistent Task State: Missing task id for tag`, {
tagId: tag.id,
tid,
});
return false;
}
}
@ -324,15 +391,18 @@ const validateNotes = (
for (const nid of project.noteIds) {
const note = d.note.entities[nid];
if (!note) {
_validityError(`Missing note data (tid: ${nid}) for Project ${project.title}`, {
project,
d,
_validityError('Missing note data for project', {
nid,
projectId: project.id,
});
return false;
}
if (note.projectId !== project.id) {
_validityError('Inconsistent note projectId', { note, project, d });
_validityError('Inconsistent note projectId', {
nid: note.id,
projectId: project.id,
});
return false;
}
}
@ -343,7 +413,7 @@ const validateNotes = (
if (!noteIds.has(nid)) {
_validityError(
`Inconsistent Note State: Missing note id ${nid} for note.todayOrder`,
{ d },
{ nid },
);
return false;
}
@ -366,8 +436,8 @@ const validateSubTasks = (
// Check if parent exists
if (task.parentId && !d.task.entities[task.parentId]) {
_validityError(`Inconsistent Task State: Lonely Sub Task in Today ${task.id}`, {
task,
d,
taskId: task.id,
parentId: task.parentId,
});
return false;
}
@ -377,7 +447,7 @@ const validateSubTasks = (
if (!d.task.entities[subId]) {
_validityError(
`Inconsistent Task State: Missing sub task data in today ${subId}`,
{ task, d },
{ taskId: task.id, subId },
);
return false;
}
@ -396,8 +466,8 @@ const validateSubTasks = (
_validityError(
`Inconsistent Task State: Lonely Sub Task in Archive ${task.id}`,
{
task,
d,
taskId: task.id,
parentId: task.parentId,
},
);
return false;
@ -410,7 +480,7 @@ const validateSubTasks = (
if (!d.archiveOld.task?.entities?.[subId]) {
_validityError(
`Inconsistent Task State: Missing sub task data in archive ${subId}`,
{ task, d },
{ taskId: task.id, subId },
);
return false;
}
@ -431,8 +501,8 @@ const validateSubTasks = (
_validityError(
`Inconsistent Task State: Lonely Sub Task in Old Archive ${task.id}`,
{
task,
d,
taskId: task.id,
parentId: task.parentId,
},
);
return false;
@ -445,7 +515,7 @@ const validateSubTasks = (
if (!d.archiveYoung.task?.entities?.[subId]) {
_validityError(
`Inconsistent Task State: Missing sub task data in old archive ${subId}`,
{ task, d },
{ taskId: task.id, subId },
);
return false;
}
@ -463,7 +533,7 @@ const validateIssueProviders = (d: AppDataComplete, projectIds: Set<string>): bo
if (ip && ip.defaultProjectId && !projectIds.has(ip.defaultProjectId)) {
_validityError(
`defaultProjectId ${ip.defaultProjectId} from issueProvider not existing`,
{ ip, d },
{ ipId: ip.id, defaultProjectId: ip.defaultProjectId },
);
return false;
}
@ -489,7 +559,7 @@ const validateMenuTree = (
): boolean => {
for (const node of nodes) {
if (!node || typeof node !== 'object') {
_validityError(`Invalid menuTree node in ${treeType}`, { node, d });
_validityError(`Invalid menuTree node in ${treeType}`, { treeType });
return false;
}
@ -497,16 +567,18 @@ const validateMenuTree = (
// Validate folder structure
if (!node.id || !node.name) {
_validityError(`Invalid folder node in ${treeType} - missing id or name`, {
node,
d,
treeType,
id: node.id,
nodeKind: node.k,
});
return false;
}
if (!Array.isArray(node.children)) {
_validityError(`Invalid folder node in ${treeType} - children not array`, {
node,
d,
treeType,
id: node.id,
nodeKind: node.k,
});
return false;
}
@ -518,33 +590,43 @@ const validateMenuTree = (
} else if (treeType === 'projectTree' && node.k === MenuTreeKind.PROJECT) {
// Validate project reference
if (!node.id) {
_validityError(`Project node in menuTree missing id`, { node, d });
_validityError(`Project node in menuTree missing id`, {
treeType,
nodeKind: node.k,
});
return false;
}
if (!projectIds.has(node.id)) {
_validityError(
`Orphaned project reference in menuTree - project ${node.id} doesn't exist`,
{ node, treeType, d },
{ id: node.id, treeType, nodeKind: node.k },
);
return false;
}
} else if (treeType === 'tagTree' && node.k === MenuTreeKind.TAG) {
// Validate tag reference
if (!node.id) {
_validityError(`Tag node in menuTree missing id`, { node, d });
_validityError(`Tag node in menuTree missing id`, {
treeType,
nodeKind: node.k,
});
return false;
}
if (!tagIds.has(node.id)) {
_validityError(
`Orphaned tag reference in menuTree - tag ${node.id} doesn't exist`,
{ node, treeType, d },
{ id: node.id, treeType, nodeKind: node.k },
);
return false;
}
} else {
_validityError(`Invalid node kind in ${treeType}: ${node.k}`, { node, d });
_validityError(`Invalid node kind in ${treeType}`, {
treeType,
id: node.id,
nodeKind: node.k,
});
return false;
}
}
@ -554,7 +636,7 @@ const validateMenuTree = (
// Validate projectTree
if (d.menuTree?.projectTree) {
if (!Array.isArray(d.menuTree.projectTree)) {
_validityError('menuTree.projectTree is not an array', { d });
_validityError('menuTree.projectTree is not an array');
return false;
}
if (!validateTreeNodes(d.menuTree.projectTree, 'projectTree')) {
@ -565,7 +647,7 @@ const validateMenuTree = (
// Validate tagTree
if (d.menuTree?.tagTree) {
if (!Array.isArray(d.menuTree.tagTree)) {
_validityError('menuTree.tagTree is not an array', { d });
_validityError('menuTree.tagTree is not an array');
return false;
}
if (!validateTreeNodes(d.menuTree.tagTree, 'tagTree')) {