Refactor/node list index

[Huongg]

Description

Original issue:
The index file had grown significantly, with numerous if/else statements due to shared functionality between node-list and filters.

I've now split the logic into two separate contexts: node-list-context, specifically for the node list, and filters-context, dedicated to filters. Each context now contains its own selectors, actions, and state, tailored for its specific purpose, along with related functionality like itemChanged and itemClicked. This has removed the need for shared logic, making the codebase more modular and maintainable.

Checklist

• Read the contributing guidelines
• Opened this PR as a 'Draft Pull Request' if it is work-in-progress
• Updated the documentation to reflect the code changes
• Added new entries to the RELEASE.md file
• Added tests to cover my changes

[rashidakanchwala]

There are selectors in this file that can be moved to selector folder

[rashidakanchwala]

I think some functions live in utils and others in selectors so maybe we can move it accordingly?

[Huongg]

i've now moved them all to be in the selectors and rename it to be selectors/filtered-node-list-items.js

[rashidakanchwala]

this can be moved to node-list-tree

[Huongg]

yup good point 👍

[Huongg]

actually now looking at the code, i think it makes more sense to do this at the node-list, then just pass down the modularPipelinesSearchResult to the node-list-tree as a prop

[rashidakanchwala]

the redux connection in this file can also be moved to the NodeListContextProvider , i suppose?

[Huongg]

yup it will be 👍

[rashidakanchwala]

I had a first pass, and I love it to so much! thanks @Huongg , i am also new to useContext, so reading about it. It makes it so much cleaner <3

[ravi-kumar-pilla]

Using context looks great. One question I have is does NodeList gets rendered everytime there is a change in AppContext whether or not NodeList depends on the change ? I see AppContext has NodeListContextProvider. A bit confused on why can't we have -

 <NodeListContextProvider>
      <NodeList faded={faded} />

Thank you

[Huongg]

Hey, yes, the NodeList component does re-render every time there's a change in AppContext. However, even without context when we just pass everything down as a props through index.js, it will re-render whenever its props change. Good point, though! We could use React.memo to optimise it, but that would be independent of useContext

[Huongg]

without wrapping it around AppContext it will look like this. I thought it looks nicer with just one AppContext. WDYT?

 <NodeListContextProvider>
     <FiltersContextProvider>
         <NodeList faded={faded} />
....

[ravi-kumar-pilla]

Sorry for the late response. For me AppContext seems more like a generic context, lets say if we want to pass theme of the app or something generic across app. I would rather have individual context or create something like below -

A group component:

<NodePanelContext>
    <NodePanel>
 <NodePanelContext>

<NodePanel>
     <PipelineDropDown/>
     <NodeSearch/>
     <NodeList/>
     <FilterList/>
 </NodePanel>

If possible try to have minimum re-renders to children inside NodePanel. For example if someone changes state of FilterList, PipelineDropDown or NodeSearch is not re-rendered . Thank you

[ravi-kumar-pilla]

[Nit] I know the PR is still in draft. If we want to have the components - SearchList, NodeListTree, Filters in this file, this file needs to be shifted to a UI section or needs to be renamed. As node-list.js suggests only the list of nodes we see in the pipeline-sidebar. Wdyt @Huongg

[rashidakanchwala]

Agree. Maybe we could rename as follows

Parent component could be called Node Explorer, and within that we have SearchList, NodeListTree, and Filters -- all of these could be top level components, i think both SearchList and Fitlers are already and NodeListTree can be too.

@Huongg , @ravi-kumar-pilla

[Huongg]

hey @rashidakanchwala would NodeListManager be a suitable name since it manages NodeListTree, SearchList, and Filters

[ravi-kumar-pilla]

Hi @Huongg , Great work. I had a brief look at the overall structure. As @rashidakanchwala mentioned, it looks more readable having context passed to children. I left few comments. My suggestion would be to pass only the required context to the children components to avoid re-rendering. Thank you 💯

[jitu5]

@Huongg When testing locally, I noticed a bug related to the filter. When you select any filter (try using tags), it gets updated in the URL. However, if you deselect the same filter, it does not get removed from the URL.

Please notice when I click on "companies" tag.

[Huongg]

thanks for spotting it, and walking me through the issue. @jitu5. I missed the function handleUrlParamsUpdateOnFilter when refactoring the code. I believe it should be working as usual now. Do let me know if its still not right from your side. I've also reported a separate issue we found on the call together #2186

[rashidakanchwala]

LGTM.

[jitu5]

@Huongg, I had a brief look at the PR, and it looks really good, especially the use of React context, even though I am still a bit new to it. I retested the filter issue, and it’s now fixed—thanks for that!

However, I am a bit confused about the component naming and the parent-child connections, as some new components have been introduced and some existing ones have been renamed from your previous PR. It would be really great if you could add an updated flowchart or block diagram to this PR.

Overall this is excellent work. 🎉

[ravi-kumar-pilla]

Hi @Huongg, I see all the big refactor and tons of work you have put in 🥇 . It would be helpful to review and revisit, if there is a flowchart explaining -

1. Grouped components
2. Parent components
3. Children components
4. Flow of data from parent <-> child - This can be a simple example of what happens if a pipeline dropdown is changed or someone searches in node search or someone filters
5. Flow of data if any from redux <-> components - This can be a simple example of what happens if a pipeline dropdown is changed or someone searches in node search or someone filters
6. Context providers
7. Any particular part of code you feel is the biggest change - For example use of ContextProviders
8. Component re-renders when NodesPanel state changes

Thank you

[Huongg]

hey @ravi-kumar-pilla and @jitu5 I've revised the diagram to reflect the updated component structure.

with details miro board: https://miro.com/app/board/uXjVLTeWe88=/

Here's an overview of the new structure:

1. nodes-panel: This serves as the parent component, importing the other three components—search-list, node-list-tree, and filters. It includes two context providers:

• node-list-context: Focused on the node list, handling selectors, actions, and state specific to nodes.
• filters-context: Dedicated to filters, with selectors, actions, and state for managing filter functionality. Both contexts support related functions like itemChanged and itemClicked.

2. node-list-tree: This renders the tree list of nodes and modular pipelines. This work is currently part of a separate ticket (#2177). Per @rashidakanchwala, this shouldn’t involve major changes to the existing components.

3. filters: Responsible for rendering the filters list.

4. Data Flow: The nodes-panel parent component manages everything, with logic split between the two contexts mentioned above. A shared state allows data to flow between the search input, node-list-tree, and filters.

5. The most significant update in this PR is the context setup, though two earlier PRs—#2143 and #2166—introduced other substantial changes as part of a phased refactor.

6. Components will re-render as the state changes. Let me know if there's a specific part you’d like more detail on.

Happy to get on a call if you want to further discuss about this?