Conversation
wrenj
requested review from
dmandar,
gspencergoog,
jacobsimionato and
sugoi-yuzuru
![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 introduces an A2UI over MCP sample, including a README, Python server code, and A2UI JSON definition. The changes are generally well-structured, but I've identified a few areas for improvement related to correctness, maintainability, and minor stylistic issues. Specifically, there's an invalid icon name in the A2UI JSON, reliance on a private API in the server, and a suppressed type ignore that could be clarified or resolved.
| "literalString": "local_fire_department" | ||
| } | ||
| } |
There was a problem hiding this comment.
The icon name "local_fire_department" is not listed as an allowed enum value in the standard_catalog_definition.json (lines 90-139). Using an undefined icon might lead to rendering issues or an empty icon being displayed. Please choose an icon from the allowed enum values to ensure correct rendering.
| sse = SseServerTransport("/messages/") | ||
|
|
||
| async def handle_sse(request: Request): | ||
| async with sse.connect_sse(request.scope, request.receive, request._send) as streams: # type: ignore[reportPrivateUsage] |
There was a problem hiding this comment.
Relying on a private API (request._send) and suppressing the reportPrivateUsage warning can lead to unexpected behavior or breakage in future versions of starlette. It's generally safer to use public APIs or consider contributing a public alternative if the required functionality is not exposed.
|
|
||
| from server import main | ||
|
|
||
| sys.exit(main()) # type: ignore[call-arg] No newline at end of file |
There was a problem hiding this comment.
The type: ignore[call-arg] comment suggests a type incompatibility that is being suppressed. It's best practice to either fix the underlying type issue to ensure type safety or provide a more detailed comment explaining why the ignore is necessary and what the expected behavior is, to aid future maintainers.
| # MCP throws an error for "type":"array" so wrapping in an object | ||
| # TODO fix this in MCP SDK |
There was a problem hiding this comment.
The TODO comment indicates a known issue with the MCP SDK. While the workaround is in place, it would be beneficial to track this issue (e.g., with a link to a bug report or internal ticket) to ensure it's addressed in the SDK itself, improving the robustness and clarity of this code.
samples/agent/adk/mcp/server.py
Outdated
|
|
||
| server_to_client_json["properties"]["surfaceUpdate"]["properties"]["components"]["items"]["properties"]["component"]["properties"] = standard_catalog_json | ||
|
|
||
| return wrap_as_json_array(server_to_client_json) |
There was a problem hiding this comment.
There is trailing whitespace at the end of this line. Please remove it for consistency and adherence to common Python style guidelines.
| return wrap_as_json_array(server_to_client_json) | |
| return wrap_as_json_array(server_to_client_json) |
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
2 participants
MCP Inspector Screenshot validating server: