Avoid remount of RouterTab with forceRender={true} when `RouterTa…

…bs` are used inside a `Stack` (#1439)

Remounting causes local state to be lost, including final-form values.

Problem was the missing `StackBreadcrumb` wrapper, as react remounts in
that case
(https://legacy.reactjs.org/docs/reconciliation.html#elements-of-different-types)

---------

Co-authored-by: Johannes Obermair <48853629+johnnyomair@users.noreply.github.com>

.changeset/breezy-scissors-warn.md

@@ -0,0 +1,5 @@
+---
+"@comet/admin": patch
+---
+
+Avoid remount of `RouterTab` with `forceRender={true}` when `RouterTabs` are used inside a `Stack`

packages/admin/admin-stories/src/admin/tabs/RouterTabsInStack.tsx

@@ -0,0 +1,59 @@
+import { RouterTab, RouterTabs, Stack, StackPage, StackSwitch } from "@comet/admin";
+import { storiesOf } from "@storybook/react";
+import * as React from "react";
+
+import { storyRouterDecorator } from "../../story-router.decorator";
+
+const mountCount: Record<string, number> = {};
+
+function RenderCount(props: { name: string }) {
+    React.useEffect(() => {
+        mountCount[props.name] = (mountCount[props.name] || 0) + 1;
+    }, [props.name]);
+    const renderCount = React.useRef(0);
+    renderCount.current++;
+    return (
+        <div>
+            Render count {props.name}: {renderCount.current}
+        </div>
+    );
+}
+
+function PrintMountCount() {
+    const [, rerender] = React.useState(0);
+    React.useEffect(() => {
+        const timer = setInterval(() => {
+            rerender(new Date().getTime());
+        }, 100);
+        return () => clearInterval(timer);
+    }, []);
+    return <div>Mount count: {JSON.stringify(mountCount)}</div>;
+}
+
+function Story() {
+    return (
+        <Stack topLevelTitle="Nested Stack">
+            <PrintMountCount />
+            <StackSwitch>
+                <StackPage name="xxx">
+                    <RenderCount name="StackPage" />
+                    <RouterTabs>
+                        <RouterTab label="Foo" path="" forceRender={true}>
+                            Foo
+                            <RenderCount name="Foo" />
+                        </RouterTab>
+                        <RouterTab label="Bar" path="/bar" forceRender={true}>
+                            Bar
+                            <RenderCount name="Bar" />
+                        </RouterTab>
+                    </RouterTabs>
+                </StackPage>
+                <StackPage name="yyy">yyy</StackPage>
+            </StackSwitch>
+        </Stack>
+    );
+}
+
+storiesOf("@comet/admin/tabs", module)
+    .addDecorator(storyRouterDecorator())
+    .add("RouterTabs in Stack", () => <Story />);

packages/admin/admin/src/tabs/RouterTabs.test.tsx

@@ -7,6 +7,9 @@ import { Router, useRouteMatch } from "react-router";
 
 import { MuiThemeProvider } from "../mui/ThemeProvider";
 import { SubRoute, useSubRoutePrefix } from "../router/SubRoute";
+import { StackPage } from "../stack/Page";
+import { Stack } from "../stack/Stack";
+import { StackSwitch } from "../stack/Switch";
 import { RouterTab, RouterTabs } from "./RouterTabs";
 
 test("RouterTabs in SubRoute", async () => {
@@ -47,3 +50,66 @@ test("RouterTabs in SubRoute", async () => {
     expect(rendered.getByText("bar tab content")).toBeInTheDocument();
     expect(rendered.getByText("matchUrl=/sub/bar")).toBeInTheDocument();
 });
+
+test("RouterTabs must not remount content", async () => {
+    let mountCountFoo = 0;
+    function MountCountFoo() {
+        React.useEffect(() => {
+            mountCountFoo++;
+        });
+        return null;
+    }
+
+    let mountCountBar = 0;
+    function MountCountBar() {
+        React.useEffect(() => {
+            mountCountBar++;
+        });
+        return null;
+    }
+
+    function Story() {
+        return (
+            <Stack topLevelTitle="Nested Stack">
+                <StackSwitch>
+                    <StackPage name="xxx">
+                        <RouterTabs>
+                            <RouterTab label="FooTab" path="" forceRender={true}>
+                                FooContent
+                                <MountCountFoo />
+                            </RouterTab>
+                            <RouterTab label="BarTab" path="/bar" forceRender={true}>
+                                BarContent
+                                <MountCountBar />
+                            </RouterTab>
+                        </RouterTabs>
+                    </StackPage>
+                    <StackPage name="yyy">yyy</StackPage>
+                </StackSwitch>
+            </Stack>
+        );
+    }
+
+    const history = createMemoryHistory();
+
+    const rendered = render(
+        <MuiThemeProvider theme={createTheme()}>
+            <Router history={history}>
+                <Story />
+            </Router>
+        </MuiThemeProvider>,
+    );
+    expect(rendered.getByText("FooContent")).toBeInTheDocument();
+    expect(mountCountFoo).toBe(1);
+    expect(mountCountBar).toBe(1);
+
+    fireEvent.click(rendered.getByText("BarTab"));
+    expect(rendered.getByText("BarContent")).toBeInTheDocument();
+    expect(mountCountFoo).toBe(1);
+    expect(mountCountBar).toBe(1);
+
+    fireEvent.click(rendered.getByText("FooTab"));
+    expect(rendered.getByText("FooContent")).toBeInTheDocument();
+    expect(mountCountFoo).toBe(1);
+    expect(mountCountBar).toBe(1);
+});

packages/admin/admin/src/tabs/RouterTabs.tsx

@@ -124,20 +124,24 @@ function RouterTabsComponent({
                 return (
                     <Route path={path}>
                         {({ match }) => {
-                            if (match && stackApi && stackSwitchApi && !foundFirstMatch) {
+                            let ret = null;
+                            if (match && !foundFirstMatch) {
                                 foundFirstMatch = true;
+                                ret = <div className={classes.content}>{child.props.children}</div>;
+                            } else if (child.props.forceRender) {
+                                ret = <div className={`${classes.content} ${classes.contentHidden}`}>{child.props.children}</div>;
+                            } else {
+                                // don't render tab contents, return early as we don't need StackBreadcrumb either
+                                return null;
+                            }
+                            if (stackApi && stackSwitchApi) {
                                 return (
                                     <StackBreadcrumb url={path} title={child.props.label} invisible={true}>
-                                        <div className={classes.content}>{child.props.children}</div>
+                                        {ret}
                                     </StackBreadcrumb>
                                 );
-                            } else if (match && !foundFirstMatch) {
-                                foundFirstMatch = true;
-                                return <div className={classes.content}>{child.props.children}</div>;
-                            } else if (child.props.forceRender) {
-                                return <div className={`${classes.content} ${classes.contentHidden}`}>{child.props.children}</div>;
                             } else {
-                                return null;
+                                return ret;
                             }
                         }}
                     </Route>