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.
| * @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. | ||
| * |
| @@ -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.
bbaefd0 to
3acfc9b
Compare
- Refactor components to mirror Lit Shadow DOM structure for visual parity - Add CSS fixes for specificity and selector transformations - Add unit tests for all components (220 tests) - Add visual parity test infrastructure with Playwright - Update documentation with build and test instructions
The Lit renderer bug has been fixed, so this fixture no longer needs to be skipped.
| * 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 :)
|
|
||
| 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.
| /* ========================================================================= | ||
| * Card (from Lit card.ts static styles) | ||
| * ========================================================================= */ |
There was a problem hiding this comment.
Why are these comments part of the output string?
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)
The Icon component uses Material Symbols Outlined font, not Lucide.
|
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: |
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