Skip to content

Fix security, build reproducibility, and correctness issues from code review#89

Merged
kwiens merged 2 commits into
mainfrom
fix/code-review-modernization
Jun 21, 2026
Merged

Fix security, build reproducibility, and correctness issues from code review#89
kwiens merged 2 commits into
mainfrom
fix/code-review-modernization

Conversation

@kwiens

@kwiens kwiens commented Jun 21, 2026

Copy link
Copy Markdown
Owner

Addresses the prioritized findings from a full code review. All changes are on a fresh branch off main.

Validation

  • pnpm lint ✅ (now includes typecheck + biome lint --error-on-warnings)
  • pnpm test:run325/325
  • pnpm build ✅ — previously failed because next/font/google fetched Geist at build time

Changes

P1 — Security

  • Popup XSS: live GBFS station data (name/description/address) flowed unescaped into Popup.setHTML, allowing HTML/JS injection from the feed. Added shared src/utils/html.ts (escapeHtml); every popup value is escaped and the Google Maps query URL is encodeURIComponent'd.

P2

  • Privacy/battery: stop requesting geolocation on map load — the GPS watch now starts only on explicit user intent (tracking toggle / ride recording). initializeLocationMarker is idempotent and setLocationWatch clears any prior recenter interval before creating a new one (fixes duplicate intervals).
  • Build reproducibility: self-host Geist Sans/Mono via next/font/local (vendored variable woff2 in src/app/fonts/).
  • Dev tooling exposure: gate the /test fixture/editor page (incl. "Clear All Rides") behind NODE_ENV !== 'production' via notFound().
  • Typecheck: add typecheck script (tsc --noEmit) and fold it into lint; fix all pre-existing type errors in tests; bump tsconfig target ES2017 → ES2022 (also unblocks the dotAll regex flag in gpx.test.ts).
  • Trail bounds: calculateTrailBounds now queries MTN_BIKE_SOURCE_ID (the runtime-attached MTB source) instead of the Studio composite source; dead getNorth() !== undefined checks replaced with bounds.isEmpty().
  • Stale trail UI: map route-click now also dispatches TRAIL_DESELECT (mirrors the sidebar) and ElevationProfile clears any active trail/ride profile on ROUTE_SELECT.
  • GBFS toggle race + duplication: guard the rentals layer against re-showing markers after the user toggles off during a slow fetch; extract shared fetchBikeRentalLocations() used by both Map.tsx and BikeRentalList.tsx.

P3

  • Lint strictness: biome lint runs with --error-on-warnings. Removed the 4 production non-null assertions (useRideRecording, dem); ! is allowed only in test files via a Biome override.
  • Accessibility: re-enable pinch-to-zoom by dropping maximum-scale=1 / user-scalable=no and moving viewport + theme-color into a Next viewport export.

Misc

  • Moved eslint-config-prettier and eslint-import-resolver-typescript to devDependencies.

Notes / judgment calls

  • Removing GPS-on-load made setLocationWatch trip react-hooks/exhaustive-deps on the once-mount recording effect. It is genuinely ref-stable, so I used a justified eslint-disable-next-line pointing at the deferred GPS/compass hook extraction rather than forcing that refactor here.
  • Two GPS watches still exist (Map's location marker + useRideRecording); merging them is the same deferred hook refactor. The privacy win (no watch until intent) is in place regardless.

Out of scope (follow-ups from the review)

GBFS fetch timeout/validation + is_renting gating, the About-page SVG <Image> rendering issue, deleting the dead src/components/ui/ tree, pointing add_trail_bounds.py at mountain-bike-trails.ts (it currently writes to the barrel), and noUncheckedIndexedAccess (full strictness was intentionally deferred).

… review

P1 — Security
- Escape all map-popup values (name/description/address/type/price/hours) and
  encodeURIComponent the Google Maps query URL. Live GBFS station data flowed
  unescaped into Popup.setHTML, allowing HTML/JS injection from the feed.
  New shared helper src/utils/html.ts (escapeHtml).

P2 — Privacy / battery
- Stop requesting geolocation on map load. The GPS watch now starts only on
  explicit user intent (tracking toggle or ride recording). initializeLocationMarker
  is idempotent and setLocationWatch clears any prior recenter interval before
  creating a new one (fixes duplicate intervals).

P2 — Build reproducibility
- Self-host Geist Sans/Mono via next/font/local (vendored variable woff2 in
  src/app/fonts) instead of next/font/google, which fetched from Google Fonts at
  build time and broke offline/sandboxed builds.

P2 — Dev tooling exposure
- Gate the /test fixture/editor page (incl. "Clear All Rides") behind
  NODE_ENV !== 'production' via notFound().

P2 — Typecheck
- Add `typecheck` script (tsc --noEmit) and fold it into `lint`. Fix all
  pre-existing type errors in tests. Bump tsconfig target ES2017 -> ES2022
  (also unblocks the dotAll regex flag in gpx.test.ts).

P2 — Trail bounds
- calculateTrailBounds now queries MTN_BIKE_SOURCE_ID (the runtime-attached MTB
  source) instead of the Studio "composite" source, so runtime bounds work
  without precalculated defaultBounds. Replace dead getNorth()!==undefined checks
  with bounds.isEmpty().

P2 — Stale trail UI on route select
- Map route-click now also dispatches TRAIL_DESELECT (mirrors the sidebar), and
  ElevationProfile clears any active trail/ride profile on ROUTE_SELECT.

P2 — GBFS toggle race + duplication
- Guard the rentals layer against re-showing markers after the user toggles off
  during a slow fetch. Extract shared fetchBikeRentalLocations() used by both
  Map.tsx and BikeRentalList.tsx (removes a duplicated fetch+join pipeline).

P3 — Lint strictness
- biome lint now runs with --error-on-warnings. Remove the 4 production
  non-null assertions (useRideRecording, dem) and allow `!` only in test files
  via a Biome override.

P3 — Accessibility
- Re-enable pinch-to-zoom: drop maximum-scale=1/user-scalable=no by moving
  viewport + theme-color to a Next viewport export.

Misc: move eslint-config-prettier and eslint-import-resolver-typescript to
devDependencies.
@vercel

vercel Bot commented Jun 21, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
bikemap Ready Ready Preview Jun 21, 2026 5:21am

Request Review

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

Resolved conflicts:
- package.json: take main's dependency bumps; keep eslint-config-prettier and
  eslint-import-resolver-typescript in devDependencies (my relocation) and the
  typecheck/lint script changes.
- src/app/layout.tsx: keep self-hosted fonts + the viewport export (now using
  siteConfig.themeColor and pinch-zoom enabled); drop the static manifest link
  (main provides it via manifest.ts) — superseded main's inline viewport/theme
  meta tags.
- src/app/test/page.tsx: accept main's deletion (supersedes my dev-only gate).
- pnpm-lock.yaml: regenerated from the merged package.json.
- biome.json: bump $schema to match main's @biomejs/biome 2.4.16.

Verified post-merge: tsc --noEmit, eslint, biome --error-on-warnings,
329 tests, and next build all pass.
@kwiens

kwiens commented Jun 21, 2026

Copy link
Copy Markdown
Owner Author

CR 👍

@kwiens
kwiens merged commit 5ad9d2c into main Jun 21, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant