diff --git a/src/app/features/tasks/store/task.actions.ts b/src/app/features/tasks/store/task.actions.ts index 44941b5872..ce0d68fa6b 100644 --- a/src/app/features/tasks/store/task.actions.ts +++ b/src/app/features/tasks/store/task.actions.ts @@ -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 }>(), + (taskProps: { task: Update }) => ({ + ...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: { diff --git a/src/app/features/tasks/store/task.reducer.spec.ts b/src/app/features/tasks/store/task.reducer.spec.ts index 98ae25be1c..d66e73d3d5 100644 --- a/src/app/features/tasks/store/task.reducer.spec.ts +++ b/src/app/features/tasks/store/task.reducer.spec.ts @@ -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 => ({ @@ -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); + }); + }); }); diff --git a/src/app/features/tasks/store/task.reducer.ts b/src/app/features/tasks/store/task.reducer.ts index a09e13d0a3..dd06fe8a80 100644 --- a/src/app/features/tasks/store/task.reducer.ts +++ b/src/app/features/tasks/store/task.reducer.ts @@ -12,7 +12,6 @@ import { setCurrentTask, setSelectedTask, toggleStart, - toggleTaskHideSubTasks, unsetCurrentTask, updateTaskUi, } from './task.actions'; @@ -362,49 +361,6 @@ export const taskReducer = createReducer( 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 diff --git a/src/app/features/tasks/task.service.ts b/src/app/features/tasks/task.service.ts index 27f5c02bae..3c595319ed 100644 --- a/src/app/features/tasks/task.service.ts +++ b/src/app/features/tasks/task.service.ts @@ -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 { diff --git a/src/app/features/tasks/util/get-next-hide-sub-tasks-mode.spec.ts b/src/app/features/tasks/util/get-next-hide-sub-tasks-mode.spec.ts new file mode 100644 index 0000000000..1444fd5ae8 --- /dev/null +++ b/src/app/features/tasks/util/get-next-hide-sub-tasks-mode.spec.ts @@ -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); + }); +}); diff --git a/src/app/features/tasks/util/get-next-hide-sub-tasks-mode.ts b/src/app/features/tasks/util/get-next-hide-sub-tasks-mode.ts new file mode 100644 index 0000000000..0c7c57acb9 --- /dev/null +++ b/src/app/features/tasks/util/get-next-hide-sub-tasks-mode.ts @@ -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; +}; diff --git a/src/app/op-log/core/action-types.enum.spec.ts b/src/app/op-log/core/action-types.enum.spec.ts index c7880d0796..e157dd2b5c 100644 --- a/src/app/op-log/core/action-types.enum.spec.ts +++ b/src/app/op-log/core/action-types.enum.spec.ts @@ -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', () => { diff --git a/src/app/op-log/core/action-types.enum.ts b/src/app/op-log/core/action-types.enum.ts index c9dcb680b7..f6f587b0dd 100644 --- a/src/app/op-log/core/action-types.enum.ts +++ b/src/app/op-log/core/action-types.enum.ts @@ -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', diff --git a/src/app/op-log/persistence/compact/action-type-codes.ts b/src/app/op-log/persistence/compact/action-type-codes.ts index e0ac03286d..e13adf6033 100644 --- a/src/app/op-log/persistence/compact/action-type-codes.ts +++ b/src/app/op-log/persistence/compact/action-type-codes.ts @@ -193,6 +193,7 @@ export const ACTION_TYPE_TO_CODE: Record = { [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',