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

kgabryje · 2024-03-27T09:29:19Z

SUMMARY

Due to a bug in useChildElementTruncation, there was an edge case which could trigger an infinite loop of rerenders. The bug occurs when the difference in width of subsequent numbers after the plus sign (e.g. +4 and +5) is just big enough to change the truncation state to the other number. This PR ensures that the calculation is performed only once, and if the edge case is triggered, we simply accept the off-by-one error.
This PR also implements a ResizeObserver which recalculates the truncations if the container size changes. Previously we relied on the width values from references in useLayoutEffect's dependency array, which was not a correct approach, since the changes of those values would not trigger a rerender.

This change allowed us to bring back the feature that relied on the truncation logic, which is dashboard cross links on Chart List page. The feature adds a column to the list which displays the dashboards that given chart is added to.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before (occurred long ago when "dashboards added to" column was not yet hidden):

After:

TESTING INSTRUCTIONS

Visit ChartList
Verify that "Dashboards added to" column is visible and that it correctly displays the dashboards. Dashboard names should be clickable and navigate to given dashboard. Truncated dashboards should appear in a tooltip

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

codecov · 2024-03-27T13:31:04Z

Codecov Report

Attention: Patch coverage is 97.91667% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 69.79%. Comparing base (951d7d6) to head (62932b3).

Files	Patch %	Lines
...d/components/nativeFilters/FilterCard/ScopeRow.tsx	50.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #27701      +/-   ##
==========================================
+ Coverage   69.78%   69.79%   +0.01%     
==========================================
  Files        1911     1914       +3     
  Lines       75024    75103      +79     
  Branches     8355     8390      +35     
==========================================
+ Hits        52352    52415      +63     
- Misses      20622    20624       +2     
- Partials     2050     2064      +14

Flag	Coverage Δ
javascript	`57.52% <97.91%> (+0.04%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

justinpark · 2024-03-27T21:21:38Z

superset-frontend/src/pages/ChartList/index.tsx

@@ -157,6 +157,20 @@ const StyledActions = styled.div`
  color: ${({ theme }) => theme.colors.grayscale.base};
 `;

+const DashboardCrossLinks = React.memo(


It appears to be more beneficial to create a dedicated file specifically for this component.

Agreed 👍

…hartList

kgabryje · 2024-04-03T13:42:33Z

@justinpark would you be able to do another pass?

kgabryje · 2024-04-03T13:42:52Z

/testenv up

github-actions · 2024-04-03T13:52:04Z

@kgabryje Ephemeral environment spinning up at http://34.220.216.100:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

betodealmeida · 2024-04-03T21:54:32Z

...rset-frontend/packages/superset-ui-core/src/hooks/useTruncation/useChildElementTruncation.ts

+        // "..." is around 6px wide
+        const truncationWidth = 6;


I thought we had a function for computing the width of a given element? Can we use something like element.getBoundingClientRect().width to get the exact width of the ellipsis rendered in an invisible element?

Not really - the ellipsis is not something that we can grab, because it's not an element in the DOM. What we could do to get a more precise measurement is render <span>...</span> somewhere off screen, measure the width, remove that element and use that width here.
To be honest I'm not sure if it's worth it, the worst thing that can happen here is an off-by-one error in some edge cases

Here's what ChatGPT has to say about this:

betodealmeida · 2024-04-03T21:55:12Z

...rset-frontend/packages/superset-ui-core/src/hooks/useTruncation/useChildElementTruncation.ts

+    return () => {
+      obs.disconnect();
+    };


Suggested change

return () => {

obs.disconnect();

};

return obs.disconnect;

Oh good point! Though to be fair, I’m not sure about readability - I was halfway through writing a comment about how we should return a cleanup function rather than returning obs.disconnect()… but it’s early morning, so maybe it’s my fault 😉

I'm almost 50-50 here, but I'm leaning in the direction of @betodealmeida here, so I'd say +1 for changing to return obs.disconnect;

2 vs 1, request for change has passed 🙂

@villebro @betodealmeida With return obs.disconnect, the page is crashing with illegal invocation error. To be honest I don't know why it doesn't work, but it does work with return () => { obs.disconnect(); } so I'm going with that

betodealmeida · 2024-04-03T21:56:13Z

superset-frontend/src/dashboard/components/nativeFilters/FilterCard/NameRow.tsx

@@ -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();


villebro

In addition to the fix, very nice cleanup work here 👍 A non-blocking comment on the column name + supporting Beto's change request.

villebro · 2024-04-08T19:33:29Z

...rset-frontend/packages/superset-ui-core/src/hooks/useTruncation/useChildElementTruncation.ts

+    return () => {
+      obs.disconnect();
+    };


I'm almost 50-50 here, but I'm leaning in the direction of @betodealmeida here, so I'd say +1 for changing to return obs.disconnect;

villebro · 2024-04-08T19:34:41Z

superset-frontend/cypress-base/cypress/e2e/chart_list/list.test.ts

-      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('Dashboards added to');


This is maybe out of scope, but I feel Dashboards added to is too verbose, and I think just going with Dashboards or On dashboards makes more sense.

@yousoph @kasiazjc wdyt?

@kasiazjc says that "On dashboards" sounds fine. I changed it here (in charts list) and also in Explore for consistence

github-actions · 2024-04-09T10:31:29Z

Ephemeral environment shutdown and build artifacts deleted.

…hartList (apache#27701)

…hartList (#27701) (cherry picked from commit ae0f2ce)

…hartList (apache#27701)

kgabryje requested review from eschutho, Antonio-RiveroMartnez and geido March 27, 2024 09:29

pull-request-size bot added the size/L label Mar 27, 2024

github-actions bot added the packages label Mar 27, 2024

kgabryje requested review from jinghua-qa, rusackas and betodealmeida as code owners March 27, 2024 13:05

kgabryje requested a review from villebro March 27, 2024 14:32

kgabryje force-pushed the fix/useTruncation-infinite-loop branch from e9830f9 to 08827ca Compare March 27, 2024 14:36

justinpark reviewed Mar 27, 2024

View reviewed changes

kgabryje added 7 commits March 28, 2024 14:10

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

d6b37f2

…hartList

Fix tests

75ecbc1

Improve coverage

d0f26ed

Reenable cypress tests

df93052

Move DashboardCrossLinks to separate file

470e8c4

Open dashboard in the same tab

c59ad29

Add license

62932b3

kgabryje force-pushed the fix/useTruncation-infinite-loop branch from 49f24f0 to 62932b3 Compare March 28, 2024 13:10

betodealmeida reviewed Apr 3, 2024

View reviewed changes

eschutho approved these changes Apr 8, 2024

View reviewed changes

villebro approved these changes Apr 8, 2024

View reviewed changes

kgabryje added 2 commits April 9, 2024 10:34

Address comments

bdd1b21

Revert obs.disconnect change

9b4456c

kgabryje merged commit ae0f2ce into apache:master Apr 9, 2024
29 checks passed

michael-s-molina added the v4.0 Label added by the release manager to track PRs to be included in the 4.0 branch label Apr 11, 2024

EnxDev pushed a commit to EnxDev/superset that referenced this pull request Apr 15, 2024

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

a3fd451

…hartList (apache#27701)

michael-s-molina pushed a commit that referenced this pull request Apr 16, 2024

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

42e1329

…hartList (#27701) (cherry picked from commit ae0f2ce)

betodealmeida pushed a commit that referenced this pull request Apr 25, 2024

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

292c3f2

…hartList (#27701) (cherry picked from commit ae0f2ce)

qleroy pushed a commit to qleroy/superset that referenced this pull request Apr 28, 2024

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

dd5531e

…hartList (apache#27701)

jzhao62 pushed a commit to jzhao62/superset that referenced this pull request May 16, 2024

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

e0e15df

…hartList (apache#27701)

mistercrunch added 🍒 4.0.1 🍒 4.0.2 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels labels Jul 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kgabryje commented Mar 27, 2024 •

edited

Loading

codecov bot commented Mar 27, 2024 •

edited

Loading

justinpark Mar 27, 2024

kgabryje Mar 28, 2024

kgabryje Mar 28, 2024

kgabryje commented Apr 3, 2024

kgabryje commented Apr 3, 2024

github-actions bot commented Apr 3, 2024

betodealmeida Apr 3, 2024

kgabryje Apr 4, 2024 •

edited

Loading

kgabryje Apr 4, 2024

betodealmeida Apr 3, 2024

kgabryje Apr 4, 2024

villebro Apr 8, 2024

kgabryje Apr 9, 2024

kgabryje Apr 9, 2024

betodealmeida Apr 3, 2024

villebro left a comment

villebro Apr 8, 2024

villebro Apr 8, 2024

kgabryje Apr 9, 2024

kgabryje Apr 9, 2024

github-actions bot commented Apr 9, 2024

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

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

Conversation

kgabryje commented Mar 27, 2024 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Mar 27, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgabryje commented Apr 3, 2024

kgabryje commented Apr 3, 2024

github-actions bot commented Apr 3, 2024

Choose a reason for hiding this comment

kgabryje Apr 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

villebro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 9, 2024

kgabryje commented Mar 27, 2024 •

edited

Loading

codecov bot commented Mar 27, 2024 •

edited

Loading

kgabryje Apr 4, 2024 •

edited

Loading