docs: separate REWORK_PLAN (infra/milestones) from TODO (bugs)

REWORK_PLAN.md: backend rework plan only. M0-M3 done, M4 = dead-not-skipped
(the one feature request), M5 docker, M6 undo, M7 e2e. Removed bug-stuff
and hallucinated JUMP_TURN_TO. Server = generic KV doc store.

TODO.md: bugs only. BUG-1/2 resolved, BUG-4/5/6/7/8/9 confirmed. No
milestones, no features. Pipeline stripped to bugs.

M4 (dead-not-skipped) lives in REWORK_PLAN only.
This commit is contained in:
david raistrick
2026-06-30 16:26:10 -04:00
parent fb7fbb2263
commit 260fb314bc
2 changed files with 63 additions and 99 deletions
+3 -33
View File
@@ -1,33 +1,7 @@
# TODO # TODO
## Milestone M4 — Initiative rotation bugs + features Confirmed bugs found via testing. M4 (dead-not-skipped) lives in
REWORK_PLAN.md — not here. This file = bugs only.
Split: bug (rotation corruption) vs feature (dead-participant handling).
### BUG-5: Initiative skip (mid-round add/revive corrupts rotation)
- **Real bug.** Rotation corrupts when participant added/revived mid-round.
- Test: `shared/tests/turn.combat.test.js` (jest, seeded, RED).
- 13 dupes / 100 rounds. First at round 4 (Cleric twice).
- Root cause: `computeTurnOrderAfterAddition` appends id to turnOrderIds
end. Round wrap re-sorts by initiative. `currentTurnParticipantId` pointer
stale → nextTurn revisits.
- See full detail below in Confirmed bugs section.
### FEAT-1: Dead participants should stay in turn order (as-designed→change)
- **Feature request, not bug.** Current behavior is as-designed (dead =
inactive = skipped). User wants change: dead occupy initiative slot,
PCs get death-save turn.
- Saw Saturday game.
- Desired:
- dead PC not removed from turnOrderIds
- dead PC turn still comes up (nextTurn visits them)
- dead PC on their turn can deathSave
- dead PC not auto-set isActive=false by applyHpChange
- Affects: `shared/turn.js` `nextTurn` (filters `isActive`), `applyHpChange`
(sets isActive=false on death), `computeTurnOrderAfterRemoval`.
- Characterization tests (`src/tests/Combat.characterization.test.js`) lock
CURRENT behavior — UPDATE to desired when implementing.
- RED test locked (desired state): `shared/tests/turn.dead-skip.test.js` (4 tests).
## Confirmed bugs (tests written, NOT fixed) ## Confirmed bugs (tests written, NOT fixed)
@@ -114,12 +88,8 @@ Split: bug (rotation corruption) vs feature (dead-participant handling).
- Display frozen until full reload. - Display frozen until full reload.
- Fix: `onclose` → reconnect + re-subscribe existing paths. - Fix: `onclose` → reconnect + re-subscribe existing paths.
## Pipeline ## Pipeline (bugs only — milestones live in REWORK_PLAN.md)
- [ ] BUG-4: fix setDoc→updateDoc for all 5 activeDisplay sites - [ ] BUG-4: fix setDoc→updateDoc for all 5 activeDisplay sites
- [ ] BUG-5: fix computeTurnOrderAfterAddition currentTurn re-anchor - [ ] BUG-5: fix computeTurnOrderAfterAddition currentTurn re-anchor
- [ ] BUG-6: reorderParticipants update turnOrderIds - [ ] BUG-6: reorderParticipants update turnOrderIds
- [ ] BUG-8: ws adapter reconnect - [ ] BUG-8: ws adapter reconnect
- [ ] FEAT-1: dead participants stay in turn order (update characterization)
- [ ] M5 docker-compose
- [ ] M6 undo rework (transactional events table)
- [ ] M7 Playwright E2E
+60 -66
View File
@@ -9,16 +9,16 @@ Upstream: `code.draft13.com/robert/ttrpg-initiative-tracker` (friend's Gitea)
## Goals ## Goals
1. **Replace Firebase with self-hosted backend.** Browser cannot own a DB file (sandbox). Cross-device (DM + tablet + player view) requires a real backend. Backend is the foundation, built first. 1. **Replace Firebase with self-hosted backend.** Browser cannot own a DB file (sandbox). Cross-device (DM + tablet + player view) requires a real backend. Backend is the foundation, built first.
2. **Automated test ecosystem as the baseline.** Lock current behavior before changing it. Skip bug must become provably impossible to reintroduce. 2. **Automated test ecosystem as the baseline.** Lock current behavior before changing it.
3. **Remain mergeable upstream.** Default behavior (Firebase) preserved behind flag. Upstream `main` stays clean. Friend keeps Firebase path. 3. **Remain mergeable upstream.** Default behavior (Firebase) preserved behind flag. Upstream `main` stays clean. Friend keeps Firebase path.
4. **Self-hostable in local Docker** (in-house network). Public exposure = future, only after auth + multiuser safety. 4. **Self-hostable in local Docker** (in-house network). Public exposure = future, only after auth + multiuser safety.
## Non-Goals (this plan) ## Non-Goals (this plan)
- Changing user-visible functionality beyond the documented bug fixes (skip, manual turn override).
- Ripping Firebase. Kept as default adapter upstream. - Ripping Firebase. Kept as default adapter upstream.
- Public/multiuser deployment. Deferred. - Public/multiuser deployment. Deferred.
- Rewriting the entire 2935-line `App.js`. Only extract what testability demands. - Rewriting the entire 2935-line `App.js`. Only extract what testability demands.
- Feature/bug work. That lives in `TODO.md`. This plan = infra + backend + test harness only.
--- ---
@@ -34,15 +34,6 @@ Upstream: `code.draft13.com/robert/ttrpg-initiative-tracker` (friend's Gitea)
### Why a backend is mandatory ### Why a backend is mandatory
Browser sandbox cannot write the filesystem. No sqlite file, no `/data/db.sqlite`, nothing. Browser JS is blocked from disk by design. Therefore cross-device storage (DM ↔ tablet ↔ player view) requires a separate Node process owning the DB file and serving the browser over HTTP/WebSocket. There is no browser-only path. **The backend is step one, not deferred.** Browser sandbox cannot write the filesystem. No sqlite file, no `/data/db.sqlite`, nothing. Browser JS is blocked from disk by design. Therefore cross-device storage (DM ↔ tablet ↔ player view) requires a separate Node process owning the DB file and serving the browser over HTTP/WebSocket. There is no browser-only path. **The backend is step one, not deferred.**
### Known bug: initiative skips / lost state
Two failure classes observed:
1. **Race / data loss.** Every turn mutation = client reads snapshot → computes → writes whole doc back. Two interleaved actions → last-write-wins → state lost → skip. Firebase gives eventual snapshots, no transactions. Even single-user bites via optimistic UI vs server round-trip.
2. **Logic drift.** `turnOrderIds` array vs `participants` array vs `isActive` filter drift across mid-combat add/remove/toggle. `currentIndex === -1` fallback path is fragile. No invariant enforced. No way for DM to manually say "this participant's turn now."
### Undo is fragile
Current undo = stale snapshot write-back. Interleaved undos = data loss. Suspected already bitten during live game.
--- ---
## Architecture ## Architecture
@@ -59,9 +50,8 @@ Postgres deferred until public multiuser exposure is real. SQLite schema ports e
### Backend design ### Backend design
- Owns SQLite file. Only writer. - Owns SQLite file. Only writer.
- Holds authoritative state. Turn logic (initiative order, next-turn, add/remove mid-combat) runs server-side inside SQLite transaction. - Holds authoritative state.
- Client sends **action** (e.g. `NEXT_TURN`, not the resulting state). Server computes result, persists, broadcasts diff. - Generic KV doc store (firebase mirror): single `docs` table (path PK, parent, data JSON, updated_at). Opaque JSON at arbitrary path strings. No shape-specific endpoints. App logic stays client-side.
- Kills last-write-wins races by construction.
- WS broadcast on every state change → all connected clients (DM view, player display, tablet) update instantly. - WS broadcast on every state change → all connected clients (DM view, player display, tablet) update instantly.
### Three storage impls, one interface (frontend) ### Three storage impls, one interface (frontend)
@@ -80,7 +70,7 @@ The storage interface is the test seam and the upstream-compat layer.
- `subscribeDoc(path, cb)` / `subscribeCollection(path, cb)` → real-time push - `subscribeDoc(path, cb)` / `subscribeCollection(path, cb)` → real-time push
Firebase impl: existing `onSnapshot` + SDK calls, moved verbatim behind interface (M2). Firebase impl: existing `onSnapshot` + SDK calls, moved verbatim behind interface (M2).
WS impl: thin client; dispatches **actions** to backend, receives **state updates** via WS subscribe (M2). WS impl: thin adapter; generic KV ops, receives **state updates** via WS subscribe (M2).
Memory impl: in-memory Map + EventEmitter, for tests (M3). Memory impl: in-memory Map + EventEmitter, for tests (M3).
### Repo layout (npm workspaces) ### Repo layout (npm workspaces)
@@ -94,20 +84,23 @@ Memory impl: in-memory Map + EventEmitter, for tests (M3).
firebase.js # extracted from current App.js (verbatim) firebase.js # extracted from current App.js (verbatim)
ws.js # NEW — talks to backend ws.js # NEW — talks to backend
memory.js # NEW — test only memory.js # NEW — test only
types.js # interface contract (JSDoc) contract.js # interface spec (runStorageContract)
tests/ # frontend tests
server/ # NEW server/ # NEW
index.js # Express + ws bootstrap, generic KV REST index.js # Express + ws bootstrap, generic KV REST
db.js # better-sqlite3, docs table (KV), broadcast db.js # better-sqlite3, docs table (KV), broadcast
ws-contract.test.js # adapter vs live backend (Layer 2 test) handlers.js # REST handlers
tests/ # adapter vs live backend (Layer 2 test)
shared/ # pure logic, no I/O, importable by client + server + tests shared/ # pure logic, no I/O, importable by client + server + tests
turn.js # turn logic (single source; server imports, tests import) turn.js # turn logic (single source; tests import)
types.js tests/ # turn logic unit tests (characterization + desired)
shared.test/ # turn logic unit tests (characterization + desired) data/ # gitignored sqlite DB
turn.characterization.test.js
turn.desired.test.js
docker-compose.yml # NEW — M5 docker-compose.yml # NEW — M5
docs/ docs/
REWORK_PLAN.md # this file REWORK_PLAN.md # this file
DEVELOPMENT.md
GLOSSARY.md
TODO.md # bugs + features (separate from this plan)
``` ```
### Auth ### Auth
@@ -125,8 +118,8 @@ Each milestone = independently mergeable PR upstream (unless marked ❌).
| 0 | repo, branch, remotes | no | | 0 | repo, branch, remotes | no |
| 1 | build backend (Node+Express+ws+better-sqlite3) | unit tests as built | | 1 | build backend (Node+Express+ws+better-sqlite3) | unit tests as built |
| 2 | frontend WS adapter — app runs vs backend, cross-device works | yes | | 2 | frontend WS adapter — app runs vs backend, cross-device works | yes |
| 3 | characterization tests lock current behavior (skip bug included) | yes | | 3 | characterization tests lock current behavior | yes |
| 4 | skip fix + manual override, regression-protected | yes | | 4 | dead participants not skipped (M4 below) | yes |
| 5 | docker compose in-house | smoke | | 5 | docker compose in-house | smoke |
| 6 | undo rework (tx events) | unit | | 6 | undo rework (tx events) | unit |
| 7 | playwright multi-window e2e (deferred) | e2e | | 7 | playwright multi-window e2e (deferred) | e2e |
@@ -137,48 +130,48 @@ Each milestone = independently mergeable PR upstream (unless marked ❌).
- Push origin = `keen99/ttrpg-initiative-tracker` (private). - Push origin = `keen99/ttrpg-initiative-tracker` (private).
- npm workspaces root config. - npm workspaces root config.
- Commit this plan. - Commit this plan.
- **Exit criteria:** clean branch, plan committed, remotes set. ✅ DONE (commit ad7979d, then plan-restored). - **Exit criteria:** clean branch, plan committed, remotes set. ✅ DONE.
- **Upstream-PRable:** n/a (fork infra) - **Upstream-PRable:** n/a (fork infra)
### Milestone 1 — Build backend ### Milestone 1 — Build backend
- `server/`: Express + ws + better-sqlite3. - `server/`: Express + ws + better-sqlite3.
- Schema mirrors current Firestore doc tree (campaigns, encounters subcoll, activeDisplay, logs). - Generic KV doc store (firebase mirror): `docs` table (path PK, parent, data JSON, updated_at). REST: GET/PUT/PATCH/DELETE `/api/doc?path=`, GET `/api/collection?path=`, POST `/api/collection`, POST `/api/batch`. WS: subscribe by path.
- Turn logic (initiative order, next-turn, add/remove mid-combat) ported from `App.js` into `server/turn.js` (pure function, server-authoritative). Port verbatim — bugs included for now. - Server holds authoritative state. No turn logic server-side (logic stays client-side in `shared/turn.js`).
- Actions dispatched to backend; server computes result, persists in SQLite tx, broadcasts via WS. - **Exit criteria:** backend boots, serves state over WS, persists to SQLite, unit tests green. ✅ DONE.
- Unit tests as built: turn logic unit tests (characterization capturing current behavior), plus basic API/WS smoke.
- **Exit criteria:** backend boots, serves state over WS, persists to SQLite, unit tests green.
- **Upstream-PRable:** ❌ divergence (friend stays Firebase). - **Upstream-PRable:** ❌ divergence (friend stays Firebase).
### Milestone 2 — Frontend WS adapter ### Milestone 2 — Frontend WS adapter
- Define `storage/types.js` interface. - Define `storage/contract.js` interface spec.
- Move all ~30 Firestore call sites from `App.js` into `storage/firebase.js` behind interface (verbatim). - Move all Firestore call sites from `App.js` into `storage/firebase.js` behind interface (verbatim).
- Implement `storage/ws.js` per interface, talking to backend. Dispatches actions, subscribes to WS. - Implement `storage/ws.js` per interface, talking to backend. Generic KV ops, subscribes to WS.
- Implement `storage/memory.js` for frontend unit tests. - Implement `storage/memory.js` for frontend unit tests.
- `storage/index.js` factory: `STORAGE` env → pick impl. Default `firebase` (upstream unchanged). - `storage/index.js` factory: `STORAGE` env → pick impl. Default `firebase` (upstream unchanged).
- App runs against backend with `STORAGE=ws`. - App runs against backend with `STORAGE=ws`.
- Cross-device verified manually: DM view + player display + tablet. - Cross-device verified manually: DM view + player display + tablet.
- **Exit criteria:** app runs fully against local backend, no Firebase. Multi-device sync works. - **Exit criteria:** app runs fully against local backend, no Firebase. Multi-device sync works. ✅ DONE.
- **Upstream-PRable:** ⚠️ partial. Storage interface + firebase extract = ✅. WS impl = ❌. - **Upstream-PRable:** ⚠️ partial. Storage interface + firebase extract = ✅. WS impl = ❌.
### Milestone 3 — Characterization tests lock current behavior ### Milestone 3 — Characterization tests lock current behavior
- Lock current behavior end-to-end via integration tests against running backend (turn logic now server-side). - Lock current behavior via tests.
- Capture the skip bug as a characterization test (whatever current does = locked, bugs included).
- Cover: START, NEXT_TURN, PAUSE, RESUME, ADD_PARTICIPANT, REMOVE_PARTICIPANT, TOGGLE_ACTIVE, REORDER, APPLY_DAMAGE/HEAL, DEATH_SAVE, END. - Cover: START, NEXT_TURN, PAUSE, RESUME, ADD_PARTICIPANT, REMOVE_PARTICIPANT, TOGGLE_ACTIVE, REORDER, APPLY_DAMAGE/HEAL, DEATH_SAVE, END.
- Two layers: Layer 1 (App + firebase mock, proves call shape), Layer 2 (ws adapter vs live backend, proves translation).
- Iterate until confident: baseline solid, regressions impossible to silently slip. - Iterate until confident: baseline solid, regressions impossible to silently slip.
- **Exit criteria:** characterization suite green against backend. Baseline locked. - **Exit criteria:** characterization suite green. Baseline locked. ✅ DONE.
- **Upstream-PRable:** ✅ if kept storage-agnostic (tests target turn logic shape). - **Upstream-PRable:** ✅ if kept storage-agnostic (tests target turn logic shape).
### Milestone 4 — Skip fix + manual turn override ### Milestone 4 — Dead participants stay in turn order
- Write desired-behavior tests (red): - **The one feature request:** dead (HP=0) participants must NOT be skipped.
- Never-skip invariant: after `NEXT_TURN`, current participant is always a valid active participant, or encounter cleanly ends. - Current: dead → `isActive=false` → removed from turn order → skipped.
- Mid-combat add enters turn order correctly. - Desired: dead occupy initiative slot, turn still comes up. PCs get
- Remove mid-combat doesn't skip next. death-save turn.
- Pause/resume preserves order. - Affects: `shared/turn.js` `nextTurn` (filters `isActive`), `applyHpChange`
- Fix turn logic until red tests go green. Skip bug dies. (sets isActive=false on death), `computeTurnOrderAfterRemoval`.
- Add new action: `JUMP_TURN_TO(participantId)`. DM clicks participant → cursor jumps → that participant's turn now → future `NEXT_TURN` continues from there. UI button label: "Make This Turn". - Characterization tests (`src/tests/Combat.characterization.test.js`) lock
- Regression-protected by M3 characterization + new desired tests. CURRENT behavior — UPDATE to desired when implementing.
- **Exit criteria:** skip bug gone + provably cannot regress. Manual override works. - RED test locked (desired state): `shared/tests/turn.dead-skip.test.js`.
- **Upstream-PRable:** ✅ logic fix + new feature, both beneficial. - Uses M1/M3 characterization as foundation.
- **Exit criteria:** dead participant turn comes up, RED tests go green.
- **Upstream-PRable:** ✅ beneficial feature.
### Milestone 5 — Docker compose ### Milestone 5 — Docker compose
- `docker-compose.yml`: - `docker-compose.yml`:
@@ -214,14 +207,13 @@ Each milestone = independently mergeable PR upstream (unless marked ❌).
## Testing strategy ## Testing strategy
### Layers ### Layers
1. **Turn logic unit tests** (Jest, pure functions) — every turn transition, skip invariants, manual override. Built in M1 (characterization), extended in M4 (desired). Cheap, essential. 1. **Turn logic unit tests** (Jest, pure functions, `shared/tests/`). Characterization + desired. Cheap, essential.
2. **Backend integration tests** (Jest) — spin server on random port, assert WS pushes + SQLite persists + transactional correctness. (M1+) 2. **Backend integration tests** (Jest, `server/tests/`) — spin server on random port, assert WS pushes + SQLite persists + transactional correctness.
3. **Frontend adapter contract tests** (Jest, `memory`) — impl parity against interface. (M2) 3. **Frontend adapter contract tests** (Jest, `src/tests/`) — impl parity against interface (memory). Firebase mock harness for Layer 1 App tests.
4. **Playwright multi-window E2E** — deferred. Only realtime sync glue turn logic can't reach. (M7)
### Two-pass on turn logic (M1 → M4) ### Characterization → desired
1. **Characterization** (M1/M3) — capture current behavior exactly (bugs included). Locks extraction/port as provably identical. Lets later fix be provable. 1. **Characterization** — capture current behavior exactly (bugs included). Locks extraction/port as provably identical. Lets later fix be provable.
2. **Desired-behavior (red)** (M4) — write what *should* happen. Fail today. Fix → green. Bug dies, stays dead. 2. **Desired-behavior (red)** — write what *should* happen. Fail today. Fix → green. Bug stays dead. (Bug fixes live in TODO.md, tracked separately.)
### Manual smoke via config flags ### Manual smoke via config flags
- `STORAGE=firebase` → current behavior (friend's path, upstream default). - `STORAGE=firebase` → current behavior (friend's path, upstream default).
@@ -234,7 +226,7 @@ Each milestone = independently mergeable PR upstream (unless marked ❌).
--- ---
## Mergability upstream ## Mergeability upstream
| Milestone | Upstream-PRable? | Why | | Milestone | Upstream-PRable? | Why |
|---|---|---| |---|---|---|
@@ -242,7 +234,7 @@ Each milestone = independently mergeable PR upstream (unless marked ❌).
| 1 backend | ❌ | divergence (friend stays Firebase) | | 1 backend | ❌ | divergence (friend stays Firebase) |
| 2 WS adapter | ⚠️ partial | interface + firebase extract ✅, WS ❌ | | 2 WS adapter | ⚠️ partial | interface + firebase extract ✅, WS ❌ |
| 3 characterization tests | ✅ | if storage-agnostic | | 3 characterization tests | ✅ | if storage-agnostic |
| 4 skip fix + manual override | ✅ | logic fix + beneficial feature | | 4 dead-not-skipped | ✅ | beneficial feature |
| 5 docker compose | ❌ | divergence | | 5 docker compose | ❌ | divergence |
| 6 undo rework | ⚠️ partial | turn-logic-level ✅, events table ❌ | | 6 undo rework | ⚠️ partial | turn-logic-level ✅, events table ❌ |
| 7 playwright | ✅ | if test infra shared | | 7 playwright | ✅ | if test infra shared |
@@ -254,7 +246,6 @@ Default `STORAGE=firebase` + `AUTH_MODE=none` (unset) = upstream sees literally
## Risks ## Risks
- **CRA + workspaces friction.** Create React App may resist monorepo layout. Mitigation: keep `src/` as CRA root, `server/` + `shared/` as separate workspaces imported via alias. Eject/craco only if forced. - **CRA + workspaces friction.** Create React App may resist monorepo layout. Mitigation: keep `src/` as CRA root, `server/` + `shared/` as separate workspaces imported via alias. Eject/craco only if forced.
- **Turn logic port correctness.** Current logic tangled; verbatim port risks subtle drift. Mitigation: characterization tests in M1/M3 lock behavior before any fix.
- **Firebase drift untested.** Mitigation: interface contract; friend's path his to maintain. - **Firebase drift untested.** Mitigation: interface contract; friend's path his to maintain.
- **Undo history migration.** Existing log entries use old snapshot format. Mitigation: keep old undo working until cleared, new format for new entries. - **Undo history migration.** Existing log entries use old snapshot format. Mitigation: keep old undo working until cleared, new format for new entries.
- **WS reconnect/state-sync edge cases.** Transient drop mid-combat. Mitigation: client requests full state resync on (re)connect; server is source of truth. - **WS reconnect/state-sync edge cases.** Transient drop mid-combat. Mitigation: client requests full state resync on (re)connect; server is source of truth.
@@ -264,12 +255,15 @@ Default `STORAGE=firebase` + `AUTH_MODE=none` (unset) = upstream sees literally
## Decisions (locked) ## Decisions (locked)
1. **Branch:** `rework-backend` off `main`. 1. **Branch:** `rework-backend` off `main`.
2. **Manual turn override:** action `JUMP_TURN_TO(participantId)`. UI button "Make This Turn". 2. **npm workspaces** for `server/` + `shared/` alongside CRA `src/`. Fallback alias if CRA fights.
3. **npm workspaces** for `server/` + `shared/` alongside CRA `src/`. Fallback alias if CRA fights. 3. **Backend = generic KV doc store** (firebase mirror), not shape-specific endpoints. Thin adapter passthrough. Opaque JSON at arbitrary path strings.
--- ---
## Next action ## Current status
M0 ✅ DONE. - M0 ✅, M1 ✅, M2 ✅, M3 ✅
M1 kickoff: scaffold `server/` workspace, set up better-sqlite3 + Express + ws, port turn logic from `App.js` into `server/turn.js`, write first unit tests. - Backend live: port 4001, db `./data/tracker.sqlite`
- Frontend: port 3999 with `REACT_APP_STORAGE=ws`
- Test suite: ~160 tests (shared + server + FE). Bugs tracked in `TODO.md`.
- Next milestones: M5 docker-compose, M6 undo rework.