Describe the bug

SearchListControl is a component that accepts a list of items and builds a list searchable through a text input of items. The list is hierarchical and collapsible. In such cases, it may happen that the list hierarchy is built wrongly due to the nature of the data.

For example, Attribute terms (e.g. “Blue”, “Yellow”) have their own id property which sometimes can overlap with their Taxonomy id (e.g. in the test case, “Blue” had the same id as the “Size” taxonomy), and the function building the tree representation for the SearchListControl would erroneously assign children to it.

To reproduce

Steps to reproduce the behavior:

0. Use https://github.com/nielslange/woo-test-environment to spin a new test environment (this is required because it is known to create overlapping ids).
1. Add a Products (Beta) block to your page.
2. Open the Inspector Controls and add a “Product Attributes” advanced filter.
3. Uncollapse “Color”.
4. Notice that “Blue” is erroneously marked as a collapsible item and selecting it would also select all the sizes terms (this is because “Blue” and “Size” share the same id).

Expected behavior

The component should build a correct tree representation of the list passed to it.

Video

Additional context

As I'm currently assigned to a different project, I've tried to quickly patch this bug in this PR. However, the patch quickly lead to a cascade of other problems, possibly adding an inordinate amount of tech debt. I'm sharing here what I have found out and directions to the next developer tackling this problem. These are the approaches I tried:

Approach 1: Differentiating between id and termId depending on the kind of object

This is the approach in the above-mentioned PR.

Unfortunately, that leads to having to compare both these keys every time id is normally used (for example, to see whether something is checked or not). This meant creating complex types (because an object EITHER had id or termId defined), and an incredible amount of guard clauses and weird comparisons.

If this patchy approach is to be continued, a refactoring of all the comparison functions should be done so that they can use some sort of reusable utility function.

Approach 2: Adding a _uid property so that it is normalized and conflicts between id and termId do not happen

Since ids can overlap, I thought of adding a _uid (thanks to @nielslange for the idea during our pairing session) before passing the list down, so that it could be used to disambiguate the two properties. This still requires changes to all the comparison functions, but it's a simple replacing id with _uid without complex logic.

However, the drawback of this is that it now requires a whole lot of changes upstream in the way attributes and other things from the SearchListControl are marked as selected.

Next steps?

Overall, both the above solutions could be viable, but I'm not satisfied with either of them.

The actual solution is not patching, but refactoring. The problem, IMHO, is the buildTermsTree function. Because it expects a flattened array, the overlapping IDs issue happens, as distinctions are lost. This was not caught in the unit tests.

Actually, in my implementation of the ProductAttributesTermsControl, we don’t even need the buildTermsTree function to make the tree, because my useProductAttributes hook already returns a nested object. I actually need to un-nest it, so that buildTermsTree can re-nest it and I can work with the SearchListControl component.

The solution, IMO, is to change this logic: we ideally don’t need a buildTermsTree at all, and we just pass already hierarchical data to the component. This means going through the code and find out who uses SearchListControl (for example, blocks like Filter By Attribute block or Featured Product/Category) and make sure that they pass the list in the correct shape. I think that theoretically we only need to change the blocks involving categories as they are the ones which can pass nested data, but more testing is required.

This should be helped somewhat by the fact that I migrated the component to TypeScript, so the expected shape should be clearer now.