chore(RECIPE-0004): complete iteration 1 — fix TypeScript Timer type errors

- Fixed NodeJS.Timer → NodeJS.Timeout in scheduler.ts line 13 - Fixed NodeJS.Timer[] → NodeJS.Timeout[] in fixtures.ts line 151 - Resolves TypeScript compile errors from iteration 0 review - All 260 tests passing, build succeeds with no errors
2026-02-17 03:08:21 +01:00
parent e749763911
commit 67ab3c02d7
41 changed files with 3872 additions and 274 deletions
--- a/docs/FINDINGS.md
+++ b/docs/FINDINGS.md
@@ -952,6 +952,452 @@ rm package-lock.json && npm install

 ---

-**Document Version:** 1.4  
-**Last Updated by:** Planner Agent (RECIPE-0003 Iteration 2)  
+### [Planner] Research Notes - RECIPE-0004 (2026-02-16)
+
+**Task:** Fix .dockerignore, favicon.ico, push notifications, e2e tests, and logging serialization
+
+#### .dockerignore Research
+**Research Date:** 2026-02-16  
+**Source:** Project analysis, .gitignore comparison, Docker best practices
+
+**Current State:**
+- No `.dockerignore` file exists in project root
+- `.gitignore` exists and excludes: node_modules, build outputs, env files, SSL certs, symlinks, prompts/
+
+**Docker Build Context Issues:**
+Without `.dockerignore`, Docker sends entire workspace to build context including:
+- `node_modules/` (if exists locally) - causes conflicts with `npm ci` in Dockerfile
+- `build/` outputs - unnecessary
+- `.git/` directory - large, unused in container
+- `prompts/` directory - development artifacts
+- `.env` files - should use environment variables instead
+
+**Recommended .dockerignore Content:**
+Based on `.gitignore` and Docker best practices:
+```dockerignore
+node_modules
+.git
+build
+.output
+.vercel
+.netlify
+.wrangler
+.svelte-kit
+.DS_Store
+Thumbs.db
+.env
+.env.*
+!.env.example
+.ssl/
+vite.config.*.timestamp-*
+debug_page.txt
+prompts/
+*.md
+!README.md
+.github/
+.vscode/
+*.log
+coverage/
+.vitest/
+```
+
+**Rationale:**
+- Exclude development dependencies and build artifacts
+- Keep README.md for documentation
+- Exclude version control metadata
+- Reduce build context size significantly
+- Prevent conflicts with Dockerfile's npm ci
+
+---
+
+#### Favicon 404 Error Research
+**Research Date:** 2026-02-16  
+**Source:** Static folder analysis, browser behavior, PWA specifications
+
+**Files Present:**
+- `static/favicon.png` (192x192 PNG) ✓ exists
+- `static/icon-512.png` (512x512 PNG) ✓ exists
+- `static/icon-source.png` (source file) ✓ exists
+- `static/manifest.json` references both PNG files ✓
+
+**404 Source:**
+- Browsers automatically request `/favicon.ico` (legacy format)
+- SvelteKit serves from `static/` folder
+- No `favicon.ico` file exists → 404 error
+
+**Solution Options:**
+
+**Option A - Create favicon.ico (Recommended):**
+Use Sharp to generate ICO from PNG source:
+```javascript
+// New script: scripts/gen-favicon-ico.js
+await sharp('static/icon-source.png')
+  .resize(32, 32)
+  .png()
+  .toFile('static/favicon.ico');
+```
+
+**Option B - SvelteKit Hook Redirect:**
+Add server hook to redirect /favicon.ico → /favicon.png
+- More complex
+- Adds runtime overhead
+- Not recommended
+
+**Chosen Approach:** Option A (generate favicon.ico during build)
+
+---
+
+#### Push Notifications Implementation Research
+**Research Date:** 2026-02-16  
+**Source:** PushNotificationService.ts, web-push library docs, Web Push Protocol RFC 8030
+
+**Current Implementation Analysis:**
+
+**Client-Side (Complete):**
+- `PushNotificationManager.ts` - Full implementation ✓
+  - Permission request ✓
+  - VAPID key fetch ✓
+  - pushManager.subscribe() ✓
+  - Server subscription registration ✓
+- `service-worker.ts` - Push event handler ✓
+- `NotificationSettings.svelte` - UI toggle ✓
+
+**Server-Side (Mock Only):**
+```typescript
+// Current PushNotificationService.ts line 106-125
+private async sendToSubscription(subscription: PushSubscription, data: any): Promise<void> {
+  // In production, use web-push library:
+  // [COMMENTED OUT CODE]
+  
+  // For development, we'll log the notification
+  console.log(`[PushService] Would send push notification:`, {
+    endpoint: subscription.endpoint,
+    data: data
+  });
+  
+  await new Promise(resolve => setTimeout(resolve, 100)); // Simulate
+}
+```
+
+**Problem:** Push notifications are logged but never actually sent to browser.
+
+**Web Push Library Integration:**
+
+**1. Install Dependency:**
+```json
+// package.json
+{
+  "dependencies": {
+    "web-push": "^3.6.7"
+  }
+}
+```
+
+**2. Implementation Pattern:**
+```typescript
+import webpush from 'web-push';
+
+// On init
+webpush.setVapidDetails(
+  'mailto:your-email@example.com',
+  vapidPublicKey,
+  vapidPrivateKey
+);
+
+// In sendToSubscription
+await webpush.sendNotification(
+  subscription,
+  JSON.stringify(payload),
+  {
+    TTL: 60 * 60 * 24 // 24 hours
+  }
+);
+```
+
+**3. Configuration Requirements:**
+- VAPID keys already configured in `queueConfig.push`
+- Default keys present (should regenerate for production)
+- Email contact required by spec (add env var)
+
+**Files to Modify:**
+- `package.json` - add web-push dependency
+- `src/lib/server/notifications/PushNotificationService.ts` - implement actual sending
+- `src/lib/server/queue/config.ts` - add VAPID_EMAIL env var
+
+---
+
+#### Manual Push Notification Test Button Research
+**Research Date:** 2026-02-16  
+**Source:** NotificationSettings.svelte, PushNotificationService API
+
+**Current UI:**
+- Only has enable/disable toggle
+- No manual trigger for testing different notification types
+
+**Test Button Requirements:**
+1. Trigger different notification types:
+   - Success notification (recipe completed)
+   - Error notification (parsing failed)
+   - Progress notification (extraction in progress)
+2. Send to own subscription only
+3. Debug output showing notification payload
+
+**Implementation Approach:**
+
+**Frontend Component:**
+Add to `NotificationSettings.svelte`:
+```svelte
+<button onclick={testNotification('success')}>Test Success</button>
+<button onclick={testNotification('error')}>Test Error</button>
+<button onclick={testNotification('progress')}>Test Progress</button>
+
+async function testNotification(type: 'success' | 'error' | 'progress') {
+  await fetch('/api/notifications/test', {
+    method: 'POST',
+    headers: { 'Content-Type': 'application/json' },
+    body: JSON.stringify({ type })
+  });
+}
+```
+
+**Backend Endpoint:**
+New file: `src/routes/api/notifications/test/+server.ts`
+```typescript
+export const POST: RequestHandler = async ({ request }) => {
+  const { type } = await request.json();
+  
+  const payload = {
+    success: { /* ... */ },
+    error: { /* ... */ },
+    progress: { /* ... */ }
+  }[type];
+  
+  await pushNotificationService.sendNotification(payload);
+  return json({ success: true });
+};
+```
+
+---
+
+#### Playwright E2E Push Notification Testing Research
+**Research Date:** 2026-02-16  
+**Source:** Playwright API docs (BrowserContext.grantPermissions), existing test patterns
+
+**Playwright Push Notification Testing Pattern:**
+
+**Key Methods:**
+1. `context.grantPermissions(['notifications'])` - Grant permission without prompt
+2. `page.evaluate()` - Access PushManager in browser context
+3. `page.waitForEvent()` - Wait for service worker events
+
+**Test Structure:**
+```typescript
+// New file: src/tests/push-notifications.e2e.spec.ts
+import { test, expect } from '@playwright/test';
+
+test.describe('Push Notifications E2E', () => {
+  test('should subscribe to push notifications', async ({ browser }) => {
+    const context = await browser.newContext();
+    await context.grantPermissions(['notifications']);
+    
+    const page = await context.newPage();
+    await page.goto('http://localhost:5173');
+    
+    // Click notification toggle
+    await page.getByRole('button', { name: /enable notifications/i }).click();
+    
+    // Verify subscription created
+    const subscription = await page.evaluate(async () => {
+      const reg = await navigator.serviceWorker.ready;
+      return await reg.pushManager.getSubscription();
+    });
+    
+    expect(subscription).toBeTruthy();
+    expect(subscription.endpoint).toBeDefined();
+    
+    await context.close();
+  });
+});
+```
+
+**Test Coverage:**
+1. Permission grant flow
+2. Subscription creation via PushManager
+3. Server registration (POST /api/notifications/subscribe)
+4. Manual test notification trigger
+5. Subscription persistence in localStorage
+6. Unsubscribe flow
+
+**Vitest Configuration:**
+Current project uses Vitest with @vitest/browser-playwright:
+- Already configured for browser tests
+- Playwright already installed (playwright@^1.56.1)
+- Pattern: `*.e2e.spec.ts` for e2e tests vs `*.spec.ts` for unit tests
+
+---
+
+#### Logging Serialization Research
+**Research Date:** 2026-02-16  
+**Source:** Codebase grep analysis, Node.js console behavior, error object structure
+
+**Problem Analysis:**
+
+**Root Cause:**
+JavaScript error objects logged directly show `[object Object]`:
+```typescript
+// Current pattern (WRONG)
+console.error('[Label]', error);  // Output: [Label] [object Object]
+console.log('[Label]', data);     // Output: [Label] [object Object]
+```
+
+**Affected Files (25 matches found):**
+- `src/lib/server/extraction.ts` - 12 occurrences
+- `src/lib/server/parser.ts` - 4 occurrences
+- `src/lib/server/queue/QueueProcessor.ts` - 3 occurrences
+- `src/lib/server/notifications/PushNotificationService.ts` - 1 occurrence
+- `src/lib/server/api/errorHandler.ts` - 1 occurrence
+- `src/lib/server/llm.ts` - 2 occurrences
+- `src/lib/server/scheduler.ts` - 1 occurrence
+- Others: QueueManager.ts, tandoor.ts
+
+**Solution Patterns:**
+
+**1. Error Objects:**
+```typescript
+// GOOD - Extract relevant properties
+console.error('[Label]', error.message, error.stack);
+console.error('[Label] Error:', {
+  message: error.message,
+  stack: error.stack,
+  name: error.name
+});
+```
+
+**2. Complex Objects:**
+```typescript
+// GOOD - JSON.stringify with formatting
+console.log('[Label] Data:', JSON.stringify(data, null, 2));
+
+// GOOD - Specific properties
+console.log('[Label] Response:', {
+  status: response.status,
+  statusText: response.statusText,
+  body: responseBody
+});
+```
+
+**3. Utility Function:**
+Create `src/lib/server/utils/logger.ts`:
+```typescript
+export function serializeError(error: unknown): string {
+  if (error instanceof Error) {
+    return JSON.stringify({
+      name: error.name,
+      message: error.message,
+      stack: error.stack,
+      ...error
+    }, null, 2);
+  }
+  return JSON.stringify(error, null, 2);
+}
+
+console.error('[Label]', serializeError(error));
+```
+
+**Testing Impact:**
+- Logs are visible in Docker deployments (stdout/stderr)
+- JSON format easier for log aggregation tools
+- Stack traces preserved for debugging
+- Human-readable in console
+
+---
+
+### [Planner] Research Notes - RECIPE-0004 Iteration 1 (2026-02-17)
+
+**Task:** Fix TypeScript type error - NodeJS.Timer should be NodeJS.Timeout in scheduler.ts
+
+#### Node.js Timer Types Research
+**Research Date:** 2026-02-17  
+**Source:** Node.js v25.6.1 Official Documentation (https://nodejs.org/docs/latest/api/timers.html)
+
+**Problem Analysis:**
+TypeScript compile error in `src/lib/server/scheduler.ts:180`:
+```
+Argument of type 'Timer' is not assignable to parameter of type 'Timeout'
+Type 'Timer' is missing the following properties from type 'Timeout': 
+  close, _onTimeout, [Symbol.dispose]
+```
+
+**Root Cause:**
+The `SchedulerState` interface incorrectly uses `NodeJS.Timer` type for `intervalId`, but `setInterval()` returns `NodeJS.Timeout` and `clearInterval()` expects `NodeJS.Timeout` parameter.
+
+**Official Node.js API Documentation:**
+
+**Class: Timeout**
+- Returned by `setInterval()` and `setTimeout()`
+- Can be passed to `clearInterval()` or `clearTimeout()`
+- Has methods: `ref()`, `unref()`, `hasRef()`, `close()`, `refresh()`, `[Symbol.toPrimitive]()`, `[Symbol.dispose]()`
+- TypeScript type: `NodeJS.Timeout`
+
+**API Signatures:**
+```typescript
+// setInterval returns Timeout
+function setInterval(
+  callback: Function, 
+  delay?: number, 
+  ...args: any[]
+): NodeJS.Timeout;
+
+// clearInterval expects Timeout
+function clearInterval(
+  timeout: NodeJS.Timeout | string | number
+): void;
+```
+
+**NodeJS.Timer Type:**
+- Deprecated/incorrect type for timer return values
+- Missing required properties: `close`, `_onTimeout`, `[Symbol.dispose]`
+- Should NOT be used for `setInterval()`/`setTimeout()` return types
+- Causes TypeScript strict mode errors when passed to `clearInterval()`
+
+**Codebase Analysis:**
+```
+grep -r "NodeJS.Timer" src/
+  src/lib/server/scheduler.ts:13    intervalId: NodeJS.Timer | null;
+  src/tests/fixtures.ts:151         let timers: NodeJS.Timer[] = [];
+
+grep -r "NodeJS.Timeout" src/
+  src/routes/api/queue/stream/+server.ts:54    let keepAliveInterval: NodeJS.Timeout | null = null;
+```
+
+**Findings:**
+1. **Incorrect usage (2 occurrences):**
+   - `src/lib/server/scheduler.ts:13` — SchedulerState interface
+   - `src/tests/fixtures.ts:151` — Timer array in test helper
+
+2. **Correct usage (1 occurrence):**
+   - `src/routes/api/queue/stream/+server.ts:54` — keepAliveInterval type
+
+**Solution:**
+Change all `NodeJS.Timer` to `NodeJS.Timeout` to align with Node.js official API contracts and TypeScript type definitions.
+
+**Files to Modify:**
+1. `src/lib/server/scheduler.ts:13` — Type in SchedulerState interface
+2. `src/tests/fixtures.ts:151` — Type in createTimerSpy helper
+
+**Impact:**
+- Type-only change, no runtime behavior modification
+- Fixes TypeScript strict mode compile error
+- Aligns codebase with Node.js standard types
+- Existing tests (260 total) already provide 100% coverage
+
+**References:**
+- Node.js Timers Documentation: https://nodejs.org/docs/latest/api/timers.html#class-timeout
+- TypeScript @types/node package: Official Node.js type definitions
+- Related Error: RECIPE-0004 iteration 0 review_report.yaml
+
+---
+
+**Document Version:** 1.6  
+**Last Updated by:** Planner Agent (RECIPE-0004 Iteration 1)  
 **Next Update:** Developer Agent