Skip to content

Rizzcharts java#1015

Open
jgindin wants to merge 11 commits intogoogle:mainfrom
jgindin:rizzcharts-java
Open

Rizzcharts java#1015
jgindin wants to merge 11 commits intogoogle:mainfrom
jgindin:rizzcharts-java

Conversation

@jgindin
Copy link
Copy Markdown
Collaborator

@jgindin jgindin commented Mar 27, 2026

Description

New Java version of the rizzcharts sample using the Kotlin version of the A2UI SDK.

Pre-launch Checklist

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a 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 the A2UI Kotlin SDK, providing core logic for schema management, payload validation, and A2A protocol support. It also adds a Java sample for the Rizzcharts agent and updates the Python sample's workspace configuration. The review feedback identifies a critical build configuration error in the Java sample that disables logging, suggests using variables for dependency versions to improve maintainability, and recommends best practices for logging and performance, such as avoiding printStackTrace and sharing ObjectMapper instances.

Comment on lines +53 to +55
configurations.all {
exclude(group = "org.slf4j", module = "slf4j-simple")
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The configurations.all { ... } block excludes slf4j-simple from all configurations, including implementation. This effectively nullifies the dependency declaration on line 38 (implementation("org.slf4j:slf4j-simple:2.0.12")) and may prevent any logging from appearing, making debugging difficult.

The exclusion on google-adk (line 29) is sufficient to control the version of slf4j-simple. This global exclusion block is likely a mistake and should be removed to ensure logging works as expected.

Comment on lines +2 to +3
kotlin("jvm") version "2.1.10"
kotlin("plugin.serialization") version "2.1.10"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The Kotlin version 2.1.10 is hardcoded in multiple places. To improve maintainability and ensure consistency when updating, it's better to define it as a variable and reuse it.

For example:

val kotlinVersion = "2.1.10"

plugins {
  kotlin("jvm") version kotlinVersion
  kotlin("plugin.serialization") version kotlinVersion
  // ...
}

response["error"] = mapOf("code" to -32601, "message" to "Method not found")
}
} catch (e: Exception) {
e.printStackTrace()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using e.printStackTrace() is generally discouraged in server-side code. It prints to the standard error stream, which might not be captured by your logging infrastructure, making it harder to debug issues in production. It's better to use a proper logger to record the full exception details.

You can add a logger to the class like this:

private val logger = java.util.logging.Logger.getLogger(A2aHandler::class.java.name)
Suggested change
e.printStackTrace()
logger.log(java.util.logging.Level.SEVERE, "Error handling A2A POST request", e)

"---BEGIN $basename---\n$content\n---END $basename---"
}
} catch (e: Exception) {
logger.warning("Failed to load example ${file.path}: ${e.message}")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

When catching an exception while loading an example, only the exception message is logged. This can make debugging difficult as the stack trace is lost. It's better to log the full exception object to get more context about the error.

Suggested change
logger.warning("Failed to load example ${file.path}: ${e.message}")
logger.log(java.util.logging.Level.WARNING, "Failed to load example ${file.path}", e)

*/
class A2uiValidator(private val catalog: A2uiCatalog) {
private val validator: JsonSchema = buildValidator()
private val mapper = ObjectMapper()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The ObjectMapper is thread-safe and its instantiation can be resource-intensive. For better performance, it's recommended to share a single instance across all A2uiValidator instances. You can achieve this by removing this instance field and defining the mapper in a companion object instead:

private companion object {
    private val mapper = ObjectMapper()
}

RizzchartsAgentExecutor.A2UI_EXAMPLES_KEY,
schemaManager.loadExamples(schemaManager.getSelectedCatalog()));
} catch (Exception e) {
// Ignore loading error
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This empty catch block silently ignores exceptions, which can hide problems with loading example files (e.g., file not found, malformed content) and make debugging difficult. At a minimum, the exception should be logged.

              // Log error and continue, as examples are not critical for agent operation.
              System.err.println(
                  "Could not load A2UI examples for the selected catalog: " + e.getMessage());

jgindin and others added 10 commits March 27, 2026 15:00
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Decompose the `handleA2aPost` method into smaller ones.

Also, fix the previous commit, which Github messed up.
Directly ported from the Python sample.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

1 participant