Conversation
![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 Vega-Lite support to the Rizzcharts sample by updating agent instructions, adding catalog definitions, and implementing a new Angular rendering component. Review feedback identifies several areas for improvement, including the removal of debug print/log statements and commented-out code, and the need for unit tests for the new component to comply with the repository style guide. Concerns were also raised regarding the fragility of the post-install build script and the potential for invalid URLs if Markdown syntax is used in system instructions.
| s = "\n\n".join(parts) | ||
| print("System prompt: \n" + s) | ||
| return s |
There was a problem hiding this comment.
Debug print statements should not be included in the SDK source code. This information should be handled by a logging framework if necessary.
| s = "\n\n".join(parts) | |
| print("System prompt: \n" + s) | |
| return s | |
| return "\n\n".join(parts) |
| * **Cars**: `"url": "[https://vega.github.io/vega-lite/examples/data/cars.json](https://vega.github.io/vega-lite/examples/data/cars.json)"` | ||
| * *Schema*: `Name` (str), `Miles_per_Gallon` (num), `Cylinders` (num), `Displacement` (num), `Horsepower` (num), `Weight_in_lbs` (num), `Acceleration` (num), `Year` (date), `Origin` (str). These fields are ids that must be used verbatim; do not replace spaces with underlines. | ||
| * **Movies**: `"url": "[https://vega.github.io/vega-lite/examples/data/movies.json](https://vega.github.io/vega-lite/examples/data/movies.json)"` | ||
| * *Schema*: `Title` (str), `US Gross` (num), `Worldwide Gross` (num), `US DVD Sales` (num), `Production Budget` (num), `Release Date` (str), `MPAA Rating` (str), `Running Time min` (num), `Distributor` (str), `Source` (str), `Major Genre` (str), `Creative Type` (str), `Director` (str), `Rotten Tomatoes Rating` (num), `IMDB Rating` (num), `IMDB Votes` (num). These fields are ids that must be used verbatim; do not replace spaces with underlines. |
There was a problem hiding this comment.
The system instructions use Markdown link syntax for URLs (e.g., [url](url)). This can lead the LLM to include the Markdown brackets and parentheses in the generated JSON, resulting in invalid URLs in the Vega-Lite specification. It is recommended to provide the raw URL strings instead.
| # get_store_sales, | ||
| # get_sales_data, |
There was a problem hiding this comment.
Commented-out code should be removed to keep the codebase clean and maintainable.
| "demo:restaurant": "npm run build:renderer && concurrently -k -n \"AGENT,WEB\" -c \"magenta,blue\" \"npm run serve:agent:restaurant\" \"npm start -- restaurant\"", | ||
| "demo:rizzcharts": "npm run build:renderer && concurrently -k -n \"AGENT,WEB\" -c \"magenta,blue\" \"npm run serve:agent:rizzcharts\" \"npm start -- rizzcharts\"", | ||
| "run:rizzcharts": "concurrently -k -n \"AGENT,WEB\" -c \"magenta,blue\" \"npm run serve:agent:rizzcharts\" \"npm start -- rizzcharts\"", | ||
| "postinstall": "npm run build:renderer", |
There was a problem hiding this comment.
The postinstall script executes a build process for external components using relative paths. This makes the installation process heavy and fragile, as it assumes a specific directory structure that might not be present in all environments (e.g., CI/CD). Consider moving this to a separate initialization or build script.
| if (json.contextId || json.result?.contextId) { | ||
| this.contextId = json.contextId || json.result?.contextId; | ||
| } | ||
| console.log("Received response: " + JSON.stringify(json, null, 2)); |
There was a problem hiding this comment.
Remove debug console.log statements before merging.
| * limitations under the License. | ||
| */ | ||
|
|
||
| import { Component, Input, ViewChild, ElementRef, OnChanges, SimpleChanges } from '@angular/core'; |
There was a problem hiding this comment.
According to the Repository Style Guide (line 17), code changes should have tests. Please add unit tests for the new A2uiVegaChartComponent to ensure it correctly handles specification changes and interacts with the Vega-Lite library as expected.
References
- If there are code changes, code should have tests. (link)
1 participant
No description provided.