fix: phase 2 refactoring
This commit is contained in:
@@ -8,7 +8,7 @@ Scope: Reduce high-impact code duplication in production and test-adjacent areas
|
|||||||
| Phase | Area | Status | Notes |
|
| Phase | Area | Status | Notes |
|
||||||
|---|---|---|---|
|
|---|---|---|---|
|
||||||
| 1 | Electron API contract | ✅ Completed | Shared contract now covers renderer store data interfaces too. |
|
| 1 | Electron API contract | ✅ Completed | Shared contract now covers renderer store data interfaces too. |
|
||||||
| 2 | Tag mutation workflow | ⚠️ Partial | Helpers exist; duplicate blocks still present in task flows. |
|
| 2 | Tag mutation workflow | ✅ Completed | Shared task workflow now used by delete/merge/rename paths. |
|
||||||
| 3 | Post-media single/batch ops | ⚠️ Partial | Core primitives extracted; some duplicate link/unlink logic remains. |
|
| 3 | Post-media single/batch ops | ⚠️ Partial | Core primitives extracted; some duplicate link/unlink logic remains. |
|
||||||
| 4 | Project mapping + delete guards | ✅ Completed | Guard + mapper extracted and used in target methods. |
|
| 4 | Project mapping + delete guards | ✅ Completed | Guard + mapper extracted and used in target methods. |
|
||||||
| 5 | Metadata sync loop scaffolding | ✅ Completed | `runSyncLoop` extracted and reused by both sync directions. |
|
| 5 | Metadata sync loop scaffolding | ✅ Completed | `runSyncLoop` extracted and reused by both sync directions. |
|
||||||
@@ -82,7 +82,7 @@ Create one canonical API contract and consume it from both preload and renderer
|
|||||||
|
|
||||||
## Phase 2 — Extract Common Tag Mutation Workflow
|
## Phase 2 — Extract Common Tag Mutation Workflow
|
||||||
|
|
||||||
Status: ⚠️ Partially complete
|
Status: ✅ Completed
|
||||||
|
|
||||||
### Problem
|
### Problem
|
||||||
`TagEngine` repeats similar logic in:
|
`TagEngine` repeats similar logic in:
|
||||||
@@ -106,7 +106,7 @@ Refactor shared behavior into private helpers while preserving all task/event se
|
|||||||
|
|
||||||
### Progress Check
|
### Progress Check
|
||||||
- Completed: helper extraction has started (`queryPostsContainingTag`, `updatePostTagsAndSync`, and shared update path usage).
|
- Completed: helper extraction has started (`queryPostsContainingTag`, `updatePostTagsAndSync`, and shared update path usage).
|
||||||
- Remaining: duplicate progress/task scaffolding still exists across `deleteTag` / `mergeTags` / `renameTag`.
|
- Completed: duplicate progress/task scaffolding was consolidated across `deleteTag` / `mergeTags` / `renameTag` via shared task workflow.
|
||||||
|
|
||||||
### Acceptance Criteria
|
### Acceptance Criteria
|
||||||
- Public method behavior and events unchanged.
|
- Public method behavior and events unchanged.
|
||||||
|
|||||||
@@ -215,6 +215,38 @@ export class TagEngine extends EventEmitter {
|
|||||||
};
|
};
|
||||||
}
|
}
|
||||||
|
|
||||||
|
private async runTagMutationTask<TResult>(options: {
|
||||||
|
taskId: string;
|
||||||
|
taskName: string;
|
||||||
|
startMessage: string;
|
||||||
|
runUpdates: (onProgress: (progress: number, message: string) => void) => Promise<number>;
|
||||||
|
finalizeMessage: string;
|
||||||
|
finalize: () => Promise<void>;
|
||||||
|
buildResult: (postsUpdated: number) => TResult;
|
||||||
|
onComplete: (result: TResult) => Promise<void>;
|
||||||
|
}): Promise<TResult> {
|
||||||
|
return taskManager.runTask({
|
||||||
|
id: options.taskId,
|
||||||
|
name: options.taskName,
|
||||||
|
execute: async (onProgress) => {
|
||||||
|
onProgress(0, options.startMessage);
|
||||||
|
|
||||||
|
const postsUpdated = await options.runUpdates(onProgress);
|
||||||
|
|
||||||
|
onProgress(90, options.finalizeMessage);
|
||||||
|
await options.finalize();
|
||||||
|
|
||||||
|
onProgress(100, 'Complete');
|
||||||
|
|
||||||
|
const result = options.buildResult(postsUpdated);
|
||||||
|
await options.onComplete(result);
|
||||||
|
await this.saveTagsToFile();
|
||||||
|
|
||||||
|
return result;
|
||||||
|
},
|
||||||
|
});
|
||||||
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Returns the default internal project directory (in userData).
|
* Returns the default internal project directory (in userData).
|
||||||
*/
|
*/
|
||||||
@@ -408,38 +440,32 @@ export class TagEngine extends EventEmitter {
|
|||||||
const tag = await this.getTagRowOrThrow(id);
|
const tag = await this.getTagRowOrThrow(id);
|
||||||
const tagName = tag.name;
|
const tagName = tag.name;
|
||||||
|
|
||||||
// Run the deletion as a background task
|
return this.runTagMutationTask({
|
||||||
return taskManager.runTask({
|
taskId: `delete-tag-${id}-${Date.now()}`,
|
||||||
id: `delete-tag-${id}-${Date.now()}`,
|
taskName: `Delete tag "${tagName}"`,
|
||||||
name: `Delete tag "${tagName}"`,
|
startMessage: `Finding posts with tag "${tagName}"...`,
|
||||||
execute: async (onProgress) => {
|
runUpdates: async (onProgress) => {
|
||||||
onProgress(0, `Finding posts with tag "${tagName}"...`);
|
|
||||||
|
|
||||||
const updateOperation = await this.updateMatchingPosts(
|
const updateOperation = await this.updateMatchingPosts(
|
||||||
tagName,
|
tagName,
|
||||||
(postTags) => postTags.filter((tagEntry) => tagEntry !== tagName)
|
(postTags) => postTags.filter((tagEntry) => tagEntry !== tagName)
|
||||||
);
|
);
|
||||||
|
|
||||||
const updated = await updateOperation.process((updatedCount, totalCount) => {
|
return updateOperation.process((updatedCount, totalCount) => {
|
||||||
onProgress((updatedCount / totalCount) * 80, `Updated ${updatedCount}/${totalCount} posts...`);
|
onProgress((updatedCount / totalCount) * 80, `Updated ${updatedCount}/${totalCount} posts...`);
|
||||||
});
|
});
|
||||||
|
},
|
||||||
onProgress(90, 'Deleting tag...');
|
finalizeMessage: 'Deleting tag...',
|
||||||
|
finalize: async () => {
|
||||||
// Delete the tag
|
|
||||||
await db
|
await db
|
||||||
.delete(tags)
|
.delete(tags)
|
||||||
.where(and(
|
.where(and(
|
||||||
eq(tags.id, id),
|
eq(tags.id, id),
|
||||||
eq(tags.projectId, this.currentProjectId)
|
eq(tags.projectId, this.currentProjectId)
|
||||||
));
|
));
|
||||||
|
},
|
||||||
onProgress(100, 'Complete');
|
buildResult: (postsUpdated) => ({ success: true, postsUpdated }),
|
||||||
|
onComplete: async () => {
|
||||||
this.emit('tagDeleted', id);
|
this.emit('tagDeleted', id);
|
||||||
await this.saveTagsToFile();
|
|
||||||
|
|
||||||
return { success: true, postsUpdated: updated };
|
|
||||||
},
|
},
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
@@ -486,19 +512,16 @@ export class TagEngine extends EventEmitter {
|
|||||||
const targetName = targetTag.name;
|
const targetName = targetTag.name;
|
||||||
const sourceNames = sourceTags.map(t => t.name);
|
const sourceNames = sourceTags.map(t => t.name);
|
||||||
|
|
||||||
// Run as background task
|
return this.runTagMutationTask({
|
||||||
return taskManager.runTask({
|
taskId: `merge-tags-${Date.now()}`,
|
||||||
id: `merge-tags-${Date.now()}`,
|
taskName: `Merge tags into "${targetName}"`,
|
||||||
name: `Merge tags into "${targetName}"`,
|
startMessage: 'Finding posts to update...',
|
||||||
execute: async (onProgress) => {
|
runUpdates: async (onProgress) => {
|
||||||
onProgress(0, 'Finding posts to update...');
|
|
||||||
|
|
||||||
const updatedPostTagsById = new Map<string, string[]>();
|
const updatedPostTagsById = new Map<string, string[]>();
|
||||||
|
|
||||||
// For each source tag, compute final post tags per post ID
|
|
||||||
for (let index = 0; index < sourceNames.length; index++) {
|
for (let index = 0; index < sourceNames.length; index++) {
|
||||||
const sourceName = sourceNames[index];
|
const sourceName = sourceNames[index];
|
||||||
onProgress((index / sourceNames.length) * 80, `Processing tag "${sourceName}"...`);
|
onProgress(((index + 1) / sourceNames.length) * 80, `Processing tag "${sourceName}"...`);
|
||||||
|
|
||||||
const postsWithSourceTag = await this.queryPostsContainingTag(sourceName);
|
const postsWithSourceTag = await this.queryPostsContainingTag(sourceName);
|
||||||
for (const row of postsWithSourceTag) {
|
for (const row of postsWithSourceTag) {
|
||||||
@@ -520,33 +543,27 @@ export class TagEngine extends EventEmitter {
|
|||||||
await this.updatePostTagsAndSync(postId, updatedTags);
|
await this.updatePostTagsAndSync(postId, updatedTags);
|
||||||
}
|
}
|
||||||
|
|
||||||
const totalPostsUpdated = updatedPostTagsById.size;
|
return updatedPostTagsById.size;
|
||||||
|
},
|
||||||
onProgress(90, 'Deleting source tags...');
|
finalizeMessage: 'Deleting source tags...',
|
||||||
|
finalize: async () => {
|
||||||
// Delete source tags
|
for (const sourceId of sourceTagIds) {
|
||||||
for (const id of sourceTagIds) {
|
|
||||||
await db
|
await db
|
||||||
.delete(tags)
|
.delete(tags)
|
||||||
.where(and(
|
.where(and(
|
||||||
eq(tags.id, id),
|
eq(tags.id, sourceId),
|
||||||
eq(tags.projectId, this.currentProjectId)
|
eq(tags.projectId, this.currentProjectId)
|
||||||
));
|
));
|
||||||
}
|
}
|
||||||
|
},
|
||||||
onProgress(100, 'Complete');
|
buildResult: (postsUpdated) => ({
|
||||||
|
success: true,
|
||||||
const result: MergeTagsResult = {
|
postsUpdated,
|
||||||
success: true,
|
tagsDeleted: sourceTagIds.length,
|
||||||
postsUpdated: totalPostsUpdated,
|
targetTag: targetName,
|
||||||
tagsDeleted: sourceTagIds.length,
|
}),
|
||||||
targetTag: targetName,
|
onComplete: async (result) => {
|
||||||
};
|
|
||||||
|
|
||||||
this.emit('tagsMerged', result);
|
this.emit('tagsMerged', result);
|
||||||
await this.saveTagsToFile();
|
|
||||||
|
|
||||||
return result;
|
|
||||||
},
|
},
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
@@ -596,25 +613,22 @@ export class TagEngine extends EventEmitter {
|
|||||||
throw new Error(`Tag "${newName}" already exists`);
|
throw new Error(`Tag "${newName}" already exists`);
|
||||||
}
|
}
|
||||||
|
|
||||||
// Run as background task
|
return this.runTagMutationTask({
|
||||||
return taskManager.runTask({
|
taskId: `rename-tag-${id}-${Date.now()}`,
|
||||||
id: `rename-tag-${id}-${Date.now()}`,
|
taskName: `Rename tag "${oldName}" to "${newName}"`,
|
||||||
name: `Rename tag "${oldName}" to "${newName}"`,
|
startMessage: 'Finding posts to update...',
|
||||||
execute: async (onProgress) => {
|
runUpdates: async (onProgress) => {
|
||||||
onProgress(0, 'Finding posts to update...');
|
|
||||||
|
|
||||||
const updateOperation = await this.updateMatchingPosts(
|
const updateOperation = await this.updateMatchingPosts(
|
||||||
oldName,
|
oldName,
|
||||||
(postTags) => postTags.map((tagEntry) => tagEntry === oldName ? newName : tagEntry)
|
(postTags) => postTags.map((tagEntry) => tagEntry === oldName ? newName : tagEntry)
|
||||||
);
|
);
|
||||||
|
|
||||||
const updated = await updateOperation.process((updatedCount, totalCount) => {
|
return updateOperation.process((updatedCount, totalCount) => {
|
||||||
onProgress((updatedCount / totalCount) * 80, `Updated ${updatedCount}/${totalCount} posts...`);
|
onProgress((updatedCount / totalCount) * 80, `Updated ${updatedCount}/${totalCount} posts...`);
|
||||||
});
|
});
|
||||||
|
},
|
||||||
onProgress(90, 'Updating tag record...');
|
finalizeMessage: 'Updating tag record...',
|
||||||
|
finalize: async () => {
|
||||||
// Update the tag name
|
|
||||||
await db
|
await db
|
||||||
.update(tags)
|
.update(tags)
|
||||||
.set({
|
.set({
|
||||||
@@ -625,20 +639,15 @@ export class TagEngine extends EventEmitter {
|
|||||||
eq(tags.id, id),
|
eq(tags.id, id),
|
||||||
eq(tags.projectId, this.currentProjectId)
|
eq(tags.projectId, this.currentProjectId)
|
||||||
));
|
));
|
||||||
|
},
|
||||||
onProgress(100, 'Complete');
|
buildResult: (postsUpdated) => ({
|
||||||
|
success: true,
|
||||||
const result: RenameTagResult = {
|
postsUpdated,
|
||||||
success: true,
|
oldName,
|
||||||
postsUpdated: updated,
|
newName,
|
||||||
oldName,
|
}),
|
||||||
newName,
|
onComplete: async (result) => {
|
||||||
};
|
|
||||||
|
|
||||||
this.emit('tagRenamed', result);
|
this.emit('tagRenamed', result);
|
||||||
await this.saveTagsToFile();
|
|
||||||
|
|
||||||
return result;
|
|
||||||
},
|
},
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -9,6 +9,7 @@ import { describe, it, expect, beforeEach, vi, afterEach } from 'vitest';
|
|||||||
import { TagEngine, TagData, TagWithCount, MergeTagsResult, DeleteTagResult } from '../../src/main/engine/TagEngine';
|
import { TagEngine, TagData, TagWithCount, MergeTagsResult, DeleteTagResult } from '../../src/main/engine/TagEngine';
|
||||||
import { resetMockCounters } from '../utils/factories';
|
import { resetMockCounters } from '../utils/factories';
|
||||||
import { getDatabase } from '../../src/main/database';
|
import { getDatabase } from '../../src/main/database';
|
||||||
|
import { taskManager } from '../../src/main/engine/TaskManager';
|
||||||
|
|
||||||
// Create mock data stores
|
// Create mock data stores
|
||||||
const mockTags = new Map<string, any>();
|
const mockTags = new Map<string, any>();
|
||||||
@@ -135,6 +136,18 @@ vi.mock('../../src/main/engine/PostEngine', () => ({
|
|||||||
describe('TagEngine', () => {
|
describe('TagEngine', () => {
|
||||||
let tagEngine: TagEngine;
|
let tagEngine: TagEngine;
|
||||||
|
|
||||||
|
const captureProgressForNextTask = (): Array<{ progress: number; message: string }> => {
|
||||||
|
const progressCalls: Array<{ progress: number; message: string }> = [];
|
||||||
|
|
||||||
|
vi.mocked(taskManager.runTask).mockImplementationOnce(async (task: any) => {
|
||||||
|
return task.execute((progress: number, message: string) => {
|
||||||
|
progressCalls.push({ progress, message });
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
return progressCalls;
|
||||||
|
};
|
||||||
|
|
||||||
beforeEach(() => {
|
beforeEach(() => {
|
||||||
vi.clearAllMocks();
|
vi.clearAllMocks();
|
||||||
mockTags.clear();
|
mockTags.clear();
|
||||||
@@ -320,6 +333,29 @@ describe('TagEngine', () => {
|
|||||||
expect(result.postsUpdated).toBe(1);
|
expect(result.postsUpdated).toBe(1);
|
||||||
expect(mockPostEngine.syncPublishedPostFile).toHaveBeenCalledTimes(1);
|
expect(mockPostEngine.syncPublishedPostFile).toHaveBeenCalledTimes(1);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('should report stable progress checkpoints during delete', async () => {
|
||||||
|
mockSelectDataQueue = [
|
||||||
|
[{ id: 'tag-1', name: 'react', color: null, projectId: 'default', createdAt: new Date(), updatedAt: new Date() }],
|
||||||
|
];
|
||||||
|
mockLocalClient.execute.mockResolvedValueOnce({
|
||||||
|
rows: [
|
||||||
|
{ id: 'post-1', tags: '["react", "typescript"]' },
|
||||||
|
{ id: 'post-2', tags: '["react"]' },
|
||||||
|
],
|
||||||
|
});
|
||||||
|
|
||||||
|
const progressCalls = captureProgressForNextTask();
|
||||||
|
await tagEngine.deleteTag('tag-1');
|
||||||
|
|
||||||
|
expect(progressCalls).toEqual([
|
||||||
|
{ progress: 0, message: 'Finding posts with tag "react"...' },
|
||||||
|
{ progress: 40, message: 'Updated 1/2 posts...' },
|
||||||
|
{ progress: 80, message: 'Updated 2/2 posts...' },
|
||||||
|
{ progress: 90, message: 'Deleting tag...' },
|
||||||
|
{ progress: 100, message: 'Complete' },
|
||||||
|
]);
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
describe('mergeTags', () => {
|
describe('mergeTags', () => {
|
||||||
@@ -386,6 +422,29 @@ describe('TagEngine', () => {
|
|||||||
expect(result.postsUpdated).toBe(1);
|
expect(result.postsUpdated).toBe(1);
|
||||||
expect(mockPostEngine.syncPublishedPostFile).toHaveBeenCalledTimes(1);
|
expect(mockPostEngine.syncPublishedPostFile).toHaveBeenCalledTimes(1);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('should report stable progress checkpoints during merge', async () => {
|
||||||
|
mockSelectDataQueue = [
|
||||||
|
[{ id: 'tag-1', name: 'js', projectId: 'default', createdAt: new Date(), updatedAt: new Date() }],
|
||||||
|
[{ id: 'tag-2', name: 'javascript', projectId: 'default', createdAt: new Date(), updatedAt: new Date() }],
|
||||||
|
[{ id: 'tag-3', name: 'ecmascript', projectId: 'default', createdAt: new Date(), updatedAt: new Date() }],
|
||||||
|
];
|
||||||
|
|
||||||
|
mockLocalClient.execute
|
||||||
|
.mockResolvedValueOnce({ rows: [{ id: 'post-1', tags: '["js"]' }] })
|
||||||
|
.mockResolvedValueOnce({ rows: [{ id: 'post-2', tags: '["javascript"]' }] });
|
||||||
|
|
||||||
|
const progressCalls = captureProgressForNextTask();
|
||||||
|
await tagEngine.mergeTags(['tag-1', 'tag-2'], 'tag-3');
|
||||||
|
|
||||||
|
expect(progressCalls).toEqual([
|
||||||
|
{ progress: 0, message: 'Finding posts to update...' },
|
||||||
|
{ progress: 40, message: 'Processing tag "js"...' },
|
||||||
|
{ progress: 80, message: 'Processing tag "javascript"...' },
|
||||||
|
{ progress: 90, message: 'Deleting source tags...' },
|
||||||
|
{ progress: 100, message: 'Complete' },
|
||||||
|
]);
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
describe('renameTags (batch rename)', () => {
|
describe('renameTags (batch rename)', () => {
|
||||||
@@ -440,6 +499,30 @@ describe('TagEngine', () => {
|
|||||||
expect(result.postsUpdated).toBe(1);
|
expect(result.postsUpdated).toBe(1);
|
||||||
expect(mockPostEngine.syncPublishedPostFile).toHaveBeenCalledTimes(1);
|
expect(mockPostEngine.syncPublishedPostFile).toHaveBeenCalledTimes(1);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('should report stable progress checkpoints during rename', async () => {
|
||||||
|
mockSelectDataQueue = [
|
||||||
|
[{ id: 'tag-1', name: 'old-name', projectId: 'default', createdAt: new Date(), updatedAt: new Date() }],
|
||||||
|
[],
|
||||||
|
];
|
||||||
|
mockLocalClient.execute.mockResolvedValueOnce({
|
||||||
|
rows: [
|
||||||
|
{ id: 'post-1', tags: '["old-name"]' },
|
||||||
|
{ id: 'post-2', tags: '["old-name", "other"]' },
|
||||||
|
],
|
||||||
|
});
|
||||||
|
|
||||||
|
const progressCalls = captureProgressForNextTask();
|
||||||
|
await tagEngine.renameTag('tag-1', 'new-name');
|
||||||
|
|
||||||
|
expect(progressCalls).toEqual([
|
||||||
|
{ progress: 0, message: 'Finding posts to update...' },
|
||||||
|
{ progress: 40, message: 'Updated 1/2 posts...' },
|
||||||
|
{ progress: 80, message: 'Updated 2/2 posts...' },
|
||||||
|
{ progress: 90, message: 'Updating tag record...' },
|
||||||
|
{ progress: 100, message: 'Complete' },
|
||||||
|
]);
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
describe('getTag', () => {
|
describe('getTag', () => {
|
||||||
|
|||||||
Reference in New Issue
Block a user