-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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: Multiple dashboard refresh triggers for the same session #16094
fix: Multiple dashboard refresh triggers for the same session #16094
Conversation
Codecov Report
@@ Coverage Diff @@
## master #16094 +/- ##
==========================================
- Coverage 76.88% 76.85% -0.04%
==========================================
Files 995 995
Lines 52864 52887 +23
Branches 6712 6721 +9
==========================================
- Hits 40646 40645 -1
- Misses 11992 12017 +25
+ Partials 226 225 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
your BEFORE AFTER videos crack me up LOL! ∞∞∞∞∞∞∞ .thanks for the fix! LGTM |
@@ -366,7 +366,8 @@ export const hydrateDashboard = (dashboardData, chartData) => ( | |||
directPathLastUpdated: Date.now(), | |||
focusedFilterField: null, | |||
expandedSlices: metadata?.expanded_slices || {}, | |||
refreshFrequency: metadata?.refresh_frequency || 0, | |||
refreshFrequency: | |||
metadata?.refresh_frequency || dashboardState.refreshFrequency || 0, |
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.
I'm not sure about the || dashboardState.refreshFrequency
here. If you start at dashboard A (which has refresh), navigate to the dashboard list and then go to dashboard B (which has no refresh) wouldn't B inherit the refresh frequency from A?
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.
I wish we had a redux store where these things were keyed by dashboard id
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.
@suddjian You are totally right! I tested and B is inheriting from A. I'll try to fix it.
/testenv up |
@suddjian Ephemeral environment spinning up at http://34.220.92.135:8080. Credentials are |
84e48bc
to
e9af0c1
Compare
@suddjian @junlincc Since our redux store is not indexed by the dashboard, and changing that can potentially break other functions, I decided to cancel the timers as soon as the user leaves the dashboard. Is not the ideal solution, but it's better than the current state, where the timers are being duplicated. |
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.
nice fix!
Ephemeral environment shutdown and build artifacts deleted. |
🏷 2021.31 |
(cherry picked from commit 07f3399)
SUMMARY
Fix: Multiple dashboard refresh triggers for the same session.
@junlincc @jinghua-qa
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
screen-recording-2021-08-05-at-22337-pm_rshjFmrR.mp4
screen-recording-2021-08-05-at-21917-pm_umGDz3Fq.mp4
TESTING INSTRUCTIONS
Check the videos for instructions.
ADDITIONAL INFORMATION