Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(dashboard): race condition between hydrating dashboard and set active tabs #17084

Merged
merged 3 commits into from
Oct 13, 2021

Conversation

kgabryje
Copy link
Member

SUMMARY

Fixes #17060.
Due to a race condition, sometimes hydrateDashboard was dispatched after setActiveTabs. That caused an override of current active tabs by initial state, which is an empty array. That caused the native filters that are in scope of current tab to be considered as out of scope.
This PR fixes that behaviour by ensuring that hydrateDashboard gets called first. Previously, we called it in useEffect, which resulted in calling the function in the NEXT render cycle, after the dashboard components are rendered (setActiveTabs is called in mount lifecycle event of the Tabs component). I replaced useEffect with an if condition based on whether the dashboard has already been hydrated or not (useRef was used to ensure that we call it only once).

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before: see linked issue
After:
https://user-images.githubusercontent.com/15073128/137131275-2994e81e-870c-480f-be5f-7bcc5b1b46cd.mov

TESTING INSTRUCTIONS

  1. Open a dashboard with native filters
  2. Verify that filters are showed as "in scope"
  3. Go back to dashboard list
  4. Go back to the dashboard and verify that native filters still show as they're supposed to

ADDITIONAL INFORMATION

CC @junlincc

@@ -141,7 +141,12 @@ export function initEnhancer(
const composeEnhancers =
process.env.WEBPACK_MODE === 'development'
? /* eslint-disable-next-line no-underscore-dangle, dot-notation */
window['__REDUX_DEVTOOLS_EXTENSION_COMPOSE__'] || compose
window['__REDUX_DEVTOOLS_EXTENSION_COMPOSE__']
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not directly related to the fixed bug, but this will allow us to trace which component dispatched an action from the redux inspector tool

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the added test 👍

@kgabryje
Copy link
Member Author

/testenv up

@github-actions
Copy link
Contributor

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

@codecov
Copy link

codecov bot commented Oct 13, 2021

Codecov Report

Merging #17084 (d0662c1) into master (9e980b6) will decrease coverage by 0.04%.
The diff coverage is 27.27%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17084      +/-   ##
==========================================
- Coverage   76.94%   76.89%   -0.05%     
==========================================
  Files        1030     1031       +1     
  Lines       55088    55190     +102     
  Branches     7480     7504      +24     
==========================================
+ Hits        42385    42437      +52     
- Misses      12452    12503      +51     
+ Partials      251      250       -1     
Flag Coverage Δ
javascript 70.82% <27.27%> (-0.08%) ⬇️

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

Impacted Files Coverage Δ
...rontend/src/dashboard/containers/DashboardPage.tsx 0.00% <0.00%> (ø)
superset-frontend/src/reduxUtils.ts 69.76% <0.00%> (-1.67%) ⬇️
...-frontend/src/dashboard/reducers/dashboardState.js 71.01% <100.00%> (+9.04%) ⬆️
...explore/components/controls/ColorSchemeControl.jsx 60.60% <0.00%> (-28.68%) ⬇️
...nd/src/views/CRUD/data/dataset/AddDatasetModal.tsx 54.54% <0.00%> (-8.32%) ⬇️
...nd/src/components/ErrorMessage/BasicErrorAlert.tsx 95.23% <0.00%> (-4.77%) ⬇️
...et-frontend/src/components/TableSelector/index.tsx 74.52% <0.00%> (-3.03%) ⬇️
...onfigModal/FiltersConfigForm/FiltersConfigForm.tsx 71.60% <0.00%> (-2.23%) ⬇️
...erset-frontend/src/datasource/DatasourceEditor.jsx 71.91% <0.00%> (-1.94%) ⬇️
...rontend/src/components/ErrorMessage/ErrorAlert.tsx 92.59% <0.00%> (-1.86%) ⬇️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9e980b6...d0662c1. Read the comment docs.

@kgabryje kgabryje merged commit 3ad7483 into apache:master Oct 13, 2021
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

@villebro villebro added the v1.4 label Oct 15, 2021
eschutho pushed a commit to preset-io/superset that referenced this pull request Oct 27, 2021
…tive tabs (apache#17084)

* fix(dashboard): race condition between hydrating dashboard and set active tabs

* Fix linting

* Change variable name

(cherry picked from commit 3ad7483)
opus-42 pushed a commit to opus-42/incubator-superset that referenced this pull request Nov 14, 2021
…tive tabs (apache#17084)

* fix(dashboard): race condition between hydrating dashboard and set active tabs

* Fix linting

* Change variable name
@mistercrunch mistercrunch added 🍒 1.4.0 🍒 1.4.1 🍒 1.4.2 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/M v1.4 🍒 1.4.0 🍒 1.4.1 🍒 1.4.2 🚢 1.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[native_filter]Filters being put in out of scope when first opening the dashboard
3 participants