Skip to content

[HSF-1642] fix: resolve hook methods against the live subscription - #55

Merged
daytonmux merged 2 commits into
masterfrom
daytonmux/pasted-text
Nov 6, 2026
Merged

daytonmux merged 2 commits into
masterfrom
daytonmux/pasted-text

Conversation

@daytonmux

@daytonmux daytonmux commented Nov 6, 2026 •

Copy link
Copy Markdown
Member

Fixes the bug behind HSF-1642 (worked around in the app by flowcircuits/hvakr#8248). When a loading-status hook mounted before a writer on the same resource, the writer's update/add/remove/set/load/sync stayed bound to an abandoned provisional subscription. That subscription has no snapshot, so writes were dropped, with no signal at all in production. Create Zone and Create System on a new project did nothing because of this. Hook order no longer matters.

Summary

Each hook builds a provisional subscription during render. On commit, every hook after the first adopts the first one's entry (acquire() sets ent = committed). The returned handle copied its methods at render time and copied them again only when the selected slice changed, so a value-equal slice kept the stale methods. Examples: an empty collection's {}, a missing doc's undefined, a writer-only selector.

 useCollection / useDocument  (src/react/hooks.ts)
+  methods = useMemo(() => ({
+    update: (...a) => getHandle().update(...a),   // looked up per call
+    add, remove, set, delete, load, sync: same
+  }), [getHandle])                                // one set per `shared`
   return useMemo(() => ({
     data: selection,
-    update: getHandle().update,                   // copied at render
+    update: methods.update,
     ...
   }), [selection, getHandle, hasSelector, methods])
  • Adoption (ent = committed): the wrappers go through the facade, so they reach the adopted entry.
  • Replacement (ent.sub = buildSub(ent) after refCount hits 0): a method held from before keeps its identity and calls the rebuilt subscription. I chose this because identity is tied to the facade (shared), which survives the rebuild. Changing identity would only help callers that list the method as a dependency. A method already captured in a closure would still hit the stopped subscription.
  • Read-only facade: the wrappers call the facade's getHandle(), which still neuters writers and sync. load passes through to the current subscription.
  • Identity: stable across rerenders and changes only when shared changes (definition, path, or query). Before this fix it also changed silently mid-life, when the first snapshot rebound methods from the provisional to the adopted subscription.

No options or flags. Contract text is updated in AGENTS.md, docs/architecture.md, and README.md.

Evidence

New block in src/react/hooks.test.ts (only onSnapshot is mocked; queries and refs are real):

mount <LoadingStatus; Writer(selector: s => s.data)>   // status hook first
mount <Reader>; reader.load(); deliver empty snapshot; isLoaded
writer.update({ first: { name: 'x' } })
expect(reader.data.first.name).toBe('x')
Case master This PR
collection, empty: update() ✗ undefined ✓
collection, empty: add() ✗ undefined ✓
collection: remove() (writer-only slice) ✗ doc still present ✓
document, missing: set() ✗ undefined ✓
document: update() (writer-only slice) ✗ { name: 'a' } ✓
read-only: load() reaches live sub, writers blocked ✗ status stays idle ✓
replacement: re-keyed remount (refCount 0, then re-acquire), held update ✗ undefined ✓
method identity across rerenders ✗ updateState ≠ updateState ✓
control: writer mounted before status hook ✓ ✓
control: data in first snapshot ✓ ✓
master:   Tests  8 failed | 18 passed (26)   src/react/hooks.test.ts
this PR:  Tests  26 passed (26)
full:     Tests  307 passed | 3 skipped (310); pnpm typecheck and pnpm build clean

The replacement case uses a re-keyed remount, not StrictMode, because react-test-renderer 18.3.1 never double-runs effects. The new hooks render while the entry is still live. The old hooks then release it (refCount 0), and the new ones re-acquire it, which takes the same buildSub path that StrictMode and offscreen take.

Release

package.json is bumped to 0.2.2 in its own chore(release) commit, like #53. The repo has no CHANGELOG file; release notes are generated from PR titles when the GitHub release is published. Suggested note: Fix hook methods (update/add/remove/set/load/…) staying bound to an abandoned subscription, so writes were dropped depending on hook mount order (HSF-1642).

Follow-ups (out of scope)

  • Silent dropped writes in production. A write to a subscription that has no snapshot yet only calls warnNoSnapshot, and only outside production, so the caller can't tell the write was dropped. Proposal: report it through the store's onError and/or return a result from the writers. That is an API change, so it is not built here.
  • HVAKR app. After this is released, bump @hvakr/firestate in the app. The hook-order workaround from flowcircuits/hvakr#8248 can then be reverted. The app is not changed in this PR.
  • ref is still read when the handle is built. After adoption it can be the provisional subscription's ref object: same path and refEqual, but a different identity. This doesn't affect correctness.

Merge Danger

Door: two-way

Revert the commit to roll back. No data or storage changes.

Blast Radius: hooks

Every useDocument and useCollection handle, including the generated createFirestate hooks, now returns wrapper functions instead of the subscription's own functions. Calls and return values are unchanged. Code that compared a handle method by identity against subscription.getHandle().update would now see a different function; nothing in the repo does this. Disabled handles (enabled: false) also return per-hook no-op wrappers instead of the shared module-level no-ops. They are still stable across renders.

🤖 Generated with Claude Code


Autofix with [code]smith
Open in [code]smith · Need help on this PR? Tag @codesmith-bot with what you need.

daytonmux and others added 2 commits November 6, 2026 00:37
useCollection/useDocument copied update/add/remove/set/delete/load/sync off
the subscription at render time and re-copied them only when the selected
slice changed. When a hook adopted a sibling's committed entry in acquire()
(e.g. a loading-status hook mounted before a writer), or the entry's
subscription was rebuilt after refCount hit 0, a value-equal slice kept the
methods bound to the abandoned subscription. It has no snapshot, so writes
were dropped (silently in production).

Each method is now a wrapper built once per `shared` facade that calls
getHandle() when invoked, so it reaches the current subscription and keeps
its identity across rerenders.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Bump @hvakr/firestate from 0.2.1 to 0.2.2 to release the hook method
binding fix.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@daytonmux
daytonmux merged commit f2c2509 into master Nov 6, 2026
1 check passed
@daytonmux
daytonmux deleted the daytonmux/pasted-text branch November 6, 2026 08:45
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