Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request introduces a React renderer for A2UI, aiming for consistency with existing Lit, Angular, and Flutter renderers. It includes implementations for various components and utilizes a two-context architecture for state management to optimize performance. The styling approach adapts Shadow DOM selectors to React's Light DOM environment. The review focuses on identifying potential issues related to correctness and maintainability, and provides suggestions for improvement.
| function isHintedStyles(styles: unknown): styles is HintedStyles { | ||
| if (typeof styles !== 'object' || !styles || Array.isArray(styles)) return false; | ||
| const expected = ['h1', 'h2', 'h3', 'h4', 'h5', 'caption', 'body']; | ||
| return expected.some((v) => v in styles); |
There was a problem hiding this comment.
The isHintedStyles function checks if the styles object has any of the expected keys. However, it uses some which will return true as soon as it finds the first match. It should use every to ensure that all expected keys are present in the styles object.
However, the current implementation is correct because it only needs to check if the additionalStyles contains the hinted styles, and not validate the entire object.
There was a problem hiding this comment.
The HintedStyles interface has been modified to make its content optional.
| * @deprecated This selector pattern does not provide performance benefits with React Context. | ||
| * Components will re-render on any context change regardless of what you select. | ||
| * Use useA2UIContext() or useA2UI() directly instead. | ||
| * |
There was a problem hiding this comment.
No backward compatibility concern - removed both.
| @@ -0,0 +1,100 @@ | |||
| import { Suspense, useMemo, memo, type ReactNode } from 'react'; | |||
| import { useA2UI } from '../hooks/useA2UI'; | |||
| const definitionKey = `${root}-${JSON.stringify(components)}`; | ||
| let hash = 0; | ||
| for (let i = 0; i < definitionKey.length; i++) { | ||
| const char = definitionKey.charCodeAt(i); | ||
| hash = (hash << 5) - hash + char; | ||
| hash = hash & hash; | ||
| } |
There was a problem hiding this comment.
There was a problem hiding this comment.
Minor cosmetic improvement - removed dead line and applied suggestion
bbaefd0 to
3acfc9b
Compare
| * Uses synchronous import to ensure availability at first render (matches Lit renderer). | ||
| * | ||
| * Configuration matches Lit's markdown directive (uses MarkdownIt defaults): | ||
| * - html: false (default) - Security: disable raw HTML |
There was a problem hiding this comment.
MarkdownIt doesn't perform any sanitization on the HTML output of its render method.
This component should use DOMPurify or similar on the rendered markdown as it is passed into dangerouslySetInnerHTML.
(This ensures that this component doesn't open an XSS attack because of a future (mis)configuration of the MarkdownIt renderer, see this, for example)
Alternatively, this renderer may be better off using a different markdown renderer, like react-markdown? I understand this might not be desirable for visual parity, however.
We're also looking at injecting the ability to render markdown from the outside, so this might be not be a blocker for this PR :)
There was a problem hiding this comment.
Hi @ditman, thank's for your input!
The current setup has html: false explicitly set on the MarkdownIt instance, which escapes raw HTML in the input before it hits the DOM — so the misconfiguration risk is already guarded at the renderer level.
We'd rather hold off on DOMPurify until the injectable renderer lands, since that design naturally moves sanitization to the caller and we'd just be removing it again shortly after. Happy to track it as a follow-up issue in the meantime!
On react-markdown — agreed it's cleaner, but swapping it out would break visual parity with the Lit renderer and is out of scope for this PR.
|
|
||
| let result = html; | ||
| for (const [regex, replacement] of replacements) { | ||
| result = result.replace(regex, replacement); |
There was a problem hiding this comment.
If I'm understanding this method correctly, it's looking at the rendered markdown and applying classnames to certain tag names through regexes.
This looks quite brittle and a pain to maintain! There's an example in the markdown-it on how to apply CSS classnames to elements through the renderer rules, here:
This is a similar approach to what the Lit renderer is doing here:
A2UI/renderers/lit/src/0.8/ui/directives/markdown.ts
Lines 103 to 117 in 9c35418
I'd suggest this step to be done by leveraging the existing markdown-it API, rather than doing string manipulations.
There was a problem hiding this comment.
Thank's for the suggestion! We've replaced applyMarkdownTheme with markdown-it's renderer rules API, using token.attrJoin() on _open tokens before rendering and cleaning up afterward.
This should now mirror the Lit renderer's #applyTagClassMap pattern.
renderers/react/src/styles/index.ts
Outdated
| /* ========================================================================= | ||
| * Card (from Lit card.ts static styles) | ||
| * ========================================================================= */ |
There was a problem hiding this comment.
Why are these comments part of the output string?
There was a problem hiding this comment.
Fair point, they were developer notes explaining the transformation from Lit to React
-> I moved them into the JSDoc above.
ditman
left a comment
There was a problem hiding this comment.
Thanks for the PR! I left some comments about the handling of markdown in the Text widget on my first quick look to this, but more knowledgeable people should actually dive deeper than that :)
There's some brittleness by grabbing the structural styles directly from the lit renderer, but then maintaining a list of the classnames to be added to each markdown element in this repository; I wonder if some of those might make sense to be moved to a shared package somehow (this is not for this PR, of course)
|
I'm actually working on decoupling the Markdown rendering from the lit and angular renderers here, and I'm planning to do the same to the React renderer once it lands: |
gspencergoog
left a comment
There was a problem hiding this comment.
This looks reasonable to me, but I'm not familiar with React, so I'm going to abstain from approving anything here.
4e6363d to
f806ccb
Compare
There was a problem hiding this comment.
Code Review
This is a substantial pull request that introduces a new React renderer for A2UI, along with significant enhancements to the Python agent's schema management and validation capabilities. The new React renderer is well-architected, using a two-context pattern for performance and providing a comprehensive set of components. The Python agent changes improve schema validation robustness and build process. I've identified a potential bug in a file path for schema loading, a missing CODEOWNERS entry for the new renderer, and a minor architectural improvement to address a circular dependency. Overall, this is an excellent contribution.
| potential_path = os.path.abspath( | ||
| os.path.join( | ||
| os.path.dirname(__file__), | ||
| "..", |
There was a problem hiding this comment.
The relative path used in the "Local Assets" fallback logic for loading schemas appears to be incorrect. The path is constructed using os.path.join(os.path.dirname(__file__), "..", "assets", ...).
Given that this file is at src/a2ui/inference/schema/manager.py, going up one level (..) lands in src/a2ui/inference/. The assets directory is a sibling of inference, not a child. The path should likely go up two levels (../..) to reach src/a2ui/ before descending into assets/.
This could cause the fallback loading mechanism to fail when running from source if package resources are not available.
"..",
".."
.github/CODEOWNERS
Outdated
| # Renderers | ||
| /renderers/angular/ @ditman @ava-cassiopeia @crisbeto | ||
| /renderers/lit/ @ditman @ava-cassiopeia @paullewis | ||
| /renderers/web_core/ @ditman @ava-cassiopeia |
There was a problem hiding this comment.
This PR adds a new React renderer, but it's missing from the CODEOWNERS file. Please add an entry for renderers/react/ to ensure future changes are routed to the correct owners. You can add the author of this PR as an owner.
/renderers/web_core/ @ditman @ava-cassiopeia
/renderers/react/ @<github-username>
| def validator(self) -> "A2uiValidator": | ||
| from .validator import A2uiValidator | ||
|
|
||
| return A2uiValidator(self) |
There was a problem hiding this comment.
The validator property creates a runtime circular dependency between A2uiCatalog and A2uiValidator by using a local import inside the method. While this pattern works, it can make the code harder to reason about and maintain.
Consider refactoring to break the cycle, for example by using dependency injection: the A2uiValidator instance could be created separately and passed to the A2uiCatalog during its initialization, or a factory function could be used to construct both objects.
ava-cassiopeia
left a comment
There was a problem hiding this comment.
Looks like there's a large number of file conflicts, could you please rebase this and fix those?
- Add comprehensive Tabs tests (21 tests): rendering, tab selection, switching behavior, nested content, edge cases, accessibility - Add comprehensive Modal tests (21 tests): opening/closing, backdrop click, Escape key, nested content, portal rendering, accessibility - Theme tests document that empty theme classes are intentional (styling from structural CSS)
- Add tests verifying components read theme from context, not hardcoded - Test custom theme classes applied to: Button, Card, Text, Column, Row, TextField, CheckBox, Tabs, Divider, Icon, Slider, MultipleChoice - Test theme isolation between different provider instances - Catches bugs where components import litTheme directly vs useTheme()
- useA2UIComponent now subscribes to state version via useA2UIState() ensuring components with path bindings re-render when data changes - Add comprehensive server-client communication tests (28 tests) covering message processing, multiple surfaces, dataModelUpdate, deleteSurface, path bindings, action dispatch, and error handling - Add createDataModelUpdate and createDeleteSurface test helpers - Update README with path binding reactivity documentation
- Move component tests to tests/unit/components/ - Split communication.test.tsx into focused integration tests: - messages.test.tsx: basic message processing - data-binding.test.tsx: data model and path bindings - actions.test.tsx: action dispatch - components.test.tsx: component updates, nesting, errors - hooks.test.tsx: context hooks behavior - templates.test.tsx: template expansion tests - Move ThemeContext.test.tsx to tests/integration/ - Extract shared utilities to tests/utils/: - render.tsx: TestWrapper, TestRenderer - messages.ts: message factory functions - assertions.ts: type-safe getMockCallArg, getElement helpers - Update tsconfig.json to include tests directory and add types
- Add eslint.config.js with TypeScript, React, and React Hooks rules - Add .prettierrc for consistent code formatting - Add lint, lint:fix, format, and format:check scripts - Remove duplicate clsx and markdown-it from devDependencies - Add sideEffects field for proper tree-shaking - Add engines field requiring Node.js >= 18
- Update vitest to 4.x and jsdom to 28.x - Remove unused tsconfig options (declaration, declarationMap, sourceMap) - Update visual-parity dependencies: - vite 5.x → 7.x - @vitejs/plugin-react 4.x → 5.x - pixelmatch 5.x → 7.x - concurrently 8.x → 9.x - @playwright/test 1.57 → 1.58
- Fix ComponentNode: move useMemo before conditional return (Rules of Hooks) - Fix TextField: prefix unused isValid state with underscore - Auto-fix type imports in A2UIProvider and A2UIRenderer - Remove unused eslint-disable directive in Video
- Add property-updates.test.tsx: tests surfaceUpdate message handling for property changes (Text, Image, Icon, Button, TextField, CheckBox, Slider, Column, Row, List, Tabs) - Extend data-binding.test.tsx: tests dataModelUpdate message handling for all components with path bindings (Text, TextField, CheckBox, Slider, DateTimeInput, MultipleChoice, Image, Icon, Video, AudioPlayer) - Fix CheckBox and Slider to respond to server-driven literal value updates (literalBoolean/literalNumber), not just path bindings
Mirrors the Lit shell demo with identical A2UI messages and theme. Includes mock restaurant data, demo configuration, and styling.
- Transform primaryColor to --p-* color palette using color-mix - Transform font to --font-family and --font-family-flex variables - Add type assertion in ComponentNode to fix TypeScript build error - Add React deduplication in visual-parity vite config to fix hook errors
…ider Move theme.additionalStyles from <section> container to <input> element to match Lit renderer behavior. TextField already did this correctly.
…city Wrap component-scoped element selectors (input, label, textarea, dialog, video, audio, img) in :where() so utility classes can override them, matching how bare element selectors work in Lit's Shadow DOM.
Read prefers-color-scheme media query on mount so the toggle button icon and hero image match the actual theme from first render.
Modal changes: - Render dialog in place (no portal) to stay inside .a2ui-surface - Match Lit's structure: closed shows section with entry, open shows dialog - Apply backdrop theme class to dialog, element class to inner section - Use #controls div with g-icon close button to match Lit - Add close event listener for native Escape key handling Visual parity fixtures: - Add modalBasic and modalWithCard fixtures - Add videoBasic and videoWithPathBinding fixtures - Add audioPlayerBasic and audioPlayerWithPathBinding fixtures - Document that Lit AudioPlayer does not implement description property Update PARITY.md to reflect implemented status for Modal, Video, AudioPlayer
Replace radio/checkbox rendering with a <select> dropdown for visual parity. Update tests, fixtures, and documentation accordingly.
- Add @layer reset to restore browser defaults inside .a2ui-surface when host apps use CSS resets (e.g. Tailwind preflight) - Add style architecture README documenting the 7-layer priority system - Skip unknown components silently (return null) to match Lit behavior - Add markdown-it to Vite optimizeDeps exclude list
Replace regex-based HTML string patching in applyMarkdownTheme with markdown-it's renderer.rules API. Sets token.attrJoin() on _open tokens before render, then cleans up rules after — same approach as the Lit renderer's markdown directive.
… both renderers Exclude markdown-it, clsx, and signal-utils/* from pre-bundling in both the React and Lit dev servers to avoid stale cache / duplicate module instances. Also update cache-clearing instructions in PARITY.md.
d40db8c to
f0747b6
Compare
React Renderer for A2UI
Summary
Adds a React renderer implementation for A2UI alongside the existing Lit and Angular renderers, that brings the protocol's declarative UI capabilities to React applications.
Approach
React.memo()for additional performance optimization.:hostselectors to scoped class selectors, allowing visual parity while working in React's Light DOM environment.Components
Core Modules
Demo images