fix: 🐛 Fix filters sometimes not being displayed #3050
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
hashicc
left a comment
hashicc
left a comment
There was a problem hiding this comment.
It looks good, I have one partial suggestion and one comment for discussion, but nothing blocking. Thanks for looking back over the recent changes for this edge case.
| name: item.name, | ||
| type: key, | ||
| })); | ||
| return uniqueSelectedFilters.filter(Boolean).map((item) => { |
There was a problem hiding this comment.
I'm not sure how much this would change things in reality, but does it make sense to start with iterating on selectedFilters (as uniqueSelectedFilters) as the top-level loop instead of starting from allFilters? This would prevent valueMap being created if a filter doesn't exist in selectedFilters- which could be fixed with an early return but it seems like get filters() is focused on returning a mapped selectedFilters + augmented name which it needs from a lookup on allFilters.
There was a problem hiding this comment.
I think I get what you're saying but I think it would be same either way? The loop on allFilters would still be the same if it was on selectedFilters since the initial loop is over each filter type, and not the actual filters.
i.e. it's the same regardless of which one I iterate over:
allFilters: {
users: [],
targets: [],
status: [],
},
selectedFilters: {
users: [],
targets: [],
status: [],
},
The real loop is on uniqueSelectedFilters. The map was only an (debatable if needed) optimization so I could do an O(1) lookup on getting the filter names, it seems like the choice of using it would be the same either way regardless of which one I do the initial loop on.
Let me know if I misunderstood what you meant
There was a problem hiding this comment.
If a key of uniqueSelectedFilters has an empty array, does the map based on allFilters need to get set up at all? That makes sense that it doesn't really matter as long as allFilters and selectedFilters have the same set of keys which it looks like in reality they do
There was a problem hiding this comment.
Yeah good point, it doesn't need need the map if it's empty. I added the early return to handle that
| @tracked search; | ||
| @tracked options = []; | ||
| @tracked _options = []; | ||
| #allOptions = new Map(); |
There was a problem hiding this comment.
I'm wondering if the interface should be: selectedOptions and options? I'm struggling with the idea that "all" is never really "all" in large filter sets, as the set could be constantly changing as 250 results get loaded based on different search terms and then options ends up just being a cache of any previously seen option... At least that's my understanding from reading the code. Or maybe allOptions needs a nice comment of what "all" means
There was a problem hiding this comment.
So it's actually kind of tough, these are also not the selectedOptions, since that's being tracked by the query params separately which I would find even more confusing if it was called selectedOptions. I think a comment might be better here because you're right, it's not "all" but it's just keeping track of any previously loaded/seen options from the dropdown.
There was a problem hiding this comment.
That makes sense, I think some sort of hint would help
ZedLi
force-pushed
the
fix-filter-display
branch
from
65c4576 to
b8526a0
Compare
priya-patel04
left a comment
priya-patel04
left a comment
There was a problem hiding this comment.
Tested and works as expected. Thanks for the fix!
3 participants
Description
One of the changes that was made in #2981 was not retrieving all resources for the dropdown filters. This had an unintended consequence when displaying the selected filters as it's possible for the selected filter to not be in the list of
allFilters.There's two main scenarios this happens (the prerequisite is having more filter options than we display which is 250):
Screenshots (if appropriate)
Before:
Screen.Recording.2025-11-04.at.2.23.52.PM.mov
After:
Screen.Recording.2025-11-04.at.2.25.09.PM.mov
How to Test
Use this cluster. Try applying different searches for the target dropdown and confirm that the filter tags always appear.
Checklist
[ ] I have added JSON response output for API changes[ ] I have addeda11y-testslabel to run a11y audit tests if neededPCI review checklist
Examples of changes to security controls include using new access control methods, adding or removing logging pipelines, etc.