fix(journal): surface variable eval errors

Collect and report failed declarations, failed tag modifier expressions, and
circular dependencies instead of swallowing them. Diagnostics are deduped,
logged with key/expression context, cleared on re-init, and exposed through
useJournalCompletions and the VariableView warning banner. A bad declaration
no longer aborts the rest of a cascade.

Reject malformed numeric literals in the tokenizer so inputs like "1.2.3"
throw instead of being truncated by parseFloat.
This commit is contained in:
hyper
2026-09-12 22:22:18 +08:00
parent a26d53cbeb
commit 9f372fa2b0
7 changed files with 399 additions and 9 deletions
+129
View File
@@ -0,0 +1,129 @@
# 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 — medium
- **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.
- **Fix:** either wire a `/del` command and clear engine state (base + mods) on
delete, or drop `del` from the schema/reducer if it is intentionally unused.
### 3. Reactivity is initialized once, at import — medium
- **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.
- **Fix:** expose a re-init entry point keyed to content/session changes, and
either use or remove the unused context fields.
### 4. `lookup` contract disagrees with its docs — low
- **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.
- **Fix:** update the doc comment to state the argument is the bare name (no
`$`), or stop stripping in `parseFactor` and update callers.
### 5. Undefined variables evaluate to `0` — low
- **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.
- **Fix:** consider erroring on unknown variables in `/set` context while
keeping lenient behavior for display/roll contexts, or warn.
### 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 — low
- **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.
- **Fix:** only visit/topologically order keys in `affected`.
### 8. Failed `initReactivity` leaves partial state — low
- **Where:** `initReactivity` populates `depGraph` / `declExprs` / `tagModMap`
before `checkCircular()` throws; `inFlight` is never cleared on re-init.
- **Fix:** build into locals and commit on success, or roll back on failure.
Clear `inFlight` in `initReactivity`.
### 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.