From 78d72ca70a235bda3fb4b3e4942487fa4b032dcd Mon Sep 17 00:00:00 2001 From: David Matejka Date: Thu, 25 Jun 2026 10:50:15 +0200 Subject: [PATCH] fix(bindx): drop vestigial handle dispose lifecycle that broke late writes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Handles are stateless live views over the store: the only subscriptions they create are returned from subscribe() and owned by useSyncExternalStore, not stored on the handle. So dispose() freed nothing — it only set _disposed and armed assertNotDisposed() to throw on legitimate late writes through an accessor a consumer still holds (e.g. the Slate block editor dispatching setValue from an async onChange after the entity version bumped and EntityHandleRenderer recreated the handle). The dispose machinery was orphaned by the migration away from the eager identityMap subscription model, where handles really did hold _unsubscribe. Remove the dispose lifecycle from handles (dispose/isDisposed/assertNotDisposed in BaseHandle, the dispose() overrides, 25 assertNotDisposed() call sites, and dispose() from the internal RelationHandleRaw interface), delete the now-dead core/Disposable.ts, and stop disposing superseded handles in useEntity and . reset() stays — it is a real operation. Add a regression test covering a captured field accessor that stays writable across a data-driven re-render. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01EAKx7JbV7Z7EVXofqRgwop --- packages/bindx-react/src/hooks/useEntity.ts | 9 +- .../bindx-react/src/jsx/components/Entity.tsx | 7 +- packages/bindx/src/core/Disposable.ts | 126 ------------------ packages/bindx/src/handles/BaseHandle.ts | 39 ++---- packages/bindx/src/handles/EntityHandle.ts | 27 +--- packages/bindx/src/handles/FieldHandle.ts | 4 - .../bindx/src/handles/HasManyListHandle.ts | 22 --- packages/bindx/src/handles/HasOneHandle.ts | 19 --- ...ntityHandleDisposalAcrossRerender.test.tsx | 63 +++++++++ tests/unit/handles/entityHandle.test.ts | 18 --- 10 files changed, 84 insertions(+), 250 deletions(-) delete mode 100644 packages/bindx/src/core/Disposable.ts create mode 100644 tests/react/jsx/entityHandleDisposalAcrossRerender.test.tsx diff --git a/packages/bindx-react/src/hooks/useEntity.ts b/packages/bindx-react/src/hooks/useEntity.ts index e750dcb7..5331ef23 100644 --- a/packages/bindx-react/src/hooks/useEntity.ts +++ b/packages/bindx-react/src/hooks/useEntity.ts @@ -333,11 +333,10 @@ export function useEntity( [rawHandle], ) - useEffect(() => { - return () => { - rawHandle.dispose() - } - }, [rawHandle]) + // `snapshot` is a useMemo dep, so `rawHandle` is recreated on every entity data change (to give + // memoized children a fresh reference). Superseded handles need no cleanup: EntityHandle is a + // stateless live view that owns no resources (see BaseHandle), so it is reclaimed by GC once + // unreferenced and a handle a consumer still holds stays usable for late reads/writes. // --- Persist & reset callbacks --- const persist = useCallback(async () => { diff --git a/packages/bindx-react/src/jsx/components/Entity.tsx b/packages/bindx-react/src/jsx/components/Entity.tsx index f229ca83..acdee09d 100644 --- a/packages/bindx-react/src/jsx/components/Entity.tsx +++ b/packages/bindx-react/src/jsx/components/Entity.tsx @@ -320,7 +320,12 @@ function EntityHandleRenderer({ [rawHandle], ) - useEffect(() => () => { rawHandle.dispose() }, [rawHandle]) + // `version` is a useMemo dep, so `rawHandle` is recreated on every entity data change (to give + // memoized children a fresh reference). Superseded handles need no cleanup: EntityHandle and its + // child Field/HasMany handles are stateless live views that own no resources (see BaseHandle), + // so they are reclaimed by GC once unreferenced — and a consumer that still holds an accessor + // from an earlier render (e.g. the Slate block editor dispatching `setValue` from an async + // onChange after the version bumped) can keep using it. const result = children(handle as EntityAccessor) return <>{annotateElement(result, { 'data-entity': entityType, 'data-entity-id': entityId })} diff --git a/packages/bindx/src/core/Disposable.ts b/packages/bindx/src/core/Disposable.ts deleted file mode 100644 index 6f9acca9..00000000 --- a/packages/bindx/src/core/Disposable.ts +++ /dev/null @@ -1,126 +0,0 @@ -/** - * Disposable pattern for cleanup management. - * Ensures proper cleanup of subscriptions and resources. - */ - -/** - * Interface for objects that can be disposed. - */ -export interface Disposable { - dispose(): void -} - -/** - * A group of disposables that can be disposed together. - * Provides safe cleanup even if individual dispose() calls throw. - */ -export class DisposableGroup implements Disposable { - private disposables: Disposable[] = [] - private disposed = false - - /** - * Adds a disposable to the group. - * If the group is already disposed, the disposable is disposed immediately. - */ - add(disposable: Disposable): void { - if (this.disposed) { - // Already disposed, dispose immediately - try { - disposable.dispose() - } catch (error) { - console.error('Error disposing:', error) - } - return - } - this.disposables.push(disposable) - } - - /** - * Adds a callback as a disposable. - */ - addCallback(callback: () => void): void { - this.add({ dispose: callback }) - } - - /** - * Adds an unsubscribe function as a disposable. - */ - addUnsubscribe(unsubscribe: () => void): void { - this.addCallback(unsubscribe) - } - - /** - * Disposes all items in the group. - * Each item is disposed even if previous ones throw. - */ - dispose(): void { - if (this.disposed) return - this.disposed = true - - const errors: Error[] = [] - - for (const disposable of this.disposables) { - try { - disposable.dispose() - } catch (error) { - errors.push(error instanceof Error ? error : new Error(String(error))) - } - } - - this.disposables = [] - - if (errors.length > 0) { - console.error('Errors during disposal:', errors) - } - } - - /** - * Checks if the group has been disposed. - */ - isDisposed(): boolean { - return this.disposed - } - - /** - * Gets the number of disposables in the group. - */ - get size(): number { - return this.disposables.length - } -} - -/** - * Creates a disposable from a callback function. - */ -export function createDisposable(callback: () => void): Disposable { - let disposed = false - return { - dispose() { - if (disposed) return - disposed = true - callback() - }, - } -} - -/** - * Combines multiple disposables into one. - */ -export function combineDisposables(...disposables: Disposable[]): Disposable { - const group = new DisposableGroup() - for (const d of disposables) { - group.add(d) - } - return group -} - -/** - * Type guard to check if something is disposable. - */ -export function isDisposable(value: unknown): value is Disposable { - return ( - typeof value === 'object' && - value !== null && - typeof (value as Disposable).dispose === 'function' - ) -} diff --git a/packages/bindx/src/handles/BaseHandle.ts b/packages/bindx/src/handles/BaseHandle.ts index 6dbd5ae3..6467926a 100644 --- a/packages/bindx/src/handles/BaseHandle.ts +++ b/packages/bindx/src/handles/BaseHandle.ts @@ -1,19 +1,21 @@ -import type { Disposable, DisposableGroup } from '../core/Disposable.js' import type { ActionDispatcher } from '../core/ActionDispatcher.js' import type { SnapshotStore } from '../store/SnapshotStore.js' /** * Base class for all handles. - * Provides common functionality for subscription and disposal. * - * Handles are stable objects that: + * Handles are stateless live views over the store: * - Have stable identity (same instance across renders) - * - Provide subscribe/getSnapshot for useSyncExternalStore + * - Provide subscribe/getVersion for useSyncExternalStore * - Dispatch actions for mutations + * + * They own no resources — the only subscriptions a handle creates are returned + * from `subscribe()` and owned by `useSyncExternalStore`, not stored on the + * handle. There is therefore intentionally no dispose lifecycle: a handle is + * reclaimed by GC once unreferenced, and a superseded handle (one replaced by a + * fresh instance on a data change) stays fully usable for late reads/writes. */ -export abstract class BaseHandle implements Disposable { - protected _disposed = false - +export abstract class BaseHandle { constructor( protected readonly store: SnapshotStore, protected readonly dispatcher: ActionDispatcher, @@ -28,29 +30,6 @@ export abstract class BaseHandle implements Disposable { * Get current version for change detection. */ abstract getVersion(): number - - /** - * Dispose resources (unsubscribe, cleanup). - */ - dispose(): void { - this._disposed = true - } - - /** - * Check if the handle has been disposed. - */ - get isDisposed(): boolean { - return this._disposed - } - - /** - * Throws if the handle has been disposed. - */ - protected assertNotDisposed(): void { - if (this._disposed) { - throw new Error('Handle has been disposed') - } - } } /** diff --git a/packages/bindx/src/handles/EntityHandle.ts b/packages/bindx/src/handles/EntityHandle.ts index d5db13a5..6cd8a47f 100644 --- a/packages/bindx/src/handles/EntityHandle.ts +++ b/packages/bindx/src/handles/EntityHandle.ts @@ -29,11 +29,10 @@ import { createHandleProxy } from './proxyFactory.js' import type { SelectionMeta } from '../selection/types.js' import { UnfetchedFieldError } from '../errors/UnfetchedFieldError.js' -/** Minimal internal interface for cached relation handles that need reset/dispose. - * At runtime, the proxied handles satisfy this through delegation, even if the public type doesn't expose dispose(). */ +/** Minimal internal interface for cached relation handles that need reset. + * At runtime, the proxied handles satisfy this through delegation. */ interface RelationHandleRaw { reset(): void - dispose(): void } interface CachedFieldHandle { @@ -428,7 +427,6 @@ export class EntityHandle extends Enti * Also resets all relation states (hasOne, hasMany). */ reset(): void { - this.assertNotDisposed() this.dispatcher.dispatch(resetEntity(this.entityType, this.entityId)) // Reset all cached relation handles @@ -441,27 +439,9 @@ export class EntityHandle extends Enti * Commits changes (serverData = data). */ commit(): void { - this.assertNotDisposed() this.dispatcher.dispatch(commitEntity(this.entityType, this.entityId)) } - /** - * Disposes the handle and all cached child handles. - */ - override dispose(): void { - super.dispose() - - for (const { raw } of this.fieldHandleCache.values()) { - raw.dispose() - } - this.fieldHandleCache.clear() - - for (const { raw } of this.relationHandleCache.values()) { - raw.dispose() - } - this.relationHandleCache.clear() - } - /** * Creates a proxy for field access. * Returns appropriate handle type based on schema field definition: @@ -541,7 +521,6 @@ export class EntityHandle extends Enti * Adds a client-side error to this entity. */ addError(error: ErrorInput): void { - this.assertNotDisposed() this.dispatcher.dispatch( addEntityError(this.entityType, this.entityId, createClientError(error)), ) @@ -551,7 +530,6 @@ export class EntityHandle extends Enti * Clears entity-level errors. */ clearErrors(): void { - this.assertNotDisposed() this.dispatcher.dispatch( clearEntityErrors(this.entityType, this.entityId), ) @@ -561,7 +539,6 @@ export class EntityHandle extends Enti * Clears all errors (entity-level, fields, and relations). */ clearAllErrors(): void { - this.assertNotDisposed() this.dispatcher.dispatch( clearAllErrorsAction(this.entityType, this.entityId), ) diff --git a/packages/bindx/src/handles/FieldHandle.ts b/packages/bindx/src/handles/FieldHandle.ts index d2cdcde9..5e223e59 100644 --- a/packages/bindx/src/handles/FieldHandle.ts +++ b/packages/bindx/src/handles/FieldHandle.ts @@ -124,7 +124,6 @@ export class FieldHandle extends EntityRelatedHandle { * Call this on blur or other interaction events. */ touch(): void { - this.assertNotDisposed() this.store.setFieldTouched(this.entityType, this.entityId, this.fieldName, true) } @@ -133,7 +132,6 @@ export class FieldHandle extends EntityRelatedHandle { * Also clears non-sticky client errors. */ setValue(value: T | null): void { - this.assertNotDisposed() // Clear non-sticky errors when value changes this.store.clearNonStickyFieldErrors(this.entityType, this.entityId, this.fieldName) this.dispatcher.dispatch( @@ -159,7 +157,6 @@ export class FieldHandle extends EntityRelatedHandle { * Adds a client-side error to this field. */ addError(error: ErrorInput): void { - this.assertNotDisposed() this.dispatcher.dispatch( addFieldError(this.entityType, this.entityId, this.fieldName, createClientError(error)), ) @@ -169,7 +166,6 @@ export class FieldHandle extends EntityRelatedHandle { * Clears all errors from this field. */ clearErrors(): void { - this.assertNotDisposed() this.dispatcher.dispatch( clearFieldErrors(this.entityType, this.entityId, this.fieldName), ) diff --git a/packages/bindx/src/handles/HasManyListHandle.ts b/packages/bindx/src/handles/HasManyListHandle.ts index 6ab29ce6..ae1f3e22 100644 --- a/packages/bindx/src/handles/HasManyListHandle.ts +++ b/packages/bindx/src/handles/HasManyListHandle.ts @@ -340,7 +340,6 @@ export class HasManyListHandle): string { - this.assertNotDisposed() // Pre-create entity so we can return tempId const tempId = this.store.createEntity(this.itemType, data as Record) @@ -399,7 +395,6 @@ export class HasManyListHandle is not assignable to HasManyRef. * This is a phantom property that only exists in the type system. @@ -489,7 +469,6 @@ export class HasManyListHandle * Accessible via proxy as `$create()`. */ create(data?: Partial): string { - this.assertNotDisposed() const tempId = this.store.createEntity(this.targetType, data as Record) this.dispatcher.dispatch( connectRelation(this.entityType, this.entityId, this.fieldName, tempId, this.targetType), @@ -386,7 +385,6 @@ export class HasOneHandle * Connects the relation to an entity. */ connect(targetId: string): void { - this.assertNotDisposed() this.dispatcher.dispatch( connectRelation(this.entityType, this.entityId, this.fieldName, targetId, this.targetType), ) @@ -397,7 +395,6 @@ export class HasOneHandle * Sets the FK to null — only works when the FK column is nullable. */ disconnect(): void { - this.assertNotDisposed() this.dispatcher.dispatch( disconnectRelation(this.entityType, this.entityId, this.fieldName), ) @@ -407,7 +404,6 @@ export class HasOneHandle * Marks the related entity for deletion. */ delete(): void { - this.assertNotDisposed() this.dispatcher.dispatch( deleteRelation(this.entityType, this.entityId, this.fieldName), ) @@ -432,22 +428,9 @@ export class HasOneHandle * Resets the relation to server state. */ reset(): void { - this.assertNotDisposed() this.store.resetRelation(this.entityType, this.entityId, this.fieldName) } - /** - * Disposes the handle. - */ - override dispose(): void { - super.dispose() - this.entityHandleCacheRaw?.dispose() - this.entityHandleCacheRaw = null - this.entityHandleCacheProxy = null - this.placeholderCacheRaw = null - this.placeholderCacheProxy = null - } - /** * Type brand - ensures HasOneRef is not assignable to HasOneRef. * This is a phantom property that only exists in the type system. @@ -474,7 +457,6 @@ export class HasOneHandle * Adds a client-side error to this relation. */ addError(error: ErrorInput): void { - this.assertNotDisposed() this.dispatcher.dispatch( addRelationError(this.entityType, this.entityId, this.fieldName, createClientError(error)), ) @@ -484,7 +466,6 @@ export class HasOneHandle * Clears all errors from this relation. */ clearErrors(): void { - this.assertNotDisposed() this.dispatcher.dispatch( clearRelationErrors(this.entityType, this.entityId, this.fieldName), ) diff --git a/tests/react/jsx/entityHandleDisposalAcrossRerender.test.tsx b/tests/react/jsx/entityHandleDisposalAcrossRerender.test.tsx new file mode 100644 index 00000000..25c8e06f --- /dev/null +++ b/tests/react/jsx/entityHandleDisposalAcrossRerender.test.tsx @@ -0,0 +1,63 @@ +import '../../setup' +import { describe, test, expect, afterEach } from 'bun:test' +import { render, waitFor, act, cleanup } from '@testing-library/react' +import React, { useRef } from 'react' +import { BindxProvider, MockAdapter, Entity } from '@contember/bindx-react' +import type { FieldAccessor } from '@contember/bindx' +import { getByTestId, queryByTestId, createMockData, schema, testSchema } from '../../shared' + +afterEach(() => { + cleanup() +}) + +describe(' accessor lifetime across data-driven re-render', () => { + // Models the editor integration: a consumer (e.g. the Slate BlockEditor) captures a field + // accessor handed to it during render and writes to it again on a later event. Between those + // writes the entity version changes (the first write itself bumps it), which makes + // EntityHandleRenderer recreate the EntityHandle and dispose the previous one — disposing the + // very accessor the consumer still holds. The second write must still succeed: dispose() frees + // no real resources, so a captured accessor for a still-mounted entity must remain writable. + test('a captured field accessor stays writable after the entity re-renders', async () => { + const adapter = new MockAdapter(createMockData(), { delay: 0 }) + + const captured = { field: null as FieldAccessor | null } + + function Editor({ title }: { title: FieldAccessor }) { + // Capture once, like Slate captures its onChange/field at mount time. + const ref = useRef | null>(null) + if (!ref.current) { + ref.current = title + captured.field = title + } + return {title.value} + } + + const { container } = render( + + + {article => } />} + + , + ) + + await waitFor(() => { + expect(queryByTestId(container, 'title')).not.toBeNull() + }) + + // First write through the captured accessor: succeeds and bumps the entity version, + // which triggers EntityHandleRenderer to recreate (and dispose) the handle. + act(() => { + captured.field!.setValue('first') + }) + expect(getByTestId(container, 'title').textContent).toBe('first') + + // Second write through the SAME captured accessor. Before the fix this threw + // "Handle has been disposed". + expect(() => { + act(() => { + captured.field!.setValue('second') + }) + }).not.toThrow() + expect(getByTestId(container, 'title').textContent).toBe('second') + }) +}) diff --git a/tests/unit/handles/entityHandle.test.ts b/tests/unit/handles/entityHandle.test.ts index b972b0fb..7b92d599 100644 --- a/tests/unit/handles/entityHandle.test.ts +++ b/tests/unit/handles/entityHandle.test.ts @@ -436,22 +436,4 @@ describe('EntityHandle', () => { }) }) - - // ==================== Dispose ==================== - - describe('Dispose', () => { - test('should dispose handle and clear caches', () => { - store.setEntityData('Article', 'a-1', { id: 'a-1', title: 'Test' }, true) - const handle = createEntityHandleRaw() - - // Access some fields to populate cache - handle.field('title') - handle.hasOne('author') - - handle.dispose() - - // After dispose, handle should throw on operations - expect(() => handle.reset()).toThrow() - }) - }) })