feat: implement node layer architecture#1294
feat: implement node layer architecture#1294jacobsimionato wants to merge 4 commits intogoogle:mainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a "Node Layer" to the A2UI framework, refactoring the React renderer and the core GenericBinder to use fully resolved A2uiNode instances. This architectural shift centralizes component lifecycle management and property resolution through a reference-counted NodeManager. Feedback identifies a critical memory leak in template-based ChildList updates and a bug in A2uiSurface where the isDestroyed state fails to update. Additionally, there are concerns regarding the performance of global event subscriptions in GenericBinder and suggestions to improve type safety by avoiding any usages in internal state.
| const resolvedChildren = arr.map((_, i) => { | ||
| const childPath = listContext.nested(String(i)).path; | ||
| if (this.nodeResolver) { | ||
| const node = this.nodeResolver.resolveNode( | ||
| value.componentId, | ||
| childPath, | ||
| ); | ||
| this.resolvedNodes.add(node); | ||
| return node; | ||
| } | ||
| return { | ||
| id: value.componentId, | ||
| basePath: childPath, | ||
| }; | ||
| }); | ||
|
|
||
| // Cleanup old nodes from this specific prop that are no longer present | ||
| if (this.nodeResolver && Array.isArray(oldNodes)) { | ||
| for (const oldNode of oldNodes) { | ||
| if (!resolvedChildren.includes(oldNode)) { | ||
| this.nodeResolver.releaseNode(oldNode); | ||
| this.resolvedNodes.delete(oldNode); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This logic introduces a memory leak in template-based ChildList updates. resolveNode (line 354) increments the reference count of the node. If a node persists across updates, its count is incremented again, but it is never decremented because the cleanup loop (line 369) skips nodes present in resolvedChildren. Consequently, reference counts will grow indefinitely, preventing disposal. You should ensure that for nodes remaining in the list, the reference count is balanced (e.g., by releasing the old reference before or after resolving the new one).
| const isDestroyed = useSyncExternalStore( | ||
| useCallback((cb: () => void) => { | ||
| let active = true; | ||
| const sub = node.onDestroyed.subscribe(() => { | ||
| if (active) cb(); | ||
| }); | ||
| return () => { | ||
| unsub1.unsubscribe(); | ||
| unsub2.unsubscribe(); | ||
| active = false; | ||
| sub.unsubscribe(); | ||
| }; | ||
| }, | ||
| getSnapshot: () => { | ||
| const comp = surface.componentsModel.get(id); | ||
| // We use instance identity + version as the snapshot to ensure | ||
| // type replacements (e.g. Button -> Text) trigger a re-render. | ||
| return comp ? `${comp.type}-${version}` : `missing-${version}`; | ||
| }, | ||
| }; | ||
| }, [surface, id]); | ||
| }, [node]), | ||
| () => false // It's only true if the callback fires, causing unmount | ||
| ); |
There was a problem hiding this comment.
The isDestroyed logic using useSyncExternalStore is flawed because the getSnapshot function (line 65) always returns false. When the onDestroyed event fires, React triggers a re-render and calls getSnapshot, which still returns false, meaning isDestroyed never becomes true. Since A2uiNode lacks a synchronous isDisposed property, you should use useState and useEffect to track this state.
| const sub = this.context.surfaceComponents.onCreated.subscribe(() => { | ||
| this.rebuildAllBindings(); | ||
| }); | ||
| this.componentsUnsub = () => sub.unsubscribe(); |
There was a problem hiding this comment.
Subscribing to the global onCreated event of surfaceComponents in every GenericBinder instance is a performance concern. Every time any component is added to the surface, every active binder will trigger a full rebuildAllBindings(). In large surfaces, this leads to O(N^2) work. Consider debouncing the rebuild or subscribing only to the specific component IDs that this binder needs to resolve.
| private isConnected = false; | ||
|
|
||
| // Track nodes resolved by this binder to manage their lifecycle | ||
| private resolvedNodes: Set<any> = new Set(); |
There was a problem hiding this comment.
Avoid using any for the resolvedNodes set. Explicitly typing it as Set<A2uiNode> improves type safety and clarity.
| private resolvedNodes: Set<any> = new Set(); | |
| private resolvedNodes: Set<A2uiNode> = new Set(); |
| */ | ||
| export class NodeImpl<TProps = Record<string, any>> implements A2uiNode<TProps> { | ||
| private readonly _onDestroyed = new EventEmitter<void>(); | ||
| private readonly _props = signal<TProps>({} as TProps); |
There was a problem hiding this comment.
Initializing _props with {} as TProps is unsafe as it hides potential missing required properties in TProps. While the constructor immediately overwrites this with the binder's snapshot, it's better to initialize with a more explicit 'empty' state or handle the initialization purely within the constructor.
| private readonly _props = signal<TProps>({} as TProps); | |
| private readonly _props = signal<TProps>(undefined as any); |
1 participant
Description of Changes
This PR implements the Node Layer architecture as defined in the A2UI v0.9 specification (
node_layer_design.md). It fundamentally shifts the A2UI client architecture from a flat adjacency list with manual binders to a centrally-managed, reactive view hierarchy.Key features include:
web_coreEnhancements:A2uiNodeandNodeManagerto encapsulate the mapping between rawComponentModels and fully resolved, reactive node instances.GenericBinderto identify and resolve structural properties (likechildren,child,trigger) automatically, supporting both static arrays and template-based expansion from the data model.NodeManager, ensuring reactive data model subscriptions are robustly managed and recursively disposed of when components unmount, fixing potential memory leaks.SurfaceModelnow exports a reactiverootNodesignal, acting as the entry point for UI frameworks.reactRenderer Migration:A2uiSurfaceand the React adapter to natively consume and observeA2uiNodeinstances.<NodeRenderer />for seamless, recursive rendering of child nodes.buildChildfunction to the adapter props. This ensures existing components (and the Basic Catalog) continue to work while mapping seamlessly to the new Node Layer under the hood.Validation & Testing:
web_coreverifying 1-to-N node expansion, reference counting for shared nodes, and recursive destruction.a2ui_explorerapplication demonstrating progressive rendering and dynamic reactivity across the entire stack.Rationale
The previous architecture forced every framework adapter to manually manage tree recursion, templated list expansions, and the complex lifecycle of reactive binders. This led to high complexity, inconsistencies, and risks of memory leaks.
By pushing the
Nodelogic into the framework-agnosticweb_core, we drastically simplify the requirements for native view adapters (React, Flutter, etc.). They now receive a strictly-typed, fully resolvedA2uiNodeand merely need to map its properties to native UI components.Testing/Running Instructions
npm install && npm run buildfrom the root or insiderenderers/web_coreandrenderers/react.renderers/react/a2ui_explorer.npm testto execute the integration test suite.npm run devto launch the Gallery Application and manually verify the interactive samples and stepper functionality.