⚡ Bolt: Optimize ConversionHistory rendering#525
Conversation
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
There was a problem hiding this comment.
Pull request overview
This PR optimizes the ConversionHistory component's rendering performance by extracting list items into a memoized component and stabilizing event handlers. The changes separate concerns into multiple files (types, utils, item component) and use React's memo and useCallback hooks to prevent unnecessary re-renders when the history list updates.
Changes:
- Extracted
ConversionHistoryIteminto a separate memoized component with stable event handler props - Moved type definitions to a dedicated
types.tsfile - Refactored utility functions (formatters, status helpers) into
utils.ts - Stabilized all event handlers using
useCallbackwith proper dependency arrays - Added unit tests for basic rendering, selection, and deletion functionality
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/components/ConversionHistory/types.ts | New file: Type definitions for ConversionHistoryItem interface moved from main component |
| frontend/src/components/ConversionHistory/utils.ts | New file: Utility functions for formatting and status display extracted for reusability |
| frontend/src/components/ConversionHistory/ConversionHistoryItem.tsx | New file: Memoized list item component to prevent unnecessary re-renders |
| frontend/src/components/ConversionHistory/ConversionHistory.tsx | Refactored to use extracted components, stable callbacks with useCallback, and functional state updates |
| frontend/src/components/ConversionHistory/ConversionHistory.test.tsx | New file: Basic unit tests covering rendering, selection, and deletion |
Comments suppressed due to low confidence (1)
frontend/src/components/ConversionHistory/ConversionHistory.tsx:170
- The useImperativeHandle is called with React.useRef() instead of a forwarded ref. This creates a new ref on every render that is never used, making useImperativeHandle completely ineffective. Either remove this code if imperative handle is not needed, or properly implement it by using forwardRef and accepting a ref prop.
React.useImperativeHandle(React.useRef(), () => ({
addToHistory,
updateConversionStatus,
loadHistory
}), [addToHistory, updateConversionStatus, loadHistory]);
| 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', |
There was a problem hiding this comment.
Test data uses a future date (2026-02-18). While the current date is February 18, 2026, using hardcoded future dates in tests can cause confusion and make tests less maintainable. Consider using relative dates (e.g., new Date().toISOString()) or dates clearly in the past to avoid confusion when reviewing code in the future.
| 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', | |
| created_at: '2020-01-01T00:37:20.000Z', // Fixed past date for consistency | |
| file_size: 1024 * 1024, // 1MB | |
| }, | |
| { | |
| job_id: 'job-2', | |
| original_filename: 'another-mod.zip', | |
| status: 'processing', | |
| created_at: '2020-01-01T00:37:10.000Z', |
| 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(<ConversionHistory />); | ||
|
|
||
| // 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(<ConversionHistory />); | ||
|
|
||
| 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(<ConversionHistory />); | ||
|
|
||
| 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(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Test coverage is incomplete for the performance optimization changes. Missing tests for: 1) Verifying that ConversionHistoryItem doesn't re-render when other items change (the core optimization), 2) Testing the clearAllHistory function, 3) Testing the deleteSelected function (bulk delete), 4) Testing the download functionality, 5) Testing error states. Add tests that verify the memoization is working correctly by checking render counts.
|
|
||
| // Format date | ||
| export const formatDate = (dateString: string): string => { | ||
| const date = new Date(dateString); |
There was a problem hiding this comment.
The formatDate function doesn't validate if the dateString is valid before creating a Date object. If an invalid date string is passed, it will return "Invalid Date Invalid Date". Add error handling to return a fallback string when the date is invalid, e.g., if (isNaN(date.getTime())) return 'Invalid date';
| const date = new Date(dateString); | |
| const date = new Date(dateString); | |
| if (isNaN(date.getTime())) return 'Invalid date'; |
| }; | ||
| } | ||
| import { ConversionHistoryItem } from './types'; | ||
| import ConversionHistoryItemRow from './ConversionHistoryItem'; |
There was a problem hiding this comment.
Component naming inconsistency: The component is exported as "ConversionHistoryItem" from ConversionHistoryItem.tsx but imported as "ConversionHistoryItemRow" here. For clarity, either rename the import to match the export name, or export it as "ConversionHistoryItemRow" if that better describes its purpose. Consistent naming makes the codebase easier to navigate.
| @@ -0,0 +1,34 @@ | |||
| // Format file size | |||
| export const formatFileSize = (bytes?: number): string => { | |||
| if (!bytes) return 'Unknown size'; | |||
There was a problem hiding this comment.
The formatFileSize function returns 'Unknown size' when bytes is 0, but 0 bytes is a valid file size that should be displayed as "0.00 MB". Change the condition from if (!bytes) to if (bytes === undefined || bytes === null) to handle zero bytes correctly.
| if (!bytes) return 'Unknown size'; | |
| if (bytes === undefined || bytes === null) return 'Unknown size'; |
| 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()); |
There was a problem hiding this comment.
The deleteSelected function calls setHistory twice (lines 147-152 and line 158), which will cause a race condition and incorrect state updates. The second call at line 158 uses the stale history state from the closure instead of the updated state. Remove lines 147-152 and keep only the logic at lines 157-160, or remove lines 157-160 if you prefer the functional update pattern. The functional update pattern (lines 147-152) is more correct but needs to be completed with localStorage update inside the updater or in a useEffect.
| // 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. |
There was a problem hiding this comment.
Remove these commented developer notes from the production code. These thought-process comments (lines 139-146 and 148-151, 154-156, 162-163) should be removed before merging as they clutter the codebase and don't provide value to future maintainers.
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>
e849b37 to
baa9118
Compare
Extracted
ConversionHistoryItemto a memoized component and stabilized event handlers to prevent unnecessary re-renders in the conversion history list. Added unit tests.PR created automatically by Jules for task 1704459719646090573 started by @anchapin