✅ 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!

Summary

This PR fixes a bug where a TransientNode (e.g., spinner) without an anchor would not properly capture the existing BlockNode when replacing it at an empty delta path. The fix preserves the original block structure so it can be restored when transient elements are cleared.

Changes:

• SetNodeByDeltaPathVisitor.ts: Added 13 lines to visitBlockNode that mirror the existing behavior in visitElementNode - when replacing with a TransientNode that has no anchor, capture the current BlockNode as the anchor.
• SetNodeByDeltaPathVisitor.test.ts: Added 2 unit tests covering positive and negative cases.
• st_spinner.py: Added test scenario for the regression.
• st_spinner_test.py: Added e2e test test_spinner_with_delayed_container_write.

Code Quality

The code change is minimal, targeted, and follows existing patterns in the codebase:

  visitBlockNode(node: BlockNode): AppNode {
    if (this.deltaPath.length === 0) {
      // When replacing a BlockNode with a TransientNode that has no anchor,
      // capture the current BlockNode as the anchor. This preserves the original
      // block structure so it can be restored when transient elements are cleared.
      // This mirrors the analogous behavior in visitElementNode.
      if (this.nodeToSet instanceof TransientNode && !this.nodeToSet.anchor) {
        return new TransientNode(
          this.nodeToSet.scriptRunId,
          node,
          this.nodeToSet.transientNodes,
          this.nodeToSet.deltaMsgReceivedAt
        )
      }

      return this.nodeToSet
    }

Minor Observation: There's a subtle inconsistency between visitBlockNode (new fix) and visitElementNode (existing code) in which scriptRunId is used:

• visitElementNode uses this.scriptRunId (from the visitor)
• visitBlockNode uses this.nodeToSet.scriptRunId (from the TransientNode)

This is unlikely to cause issues in practice since these values should typically be the same when setting a TransientNode, but for consistency, consider using the same approach in both methods. The existing code in visitElementNode (lines 49-56) uses this.scriptRunId:

    if (this.nodeToSet instanceof TransientNode && !this.nodeToSet.anchor) {
      return new TransientNode(
        this.scriptRunId,
        node,
        this.nodeToSet.transientNodes,
        this.nodeToSet.deltaMsgReceivedAt
      )
    }

Test Coverage

Unit Tests (TypeScript):
The two new unit tests are well-structured and follow best practices:

1. wraps block node as anchor when setting TransientNode without anchor - Tests the positive case where the BlockNode should be captured as anchor.
2. does not modify TransientNode when it already has an anchor - Tests the negative case where an existing anchor should be preserved.

Both tests properly verify:

• The result is a TransientNode instance
• The anchor is correctly set or preserved
• Other properties (transientNodes, deltaMsgReceivedAt) are preserved

E2E Test (Playwright):
The e2e test test_spinner_with_delayed_container_write follows best practices:

• Uses expect for auto-wait assertions
• Uses get_button helper from shared app_utils
• Uses stable locators (get_by_test_id, get_by_text)
• Includes a clear docstring explaining the test purpose
• Has a negative assertion (to_have_count(0)) verifying the spinner disappears
• Uses wait_for_app_run for proper synchronization

The coverage is adequate for this bug fix, testing both the unit-level behavior and the real-world scenario end-to-end.

Backwards Compatibility

No breaking changes. This is a pure bug fix that:

• Only affects the specific edge case where a TransientNode without an anchor replaces a BlockNode
• Is additive and follows existing patterns
• Does not change any public APIs

Security & Risk

Security: No security concerns identified. The change is purely internal to the render tree visitor logic.

Risk Assessment: Low risk of regression because:

• The fix is minimal and well-targeted
• It mirrors existing, proven behavior in visitElementNode
• The e2e test covers the specific regression scenario
• The unit tests validate the boundary conditions

Recommendations

1. Optional Consistency Fix: Consider aligning the scriptRunId usage between visitBlockNode and visitElementNode for consistency. Both should probably use this.scriptRunId (the visitor's scriptRunId) to match the existing pattern:

// In visitBlockNode, change:
this.nodeToSet.scriptRunId
// To:
this.scriptRunId

This is a minor suggestion and the current implementation should work correctly in practice.

Verdict

APPROVED: This is a well-crafted bug fix with appropriate test coverage. The code follows existing patterns, the tests are comprehensive, and there are no backwards compatibility or security concerns. The minor scriptRunId inconsistency noted above is unlikely to cause issues in practice.

This is an automated AI review using opus-4.5-thinking. Please verify the feedback and use your judgment.