fix(native-filters): filter indicator stale state #16831
Conversation
Codecov Report
@@ Coverage Diff @@
## master #16831 +/- ##
==========================================
- Coverage 76.99% 76.99% -0.01%
==========================================
Files 1018 1018
Lines 54668 54673 +5
Branches 7456 7456
==========================================
- Hits 42094 42093 -1
- Misses 12329 12335 +6
Partials 245 245
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
/testenv up FEATURE_DASHBOARD_NATIVE_FILTERS=true FEATURE_DASHBOARD_CROSS_FILTERS=true FEATURE_DASHBOARD_NATIVE_FILTERS_SET=true FEATURE_DASHBOARD_FILTERS_EXPERIMENTAL=true |
|
@geido Ephemeral environment spinning up at http://54.214.186.247:8080. Credentials are |
There was a problem hiding this comment.
The fix looks good to me for what concerns the applied filters. However, in the original issue, it seems that the query is also not updated sometimes. I tested the Video Games Sales dashboard and it appears that Charts that have temporal columns, such as the # of Games That Hit 100k in Sales By Release Year, didn't get the query updated.
As discussed, I agree that this should be filed separately as the problem seems to be affecting the Dashboard persistently and it does not look to be related to the filter indicators. CC @junlincc @jinghua-qa
|
With this fix, the indicator got updated when filter applied and when filter clear, but I still saw the 2nd issue I mentioned in #16806: when filter applied, sometimes the query did not got updated, should i file this seperately? |
@jinghua-qa let's file a separate issue, as they are unrelated bugs |
|
Ephemeral environment shutdown and build artifacts deleted. |
|
🏷️ 2021.38 |
(cherry picked from commit 42fa548)
mistercrunch
added
🏷️ bot
SUMMARY
The recent perf improvement PR #16545 introduced a regression where some chart indicators didn't update correctly after applying filters. This was caused by all indicators sharing the same state that was used to check if the relevant data had changed. This changes the logic so that all indicators have their own set of "previous" objects to ensure that the result is recalculated when a relevant change has occurred.
When checking if this was also a problem with the legacy indicators I noticed that the logic there mostly resembles the proposed solution here, so this shouldn't be a problem there.
Fixes: #16806
AFTER
When applying new filters, all filter indicators are now updated correctly:
BEFORE
Previously only the first few charts' indicators were updated; the later ones always hit the cache due to the shared state having already been updated and hence satisfying the equality checks:
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION