-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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 when setting activeTabs with nested tabs #17007
Conversation
Codecov Report
@@ Coverage Diff @@
## master #17007 +/- ##
==========================================
- Coverage 76.98% 76.91% -0.07%
==========================================
Files 1028 1030 +2
Lines 55074 55032 -42
Branches 7486 7469 -17
==========================================
- Hits 42398 42330 -68
- Misses 12428 12451 +23
- Partials 248 251 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
/testenv up |
@geido Ephemeral environment spinning up at http://54.200.62.35:8080. Credentials are |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code changes LGTM - it can be tricky to create a test that consistently reproduces this using the full layout, but I'd be fine with a test that asserts that redux state looks correct when called in a similar fashion to when the bug was still there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discussed testing approaches here at length with @kgabryje , and concluded that in the absence of a proper native filters RTL test suite (a big task that warrants a PR of it's own) and the relative simplicity of the reducer in question, I feel we can merge this as-is as long as we start working on integration tests as a follow-up task.
After discussion with @villebro: |
Ephemeral environment shutdown and build artifacts deleted. |
🏷️ 2021.40 |
…bs (apache#17007) * fix(dashboard): Race condition when setting activeTabs with nested tabs * Remove activeTabs prop from Tabs (cherry picked from commit 45908ff)
…bs (apache#17007) * fix(dashboard): Race condition when setting activeTabs with nested tabs * Remove activeTabs prop from Tabs (cherry picked from commit 45908ff)
…bs (apache#17007) * fix(dashboard): Race condition when setting activeTabs with nested tabs * Remove activeTabs prop from Tabs
…bs (apache#17007) * fix(dashboard): Race condition when setting activeTabs with nested tabs * Remove activeTabs prop from Tabs
SUMMARY
Fixes #16972
Active tabs were calculated in mount/update lifecycle events in Tabs components by taking current active tabs state and appending active tab key to it. That approach resulted in race condition when there are multiple Tabs components added to a dashboard - they were all operating on the same array and when 1 component finished its job, the rest were still operating on the now outdated array of active tabs. The result was overwriting the results calculated by other Tabs components.
In order to avoid that, I moved that logic into the Redux reducer, which is guaranteed to be synchronous. Now the Tabs only send current active key and previous active key as strings to Redux and the reducer handles calculating active tabs synchronously.
The described issue was the direct reason of the linked bug - the filters were out of scope, because
activeTabs
was not calculated correctly.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before: see linked issue
After:
https://user-images.githubusercontent.com/15073128/136361545-77fa09ff-dffe-4832-b405-53af47d98727.mov
TESTING INSTRUCTIONS
dashboardState.activeTabs
in Redux has correct valueADDITIONAL INFORMATION
CC @junlincc @graceguo-supercat