From 2a7fcd7d5fb5ba86a08b4ea05be88cb4d2ad5c04 Mon Sep 17 00:00:00 2001 From: Sergej-Vlasov <37613182+Sergej-Vlasov@users.noreply.github.com> Date: Tue, 2 Sep 2025 17:09:55 +0300 Subject: [PATCH] TabsLayoutManager: Tab url sync rethink (#110274) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 --- .../scene/layout-tabs/TabItem.tsx | 9 +- .../scene/layout-tabs/TabItemRenderer.tsx | 50 +++++++-- .../scene/layout-tabs/TabItemRepeater.tsx | 1 + .../layout-tabs/TabsLayoutManager.test.tsx | 37 +++---- .../scene/layout-tabs/TabsLayoutManager.tsx | 103 +++++++++--------- .../layout-tabs/TabsLayoutManagerRenderer.tsx | 41 ++----- .../scene/layouts-shared/addNew.ts | 3 + 7 files changed, 130 insertions(+), 114 deletions(-) diff --git a/public/app/features/dashboard-scene/scene/layout-tabs/TabItem.tsx b/public/app/features/dashboard-scene/scene/layout-tabs/TabItem.tsx index 8e606fa4eee..23d17df71ef 100644 --- a/public/app/features/dashboard-scene/scene/layout-tabs/TabItem.tsx +++ b/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() }); } } diff --git a/public/app/features/dashboard-scene/scene/layout-tabs/TabItemRenderer.tsx b/public/app/features/dashboard-scene/scene/layout-tabs/TabItemRenderer.tsx index 3e7f7778d4c..3390e4c0133 100644 --- a/public/app/features/dashboard-scene/scene/layout-tabs/TabItemRenderer.tsx +++ b/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) { - 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) { return null; } + if (soloPanelContext) { + return ; + } + 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 ( + + + {isEditing && conditionalRenderingOverlay} + + ); +} + +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), + }), }); diff --git a/public/app/features/dashboard-scene/scene/layout-tabs/TabItemRepeater.tsx b/public/app/features/dashboard-scene/scene/layout-tabs/TabItemRepeater.tsx index ae545b9488e..78eac159ea7 100644 --- a/public/app/features/dashboard-scene/scene/layout-tabs/TabItemRepeater.tsx +++ b/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(); } /** diff --git a/public/app/features/dashboard-scene/scene/layout-tabs/TabsLayoutManager.test.tsx b/public/app/features/dashboard-scene/scene/layout-tabs/TabsLayoutManager.test.tsx index 280c312e1c3..53556c7a116 100644 --- a/public/app/features/dashboard-scene/scene/layout-tabs/TabsLayoutManager.test.tsx +++ b/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()); }); }); }); diff --git a/public/app/features/dashboard-scene/scene/layout-tabs/TabsLayoutManager.tsx b/public/app/features/dashboard-scene/scene/layout-tabs/TabsLayoutManager.tsx index ec14a3c2d3a..7c1a93720ff 100644 --- a/public/app/features/dashboard-scene/scene/layout-tabs/TabsLayoutManager.tsx +++ b/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 implements DashboardLayoutManager { @@ -58,7 +58,6 @@ export class TabsLayoutManager extends SceneObjectBase i super({ ...state, tabs: state.tabs ?? [new TabItem()], - currentTabIndex: state.currentTabIndex ?? 0, }); } @@ -74,7 +73,7 @@ export class TabsLayoutManager extends SceneObjectBase 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 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((acc, tab) => { + return this.state.tabs.reduce((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 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 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 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 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 { diff --git a/public/app/features/dashboard-scene/scene/layout-tabs/TabsLayoutManagerRenderer.tsx b/public/app/features/dashboard-scene/scene/layout-tabs/TabsLayoutManagerRenderer.tsx index f3a57fe3363..dbab143d146 100644 --- a/public/app/features/dashboard-scene/scene/layout-tabs/TabsLayoutManagerRenderer.tsx +++ b/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 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 ; + return tabs.map((tab) => ); } const isClone = isRepeatCloneOrChildOf(model); @@ -94,18 +98,7 @@ export function TabsLayoutManagerRenderer({ model }: SceneComponentProps - {isEditing && ( - - {currentTab && } - {conditionalRenderingOverlay} - - )} - - {!isEditing && ( - - {currentTab && } - - )} + {currentTab && } ); } @@ -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), }), diff --git a/public/app/features/dashboard-scene/scene/layouts-shared/addNew.ts b/public/app/features/dashboard-scene/scene/layouts-shared/addNew.ts index 5ada92b7fa6..0fff57c3b08 100644 --- a/public/app/features/dashboard-scene/scene/layouts-shared/addNew.ts +++ b/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); }