Implement A2UI catalog negotiation #576
Conversation
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully implements the A2UI catalog negotiation process. The changes are comprehensive, updating models, managers, and connectors to support multiple catalogs and client capabilities. The introduction of catalogId and the associated validation logic in GenUiSurface are well-implemented. The migration of examples and tests to the new APIs is also thorough.
My review includes two main points:
- A potential regression in an example app due to a missing configuration during the API migration.
- A suggestion to refactor duplicated code in
GenUiSurfaceto improve maintainability.
Overall, this is a solid implementation of a key feature for the A2UI protocol.
| final genUiManager = GenUiManager.withSingleCatalog( | ||
| catalog: travelAppCatalog, | ||
| configuration: const GenUiConfiguration( | ||
| actions: ActionsConfig( | ||
| allowCreate: true, | ||
| allowUpdate: true, | ||
| allowDelete: true, | ||
| ), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
When migrating to GenUiManager.withSingleCatalog, the GenUiConfiguration was removed. The default configuration disables all actions (allowCreate, allowUpdate, allowDelete are false by default), which might break the functionality of this example app. This configuration should be passed to the new constructor to maintain the original behavior.
| final genUiManager = GenUiManager.withSingleCatalog( | |
| catalog: travelAppCatalog, | |
| configuration: const GenUiConfiguration( | |
| actions: ActionsConfig( | |
| allowCreate: true, | |
| allowUpdate: true, | |
| allowDelete: true, | |
| ), | |
| ), | |
| ); | |
| final genUiManager = GenUiManager.withSingleCatalog( | |
| catalog: travelAppCatalog, | |
| configuration: const GenUiConfiguration( | |
| actions: ActionsConfig( | |
| allowCreate: true, | |
| allowUpdate: true, | |
| allowDelete: true, | |
| ), | |
| ), | |
| ); |
There was a problem hiding this comment.
Don't worry about this - we don't need it
| final String catalogId = | ||
| definition.catalogId ?? CoreCatalogItems.standardCatalogId; | ||
| final Catalog? catalog = widget.host.catalogs.firstWhereOrNull( | ||
| (c) => c.catalogId == catalogId, | ||
| ); | ||
|
|
||
| if (catalog == null) { | ||
| genUiLogger.severe( | ||
| 'Catalog with id "$catalogId" not found for surface ' | ||
| '"${widget.surfaceId}". Ensure the catalog is provided to ' | ||
| 'GenUiManager.', | ||
| ); | ||
| return Container(); | ||
| } |
There was a problem hiding this comment.
The logic to find the correct catalog based on the catalogId is duplicated here and in the _dispatchEvent method (lines 136-148). To improve maintainability and reduce code duplication, this logic could be extracted into a private helper method within the _GenUiSurfaceState class.
For example, you could add a method that returns both the catalog and its ID:
(Catalog?, String) _findCatalogForDefinition(UiDefinition definition) {
final String catalogId =
definition.catalogId ?? CoreCatalogItems.standardCatalogId;
final Catalog? catalog = widget.host.catalogs.firstWhereOrNull(
(c) => c.catalogId == catalogId,
);
return (catalog, catalogId);
}Then, you can use this helper in both build and _dispatchEvent to simplify the code and logging.
There was a problem hiding this comment.
Let's fix this
There was a problem hiding this comment.
Code Review
This pull request successfully implements the A2UI catalog negotiation feature, a crucial part of the protocol. The changes are extensive, touching models, the GenUiManager, the UI surface rendering, and various backend connectors. The introduction of GenUiManager.withSingleCatalog is a thoughtful touch for backward compatibility, and the new tests for catalog validation are excellent.
I've identified a few issues:
- A regression in an example app where a configuration was accidentally removed, which will likely break its functionality.
- A couple of the
ContentGeneratorimplementations (FirebaseAiContentGeneratorandGoogleGenerativeAiContentGenerator) were updated to acceptclientCapabilitiesbut don't actually pass this information to the model, which defeats the purpose of this feature for those backends. - A minor code duplication in
GenUiSurfacethat could be refactored for better maintainability.
Overall, this is a solid implementation of a key feature, and with these fixes, it will be ready to merge.
jacobsimionato
changed the title
| if (catalog == null) { | ||
| genUiLogger.severe( | ||
| 'Cannot show modal for surface "${widget.surfaceId}" because ' | ||
| 'a catalog was not found.', |
There was a problem hiding this comment.
Maybe log the catalog ID of the catalog it was looking for?
| // found in the LICENSE file. | ||
|
|
||
| /// The catalog ID for the standard catalog. | ||
| const String standardCatalogId = 'a2ui.org:standard_catalog_0_8_0'; |
There was a problem hiding this comment.
Should this be 0_8_1? And while I thought we changed the naming of the field, I thought we were still going to use URIs as the ID, no? They're nicely self-documenting.
2 participants
This PR implements the A2UI catalog negotiation process, allowing the client to declare its supported UI component catalogs and for the server to select a compatible catalog for rendering a given UI surface.
This addresses a key part of the A2UI protocol that was previously missing.
Changes
CatalogandUiDefinitionmodels inpackages/genuiupdated to include acatalogId.CoreCatalogItemsinpackages/genuinow uses the standard catalog ID.GenUiManagerupdated to support a list of catalogs.GenUiSurfacenow performs catalog validation. If thecatalogIdfrom abeginRenderingmessage does not match a supported catalog, an error is logged and an empty container is rendered.ContentGeneratorinterface updated to passA2UiClientCapabilities. All implementations have been updated.A2uiAgentConnectorupdated to includea2uiClientCapabilitiesin the A2A message metadata.Fixes #373 and #426