diff --git a/src/app/core-ui/drop-list/drop-list.service.ts b/src/app/core-ui/drop-list/drop-list.service.ts index 824b82c061..62c8df0269 100644 --- a/src/app/core-ui/drop-list/drop-list.service.ts +++ b/src/app/core-ui/drop-list/drop-list.service.ts @@ -3,6 +3,11 @@ import { CdkDropList } from '@angular/cdk/drag-drop'; import { BehaviorSubject, merge, of, Subject, timer } from 'rxjs'; import { map, startWith, switchMap } from 'rxjs/operators'; +export interface DragPointer { + x: number; + y: number; +} + @Injectable({ providedIn: 'root', }) @@ -22,13 +27,13 @@ export class DropListService { private _list: CdkDropList[] = []; private _flushScheduled = false; - private _activeDragPointer: { x: number; y: number } | null = null; + private _activeDragPointer: DragPointer | null = null; - activeDragPointer(): { x: number; y: number } | null { + activeDragPointer(): DragPointer | null { return this._activeDragPointer; } - setActiveDragPointer(pointer: { x: number; y: number } | null): void { + setActiveDragPointer(pointer: DragPointer | null): void { this._activeDragPointer = pointer; } diff --git a/src/app/features/tasks/util/can-convert-task-to-sub-task.ts b/src/app/features/tasks/util/can-convert-task-to-sub-task.ts index d4bc78b4d0..230180fce7 100644 --- a/src/app/features/tasks/util/can-convert-task-to-sub-task.ts +++ b/src/app/features/tasks/util/can-convert-task-to-sub-task.ts @@ -23,3 +23,22 @@ export const canConvertTaskToSubTask = (task: ConvertibleTaskFields): boolean => !task.dueWithTime && !task.reminderId && !task.remindAt; + +/** + * Whether a `convertToSubTask` op may be applied to the given (already + * looked-up) task and target parent. Used by BOTH the section and crud + * meta-reducers so their guards stay in lock-step — if they diverge, one + * reducer can strip the task from its section while the other leaves it + * top-level. Rejects a missing target, self-nesting, and nesting under a task + * that is itself a subtask (the UI renders only two levels, so deeper nesting + * would orphan the task and leave parent time aggregation stale). + */ +export const canApplyConvertToSubTask = ( + task: (ConvertibleTaskFields & Pick) | undefined, + targetParent: Pick | undefined, +): boolean => + !!task && + !!targetParent && + task.id !== targetParent.id && + !targetParent.parentId && + canConvertTaskToSubTask(task); diff --git a/src/app/op-log/validation/validate-operation-payload.ts b/src/app/op-log/validation/validate-operation-payload.ts index 8dfca9faa9..e81bbc29e9 100644 --- a/src/app/op-log/validation/validate-operation-payload.ts +++ b/src/app/op-log/validation/validate-operation-payload.ts @@ -228,7 +228,11 @@ const validateUpdatePayload = ( } // convertToSubTask uses a compact intent shape rather than { task: { changes } }. - if (entityType === 'TASK' && 'taskId' in p && 'targetParentId' in p) { + if ( + entityType === 'TASK' && + typeof p['taskId'] === 'string' && + typeof p['targetParentId'] === 'string' + ) { return { success: true }; } diff --git a/src/app/root-store/meta/task-shared-meta-reducers/section-shared.reducer.spec.ts b/src/app/root-store/meta/task-shared-meta-reducers/section-shared.reducer.spec.ts index 3c6ebc858d..cc6811665c 100644 --- a/src/app/root-store/meta/task-shared-meta-reducers/section-shared.reducer.spec.ts +++ b/src/app/root-store/meta/task-shared-meta-reducers/section-shared.reducer.spec.ts @@ -227,6 +227,62 @@ describe('sectionSharedMetaReducer', () => { expect(mockReducer.calls.mostRecent().args[0]).toBe(state); }); + it('keeps section membership when the target parent does not exist', () => { + const state = stateWith({ t1: {} }, [ + { + id: 's1', + contextId: 'p1', + contextType: WorkContextType.PROJECT, + title: 'A', + taskIds: ['t1'], + }, + ]); + + metaReducer( + state, + TaskSharedActions.convertToSubTask({ + taskId: 't1', + targetParentId: 'missing-parent', + afterTaskId: null, + }), + ); + + // Must stay in lock-step with the crud reducer, which rejects a missing + // target parent — otherwise the task would vanish from its section while + // remaining top-level. + expect(mockReducer.calls.mostRecent().args[0]).toBe(state); + }); + + it('keeps section membership when the target parent is itself a subtask', () => { + const state = stateWith( + { + grandparent: { subTaskIds: ['parentSub'] }, + parentSub: { parentId: 'grandparent' }, + t1: {}, + }, + [ + { + id: 's1', + contextId: 'p1', + contextType: WorkContextType.PROJECT, + title: 'A', + taskIds: ['t1'], + }, + ], + ); + + metaReducer( + state, + TaskSharedActions.convertToSubTask({ + taskId: 't1', + targetParentId: 'parentSub', + afterTaskId: null, + }), + ); + + expect(mockReducer.calls.mostRecent().args[0]).toBe(state); + }); + it('passes through unrelated actions unchanged', () => { const state = stateWith({ t1: {} }, [ { diff --git a/src/app/root-store/meta/task-shared-meta-reducers/section-shared.reducer.ts b/src/app/root-store/meta/task-shared-meta-reducers/section-shared.reducer.ts index 0c35f6ead8..69986f0d7a 100644 --- a/src/app/root-store/meta/task-shared-meta-reducers/section-shared.reducer.ts +++ b/src/app/root-store/meta/task-shared-meta-reducers/section-shared.reducer.ts @@ -18,7 +18,7 @@ import { Task } from '../../../features/tasks/task.model'; import { WorkContextType } from '../../../features/work-context/work-context.model'; import { TODAY_TAG } from '../../../features/tag/tag.const'; import { moveItemAfterAnchor } from '../../../features/work-context/store/work-context-meta.helper'; -import { canConvertTaskToSubTask } from '../../../features/tasks/util/can-convert-task-to-sub-task'; +import { canApplyConvertToSubTask } from '../../../features/tasks/util/can-convert-task-to-sub-task'; // Must run before taskSharedCrudMetaReducer — handlers read pre-update // task state to compute cleanups. Position pinned by @@ -363,9 +363,14 @@ const ACTION_HANDLERS: Record = { return handleMoveToOtherProject(state, task.id, targetProjectId); }, [TaskSharedActions.convertToSubTask.type]: (state, action) => { - const { taskId } = action as ReturnType; + const { taskId, targetParentId } = action as ReturnType< + typeof TaskSharedActions.convertToSubTask + >; const task = state[TASK_FEATURE_NAME].entities[taskId] as Task | undefined; - if (!task || !canConvertTaskToSubTask(task)) { + const targetParent = state[TASK_FEATURE_NAME].entities[targetParentId] as + | Task + | undefined; + if (!canApplyConvertToSubTask(task, targetParent)) { return state; } return handleTaskRemoval(state, [taskId]); diff --git a/src/app/root-store/meta/task-shared-meta-reducers/task-shared-crud.reducer.spec.ts b/src/app/root-store/meta/task-shared-meta-reducers/task-shared-crud.reducer.spec.ts index 5c29baf269..ba61fa5c7f 100644 --- a/src/app/root-store/meta/task-shared-meta-reducers/task-shared-crud.reducer.spec.ts +++ b/src/app/root-store/meta/task-shared-meta-reducers/task-shared-crud.reducer.spec.ts @@ -801,6 +801,28 @@ describe('taskSharedCrudMetaReducer', () => { mockReducer.calls.reset(); }); }); + + it('should not convert under a target parent that is itself a subtask', () => { + // Nesting under a subtask would create a third level the UI cannot render + // (orphaning the task) and leave the grandparent's time aggregation stale. + const testState = createConvertToSubTaskState({}, { parentId: 'grandparent' }); + const action = createConvertToSubTaskAction(); + + metaReducer(testState, action); + expect(mockReducer).toHaveBeenCalledWith(testState, action); + }); + + it('should not convert a task onto itself', () => { + const testState = createConvertToSubTaskState(); + const action = TaskSharedActions.convertToSubTask({ + taskId: 'task1', + targetParentId: 'task1', + afterTaskId: null, + }); + + metaReducer(testState, action); + expect(mockReducer).toHaveBeenCalledWith(testState, action); + }); }); describe('deleteTask action', () => { diff --git a/src/app/root-store/meta/task-shared-meta-reducers/task-shared-crud.reducer.ts b/src/app/root-store/meta/task-shared-meta-reducers/task-shared-crud.reducer.ts index 6c6d0883ce..868fd47487 100644 --- a/src/app/root-store/meta/task-shared-meta-reducers/task-shared-crud.reducer.ts +++ b/src/app/root-store/meta/task-shared-meta-reducers/task-shared-crud.reducer.ts @@ -28,7 +28,7 @@ import { unique } from '../../../util/unique'; import { appStateFeatureKey } from '../../app-state/app-state.reducer'; import { getDbDateStr } from '../../../util/get-db-date-str'; import { moveItemAfterAnchor } from '../../../features/work-context/store/work-context-meta.helper'; -import { canConvertTaskToSubTask } from '../../../features/tasks/util/can-convert-task-to-sub-task'; +import { canApplyConvertToSubTask } from '../../../features/tasks/util/can-convert-task-to-sub-task'; import { ActionHandlerMap, addTaskToList, @@ -140,6 +140,28 @@ const handleAddTask = ( return updatedState; }; +/** + * Removes the given task IDs from every tag's `taskIds`. Scans all tags from + * the CURRENT state (not a payload-provided list) so sync replays with + * divergent tag associations are fully cleaned up. Uses a Set for O(1) lookup. + * Returns state unchanged when no tag references the tasks. + */ +const removeTasksFromAllTags = (state: RootState, taskIds: string[]): RootState => { + const taskIdSet = new Set(taskIds); + const tagUpdates = (state[TAG_FEATURE_NAME].ids as string[]) + .map((tagId) => state[TAG_FEATURE_NAME].entities[tagId]) + .filter((tag): tag is Tag => !!tag && tag.taskIds.some((id) => taskIdSet.has(id))) + .map( + (tag): Update => ({ + id: tag.id, + changes: { + taskIds: removeTasksFromList(tag.taskIds, taskIds), + }, + }), + ); + return updateTags(state, tagUpdates); +}; + const handleConvertToMainTask = ( state: RootState, task: Task, @@ -157,13 +179,13 @@ const handleConvertToMainTask = ( const todayStr = state[appStateFeatureKey]?.todayStr ?? getDbDateStr(); const resolvedParentTagIds = parentTagIds ?? parentTask.tagIds; const positionConvertedTask = (taskIds: string[]): string[] => { - if (afterTaskId === undefined) { - return unique([task.id, ...taskIds]); - } - if (afterTaskId === null && isDone) { + // Dropped at the start of DONE → append to the bottom of the done list. + if (afterTaskId == null && isDone) { return [...removeTasksFromList(taskIds, [task.id]), task.id]; } - return moveItemAfterAnchor(task.id, afterTaskId, taskIds); + // afterTaskId === undefined (legacy callers) and null both mean "prepend", + // which moveItemAfterAnchor already does for a null anchor. + return moveItemAfterAnchor(task.id, afterTaskId ?? null, taskIds); }; // Handle parent-child relationship cleanup and task entity updates @@ -248,12 +270,10 @@ const handleConvertToSubTask = ( | Task | undefined; - if ( - !task || - !targetParent || - task.id === targetParent.id || - !canConvertTaskToSubTask(task) - ) { + // The `!task || !targetParent` checks also narrow the types below; the full + // eligibility rule (incl. self-target and not-a-subtask) lives in the shared + // guard so the section meta-reducer stays in lock-step. + if (!task || !targetParent || !canApplyConvertToSubTask(task, targetParent)) { return state; } @@ -267,19 +287,7 @@ const handleConvertToSubTask = ( }); } - const taskIdSet = new Set([task.id]); - const tagUpdates = (state[TAG_FEATURE_NAME].ids as string[]) - .map((tagId) => state[TAG_FEATURE_NAME].entities[tagId]) - .filter((tag): tag is Tag => !!tag && tag.taskIds.some((id) => taskIdSet.has(id))) - .map( - (tag): Update => ({ - id: tag.id, - changes: { - taskIds: removeTasksFromList(tag.taskIds, [task.id]), - }, - }), - ); - updatedState = updateTags(updatedState, tagUpdates); + updatedState = removeTasksFromAllTags(updatedState, [task.id]); updatedState = removeTaskFromPlannerDays(updatedState, task.id); let taskState = updatedState[TASK_FEATURE_NAME]; @@ -346,34 +354,10 @@ const handleDeleteTask = ( }); } - // Update tags - find affected tags from CURRENT STATE, not payload. - // During sync, the receiving client may have different tag associations - // (e.g. an LWW Update recreated the task with different tags), so we must - // iterate all tags to ensure complete cleanup. This matches handleDeleteTasks. - const taskIdsToRemove = [task.id, ...(task.subTaskIds || [])]; - const taskIdsToRemoveSet = new Set(taskIdsToRemove); - - const affectedTagIds = (updatedState[TAG_FEATURE_NAME].ids as string[]).filter( - (tagId) => { - const tag = updatedState[TAG_FEATURE_NAME].entities[tagId]; - if (!tag) return false; - return tag.taskIds.some((taskId) => taskIdsToRemoveSet.has(taskId)); - }, - ); - - const tagUpdates = affectedTagIds.map( - (tagId): Update => ({ - id: tagId, - changes: { - taskIds: removeTasksFromList( - getTag(updatedState, tagId).taskIds, - taskIdsToRemove, - ), - }, - }), - ); - - return updateTags(updatedState, tagUpdates); + // Find affected tags from CURRENT STATE, not payload — during sync the + // receiving client may have different tag associations, so all tags are + // scanned (incl. the task's subtask ids) for complete cleanup. + return removeTasksFromAllTags(updatedState, [task.id, ...(task.subTaskIds || [])]); }; const handleDeleteTasks = (state: RootState, taskIds: string[]): RootState => { @@ -432,26 +416,8 @@ const handleDeleteTasks = (state: RootState, taskIds: string[]): RootState => { } } - // Only update tags that actually contain at least one of the tasks being deleted - // Use allIds (includes subtasks) to ensure subtask IDs are also removed from tags - // PERF: Use Set for O(1) lookup instead of O(n) Array.includes() - fixes O(n³) bottleneck - const allIdsSet = new Set(allIds); - const affectedTags = (state[TAG_FEATURE_NAME].ids as string[]).filter((tagId) => { - const tag = state[TAG_FEATURE_NAME].entities[tagId]; - if (!tag) return false; - return tag.taskIds.some((taskId) => allIdsSet.has(taskId)); - }); - - const tagUpdates = affectedTags.map( - (tagId): Update => ({ - id: tagId, - changes: { - taskIds: removeTasksFromList(getTag(state, tagId).taskIds, allIds), - }, - }), - ); - - return updateTags(updatedState, tagUpdates); + // Remove the deleted task ids (incl. subtasks via allIds) from all tags. + return removeTasksFromAllTags(updatedState, allIds); }; /**