Skip to content

Node layer for a2ui_core + Flutter#1011

Draft
andrewkolos wants to merge 4 commits into
flutter:mainfrom
andrewkolos:node-layer-dart
Draft

Node layer for a2ui_core + Flutter#1011
andrewkolos wants to merge 4 commits into
flutter:mainfrom
andrewkolos:node-layer-dart

Conversation

@andrewkolos

Copy link
Copy Markdown
Collaborator

Not ready for review. Submitting this early as a draft so it's easier to reference by implementers of other renderers/adapters. The same prototype exists for web_core + React as a2ui-project/a2ui#2077.

(ai-generated; rewrite coming later)

What it is

The node layer compiles a surface's flat component map plus data model into a live tree of ComponentNodes whose resolved properties update reactively. NodeSurface ships beside the existing Surface, with no changes to it.

The approach

Nodes are long-lived, each with its own props signal. NodeSurface wraps each node's subtree in a builder subscribed to that node's props, so a data write rebuilds only the node whose resolved properties changed, never the widget tree. The existing Surface rebuilds the whole widget tree from an immutable snapshot on every update; NodeSurface is the granular sibling, so per-node updates are achievable in a declarative UI framework without whole-surface rebuilds.

What's here

  • a2ui_core: ComponentNode + NodeResolver, with the conformance suite ported from web_core (node_resolver_test.dart) covering identity, notification granularity, placeholders, templates, and late action resolution.
  • genui: NodeSurface, reusing the existing catalog views unchanged, each node subscribed to its own props signal (node_surface_test.dart).
  • examples/simple_chat: opts into NodeSurface behind --dart-define=nodes=true for manual end-to-end runs.

Port the node layer from web_core: NodeResolver turns a surface's flat
component map into a live tree of resolved ComponentNodes. Child
references resolve to live nodes, templates spawn one node per array
item, missing components render as placeholders upgraded in place, and
node identity is parent-scoped so shared components never tear down a
sibling's subtree. Ref-field classification derives from the REF:
description pointers already present in CommonSchemas.

Includes the cross-language conformance suite (27 tests) ported from
web_core's node-resolver.test.ts.
NodeSurface renders the live resolved tree from a NodeResolver instead
of rebuilding from an immutable SurfaceDefinition snapshot: each node's
subtree rebuilds only when that node's own resolved properties change.
Catalog views are reused unchanged; the node layer supplies resolved
children (template expansion, placeholder upgrades, list
reconciliation), and child references the schema does not mark fall
back to the legacy id-walking path.
SurfaceController gains liveSurfaceFor, exposing the live core surface
model NodeSurface renders from. simple_chat renders surfaces through
NodeSurface when built with --dart-define=nodes=true; the default build
is unchanged. NodeSurface logs on attach so runs can confirm which path
rendered.
… the node-layer contract

Resolve a missing catalog function to null with an EXPRESSION_ERROR event
instead of an uncaught throw during node creation, and stop re-processing a
functionCall action's result in SurfaceModel.dispatchAction (the action
closure already executes it through the evaluator). Adds conformance tests
for both.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces an experimental node-based rendering layer (NodeSurface and NodeResolver) to replace the legacy Surface rendering in genui. Instead of rebuilding the entire widget tree from immutable snapshots on every update, the new node layer resolves a live tree of ComponentNodes, allowing subtrees to rebuild reactively only when their own resolved properties change. The review feedback focuses on improving the robustness and performance of this new layer, specifically by simplifying the NodeResolver constructor, guarding addCleanup against post-disposal calls, copying the cleanup list during disposal to prevent ConcurrentModificationErrors, and optimizing _shallowEqual to avoid unnecessary MapEntry allocations.

Comment on lines +84 to +90
NodeResolver(this._surface, this._catalog) {
if (!identical(_catalog, _surface.catalog)) {
throw A2uiStateError(
'NodeResolver requires the same catalog instance its surface was '
'constructed with.',
);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The _catalog parameter in the NodeResolver constructor is redundant because it is strictly required to be identical to _surface.catalog. We can simplify the constructor signature by removing this parameter and initializing the _catalog field directly from _surface.catalog in the initializer list. This reduces the risk of mismatched catalog errors and simplifies instantiation across the codebase.

Suggested change
NodeResolver(this._surface, this._catalog) {
if (!identical(_catalog, _surface.catalog)) {
throw A2uiStateError(
'NodeResolver requires the same catalog instance its surface was '
'constructed with.',
);
}
NodeResolver(this._surface) : _catalog = _surface.catalog {

Comment on lines +73 to +75
void addCleanup(void Function() cleanup) {
_cleanups.add(cleanup);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If addCleanup is called on an already disposed node, it will append the cleanup function to _cleanups which will never be executed. We should guard addCleanup to ignore or warn when called after disposal.

Suggested change
void addCleanup(void Function() cleanup) {
_cleanups.add(cleanup);
}
void addCleanup(void Function() cleanup) {
if (_disposed) {
_log.warning('addCleanup called on a disposed ComponentNode ($instanceId)');
return;
}
_cleanups.add(cleanup);
}

Comment on lines +93 to +110
void dispose() {
if (_disposed) {
return;
}
_disposed = true;
for (final void Function() cleanup in _cleanups) {
try {
cleanup();
} catch (error, stackTrace) {
// A failing cleanup must not prevent the remaining ones from running.
_log.severe(
'ComponentNode cleanup error ($instanceId)',
error,
stackTrace,
);
}
}
_cleanups = [];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Iterating directly over _cleanups while executing cleanup functions can lead to a ConcurrentModificationError if any cleanup function triggers a modification of the _cleanups list (e.g., by transitively calling addCleanup or disposing related resources). To make this robust, copy the cleanups to a temporary list using List.of(_cleanups) and clear the original list before executing them.

  void dispose() {
    if (_disposed) {
      return;
    }
    _disposed = true;
    final cleanups = List<void Function()>.of(_cleanups);
    _cleanups.clear();
    for (final void Function() cleanup in cleanups) {
      try {
        cleanup();
      } catch (error, stackTrace) {
        // A failing cleanup must not prevent the remaining ones from running.
        _log.severe(
          'ComponentNode cleanup error ($instanceId)',
          error,
          stackTrace,
        );
      }
    }
  }

Comment on lines +161 to +174
bool _shallowEqual(NodeProps a, NodeProps b) {
if (identical(a, b)) {
return true;
}
if (a.length != b.length) {
return false;
}
for (final MapEntry<String, Object?> entry in a.entries) {
if (!b.containsKey(entry.key) || !sameValue(entry.value, b[entry.key])) {
return false;
}
}
return true;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Iterating over a.entries allocates a MapEntry object for every property of the node on every shallow equality check. Since _shallowEqual is called frequently during updates, we can optimize this by iterating over a.keys instead, which avoids these unnecessary allocations.

Suggested change
bool _shallowEqual(NodeProps a, NodeProps b) {
if (identical(a, b)) {
return true;
}
if (a.length != b.length) {
return false;
}
for (final MapEntry<String, Object?> entry in a.entries) {
if (!b.containsKey(entry.key) || !sameValue(entry.value, b[entry.key])) {
return false;
}
}
return true;
}
bool _shallowEqual(NodeProps a, NodeProps b) {
if (identical(a, b)) {
return true;
}
if (a.length != b.length) {
return false;
}
for (final String key in a.keys) {
if (!b.containsKey(key) || !sameValue(a[key], b[key])) {
return false;
}
}
return true;
}

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