fix(tasks): harden convertToSubTask guards and dedupe tag cleanup

Address review findings on the drag-to-subtask feature (#7905):

- Keep the section and crud meta-reducer guards in lock-step via a shared
  canApplyConvertToSubTask(). Previously the section reducer stripped a task
  from its section even when the crud reducer rejected the convert (missing
  target parent or self-target), leaving the task top-level yet dropped from
  section ordering on a replayed/concurrent op.
- Reject nesting under a target that is itself a subtask. The UI renders only
  two levels, so deeper nesting would orphan the task and leave the
  grandparent's time aggregation stale.
- Tighten op-log payload validation to require string taskId/targetParentId.
- Dedupe the "remove task ids from all tags" logic shared by
  convertToSubTask/deleteTask/deleteTasks into removeTasksFromAllTags().
- Collapse the tri-state afterTaskId positioning in handleConvertToMainTask
  (undefined and null both prepend via moveItemAfterAnchor).
- Name the DragPointer type in DropListService.

Adds reducer specs for the rejected-target-parent cases.

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;
   }
 

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<Task, 'id'>) | undefined,
+  targetParent: Pick<Task, 'id' | 'parentId'> | undefined,
+): boolean =>
+  !!task &&
+  !!targetParent &&
+  task.id !== targetParent.id &&
+  !targetParent.parentId &&
+  canConvertTaskToSubTask(task);

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 };
   }
 

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: {} }, [
       {

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<string, Handler> = {
     return handleMoveToOtherProject(state, task.id, targetProjectId);
   },
   [TaskSharedActions.convertToSubTask.type]: (state, action) => {
-    const { taskId } = action as ReturnType<typeof TaskSharedActions.convertToSubTask>;
+    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]);

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', () => {

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<Tag> => ({
+        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<Tag> => ({
-        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<Tag> => ({
-      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<Tag> => ({
-      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);
 };
 
 /**