mirror of
https://github.com/johannesjo/super-productivity.git
synced 2026-01-23 02:36:05 +00:00
fix(sync): improve robustness of sync and counter operations
Code review improvements addressing critical and high priority issues: Archive Handler: - Rollback BOTH archiveYoung and archiveOld on flush failure - Prevents data loss when partial write occurs Cache Invalidation: - Add _unsyncedCache invalidation in deleteOpsWhere - Prevents stale data when deleted ops include unsynced operations Simple Counter: - Extract _getCounterValue helper to reduce code duplication - Use selectSimpleCounterById (O(1)) instead of selectAllSimpleCounters+find (O(n)) - Update tests to properly mock both selectors Operation Log Sync: - Add infinite loop prevention when force download returns no clocks - Add GREATER_THAN corruption detection (treats as CONCURRENT to be safe) ESLint Hydration Guard Rule: - Fix combineLatest detection at root level vs nested in operator callbacks - Add comprehensive test suite (17 test cases) E2E Tests: - Fix flaky reminders-schedule-page tests (tasks disappear after scheduling)
This commit is contained in:
Johannes Millan
parent
8135953836
commit
3d4bfb5637
8 changed files with 521 additions and 135 deletions
|
|
@ -2,7 +2,6 @@ import type { Locator, Page } from '@playwright/test';
|
|||
import { test, expect } from '../../fixtures/test.fixture';
|
||||
|
||||
const TASK = 'task';
|
||||
const TASK_SCHEDULE_BTN = '.ico-btn.schedule-btn';
|
||||
const SCHEDULE_DIALOG = 'dialog-schedule-task';
|
||||
const SCHEDULE_DIALOG_TIME_INPUT = 'dialog-schedule-task input[type="time"]';
|
||||
const SCHEDULE_DIALOG_CONFIRM = 'mat-dialog-actions button:last-child';
|
||||
|
|
@ -94,10 +93,9 @@ test.describe('Reminders Schedule Page', () => {
|
|||
// Open detail panel to access schedule action
|
||||
await scheduleTaskViaDetailPanel(page, targetTask, scheduleTime);
|
||||
|
||||
// Wait for schedule indicator to appear on the task
|
||||
await targetTask
|
||||
.locator(TASK_SCHEDULE_BTN)
|
||||
.waitFor({ state: 'visible', timeout: 10000 });
|
||||
// Note: After scheduling with time, task may disappear from Today view
|
||||
// because scheduleTaskWithTime sets dueDay: undefined.
|
||||
// We skip checking for schedule button visibility and go directly to scheduled list.
|
||||
|
||||
// Navigate to scheduled page
|
||||
try {
|
||||
|
|
@ -139,10 +137,9 @@ test.describe('Reminders Schedule Page', () => {
|
|||
|
||||
await scheduleTaskViaDetailPanel(page, task, scheduleTime);
|
||||
|
||||
await task
|
||||
.locator(TASK_SCHEDULE_BTN)
|
||||
.first()
|
||||
.waitFor({ state: 'visible', timeout: 10000 });
|
||||
// Note: After scheduling with time, task may disappear from Today view
|
||||
// because scheduleTaskWithTime sets dueDay: undefined.
|
||||
// We skip checking for schedule button visibility.
|
||||
};
|
||||
|
||||
// Add and schedule first task
|
||||
|
|
@ -177,13 +174,9 @@ test.describe('Reminders Schedule Page', () => {
|
|||
|
||||
await scheduleTask(title2, scheduleTime2);
|
||||
|
||||
// Verify both tasks have schedule indicators
|
||||
// Use first() to avoid multiple element issues if there are duplicates
|
||||
const task1 = page.locator(TASK).filter({ hasText: title1 }).first();
|
||||
const task2 = page.locator(TASK).filter({ hasText: title2 }).first();
|
||||
|
||||
await expect(task1.locator(TASK_SCHEDULE_BTN).first()).toBeVisible();
|
||||
await expect(task2.locator(TASK_SCHEDULE_BTN).first()).toBeVisible();
|
||||
// Note: After scheduling with time, tasks disappear from Today view
|
||||
// (scheduleTaskWithTime sets dueDay: undefined).
|
||||
// We verify tasks in the scheduled list instead.
|
||||
|
||||
// Navigate to scheduled page
|
||||
try {
|
||||
|
|
|
|||
|
|
@ -151,33 +151,54 @@ module.exports = {
|
|||
};
|
||||
|
||||
/**
|
||||
* Check if a selector is used as a secondary source (e.g., in withLatestFrom, combineLatest).
|
||||
* Check if a selector is used as a secondary source (e.g., in withLatestFrom, combineLatestWith).
|
||||
* These selectors don't drive the observable - they're just providing supplemental data.
|
||||
*
|
||||
* IMPORTANT: This distinguishes between:
|
||||
* - `combineLatest([sel1, sel2]).pipe(...)` at root level → selectors ARE primary (not secondary)
|
||||
* - `actions$.pipe(combineLatestWith(sel))` → selector IS secondary
|
||||
* - `actions$.pipe(withLatestFrom(sel))` → selector IS secondary
|
||||
*/
|
||||
const isSecondarySource = (selectorNode) => {
|
||||
let current = selectorNode.parent;
|
||||
let passedFunctionBoundary = false;
|
||||
|
||||
while (current) {
|
||||
// Track if we've passed a function boundary (indicating we're inside an operator callback)
|
||||
if (
|
||||
current.type === 'ArrowFunctionExpression' ||
|
||||
current.type === 'FunctionExpression'
|
||||
) {
|
||||
passedFunctionBoundary = true;
|
||||
}
|
||||
|
||||
if (current.type === 'CallExpression') {
|
||||
const callee = current.callee;
|
||||
if (callee.type === 'Identifier') {
|
||||
const opName = callee.name;
|
||||
// Operators where selectors are secondary sources
|
||||
if (
|
||||
[
|
||||
'withLatestFrom',
|
||||
'combineLatest',
|
||||
'combineLatestWith',
|
||||
'forkJoin',
|
||||
'zip',
|
||||
'zipWith',
|
||||
].includes(opName)
|
||||
) {
|
||||
// Check if the selector is in the arguments of this operator
|
||||
|
||||
// withLatestFrom is ALWAYS secondary (only used inside pipe chains)
|
||||
if (opName === 'withLatestFrom') {
|
||||
if (current.arguments.some((arg) => containsNode(arg, selectorNode))) {
|
||||
return true;
|
||||
}
|
||||
}
|
||||
|
||||
// combineLatestWith, zipWith are ALWAYS secondary (only used inside pipe chains)
|
||||
if (['combineLatestWith', 'zipWith'].includes(opName)) {
|
||||
if (current.arguments.some((arg) => containsNode(arg, selectorNode))) {
|
||||
return true;
|
||||
}
|
||||
}
|
||||
|
||||
// combineLatest, forkJoin, zip at ROOT level (not inside a function) → NOT secondary
|
||||
// But if inside a function (like switchMap callback) → IS secondary
|
||||
if (['combineLatest', 'forkJoin', 'zip'].includes(opName)) {
|
||||
if (current.arguments.some((arg) => containsNode(arg, selectorNode))) {
|
||||
// Only secondary if we're inside an operator callback
|
||||
return passedFunctionBoundary;
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
current = current.parent;
|
||||
|
|
@ -297,6 +318,13 @@ module.exports = {
|
|||
findPrimarySelectorCalls(node.argument, effectFnNode, results);
|
||||
}
|
||||
|
||||
// Handle ArrayExpression (e.g., combineLatest([sel1, sel2]))
|
||||
if (node.type === 'ArrayExpression') {
|
||||
for (const element of node.elements || []) {
|
||||
findPrimarySelectorCalls(element, effectFnNode, results);
|
||||
}
|
||||
}
|
||||
|
||||
return results;
|
||||
};
|
||||
|
||||
|
|
|
|||
243
eslint-local-rules/rules/require-hydration-guard.spec.js
Normal file
243
eslint-local-rules/rules/require-hydration-guard.spec.js
Normal file
|
|
@ -0,0 +1,243 @@
|
|||
/**
|
||||
* Tests for require-hydration-guard ESLint rule
|
||||
*/
|
||||
const { RuleTester } = require('eslint');
|
||||
const rule = require('./require-hydration-guard');
|
||||
|
||||
const ruleTester = new RuleTester({
|
||||
parserOptions: {
|
||||
ecmaVersion: 2020,
|
||||
sourceType: 'module',
|
||||
},
|
||||
});
|
||||
|
||||
ruleTester.run('require-hydration-guard', rule, {
|
||||
valid: [
|
||||
// Action-based effect - should NOT flag (actions$.pipe is primary source)
|
||||
{
|
||||
code: `
|
||||
createEffect(() =>
|
||||
this._actions$.pipe(
|
||||
ofType(someAction),
|
||||
withLatestFrom(this._store$.select(mySelector)),
|
||||
map(([action, data]) => anotherAction())
|
||||
)
|
||||
)
|
||||
`,
|
||||
},
|
||||
|
||||
// Selector with skipDuringSync guard - should NOT flag
|
||||
{
|
||||
code: `
|
||||
createEffect(() =>
|
||||
this._store$.select(mySelector).pipe(
|
||||
skipDuringSync(),
|
||||
tap(() => this.doSomething())
|
||||
)
|
||||
)
|
||||
`,
|
||||
},
|
||||
|
||||
// Selector with isApplyingRemoteOps guard - should NOT flag
|
||||
{
|
||||
code: `
|
||||
createEffect(() =>
|
||||
this._store$.select(mySelector).pipe(
|
||||
filter(() => !this.hydrationState.isApplyingRemoteOps()),
|
||||
tap(() => this.doSomething())
|
||||
)
|
||||
)
|
||||
`,
|
||||
},
|
||||
|
||||
// Nested selector inside switchMap - should NOT flag (protected by outer stream)
|
||||
{
|
||||
code: `
|
||||
createEffect(() =>
|
||||
this._actions$.pipe(
|
||||
ofType(someAction),
|
||||
switchMap(() => this._store$.select(mySelector)),
|
||||
map(data => anotherAction())
|
||||
)
|
||||
)
|
||||
`,
|
||||
},
|
||||
|
||||
// Selector in withLatestFrom (secondary source) - should NOT flag
|
||||
{
|
||||
code: `
|
||||
createEffect(() =>
|
||||
this._actions$.pipe(
|
||||
ofType(someAction),
|
||||
withLatestFrom(this._store$.select(selectorA), this._store$.select(selectorB)),
|
||||
map(([action, a, b]) => anotherAction())
|
||||
)
|
||||
)
|
||||
`,
|
||||
},
|
||||
|
||||
// Selector in combineLatestWith (secondary source, inside operator chain) - should NOT flag
|
||||
{
|
||||
code: `
|
||||
createEffect(() =>
|
||||
this._actions$.pipe(
|
||||
ofType(someAction),
|
||||
combineLatestWith(this._store$.select(mySelector)),
|
||||
map(([action, data]) => anotherAction())
|
||||
)
|
||||
)
|
||||
`,
|
||||
},
|
||||
|
||||
// Using store instead of _store$
|
||||
{
|
||||
code: `
|
||||
createEffect(() =>
|
||||
this.store.select(mySelector).pipe(
|
||||
skipDuringSync(),
|
||||
tap(() => this.doSomething())
|
||||
)
|
||||
)
|
||||
`,
|
||||
},
|
||||
|
||||
// Using store$ property
|
||||
{
|
||||
code: `
|
||||
createEffect(() =>
|
||||
this.store$.select(mySelector).pipe(
|
||||
filter(() => !this.hydrationState.isApplyingRemoteOps()),
|
||||
tap(() => this.doSomething())
|
||||
)
|
||||
)
|
||||
`,
|
||||
},
|
||||
|
||||
// Guard in second pipe call - should NOT flag
|
||||
{
|
||||
code: `
|
||||
createEffect(() =>
|
||||
this._store$.select(mySelector)
|
||||
.pipe(map(x => x))
|
||||
.pipe(skipDuringSync())
|
||||
.pipe(tap(() => this.doSomething()))
|
||||
)
|
||||
`,
|
||||
},
|
||||
|
||||
// Not a createEffect call - should NOT flag
|
||||
{
|
||||
code: `
|
||||
someOtherFunction(() =>
|
||||
this._store$.select(mySelector).pipe(
|
||||
tap(() => this.doSomething())
|
||||
)
|
||||
)
|
||||
`,
|
||||
},
|
||||
|
||||
// combineLatest INSIDE switchMap - should NOT flag (nested in operator callback)
|
||||
{
|
||||
code: `
|
||||
createEffect(() =>
|
||||
this._actions$.pipe(
|
||||
ofType(someAction),
|
||||
switchMap(() =>
|
||||
combineLatest([
|
||||
this._store$.select(selectorA),
|
||||
this._store$.select(selectorB)
|
||||
])
|
||||
),
|
||||
map(([a, b]) => anotherAction())
|
||||
)
|
||||
)
|
||||
`,
|
||||
},
|
||||
],
|
||||
|
||||
invalid: [
|
||||
// Selector without guard - SHOULD flag
|
||||
{
|
||||
code: `
|
||||
createEffect(() =>
|
||||
this._store$.select(mySelector).pipe(
|
||||
tap(() => this.doSomething())
|
||||
)
|
||||
)
|
||||
`,
|
||||
errors: [{ messageId: 'missingHydrationGuard' }],
|
||||
},
|
||||
|
||||
// _store$ variant without guard - SHOULD flag
|
||||
{
|
||||
code: `
|
||||
createEffect(() =>
|
||||
this._store$.select(selector).pipe(
|
||||
map(x => x)
|
||||
)
|
||||
)
|
||||
`,
|
||||
errors: [{ messageId: 'missingHydrationGuard' }],
|
||||
},
|
||||
|
||||
// store (not _store$) without guard - SHOULD flag
|
||||
{
|
||||
code: `
|
||||
createEffect(() =>
|
||||
this.store.select(mySelector).pipe(
|
||||
filter(x => x !== null),
|
||||
tap(() => this.doSomething())
|
||||
)
|
||||
)
|
||||
`,
|
||||
errors: [{ messageId: 'missingHydrationGuard' }],
|
||||
},
|
||||
|
||||
// Multiple pipe calls without guard - SHOULD flag
|
||||
{
|
||||
code: `
|
||||
createEffect(() =>
|
||||
this._store$.select(mySelector)
|
||||
.pipe(map(x => x))
|
||||
.pipe(tap(() => this.doSomething()))
|
||||
)
|
||||
`,
|
||||
errors: [{ messageId: 'missingHydrationGuard' }],
|
||||
},
|
||||
|
||||
// Complex pipe chain without guard - SHOULD flag
|
||||
{
|
||||
code: `
|
||||
createEffect(() =>
|
||||
this._store$.select(selectFeatureState).pipe(
|
||||
distinctUntilChanged(),
|
||||
filter(state => state.isReady),
|
||||
map(state => someAction({ data: state.data }))
|
||||
)
|
||||
)
|
||||
`,
|
||||
errors: [{ messageId: 'missingHydrationGuard' }],
|
||||
},
|
||||
|
||||
// combineLatest at ROOT level without guard - SHOULD flag (both selectors are primary)
|
||||
// Note: This is different from combineLatestWith inside a pipe chain
|
||||
{
|
||||
code: `
|
||||
createEffect(() =>
|
||||
combineLatest([
|
||||
this._store$.select(selectorA),
|
||||
this._store$.select(selectorB)
|
||||
]).pipe(
|
||||
map(([a, b]) => someAction({ a, b }))
|
||||
)
|
||||
)
|
||||
`,
|
||||
errors: [
|
||||
{ messageId: 'missingHydrationGuard' },
|
||||
{ messageId: 'missingHydrationGuard' },
|
||||
],
|
||||
},
|
||||
],
|
||||
});
|
||||
|
||||
console.log('All tests passed!');
|
||||
|
|
@ -233,37 +233,78 @@ export class ArchiveOperationHandler {
|
|||
const timestamp = (action as ReturnType<typeof flushYoungToOld>).timestamp;
|
||||
const pfapi = this._getPfapiService();
|
||||
|
||||
const archiveYoung = await pfapi.m.archiveYoung.load();
|
||||
const archiveOld = await pfapi.m.archiveOld.load();
|
||||
// Load original state for potential rollback
|
||||
const originalArchiveYoung = await pfapi.m.archiveYoung.load();
|
||||
const originalArchiveOld = await pfapi.m.archiveOld.load();
|
||||
|
||||
const newSorted = sortTimeTrackingAndTasksFromArchiveYoungToOld({
|
||||
archiveYoung,
|
||||
archiveOld,
|
||||
archiveYoung: originalArchiveYoung,
|
||||
archiveOld: originalArchiveOld,
|
||||
threshold: ARCHIVE_TASK_YOUNG_TO_OLD_THRESHOLD,
|
||||
now: timestamp,
|
||||
});
|
||||
|
||||
await pfapi.m.archiveYoung.save(
|
||||
{
|
||||
...newSorted.archiveYoung,
|
||||
lastTimeTrackingFlush: timestamp,
|
||||
},
|
||||
{
|
||||
isUpdateRevAndLastUpdate: true,
|
||||
isIgnoreDBLock: true, // Remote ops: DB is locked during sync processing
|
||||
},
|
||||
);
|
||||
try {
|
||||
await pfapi.m.archiveYoung.save(
|
||||
{
|
||||
...newSorted.archiveYoung,
|
||||
lastTimeTrackingFlush: timestamp,
|
||||
},
|
||||
{
|
||||
isUpdateRevAndLastUpdate: true,
|
||||
isIgnoreDBLock: true, // Remote ops: DB is locked during sync processing
|
||||
},
|
||||
);
|
||||
|
||||
await pfapi.m.archiveOld.save(
|
||||
{
|
||||
...newSorted.archiveOld,
|
||||
lastTimeTrackingFlush: timestamp,
|
||||
},
|
||||
{
|
||||
isUpdateRevAndLastUpdate: true,
|
||||
isIgnoreDBLock: true, // Remote ops: DB is locked during sync processing
|
||||
},
|
||||
);
|
||||
await pfapi.m.archiveOld.save(
|
||||
{
|
||||
...newSorted.archiveOld,
|
||||
lastTimeTrackingFlush: timestamp,
|
||||
},
|
||||
{
|
||||
isUpdateRevAndLastUpdate: true,
|
||||
isIgnoreDBLock: true, // Remote ops: DB is locked during sync processing
|
||||
},
|
||||
);
|
||||
} catch (e) {
|
||||
// Attempt rollback: restore BOTH archiveYoung and archiveOld to original state
|
||||
Log.err('Archive flush failed, attempting rollback...', e);
|
||||
const rollbackErrors: Error[] = [];
|
||||
|
||||
// Rollback archiveYoung
|
||||
try {
|
||||
if (originalArchiveYoung) {
|
||||
await pfapi.m.archiveYoung.save(originalArchiveYoung, {
|
||||
isUpdateRevAndLastUpdate: true,
|
||||
isIgnoreDBLock: true,
|
||||
});
|
||||
}
|
||||
} catch (rollbackErr) {
|
||||
rollbackErrors.push(rollbackErr as Error);
|
||||
}
|
||||
|
||||
// Rollback archiveOld
|
||||
try {
|
||||
if (originalArchiveOld) {
|
||||
await pfapi.m.archiveOld.save(originalArchiveOld, {
|
||||
isUpdateRevAndLastUpdate: true,
|
||||
isIgnoreDBLock: true,
|
||||
});
|
||||
}
|
||||
} catch (rollbackErr) {
|
||||
rollbackErrors.push(rollbackErr as Error);
|
||||
}
|
||||
|
||||
if (rollbackErrors.length > 0) {
|
||||
Log.err(
|
||||
'Archive flush rollback FAILED - archive may be inconsistent',
|
||||
rollbackErrors,
|
||||
);
|
||||
} else {
|
||||
Log.log('Archive flush rollback successful');
|
||||
}
|
||||
throw e; // Re-throw original error
|
||||
}
|
||||
|
||||
Log.log(
|
||||
'______________________\nFLUSHED ALL FROM ARCHIVE YOUNG TO OLD (via remote op handler)\n_______________________',
|
||||
|
|
|
|||
|
|
@ -418,13 +418,22 @@ export class OperationLogStoreService {
|
|||
const tx = this.db.transaction('ops', 'readwrite');
|
||||
const store = tx.objectStore('ops');
|
||||
let cursor = await store.openCursor();
|
||||
let deletedCount = 0;
|
||||
while (cursor) {
|
||||
if (predicate(cursor.value)) {
|
||||
await cursor.delete();
|
||||
deletedCount++;
|
||||
}
|
||||
cursor = await cursor.continue();
|
||||
}
|
||||
await tx.done;
|
||||
|
||||
// Invalidate caches if any ops were deleted to prevent stale data
|
||||
if (deletedCount > 0) {
|
||||
this._appliedOpIdsCache = null;
|
||||
this._cacheLastSeq = 0;
|
||||
this._invalidateUnsyncedCache();
|
||||
}
|
||||
}
|
||||
|
||||
async getLastSeq(): Promise<number> {
|
||||
|
|
|
|||
|
|
@ -381,8 +381,20 @@ export class OperationLogSyncService {
|
|||
forceDownloadResult.allOpClocks,
|
||||
);
|
||||
} else {
|
||||
// No extra clocks from force download, resolve with what we have
|
||||
mergedOpsCreated += await this._resolveStaleLocalOps(stillPendingOps);
|
||||
// Force download returned no clocks but we have concurrent ops.
|
||||
// This is an unrecoverable edge case - cannot safely resolve without server clocks.
|
||||
// Mark ops as rejected to prevent infinite retry loop.
|
||||
OpLog.err(
|
||||
`OperationLogSyncService: Force download returned no clocks. ` +
|
||||
`Cannot safely resolve ${stillPendingOps.length} concurrent ops. Marking as rejected.`,
|
||||
);
|
||||
for (const { opId } of stillPendingOps) {
|
||||
await this.opLogStore.markRejected([opId]);
|
||||
}
|
||||
this.snackService.open({
|
||||
type: 'ERROR',
|
||||
msg: T.F.SYNC.S.CONFLICT_RESOLUTION_FAILED,
|
||||
});
|
||||
}
|
||||
}
|
||||
} else {
|
||||
|
|
@ -1358,18 +1370,21 @@ export class OperationLogSyncService {
|
|||
|
||||
/**
|
||||
* Adjusts comparison result for potential per-entity clock corruption.
|
||||
* Converts LESS_THAN to CONCURRENT if corruption is suspected.
|
||||
* Converts LESS_THAN or GREATER_THAN to CONCURRENT if corruption is suspected.
|
||||
*
|
||||
* ## Known Limitation
|
||||
* This only handles LESS_THAN corruption (where local ops would be incorrectly
|
||||
* skipped). GREATER_THAN corruption (where remote ops would be skipped) is NOT
|
||||
* handled because:
|
||||
* 1. It's extremely rare (requires specific corruption pattern)
|
||||
* 2. Local pending ops will eventually sync on next sync cycle
|
||||
* 3. Adding complexity for this edge case isn't worth the risk of introducing bugs
|
||||
* ## Corruption Detection
|
||||
* Potential corruption is detected when:
|
||||
* - Entity has pending local ops (we made changes)
|
||||
* - But has no snapshot clock AND empty local frontier
|
||||
* - This suggests the clock data was lost/corrupted
|
||||
*
|
||||
* If you encounter data loss from GREATER_THAN corruption, a full re-sync
|
||||
* (clear local data and download from server) will restore consistency.
|
||||
* ## Safety Behavior
|
||||
* When corruption is suspected:
|
||||
* - LESS_THAN → CONCURRENT: Prevents incorrectly skipping local ops
|
||||
* - GREATER_THAN → CONCURRENT: Prevents incorrectly skipping remote ops
|
||||
*
|
||||
* Converting to CONCURRENT forces conflict resolution, which is safer than
|
||||
* silently skipping either local or remote operations.
|
||||
*/
|
||||
private _adjustForClockCorruption(
|
||||
comparison: VectorClockComparison,
|
||||
|
|
@ -1390,6 +1405,15 @@ export class OperationLogSyncService {
|
|||
);
|
||||
return VectorClockComparison.CONCURRENT;
|
||||
}
|
||||
|
||||
if (potentialCorruption && comparison === VectorClockComparison.GREATER_THAN) {
|
||||
OpLog.warn(
|
||||
`OperationLogSyncService: Converting GREATER_THAN to CONCURRENT for entity ${entityKey} due to potential clock corruption. ` +
|
||||
`Remote op will be processed via conflict resolution instead of being skipped.`,
|
||||
);
|
||||
return VectorClockComparison.CONCURRENT;
|
||||
}
|
||||
|
||||
return comparison;
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -36,6 +36,25 @@ describe('SimpleCounterService', () => {
|
|||
...partial,
|
||||
});
|
||||
|
||||
/**
|
||||
* Helper to mock counters for both selectAllSimpleCounters and selectSimpleCounterById.
|
||||
* Since the service now uses selectSimpleCounterById (O(1) lookup), we need to mock both.
|
||||
*/
|
||||
const mockCounters = (counters: SimpleCounter[]): void => {
|
||||
store.overrideSelector(selectAllSimpleCounters, counters);
|
||||
// For selectSimpleCounterById, we override the base state since it uses props
|
||||
const entities: Record<string, SimpleCounter> = {};
|
||||
for (const counter of counters) {
|
||||
entities[counter.id] = counter;
|
||||
}
|
||||
store.setState({
|
||||
simpleCounter: {
|
||||
ids: counters.map((c) => c.id),
|
||||
entities,
|
||||
},
|
||||
});
|
||||
};
|
||||
|
||||
beforeEach(() => {
|
||||
TestBed.configureTestingModule({
|
||||
providers: [
|
||||
|
|
@ -60,10 +79,11 @@ describe('SimpleCounterService', () => {
|
|||
service = TestBed.inject(SimpleCounterService);
|
||||
dispatchSpy = spyOn(store, 'dispatch').and.callThrough();
|
||||
|
||||
// Mock the selector to return counters
|
||||
store.overrideSelector(selectAllSimpleCounters, [
|
||||
// Mock the counters with CURRENT state (BEFORE increment)
|
||||
// New behavior: we read state first, calculate new value, then dispatch both actions
|
||||
mockCounters([
|
||||
createCounter('counter1', {
|
||||
countOnDay: { '2024-01-15': 6 },
|
||||
countOnDay: { '2024-01-15': 5 },
|
||||
}),
|
||||
]);
|
||||
});
|
||||
|
|
@ -92,6 +112,7 @@ describe('SimpleCounterService', () => {
|
|||
|
||||
// Should dispatch both the increment action AND the sync action
|
||||
expect(dispatchSpy.calls.count()).toBe(2);
|
||||
// New behavior: reads state (5) BEFORE dispatch, calculates newVal = 5 + 1 = 6
|
||||
expect(dispatchSpy).toHaveBeenCalledWith(
|
||||
setSimpleCounterCounterToday({
|
||||
id: 'counter1',
|
||||
|
|
@ -104,10 +125,11 @@ describe('SimpleCounterService', () => {
|
|||
|
||||
describe('decreaseCounterToday', () => {
|
||||
beforeEach(() => {
|
||||
// Mock the selector to return the counter after decrement
|
||||
store.overrideSelector(selectAllSimpleCounters, [
|
||||
// Mock the counters BEFORE decrement
|
||||
// New behavior: reads state first, calculates new value, then dispatches
|
||||
mockCounters([
|
||||
createCounter('counter1', {
|
||||
countOnDay: { '2024-01-15': 4 },
|
||||
countOnDay: { '2024-01-15': 5 },
|
||||
}),
|
||||
]);
|
||||
});
|
||||
|
|
@ -131,6 +153,7 @@ describe('SimpleCounterService', () => {
|
|||
|
||||
// Should dispatch both the decrement action AND the sync action
|
||||
expect(dispatchSpy.calls.count()).toBe(2);
|
||||
// New behavior: reads state (5) BEFORE dispatch, calculates newVal = 5 - 1 = 4
|
||||
expect(dispatchSpy).toHaveBeenCalledWith(
|
||||
setSimpleCounterCounterToday({
|
||||
id: 'counter1',
|
||||
|
|
@ -215,7 +238,7 @@ describe('SimpleCounterService', () => {
|
|||
|
||||
describe('deleteSimpleCounters', () => {
|
||||
beforeEach(() => {
|
||||
store.overrideSelector(selectAllSimpleCounters, [
|
||||
mockCounters([
|
||||
createCounter('counter1', { countOnDay: { '2024-01-15': 6 } }),
|
||||
createCounter('counter2', { countOnDay: { '2024-01-15': 3 } }),
|
||||
]);
|
||||
|
|
@ -262,7 +285,7 @@ describe('SimpleCounterService', () => {
|
|||
|
||||
describe('stopwatch sync with absolute values', () => {
|
||||
beforeEach(() => {
|
||||
store.overrideSelector(selectAllSimpleCounters, [
|
||||
mockCounters([
|
||||
createCounter('stopwatch1', {
|
||||
type: SimpleCounterType.StopWatch,
|
||||
countOnDay: { '2024-01-15': 20000 }, // 20 seconds
|
||||
|
|
@ -282,7 +305,7 @@ describe('SimpleCounterService', () => {
|
|||
it('should use setSimpleCounterCounterToday for stopwatch sync (not relative duration)', fakeAsync(() => {
|
||||
// Verify the sync uses absolute values by checking the action type
|
||||
// When stopwatch syncs, it should dispatch setSimpleCounterCounterToday with absolute value
|
||||
store.overrideSelector(selectAllSimpleCounters, [
|
||||
mockCounters([
|
||||
createCounter('stopwatch1', {
|
||||
type: SimpleCounterType.StopWatch,
|
||||
countOnDay: { '2024-01-15': 20000 },
|
||||
|
|
@ -298,15 +321,17 @@ describe('SimpleCounterService', () => {
|
|||
|
||||
describe('click counter immediate sync - edge cases', () => {
|
||||
it('should sync with correct absolute value when counter starts from 0', fakeAsync(() => {
|
||||
store.overrideSelector(selectAllSimpleCounters, [
|
||||
// BEFORE increment: counter is at 0
|
||||
mockCounters([
|
||||
createCounter('counter1', {
|
||||
countOnDay: { '2024-01-15': 1 }, // After increment from 0
|
||||
countOnDay: { '2024-01-15': 0 },
|
||||
}),
|
||||
]);
|
||||
|
||||
service.increaseCounterToday('counter1', 1);
|
||||
tick();
|
||||
|
||||
// New behavior: reads state (0) BEFORE dispatch, calculates newVal = 0 + 1 = 1
|
||||
expect(dispatchSpy).toHaveBeenCalledWith(
|
||||
setSimpleCounterCounterToday({
|
||||
id: 'counter1',
|
||||
|
|
@ -317,15 +342,17 @@ describe('SimpleCounterService', () => {
|
|||
}));
|
||||
|
||||
it('should sync with 0 when decrementing to 0', fakeAsync(() => {
|
||||
store.overrideSelector(selectAllSimpleCounters, [
|
||||
// BEFORE decrement: counter is at 1
|
||||
mockCounters([
|
||||
createCounter('counter1', {
|
||||
countOnDay: { '2024-01-15': 0 }, // After decrement to 0
|
||||
countOnDay: { '2024-01-15': 1 },
|
||||
}),
|
||||
]);
|
||||
|
||||
service.decreaseCounterToday('counter1', 1);
|
||||
tick();
|
||||
|
||||
// New behavior: reads state (1) BEFORE dispatch, calculates newVal = max(0, 1 - 1) = 0
|
||||
expect(dispatchSpy).toHaveBeenCalledWith(
|
||||
setSimpleCounterCounterToday({
|
||||
id: 'counter1',
|
||||
|
|
@ -336,7 +363,8 @@ describe('SimpleCounterService', () => {
|
|||
}));
|
||||
|
||||
it('should handle multiple counters independently', fakeAsync(() => {
|
||||
store.overrideSelector(selectAllSimpleCounters, [
|
||||
// BEFORE increment: counters at 10 and 20
|
||||
mockCounters([
|
||||
createCounter('counter1', { countOnDay: { '2024-01-15': 10 } }),
|
||||
createCounter('counter2', { countOnDay: { '2024-01-15': 20 } }),
|
||||
]);
|
||||
|
|
@ -344,11 +372,11 @@ describe('SimpleCounterService', () => {
|
|||
service.increaseCounterToday('counter1', 1);
|
||||
tick();
|
||||
|
||||
// Should sync counter1 with its absolute value
|
||||
// Should sync counter1: reads 10, calculates 10 + 1 = 11
|
||||
expect(dispatchSpy).toHaveBeenCalledWith(
|
||||
setSimpleCounterCounterToday({
|
||||
id: 'counter1',
|
||||
newVal: 10,
|
||||
newVal: 11,
|
||||
today: '2024-01-15',
|
||||
}),
|
||||
);
|
||||
|
|
@ -357,20 +385,21 @@ describe('SimpleCounterService', () => {
|
|||
service.increaseCounterToday('counter2', 1);
|
||||
tick();
|
||||
|
||||
// Should sync counter2 with its absolute value
|
||||
// Should sync counter2: reads 20, calculates 20 + 1 = 21
|
||||
expect(dispatchSpy).toHaveBeenCalledWith(
|
||||
setSimpleCounterCounterToday({
|
||||
id: 'counter2',
|
||||
newVal: 20,
|
||||
newVal: 21,
|
||||
today: '2024-01-15',
|
||||
}),
|
||||
);
|
||||
}));
|
||||
|
||||
it('should handle increment by values greater than 1', fakeAsync(() => {
|
||||
store.overrideSelector(selectAllSimpleCounters, [
|
||||
// BEFORE increment: counter is at 10
|
||||
mockCounters([
|
||||
createCounter('counter1', {
|
||||
countOnDay: { '2024-01-15': 15 }, // After increment by 5
|
||||
countOnDay: { '2024-01-15': 10 },
|
||||
}),
|
||||
]);
|
||||
|
||||
|
|
@ -385,6 +414,7 @@ describe('SimpleCounterService', () => {
|
|||
}),
|
||||
);
|
||||
|
||||
// New behavior: reads state (10) BEFORE dispatch, calculates newVal = 10 + 5 = 15
|
||||
expect(dispatchSpy).toHaveBeenCalledWith(
|
||||
setSimpleCounterCounterToday({
|
||||
id: 'counter1',
|
||||
|
|
@ -394,32 +424,20 @@ describe('SimpleCounterService', () => {
|
|||
);
|
||||
}));
|
||||
|
||||
it('should not dispatch sync if counter not found', fakeAsync(() => {
|
||||
store.overrideSelector(selectAllSimpleCounters, []);
|
||||
it('should not dispatch anything if counter not found', fakeAsync(() => {
|
||||
mockCounters([]);
|
||||
|
||||
service.increaseCounterToday('nonexistent', 1);
|
||||
tick();
|
||||
|
||||
// Should dispatch the increment action
|
||||
expect(dispatchSpy).toHaveBeenCalledWith(
|
||||
increaseSimpleCounterCounterToday({
|
||||
id: 'nonexistent',
|
||||
increaseBy: 1,
|
||||
today: '2024-01-15',
|
||||
}),
|
||||
);
|
||||
|
||||
// Should NOT dispatch sync since counter not found
|
||||
const syncCalls = dispatchSpy.calls
|
||||
.allArgs()
|
||||
.filter(
|
||||
(args) => args[0].type === '[SimpleCounter] Set SimpleCounter Counter Today',
|
||||
);
|
||||
expect(syncCalls.length).toBe(0);
|
||||
// New behavior: reads state first, counter not found, returns early
|
||||
// Should NOT dispatch ANY actions since counter not found
|
||||
expect(dispatchSpy.calls.count()).toBe(0);
|
||||
}));
|
||||
|
||||
it('should handle missing countOnDay for today', fakeAsync(() => {
|
||||
store.overrideSelector(selectAllSimpleCounters, [
|
||||
// BEFORE increment: counter has no entry for today (defaults to 0)
|
||||
mockCounters([
|
||||
createCounter('counter1', {
|
||||
countOnDay: { '2024-01-14': 5 }, // Yesterday, not today
|
||||
}),
|
||||
|
|
@ -428,11 +446,11 @@ describe('SimpleCounterService', () => {
|
|||
service.increaseCounterToday('counter1', 1);
|
||||
tick();
|
||||
|
||||
// Should sync with 0 since today's count is missing
|
||||
// New behavior: reads state, today's count is 0 (undefined), calculates 0 + 1 = 1
|
||||
expect(dispatchSpy).toHaveBeenCalledWith(
|
||||
setSimpleCounterCounterToday({
|
||||
id: 'counter1',
|
||||
newVal: 0,
|
||||
newVal: 1,
|
||||
today: '2024-01-15',
|
||||
}),
|
||||
);
|
||||
|
|
|
|||
|
|
@ -5,6 +5,7 @@ import {
|
|||
selectEnabledAndToggledSimpleCounters,
|
||||
selectEnabledSimpleCounters,
|
||||
selectEnabledSimpleStopWatchCounters,
|
||||
selectSimpleCounterById,
|
||||
} from './store/simple-counter.reducer';
|
||||
import {
|
||||
addSimpleCounter,
|
||||
|
|
@ -180,18 +181,19 @@ export class SimpleCounterService implements OnDestroy {
|
|||
* This ensures remote clients get the correct value regardless of their current state.
|
||||
*/
|
||||
private _syncStopwatchAbsoluteValue(id: string, date: string): void {
|
||||
// Read current state synchronously via firstValueFrom
|
||||
firstValueFrom(this._store$.pipe(select(selectAllSimpleCounters))).then(
|
||||
(counters) => {
|
||||
const counter = counters.find((c) => c.id === id);
|
||||
// Read current state via selectSimpleCounterById (O(1) lookup)
|
||||
firstValueFrom(this._store$.pipe(select(selectSimpleCounterById, { id })))
|
||||
.then((counter) => {
|
||||
if (counter) {
|
||||
const newVal = counter.countOnDay[date] || 0;
|
||||
this._store$.dispatch(
|
||||
setSimpleCounterCounterToday({ id, newVal, today: date }),
|
||||
);
|
||||
}
|
||||
},
|
||||
);
|
||||
})
|
||||
.catch((error) => {
|
||||
console.error('[SimpleCounterService] Error syncing stopwatch value:', error);
|
||||
});
|
||||
}
|
||||
|
||||
updateAll(items: SimpleCounter[]): void {
|
||||
|
|
@ -207,36 +209,64 @@ export class SimpleCounterService implements OnDestroy {
|
|||
this._store$.dispatch(setSimpleCounterCounterForDate({ id, newVal, date }));
|
||||
}
|
||||
|
||||
async increaseCounterToday(id: string, increaseBy: number): Promise<void> {
|
||||
const today = this._dateService.todayStr();
|
||||
// Local UI update (non-persistent)
|
||||
this._store$.dispatch(increaseSimpleCounterCounterToday({ id, increaseBy, today }));
|
||||
|
||||
// Immediately sync with absolute value (persistent)
|
||||
const counters = await firstValueFrom(
|
||||
this._store$.pipe(select(selectAllSimpleCounters)),
|
||||
/**
|
||||
* Get current counter value for today using O(1) selector lookup.
|
||||
* Returns null if counter doesn't exist.
|
||||
*/
|
||||
private async _getCounterValue(id: string, today: string): Promise<number | null> {
|
||||
const counter = await firstValueFrom(
|
||||
this._store$.pipe(select(selectSimpleCounterById, { id })),
|
||||
);
|
||||
const counter = counters.find((c) => c.id === id);
|
||||
if (counter) {
|
||||
const newVal = counter.countOnDay[today] || 0;
|
||||
this._store$.dispatch(setSimpleCounterCounterToday({ id, newVal, today }));
|
||||
}
|
||||
if (!counter) return null;
|
||||
return counter.countOnDay[today] || 0;
|
||||
}
|
||||
|
||||
/**
|
||||
* Increase counter and sync with absolute value.
|
||||
*
|
||||
* Uses two-phase approach to ensure correct sync:
|
||||
* 1. Read current value FIRST to calculate correct new value
|
||||
* 2. Dispatch local update for immediate UI feedback (non-persistent)
|
||||
* 3. Dispatch sync action with calculated absolute value (persistent)
|
||||
*
|
||||
* This prevents race conditions where NgRx hasn't processed the local update
|
||||
* before we read the state for syncing.
|
||||
*/
|
||||
async increaseCounterToday(id: string, increaseBy: number): Promise<void> {
|
||||
const today = this._dateService.todayStr();
|
||||
const currentVal = await this._getCounterValue(id, today);
|
||||
if (currentVal === null) return;
|
||||
|
||||
const newVal = currentVal + increaseBy;
|
||||
|
||||
// Local UI update (non-persistent)
|
||||
this._store$.dispatch(increaseSimpleCounterCounterToday({ id, increaseBy, today }));
|
||||
// Sync with calculated absolute value (persistent)
|
||||
this._store$.dispatch(setSimpleCounterCounterToday({ id, newVal, today }));
|
||||
}
|
||||
|
||||
/**
|
||||
* Decrease counter and sync with absolute value.
|
||||
*
|
||||
* Uses two-phase approach to ensure correct sync:
|
||||
* 1. Read current value FIRST to calculate correct new value
|
||||
* 2. Dispatch local update for immediate UI feedback (non-persistent)
|
||||
* 3. Dispatch sync action with calculated absolute value (persistent)
|
||||
*
|
||||
* This prevents race conditions where NgRx hasn't processed the local update
|
||||
* before we read the state for syncing.
|
||||
*/
|
||||
async decreaseCounterToday(id: string, decreaseBy: number): Promise<void> {
|
||||
const today = this._dateService.todayStr();
|
||||
const currentVal = await this._getCounterValue(id, today);
|
||||
if (currentVal === null) return;
|
||||
|
||||
const newVal = Math.max(0, currentVal - decreaseBy);
|
||||
|
||||
// Local UI update (non-persistent)
|
||||
this._store$.dispatch(decreaseSimpleCounterCounterToday({ id, decreaseBy, today }));
|
||||
|
||||
// Immediately sync with absolute value (persistent)
|
||||
const counters = await firstValueFrom(
|
||||
this._store$.pipe(select(selectAllSimpleCounters)),
|
||||
);
|
||||
const counter = counters.find((c) => c.id === id);
|
||||
if (counter) {
|
||||
const newVal = counter.countOnDay[today] || 0;
|
||||
this._store$.dispatch(setSimpleCounterCounterToday({ id, newVal, today }));
|
||||
}
|
||||
// Sync with calculated absolute value (persistent)
|
||||
this._store$.dispatch(setSimpleCounterCounterToday({ id, newVal, today }));
|
||||
}
|
||||
|
||||
toggleCounter(id: string): void {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue