fix: DH-22867: Fix Table Options slide animation

packages/components/src/navigation/Stack.test.tsx

@@ -42,7 +42,7 @@ beforeAll(() => {
 
 describe('stack push and pop tests', () => {
   let texts: string[];
-  let stackItems: React.ReactNode[];
+  let stackItems: React.ReactElement[];
   let rerender: (ui: React.ReactElement) => void;
 
   function renderStack(stackCount: number, itemCount = 5) {
@@ -130,3 +130,170 @@ describe('stack push and pop tests', () => {
     expectStack([0]);
   });
 });
+
+describe('key-based view tracking', () => {
+  function runTimers() {
+    act(() => {
+      jest.runOnlyPendingTimers();
+    });
+  }
+
+  /** Builds a stack item identified by `key` but with arbitrary content. */
+  function keyedItem(key: string, content: string) {
+    return (
+      <div key={key} className={key}>
+        {content}
+      </div>
+    );
+  }
+
+  it('keeps content live when re-rendered with new props for the same key', () => {
+    // The element instance changes but the key does not - this mimics a parent
+    // (e.g. IrisGrid) re-rendering with fresh children for the same logical
+    // view. The rendered view should reflect the latest props, and no slide
+    // animation should be triggered.
+    const { rerender } = render(<Stack>{[keyedItem('a', 'v1')]}</Stack>);
+    expect(screen.getByText('v1')).toBeTruthy();
+
+    rerender(<Stack>{[keyedItem('a', 'v2')]}</Stack>);
+
+    expect(screen.getByText('v2')).toBeTruthy();
+    expect(screen.queryByText('v1')).toBeNull();
+    // No push/pop animation should have started.
+    expect(document.querySelector('.pushing-view')).toBeNull();
+    expect(document.querySelector('.popping-view')).toBeNull();
+  });
+
+  it('does not churn or remount when repeatedly re-rendered with new instances for the same keys', () => {
+    // Regression: a host (e.g. IrisGrid) re-renders frequently, handing the
+    // stack brand-new element objects that represent the same logical views.
+    // Tracking by `key` means these re-renders must not trigger a push/pop
+    // animation, and the mounted view must stay mounted (same DOM node).
+    const { rerender } = render(
+      <Stack>{[keyedItem('a', 'A'), keyedItem('b', 'B')]}</Stack>
+    );
+    const mountedView = screen.getByText('B');
+
+    for (let i = 0; i < 5; i += 1) {
+      // Fresh element instances, identical keys.
+      rerender(<Stack>{[keyedItem('a', 'A'), keyedItem('b', 'B')]}</Stack>);
+
+      // The same DOM node remains mounted - it was never unmounted/remounted.
+      expect(screen.getByText('B')).toBe(mountedView);
+      // No slide animation was triggered by the re-render.
+      expect(document.querySelector('.pushing-view')).toBeNull();
+      expect(document.querySelector('.popping-view')).toBeNull();
+    }
+
+    // Running any timers must not flush a pending animation - there is none.
+    runTimers();
+    expect(screen.getByText('B')).toBe(mountedView);
+    expect(screen.queryByText('A')).toBeNull();
+  });
+
+  it('does not retrigger the push animation when re-rendered mid-push with the same top key', () => {
+    const { rerender } = render(<Stack>{[keyedItem('a', 'A')]}</Stack>);
+    rerender(<Stack>{[keyedItem('a', 'A'), keyedItem('b', 'B')]}</Stack>);
+    // Mid-push: main 'A' plus pushing 'B'.
+    expect(screen.getByText('A')).toBeTruthy();
+    expect(screen.getByText('B')).toBeTruthy();
+
+    // Re-render with brand-new element instances for the same keys/length.
+    rerender(<Stack>{[keyedItem('a', 'A'), keyedItem('b', 'B')]}</Stack>);
+    // Still mid-push to the same view - nothing changes.
+    expect(screen.getByText('A')).toBeTruthy();
+    expect(screen.getByText('B')).toBeTruthy();
+
+    runTimers();
+    // Push completes - 'B' becomes the main view.
+    expect(screen.getByText('B')).toBeTruthy();
+    expect(screen.queryByText('A')).toBeNull();
+  });
+
+  it('redirects the push to a new view when a different view is pushed mid-push', () => {
+    const { rerender } = render(<Stack>{[keyedItem('a', 'A')]}</Stack>);
+    rerender(<Stack>{[keyedItem('a', 'A'), keyedItem('b', 'B')]}</Stack>);
+    expect(screen.getByText('B')).toBeTruthy();
+
+    // Push a third view before the first push completes.
+    rerender(
+      <Stack>
+        {[keyedItem('a', 'A'), keyedItem('b', 'B'), keyedItem('c', 'C')]}
+      </Stack>
+    );
+    // The push now targets 'C' instead of 'B'.
+    expect(screen.getByText('A')).toBeTruthy();
+    expect(screen.getByText('C')).toBeTruthy();
+    expect(screen.queryByText('B')).toBeNull();
+
+    runTimers();
+    expect(screen.getByText('C')).toBeTruthy();
+  });
+
+  it('redirects the main view when the top changes mid-pop', () => {
+    const { rerender } = render(
+      <Stack>{[keyedItem('a', 'A'), keyedItem('b', 'B')]}</Stack>
+    );
+    expect(screen.getByText('B')).toBeTruthy();
+
+    // Pop down to 'A' - starts the pop animation for 'B'.
+    rerender(<Stack>{[keyedItem('a', 'A')]}</Stack>);
+    expect(screen.getByText('A')).toBeTruthy();
+    expect(screen.getByText('B')).toBeTruthy();
+
+    // While 'B' is still popping out, swap the remaining view to 'C'.
+    rerender(<Stack>{[keyedItem('c', 'C')]}</Stack>);
+    expect(screen.getByText('C')).toBeTruthy();
+    expect(screen.queryByText('A')).toBeNull();
+    // 'B' is still animating out.
+    expect(screen.getByText('B')).toBeTruthy();
+
+    runTimers();
+    expect(screen.getByText('C')).toBeTruthy();
+    expect(screen.queryByText('B')).toBeNull();
+  });
+
+  it('does not churn the main view when re-rendered mid-pop with the same remaining view', () => {
+    const { rerender } = render(
+      <Stack>{[keyedItem('a', 'A'), keyedItem('b', 'B')]}</Stack>
+    );
+
+    // Pop down to 'A' - starts the pop animation for 'B'.
+    rerender(<Stack>{[keyedItem('a', 'A')]}</Stack>);
+    expect(screen.getByText('A')).toBeTruthy();
+    expect(screen.getByText('B')).toBeTruthy();
+
+    // Re-render mid-pop with a new element instance for the same key 'A'.
+    // The top key is unchanged, so the main view is left untouched.
+    rerender(<Stack>{[keyedItem('a', 'A2')]}</Stack>);
+    expect(screen.getByText('A2')).toBeTruthy();
+    // 'B' is still animating out.
+    expect(screen.getByText('B')).toBeTruthy();
+
+    runTimers();
+    expect(screen.getByText('A2')).toBeTruthy();
+    expect(screen.queryByText('B')).toBeNull();
+  });
+
+  it('swaps the main view without animating when the top key changes at the same depth', () => {
+    // Same stack length but a different top key - the main view is replaced
+    // directly, with no push/pop animation.
+    const { rerender } = render(<Stack>{[keyedItem('a', 'A')]}</Stack>);
+    expect(screen.getByText('A')).toBeTruthy();
+
+    rerender(<Stack>{[keyedItem('b', 'B')]}</Stack>);
+    expect(screen.getByText('B')).toBeTruthy();
+    expect(screen.queryByText('A')).toBeNull();
+    expect(document.querySelector('.pushing-view')).toBeNull();
+    expect(document.querySelector('.popping-view')).toBeNull();
+  });
+
+  it('renders nothing when there are no children', () => {
+    const { container } = render(<Stack>{[]}</Stack>);
+    const mainView = container.querySelector('.main-view');
+    expect(mainView).not.toBeNull();
+    expect(mainView).toBeEmptyDOMElement();
+    expect(document.querySelector('.pushing-view')).toBeNull();
+    expect(document.querySelector('.popping-view')).toBeNull();
+  });
+});

packages/components/src/navigation/Stack.tsx

@@ -4,10 +4,52 @@ import './Stack.scss';
 import { SlideTransition } from '../transitions';
 
 export type StackProps = {
-  children: React.ReactNode | React.ReactNode[];
+  /**
+   * Stack views to animate between. Each child must be a React element with a
+   * stable `key`; views are tracked by that key across re-renders (primitive
+   * nodes such as strings/numbers are not supported).
+   */
+  children: React.ReactElement | React.ReactElement[];
   'data-testid'?: string;
 };
 
+/**
+ * Returns the React `key` for an element, or `null` for non-elements.
+ *
+ * The parent that renders the stack may re-render frequently (e.g. an
+ * `IrisGrid` re-renders on every table tick) and hand us brand-new element
+ * objects that represent the same logical view. Tracking the animating views
+ * by `key` (rather than by element reference) means those re-renders neither
+ * retrigger the slide animation nor churn state every frame.
+ *
+ * Children are always run through `React.Children.toArray`, which assigns a
+ * stable key to every element, so element views always have a key.
+ */
+function getViewKey(node: React.ReactNode): React.Key | null {
+  return React.isValidElement(node) ? node.key : null;
+}
+
+/**
+ * Finds the child that currently represents the view identified by `key`.
+ *
+ * Returning the *current* element (instead of a stored copy) keeps the rendered
+ * view's content live: when a parent re-renders with a new element instance for
+ * the same logical view, the view's latest props are forwarded to the mounted
+ * component. This matters for prop-driven panels such as the "Organize Columns"
+ * (`VisibilityOrderingBuilder`) sidebar, whose list contents and Undo/Redo state
+ * arrive entirely via props from the grid - freezing those props would stop the
+ * panel from reflecting moves, hides, groupings, and undo/redo.
+ */
+function findViewByKey(
+  views: readonly React.ReactNode[],
+  key: React.Key | null
+): React.ReactNode {
+  if (key == null) {
+    return null;
+  }
+  return views.find(view => getViewKey(view) === key) ?? null;
+}
+
 /**
  * Pass a full navigation stack of children, and then automatically does a sliding animation when the stack changes.
  * Adding items to the stack will do a "push" animation, and removing items from the stack will do a "pop" animation.
@@ -21,11 +63,21 @@ export function Stack({
     [children]
   );
   const prevChildrenArray = usePrevious(childrenArray);
-  const [mainView, setMainView] = useState<React.ReactNode>(
-    childrenArray[childrenArray.length - 1]
+
+  // The animating views are tracked by `key`, not element identity, so that
+  // frequent parent re-renders (which produce new-but-equivalent elements)
+  // neither retrigger animations nor freeze content. The element actually
+  // rendered is always looked up from the latest `childrenArray` via
+  // `findViewByKey`, keeping each view's props live.
+  const [mainViewKey, setMainViewKey] = useState<React.Key | null>(() =>
+    getViewKey(childrenArray[childrenArray.length - 1])
   );
 
-  const [pushingView, setPushingView] = useState<React.ReactNode>(null);
+  const [pushingViewKey, setPushingViewKey] = useState<React.Key | null>(null);
+
+  // The popping view has already been removed from `children`, so we keep the
+  // element instance itself - it is only animating out and its content does not
+  // need to stay live.
   const [poppingView, setPoppingView] = useState<React.ReactNode>(null);
 
   /**
@@ -36,6 +88,10 @@ export function Stack({
    *
    * When the `pushingView` or `poppingView` is set, that will kick off their animation.
    * Completion of the animation is handled in `pushComplete` or `popComplete`, and then the stack is in an idle state again.
+   *
+   * Views are compared by `key` (see `getViewKey`) so that a parent that
+   * re-renders with new element instances for the same logical view does not
+   * retrigger animations or churn state on every render.
    */
   useEffect(
     function initAnimation() {
@@ -45,42 +101,51 @@ export function Stack({
       ) {
         return;
       }
-      const topChild = childrenArray[childrenArray.length - 1];
-      if (
+      const topChildKey = getViewKey(childrenArray[childrenArray.length - 1]);
+
+      if (pushingViewKey !== null || poppingView !== null) {
+        // We're mid-animation. Keep the animating view pointed at the current
+        // top view, but don't start a new animation.
+        if (pushingViewKey !== null) {
+          if (topChildKey !== pushingViewKey) {
+            // A different view was pushed mid-animation - animate to it instead
+            setPushingViewKey(topChildKey);
+          }
+        } else if (topChildKey !== mainViewKey) {
+          setMainViewKey(topChildKey);
+        }
+      } else if (
         childrenArray.length === prevChildrenArray.length ||
-        prevChildrenArray.length === 0 ||
-        pushingView !== null ||
-        poppingView !== null
+        prevChildrenArray.length === 0
       ) {
-        // 1) Stack is the same size, we've just mounted, or we're already in an animation - just update the view
-        if (pushingView !== null && topChild !== pushingView) {
-          // Stack was updated mid animation
-          setPushingView(topChild);
-        } else if (topChild !== poppingView && topChild !== mainView) {
-          // Replace the current view
-          setMainView(topChild);
+        // Stack is the same size or we've just mounted - just update the view
+        if (topChildKey !== mainViewKey) {
+          setMainViewKey(topChildKey);
         }
       } else if (childrenArray.length > prevChildrenArray.length) {
-        // 2) Stack has grown - start the push animation
-        setPushingView(topChild);
-      } else if (childrenArray.length < prevChildrenArray.length) {
-        // 3) Stack has shrunk - start the pop animation
-        setMainView(topChild);
+        // Stack has grown - start the push animation
+        setPushingViewKey(topChildKey);
+      } else {
+        // Stack has shrunk - start the pop animation
+        setMainViewKey(topChildKey);
         setPoppingView(prevChildrenArray[prevChildrenArray.length - 1]);
       }
     },
-    [childrenArray, prevChildrenArray, pushingView, poppingView, mainView]
+    [childrenArray, prevChildrenArray, pushingViewKey, poppingView, mainViewKey]
   );
 
   const pushComplete = useCallback(() => {
-    setMainView(pushingView);
-    setPushingView(null);
-  }, [pushingView]);
+    setMainViewKey(pushingViewKey);
+    setPushingViewKey(null);
+  }, [pushingViewKey]);
 
   const popComplete = useCallback(() => {
     setPoppingView(null);
   }, []);
 
+  const mainView = findViewByKey(childrenArray, mainViewKey);
+  const pushingView = findViewByKey(childrenArray, pushingViewKey);
+
   return (
     <div className="navigation-stack">
       <div className="main-view" data-testid={dataTestId}>