Jesse.MarkowitzandClaude Opus 5.5
8a29de54ee
The static site on the File Browser host was retired with the 0.4.9 line on 2026-09-29. README, TODO and the structure plan no longer describe a site deploy as the release step or the zero-dependency rule as what keeps the site a static upload. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019rwKTmug58sEsJ72AuWsEi
114 lines
6.5 KiB
Markdown
114 lines
6.5 KiB
Markdown
# Structure — a proposal (2026-09-29)
|
|
|
|
What the 2026-09-29 audit found about the SHAPE of the code, as distinct from its faults, and what
|
|
to do about it. Nothing here changes behaviour; each item is a seam that would let the next fault be
|
|
found by a test instead of by a reviewer reading three thousand lines. Ordered by payoff.
|
|
|
|
The faults the audit fixed in 0.8.3 and 0.8.4 were nearly all in the three largest functions or in
|
|
the one file no test stood up. That is the argument: the structure is where the bugs were.
|
|
|
|
## 1. `http.ts` — a route table, and one place for authentication
|
|
|
|
**Today.** `startServer` is one 600-line async closure of `if (pathname === … && method === …)`
|
|
chains. The token → session → lobby-or-game resolution is copy-pasted eight times; the admin-secret
|
|
check, the join-secret check and the host check are each inline where they are needed. The catch at
|
|
the bottom is the only error path, and until 0.8.4 it was itself a crash.
|
|
|
|
**Proposed.** A table of routes, each `{ method, path, auth, handler }`, with three auth wrappers:
|
|
|
|
```
|
|
withToken(handler) // resolves { ps, lobby?, game? } from the token, 404s if none
|
|
withHost(handler) // withToken, then refuses anyone but lobby.hostToken
|
|
withAdmin(handler) // the x-admin-secret gate; the whole prefix is absent when unset
|
|
```
|
|
|
|
Handlers become ten to thirty lines each and take a typed context. The one dispatcher owns
|
|
`readJson`, `sendJson`, the `HttpError` catch and the `headersSent` guard — one place, so 0.8.4's
|
|
crash fix cannot be forgotten by the next route. The per-game `inTurn` queue becomes a property of
|
|
the game context rather than something a handler has to remember to call.
|
|
|
|
**Cost.** A day. Every route is exercised by `test/server/http.test.ts` now, so the move is
|
|
mechanical and verifiable. Do it before adding the next route (the seatless display stream, #20).
|
|
|
|
## 2. `main.ts` — five extractions, and a `Selection` value
|
|
|
|
**Today.** 3200 lines, ~21 module-level `let`s (`session`, `selected`, `mode`, `pendingAt`,
|
|
`selectedCrew`, `peekPlayer`, `zoom`, `soundOn`, `gameCode`, `remoteToken`, …), and `start()` runs at
|
|
import. `render()` is ~470 lines; `renderActions` ~360. `test/web.test.ts` has to stub the DOM before
|
|
importing, can never build two pages, and cannot call `render()` with a `Frame` of its choosing —
|
|
which is why every screen fault in the audit was found by reading, not by a test.
|
|
|
|
**Proposed, in the order they pay:**
|
|
|
|
1. **`web/remote-store.ts`** — `RemoteRecord`, `readStore`/`writeStore`, `loadRemote`/`saveRemote`/
|
|
`forgetRemote`/`knownRemote`, and `lobbyHandlers`. Pure functions over `localStorage`; testable
|
|
with a Map. Today they are untestable except through the page.
|
|
2. **`web/url-options.ts`** — `RULE_PARAMS`/`VICTORY_PARAMS`/`OPTIONAL_PARAMS`, `gameOptionsFromUrl`,
|
|
`solitaireDefaults`, `rulesToUrl`. A pure round trip; the audit found the `location.search = ''`
|
|
no-op assumption (#114) by reading this code, and a test on the round trip would have found it.
|
|
3. **`web/setup-screen.ts`** — `wireGameTypeBlock`, `commitNewGame`, `runSolitaireSetup`.
|
|
4. **A `Selection` object** replacing the five `let`s (`selected`, `mode`, `pendingAt`,
|
|
`selectedCrew`, `peekPlayer`) that every click handler mutates. One value, one `reset()`, passed
|
|
to `render` rather than read from module scope — which is what lets a test call `render(frame,
|
|
selection)` and assert on the HTML.
|
|
5. **`web/game-shell.ts`** — session routing: `beginRemote`, `abandonRemote`, `claimSeat`, `start`,
|
|
`applyCapabilities`, the handoff. This is where the animation loop outliving the session (#114)
|
|
lives, and it is easier to see once it is not surrounded by rendering.
|
|
|
|
Keep `render()` in `main.ts` but split the board-and-hand wiring (~240 lines of `addEventListener`)
|
|
into `wireBoard(selection)` so what is DRAWN and what is CLICKABLE are separate functions.
|
|
|
|
**Cost.** Two to three days across the five, each its own commit; the first two are an afternoon
|
|
each and carry no risk.
|
|
|
|
## 3. The engine — `check` per phase, and one car category
|
|
|
|
**Today.** `check` (~600 lines) and `reduce` (~700) are single switches mixing every phase;
|
|
`moveTrain` nests four position kinds by three decision kinds. Car category (coach / caboose /
|
|
freight) is computed in three places with three spellings. The seat guard 0.8.3 added to the four
|
|
switching intents is the same four lines four times.
|
|
|
|
**Proposed.**
|
|
|
|
- Split `check` by phase — `checkLocalOps`, `checkNewTrain`, `checkLoadUnload`, `checkMainline` —
|
|
each a switch over its own intents, dispatched by `inPhase`. The shared guards (`ownTray`, the
|
|
option-chosen test, the Moves test) become the first lines of `checkLocalOps` rather than four
|
|
repeats. `reduce` splits the same way.
|
|
- One exported `carCategory(type)` in `content.ts`, used by `acceptsCar`, `newTrainPhase`,
|
|
`consistNeeds` and `isFreight`/`carriesLoad`.
|
|
- `maneuver.flyingSwitch` is a weaker copy of `switch.dropCars` (no `switchingRefusal`, no
|
|
`engineAt` clamp, no `standingWest` handling). It should CALL the dropCars path with a flag,
|
|
or be deleted until the card is dealt (it is at 0 copies). Deciding is #115's job; the structure
|
|
point is that there must be one cut-dropping reducer.
|
|
|
|
**Cost.** The `check`/`reduce` split is a day and is pure motion — every test in `apply.test.ts`
|
|
and `advance.test.ts` runs unchanged. `carCategory` is an hour.
|
|
|
|
## 4. One `Push` type
|
|
|
|
`web/session.ts` mirrors `server/session.ts`'s `Push` by hand so the browser bundle never imports
|
|
from `src/server/`. Every envelope change (0.8.4 added `lastSeq`) is two edits. Move the wire types
|
|
— `Push`, `LobbyPush`, `LobbyPreview` — into `src/sim/wire.ts`, which both sides already import
|
|
from, and delete the mirror.
|
|
|
|
**Cost.** An hour.
|
|
|
|
## 5. Tests that would then exist
|
|
|
|
Each extraction above names the test it makes possible:
|
|
|
|
| after | test |
|
|
| --- | --- |
|
|
| route table | one test per route for the 404/403/400 answers, from a table |
|
|
| `remote-store.ts` | the `RemoteRecord` round trip, the "seat tied to this browser" notice (#33) |
|
|
| `url-options.ts` | `rulesToUrl(gameOptionsFromUrl(x)) === x`, and the `search = ''` case |
|
|
| `Selection` | `render(frame, selection)` snapshot tests for each screen state |
|
|
| `check` per phase | the seat guard once, as a property over every switching intent |
|
|
|
|
## What not to do
|
|
|
|
- Do not refactor `render()` and fix a screen bug in the same commit. The audit's value was that
|
|
each finding could be verified against unchanged code.
|
|
- Do not introduce a framework or a runtime dependency to do any of this. The zero-dependency rule
|
|
is what keeps the package a 63 MB `.s9pk`.
|