Add Visitor pattern to AppNodes with a Debug Visitor #12810
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
✅ 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. |
Contributor
✅ PR preview is ready!
|
Collaborator
Author
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR introduces the Visitor design pattern to the AppNode hierarchy, enabling operations across different node types to be defined in a single location rather than scattered across node files. A DebugVisitor implementation is provided as a demonstration that generates tree-like debug representations of the node hierarchy.
Key changes:
- Added
AppNodeVisitor<T>interface defining visit methods for BlockNode and ElementNode - Implemented
DebugVisitorthat generates formatted debug output showing node structure, types, and metadata - Extended
AppNodeinterface and implementations withaccept()anddebug()methods
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
frontend/lib/src/render-tree/visitors/AppNodeVisitor.interface.ts |
Defines the visitor interface with visit methods for each node type |
frontend/lib/src/render-tree/visitors/DebugVisitor.ts |
Implements a visitor that generates tree-structured debug output |
frontend/lib/src/render-tree/visitors/DebugVisitor.test.ts |
Tests for DebugVisitor covering various node configurations and output formatting |
frontend/lib/src/render-tree/AppNode.interface.ts |
Adds accept() and debug() method signatures to the AppNode interface |
frontend/lib/src/render-tree/ElementNode.ts |
Implements visitor pattern methods on ElementNode |
frontend/lib/src/render-tree/ElementNode.test.ts |
Tests for ElementNode visitor functionality |
frontend/lib/src/render-tree/BlockNode.ts |
Implements visitor pattern methods on BlockNode |
frontend/lib/src/render-tree/BlockNode.test.ts |
Tests for BlockNode visitor functionality |
frontend/lib/src/render-tree/AppRoot.ts |
Adds debug method to AppRoot that labels and formats child sections |
frontend/lib/src/render-tree/AppRoot.test.ts |
Tests AppRoot debug output format |
Contributor
📈 Frontend coverage change detectedThe frontend unit test (vitest) coverage has increased by 0.0200%
✅ Coverage change is within normal range. |
This was referenced Oct 18, 2025
710a7b8 to
5a5634d
Compare
This was referenced Oct 18, 2025
5a5634d to
f300282
Compare
fd9ced5 to
0004f15
Compare
f300282 to
f9e4e45
Compare
0004f15 to
da3475c
Compare
This was referenced Oct 19, 2025
f9e4e45 to
f4db9e6
Compare
da3475c to
7627ccb
Compare
This was referenced Oct 19, 2025
This was referenced Oct 19, 2025
7627ccb to
f5f6301
Compare
9ee7726 to
8c7d056
Compare
f5f6301 to
17e8ba1
Compare
8c7d056 to
8887654
Compare
17e8ba1 to
19b19a3
Compare
Contributor
📉 Python coverage change detectedThe Python unit test coverage has decreased by 0.0000%
✅ Coverage change is within normal range. Coverage by files
|
8887654 to
09fe9dc
Compare
09fe9dc to
53dcbe4
Compare
acbcb79 to
3e9204c
Compare
mayagbarnes
reviewed
Oct 28, 2025
| expect(result).toBeUndefined() | ||
| }) | ||
|
|
||
| it("can return a modified BlockNode through visitor", () => { |
Collaborator
There was a problem hiding this comment.
Can you help me understand this scenario a little better?
Collaborator
Author
There was a problem hiding this comment.
This seems like an overzealous AI attempt. The beauty of visitors is that they can effectively return anything, and this test is effectively testing that undefined is an option. But this is probably too much, so I will just remove.
| const runId = "1234567890abcdef" | ||
| const b = block([text("c")], runId) | ||
| const out = b.accept(new DebugVisitor()) | ||
| expect(out.split("\n")[0]).toContain("(run: 123456)") |
Contributor
There was a problem hiding this comment.
nit (non-blocking): You could make this slightly more stable by utilizing the MAX_HASH_LENGTH to create the expected value.
19b19a3 to
d469706
Compare
3e9204c to
3c19282
Compare
Base automatically changed from
10-18-break_up_appnode_into_separate_files
to
develop
October 29, 2025 00:16
3c19282 to
fd5a951
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Describe your changes
This change introduces the Visitor Design Pattern to our App Node hierarchy. This allows having one file represent an operation across all of the node types rather having to edit every node file (and lose context).
To keep the PR small and not touch other methods, I created a DebugVisitor that is designed to print out the node hierarchy (which I think will be useful in debugging). It also demonstrates the purpose of the design pattern.
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.