mirror of
https://github.com/johannesjo/super-productivity.git
synced 2026-07-18 00:46:45 +00:00
fix(tasks): persist collapsed subtask state across restart (#8788)
* fix(tasks): persist collapsed subtask state across restart #8781 Collapse/expand of a task's subtasks was written by non-persistent actions, so the op-log never captured it and the state reset to expanded on every reload. Route collapse changes through the absolute, replay-safe updateTaskUi op. toggleSubTaskMode now resolves the next _hideSubTasksMode at dispatch time (extracted into a pure getNextHideSubTasksMode helper) and persists that absolute value, so it is captured to the op-log and restored on reload. Remove the old relative toggleTaskHideSubTasks action and its reducer, which was non-deterministic on replay and threw on a missing task. Register [Task] Update Task Ui in the ActionType enum and compaction code map. * test(tasks): add op-log round-trip test for _hideSubTasksMode #8781 The existing updateTaskUi test only asserted the action's persistence metadata. Add a test that runs the value through the full op-log path (dispatch -> capture -> JSON serialize -> convertOpToAction -> reducer replay) and asserts the collapse value survives. The JSON hop is where a dropped value would regress on the sync transport and SQLite op-log backend.
This commit is contained in:
parent
c08832c9dc
commit
3dc9dc8db2
9 changed files with 238 additions and 55 deletions
|
|
@ -37,9 +37,20 @@ export const __updateMultipleTaskSimple = createAction(
|
|||
}),
|
||||
);
|
||||
|
||||
// NOTE: despite the "Ui" name this carries _hideSubTasksMode, which is task
|
||||
// data that must survive a restart. It is persisted (op-log) like updateTask so
|
||||
// the collapse state is not lost on reload. See issue #8781.
|
||||
export const updateTaskUi = createAction(
|
||||
'[Task] Update Task Ui',
|
||||
props<{ task: Update<Task> }>(),
|
||||
(taskProps: { task: Update<Task> }) => ({
|
||||
...taskProps,
|
||||
meta: {
|
||||
isPersistent: true,
|
||||
entityType: 'TASK',
|
||||
entityId: taskProps.task.id as string,
|
||||
opType: OpType.Update,
|
||||
} satisfies PersistentActionMeta,
|
||||
}),
|
||||
);
|
||||
|
||||
export const removeTagsForAllTasks = createAction(
|
||||
|
|
@ -47,11 +58,6 @@ export const removeTagsForAllTasks = createAction(
|
|||
props<{ tagIdsToRemove: string[] }>(),
|
||||
);
|
||||
|
||||
export const toggleTaskHideSubTasks = createAction(
|
||||
'[Task] Toggle Show Sub Tasks',
|
||||
props<{ taskId: string; isShowLess: boolean; isEndless: boolean }>(),
|
||||
);
|
||||
|
||||
export const moveSubTask = createAction(
|
||||
'[Task] Move sub task',
|
||||
(taskProps: {
|
||||
|
|
|
|||
|
|
@ -1,6 +1,7 @@
|
|||
/* eslint-disable @typescript-eslint/naming-convention */
|
||||
import { Task, TaskDetailTargetPanel, TaskState } from '../task.model';
|
||||
import { HideSubTasksMode, Task, TaskDetailTargetPanel, TaskState } from '../task.model';
|
||||
import { initialTaskState, taskReducer } from './task.reducer';
|
||||
import { convertOpToAction } from '../../../op-log/apply/operation-converter.util';
|
||||
import * as fromActions from './task.actions';
|
||||
import { TaskSharedActions } from '../../../root-store/meta/task-shared.actions';
|
||||
import { INBOX_PROJECT } from '../../project/project.const';
|
||||
|
|
@ -11,6 +12,7 @@ import {
|
|||
import { _resetDevErrorState } from '../../../util/dev-error';
|
||||
import { PlannerActions } from '../../planner/store/planner.actions';
|
||||
import { loadAllData } from '../../../root-store/meta/load-all-data.action';
|
||||
import { ActionType, OpType, Operation } from '../../../op-log/core/operation.types';
|
||||
|
||||
describe('Task Reducer', () => {
|
||||
const createTask = (id: string, partial: Partial<Task> = {}): Task => ({
|
||||
|
|
@ -1114,4 +1116,69 @@ describe('Task Reducer', () => {
|
|||
expect(() => taskReducer(stateWithUndefined, action)).not.toThrow();
|
||||
});
|
||||
});
|
||||
|
||||
// Regression: subtask collapse state (_hideSubTasksMode) must survive a restart.
|
||||
// It only persists if the action that writes it is captured to the op-log,
|
||||
// which requires isPersistent metadata. `updateTaskUi` carries an absolute
|
||||
// value (replay-safe); toggleSubTaskMode resolves the value and dispatches it.
|
||||
// See issue #8781.
|
||||
describe('updateTaskUi persistence metadata', () => {
|
||||
it('should be a persistent TASK Update action', () => {
|
||||
const action = fromActions.updateTaskUi({
|
||||
task: { id: 'task1', changes: { _hideSubTasksMode: undefined } },
|
||||
});
|
||||
|
||||
expect(action.meta).toBeDefined();
|
||||
expect(action.meta.isPersistent).toBe(true);
|
||||
expect(action.meta.entityType).toBe('TASK');
|
||||
expect(action.meta.entityId).toBe('task1');
|
||||
expect(action.meta.opType).toBe(OpType.Update);
|
||||
});
|
||||
|
||||
// The metadata assertion above proves the change is *eligible* for capture.
|
||||
// This one proves it actually survives the whole op-log path end to end:
|
||||
// dispatch -> capture into an operation payload -> serialize -> convert the
|
||||
// op back to an action -> replay through the reducer. The JSON serialize hop
|
||||
// mirrors the sync transport and the SQLite op-log backend, both of which
|
||||
// round-trip op payloads as JSON, so it is where a lost value would regress.
|
||||
// See issue #8781.
|
||||
it('should round-trip _hideSubTasksMode through capture, serialization and replay', () => {
|
||||
const stateShown: TaskState = {
|
||||
...initialTaskState,
|
||||
ids: ['task1'],
|
||||
entities: { task1: createTask('task1') },
|
||||
};
|
||||
|
||||
// 1. Dispatch: the persistent action toggleSubTaskMode dispatches on collapse.
|
||||
const action = fromActions.updateTaskUi({
|
||||
task: {
|
||||
id: 'task1',
|
||||
changes: { _hideSubTasksMode: HideSubTasksMode.HideAll },
|
||||
},
|
||||
});
|
||||
|
||||
// 2. Capture: the effects store the action fields under payload.actionPayload.
|
||||
const op: Operation = {
|
||||
id: 'op-8781',
|
||||
actionType: action.type as ActionType,
|
||||
opType: action.meta.opType,
|
||||
entityType: action.meta.entityType,
|
||||
entityId: action.meta.entityId as string,
|
||||
payload: { actionPayload: { task: action.task }, entityChanges: [] },
|
||||
clientId: 'clientA',
|
||||
vectorClock: { clientA: 1 },
|
||||
timestamp: 0,
|
||||
schemaVersion: 1,
|
||||
};
|
||||
|
||||
// 3. Serialize over the wire / into the op-log, then read it back.
|
||||
const wireOp = JSON.parse(JSON.stringify(op)) as Operation;
|
||||
|
||||
// 4. Convert the persisted op back into a replayable action and replay it.
|
||||
const replayAction = convertOpToAction(wireOp);
|
||||
const replayed = taskReducer(stateShown, replayAction);
|
||||
|
||||
expect(replayed.entities.task1?._hideSubTasksMode).toBe(HideSubTasksMode.HideAll);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -12,7 +12,6 @@ import {
|
|||
setCurrentTask,
|
||||
setSelectedTask,
|
||||
toggleStart,
|
||||
toggleTaskHideSubTasks,
|
||||
unsetCurrentTask,
|
||||
updateTaskUi,
|
||||
} from './task.actions';
|
||||
|
|
@ -362,49 +361,6 @@ export const taskReducer = createReducer<TaskState>(
|
|||
return taskAdapter.updateMany(tasks, state);
|
||||
}),
|
||||
|
||||
// TODO simplify
|
||||
on(toggleTaskHideSubTasks, (state, { taskId, isShowLess, isEndless }) => {
|
||||
const task = getTaskById(taskId, state);
|
||||
const subTasks = task.subTaskIds.map((id) => getTaskById(id, state));
|
||||
const doneTasksLength = subTasks.filter((t) => t.isDone).length;
|
||||
const isDoneTaskCaseNeeded = doneTasksLength && doneTasksLength < subTasks.length;
|
||||
// for easier calculations we use 0 instead of undefined for show state
|
||||
const oldVal = task._hideSubTasksMode || 0;
|
||||
let newVal: number = isShowLess ? oldVal + 1 : oldVal - 1;
|
||||
|
||||
if (!isDoneTaskCaseNeeded && newVal === 1) {
|
||||
if (isShowLess) {
|
||||
newVal = 2;
|
||||
} else {
|
||||
newVal = 0;
|
||||
}
|
||||
}
|
||||
|
||||
if (isEndless) {
|
||||
if (newVal < 0) {
|
||||
newVal = 2;
|
||||
} else if (newVal > 2) {
|
||||
newVal = 0;
|
||||
}
|
||||
} else {
|
||||
if (newVal < 0) {
|
||||
newVal = 0;
|
||||
} else if (newVal > 2) {
|
||||
newVal = 2;
|
||||
}
|
||||
}
|
||||
|
||||
return taskAdapter.updateOne(
|
||||
{
|
||||
id: taskId,
|
||||
changes: {
|
||||
_hideSubTasksMode: newVal || undefined,
|
||||
},
|
||||
},
|
||||
state,
|
||||
);
|
||||
}),
|
||||
|
||||
on(moveSubTask, (state, { taskId, srcTaskId, targetTaskId, afterTaskId }) => {
|
||||
// Guard against invalid moves (e.g. 'UNDONE' passed as ID) that may
|
||||
// appear in older op-log entries. Also reject self-moves where the
|
||||
|
|
|
|||
|
|
@ -30,10 +30,10 @@ import {
|
|||
setCurrentTask,
|
||||
setSelectedTask,
|
||||
toggleStart,
|
||||
toggleTaskHideSubTasks,
|
||||
unsetCurrentTask,
|
||||
updateTaskUi,
|
||||
} from './store/task.actions';
|
||||
import { getNextHideSubTasksMode } from './util/get-next-hide-sub-tasks-mode';
|
||||
import { IssueProviderKey } from '../issue/issue.model';
|
||||
import { GlobalTrackingIntervalService } from '../../core/global-tracking-interval/global-tracking-interval.service';
|
||||
import { BatchedTimeSyncAccumulator } from '../../core/util/batched-time-sync-accumulator';
|
||||
|
|
@ -1156,7 +1156,28 @@ export class TaskService {
|
|||
isShowLess: boolean = true,
|
||||
isEndless: boolean = false,
|
||||
): void {
|
||||
this._store.dispatch(toggleTaskHideSubTasks({ taskId, isShowLess, isEndless }));
|
||||
const entities = this._taskEntities();
|
||||
const task = entities[taskId];
|
||||
if (!task) {
|
||||
return;
|
||||
}
|
||||
const subTasks = task.subTaskIds
|
||||
.map((id) => entities[id])
|
||||
.filter((t): t is Task => !!t);
|
||||
const doneCount = subTasks.filter((t) => t.isDone).length;
|
||||
// Persist the resolved absolute value via updateTaskUi (replay-safe) rather
|
||||
// than a relative toggle command, so the collapse state survives a restart.
|
||||
// Replaying a relative command would recompute from live state and diverge
|
||||
// across devices. See issue #8781.
|
||||
this.updateUi(taskId, {
|
||||
_hideSubTasksMode: getNextHideSubTasksMode(
|
||||
task._hideSubTasksMode,
|
||||
doneCount,
|
||||
subTasks.length,
|
||||
isShowLess,
|
||||
isEndless,
|
||||
),
|
||||
});
|
||||
}
|
||||
|
||||
hideSubTasks(id: string): void {
|
||||
|
|
|
|||
|
|
@ -0,0 +1,75 @@
|
|||
import { getNextHideSubTasksMode } from './get-next-hide-sub-tasks-mode';
|
||||
import { HideSubTasksMode } from '../task.model';
|
||||
|
||||
const { HideDone, HideAll } = HideSubTasksMode;
|
||||
|
||||
describe('getNextHideSubTasksMode', () => {
|
||||
describe('with a mix of done and undone subtasks (HideDone step is available)', () => {
|
||||
// 3 subtasks, 1 done → isDoneTaskCaseNeeded
|
||||
const next = (
|
||||
cur: HideSubTasksMode | undefined,
|
||||
isShowLess: boolean,
|
||||
isEndless = false,
|
||||
): HideSubTasksMode | undefined =>
|
||||
getNextHideSubTasksMode(cur, 1, 3, isShowLess, isEndless);
|
||||
|
||||
it('shows-less stepping: show → HideDone → HideAll (then clamps)', () => {
|
||||
expect(next(undefined, true)).toBe(HideDone);
|
||||
expect(next(HideDone, true)).toBe(HideAll);
|
||||
expect(next(HideAll, true)).toBe(HideAll);
|
||||
});
|
||||
|
||||
it('shows-more stepping: HideAll → HideDone → show (then clamps)', () => {
|
||||
expect(next(HideAll, false)).toBe(HideDone);
|
||||
expect(next(HideDone, false)).toBeUndefined();
|
||||
expect(next(undefined, false)).toBeUndefined();
|
||||
});
|
||||
});
|
||||
|
||||
describe('without a mix (no done, or all done) — HideDone step is skipped', () => {
|
||||
// 3 subtasks, 0 done → not isDoneTaskCaseNeeded
|
||||
const next = (
|
||||
cur: HideSubTasksMode | undefined,
|
||||
isShowLess: boolean,
|
||||
isEndless = false,
|
||||
): HideSubTasksMode | undefined =>
|
||||
getNextHideSubTasksMode(cur, 0, 3, isShowLess, isEndless);
|
||||
|
||||
it('shows-less jumps straight from show to HideAll (skipping HideDone)', () => {
|
||||
expect(next(undefined, true)).toBe(HideAll);
|
||||
});
|
||||
|
||||
it('shows-more jumps straight from HideAll to show (skipping HideDone)', () => {
|
||||
expect(next(HideAll, false)).toBeUndefined();
|
||||
});
|
||||
|
||||
it('treats all-done the same as none-done (no HideDone step)', () => {
|
||||
expect(getNextHideSubTasksMode(undefined, 3, 3, true, false)).toBe(HideAll);
|
||||
});
|
||||
});
|
||||
|
||||
describe('endless wrap-around', () => {
|
||||
const next = (
|
||||
cur: HideSubTasksMode | undefined,
|
||||
isShowLess: boolean,
|
||||
): HideSubTasksMode | undefined =>
|
||||
getNextHideSubTasksMode(cur, 0, 3, isShowLess, true);
|
||||
|
||||
it('wraps HideAll → show when stepping past the hide end', () => {
|
||||
expect(next(HideAll, true)).toBeUndefined();
|
||||
});
|
||||
|
||||
it('wraps show → HideAll when stepping past the show end', () => {
|
||||
expect(next(undefined, false)).toBe(HideAll);
|
||||
});
|
||||
});
|
||||
|
||||
it('returns undefined (not 0) when clearing collapse state', () => {
|
||||
// decrement from HideAll with no mix clears back to fully-shown
|
||||
expect(getNextHideSubTasksMode(HideAll, 0, 3, false, false)).toBeUndefined();
|
||||
});
|
||||
|
||||
it('handles a task with no subtasks without stepping into HideDone', () => {
|
||||
expect(getNextHideSubTasksMode(undefined, 0, 0, true, false)).toBe(HideAll);
|
||||
});
|
||||
});
|
||||
54
src/app/features/tasks/util/get-next-hide-sub-tasks-mode.ts
Normal file
54
src/app/features/tasks/util/get-next-hide-sub-tasks-mode.ts
Normal file
|
|
@ -0,0 +1,54 @@
|
|||
import { HideSubTasksMode } from '../task.model';
|
||||
|
||||
/**
|
||||
* Computes the next `_hideSubTasksMode` value when cycling a task's subtask
|
||||
* collapse state.
|
||||
*
|
||||
* Extracted from the old `toggleTaskHideSubTasks` reducer so the *resolved
|
||||
* absolute value* can be persisted via `updateTaskUi` (which is replay-safe)
|
||||
* instead of persisting a relative toggle command. Replaying a relative
|
||||
* command recomputes from live state and is non-deterministic across devices.
|
||||
* See issue #8781.
|
||||
*
|
||||
* @param currentMode current `_hideSubTasksMode` (undefined = fully shown)
|
||||
* @param subTaskDoneCount number of currently-done subtasks
|
||||
* @param subTaskCount total number of subtasks
|
||||
* @param isShowLess step towards hiding more (true) or showing more (false)
|
||||
* @param isEndless wrap around the ends instead of clamping
|
||||
*/
|
||||
export const getNextHideSubTasksMode = (
|
||||
currentMode: HideSubTasksMode | undefined,
|
||||
subTaskDoneCount: number,
|
||||
subTaskCount: number,
|
||||
isShowLess: boolean,
|
||||
isEndless: boolean,
|
||||
): HideSubTasksMode | undefined => {
|
||||
const isDoneTaskCaseNeeded = subTaskDoneCount > 0 && subTaskDoneCount < subTaskCount;
|
||||
// for easier calculations we use 0 instead of undefined for show state
|
||||
const oldVal = currentMode || 0;
|
||||
let newVal: number = isShowLess ? oldVal + 1 : oldVal - 1;
|
||||
|
||||
if (!isDoneTaskCaseNeeded && newVal === 1) {
|
||||
if (isShowLess) {
|
||||
newVal = 2;
|
||||
} else {
|
||||
newVal = 0;
|
||||
}
|
||||
}
|
||||
|
||||
if (isEndless) {
|
||||
if (newVal < 0) {
|
||||
newVal = 2;
|
||||
} else if (newVal > 2) {
|
||||
newVal = 0;
|
||||
}
|
||||
} else {
|
||||
if (newVal < 0) {
|
||||
newVal = 0;
|
||||
} else if (newVal > 2) {
|
||||
newVal = 2;
|
||||
}
|
||||
}
|
||||
|
||||
return (newVal || undefined) as HideSubTasksMode | undefined;
|
||||
};
|
||||
|
|
@ -12,8 +12,8 @@ describe('ActionType enum', () => {
|
|||
const enumValues = Object.values(ActionType) as string[];
|
||||
const mappingKeys = Object.keys(ACTION_TYPE_TO_CODE);
|
||||
|
||||
it('should have exactly 146 members', () => {
|
||||
expect(enumValues.length).toBe(146);
|
||||
it('should have exactly 147 members', () => {
|
||||
expect(enumValues.length).toBe(147);
|
||||
});
|
||||
|
||||
it('should have 1:1 correspondence with ACTION_TYPE_TO_CODE', () => {
|
||||
|
|
|
|||
|
|
@ -76,6 +76,9 @@ export enum ActionType {
|
|||
TASK_SHARED_REMOVE_DEADLINE = '[Task Shared] removeDeadline',
|
||||
TASK_SHARED_CLEAR_DEADLINE_REMINDER = '[Task Shared] clearDeadlineReminder',
|
||||
|
||||
// Task feature action carrying persisted _hideSubTasksMode (issue #8781)
|
||||
TASK_UPDATE_UI = '[Task] Update Task Ui',
|
||||
|
||||
// IssueProvider actions (I)
|
||||
ISSUE_PROVIDER_ADD = '[IssueProvider/API] Add IssueProvider',
|
||||
ISSUE_PROVIDER_UPDATE = '[IssueProvider/API] Update IssueProvider',
|
||||
|
|
|
|||
|
|
@ -193,6 +193,7 @@ export const ACTION_TYPE_TO_CODE: Record<ActionType, string> = {
|
|||
[ActionType.TASK_REMOVE_TIME_SPENT]: 'TR',
|
||||
[ActionType.TASK_ROUND_TIME_SPENT]: 'TRD',
|
||||
[ActionType.TASK_ADD_TAGS_SHORT_SYNTAX]: 'TGS',
|
||||
[ActionType.TASK_UPDATE_UI]: 'TUU',
|
||||
|
||||
// Plugin actions (U)
|
||||
[ActionType.PLUGIN_UPSERT_USER_DATA]: 'UU',
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue