Skip to content
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 deps filter crash bug by limiting tag grouping calculation to tag filters #1946

Closed
wants to merge 1 commit into from

Conversation

ibolton336
Copy link
Member

Addresses issue #1944 introduced by #1903

Currently the operation to build the tag category labels is very expensive & is causing maximum update depth exceeded errors when performing a filter operation in some cases.

Screenshot 2024-06-06 at 2 15 12 PM

This PR introduces a quick fix to limit the label creation operation by categoryKey === 'tags'. We should circle back on this & see if there is a better way to implement the category / name grouping.

Copy link

codecov bot commented Jun 6, 2024

Codecov Report

Attention: Patch coverage is 0% with 23 lines in your changes missing coverage. Please review.

Project coverage is 42.06%. Comparing base (b654645) to head (47a076e).
Report is 150 commits behind head on main.

Files Patch % Lines
...ponents/FilterToolbar/MultiselectFilterControl.tsx 0.00% 23 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1946      +/-   ##
==========================================
+ Coverage   39.20%   42.06%   +2.85%     
==========================================
  Files         146      163      +17     
  Lines        4857     5240     +383     
  Branches     1164     1311     +147     
==========================================
+ Hits         1904     2204     +300     
- Misses       2939     3020      +81     
- Partials       14       16       +2     
Flag Coverage Δ
client 42.06% <0.00%> (+2.85%) ⬆️
server ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sjd78 sjd78 requested review from rszwajko and sjd78 June 7, 2024 13:47
@rszwajko
Copy link
Collaborator

rszwajko commented Jun 7, 2024

@ibolton336 @sjd78
The root cause seem to be chip group name collision. Both "Language" filter and "Tags" filter create chip group with title "Language". After renaming one of them the problem is solved (see below).
However tag categories, in general, are editable. We might run into more conflicts. The solution might be using a prefix for chip groups created from tags categories i.e. Tags/Language. What do you think?

diff --git a/client/src/app/pages/dependencies/dependencies.tsx b/client/src/app/pages/dependencies/dependencies.tsx
index 97db65ce..166ff7aa 100644
--- a/client/src/app/pages/dependencies/dependencies.tsx
+++ b/client/src/app/pages/dependencies/dependencies.tsx
@@ -75,7 +75,7 @@ export const Dependencies: React.FC = () => {
       },
       {
         categoryKey: "provider",
-        title: t("terms.language"),
+        title: "LL",
         type: FilterType.search,
         filterGroup: "Dependency",
         placeholderText:

Screenshot from 2024-06-07 17-10-50

@rszwajko
Copy link
Collaborator

rszwajko commented Jun 8, 2024

@ibolton336 @sjd78
Further research revealed that the problem is in duplicated keys - ToolbarFilter derrives them from categoryName which in our filters usually comes from category.title.
I've created a draft PR with the fix ( #1949 ).

rszwajko added a commit that referenced this pull request Jun 10, 2024
Addresses issue #1944 introduced by #1903  (follow-up to #1946)

The MultiselectFilterControl component discovers groups from provided
data. Each group results in a new ToolbarFilter/ChipGroup pair. Before
this fix, the keys were based only on group names. In case of Language
filter it created a collision with Language group derived from Tags.
After this fix, the keys are built by concatenating category title and
group name. The category title is known at compile time and is expected
to be unique among all filters on the screen.

Reference-Url: #1944
Reference-Url: #1903
Reference-Url: #1946

Signed-off-by: Radoslaw Szwajkowski <[email protected]>
@ibolton336 ibolton336 closed this Jun 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants