ttrpg-tools/docs/variable-store-plan.md

# Variable store — outstanding fixes

Notes from an audit of the client-side variable / tag reactivity system:

- `src/components/journal/var-reactivity.ts` — engine (dependency graph, base/mod
  separation, threshold-gated tag activation, cascade).
- `src/components/journal/variable-expression.ts` — expression evaluator.
- `src/components/journal/command-dispatcher.ts` — `/set`, `/rolltag`, cascade.
- `src/components/journal/completions.ts` — init + seed.
- `src/components/stores/journalStream.ts` — replay + hydration rebuild.
- `src/components/journal/types/var.tsx` — `var` message + reducer.

## Already fixed (context)

Commit `a26d53c` (`fix(journal): keep variable bases in the stream`):

- Hydration double-counted active tag modifiers because the stream carried only
  combined values. `var` messages now carry `base` alongside `value`, the store
  keeps `variableBases`, and `rebuildReactivityFromStore` restores exact bases.
- Seeding ran at import against an empty store (clobbering persisted values and
  appending duplicate `var` messages on every load). It now runs after hydration
  and only for keys absent from the store.

The items below are still open.

## Open issues

### 1. Modifier and declaration errors are swallowed — ✅ fixed

- **Where:** `computeInitialValues` (`catch {}`, "skip failed evaluations at init
  time") and `applyTagMapActivations` (`catch {}`, "skip failed modifier") in
  `var-reactivity.ts`; `initReactivity` throws are reduced to `console.warn` in
  `completions.ts`.
- **Symptom:** a typo'd declaration or modifier expression silently yields a
  wrong value (often `0`) with no author-facing feedback.
- **Fixed:** the engine now collects `ReactivityDiagnostic` entries for failed
  declarations (`computeInitialValues`, `reevaluateDependents`), failed modifier
  expressions (`applyTagMapActivations`), and circular declarations
  (`initReactivity`). They are deduped, logged via `console.warn` with
  key/expression/tag context, cleared on each `initReactivity`, and bridged into
  a signal exposed as `useJournalCompletions().diagnostics()`. `VariableView`
  renders them as a warning banner. A bad declaration no longer aborts the rest
  of a cascade.

### 2. `var del` does not reach the engine — ✅ fixed (dropped)

- **Where:** reducer in `types/var.tsx` supports `action: "del"`; the parser
  (`command-parser.ts`) has no `/del`; `var-reactivity.ts` never clears
  `baseValues` and `rebuildReactivityFromStore` never removes stale keys.
- **Symptom:** a deleted variable still resolves locally via `getCombined`,
  because it reads `baseValues` first.
- **Fixed:** `/del` was never reachable (no parser emits it, and replay does not
  validate), so the `del` action was removed from the `var` schema, render, and
  reducer. The wire type is now `action: "set"` only; a legacy `del` message in
  an old stream is a no-op on replay rather than resurrecting a deleted key.

### 3. Reactivity is initialized once, at import — ✅ fixed

- **Where:** `bootstrapReactivity` in `completions.ts` runs `initReactivity`
  once; nothing re-runs it when declare blocks change or a session/document is
  switched.
- **Also:** `DispatchContext.declarations` / `tagModifiers`
  (`command-dispatcher.ts`) are declared but never read — a vestige of a
  per-dispatch init that was never wired up.
- **Symptom:** editing `role=declare` content requires a full reload; stale
  graph/expressions persist.
- **Fixed:** `bootstrapReactivity` is now `applyReactivity` behind an idempotent
  `ensureReactivity()` that reads the live completions signal, compares a
  signature, and re-initializes only when the declaration set changes.
  `dispatchCommand` calls it before every dispatch, so a content rescan takes
  effect on the next command without a reload. The unused
  `DispatchContext.declarations` / `tagModifiers` fields were removed (they were
  mount-time snapshots and could not be trusted). Session switches were already
  covered by the hydration rebuild, which now also passes
  `journalStreamState.variableBases` so bases are restored exactly instead of
  falling back to combined values.

### 4. `lookup` contract disagrees with its docs — ✅ fixed

- **Where:** `EvalContext.lookup` documents "Resolve `$var`", but
  `parseFactor` calls `ctx.lookup(varName.slice(1))` (bare name), and every
  caller re-adds `$`.
- **Symptom:** a caller following the doc would key variables with `$` and
  silently get `0`s.
- **Fixed:** `EvalContext.lookup` now receives the full `$var` name (including
  `$`), consistent with every other variable API (`getCombined`, `setBase`,
  `getMods`, `resolveVariable`). Callers no longer add or strip `$`.

### 5. Undefined variables evaluate to `0` — ✅ fixed (strict for commands)

- **Where:** `parseFactor` (`variable-expression.ts`) returns a numeric `0` for
  an unresolved `$var`.
- **Symptom:** `/set $hp $cn*5` (typo) silently sets `0` instead of failing.
- **Fixed:** `parseFactor` now throws `Unknown variable: $x` whenever `lookup`
  returns `undefined`, and `resolveVariable()` distinguishes unknown from a real
  `0`. `/set` and `/roll` use the strict resolver, so typos fail loudly via
  `dispatchError`. Declarations and tag modifiers stay lenient on purpose: they
  resolve unknown references to `0` through `getCombined`, because the
  documented pattern (`$hp = $con*5+$mod_hp`) relies on external/undeclared
  variables defaulting to 0.

### 6. Tokenizer accepts malformed numbers — ✅ fixed

- **Where:** the number branch of `tokenize` consumes `[0-9.]` greedily, so
  `1.2.3` becomes one "number" and `parseFloat` truncates it.
- **Fixed:** the tokenizer now validates the scanned chunk against
  `^\d+(\.\d+)?$` and throws `Invalid number literal: "…"` instead of pushing a
  silently-truncated value. The error propagates through the diagnostics channel
  (#1) for declarations/modifiers and through `dispatchError` for `/set` and
  `/roll`.

### 7. `topoSortAffected` re-evaluates untouched declarations — ✅ fixed

- **Where:** in `var-reactivity.ts`, the visit condition
  `affected.has(dep) || declExprs.has(dep)` pulls declared dependencies that are
  not in `affected` into the result.
- **Symptom:** `reevaluateDependents` re-evaluates and re-publishes variables
  that did not change. Mostly harmless, but noisy — and a re-evaluated
  declaration could overwrite a value that had just been `/set`.
- **Fixed:** the visit condition now only follows dependencies in `affected`,
  so a cascade re-evaluates exactly the changed variable's dependents.

### 8. Failed `initReactivity` leaves partial state — ✅ fixed

- **Where:** `initReactivity` populates `depGraph` / `declExprs` / `tagModMap`
  before `checkCircular()` throws; `inFlight` is never cleared on re-init.
- **Fixed:** `initReactivity` now builds the declaration map, dependency graph,
  and tag-modifier map into locals and commits them only after `checkCircular`
  passes, so a failed (re-)init leaves the previous engine intact. Runtime state
  (`baseValues`, mods, `sourceActivations`) and `inFlight` are reset on commit.

### 9. Residual base / tagmap-target edges — low

- **Base fallback:** `rebuildReactivityFromStore` falls back to the combined
  `value` when a message has no `base`, which double-counts mods. In-repo
  emitters always send `base`; only hand-crafted/external `var` messages hit
  this.
- **Tagmap targets:** rebuild derives activations from each source's *base*
  tagmap (this fixed a feedback loop where a tagmap-valued target re-triggered
  its own modifier on every rebuild). If a variable's effective count crosses a
  threshold purely via mods, that activation is not reconstructed on hydration.
  Revisit if tagmap-valued modifier targets come into use.

## Suggested order

1. #3 (re-init) and #2 (`del`) — correctness of engine lifecycle.
2. #1 (diagnostics) — authoring feedback.
3. #4–#8 — cleanup and hardening.
4. #9 — only if the tagmap-target pattern is adopted.