fix: useTruncation infinite loop, reenable dashboard cross links on ChartList (#27701)

This commit is contained in:
Kamil Gabryjelski 2024-04-09 12:30:57 +02:00 committed by GitHub
parent 4ecfce98f6
commit ae0f2ce3c1
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
13 changed files with 282 additions and 179 deletions

View File

@ -54,7 +54,7 @@ function visitChartList() {
}
describe('Charts list', () => {
describe.skip('Cross-referenced dashboards', () => {
describe('Cross-referenced dashboards', () => {
beforeEach(() => {
cy.createSampleDashboards([0, 1, 2, 3]);
cy.createSampleCharts([0]);
@ -112,9 +112,10 @@ describe('Charts list', () => {
cy.getBySel('sort-header').eq(1).contains('Name');
cy.getBySel('sort-header').eq(2).contains('Type');
cy.getBySel('sort-header').eq(3).contains('Dataset');
cy.getBySel('sort-header').eq(4).contains('Owners');
cy.getBySel('sort-header').eq(5).contains('Last modified');
cy.getBySel('sort-header').eq(6).contains('Actions');
cy.getBySel('sort-header').eq(4).contains('On dashboards');
cy.getBySel('sort-header').eq(5).contains('Owners');
cy.getBySel('sort-header').eq(6).contains('Last modified');
cy.getBySel('sort-header').eq(7).contains('Actions');
});
it('should sort correctly in list mode', () => {

View File

@ -31,13 +31,13 @@ const SAMPLE_DASHBOARDS_INDEXES = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10];
function openDashboardsAddedTo() {
cy.getBySel('actions-trigger').click();
cy.get('.ant-dropdown-menu-submenu-title')
.contains('Dashboards added to')
.contains('On dashboards')
.trigger('mouseover', { force: true });
}
function closeDashboardsAddedTo() {
cy.get('.ant-dropdown-menu-submenu-title')
.contains('Dashboards added to')
.contains('On dashboards')
.trigger('mouseout', { force: true });
cy.getBySel('actions-trigger').click();
}

View File

@ -20,6 +20,10 @@ import { renderHook } from '@testing-library/react-hooks';
import { RefObject } from 'react';
import useChildElementTruncation from './useChildElementTruncation';
let observeMock: jest.Mock;
let disconnectMock: jest.Mock;
let originalResizeObserver: typeof ResizeObserver;
const genElements = (
scrollWidth: number,
clientWidth: number,
@ -34,26 +38,87 @@ const genElements = (
};
return [elementRef, plusRef];
};
const useTruncation = (elementRef: any, plusRef: any) =>
useChildElementTruncation(
elementRef as RefObject<HTMLElement>,
plusRef as RefObject<HTMLElement>,
const testTruncationHookWithInitialValues = (
[scrollWidth, clientWidth, offsetWidth, childNodes = []]: [
number,
number,
number | undefined,
any?,
],
expectedElementsTruncated: number,
shouldHaveHiddenElements: boolean,
) => {
const [elementRef, plusRef] = genElements(
scrollWidth,
clientWidth,
offsetWidth,
childNodes,
);
const { result, rerender } = renderHook(() => useChildElementTruncation());
Object.defineProperty(result.current[0], 'current', {
value: elementRef.current,
});
Object.defineProperty(result.current[1], 'current', {
value: plusRef.current,
});
rerender();
expect(result.current).toEqual([
elementRef,
plusRef,
expectedElementsTruncated,
shouldHaveHiddenElements,
]);
};
beforeAll(() => {
// Store the original ResizeObserver
originalResizeObserver = window.ResizeObserver;
// Mock ResizeObserver
observeMock = jest.fn();
disconnectMock = jest.fn();
window.ResizeObserver = jest.fn(() => ({
observe: observeMock,
disconnect: disconnectMock,
})) as unknown as typeof ResizeObserver;
});
afterAll(() => {
// Restore original ResizeObserver after all tests are done
window.ResizeObserver = originalResizeObserver;
});
afterEach(() => {
observeMock.mockClear();
disconnectMock.mockClear();
});
test('should return [0, false] when elementRef.current is not defined', () => {
const { result } = renderHook(() =>
useTruncation({ current: undefined }, { current: undefined }),
);
const { result } = renderHook(() => useChildElementTruncation());
expect(result.current).toEqual([
{ current: null },
{ current: null },
0,
false,
]);
expect(result.current).toEqual([0, false]);
expect(observeMock).not.toHaveBeenCalled();
});
test('should not recompute when previousEffectInfo is the same as previous', () => {
const elementRef = { current: document.createElement('div') };
const plusRef = { current: document.createElement('div') };
const { result, rerender } = renderHook(() =>
useTruncation(elementRef, plusRef),
);
const { result, rerender } = renderHook(() => useChildElementTruncation());
Object.defineProperty(result.current[0], 'current', {
value: document.createElement('div'),
});
Object.defineProperty(result.current[1], 'current', {
value: document.createElement('div'),
});
const previousEffectInfo = result.current;
rerender();
@ -62,41 +127,96 @@ test('should not recompute when previousEffectInfo is the same as previous', ()
});
test('should return [0, false] when there are no truncated/hidden elements', () => {
const [elementRef, plusRef] = genElements(100, 100, 10);
const { result } = renderHook(() => useTruncation(elementRef, plusRef));
expect(result.current).toEqual([0, false]);
testTruncationHookWithInitialValues([100, 100, 10], 0, false);
});
test('should return [1, false] when there is only one truncated element', () => {
const [elementRef, plusRef] = genElements(150, 100, 10);
const { result } = renderHook(() => useTruncation(elementRef, plusRef));
expect(result.current).toEqual([1, false]);
testTruncationHookWithInitialValues([150, 100, 10], 1, false);
});
test('should return [1, true] with one truncated and hidden elements', () => {
const [elementRef, plusRef] = genElements(150, 100, 10, [
{ offsetWidth: 150 } as HTMLElement,
{ offsetWidth: 150 } as HTMLElement,
]);
const { result } = renderHook(() => useTruncation(elementRef, plusRef));
expect(result.current).toEqual([1, true]);
testTruncationHookWithInitialValues(
[
150,
100,
10,
[
{ offsetWidth: 150 } as HTMLElement,
{ offsetWidth: 150 } as HTMLElement,
],
],
1,
true,
);
});
test('should return [2, true] with 2 truncated and hidden elements', () => {
const [elementRef, plusRef] = genElements(150, 100, 10, [
{ offsetWidth: 150 } as HTMLElement,
{ offsetWidth: 150 } as HTMLElement,
{ offsetWidth: 150 } as HTMLElement,
]);
const { result } = renderHook(() => useTruncation(elementRef, plusRef));
expect(result.current).toEqual([2, true]);
testTruncationHookWithInitialValues(
[
150,
100,
10,
[
{ offsetWidth: 150 } as HTMLElement,
{ offsetWidth: 150 } as HTMLElement,
{ offsetWidth: 150 } as HTMLElement,
],
],
2,
true,
);
});
test('should return [1, true] with plusSize offsetWidth undefined', () => {
const [elementRef, plusRef] = genElements(150, 100, undefined, [
{ offsetWidth: 150 } as HTMLElement,
{ offsetWidth: 150 } as HTMLElement,
]);
const { result } = renderHook(() => useTruncation(elementRef, plusRef));
expect(result.current).toEqual([1, true]);
testTruncationHookWithInitialValues(
[
150,
100,
undefined,
[
{ offsetWidth: 150 } as HTMLElement,
{ offsetWidth: 150 } as HTMLElement,
],
],
1,
true,
);
});
test('should call ResizeObserver.observe on element parent', () => {
const elementRef = { current: document.createElement('div') };
Object.defineProperty(elementRef.current, 'parentElement', {
value: document.createElement('div'),
});
const plusRef = { current: document.createElement('div') };
const { result, rerender } = renderHook(() => useChildElementTruncation());
Object.defineProperty(result.current[0], 'current', {
value: elementRef.current,
});
Object.defineProperty(result.current[1], 'current', {
value: plusRef.current,
});
rerender();
expect(observeMock).toHaveBeenCalled();
expect(observeMock).toHaveBeenCalledWith(elementRef.current.parentElement);
});
test('should not call ResizeObserver.observe if element parent is undefined', () => {
const elementRef = { current: document.createElement('div') };
const plusRef = { current: document.createElement('div') };
const { result, rerender } = renderHook(() => useChildElementTruncation());
Object.defineProperty(result.current[0], 'current', {
value: elementRef.current,
});
Object.defineProperty(result.current[1], 'current', {
value: plusRef.current,
});
rerender();
expect(observeMock).not.toHaveBeenCalled();
});

View File

@ -16,7 +16,7 @@
* specific language governing permissions and limitations
* under the License.
*/
import { RefObject, useLayoutEffect, useState, useRef } from 'react';
import { useLayoutEffect, useRef, useState } from 'react';
/**
* This hook encapsulates logic to support truncation of child HTML
@ -27,92 +27,68 @@ import { RefObject, useLayoutEffect, useState, useRef } from 'react';
* (including those completely hidden) and whether any elements
* are completely hidden.
*/
const useChildElementTruncation = (
elementRef: RefObject<HTMLElement>,
plusRef?: RefObject<HTMLElement>,
) => {
const useChildElementTruncation = () => {
const [elementsTruncated, setElementsTruncated] = useState(0);
const [hasHiddenElements, setHasHiddenElements] = useState(false);
const previousEffectInfoRef = useRef({
scrollWidth: 0,
parentElementWidth: 0,
plusRefWidth: 0,
});
const elementRef = useRef<HTMLDivElement>(null);
const plusRef = useRef<HTMLDivElement>(null);
useLayoutEffect(() => {
const currentElement = elementRef.current;
const plusRefElement = plusRef?.current;
if (!currentElement) {
return;
}
const { scrollWidth, clientWidth, childNodes } = currentElement;
// By using the result of this effect to truncate content
// we're effectively changing it's size.
// That will trigger another pass at this effect.
// Depending on the content elements width, that second rerender could
// yield a different truncate count, thus potentially leading to a
// rendering loop.
// There's only a need to recompute if the parent width or the width of
// the child nodes changes.
const previousEffectInfo = previousEffectInfoRef.current;
const parentElementWidth = currentElement.parentElement?.clientWidth || 0;
const plusRefWidth = plusRefElement?.offsetWidth || 0;
previousEffectInfoRef.current = {
scrollWidth,
parentElementWidth,
plusRefWidth,
};
if (
previousEffectInfo.parentElementWidth === parentElementWidth &&
previousEffectInfo.scrollWidth === scrollWidth &&
previousEffectInfo.plusRefWidth === plusRefWidth
) {
return;
}
if (scrollWidth > clientWidth) {
// "..." is around 6px wide
const truncationWidth = 6;
const plusSize = plusRefElement?.offsetWidth || 0;
const maxWidth = clientWidth - truncationWidth;
const elementsCount = childNodes.length;
let width = 0;
let hiddenElements = 0;
for (let i = 0; i < elementsCount; i += 1) {
const itemWidth = (childNodes[i] as HTMLElement).offsetWidth;
const remainingWidth = maxWidth - truncationWidth - width - plusSize;
// assures it shows +{number} only when the item is not visible
if (remainingWidth <= 0) {
hiddenElements += 1;
}
width += itemWidth;
const onResize = () => {
const currentElement = elementRef.current;
if (!currentElement) {
return;
}
const plusRefElement = plusRef.current;
const { scrollWidth, clientWidth, childNodes } = currentElement;
if (elementsCount > 1 && hiddenElements) {
setHasHiddenElements(true);
setElementsTruncated(hiddenElements);
if (scrollWidth > clientWidth) {
// "..." is around 6px wide
const truncationWidth = 6;
const plusSize = plusRefElement?.offsetWidth || 0;
const maxWidth = clientWidth - truncationWidth;
const elementsCount = childNodes.length;
let width = 0;
let hiddenElements = 0;
for (let i = 0; i < elementsCount; i += 1) {
const itemWidth = (childNodes[i] as HTMLElement).offsetWidth;
const remainingWidth = maxWidth - width - plusSize;
// assures it shows +{number} only when the item is not visible
if (remainingWidth <= 0) {
hiddenElements += 1;
}
width += itemWidth;
}
if (elementsCount > 1 && hiddenElements) {
setHasHiddenElements(true);
setElementsTruncated(hiddenElements);
} else {
setHasHiddenElements(false);
setElementsTruncated(1);
}
} else {
setHasHiddenElements(false);
setElementsTruncated(1);
setElementsTruncated(0);
}
} else {
setHasHiddenElements(false);
setElementsTruncated(0);
}
}, [
elementRef.current?.offsetWidth,
elementRef.current?.clientWidth,
elementRef,
]);
};
const obs = new ResizeObserver(onResize);
return [elementsTruncated, hasHiddenElements];
const element = elementRef.current?.parentElement;
if (element) {
obs.observe(element);
}
onResize();
return () => {
obs.disconnect();
};
}, [plusRef.current]); // plus is rendered dynamically - the component rerenders the hook when plus appears, this makes sure that useLayoutEffect is rerun
return [elementRef, plusRef, elementsTruncated, hasHiddenElements] as const;
};
export default useChildElementTruncation;

View File

@ -16,7 +16,7 @@
* specific language governing permissions and limitations
* under the License.
*/
import React, { useMemo, useRef } from 'react';
import React, { useMemo } from 'react';
import { styled, useTruncation } from '@superset-ui/core';
import { Link } from 'react-router-dom';
import CrossLinksTooltip from './CrossLinksTooltip';
@ -60,17 +60,13 @@ const StyledCrossLinks = styled.div`
`}
`;
export default function CrossLinks({
function CrossLinks({
crossLinks,
maxLinks = 20,
linkPrefix = '/superset/dashboard/',
}: CrossLinksProps) {
const crossLinksRef = useRef<HTMLDivElement>(null);
const plusRef = useRef<HTMLDivElement>(null);
const [elementsTruncated, hasHiddenElements] = useTruncation(
crossLinksRef,
plusRef,
);
const [crossLinksRef, plusRef, elementsTruncated, hasHiddenElements] =
useTruncation();
const hasMoreItems = useMemo(
() =>
crossLinks.length > maxLinks ? crossLinks.length - maxLinks : undefined,
@ -80,18 +76,13 @@ export default function CrossLinks({
() => (
<span className="truncated" ref={crossLinksRef} data-test="crosslinks">
{crossLinks.map((link, index) => (
<Link
key={link.id}
to={linkPrefix + link.id}
target="_blank"
rel="noreferer noopener"
>
<Link key={link.id} to={linkPrefix + link.id}>
{index === 0 ? link.title : `, ${link.title}`}
</Link>
))}
</span>
),
[crossLinks],
[crossLinks, crossLinksRef, linkPrefix],
);
const tooltipLinks = useMemo(
() =>
@ -99,7 +90,7 @@ export default function CrossLinks({
title: l.title,
to: linkPrefix + l.id,
})),
[crossLinks, maxLinks],
[crossLinks, linkPrefix, maxLinks],
);
return (
@ -119,3 +110,5 @@ export default function CrossLinks({
</StyledCrossLinks>
);
}
export default React.memo(CrossLinks);

View File

@ -0,0 +1,37 @@
/**
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
import React, { useMemo } from 'react';
import { ensureIsArray } from '@superset-ui/core';
import { ChartLinkedDashboard } from 'src/types/Chart';
import CrossLinks from './CrossLinks';
export const DashboardCrossLinks = React.memo(
({ dashboards }: { dashboards: ChartLinkedDashboard[] }) => {
const crossLinks = useMemo(
() =>
ensureIsArray(dashboards).map((d: ChartLinkedDashboard) => ({
title: d.dashboard_title,
id: d.id,
})),
[dashboards],
);
return <CrossLinks crossLinks={crossLinks} />;
},
);

View File

@ -17,7 +17,7 @@
* under the License.
*/
import React, { ReactNode, useMemo, useRef } from 'react';
import React, { ReactNode, useMemo } from 'react';
import { styled, t, useTruncation } from '@superset-ui/core';
import { Tooltip } from '../Tooltip';
@ -99,12 +99,8 @@ export default function TruncatedList<ListItemType>({
getKey = item => item as unknown as React.Key,
maxLinks = 20,
}: TruncatedListProps<ListItemType>) {
const itemsNotInTooltipRef = useRef<HTMLDivElement>(null);
const plusRef = useRef<HTMLDivElement>(null);
const [elementsTruncated, hasHiddenElements] = useTruncation(
itemsNotInTooltipRef,
plusRef,
) as [number, boolean];
const [itemsNotInTooltipRef, plusRef, elementsTruncated, hasHiddenElements] =
useTruncation();
const nMoreItems = useMemo(
() => (items.length > maxLinks ? items.length - maxLinks : undefined),

View File

@ -16,7 +16,7 @@
* specific language governing permissions and limitations
* under the License.
*/
import React, { useCallback, useMemo, useRef } from 'react';
import React, { useCallback, useMemo } from 'react';
import { useDispatch } from 'react-redux';
import { css, t, useTheme, useTruncation } from '@superset-ui/core';
import Icons from 'src/components/Icons';
@ -53,12 +53,8 @@ const DependencyValue = ({
export const DependenciesRow = React.memo(({ filter }: FilterCardRowProps) => {
const dependencies = useFilterDependencies(filter);
const dependenciesRef = useRef<HTMLDivElement>(null);
const plusRef = useRef<HTMLDivElement>(null);
const [elementsTruncated, hasHiddenElements] = useTruncation(
dependenciesRef,
plusRef,
);
const [dependenciesRef, plusRef, elementsTruncated, hasHiddenElements] =
useTruncation();
const theme = useTheme();
const tooltipText = useMemo(

View File

@ -16,7 +16,7 @@
* specific language governing permissions and limitations
* under the License.
*/
import React, { useRef } from 'react';
import React from 'react';
import { useSelector } from 'react-redux';
import { css, SupersetTheme, useTheme, useTruncation } from '@superset-ui/core';
import Icons from 'src/components/Icons';
@ -31,8 +31,7 @@ export const NameRow = ({
hidePopover,
}: FilterCardRowProps & { hidePopover: () => void }) => {
const theme = useTheme();
const filterNameRef = useRef<HTMLElement>(null);
const [elementsTruncated] = useTruncation(filterNameRef);
const [filterNameRef, , elementsTruncated] = useTruncation();
const dashboardId = useSelector<RootState, number>(
({ dashboardInfo }) => dashboardInfo.id,
);

View File

@ -16,7 +16,7 @@
* specific language governing permissions and limitations
* under the License.
*/
import React, { useMemo, useRef } from 'react';
import React, { useMemo } from 'react';
import { t, useTruncation } from '@superset-ui/core';
import { useFilterScope } from './useFilterScope';
import {
@ -44,13 +44,9 @@ const getTooltipSection = (items: string[] | undefined, label: string) =>
export const ScopeRow = React.memo(({ filter }: FilterCardRowProps) => {
const scope = useFilterScope(filter);
const scopeRef = useRef<HTMLDivElement>(null);
const plusRef = useRef<HTMLDivElement>(null);
const [elementsTruncated, hasHiddenElements] = useTruncation(
scopeRef,
plusRef,
);
const [scopeRef, plusRef, elementsTruncated, hasHiddenElements] =
useTruncation();
const tooltipText = useMemo(() => {
if (elementsTruncated === 0 || !scope) {
return null;
@ -81,7 +77,7 @@ export const ScopeRow = React.memo(({ filter }: FilterCardRowProps) => {
))
: t('None')}
</RowValue>
{hasHiddenElements > 0 && (
{hasHiddenElements && (
<RowTruncationCount ref={plusRef}>
+{elementsTruncated}
</RowTruncationCount>

View File

@ -30,7 +30,7 @@ const asyncRender = (numberOfItems: number) =>
}
render(
<Menu openKeys={['menu']}>
<Menu.SubMenu title="Dashboards added to" key="menu">
<Menu.SubMenu title="On dashboards" key="menu">
<DashboardItems key="menu" dashboards={dashboards} />
</Menu.SubMenu>
</Menu>,

View File

@ -310,7 +310,7 @@ export const useExploreAdditionalActionsMenu = (
</Menu.Item>
)}
<Menu.SubMenu
title={t('Dashboards added to')}
title={t('On dashboards')}
key={MENU_KEYS.DASHBOARDS_ADDED_TO}
>
<DashboardsSubMenu

View File

@ -17,7 +17,6 @@
* under the License.
*/
import {
ensureIsArray,
isFeatureEnabled,
FeatureFlag,
getChartMetadataRegistry,
@ -53,13 +52,12 @@ import ListView, {
ListViewProps,
SelectOption,
} from 'src/components/ListView';
import CrossLinks from 'src/components/ListView/CrossLinks';
import Loading from 'src/components/Loading';
import { dangerouslyGetItemDoNotUse } from 'src/utils/localStorageHelpers';
import withToasts from 'src/components/MessageToasts/withToasts';
import PropertiesModal from 'src/explore/components/PropertiesModal';
import ImportModelsModal from 'src/components/ImportModal/index';
import Chart, { ChartLinkedDashboard } from 'src/types/Chart';
import Chart from 'src/types/Chart';
import Tag from 'src/types/TagType';
import { Tooltip } from 'src/components/Tooltip';
import Icons from 'src/components/Icons';
@ -72,6 +70,7 @@ import FacePile from 'src/components/FacePile';
import ChartCard from 'src/features/charts/ChartCard';
import { UserWithPermissionsAndRoles } from 'src/types/bootstrapTypes';
import { findPermission } from 'src/utils/findPermission';
import { DashboardCrossLinks } from 'src/components/ListView/DashboardCrossLinks';
import { ModifiedInfo } from 'src/components/AuditInfo';
import { QueryObjectColumns } from 'src/views/CRUD/types';
@ -390,21 +389,11 @@ function ChartList(props: ChartListProps) {
row: {
original: { dashboards },
},
}: any) => (
<CrossLinks
crossLinks={ensureIsArray(dashboards).map(
(d: ChartLinkedDashboard) => ({
title: d.dashboard_title,
id: d.id,
}),
)}
/>
),
Header: t('Dashboards added to'),
}: any) => <DashboardCrossLinks dashboards={dashboards} />,
Header: t('On dashboards'),
accessor: 'dashboards',
disableSortBy: true,
size: 'xxl',
hidden: true,
},
{
Cell: ({