-
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
perf(native-filters): improve native filter modal form performance #21821
perf(native-filters): improve native filter modal form performance #21821
Conversation
Codecov Report
@@ Coverage Diff @@
## master #21821 +/- ##
==========================================
+ Coverage 66.84% 66.89% +0.04%
==========================================
Files 1800 1805 +5
Lines 68951 69059 +108
Branches 7335 7372 +37
==========================================
+ Hits 46092 46197 +105
+ Misses 20965 20952 -13
- Partials 1894 1910 +16
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
Great work @stephenLYZ! Really improved the performance. Code LGTM.
During the tests, I found a small issue that may have been there the whole time. These are the steps to reproduce it:
- Select Scoping in the first filter
- Navigate to the second filter
- Go back to the first filter
- Select Settings
- You will see that the panels are collapsed and you need to click twice to open them
Screen.Recording.2022-10-17.at.6.34.33.PM.mov
@jinghua-qa I tested the module but we'll definitely need your help here. Can you please book some time to test this PR? |
/testenv up |
@stephenLYZ Ephemeral environment spinning up at http://34.210.29.66: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 LGTM. Looking for QA approval.
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.
Test in my local. codes LGTM.
It's my first time reading the Filter Dependence Tree codes, IMO, the data structure of dependencyMap
should be improved. Currently, we use a hashmap to maintain a hierarchical data structure, for instance, the type of buildDependencyMap
is Map<string, string[]>
, so the hasCircularDependency
and getDependencySuggestion
have to make recursion getting results.
const getInitialFilterOrder = () => Object.keys(filterConfigMap); | ||
|
||
// State for tracking the re-ordering of filters | ||
const [orderedFilters, setOrderedFilters] = useState<string[]>( | ||
getInitialFilterOrder(), | ||
); | ||
|
||
// State for rendered filter to improve performance | ||
const [renderedFilters, setRenderedFilters] = useState<string[]>([ |
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.
nits: use Set for this case.
const [renderedFilters, setRenderedFilters] = useState<Set[]>([
@jinghua-qa Can you please help to test this PR? |
Ephemeral environment shutdown and build artifacts deleted. |
SUMMARY
Improve native filter modal performance by adding memo or reduce rendering.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Test case : 30+ native filters, 20+ charts
before
2022-10-16.2.23.33.mov
after
2022-10-16.2.27.48.mov
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION