fix: useTruncation infinite loop, reenable dashboard cross links on C…

…hartList (#27701)
apache · Apr 9, 2024 · ae0f2ce · ae0f2ce
1 parent 4ecfce9
commit ae0f2ce
Show file tree

Hide file tree

Showing 13 changed files with 278 additions and 175 deletions.
diff --git a/superset-frontend/cypress-base/cypress/e2e/chart_list/list.test.ts b/superset-frontend/cypress-base/cypress/e2e/chart_list/list.test.ts
@@ -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', () => {

diff --git a/superset-frontend/cypress-base/cypress/e2e/explore/chart.test.js b/superset-frontend/cypress-base/cypress/e2e/explore/chart.test.js
@@ -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();
 }

diff --git a/...ntend/packages/superset-ui-core/src/hooks/useTruncation/useChildElementTruncation.test.ts b/...ntend/packages/superset-ui-core/src/hooks/useTruncation/useChildElementTruncation.test.ts
@@ -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();
 });