-
Notifications
You must be signed in to change notification settings - Fork 735
Generalize A2UI Actions (v0.9) #500
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
base: main
Are you sure you want to change the base?
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 generalizes the A2UI action concept by moving it to a shared type in common_types.json and adding support for local client-side actions. The changes to the JSON schemas are well-structured and the refactoring in standard_catalog.json is correct. The documentation has also been updated to reflect these new capabilities.
My review includes two main points. First, the examples in the updated documentation for the new action types on Button components are incorrect as they use a non-existent text property instead of the required child property. Second, the pull request is missing tests for the new local action functionality, which is required by the repository's contribution guidelines. Addressing these points will improve the accuracy of the documentation and ensure the new feature is properly tested.
| "Action": { | ||
| "description": "Defines an interaction handler that can either trigger a server-side event or execute a local client-side function.", | ||
| "oneOf": [ | ||
| { | ||
| "type": "object", | ||
| "description": "Triggers a server-side event.", | ||
| "properties": { | ||
| "name": { | ||
| "type": "string", | ||
| "description": "The name of the action to be dispatched to the server." | ||
| }, | ||
| "context": { | ||
| "type": "object", | ||
| "description": "A JSON object containing the key-value pairs for the action context. Values can be literals or paths. Use literal values unless the value must be dynamically bound to the data model. Do NOT use paths for static IDs.", | ||
| "additionalProperties": { | ||
| "$ref": "#/$defs/DynamicValue" | ||
| } | ||
| } | ||
| }, | ||
| "required": ["name"], | ||
| "additionalProperties": false | ||
| }, | ||
| { | ||
| "type": "object", | ||
| "description": "Executes a local client-side function.", | ||
| "properties": { | ||
| "function": { | ||
| "type": "string", | ||
| "description": "A string specifying the local function to trigger (e.g., 'openUrl(${/url})'). This string can represent nested function calls and data model references." | ||
| } | ||
| }, | ||
| "required": ["function"], | ||
| "additionalProperties": false | ||
| }, | ||
| { | ||
| "type": "string", | ||
| "description": "Shorthand for a local client-side function execution (e.g. 'openUrl(${/url})')." | ||
| } | ||
| ] | ||
| } |
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.
This pull request introduces a significant new feature: local actions, with both an object and a string shorthand form. However, there are no new test cases being added to verify this new functionality. Adding tests for the new local action formats is important to ensure they are parsed and handled correctly by clients.
References
- The repository style guide requires that code changes are accompanied by tests. This change adds a new feature without corresponding tests. (link)
| "component": "Button", | ||
| "text": "Submit", |
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.
The Button component examples in the "Defining Actions" section are incorrect. According to the standard_catalog.json schema, Button does not have a text property. Instead, it requires a child property which should be the ID of a child component (like Text). This issue is present in the "Server Actions", "Local Actions (String Shorthand)", and "Local Actions (Object Definition)" examples.
For example, the server action snippet should be structured like this:
{
"component": "Button",
"child": "submit_button_label",
"action": {
"name": "submit_form",
"context": {
"itemId": "123"
}
}
}...where submit_button_label would be the ID of a Text component defined separately (e.g., {"id": "submit_button_label", "component": "Text", "text": "Submit"}).
Please correct all three examples to align with the Button component's schema.
| "component": "Button", | |
| "text": "Submit", | |
| "component": "Button", | |
| "child": "submit_button_label", |
Generalizes the concept of A2UI actions by moving the
actionschema from theButtoncomponent tocommon_types.jsonas a sharedActiontype.This new
Actiontype supports:nameandcontext).functionparameter (string or object).Example of local action usage:
"action": "openUrl(${/url})"This allows components other than Button to potentially define actions in the future, and enables local client-side behavior.