Implement visitTransientNode on SetNodeByDeltaPathVisitor #12824
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!
|
This was referenced Oct 19, 2025
This was referenced Oct 19, 2025
This was referenced Oct 19, 2025
Collaborator
Author
Contributor
📈 Frontend coverage change detectedThe frontend unit test (vitest) coverage has increased by 0.0100%
✅ Coverage change is within normal range. |
This was referenced Oct 19, 2025
df55dda to
b29ab22
Compare
258038e to
d33a158
Compare
b29ab22 to
2494645
Compare
Contributor
📉 Python coverage change detectedThe Python unit test coverage has decreased by 0.0000%
✅ Coverage change is within normal range. Coverage by files
|
d33a158 to
f04f690
Compare
2494645 to
ddb5f47
Compare
f04f690 to
fc1217d
Compare
42286fa to
a685eb1
Compare
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR implements the replaceTransientNodeWithSelf method across the render tree node hierarchy to properly handle TransientNode replacement operations. The method is added to the AppNode interface and implemented in all three concrete node types: TransientNode, ElementNode, and BlockNode.
Key changes:
- Adds
replaceTransientNodeWithSelfmethod toAppNodeinterface for consistent API across all node types - Implements the visitor pattern in
SetNodeByDeltaPathVisitor.visitTransientNodeto handle delta path traversal and node replacement - Provides comprehensive test coverage for all new methods and their edge cases
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| AppNode.interface.ts | Adds replaceTransientNodeWithSelf method signature to the interface |
| TransientNode.ts | Implements replacement logic that merges transient node information based on timestamps |
| TransientNode.test.ts | Tests timestamp-based replacement and anchor fallback behavior |
| ElementNode.ts | Implements replacement that filters stale transient nodes and creates new TransientNode with element as anchor |
| ElementNode.test.ts | Tests scriptRunId validation, empty transient handling, and filtering |
| BlockNode.ts | Implements replacement similar to ElementNode but for block nodes |
| BlockNode.test.ts | Tests scriptRunId validation and transient filtering for blocks |
| SetNodeByDeltaPathVisitor.ts | Implements visitTransientNode using the new method to drill through anchors and replace nodes |
| SetNodeByDeltaPathVisitor.test.ts | Tests anchor drilling, error handling, and delegation to replaceTransientNodeWithSelf |
7cc56f3 to
cf891c6
Compare
d31f7d6 to
c243fdb
Compare
cf891c6 to
b440ae2
Compare
c243fdb to
ef039c6
Compare
98580f3 to
a508d7a
Compare
ef039c6 to
d576da1
Compare
Comment on lines
+84
to
+102
| public replaceTransientNodeWithSelf(node: TransientNode): AppNode { | ||
| if ( | ||
| !this.deltaMsgReceivedAt || | ||
| !node.deltaMsgReceivedAt || | ||
| this.deltaMsgReceivedAt >= node.deltaMsgReceivedAt | ||
| ) { | ||
| // Replace with this node's information, but keep the anchor if it is not being replaced | ||
|
|
||
| return new TransientNode( | ||
| this.scriptRunId, | ||
| this.anchor ?? node.anchor, | ||
| this.transientNodes, | ||
| this.deltaMsgReceivedAt | ||
| ) | ||
| } | ||
|
|
||
| // Return the original node since it was more recent | ||
| return node | ||
| } |
Contributor
There was a problem hiding this comment.
Unlike the implementations in BlockNode and ElementNode, this method doesn't check if the script run IDs differ between this and node. This inconsistency could lead to unexpected behavior when handling transient nodes from different script runs.
Consider adding a script run ID check at the beginning of the method, similar to:
if (node.scriptRunId !== this.scriptRunId) {
// Handle different script run IDs
}This would make the behavior consistent across all node types and ensure proper handling of transient nodes from different script runs.
Suggested change
| public replaceTransientNodeWithSelf(node: TransientNode): AppNode { | |
| if ( | |
| !this.deltaMsgReceivedAt || | |
| !node.deltaMsgReceivedAt || | |
| this.deltaMsgReceivedAt >= node.deltaMsgReceivedAt | |
| ) { | |
| // Replace with this node's information, but keep the anchor if it is not being replaced | |
| return new TransientNode( | |
| this.scriptRunId, | |
| this.anchor ?? node.anchor, | |
| this.transientNodes, | |
| this.deltaMsgReceivedAt | |
| ) | |
| } | |
| // Return the original node since it was more recent | |
| return node | |
| } | |
| public replaceTransientNodeWithSelf(node: TransientNode): AppNode { | |
| if (node.scriptRunId !== this.scriptRunId) { | |
| // Handle different script run IDs | |
| return node | |
| } | |
| if ( | |
| !this.deltaMsgReceivedAt || | |
| !node.deltaMsgReceivedAt || | |
| this.deltaMsgReceivedAt >= node.deltaMsgReceivedAt | |
| ) { | |
| // Replace with this node's information, but keep the anchor if it is not being replaced | |
| return new TransientNode( | |
| this.scriptRunId, | |
| this.anchor ?? node.anchor, | |
| this.transientNodes, | |
| this.deltaMsgReceivedAt | |
| ) | |
| } | |
| // Return the original node since it was more recent | |
| return node | |
| } |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
a508d7a to
ca0d171
Compare
5656d58 to
d739b88
Compare
ca0d171 to
8a8cd4b
Compare
8a8cd4b to
8c4e150
Compare
d739b88 to
1572978
Compare
Contributor
📈 Significant wheel size change detectedThe wheel file size has increased by 11.37% (threshold: 0.25%)
Please verify this change is expected. |
1572978 to
4cf1b51
Compare
8c4e150 to
5c50853
Compare
Base automatically changed from
10-19-implement_visittransientnode_on_most_visitors
to
develop
December 1, 2025 15:46
4cf1b51 to
c6876b6
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
Implemented the
replaceTransientNodeWithSelfmethod on allAppNodeimplementations to properly handle transient node replacement during delta updates. This method allows nodes to decide how to best replace a transient node with themselves, which is crucial for maintaining proper state during incremental updates.Key changes:
replaceTransientNodeWithSelfto theAppNodeinterfaceBlockNode,ElementNode, andTransientNodeclassesSetNodeByDeltaPathVisitorto use this method when replacing transient nodesTesting Plan
replaceTransientNodeWithSelfmethod in all node typesContribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.