-
Notifications
You must be signed in to change notification settings - Fork 791
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -148,12 +148,18 @@ This message signals the client to create a new surface and begin rendering it. | |||||||||||||||||||
|
|
||||||||||||||||||||
| ### `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. | ||||||||||||||||||||
|
|
||||||||||||||||||||
| **Adding or Updating Components:** | ||||||||||||||||||||
| 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 set the `component` property to `null`. This removes the component from the client's memory. | ||||||||||||||||||||
|
|
||||||||||||||||||||
| **Properties:** | ||||||||||||||||||||
|
|
||||||||||||||||||||
| - `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). | ||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about the adjacency list description? I think we should keep that. |
||||||||||||||||||||
|
|
||||||||||||||||||||
| **Example:** | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
@@ -173,9 +179,8 @@ This message provides a list of UI components to be added to or updated within a | |||||||||||||||||||
| "text": "John Doe" | ||||||||||||||||||||
| }, | ||||||||||||||||||||
| { | ||||||||||||||||||||
| "id": "user_title", | ||||||||||||||||||||
| "component": "Text", | ||||||||||||||||||||
| "text": "Software Engineer" | ||||||||||||||||||||
| "id": "old_component_id", | ||||||||||||||||||||
| "component": null | ||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why wouldn't it work to just exclude component? { updateDataModel overwrites instead of merging, and it treats null as remove key, so it should have the same effect, no? |
||||||||||||||||||||
| } | ||||||||||||||||||||
|
Comment on lines
181
to
184
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The updated example is a bit confusing. The To make the example clearer and self-consistent, I recommend showing the deletion of an existing component from the example, like Here is a suggestion to make the intent clearer, though the
Suggested change
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||||||||||||||||||||
| ] | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| { | ||
| "schema": "server_to_client.json", | ||
| "tests": [ | ||
| { | ||
| "description": "UpdateComponents with component deletion (tombstone)", | ||
| "valid": true, | ||
| "data": { | ||
| "updateComponents": { | ||
| "surfaceId": "test_surface", | ||
| "components": [ | ||
| { | ||
| "id": "item_to_delete", | ||
| "component": null | ||
| } | ||
| ] | ||
| } | ||
| } | ||
| }, | ||
| { | ||
| "description": "UpdateComponents mixing update and deletion", | ||
| "valid": true, | ||
| "data": { | ||
| "updateComponents": { | ||
| "surfaceId": "test_surface", | ||
| "components": [ | ||
| { | ||
| "id": "new_item", | ||
| "component": "Text", | ||
| "text": "Hello" | ||
| }, | ||
| { | ||
| "id": "old_item", | ||
| "component": null | ||
| } | ||
| ] | ||
| } | ||
| } | ||
| }, | ||
| { | ||
| "description": "Deletion with extra properties (should be invalid)", | ||
| "valid": false, | ||
| "data": { | ||
| "updateComponents": { | ||
| "surfaceId": "test_surface", | ||
| "components": [ | ||
| { | ||
| "id": "item_to_delete", | ||
| "component": null, | ||
| "text": "Should not be here" | ||
| } | ||
| ] | ||
| } | ||
| } | ||
| }, | ||
| { | ||
| "description": "Deletion missing ID (should be invalid)", | ||
| "valid": false, | ||
| "data": { | ||
| "updateComponents": { | ||
| "surfaceId": "test_surface", | ||
| "components": [ | ||
| { | ||
| "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 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
updateComponentssection. 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.