feat: active connections filter COMPASS-7772

[paula-stacho]

Description

The filtering and expanding logic & state used to be handled in the store -> databases slice. Now that it concerns both databases and connections, this didn't make sense anymore. Especially because the parent-children relationship often needs to be taken into account.
Because the connections are currently coming through a hook, the logic & state was moved into the react components. This might change in the future if we move active connections into the store. It shouldn't be too difficult to move as the logic will stay the same.

Old filter: databases & collections

has been refactored to but should behave the same

1. Items which match the regex are included in the results.
2. Parents whose children match the regex are also included in the results.
3. All parents included in the results are expanded.
4. User can still collapse these parents (while the filter is active).
5. After the user clears out the filter, the parents from point above return to the user controlled state.

New filter: connections & databases & collections:

Follows the same logic, except:

3. Only parents of matching children are expanded, so that the user can see the matching children. If only the parent is the match, it is included with all it's children, but not auto-expanded.

This way the results look much leaner and it's easier to find the thing that we were actually looking for. For even better experience, we're planning to implement match highlighting in COMPASS-7952

Note: The connections & databases differ in their expanding logic, because the former is expanded by default, the latter collapsed by default.

I have included a number of tests trying to cover various scenarios of filtering and expanding. I still recommend to play around with the filter and let me know if you see some weird behaviour 🙏

Checklist

• New tests and/or benchmarks are included
• Documentation is changed or added
• I have signed the MongoDB Contributor License Agreement (https://www.mongodb.com/legal/contributor-agreement)

Motivation and Context

• Bugfix
• New feature
• Dependency update
• Misc

Dependents

Types of changes

• Backport Needed
• Patch (non-breaking change which fixes an issue)
• Minor (non-breaking change which adds functionality)
• Major (fix or feature that would cause existing functionality to change)

[himanshusinghs]

nit: You might want the new tokens here?

[himanshusinghs]

Maybe we can do this in a memo in the component to avoid doing this re-filtering everytime something else changes?

[himanshusinghs]

nit: We can probably avoid having React state for these by having a memoized variable that will always point to either a filteredConnections or all the connections. This way you also eliminate the need to have tempExpanded as one of the state of expandedConnections and the corresponding useEffect below because the memoization can take care of adding the 'tempExpanded' to the returned data structure.

[gribnoysup]

Services should be only accessed through the store actions. I understand it that we are continuing to accumulate the tech debt with all this work, but please, let's add a todo to clean it up

Also please let's continue running this if filter is actually present, this operation is extremely expensive, even more so with multiple connections.

[paula-stacho]

gosh, thanks! this made me realise I missed to emit the 'sidebar-expand-database'. I didn't notice because this one was called immediately and fetched all collections!

[paula-stacho]

moved the two events emitting to thunk. feels a bit weird as the actions do not affect the state. but if we later consolidate connections and databases in the store, at least we won't have to search for this bit.

[gribnoysup]

Feels like we're unnecessarily overcomplicating the logic here (I'm leaving a comment on this method, but this touches most of the code here) by mixing separate derived states into a persisted one instead of keeping it derived.

Your source of truth for this is user triggered collapsed state (like literally clicking / pressing a button), filter regex, current active tab. From this you can always derive expanded state as userExpanded || (filter && matchesFilter) || matchesActiveTab. The only caveat here is that user changing a filter should have a side effect on the user emitted expanded state and reset it, but dispatching one side effect like that feels more straightforward than reacting to all possible side effects all the time in various different places

[paula-stacho]

I'm not sure if I follow what exactly you're proposing. I tried to have the "default + user controlled expanded" as a persisted state and the "filter expanded" as a derived memo. But then I had trouble covering 'User can still collapse these parents (while the filter is active).'
Basically the filter overrides the previous default + user decided state, but subsequent user actions can override the filter-induced state.

[paula-stacho]

Having both of them in the state, they can override each other, otherwise I'm not sure how to support this

[gribnoysup]

I'm not sure what exactly you mean here by "having both of them in the state", if you mean redux store, I don't see why this would be required, but we can keep it there, yeah, especially as in some cases expanding causes a side-effect requiring a thunk action.

Generally speaking, yes, this approach would require keeping filter and "user expanded" state in one place, as this is purely UI state, useReducer seems like a good fit for that. I made a small demo to illustrate. The "filter overrides the previous default + user decided state" is not a state, it's just derived from filter, I think that might be why you have so much extra code to calculate this in various places and persist to state through hard to reason about extra states like "tempExpanded".

If this still doesn't make sense, feel free to ignore, my goal with this feedback is to try to keep the unnecessary code to the minimum, in the current implementation it will be really hard to change anything about filtering behavior without the need to touch a lot of intertwined code in multiple places that seems to me can be completely avoided otherwise.

[paula-stacho]

I didn't mean redux specifically, I just meant that they need to be in the state as opposed to e.g. one of them being derived
I like the useReducer approach and will try if this would make the code more readable (if not simpler).
let me explain the "tempExpanded": once we have them both in a single state, we run into another problem - how to persist the user choices (from before and during filtering)? my solution was to add a third state - "tempExpanded", which gets reverted afterwards.
I can definitely see how the code and the whole solution is not very easy to wrap one's head around. there are several factors which contribute to the complexity (the 3 levels, different default state of connections and databases and the "lasting default + user-choice" vs "temporary filter-induced" expanding).
I'll try to play around and see if I can make the code clearer.

[paula-stacho]

we could avoid "tempExpanded" if we're okay with just reverting to the "pre-filter" expanded state after the filter ended. then we could just make a copy and return to that - not perform a cleanup. it is not the current behaviour, but it seems good enough to me and it would make things simpler. I will try to find some other examples in the world and see what is the standard.

[gribnoysup]

User expanded and user filter input should be in the state, yes, they are state that we need to control, we are not in diagreement here. But the final filtered list and the final expanded state are derived values based on user expanded and user filter and the list itself.

We can avoid it by deriving the value and it would preserve the user input before, after, and during using the filter. I'm not sure if you had a chance to look at my example, but it literally does it without keeping any extra state for the expanded that is not the user input. Obviously the tree I'm using there is simpler, I understand that, but there is really no difference in the logic no matter how much nesting you'll introduce, it mostly changes how exactly you check that children are matching the filter.

Deriving this state by resetting the user expanded choice in some cases is also pretty close to what was already happening in the app, but removed in this PR

[paula-stacho]

I did have a look at your example, I think however that you've changed it since then - now it's not resetting everything to collapsed after the filter is cleared.

I am already moving things around with the reducer, having the actions listed like that will surely be more readable than having stuff happening in the effects. Afterwards if it still looks confusing, I will get rid of the tempExpanded.