fegger
77b8c6db98
Update CHECKLIST.md and REWRITE_PLAN.md to reflect the current post-rewrite status and remaining tasks. - Add input validation to /api/hafas route to enforce request shape and cap results - Fix SSR crash in useBikeRoute by using relative fetch URLs - Wire CalendarPanel to fetch calendar events and merge them into the global events store
572 lines
17 KiB
Markdown
572 lines
17 KiB
Markdown
# TimeToLeave — Post-Rewrite Fix Plan
|
||
|
||
> **Status:** In Progress
|
||
> **Updated:** After post-rewrite code review
|
||
> **Scope:** Fix blocking bugs, deduplicate code, improve performance/UX, restore tests
|
||
|
||
---
|
||
|
||
## 1. Current State
|
||
|
||
| Layer | Technology | Status |
|
||
|-------|-----------|--------|
|
||
| Framework | Next.js 16 + App Router | ✅ Working |
|
||
| Language | TypeScript (strict) | ✅ Compiles clean |
|
||
| State | React Context + localStorage | ✅ Working |
|
||
| Styling | Tailwind CSS v4 (dark classes present) | ⚠️ No toggle |
|
||
| Tests | Vitest + jsdom + RTL | ⚠️ Setup file missing |
|
||
| Build | Docker multi-stage standalone | ✅ Working |
|
||
| HAFAS Integration | ApiClient + HafasClient + route | ⚠️ Duplicated parsing |
|
||
| Geocoding | ApiClient + GeocodingClient | ⚠️ Client unused by route |
|
||
| Bike Routing | ApiClient + BikeRoutingClient | ⚠️ Client unused by route |
|
||
|
||
---
|
||
|
||
## 2. Findings Summary
|
||
|
||
### 🔴 Blocking (3 issues)
|
||
|
||
| # | Issue | File | Impact |
|
||
|---|-------|------|--------|
|
||
| 1 | SSR crash: `window.location.href` in `useBikeRoute` | `src/hooks/useBikeRoute.ts:22` | Crashes during server-side rendering |
|
||
| 2 | Calendar events lost: `useCalendar` never writes to `EventsStore` | `src/hooks/useCalendar.ts`, calendar pages | Imported events disappear on navigation |
|
||
| 3 | No input validation on `/api/hafas` | `src/app/api/hafas/route.ts` | Any client can abuse ÖBB API through proxy |
|
||
|
||
### 🟡 Code Quality (6 issues)
|
||
|
||
| # | Issue | File | Impact |
|
||
|---|-------|------|--------|
|
||
| 4 | HAFAS journey parsing duplicated | `hafas-client.ts` + `useJourneys.ts` | Maintenance burden, drift risk |
|
||
| 5 | `GeocodingClient` / `BikeRoutingClient` never used by API routes | `api/geocode/route.ts`, `api/bike-route/route.ts` | Dead code, routes miss caching/retry |
|
||
| 6 | O(n×m) calendar cell rendering | `CalendarView.tsx` | Slow with many events |
|
||
| 7 | No debounce on geocode/station lookups | `useGeocode.ts`, `useDestinationStation.ts` | Fires API call per keystroke |
|
||
| 8 | `LiveStatusUtils` is an empty stub | `live-status-utils.ts` | Dead code |
|
||
| 9 | Dark mode classes present but no toggle | `layout.tsx`, `Header.tsx` | Users can't switch themes |
|
||
|
||
### 🟢 Minor (3 issues)
|
||
|
||
| # | Issue | File | Impact |
|
||
|---|-------|------|--------|
|
||
| 10 | Bike route API doesn't request `steps=true` | `api/bike-route/route.ts` | Steps always empty |
|
||
| 11 | Test setup file missing | `src/test/setup.ts` | Tests may fail |
|
||
| 12 | No correlation IDs in API errors | All API routes | Hard to debug production issues |
|
||
|
||
---
|
||
|
||
## 3. Implementation Phases
|
||
|
||
### Phase 1 — Unblock Runtime (~45 min)
|
||
|
||
Fix bugs that crash the app or lose user data.
|
||
|
||
---
|
||
|
||
#### Step 1: Fix SSR Crash in `useBikeRoute` (~10 min)
|
||
|
||
**File:** `src/hooks/useBikeRoute.ts`
|
||
|
||
**Problem:** Line 22 calls `new URL("/api/bike-route", window.location.href)` which throws during SSR because `window` is undefined.
|
||
|
||
**Change:** Replace the `new URL` + `searchParams` pattern with a relative fetch URL:
|
||
|
||
```ts
|
||
// BEFORE:
|
||
const url = new URL("/api/bike-route", window.location.href);
|
||
url.searchParams.set("fromLat", String(fromLat));
|
||
url.searchParams.set("fromLng", String(fromLng));
|
||
url.searchParams.set("toLat", String(toLat));
|
||
url.searchParams.set("toLng", String(toLng));
|
||
const response = await fetch(url.toString());
|
||
|
||
// AFTER:
|
||
const response = await fetch(
|
||
`/api/bike-route?fromLat=${fromLat}&fromLng=${fromLng}&toLat=${toLat}&toLng=${toLng}`,
|
||
);
|
||
```
|
||
|
||
**Why it works:** Next.js rewrites relative fetch URLs to the correct origin during SSR. In the browser, relative URLs resolve to the current origin. No `window` needed.
|
||
|
||
---
|
||
|
||
#### Step 2: Wire Calendar Events into `EventsStore` (~15 min)
|
||
|
||
**Files to read:** `src/app/calendar/UrlTab.tsx`, `src/app/calendar/FileTab.tsx`
|
||
|
||
**Problem:** `useCalendar` holds events in local hook state. The global `EventsProvider` is never updated. When the user navigates away from `/calendar`, imported events are gone.
|
||
|
||
**Approach:** After a successful import in `UrlTab` and `FileTab`, merge the results into the global store.
|
||
|
||
```tsx
|
||
// In UrlTab.tsx and FileTab.tsx, after successful fetch:
|
||
const { mergeEvents } = useEventsStore();
|
||
|
||
// Convert CalendarEvent[] (string eventTime) → Event[] (Date eventTime)
|
||
const storeEvents: Event[] = calendarEvents.map((e) => ({
|
||
id: e.id,
|
||
title: e.title,
|
||
destination: e.destination,
|
||
eventTime: new Date(e.eventTime),
|
||
source: e.source,
|
||
}));
|
||
mergeEvents(storeEvents);
|
||
```
|
||
|
||
**Key detail:** `CalendarEvent.eventTime` is an ISO string, `Event.eventTime` is a `Date`. The conversion must happen before storing.
|
||
|
||
---
|
||
|
||
#### Step 3: Add Input Validation to `/api/hafas` (~20 min)
|
||
|
||
**File:** `src/app/api/hafas/route.ts`
|
||
|
||
**Problem:** `request.json()` is forwarded directly to the ÖBB API with no validation. A malicious or buggy client can send arbitrary methods.
|
||
|
||
**Add before the `fetch(HAFAS_URL)` call:**
|
||
|
||
```ts
|
||
// Validate body shape
|
||
if (!body || !Array.isArray(body.svcReqL) || body.svcReqL.length === 0) {
|
||
return NextResponse.json({ error: "Invalid HAFAS request body" }, { status: 400 });
|
||
}
|
||
|
||
const svcReq = body.svcReqL[0];
|
||
const allowedMethods = ["TripSearch", "LocMatch"];
|
||
|
||
if (!svcReq || typeof svcReq !== "object" || !allowedMethods.includes(svcReq.meth)) {
|
||
return NextResponse.json(
|
||
{ error: `Invalid HAFAS method. Allowed: ${allowedMethods.join(", ")}` },
|
||
{ status: 400 },
|
||
);
|
||
}
|
||
|
||
// Optional: limit TripSearch results
|
||
if (svcReq.meth === "TripSearch" && svcReq.req?.numF > 10) {
|
||
svcReq.req.numF = 10;
|
||
}
|
||
```
|
||
|
||
---
|
||
|
||
### Phase 2 — Deduplicate Code (~2 hours)
|
||
|
||
Eliminate duplicated logic so each integration has one source of truth.
|
||
|
||
---
|
||
|
||
#### Step 4: Consolidate HAFAS Journey Parsing (~40 min)
|
||
|
||
**Files:** `src/lib/hafas-client.ts`, `src/hooks/useJourneys.ts`
|
||
|
||
**Problem:** Both files contain nearly identical logic to parse `HafasJourney[]` → `Journey[]`. The `hafas-client.ts` `fetchJourneys()` method is never called (the API route is a dumb proxy, and `useJourneys` does its own parsing).
|
||
|
||
**Option A — Export shared parser (minimal risk, recommended):**
|
||
|
||
1. Move the `parseHafasJourneys` function from `useJourneys.ts` into `hafas-client.ts` and export it.
|
||
2. In `useJourneys.ts`, import and call `parseHafasJourneys` from `@/lib/hafas-client`.
|
||
|
||
**Option B — Full refactor (more work, cleaner long-term):**
|
||
|
||
1. Make `api/hafas/route.ts` use `HafasClient` internally instead of proxying raw JSON.
|
||
2. Have the route return parsed `Journey[]` or `Station[]` directly.
|
||
3. Remove all HAFAS response parsing from hooks — they trust the API route's output shape.
|
||
|
||
Start with **Option A** to eliminate duplication quickly. Pursue **Option B** later if the HAFAS response shape changes.
|
||
|
||
---
|
||
|
||
#### Step 5: Wire API Routes to Use Library Clients (~30 min)
|
||
|
||
**Files:** `src/app/api/geocode/route.ts`, `src/app/api/bike-route/route.ts`
|
||
|
||
**Problem:** `GeocodingClient` and `BikeRoutingClient` are fully implemented with retry + caching via `ApiClient`, but the API routes bypass them with raw `fetch()`.
|
||
|
||
**Changes — `api/geocode/route.ts`:**
|
||
|
||
```ts
|
||
import { GeocodingClient } from "@/lib/geocoding-client";
|
||
|
||
// Module-level singleton — cache persists across requests
|
||
const client = new GeocodingClient();
|
||
|
||
export async function GET(request: NextRequest) {
|
||
const { searchParams } = new URL(request.url);
|
||
const name = searchParams.get("name");
|
||
const countrycodes = searchParams.get("countrycodes");
|
||
|
||
if (!name) {
|
||
return NextResponse.json({ error: "Missing 'name' parameter" }, { status: 400 });
|
||
}
|
||
|
||
const results = await client.geocode(name, countrycodes || undefined);
|
||
|
||
if (results.length === 0) {
|
||
return NextResponse.json({ error: "No results found" }, { status: 404 });
|
||
}
|
||
|
||
return NextResponse.json(results[0]);
|
||
}
|
||
```
|
||
|
||
**Changes — `api/bike-route/route.ts`:**
|
||
|
||
```ts
|
||
import { BikeRoutingClient } from "@/lib/bike-routing-client";
|
||
|
||
const client = new BikeRoutingClient();
|
||
|
||
export async function GET(request: NextRequest) {
|
||
const { searchParams } = new URL(request.url);
|
||
const fromLat = parseFloat(searchParams.get("fromLat") ?? "");
|
||
const fromLng = parseFloat(searchParams.get("fromLng") ?? "");
|
||
const toLat = parseFloat(searchParams.get("toLat") ?? "");
|
||
const toLng = parseFloat(searchParams.get("toLng") ?? "");
|
||
|
||
if (isNaN(fromLat) || isNaN(fromLng) || isNaN(toLat) || isNaN(toLng)) {
|
||
return NextResponse.json(
|
||
{ error: "Missing required parameters (fromLat, fromLng, toLat, toLng)" },
|
||
{ status: 400 },
|
||
);
|
||
}
|
||
|
||
const route = await client.getBikeRoute(fromLat, fromLng, toLat, toLng);
|
||
|
||
if (!route) {
|
||
return NextResponse.json({ error: "No route found" }, { status: 404 });
|
||
}
|
||
|
||
return NextResponse.json(route);
|
||
}
|
||
```
|
||
|
||
**Also add `steps=true` to `BikeRoutingClient.getBikeRoute`** (see Step 11).
|
||
|
||
---
|
||
|
||
#### Step 6: Remove Dead Code (~5 min)
|
||
|
||
**File:** `src/lib/live-status-utils.ts`
|
||
|
||
Delete the file entirely. `LiveStatusUtils` returns `null` for everything and is never imported.
|
||
|
||
**Also:** Remove the export from `src/lib/index.ts`:
|
||
```ts
|
||
// REMOVE this line:
|
||
export * from "./live-status-utils";
|
||
```
|
||
|
||
---
|
||
|
||
#### Step 7: Create Missing Test Setup File (~10 min)
|
||
|
||
**File:** `src/test/setup.ts`
|
||
|
||
`vitest.config.ts` references this file but it doesn't exist. Create it:
|
||
|
||
```ts
|
||
import "@testing-library/jest-dom/vitest";
|
||
```
|
||
|
||
This registers the `@testing-library/jest-dom` matchers (`toBeInTheDocument`, `toHaveTextContent`, etc.) globally for Vitest.
|
||
|
||
---
|
||
|
||
### Phase 3 — Performance & UX (~1.5 hours)
|
||
|
||
---
|
||
|
||
#### Step 8: Add Debounce to Lookup Hooks (~25 min)
|
||
|
||
**Files:** `src/hooks/useGeocode.ts`, `src/hooks/useDestinationStation.ts`
|
||
|
||
**Problem:** Both hooks fire a network request on every `destination` string change. Typing "Graz Hbf" sends 8 requests.
|
||
|
||
**Pattern — wrap the fetch in a debounced `setTimeout`:**
|
||
|
||
```ts
|
||
// In useDestinationStation.ts (useGeocode.ts same pattern):
|
||
useEffect(() => {
|
||
if (!destination.trim()) return;
|
||
let isMounted = true;
|
||
let abortController: AbortController | null = null;
|
||
|
||
const timeoutId = setTimeout(async () => {
|
||
setLoading(true);
|
||
setError(null);
|
||
|
||
abortController = new AbortController();
|
||
|
||
try {
|
||
// ... existing fetch logic, pass { signal: abortController.signal } ...
|
||
} catch (err) {
|
||
if (err instanceof DOMException && err.name === "AbortError") return;
|
||
if (!isMounted) return;
|
||
setError(err instanceof Error ? err.message : "Station lookup failed");
|
||
setLoading(false);
|
||
}
|
||
}, 400);
|
||
|
||
return () => {
|
||
isMounted = false;
|
||
clearTimeout(timeoutId);
|
||
abortController?.abort();
|
||
};
|
||
}, [destination]);
|
||
```
|
||
|
||
**400 ms** is the sweet spot: fast enough to feel responsive, slow enough to cancel in-flight requests from partial input.
|
||
|
||
---
|
||
|
||
#### Step 9: Pre-Group Calendar Events by Date (~20 min)
|
||
|
||
**File:** `src/app/calendar/CalendarView.tsx`
|
||
|
||
**Problem:** `renderCells` filters the entire events array for each day cell (~31-42 cells). With N events and M cells, that's O(N×M) `new Date()` allocations and comparisons per render.
|
||
|
||
**Fix — build a Map once with `useMemo`:**
|
||
|
||
```ts
|
||
// Inside CalendarView component, before renderCells:
|
||
const eventsByDate = React.useMemo(() => {
|
||
const map = new Map<string, Event[]>();
|
||
for (const event of events) {
|
||
const key = new Date(event.eventTime).toISOString().slice(0, 10); // "YYYY-MM-DD"
|
||
const existing = map.get(key);
|
||
if (existing) {
|
||
existing.push(event);
|
||
} else {
|
||
map.set(key, [event]);
|
||
}
|
||
}
|
||
return map;
|
||
}, [events]);
|
||
|
||
// Inside renderCells, replace the filter:
|
||
const dayKey = day.toISOString().slice(0, 10);
|
||
const dayEvents = eventsByDate.get(dayKey) ?? [];
|
||
```
|
||
|
||
This reduces per-cell cost from O(N) to O(1) and eliminates redundant `new Date()` construction.
|
||
|
||
---
|
||
|
||
#### Step 10: Add Dark Mode Toggle (~20 min)
|
||
|
||
**Files:** `src/hooks/useTheme.ts` (new), `src/app/layout/Header.tsx`
|
||
|
||
**Create `src/hooks/useTheme.ts`:**
|
||
|
||
```ts
|
||
"use client";
|
||
|
||
import { useState, useEffect, useCallback } from "react";
|
||
|
||
const THEME_KEY = "ttl_theme";
|
||
|
||
function getSystemTheme(): "dark" | "light" {
|
||
if (typeof window === "undefined") return "light";
|
||
return window.matchMedia("(prefers-color-scheme: dark)").matches ? "dark" : "light";
|
||
}
|
||
|
||
export function useTheme() {
|
||
const [dark, setDark] = useState(false);
|
||
|
||
useEffect(() => {
|
||
const stored = localStorage.getItem(THEME_KEY);
|
||
const theme = stored ?? getSystemTheme();
|
||
const isDark = theme === "dark";
|
||
setDark(isDark);
|
||
document.documentElement.classList.toggle("dark", isDark);
|
||
}, []);
|
||
|
||
const toggle = useCallback(() => {
|
||
setDark((prev) => {
|
||
const next = !prev;
|
||
localStorage.setItem(THEME_KEY, next ? "dark" : "light");
|
||
document.documentElement.classList.toggle("dark", next);
|
||
return next;
|
||
});
|
||
}, []);
|
||
|
||
return { dark, toggle };
|
||
}
|
||
```
|
||
|
||
**Add to `Header.tsx`:**
|
||
|
||
```tsx
|
||
import { useTheme } from "@/hooks/useTheme";
|
||
|
||
const { dark, toggle } = useTheme();
|
||
|
||
// In the header JSX, next to the Add Event button:
|
||
<button
|
||
onClick={toggle}
|
||
aria-label="Toggle dark mode"
|
||
className="p-2 rounded-md hover:bg-gray-100 dark:hover:bg-gray-700"
|
||
>
|
||
{dark ? "☀️" : "🌙"}
|
||
</button>
|
||
```
|
||
|
||
---
|
||
|
||
#### Step 11: Fix Bike Route Steps (~5 min)
|
||
|
||
**File:** `src/lib/bike-routing-client.ts`
|
||
|
||
**Problem:** The `BikeRoutingClient` doesn't pass `steps=true` to OSRM, so the steps array is always empty.
|
||
|
||
**Add `steps` parameter to the query in `getBikeRoute`:**
|
||
|
||
```ts
|
||
const res = await this.client.get<OsrmResponse>(
|
||
path,
|
||
{ overview: "false", steps: "true" }, // ← add steps: "true"
|
||
{ cacheKey, ttl: this.defaultTtlMs },
|
||
);
|
||
```
|
||
|
||
---
|
||
|
||
### Phase 4 — Monitoring & Testing (~1 hour)
|
||
|
||
---
|
||
|
||
#### Step 12: Add Correlation IDs to API Errors (~15 min)
|
||
|
||
**Files:** All `src/app/api/*/route.ts`
|
||
|
||
**Add to each route's error handler:**
|
||
|
||
```ts
|
||
import { randomUUID } from "crypto";
|
||
|
||
// In catch blocks:
|
||
} catch (error) {
|
||
const corrId = randomUUID().slice(0, 8);
|
||
console.error(`[${corrId}] API error:`, error);
|
||
return NextResponse.json(
|
||
{ error: "Internal server error", correlationId: corrId },
|
||
{ status: 500 },
|
||
);
|
||
}
|
||
```
|
||
|
||
This lets the frontend display the correlation ID to users so they can report it when filing bugs.
|
||
|
||
---
|
||
|
||
#### Step 13: Add Hook Tests (~30 min)
|
||
|
||
**Files:** `src/hooks/__tests__/useJourneys.test.ts`, `src/hooks/__tests__/useBikeRoute.test.ts`
|
||
|
||
**Pattern — mock `global.fetch` and test state transitions:**
|
||
|
||
```ts
|
||
// src/hooks/__tests__/useJourneys.test.ts
|
||
import { renderHook, waitFor } from "@testing-library/react";
|
||
import { useJourneys } from "../useJourneys";
|
||
|
||
beforeEach(() => {
|
||
vi.restoreAllMocks();
|
||
});
|
||
|
||
it("returns journeys when API succeeds", async () => {
|
||
vi.spyOn(globalThis, "fetch").mockResolvedValue({
|
||
ok: true,
|
||
json: async () => ({
|
||
svcResL: [{
|
||
res: {
|
||
outConL: [{
|
||
ctxRecon: "test-journey",
|
||
secL: [
|
||
{ dep: { dTimeS: "20250101120000", dTimeR: "20250101120000" } },
|
||
{ arr: { aTimeS: "20250101130000", aTimeR: "20250101130000" } },
|
||
],
|
||
}],
|
||
},
|
||
}],
|
||
}),
|
||
});
|
||
|
||
const { result } = renderHook(() =>
|
||
useJourneys("0WB0F0000600", "0WB0F0001500", new Date("2025-01-01T12:00:00"), 0)
|
||
);
|
||
|
||
await waitFor(() => expect(result.current.loading).toBe(false));
|
||
expect(result.current.journeys).toHaveLength(1);
|
||
expect(result.current.journeys[0].id).toBe("test-journey");
|
||
});
|
||
|
||
it("sets error when API fails", async () => {
|
||
vi.spyOn(globalThis, "fetch").mockResolvedValue({
|
||
ok: false,
|
||
status: 500,
|
||
});
|
||
|
||
const { result } = renderHook(() =>
|
||
useJourneys("ext1", "ext2", new Date(), 0)
|
||
);
|
||
|
||
await waitFor(() => expect(result.current.error).toBeTruthy());
|
||
});
|
||
```
|
||
|
||
---
|
||
|
||
#### Step 14: Add Component Tests (~15 min)
|
||
|
||
**Files:** `src/app/event/__tests__/EventCard.test.tsx`, `src/app/calendar/__tests__/CalendarView.test.tsx`
|
||
|
||
**Pattern — render with mock data, verify key elements:**
|
||
|
||
```tsx
|
||
// src/app/event/__tests__/EventCard.test.tsx
|
||
import { render, screen } from "@testing-library/react";
|
||
import EventCard from "../EventCard";
|
||
|
||
const mockEvent = {
|
||
id: "test-1",
|
||
title: "Team Meeting",
|
||
destination: "Wien Hbf",
|
||
eventTime: new Date("2025-12-01T14:00:00"),
|
||
source: "manual" as const,
|
||
};
|
||
|
||
const mockStation = { name: "Graz Hbf", extId: "0WB0F0000600" };
|
||
|
||
it("renders event title and destination", () => {
|
||
render(<EventCard event={mockEvent} originStation={mockStation} />);
|
||
expect(screen.getByText("Team Meeting")).toBeInTheDocument();
|
||
expect(screen.getByText("Wien Hbf")).toBeInTheDocument();
|
||
});
|
||
```
|
||
|
||
---
|
||
|
||
## 4. Dependencies
|
||
|
||
No new dependencies required. All changes use existing packages:
|
||
|
||
| Package | Used For |
|
||
|---------|----------|
|
||
| `next` | API routes, SSR, App Router |
|
||
| `react` / `react-dom` | Components, hooks |
|
||
| `date-fns` | Calendar date manipulation |
|
||
| `node-ical` | ICS parsing |
|
||
| `vitest` + `@testing-library/react` | Tests |
|
||
| `@testing-library/jest-dom` | Test matchers |
|
||
|
||
---
|
||
|
||
## 5. Estimated Effort
|
||
|
||
| Phase | Time | Steps |
|
||
|-------|------|-------|
|
||
| 1 — Unblock Runtime | ~45 min | 3 (SSR fix, calendar wiring, HAFAS validation) |
|
||
| 2 — Deduplicate Code | ~2 hours | 4 (HAFAS dedup, wire clients, dead code, test setup) |
|
||
| 3 — Performance & UX | ~1.5 hours | 4 (debounce, calendar perf, dark mode, bike steps) |
|
||
| 4 — Monitoring & Testing | ~1 hour | 3 (correlation IDs, hook tests, component tests) |
|
||
| **Total** | **~5 hours** | **14 steps** |
|