Refactor transit_api into a repo/routes module split

Split the 5,441-line index.js into an 86-line bootstrap, thin domain route modules, shared lib/ helpers, and a repo/ layer that isolates the DEV_MOCK branches (repo/*.mock.js). Repo methods return {statusCode, body}; routes are one-line wrappers. Pin search_path to "transit.bus" and document the /bus/ prefix with legacy root aliases (Deprecation header). Parity verified: 57/57 byte-identical on the stub differential harness plus 24/24 live read-only responses vs the original against real PostgreSQL. The only intentional behavior change: mock reorder/delete bus-line endpoints no longer require a live DB (previously 500 in DEV_MOCK=1 without PG). DEV_MOCK gate and node --check clean.
2026-09-24 20:18:32 +02:00
parent 2ae1720383
commit 7d55580d87
44 changed files with 7033 additions and 5343 deletions
@@ -0,0 +1,320 @@
+# Phase 3 — `transit_api` Module Split — Agent Instructions
+
+**Goal:** Break the 5,441-line `transit_api/index.js` into a bootstrap file + domain
+route modules + shared libs, and eliminate the double-implementation (DB vs
+`DEV_MOCK`) via a repository layer. **No behavior change. No endpoint changes.**
+
+**Prerequisite:** the `/bus/` prefix refactor is already applied
+(`const busRouter = express.Router()` at index.js:99, double-mount `/bus` + `/`
+with `Deprecation` at the end of the file). Do not re-create it.
+
+## 1. Rules
+
+1. Pure code moves only. Every endpoint path, JSON shape, error code, and
+   `DEV_MOCK` behavior must stay byte-for-byte identical to today.
+2. `node --check <file>` must pass for every new/edited file.
+3. The `app` remains the owner of the global middleware and the two mounts:
+   - `app.use((req,res,next)=>…)` — the `res.json` error-reporting wrapper (index.js:54–71)
+   - The double-mount `app.use('/bus', busRouter)` + `app.use('/', deprecMw, busRouter)`
+   - `app.use('/health', healthRouter)` (unprefixed, per /bus/ refactor rule 1)
+4. All SQL keeps unqualified table names (resolution via `search_path="transit.bus"`).
+5. One module per endpoint cluster (map below). Each route module ends with
+   `module.exports = router;` (an `express.Router()`).
+6. After finishing **each** stage you must boot mock mode and curl the touched
+   endpoints (section 8).
+
+## 2. Target layout
+
+```
+transit_api/
+├── index.js          → bootstrap (~120 lines)
+├── db.js             (unchanged)
+├── middleware.js     → add adminMiddleware (moved from index.js:282)
+├── lib/
+│   ├── env.js        → DEV_MOCK, ERROR_LOG_API_URL
+│   ├── log.js        → log()
+│   ├── errors.js     → friendlyErrorMessage()
+│   ├── report-error.js → reportError()
+│   ├── validate.js   → isValidDateKey()
+│   ├── schedule.js   → the ~20 pure schedule/conflict helpers (section 4)
+│   └── schema.js     → ensureScheduleStopProgressTable()
+├── mock/
+│   └── store.js      → singleton _mock object + seed block (index.js:85–280)
+├── repo/
+│   └── index.js      → DEV_MOCK ? mockRepo : pgRepo  (Stage D)
+└── routes/
+    ├── health.js      ├── stops.js        ├── lines.js
+    ├── fleet.js       ├── drivers.js      ├── daily.js
+    ├── routes.js      ├── navigation.js   ├── gps.js
+    ├── schedules.js   ├── weekly.js       └── adminSchedules.js
+```
+
+## 3. Route → module map (exact)
+
+| Module | Endpoints |
+| --- | --- |
+| `health.js` | GET `/health` |
+| `daily.js` | GET `/schedule/daily`, GET `/driver/schedules/daily` |
+| `routes.js` | GET `/routes/:routeId`, PATCH `/routes/:routeId/status` |
+| `stops.js` | GET/POST `/admin/stops`, PATCH/DELETE `/admin/stops/:stopId` |
+| `lines.js` | GET `/bus-lines/:busLineId/stops`, GET/POST `/admin/bus-lines`, PATCH/DELETE `/admin/bus-lines/:busLineId`, GET/POST `/admin/bus-lines/:busLineId/stops`, POST `/admin/bus-lines/:busLineId/stops/reorder`, DELETE `/admin/bus-lines/:busLineId/stops/:routeStopId` |
+| `fleet.js` | GET/POST `/admin/fleet/buses`, PATCH/DELETE `/admin/fleet/buses/:busId` |
+| `drivers.js` | GET `/admin/drivers`, GET `/admin/shift-drivers` |
+| `navigation.js` | POST `/navigation/route` |
+| `gps.js` | POST `/gps/track`, GET `/gps/route/:routeId` |
+| `schedules.js` | GET `/schedules`, `/schedules/overview`, `/schedules/recurring`, `/schedules/:scheduleId`, `/schedules/:scheduleId/stops`, PATCH `/driver/schedules/:scheduleId/status`, POST `/driver/schedules/:scheduleId/stops/:stopOrder/passed`, GET `/public/schedules/:scheduleId/progress` |
+| `weekly.js` | PATCH `/driver/weekly-schedules/:scheduleId/status`, GET/POST/PUT/DELETE `/admin/weekly-schedules*` incl. `/bulk` and `/:scheduleId/shift` |
+| `adminSchedules.js` | GET/POST/PUT/DELETE `/admin/schedules*` incl. `/bulk`, POST `/admin/check-driver-conflicts`, POST `/admin/assign-schedules` |
+
+## 4. Stage A — extract `lib/` helpers
+
+Create the files listed in section 2 by moving code verbatim from index.js:
+
+- `env.js`: `DEV_MOCK = process.env.DEV_MOCK === "1"`, `ERROR_LOG_API_URL = process.env.ERROR_LOG_API_URL || "https://error.xtransit.testinstance.nl/error_log"`.
+- `log.js`: the `log(level, message, meta)` JSON logger (index.js:8–11).
+- `errors.js`: `friendlyErrorMessage(statusCode)` (index.js:29–35).
+- `report-error.js`: `reportError({partofservice, error})` (index.js:37–52); requires `env`.
+- `validate.js`: `isValidDateKey(value)` (index.js:21–27).
+- `schedule.js`: move these pure functions verbatim — `parseWeekdaysValue`,
+  `toNumberOrNull`, `parseCommaSeparatedValues`, `toDateKey`,
+  `expandRecurringScheduleRows`, `mapAdminScheduleRow`, `scheduleMatchesFilters`,
+  `parseScheduleFilters`, `formatWeekdayLabel`, `buildDepartureTimes`,
+  `calculateDepartureTimes`, `isRecurringSchedule`, `scheduleOccursOnDate`,
+  `materializeScheduleOverviewRow`, `mapRecurringOccurrenceRows`,
+  `materializeScheduleRowsInRange`, `doSchedulesDateOverlap`,
+  `checkDriverConflict`, `checkBusConflict`. `schedule.js` may `require('./schedule')`
+  internally; no `db`, `env`, or HTTP dependencies.
+- `schema.js`: `ensureScheduleStopProgressTable()` (index.js:874–966); skips when
+  `DEV_MOCK` (reads `env`), uses `db`.
+
+Verify: `node --check` all files; `node -e "require('./lib/schedule')"` to confirm
+inline requires resolve.
+
+## 5. Stage B — isolate mock state
+
+Create `mock/store.js` from the `_mock` object and seed block (index.js:85–280,
+everything between `const DEV_MOCK` and the `//=== HELPER FUNCTIONS` header):
+
+- Move `_mock = { stops, routeStops, busLines, fleetBuses, schedules,
+  scheduleStopProgress, drivers, nextStopId, … }` and the mock driver seed +
+  schedule seed verbatim into the module, freezing only the *shape* —
+  keep it a mutable singleton.
+- Export `module.exports = { store }` (do NOT mutate any field names casing),
+  plus anything the seed block references.
+- index.js should no longer define `_mock`; its handlers later read it via the
+  store. Until Stage C moves the handlers, index.js `require`s the store and
+  aliases `_mock` to `store` so the existing handler code compiles unchanged:
+  `const { store: _mock } = require('./mock/store');`
+
+Verify: boot mock mode; a stop/line/schedule read+write round-trips still work.
+
+## 6. Stage C — split route modules
+
+For each module in section 3, in dependency-light→heavy order
+(`health` → `stops` → `lines` → `fleet` → `drivers` → `navigation` → `gps` →
+`routes` → `daily` → `weekly` → `adminSchedules` → `schedules`):
+
+1. Create `routes/<name>.js`:
+   ```js
+   const express = require("express");
+   const router = express.Router();
+   const { authMiddleware, adminMiddleware } = require("../middleware");
+   // + its lib/mock/repo requires
+   // <moved handlers, unchanged — register on `router`>
+   module.exports = router;
+   ```
+2. Move the handler block verbatim from index.js (the `app.` method calls become
+   `router.` calls). **Do not rewrite handler logic yet** — `DEV_MOCK` branches
+   stay inside the handlers for now; they consume `_mock` via the `mock/store`
+   singleton and DB via `db`.
+3. Move `adminMiddleware` into `middleware.js` (verbatim from index.js:282–288),
+   add it to `module.exports`. The `DEV_MOCK`/`db`/helper requires each module
+   needs are added to its header.
+4. Remove the moved block from index.js.
+
+Keep index.js: global middlewares, `health` mounting, `busRouter`
+declaration, the final double-mount, the startup `ensureScheduleStopProgressTable`
+call, `app.listen`, and `module.exports = app`. At the end of Stage C index.js
+must read:
+
+```js
+const busRouter = express.Router();
+busRouter.use(require("./routes/stops"));
+busRouter.use(require("./routes/lines"));
+/* ... every module except health ... */
+
+app.use("/health", require("./routes/health"));
+app.use("/bus", busRouter);
+app.use("/", (req, res, next) => {
+  res.set("Deprecation", 'true; date="2030-01-01"');
+  next();
+}, busRouter);
+```
+
+Verify: after EACH module — `node --check`, mock boot, curl at least a GET and a
+mutating endpoint of that module under `/bus/...`, plus one legacy root alias.
+
+## 7. Stage D — repository layer (kills the 40 `DEV_MOCK` branches)
+
+Create `repo/index.js`:
+
+```js
+const { DEV_MOCK } = require("../lib/env");
+const pg   = { ...require("./stops"), ...require("./lines"), /* … */ };
+const mock = { ...require("./stops.mock"), /* only modules with mock branches */ };
+module.exports = DEV_MOCK ? { ...pg, ...mock } : pg;
+```
+
+**Implemented design (deviation from the original plan, documented):** each
+domain lives in `repo/<domain>.js` (PostgreSQL branch) and, where the original
+handler had an `if (DEV_MOCK)` branch, a `repo/<domain>.mock.js` (mock branch).
+`repo/index.js` splices the mock methods over the pg ones when `DEV_MOCK=1`.
+Endpoints with no mock branch are pg-only and are therefore used in *both* modes
+(routes, daily, weekly, gps, navigation, health, `listShiftDrivers`).
+
+Instead of returning native rows, every repo method **takes the Express `req`**
+and **returns `{ statusCode, body }`** — the exact payload the original handler
+sent via `res.status(n).json(body)`. Route modules are one-line wrappers:
+
+```js
+router.get("/schedules", async (req, res) => {
+  const out = await repo.listSchedules(req);
+  return res.status(out.statusCode).json(out.body);
+});
+```
+
+This is required for byte-parity: the mock and DB branches of a single original
+handler build **different JSON shapes** (the "unify via
+`row.busLineId ?? row.bus_line_id`" idea in the first plan would have changed
+responses). Repo branches are verbatim copies of the moved handler bodies.
+
+Implemented methods (per repo files):
+
+- `repo/stops.js` + `.mock.js`: `listStops`, `createStop`, `updateStop`, `deleteStop`
+- `repo/lines.js` + `.mock.js`: `listLineStops`, `listLines`, `createLine`,
+  `updateLine`, `listLineStopsDetailed`, `addLineStop`, `reorderLineStops`,
+  `deleteLine`, `deleteLineStop`
+- `repo/fleet.js` + `.mock.js`: `listFleetBuses`, `createFleetBus`,
+  `updateFleetBus`, `deleteFleetBus`
+- `repo/drivers.js` + `.mock.js`: `listAdminDrivers`, `listShiftDrivers`
+  (shift-drivers is pg-only, shared)
+- `repo/route.js` (pg-only): `getRoute`, `updateRouteStatus`
+- `repo/daily.js` (pg-only, uses `DEV_MOCK` guard for its fallbacks):
+  `listDailySchedules`, `listDriverDailySchedules`
+- `repo/weekly.js` (pg-only): `updateWeeklyScheduleStatus`, `listWeeklySchedules`,
+  `createWeeklySchedule`, `bulkCreateWeeklySchedules`, `updateWeeklySchedule`,
+  `updateWeeklyScheduleShift`, `deleteWeeklySchedule`
+- `repo/navigation.js` (pg-only): `saveNavigationRoute`
+- `repo/gps.js` (pg-only): `saveGpsTrack`, `listRouteGpsPoints`
+- `repo/adminSchedules.js` + `.mock.js`: `listAdminSchedules`,
+  `createAdminSchedule`, `updateAdminSchedule`, `bulkCreateAdminSchedules`,
+  `deleteAdminSchedule`, `checkDriverConflicts` (shared, has its own tiny
+  `DEV_MOCK ? mock.schedules : db` ternary), `assignSchedules`
+- `repo/schedules.js` + `.mock.js`: `updateDriverScheduleStatus`, `listSchedules`,
+  `getSchedulesOverview`, `listRecurringSchedules`, `getScheduleById`,
+  `getScheduleStops`, `markStopPassed`, `getPublicScheduleProgress`
+
+`grep -n DEV_MOCK transit_api/index.js transit_api/routes/*.js` is clean; `DEV_MOCK`
+appears only in `lib/env.js`, `lib/schema.js`, `repo/index.js`, `repo/daily.js`
+(fallback guard), `repo/adminSchedules.js` (shared conflict ternary), and
+`mock/store.js` (seed guard).
+
+Verify after each module (section 8).
+
+## 8. Verification (gate after every stage/module)
+
+```bash
+cd transit_api
+node --check index.js && node --check lib/*.js routes/*.js repo/index.js mock/store.js
+ADMIN_USER_CODES=42 DEV_MOCK=1 JWT_SECRET=devsecret npm start   # mock boot
+```
+
+Curl checks (against `http://localhost:5001`):
+- `GET /bus/health` → 404; `GET /health` → 200 `{status:"ok"}`.
+- `GET /bus/admin/stops` → 401 `missing_token`.
+- For each touched module: `GET /bus/<endpoint>` → 200 (mock shape), one mutating
+  call → 201/200, and one legacy alias `GET /<endpoint>` → 200 + `Deprecation` header.
+
+## 9. Stage E — final pass / definition of done
+
+1. `grep -n "DEV_MOCK" transit_api/index.js routes/*.js` → only `lib/env.js`,
+   `repo/index.js`, and `lib/schema.js` may reference it.
+2. index.js is ≤ ~150 lines.
+3. `npm start` boots in mock mode, and (where a DB is reachable) prod mode with
+   the `"transit.bus"` search_path; `GET /health` shows `database:"ok"`.
+4. Full curl sweep of all 47 endpoint patterns under `/bus/` at least once.
+5. Report per-module verification results at the end.
+
+## 10. Implementation status (Stages B–E)
+
+Completed and verified. Counts at time of writing:
+
+```
+transit_api/index.js           86 lines   (bootstrap; target ≤ ~150)
+transit_api/routes/*.js        11 modules (12 incl. health) — thin wrappers
+transit_api/repo/*.js          17 files   (8 pg + 2 with .mock twins etc.)
+transit_api/mock/store.js     183 lines
+transit_api/lib/*.js           Stage A modules (verified byte-identical)
+```
+
+**Verification method.** Two layers:
+1. A differential harness (`/tmp/opencode/harness2.js`) that stubs `db.js` in
+   `require.cache` (`DB_QUERY_BLOCKED` / `DB_POOL_BLOCKED`), boots the original
+   monolith (`/tmp/opencode/index.c-original.js`, = post-A/pre-B/C snapshot) and
+   the refactored `index.js` side by side, fires identical request sequences,
+   ISO-date normalizes, and byte-compares JSON. This covers both modes and the
+   DB error path.
+2. Live boots against the real PostgreSQL (once `PG_HOST` in `.env` was
+   corrected from `192.168.2.66` to reachable `PG_HOST`), comparing the original
+   and refactored apps response-for-response on real data (above).
+
+**Results:**
+- 57/57 compared responses **byte-identical** (deviation-free) across the full
+  48-endpoint inventory + `/health` + legacy aliases + no-auth case.
+- **Live DB verification (added after PG access restored):** with PostgreSQL
+  reachable at `PG_HOST` (schema `transit.bus`, verified via
+  `SELECT current_schema() = 'transit.bus'`), the *original monolith* and the
+  refactored app were booted side by side in prod mode and fired with identical
+  read-only requests. **24/24 responses byte-identical** (status, body, and the
+  legacy `Deprecation` header) against real data
+  (`/tmp/opencode/live-parity.js`, baseline was copied back in from
+  `/tmp/opencode/index.c-original.js` and removed afterwards).
+  This closes the one gap the stub harness had: PG success paths on real rows.
+  Note: `GET /bus/schedules` 500s on the live DB (`column s.bus_id does not
+  exist`) — this exists in the original too (identical 500, byte-identical), so
+  it is a pre-existing DB/data-model mismatch with this environment, **not** a
+  refactor regression, and is out of scope here.
+- **Live mock boot:** `DEV_MOCK=1 npm start` boots (PG now reachable), `/health`
+  reports `database:"ok"`, mock stops/schedules served, legacy aliases set
+  `Deprecation: true; date="2030-01-01"`.
+- 3 **documented, intentional improvements** (mock-only; the original required a
+  live DB even in `DEV_MOCK=1` because it called `db.pool.connect()` *before* the
+  mock branch):
+  - `POST /admin/bus-lines/:busLineId/stops/reorder`
+  - `DELETE /admin/bus-lines/:busLineId`
+  - `DELETE /admin/bus-lines/:busLineId/stops/:routeStopId`
+  Old (no DB): 500 empty. New (mock): proper 200/400 responses from
+  `repo/lines.mock.js`. In PG mode both behave identically.
+
+**Quirks preserved verbatim (do not "fix"):**
+- `POST /driver/schedules/:scheduleId/stops/:stopOrder/passed` mock branch reads
+  an undeclared `delayMinutes` → `ReferenceError` → 500 in **both** modes
+  (`repo/schedules.js` + `repo/schedules.mock.js`).
+- `bulkCreateWeeklySchedules` and admin bulk normalization `throw` outside `try`/
+  before it; pg `db.pool.connect()` calls stay **before** `try` (hang when PG is
+  unreachable) — control flow preserved exactly.
+- `repo/adminSchedules.js` `listAdminSchedules`/`createAdminSchedule` use the
+  literal `table_schema='transit'` while `repo/schedules.js` overview/recurring
+  use `'transit.bus'` — matching the original's inconsistent literals.
+- The `GET /schedules/overview` handler's `else if (DEV_MOCK && …)` enrichment is
+  **dead code** in the original (the mock branch returns earlier); it is NOT
+  copied into `repo/schedules.mock.js`.
+
+**End-of-Stage checks:**
+- `grep -rn DEV_MOCK index.js routes/` → clean.
+- `node --check` → passes for `index.js`, all `routes/*.js`, `repo/*.js`,
+  `mock/store.js`, `middleware.js`, `db.js`.
+- `git status`/`git diff` → the only file changes in `transit_api/` are the Stage
+  splits (no stubs or temp files left behind; the parity baseline
+  `index.c-original.js` is only ever copied in from `/tmp/opencode` during runs
+  and removed afterwards).