diff --git a/REFACTOR_DUPLICATION.md b/REFACTOR_DUPLICATION.md index a1da60f..a983815 100644 --- a/REFACTOR_DUPLICATION.md +++ b/REFACTOR_DUPLICATION.md @@ -9,7 +9,7 @@ Scope: Reduce high-impact code duplication in production and test-adjacent areas |---|---|---|---| | 1 | Electron API contract | ✅ Completed | Shared contract now covers renderer store data interfaces too. | | 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 | ✅ Completed | Single/batch link and unlink now share common internals. | | 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. | | 6 | Shared color utility | ✅ Completed | Shared `getContrastColor` utility is in use. | @@ -128,7 +128,7 @@ Refactor shared behavior into private helpers while preserving all task/event se ## Phase 3 — Unify Single/Batch Post-Media Operations -Status: ⚠️ Partially complete +Status: ✅ Completed ### Problem `PostMediaEngine` duplicates linking/unlinking logic across single and batch methods: @@ -148,7 +148,7 @@ Create common primitives and make single/batch methods compose them. ### Progress Check - Completed: shared primitives exist (`createPostMediaLink`, `removePostMediaLink`, sidecar helpers). -- Remaining: small duplicate segments remain between single/batch paths and can still be collapsed further. +- Completed: remaining single/batch duplicate link/unlink segments were collapsed into shared internal paths. ### Acceptance Criteria - No behavior regressions in link/unlink semantics. diff --git a/src/main/engine/PostMediaEngine.ts b/src/main/engine/PostMediaEngine.ts index 7750674..aa0f90c 100644 --- a/src/main/engine/PostMediaEngine.ts +++ b/src/main/engine/PostMediaEngine.ts @@ -98,6 +98,47 @@ export class PostMediaEngine extends EventEmitter { return this.createLinkData(link); } + private async getLinkState(postId: string): Promise<{ + existingByMediaId: Map; + nextSortOrder: number; + }> { + const existingLinks = await this.getLinkedMediaForPost(postId); + const existingByMediaId = new Map(existingLinks.map((link) => [link.mediaId, link])); + const maxSortOrder = existingLinks.length > 0 + ? Math.max(...existingLinks.map((link) => link.sortOrder)) + : -1; + + return { + existingByMediaId, + nextSortOrder: maxSortOrder + 1, + }; + } + + private async tryLinkMediaToPost( + postId: string, + mediaId: string, + state: { + existingByMediaId: Map; + nextSortOrder: number; + }, + createdAt: Date + ): Promise<{ linked: true; link: PostMediaLinkData } | { linked: false; existing: PostMediaLinkData }> { + const existing = state.existingByMediaId.get(mediaId); + if (existing) { + return { linked: false, existing }; + } + + const link = await this.createPostMediaLink(postId, mediaId, state.nextSortOrder, createdAt); + state.nextSortOrder++; + state.existingByMediaId.set(mediaId, link); + + return { linked: true, link }; + } + + private async unlinkMediaFromPostCore(postId: string, mediaId: string): Promise { + await this.removePostMediaLink(postId, mediaId); + } + private async removePostMediaLink(postId: string, mediaId: string): Promise { const db = this.getDb(); @@ -123,19 +164,14 @@ export class PostMediaEngine extends EventEmitter { * Link a media file to a post */ async linkMediaToPost(postId: string, mediaId: string): Promise { - const existingLinks = await this.getLinkedMediaForPost(postId); - const existing = existingLinks.find(link => link.mediaId === mediaId); - if (existing) { - return existing; + const state = await this.getLinkState(postId); + const result = await this.tryLinkMediaToPost(postId, mediaId, state, new Date()); + + if (!result.linked) { + return result.existing; } - // Get current highest sortOrder for this post - const maxSortOrder = existingLinks.length > 0 - ? Math.max(...existingLinks.map(l => l.sortOrder)) - : -1; - - const now = new Date(); - const linkData = await this.createPostMediaLink(postId, mediaId, maxSortOrder + 1, now); + const linkData = result.link; this.emit('mediaLinked', linkData); return linkData; @@ -145,7 +181,7 @@ export class PostMediaEngine extends EventEmitter { * Unlink a media file from a post */ async unlinkMediaFromPost(postId: string, mediaId: string): Promise { - await this.removePostMediaLink(postId, mediaId); + await this.unlinkMediaFromPostCore(postId, mediaId); this.emit('mediaUnlinked', { postId, mediaId }); } @@ -160,28 +196,17 @@ export class PostMediaEngine extends EventEmitter { const skipped: string[] = []; const uniqueMediaIds = this.getUniqueMediaIds(mediaIds); - // Get all existing links for this post to check what's already linked - const existingLinks = await this.getLinkedMediaForPost(postId); - const existingMediaIds = new Set(existingLinks.map(l => l.mediaId)); - - let maxSortOrder = existingLinks.length > 0 - ? Math.max(...existingLinks.map(l => l.sortOrder)) - : -1; - + const state = await this.getLinkState(postId); const now = new Date(); for (const mediaId of uniqueMediaIds) { - // Skip if already linked - if (existingMediaIds.has(mediaId)) { + const result = await this.tryLinkMediaToPost(postId, mediaId, state, now); + if (!result.linked) { skipped.push(mediaId); continue; } - maxSortOrder++; - await this.createPostMediaLink(postId, mediaId, maxSortOrder, now); - linked.push(mediaId); - existingMediaIds.add(mediaId); // Track to avoid duplicates within batch } // Emit a single batch event instead of per-item events @@ -201,7 +226,7 @@ export class PostMediaEngine extends EventEmitter { const uniqueMediaIds = this.getUniqueMediaIds(mediaIds); for (const mediaId of uniqueMediaIds) { - await this.removePostMediaLink(postId, mediaId); + await this.unlinkMediaFromPostCore(postId, mediaId); unlinked.push(mediaId); } diff --git a/tests/engine/PostMediaEngine.test.ts b/tests/engine/PostMediaEngine.test.ts index 413eee6..338eb95 100644 --- a/tests/engine/PostMediaEngine.test.ts +++ b/tests/engine/PostMediaEngine.test.ts @@ -395,6 +395,32 @@ describe('PostMediaEngine', () => { const sortOrders = insertedValues.map(v => v.sortOrder); expect(sortOrders).toEqual([0, 1, 2]); }); + + it('should produce same persisted link state as single-link path for one media item', async () => { + const postId = 'post-1'; + const mediaId = 'media-1'; + + selectMockData = []; + mockGetMedia.mockResolvedValue(createMockMedia({ id: mediaId, linkedPostIds: [] })); + + await engine.linkMediaToPost(postId, mediaId); + const singleInsert = insertedValues[0]; + const singleUpdateArgs = mockUpdateMedia.mock.calls[0][1]; + + insertedValues = []; + mockUpdateMedia.mockClear(); + selectMockData = []; + + const batchResult = await engine.linkManyToPost(postId, [mediaId]); + const batchInsert = insertedValues[0]; + const batchUpdateArgs = mockUpdateMedia.mock.calls[0][1]; + + expect(batchResult).toEqual({ linked: [mediaId], skipped: [] }); + expect(singleInsert.postId).toBe(batchInsert.postId); + expect(singleInsert.mediaId).toBe(batchInsert.mediaId); + expect(singleInsert.sortOrder).toBe(batchInsert.sortOrder); + expect(singleUpdateArgs).toEqual(batchUpdateArgs); + }); }); describe('unlinkManyFromPost', () => { @@ -481,6 +507,30 @@ describe('PostMediaEngine', () => { expect(deleteCallCount).toBe(2); expect(mockUpdateMedia).toHaveBeenCalledTimes(2); }); + + it('should produce same sidecar and delete effects as single-unlink path for one media item', async () => { + const postId = 'post-1'; + const mediaId = 'media-1'; + + mockGetMedia.mockResolvedValue(createMockMedia({ id: mediaId, linkedPostIds: [postId, 'other-post'] })); + + await engine.unlinkMediaFromPost(postId, mediaId); + const singleDeleteCount = deleteCallCount; + const singleUpdateArgs = mockUpdateMedia.mock.calls[0][1]; + + deleteCallCount = 0; + deleteCalled = false; + mockUpdateMedia.mockClear(); + + const batchResult = await engine.unlinkManyFromPost(postId, [mediaId]); + const batchDeleteCount = deleteCallCount; + const batchUpdateArgs = mockUpdateMedia.mock.calls[0][1]; + + expect(batchResult).toEqual({ unlinked: [mediaId] }); + expect(singleDeleteCount).toBe(1); + expect(batchDeleteCount).toBe(1); + expect(singleUpdateArgs).toEqual(batchUpdateArgs); + }); }); describe('getLinkedPostsForMedia', () => {