From f06216b567cf734146c4c7102a98612031734295 Mon Sep 17 00:00:00 2001 From: Lucas Berger Date: Thu, 11 Jun 2026 21:08:54 -0400 Subject: [PATCH] docs(13): add code review report --- .../13-real-lint-gate-eslint/13-REVIEW.md | 161 ++++++++++++++++++ 1 file changed, 161 insertions(+) create mode 100644 .planning/phases/13-real-lint-gate-eslint/13-REVIEW.md diff --git a/.planning/phases/13-real-lint-gate-eslint/13-REVIEW.md b/.planning/phases/13-real-lint-gate-eslint/13-REVIEW.md new file mode 100644 index 0000000..5ca53b5 --- /dev/null +++ b/.planning/phases/13-real-lint-gate-eslint/13-REVIEW.md @@ -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`. 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_