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

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

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

✅ PR preview is ready!

• Add ability to clear transient nodes when the script is run #13166 👈 (View in Graphite)
• Implement transient spinners #12826
• Migrate spinner to a Mixin #12829
• Implement visitTransientNode on RenderNodeVisitor #12825
• Implement visitTransientNode on SetNodeByDeltaPathVisitor #12824
• Implement visitTransientNode on most visitors #12823
• Introduce TransientNode #12822
• Convert setIn to SetNodeByDeltaPathVisitor #12820
• Convert clearStaleNodes to ClearStaleNodesVisitor #12819
• Create RenderNodeVisitor #12814
• Migrate filterMainScriptElements to FilterMainScriptElementsVisitor #12813
• Migrate getIn to GetNodeByDeltaPathVisitor #12812
• Migrate getElements to the ElementsSetVisitor #12811
• Add Visitor pattern to AppNodes with a Debug Visitor #12810
• Break up AppNode into separate files #12809
• Create E2E tests for common AppNode scenarios not particularly tested #12818
• develop

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

📉 Python coverage change detected

The Python unit test coverage has decreased by 0.0047%

• Current PR: 92.6944% (21203 statements, 1549 missed)
• Latest develop: 92.6991% (21203 statements, 1548 missed)

✅ Coverage change is within normal range.

📊 View detailed coverage comparison

Something seems broken with this PR, it seems to contain all the changes from the already merged #12826. Maybe try to bring it up-to-date with develop?

Summary

This PR adds the ability to clear transient nodes from the render tree when a script is re-run. The implementation:

1. Adds a new ClearTransientNodesVisitor class that traverses the tree and replaces transient nodes with their anchor nodes
2. Adds a clearTransientNodes method to AppRoot that delegates to the visitor
3. Updates App.tsx to call clearTransientNodes before executing a script run
4. Includes a minor import path fix in ClearStaleNodeVisitor.ts

The purpose is to ensure transient elements (like status messages, spinners) don't persist inappropriately between script runs.

Code Quality

Overall Assessment

The code follows the existing visitor pattern well and is consistent with ClearStaleNodeVisitor. The implementation is clean and includes a performance optimization that returns the same node reference when children haven't changed.

Issues Found

1. Naming Inconsistency (Minor)

• File: ClearTransientNodeVisitor.ts (singular "Node")
• Class: ClearTransientNodesVisitor (plural "Nodes")

This is inconsistent with ClearStaleNodeVisitor.ts where both file and class use singular. Recommend renaming the class to ClearTransientNodeVisitor for consistency.

2. App.tsx setState Pattern (Minor Observation)
The change in App.tsx (lines 1369-1373) correctly uses the updater function pattern for setState to access prevState.elements. This is the proper way to handle state updates that depend on previous state.

this.setState(prevState => ({
  elements: prevState.elements.clearTransientNodes(fragmentIdsThisRun),
  scriptRunId,
}))

This is correct and follows React best practices.

Test Coverage

Strengths

• ClearTransientNodeVisitor.test.ts: Comprehensive unit tests covering:

  • Element nodes returned unchanged
  • Transient nodes replaced with their anchors
  • Block node recursion
  • Performance optimization (same node reference when no changes)
  • Fragment ID filtering with fragmentIdsThisRun
  • Blocks without fragmentId still get traversed

• AppRoot.test.ts: Integration tests for clearTransientNodes covering:

  • Basic clearing behavior (transient → anchor)
  • Fragment ID filtering

Potential Gaps

1. No explicit test for TransientNode with undefined anchor: While implicitly covered by the "respects fragmentIdsThisRun when clearing" test in AppRoot.test.ts (where the transient at [0, 2, 0] has no anchor and returns undefined), an explicit unit test in ClearTransientNodeVisitor.test.ts would improve clarity:

it("returns undefined when transient node has no anchor", () => {
  const transientWithNoAnchor = new TransientNode(
    "script_run_id",
    undefined, // no anchor
    [text("Transient")],
    Date.now()
  )
  const visitor = new ClearTransientNodesVisitor([])
  expect(visitor.visitTransientNode(transientWithNoAnchor)).toBeUndefined()
})

2. E2E Test Coverage: No E2E tests are included. While this may be acceptable for internal render-tree logic, consider whether end-to-end verification of transient clearing behavior is needed for user-facing scenarios.

Backwards Compatibility

Assessment: No Breaking Changes

• The clearTransientNodes method is additive to AppRoot
• The new visitor follows the established interface pattern
• The App.tsx change is internal to the app component
• No public API changes

Security & Risk

Low Risk

• No external input handling
• No security-sensitive operations
• The change is isolated to render tree manipulation
• Follows immutable data patterns (creates new nodes rather than mutating)

Potential Risk: Fragment ID Handling

The fragment ID filtering logic mirrors ClearStaleNodeVisitor, which is good for consistency. However, ensure this aligns with the expected behavior when running fragments:

• If fragmentIdsThisRun is empty/undefined, all transient nodes are cleared
• If fragmentIdsThisRun contains IDs, only blocks with matching fragment IDs (or no fragment ID) are traversed

This appears to be the intended behavior based on the test cases.

Recommendations

1. Rename class for consistency (Minor)

   In frontend/lib/src/render-tree/visitors/ClearTransientNodeVisitor.ts:

   • Rename ClearTransientNodesVisitor → ClearTransientNodeVisitor (singular)
   • This matches the file name and is consistent with ClearStaleNodeVisitor

2. Add explicit test for undefined anchor (Minor)

   In ClearTransientNodeVisitor.test.ts, add a test case for transient nodes without anchors:

   it("returns undefined when transient node has no anchor", () => {
     const transientWithNoAnchor = new TransientNode(
       "script_run_id",
       undefined,
       [text("Transient")],
       Date.now()
     )
     const visitor = new ClearTransientNodesVisitor([])
     expect(visitor.visitTransientNode(transientWithNoAnchor)).toBeUndefined()
   })

3. Consider adding JSDoc to the public method

   The clearTransientNodes method in AppRoot.ts could benefit from a brief JSDoc comment explaining when/why it's called, similar to clearStaleNodes.

Verdict

APPROVED: The implementation is solid, follows established patterns, and includes comprehensive test coverage. The naming inconsistency and missing explicit test case for undefined anchors are minor issues that don't block merging. The changes are backwards compatible and pose no security risks.

This is an automated AI review. Please verify the feedback and use your judgment.

@cursor review