-
Notifications
You must be signed in to change notification settings - Fork 772
Simplify server-side validation of A2UI messages by adding ChildId to explicitly mark component references #510
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
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 a ComponentIdRef type to explicitly mark component ID references within the A2UI JSON schema. This is a valuable change that improves the schema's clarity and simplifies server-side validation by making it easier to identify and check structural links in the UI tree. The changes are applied consistently across the common types and the standard component catalog, and the documentation is updated to reflect this new requirement. Overall, this is a solid improvement for the protocol's robustness. I have one minor suggestion to further enhance validation.
| "ComponentIdRef": { | ||
| "type": "string", | ||
| "description": "A reference to the unique ID of another component within the same surface." | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To improve schema validation, consider adding "minLength": 1 to this definition. This would prevent empty strings from being considered valid component ID references, which is likely the intended behavior. For consistency, the same constraint could be applied to the main id definition as well.
"ComponentIdRef": {
"type": "string",
"description": "A reference to the unique ID of another component within the same surface.",
"minLength": 1
},| "description": "A template for generating a dynamic list of children from a data model list. The `componentId` is the component to use as a template.", | ||
| "properties": { | ||
| "componentId": { | ||
| "$ref": "#/$defs/id" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this PR remove the id type, since it's no longer used? Theid was meant to serve a similar purpose, with perhaps a too-generic name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes! Ummm sorry I was vibecoding too freely and missed that there was an ID type already defined right above my new one.
|
|
||
| To ensure that automated validators can verify the integrity of your UI tree (checking that parents reference existing children), custom catalogs MUST adhere to the following strict typing rules: | ||
|
|
||
| 1. **Single Child References:** Any property that holds the ID of another component MUST use the `ComponentIdRef` type defined in `common_types.json`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be more specific to name it childIdRef?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about just ComponentId?
I like the idea of including "Ref" in the name, to imply that it's a validated reference. But I think it's even neater if we can share the same type between declarations and references, in which case a generic name makes more sense.
I think when we write a validator, we can have special cased logic so that it knows that the root id parameter of each Component is a declaration (seeing as this is part of the core A2UI specification), but any other parameters with type ComponentId must be references.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me. Thanks!
This PR introduces a dedicated
ChildIdtype to thecommon_types.jsonschema and updates the standard catalog and documentation to use it. It also completely removes the redundantidtype definition, replacing all references withChildId.Rationale
In A2UI v0.9, validating referential integrity (ensuring that a component's
childortriggerproperty points to a valid ID in the same surface) currently relies on heuristics. A validator has to "guess" which fields represent IDs based on naming conventions (e.g., checking if a property is namedchild,children,content, ortrigger).This approach is brittle, especially for Custom Catalogs, where a developer might introduce a new component (e.g.,
SplitView) with properties likeleftPaneandrightPane. A generic validator would not know these are structural links that need to be checked.The Fix
We introduce a semantic marker type in
common_types.json:We then update the standard catalog, the definition of
ChildList, andComponentCommonto explicitly$refthis type. This ensures that everyidproperty and every property that refers to anidis explicitly typed.Validator Logic (Hypothetical)
With this change, a validator can be implemented deterministically without hardcoded property names. It simply inspects the resolved schema for each property:
This ensures that any component in any catalog (standard or custom) that uses these types will automatically benefit from referential integrity checks.