Repository navigation
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the ExpressCompiler and ExpressDecompiler to support userMessage and context keyword arguments on Event(...) for protocol v1.0, and enables compiling nested action slots (such as inside list items) as functionCall actions. The review feedback suggests clarifying an error message in ExpressCompiler when invalid keyword arguments are passed to Event(...), as the current message incorrectly lists the positional-only name parameter as an available parameter.
85db253 to
51fdad2
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the ExpressCompiler and ExpressDecompiler to support userMessage and context keyword arguments on Event(...) for protocol version 1.0 and later, including strict validation and error handling for unsupported or duplicate arguments. Additionally, it enhances sub-schema resolution via CatalogSchemaHelper.resolveSubschema to correctly compile nested action slots (such as those inside list items or object properties) as functionCall actions. Comprehensive unit and conformance tests have been added to verify these changes. No review comments were provided, so there is no feedback to address.
51fdad2 to
8c35b42
Compare
nan-yu
left a comment
There was a problem hiding this comment.
This is a clean port: _Slot and _Context.sub fit the way Dart already passes context around, and switching the decompiler from _property to _value is a tidy way to reach nested action slots. A function call inside an event context stays a plain call, which I checked. The full dart test run passes on the branch.
One issue carried over from #3634 and #3635, and one missing changelog entry, inline:
Must address
userMessagerejects function calls, which v1.0 allows (DynamicString). Dart's decompiler also writes such an event differently from TS and Python.
Should address
2. Event(name=...) used to work in Dart and now throws. The CHANGELOG should record it as Breaking:.
| userMessageNode, | ||
| context.child('userMessage'), | ||
| ); | ||
| if (userMessage is! String && !_isBinding(userMessage)) { |
There was a problem hiding this comment.
Must address: function calls are rejected (same as #3634 and #3635).
In v1.0, Action.event.userMessage is $ref: #/$defs/DynamicString, which is oneOf a string, a DataBinding or a FunctionCall. This check only accepts a string or a binding. On this branch, root = Button(label, Event("r", userMessage=concat("a", "b"))) throws A2uiValidationError: The userMessage of Event must be a string or a data path.
Could this also accept a compiled function call, fixed together with the TS and Python PRs so the three SDKs stay in agreement?
There was a problem hiding this comment.
_eventnow accepts_isCall(userMessage)alongsideStringand_isPath(userMessage, context).- Replaced the raw
userMessageNodeAST interpolation in the error message with_describe(userMessageNode)(e.g.number 42,boolean true,list,map).
| (context == null || (context is Map && context.isNotEmpty)) && | ||
| (userMessage == null || | ||
| userMessage is String || | ||
| _binding(_asMap(userMessage)) != null); |
There was a problem hiding this comment.
Must address (same issue, decompiler side): the three SDKs write this event differently.
Because this check doesn't allow a function call, a valid v1.0 action {event: {name: "r", userMessage: {"@call": "concat", "args": {...}}}} isn't treated as an event. It's written as a raw map literal:
root = Button(label, {event: {name: "r", userMessage: concat("x", "y")}})
That compiles back, so the Dart round trip holds. But TS and Python write Event("r", userMessage=concat("x", "y")) for the same payload. Once the compiler accepts a function call, this check can allow one too, so all three write Event(..., userMessage=fn(...)).
There was a problem hiding this comment.
isEvent now accepts _call(_asMap(userMessage)) != null in addition to String and _path(_asMap(userMessage)) != null, so {event: {name: "r", userMessage: {"@call": ...}}} decompiles to Event("r", userMessage=formatString(...)).
polina-c
left a comment
There was a problem hiding this comment.
Comments are inline, most significant first. The first five are behavior issues: the items key collision in resolveSubschema, userMessage narrower than DynamicString, Event(name=...) no longer compiling, the v0.9 decompile round-trip, and the decompiler not following $refs the way the compiler now does. The rest are about tests and cleanup.
🤖 Agent-generated
|
|
||
| Map<String, Object?>? resolve(Map<String, Object?>? s) { | ||
| if (s == null) return null; | ||
| if (key == 'items') { |
There was a problem hiding this comment.
A map entry whose key is literally items takes this branch too, because map entries pass their own key here (context.sub(entry.key, entry.key)). An object property named items is then looked up as schema['items'], never as properties['items'].
For example, with a catalog property menu: {type: object, properties: {items: {type: array, items: {properties: {label: ..., action: {$ref: Action}}}}}}, the Express Menu(menu: {items: [{label: "a", action: openUrl("x")}]}) finds no slot for items, so openUrl(...) compiles to a bare call and not to {functionCall: ...}.
Could array items use a key that can't collide with a property name, such as a separate item method or a null key?
🤖 Agent-generated
There was a problem hiding this comment.
Good catch, and it was the same in TypeScript and Python. All three now have separate item and property resolution (resolveItemSchema / resolvePropertySchema), with a test for a property named items whose item schema is a local $ref.
| userMessageNode, | ||
| context.child('userMessage'), | ||
| ); | ||
| if (userMessage is! String && !_isBinding(userMessage)) { |
There was a problem hiding this comment.
The v1.0 schema types event.userMessage as DynamicString (common_types.json), which also allows a function call. With this check, Event("save", userMessage=formatString("Saved ${/name}")) throws even though the compiled action would be valid.
The decompiler has the same limit at decompiler.dart:581-583: such an event is written as a {event: {...}} map literal instead of Event(...).
🤖 Agent-generated
There was a problem hiding this comment.
Agreed. Before v1.0 the decompiler now leaves such an event as a map literal, in all three SDKs, so it compiles back as written. The lowered-v0.9 conformance cases still have to be skipped, since their expect_contains is the Event(..., userMessage=...) text that only v1.0 writes.
| final arguments = <String>[ | ||
| _string(name), | ||
| if (context != null) _value(context, null), | ||
| if (userMessage != null) 'userMessage=${_value(userMessage, null)}', |
There was a problem hiding this comment.
This writes userMessage= at any version, but the compiler rejects userMessage before v1.0 (compiler.dart:1110-1115). A v0.9 payload with {event: {name: "x", userMessage: "hi"}} used to decompile to a map literal that recompiled. It now becomes Event("x", userMessage="hi"), which fails to compile at v0.9.
Should isEvent also require _v1 when userMessage is set, so v0.9 falls back to the map literal? That might also let the lowered v0.9 conformance case run instead of being skipped.
🤖 Agent-generated
There was a problem hiding this comment.
isEvent now requires _v1 when userMessage != null, so in v0.9 an event object carrying userMessage falls back to a map literal instead of emitting Event(..., userMessage=...) that the v0.9 compiler would reject. Added a v0.9 unit test in express_decompiler_test.dart covering this.
(Note: the lowered v0.9 conformance cases in express_conformance_test.dart stay in _v1OnlyCases because their shared expect_contains assertions look for the Event(..., userMessage=...) string, which is only emitted in v1.0.)
| } | ||
|
|
||
| String _value(Object? value, Map<String, Object?>? schema) { | ||
| if (isAction(schema) && value != null) return _action(value); |
There was a problem hiding this comment.
The compiler now finds nested slots with resolveSubschema, which follows $ref, allOf, oneOf and anyOf. The decompiler still reaches nested schemas through _propertySchema and _itemSchema, which read only direct properties and items.
So if a catalog writes the item schema as items: {$ref: '#/$defs/LinkItem'}, the compiler wraps openUrl(...) in functionCall, but decompiling that JSON writes a {functionCall: ...} map literal instead of openUrl(...). Could the decompiler use helper.resolveSubschema as well?
🤖 Agent-generated
There was a problem hiding this comment.
_Component now holds CatalogSchemaHelper? _helper and delegates property(name) and item() to _helper?.resolvePropertySchema(_schema, name) and _helper?.resolveItemSchema(_schema). Removed the standalone _propertySchema and _itemSchema helpers, and added a decompiler test where items is a $ref to #/$defs/NavItem with a functionCall action.
| contains('userMessage requires protocol v1.0'), | ||
| ), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
These tests use only inline items and properties schemas. Nothing covers the $ref, allOf/oneOf/anyOf, additionalProperties and self-reference paths of resolveSubschema, or a map key named items. The items key collision and the decompiler mismatch from the other comments both pass the current suite.
🤖 Agent-generated
There was a problem hiding this comment.
Added unit tests in express_test.dart and express_decompiler_test.dart covering:
itemsas a$refto#/$defs/NavItem(both compiler and decompiler)allOf/anyOfcombinators andadditionalPropertiessubschemas- self-referential
$defs(#/$defs/Loop->#/$defs/Loop) terminating cleanly without stack overflow - an object property literally named
items(both compiler and decompiler)
| return _Context( | ||
| idPrefix: '${idPrefix}_$suffix', | ||
| hoisted: hoisted, | ||
| isAction: sub?.holdsAction ?? false, |
There was a problem hiding this comment.
isAction is now always derived from the slot: this call passes sub?.holdsAction, and the property context passes isAction(schema) next to _Slot(schema, helper). Could isAction become a getter, bool get isAction => slot?.holdsAction ?? false;, so a future _Context(...) can't set one without the other?
🤖 Agent-generated
There was a problem hiding this comment.
Removed the isAction field and copyWith parameter from _Context and replaced it with bool get isAction => slot?.holdsAction ?? false;.
| ArrayNode(:final List<ExpressNode> items) => [ | ||
| for (final (int index, ExpressNode item) in items.indexed) | ||
| _value(item, context.child(index)), | ||
| _value(item, context.sub(index, 'items')), |
There was a problem hiding this comment.
context.sub(index, 'items') resolves the item schema again for every element, walking the same $ref and combinator tree each time, though the result is the same for the whole array. Resolving the item slot once per ArrayNode and building each item's context from it would avoid that for long lists.
🤖 Agent-generated
There was a problem hiding this comment.
Added _list(ArrayNode node, _Context context) which computes context.withSlot(context.slot?.item) once before mapping over node.elements.
| return more.cast<String, Object?>(); | ||
| } | ||
| } | ||
| if (_refName(s) case final String ref when visitedRefs.add(ref)) { |
There was a problem hiding this comment.
Optional, not blocking: this looks up $defs by the ref's last path segment, as isAction and the other helpers in this file already do. a2ui_core's ReferenceSchemaReader resolves refs as JSON pointers with cycle guards. A ref like #/$defs/Foo/properties/bar would resolve to defs['bar'] here. Moving subschema resolution into core could be its own change.
🤖 Agent-generated
There was a problem hiding this comment.
Keeping _refName here for now so it stays consistent with the other 7 _refName call sites in CatalogSchemaHelper (which currently only holds the raw catalogSchema map rather than a Catalog / ReferenceSchemaReader instance). Happy to follow up in a separate PR to wire ReferenceSchemaReader through CatalogSchemaHelper across all $defs lookups at once!
8c35b42 to
564e4b0
Compare
564e4b0 to
2295130
Compare
… Express
`Event("reply", userMessage="...")` silently dropped `userMessage`, and a
function call used as the action of a list item (for example
`Links([{label: "Docs", action: openUrl("...")}])`) compiled to a bare
`{"@call": ...}` that the processor rejects.
- `Event` now accepts `userMessage=` and `context=` keyword arguments.
`userMessage` compiles to the v1.0 `event.userMessage` field and throws
`A2uiValidationError` before v1.0; any other keyword on `Event` is
rejected rather than dropped.
- `_Context` carries the catalog schema at the current position, so an
`Action`-typed sub-schema inside array items or object properties wraps
function calls in `functionCall` like a top-level action slot does.
`CatalogSchemaHelper.resolveSubschema` descends into `items` and
properties through `$ref`s and `allOf`/`oneOf`/`anyOf`.
- `ExpressDecompiler` writes `userMessage=` back so events round-trip, and
writes a `functionCall` nested in a list item as a plain call.
- The Express conformance harness runs the cases that carry `userMessage`
as written in v1.0 only, since the v0.9 Action has no such field.
Fixes points 1 and 2 of #3598 for the Dart SDK.
The name is positional, so listing it among the parameters an unknown keyword could have been contradicted the error itself.
Action.event.userMessage is a DynamicString: a string, a data binding or a function call. The compiler accepted only the first two, and the decompiler wrote such an event as a map literal where TypeScript and Python write Event(..., userMessage=fn(...)). Both now take a call. The error for any other value names what the model wrote. Record the removal of Event(name=...) as a breaking change.
resolveSubschema took the key 'items' to mean the array items, so an object property named items was never looked up among the properties. It is now resolveItemSchema and resolvePropertySchema, the item slot is resolved once per array, and _Context derives isAction from its slot so the two cannot be set apart. The decompiler reaches nested schemas through the same helpers, so an item schema written as a $ref writes its action slots as calls, as the compiler reads them. Before v1.0 it writes an event carrying a userMessage as a map literal, since that version's compiler rejects userMessage= and the literal compiles back as written.
2295130 to
e06b0de
Compare
Dart mirror of the
@a2ui/agentfix for points 1 and 2 of #3598.EventacceptsuserMessage=andcontext=;userMessagecompiles to the v1.0event.userMessagefield and throwsA2uiValidationErrorbefore v1.0, as does any keywordEventdoes not define (rather than dropping it) and a context given twice._Contextcarries a_Slot(schema + helper) and derives child contexts for array items and map entries through the newCatalogSchemaHelper.resolveSubschema, so anAction-typed sub-schema inside a list item wraps function calls infunctionCalllike a top-level action slot does.ExpressDecompilerwritesuserMessage=back and writes afunctionCallnested in a list item as a plain call.userMessagein_v1OnlyCases, since the lowered-to-v0.9 run cannot express them.Tests
New unit tests in
test/express_test.dartandtest/express_decompiler_test.dart.dart analyzeclean,dart formatclean,dart test: 1375 passed. The new conformance cases from the last PR of this stack pass against this branch (366 passed, 18 skipped).Pre-launch Checklist
One time:
For this PR:
If you need help, consider asking for advice on the discussion board.
Stack created with GitHub Stacks CLI • Give Feedback 💬