From 23cd48b1e50525df5fb8bb793667f211ae0c84ec Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 8 Oct 2026 13:36:47 +0000 Subject: [PATCH] Copy what query state adopts from shared source results (#118) Sources hand the same result object to every query and keep it cached, so adopting it into one query's state broke the next query that received it, and rows inserted by reconciliation were shared across queries. Query state now copies objects only where it adopts them: wholesale replacement, initial state, rows reconciliation inserts, and fields it rewrites. Unchanged rows cost no copy, frozen leaves and nodes the tree already manages stay shared, and a repeated emission that state already mirrors skips restoring. Built-in paths no longer alias the baseline, so the dirty-window clone now runs only for custom reconcilers and rollback no longer copies twice. Claude-Session: https://claude.ai/code/session_01Us9f7SPLJUYScpdDnA8oM4 --- .changeset/query-detach-shared-results.md | 10 + ...onvexPaginatedQueryNode.optimistic.spec.ts | 20 ++ .../src/ConvexPaginatedQueryNode.ts | 4 +- packages/retree-core/src/internals/index.ts | 1 + packages/retree-query/src/QueryNode.spec.ts | 198 +++++++++++++++++- packages/retree-query/src/QueryNode.ts | 88 +++++--- packages/retree-query/src/internals/detach.ts | 77 +++++++ .../retree-query/src/internals/reconcile.ts | 12 +- packages/retree-query/src/types.ts | 5 + 9 files changed, 382 insertions(+), 33 deletions(-) create mode 100644 .changeset/query-detach-shared-results.md create mode 100644 packages/retree-query/src/internals/detach.ts diff --git a/.changeset/query-detach-shared-results.md b/.changeset/query-detach-shared-results.md new file mode 100644 index 0000000..5208dfe --- /dev/null +++ b/.changeset/query-detach-shared-results.md @@ -0,0 +1,10 @@ +--- +"@retreejs/query": patch +"@retreejs/convex": patch +--- + +Queries no longer adopt the objects a source emits, so two queries can share one cached result. + +Two queries that received the same cached result threw "Retree cannot assign this node because it already has a structural parent". Rows added during reconciliation were also shared between queries, so editing one query changed the other. Query state now holds its own copies, including of `initialState`. Frozen objects and opaque built-ins are still shared, and reconciliation copies only the rows and fields it writes. + +Custom reconcilers should not write objects from `next` into `current` directly. Use `reconcileArray`, or return a replacement value; it is copied, except for rows reused from `current`. diff --git a/packages/retree-convex/src/ConvexPaginatedQueryNode.optimistic.spec.ts b/packages/retree-convex/src/ConvexPaginatedQueryNode.optimistic.spec.ts index 66ca57d..a37205e 100644 --- a/packages/retree-convex/src/ConvexPaginatedQueryNode.optimistic.spec.ts +++ b/packages/retree-convex/src/ConvexPaginatedQueryNode.optimistic.spec.ts @@ -437,3 +437,23 @@ describe("ConvexPaginatedQueryNode optimistic updates", () => { ); }); }); + +describe("ConvexPaginatedQueryNode shared pages", () => { + it("loads one cached page into two queries without sharing rows", () => { + const client = new FakePaginatedConvexClient(); + const first = subscribePaginatedNode(client); + const second = subscribePaginatedNode(client); + const page: RetreePaginatedQueryResult = { + results: [{ _id: "task-1", text: "Server", isCompleted: false }], + status: "CanLoadMore", + loadMore: () => true, + }; + + client.paginatedSubscriptions[0].callback(page); + client.paginatedSubscriptions[1].callback(page); + first.state!.results[0].text = "Edited"; + + expect(second.state?.results[0].text).toBe("Server"); + expect(page.results[0].text).toBe("Server"); + }); +}); diff --git a/packages/retree-convex/src/ConvexPaginatedQueryNode.ts b/packages/retree-convex/src/ConvexPaginatedQueryNode.ts index 60fec9a..1d32b0d 100644 --- a/packages/retree-convex/src/ConvexPaginatedQueryNode.ts +++ b/packages/retree-convex/src/ConvexPaginatedQueryNode.ts @@ -259,14 +259,14 @@ export class ConvexPaginatedQueryNode< const current = this.state; if (current === undefined) { - this.state = next; + this.state = this.detach(next); return; } if (!tryReconcileConvexDocuments(current.results, next.results)) { // Rows without `_id` cannot be reconciled by identity; replace the // loaded rows wholesale like the non-paginated node does. - current.results = next.results; + current.results = this.detach(next.results); } // Page bookkeeping: compare against the raw view so the function diff --git a/packages/retree-core/src/internals/index.ts b/packages/retree-core/src/internals/index.ts index 40e683a..f4497ea 100644 --- a/packages/retree-core/src/internals/index.ts +++ b/packages/retree-core/src/internals/index.ts @@ -6,6 +6,7 @@ export * from "./TypedEventEmitter.js"; export * from "./dependencies.js"; export * from "./dependency-tracking.js"; +export { isLeafObject } from "./leaves.js"; export * from "./proxy.js"; export * from "./reproxy.js"; export * from "./select.js"; diff --git a/packages/retree-query/src/QueryNode.spec.ts b/packages/retree-query/src/QueryNode.spec.ts index cc3a0eb..d17ec47 100644 --- a/packages/retree-query/src/QueryNode.spec.ts +++ b/packages/retree-query/src/QueryNode.spec.ts @@ -2,7 +2,11 @@ import { describe, expect, it, vi } from "vitest"; import { Retree } from "@retreejs/core"; import { QueryNode } from "./QueryNode.js"; import { reconcileArrayById } from "./reconcile.js"; -import { IQuerySubscriptionSource, IQuerySubscriptionHandle } from "./types.js"; +import { + IQuerySubscriptionSource, + IQuerySubscriptionHandle, + IStateReconciler, +} from "./types.js"; interface ITaskArgs { listId: string; @@ -14,6 +18,11 @@ interface ITask { isCompleted: boolean; } +interface ITaskWithMeta { + id: string; + meta?: { owner: string }; +} + class FakeQuerySource implements IQuerySubscriptionSource { @@ -632,3 +641,190 @@ describe("QueryNode optimistic updates", () => { ); }); }); + +describe("QueryNode shared source values", () => { + function subscribeTwo( + source: FakeQuerySource, + reconcile?: IStateReconciler + ) { + const nodes = [0, 1].map(() => + Retree.root( + new QueryNode(source, { args: { listId: "today" }, reconcile }) + ) + ); + for (const node of nodes) { + Retree.on(node, "nodeChanged", () => undefined); + } + return nodes; + } + + it("observes one cached result from two queries without sharing it", () => { + const source = new FakeQuerySource(); + const cached = [{ id: "task-1", text: "Server", isCompleted: false }]; + source.nextCurrentValue = cached; + const [first, second] = subscribeTwo(source); + + first.state![0].text = "Edited"; + + expect(first.state![0].text).toBe("Edited"); + expect(second.state![0].text).toBe("Server"); + expect(cached[0].text).toBe("Server"); + }); + + it("copies rows that reconciliation inserts", () => { + const source = new FakeQuerySource(); + const [first, second] = subscribeTwo(source, reconcileArrayById("id")); + const initial = { id: "task-1", text: "First", isCompleted: false }; + source.subscriptions[0].onValue([{ ...initial }]); + source.subscriptions[1].onValue([{ ...initial }]); + + const next = [ + initial, + { id: "task-2", text: "Second", isCompleted: false }, + ]; + source.subscriptions[0].onValue(next); + source.subscriptions[1].onValue(next); + first.state![1].text = "Edited"; + + expect(second.state![1].text).toBe("Second"); + expect(next[1].text).toBe("Second"); + }); + + it("copies objects that reconciliation writes into existing rows", () => { + const source = new FakeQuerySource(); + const nodes = [0, 1].map(() => + Retree.root( + new QueryNode(source, { + args: { listId: "today" }, + reconcile: reconcileArrayById("id"), + }) + ) + ); + for (const node of nodes) { + Retree.on(node, "nodeChanged", () => undefined); + } + source.subscriptions[0].onValue([{ id: "task-1" }]); + source.subscriptions[1].onValue([{ id: "task-1" }]); + + const next = [{ id: "task-1", meta: { owner: "Ada" } }]; + source.subscriptions[0].onValue(next); + source.subscriptions[1].onValue(next); + nodes[0].state![0].meta!.owner = "Grace"; + + expect(nodes[1].state![0].meta?.owner).toBe("Ada"); + expect(next[0].meta.owner).toBe("Ada"); + }); + + it("copies a replacement value but keeps rows it reuses", () => { + const source = new FakeQuerySource(); + const replaceById: IStateReconciler = { + reconcile(current, next) { + return next.map( + (task) => current?.find((row) => row.id === task.id) ?? task + ); + }, + }; + const [first, second] = subscribeTwo(source, replaceById); + const initial = { id: "task-1", text: "First", isCompleted: false }; + source.subscriptions[0].onValue([{ ...initial }]); + source.subscriptions[1].onValue([{ ...initial }]); + const row = first.state![0]; + + const next = [ + initial, + { id: "task-2", text: "Second", isCompleted: false }, + ]; + source.subscriptions[0].onValue(next); + source.subscriptions[1].onValue(next); + first.state![1].text = "Edited"; + + expect(first.state![0]).toBe(row); + expect(second.state![1].text).toBe("Second"); + }); + + it("rolls back when a custom reconciler adopts emitted rows", async () => { + const source = new FakeQuerySource(); + const adoptRows: IStateReconciler = { + reconcile(current, next) { + if (current === undefined) return next; + next.forEach((task, index) => { + current[index] = task; + }); + return current; + }, + }; + const node = Retree.root( + new QueryNode(source, { + args: { listId: "today" }, + reconcile: adoptRows, + }) + ); + Retree.on(node, "nodeChanged", () => undefined); + source.subscriptions[0].onValue([ + { id: "task-1", text: "Server", isCompleted: false }, + ]); + source.subscriptions[0].onValue([ + { id: "task-1", text: "Server", isCompleted: true }, + ]); + const mutation = Promise.reject(new Error("Mutation failed")); + + node.optimisticUpdate({ + ctx: { promise: mutation }, + apply(tasks) { + tasks[0].text = "Optimistic"; + }, + }); + await mutation.catch(() => undefined); + + expect(node.state![0].text).toBe("Server"); + }); + + it("gives each query its own copy of a shared initial state", () => { + const source = new FakeQuerySource(); + const empty: ITask[] = []; + const [first, second] = [0, 1].map(() => + Retree.root( + new QueryNode(source, { + args: { listId: "today" }, + initialState: empty, + }) + ) + ); + + first.state!.push({ id: "task-1", text: "Local", isCompleted: false }); + + expect(second.state).toEqual([]); + expect(empty).toEqual([]); + }); + + it("keeps state when the source repeats the emission it mirrors", () => { + const source = new FakeQuerySource(); + const cached = [{ id: "task-1", text: "Server", isCompleted: false }]; + source.nextCurrentValue = cached; + const node = Retree.root( + new QueryNode(source, { args: { listId: "today" } }) + ); + Retree.on(node, "nodeChanged", () => undefined)(); + const state = node.state; + + Retree.on(node, "nodeChanged", () => undefined); + + expect(node.state).toBe(state); + }); + + it("shares frozen leaves instead of copying them", () => { + const source = new FakeQuerySource(); + const task = Object.freeze({ + id: "task-1", + text: "Server", + isCompleted: false, + }); + source.nextCurrentValue = { task }; + const node = Retree.root( + new QueryNode(source, { args: { listId: "today" } }) + ); + Retree.on(node, "nodeChanged", () => undefined); + + expect(node.state?.task).toBe(task); + }); +}); diff --git a/packages/retree-query/src/QueryNode.ts b/packages/retree-query/src/QueryNode.ts index 601a07a..ce6cbc7 100644 --- a/packages/retree-query/src/QueryNode.ts +++ b/packages/retree-query/src/QueryNode.ts @@ -5,6 +5,7 @@ import { ignore, ReactiveNode, Retree, TreeNode } from "@retreejs/core"; import { getUnproxiedNode } from "@retreejs/core/internal"; +import { detach } from "./internals/detach.js"; import { isDevMode } from "./internals/env.js"; import { deepEquals } from "./internals/equality.js"; import { @@ -96,6 +97,12 @@ export class QueryNode extends ReactiveNode { */ @ignore private lastEmittedState: TState | undefined; + /** + * Emission that `state` was last restored from and still mirrors, so a + * repeated emission (such as a cached value on resubscribe) skips the copy. + */ + @ignore + private stateSource: TState | undefined; /** * True while local optimistic state differs from the last clean server * baseline and may still need confirmation or rollback. @@ -151,9 +158,9 @@ export class QueryNode extends ReactiveNode { this.args = getInitialArgs(options, queryOptions); this.reconciler = queryOptions?.reconcile; this.keepPreviousData = queryOptions?.keepPreviousData ?? false; - this.state = queryOptions?.initialState; + this.state = this.detach(queryOptions?.initialState); this.result = getInitialResult(options, this.state); - this.lastEmittedState = this.cloneState(this.state); + this.lastEmittedState = queryOptions?.initialState; } get dependencies() { @@ -392,21 +399,10 @@ export class QueryNode extends ReactiveNode { return; } - // Clone at rejection time so the rollback restores the latest - // clean server baseline — including confirmations of older - // mutations that advanced `lastEmittedState` mid-window — and - // so the baseline never aliases objects written into managed - // state during the restore. - const rollbackState = this.cloneState(this.lastEmittedState); - if ( - this.state !== undefined && - rollbackState !== undefined && - transform.revert !== undefined - ) { - transform.revert(this.state, rollbackState); - } else { - this.restoreState(rollbackState); - } + // Restore the latest clean server baseline, including + // confirmations of older mutations that advanced + // `lastEmittedState` mid-window. + this.rollback(transform.revert); this.clearOptimisticDirty(); this.setResultFromState(); this.error = this.toRollbackError(error); @@ -414,9 +410,28 @@ export class QueryNode extends ReactiveNode { }); } + private rollback( + revert: IOptimisticQueryTransform["revert"] + ): void { + if (revert === undefined || this.state === undefined) { + // restoreState copies whatever it adopts from the baseline. + this.restoreState(this.lastEmittedState); + return; + } + + // The snapshot is the caller's to mutate or adopt. + const snapshot = this.cloneState(this.lastEmittedState); + if (snapshot === undefined) { + this.restoreState(undefined); + return; + } + revert(this.state, snapshot); + } + private setPending(): void { Retree.runTransaction(() => { this.state = undefined; + this.stateSource = undefined; this.error = null; this.result = { status: "pending" }; }); @@ -438,6 +453,7 @@ export class QueryNode extends ReactiveNode { private setSkipped(): void { Retree.runTransaction(() => { this.state = undefined; + this.stateSource = undefined; this.lastEmittedState = undefined; this.clearOptimisticDirty(); this.error = null; @@ -478,10 +494,13 @@ export class QueryNode extends ReactiveNode { } this.clearOptimisticDirty(); - this.restoreState(next); - // Store the emitted value by reference. Reconciliation may alias parts - // of `next` into managed state, so `markOptimisticDirty` detaches the - // baseline with a clone before any optimistic transform can mutate it. + // restoreState copies what it adopts, so `state` never aliases `next` + // and the baseline can keep the emitted value by reference. + const isRepeat = next !== undefined && next === this.stateSource; + if (!isRepeat) { + this.restoreState(next); + this.stateSource = next; + } this.lastEmittedState = next; this.setResultFromState(next); } @@ -509,6 +528,10 @@ export class QueryNode extends ReactiveNode { * replacing the state wholesale. Subclasses with non-plain state shapes * (for example paginated results carrying a `loadMore` function) override * this. + * + * `next` may be shared with other queries and the source's cache, so + * overrides must copy any object they write into `state` with + * {@link QueryNode.detach} or reconcile it with `reconcileArray`. */ protected restoreState(next: TState | undefined): void { if (next === undefined) { @@ -521,7 +544,7 @@ export class QueryNode extends ReactiveNode { return; } - this.state = next; + this.state = this.detach(next); return; } @@ -538,7 +561,15 @@ export class QueryNode extends ReactiveNode { return; } - this.state = reconciled; + this.state = this.detach(reconciled); + } + + /** + * Copy every object Retree would adopt from `value`, sharing leaves and + * functions. Use it in {@link QueryNode.restoreState} overrides. + */ + protected detach(value: T): T { + return detach(value); } /** @@ -610,13 +641,14 @@ export class QueryNode extends ReactiveNode { } private markOptimisticDirty(): number { - if (!this.isOptimisticDirty) { - this.isOptimisticDirty = true; - // The emitted baseline may alias objects that reconciliation wrote - // into managed state. Detach it now — before the first optimistic - // transform mutates state — so the rollback baseline stays clean. + if (!this.isOptimisticDirty && this.reconciler !== undefined) { + // A custom reconciler may have written baseline objects into + // `state`; detach the baseline before the transform mutates it. this.lastEmittedState = this.cloneState(this.lastEmittedState); } + // The transform is about to make `state` diverge from its source. + this.stateSource = undefined; + this.isOptimisticDirty = true; this.optimisticGeneration++; // The first generation in a dirty window is the oldest optimistic write diff --git a/packages/retree-query/src/internals/detach.ts b/packages/retree-query/src/internals/detach.ts new file mode 100644 index 0000000..528c465 --- /dev/null +++ b/packages/retree-query/src/internals/detach.ts @@ -0,0 +1,77 @@ +/*! + * Copyright (c) Ryan Bliss. All rights reserved. + * Licensed under the MIT License. + */ + +import { getUnproxiedNode, isLeafObject } from "@retreejs/core/internal"; + +/** + * Copy every object Retree would adopt as a structural child. + * + * @remarks + * Sources may hand the same object to several queries and keep it in their + * own cache, so query state must never adopt it. Leaves (frozen objects and + * opaque built-ins), functions, and nodes the tree already manages are + * shared. Copies keep own enumerable string keys, like the source data + * queries carry. + */ +export function detach(value: T): T { + return detachValue(value) as T; +} + +function detachValue(value: unknown): unknown { + if (value === null) return value; + if (typeof value !== "object") return value; + if (isLeafObject(value)) return value; + if (getUnproxiedNode(value) !== value) return value; + if (Array.isArray(value)) return value.map(detachValue); + if (isPlainRecord(value)) { + // Bracket access is ~3x faster than Reflect for payload-sized copies. + const copy: Record = {}; + for (const key of Object.keys(value)) { + setField(copy, key, detachValue(value[key])); + } + return copy; + } + if (value instanceof Date) return new Date(value.getTime()); + if (value instanceof Map) { + const copy = new Map(); + for (const [key, item] of value) copy.set(key, detachValue(item)); + return copy; + } + if (value instanceof Set) { + const copy = new Set(); + for (const item of value) copy.add(detachValue(item)); + return copy; + } + + const copy: Record = Object.create( + Object.getPrototypeOf(value) + ); + for (const key of Object.keys(value)) { + setField(copy, key, detachValue(Reflect.get(value, key))); + } + return copy; +} + +function isPlainRecord(value: object): value is Record { + return Object.getPrototypeOf(value) === Object.prototype; +} + +function setField( + copy: Record, + key: string, + value: unknown +): void { + if (key === "__proto__") { + // Assignment would invoke the prototype setter. + Object.defineProperty(copy, key, { + value, + writable: true, + enumerable: true, + configurable: true, + }); + return; + } + copy[key] = value; +} diff --git a/packages/retree-query/src/internals/reconcile.ts b/packages/retree-query/src/internals/reconcile.ts index 48d7b31..ae1f528 100644 --- a/packages/retree-query/src/internals/reconcile.ts +++ b/packages/retree-query/src/internals/reconcile.ts @@ -5,6 +5,7 @@ import { getUnproxiedNode } from "@retreejs/core/internal"; import { TreeNode } from "@retreejs/core"; +import { detach } from "./detach.js"; import { deepEquals } from "./equality.js"; function unwrapRaw(value: T): T { @@ -42,6 +43,13 @@ export function tryReconcileDocumentsById( return true; } +/** + * Reconcile `next` into `current` by item id, writing only the diffs. + * + * @remarks + * Objects written into `current` are copies, so `next` stays free for other + * queries and the source's cache. + */ export function reconcileArray( current: TItem[], next: TItem[], @@ -96,7 +104,7 @@ export function reconcileArray( const nextId = getId(nextItem); const currentItem = currentById.get(nextId); if (currentItem === undefined) { - current[index] = nextItem; + current[index] = detach(nextItem); continue; } @@ -243,7 +251,7 @@ function reconcileObject( if (deepEquals(rawValue, value) && Object.hasOwn(target.raw, key)) { continue; } - Reflect.set(getManagedTarget(target), key, value); + Reflect.set(getManagedTarget(target), key, detach(value)); } } diff --git a/packages/retree-query/src/types.ts b/packages/retree-query/src/types.ts index fdc82fd..ca3611e 100644 --- a/packages/retree-query/src/types.ts +++ b/packages/retree-query/src/types.ts @@ -105,6 +105,11 @@ export interface IStateReconciler { * changed rows emit `nodeChanged` and item identity stays stable for * `useNode` rows; writes to `rawCurrent` skip emission entirely. * + * `next` may be shared with other queries and the source's cache, so + * never write its objects into `current` directly. `reconcileArray` + * copies what it writes, and a returned replacement value is copied + * except for rows it reuses from `current`. + * * @param current Current query state, if any. Write surface. * @param next Newly emitted query state (raw server data). * @param rawCurrent Raw view of `current` for fast reads; `undefined`