Migrate getIn to GetNodeByDeltaPathVisitor #12812

kmcgrady · 2025-10-18T21:22:30Z

Describe your changes

A little more complicated than the ElementSet Visitor, but this change updates the getIn Method to a GetNodeByDeltaPath visitor.

Testing Plan

JS tests are updated
E2E tests should pass

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 · 2025-10-18T21:22:41Z

✅ 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 · 2025-10-18T21:22:45Z

✅ PR preview is ready!

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

kmcgrady · 2025-10-18T21:22:49Z

This stack of pull requests is managed by Graphite. Learn more about stacking.

Copilot

Pull Request Overview

This PR migrates the getIn method functionality from AppNode implementations to a new GetNodeByDeltaPathVisitor class, following the visitor pattern. This change replaces the recursive getIn method that was directly implemented in BlockNode and ElementNode classes.

Key changes:

Introduces a new visitor pattern for node traversal by delta path
Removes the getIn method from the AppNode interface and implementations
Updates all existing usages to use the new visitor pattern

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
`GetNodeByDeltaPathVisitor.ts`	New visitor class that implements delta path traversal logic
`GetNodeByDeltaPathVisitor.test.ts`	Comprehensive test suite for the new visitor
`ElementNode.ts`	Removes the `getIn` method implementation
`BlockNode.ts`	Removes the `getIn` method implementation
`BlockNode.test.ts`	Updates tests to use visitor pattern via helper function
`AppRoot.ts`	Adds visitor usage and helper methods for node traversal
`AppRoot.test.ts`	Updates tests to use the new visitor pattern
`AppNode.interface.ts`	Removes `getIn` method from interface definition

Copilot

Pull Request Overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

frontend/lib/src/render-tree/visitors/GetNodeByDeltaPathVisitor.ts

frontend/lib/src/render-tree/AppRoot.ts

github-actions · 2025-10-18T22:02:13Z

📉 Frontend coverage change detected

The frontend unit test (vitest) coverage has decreased by 0.0000%

Current PR: 86.0300% (49876 lines, 6964 missed)
Latest develop: 86.0300% (49844 lines, 6963 missed)

✅ Coverage change is within normal range.

📊 View detailed coverage comparison

sfc-gh-lwilby · 2025-10-29T13:49:38Z

frontend/lib/src/render-tree/visitors/GetNodeByDeltaPathVisitor.ts

+/**
+ * A visitor that retrieves a node at a specific delta path.
+ *
+ * This replaces the getIn method functionality from AppNode implementations.


[suggestion] I can see this context being useful at first, but maybe it is better in the PR description/commit messages and then keep the docstring referencing only the current functionality.

sfc-gh-lwilby · 2025-10-29T14:06:29Z

frontend/lib/src/render-tree/AppRoot.ts

    )
  }

  private addBlock(


[suggestion] not for this PR, but should this actually be called replaceNode?

Yea. It's weird because it's aligning to delta protobuf, but this is practically a replacement.

sfc-gh-lwilby · 2025-10-29T14:16:50Z

frontend/lib/src/render-tree/visitors/GetNodeByDeltaPathVisitor.ts

+  }
+
+  visitElementNode(_node: ElementNode): AppNode | undefined {
+    // ElementNodes are leaf nodes - they have no children to traverse


Suggested change

// ElementNodes are leaf nodes - they have no children to traverse

// ElementNodes have no children and therefore we do not check them for the element.

mayagbarnes · 2025-10-30T01:35:13Z

frontend/lib/src/render-tree/visitors/GetNodeByDeltaPathVisitor.ts

+    }
+
+    if (remainingPath.length === 0) {
+      // Base case: we're at the target location, return the child


Nit: Perhaps
// Base case: we've consumed the entire path (at target location), return the target node

mayagbarnes · 2025-10-30T01:35:43Z

frontend/lib/src/render-tree/visitors/GetNodeByDeltaPathVisitor.ts

+      return node.children[currentIndex]
+    }
+
+    // Recursive case: continue down the path


Nit: Perhaps
// Recursive case: create a new visitor and continue traversal

This was referenced Oct 18, 2025

Break up AppNode into separate files #12809

Merged

Add Visitor pattern to AppNodes with a Debug Visitor #12810

Merged

kmcgrady mentioned this pull request Oct 18, 2025

Migrate getElements to the ElementsSetVisitor #12811

Merged

kmcgrady added impact:internal PR changes only affect internal code change:refactor PR contains code refactoring without behavior change security-assessment-completed Security assessment has been completed for PR labels Oct 18, 2025

kmcgrady requested a review from Copilot October 18, 2025 21:24

Copilot AI reviewed Oct 18, 2025

View reviewed changes

kmcgrady force-pushed the 10-18-migrate_getin_to_getnodebydeltapathvisitor branch from 8f785ac to 4ee6955 Compare October 18, 2025 21:36

kmcgrady force-pushed the 10-18-migrate_getelements_to_the_elementssetvisitor branch 2 times, most recently from eef2346 to fbd52d2 Compare October 18, 2025 21:41

kmcgrady force-pushed the 10-18-migrate_getin_to_getnodebydeltapathvisitor branch from 4ee6955 to 7d9edc4 Compare October 18, 2025 21:41

kmcgrady force-pushed the 10-18-migrate_getelements_to_the_elementssetvisitor branch from fbd52d2 to 1a8a94b Compare October 18, 2025 21:43

kmcgrady force-pushed the 10-18-migrate_getin_to_getnodebydeltapathvisitor branch from 7d9edc4 to 3448d32 Compare October 18, 2025 21:43

kmcgrady requested a review from Copilot October 18, 2025 21:43

Copilot AI reviewed Oct 18, 2025

View reviewed changes

frontend/lib/src/render-tree/visitors/GetNodeByDeltaPathVisitor.ts Outdated Show resolved Hide resolved

frontend/lib/src/render-tree/AppRoot.ts Outdated Show resolved Hide resolved

kmcgrady force-pushed the 10-18-migrate_getin_to_getnodebydeltapathvisitor branch from 3448d32 to b320d2a Compare October 18, 2025 21:47

kmcgrady force-pushed the 10-18-migrate_getelements_to_the_elementssetvisitor branch from 1a8a94b to 38db539 Compare October 18, 2025 21:47

This was referenced Oct 18, 2025

Migrate filterMainScriptElements to FilterMainScriptElementsVisitor #12813

Merged

Create RenderNodeVisitor #12814

Merged

kmcgrady force-pushed the 10-18-migrate_getin_to_getnodebydeltapathvisitor branch from b320d2a to 9f84e8e Compare October 19, 2025 04:21

kmcgrady force-pushed the 10-18-migrate_getelements_to_the_elementssetvisitor branch 2 times, most recently from c65a132 to 1f05240 Compare October 19, 2025 16:20

kmcgrady force-pushed the 10-18-migrate_getin_to_getnodebydeltapathvisitor branch from 9f84e8e to 770c9d0 Compare October 19, 2025 16:20

This was referenced Oct 19, 2025

Create E2E tests for common AppNode scenarios not particularly tested #12818

Merged

Convert clearStaleNodes to ClearStaleNodesVisitor #12819

Merged

kmcgrady force-pushed the 10-18-migrate_getelements_to_the_elementssetvisitor branch from 951bcac to c3d6ea0 Compare October 24, 2025 18:50

kmcgrady force-pushed the 10-18-migrate_getin_to_getnodebydeltapathvisitor branch from 80b1478 to 97cd5f2 Compare October 24, 2025 18:50

kmcgrady force-pushed the 10-18-migrate_getelements_to_the_elementssetvisitor branch from c3d6ea0 to ad210aa Compare October 24, 2025 18:53

kmcgrady force-pushed the 10-18-migrate_getin_to_getnodebydeltapathvisitor branch from 97cd5f2 to 0b3dd40 Compare October 24, 2025 18:53

kmcgrady force-pushed the 10-18-migrate_getelements_to_the_elementssetvisitor branch from ad210aa to 22c1b20 Compare October 24, 2025 18:55

kmcgrady force-pushed the 10-18-migrate_getin_to_getnodebydeltapathvisitor branch from 0b3dd40 to 1788c2a Compare October 24, 2025 18:55

kmcgrady force-pushed the 10-18-migrate_getelements_to_the_elementssetvisitor branch from 22c1b20 to cfa91f3 Compare October 24, 2025 18:59

kmcgrady force-pushed the 10-18-migrate_getin_to_getnodebydeltapathvisitor branch from 1788c2a to c52b408 Compare October 24, 2025 18:59

kmcgrady marked this pull request as ready for review October 24, 2025 19:03

kmcgrady force-pushed the 10-18-migrate_getin_to_getnodebydeltapathvisitor branch from c52b408 to b9afac2 Compare October 28, 2025 23:26

kmcgrady force-pushed the 10-18-migrate_getelements_to_the_elementssetvisitor branch 2 times, most recently from d7b0129 to 7b930f0 Compare October 29, 2025 00:18

kmcgrady force-pushed the 10-18-migrate_getin_to_getnodebydeltapathvisitor branch 2 times, most recently from 3ecb1d2 to 97426f6 Compare October 29, 2025 01:30

kmcgrady force-pushed the 10-18-migrate_getelements_to_the_elementssetvisitor branch from 7b930f0 to 0bf7c7e Compare October 29, 2025 01:30

sfc-gh-lwilby reviewed Oct 29, 2025

View reviewed changes

sfc-gh-lwilby approved these changes Oct 29, 2025

View reviewed changes

mayagbarnes reviewed Oct 30, 2025

View reviewed changes

mayagbarnes approved these changes Oct 30, 2025

View reviewed changes

kmcgrady force-pushed the 10-18-migrate_getin_to_getnodebydeltapathvisitor branch from 97426f6 to 96013c8 Compare October 30, 2025 03:59

kmcgrady force-pushed the 10-18-migrate_getelements_to_the_elementssetvisitor branch from 0bf7c7e to dc7d050 Compare October 30, 2025 03:59

Base automatically changed from 10-18-migrate_getelements_to_the_elementssetvisitor to develop October 30, 2025 12:30

Migrate getIn to GetNodeByDeltaPathVisitor

4e7ec2f

kmcgrady force-pushed the 10-18-migrate_getin_to_getnodebydeltapathvisitor branch from 96013c8 to 4e7ec2f Compare October 30, 2025 12:48

kmcgrady merged commit d0741e6 into develop Oct 30, 2025
37 checks passed

kmcgrady deleted the 10-18-migrate_getin_to_getnodebydeltapathvisitor branch October 30, 2025 14:11

kmcgrady mentioned this pull request Dec 1, 2025

Add ability to clear transient nodes when the script is run #13166

Merged

	// ElementNodes are leaf nodes - they have no children to traverse
	// ElementNodes have no children and therefore we do not check them for the element.

Migrate getIn to GetNodeByDeltaPathVisitor #12812

Migrate getIn to GetNodeByDeltaPathVisitor #12812

Uh oh!

Conversation

kmcgrady commented Oct 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Describe your changes

Testing Plan

Uh oh!

snyk-io bot commented Oct 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Uh oh!

github-actions bot commented Oct 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ PR preview is ready!

Uh oh!

kmcgrady commented Oct 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Oct 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📉 Frontend coverage change detected

Uh oh!

sfc-gh-lwilby Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

sfc-gh-lwilby Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

kmcgrady Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

sfc-gh-lwilby Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

mayagbarnes Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

mayagbarnes Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kmcgrady commented Oct 18, 2025 •

edited

Loading

snyk-io bot commented Oct 18, 2025 •

edited

Loading

github-actions bot commented Oct 18, 2025 •

edited

Loading

kmcgrady commented Oct 18, 2025 •

edited

Loading

github-actions bot commented Oct 18, 2025 •

edited

Loading