TabsLayoutManager: Tab url sync rethink (#110274)

* Url sync for tabs rehink

* Update

* Thinks are working pretty well

* wip: solve tab not found

* fix lint and tests

* adjust wait for repeats in getCurrentTab

* set currentTabSlug in useEffect

* set currentTabSlug on tab title change

---------

Co-authored-by: Torkel Ödegaard <torkel@grafana.com>

public/app/features/dashboard-scene/scene/layout-tabs/TabItem.tsx

@@ -175,6 +175,8 @@ export class TabItem
 
   public onChangeTitle(title: string) {
     this.setState({ title });
+    const currentTabSlug = this.getSlug();
+    this.getParentLayout().setState({ currentTabSlug });
   }
 
   public onChangeName(name: string): void {
@@ -202,13 +204,14 @@ export class TabItem
 
   public draggedPanelInside(panel: VizPanel) {
     panel.clearParent();
+
     this.getLayout().addPanel(panel);
     this.setIsDropTarget(false);
 
     const parentLayout = this.getParentLayout();
-    const tabIndex = parentLayout.state.tabs.findIndex((tab) => tab === this);
-    if (tabIndex !== parentLayout.state.currentTabIndex) {
-      parentLayout.setState({ currentTabIndex: tabIndex });
+
+    if (parentLayout.state.currentTabSlug !== this.getSlug()) {
+      parentLayout.setState({ currentTabSlug: this.getSlug() });
     }
   }
 

public/app/features/dashboard-scene/scene/layout-tabs/TabItemRenderer.tsx

@@ -2,34 +2,36 @@ import { css, cx } from '@emotion/css';
 import { Draggable } from '@hello-pangea/dnd';
 import { useLocation } from 'react-router';
 
-import { locationUtil, textUtil } from '@grafana/data';
+import { GrafanaTheme2, locationUtil, textUtil } from '@grafana/data';
 import { t } from '@grafana/i18n';
 import { SceneComponentProps, sceneGraph } from '@grafana/scenes';
-import { Box, Icon, Tab, Tooltip, useElementSelection, usePointerDistance, useStyles2 } from '@grafana/ui';
+import { Box, Icon, Tab, TabContent, Tooltip, useElementSelection, usePointerDistance, useStyles2 } from '@grafana/ui';
 
 import { useIsConditionallyHidden } from '../../conditional-rendering/useIsConditionallyHidden';
 import { isRepeatCloneOrChildOf } from '../../utils/clone';
 import { useDashboardState } from '../../utils/utils';
+import { useSoloPanelContext } from '../SoloPanelContext';
 
 import { TabItem } from './TabItem';
 
 export function TabItemRenderer({ model }: SceneComponentProps<TabItem>) {
-  const { title, key, isDropTarget } = model.useState();
+  const { title, key, isDropTarget, layout } = model.useState();
   const parentLayout = model.getParentLayout();
-  const { currentTabIndex } = parentLayout.useState();
+  const { currentTabSlug } = parentLayout.useState();
   const titleInterpolated = sceneGraph.interpolate(model, title, undefined, 'text');
   const { isSelected, onSelect, isSelectable } = useElementSelection(key);
   const { isEditing } = useDashboardState(model);
   const mySlug = model.getSlug();
   const urlKey = parentLayout.getUrlKey();
-  const myIndex = parentLayout.getTabs().findIndex((tab) => tab === model);
-  const isActive = myIndex === currentTabIndex;
+  const isActive = mySlug === currentTabSlug;
+  const myIndex = parentLayout.state.tabs.findIndex((tab) => tab === model);
   const location = useLocation();
   const href = textUtil.sanitize(locationUtil.getUrlForPartial(location, { [urlKey]: mySlug }));
   const styles = useStyles2(getStyles);
   const pointerDistance = usePointerDistance();
   const [isConditionallyHidden] = useIsConditionallyHidden(model);
   const isClone = isRepeatCloneOrChildOf(model);
+  const soloPanelContext = useSoloPanelContext();
 
   const isDraggable = !isClone && isEditing;
 
@@ -37,6 +39,10 @@ export function TabItemRenderer({ model }: SceneComponentProps<TabItem>) {
     return null;
   }
 
+  if (soloPanelContext) {
+    return <layout.Component model={layout} />;
+  }
+
   let titleCollisionProps = {};
 
   if (!model.hasUniqueTitle()) {
@@ -107,7 +113,25 @@ function IsHiddenSuffix() {
   );
 }
 
-const getStyles = () => ({
+interface TabItemLayoutRendererProps {
+  tab: TabItem;
+  isEditing?: boolean;
+}
+
+export function TabItemLayoutRenderer({ tab, isEditing }: TabItemLayoutRendererProps) {
+  const { layout } = tab.useState();
+  const styles = useStyles2(getStyles);
+  const [_, conditionalRenderingClass, conditionalRenderingOverlay] = useIsConditionallyHidden(tab);
+
+  return (
+    <TabContent className={cx(styles.tabContentContainer, isEditing && conditionalRenderingClass)}>
+      <layout.Component model={layout} />
+      {isEditing && conditionalRenderingOverlay}
+    </TabContent>
+  );
+}
+
+const getStyles = (theme: GrafanaTheme2) => ({
   dragging: css({
     cursor: 'move',
   }),
@@ -118,4 +142,16 @@ const getStyles = () => ({
       opacity: 1,
     }),
   }),
+  tabContentContainer: css({
+    backgroundColor: 'transparent',
+    position: 'relative',
+    display: 'flex',
+    flexDirection: 'column',
+    flex: 1,
+    // Without this min height, the custom grid (SceneGridLayout) wont render
+    // Should be bigger than paddingTop value
+    // consist of paddingTop + 0.125 = 9px
+    minHeight: theme.spacing(1 + 0.125),
+    paddingTop: theme.spacing(1),
+  }),
 });

public/app/features/dashboard-scene/scene/layout-tabs/TabItemRepeater.tsx

@@ -98,6 +98,7 @@ export function performTabRepeats(variable: MultiValueVariable, tab: TabItem, co
   const clonedTabs = createTabRepeats({ values, texts, variable, tab });
 
   tab.setState({ repeatedTabs: clonedTabs });
+  tab.parent?.forceRender();
 }
 
 /**

public/app/features/dashboard-scene/scene/layout-tabs/TabsLayoutManager.test.tsx

@@ -32,6 +32,9 @@ describe('TabsLayoutManager', () => {
         tabs: [new TabItem({ title: 'Performance' })],
       });
 
+      // currentTabSlug is set during rendering so forcing here
+      tabsLayoutManager.setState({ currentTabSlug: tabsLayoutManager.getCurrentTab()?.getSlug() });
+
       const urlState = tabsLayoutManager.getUrlState();
       expect(urlState).toEqual({ dtab: 'performance' });
     });
@@ -41,6 +44,9 @@ describe('TabsLayoutManager', () => {
         tabs: [new TabItem({ title: 'Performance' })],
       });
 
+      // currentTabSlug is set during rendering so forcing here
+      innerMostTabs.setState({ currentTabSlug: innerMostTabs.getCurrentTab()?.getSlug() });
+
       new RowsLayoutManager({
         rows: [
           new RowItem({
@@ -129,35 +135,19 @@ describe('TabsLayoutManager', () => {
       lastUndo = undefined;
     });
 
-    it('should remove a non-current tab without using removeElement', () => {
-      const manager = new TabsLayoutManager({ tabs: [] });
-      const tab1 = manager.addNewTab(new TabItem({ title: 'Tab 1' }));
-      const tab2 = manager.addNewTab(new TabItem({ title: 'Tab 2' }));
-
-      expect(manager.state.tabs).toHaveLength(2);
-      expect(manager.state.currentTabIndex).toBe(1); // tab2 is current
-
-      manager.removeTab(tab1);
-
-      expect(manager.state.tabs).toHaveLength(1);
-      expect(manager.state.tabs[0]).toBe(tab2);
-      expect(manager.state.currentTabIndex).toBe(0);
-      expect(dashboardEditActions.removeElement).not.toHaveBeenCalled();
-    });
-
     it('should remove the current tab using removeElement', () => {
       const manager = new TabsLayoutManager({ tabs: [] });
       const tab1 = manager.addNewTab(new TabItem({ title: 'Tab 1' }));
       const tab2 = manager.addNewTab(new TabItem({ title: 'Tab 2' }));
 
       expect(manager.state.tabs).toHaveLength(2);
-      expect(manager.state.currentTabIndex).toBe(1); // tab2 is current
+      expect(manager.state.currentTabSlug).toBe(tab2.getSlug()); // tab2 is current
 
       manager.removeTab(tab2);
 
       expect(manager.state.tabs).toHaveLength(1);
       expect(manager.state.tabs[0]).toBe(tab1);
-      expect(manager.state.currentTabIndex).toBe(0);
+      expect(manager.state.currentTabSlug).toBe(tab1.getSlug());
       expect(dashboardEditActions.removeElement).toHaveBeenCalled();
     });
 
@@ -167,7 +157,7 @@ describe('TabsLayoutManager', () => {
       const tab2 = manager.addNewTab(new TabItem({ title: 'Tab 2' }));
 
       expect(manager.state.tabs).toHaveLength(2);
-      expect(manager.state.currentTabIndex).toBe(1); // tab2 is current
+      expect(manager.state.currentTabSlug).toBe(tab2.getSlug()); // tab2 is current
 
       manager.removeTab(tab2);
 
@@ -181,7 +171,7 @@ describe('TabsLayoutManager', () => {
       expect(manager.state.tabs).toHaveLength(2);
       expect(manager.state.tabs).toContain(tab1);
       expect(manager.state.tabs).toContain(tab2);
-      expect(manager.state.currentTabIndex).toBe(1); // tab2 should be current again
+      expect(manager.state.currentTabSlug).toBe(tab2.getSlug()); // tab2 should be current again
     });
   });
 
@@ -231,19 +221,18 @@ describe('TabsLayoutManager', () => {
       const tab3 = manager.addNewTab(new TabItem({ title: 'Tab 3' }));
 
       // Set tab2 as current
-      manager.setState({ currentTabIndex: 1 });
-      expect(manager.state.currentTabIndex).toBe(1);
+      manager.setState({ currentTabSlug: tab2.getSlug() });
 
       manager.moveTab(1, 0);
 
       expect(manager.state.tabs).toEqual([tab2, tab1, tab3]);
-      expect(manager.state.currentTabIndex).toBe(0);
+      expect(manager.state.currentTabSlug).toBe(tab2.getSlug());
 
       // Undo should restore the original state
       lastUndo && lastUndo();
 
       expect(manager.state.tabs).toEqual([tab1, tab2, tab3]);
-      expect(manager.state.currentTabIndex).toBe(1);
+      expect(manager.state.currentTabSlug).toBe(tab2.getSlug());
     });
   });
 });

public/app/features/dashboard-scene/scene/layout-tabs/TabsLayoutManager.tsx

@@ -25,7 +25,7 @@ import { TabsLayoutManagerRenderer } from './TabsLayoutManagerRenderer';
 
 interface TabsLayoutManagerState extends SceneObjectState {
   tabs: TabItem[];
-  currentTabIndex: number;
+  currentTabSlug?: string;
 }
 
 export class TabsLayoutManager extends SceneObjectBase<TabsLayoutManagerState> implements DashboardLayoutManager {
@@ -58,7 +58,6 @@ export class TabsLayoutManager extends SceneObjectBase<TabsLayoutManagerState> i
     super({
       ...state,
       tabs: state.tabs ?? [new TabItem()],
-      currentTabIndex: state.currentTabIndex ?? 0,
     });
   }
 
@@ -74,7 +73,7 @@ export class TabsLayoutManager extends SceneObjectBase<TabsLayoutManagerState> i
 
   public getUrlState() {
     const key = this.getUrlKey();
-    return { [key]: this.getCurrentTab().getSlug() };
+    return { [key]: this.state.currentTabSlug };
   }
 
   public updateFromUrl(values: SceneObjectUrlValues) {
@@ -86,35 +85,49 @@ export class TabsLayoutManager extends SceneObjectBase<TabsLayoutManagerState> i
     }
 
     if (typeof values[key] === 'string') {
-      // find tab with matching slug
-      const matchIndex = this.getTabs().findIndex((tab) => tab.getSlug() === urlValue);
-      if (matchIndex !== -1) {
-        this.setState({ currentTabIndex: matchIndex });
-      }
+      this.setState({ currentTabSlug: values[key] });
     }
   }
 
   public switchToTab(tab: TabItem) {
-    this.setState({ currentTabIndex: this.getTabs().indexOf(tab) });
+    this.setState({ currentTabSlug: tab.getSlug() });
   }
 
-  public getCurrentTab(): TabItem {
-    return this.getTabs().length > this.state.currentTabIndex
-      ? this.getTabs()[this.state.currentTabIndex]
-      : this.getTabs()[0];
+  public getCurrentTab(): TabItem | undefined {
+    const tabs = this.getTabs();
+    const selectedTab = tabs.find((tab) => tab.getSlug() === this.state.currentTabSlug);
+    if (selectedTab) {
+      return selectedTab;
+    }
+
+    // return undefined either if variable is loading or repeats were not processed yet
+    for (const tab of tabs) {
+      if (tab.state.repeatByVariable) {
+        const variable = sceneGraph.lookupVariable(tab.state.repeatByVariable, this);
+        if ((variable && variable.state.loading) || !tab.state.repeatedTabs) {
+          return;
+        }
+      }
+    }
+
+    // return first tab if no hits and variables finished loading
+    return tabs[0];
   }
 
   public getTabs(): TabItem[] {
-    const tabsWithRepeats = this.state.tabs.reduce<TabItem[]>((acc, tab) => {
+    return this.state.tabs.reduce<TabItem[]>((acc, tab) => {
       acc.push(tab, ...(tab.state.repeatedTabs ?? []));
 
       return acc;
     }, []);
-    return tabsWithRepeats;
   }
 
   public addPanel(vizPanel: VizPanel) {
-    this.getCurrentTab().getLayout().addPanel(vizPanel);
+    const tab = this.getCurrentTab();
+
+    if (tab) {
+      tab.getLayout().addPanel(vizPanel);
+    }
   }
 
   public getVizPanels(): VizPanel[] {
@@ -163,16 +176,12 @@ export class TabsLayoutManager extends SceneObjectBase<TabsLayoutManagerState> i
     dashboardEditActions.addElement({
       addedObject: newTab,
       source: this,
-      perform: () => this.setState({ tabs: [...this.state.tabs, newTab], currentTabIndex: this.getTabs().length }),
+      perform: () => this.setState({ tabs: [...this.state.tabs, newTab], currentTabSlug: newTab.getSlug() }),
       undo: () => {
-        const indexOfNewTab = this.getTabs().findIndex((t) => t === newTab);
         this.setState({
           tabs: this.state.tabs.filter((t) => t !== newTab),
           // if the new tab was the current tab, set the current tab to the previous tab
-          currentTabIndex:
-            this.state.currentTabIndex === indexOfNewTab
-              ? Math.max(0, this.state.currentTabIndex - 1)
-              : this.state.currentTabIndex,
+          currentTabSlug: this.state.currentTabSlug === newTab.getSlug() ? undefined : this.state.currentTabSlug,
         });
       },
     });
@@ -201,35 +210,29 @@ export class TabsLayoutManager extends SceneObjectBase<TabsLayoutManagerState> i
       return;
     }
 
-    const currentTab = this.getCurrentTab();
+    const tabIndex = this.state.tabs.findIndex((t) => t === tabToRemove);
 
-    if (currentTab === tabToRemove) {
-      const currentTabIndex = this.state.currentTabIndex;
-      const indexOfTabToRemove = this.state.tabs.findIndex((t) => t === tabToRemove);
-      const nextTabIndex = currentTabIndex > 0 ? currentTabIndex - 1 : 0;
+    dashboardEditActions.removeElement({
+      removedObject: tabToRemove,
+      source: this,
+      perform: () => {
+        const tabs = this.state.tabs;
+        const tabIndex = tabs.findIndex((t) => t === tabToRemove);
+        const newCurrentTabIndex = tabIndex > 0 ? tabIndex - 1 : 0;
 
-      dashboardEditActions.removeElement({
-        removedObject: tabToRemove,
-        source: this,
-        perform: () =>
-          this.setState({
-            tabs: this.state.tabs.filter((t) => t !== tabToRemove),
-            currentTabIndex: currentTabIndex === indexOfTabToRemove ? nextTabIndex : currentTabIndex,
-          }),
-        undo: () => {
-          const tabs = [...this.state.tabs];
-          tabs.splice(indexOfTabToRemove, 0, tabToRemove);
-          this.setState({ tabs, currentTabIndex });
-        },
-      });
+        const newTabsState = tabs.filter((t) => t !== tabToRemove);
 
-      return;
-    }
-
-    const filteredTab = this.state.tabs.filter((tab) => tab !== tabToRemove);
-    const tabs = filteredTab.length === 0 ? [new TabItem()] : filteredTab;
-
-    this.setState({ tabs, currentTabIndex: 0 });
+        this.setState({
+          tabs: newTabsState,
+          currentTabSlug: newTabsState[newCurrentTabIndex]?.getSlug(),
+        });
+      },
+      undo: () => {
+        const tabs = [...this.state.tabs];
+        tabs.splice(tabIndex, 0, tabToRemove);
+        this.setState({ tabs, currentTabSlug: tabToRemove.getSlug() });
+      },
+    });
   }
 
   public moveTab(fromIndex: number, toIndex: number) {
@@ -274,7 +277,7 @@ export class TabsLayoutManager extends SceneObjectBase<TabsLayoutManagerState> i
     const tabs = [...this.state.tabs];
     const [removed] = tabs.splice(fromIndex, 1);
     tabs.splice(toIndex, 0, removed);
-    this.setState({ tabs, currentTabIndex: selectedTabIndex });
+    this.setState({ tabs });
     this.publishEvent(new ObjectsReorderedOnCanvasEvent(this), true);
   }
 
@@ -288,7 +291,7 @@ export class TabsLayoutManager extends SceneObjectBase<TabsLayoutManagerState> i
 
     const editPane = getDashboardSceneFor(this).state.editPane;
     editPane.selectObject(tab!, tabKey, { force: true, multi: false });
-    this.setState({ currentTabIndex: tabIndex });
+    this.setState({ currentTabSlug: tab.getSlug() });
   }
 
   public static createEmpty(): TabsLayoutManager {

public/app/features/dashboard-scene/scene/layout-tabs/TabsLayoutManagerRenderer.tsx

@@ -1,14 +1,13 @@
 import { css, cx } from '@emotion/css';
 import { DragDropContext, Droppable } from '@hello-pangea/dnd';
-import { useMemo } from 'react';
+import { useEffect, useMemo } from 'react';
 
 import { GrafanaTheme2 } from '@grafana/data';
 import { selectors } from '@grafana/e2e-selectors';
 import { Trans } from '@grafana/i18n';
 import { MultiValueVariable, SceneComponentProps, sceneGraph, useSceneObjectState } from '@grafana/scenes';
-import { Button, TabContent, TabsBar, useStyles2 } from '@grafana/ui';
+import { Button, TabsBar, useStyles2 } from '@grafana/ui';
 
-import { useIsConditionallyHidden } from '../../conditional-rendering/useIsConditionallyHidden';
 import { isRepeatCloneOrChildOf } from '../../utils/clone';
 import { getDashboardSceneFor } from '../../utils/utils';
 import { useSoloPanelContext } from '../SoloPanelContext';
@@ -16,6 +15,7 @@ import { dashboardCanvasAddButtonHoverStyles } from '../layouts-shared/styles';
 import { useClipboardState } from '../layouts-shared/useClipboardState';
 
 import { TabItem } from './TabItem';
+import { TabItemLayoutRenderer } from './TabItemRenderer';
 import { TabItemRepeater } from './TabItemRepeater';
 import { TabsLayoutManager } from './TabsLayoutManager';
 
@@ -23,16 +23,20 @@ export function TabsLayoutManagerRenderer({ model }: SceneComponentProps<TabsLay
   const styles = useStyles2(getStyles);
   const { tabs, key } = model.useState();
   const currentTab = model.getCurrentTab();
-  const { layout } = currentTab.useState();
   const dashboard = getDashboardSceneFor(model);
   const { isEditing } = dashboard.useState();
   const { hasCopiedTab } = useClipboardState();
-  const [_, conditionalRenderingClass, conditionalRenderingOverlay] = useIsConditionallyHidden(currentTab);
   const isNestedInTab = useMemo(() => model.parent instanceof TabItem, [model.parent]);
   const soloPanelContext = useSoloPanelContext();
 
+  useEffect(() => {
+    if (currentTab && currentTab.getSlug() !== model.state.currentTabSlug) {
+      model.setState({ currentTabSlug: currentTab.getSlug() });
+    }
+  }, [currentTab, model]);
+
   if (soloPanelContext) {
-    return <layout.Component model={layout} />;
+    return tabs.map((tab) => <TabWrapper tab={tab} manager={model} key={tab.state.key!} />);
   }
 
   const isClone = isRepeatCloneOrChildOf(model);
@@ -94,18 +98,7 @@ export function TabsLayoutManagerRenderer({ model }: SceneComponentProps<TabsLay
         </DragDropContext>
       </TabsBar>
 
-      {isEditing && (
-        <TabContent className={cx(styles.tabContentContainer, conditionalRenderingClass)}>
-          {currentTab && <layout.Component model={layout} />}
-          {conditionalRenderingOverlay}
-        </TabContent>
-      )}
-
-      {!isEditing && (
-        <TabContent className={styles.tabContentContainer}>
-          {currentTab && <layout.Component model={layout} />}
-        </TabContent>
-      )}
+      {currentTab && <TabItemLayoutRenderer tab={currentTab} isEditing={isEditing} />}
     </div>
   );
 }
@@ -144,18 +137,6 @@ const getStyles = (theme: GrafanaTheme2) => ({
     paddingInline: theme.spacing(0.125),
     paddingTop: '1px',
   }),
-  tabContentContainer: css({
-    backgroundColor: 'transparent',
-    position: 'relative',
-    display: 'flex',
-    flexDirection: 'column',
-    flex: 1,
-    // Without this min height, the custom grid (SceneGridLayout) wont render
-    // Should be bigger than paddingTop value
-    // consist of paddingTop + 0.125 = 9px
-    minHeight: theme.spacing(1 + 0.125),
-    paddingTop: theme.spacing(1),
-  }),
   nestedTabsMargin: css({
     marginLeft: theme.spacing(2),
   }),

public/app/features/dashboard-scene/scene/layouts-shared/addNew.ts

@@ -50,6 +50,9 @@ export function addNewRowTo(layout: DashboardLayoutManager): RowItem | SceneGrid
 
   if (layout instanceof TabsLayoutManager) {
     const currentTab = layout.getCurrentTab();
+    if (!currentTab) {
+      throw new Error('Could find currently active tab');
+    }
     return addNewRowTo(currentTab.state.layout);
   }