docs(13): add code review report

2026-06-11 21:08:54 -04:00
parent 90d94febea
commit f06216b567
1 changed files with 161 additions and 0 deletions
@@ -0,0 +1,161 @@
+---
+phase: 13-real-lint-gate-eslint
+reviewed: 2026-06-11T00:00:00Z
+depth: standard
+files_reviewed: 18
+files_reviewed_list:
+  - apps/api/src/broker/expand.ts
+  - apps/api/src/broker/outboxWorker.ts
+  - apps/api/src/broker/poller.ts
+  - apps/api/src/broker/reminderScheduler.ts
+  - apps/api/src/broker/spike.ts
+  - apps/api/src/broker/sync.ts
+  - apps/api/src/broker/vevent.ts
+  - apps/api/src/routes/lists.ts
+  - apps/api/src/routes/sse.ts
+  - apps/pwa/src/components/CalendarShell.tsx
+  - apps/pwa/src/components/CreateListSheet.tsx
+  - apps/pwa/src/components/EventForm.tsx
+  - apps/pwa/src/components/ListCard.tsx
+  - apps/pwa/src/hooks/useListSSE.ts
+  - apps/pwa/src/routes/ListDetail.tsx
+  - apps/pwa/src/routes/ListsIndex.tsx
+  - apps/pwa/src/sw.ts
+  - eslint.config.js
+findings:
+  critical: 0
+  warning: 1
+  info: 2
+  total: 3
+status: issues_found
+---
+
+# Phase 13: Code Review Report
+
+**Reviewed:** 2026-06-11
+**Depth:** standard
+**Files Reviewed:** 18
+**Status:** issues_found
+
+## Summary
+
+Phase 13 introduced a type-aware ESLint gate (`typescript-eslint recommendedTypeChecked`) and
+made the source changes required to pass it. The diff base is commit 66c5450. The changes
+across all 18 files are overwhelmingly mechanical: semicolons added throughout (previously
+semicolon-free TypeScript style), `eslint-disable-next-line` comments added to suppress
+justified ical.js `any`-boundary violations, unnecessary `as Type` casts removed after Zod
+validation made them redundant, and `void` prefixes added to fire-and-forget promises.
+
+**Primary concern (D-13-06) verdict:** No floating-promise fix masked a real bug. Every
+`void somePromise` in the changed lines is genuinely fire-and-forget; every delivery-critical
+path (outbox dispatch, CalDAV writes, push, reminder send) remains `await`-ed or
+`.catch()`-handled. The one substantive behavioral change — the SSE list-event handler in
+`sse.ts` — is strictly an improvement: the old `async (event) => { await writeSSE(...) }`
+passed to `subscribeListEvents` was itself a floating promise (the emitter called it but
+never awaited the return), silently swallowing `writeSSE` rejections. The new
+`void (async () => { ... })().catch(err => console.error(...))` correctly surfaces errors.
+
+Three findings follow: one warning (a genuine edge-case correctness gap in `sw.ts`), and two
+info items (one eslint.config.js scope gap and one redundant `as string` cast in `sw.ts`).
+
+---
+
+## Warnings
+
+### WR-01: `sw.ts` notificationclick — `client.navigate(url)` result not handled; open-window fallback unreachable when `focus` succeeds
+
+**File:** `apps/pwa/src/sw.ts:172-179`
+
+**Issue:** The `notificationclick` handler iterates `clientList` and, on the first client
+with `focus`, calls `client.focus().then(() => client.navigate(url))`. Both `focus()` and
+`navigate()` return `Promise<WindowClient | null>`. If `navigate()` resolves to `null`
+(browser rejected the navigation — e.g. the URL was cross-origin or the client was already
+navigating), the failure is silently dropped and the user sees no deep-link behaviour.
+
+More importantly, the `return` inside the `for` loop short-circuits as soon as the first
+`client.focus()` call is dispatched — whether or not `focus()` resolves successfully. If
+`focus()` rejects (e.g. the window was closed between `matchAll` and `focus`), the rejection
+propagates into the `.then(clientList)` chain wrapped by `event.waitUntil`, which means the
+notification click is counted as handled and the fallback `openWindow` branch is never
+reached. The user gets neither navigation nor a new window.
+
+This is a pre-existing logic issue that was present before Phase 13 (Phase 13 only removed
+the `WindowClient` cast). However, Phase 13 also touched these lines and is responsible for
+the current state.
+
+**Fix:** Chain a `.catch()` on the `focus().then(navigate())` to fall through to
+`openWindow` on failure, and check the `navigate()` result:
+
+```typescript
+event.waitUntil(
+  self.clients.matchAll({ type: 'window', includeUncontrolled: true }).then((clientList) => {
+    for (const client of clientList) {
+      if ('focus' in client) {
+        return client
+          .focus()
+          .then(() => client.navigate(url))
+          .then((navigated) => {
+            if (navigated === null && self.clients.openWindow) {
+              return self.clients.openWindow(url);
+            }
+          })
+          .catch(() => {
+            if (self.clients.openWindow) return self.clients.openWindow(url);
+          });
+      }
+    }
+    if (self.clients.openWindow) return self.clients.openWindow(url);
+  }),
+);
+```
+
+---
+
+## Info
+
+### IN-01: `eslint.config.js` disableTypeChecked block does not cover `apps/api/src/broker/spike.ts`
+
+**File:** `eslint.config.js:88-104`
+
+**Issue:** `spike.ts` is a dev-only script that is included in `apps/api/tsconfig.json`
+(via `src/**/*`) and so IS type-checked by `projectService`. This is intentional and
+currently works fine. However, the file uses `console.log` extensively and raw `as { ctag?:
+string }` casts on tsdav objects that may generate `@typescript-eslint/no-unsafe-*` warnings
+in future ical.js or tsdav type definition updates. Since `spike.ts` is never imported by the
+app and is a one-time investigative script, disabling type-aware rules for it would be safer
+than relying on its continued type-compatibility. This is low-urgency — the file passes lint
+today — but worth noting for maintainability.
+
+**Fix:** Add `'apps/api/src/broker/spike.ts'` to the `disableTypeChecked` file list at
+`eslint.config.js:88`, or (better) move the spike out of `src/` into a `scripts/` directory
+excluded from tsconfig.
+
+### IN-02: `sw.ts` notificationclick — redundant `as string` cast after typeof guard
+
+**File:** `apps/pwa/src/sw.ts:164`
+
+**Issue:** The code reads:
+
+```typescript
+if (typeof event.notification.data?.url === 'string') {
+  // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
+  url = event.notification.data.url as string;
+}
+```
+
+Inside the `typeof === 'string'` branch, TypeScript narrowing already knows the value is
+`string`. The `as string` cast is redundant. The `eslint-disable-next-line` is still
+required to suppress `no-unsafe-member-access` (because `Notification.data` is `any`), but
+the cast itself adds noise.
+
+**Fix:** Remove the `as string` suffix:
+
+```typescript
+url = event.notification.data.url; // eslint-disable-next-line already on prior line
+```
+
+---
+
+_Reviewed: 2026-06-11_
+_Reviewer: Claude (gsd-code-reviewer)_
+_Depth: standard_