Skip to content

Conversation

@kmcgrady
Copy link
Collaborator

@kmcgrady kmcgrady commented Oct 18, 2025

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

  • 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.

@snyk-io
Copy link
Contributor

snyk-io bot commented Oct 18, 2025

Snyk checks have passed. No issues have been found so far.

Status Scanner Critical High Medium Low Total (0)
Licenses 0 0 0 0 0 issues
Open Source Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 18, 2025

✅ PR preview is ready!

Name Link
📦 Wheel file https://core-previews.s3-us-west-2.amazonaws.com/pr-12814/streamlit-1.51.0-py3-none-any.whl
📦 @streamlit/component-v2-lib Download from artifacts
🕹️ Preview app pr-12814.streamlit.app (☁️ Deploy here if not accessible)

@kmcgrady kmcgrady added security-assessment-completed Security assessment has been completed for PR impact:internal PR changes only affect internal code change:refactor PR contains code refactoring without behavior change labels Oct 18, 2025
@kmcgrady kmcgrady requested a review from Copilot October 18, 2025 22:26
Copy link
Contributor

Copilot AI left a 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

@github-actions
Copy link
Contributor

github-actions bot commented Oct 18, 2025

📈 Frontend coverage change detected

The frontend unit test (vitest) coverage has increased by 0.0300%

  • Current PR: 86.0400% (49990 lines, 6974 missed)
  • Latest develop: 86.0100% (49932 lines, 6981 missed)

🎉 Great job on improving test coverage!

📊 View detailed coverage comparison

@kmcgrady kmcgrady force-pushed the 10-18-migrate_filtermainscriptelements_to_filtermainscriptelementsvisitor branch from 11248f5 to d07f3b1 Compare October 19, 2025 04:21
@kmcgrady kmcgrady force-pushed the 10-18-create_rendernodevisitor branch 2 times, most recently from b7dbed4 to dd70991 Compare October 19, 2025 16:20
@kmcgrady kmcgrady force-pushed the 10-18-migrate_filtermainscriptelements_to_filtermainscriptelementsvisitor branch from d07f3b1 to ba93847 Compare October 19, 2025 16:20
@kmcgrady kmcgrady force-pushed the 10-18-create_rendernodevisitor branch from dd70991 to e17221a Compare October 19, 2025 16:47
@kmcgrady kmcgrady force-pushed the 10-18-migrate_filtermainscriptelements_to_filtermainscriptelementsvisitor branch from ba93847 to ff00c6d Compare October 19, 2025 16:47
@kmcgrady kmcgrady force-pushed the 10-18-create_rendernodevisitor branch from e17221a to 9fdebcf Compare October 24, 2025 15:05
@kmcgrady kmcgrady force-pushed the 10-18-create_rendernodevisitor branch from 0544072 to 16dcf93 Compare October 24, 2025 18:59
@kmcgrady kmcgrady force-pushed the 10-18-migrate_filtermainscriptelements_to_filtermainscriptelementsvisitor branch from d81a036 to 030ba55 Compare October 24, 2025 18:59
@kmcgrady kmcgrady force-pushed the 10-18-create_rendernodevisitor branch from 16dcf93 to b8f1b65 Compare October 24, 2025 19:40
@kmcgrady kmcgrady marked this pull request as ready for review October 24, 2025 19:41
@kmcgrady kmcgrady force-pushed the 10-18-migrate_filtermainscriptelements_to_filtermainscriptelementsvisitor branch from 030ba55 to df8e8be Compare October 28, 2025 23:26
@kmcgrady kmcgrady force-pushed the 10-18-create_rendernodevisitor branch from b8f1b65 to 7ba0586 Compare October 28, 2025 23:26
@kmcgrady kmcgrady force-pushed the 10-18-migrate_filtermainscriptelements_to_filtermainscriptelementsvisitor branch from df8e8be to ac65e88 Compare October 29, 2025 00:18
@kmcgrady kmcgrady force-pushed the 10-18-create_rendernodevisitor branch from 7ba0586 to 477f035 Compare October 29, 2025 00:18
@kmcgrady kmcgrady force-pushed the 10-18-migrate_filtermainscriptelements_to_filtermainscriptelementsvisitor branch from ac65e88 to ec71eed Compare October 29, 2025 01:30
@kmcgrady kmcgrady force-pushed the 10-18-create_rendernodevisitor branch from 477f035 to 9fe4523 Compare October 29, 2025 01:30

export type OptionalReactElement = ReactElement | null

export class RenderNodeVisitor
Copy link
Collaborator

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[]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Why public reactElements?

Copy link
Collaborator Author

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
}

Copy link
Collaborator

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(
Copy link
Collaborator

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)}</>
 * }
 */

Copy link
Collaborator

@mayagbarnes mayagbarnes left a 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

@kmcgrady kmcgrady force-pushed the 10-18-migrate_filtermainscriptelements_to_filtermainscriptelementsvisitor branch from ec71eed to e8e053c Compare October 30, 2025 03:59
@kmcgrady kmcgrady force-pushed the 10-18-create_rendernodevisitor branch 2 times, most recently from 9671db9 to e358528 Compare October 30, 2025 04:09
@kmcgrady kmcgrady force-pushed the 10-18-migrate_filtermainscriptelements_to_filtermainscriptelementsvisitor branch from e8e053c to e09e16d Compare October 30, 2025 12:48
@kmcgrady kmcgrady force-pushed the 10-18-create_rendernodevisitor branch 2 times, most recently from 2e0ae39 to 8545204 Compare October 30, 2025 14:19
@kmcgrady kmcgrady force-pushed the 10-18-migrate_filtermainscriptelements_to_filtermainscriptelementsvisitor branch from e09e16d to 2c656d7 Compare October 30, 2025 14:19
Base automatically changed from 10-18-migrate_filtermainscriptelements_to_filtermainscriptelementsvisitor to develop October 30, 2025 17:29
@kmcgrady kmcgrady force-pushed the 10-18-create_rendernodevisitor branch from 8545204 to 0f55447 Compare October 30, 2025 17:31
@kmcgrady kmcgrady force-pushed the 10-18-create_rendernodevisitor branch from 0f55447 to 761928f Compare October 31, 2025 16:42
@kmcgrady kmcgrady merged commit ba942cf into develop Oct 31, 2025
37 checks passed
@kmcgrady kmcgrady deleted the 10-18-create_rendernodevisitor branch October 31, 2025 18:58
sfc-gh-mbarnes pushed a commit that referenced this pull request Oct 31, 2025
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

change:refactor PR contains code refactoring without behavior change impact:internal PR changes only affect internal code security-assessment-completed Security assessment has been completed for PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants