feat(recordings): more tools for filtering by labels

[tthvo]

Fixes #79
Fixes #488
Fixes #441
Related to #486
Fixes #440 (closing parent)

Improvements:

• Refactor filters to their own source files.
• Users should be able to delete all filters in the same group/category (More convenient?).
• Labels should be clickable for users to select matching recordings. Clicking on selected labels will deselect them.
• Filtered labels should be highlighted (i.e. blue).
• Filter states (i.e. selected category and filters) should persist between:
  • Route changes (i.e. Redux)
  • Page reloads (local state unless disabled).
• Clickable labels should be styled for hover action.
• Recording state filter should allow toggle.
• Timepicker needs to render correctly (i.e. spacings are misaligned). Appending the popever menu to component higher in the tree solved the issue.
• Date and time selection must sync (currently changing date does not reflect currently selected time).
• Row checkboxes should update with recording filters (i.e. find alternative to just list index).
• Labels in filter should be sorted for easier selection (essentially grouped together based on key).
• Selected names/labels should not appear in selection options.
• Optimize subscriptions to message channels (i.e duplicate parent and child sub causes unnecessary rebuilds)
• Toolbar rebuilding should not always cause DOM updates (i.e. extract as a component instead of inner func).
• Optimize context.settings calls with React.useMemo.

Test:

• Added unit tests for recording search/filters.
  • Separate tests for each filter.
  • RecordingFilters as a whole (no snapshots, see comment)
  • LabelCells
  • BulkEditLabels
  • Active/Archived Table updates (no snapshots, see comment)
• Use historyMemory instead of mock.
• Added missing clean up calls after each tests.
• Added wrapper for React Testing Library render to render with Context, Route, redux.Provider.

[tthvo]

Any thoughts? I will add unit tests based on these new behaviours :D

For the Category of the currently added filter should remain., there is a case where the user selects the category but does not add the filter yet and a new notification comes, the dropdown menu will not reflect the currently selected category. A quick fix to trigger parent rebuilds every time the category is chosen but I am thinking it becomes too many unnecessary rebuilds.

[andrewazores]

I haven't looked at the implementation but just by testing it out by hand, I'm really excited to see this. It looks and feels fantastic.

For the Category of the currently added filter should remain., there is a case where the user selects the category but does not add the filter yet and a new notification comes, the dropdown menu will not reflect the currently selected category. A quick fix to trigger parent rebuilds every time the category is chosen but I am thinking it becomes too many unnecessary rebuilds.

To check my understanding, say I am using the Name filter. I select that and click the dropdown, which displays a selection of recording names. If a new recording appears now, perhaps due to an Automated Rule, then the dropdown contents are not updated, so I have unsynced information between the dropdown list and the table I can see behind it. Is that what you mean?

[tthvo]

To check my understanding, say I am using the Name filter. I select that and click the dropdown, which displays a selection of recording names. If a new recording appears now, perhaps due to an Automated Rule, then the dropdown contents are not updated, so I have unsynced information between the dropdown list and the table I can see behind it. Is that what you mean?

Oh sorry actually the issue is, as in the situation above, instead we use Label filter (any other than Name) for example, then when a recording appears now, the dropdown actually changes back to Name abruptly.

[andrewazores]

To check my understanding, say I am using the Name filter. I select that and click the dropdown, which displays a selection of recording names. If a new recording appears now, perhaps due to an Automated Rule, then the dropdown contents are not updated, so I have unsynced information between the dropdown list and the table I can see behind it. Is that what you mean?

Oh sorry actually the issue is, as in the situation above, instead we use Label filter (any other than Name) for example, then when a recording appears now, the dropdown actually changes back to Name abruptly.

Ah, I see. I suppose that's because of how the component relationships and lifecycles are set up - updating the content of the table causes the toolbar to also be re-rendered and we lose that selection state? That sounds like it matches what you said with A quick fix to trigger parent rebuilds every time the category is chosen but I am thinking it becomes too many unnecessary rebuilds.

[tthvo]

Yess exactly the parent table now holds the state for currently selected category of the filters. I will try to find a better way to resolve this as rebuilding with large number of recordings for only a change in category dropdown is not ideal.

[tthvo]

I played around with Redux and finally got some new refinements to our recording filters. Since last time, I added:

• Recording filters for each target are persistent across route changes and page reload.
• Filter for recording state now allows toggling as it uses checkboxes.
• Datetime filter layout is fixed (no longer misaligned and cut off due to overlapping).
• A nicer hover styling for clickable labels.

Tests are still failing everywhere with addition of Redux. I will fix them after. Any thoughts @andrewazores @maxcao13?

[andrewazores]

This looks great. I've looked into Redux before but never truly used it, so my review of some parts of the implementation is very superficial, but things look good. Testing it out manually this also looks and feels excellent.

[tthvo]

Great :D I will fix and add tests for unit tests and mark it ready when done.

[maxcao13]

It may be redundant to have a "Clear all filters" button as well as the x which does it as well. Not sure.

[maxcao13]

2022-09-12.13-03-27.mp4

Small bug with the way that the checkboxes are using state with the rowIndexes.

[tthvo]

It may be redundant to have a "Clear all filters" button as well as the x which does it as well. Not sure.

Clear all filters will clear all filters while the x in chip group just clear filters in a specific group :D I think we need both for flexibility.

[tthvo]

2022-09-12.13-03-27.mp4

Small bug with the way that the checkboxes are using state with the rowIndexes.

Ahh right! I think the selectAll checkbox needs updating along with filters. I will look into and fix it.

[maxcao13]

The bug is fixed and works well!

Just a couple of small things:

1. Maybe it might be better for the user if the dropdown for Label filters greys out/disables any filters that are already applied? Just a strictly visual improvement.
2. This should probably be a future feature in a future PR, but something that would be cool is "strict" filters? For example, the recording MUST have these labels/template otherwise it does not show. Right now I think the filters are just a union function, implementing an intersection would be cool as well!

[andrewazores]

The original #441 issue does describe the union/intersection filter application :-) (as OR and AND)

[tthvo]

Maybe it might be better for the user if the dropdown for Label filters greys out/disables any filters that are already applied?

I think we could remove any "already selected" options for Name and Label Filters then.

This should probably be a future feature in a future PR, but something that would be cool is "strict" filters? For example, the recording MUST have these labels/template otherwise it does not show. Right now I think the filters are just a union function, implementing an intersection would be cool as well!

That would be super nice :D Probably a little tweak from Patternfly guideline tho as said here:

https://www.patternfly.org/v4/guidelines/filters/#filter-chip

Filters between attribute categories should be combined with a boolean “AND” operator. Filters within a category are shown grouped together and should be combined with a boolean “OR” operator.

[tthvo]

The original #441 issue does describe the union/intersection filter application :-) (as OR and AND)

I guess we follow the guideline for now? https://www.patternfly.org/v4/guidelines/filters/#filter-chip

[andrewazores]

Sure, that sounds good.

[maxcao13]

Works well! I don't see any other problems, so I'll approve when tests are fixed. Good work!

[tthvo]

I am trying to work around this issue with umounted components:

Warning: Can't perform a React state update on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in the componentWillUnmount method.
    at Select (/home/thvo/workspace/cryostat-web/node_modules/@patternfly/react-core/dist/js/components/Select/Select.js:33:5)
    at NameFilter (/home/thvo/workspace/cryostat-web/src/app/Recordings/Filters/NameFilter.tsx:50:37)

I think the issue is:

• Patternfly Select component when calling onToggle also calls setState for its own component: https://github.com/patternfly/patternfly-react/blob/a61551a4c549a595f39839ce3ce8cdabd70717dd/packages/react-core/src/components/Select/Select.tsx#L371
• That passed toggle callback calls setIsOpen to update our filters NameFilter and LabelFilter:

  cryostat-web/src/app/Recordings/NameFilter.tsx

  Line 79 in b355296

  onToggle={setIsOpen}

• This causes *Filter to be unmounted and updates on unmounted child Select[0] as it tries above.
• Also, the onToggle is called twice, which again causes updates on unmounted parent NameFilter or LabelFilter (unmounted due to previous call) [1].

For [1], I did add a ref to hold mounted state and avoid updates if unmounted. However, [0] is internal to patternfly. Any guide on this @andrewazores?

[tthvo]

Interestingly, the issue occurs only at the second time I select an option, not any subsequent selections.

[andrewazores]

Sorry, you've already caught up to my knowledge of React/Patternfly, so I don't have much insight to offer here :-)

Would it help if the components communicated by passing each other ex. Subject/Observable that could be used to pub/sub events, rather than passing direct callback functions? This is a technique I used a few years back on a different project using Angular but IIRC there were similar reasons for it.

[tthvo]

Sorry, you've already caught up to my knowledge of React/Patternfly, so I don't have much insight to offer here :-)

Would it help if the components communicated by passing each other ex. Subject/Observable that could be used to pub/sub events, rather than passing direct callback functions? This is a technique I used a few years back on a different project using Angular but IIRC there were similar reasons for it.

Oh right! Thanks for the tips! I will read and give it a try. Since its just a warning, I guess I will finish up the tests first.

[tthvo]

Sorry, just a final touch to make sure the filter is rendered correctly. The category dropdown now does not depend on the passed recording filters but rather the static array of allow keys based on recording types to ensure the above (i.e. state filter pops up in archived view) issue does not appear.

[maxcao13]

LGTM!