• [DynamicContainers] Callback support for expanders #14008
• [DynamicContainers] Dynamic Expanders #13888
• [refactor] improve Expander animation with ResizeObserver, smooth interruption, and nullable expanded handling #13934
• [refactor] extract Expander animation logic into useDetailsAnimation hook #13933 👈 (View in Graphite)
• develop

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

✅ 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 refactors the Expander component by extracting its animation logic into two new modules:

1. animateHeight.ts — A pure utility that wraps the Web Animations API for height transitions, returning a cancelable AnimationHandle.
2. useDetailsAnimation.ts — A custom React hook encapsulating the <details> open/close state, refs, backend proto sync, and the Safari two-step repaint workaround.

The Expander.tsx component is simplified from ~150 lines of logic to ~50, consuming the hook and focusing purely on rendering. No behavioral changes are intended.

Code Quality

The refactoring is well-structured and follows the project's coding conventions:

• Named constants: Magic numbers (100 ms delay, 5 px height) are replaced with descriptive module-level constants (SAFARI_REPAINT_DELAY_MS, SAFARI_REPAINT_HEIGHT), improving readability.
• JSDoc comments: Both new modules have thorough documentation, including the AnimationHandle interface, hook options, and the Safari workaround rationale.
• Referential stability: cancelAnimation, startAnimation, and handleToggle are wrapped in useCallback with correct dependency arrays. The handleToggle dependency on isOpen is necessary since the animation branching logic reads the current state.
• Cleanup: The unmount effect properly cancels running animations and pending timeouts. The animateHeight utility correctly differentiates between "finish" (clears styles) and "cancel" (preserves styles for the caller).
• RORO pattern: The hook follows the Receive-an-Object, Return-an-Object pattern per AGENTS.md.
• Naming: Refs use the Ref suffix (detailsRef, summaryRef, etc.) and handlers use the handle prefix (handleToggle), both enforced by project conventions.
• Effect usage: The useEffect for syncing backend proto state is justified — it synchronizes with an external system (the protobuf state from the server). The eslint-disable comment is updated from a vague TODO to a clear Syncing with external proto state.

Minor observation (not a blocker): The startAnimation function in useDetailsAnimation.ts always calls cancelAnimation() which clears both animation and timeout refs independently. The original code only cleared the timeout when there was an animation to cancel. This is actually a slight improvement — it's more robust against edge cases where a timeout exists without a corresponding animation.

Behavioral equivalence note: I verified the onAnimationFinish behavior. In the original code, the finish handler set detailsEl.open for both open and close directions. In the refactored code, onFinish is only provided for the closing path. This is correct because during opening, detailsEl.open = true is already set at the start of the toggle handler, making the finish handler's open = true a no-op. The style clearing order differs slightly (new code clears styles before calling onFinish), but since both operations execute synchronously in the same event handler callback, no visual difference is possible.

Test Coverage

New unit tests are comprehensive:

• animateHeight.test.ts (175 lines): Tests animation creation with default/custom params, cancel behavior, finish behavior (style clearing, onFinish callback, promise resolution), and the cancel-does-not-clear-styles contract.
• useDetailsAnimation.test.ts (221 lines): Tests initial state for all input types (true, false, null, undefined), toggle behavior, preventDefault calling, backend sync (prop changes, null/ClearField preservation, label-change reset), and unmount cleanup.

Existing tests are unaffected:

• Expander.test.tsx continues to cover the integrated component behavior (rendering, expanding/collapsing, icon display, inert attribute toggling).
• E2E tests (st_expander_test.py, st_expander_state_test.py) exist and cover end-to-end behavior.
• The vitest.setup.ts change adds cancel: vi.fn() to the global Element.prototype.animate mock, which the new animateHeight utility needs.

One area for potential improvement (non-blocking): The useDetailsAnimation hook tests don't exercise the animation branching logic within handleToggle (opening vs. closing paths, the Safari workaround). This is understandable since renderHook doesn't create real DOM elements with dimensions (getBoundingClientRect returns zeros), making the refs null and the animation code unreachable. The integration-level Expander tests and E2E tests provide this coverage.

Backwards Compatibility

No breaking changes. This is a purely internal refactoring:

• No public API changes (Python or TypeScript exports).
• No protobuf changes.
• The Expander component's props, rendered output, test IDs, and CSS class names are all unchanged.
• The ExpanderIcon component is unmodified.
• The BORDER_SIZE constant remains exported from styled-components.ts and is now also consumed by the hook (was previously consumed by the component directly).

Security & Risk

No security concerns. This is a frontend-only refactoring with no changes to data handling, network communication, or authentication. The risk of regression is low due to:

• The animation logic is a faithful extraction with no intentional behavioral changes.
• Existing unit tests and E2E tests pass.
• The refactoring introduces better cleanup (unmount effect) that the original code lacked.

Accessibility

No accessibility impact. The refactoring preserves all existing accessibility features:

• The <details>/<summary> semantic HTML structure is unchanged.
• The inert attribute behavior on collapsed content is preserved.
• Keyboard interaction (via the native <summary> element) is unaffected.
• The onClick handler on <summary> (a natively interactive element) remains appropriate.

Recommendations

1. Consider adding a requestAnimationFrame cleanup (optional, non-blocking): Neither the old nor the new code cancels the requestAnimationFrame callback on unmount. If the component unmounts during the opening animation's rAF frame, the callback will execute against a detached DOM element. This is harmless but technically wasteful. A useRef for the rAF ID with cleanup in the unmount effect would be a minor improvement. However, since this is pre-existing behavior and the edge case is extremely narrow, this is not a required change for this PR.

2. Extract statusIconTestIds to module scope (optional, non-blocking): In Expander.tsx line 51, the statusIconTestIds record is recreated on every render of ExpanderIcon. Per the project's "Static Data Structures" guidelines, this should be at module level. This is pre-existing and not introduced by this PR, but could be addressed as a follow-up.

Verdict

APPROVED: Clean, well-tested refactoring that extracts Expander animation logic into a reusable hook and utility with no behavioral changes, improved documentation, and proper cleanup.

This is an automated AI review by opus-4.6-thinking.

Consider adding a requestAnimationFrame cleanup (optional, non-blocking): Neither the old nor the new code cancels the requestAnimationFrame callback on unmount. If the component unmounts during the opening animation's rAF frame, the callback will execute against a detached DOM element. This is harmless but technically wasteful. A useRef for the rAF ID with cleanup in the unmount effect would be a minor improvement. However, since this is pre-existing behavior and the edge case is extremely narrow, this is not a required change for this PR.

This seems better to address in the next PR where the behaviour is changed.

📈 Frontend coverage change detected

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

• Current PR: 86.9600% (13936 lines, 1817 missed)
• Latest develop: 86.8900% (13920 lines, 1824 missed)

🎉 Great job on improving test coverage!

📊 View detailed coverage comparison