feat: Add support for metadata value dict parsing#122
feat: Add support for metadata value dict parsing#122vinoo999 wants to merge 5 commits intoa2aproject:mainfrom
Conversation
Summary of ChangesHello @vinoo999, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant enhancement to the a2a inspector by enabling it to intelligently parse metadata values. Previously, metadata values were treated as simple strings, but with this change, the system can now detect and parse JSON-formatted values into their native data structures. This improves the accuracy and utility of the inspector when dealing with complex or structured metadata. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 adds support for parsing metadata values as JSON objects in the A2A inspector. The changes are a good step forward, but I've identified a critical bug where the parsed JSON object is incorrectly overwritten by its original string value. Additionally, there's a TypeScript type mismatch that will lead to a compilation error. I've provided specific comments and suggestions to address these issues.
There was a problem hiding this comment.
Code Review
This pull request adds support for parsing metadata values as JSON objects. The changes look good, but I have a couple of suggestions. First, I've pointed out a type safety issue where changing the return type of getKeyValuePairs to Record<string, any> breaks other parts of the code that expect Record<string, string>. I've suggested using function overloads to fix this. Second, I've suggested a small refactoring to simplify the JSON parsing logic and make it more readable and maintainable. Overall, a good feature addition with a couple of areas for improvement.
| parseJson: boolean = false, | ||
| ): Record<string, any> { |
There was a problem hiding this comment.
Changing the return type to Record<string, any> is too broad and breaks type safety for callers that expect string values, like getCustomHeaders. This will cause a TypeScript error because getCustomHeaders is typed to return Record<string, string>.
A better approach is to use function overloads to specify the return type based on the parseJson parameter. This maintains type safety for all call sites.
You can define the overloads like this before the function implementation:
// Overload signatures
function getKeyValuePairs(
list: HTMLElement,
itemSelector: string,
keySelector: string,
valueSelector: string,
parseJson: true,
): Record<string, any>;
function getKeyValuePairs(
list: HTMLElement,
itemSelector: string,
keySelector: string,
valueSelector: string,
parseJson?: false | undefined,
): Record<string, string>;
// Implementation
function getKeyValuePairs(
list: HTMLElement,
itemSelector: string,
keySelector: string,
valueSelector: string,
parseJson: boolean = false,
): Record<string, any> {
// ... current implementation
}This will ensure getCustomHeaders gets a Record<string, string> and getMessageMetadata gets a Record<string, any>.
| if (parseJson) { | ||
| try { | ||
| acc[key] = JSON.parse(value); | ||
| } catch { | ||
| // If not valid JSON, keep as string | ||
| acc[key] = value; | ||
| } | ||
| } else { | ||
| acc[key] = value; | ||
| } |
There was a problem hiding this comment.
This logic can be simplified to avoid repetition. You can declare a variable for the value, attempt to parse it if parseJson is true, and then assign it to the accumulator. This makes the code more concise and easier to read.
let valueToSet: any = value;
if (parseJson) {
try {
valueToSet = JSON.parse(value);
} catch {
// Not valid JSON, so we keep the original string value.
}
}
acc[key] = valueToSet;1 participant
Description
Add support for Metadata parsing in the a2a inspector. Metadata is a map and should be parsed as such in the inspector. This adds support accordingly.
Thank you for opening a Pull Request!
Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
CONTRIBUTINGGuide.fix:which represents bug fixes, and correlates to a SemVer patch.feat:represents a new feature, and correlates to a SemVer minor.feat!:, orfix!:,refactor!:, etc., which represent a breaking change (indicated by the!) and will result in a SemVer major.bash scripts/format.shfrom the repository root to format)Fixes #<issue_number_goes_here> 🦕