From baa911894266fdc70991a2f9d4713b36965e7457 Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Wed, 18 Feb 2026 00:47:42 +0000 Subject: [PATCH] feat(frontend): optimize ConversionHistory rendering with memoization Extract `ConversionHistoryItem` into a memoized component to prevent unnecessary re-renders of the entire list when a single item is toggled or when the parent component updates. Refactor handlers (`onToggle`, `onDelete`, `onDownload`) to be stable using `useCallback` and functional state updates. Add unit tests for `ConversionHistory` component to verify rendering and interactions. Impact: - Reduces re-renders of list items when selecting/deselecting items. - Improves responsiveness of the history list, especially with many items. - Adds test coverage for the component. Verification: - `pnpm test src/components/ConversionHistory` passes. - Frontend verification script confirmed UI functionality. Co-authored-by: anchapin <6326294+anchapin@users.noreply.github.com> --- .../ConversionHistory.test.tsx | 103 ++++++++ .../ConversionHistory/ConversionHistory.tsx | 249 ++++++------------ .../ConversionHistoryItem.tsx | 104 ++++++++ .../src/components/ConversionHistory/types.ts | 14 + .../src/components/ConversionHistory/utils.ts | 34 +++ 5 files changed, 332 insertions(+), 172 deletions(-) create mode 100644 frontend/src/components/ConversionHistory/ConversionHistory.test.tsx create mode 100644 frontend/src/components/ConversionHistory/ConversionHistoryItem.tsx create mode 100644 frontend/src/components/ConversionHistory/types.ts create mode 100644 frontend/src/components/ConversionHistory/utils.ts diff --git a/frontend/src/components/ConversionHistory/ConversionHistory.test.tsx b/frontend/src/components/ConversionHistory/ConversionHistory.test.tsx new file mode 100644 index 00000000..5ec1a679 --- /dev/null +++ b/frontend/src/components/ConversionHistory/ConversionHistory.test.tsx @@ -0,0 +1,103 @@ +import { render, screen, fireEvent, waitFor } from '@testing-library/react'; +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import ConversionHistory from './ConversionHistory'; +import { ConversionHistoryItem } from './types'; +import React from 'react'; + +// Mock localStorage +const localStorageMock = (() => { + let store: Record = {}; + return { + getItem: vi.fn((key: string) => store[key] || null), + setItem: vi.fn((key: string, value: string) => { + store[key] = value.toString(); + }), + removeItem: vi.fn((key: string) => { + delete store[key]; + }), + clear: vi.fn(() => { + store = {}; + }), + }; +})(); + +Object.defineProperty(window, 'localStorage', { + value: localStorageMock, +}); + +// Mock fetch +global.fetch = vi.fn(); + +describe('ConversionHistory', () => { + const mockHistoryItems: ConversionHistoryItem[] = [ + { + job_id: 'job-1', + original_filename: 'test-mod.jar', + status: 'completed', + created_at: '2026-02-18T00:37:20.000Z', // Fixed date for consistency + file_size: 1024 * 1024, // 1MB + }, + { + job_id: 'job-2', + original_filename: 'another-mod.zip', + status: 'processing', + created_at: '2026-02-18T00:37:10.000Z', + }, + ]; + + beforeEach(() => { + vi.clearAllMocks(); + localStorageMock.clear(); + // Setup initial localStorage data + localStorageMock.setItem( + 'modporter_conversion_history', + JSON.stringify(mockHistoryItems) + ); + }); + + it('renders history items from localStorage', async () => { + render(); + + // Check items loaded + await waitFor(() => { + expect(screen.getByText('test-mod.jar')).toBeInTheDocument(); + expect(screen.getByText('another-mod.zip')).toBeInTheDocument(); + }); + + // Check status icons/text + expect(screen.getByText('Completed')).toBeInTheDocument(); + expect(screen.getByText('Processing')).toBeInTheDocument(); + }); + + it('handles item selection', async () => { + render(); + + await waitFor(() => { + expect(screen.getByText('test-mod.jar')).toBeInTheDocument(); + }); + + const checkbox = screen.getByLabelText('Select test-mod.jar'); + fireEvent.click(checkbox); + + // Check if delete selected button appears + expect(screen.getByText(/Delete Selected/i)).toBeInTheDocument(); + }); + + it('deletes an item', async () => { + render(); + + await waitFor(() => { + expect(screen.getByText('test-mod.jar')).toBeInTheDocument(); + }); + + const deleteButtons = screen.getAllByTitle('Remove from history'); + fireEvent.click(deleteButtons[0]); + + await waitFor(() => { + expect(screen.queryByText('test-mod.jar')).not.toBeInTheDocument(); + }); + + // Verify localStorage update + expect(localStorageMock.setItem).toHaveBeenCalled(); + }); +}); diff --git a/frontend/src/components/ConversionHistory/ConversionHistory.tsx b/frontend/src/components/ConversionHistory/ConversionHistory.tsx index dc3ef4a7..f9195901 100644 --- a/frontend/src/components/ConversionHistory/ConversionHistory.tsx +++ b/frontend/src/components/ConversionHistory/ConversionHistory.tsx @@ -3,23 +3,10 @@ * Shows user's previous conversions with status, download options, and management features */ -import React, { useState, useEffect } from 'react'; +import React, { useState, useEffect, useCallback, useRef } from 'react'; import './ConversionHistory.css'; - -interface ConversionHistoryItem { - job_id: string; - original_filename: string; - status: 'completed' | 'failed' | 'processing' | 'queued'; - created_at: string; - completed_at?: string; - file_size?: number; - error_message?: string; - options?: { - smartAssumptions: boolean; - includeDependencies: boolean; - modUrl?: string; - }; -} +import { ConversionHistoryItem } from './types'; +import ConversionHistoryItemRow from './ConversionHistoryItem'; interface ConversionHistoryProps { className?: string; @@ -35,22 +22,15 @@ export const ConversionHistory: React.FC = ({ const [error, setError] = useState(null); const [selectedItems, setSelectedItems] = useState>(new Set()); - const API_BASE_URL = import.meta.env.VITE_API_URL || 'http://localhost:8080/api/v1'; + // Use a ref for API_BASE_URL to avoid dependency changes, though it's constant + const apiBaseUrlRef = useRef(import.meta.env.VITE_API_URL || 'http://localhost:8080/api/v1'); - // Load conversion history from localStorage for now - // In production, this would come from the backend API - const loadHistoryCallback = React.useCallback(() => loadHistory(), [loadHistory]); - - useEffect(() => { - loadHistoryCallback(); - }, [loadHistoryCallback]); - - const loadHistory = React.useCallback(async () => { + // Load conversion history from localStorage + const loadHistory = useCallback(async () => { try { setLoading(true); // For MVP, use localStorage to store conversion history - // In production, this would be an API call to the backend const storedHistory = localStorage.getItem('modporter_conversion_history'); const parsedHistory: ConversionHistoryItem[] = storedHistory ? JSON.parse(storedHistory) : []; @@ -70,28 +50,34 @@ export const ConversionHistory: React.FC = ({ } }, [maxItems]); + useEffect(() => { + loadHistory(); + }, [loadHistory]); + // Add new conversion to history - const addToHistory = React.useCallback((item: ConversionHistoryItem) => { - const updatedHistory = [item, ...history]; - setHistory(updatedHistory.slice(0, maxItems)); - - // Save to localStorage - localStorage.setItem('modporter_conversion_history', JSON.stringify(updatedHistory)); - }, [history, maxItems]); + const addToHistory = useCallback((item: ConversionHistoryItem) => { + setHistory(prevHistory => { + const updatedHistory = [item, ...prevHistory].slice(0, maxItems); + localStorage.setItem('modporter_conversion_history', JSON.stringify(updatedHistory)); + return updatedHistory; + }); + }, [maxItems]); // Update existing conversion status - const updateConversionStatus = React.useCallback((jobId: string, updates: Partial) => { - const updatedHistory = history.map(item => - item.job_id === jobId ? { ...item, ...updates } : item - ); - setHistory(updatedHistory); - localStorage.setItem('modporter_conversion_history', JSON.stringify(updatedHistory)); - }, [history]); + const updateConversionStatus = useCallback((jobId: string, updates: Partial) => { + setHistory(prevHistory => { + const updatedHistory = prevHistory.map(item => + item.job_id === jobId ? { ...item, ...updates } : item + ); + localStorage.setItem('modporter_conversion_history', JSON.stringify(updatedHistory)); + return updatedHistory; + }); + }, []); // Download conversion result - const downloadConversion = async (jobId: string, filename: string) => { + const downloadConversion = useCallback(async (jobId: string, filename: string) => { try { - const response = await fetch(`${API_BASE_URL}/convert/${jobId}/download`); + const response = await fetch(`${apiBaseUrlRef.current}/convert/${jobId}/download`); if (!response.ok) { throw new Error('Download failed'); @@ -110,29 +96,33 @@ export const ConversionHistory: React.FC = ({ console.error('Download failed:', err); setError('Failed to download file'); } - }; + }, []); // Delete conversion from history - const deleteConversion = (jobId: string) => { - const updatedHistory = history.filter(item => item.job_id !== jobId); - setHistory(updatedHistory); - localStorage.setItem('modporter_conversion_history', JSON.stringify(updatedHistory)); + const deleteConversion = useCallback((jobId: string) => { + setHistory(prevHistory => { + const updatedHistory = prevHistory.filter(item => item.job_id !== jobId); + localStorage.setItem('modporter_conversion_history', JSON.stringify(updatedHistory)); + return updatedHistory; + }); + setSelectedItems(prev => { + if (!prev.has(jobId)) return prev; const updated = new Set(prev); updated.delete(jobId); return updated; }); - }; + }, []); // Clear all history - const clearAllHistory = () => { + const clearAllHistory = useCallback(() => { setHistory([]); setSelectedItems(new Set()); localStorage.removeItem('modporter_conversion_history'); - }; + }, []); // Toggle item selection - const toggleSelection = (jobId: string) => { + const toggleSelection = useCallback((jobId: string) => { setSelectedItems(prev => { const updated = new Set(prev); if (updated.has(jobId)) { @@ -142,50 +132,35 @@ export const ConversionHistory: React.FC = ({ } return updated; }); - }; + }, []); // Delete selected items - const deleteSelected = () => { - const updatedHistory = history.filter(item => !selectedItems.has(item.job_id)); - setHistory(updatedHistory); - localStorage.setItem('modporter_conversion_history', JSON.stringify(updatedHistory)); - setSelectedItems(new Set()); - }; - - // Format file size - const formatFileSize = (bytes?: number): string => { - if (!bytes) return 'Unknown size'; - const mb = bytes / (1024 * 1024); - return `${mb.toFixed(2)} MB`; - }; - - // Format date - const formatDate = (dateString: string): string => { - const date = new Date(dateString); - return date.toLocaleDateString() + ' ' + date.toLocaleTimeString(); - }; - - // Get status icon - const getStatusIcon = (status: string): string => { - switch (status) { - case 'completed': return '✅'; - case 'failed': return '❌'; - case 'processing': return '⏳'; - case 'queued': return '⏸️'; - default: return '❓'; - } - }; + const deleteSelected = useCallback(() => { + // We need the current selected items set to filter history + // But we can't access state inside setHistory callback cleanly if we want to depend only on stable refs. + // However, deleteSelected depends on selectedItems state anyway, so it will change when selection changes. + // That is acceptable as it's attached to the "Delete Selected" button, NOT passed to individual rows. + + // Wait, deleteSelected is NOT passed to rows. So it doesn't need to be perfectly stable for rows. + // Only toggleSelection, deleteConversion, downloadConversion need to be stable for rows. + + setHistory(currentHistory => { + // Need access to current selectedItems. + // Since we are inside a callback that depends on selectedItems, we can use it. + // But wait, the callback below needs selectedItems from closure. + return currentHistory.filter(item => !selectedItems.has(item.job_id)); + }); - // Get status color - const getStatusColor = (status: string): string => { - switch (status) { - case 'completed': return '#4caf50'; - case 'failed': return '#f44336'; - case 'processing': return '#ff9800'; - case 'queued': return '#2196f3'; - default: return '#9e9e9e'; - } - }; + // We also need to update localStorage with the result. + // The setHistory updater function is pure-ish. + // Better: + const newHistory = history.filter(item => !selectedItems.has(item.job_id)); + setHistory(newHistory); + localStorage.setItem('modporter_conversion_history', JSON.stringify(newHistory)); + setSelectedItems(new Set()); + }, [history, selectedItems]); + // deleteSelected depends on history and selectedItems. + // It's attached to the header button, so it's fine if it re-renders the header button. // Expose methods for parent components React.useImperativeHandle(React.useRef(), () => ({ @@ -248,84 +223,14 @@ export const ConversionHistory: React.FC = ({ ) : (
{history.map((item) => ( -
-
- toggleSelection(item.job_id)} - aria-label={`Select ${item.original_filename}`} - /> -
- -
- - {getStatusIcon(item.status)} - -
- -
-
-
- {item.original_filename} - #{item.job_id.slice(0, 8)} -
- -
- - {item.status.charAt(0).toUpperCase() + item.status.slice(1)} - - {formatDate(item.created_at)} - {item.file_size && ( - {formatFileSize(item.file_size)} - )} -
- - {item.options && ( -
- {item.options.smartAssumptions && ( - 🧠 Smart Assumptions - )} - {item.options.includeDependencies && ( - 📦 Dependencies - )} - {item.options.modUrl && ( - 🔗 URL Source - )} -
- )} - - {item.error_message && ( -
- ⚠️ {item.error_message} -
- )} -
-
- -
- {item.status === 'completed' && ( - - )} - - -
-
+ ))}
)} @@ -371,4 +276,4 @@ export const useConversionHistory = () => { }; }; -export default ConversionHistory; \ No newline at end of file +export default ConversionHistory; diff --git a/frontend/src/components/ConversionHistory/ConversionHistoryItem.tsx b/frontend/src/components/ConversionHistory/ConversionHistoryItem.tsx new file mode 100644 index 00000000..bf10aa38 --- /dev/null +++ b/frontend/src/components/ConversionHistory/ConversionHistoryItem.tsx @@ -0,0 +1,104 @@ +import React, { memo } from 'react'; +import { ConversionHistoryItem as IConversionHistoryItem } from './types'; +import { formatDate, formatFileSize, getStatusColor, getStatusIcon } from './utils'; +import './ConversionHistory.css'; + +interface ConversionHistoryItemProps { + item: IConversionHistoryItem; + isSelected: boolean; + onToggle: (jobId: string) => void; + onDelete: (jobId: string) => void; + onDownload: (jobId: string, filename: string) => void; +} + +export const ConversionHistoryItem = memo(({ + item, + isSelected, + onToggle, + onDelete, + onDownload +}: ConversionHistoryItemProps) => { + return ( +
+
+ onToggle(item.job_id)} + aria-label={`Select ${item.original_filename}`} + /> +
+ +
+ + {getStatusIcon(item.status)} + +
+ +
+
+
+ {item.original_filename} + #{item.job_id.slice(0, 8)} +
+ +
+ + {item.status.charAt(0).toUpperCase() + item.status.slice(1)} + + {formatDate(item.created_at)} + {item.file_size && ( + {formatFileSize(item.file_size)} + )} +
+ + {item.options && ( +
+ {item.options.smartAssumptions && ( + 🧠 Smart Assumptions + )} + {item.options.includeDependencies && ( + 📦 Dependencies + )} + {item.options.modUrl && ( + 🔗 URL Source + )} +
+ )} + + {item.error_message && ( +
+ ⚠️ {item.error_message} +
+ )} +
+
+ +
+ {item.status === 'completed' && ( + + )} + + +
+
+ ); +}); + +ConversionHistoryItem.displayName = 'ConversionHistoryItem'; + +export default ConversionHistoryItem; diff --git a/frontend/src/components/ConversionHistory/types.ts b/frontend/src/components/ConversionHistory/types.ts new file mode 100644 index 00000000..1b96a848 --- /dev/null +++ b/frontend/src/components/ConversionHistory/types.ts @@ -0,0 +1,14 @@ +export interface ConversionHistoryItem { + job_id: string; + original_filename: string; + status: 'completed' | 'failed' | 'processing' | 'queued'; + created_at: string; + completed_at?: string; + file_size?: number; + error_message?: string; + options?: { + smartAssumptions: boolean; + includeDependencies: boolean; + modUrl?: string; + }; +} diff --git a/frontend/src/components/ConversionHistory/utils.ts b/frontend/src/components/ConversionHistory/utils.ts new file mode 100644 index 00000000..3256241b --- /dev/null +++ b/frontend/src/components/ConversionHistory/utils.ts @@ -0,0 +1,34 @@ +// Format file size +export const formatFileSize = (bytes?: number): string => { + if (!bytes) return 'Unknown size'; + const mb = bytes / (1024 * 1024); + return `${mb.toFixed(2)} MB`; +}; + +// Format date +export const formatDate = (dateString: string): string => { + const date = new Date(dateString); + return date.toLocaleDateString() + ' ' + date.toLocaleTimeString(); +}; + +// Get status icon +export const getStatusIcon = (status: string): string => { + switch (status) { + case 'completed': return '✅'; + case 'failed': return '❌'; + case 'processing': return '⏳'; + case 'queued': return '⏸️'; + default: return '❓'; + } +}; + +// Get status color +export const getStatusColor = (status: string): string => { + switch (status) { + case 'completed': return '#4caf50'; + case 'failed': return '#f44336'; + case 'processing': return '#ff9800'; + case 'queued': return '#2196f3'; + default: return '#9e9e9e'; + } +};