Skip to content

W3-8: keep the log cursor valid across any location change; drop two hook suppressions; typed materialization fixtures - #85

Merged
vladmesh merged 2 commits into
mainfrom
claude/hygiene-eslint-enum-logcursor-rkp3w7
Sep 29, 2026
Merged

vladmesh merged 2 commits into
mainfrom
claude/hygiene-eslint-enum-logcursor-rkp3w7

Conversation

@vladmesh

@vladmesh vladmesh commented Sep 29, 2026 •

Copy link
Copy Markdown
Owner

Card W3-8. On main at 2f79530, none of the three items had been fixed yet.

1. react-hooks/exhaustive-deps suppressions (frontend)

EventLog.tsx (FullLog auto-scroll). The effect used virtualizer and stickyRef without listing them as deps. Both are stable. useVirtualizer creates one instance per mount with useState(() => new Virtualizer), and stickyRef is a ref. So they are now listed as deps and the suppression is gone. The effect still runs only when entryCount changes.

SchemaForm.tsx (reset on initialValues). formDefaults is rebuilt on every render. The suppression was there so the form resets only when initialValues changes, not on every render and not when schema or lang change (that would wipe the user's edits). The effect now calls useEffectEvent(() => reset(formDefaults)) and lists only [initialValues] as a dep. Behaviour is unchanged. React 19.2 and eslint-plugin-react-hooks 7 both support this.

Tests. These guard the behaviour the suppressions protected, so they pass on main as well. I checked that they fail against a naive fix:

  • EventLog.autoscroll.test.tsx (new file; spies on scrollToIndex):
    • scrolls once per appended entry;
    • does not re-run on re-renders that add no entries (expanding an aggregated move, a parent re-render, a store update with the same length);
    • stops following new entries after the user scrolls up, and resumes once they are back at the bottom.
  • SchemaForm.test.tsx:
    • edits survive a re-render with the same initialValues (new onSubmit, new but equal schema);
    • edits survive a lang change;
    • the form resets to the new values (with schema defaults merged in) when initialValues changes.
  • Removing the deps array in EventLog makes 2 tests fail. Adding schema, lang to the SchemaForm deps makes 2 tests fail.

Note. With the suppression gone, the React Compiler lint now analyses FullLog. It reports one warning (not an error): react-hooks/incompatible-library for useVirtualizer(). The React Compiler is not enabled in the Vite build, and before this change the suppression made the lint skip FullLog anyway, so nothing changes at runtime. eslint src/ exits 0. I did not add a new suppression to hide the warning.

2. tests/unit/test_materialization.py fixtures (tests only)

The fixtures now use Race.HUMAN, CharClass.FIGHTER, Ability.STR and (DamageComponent("1d4", DamageType.PIERCING),) instead of raw strings. The old values were also wrong for their types: ability="strength" is not a valid Ability value (the enum uses "str"), and damage="1d4" should be a tuple of DamageComponent.

To make "mypy strict catches drift" actually true for this file, I also:

  • typed the helper to return QueryFn / EmitFn;
  • typed entities as list[Entity];
  • removed a stale type: ignore;
  • added an isinstance(info.value, SquadInfo) narrowing.

The file now passes MYPYPATH=src mypy tests/unit/test_materialization.py (31 errors before). But make typecheck only runs mypy src/, so CI does not type-check tests at all. See the findings below.

3. Log cursor not reset on a GM move of the player (backend)

Symptom. A GM moves the player with PATCH /api/master/sessions/{sid}/creatures/{id} and {"location_id": ...}. The player then misses the first events at the destination, or reads that location's history from before they arrived.

Root cause. _last_seen_log_index is an index into one location's log, but nothing recorded which location. PR #75 re-anchored it only in ActivationManager._record_arrival (travel). Every other location change kept the old index:

  • commands_creatures.patch_creature;
  • the GameSession.player_location setter;
  • the NPC schedule move in activation;
  • content loading.

If the old index was larger than the destination's log, the player missed events; if smaller, they read old ones.

Fix (one central place).

  • Entity now has _log_cursor_location, set to the entity's starting location in __post_init__.
  • EventLog.append(location_id, event) is now the only way the entities layer writes its log. record() uses it, and so does log_round_start, which used to append directly. Before each append it re-anchors the cursor of every creature that has moved since the log last saw it. Anything already logged where the creature now stands predates its arrival, so the cursor starts at the end of that log. A creature that left the location gets re-anchored at its new location.
  • Readers (perceived_events, get_new_perceived_events, get_new_raw_events) share unread_events / mark_log_read. If the cursor points at another location, the reader treats it as "nothing new here".
  • This covers any location change, whoever assigns location_id. The explicit travel reset from PR Log how long a flee or journey took and where it ended #75 is now redundant and removed; the PR Log how long a flee or journey took and where it ended #75 tests still pass.

Regression test. tests/unit/test_log_cursor_relocation.py has 7 tests; I verified all 7 fail on main (source reverted, tests kept) and pass with the fix:

  • GM move via the API: the player reads the first event at the destination;
  • GM move: the player does not read the destination's older history;
  • a read before any event at the destination returns nothing and loses nothing;
  • after a round trip, the player does not read what was said while they were away;
  • the session.player_location setter;
  • ROUND_START at the destination is read;
  • the raw-event peek follows the move.

No other real bugs turned up while writing the tests.

Checks

  • make check: green. Backend 2946 passed; frontend 46 files / 359 tests passed; lint and typecheck clean apart from the one warning above.
  • Integration: Docker is not available in this environment, so I ran the backend locally with the docker-compose.test.yml settings (DND_DICE_SEED=42, DND_CONTENT_DIR=tests/integration/content, uvicorn on :8001) and ran BACKEND_URL=http://localhost:8001 uv run pytest tests/integration/: 184 passed.

Found but not fixed (out of scope)

  • Tests are not type-checked in CI (make typecheck = mypy src/). Running mypy strict over tests/ would need MYPYPATH=src (or a py.typed marker), and many existing test files would have to be cleaned up first.
  • CombatManager._record_directly (the fallback used when no record_event is injected, which is only in standalone unit tests) still appends to the log directly, bypassing EventLog.append. It is not used in the wired layer.
  • The log cursor and location logs are not persisted in saves. After a load, every creature's cursor starts at 0 on an empty log. This is unchanged by this PR.

…uppressions

The perception log cursor is an index into one location's log. PR #75 re-anchored it
on travel arrival only, so a GM move (PATCH creature location_id), the session
player_location setter or any other direct location change left it indexing the old
location's log: the player missed the first events at the destination, or read the
destination's history from before it arrived. The cursor now remembers which
location it indexes; EventLog.append (the one writer of the entities log, now also
used for ROUND_START) re-anchors every moved creature before the first event after
the move, and readers treat a cursor for another location as "nothing new here".
The explicit travel-arrival reset is subsumed.

EventLog: the auto-scroll effect lists the stable virtualizer and sticky ref as deps.
SchemaForm: the reset-on-initialValues effect reads the latest defaults through
useEffectEvent, so it still runs only when initialValues changes.

test_materialization fixtures use Race, CharClass, Ability and DamageComponent
instead of raw strings, and the file passes strict mypy.
@vladmesh

Copy link
Copy Markdown
Owner Author

Independent review: PASS

  • Item 1, exhaustive-deps:
    • EventLog now lists its stable deps (virtualizer, stickyRef).
    • SchemaForm uses useEffectEvent (React 19.2) with [initialValues] as its only dep.
    • Both suppressions are removed and behaviour is unchanged. New guard tests cover auto-scroll and the reset rules. They pass on main by design, and the PR shows they catch the naive fixes.
  • Item 2, test_materialization.py: the fixtures now use real enums and DamageComponent, and the file is clean under mypy strict. The PR's note that CI does not type-check tests/ is a fair follow-up for the backlog.
  • Item 3, GM move and the log cursor:
    • The cursor is now bound to a location (_log_cursor_location).
    • EventLog.append re-anchors moved entities, and the readers share unread_events and mark_log_read.
    • This covers every way location_id can change, not just travel. The explicit reset from PR Log how long a flee or journey took and where it ended #75 is removed, and its tests still pass.
    • One direct _location_log[...].append is left: CombatManager._record_directly. It is only the fallback when no recorder is injected, and the layer always injects one, so it is not a production path.
  • Checks: local make check is green and required CI is green.
  • Regression check: all 7 tests in test_log_cursor_relocation.py fail against main's src/ and pass with the fix.

@vladmesh
vladmesh merged commit be422ce into main Sep 29, 2026
4 checks passed
@vladmesh
vladmesh deleted the claude/hygiene-eslint-enum-logcursor-rkp3w7 branch September 29, 2026 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant