-
Notifications
You must be signed in to change notification settings - Fork 810
feat: Support component deletion via component: null #528
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?
feat: Support component deletion via component: null #528
Conversation
Update server_to_client.json schema to allow component: null. Update protocol documentation. Add component_deletion.json test cases. Update evaluation framework to support and test this feature.
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 valuable feature to support component deletion using a component: null tombstone. The implementation is solid, with corresponding updates to the protocol documentation, JSON schema, and evaluation logic. I have a couple of suggestions to improve the clarity and consistency of the documentation. Additionally, please remember to fill out the Pre-Review Checklist in the pull request description as required by the repository's contribution guidelines.
| ### `updateComponents` | ||
|
|
||
| This message provides a list of UI components to be added to or updated within a specific surface. The components are provided as a flat list, and their relationships are defined by ID references in an adjacency list. This message may only be sent to a surface that has already been created. Note that components may reference children or data bindings that do not yet exist; clients should handle this gracefully by rendering placeholders (progressive rendering). | ||
| This message provides a list of UI components to be added, updated, or removed within a specific surface. The components are provided as a flat list, and their relationships are defined by ID references in an adjacency list. This message may only be sent to a surface that has already been created. |
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 previous version of this paragraph included an important note about progressive rendering: "Note that components may reference children or data bindings that do not yet exist; clients should handle this gracefully by rendering placeholders (progressive rendering)."
While this concept is mentioned elsewhere in the documentation, it is highly relevant in the updateComponents section. Its removal could cause confusion for developers implementing a client. I recommend re-adding a concise version of this note to ensure client implementation details are clear in this context.
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.
Yes, we should keep the part describing progressive rendering.
| { | ||
| "id": "user_title", | ||
| "component": "Text", | ||
| "text": "Software Engineer" | ||
| "id": "old_component_id", | ||
| "component": null | ||
| } |
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 updated example is a bit confusing. The root component (defined on lines 172-174) references a component with id: "user_title" in its children array. However, this change removes the user_title component and replaces it with a deletion request for old_component_id. This leaves a dangling reference in root and it's not clear what old_component_id refers to.
To make the example clearer and self-consistent, I recommend showing the deletion of an existing component from the example, like user_title. You would also need to update the root component to remove user_title from its children array for a fully consistent state.
Here is a suggestion to make the intent clearer, though the root component would still need to be updated separately.
| { | |
| "id": "user_title", | |
| "component": "Text", | |
| "text": "Software Engineer" | |
| "id": "old_component_id", | |
| "component": null | |
| } | |
| "id": "user_title", | |
| "component": null |
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 is a valid issue don't remove user_title: it's referenced.
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. Can you update the example so that it actions this feedback by reinstating user_title, then adding a second json block that includes messages to update the root component to remove the reference to user_title, and then delete user_title?
|
This addresses #235 |
| "component": "Text", | ||
| "text": "Software Engineer" | ||
| "id": "old_component_id", | ||
| "component": null |
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.
why wouldn't it work to just exclude component?
{
"id": "old_component_id"
}
updateDataModel overwrites instead of merging, and it treats null as remove key, so it should have the same effect, no?
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 that's a great idea! Let's make the "component" property optional, but include description for that property that makes it clear that it should only be omitted to indicate deleting an existing component. I think I should run an eval on that too to make sure that removing the "required" aspect does confuse LLMs too much.
|
How do we expect this to be used? Will we be prompting the LLM to do garbage collection? Wouldn't it be more effective to just have a message from the LLM to the client that it believes all relevant components are being referenced, and the client is free to do garbage collection? I have doubts that the LLM will be able (or have the information to) keep track of all component references. |
| ### `updateComponents` | ||
|
|
||
| This message provides a list of UI components to be added to or updated within a specific surface. The components are provided as a flat list, and their relationships are defined by ID references in an adjacency list. This message may only be sent to a surface that has already been created. Note that components may reference children or data bindings that do not yet exist; clients should handle this gracefully by rendering placeholders (progressive rendering). | ||
| This message provides a list of UI components to be added, updated, or removed within a specific surface. The components are provided as a flat list, and their relationships are defined by ID references in an adjacency list. This message may only be sent to a surface that has already been created. |
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.
Yes, we should keep the part describing progressive rendering.
|
|
||
| - `surfaceId` (string, required): The unique identifier for the UI surface to be updated. This is typically a name with meaning (e.g. "user_profile_card"), and it has to be unique within the context of the GenUI session. | ||
| - `components` (array, required): A list of component objects. The components are provided as a flat list, and their relationships are defined by ID references in an adjacency list. | ||
| - `components` (array, required): A list of component objects (for updates) or tombstone objects (for deletions). |
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.
What about the adjacency list description? I think we should keep that.
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, let's reinstate that sorry!
| { | ||
| "id": "user_title", | ||
| "component": "Text", | ||
| "text": "Software Engineer" | ||
| "id": "old_component_id", | ||
| "component": null | ||
| } |
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 is a valid issue don't remove user_title: it's referenced.
|
I have addressed the feedback. Please take another look. |
|
/gemini review |
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 useful feature for deleting components via updateComponents. The implementation is mostly solid, with updates to the schema, documentation, and new tests.
However, there's a significant inconsistency between how component deletion is described in the pull request title/description (component: null) and how it's actually implemented and documented in the code (omitting the component property). My review comments focus on resolving this inconsistency to ensure the feature is clear, correct, and maintainable.
I've also suggested tightening the validation logic and adding a test case to make the contract for deletion explicit.
Additionally, I noticed some unrelated but correct fixes to the action property in several test files. While it's better to keep PRs focused on a single feature, these changes are valid.
| To add or update a component, provide the full component definition object. If a component with the same `id` already exists, it will be replaced. | ||
|
|
||
| **Deleting Components:** | ||
| To delete a component, provide an object with the component's `id` and omit the `component` property. This removes the component from the client's memory. |
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 documentation correctly states that deletion is done by omitting the component property, which aligns with the JSON schema. However, the pull request title (feat: Support component deletion via component: null) and description are inconsistent with this, as they state deletion is done via component: null. Please update the PR title and description to match the implemented behavior to avoid confusion.
| if (componentType === undefined || componentType === null) { | ||
| return; | ||
| } |
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 check is more permissive than what the JSON schema allows. The schema (server_to_client.json) defines a tombstone for deletion as an object with only an id property, enforced by additionalProperties: false. This means a payload with component: null would fail schema validation before this custom validation code is even reached. To avoid confusion and align strictly with the schema, this check should only allow for an omitted component property.
| if (componentType === undefined || componentType === null) { | |
| return; | |
| } | |
| if (componentType === undefined) { | |
| // This is a tombstone for component deletion, which is valid per the schema. | |
| return; | |
| } |
| ] | ||
| } | ||
| } | ||
| } |
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.
These test cases are great for verifying the deletion-by-omission behavior. To make the contract even more explicit and prevent future regressions, it would be beneficial to add a test case that asserts that using component: null is invalid, as per the current JSON schema. This would help solidify that only omitting the component property is the supported method for deletion.
},
{
"description": "Deletion with component: null (should be invalid per schema)",
"valid": false,
"data": {
"updateComponents": {
"surfaceId": "test_surface",
"components": [
{
"id": "item_to_delete",
"component": null
}
]
}
}
}
Description
This PR introduces a mechanism to explicitly delete components from a surface within the
updateComponentsmessage.Changes
server_to_client.jsonschema forupdateComponentshas been modified. Thecomponentslist now accepts a 'tombstone' object in addition to standard component definitions.idand thecomponentproperty set explicitly tonull.a2ui_protocol.mdto document this new behavior.component_deletion.jsonto the test suite and updated the evaluation framework to support this feature.Rationale
Previously, removing a component typically required re-sending the entire parent structure or using
deleteSurfaceand recreating it, which is inefficient and can disrupt the user experience. This change allows for granular updates where specific components can be removed while others are added or updated in the same atomic message.Sample Code
To delete a component with ID
loading-spinnerand simultaneously add a newstatus-textcomponent:{ "updateComponents": { "surfaceId": "main", "components": [ { "id": "loading-spinner", "component": null }, { "id": "status-text", "component": "Text", "text": "Loaded Successfully" } ] } }