`REVIEW-2026-05-04.md`

57 lines
# lacountyeats — overnight debate-team review (2026-05-04)

Two-agent debate (code-reviewer + architect-reviewer) ran against the codebase. **5 patches applied locally** + 1 P0 noted but deferred (CSP refactor) + architecture roadmap captured.

## Patches applied (local — NOT deployed)

| # | File:line | Issue | Fix |
|---|---|---|---|
| 1 | `src/web/views/map.ejs:23-26` | **P0 stored XSS** — `bindPopup(\`<strong>${p.name}</strong>...\`)` interpolates raw DB names + slugs into HTML | Added `esc()` helper (HTML-entity-encode) + `encodeURIComponent(p.slug)` |
| 2 | `src/api/server.ts:303` | `offset` param unbounded — caller could pass `999999999` and force a deep-scan skip on 37K rows | `Math.max(0, Math.min(100000, parseInt(...) \|\| 0))` |
| 3 | `src/api/server.ts:305` | `/api/restaurants` missing `f.is_active = true` filter (closed restaurants leak to API) | Added to `where` array init |
| 4 | `src/api/server.ts:320` | `/api/restaurants/:slug` missing `is_active` filter | Added `f.is_active = true AND` to WHERE |
| 5 | `src/api/server.ts:332` | `/api/pins` missing `is_active` filter (closed restaurants on map) | Added `is_active = true AND` |
| 6 | `src/api/server.ts:378,394` | Sitemap `r.last_updated_at.slice(0,10)` NPE on NULL | `(r.last_updated_at ?? new Date().toISOString()).slice(0,10)` |
| 7 | `src/web/views/list.ejs:109,114` | **Active bug** — pager `URLSearchParams` omits `cuisine` (filter by cuisine + paginate = lose filter) | Added `...(cuisine?{cuisine}:{})` to both spreads |

`tsc --noEmit` passes. Ready to ship with one `deploy lacountyeats` word.

## P0 deferred — needs deliberate refactor

**`script-src 'unsafe-inline'` in CSP nullifies XSS protection.** Forced by inline `<script>` blocks in `map.ejs` + Leaflet bootstrap. To remove: extract those blocks to `/public/js/map.js`, drop `'unsafe-inline'` from `scriptSrc`. ~30 min work + needs prod test of map. Not done overnight — wants careful review.

## Architecture roadmap (architect-reviewer findings)

`server.ts` is 407 lines and at the inflection point. Three projects of yours hit tar pit at this size before. **The next 2-3 features (mockups · claim flow · Stripe · multi-county) will either land in 3 thin module splits or triple `server.ts` overnight.** The cuts are obvious now and 3 hours of work; later they're 3 days.

**Highest-leverage 5 cuts (in order):**

1. **Split `server.ts` along 3 seams**
   - `src/api/db.ts` — Pool + `LIST_QUERY` + `FacilityRow` type
   - `src/api/queries/facilities.ts` — `listFacilities()`, `getFacilityBySlug()`, `getCityBySlug()`, `topCuisines()`. **Biggest win**: `where`/`params` builder duplicated between `app.get('/')` (line 127-135) and `app.get('/api/restaurants')` (305-312) becomes one `buildFacilityFilters(query)` helper.
   - `src/api/routes/{html,api,sitemap}.ts` — three Express routers; `server.ts` shrinks to ~80 lines.

2. **Helper duplication (do now, before mockups land)**
   - Slug→city reverse-resolution duplicated at `server.ts:189` + `:369`. Extract to a Postgres `IMMUTABLE` function `city_slug(text)` + add a functional index — sitemap city queries currently full-scan.
   - Grade badge classes (`grade-A`/`B`/`C`) scattered across `list.ejs`, `city.ejs`, `detail.ejs`. Extract to `partials/grade-pill.ejs`.
   - `formatYearMonth(d)` helper in `views/locals.ts` registered via `app.locals` — keeps presentation logic out of templates.

3. **Tests — the lightest harness pays back week one**
   - Add `vitest` + `supertest` (~30 LOC config).
   - 6 smoke tests against ephemeral schema + 5 fixture rows: `GET /`, `GET /r/known-slug`, `GET /r/missing` (404), `GET /api/restaurants?city=LOS%20ANGELES`, `GET /healthz`, `GET /sitemap.xml`.
   - 1 query test per filter combination — would have caught the `cuisine` pager bug above.
   - ~150 LOC total. Wire to `npm test` + pre-deploy hook in `weekly-ingest.sh`.

4. **Configuration — finish before it bites prod**
   - No `.env.example` in repo. Add `src/config.ts` that parses + validates env once at startup, fails fast if `NODE_ENV=production` without `PGPASSWORD`.
   - `weekly-ingest.sh` reads `GEORGE_AUTH=admin:DWSecure2024!` from a default — hardcoded credential leaks via `ps auxe`. Move to launchd `EnvironmentVariables` plist + the `secrets` skill (per global rules).
   - Add `FEATURE_MOCKUPS=0` style flags now so half-built routes can ship behind `if (config.features.mockups)`.

5. **Deploy story — codify before adding services**
   - Today's deploy is implicit (manual scp + pm2 restart). Add `scripts/deploy.sh`: `rsync src/ public/ db/migrations/ package.json` → Kamatera → `npm ci --production` → `pm2 reload lacountyeats --update-env` → `curl -fsS https://lacountyeats.com/healthz` + `curl -fsS https://lacountyeats.com/r/<known-slug>` (deploy-verification rule: hit the changed code path).
   - **Migrations have no runner.** `db/migrations/002_enrichment_index.sql` exists alone — what applied 001? Adopt 30-line `scripts/migrate.ts` that tracks applied filenames in a `schema_migrations` table.

## Active bug also surfaced (already fixed in patch #7)

`src/api/server.ts:109,114` — pager `URLSearchParams` spreads `q`, `city`, `grade` but omits `cuisine`. Filtering by cuisine then paginating drops the filter. Fixed locally.