Improve Group imperative API when in hidden sub-tree (#549)

Relates to #536

Author: bvaughn

CHANGELOG.md

@@ -5,6 +5,7 @@
 - [#542](https://github.com/bvaughn/react-resizable-panels/pull/542): Clicks on higher `z-index` elements (e.g. modals) should not trigger separators behind them
 - [#547](https://github.com/bvaughn/react-resizable-panels/pull/547): Don't re-mount Group when `defaultLayout` or `disableCursor` props change
 - [#548](https://github.com/bvaughn/react-resizable-panels/pull/548): Bugfix: Gracefully handle `Panel` id changes
+- [#549](https://github.com/bvaughn/react-resizable-panels/pull/549): Improve DevX when Group within hidden DOM subtree; defer layout-change events
 
 ## 4.0.8
 

integrations/vite/tests/visibility.spec.tsx

@@ -54,19 +54,18 @@ test.describe("visibility", () => {
       "/e2e/visibility/display/hidden/"
     );
 
-    await expect(page.getByText('"onLayoutCount": 1')).toBeVisible();
-    // Actual layout values are not meaningful since the Group is not visible
+    await expect(page.getByText('"onLayoutCount": 0')).toBeVisible();
 
     await page.getByText("toggle display hidden → visible").click();
 
-    await expect(page.getByText('"onLayoutCount": 2')).toBeVisible();
+    await expect(page.getByText('"onLayoutCount": 1')).toBeVisible();
     await expect(page.getByText("id: left")).toContainText("25%");
     await expect(page.getByRole("separator")).toBeVisible();
     await expect(page.getByText("id: right")).toContainText("75%");
 
     await page.getByText("toggle display visible → hidden").click();
 
-    await expect(page.getByText('"onLayoutCount": 2')).toBeVisible();
+    await expect(page.getByText('"onLayoutCount": 1')).toBeVisible();
     // Actual layout values are not meaningful since the Group is not visible
 
     await page.setViewportSize({
@@ -75,7 +74,7 @@ test.describe("visibility", () => {
     });
     await page.getByText("toggle display hidden → visible").click();
 
-    await expect(page.getByText('"onLayoutCount": 3')).toBeVisible();
+    await expect(page.getByText('"onLayoutCount": 2')).toBeVisible();
     await expect(page.getByText("id: left")).toContainText("33%");
     await expect(page.getByRole("separator")).toBeVisible();
     await expect(page.getByText("id: right")).toContainText("67%");

lib/components/group/Group.test.tsx

@@ -7,6 +7,8 @@ import { setElementBoundsFunction } from "../../utils/test/mockBoundingClientRec
 import { Panel } from "../panel/Panel";
 import { Separator } from "../separator/Separator";
 import { Group } from "./Group";
+import { createRef } from "react";
+import type { GroupImperativeHandle } from "./types";
 
 describe("Group", () => {
   test("changes to defaultProps or disableCursor should not cause Group to remount", () => {
@@ -28,7 +30,9 @@ describe("Group", () => {
         <Panel id="b" />
       </Group>
     );
-    expect(onMountedGroupsChange).toHaveBeenCalledTimes(1);
+    expect(onMountedGroupsChange).toHaveBeenCalled();
+
+    onMountedGroupsChange.mockReset();
 
     rerender(
       <Group
@@ -42,7 +46,7 @@ describe("Group", () => {
         <Panel id="b" />
       </Group>
     );
-    expect(onMountedGroupsChange).toHaveBeenCalledTimes(1);
+    expect(onMountedGroupsChange).not.toHaveBeenCalled();
 
     removeListener();
   });
@@ -77,6 +81,81 @@ describe("Group", () => {
     );
   });
 
+  describe("groupRef", () => {
+    test("should work with an empty Group", () => {
+      const onLayoutChange = vi.fn();
+      const groupRef = createRef<GroupImperativeHandle>();
+
+      render(<Group groupRef={groupRef} onLayoutChange={onLayoutChange} />);
+
+      const group = groupRef.current;
+
+      assert(group !== null);
+      expect(group.getLayout()).toEqual({});
+      expect(onLayoutChange).not.toHaveBeenCalled();
+
+      // This is meaningless but technically valid
+      group.setLayout({});
+
+      // This is still invalid
+      expect(() =>
+        group.setLayout({
+          foo: 50,
+          bar: 50
+        })
+      ).toThrow("Invalid 0 panel layout: 50%, 50%");
+    });
+
+    test("should work within a hidden subtree", () => {
+      // Note this test mimics the hidden subtree scenario by using a groupSize of 0
+      setElementBoundsFunction(() => new DOMRect(0, 0, 0, 0));
+
+      const onLayoutChange = vi.fn();
+      const groupRef = createRef<GroupImperativeHandle>();
+
+      render(
+        <Group groupRef={groupRef} onLayoutChange={onLayoutChange}>
+          <Panel defaultSize="35%" id="left" maxSize="45%">
+            left
+          </Panel>
+          <Panel id="right">right</Panel>
+        </Group>
+      );
+
+      const group = groupRef.current;
+
+      assert(group !== null);
+      expect(onLayoutChange).not.toHaveBeenCalled();
+
+      // Size constraints can't really be validated while the Group is hidden
+      expect(group.getLayout()).toEqual({});
+
+      // This is essentially a no-op as well
+      // Any values set at this point can't be validated and so they'll be overridden when the Group becomes visible
+      group.setLayout({
+        left: 45,
+        right: 55
+      });
+
+      // Simulate the Group becoming visible; this should trigger default layout calculation
+      setElementBoundsFunction((element) => {
+        if (element.hasAttribute("data-panel")) {
+          return new DOMRect(0, 0, 50, 50);
+        } else {
+          return new DOMRect(0, 0, 100, 50);
+        }
+      });
+      expect(group.getLayout()).toEqual({
+        left: 35,
+        right: 65
+      });
+      expect(onLayoutChange).toHaveBeenCalledWith({
+        left: 35,
+        right: 65
+      });
+    });
+  });
+
   describe("onLayoutChange", () => {
     beforeEach(() => {
       setElementBoundsFunction((element) => {

lib/components/group/Group.tsx

@@ -7,6 +7,7 @@ import { useId } from "../../hooks/useId";
 import { useIsomorphicLayoutEffect } from "../../hooks/useIsomorphicLayoutEffect";
 import { useMergedRefs } from "../../hooks/useMergedRefs";
 import { useStableCallback } from "../../hooks/useStableCallback";
+import { useStableObject } from "../../hooks/useStableObject";
 import { POINTER_EVENTS_CSS_PROPERTY_NAME } from "../panel/constants";
 import type { RegisteredPanel } from "../panel/types";
 import type { RegisteredSeparator } from "../separator/types";
@@ -15,7 +16,6 @@ import { GroupContext } from "./GroupContext";
 import { sortByElementOffset } from "./sortByElementOffset";
 import type { GroupProps, Layout, RegisteredGroup } from "./types";
 import { useGroupImperativeHandle } from "./useGroupImperativeHandle";
-import { useStableObject } from "../../hooks/useStableObject";
 
 /**
  * A Group wraps a set of resizable Panel components.
@@ -63,12 +63,13 @@ export function Group({
   const [panels, setPanels] = useState<RegisteredPanel[]>([]);
   const [separators, setSeparators] = useState<RegisteredSeparator[]>([]);
 
-  const inMemoryLastExpandedPanelSizesRef = useRef<{
-    [panelIds: string]: number;
-  }>({});
-  const inMemoryLayoutsRef = useRef<{
-    [panelIds: string]: Layout;
-  }>({});
+  const inMemoryValuesRef = useRef<{
+    lastExpandedPanelSizes: { [panelIds: string]: number };
+    layouts: { [panelIds: string]: Layout };
+  }>({
+    lastExpandedPanelSizes: {},
+    layouts: {}
+  });
 
   const mergedRef = useMergedRefs(setElement, elementRef);
 
@@ -108,7 +109,7 @@ export function Group({
   // Register Group and child Panels/Separators with global state
   // Listen to global state for drag state related to this Group
   useIsomorphicLayoutEffect(() => {
-    if (element === null || panels.length === 0) {
+    if (element === null) {
       return;
     }
 
@@ -118,8 +119,9 @@ export function Group({
       disabled: !!disabled,
       element,
       id,
-      inMemoryLastExpandedPanelSizes: inMemoryLastExpandedPanelSizesRef.current,
-      inMemoryLayouts: inMemoryLayoutsRef.current,
+      inMemoryLastExpandedPanelSizes:
+        inMemoryValuesRef.current.lastExpandedPanelSizes,
+      inMemoryLayouts: inMemoryValuesRef.current.layouts,
       orientation,
       panels,
       separators
@@ -132,8 +134,15 @@ export function Group({
     const globalState = read();
     const match = globalState.mountedGroups.get(group);
     if (match) {
-      setLayout(match.layout);
-      onLayoutChangeStable?.(match.layout);
+      const { defaultLayoutDeferred, derivedPanelConstraints, layout } = match;
+
+      if (!defaultLayoutDeferred && derivedPanelConstraints.length > 0) {
+        // This indicates that the Group has not finished mounting yet
+        // Likely because it has been rendered inside of a hidden DOM subtree
+        // Ignore layouts in this case because they will not have been validated
+        setLayout(layout);
+        onLayoutChangeStable?.(layout);
+      }
     }
 
     const removeInteractionStateChangeListener = eventEmitter.addListener(
@@ -160,9 +169,19 @@ export function Group({
       "mountedGroupsChange",
       (mountedGroups) => {
         const match = mountedGroups.get(group);
-        if (match && match.derivedPanelConstraints.length > 0) {
-          setLayout(match.layout);
-          onLayoutChangeStable?.(match.layout);
+        if (match) {
+          const { defaultLayoutDeferred, derivedPanelConstraints, layout } =
+            match;
+
+          if (defaultLayoutDeferred || derivedPanelConstraints.length === 0) {
+            // This indicates that the Group has not finished mounting yet
+            // Likely because it has been rendered inside of a hidden DOM subtree
+            // Ignore layouts in this case because they will not have been validated
+            return;
+          }
+
+          setLayout(layout);
+          onLayoutChangeStable?.(layout);
         }
       }
     );

lib/components/group/useDefaultLayout.test.tsx

@@ -5,6 +5,7 @@ import { Panel } from "../panel/Panel";
 import { Group } from "./Group";
 import type { GroupImperativeHandle, LayoutStorage } from "./types";
 import { useDefaultLayout } from "./useDefaultLayout";
+import { setDefaultElementBounds } from "../../utils/test/mockBoundingClientRect";
 
 describe("useDefaultLayout", () => {
   test("should read/write from the provided Storage API", () => {
@@ -45,6 +46,8 @@ describe("useDefaultLayout", () => {
   test("should not break when coupled with dynamic layouts", () => {
     const groupRef = createRef<GroupImperativeHandle>();
 
+    setDefaultElementBounds(new DOMRect(0, 0, 100, 50));
+
     function Test({ hideMiddlePanel }: { hideMiddlePanel?: boolean }) {
       const { defaultLayout, onLayoutChange } = useDefaultLayout({
         groupId: "test-group-id",

lib/global/mountGroup.ts

@@ -66,12 +66,14 @@ export function mountGroup(group: RegisteredGroup) {
 
               return {
                 mountedGroups: new Map(prevState.mountedGroups).set(group, {
+                  defaultLayoutDeferred: false,
                   derivedPanelConstraints: nextDerivedPanelConstraints,
                   layout: nextLayout,
                   separatorToPanels: match.separatorToPanels
                 })
               };
             }
+
             return prevState;
           });
         }

lib/global/mutableState.ts

@@ -18,7 +18,7 @@ export type SeparatorToPanelsMap = Map<
 export type MountedGroupMap = Map<
   RegisteredGroup,
   {
-    defaultLayoutDeferred?: boolean | undefined;
+    defaultLayoutDeferred: boolean;
     derivedPanelConstraints: PanelConstraints[];
     layout: Layout;
     separatorToPanels: SeparatorToPanelsMap;

lib/global/utils/adjustLayoutForSeparator.ts

@@ -43,6 +43,7 @@ export function adjustLayoutForSeparator(
   if (!layoutsEqual(prevLayout, nextLayout)) {
     update((prevState) => ({
       mountedGroups: new Map(prevState.mountedGroups).set(group, {
+        defaultLayoutDeferred: mountedGroup.defaultLayoutDeferred,
         derivedPanelConstraints: mountedGroup.derivedPanelConstraints,
         layout: nextLayout,
         separatorToPanels: mountedGroup.separatorToPanels

lib/global/utils/getImperativeGroupMethods.test.ts

@@ -67,7 +67,7 @@ describe("getImperativeGroupMethods", () => {
         getImperativeGroupMethods({
           groupId: "A"
         }).getLayout()
-      ).toThrow("Group A not found");
+      ).toThrowError('Could not find Group with id "A"');
     });
 
     test("returns the current group layout", () => {
@@ -93,7 +93,7 @@ describe("getImperativeGroupMethods", () => {
         getImperativeGroupMethods({
           groupId: "A"
         }).setLayout({})
-      ).toThrow("Group A not found");
+      ).toThrowError('Could not find Group with id "A"');
     });
 
     test("ignores a no-op layout update", () => {

lib/global/utils/getImperativeGroupMethods.ts

@@ -19,17 +19,25 @@ export function getImperativeGroupMethods({
       }
     }
 
-    throw Error(`Group ${groupId} not found`);
+    throw Error(`Could not find Group with id "${groupId}"`);
   };
 
   return {
     getLayout() {
-      const { layout } = find();
+      const { defaultLayoutDeferred, layout } = find();
+
+      if (defaultLayoutDeferred) {
+        // This indicates that the Group has not finished mounting yet
+        // Likely because it has been rendered inside of a hidden DOM subtree
+        // Any layout value will not have been validated and so it should not be returned
+        return {};
+      }
 
       return layout;
     },
     setLayout(unsafeLayout: Layout) {
       const {
+        defaultLayoutDeferred,
         derivedPanelConstraints,
         group,
         layout: prevLayout,
@@ -41,9 +49,19 @@ export function getImperativeGroupMethods({
         panelConstraints: derivedPanelConstraints
       });
 
+      if (defaultLayoutDeferred) {
+        // This indicates that the Group has not finished mounting yet
+        // Likely because it has been rendered inside of a hidden DOM subtree
+        // In this case we cannot fully validate the layout, so we shouldn't apply it
+        // It's okay to run the validate function above though,
+        // it will still warn about certain types of errors (e.g. wrong number of panels)
+        return prevLayout;
+      }
+
       if (!layoutsEqual(prevLayout, nextLayout)) {
         update((prevState) => ({
           mountedGroups: new Map(prevState.mountedGroups).set(group, {
+            defaultLayoutDeferred,
             derivedPanelConstraints,
             layout: nextLayout,
             separatorToPanels