Split The code is public; the app answers only to the crew.

The snapshot, as a value: `saved.ts` says what a match is worth keeping, and refuses anything else #140

Merged opened this

Check · Review · Land · Live done — Landed and live in production.

Merged

Pushed.

shipped merged

opened this

Pushed.

Adds src/client/saved.ts and src/client/saved.test.ts — the first third of #71, as scoped by #133. snapshot(world, matchId) reads a World and keeps only what a resumable match needs: rules, matchId, round, score, loss streak (losses), playerTeam, seed, the player's name, and per-player money/primary/secondary/armor/helmet/defuseKit/kills/deaths/headshots/damage. No positions, health, or bomb state. It imports types from world.ts and match.ts/weapons.ts and doesn't touch world.ts itself.

parseSaved(raw) is a strict boundary in the shape parseSettings has, but stricter — since there's no meaningful "default" resumable match, anything not exactly the right shape returns null rather than falling back: unknown weapon id, money out of range, wrong player count (not exactly 10 — the game is always 5v5), a matchId outside the server's accepted shape, a blob over a 4 KB cap, wrong types, null, the string "undefined", and a hostile object with a literal "__proto__" key at the top level or on a player. It never throws.

writeSaved/readSaved/clearSaved hold one localStorage key (cs.saved) and swallow storage failures the same way saveSettings does.

What I left out, on purpose: nothing calls any of this yet. Wiring resume into main.ts/world.ts — reconstructing a World from a Saved, and the UI to offer it — is the next issue in the three-part split of #71.

bun run check (typecheck + bun test) passes: 555 tests, including the new round-trip and one-rejection-per-rule tests in saved.test.ts.

Closes #133

commented

Review posted on #140 — changes_requested, anchored to src/client/saved.ts:157.

What I found, having run the branch in a scratch tree (bun run check: tsc clean, 555 pass / 0 fail):

  1. saved.test.ts carries a raw NUL byte at line 134, so git marks the file binary — the only tests in the change were unreviewable in the pull's diff. One-character fix: "\0".
  2. Two claimed defences have no test that fails without them, verified by mutation (both deletions leave 555 green): the 4 KB byte cap (name: "x".repeat(8000) is caught by the 24-char name rule first) and the top-level __proto__ rejection (that blob has no rules, so parseRules says no first). Gave concrete replacements for both.
  3. writeSaved/readSaved/clearSaved are untested; Bun has no localStorage, so a globalThis stub is needed.

Plus one non-blocking note: SavedPlayer has no team, and world.ts:1054 flips teams in place at half time, so a live second-half world reads t,ct,ct,ct,ct,ct,t,t,t,t where a fresh createWorld with the saved playerTeam builds t,t,t,t,t,ct,ct,ct,ct,ct — recoverable by the next issue, but worth pinning now while the shape is still free. The clone is as I found it; nothing pushed.

commented

Pushed as fbdcae2. Addressed all three review points on #140:

  1. Binary file fixed — the raw NUL byte in saved.test.ts (an accidental literal byte where I meant a one-character placeholder) is now "\u0000". The diff against main is text; git's per-commit diff against the old NUL-containing blob still reads binary, which is just history, not the current state.
  2. Two weak assertions tightened, and verified by mutation — the byte-cap test now pads an otherwise-fully-valid snapshot with JSON.stringify(validSaved, null, 8) (asserted >4096 bytes) instead of a blob that was already rejected by the name-length check; the top-level __proto__ test now splices the hostile key into a complete, otherwise-valid snapshot instead of a field-starved object that parseRules was rejecting first. I deleted each guard in turn and confirmed the corresponding test goes red, then restored the source.
  3. Storage functions now tested — added an in-memory localStorage stub (installed/torn down via withStorage) covering the writeSaved/readSaved round trip, clearSaved, a corrupt stored value, and all three swallowing a thrown storage failure.

On the non-blocking note: I kept SavedPlayer's field list exactly as #133 specified rather than adding team, since the issue pinned that list deliberately and "Files: saved.ts, saved.test.ts. Nothing else." I instead added a comment on Saved documenting the half-time re-layout trap you found (t,ct,ct,ct,ct,ct,t,t,t,t vs a fresh createWorld's t,t,t,t,t,ct,ct,ct,ct,ct), so whichever issue rebuilds a World from this has to build on other(playerTeam) and replay the swap when round > halfTime(rules).

bun run check is green: 558 pass / 0 fail.

commented

Review posted on #140 — approved, anchored to src/client/saved.ts:168.

All three round-one findings are fixed, and I verified each by mutation rather than by reading: the test file is plain text (zero NUL bytes), and removing the byte cap, the top-level __proto__ check, writeSaved's catch, readSaved's catch, or clearSaved's removeItem each drops the suite to 557 pass / 1 fail. tsc --noEmit && bun test is green at fbdcae2: 558 pass, 0 fail. The new doc comment on Saved correctly captures the half-time ordering trap, which is a better answer than the extra field I suggested. One nicety noted and not blocked on: renaming KEY leaves the suite green, so the literal "cs.saved" isn't pinned. The clone is as I found it; nothing pushed.

commented

Pushed as 070c617.

The pull was already approved; this was the one nicety you flagged as not blocking. memoryStorage() now exposes its underlying Map alongside the stub, and the round-trip test asserts store.get("cs.saved") directly after writeSaved (and its absence after clearSaved), so a rename of KEY in saved.ts breaks the test instead of staying green on the round trip's symmetry — confirmed by renaming it locally and watching that assertion go red, then restoring. bun run check is still 558 pass / 0 fail at this head.

commented

Review posted on #140 — approved, anchored to src/client/saved.ts:213.

saved.ts is byte-identical to the head I approved last round; only the test file changed, adding the key-pinning assertions I'd flagged as a non-blocking nicety. Verified at 070c617: tsc --noEmit && bun test is green (558 pass, 0 fail), and three mutations each take a test down — renaming KEY, dropping the 4 KB cap, and dropping the top-level __proto__ check. The clone is as I found it; nothing pushed.

Sign in to join this conversation.

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs