Add rizzcharts back again, disable Lit version#784
Conversation
This reverts commit 9ebe774.
gspencergoog
requested review from
ditman,
dmandar,
jacobsimionato,
yjbanov and
zeroasterisk
as code owners
gspencergoog
removed request for
ditman,
dmandar,
jacobsimionato,
yjbanov and
zeroasterisk
gspencergoog
changed the title
![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 the rizzcharts sample, which includes a Python-based agent and an Angular client, and updates documentation to reflect this new addition. My review focuses on improving the sample's correctness and usability by fixing a broken test, addressing configuration issues like port conflicts and incorrect paths, and improving the robustness of the client-side code to ensure a better developer experience.
Note: Security Review did not run due to the size of the PR.
| it('should render title', () => { | ||
| const fixture = TestBed.createComponent(App); | ||
| fixture.detectChanges(); | ||
| const compiled = fixture.nativeElement as HTMLElement; | ||
| expect(compiled.querySelector('h1')?.textContent).toContain('Hello, chat_canvas'); | ||
| }); |
There was a problem hiding this comment.
This test case is broken and will fail. It asserts that an h1 element contains 'Hello, chat_canvas', but the component's template (app.html) does not contain such an element. The welcome message is within a <ng-template #welcomeMessage> and does not have an h1. Please update the test to reflect the actual component template.
| ngOnInit() { | ||
| const script = this._renderer2.createElement('script'); | ||
| script.src = `https://maps.googleapis.com/maps/api/js?key=${environment.googleMapsApiKey}&callback=initMap&libraries=marker`; | ||
| script.async = true; | ||
| script.defer = true; | ||
| this._renderer2.appendChild(this._document.body, script); |
There was a problem hiding this comment.
Manually injecting the Google Maps script with a global initMap callback is a fragile approach and can lead to runtime errors. Since @angular/google-maps is already a dependency, it's recommended to leverage its built-in mechanisms for loading the Google Maps API, which is typically handled by providing the API key. This manual script injection is likely redundant and could conflict with the library's loading mechanism. Please remove this manual script injection and rely on the library to handle it.
|
|
||
| @click.command() | ||
| @click.option("--host", default="localhost") | ||
| @click.option("--port", default=10002) |
There was a problem hiding this comment.
The default port is set to 10002, which conflicts with the default port for the orchestrator agent. The orchestrator's README instructs running rizzcharts on port 10005. To avoid this conflict and improve the out-of-the-box experience when running multiple samples, please change the default port to a unique value, for example, 10005.
| @click.option("--port", default=10002) | |
| @click.option("--port", default=10005) |
| version = "0.1.0" | ||
| description = "Sample Google ADK-based rizzcharts agent that uses a2ui extension with a custom catalog and is hosted as an A2A server agent." | ||
| readme = "README.md" | ||
| requires-python = ">=3.13" |
There was a problem hiding this comment.
The requires-python is set to >=3.13. This is a very recent version and is inconsistent with other samples in the repository which use >=3.9. This high requirement could be a barrier for users trying to run this sample. Please consider lowering it to >=3.9 for consistency and broader compatibility, unless there are specific features from Python 3.13+ being used.
| requires-python = ">=3.13" | |
| requires-python = ">=3.9" |
| "styles": [ | ||
| "projects/restaurant/src/styles.css" | ||
| "projects/rizzcharts/src/styles.css" | ||
| ] |
There was a problem hiding this comment.
The test architect for the rizzcharts project specifies "projects/rizzcharts/src/styles.css" as a style file. However, this file does not exist. The correct file seems to be "projects/rizzcharts/src/styles.scss", which is used in the build architect. This will cause tests to fail or not be styled correctly.
| "styles": [ | |
| "projects/restaurant/src/styles.css" | |
| "projects/rizzcharts/src/styles.css" | |
| ] | |
| "styles": [ | |
| "projects/rizzcharts/src/styles.scss" | |
| ] |
|
|
||
| app.add_middleware( | ||
| CORSMiddleware, | ||
| allow_origins=["http://localhost:5173"], |
There was a problem hiding this comment.
The CORS middleware allows origins from http://localhost:5173. However, the Angular sample client for this project runs on http://localhost:4200 by default. This mismatch will block requests from the client to the agent. Please update the allowed origin to match the client's address.
| allow_origins=["http://localhost:5173"], | |
| allow_origins=["http://localhost:4200"], |
2 participants
Description
This reverts #782 and instead ONLY removes the Lit demo, since that is broken.
@nan-yu pointed out that this is the only demo that can switch catalogs dynamically, so she wanted to keep just the Angular version around.