Spike: state history & revert (component identity + live authoritative registry) #1
Loading…
Reference in a new issue
No description provided.
Delete branch "wip/identity-replay"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
1. Component identity (foundation)
mount(target, node, { id })sets a root identity (default"root"); backward-compatible.keyis consumed bycreateElement(intrinsic and component); stripped before props/DOM.parent.type[discriminator], discriminator =key(URI-encoded) else#position; exposed to components aslifecycle.id.calc/collection/Viewlists and destroy/recreate/remount. (A review caught a real blocker here — dynamic lists produced empty ids — now fixed and regression-tested.)2. State history (the spike)
lifecycle:field/collection/dict/model, idempotent by name.flush()+ captures a baseline; one event per flush (an atomic moment); events{ id, changes: [{ name, snapshot }] }arranged as a tree (an event's parent is the HEAD at emit time).revertTo(id): apply the nearest ancestor keyframe + forward-replay deltas; runs under a scoped applying-history guard; a write after a revert forks the tree.revertTois O(cells + N), not O(cells × path).field/dict/collection/modelregisters on retain (0→1) and unregisters on release; the recorder bootstraps from it. So module-scope state with nolifecycle.*binding is still covered. Identity (stable names) still comes from the bindings.Verification
@srhazi/gooey-testrunner ported:node ci/test-headless.mjs.tsc --noEmitclean.a→b→c→{d1→e1→f1 | d2→e2→f2}restored across both branches.Known concerns for review
Model, whose liveness is its backing dict's) and the observer effect: the recorder retains every tracked cell to observe it, keeping it live.Add a minimal Gooey state-history spike (docs/increment-2-replay.md): lifecycle.field(init, name) binds an authoritative cell under `${lifecycle.id}.${name}`, idempotent across unmount/remount; a single recording session subscribes to registered fields and emits one batched event per flush (baseline + monotonic ids) into a history tree; revertTo sets every registered cell along the root->id path under a scoped applying-history guard (no self-recording) and forks on the next write. Adds an engine after-flush hook and a statehistory test suite covering idempotency, batching, linear/cross-branch revert, fork, no-self-record, structural remount, and reset.Add dense keyframes placed by path-depth since the last ancestor keyframe (own cadence per branch; baseline is a depth-0 keyframe), a configurable interval (default 24), and revertTo via nearest-ancestor keyframe + forward delta replay -- semantically identical to the root->id fold but O(cells + N). Forward-only deltas. Expose getRevertStats() {keyframeApplied, keyframeId, deltasReplayed, cellsSet} and setKeyframeInterval(). Tests build the spec fork tree with one flush per step over x/p/q and restore to every checkpoint across both branches (all three fields), asserting branch keyframes are used and deltasReplayed <= N; plus an equivalence test with a large N.Generalize the registered cell to {kind, initial, cell, snapshot, restore, subscribe}; bindField wraps a Field, bindCollection wraps a Collection (snapshot = fresh Array.from copy, restore = in-place chunked splice). Change tracking becomes 'mark dirty, snapshot at flush' so a collection records its final array at each flush (slice/ArrayEvent detail deliberately ignored) while fields are unchanged. Add lifecycle.collection(initial, name), idempotent by name like field. Tests: mixed field+collection fork tree across both branches (push/ splice/reverse/moveSlice), one-event batching for multiple collection ops, snapshot-copy isolation, and collection idempotent remount.@ -0,0 +1,263 @@// Headless in-browser test harness for Gooey.This should be a part of
@srhazi/gooey-test-- we should make a new version of that package with this "headless" test runner exported as part of it.Moved into
@srhazi/gooey-testas a real export —@srhazi/gooey-test/headless(runHeadless+ agooey-test-headlessCLI). PR: sufian/gooey-test#1.ci/test-headless.mjsands/test-headlessare removed from this branch (commitf0fe184).@ -0,0 +28,4 @@const VITE_STARTUP_TIMEOUT_MS = 30_000;// Gooey's own `s/test` runs the perf suite only with `--perf`; match that.const PERF_TEST = 'src/performance.test.tsx';Because we're lifting it into
@srhazi/gooey-test, it will need to be properly parameterizedDone in the
gooey-testpackage — the runner is parameterized:cwd,include/testFiles,viteConfigFile/viteArgs,host/port/strictPort,timeoutMs,browser,keepOpenOnFailure,remoteDebuggingPort,artifactsDir.@ -0,0 +260,4 @@.catch((err) => {console.error('Unexpected error:', err);process.exit(1);});Overall, the headless test runner looks good, I think it should have the option to keep the playwright session running on test failure, and expose a remote debugging port for debugging the test failure with chrome devtools.
Both added in the
gooey-testpackage:keepOpenOnFailure(leaves the browser + Vite running and prints the testrunner URL) andremoteDebuggingPort(launches Chromium with--remote-debugging-port; verified/json/versionresponds).@ -0,0 +1,285 @@# Increment 2 — State history & revert (design spec)This shouldn't be committed, these are just notes for our session
Removed —
docs/increment-2-replay.mdis deleted in revision 2 (commitf0fe184). Agreed these were session notes, not repo content.@ -0,0 +5,4 @@* engine itself depends on, like render-node identity) can register cleanup* without introducing an import cycle back through `model/engine`.*/const resetCallbacks: (() => void)[] = [];It looks like we don't have any mechanism to clear this list. We should, at least for tests so we don't leak across tests.
We should also have away to unregister a callback; maybe have
registerResetreturn a function that removes the callback when called, similar to other patterns in the codebase.registerResetnow returns a disposer, andclearResetCallbacks()clears the list (test-only). Commit817b330.@ -0,0 +41,4 @@const c = collection([1, 2]);assert.is(getAuthoritativeCells().length, 0);c.retain();This test is good for retaining the entire collection, but what if something is retaining one of the collection items? i.e. if
const x = calc(() => c[0]); x.retain();Added — a test retaining a single collection item (
const x = calc(() => c[0]); x.retain()). Commit817b330.@ -0,0 +56,4 @@const d = dict<string, number>([['a', 1]]);assert.is(getAuthoritativeCells().length, 0);d.retain();Same comment about retaining a single key
Added for dict — retaining a single key. Commit
817b330.@ -0,0 +72,4 @@const backing = getModelDict(m);assert.is(getAuthoritativeCells().length, 0);backing.retain();Same about a single model key
Added for model — retaining a single property. Commit
817b330.@ -0,0 +26,4 @@>();export function registerAuthoritative(cell: object,We should avoid the generic
objecttype and use something more specific; not just here, across this change. We should avoid using overly broad types unless something is truly generic across any type, in which case we should rely on type paramaters.Addressed — introduced a named
AuthoritativeCelltype; the broadobjectis gone. Commit817b330.@ -0,0 +40,4 @@const kind = cells.get(cell);if (kind === undefined) return;cells.delete(cell);for (const listener of [...listeners]) {Doesn't Set have an iterator? Do we need to
[...listeners]?Right — it now iterates the
Setdirectly; the[...listeners]spread is gone. Commit817b330.@ -70,0 +79,4 @@*/export interface DictAuthoritativeOverride {cell: object | null;kind: AuthoritativeKind;This is only ever going to be
'model' | 'dict', right?Correct — narrowed to
'dict' | 'model'. Commit817b330.@ -288,0 +324,4 @@export function dict<K, V>(entries: [K, V][] = [],debugName?: string,authoritativeOverride?: DictAuthoritativeOverride | undefinedBy allowing passing the
authoritativeOverridein todict, we are exposing that to the public interface. I don't think this should be exposed like that. Is there a better way to do this? (Maybe we usenew Dict()instead ofdictin model.ts)Agreed —
authoritativeOverrideis no longer part of the publicdict()signature. Commit817b330.@ -34,0 +37,4 @@cell: null as object | null,kind: 'model' as const,};const modelDict = dict(Yeah, maybe use
new Dict()here to avoid making the authoritativeOverride publicDone —
model.tsusesnew Dict(...)internally to avoid the public override. Commit817b330.@ -0,0 +19,4 @@reset();subscribe();subscribeCommit();});Each test calls createTarget() and cleanup(); why not do that in beforeEach()?
Moved target/cleanup into
beforeEach. Commit817b330.@ -0,0 +505,4 @@* Like `buildForkTree`, but with a `tags` collection mutated on both branches* (push / splice / reverse / moveSlice) alongside the fields.*/function buildMixedForkTree(keyframeInterval: number) {I think the tests would be better if they interacted with the mounted DOM to trigger behaviors (i.e. click buttons, set input values, etc...), instead of pulling x/p/q/tags out from the last render call of App. This will prevent us from accidentally manipulating state when App is unmounted.
And similarly, they should assert state by asserting the contents of what's rendered.
Rewritten — tests now render a component exposing buttons/inputs bound to the global cells, click/type, and assert the rendered output (commit
817b330). Both this comment and the follow-up are covered.@ -0,0 +104,4 @@unsubscribe: (() => void) | undefined;}const DEFAULT_KEYFRAME_INTERVAL = 24;"keyframe" here is a flush()?
"keyframe" there is the keyframe interval — the number of flushes between dense snapshots — not a flush. One history node per
flush(); a node is a keyframe when its depth since the last keyframe reaches the interval. I'll make that naming unambiguous.@ -0,0 +167,4 @@function ensureListener(): void {if (listenerRegistered) return;listenerRegistered = true;registerAfterFlush(() => {Why do we need to do work after flush instead of initializing things here?
The snapshot must be read after the graph processes: a
flush()batches several writes to final values (e.g. a collection records its whole array, not slice events). Reading at subscribe/init time would capture pre-flush/slice-level state. The per-flush event is emitted from an after-flush hook.Please leave a comment here explaining why this is the case.
Also, do we ever need to unregister this handler? I'm wary of code that doesn't have a mechanism to clean up after itself.
Two things:
flush()batches a cell's writes to their final value (a collection records its whole array, not slice events), so reading at subscribe/init time would capture pre-flush / slice-level state.afterFlushUnsubscribe?.()), alongside the registry add/remove listener and releasing the recorded cells — seestatehistory.ts:424–434. There is no module-level permanent registration; I'll make that explicit in the comment too.@ -0,0 +106,4 @@const DEFAULT_KEYFRAME_INTERVAL = 24;const fields = new Map<string, BoundCell>();fields is a misnomer here, since these are all bound cells
Renamed to
cells. Commit817b330.@ -0,0 +231,4 @@let isInitial = true;bound.unsubscribe = bound.subscribe(() => {if (isInitial) {isInitial = false;Doing things once on subscriptions is a code smell, is this necessary? Set adds are idempotent, so maybe this was just for perf?
Removed the module-level one-time flag — the after-flush listener is now registered per session and torn down on unsubscribe. Commit
817b330.@ -0,0 +322,4 @@/** Replaces a collection's contents in place, chunked to avoid spread limits. */function replaceCollection<T>(cell: Collection<T>, value: unknown): void {const items = Array.isArray(value) ? (value as T[]) : [];We should avoid casting where possible
Reduced — the model cast is gone (the id now comes straight from
__debugName, commit4702b7a). One cast remains where the heterogeneous registry is dispatched to a concrete kind; I'll see whether thecellBuilders[kind]typing can absorb it.Ok, let me know what happens and if there are other casts we are performing that could be cleaned up
Will report back. Current state: the only new casts in the history/registry code are the four kind-dispatch casts at
statehistory.ts:294–297(cell as Field<unknown>, etc.) plus onevalue as T[]at:349. Those four are inherent to a heterogeneousMap<id, AuthoritativeCell>resolved to a concrete kind; I'll see whether a per-kind map or a generic cell type can absorb them. The otherascasts you may notice in the diff (collection.ts/model.ts/field.ts) are pre-existing Gooey code, not from this change.@ -0,0 +419,4 @@}/*** A session-stable name for an unbound cell: its `__debugName` when meaningfulDoes this mean that if a unique id is not passed in, we still get the benefits of being able to restore to a previous point in time and then move back? I suspect this may not work well
Per your direction: recording now requires a unique id. A cell with a default (or duplicate) id is excluded from recording entirely, and duplicates warn (commit
5cbda36) — so it gets no partial restore.@ -0,0 +453,4 @@if (cellNames.has(cell)) return;const name = deriveName(cell, kind);let bound: BoundCell;switch (kind) {This probably could be a dict lookup; like:
lookup[kind](cell)Done —
cellBuilders[kind](cell)lookup. Commit817b330.@ -22,1 +31,4 @@);// Re-label all children so unkeyed siblings track their// post-splice positions.relabelContainer(renderNode);Why do we need to do this?
Reverted to
main— it only existed for the (now-dropped) component-identity labeling. Commit58c6e8e.@ -51,0 +54,4 @@// the DOM (as a bogus attribute). Preferring keys over sibling position is// what makes identities stable across list reorders; see// `viewcontroller/identity.ts`.let key: unknown;I'm having second thoughts about
keyThe whole point of key was to be able to do:
So that the
Itemcomponent could pass in a globally unique/stablekeyso that it could use thefieldand other authoritative cells passed in.However, not all components will need to use those. Only components that define authoritative state.
So the user is forced to pay the cost of adding these keys even if they aren't needed.
I'd like to take a smaller change (just the recording and restore), so that we don't have to deal with changing the component interface. I realize this is backing off a lot from our plan, but I'd rather add things incrementally.
Agreed and reverted — the whole component-interface change is gone: no
key, nolifecycle.id, no factories;createelement/jsx/mount/render nodes matchmain(commit58c6e8e). History now runs over global unique-id cells.Please see comments
@ -0,0 +10,4 @@import { getModelDict, model } from './model';/*** Gooey state-history (SPIKE)Drop the "SPIKE" language here, this is meant to be real implementation of this.
Agreed — I'll drop the "SPIKE" framing and write this as the real implementation (module header + comments). No changes made yet, per your instruction.
@ -0,0 +13,4 @@* Gooey state-history (SPIKE)* ---------------------------** Records changes to *authoritative global state* as a stream of batched,I'd rewrite this as:
Record changes to authoritative gooey state cells (field, collection, dict, model) as a stream events forming a history tree.
Each node in the tree corresponds to a
flush()(called manually or automatically). The consumer may move theHEADpointer to any node viarevertTo()with writes after a revert forking the tree.For authoritative gooey state cells to participate in recording, they must be given an explicit unique id (their debug name):
field(0, 'x')collection([], 'tags')dict([], 'attrs')model({...}, 'prog')Each live cell is held in a registry. When recording starts, these cells are retained and not released until recording stops.
Cells that do not have a unique id are NOT recorded. If a cell is created that shares the same id as another cell, it is treated as the same underlying authoritative state (while recording).
Every
keyframeIntervala full snapshot is taken, which is aname -> valueof each cell. Changes between keyframes are sparse forward-only deltas.revertTofirst applies the nearest full snapshot, and replays deltas until the target is reached.Changes are recorded only on flush, so values written to same cell multiple times only have the last written value recorded.
A question about this:
Is this true?
Your rewrite reads well; I'll adopt it (sans "SPIKE"). One correction on the duplicate-id sentence: it is not currently true that two cells sharing an id are treated as the same state. Per your earlier direction, a duplicate id is treated as a programming error — the later cell is excluded from recording and a warning is logged (commit
5cbda36); it is not aliased to the first. Everything else matches: nodes are per-flush(), the registry retains live cells for the session,keyframeIntervaldense snapshots with forward-only sparse deltas between, and only the last value per cell per flush is recorded.Question back: for duplicates, do you want exclude + warn (current) or true aliasing (same id ⇒ same underlying state)?
Ah, I think the current behavior (excluding the later cell from recording) is more surprising than aliasing.
I have a question though: since we're already keeping track of live id -> cell values even when not recording, how difficult would it be to make it so that two live cell values alias to the same underlying state? If it's trivial, we should do that, it makes a consistent rule (all authoritative state that has an id refers to the same underlying state). I don't think it needs to be that
a === bif both have the same id, but it would be a nice bonus if that was simple to implement.Agreed that exclude+warn is the more surprising rule; aliasing is more consistent with "an id names one piece of authoritative state".
Difficulty: the
a === bform is modest but it is a core-semantics change, so I want to flag the trade before doing it.Since every authoritative cell is made by a factory (
field/dict/collection/model), the simple rule is: when an explicit id is given and a cell with that id already exists, the factory returns the existing cell instead of creating a new one — that yieldsa === bfor free. Pieces:id -> cellmap consulted by the four factories (only for explicit ids — never the default'field'/'dict'names, or every anonymous cell would alias to the previous one);The non-
===version (two distinct objects sharing one underlying store, likemodelover adict) is more work — it needs a proxying layer per kind.So the
a === broute is a small change (a lookup + map lifecycle in the four constructors) with the nicer rule; the cost is that it changes the meaning of the public constructors for all Gooey users, not just recording. If that's acceptable, I'd do thea === bform.One question before implementing: should a same id across kinds be an error (e.g.
field(0, 'x')andcollection([], 'x'))? I'd lean error — an id should be unique, and cross-kind reuse is almost certainly a bug.Please respond to each thread.
Thanks, I've left a few more comments, can you reply to those?
⏸ Resume notes — session paused
Pausing here. This is what is not inferable from the MR/diff, for whoever (or whatever) resumes.
Open decisions (blocking the next code change)
a === b. Not decided/implemented.a === brequires resolving an id at construction time — before a cell is retained — which means a registry of all constructed authoritative cells, not the current live-only registry. That's a different (and, unless GC'd, unbounded) structure. Open question: does aliasing force a construction-timeid → cellmap, and how is it reclaimed? (Constructed-but-never-retained cells, GC on__dead, etc.)field(0,'x')vscollection([],'x')) — error? (proposal: error).Environment / dev loop (not in the repo)
/Users/sufian/development/2026/gooey-identity(branchwip/identity-replay).node_modules/@srhazi/gooey-testis symlinked to the gooey-test worktree/Users/sufian/development/2025/gooey-test-headless(self-contained; chromium installed).node --preserve-symlinks node_modules/@srhazi/gooey-test/dist/headless/cli.mjs --cwd "$PWD" --port 5173 --strict-port <files>→ currently 523 tests / 20 files, exit 0. Exclude
src/performance.test.tsx(needs--expose-gc).node_modules/.headless.vite.config.ts(anfs.allowhack) that is no longer needed after the default-URL fix — safe to delete;node_modulesis gitignored.sufian+orchestrator@gmail.com.Cross-repo gate
test:headlessscript (removed with the runner) — it needs a small script using@srhazi/gooey-test/headlessafter that PR lands/publishes.Design lives outside the repo
docs/increment-2-replay.mdwas intentionally removed (session notes, per review). The full design — history tree, per-flush nodes, fork-on-write-after-revert, keyframeInterval dense snapshots + forward-only sparse deltas, the live authoritative registry, unique-id-only recording — lives in this PR's discussion + session notes only, not in the code.⏸ Resume notes — session 102 (paused; fresh eyes later)
Status: the spike is working; a few small bugs the lead saw are still to be enumerated/fixed. 609 tests / 23 files green;
tscclean. Commits onwip/identity-replay:1b925cc(core),10d7667(demos overlay).What changed since the last resume note
Core — shared backing per explicit id (model (iv)).
field/dictare consumer facade + backing;collection/modelresolve.a !== b.isRetainingRead()) or an explicitretain()/subscribe()resolves; bareget()/set()do not.initial render renders treecore test). The arousal warning fires on adopt (incl. the read path).!__anonymous); a dead cell writes locally. Anonymous/transient cells are unchanged (no register/adopt/repoint).statehistoryalso gained a non-destructive stop (stop = stop only; history + backing pin survive;revertToworks while stopped),getRoot(), and a tight public namespace export.Demos — record/replay overlay.
mountWithOverlay): Record toggle, revert slider, Branch select; bottom-left, collapsible; overlay state anonymous (never recorded); dev-gated (DEBUG); demo stays interactive; revert is non-destructive (edits after a revert fork).<option selected>rather than avaluebinding — Gooey applies props before children), 7-guis sub-app restore (stable per-app ids), Cells derived-cache rebuild on remount, uncontrolled inputs (todo/flash-test) moved into bound cells../s/server(Vite,DEBUG: true).Decisions (lead-ratified)
a === b; shared underlying state, distinct cells. Cross-kind = error. Default ids never share/register. Dead-cell arousal (first-live wins, second adopts+warns).!__anonymousgate.Open / next session
isEditingfor ~2574 cells (large keyframes); possibly derive it fromCells.state.--dh-faintcaption contrast, motion budget, O(n)-per-event tree rebuild.model.retain/release(expose or ratify subscribe-only).build/; update reactivity-patterns/ui-state guard docs.statehistorysurface + stop/reset/pin semantics.Environment
/Users/sufian/development/2026/gooey-identity;node_modules/@srhazi/gooey-testsymlinked to/Users/sufian/development/2025/gooey-test-headless(restore it if a concurrentnpm installclobbers it).node --preserve-symlinks node_modules/@srhazi/gooey-test/dist/headless/cli.mjs --cwd "$PWD" --port 5173 --strict-port -x 'src/performance.test.tsx'.orchestrator/tmp/gooey-identity/(shared-state-spec.mdauthoritative) + this PR.View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.Merge
Merge the changes and update on Forgejo.Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.