---
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_