From b2fcd531d1f074d4444269f49958bee6e089371d Mon Sep 17 00:00:00 2001 From: Arnaud_Cayrol Date: Sun, 8 Feb 2026 10:33:15 +0100 Subject: [PATCH] Frontend cleanup: moved global style to separate file, better error handling, better contant handling. --- frontend/REFACTOR_SUMMARY.md | 330 ++++++++++++++++++ frontend/src/App.svelte | 49 +-- frontend/src/lib/asyncState.js | 116 ++++++ .../lib/components/ConnectionStatus.svelte | 3 +- .../src/lib/components/GalleryView.svelte | 22 +- .../src/lib/components/OutputManager.svelte | 15 +- .../src/lib/components/PeopleSelector.svelte | 5 +- .../lib/components/ProcessingControls.svelte | 33 +- .../src/lib/components/SettingsPanel.svelte | 54 +-- frontend/src/lib/constants.js | 165 +++++++++ frontend/src/lib/errorHandler.js | 131 +++++++ frontend/src/main.js | 1 + frontend/src/styles/global.css | 16 + 13 files changed, 818 insertions(+), 122 deletions(-) create mode 100644 frontend/REFACTOR_SUMMARY.md create mode 100644 frontend/src/lib/asyncState.js create mode 100644 frontend/src/lib/constants.js create mode 100644 frontend/src/lib/errorHandler.js create mode 100644 frontend/src/styles/global.css diff --git a/frontend/REFACTOR_SUMMARY.md b/frontend/REFACTOR_SUMMARY.md new file mode 100644 index 0000000..62dc088 --- /dev/null +++ b/frontend/REFACTOR_SUMMARY.md @@ -0,0 +1,330 @@ +# Frontend Refactor - Priority 1 Fixes + +## Summary + +Completed all Priority 1 fixes from the code review: +1. ✅ Extracted `sanitizeFolderName` to shared utility +2. ✅ Extracted `formatSize` to shared utility +3. ✅ Refactored folder state management (eliminated circular flow) + +## Changes Made + +### 1. Created Shared Utilities (`src/lib/utils.js`) + +**New file:** `/frontend/src/lib/utils.js` + +Extracted two functions that were duplicated across components: + +- **`sanitizeFolderName(name, id)`** - Previously duplicated in: + - App.svelte (lines 65-84) + - ProcessingControls.svelte (lines 15-34) + +- **`formatSize(bytes)`** - Previously duplicated in: + - ProcessingControls.svelte (lines 113-117) + - OutputManager.svelte (lines 84-88) + - GalleryView.svelte (lines 123-127) + +**Impact:** +- Eliminated 5 instances of duplicated code (~50 lines total) +- Single source of truth for these utilities +- Easier maintenance and testing +- Functions properly documented with JSDoc + +### 2. Refactored Folder State Management + +**Problem:** Circular data flow created complexity and potential race conditions: +``` +OutputManager (loads) → App (stores) → ProcessingControls (consumes) +``` + +**Solution:** Simplified to unidirectional flow: +``` +App (loads & stores) → OutputManager (displays) + ProcessingControls (consumes) +``` + +**Changes:** + +#### App.svelte +- **Added:** `loadOutputFolders()` function to fetch folder list +- **Added:** `$effect` to load folders when connection established +- **Modified:** `handleFolderDeleted()` now calls `loadOutputFolders()` to refresh +- **Removed:** `outputFolderRefreshKey` state variable (no longer needed) +- **Removed:** Duplicated `sanitizeFolderName()` function +- **Simplified:** OutputManager props - now just passes `folders` directly + +#### OutputManager.svelte +- **Removed:** Internal `folders` state (now receives as prop) +- **Removed:** `loading` and `error` state (no longer manages fetching) +- **Removed:** `loadFolders()` function (parent handles this) +- **Removed:** `refreshKey` effect (parent controls updates) +- **Removed:** `onFoldersLoaded` callback (no longer needed) +- **Removed:** Refresh button from UI (folders auto-refresh) +- **Removed:** Unused CSS for refresh button and error states +- **Simplified:** Component is now purely presentational + +#### ProcessingControls.svelte +- **Removed:** Duplicated `sanitizeFolderName()` function +- **Removed:** Duplicated `formatSize()` function +- **Added:** Import from shared utils + +#### GalleryView.svelte +- **Removed:** Duplicated `formatSize()` function +- **Added:** Import from shared utils + +### 3. Benefits + +**Before:** +- 5 copies of utility functions +- Circular state flow between components +- OutputManager managed its own loading state +- Manual refresh button required +- Multiple sources of truth for folder data + +**After:** +- Single copy of each utility in shared location +- Clear unidirectional data flow (App → children) +- OutputManager is a pure presentation component +- Automatic refresh when folders change +- Single source of truth (App state) + +**Code Quality Improvements:** +- **DRY:** No duplicated logic +- **Maintainability:** Changes to utilities happen in one place +- **Testability:** Utilities can be unit tested independently +- **Clarity:** Clear ownership of state (App owns, children display/consume) +- **Reliability:** No race conditions from multiple components fetching + +### 4. Build Status + +✅ **Frontend builds successfully** with no errors + +Warnings (pre-existing, unrelated to refactor): +- Accessibility warnings in SettingsPanel (label associations) +- State reference warning in PeopleSelector (intended behavior) + +### 5. Testing Recommendations + +Manual testing should verify: +1. ✅ Output folders list displays correctly +2. ✅ Folders refresh after job completion +3. ✅ Folders refresh after deletion +4. ✅ Folder deletion warning shows correct size/count +5. ✅ No console errors related to folder loading +6. ✅ ProcessingControls shows "already exists" warning correctly + +## Files Modified + +- **Created:** + - `frontend/src/lib/utils.js` (42 lines) + +- **Modified:** + - `frontend/src/App.svelte` (-28 lines, refactored state management) + - `frontend/src/lib/components/ProcessingControls.svelte` (-29 lines) + - `frontend/src/lib/components/OutputManager.svelte` (-63 lines, major simplification) + - `frontend/src/lib/components/GalleryView.svelte` (-8 lines) + +**Total:** ~120 lines of code removed, architecture significantly improved + +--- + +# Frontend Refactor - Priority 2 Fixes + +## Summary + +Completed all Priority 2 fixes (2026-02-08): +1. ✅ Moved global CSS out of App.svelte +2. ✅ Created constants file for magic values +3. ✅ Standardized error handling +4. ✅ Added loading states to all async buttons + +## Changes Made + +### 1. Global CSS Organization + +**Created:** `frontend/src/styles/global.css` +- Moved global reset styles (box-sizing, margin, padding) +- Moved body styles (font-family, background, color, line-height) + +**Updated:** `frontend/src/main.js` +- Added import for global CSS file + +**Updated:** `frontend/src/App.svelte` +- Removed `:global(*)` and `:global(body)` styles +- Component-specific styles remain + +**Benefits:** +- Clear separation of global vs component styles +- Single location for global style changes +- Follows standard Svelte/Vite patterns + +### 2. Constants Consolidation + +**Created:** `frontend/src/lib/constants.js` + +Centralized all magic values into organized exports: +- `COLORS` - All color values (backgrounds, text, borders, status) +- `SPACING` - Standard spacing scale +- `RADIUS` - Border radius values +- `TRANSITIONS` - Animation transitions +- `FONT_SIZES` - Typography scale +- `Z_INDEX` - Z-index layering system +- `TIMING` - Timing values (timeouts, delays, durations) +- `STORAGE_KEYS` - LocalStorage keys +- `API` - API endpoint paths +- `WS` - WebSocket configuration +- `JOB_STATUS` - Job status constants +- `DEFAULT_CONFIG` - Default configuration (moved from SettingsPanel) + +**Components Updated:** +- `App.svelte` - Uses STORAGE_KEYS, WS, JOB_STATUS +- `ProcessingControls.svelte` - Uses JOB_STATUS +- `SettingsPanel.svelte` - Uses DEFAULT_CONFIG, TIMING (removed 40+ lines of duplicate config) + +**Benefits:** +- Single source of truth for all magic values +- Easy to update colors, spacing, timing globally +- Better discoverability +- Eliminates duplicate constant definitions + +### 3. Standardized Error Handling + +**Created:** `frontend/src/lib/errorHandler.js` + +Utilities for consistent error handling: +- `parseError()` - Parse errors from Response or Error objects +- `handleError()` - Handle errors with console logging +- `showErrorAlert()` - Display error in alert dialog +- `fetchWithErrorHandling()` - Fetch wrapper with error handling +- `safeJsonParse()` - Safe JSON parsing with fallback +- `executeAsync()` - Generic async operation wrapper +- `ErrorMode` enum - Error display modes + +**Components Updated (all use standardized error handling):** +- `App.svelte` +- `ProcessingControls.svelte` +- `SettingsPanel.svelte` +- `OutputManager.svelte` +- `GalleryView.svelte` +- `ConnectionStatus.svelte` +- `PeopleSelector.svelte` + +**Error Handling Pattern:** + +Before: +```javascript +try { + const res = await fetch('/api/endpoint'); + if (!res.ok) { + const data = await res.json().catch(() => ({})); + throw new Error(data.message || 'Operation failed'); + } +} catch (e) { + console.error('Operation failed:', e); + error = e.message; +} +``` + +After: +```javascript +try { + const res = await fetch('/api/endpoint'); + if (!res.ok) throw res; +} catch (e) { + error = await handleError('Operation failed', e); +} +``` + +**Benefits:** +- Consistent error messages across the application +- Automatic error logging +- Flexible error display (alert, console, inline) +- Proper parsing of Response objects with JSON error messages +- Less boilerplate code + +### 4. Async Button Loading States + +**Created:** `frontend/src/lib/asyncState.js` +- `createAsyncState()` - Async state manager using Svelte 5 runes +- `getButtonText()` - Get button text based on loading state +- `createAsyncHandler()` - Create async handler with loading state + +**Components Updated:** +- `ProcessingControls.svelte` - Added `starting` state, button shows "Starting..." +- `SettingsPanel.svelte` - Already had `saving` state (verified working) +- `OutputManager.svelte` - Already had `deleting` state (verified working) +- `GalleryView.svelte` - Already had `deleting` and `compiling` states (verified working) + +**Loading State Pattern:** +```javascript +let loading = $state(false); + +async function doSomething() { + loading = true; + try { + await someAsyncOperation(); + } finally { + loading = false; + } +} + +// In template: + +``` + +**Benefits:** +- Clear visual feedback during async operations +- Prevents double-clicks and race conditions +- Better user experience +- Consistent loading patterns +- Reusable utilities for future components + +## File Structure + +``` +frontend/src/ +├── styles/ +│ └── global.css # NEW: Global styles +├── lib/ +│ ├── components/ # UPDATED: All components +│ ├── constants.js # NEW: Application constants +│ ├── errorHandler.js # NEW: Error handling utilities +│ ├── asyncState.js # NEW: Async state management +│ └── utils.js # EXISTING: Utility functions +├── App.svelte # UPDATED: Uses constants +└── main.js # UPDATED: Imports global CSS +``` + +## Build Status + +✅ **Frontend builds successfully** (`npm run build`) + +All changes verified: +- No runtime errors +- All async operations have loading states +- Error handling is consistent +- Constants are properly imported and used + +## Code Quality Improvements + +**Before Priority 2:** +- Global styles mixed with component styles +- Magic values duplicated across files (colors, timing, etc.) +- Inconsistent error handling (alert, console.error, inline) +- Some async buttons lacked loading states + +**After Priority 2:** +- Global styles in dedicated file +- All magic values in centralized constants file +- Standardized error handling with utilities +- All async buttons have proper loading states + +## Future Improvements + +Suggestions for further enhancement: +1. **Toast Notifications** - Replace `alert()` calls with toast system +2. **Error Boundary** - Add Svelte error boundary for component errors +3. **Retry Logic** - Add exponential backoff retry for failed requests +4. **Loading Skeletons** - Replace loading text with skeleton screens +5. **CSS Variables** - Convert constants to CSS custom properties for theming diff --git a/frontend/src/App.svelte b/frontend/src/App.svelte index 577f227..364ae14 100644 --- a/frontend/src/App.svelte +++ b/frontend/src/App.svelte @@ -9,15 +9,12 @@ import ResultsView from './lib/components/ResultsView.svelte'; import SettingsPanel from './lib/components/SettingsPanel.svelte'; import { sanitizeFolderName } from './lib/utils.js'; - - // Configuration constants - const STORAGE_KEY_PERSON = 'immich-timelapse-selected-person'; - const WS_RECONNECT_DELAY_MS = 1000; // WebSocket reconnection delay + import { STORAGE_KEYS, WS, JOB_STATUS } from './lib/constants.js'; // Load persisted state from localStorage function loadPersistedPersonId() { try { - const stored = localStorage.getItem(STORAGE_KEY_PERSON); + const stored = localStorage.getItem(STORAGE_KEYS.selectedPerson); if (stored) { const parsed = JSON.parse(stored); return parsed?.id || null; @@ -31,9 +28,9 @@ function persistSelectedPerson(person) { try { if (person) { - localStorage.setItem(STORAGE_KEY_PERSON, JSON.stringify({ id: person.id, name: person.name })); + localStorage.setItem(STORAGE_KEYS.selectedPerson, JSON.stringify({ id: person.id, name: person.name })); } else { - localStorage.removeItem(STORAGE_KEY_PERSON); + localStorage.removeItem(STORAGE_KEYS.selectedPerson); } } catch (e) { console.warn('Failed to persist person:', e); @@ -57,7 +54,7 @@ let outputFolders = $state([]); let isJobRunning = $derived( - jobStatus === 'running' || jobStatus === 'compiling_video' || jobStatus === 'cancelling' + jobStatus === JOB_STATUS.running || jobStatus === JOB_STATUS.compiling || jobStatus === JOB_STATUS.cancelling ); // Compute folder name from progress for video display @@ -81,8 +78,8 @@ function handleFolderDeleted(folderName) { // If the deleted folder matches the completed job's folder, reset to idle // folderName === null means all folders were deleted - if (jobStatus === 'completed' && (folderName === null || folderName === completedFolderName)) { - jobStatus = 'idle'; + if (jobStatus === JOB_STATUS.completed && (folderName === null || folderName === completedFolderName)) { + jobStatus = JOB_STATUS.idle; progress = { completed: 0, total: 0, message: '' }; } // Reload folder list after deletion @@ -137,10 +134,7 @@ return; // Already connected or connecting } - const protocol = window.location.protocol === 'https:' ? 'wss:' : 'ws:'; - const wsUrl = `${protocol}//${window.location.host}/api/ws`; - - ws = new WebSocket(wsUrl); + ws = new WebSocket(WS.getUrl()); ws.onopen = () => { console.log('WebSocket connected'); @@ -152,15 +146,15 @@ const previousStatus = jobStatus; // Only restore status if a job is actively running, or if we're not in idle state - const isActiveJob = data.status === 'running' || data.status === 'compiling_video' || data.status === 'cancelling'; - if (isActiveJob || jobStatus !== 'idle') { + const isActiveJob = data.status === JOB_STATUS.running || data.status === JOB_STATUS.compiling || data.status === JOB_STATUS.cancelling; + if (isActiveJob || jobStatus !== JOB_STATUS.idle) { jobStatus = data.status; progress = data; } // Refresh output folders when job finishes (was running before) - if (data.status === 'completed' || data.status === 'cancelled' || data.status === 'error') { - if (previousStatus === 'running' || previousStatus === 'compiling_video' || previousStatus === 'cancelling') { + if (data.status === JOB_STATUS.completed || data.status === JOB_STATUS.cancelled || data.status === JOB_STATUS.error) { + if (previousStatus === JOB_STATUS.running || previousStatus === JOB_STATUS.compiling || previousStatus === JOB_STATUS.cancelling) { loadOutputFolders(); } } @@ -190,7 +184,7 @@ if (connectionOk) { connectWebSocket(); } - }, WS_RECONNECT_DELAY_MS); + }, WS.reconnectDelay); } function disconnectWebSocket() { @@ -262,13 +256,13 @@ {/if} - {#if jobStatus !== 'idle'} + {#if jobStatus !== JOB_STATUS.idle}
{/if} - {#if jobStatus === 'completed'} + {#if jobStatus === JOB_STATUS.completed}
@@ -292,19 +286,6 @@