WIP #1444
Conversation
…ility architecture
…rectory for compile-time composition
…and aspect-fill the container
…on persistence and localized components
…log JSON Schema Viewer UI
…gestion to compiler
…logs for dynamic demo
…ynamic presentation
…catalog composition
![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 a compile-time catalog composition system for the A2UI framework, including a demo application and a catalog compiler script. The changes include the addition of new components like McpApp and WeatherWidget, a security-focused domain whitelist for catalog ingestion, and a static registry generation process. Feedback focuses on addressing a memory leak caused by recursive rendering in the demo app, improving type safety by removing unnecessary any casts and non-null assertions, and making the catalog detection logic in the compiler script more robust.
| if (feedbackText) { | ||
| activeFeedbackText = feedbackText; | ||
| // Re-render cleanly to update state reactively | ||
| renderActiveComponent(name, catalog); |
There was a problem hiding this comment.
Calling renderActiveComponent recursively inside the onAction subscription is problematic. Each call creates a new SurfaceModel and a new subscription without disposing of the previous ones. This results in a memory leak as the old SurfaceModel instances and their observers remain in memory. Additionally, it causes a full teardown and rebuild of the component playground on every action, which is inefficient and resets any local UI state (like focus or scroll position). Consider using a persistent model and updating the UI reactively.
| // --- Reactive UI Ingesting --- | ||
|
|
||
| function renderCatalogSelector() { | ||
| const container = document.getElementById('catalog-selector-buttons')!; |
There was a problem hiding this comment.
Avoid using the non-null assertion operator (!) when retrieving DOM elements. If the element catalog-selector-buttons is missing from the document for any reason, this will cause a runtime TypeError. It is safer to check for the element's existence before proceeding.
| const container = document.getElementById('catalog-selector-buttons')!; | |
| const container = document.getElementById('catalog-selector-buttons'); | |
| if (!container) return; |
| function renderActiveComponent(name: string, catalog: Catalog<any>) { | ||
| const container = document.getElementById('dynamic-surface-container')!; | ||
|
|
||
| const surfaceModel = new SurfaceModel('showcase-surface', catalog as any); |
There was a problem hiding this comment.
Avoid using as any to cast the catalog. This bypasses TypeScript's type checking and can hide potential bugs or type mismatches. If the SurfaceModel constructor expects a Catalog<any>, the cast is redundant. If it expects a more specific type, you should ensure the catalog matches that type or define a proper interface.
|
|
||
| // Connect via postMessage JSON-RPC | ||
| const msgId = 1; | ||
| iframe.contentWindow!.postMessage({ |
There was a problem hiding this comment.
The contentWindow property of an iframe can be null if the iframe is not yet attached to the DOM or has been removed. Using a non-null assertion (!) here is unsafe and could lead to a crash. It is better to use optional chaining or an explicit check.
| iframe.contentWindow!.postMessage({ | |
| iframe.contentWindow?.postMessage({ |
| } | ||
|
|
||
| // Dynamically generate the exports registry based on the loaded catalogs! | ||
| const weatherCatalogRegistered = targetCatalogs.some(url => url.includes('weather-catalog.json')); |
There was a problem hiding this comment.
The logic for detecting and registering catalogs based on hardcoded filename substrings (e.g., weather-catalog.json) is brittle. This approach prevents the --add CLI feature from working correctly with catalogs that have different filenames, even if they contain the same component types. Consider a more robust identification mechanism, such as inspecting the contents of the catalog or using a dedicated metadata field.
1 participant
Description
Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.
List which issues are fixed by this PR. For larger changes, raising an issue first helps reduce redundant work.
Pre-launch Checklist
If you need help, consider asking for advice on the discussion board.