mirror of
https://github.com/johannesjo/super-productivity.git
synced 2026-07-17 16:37:43 +00:00
fix(sync): scan all tags when archiving to avoid dangling tag refs (#8710)
* fix(sync): scan all tags when archiving to avoid dangling tag refs The archive path only cleaned tags named in each task's own tagIds, so a one-sided tag->task reference (tag.taskIds holds an id the task omits) left behind by a sync replay was never removed. That dangling reference later tripped cross-model validation and forced a reconciliation/REPAIR. Lift removeTasksFromAllTags into the shared helpers and reuse it in the archive path so cleanup is symmetric with the delete path (scans every tag). Add regression coverage. * refactor(store): route project-delete tag cleanup through shared helper Replace the hand-rolled all-tags scan in handleDeleteProject with the shared removeTasksFromAllTags helper, matching the delete and archive paths (and gaining its no-op skip for tags that don't reference the deleted tasks). Move the helper into the STATE UPDATE HELPERS section (it is a state->state transform, not a list helper) and let its JSDoc own the one-sided-tag-ref rationale, trimming the duplicated archive call-site comment to a pointer. No behavior change: final tag taskIds are identical; only unaffected tags now keep their reference identity instead of being rewritten to equal arrays.
This commit is contained in:
parent
78e546ff6a
commit
2261160436
5 changed files with 80 additions and 59 deletions
|
|
@ -3,23 +3,20 @@ import { Update } from '@ngrx/entity';
|
|||
import { RootState } from '../../root-state';
|
||||
import { TaskSharedActions } from '../task-shared.actions';
|
||||
import { PROJECT_FEATURE_NAME } from '../../../features/project/store/project.reducer';
|
||||
import { TAG_FEATURE_NAME } from '../../../features/tag/store/tag.reducer';
|
||||
import {
|
||||
TASK_FEATURE_NAME,
|
||||
taskAdapter,
|
||||
} from '../../../features/tasks/store/task.reducer';
|
||||
import { TIME_TRACKING_FEATURE_KEY } from '../../../features/time-tracking/store/time-tracking.reducer';
|
||||
import { TimeTrackingState } from '../../../features/time-tracking/time-tracking.model';
|
||||
import { Tag } from '../../../features/tag/tag.model';
|
||||
import { Task, TaskWithSubTasks } from '../../../features/tasks/task.model';
|
||||
import { unique } from '../../../util/unique';
|
||||
import {
|
||||
ActionHandlerMap,
|
||||
getProject,
|
||||
getTag,
|
||||
removeTasksFromAllTags,
|
||||
removeTasksFromList,
|
||||
updateProject,
|
||||
updateTags,
|
||||
} from './task-shared-helpers';
|
||||
import { TASK_REPEAT_CFG_FEATURE_NAME } from '../../../features/task-repeat-cfg/store/task-repeat-cfg.selectors';
|
||||
import { TaskRepeatCfgState } from '../../../features/task-repeat-cfg/task-repeat-cfg.model';
|
||||
|
|
@ -140,17 +137,9 @@ const handleDeleteProject = (
|
|||
projectId: string,
|
||||
allTaskIds: string[],
|
||||
): ExtendedState => {
|
||||
const tagUpdates = (state[TAG_FEATURE_NAME].ids as string[]).map(
|
||||
(tagId): Update<Tag> => ({
|
||||
id: tagId,
|
||||
changes: {
|
||||
taskIds: removeTasksFromList(getTag(state, tagId).taskIds, allTaskIds),
|
||||
},
|
||||
}),
|
||||
);
|
||||
|
||||
// First update tags
|
||||
const stateWithUpdatedTags = updateTags(state, tagUpdates) as ExtendedState;
|
||||
// First strip the deleted project's tasks from every tag (same all-tags
|
||||
// scan the delete/archive paths use — handles one-sided refs after sync).
|
||||
const stateWithUpdatedTags = removeTasksFromAllTags(state, allTaskIds) as ExtendedState;
|
||||
|
||||
// Cleanup TIME_TRACKING for deleted project
|
||||
const updatedTimeTracking = cleanupTimeTrackingForProject(
|
||||
|
|
|
|||
|
|
@ -39,6 +39,7 @@ import {
|
|||
ProjectTaskList,
|
||||
filterOutTodayTag,
|
||||
removeTaskFromPlannerDays,
|
||||
removeTasksFromAllTags,
|
||||
removeTasksFromList,
|
||||
TaskWithTags,
|
||||
updateProject,
|
||||
|
|
@ -140,28 +141,6 @@ 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,
|
||||
|
|
|
|||
|
|
@ -46,6 +46,34 @@ export const updateTags = (state: RootState, updates: Update<Tag>[]): RootState
|
|||
[TAG_FEATURE_NAME]: tagAdapter.updateMany(updates, state[TAG_FEATURE_NAME]),
|
||||
});
|
||||
|
||||
/**
|
||||
* Removes the given task IDs from every tag's `taskIds`. Scans ALL tags from
|
||||
* the CURRENT state (not a payload-provided list or the task's own `tagIds`)
|
||||
* so sync replays with divergent tag associations are fully cleaned up: a
|
||||
* receiving client can hold a one-sided `tag.taskIds` → task reference even
|
||||
* when the task's own `tagIds` omits that tag, and only scanning `tagIds`
|
||||
* would leave it dangling. Uses a Set for O(1) lookup; tags that reference
|
||||
* none of the ids are left untouched.
|
||||
*/
|
||||
export 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);
|
||||
};
|
||||
|
||||
// =============================================================================
|
||||
// ENTITY GETTERS
|
||||
// =============================================================================
|
||||
|
|
|
|||
|
|
@ -296,6 +296,49 @@ describe('taskSharedLifecycleMetaReducer', () => {
|
|||
);
|
||||
});
|
||||
|
||||
// Regression: divergent/one-sided tag→task reference from a sync replay.
|
||||
// A receiving client can end up with `tag.taskIds` containing a task id that
|
||||
// is NOT listed in the task's own `tagIds`. The archive path used to only
|
||||
// visit tags named in `task.tagIds`, so it left this reference dangling —
|
||||
// which later tripped cross-model validation and forced a reconciliation
|
||||
// (observed in SP-logs: an archived repeating-task instance still held by a
|
||||
// regular tag). Cleanup must scan ALL tags, like the delete path does.
|
||||
it('should remove archived task from a tag that references it even when the task.tagIds omits that tag', () => {
|
||||
const testState = createStateWithExistingTasks(
|
||||
['rpt-task', 'keep-task'],
|
||||
[],
|
||||
['rpt-task', 'keep-task'], // regular tag1 references rpt-task
|
||||
[], // TODAY empty
|
||||
);
|
||||
|
||||
// One-sided reference: the task itself does NOT list tag1.
|
||||
testState[TASK_FEATURE_NAME].entities['rpt-task'] = createMockTask({
|
||||
id: 'rpt-task',
|
||||
projectId: 'project1',
|
||||
tagIds: [],
|
||||
});
|
||||
|
||||
const tasksToArchive: TaskWithSubTasks[] = [
|
||||
createTaskWithSubTasks({
|
||||
id: 'rpt-task',
|
||||
projectId: 'project1',
|
||||
tagIds: [], // payload also omits the tag
|
||||
}),
|
||||
];
|
||||
|
||||
const action = createArchiveAction(tasksToArchive);
|
||||
|
||||
metaReducer(testState, action);
|
||||
expectStateUpdate(
|
||||
{
|
||||
...expectTagUpdate('tag1', { taskIds: ['keep-task'] }),
|
||||
},
|
||||
action,
|
||||
mockReducer,
|
||||
testState,
|
||||
);
|
||||
});
|
||||
|
||||
it('should handle archiving tasks from multiple projects', () => {
|
||||
const testState = createStateWithExistingTasks(
|
||||
['task1'],
|
||||
|
|
|
|||
|
|
@ -22,6 +22,7 @@ import {
|
|||
ActionHandlerMap,
|
||||
getProject,
|
||||
getTag,
|
||||
removeTasksFromAllTags,
|
||||
removeTasksFromList,
|
||||
TaskEntity,
|
||||
updateTags,
|
||||
|
|
@ -71,28 +72,9 @@ const handleMoveToArchive = (state: RootState, tasks: TaskWithSubTasks[]): RootS
|
|||
}
|
||||
}
|
||||
|
||||
// Get tag associations from CURRENT STATE for the same reason as above.
|
||||
// Always include TODAY_TAG to ensure cleanup even if tasks aren't in it.
|
||||
const affectedTagIds = unique([
|
||||
TODAY_TAG.id,
|
||||
...taskIdsToArchive.flatMap((taskId) => {
|
||||
const task = state[TASK_FEATURE_NAME].entities[taskId];
|
||||
return task?.tagIds ?? [];
|
||||
}),
|
||||
]);
|
||||
|
||||
const tagUpdates = affectedTagIds
|
||||
.filter((tagId) => !!state[TAG_FEATURE_NAME].entities[tagId])
|
||||
.map(
|
||||
(tagId): Update<Tag> => ({
|
||||
id: tagId,
|
||||
changes: {
|
||||
taskIds: removeTasksFromList(getTag(state, tagId).taskIds, taskIdsToArchive),
|
||||
},
|
||||
}),
|
||||
);
|
||||
|
||||
return updateTags(updatedState, tagUpdates);
|
||||
// Scan every tag, not just each task's own `tagIds` — see
|
||||
// removeTasksFromAllTags for why (one-sided tag refs after sync).
|
||||
return removeTasksFromAllTags(updatedState, taskIdsToArchive);
|
||||
};
|
||||
|
||||
/**
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue