Fix dropped treeVersion updates and follow up on #129

.changeset/keep-observed-across-resubscribe.md

@@ -4,4 +4,4 @@
 
 `useSelect` no longer re-runs `onUnobserved` and `onObserved` on a `ReactiveNode` it keeps reading when a prop moves the selector's other dependencies. Query nodes such as `ConvexQueryNode` no longer close and reopen their backend subscription on those re-renders.
 
-React hooks now release a `ReactiveNode` one microtask after its last listener unsubscribes, so `onUnobserved` runs just after unmount rather than during it.
+React hooks now release a `ReactiveNode` one microtask after its last listener unsubscribes, so `onUnobserved` runs just after unmount rather than during it. Tests that fake `queueMicrotask`, as Jest's fake timers do by default, need to flush microtasks (`jest.runAllTicks()`) before asserting on it.

.changeset/unsubscribe-after-clear-listeners.md

@@ -0,0 +1,5 @@
+---
+"@retreejs/core": patch
+---
+
+Calling a `Retree.on` unsubscribe after `Retree.clearListeners` already removed that listener no longer runs the node's `onUnobserved` a second time.

.changeset/use-select-tree-version.md

@@ -0,0 +1,8 @@
+---
+"@retreejs/core": patch
+"@retreejs/react": patch
+---
+
+A selector that reads `Retree.treeVersion(node)` now re-runs on every write under `node`.
+
+Before, a selector-only `useSelect` dropped writes to descendants the selector never read directly, unlike `Retree.select`. And `useSelect`, `Retree.select`, `Retree.effect` and `@select` getters all skipped a write to a descendant's field when the selector also read a different field of that descendant.

packages/retree-core/src/ReactiveNode.spec.ts

@@ -511,6 +511,16 @@ describe("ReactiveNode", () => {
         expect(root.unobservedCount).toBe(1);
     });
 
+    it("runs onUnobserved once when clearListeners precedes an unsubscribe", () => {
+        const root = trackRoot(Retree.root(new ObservedLifecycleNode()));
+        const unsubscribe = Retree.on(root, "nodeChanged", vi.fn());
+
+        Retree.clearListeners(root);
+        unsubscribe();
+
+        expect(root.unobservedCount).toBe(1);
+    });
+
     it("lazily proxies ReactiveNode object and array fields with parent metadata", () => {
         const root = trackRoot(Retree.root(new NestedPayloadNode()));
 

packages/retree-core/src/Retree.ts

@@ -1730,15 +1730,19 @@ export class Retree {
             if (unsubscribed) return;
             unsubscribed = true;
             const _listeners = relevantListenerMap.get(unproxiedNode);
-            if (_listeners) {
-                const findIndex = _listeners.findIndex((l) => l === callback);
-                if (findIndex !== -1) {
-                    _listeners.splice(findIndex, 1);
-                    this.handleListenerCountChanged(relevantListenerMap, -1);
-                }
-                if (_listeners.length === 0) {
-                    relevantListenerMap.delete(unproxiedNode);
-                }
+            const findIndex =
+                _listeners === undefined
+                    ? -1
+                    : _listeners.findIndex((l) => l === callback);
+            if (_listeners === undefined || findIndex === -1) {
+                // `clearListeners` already removed it and ran the teardown
+                // below; running it again would repeat `onUnobserved`.
+                return;
+            }
+            _listeners.splice(findIndex, 1);
+            this.handleListenerCountChanged(relevantListenerMap, -1);
+            if (_listeners.length === 0) {
+                relevantListenerMap.delete(unproxiedNode);
             }
             const isReactiveChangeListenerMap =
                 relevantListenerMap !== this.nodeRemovedListeners;

packages/retree-core/src/internals/select.ts

@@ -725,17 +725,19 @@ export function runTrackedSelection<TSelected>(
 }
 
 /**
- * The record that decides a `nodeChanged` from `changedRawNode`: the node's
- * own, or else the nearest ancestor record whose tree version the run read,
- * which re-reads that version. Undefined when the run read neither.
+ * The record that decides a `nodeChanged` from `changedRawNode`: the nearest
+ * record, the node's own included, whose tree version the run read, which
+ * re-reads that version; else the node's own. A tree version read wins over
+ * the node's own record, which may skip a key it never read even though the
+ * write moved the tree version. Undefined when the run read neither.
  */
 export function resolveChangedReadRecord(
     reads: ReadonlyMap<TreeNode, NodeReadRecord>,
     subtreeReads: readonly NodeReadRecord[],
     changedRawNode: TreeNode
 ): NodeReadRecord | undefined {
     const record = reads.get(changedRawNode);
-    if (record !== undefined || subtreeReads.length === 0) {
+    if (subtreeReads.length === 0 || record?.subtreeRead) {
         return record;
     }
     let handler = getBaseHandlerForUnproxiedNode(changedRawNode)?.parentHandler;
@@ -746,7 +748,7 @@ export function resolveChangedReadRecord(
         }
         handler = handler.parentHandler;
     }
-    return undefined;
+    return record;
 }
 
 /**

packages/retree-core/src/node-version.spec.ts

@@ -192,6 +192,47 @@ describe("tracked version reads", () => {
         stop();
     });
 
+    it("re-runs on an unread field of a node also read beneath a tree version read", () => {
+        class Shelf extends ReactiveNode {
+            public books = [{ title: "a", tags: ["x"] }];
+            get dependencies() {
+                return [];
+            }
+            @select
+            get summary(): string {
+                return `${Retree.treeVersion(this.books)}:${
+                    this.books[0].title
+                }`;
+            }
+        }
+        const root = Retree.root({ rows: [{ title: "a", done: false }] });
+        const shelf = Retree.root(new Shelf());
+        roots.push(root, shelf);
+        const selected: string[] = [];
+        const stopSelect = Retree.select(
+            () => `${Retree.treeVersion(root.rows)}:${root.rows[0].title}`,
+            (value) => selected.push(value)
+        );
+        let effectRuns = 0;
+        const stopEffect = Retree.effect(() => {
+            effectRuns++;
+            Retree.treeVersion(root.rows);
+            void root.rows[0].title;
+        });
+        void shelf.summary;
+        const changed = vi.fn();
+        Retree.on(shelf, "nodeChanged", changed);
+
+        root.rows[0].done = true;
+        shelf.books[0].tags = ["z"];
+
+        expect(selected).toHaveLength(1);
+        expect(effectRuns).toBe(2);
+        expect(changed).toHaveBeenCalledTimes(1);
+        stopSelect();
+        stopEffect();
+    });
+
     it("re-runs a @select getter on writes under a tree version read", () => {
         const root = Retree.root({
             library: new Library(),

packages/retree-react/src/internals/subscriptionHub.spec.ts

@@ -4,10 +4,49 @@
  */
 
 import { describe, expect, it, vi } from "vitest";
-import { Retree } from "@retreejs/core";
+import { ReactiveNode, Retree } from "@retreejs/core";
 import { subscribeToNode } from "./subscriptionHub.js";
 
+class LifecycleNode extends ReactiveNode {
+    public count = 0;
+    public observed = 0;
+    public unobserved = 0;
+    get dependencies() {
+        return [];
+    }
+    protected onObserved(): void {
+        this.observed++;
+    }
+    protected onUnobserved(): void {
+        this.unobserved++;
+    }
+}
+
 describe("subscriptionHub", () => {
+    it("defers a ReactiveNode teardown so a same-tick resubscribe keeps it observed", async () => {
+        const node = Retree.root(new LifecycleNode());
+        const listener = vi.fn();
+        subscribeToNode(node, "nodeChanged", vi.fn())();
+        subscribeToNode(node, "nodeChanged", vi.fn())();
+        const unsubscribe = subscribeToNode(node, "nodeChanged", listener);
+        await Promise.resolve();
+
+        expect(node.observed).toBe(1);
+        expect(node.unobserved).toBe(0);
+        node.count = 1;
+        expect(listener).toHaveBeenCalledTimes(1);
+
+        unsubscribe();
+        expect(node.unobserved).toBe(0);
+        await Promise.resolve();
+        expect(node.unobserved).toBe(1);
+
+        subscribeToNode(node, "nodeChanged", vi.fn())();
+        await Promise.resolve();
+        expect(node.observed).toBe(2);
+        expect(node.unobserved).toBe(2);
+    });
+
     it("does not remove another listener when unsubscribe is called twice", () => {
         const root = Retree.root({ count: 0 });
         const listenerA = vi.fn();

packages/retree-react/src/useSelect.spec.tsx

@@ -282,6 +282,33 @@ describe("useSelect", () => {
         expect(screen.getByTestId("value").textContent).toBe("1");
     });
 
+    it("re-renders a tracked treeVersion read on a descendant write", () => {
+        const root = trackRoot(
+            Retree.root({ list: [{ done: false }], label: "one" })
+        );
+        let renderCount = 0;
+
+        function Probe() {
+            renderCount += 1;
+            const version = useSelect(() => Retree.treeVersion(root.list));
+            return <div data-testid="value">{version}</div>;
+        }
+
+        render(<Probe />);
+        const initial = screen.getByTestId("value").textContent;
+
+        act(() => {
+            root.label = "two";
+        });
+        expect(renderCount).toBe(1);
+
+        act(() => {
+            root.list[0].done = true;
+        });
+        expect(screen.getByTestId("value").textContent).not.toBe(initial);
+        expect(renderCount).toBe(2);
+    });
+
     it("selects with trapped dependencies when only a selector function is passed", () => {
         const root = trackRoot(
             Retree.root({

packages/retree-react/src/useSelect.ts

@@ -22,6 +22,7 @@ import {
     hasTrackedSelectionChanged,
     normalizeDependencyEntry,
     normalizeSelectDependencies,
+    resolveChangedReadRecord,
     runTrackedSelection,
     stabilizeSelectedRetreeReferences,
     TrackedSelection,
@@ -191,7 +192,8 @@ function getTrackedListenerType(
 
 /**
  * True when a moved source can still affect the selection. With the changed
- * nodes known, only their records validate (nodes without a record are
+ * nodes known, only their records validate, or the nearest ancestor whose
+ * tree version the selector read (other nodes without a record are
  * descendants of a subtree cover the selector never read). Without them every
  * record under a moved source re-reads; validation is raw reads, far cheaper
  * than re-running the selector.
@@ -204,7 +206,11 @@ function isTrackedSnapshotChangeRelevant(
     const changes = snapshot.changes;
     if (changes !== undefined) {
         for (const change of changes) {
-            const record = reads.get(change.rawNode);
+            const record = resolveChangedReadRecord(
+                reads,
+                state.subtreeReads,
+                change.rawNode
+            );
             if (record === undefined) {
                 continue;
             }
@@ -482,6 +488,7 @@ function recomputeNodeSelectStateForSelector<TNode extends TreeNode, TSelected>(
  */
 interface TrackedSelectState<TSelected> {
     reads: TrackedSelection<TSelected>["reads"];
+    subtreeReads: TrackedSelection<TSelected>["subtreeReads"];
     sources: readonly RetreeExternalStoreSource[];
     /**
      * Swappable for the same reason as {@link NodeSelectState.store}: a
@@ -501,6 +508,7 @@ function createTrackedSelectState<TSelected>(
     const store = createRetreeSwappableCompositeExternalStore(sources);
     return {
         reads: selection.reads,
+        subtreeReads: selection.subtreeReads,
         sources,
         store,
         snapshot: store.getSnapshot(),
@@ -524,6 +532,7 @@ function refreshTrackedSelectState<TSelected>(
     const nextSelection = runTrackedSelection(selector);
     const previousReads = state.reads;
     state.reads = nextSelection.reads;
+    state.subtreeReads = nextSelection.subtreeReads;
     const stabilizedSelected = stabilizeSelectedRetreeReferences(
         state.container.selected,
         nextSelection.selected
@@ -616,6 +625,7 @@ function recomputeTrackedSelectStateForSelector<TSelected>(
     }
     return {
         reads: nextSelection.reads,
+        subtreeReads: nextSelection.subtreeReads,
         sources,
         store,
         snapshot: store.getSnapshot(),