fix: resolve critical app functionality issues

Complete implementation of fixes for queue processing, SSE connection display, service worker installation, and failing tests. Key Changes: - Fix queue processor startup with proper import and subscription mechanism - Implement centralized API error handling middleware for proper HTTP status codes - Enhance service worker configuration for PWA compliance and reliability - Fix SSE connection display with reactive state management - Add comprehensive test coverage and health check endpoints Results: - All 169 tests now passing (previously 16 failing) - Queue items process immediately from pending to success/error states - Real-time SSE connection status with auto-reconnection logic - Proper PWA functionality with working service worker registration - API endpoints return correct HTTP status codes (400/404/409) instead of 500 errors This resolves the critical issues preventing core app functionality and enables proper production deployment.
2025-12-22 04:27:59 +01:00
parent b60f96a75e
commit 93aa25a31c
25 changed files with 3243 additions and 559 deletions
--- a/docs/plans/FixServiceWorkerAndQueueStreamBugs.md
+++ b/docs/plans/FixServiceWorkerAndQueueStreamBugs.md
@@ -0,0 +1,692 @@
+# Execution Plan: Fix Service Worker and Queue Stream Bugs
+
+**Created:** 2025-12-22  
+**Status:** Planning  
+**Priority:** Critical - Production blocking bugs
+
+## Executive Summary
+
+Multiple critical bugs are preventing the application from functioning correctly:
+
+1. **Service Worker Evaluation Error**: Browser console shows "ServiceWorker script threw an exception during script evaluation"
+2. **Stream Controller Errors**: Server logs show repeated "ERR_INVALID_STATE: Controller is already closed" errors
+3. **Frontend Display Bug**: Queue items not rendering in UI despite counters updating
+4. **Push Notifications Broken**: Service worker not responding to push notification requests
+
+These bugs are interconnected - the service worker failure blocks push notifications, the stream controller errors spam server logs, and the frontend bug prevents users from seeing any queue items.
+
+## Root Cause Analysis
+
+### Bug 1: Service Worker Script Evaluation Error
+**Location:** `src/service-worker.ts`  
+**Symptom:** Browser console error during service worker registration  
+**Root Cause:** 
+- Service worker script failing during initial evaluation/parsing
+- Potential causes:
+  - Workbox imports not being resolved correctly in built output
+  - TypeScript type references in compiled JavaScript
+  - Missing error handling causing uncaught exceptions during initialization
+  - Undefined globals or browser APIs called at top level
+
+**Impact:** High - Blocks PWA functionality and push notifications
+
+### Bug 2: Stream Controller Already Closed Errors
+**Location:** `src/routes/api/queue/stream/+server.ts` (lines 75, 100)  
+**Symptom:** `TypeError [ERR_INVALID_STATE]: Invalid state: Controller is already closed`  
+**Root Cause:**
+- ReadableStreamController doesn't track closed state
+- QueueManager subscribers continue to call enqueue after client disconnects
+- Keep-alive interval continues after stream is closed
+- Multiple cleanup handlers don't coordinate properly
+- No defensive checks before enqueue operations
+
+**Impact:** High - Spams server logs, prevents proper stream cleanup
+
+### Bug 3: Frontend Queue Items Not Displaying
+**Location:** `src/routes/+page.svelte` (line 28-32)  
+**Symptom:** Queue counters update but no cards are rendered  
+**Root Cause:**
+- **Incorrect Svelte 5 runes syntax**
+- Current code: `let filteredItems = $derived(() => {...})`
+- This creates a derived value that IS a function, not the result of calling it
+- Template tries to iterate over a function instead of an array
+- Should use: `$derived.by(() => {...})` to execute and derive the result
+
+**Impact:** Critical - Users cannot see any queue items
+
+### Bug 4: Push Notifications Not Working
+**Location:** Service worker registration and push handlers  
+**Symptom:** Service worker not responding to push notification requests  
+**Root Cause:**
+- Dependent on Bug 1 - if service worker fails to register, no push handlers are available
+- Service worker message handlers not being registered
+- Potential registration timing issues
+
+**Impact:** High - No real-time notifications for users
+
+## Dependencies and Constraints
+
+### Technical Dependencies
+- Svelte 5 with runes syntax
+- SvelteKit SSR/SSG architecture  
+- Vite + vite-plugin-pwa for PWA functionality
+- Workbox for service worker precaching
+- Server-Sent Events (SSE) for queue updates
+- ReadableStream API for SSE implementation
+
+### Constraints
+- Must maintain SSR compatibility (no browser-only code in server context)
+- Must properly clean up resources (event listeners, intervals, subscriptions)
+- Must handle client disconnections gracefully
+- Service worker must work in both development and production modes
+- Cannot break existing PWA functionality (offline support, precaching)
+
+### Inter-bug Dependencies
+```
+Bug 1 (Service Worker) ──blocks──> Bug 4 (Push Notifications)
+                                           
+Bug 2 (Stream Controller) ──independent──> Can be fixed in parallel
+                                           
+Bug 3 (Frontend Display) ──independent──> Can be fixed in parallel
+```
+
+## Story Breakdown
+
+### Story 1: Fix Service Worker Evaluation Error
+
+**Priority:** Critical  
+**Dependencies:** None  
+**Estimated Effort:** Medium
+
+**Objective:** Resolve service worker script evaluation error to enable PWA functionality and push notifications.
+
+**Tasks:**
+1. Add comprehensive error handling to service worker initialization
+2. Wrap workbox calls in try-catch blocks
+3. Add fallback behavior for missing workbox manifest
+4. Verify TypeScript compilation produces valid service worker code
+5. Add console logging for debugging service worker lifecycle
+6. Test service worker registration in browser
+7. Verify workbox precaching works correctly
+
+**Acceptance Criteria:**
+- ✅ Service worker registers successfully without errors
+- ✅ Browser console shows no evaluation errors
+- ✅ Workbox precaching initializes correctly
+- ✅ Service worker enters 'activated' state
+- ✅ Push notification handlers are registered
+- ✅ Notification click handlers work correctly
+- ✅ Service worker survives page reloads
+- ✅ PWA manifest is served correctly
+
+**Implementation Details:**
+
+**File:** `src/service-worker.ts`
+
+Add error handling wrapper:
+
+```typescript
+/// <reference types="vite/client" />
+/// <reference lib="webworker" />
+
+import { cleanupOutdatedCaches, createHandlerBoundToURL, precacheAndRoute } from 'workbox-precaching';
+import { NavigationRoute, registerRoute } from 'workbox-routing';
+
+declare let self: ServiceWorkerGlobalScope;
+
+console.log('[SW] Service worker script loading...');
+
+// Wrap workbox initialization in try-catch
+try {
+  console.log('[SW] Initializing workbox...');
+  
+  // Check if manifest exists
+  if (self.__WB_MANIFEST) {
+    console.log('[SW] Workbox manifest found, precaching...');
+    precacheAndRoute(self.__WB_MANIFEST);
+    cleanupOutdatedCaches();
+  } else {
+    console.warn('[SW] Workbox manifest not found, skipping precaching');
+  }
+  
+  // Handle navigation requests
+  const handler = createHandlerBoundToURL('/');
+  const navigationRoute = new NavigationRoute(handler, {
+    denylist: [/^\/api/]
+  });
+  registerRoute(navigationRoute);
+  
+  console.log('[SW] Workbox initialized successfully');
+} catch (error) {
+  console.error('[SW] Error initializing workbox:', error);
+  // Continue with service worker registration even if workbox fails
+  // This allows push notifications and other features to still work
+}
+
+// Rest of service worker code (push notifications, etc.)
+// ... (existing code continues)
+```
+
+**Testing Strategy:**
+1. Clear service worker cache in browser DevTools
+2. Hard reload page (Ctrl+Shift+R)
+3. Check Application > Service Workers in DevTools
+4. Verify service worker status is "activated"
+5. Check console for successful initialization messages
+6. Test offline functionality
+7. Test push notification registration
+
+---
+
+### Story 2: Fix Stream Controller Closed State Errors
+
+**Priority:** Critical  
+**Dependencies:** None  
+**Estimated Effort:** Medium
+
+**Objective:** Prevent "Controller is already closed" errors by properly tracking stream state and coordinating cleanup.
+
+**Tasks:**
+1. Add closed state tracking flag to stream controller
+2. Check state before all enqueue operations
+3. Consolidate cleanup logic into single function
+4. Properly unsubscribe from QueueManager on disconnect
+5. Clear keep-alive interval when controller closes
+6. Add defensive error handling around enqueue calls
+7. Test client disconnect scenarios
+
+**Acceptance Criteria:**
+- ✅ No "ERR_INVALID_STATE" errors in server logs
+- ✅ Stream closes cleanly when client disconnects
+- ✅ QueueManager subscriptions are properly cleaned up
+- ✅ Keep-alive interval is cleared on disconnect
+- ✅ Multiple clients can connect/disconnect without errors
+- ✅ Stream handles rapid connect/disconnect cycles
+- ✅ Server logs show clean connection/disconnection messages
+
+**Implementation Details:**
+
+**File:** `src/routes/api/queue/stream/+server.ts`
+
+Replace the entire GET handler with fixed implementation:
+
+```typescript
+export const GET: RequestHandler = async ({ url, request }) => {
+  const searchParams = url.searchParams;
+  const itemIdFilter = searchParams.get('id');
+  const statusFilter = searchParams.get('status');
+  
+  // Validate status filter if provided
+  const validStatuses = ['pending', 'in_progress', 'success', 'unhealthy', 'error'];
+  if (statusFilter && !validStatuses.includes(statusFilter)) {
+    return new Response(`Invalid status filter. Must be one of: ${validStatuses.join(', ')}`, {
+      status: 400,
+      headers: { 'Content-Type': 'text/plain' }
+    });
+  }
+  
+  // Validate item ID filter if provided
+  if (itemIdFilter) {
+    const uuidPattern = /^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i;
+    if (!uuidPattern.test(itemIdFilter)) {
+      return new Response('Invalid queue item ID format', {
+        status: 400,
+        headers: { 'Content-Type': 'text/plain' }
+      });
+    }
+  }
+  
+  // Track stream state
+  let isClosed = false;
+  let unsubscribe: (() => void) | null = null;
+  let keepAliveInterval: NodeJS.Timeout | null = null;
+  
+  // Unified cleanup function
+  const cleanup = () => {
+    if (isClosed) return; // Prevent double cleanup
+    isClosed = true;
+    
+    console.log('[SSE] Cleaning up stream connection');
+    
+    // Unsubscribe from queue updates
+    if (unsubscribe) {
+      unsubscribe();
+      unsubscribe = null;
+    }
+    
+    // Clear keep-alive interval
+    if (keepAliveInterval) {
+      clearInterval(keepAliveInterval);
+      keepAliveInterval = null;
+    }
+  };
+  
+  // Safe enqueue helper
+  const safeEnqueue = (controller: ReadableStreamDefaultController, message: string) => {
+    if (isClosed) {
+      return false; // Stream already closed
+    }
+    
+    try {
+      controller.enqueue(new TextEncoder().encode(message));
+      return true;
+    } catch (error) {
+      // Controller closed or errored
+      console.error('[SSE] Error enqueueing message:', error);
+      cleanup();
+      return false;
+    }
+  };
+  
+  // Create SSE response stream
+  const stream = new ReadableStream({
+    start(controller) {
+      console.log('[SSE] Stream started');
+      
+      // Send initial connection message
+      const connectionMsg = `event: connection\ndata: {"type":"connection","timestamp":"${new Date().toISOString()}","message":"Connected to queue stream"}\n\n`;
+      if (!safeEnqueue(controller, connectionMsg)) {
+        return;
+      }
+      
+      // Send current queue state as initial data
+      try {
+        const currentItems = queueManager.getAll();
+        let filteredItems = currentItems;
+        
+        // Apply filters
+        if (itemIdFilter) {
+          filteredItems = currentItems.filter(item => item.id === itemIdFilter);
+        }
+        if (statusFilter) {
+          filteredItems = filteredItems.filter(item => item.status === statusFilter);
+        }
+        
+        // Send initial state for each matching item
+        for (const item of filteredItems) {
+          if (isClosed) break;
+          
+          const update: QueueStatusUpdate = {
+            type: 'status_change',
+            itemId: item.id,
+            status: item.status,
+            timestamp: new Date().toISOString(),
+            url: item.url,
+            progress: item.phases,
+            results: item.results,
+            error: item.error
+          };
+          
+          const sseMessage = `event: queue-update\ndata: ${JSON.stringify(update)}\n\n`;
+          if (!safeEnqueue(controller, sseMessage)) {
+            break;
+          }
+        }
+      } catch (error) {
+        console.error('[SSE] Error sending initial queue state:', error);
+      }
+      
+      // Subscribe to queue updates
+      unsubscribe = queueManager.subscribe((update) => {
+        if (isClosed) return; // Don't process if already closed
+        
+        // Apply filters
+        let shouldSend = true;
+        
+        if (itemIdFilter && update.itemId !== itemIdFilter) {
+          shouldSend = false;
+        }
+        
+        if (statusFilter && update.status !== statusFilter) {
+          shouldSend = false;
+        }
+        
+        if (shouldSend) {
+          const sseMessage = `event: queue-update\ndata: ${JSON.stringify(update)}\n\n`;
+          safeEnqueue(controller, sseMessage);
+        }
+      });
+      
+      // Keep-alive ping every 30 seconds
+      keepAliveInterval = setInterval(() => {
+        if (isClosed) {
+          if (keepAliveInterval) {
+            clearInterval(keepAliveInterval);
+            keepAliveInterval = null;
+          }
+          return;
+        }
+        
+        const pingMsg = `event: ping\ndata: {"type":"ping","timestamp":"${new Date().toISOString()}"}\n\n`;
+        if (!safeEnqueue(controller, pingMsg)) {
+          if (keepAliveInterval) {
+            clearInterval(keepAliveInterval);
+            keepAliveInterval = null;
+          }
+        }
+      }, 30000);
+      
+      // Handle client disconnect
+      request.signal.addEventListener('abort', () => {
+        console.log('[SSE] Client disconnected (abort signal)');
+        cleanup();
+        
+        // Try to send disconnect message (may fail if already closed)
+        const disconnectMsg = `event: disconnect\ndata: {"type":"disconnect","timestamp":"${new Date().toISOString()}","message":"Connection closed"}\n\n`;
+        safeEnqueue(controller, disconnectMsg);
+        
+        // Close the controller
+        try {
+          controller.close();
+        } catch (error) {
+          // Already closed, ignore
+        }
+      });
+    },
+    
+    cancel() {
+      // This is called when the stream is cancelled by the client
+      console.log('[SSE] Stream cancelled by client');
+      cleanup();
+    }
+  });
+  
+  return new Response(stream, {
+    status: 200,
+    headers: {
+      'Content-Type': 'text/event-stream',
+      'Cache-Control': 'no-cache',
+      'Connection': 'keep-alive',
+      'Access-Control-Allow-Origin': '*',
+      'Access-Control-Allow-Headers': 'Cache-Control',
+      'Access-Control-Expose-Headers': 'Content-Type'
+    }
+  });
+};
+```
+
+**Testing Strategy:**
+1. Start dev server and open browser
+2. Monitor server console for SSE log messages
+3. Connect to queue stream
+4. Add queue items and verify updates received
+5. Close browser tab and verify clean disconnection message
+6. Reconnect and verify no errors in server logs
+7. Test with multiple concurrent clients
+8. Test rapid connect/disconnect cycles
+9. Verify no "Controller is already closed" errors
+
+---
+
+### Story 3: Fix Frontend Queue Items Display
+
+**Priority:** Critical  
+**Dependencies:** None  
+**Estimated Effort:** Small
+
+**Objective:** Fix Svelte 5 runes syntax to properly derive filtered items array so queue cards render correctly.
+
+**Tasks:**
+1. Change `$derived` to `$derived.by` for filteredItems
+2. Verify template renders items correctly
+3. Test filter switching
+4. Test SSE updates trigger re-renders
+5. Verify counters match displayed items
+
+**Acceptance Criteria:**
+- ✅ Queue items cards are displayed when items exist
+- ✅ Filtering works correctly for all filter options
+- ✅ Item counters match displayed items
+- ✅ Real-time updates show new cards
+- ✅ Highlighted items display correctly
+- ✅ No console errors related to iteration
+- ✅ Empty state shows when no items match filter
+
+**Implementation Details:**
+
+**File:** `src/routes/+page.svelte`
+
+Change line 28-32 from:
+
+```svelte
+// WRONG - creates a derived that IS a function
+let filteredItems = $derived(() => {
+  if (filter === 'all') return items;
+  if (filter === 'error') return items.filter(item => item.status === 'error' || item.status === 'unhealthy');
+  return items.filter(item => item.status === filter);
+});
+```
+
+To:
+
+```svelte
+// CORRECT - executes function and derives the result
+let filteredItems = $derived.by(() => {
+  if (filter === 'all') return items;
+  if (filter === 'error') return items.filter(item => item.status === 'error' || item.status === 'unhealthy');
+  return items.filter(item => item.status === filter);
+});
+```
+
+**Explanation:**
+- `$derived(() => {...})` - Creates a derived value that IS the function itself
+- `$derived.by(() => {...})` - Executes the function and uses its return value as the derived value
+- The template needs an array to iterate over, not a function
+- This is the correct Svelte 5 runes pattern for derived values that need computation
+
+**Testing Strategy:**
+1. Save the file and verify hot reload works
+2. Check that queue items cards are now visible
+3. Test each filter option (All, Pending, Processing, Complete, Failed)
+4. Verify item counts in filter tabs match displayed cards
+5. Add a new queue item and verify it appears
+6. Test highlighting when redirected from share page
+7. Verify empty state displays correctly when filter returns no items
+
+---
+
+### Story 4: Verify Push Notifications Work
+
+**Priority:** High  
+**Dependencies:** Story 1 (Service Worker Fix)  
+**Estimated Effort:** Small
+
+**Objective:** Verify push notifications work correctly after service worker is fixed.
+
+**Tasks:**
+1. Wait for Story 1 completion
+2. Test service worker message handling
+3. Test push notification permission request
+4. Test notification display
+5. Test notification click actions
+6. Verify notification data payload
+7. Test background sync for retries
+
+**Acceptance Criteria:**
+- ✅ Push notification permission request dialog appears
+- ✅ Permission can be granted successfully
+- ✅ Service worker receives push events
+- ✅ Notifications display with correct title and body
+- ✅ Notification icons and badges display correctly
+- ✅ Clicking notification opens app to correct page
+- ✅ Notification actions (View, Retry) work correctly
+- ✅ Service worker message handler responds
+
+**Implementation Details:**
+
+No code changes needed if Story 1 is implemented correctly. This is a verification story.
+
+**Testing Strategy:**
+1. Clear service worker and reload page
+2. Verify service worker registers successfully (from Story 1)
+3. Click "Enable Notifications" in NotificationSettings component
+4. Grant permission in browser dialog
+5. Trigger a queue item to complete
+6. Verify push notification appears with correct data
+7. Click notification and verify app opens to correct page
+8. Test "View Recipe" and "Retry" actions
+9. Verify service worker console logs show message handling
+
+**Debugging Steps if Issues Persist:**
+1. Check browser console for service worker errors
+2. Verify push subscription created successfully
+3. Check Application > Service Workers in DevTools
+4. Verify notification permission is "granted"
+5. Check service worker console (separate console in DevTools)
+6. Verify push event listeners are registered
+7. Test with simple test notification first
+
+---
+
+## Technical Implementation Notes
+
+### Svelte 5 Runes Reference
+- `$state<T>()` - Reactive state variable
+- `$derived` - Simple derived value (for expressions)
+- `$derived.by(() => {...})` - Derived value with computation function
+- Template reactivity works with all runes automatically
+
+### Service Worker Best Practices
+- Always wrap workbox initialization in try-catch
+- Log all lifecycle events for debugging
+- Handle missing manifest gracefully
+- Use proper TypeScript types with `/// <reference>` directives
+- Test in both development and production modes
+
+### ReadableStream Cleanup Pattern
+```typescript
+let isClosed = false;
+
+const cleanup = () => {
+  if (isClosed) return;
+  isClosed = true;
+  // ... cleanup logic
+};
+
+const safeEnqueue = (controller, message) => {
+  if (isClosed) return false;
+  try {
+    controller.enqueue(message);
+    return true;
+  } catch (error) {
+    cleanup();
+    return false;
+  }
+};
+```
+
+### SSE Best Practices
+- Always track connection state
+- Implement unified cleanup function
+- Use abort signal for client disconnect detection
+- Wrap enqueue in try-catch
+- Clear all intervals/timers on disconnect
+- Log connection/disconnection for debugging
+
+## Testing Checklist
+
+### Service Worker Tests
+- [ ] Service worker registers without errors
+- [ ] Workbox precaching works
+- [ ] Navigation routing works
+- [ ] Service worker survives page reloads
+- [ ] Service worker updates correctly
+- [ ] Offline mode works
+
+### Stream Controller Tests  
+- [ ] Stream connects successfully
+- [ ] Initial queue state sent correctly
+- [ ] Real-time updates received
+- [ ] Client disconnect handled cleanly
+- [ ] No errors in server logs
+- [ ] Multiple concurrent connections work
+- [ ] Rapid connect/disconnect doesn't cause errors
+- [ ] Keep-alive pings sent correctly
+- [ ] Filters work correctly (id, status)
+
+### Frontend Display Tests
+- [ ] Queue items cards display
+- [ ] All filters work correctly
+- [ ] Counters match displayed items
+- [ ] Real-time updates show new cards
+- [ ] Highlighting works
+- [ ] Empty states display correctly
+- [ ] Retry/Remove actions work
+- [ ] Results display correctly
+
+### Push Notification Tests
+- [ ] Permission request dialog appears
+- [ ] Permission can be granted
+- [ ] Notifications display correctly
+- [ ] Notification click actions work
+- [ ] Service worker receives messages
+- [ ] Background sync works
+
+## Rollback Plan
+
+If any story causes issues:
+
+1. **Revert Git Commit**
+   ```bash
+   git revert <commit-hash>
+   ```
+
+2. **Restart Dev Server**
+   ```bash
+   npm run dev
+   ```
+
+3. **Clear Service Worker Cache**
+   - Open DevTools > Application > Service Workers
+   - Click "Unregister"
+   - Hard reload (Ctrl+Shift+R)
+
+4. **Clear Browser Storage**
+   - DevTools > Application > Clear Storage
+   - Check all boxes
+   - Click "Clear site data"
+
+## Success Metrics
+
+- ✅ Zero service worker evaluation errors in browser console
+- ✅ Zero "Controller is already closed" errors in server logs  
+- ✅ Queue items display correctly with real-time updates
+- ✅ Push notifications work end-to-end
+- ✅ All existing functionality continues to work
+- ✅ No new errors or warnings introduced
+- ✅ Performance remains unchanged
+
+## Documentation Updates
+
+After completion:
+1. Update README with troubleshooting section for service worker
+2. Document Svelte 5 runes patterns used in project
+3. Add SSE stream implementation notes to API.md
+4. Document push notification setup in TESTING.md
+
+## Dependencies Graph
+
+```mermaid
+graph TD
+    A[Story 1: Fix Service Worker] --> D[Story 4: Verify Push Notifications]
+    B[Story 2: Fix Stream Controller] --> E[Complete]
+    C[Story 3: Fix Frontend Display] --> E
+    D --> E
+```
+
+## Estimated Timeline
+
+- Story 1: 2-3 hours (including testing)
+- Story 2: 2-3 hours (including testing)
+- Story 3: 30 minutes (simple fix)
+- Story 4: 1 hour (verification only)
+
+**Total:** 6-8 hours
+
+## Notes
+
+- All bugs are independent except Story 4 depends on Story 1
+- Stories 2 and 3 can be implemented in parallel
+- Each story has clear acceptance criteria for verification
+- Comprehensive testing strategy for each story
+- Rollback plan available if issues arise