diff --git a/src/app/features/tasks/store/task.selectors.spec.ts b/src/app/features/tasks/store/task.selectors.spec.ts index 493d2d223e..1ff1a89895 100644 --- a/src/app/features/tasks/store/task.selectors.spec.ts +++ b/src/app/features/tasks/store/task.selectors.spec.ts @@ -1145,4 +1145,155 @@ describe('Task Selectors', () => { expect(ids).not.toContain('overdueInArchived'); expect(ids).toContain('task6'); }); + + // SPAP-19: per-subscription stable task selector factories + // ------------------------------------------------------------------------- + describe('SPAP-19 stable selector factories', () => { + const makeTask = (id: string, over: Partial = {}): Task => + ({ + ...DEFAULT_TASK, + id, + title: id, + created: 0, + subTaskIds: [], + tagIds: [], + timeSpentOnDay: {}, + ...over, + }) as Task; + + const makeTaskState = (tasks: Task[]): TaskState => ({ + ids: tasks.map((t) => t.id), + entities: tasks.reduce( + (acc, t) => { + acc[t.id] = t; + return acc; + }, + {} as Record, + ), + currentTaskId: null, + selectedTaskId: null, + lastCurrentTaskId: null, + isDataLoaded: true, + taskDetailTargetPanel: null, + }); + + // Selectors only read the TASK feature slice, so a minimal root is enough. + const wrap = (ts: TaskState): any => ({ [TASK_FEATURE_NAME]: ts }); + + describe('selectTasksWithSubTasksByIdsFactory', () => { + it('returns identical refs for tasks whose entity ref did not change (only C changed)', () => { + const a = makeTask('A'); + const b = makeTask('B'); + const c = makeTask('C'); + const sel = fromSelectors.selectTasksWithSubTasksByIdsFactory(['A', 'B', 'C']); + + const r1 = sel(wrap(makeTaskState([a, b, c]))); + // Only C's entity ref changes (e.g. a time-tracking tick bumping timeSpent) + const c2 = { ...c, timeSpent: 999 }; + const r2 = sel(wrap(makeTaskState([a, b, c2]))); + + expect(r2[0]).toBe(r1[0]); // A: same object + expect(r2[1]).toBe(r1[1]); // B: same object + expect(r2[2]).not.toBe(r1[2]); // C: rebuilt + expect(r2[2].timeSpent).toBe(999); + }); + + it('rebuilds a parent when only one of its subtask entities changed', () => { + const p = makeTask('P', { subTaskIds: ['S'] }); + const s = makeTask('S', { parentId: 'P', timeSpent: 0 }); + const sel = fromSelectors.selectTasksWithSubTasksByIdsFactory(['P']); + + const r1 = sel(wrap(makeTaskState([p, s]))); + const s2 = { ...s, timeSpent: 500 }; + const r2 = sel(wrap(makeTaskState([p, s2]))); + + expect(r2[0]).not.toBe(r1[0]); // parent rebuilt + expect(r2[0].subTasks[0]).toBe(s2); // reflects the changed subtask entity + expect(r2[0].subTasks[0].timeSpent).toBe(500); + }); + + it('keeps two concurrent factory instances independent (no cross-eviction)', () => { + const a = makeTask('A'); + const b = makeTask('B'); + const c = makeTask('C'); + const d = makeTask('D'); + const selAB = fromSelectors.selectTasksWithSubTasksByIdsFactory(['A', 'B']); + const selCD = fromSelectors.selectTasksWithSubTasksByIdsFactory(['C', 'D']); + + const ab1 = selAB(wrap(makeTaskState([a, b, c, d]))); + const cd1 = selCD(wrap(makeTaskState([a, b, c, d]))); + // New state objects (same task refs) force the projectors to re-run; + // each instance's own cache must still return identical results. + const ab2 = selAB(wrap(makeTaskState([a, b, c, d]))); + const cd2 = selCD(wrap(makeTaskState([a, b, c, d]))); + + expect(ab2[0]).toBe(ab1[0]); + expect(ab2[1]).toBe(ab1[1]); + expect(cd2[0]).toBe(cd1[0]); + expect(cd2[1]).toBe(cd1[1]); + }); + + it('produces output deep-equal to the legacy selectTasksWithSubTasksByIds (shape parity)', () => { + const p = makeTask('P', { subTaskIds: ['S1', 'S2'] }); + const s1 = makeTask('S1', { parentId: 'P' }); + const s2 = makeTask('S2', { parentId: 'P' }); + const state = wrap(makeTaskState([p, s1, s2])); + + const legacy = fromSelectors.selectTasksWithSubTasksByIds(state, { + ids: ['P'], + }); + const viaFactory = fromSelectors.selectTasksWithSubTasksByIdsFactory(['P'])( + state, + ); + + expect(viaFactory).toEqual(legacy); + }); + + it('filters out missing/deleted top-level entities exactly like the legacy selector', () => { + const a = makeTask('A'); + const state = wrap(makeTaskState([a])); + const res = fromSelectors.selectTasksWithSubTasksByIdsFactory(['A', 'gone'])( + state, + ); + expect(res.length).toBe(1); + expect(res[0].id).toBe('A'); + }); + + // Documents WHY the factory is needed: the legacy module-level props-selector + // shares ONE memo slot, so interleaving different id-sets forces a full + // recompute (new refs) — the exact thrash this ticket fixes. + it('CONTRAST: legacy shared props-selector thrashes across interleaved id-sets', () => { + const a = makeTask('A'); + const b = makeTask('B'); + const c = makeTask('C'); + const d = makeTask('D'); + const state = wrap(makeTaskState([a, b, c, d])); + + const first1 = fromSelectors.selectTasksWithSubTasksByIds(state, { + ids: ['A', 'B'], + }); + // a "different subscriber" reads a different id-set against the same state + fromSelectors.selectTasksWithSubTasksByIds(state, { ids: ['C', 'D'] }); + const first2 = fromSelectors.selectTasksWithSubTasksByIds(state, { + ids: ['A', 'B'], + }); + + // Interleaving evicted the memo → recomputed → brand-new object refs + expect(first2[0]).not.toBe(first1[0]); + }); + }); + + describe('selectTasksByIdFactory', () => { + it('returns raw entities in order and filters out missing ids', () => { + const a = makeTask('A'); + const b = makeTask('B'); + const state = wrap(makeTaskState([a, b])); + const res = fromSelectors.selectTasksByIdFactory(['A', 'gone', 'B'])(state); + + expect(res.length).toBe(2); + expect(res[0]).toBe(a); + expect(res[1]).toBe(b); + }); + }); + }); }); diff --git a/src/app/features/tasks/store/task.selectors.ts b/src/app/features/tasks/store/task.selectors.ts index 596f793030..d01b484f32 100644 --- a/src/app/features/tasks/store/task.selectors.ts +++ b/src/app/features/tasks/store/task.selectors.ts @@ -1,4 +1,4 @@ -import { createFeatureSelector, createSelector } from '@ngrx/store'; +import { createFeatureSelector, createSelector, MemoizedSelector } from '@ngrx/store'; import { TASK_FEATURE_NAME } from './task.reducer'; import { Task, @@ -515,6 +515,84 @@ export const selectTasksWithSubTasksByIds = createSelector( .map((task) => mapSubTasksToTask(task, state) as TaskWithSubTasks), ); +// SPAP-19: Per-subscription stable selector factories. +// ----------------------------------------------------- +// The module-level `selectTasksById` / `selectTasksWithSubTasksByIds` +// props-selectors above keep a SINGLE depth-1 memo slot each, shared across +// every concurrent subscriber. Different `ids` props mutually evict that slot, +// so both recompute on every dispatched action. These factories instead mint a +// FRESH `createSelector` per distinct id-set (create it inside the +// `switchMap`/`map` at the call site) so each subscription owns its own memo — +// GC'd together with the subscription. +export const selectTasksByIdFactory = (ids: string[]): MemoizedSelector => + createSelector(selectTaskFeatureState, (state: TaskState): Task[] => + ids ? ids.map((id) => state.entities[id]).filter((task): task is Task => !!task) : [], + ); + +export const selectTasksWithSubTasksByIdsFactory = ( + ids: string[], +): MemoizedSelector => { + // Per-selector-instance (per-subscription) cache keyed by task id. Because it + // lives in this closure it is naturally isolated from other subscribers and + // GC'd with the selector. On a tick where only one task's entity ref changed, + // every other task returns its identical previous `TaskWithSubTasks` object. + const cache = new Map< + string, + { task: Task; subTasks: Task[]; result: TaskWithSubTasks } + >(); + return createSelector( + selectTaskFeatureState, + (state: TaskState): TaskWithSubTasks[] => { + const result: TaskWithSubTasks[] = []; + const seen = new Set(); + for (const id of ids) { + const task = state.entities[id]; + // Same filtering as the legacy selector: drop missing/deleted entities. + if (!task) { + continue; + } + seen.add(id); + + // Resolve current subtask entities (same shaping as mapSubTasksToTask). + const subTasks: Task[] = []; + for (const subTaskId of task.subTaskIds) { + const subTask = state.entities[subTaskId]; + if (subTask) { + subTasks.push(subTask); + } else { + devError('Task data not found for ' + subTaskId); + } + } + + const cached = cache.get(id); + if ( + cached && + cached.task === task && + cached.subTasks.length === subTasks.length && + cached.subTasks.every((st, i) => st === subTasks[i]) + ) { + // Nothing referentially changed → reuse the exact previous object. + result.push(cached.result); + } else { + const built: TaskWithSubTasks = { ...task, subTasks }; + cache.set(id, { task, subTasks, result: built }); + result.push(built); + } + } + // Prune entries for ids no longer requested so the cache can't grow + // unbounded if the id-set is ever mutated in place. + if (cache.size > seen.size) { + for (const key of cache.keys()) { + if (!seen.has(key)) { + cache.delete(key); + } + } + } + return result; + }, + ); +}; + export const selectTaskByIdWithSubTaskData = createSelector( selectTaskFeatureState, (state: TaskState, props: { id: string }): TaskWithSubTasks => { diff --git a/src/app/features/tasks/task.service.ts b/src/app/features/tasks/task.service.ts index 9f64780d9c..27f5c02bae 100644 --- a/src/app/features/tasks/task.service.ts +++ b/src/app/features/tasks/task.service.ts @@ -52,7 +52,7 @@ import { selectTaskDetailTargetPanel, selectTaskEntities, selectTaskFeatureState, - selectTasksById, + selectTasksByIdFactory, selectTasksByRepeatConfigId, selectTasksByTag, selectTaskWithSubTasksByRepeatConfigId, @@ -1085,7 +1085,9 @@ export class TaskService { } getByIdsLive$(ids: string[]): Observable { - return this._store.pipe(select(selectTasksById, { ids })); + // SPAP-19: fresh per-call factory selector so concurrent subscribers with + // different id-sets don't evict each other's memo. + return this._store.pipe(select(selectTasksByIdFactory(ids))); } getByIdWithSubTaskData$(id: string): Observable { diff --git a/src/app/features/work-context/work-context-markdown.service.ts b/src/app/features/work-context/work-context-markdown.service.ts index 0616752f02..f3d825b5a7 100644 --- a/src/app/features/work-context/work-context-markdown.service.ts +++ b/src/app/features/work-context/work-context-markdown.service.ts @@ -2,7 +2,7 @@ import { Injectable, inject } from '@angular/core'; import { ProjectService } from '../project/project.service'; import { TagService } from '../tag/tag.service'; import { Store } from '@ngrx/store'; -import { selectTasksWithSubTasksByIds } from '../tasks/store/task.selectors'; +import { selectTasksWithSubTasksByIdsFactory } from '../tasks/store/task.selectors'; import { Task, TaskWithSubTasks } from '../tasks/task.model'; import { first } from 'rxjs/operators'; import { Log } from '../../core/log'; @@ -72,7 +72,7 @@ export class WorkContextMarkdownService { const tasks = (await this._store - .select(selectTasksWithSubTasksByIds, { ids }) + .select(selectTasksWithSubTasksByIdsFactory(ids)) .pipe(first()) .toPromise()) || []; diff --git a/src/app/features/work-context/work-context.service.ts b/src/app/features/work-context/work-context.service.ts index c331181b1c..8e224248c1 100644 --- a/src/app/features/work-context/work-context.service.ts +++ b/src/app/features/work-context/work-context.service.ts @@ -36,7 +36,7 @@ import { flattenTasks, selectAllTasks, selectAllTasksWithSubTasks, - selectTasksWithSubTasksByIds, + selectTasksWithSubTasksByIdsFactory, } from '../tasks/store/task.selectors'; import { ofType } from '@ngrx/effects'; import { WorklogExportSettings } from '../worklog/worklog.model'; @@ -283,6 +283,10 @@ export class WorkContextService { mainListTasks$: Observable = this.mainListTaskIds$.pipe( // tap((taskIds: string[]) => Log.log('[WorkContext] Today task IDs:', taskIds)), switchMap((taskIds: string[]) => this._getTasksByIds$(taskIds)), + // SPAP-19: with per-task referential stability upstream, an unchanged list + // is length + per-element === equal, so this stops needless re-emissions + // (and the OnPush row re-renders they trigger) every time a task ticks. + distinctUntilChanged(fastArrayCompare), // TODO find out why this is triggered so often // tap((tasks: TaskWithSubTasks[]) => // Log.log('[WorkContext] Today tasks loaded:', tasks.length, 'tasks'), @@ -320,6 +324,9 @@ export class WorkContextService { backlogTasks$: Observable = this.backlogTaskIds$.pipe( switchMap((ids) => this._getTasksByIds$(ids)), + // SPAP-19: see mainListTasks$ — suppress no-op re-emissions of an + // unchanged (element-wise identical) backlog array. + distinctUntilChanged(fastArrayCompare), ); allTasksForCurrentContext$: Observable = combineLatest([ @@ -724,7 +731,10 @@ export class WorkContextService { Log.log({ ids }); throw new Error('Invalid param provided for getByIds$ :('); } - return this._store$.select(selectTasksWithSubTasksByIds, { ids }); + // SPAP-19: mint a fresh per-id-set factory selector (called inside the + // switchMap at each call site) so each subscription owns its own memo and + // its per-task referential-stability cache. + return this._store$.select(selectTasksWithSubTasksByIdsFactory(ids)); } private _filterFutureScheduledTasksForToday( diff --git a/src/app/pages/daily-summary/plan-tasks-tomorrow/plan-tasks-tomorrow.component.ts b/src/app/pages/daily-summary/plan-tasks-tomorrow/plan-tasks-tomorrow.component.ts index 4ad1b2c381..5288befd1b 100644 --- a/src/app/pages/daily-summary/plan-tasks-tomorrow/plan-tasks-tomorrow.component.ts +++ b/src/app/pages/daily-summary/plan-tasks-tomorrow/plan-tasks-tomorrow.component.ts @@ -13,7 +13,7 @@ import { Store } from '@ngrx/store'; import { selectUndoneTodayTaskIds } from '../../../features/work-context/store/work-context.selectors'; import { PlannerActions } from '../../../features/planner/store/planner.actions'; import { first } from 'rxjs/operators'; -import { selectTasksWithSubTasksByIds } from '../../../features/tasks/store/task.selectors'; +import { selectTasksWithSubTasksByIdsFactory } from '../../../features/tasks/store/task.selectors'; import { getDbDateStr } from '../../../util/get-db-date-str'; import { TranslatePipe } from '@ngx-translate/core'; @@ -46,7 +46,7 @@ export class PlanTasksTomorrowComponent { const tomorrow = await this.plannerService.tomorrow$.pipe(first()).toPromise(); const ids = await this.leftOverTodayIds$.pipe(first()).toPromise(); const tasks = await this._store - .select(selectTasksWithSubTasksByIds, { ids }) + .select(selectTasksWithSubTasksByIdsFactory(ids ?? [])) .pipe(first()) .toPromise(); tasks.forEach((task) => {