mirror of
https://github.com/johannesjo/super-productivity.git
synced 2026-07-17 16:37:43 +00:00
perf(tasks): stable per-task selector outputs via per-subscription factories (#8809)
selectTasksWithSubTasksByIds and selectTasksById were module-level
props-selectors sharing one depth-1 memo slot across all concurrent
subscribers, so different id-sets mutually evicted the slot and both
recomputed on every dispatched action. selectTasksWithSubTasksByIds also
rebuilt {...task, subTasks} for every id whenever the entities slice
changed (every second while tracking), so mainListTasks$/backlogTasks$
emitted all-new TaskWithSubTasks arrays each tick and every OnPush row
re-evaluated its template.
Add selectTasksWithSubTasksByIdsFactory / selectTasksByIdFactory that
mint a fresh createSelector per subscription (created inside the
switchMap at each call site) so each subscription owns its memo. The
with-subtasks factory keeps a per-instance cache keyed on the task
entity ref + its subtask entity refs, returning the identical
TaskWithSubTasks object for any task whose refs are unchanged — only the
tracked task gets a new object. Belt-and-braces distinctUntilChanged
(fastArrayCompare) on mainListTasks$/backlogTasks$ suppresses no-op
re-emissions. Output shape/order/filtering are unchanged.
SPAP-19
This commit is contained in:
parent
132bd5452c
commit
e29cd4cd11
6 changed files with 250 additions and 9 deletions
|
|
@ -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> = {}): 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<string, Task>,
|
||||
),
|
||||
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);
|
||||
});
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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<object, Task[]> =>
|
||||
createSelector(selectTaskFeatureState, (state: TaskState): Task[] =>
|
||||
ids ? ids.map((id) => state.entities[id]).filter((task): task is Task => !!task) : [],
|
||||
);
|
||||
|
||||
export const selectTasksWithSubTasksByIdsFactory = (
|
||||
ids: string[],
|
||||
): MemoizedSelector<object, TaskWithSubTasks[]> => {
|
||||
// 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<string>();
|
||||
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 => {
|
||||
|
|
|
|||
|
|
@ -52,7 +52,7 @@ import {
|
|||
selectTaskDetailTargetPanel,
|
||||
selectTaskEntities,
|
||||
selectTaskFeatureState,
|
||||
selectTasksById,
|
||||
selectTasksByIdFactory,
|
||||
selectTasksByRepeatConfigId,
|
||||
selectTasksByTag,
|
||||
selectTaskWithSubTasksByRepeatConfigId,
|
||||
|
|
@ -1085,7 +1085,9 @@ export class TaskService {
|
|||
}
|
||||
|
||||
getByIdsLive$(ids: string[]): Observable<Task[]> {
|
||||
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<TaskWithSubTasks> {
|
||||
|
|
|
|||
|
|
@ -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()) || [];
|
||||
|
||||
|
|
|
|||
|
|
@ -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<TaskWithSubTasks[]> = 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<TaskWithSubTasks[]> = 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<TaskWithSubTasks[]> = 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(
|
||||
|
|
|
|||
|
|
@ -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) => {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue