mirror of
https://github.com/johannesjo/super-productivity.git
synced 2026-07-17 16:37:43 +00:00
On the LWW conflict-apply path, an invalid projectId destination was handled inconsistently with the local handleUpdateTask strip. Unknown or non-string ids fell back to the task's current project, but an explicit null orphaned the task from every list (projectId = undefined). So a null destination replayed via a disjoint-merge patch diverged: a passive client kept the task in its project, while a conflict client orphaned it. Sanitize any invalid destination — null/undefined, non-string, or unknown project — to the task's current project, in every mode, mirroring the local strip. Tasks use '' for "no project", so a null is a malformed value to sanitize (not a signal to clear); this also matches how #9001 already handled unknown/non-string ids. '' remains a valid no-project assignment. The same synthetic LWW-TASK action is processed by a twin handler in section-shared.reducer, which had the same null bug and would otherwise strip the task from its current-project section while the task slice kept it. Both handlers now leave the task in its current project for any invalid destination. Tests: - lww-update / section-shared specs: null and unknown destinations retain the current project and section, in patch and replace modes; the "current project itself deleted -> undefined" fallback is covered. - New integration spec composes section -> crud -> lww in registry order and asserts the normal-replay and LWW-patch paths converge on the same projectId, project.taskIds and section membership. Verified to fail on the pre-fix code (LWW path orphaned the task from both project and section).
This commit is contained in:
Johannes Millan
GitHub
parent
0923fe66b2
commit
cbd0641681
5 changed files with 287 additions and 6 deletions
|
|
@ -0,0 +1,155 @@
|
|||
/* eslint-disable @typescript-eslint/naming-convention */
|
||||
/**
|
||||
* Integration test for #9025.
|
||||
*
|
||||
* A single user intent — updateTask with an invalid `projectId` — must reach
|
||||
* the SAME state whether a receiving client replays it normally
|
||||
* (handleUpdateTask, which strips the invalid projectId) or applies it as a
|
||||
* resolved LWW conflict (lwwUpdateMetaReducer's patch path). The bug was that
|
||||
* the LWW path orphaned the task on an explicit `null` while the normal path
|
||||
* kept it — permanent cross-client divergence.
|
||||
*
|
||||
* These tests compose the three meta-reducers that actually touch a task's
|
||||
* project membership, in registry order (section -> crud -> lww), and assert
|
||||
* that the normal-replay path and the LWW-patch path converge on the same
|
||||
* task.projectId, project.taskIds and section membership. The same composed
|
||||
* chain routes each action type to the correct handler: a plain updateTask is
|
||||
* a no-op for lww, and a `[TASK] LWW Update` is a no-op for crud.
|
||||
*/
|
||||
import { Action, ActionReducer } from '@ngrx/store';
|
||||
import { sectionSharedMetaReducer } from './section-shared.reducer';
|
||||
import { taskSharedCrudMetaReducer } from './task-shared-crud.reducer';
|
||||
import { lwwUpdateMetaReducer } from './lww-update.meta-reducer';
|
||||
import { TaskSharedActions } from '../task-shared.actions';
|
||||
import { RootState } from '../../root-state';
|
||||
import { TASK_FEATURE_NAME } from '../../../features/tasks/store/task.reducer';
|
||||
import { PROJECT_FEATURE_NAME } from '../../../features/project/store/project.reducer';
|
||||
import { TAG_FEATURE_NAME } from '../../../features/tag/store/tag.reducer';
|
||||
import { SECTION_FEATURE_NAME } from '../../../features/section/store/section.reducer';
|
||||
import { appStateFeatureKey } from '../../app-state/app-state.reducer';
|
||||
import { getDbDateStr } from '../../../util/get-db-date-str';
|
||||
import { WorkContextType } from '../../../features/work-context/work-context.model';
|
||||
import { Task } from '../../../features/tasks/task.model';
|
||||
import { Project } from '../../../features/project/project.model';
|
||||
import { createMockTask, createMockProject } from './test-utils';
|
||||
|
||||
describe('#9025: LWW vs normal-replay projectId convergence', () => {
|
||||
const TASK_ID = 'task-1';
|
||||
const PROJECT_ID = 'proj-a';
|
||||
const SECTION_ID = 'sec-a';
|
||||
|
||||
const identity: ActionReducer<any, Action> = (s) => s;
|
||||
// Registry order: section (outer) -> crud -> lww (inner). The same chain
|
||||
// handles both action types; each is a no-op for the reducer that doesn't own it.
|
||||
const chain = sectionSharedMetaReducer(
|
||||
taskSharedCrudMetaReducer(lwwUpdateMetaReducer(identity)),
|
||||
);
|
||||
|
||||
const createState = (): RootState =>
|
||||
({
|
||||
[TASK_FEATURE_NAME]: {
|
||||
ids: [TASK_ID],
|
||||
entities: {
|
||||
[TASK_ID]: createMockTask({ id: TASK_ID, projectId: PROJECT_ID }),
|
||||
},
|
||||
currentTaskId: null,
|
||||
selectedTaskId: null,
|
||||
taskDetailTargetPanel: null,
|
||||
isDataLoaded: true,
|
||||
lastCurrentTaskId: null,
|
||||
},
|
||||
[PROJECT_FEATURE_NAME]: {
|
||||
ids: [PROJECT_ID],
|
||||
entities: {
|
||||
[PROJECT_ID]: createMockProject({ id: PROJECT_ID, taskIds: [TASK_ID] }),
|
||||
},
|
||||
},
|
||||
[TAG_FEATURE_NAME]: { ids: [], entities: {} },
|
||||
[SECTION_FEATURE_NAME]: {
|
||||
ids: [SECTION_ID],
|
||||
entities: {
|
||||
[SECTION_ID]: {
|
||||
id: SECTION_ID,
|
||||
contextId: PROJECT_ID,
|
||||
contextType: WorkContextType.PROJECT,
|
||||
title: 'Section A',
|
||||
taskIds: [TASK_ID],
|
||||
},
|
||||
},
|
||||
},
|
||||
[appStateFeatureKey]: { todayStr: getDbDateStr(), startOfNextDayDiffMs: 0 },
|
||||
}) as unknown as RootState;
|
||||
|
||||
const membership = (
|
||||
state: RootState,
|
||||
): { projectId: unknown; projectTaskIds: string[]; sectionTaskIds: string[] } => ({
|
||||
projectId: (state[TASK_FEATURE_NAME].entities[TASK_ID] as Task).projectId,
|
||||
projectTaskIds: (state[PROJECT_FEATURE_NAME].entities[PROJECT_ID] as Project).taskIds,
|
||||
sectionTaskIds: state[SECTION_FEATURE_NAME].entities[SECTION_ID]!.taskIds,
|
||||
});
|
||||
|
||||
const replayNormally = (projectId: string | null): RootState =>
|
||||
chain(createState(), {
|
||||
...TaskSharedActions.updateTask({
|
||||
task: { id: TASK_ID, changes: { projectId } as Partial<Task> },
|
||||
}),
|
||||
}) as RootState;
|
||||
|
||||
const applyAsLwwPatch = (projectId: string | null): RootState =>
|
||||
chain(createState(), {
|
||||
type: '[TASK] LWW Update',
|
||||
id: TASK_ID,
|
||||
projectId,
|
||||
meta: {
|
||||
isPersistent: true,
|
||||
entityType: 'TASK',
|
||||
entityId: TASK_ID,
|
||||
lwwUpdateMode: 'patch',
|
||||
},
|
||||
} as unknown as Action) as RootState;
|
||||
|
||||
for (const invalid of [
|
||||
{ label: 'explicit null', value: null },
|
||||
{ label: 'unknown project id', value: 'ghost-project' },
|
||||
]) {
|
||||
it(`converges for an ${invalid.label} destination`, () => {
|
||||
const normal = membership(replayNormally(invalid.value));
|
||||
const lww = membership(applyAsLwwPatch(invalid.value));
|
||||
|
||||
// Both paths keep the task in its original project and section...
|
||||
expect(normal.projectId).toBe(PROJECT_ID);
|
||||
expect(normal.projectTaskIds).toEqual([TASK_ID]);
|
||||
expect(normal.sectionTaskIds).toEqual([TASK_ID]);
|
||||
// ...and the LWW path matches the normal path exactly (no divergence).
|
||||
expect(lww).toEqual(normal);
|
||||
});
|
||||
}
|
||||
|
||||
it('still moves the task when the LWW destination is a real project', () => {
|
||||
// Guards against the fix over-stripping: a valid move must still apply.
|
||||
const state = createState();
|
||||
(state[PROJECT_FEATURE_NAME].entities as Record<string, Project>)['proj-b'] =
|
||||
createMockProject({ id: 'proj-b', taskIds: [] });
|
||||
(state[PROJECT_FEATURE_NAME].ids as string[]).push('proj-b');
|
||||
|
||||
const next = chain(state, {
|
||||
type: '[TASK] LWW Update',
|
||||
id: TASK_ID,
|
||||
projectId: 'proj-b',
|
||||
meta: {
|
||||
isPersistent: true,
|
||||
entityType: 'TASK',
|
||||
entityId: TASK_ID,
|
||||
lwwUpdateMode: 'patch',
|
||||
},
|
||||
} as unknown as Action) as RootState;
|
||||
|
||||
expect((next[TASK_FEATURE_NAME].entities[TASK_ID] as Task).projectId).toBe('proj-b');
|
||||
expect((next[PROJECT_FEATURE_NAME].entities[PROJECT_ID] as Project).taskIds).toEqual(
|
||||
[],
|
||||
);
|
||||
expect((next[PROJECT_FEATURE_NAME].entities['proj-b'] as Project).taskIds).toEqual([
|
||||
TASK_ID,
|
||||
]);
|
||||
});
|
||||
});
|
||||
|
|
@ -1799,6 +1799,10 @@ describe('lwwUpdateMetaReducer', () => {
|
|||
for (const invalidTarget of [
|
||||
{ label: 'missing', id: 'missing-project', isArchived: false },
|
||||
{ label: 'prototype-like', id: '__proto__', isArchived: false },
|
||||
// #9025: an explicit null destination must retain the current project on
|
||||
// the LWW replay path, mirroring the local `handleUpdateTask` strip.
|
||||
// Previously null orphaned the task from every project list here.
|
||||
{ label: 'null', id: null, isArchived: false },
|
||||
]) {
|
||||
it(`should retain the current project for a ${invalidTarget.label} target`, () => {
|
||||
const state = createStateWithProjects(PROJECT_A, [TASK_ID], []);
|
||||
|
|
@ -1833,6 +1837,56 @@ describe('lwwUpdateMetaReducer', () => {
|
|||
});
|
||||
}
|
||||
|
||||
it('keeps the current project for a replace-mode snapshot with an explicit null (#9025)', () => {
|
||||
// Invalid destinations are sanitized identically in every mode. A null is
|
||||
// not a "clear" signal (tasks use '' for no-project), so even an
|
||||
// authoritative replace snapshot keeps the task in its current project
|
||||
// rather than orphaning it.
|
||||
const state = createStateWithProjects(PROJECT_A, [TASK_ID], []);
|
||||
const action = {
|
||||
type: '[TASK] LWW Update',
|
||||
...createMockTask(),
|
||||
id: TASK_ID,
|
||||
projectId: null,
|
||||
meta: {
|
||||
isPersistent: true,
|
||||
entityType: 'TASK',
|
||||
entityId: TASK_ID,
|
||||
lwwUpdateMode: 'replace',
|
||||
},
|
||||
};
|
||||
|
||||
reducer(state, action);
|
||||
|
||||
const updatedState = mockReducer.calls.mostRecent().args[0] as Partial<RootState>;
|
||||
expect(updatedState[TASK_FEATURE_NAME]?.entities[TASK_ID]?.projectId).toBe(
|
||||
PROJECT_A,
|
||||
);
|
||||
expect(updatedState[PROJECT_FEATURE_NAME]?.entities[PROJECT_A]?.taskIds).toEqual([
|
||||
TASK_ID,
|
||||
]);
|
||||
});
|
||||
|
||||
it('falls back to undefined for a null when the task’s own project is gone (#9025)', () => {
|
||||
// The current-project fallback only applies when that project is itself
|
||||
// valid; an already-orphaned task (its project deleted) resolves to
|
||||
// undefined rather than keeping a dangling reference.
|
||||
const state = createStateWithProjects('gone-project', [], []);
|
||||
const action = {
|
||||
type: '[TASK] LWW Update',
|
||||
id: TASK_ID,
|
||||
projectId: null,
|
||||
meta: { isPersistent: true, entityType: 'TASK', entityId: TASK_ID },
|
||||
};
|
||||
|
||||
reducer(state, action);
|
||||
|
||||
const updatedState = mockReducer.calls.mostRecent().args[0] as Partial<RootState>;
|
||||
expect(
|
||||
updatedState[TASK_FEATURE_NAME]?.entities[TASK_ID]?.projectId,
|
||||
).toBeUndefined();
|
||||
});
|
||||
|
||||
it('should replay a move to an archived-but-existing project', () => {
|
||||
const state = createStateWithProjects(PROJECT_A, [TASK_ID], []);
|
||||
state[PROJECT_FEATURE_NAME]!.entities[PROJECT_B] = createMockProject({
|
||||
|
|
|
|||
|
|
@ -657,9 +657,14 @@ export const lwwUpdateMetaReducer: MetaReducer = (
|
|||
? currentProjectId
|
||||
: undefined;
|
||||
|
||||
if (requestedProjectId == null) {
|
||||
entityData['projectId'] = undefined;
|
||||
} else if (
|
||||
// Any invalid destination — null/undefined, a non-string, or an unknown
|
||||
// project id — falls back to the task's current project (or undefined only
|
||||
// when that is itself invalid), mirroring the local `handleUpdateTask`
|
||||
// strip so the local and LWW-replay paths stay symmetric (#9025). This
|
||||
// applies in every mode: tasks use '' for "no project", so an explicit
|
||||
// null — even in an authoritative replace snapshot — is a malformed value
|
||||
// to sanitize, not a signal to orphan the task. '' stays valid.
|
||||
if (
|
||||
typeof requestedProjectId !== 'string' ||
|
||||
(requestedProjectId !== '' &&
|
||||
!getProjectOrUndefined(rootState, requestedProjectId))
|
||||
|
|
|
|||
|
|
@ -744,6 +744,70 @@ describe('sectionSharedMetaReducer', () => {
|
|||
expect(updated.entities['sOld']?.taskIds).toEqual([]);
|
||||
});
|
||||
|
||||
it('keeps the current-project section for a patch-mode null projectId (#9025)', () => {
|
||||
// A patch-path null keeps the task in its current project (mirroring the
|
||||
// task slice), so its current-project section membership must survive
|
||||
// while stale other-project references are still repaired.
|
||||
const state = stateWith({ parent: { projectId: 'project1' } }, [
|
||||
{
|
||||
id: 'sTarget',
|
||||
contextId: 'project1',
|
||||
contextType: WorkContextType.PROJECT,
|
||||
title: 'target section',
|
||||
taskIds: ['parent'],
|
||||
},
|
||||
{
|
||||
id: 'sStale',
|
||||
contextId: 'oldP',
|
||||
contextType: WorkContextType.PROJECT,
|
||||
title: 'stale section',
|
||||
taskIds: ['parent'],
|
||||
},
|
||||
]);
|
||||
|
||||
metaReducer(state, {
|
||||
type: '[TASK] LWW Update',
|
||||
id: 'parent',
|
||||
projectId: null,
|
||||
} as Action);
|
||||
|
||||
const updated = (
|
||||
mockReducer.calls.mostRecent().args[0] as RootState & {
|
||||
[SECTION_FEATURE_NAME]: SectionState;
|
||||
}
|
||||
)[SECTION_FEATURE_NAME];
|
||||
expect(updated.entities['sTarget']?.taskIds).toEqual(['parent']);
|
||||
expect(updated.entities['sStale']?.taskIds).toEqual([]);
|
||||
});
|
||||
|
||||
it('keeps the current-project section for a replace-mode null projectId (#9025)', () => {
|
||||
// Invalid destinations are mode-independent: even a replace snapshot's
|
||||
// null keeps the task in its current project, so its section survives.
|
||||
const state = stateWith({ parent: { projectId: 'project1' } }, [
|
||||
{
|
||||
id: 'sTarget',
|
||||
contextId: 'project1',
|
||||
contextType: WorkContextType.PROJECT,
|
||||
title: 'target section',
|
||||
taskIds: ['parent'],
|
||||
},
|
||||
]);
|
||||
|
||||
metaReducer(state, {
|
||||
type: '[TASK] LWW Update',
|
||||
id: 'parent',
|
||||
projectId: null,
|
||||
meta: { lwwUpdateMode: 'replace' },
|
||||
} as Action);
|
||||
|
||||
const updated = (
|
||||
mockReducer.calls.mostRecent().args[0] as RootState & {
|
||||
[SECTION_FEATURE_NAME]: SectionState;
|
||||
}
|
||||
)[SECTION_FEATURE_NAME];
|
||||
expect(updated.entities['sTarget']?.taskIds).toEqual(['parent']);
|
||||
});
|
||||
|
||||
it('strips project sections for reverse-linked children when LWW recreates a parent', () => {
|
||||
const state = stateWith({ sub1: { projectId: 'oldP', parentId: 'parent' } }, [
|
||||
{
|
||||
|
|
|
|||
|
|
@ -465,9 +465,12 @@ const ACTION_HANDLERS: Record<string, Handler> = {
|
|||
let targetProjectId: string | undefined = currentTask.projectId;
|
||||
let requestedProjectId: string | undefined = currentTask.projectId;
|
||||
if (hasProjectId) {
|
||||
if (update.projectId == null) {
|
||||
requestedProjectId = undefined;
|
||||
} else if (
|
||||
// Only a valid destination moves the task; any invalid one (null/
|
||||
// undefined, non-string, or an unknown project) leaves it in its current
|
||||
// project — so its current-project section survives — mirroring the task
|
||||
// slice and the local handleUpdateTask strip (#9025). '' is a valid
|
||||
// no-project value.
|
||||
if (
|
||||
typeof update.projectId === 'string' &&
|
||||
(update.projectId === '' || getProjectOrUndefined(state, update.projectId))
|
||||
) {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue