162 lines
6.4 KiB
Markdown
162 lines
6.4 KiB
Markdown
---
|
|
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_
|