-
Notifications
You must be signed in to change notification settings - Fork 3.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: Preserve tree expansion state #22347
fix: Preserve tree expansion state #22347
Conversation
Thanks for taking the time to open a PR!
|
…p/CLOUD-577-cache-tree-expansion-state
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.
Looks good and functional. I defer to @marktnoonan to advise in terms of performance and memory use, but it seems to have minimal memory footprint using a sparse map.
@@ -92,7 +103,7 @@ function sortTree<T extends RawNode<T>> (tree: T) { | |||
} | |||
|
|||
export function useCollapsibleTree <T extends RawNode<T>> (tree: T, options: UseCollapsibleTreeOptions = {}) { | |||
options.expandInitially = options.expandInitially || true | |||
options.expandInitially = options.expandInitially ?? true |
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.
Good catch!
Test summaryRun details
View run in Cypress Dashboard ➡️ FlakinessThis comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
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 with minimal changes. The virtual list feels kind of bleh, I think we need to rethink this approach. If we do away with highlighting characters during fuzzy search I think we can vastly cut down on the number of DOM nodes and avoid the need virtualize it, but another battle for another day.
Will wait for a review from @marktnoonan, he's more across the UI side of app than me.
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.
Forgot to drop this approval before I logged off yesterday, but this looks good to me. The old behavior was really noticeable, especially when even collapsing a row would cause new items to render, re-expanding the row you just collapsed. Good solution.
Video: https://cypressio.slack.com/archives/C035VB3HVAL/p1655219943591069
Addressing a reactivity issue we currently have in our
useCollapsibleTree
usage. In ACI, row data is loaded as the user scrolls and periodically in the background - these fetches flow back through GQL and present as a "new" list of specs (even though it's the same specs, just with new deeply-nested content) which triggers the tree data to get rebuilt. This manifests as the tree state re-setting on scroll and is likely contributing to some of the scroll lagginess we're seeing.Without reworking the implementation which would impact multiple UI areas at present this is a next-best: incorporate an external cache that can be used to persist the expansion state of the directories so that when the tree does get rebuilt the state is maintained. This cache is then cleared when the user searches or changes the list of specs because we want the directories to expand in those situations to match 10.1.0 behavior.
User facing changelog
Additional details
I wasn't able to find a good way to test this in CT; since SpecList is using
mountFragment
and GQL I couldn't find a way to mutate the GQL data and trigger a reactive data update within a single mount cycle. If someone knows how to do this I'd love to learn howSteps to test
How has the user experience changed?
SpecsList directory expansion state should persist as ACI data is loaded during scroll
PR Tasks
cypress-documentation
?type definitions
?