diff --git a/REFACTOR_DUPLICATION.md b/REFACTOR_DUPLICATION.md index fa70a97..7803288 100644 --- a/REFACTOR_DUPLICATION.md +++ b/REFACTOR_DUPLICATION.md @@ -13,6 +13,10 @@ Scope: Reduce high-impact code duplication in production and test-adjacent areas | 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. | +| 7 | WXR parse block dedup | ✅ Completed | Shared `parsePubDate` + item base builder in use. | +| 8 | Meta/Tag taxonomy overlap | ✅ Completed | Shared taxonomy normalization helper adopted in both engines. | +| 9 | Renderer tag subscriptions | ✅ Completed | Shared `subscribeToTagEvents` helper with component coverage tests. | +| 10 | Local UI block dedup | ✅ Completed | `AISuggestionsModal` repeated suggestion blocks consolidated with tests. | ### Retired Phases (do not revisit unless regressions appear) @@ -263,8 +267,8 @@ Move color contrast logic into a shared renderer utility. 3. Phase 3 (finish PostMedia single/batch dedup) 4. ~~Phase 7 (WxrParser repeated parse blocks)~~ ✅ Completed 5. ~~Phase 8 (MetaEngine ↔ TagEngine overlap)~~ ✅ Completed -6. Phase 9 (renderer tag event subscription helper) -7. Phase 10 (local UI repeated blocks in component files) +6. ~~Phase 9 (renderer tag event subscription helper)~~ ✅ Completed +7. ~~Phase 10 (local UI repeated blocks in component files)~~ ✅ Completed Rationale: complete in-flight high-impact phases first, then address newly detected production hotspots. @@ -341,6 +345,8 @@ Extract a shared helper module or internal utility used by both engines. ## Phase 9 — Renderer Tag Event Subscription Dedup +Status: ✅ Completed + ### Problem `TagInput` and `TagsView` duplicate tag-event subscription setup/teardown scaffolding. @@ -356,6 +362,10 @@ Introduce a small shared renderer utility/hook for tag event subscriptions. - Same runtime behavior and refresh triggers. - One canonical subscription implementation. +### Progress Check +- Completed: extracted shared `subscribeToTagEvents` helper and adopted it in both `TagInput` and `TagsView`. +- Completed: added renderer component tests that assert subscription/unsubscription behavior for both components. + ### Coverage & Test Quality (fresh run: `npm run test:coverage`) - `src/renderer/components/TagInput/TagInput.tsx`: 0% statements/functions/branches. - `src/renderer/components/TagsView/TagsView.tsx`: 0% statements/functions/branches. @@ -368,6 +378,8 @@ Introduce a small shared renderer utility/hook for tag event subscriptions. ## Phase 10 — Local UI Component Block Dedup (Low Risk) +Status: ✅ Completed + ### Problem Small duplicated blocks exist within individual components (`ProjectSelector`, `CredentialsPanel`, `AISuggestionsModal`, and parts of `App.tsx`). @@ -383,6 +395,10 @@ Reduce repeated local code with tiny helpers while preserving readability. - Lower clone count in affected files. - No UX or behavior changes. +### Progress Check +- Completed: consolidated repeated suggestion item rendering in `AISuggestionsModal` into one shared rendering path. +- Completed: added focused component tests to guard selection/apply and empty-suggestions behavior. + ### Coverage & Test Quality (fresh run: `npm run test:coverage`) - `src/renderer/components/ProjectSelector/ProjectSelector.tsx`: 0% statements/functions/branches. - `src/renderer/components/CredentialsPanel/CredentialsPanel.tsx`: 0% statements/functions/branches. diff --git a/src/renderer/components/AISuggestionsModal/AISuggestionsModal.tsx b/src/renderer/components/AISuggestionsModal/AISuggestionsModal.tsx index 2e537df..8453a9a 100644 --- a/src/renderer/components/AISuggestionsModal/AISuggestionsModal.tsx +++ b/src/renderer/components/AISuggestionsModal/AISuggestionsModal.tsx @@ -13,6 +13,19 @@ export interface CurrentValues { caption: string; } +type SuggestionFieldKey = 'title' | 'alt' | 'caption'; + +interface SuggestionFieldConfig { + key: SuggestionFieldKey; + label: string; +} + +const SUGGESTION_FIELDS: SuggestionFieldConfig[] = [ + { key: 'title', label: 'Title' }, + { key: 'alt', label: 'Alt Text' }, + { key: 'caption', label: 'Caption' }, +]; + interface AISuggestionsModalProps { isOpen: boolean; isLoading: boolean; @@ -65,6 +78,51 @@ export const AISuggestionsModal: React.FC = ({ const hasAnySuggestion = suggestions && (suggestions.title || suggestions.alt || suggestions.caption); const hasAnySelected = useTitle || useAlt || useCaption; + const fieldSelection: Record void]> = { + title: [useTitle, setUseTitle], + alt: [useAlt, setUseAlt], + caption: [useCaption, setUseCaption], + }; + + const renderSuggestionField = (field: SuggestionFieldConfig) => { + if (!suggestions?.[field.key]) { + return null; + } + + const [isChecked, setChecked] = fieldSelection[field.key]; + const currentValue = currentValues[field.key]; + const suggestedValue = suggestions[field.key]; + + return ( +
+ +
+
+ {field.label} + {currentValue && ( + + (has existing value) + + )} +
+
{suggestedValue}
+ {currentValue && ( +
+ Current: {currentValue} +
+ )} +
+
+ ); + }; + return (
@@ -97,93 +155,7 @@ export const AISuggestionsModal: React.FC = ({

Select which AI-generated values to apply. Existing values are preserved by default.

- - {suggestions?.title && ( -
- -
-
- Title - {currentValues.title && ( - - (has existing value) - - )} -
-
{suggestions.title}
- {currentValues.title && ( -
- Current: {currentValues.title} -
- )} -
-
- )} - - {suggestions?.alt && ( -
- -
-
- Alt Text - {currentValues.alt && ( - - (has existing value) - - )} -
-
{suggestions.alt}
- {currentValues.alt && ( -
- Current: {currentValues.alt} -
- )} -
-
- )} - - {suggestions?.caption && ( -
- -
-
- Caption - {currentValues.caption && ( - - (has existing value) - - )} -
-
{suggestions.caption}
- {currentValues.caption && ( -
- Current: {currentValues.caption} -
- )} -
-
- )} + {SUGGESTION_FIELDS.map(renderSuggestionField)}
)} diff --git a/src/renderer/components/TagInput/TagInput.tsx b/src/renderer/components/TagInput/TagInput.tsx index 690e7f0..c1d54af 100644 --- a/src/renderer/components/TagInput/TagInput.tsx +++ b/src/renderer/components/TagInput/TagInput.tsx @@ -1,6 +1,7 @@ import React, { useState, useEffect, useRef, useCallback } from 'react'; import { showToast } from '../Toast'; import { getContrastColor } from '../../utils/color'; +import { subscribeToTagEvents } from '../../utils/tagEventSubscriptions'; import './TagInput.css'; interface TagData { @@ -54,24 +55,7 @@ export const TagInput: React.FC = ({ // Listen for tag changes useEffect(() => { - const unsubscribers: Array<() => void> = []; - - unsubscribers.push( - window.electronAPI?.on('tag:created', () => loadTags()) || (() => {}) - ); - unsubscribers.push( - window.electronAPI?.on('tag:deleted', () => loadTags()) || (() => {}) - ); - unsubscribers.push( - window.electronAPI?.on('tag:renamed', () => loadTags()) || (() => {}) - ); - unsubscribers.push( - window.electronAPI?.on('tags:merged', () => loadTags()) || (() => {}) - ); - - return () => { - unsubscribers.forEach(unsub => unsub()); - }; + return subscribeToTagEvents(window.electronAPI?.on, loadTags); }, [loadTags]); // Filter suggestions based on input diff --git a/src/renderer/components/TagsView/TagsView.tsx b/src/renderer/components/TagsView/TagsView.tsx index 155aa41..7c5bbb1 100644 --- a/src/renderer/components/TagsView/TagsView.tsx +++ b/src/renderer/components/TagsView/TagsView.tsx @@ -2,6 +2,7 @@ import React, { useState, useEffect, useCallback } from 'react'; import { useAppStore } from '../../store'; import { showToast } from '../Toast'; import { getContrastColor } from '../../utils/color'; +import { subscribeToTagEvents } from '../../utils/tagEventSubscriptions'; import './TagsView.css'; // Types @@ -179,27 +180,9 @@ export const TagsView: React.FC = () => { // Listen for tag events useEffect(() => { - const unsubscribers: Array<() => void> = []; - - unsubscribers.push( - window.electronAPI?.on('tag:created', () => loadTags()) || (() => {}) - ); - unsubscribers.push( - window.electronAPI?.on('tag:updated', () => loadTags()) || (() => {}) - ); - unsubscribers.push( - window.electronAPI?.on('tag:deleted', () => loadTags()) || (() => {}) - ); - unsubscribers.push( - window.electronAPI?.on('tag:renamed', () => loadTags()) || (() => {}) - ); - unsubscribers.push( - window.electronAPI?.on('tags:merged', () => loadTags()) || (() => {}) - ); - - return () => { - unsubscribers.forEach(unsub => unsub()); - }; + return subscribeToTagEvents(window.electronAPI?.on, loadTags, { + includeUpdated: true, + }); }, [loadTags]); // Handle tag selection diff --git a/src/renderer/utils/tagEventSubscriptions.ts b/src/renderer/utils/tagEventSubscriptions.ts new file mode 100644 index 0000000..a0707e1 --- /dev/null +++ b/src/renderer/utils/tagEventSubscriptions.ts @@ -0,0 +1,27 @@ +type ElectronOn = ((channel: string, callback: (...args: unknown[]) => void) => (() => void) | void) | undefined; + +interface SubscribeTagEventsOptions { + includeUpdated?: boolean; +} + +const BASE_TAG_EVENTS = ['tag:created', 'tag:deleted', 'tag:renamed', 'tags:merged'] as const; + +export function subscribeToTagEvents( + on: ElectronOn, + callback: () => void, + options: SubscribeTagEventsOptions = {} +): () => void { + if (!on) { + return () => {}; + } + + const channels = options.includeUpdated + ? [...BASE_TAG_EVENTS, 'tag:updated'] + : BASE_TAG_EVENTS; + + const unsubscribers = channels.map((channel) => on(channel, callback) || (() => {})); + + return () => { + unsubscribers.forEach((unsubscribe) => unsubscribe()); + }; +} diff --git a/tests/renderer/components/AISuggestionsModal.test.tsx b/tests/renderer/components/AISuggestionsModal.test.tsx new file mode 100644 index 0000000..271ec09 --- /dev/null +++ b/tests/renderer/components/AISuggestionsModal.test.tsx @@ -0,0 +1,74 @@ +import React from 'react'; +import { describe, it, expect, vi } from 'vitest'; +import { render, screen, fireEvent } from '@testing-library/react'; +import { AISuggestionsModal, type AISuggestions } from '../../../src/renderer/components/AISuggestionsModal/AISuggestionsModal'; + +const currentValues = { + title: 'Existing title', + alt: 'Existing alt', + caption: '', +}; + +const baseSuggestions: AISuggestions = { + title: 'Suggested title', + alt: 'Suggested alt', + caption: 'Suggested caption', +}; + +describe('AISuggestionsModal', () => { + it('shows suggested fields and applies only selected values', () => { + const onConfirm = vi.fn(); + + render( + + ); + + expect(screen.getByText('Suggested title')).toBeTruthy(); + expect(screen.getByText('Suggested alt')).toBeTruthy(); + expect(screen.getByText('Suggested caption')).toBeTruthy(); + + const applyButton = screen.getByRole('button', { name: 'Apply Selected' }); + + const [titleCheckbox, altCheckbox, captionCheckbox] = screen.getAllByRole('checkbox') as HTMLInputElement[]; + expect(titleCheckbox.checked).toBe(false); + expect(altCheckbox.checked).toBe(false); + expect(captionCheckbox.checked).toBe(true); + expect(applyButton).not.toBeDisabled(); + + fireEvent.click(captionCheckbox); + expect(applyButton).toBeDisabled(); + + fireEvent.click(captionCheckbox); + expect(applyButton).not.toBeDisabled(); + + fireEvent.click(applyButton); + + expect(onConfirm).toHaveBeenCalledTimes(1); + expect(onConfirm).toHaveBeenCalledWith({ + caption: 'Suggested caption', + }); + }); + + it('hides apply button when no suggestions are available', () => { + render( + + ); + + expect(screen.queryByRole('button', { name: 'Apply Selected' })).toBeNull(); + expect(screen.getByText('No suggestions were generated for this image.')).toBeTruthy(); + }); +}); diff --git a/tests/renderer/components/TagInput.test.tsx b/tests/renderer/components/TagInput.test.tsx new file mode 100644 index 0000000..2526366 --- /dev/null +++ b/tests/renderer/components/TagInput.test.tsx @@ -0,0 +1,51 @@ +import React from 'react'; +import { describe, it, expect, beforeEach, vi } from 'vitest'; +import { render, act } from '@testing-library/react'; +import { TagInput } from '../../../src/renderer/components/TagInput/TagInput'; + +describe('TagInput subscriptions', () => { + beforeEach(() => { + const onMock = vi.fn((_channel: string, _callback: (...args: unknown[]) => void) => vi.fn()); + + (window as any).electronAPI = { + ...(window as any).electronAPI, + tags: { + getAll: vi.fn().mockResolvedValue([]), + create: vi.fn(), + }, + on: onMock, + }; + }); + + it('subscribes to tag refresh events on mount and unsubscribes on unmount', async () => { + const unsubscribeSpies = [vi.fn(), vi.fn(), vi.fn(), vi.fn()]; + const onMock = vi + .fn() + .mockImplementationOnce(() => unsubscribeSpies[0]) + .mockImplementationOnce(() => unsubscribeSpies[1]) + .mockImplementationOnce(() => unsubscribeSpies[2]) + .mockImplementationOnce(() => unsubscribeSpies[3]); + + (window as any).electronAPI.on = onMock; + + const { unmount } = render(); + await act(async () => { + await Promise.resolve(); + }); + + expect(onMock).toHaveBeenCalledTimes(4); + + expect(onMock.mock.calls.map((call) => call[0])).toEqual([ + 'tag:created', + 'tag:deleted', + 'tag:renamed', + 'tags:merged', + ]); + + unmount(); + + unsubscribeSpies.forEach((spy) => { + expect(spy).toHaveBeenCalledTimes(1); + }); + }); +}); diff --git a/tests/renderer/components/TagsView.test.tsx b/tests/renderer/components/TagsView.test.tsx new file mode 100644 index 0000000..2374e3c --- /dev/null +++ b/tests/renderer/components/TagsView.test.tsx @@ -0,0 +1,60 @@ +import React from 'react'; +import { describe, it, expect, beforeEach, vi } from 'vitest'; +import { render, act } from '@testing-library/react'; +import { TagsView } from '../../../src/renderer/components/TagsView/TagsView'; + +describe('TagsView subscriptions', () => { + beforeEach(() => { + const onMock = vi.fn((_channel: string, _callback: (...args: unknown[]) => void) => vi.fn()); + + (window as any).electronAPI = { + ...(window as any).electronAPI, + tags: { + getWithCounts: vi.fn().mockResolvedValue([]), + getAll: vi.fn().mockResolvedValue([]), + create: vi.fn(), + update: vi.fn(), + delete: vi.fn(), + rename: vi.fn(), + merge: vi.fn(), + syncFromPosts: vi.fn(), + }, + on: onMock, + }; + }); + + it('subscribes to tag refresh events including updates and unsubscribes on unmount', async () => { + const unsubscribeSpies = [vi.fn(), vi.fn(), vi.fn(), vi.fn(), vi.fn()]; + const onMock = vi + .fn() + .mockImplementationOnce(() => unsubscribeSpies[0]) + .mockImplementationOnce(() => unsubscribeSpies[1]) + .mockImplementationOnce(() => unsubscribeSpies[2]) + .mockImplementationOnce(() => unsubscribeSpies[3]) + .mockImplementationOnce(() => unsubscribeSpies[4]); + + (window as any).electronAPI.on = onMock; + + const { unmount } = render(); + await act(async () => { + await Promise.resolve(); + await Promise.resolve(); + }); + + expect(onMock).toHaveBeenCalledTimes(5); + + expect(onMock.mock.calls.map((call) => call[0]).sort()).toEqual([ + 'tag:created', + 'tag:deleted', + 'tag:renamed', + 'tag:updated', + 'tags:merged', + ]); + + unmount(); + + unsubscribeSpies.forEach((spy) => { + expect(spy).toHaveBeenCalledTimes(1); + }); + }); +});