✅ PR preview is ready!

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

Summary

This PR fixes a regression (#13658) where creating container elements (st.columns, st.tabs) inside a container within an st.spinner context caused the app to crash with "Bad delta path index" errors. The root cause was an inconsistency between how SetNodeByDeltaPathVisitor and GetNodeByDeltaPathVisitor handle TransientNode traversal.

Key Changes:

• SetNodeByDeltaPathVisitor.visitTransientNode now treats TransientNode as transparent in the delta path hierarchy (matching GetNodeByDeltaPathVisitor)
• The fix ensures TransientNode doesn't consume an index from the path when drilling through to its anchor
• The transient wrapper is now preserved when modifying nodes inside its anchor

Code Quality

The implementation is clean and well-documented:

Strengths:

• The fix is minimal and precisely targeted to the bug
• Clear comments explain the intended behavior (lines 69-71 in SetNodeByDeltaPathVisitor.ts)
• The new logic correctly mirrors GetNodeByDeltaPathVisitor.visitTransientNode for consistency
• Immutability is maintained by creating a new TransientNode with the modified anchor

Analysis of the fix:

The old code incorrectly consumed the first path index:

const [, ...remainingPath] = this.deltaPath  // Bug: consumed an index

The new code passes through transparently:

const newAnchor = node.anchor.accept(this)  // Correct: same path, no index consumed

This aligns with GetNodeByDeltaPathVisitor (lines 74-76) which uses node.anchor?.accept(this) without modifying the path.

Test Coverage

The test coverage is thorough and appropriate:

Updated Tests:

1. "drills through anchor without consuming path index and preserves transient wrapper" - Correctly updated to use path [0] instead of [0, 0] and now verifies the TransientNode wrapper preservation
2. "throws when drilling required but no anchor exists" - Updated to use path [0] instead of [0, 1]
3. "delegates to nodeToSet.replaceTransientNodeWithSelf when path is empty" - Updated to use empty path [] instead of [0]

New Test:
4. "preserves transient nodes when setting deep inside anchor" (lines 547-578) - Excellent new test that directly reproduces the bug scenario: adding children to a container wrapped in a TransientNode

The tests follow TypeScript best practices:

• Uses it.each for parameterized tests where applicable
• Includes both positive and negative assertions
• Tests verify the specific bug scenario (transient nodes preserved when adding children)
• Uses appropriate matchers (toBeInstanceOf, toEqual, toHaveLength)

E2E Test Gap: There is no E2E test for this specific scenario, but given this is a frontend-only fix with comprehensive unit tests, the coverage appears adequate. The original issue is reproducible via manual testing per the issue description.

Backwards Compatibility

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

• Brings SetNodeByDeltaPathVisitor in line with the already-established behavior of GetNodeByDeltaPathVisitor
• Fixes a crash scenario without changing intended functionality
• Does not modify any public APIs

The change corrects behavior that was causing crashes, making it a strict improvement.

Security & Risk

Security: No security concerns identified. The changes are localized to tree traversal logic with no impact on data handling, authentication, or external communication.

Risk Assessment: Low risk.

• The fix is well-scoped to TransientNode handling in SetNodeByDeltaPathVisitor
• Unit tests comprehensively cover the changed behavior
• The fix addresses a clear regression from v1.53.0
• The PR author confirms all 5630 frontend tests pass

Recommendations

No critical issues found. Minor suggestions:

1. Consider adding an E2E test - While the unit tests are comprehensive, an E2E test matching the reproduction steps from issue App goes blank when creating element (st.tabs, st.columns) within a container in a spinner context #13658 would provide additional confidence:

   with st.spinner("Running..."):
       time.sleep(0.1)  # Small delay to trigger spinner rendering
       st.container().columns(2)

   This is optional given the thorough unit test coverage.

2. Documentation is sufficient - The PR description clearly explains the root cause and fix. The inline code comments adequately document the intended behavior.

Verdict

APPROVED: This is a well-implemented bug fix that correctly addresses the regression by making SetNodeByDeltaPathVisitor treat TransientNode consistently with GetNodeByDeltaPathVisitor. The fix is minimal, well-tested, and backwards compatible.

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

📈 Frontend coverage change detected

The frontend unit test (vitest) coverage has increased by 0.0500%

• Current PR: 86.4500% (13339 lines, 1807 missed)
• Latest develop: 86.4000% (13339 lines, 1814 missed)

🎉 Great job on improving test coverage!

📊 View detailed coverage comparison