CTO roadmap: harden, cache, and de-duplicate (P0–P3)#121
Merged
Conversation
Evidence-based assessment integrating engineering (CTO), design (CDO), and architecture (senior engineer) lenses into one prioritized P0–P3 roadmap, calibrated for a solo/volunteer team. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
queryBigQuery() now accepts an optional named-params object passed through to the BQ client. Convert the auth allowlist check from string interpolation to a bound @email parameter — making the safe path the easy path for future queries. Table name stays interpolated (operator-controlled env var; BQ cannot parameterize identifiers). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add cacheStatsData() helper (unstable_cache, 1h revalidate, entity-scoped keys, bypassed outside production) and wrap all five stats loaders (region/ao/pax/area/sector). Normalization runs inside the cache so the cached value is plain JSON. Cuts BQ cost/latency on repeat views and removes BQ from the hot path. Cache key excludes the user (stats data is entity-scoped, not user-scoped). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The region charts grid was built but disabled via a `hidden` class, so the
"how is my region trending?" view never rendered. Remove `hidden` and give
the bar series readable legend/tooltip labels ("Unique PAX" / "Unique Qs")
instead of raw column names.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add lib/observability.ts with reportError()/makeErrorId() — a single choke point that emits a structured JSON log line and returns a stable error id, ready to forward to Sentry (or any provider) from one TODO hook without touching call sites. Wire it into the global error boundary and the four search/list API catch blocks, and return the errorId in 500 responses so support can correlate a user-visible id with server logs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Reverts e2ca1cc. The charts were not an accidental oversight: region, area, and sector pages all hide their charts with the same `hidden` class, so the disable was a deliberate, site-wide decision (not a region-only slip). The chart components also have known quality issues (color-only encoding, raw axis labels). Restoring production behavior until charts are shipped as a proper, consistent feature rather than a flag flip. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Charts were intentionally hidden across region/area/sector, so showing them is feature work, not a flag flip. Move from P0 to P2 and record the revert. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the duplicated filter logic that was re-implemented across the stats pages, events API routes, BigQuery modules, and loaders — the drift class behind both search postmortems — with a single source of truth: - StatsFilters / DateRangeFilters types (was 10 redeclared *FilterOpts). - parseFilterParams() (page searchParams) and parseFilterSearchParams() (URLSearchParams) replace the 3 stats-page parseIdList copies and the 3 events-route parseNumberList copies. - toFiniteNumbers() replaces the 3 bq cleanNumberList copies. - filtersToQueryString() rounds out the contract. Behavior-preserving: same numeric parsing, modes now validated to include/exclude (downstream default is unchanged). The client pageFilter.tsx parser is intentionally left for the P2 component split. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Route tests for /api/region/list and /api/search asserting a BigQuery failure surfaces as HTTP 500 with an error body, NOT 200 + empty results (the exact regression from issue #60 and the silent-failures follow-up), plus 401-when-unauthed and short-query short-circuit coverage. - Unit tests for the new lib/filters.ts contract (parse/serialize/round-trip) and lib/observability.ts (stable error ids, structured logging). - Exclude lib/cache.ts from coverage (thin env-gated unstable_cache wrapper, consistent with db.ts). Tests: 55 -> 85. Coverage restored to ~93% (was failing the 70% gate after the P0/P1 lib additions). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace hardcoded ScrollShadow pixel heights with max-h-[60vh] so the cards size to content and cap at the viewport instead of swallowing it on mobile: - AchievementsCard h-[1105px] (worst offender — taller than a phone screen) - KotterCard h-[500px] - upcomingEvents h-[500px] - AOBreakdownCard h-[400px] Needs a live device audit (portrait + landscape) to eyeball, per the plan. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CI lacks OAuth + BigQuery credentials, so the authenticated/data-backed flow can't run there. Instead, add a local smoke suite (e2e/smoke.spec.ts) that mints a valid __session cookie from SESSION_SECRET (replicating createSessionValue) to exercise the real signed-in path against `npm run dev`: landing render, unauth->/ redirect, authenticated region dashboard load, and a well-formed 200 from /api/search. - playwright.config.ts targets https://localhost:3001 with ignoreHTTPSErrors. - `npm run test:e2e` script; e2e/ excluded from vitest; artifacts gitignored. - One-time setup: `npx playwright install chromium`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
These reliability/security items are operator actions in GCP/billing, not code. Document a concrete checklist: move prod secrets off the laptop to Secret Manager, rotate SESSION_SECRET, add an uptime check on the public landing page, and add a BigQuery billing budget alert + optional per-day query quota. Cross-references the P0-3 error-tracking seam. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
max-h-[60vh] alone is still ~600-800px tall on large desktop monitors, so content-heavy cards (Achievements especially) render oversized. Add an lg: cap (lg:max-h-[32rem], ~512px) so desktop gets a comfortable fixed height while mobile keeps the viewport-relative 60vh. Applies to Achievements, Kotter, Upcoming, and AOBreakdown scroll cards. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The Achievements/Kotter row is a 2-col grid where the left column holds only Achievements and the right column holds Kotter + Upcoming stacked (taller). Grid's default align-items: stretch made Achievements stretch to match, leaving empty space below its now-capped scroll area. Add lg:items-start so each column sizes to its content and Achievements stays compact at the top. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Convert every LIKE search clause and the email lookup from manual single-quote escaping to bound @params, removing the last string-injection surface in the query layer: - search.ts (3 LIKE clauses share one @term) - regions/aos/areas/sectors/pax searchByName (LIKE @term) - pax.getPaxIdByEmail (LOWER(email) = @email) Numeric IN/id clauses are left interpolated — they're already validated to finite numbers via toFiniteNumbers(), so they carry no injection risk and converting to array params would add churn for no security gain. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Delete dead BigQuery modules: aos.disable.ts, pax.disable.ts, and regions.desable.ts (note the typo) — none were imported. - Drop unused `pg` (and orphaned `@types/pg`); Postgres was never wired up. If the P3 serving-tier idea is pursued later, re-add then. - Update baseline-browser-mapping to latest (2.10.37). The remaining build warning is upstream dataset staleness (nothing else depends on it; we're already on latest) and is benign. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add a fixed-width rank number (#1..#n) before each leader, and render the metric bold in the primary color instead of plain muted text so the number reads as the hero. Long names now truncate rather than pushing the metric off-row. Applies to both region and AO leaderboards (shared component). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Per feedback, drop the bold + primary-color styling on the leaderboard metric and restore its original plain appearance. Rank numbers and the long-name truncation stay. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
On phones, an event's 20+ attendee chips render as an unreadable wall. Show
a tappable "{n} PAX" count on mobile that expands to the full chip list (and
a "Show less" control to re-collapse). Desktop is unchanged — the full chips
always render at sm+ where horizontal space isn't a problem. Per-event
expand state lives in the card.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Restructure the middle of the region page so cards size to content and read top-down by how a leader scans: Summary | Leaders -> Achievements | Kotter (celebrate / at-risk, now balanced 2-col peers) -> Upcoming Events (full width) -> Events log. This also resolves the parked Achievements whitespace: Achievements no longer sits alone beside a taller Kotter+Upcoming stack, so there's no large asymmetric dead space below it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The ~60-line "Data Not Available" migration empty state was inlined and near-identically copy-pasted across the region, AO, and PAX pages. Extract it into one <EntityDataUnavailable entity=... /> component and replace the three inline blocks (each ~68 lines -> 1 line). Copy normalized to read consistently regardless of entity (drops copy-paste artifacts like the AO page saying "your region"). area/sector keep their own shorter, aggregation- appropriate empty states. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Extract two pieces that were copy-pasted across the entity SummaryCards, with zero change to rendered output: - renderStat() -> src/lib/utils.ts (was duplicated in region/ao/area/sector), now unit-tested. - HelpHint -> the mobile-aware Tooltip/Popover "?" affordance (was an inline isMobile hook + ~20-line block in region/area/sector). region/ao/area/sector SummaryCards now import both; the per-row markup is intentionally left inline (region uses text-sm, area/sector don't — unifying that StatRow needs a visual check). pax SummaryCard is structurally different (11 rows, no renderStat) and deferred. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Centralize the breadcrumb scaffold (Home -> Nation -> optional parent -> current) in a server-safe lib/breadcrumb.ts, so all five stats pages build trails the same way. PAX opts out of the Nation crumb via includeNation: false. BreadcrumbEntry moves to lib and is re-exported from the component. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the per-loader JSON.parse(JSON.stringify(data, replacer)) dance —
two passes, and capable of silently turning a real Date into {} — with one
recursive, typed normalizeDeep<T>() shared by all five loaders. It unwraps
{ value } wrappers, converts bigint->number, and converts Date->ISO string.
Lives in lib/normalize.ts (standalone, not db.ts) so it's unit-testable
without constructing a BigQuery client at import. Behavior-preserving for the
real BQ wrapper shapes; now covered by tests.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Bare <a>/<button>/inputs (search modal buttons, jump links, the attendee collapse toggle, breadcrumb back) relied on inconsistent browser default focus rings. Add a :focus-visible outline wrapped in :where() so it has zero specificity — HeroUI components keep their own focus styling untouched, and this only fills the gap for unstyled native elements. (The search-modal loading spinner from #18 already existed.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Document the decision to keep BigQuery as the serving layer (mitigated by the P0 cache) rather than build a Postgres/materialized serving tier now, plus the concrete triggers that should prompt revisiting it and a sketch of the design. Rate limiting (the other half of P3-22) stays deferred — add only if traffic/ cost data shows the need. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The navbar brand was text-only ("PAX VAULT"). Add an inline F3 mark (circle +
stylized "F3") before it, using currentColor so it adapts to light/dark.
Cheap brand equity, confirmed visually.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Pull the entire active-filter chip subsystem out of the 1031-line pageFilter.tsx into a self-contained ActiveFilterChips component (helpers + build + render). The chip-building and URL-removal logic are pure exported functions (buildActiveChips / filtersAfterRemoval), now unit-tested. Active-filter value chips are now removable: clicking ✕ drops that value from the URL and re-navigates (re-querying the page); the mode param is cleared when a dimension empties. "Exclude" indicator chips stay non-removable. pageFilter.tsx: 1031 -> 884 lines. The state-coupled accordion drawer is intentionally left in place (a larger, separate extraction). Verified visually. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
👋 TL;DR
A CTO-review pass executing the P0–P3 roadmap: hardens the data layer (full BigQuery query parameterization, error-reporting seam, postmortem regression tests), caches stats pages in front of BigQuery, and de-duplicates/cleans up the codebase — plus targeted UX fixes. No feature/behavior changes beyond the noted UX polish. Plan + remaining-work notes are in
.context/.🔎 Details
Highlights (29 working commits; see
.context/cto-review-roadmap.mdfor the full plan and.context/remaining-work.mdfor what's intentionally deferred):Harden
LIKE/email clause and the auth allowlist now use bound@params(no string interpolation). Newparamssupport inqueryBigQuery.lib/observability.ts) wired into the global error boundary + search/list routes (returns anerrorId).[]). Test count 55 → 101, coverage ~93%.Cache
lib/cache.ts,unstable_cache, 1h revalidate, bypassed in dev). Removes BQ from the hot path on repeat views.Sharpen (structure + UX)
lib/filters.ts) replacing 4 parser variants + 10 redeclared types.normalizeDeep<T>()replacing the per-loaderJSON.parse(JSON.stringify())dance.EntityDataUnavailable/renderStat/HelpHint/buildBreadcrumb; extractedActiveFilterChips(filter chips are now removable).*.disable.ts) and unused deps (pg).Docs: ops runbook, serving-layer ADR, roadmap + remaining-work notes under
.context/.Notable scope calls: the region/area/sector charts are left hidden (they were intentionally disabled site-wide — shipping them is separate feature work); the
events.tsx/pageFilteraccordion splits are deferred to incremental work.✅ How to Test
CI gate (all green locally):
npm run format:check && npm run lint && npm run typecheck && npm run test:coverage && npm run build.Manual / QA:
npm run build && npm run start, load a region page twice within the hour → the 2nd load issues no new BigQuery job (check GCP Job History) and is fast. (Dev mode intentionally bypasses the cache.)npx playwright install chromiumthennpm run test:e2e(signs in via a minted session, checks landing/redirect/region/search).