diff --git a/eslint-local-rules/rules/require-hydration-guard.js b/eslint-local-rules/rules/require-hydration-guard.js index 3d2adc994..f914109a7 100644 --- a/eslint-local-rules/rules/require-hydration-guard.js +++ b/eslint-local-rules/rules/require-hydration-guard.js @@ -14,7 +14,8 @@ * ## Solution * * Effects that USE SELECTORS AS THE PRIMARY SOURCE should include one of: - * - `skipDuringSync()` operator + * - `skipWhileApplyingRemoteOps()` operator (preferred) + * - `skipDuringSync()` operator (deprecated alias) * - `filter(() => !this.hydrationState.isApplyingRemoteOps())` * * ## What This Rule Does NOT Flag @@ -57,7 +58,7 @@ module.exports = { }, messages: { missingHydrationGuard: - 'Selector-based effect is missing hydration guard. Add skipDuringSync() or filter with isApplyingRemoteOps() to prevent duplicate operations during sync replay.', + 'Selector-based effect is missing hydration guard. Add skipWhileApplyingRemoteOps() or filter with isApplyingRemoteOps() to prevent duplicate operations during sync replay.', }, schema: [], }, @@ -267,7 +268,9 @@ module.exports = { // Check for hydration guards return ( - chainText.includes('skipDuringSync') || chainText.includes('isApplyingRemoteOps') + chainText.includes('skipWhileApplyingRemoteOps') || + chainText.includes('skipDuringSync') || + chainText.includes('isApplyingRemoteOps') ); }; diff --git a/eslint-local-rules/rules/require-hydration-guard.spec.js b/eslint-local-rules/rules/require-hydration-guard.spec.js index df1e93441..2ec0329ec 100644 --- a/eslint-local-rules/rules/require-hydration-guard.spec.js +++ b/eslint-local-rules/rules/require-hydration-guard.spec.js @@ -26,7 +26,19 @@ ruleTester.run('require-hydration-guard', rule, { `, }, - // Selector with skipDuringSync guard - should NOT flag + // Selector with skipWhileApplyingRemoteOps guard (preferred) - should NOT flag + { + code: ` + createEffect(() => + this._store$.select(mySelector).pipe( + skipWhileApplyingRemoteOps(), + tap(() => this.doSomething()) + ) + ) + `, + }, + + // Selector with skipDuringSync guard (deprecated alias) - should NOT flag { code: ` createEffect(() => diff --git a/src/app/features/domina-mode/store/domina-mode.effects.ts b/src/app/features/domina-mode/store/domina-mode.effects.ts index 088a74293..8c54d38b5 100644 --- a/src/app/features/domina-mode/store/domina-mode.effects.ts +++ b/src/app/features/domina-mode/store/domina-mode.effects.ts @@ -3,7 +3,7 @@ import { createEffect } from '@ngrx/effects'; import { LOCAL_ACTIONS } from '../../../util/local-actions.token'; import { EMPTY, Observable, timer } from 'rxjs'; import { distinctUntilChanged, switchMap, tap, withLatestFrom } from 'rxjs/operators'; -import { skipDuringSync } from '../../../util/skip-during-sync.operator'; +import { skipWhileApplyingRemoteOps } from '../../../util/skip-during-sync.operator'; import { Store } from '@ngrx/store'; import { selectIsDominaModeConfig } from '../../config/store/global-config.reducer'; import { selectCurrentTask } from '../../tasks/store/task.selectors'; @@ -17,7 +17,8 @@ export class DominaModeEffects { dominaMode$: Observable = createEffect( () => this._store$.select(selectIsDominaModeConfig).pipe( - skipDuringSync(), + // Outer guard: skip config changes during sync + skipWhileApplyingRemoteOps(), distinctUntilChanged(), switchMap((cfg) => cfg.isEnabled && cfg.voice diff --git a/src/app/features/focus-mode/store/focus-mode.effects.ts b/src/app/features/focus-mode/store/focus-mode.effects.ts index 7a54151b2..7c02cd18f 100644 --- a/src/app/features/focus-mode/store/focus-mode.effects.ts +++ b/src/app/features/focus-mode/store/focus-mode.effects.ts @@ -3,7 +3,7 @@ import { createEffect, ofType } from '@ngrx/effects'; import { LOCAL_ACTIONS } from '../../../util/local-actions.token'; import { Store } from '@ngrx/store'; import { combineLatest, EMPTY, of } from 'rxjs'; -import { skipDuringSync } from '../../../util/skip-during-sync.operator'; +import { skipWhileApplyingRemoteOps } from '../../../util/skip-during-sync.operator'; import { distinctUntilChanged, filter, @@ -61,12 +61,12 @@ export class FocusModeEffects { this.store.select(selectFocusModeConfig), this.store.select(selectIsFocusModeEnabled), ]).pipe( - // Prevent auto-showing overlay during sync - config/task state changes from - // remote ops would otherwise trigger the overlay unexpectedly - skipDuringSync(), + // Outer guard: skip config changes during sync + skipWhileApplyingRemoteOps(), switchMap(([cfg, isFocusModeEnabled]) => isFocusModeEnabled && cfg?.isSyncSessionWithTracking && !cfg?.isStartInBackground ? this.taskService.currentTaskId$.pipe( + // currentTaskId$ is local UI state (not synced), so distinctUntilChanged is sufficient distinctUntilChanged(), filter((id) => !!id), map(() => actions.showFocusOverlay()), @@ -83,12 +83,12 @@ export class FocusModeEffects { this.store.select(selectFocusModeConfig), this.store.select(selectIsFocusModeEnabled), ]).pipe( - // Prevent auto-starting/unpausing focus session during sync - currentTaskId - // changes from remote ops would otherwise start sessions unexpectedly - skipDuringSync(), + // Outer guard: skip config changes during sync + skipWhileApplyingRemoteOps(), switchMap(([cfg, isFocusModeEnabled]) => isFocusModeEnabled && cfg?.isSyncSessionWithTracking ? this.taskService.currentTaskId$.pipe( + // currentTaskId$ is local UI state (not synced), so distinctUntilChanged is sufficient distinctUntilChanged(), filter((taskId) => !!taskId), withLatestFrom( @@ -123,7 +123,7 @@ export class FocusModeEffects { // CRITICAL: Prevent cascading dispatches during sync that cause app freeze. // Without this, rapid currentTaskId changes from remote ops trigger pairwise() // which dispatches pauseFocusSession repeatedly, overwhelming the store. - skipDuringSync(), + skipWhileApplyingRemoteOps(), pairwise(), withLatestFrom( this.store.select(selectFocusModeConfig), @@ -219,7 +219,7 @@ export class FocusModeEffects { // Only triggers when timer STOPS (isRunning becomes false) with elapsed >= duration detectSessionCompletion$ = createEffect(() => this.store.select(selectors.selectTimer).pipe( - skipDuringSync(), + skipWhileApplyingRemoteOps(), withLatestFrom(this.store.select(selectors.selectMode)), // Only consider emissions where timer just stopped running distinctUntilChanged( @@ -241,7 +241,7 @@ export class FocusModeEffects { detectBreakTimeUp$ = createEffect( () => this.store.select(selectors.selectTimer).pipe( - skipDuringSync(), + skipWhileApplyingRemoteOps(), filter( (timer) => timer.purpose === 'break' && @@ -518,7 +518,7 @@ export class FocusModeEffects { createEffect( () => this.store.select(selectors.selectProgress).pipe( - skipDuringSync(), + skipWhileApplyingRemoteOps(), withLatestFrom(this.store.select(selectors.selectIsRunning)), tap(([progress, isRunning]) => { window.ea.setProgressBar({ @@ -559,7 +559,7 @@ export class FocusModeEffects { this.store.select(selectFocusModeConfig), this.store.select(selectIsFocusModeEnabled), ]).pipe( - skipDuringSync(), + skipWhileApplyingRemoteOps(), tap( ([ isSessionRunning, @@ -798,7 +798,7 @@ export class FocusModeEffects { playTickSound$ = createEffect( () => this.store.select(selectors.selectTimer).pipe( - skipDuringSync(), + skipWhileApplyingRemoteOps(), filter( (timer) => timer.isRunning && timer.purpose === 'work' && timer.elapsed > 0, ), diff --git a/src/app/features/idle/store/idle.effects.ts b/src/app/features/idle/store/idle.effects.ts index b580aaf7a..c977cfc0c 100644 --- a/src/app/features/idle/store/idle.effects.ts +++ b/src/app/features/idle/store/idle.effects.ts @@ -1,7 +1,7 @@ import { inject, Injectable } from '@angular/core'; import { createEffect, ofType } from '@ngrx/effects'; import { LOCAL_ACTIONS } from '../../../util/local-actions.token'; -import { skipDuringSync } from '../../../util/skip-during-sync.operator'; +import { skipWhileApplyingRemoteOps } from '../../../util/skip-during-sync.operator'; import { ChromeExtensionInterfaceService } from '../../../core/chrome-extension-interface/chrome-extension-interface.service'; import { WorkContextService } from '../../work-context/work-context.service'; import { TaskService } from '../../tasks/task.service'; @@ -107,7 +107,8 @@ export class IdleEffects { // while the dialog is open, preventing the flickering between two different values. triggerIdleWhenEnabled$ = createEffect(() => this._store.select(selectIdleConfig).pipe( - skipDuringSync(), + // Outer guard: skip config changes during sync + skipWhileApplyingRemoteOps(), switchMap( ({ isEnableIdleTimeTracking, @@ -145,7 +146,9 @@ export class IdleEffects { handleIdleInit$ = createEffect(() => this._store.select(selectIsIdle).pipe( - skipDuringSync(), + // Guard: skip idle state changes during sync - CRITICAL because this effect + // has data mutations (removeTimeSpent, setCurrentId, decreaseCounterToday) + skipWhileApplyingRemoteOps(), distinctUntilChanged(), switchMap((isIdle) => iif(() => isIdle, of(isIdle), EMPTY)), withLatestFrom( diff --git a/src/app/features/reminder/store/reminder-countdown.effects.ts b/src/app/features/reminder/store/reminder-countdown.effects.ts index 0c90d2f97..2304e2331 100644 --- a/src/app/features/reminder/store/reminder-countdown.effects.ts +++ b/src/app/features/reminder/store/reminder-countdown.effects.ts @@ -14,7 +14,7 @@ import { switchMap, tap, } from 'rxjs/operators'; -import { skipDuringSync } from '../../../util/skip-during-sync.operator'; +import { skipWhileApplyingRemoteOps } from '../../../util/skip-during-sync.operator'; import { BannerId } from '../../../core/banner/banner.model'; import { T } from '../../../t.const'; import { LocaleDatePipe } from 'src/app/ui/pipes/locale-date.pipe'; @@ -49,7 +49,8 @@ export class ReminderCountdownEffects { () => this._dataInitStateService.isAllDataLoadedInitially$.pipe( concatMap(() => this._store.select(selectReminderConfig)), - skipDuringSync(), + // Outer guard: skip config changes during sync + skipWhileApplyingRemoteOps(), switchMap((reminderCfg) => reminderCfg.isCountdownBannerEnabled ? combineLatest([ @@ -67,15 +68,14 @@ export class ReminderCountdownEffects { ); }), switchMap((dueTasks) => - this._store - .select(selectCurrentTaskId) - .pipe(distinctUntilChanged()) - .pipe( - map((currentId) => ({ - currentId, - dueTasks: dueTasks.filter((t) => t.id !== currentId), - })), - ), + this._store.select(selectCurrentTaskId).pipe( + // currentTaskId is local UI state (not synced), so distinctUntilChanged is sufficient + distinctUntilChanged(), + map((currentId) => ({ + currentId, + dueTasks: dueTasks.filter((t) => t.id !== currentId), + })), + ), ), tap(({ dueTasks }) => this._showBanner(dueTasks)), ) diff --git a/src/app/features/tasks/store/task-ui.effects.ts b/src/app/features/tasks/store/task-ui.effects.ts index bc4fd36d3..adbf456f6 100644 --- a/src/app/features/tasks/store/task-ui.effects.ts +++ b/src/app/features/tasks/store/task-ui.effects.ts @@ -30,7 +30,7 @@ import { GlobalConfigService } from '../../config/global-config.service'; import { playDoneSound } from '../util/play-done-sound'; import { Task } from '../task.model'; import { EMPTY } from 'rxjs'; -import { skipDuringSync } from '../../../util/skip-during-sync.operator'; +import { skipWhileApplyingRemoteOps } from '../../../util/skip-during-sync.operator'; import { selectProjectById } from '../../project/store/project.selectors'; import { Router } from '@angular/router'; import { NavigateToTaskService } from '../../../core-ui/navigate-to-task/navigate-to-task.service'; @@ -122,13 +122,13 @@ export class TaskUiEffects { timeEstimateExceeded$ = createEffect( () => this._store$.pipe(select(selectConfigFeatureState)).pipe( - // Prevent spurious notifications during sync - currentTask state changes - // from remote ops would otherwise trigger "time estimate exceeded" alerts - skipDuringSync(), + // Outer guard: skip config changes during sync + skipWhileApplyingRemoteOps(), switchMap((globalCfg) => globalCfg && globalCfg.timeTracking.isNotifyWhenTimeEstimateExceeded ? // reset whenever the current taskId changes (but no the task data, which is polled afterwards) this._store$.pipe(select(selectCurrentTaskId)).pipe( + // currentTaskId is local UI state (not synced), so distinctUntilChanged is sufficient distinctUntilChanged(), switchMap(() => this._store$.pipe( @@ -156,9 +156,8 @@ export class TaskUiEffects { timeEstimateExceededDismissBanner$ = createEffect( () => this._store$.pipe(select(selectConfigFeatureState)).pipe( - // Prevent unexpected banner dismissal during sync - currentTaskId changes - // from remote ops would otherwise dismiss the time estimate exceeded banner - skipDuringSync(), + // Outer guard: skip config changes during sync + skipWhileApplyingRemoteOps(), switchMap((globalCfg) => globalCfg && globalCfg.timeTracking.isNotifyWhenTimeEstimateExceeded ? this._bannerService.activeBanner$.pipe( @@ -166,6 +165,7 @@ export class TaskUiEffects { activeBanner?.id === BannerId.TimeEstimateExceeded ? this._store$.pipe( select(selectCurrentTaskId), + // currentTaskId is local UI state (not synced), so distinctUntilChanged is sufficient distinctUntilChanged(), skip(1), ) diff --git a/src/app/features/work-context/store/work-context.effects.ts b/src/app/features/work-context/store/work-context.effects.ts index 8f280124e..a020a2f68 100644 --- a/src/app/features/work-context/store/work-context.effects.ts +++ b/src/app/features/work-context/store/work-context.effects.ts @@ -2,7 +2,7 @@ import { Injectable, inject } from '@angular/core'; import { createEffect, ofType } from '@ngrx/effects'; import { LOCAL_ACTIONS } from '../../../util/local-actions.token'; import { filter, map, tap, withLatestFrom, startWith } from 'rxjs/operators'; -import { skipDuringSync } from '../../../util/skip-during-sync.operator'; +import { skipWhileApplyingRemoteOps } from '../../../util/skip-during-sync.operator'; import { setSelectedTask } from '../../tasks/store/task.actions'; import { TaskService } from '../../tasks/task.service'; import { BannerId } from '../../../core/banner/banner.model'; @@ -49,7 +49,7 @@ export class WorkContextEffects { startWith(this._router.url), // Handle initial page load // Prevent dispatching setActiveWorkContext during sync - startWith() emits // immediately on subscription which could interfere with sync state hydration - skipDuringSync(), + skipWhileApplyingRemoteOps(), filter( (url) => !!url.match(/(schedule)$/) || diff --git a/src/app/util/skip-during-sync-window.operator.ts b/src/app/util/skip-during-sync-window.operator.ts index 3ae421a66..841fdfeed 100644 --- a/src/app/util/skip-during-sync-window.operator.ts +++ b/src/app/util/skip-during-sync-window.operator.ts @@ -9,7 +9,7 @@ import { HydrationStateService } from '../op-log/apply/hydration-state.service'; * * ## Why This Exists * - * `skipDuringSync()` only blocks during `isApplyingRemoteOps()` - the window + * `skipWhileApplyingRemoteOps()` only blocks during `isApplyingRemoteOps()` - the window * when operations are being applied to the store. However, there's a timing gap: * * 1. Tab gains focus → sync triggers @@ -23,7 +23,7 @@ import { HydrationStateService } from '../op-log/apply/hydration-state.service'; * * ## When to Use * - * Use this operator instead of `skipDuringSync()` for selector-based effects that: + * Use this operator instead of `skipWhileApplyingRemoteOps()` for selector-based effects that: * - Modify frequently-synced entities (TODAY_TAG, etc.) * - Perform "repair" or "consistency" operations * - Could create operations that immediately conflict with remote changes @@ -44,11 +44,11 @@ import { HydrationStateService } from '../op-log/apply/hydration-state.service'; * } * ``` * - * ## Comparison with skipDuringSync() + * ## Comparison with skipWhileApplyingRemoteOps() * * | Operator | Suppresses during | Use case | * |----------|-------------------|----------| - * | skipDuringSync() | isApplyingRemoteOps only | General selector-based effects | + * | skipWhileApplyingRemoteOps() | isApplyingRemoteOps only | General selector-based effects | * | skipDuringSyncWindow() | isApplyingRemoteOps + cooldown | Effects that modify shared entities | */ export const skipDuringSyncWindow = (): MonoTypeOperatorFunction => { diff --git a/src/app/util/skip-during-sync.operator.spec.ts b/src/app/util/skip-during-sync.operator.spec.ts new file mode 100644 index 000000000..76fbd7102 --- /dev/null +++ b/src/app/util/skip-during-sync.operator.spec.ts @@ -0,0 +1,180 @@ +import { TestBed } from '@angular/core/testing'; +import { Subject } from 'rxjs'; +import { skipWhileApplyingRemoteOps, skipDuringSync } from './skip-during-sync.operator'; +import { HydrationStateService } from '../op-log/apply/hydration-state.service'; + +describe('skipWhileApplyingRemoteOps', () => { + let hydrationStateService: jasmine.SpyObj; + let source$: Subject; + + beforeEach(() => { + hydrationStateService = jasmine.createSpyObj('HydrationStateService', [ + 'isApplyingRemoteOps', + ]); + hydrationStateService.isApplyingRemoteOps.and.returnValue(false); + + TestBed.configureTestingModule({ + providers: [{ provide: HydrationStateService, useValue: hydrationStateService }], + }); + + source$ = new Subject(); + }); + + it('should pass emissions when not applying remote ops', (done) => { + hydrationStateService.isApplyingRemoteOps.and.returnValue(false); + + TestBed.runInInjectionContext(() => { + const results: number[] = []; + source$.pipe(skipWhileApplyingRemoteOps()).subscribe({ + next: (val) => results.push(val), + complete: () => { + expect(results).toEqual([1, 2, 3]); + done(); + }, + }); + }); + + source$.next(1); + source$.next(2); + source$.next(3); + source$.complete(); + }); + + it('should skip emissions when applying remote ops', (done) => { + hydrationStateService.isApplyingRemoteOps.and.returnValue(true); + + TestBed.runInInjectionContext(() => { + const results: number[] = []; + source$.pipe(skipWhileApplyingRemoteOps()).subscribe({ + next: (val) => results.push(val), + complete: () => { + expect(results).toEqual([]); + done(); + }, + }); + }); + + source$.next(1); + source$.next(2); + source$.next(3); + source$.complete(); + }); + + it('should dynamically filter based on current state', (done) => { + TestBed.runInInjectionContext(() => { + const results: number[] = []; + source$.pipe(skipWhileApplyingRemoteOps()).subscribe({ + next: (val) => results.push(val), + complete: () => { + expect(results).toEqual([1, 3, 5]); + done(); + }, + }); + }); + + // Not syncing - should pass + hydrationStateService.isApplyingRemoteOps.and.returnValue(false); + source$.next(1); + + // Start syncing - should skip + hydrationStateService.isApplyingRemoteOps.and.returnValue(true); + source$.next(2); + + // Still syncing - should skip + source$.next(2.5); + + // Stop syncing - should pass + hydrationStateService.isApplyingRemoteOps.and.returnValue(false); + source$.next(3); + + // Start syncing again - should skip + hydrationStateService.isApplyingRemoteOps.and.returnValue(true); + source$.next(4); + + // Stop syncing - should pass + hydrationStateService.isApplyingRemoteOps.and.returnValue(false); + source$.next(5); + + source$.complete(); + }); + + it('should work with different value types', (done) => { + hydrationStateService.isApplyingRemoteOps.and.returnValue(false); + + TestBed.runInInjectionContext(() => { + const stringSource$ = new Subject(); + const results: string[] = []; + + stringSource$.pipe(skipWhileApplyingRemoteOps()).subscribe({ + next: (val) => results.push(val), + complete: () => { + expect(results).toEqual(['a', 'b', 'c']); + done(); + }, + }); + + stringSource$.next('a'); + stringSource$.next('b'); + stringSource$.next('c'); + stringSource$.complete(); + }); + }); +}); + +describe('skipDuringSync (deprecated alias)', () => { + let hydrationStateService: jasmine.SpyObj; + let source$: Subject; + + beforeEach(() => { + hydrationStateService = jasmine.createSpyObj('HydrationStateService', [ + 'isApplyingRemoteOps', + ]); + hydrationStateService.isApplyingRemoteOps.and.returnValue(false); + + TestBed.configureTestingModule({ + providers: [{ provide: HydrationStateService, useValue: hydrationStateService }], + }); + + source$ = new Subject(); + }); + + it('should work identically to skipWhileApplyingRemoteOps', (done) => { + hydrationStateService.isApplyingRemoteOps.and.returnValue(false); + + TestBed.runInInjectionContext(() => { + const results: number[] = []; + source$.pipe(skipDuringSync()).subscribe({ + next: (val) => results.push(val), + complete: () => { + expect(results).toEqual([1, 2, 3]); + done(); + }, + }); + }); + + source$.next(1); + source$.next(2); + source$.next(3); + source$.complete(); + }); + + it('should skip emissions when applying remote ops', (done) => { + hydrationStateService.isApplyingRemoteOps.and.returnValue(true); + + TestBed.runInInjectionContext(() => { + const results: number[] = []; + source$.pipe(skipDuringSync()).subscribe({ + next: (val) => results.push(val), + complete: () => { + expect(results).toEqual([]); + done(); + }, + }); + }); + + source$.next(1); + source$.next(2); + source$.next(3); + source$.complete(); + }); +}); diff --git a/src/app/util/skip-during-sync.operator.ts b/src/app/util/skip-during-sync.operator.ts index 3184fccd7..553b241c2 100644 --- a/src/app/util/skip-during-sync.operator.ts +++ b/src/app/util/skip-during-sync.operator.ts @@ -4,8 +4,8 @@ import { filter } from 'rxjs/operators'; import { HydrationStateService } from '../op-log/apply/hydration-state.service'; /** - * RxJS operator that skips emissions during remote operation application - * (hydration/sync replay). + * RxJS operator that skips emissions while remote operations are being applied + * to the store (during hydration/sync replay). * * ## Why This Exists * @@ -22,13 +22,13 @@ import { HydrationStateService } from '../op-log/apply/hydration-state.service'; * ## Usage * * ```typescript - * import { skipDuringSync } from '../../../util/skip-during-sync.operator'; + * import { skipWhileApplyingRemoteOps } from '../../../util/skip-during-sync.operator'; * * @Injectable() * export class MyEffects { * myEffect$ = createEffect(() => * this.store.select(mySelector).pipe( - * skipDuringSync(), // <-- Add this to guard selector-based effects + * skipWhileApplyingRemoteOps(), // <-- Add this to guard selector-based effects * tap(...), * ) * ); @@ -48,7 +48,12 @@ import { HydrationStateService } from '../op-log/apply/hydration-state.service'; * `createEffect()` is called during class field initialization, which is still * within an injection context. */ -export const skipDuringSync = (): MonoTypeOperatorFunction => { +export const skipWhileApplyingRemoteOps = (): MonoTypeOperatorFunction => { const hydrationState = inject(HydrationStateService); return filter(() => !hydrationState.isApplyingRemoteOps()); }; + +/** + * @deprecated Use `skipWhileApplyingRemoteOps` instead. This alias exists for backwards compatibility. + */ +export const skipDuringSync = skipWhileApplyingRemoteOps;