-
Notifications
You must be signed in to change notification settings - Fork 4k
Create RenderNodeVisitor #12814
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Create RenderNodeVisitor #12814
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
✅ PR preview is ready!
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a new RenderNodeVisitor class that extracts the rendering logic for React nodes from the Block component. The visitor pattern is used to traverse app nodes and generate the corresponding React elements, maintaining separation between the render-tree logic and React dependencies.
- Implements the visitor pattern for rendering BlockNode and ElementNode instances to React elements
- Extracts rendering logic from the Block component to improve maintainability
- Includes comprehensive unit tests for the new visitor functionality
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
frontend/lib/src/components/core/Block/RenderNodeVisitor.tsx |
New visitor class implementing AppNodeVisitor interface for React element rendering |
frontend/lib/src/components/core/Block/RenderNodeVisitor.test.tsx |
Comprehensive unit tests covering all visitor functionality and edge cases |
frontend/lib/src/components/core/Block/Block.tsx |
Refactored to use RenderNodeVisitor instead of inline rendering logic |
📈 Frontend coverage change detectedThe frontend unit test (vitest) coverage has increased by 0.0300%
🎉 Great job on improving test coverage! |
11248f5 to
d07f3b1
Compare
b7dbed4 to
dd70991
Compare
d07f3b1 to
ba93847
Compare
dd70991 to
e17221a
Compare
ba93847 to
ff00c6d
Compare
e17221a to
9fdebcf
Compare
0544072 to
16dcf93
Compare
d81a036 to
030ba55
Compare
16dcf93 to
b8f1b65
Compare
030ba55 to
df8e8be
Compare
b8f1b65 to
7ba0586
Compare
df8e8be to
ac65e88
Compare
7ba0586 to
477f035
Compare
ac65e88 to
ec71eed
Compare
477f035 to
9fe4523
Compare
|
|
||
| export type OptionalReactElement = ReactElement | null | ||
|
|
||
| export class RenderNodeVisitor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Perhaps add a class docstring like the other visitors
/**
* A visitor that renders AppNodes as React elements.
*
* Unlike other visitors in render-tree/visitors/, this visitor is
* React-specific and located in components/ to maintain dependency
* boundaries.
*
* This visitor accumulates React elements in a mutable array and tracks
* rendered element IDs to prevent duplicate rendering of widgets.
*
* Usage:
* ```typescript
* const elements = RenderNodeVisitor.collectReactElements(props, disableFullscreen)
* return <>{elements}</>
* ```
*/| private readonly props: BlockPropsWithoutWidth | ||
| private readonly disableFullscreenMode: boolean | ||
| private readonly elementKeySet: Set<string> | ||
| public readonly reactElements: OptionalReactElement[] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Why public reactElements?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly just accessing it in the tests. Open to other ideas, but creating a getter will return the same reference (unless I duplicate)
| // Initialize index to 0 as we will use it as a key in the React component | ||
| this.index = 0 | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Might be helpful to add comment explaining the key logic - this.index used for the current key, and then incremented to be used for the next key.
Alternatively, could move this to helper to clarify, but might be a bit much.
private getCurrentKey(elementId?: string): string {
const key = elementId || this.index.toString()
// Increment this.index to be used for the next key
this.index += 1
// return the current key
return key
}
visitBlockNode(node: BlockNode): OptionalReactElement {
// ...
const key = this.getCurrentKey() // Block has no element ID
// ...
}
visitElementNode(node: ElementNode): OptionalReactElement {
// ...
const key = this.getCurrentKey(getElementId(node.element)) // With element ID
// ...
}| return renderer | ||
| } | ||
|
|
||
| static collectReactElements( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Would like more documentation for this method since it is the main call in Block.tsx - something like:
/**
* Convenience method to render all children of a block as React elements.
*
* This is the primary entry point for rendering - it creates a visitor,
* traverses all children, and returns the accumulated React elements.
*
* @param props - Block props containing the node to render
* @param disableFullscreenMode - Whether to disable fullscreen mode for elements
* @returns Array of React elements ready to render (may include nulls for duplicates)
*
* @example
* const ChildRenderer = (props) => {
* return <>{RenderNodeVisitor.collectReactElements(props, false)}</>
* }
*/
mayagbarnes
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some suggestions but LGTM
ec71eed to
e8e053c
Compare
9671db9 to
e358528
Compare
e8e053c to
e09e16d
Compare
2e0ae39 to
8545204
Compare
e09e16d to
2c656d7
Compare
8545204 to
0f55447
Compare
0f55447 to
761928f
Compare
Now, we introduce a new visitor for rendering the block react node. We made this separate from the render-tree folder because we did not want to make React a dependency. This is great because we can break out the render-tree to a separate package someday. - JS Unit Tests for the RenderNode Visitor - E2E tests should ensure the rendering logic remains correct --- **Contribution License Agreement** By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

Describe your changes
Now, we introduce a new visitor for rendering the block react node. We made this separate from the render-tree folder because we did not want to make React a dependency. This is great because we can break out the render-tree to a separate package someday.
Testing Plan
Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.