Refactor on NodeListTree (#2177)

This is a smaller refactor. Previously, the logic for determining which nodes were disabled due to modular pipelines was duplicated in both the NodeListTree component and the getNodeDisabled selector. To improve maintainability and reduce redundancy, the getnodesDisabledViaModularPipeline logic was extracted and made into it's own selector. Now, this logic is shared and reused by both the NodeListTree component and the getNodeDisabled selector.
kedro-org · Nov 15, 2024 · f27384a · f27384a
1 parent aa6c8cb
commit f27384a
Show file tree

Hide file tree

Showing 5 changed files with 71 additions and 64 deletions.
diff --git a/src/components/node-list-tree/node-list-tree.js b/src/components/node-list-tree/node-list-tree.js
@@ -34,20 +34,6 @@ const StyledTreeView = styled(TreeView)({
   padding: '0 0 0 20px',
 });
 
-/**
- * Return whether the given modular pipeline ID is on focus mode path, i.e.
- * it's not the currently focused pipeline nor one of its children.
- * @param {String} focusModeID The currently focused modular pipeline ID.
- * @param {String} modularPipelineID The modular pipeline ID to check.
- * @return {Boolean} Whether the given modular pipeline ID is on focus mode path.
- */
-const isOnFocusedModePath = (focusModeID, modularPipelineID) => {
-  return (
-    modularPipelineID === focusModeID ||
-    modularPipelineID.startsWith(`${focusModeID}.`)
-  );
-};
-
 /**
  * Return the data of a modular pipeline to display as a row in the node list.
  * @param {Object} params
@@ -102,7 +88,7 @@ const getNodeRowData = (node, disabled, hoveredNode, selected, highlight) => {
     active: node.active || hoveredNode === node.id,
     selected,
     highlight,
-    faded: disabled || node.disabledNode,
+    faded: disabled || !checked,
     visible: !disabled && checked,
     checked,
     disabled,
@@ -121,38 +107,26 @@ const TreeListProvider = ({
   onItemClick,
   onNodeToggleExpanded,
   focusMode,
-  disabledModularPipeline,
   expanded,
   onToggleNodeSelected,
   slicedPipeline,
   isSlicingPipelineApplied,
+  nodesDisabledViaModularPipeline,
 }) => {
   // render a leaf node in the modular pipelines tree
   const renderLeafNode = (node) => {
     // As part of the slicing pipeline logic, child nodes not included in the sliced pipeline are assigned an empty data object.
     // Therefore, if a child node has an empty data object, it indicates it's not part of the slicing pipeline and should not be rendered.
-    if (Object.keys(node).length === 0) {
+    if (!node || Object.keys(node).length === 0) {
       return null;
     }
 
     const disabled =
       node.disabledTag ||
       node.disabledType ||
-      (focusMode &&
-        !node.modularPipelines
-          .map((modularPipelineID) =>
-            isOnFocusedModePath(focusMode.id, modularPipelineID)
-          )
-          .some(Boolean)) ||
-      (node.modularPipelines &&
-        node.modularPipelines
-          .map(
-            (modularPipelineID) => disabledModularPipeline[modularPipelineID]
-          )
-          .some(Boolean));
+      nodesDisabledViaModularPipeline[node.id];
 
     const selected = nodeSelected[node.id];
-
     const highlight = slicedPipeline.includes(node.id);
     const data = getNodeRowData(
       node,
@@ -228,7 +202,7 @@ const TreeListProvider = ({
     const data = getModularPipelineRowData({
       ...node,
       focusModeIcon,
-      disabled: focusMode && !isOnFocusedModePath(focusMode.id, node.id),
+      disabled: nodesDisabledViaModularPipeline[node.id],
       focused: isFocusedModularPipeline,
       highlight,
     });

diff --git a/src/components/nodes-panel/nodes-panel.js b/src/components/nodes-panel/nodes-panel.js
@@ -30,7 +30,6 @@ const NodesPanel = ({ faded }) => {
 
   const {
     hoveredNode,
-    disabledModularPipeline,
     expanded,
     focusMode,
     handleItemMouseEnter,
@@ -44,6 +43,7 @@ const NodesPanel = ({ faded }) => {
     modularPipelinesTree,
     selectedNodes,
     slicedPipeline,
+    nodesDisabledViaModularPipeline,
   } = useContext(NodeListContext);
 
   const modularPipelinesSearchResult = searchValue
@@ -87,7 +87,6 @@ const NodesPanel = ({ faded }) => {
                 <div className="pipeline-nodelist-section">
                   <NodeListTree
                     hoveredNode={hoveredNode}
-                    disabledModularPipeline={disabledModularPipeline}
                     expanded={expanded}
                     faded={faded}
                     focusMode={focusMode}
@@ -103,6 +102,9 @@ const NodesPanel = ({ faded }) => {
                     onToggleHoveredFocusMode={handleToggleHoveredFocusMode}
                     searchValue={searchValue}
                     slicedPipeline={slicedPipeline}
+                    nodesDisabledViaModularPipeline={
+                      nodesDisabledViaModularPipeline
+                    }
                   />
                 </div>
               </Scrollbars>

diff --git a/src/components/nodes-panel/nodes-panel.test.js b/src/components/nodes-panel/nodes-panel.test.js
@@ -251,10 +251,6 @@ describe('NodesPanel', () => {
         .find('.node-list-tree-item-row')
         .map((row) => [row.prop('title'), !row.hasClass('row--disabled')]);
 
-    const elementsEnabled = (wrapper) => {
-      return elements(wrapper).filter(([_, enabled]) => enabled);
-    };
-
     const tagItem = (wrapper) => wrapper.find('.filters-section--type-tag');
 
     const partialIcon = (wrapper) =>

diff --git a/src/components/nodes-panel/utils/node-list-context.js b/src/components/nodes-panel/utils/node-list-context.js
@@ -23,12 +23,16 @@ import {
   toggleNodesDisabled,
 } from '../../../actions/nodes';
 import { resetSlicePipeline } from '../../../actions/slice';
+import { getnodesDisabledViaModularPipeline } from '../../../selectors/disabled';
 
 // Custom hook to group useSelector calls
 const useNodeListContextSelector = () => {
   const dispatch = useDispatch();
   const hoveredNode = useSelector((state) => state.node.hovered);
   const selectedNodes = useSelector(getNodeSelected);
+  const nodesDisabledViaModularPipeline = useSelector(
+    getnodesDisabledViaModularPipeline
+  );
   const expanded = useSelector((state) => state.modularPipeline.expanded);
   const slicedPipeline = useSelector(getSlicedPipeline);
   const modularPipelinesTree = useSelector(getModularPipelinesTree);
@@ -75,6 +79,7 @@ const useNodeListContextSelector = () => {
     modularPipelinesTree,
     selectedNodes,
     slicedPipeline,
+    nodesDisabledViaModularPipeline,
     onResetSlicePipeline,
     onToggleFocusMode,
     onToggleHoveredFocusMode,
@@ -99,6 +104,7 @@ export const NodeListContextProvider = ({ children }) => {
     modularPipelinesTree,
     selectedNodes,
     slicedPipeline,
+    nodesDisabledViaModularPipeline,
     onResetSlicePipeline,
     onToggleFocusMode,
     onToggleHoveredFocusMode,
@@ -203,14 +209,14 @@ export const NodeListContextProvider = ({ children }) => {
   return (
     <NodeListContext.Provider
       value={{
-        disabledModularPipeline,
         expanded,
         focusMode,
         hoveredNode,
         isSlicingPipelineApplied,
         modularPipelinesTree,
         selectedNodes,
         slicedPipeline,
+        nodesDisabledViaModularPipeline,
         handleModularPipelineToggleExpanded: onToggleModularPipelineExpanded,
         handleNodeListRowClicked,
         handleNodeListRowChanged,

diff --git a/src/selectors/disabled.js b/src/selectors/disabled.js
@@ -63,6 +63,57 @@ export const getNodeDisabledTag = createSelector(
     })
 );
 
+/**
+ * Determine if a node is disabled via disabled modular pipeline or focused modular pipeline.
+ */
+export const getnodesDisabledViaModularPipeline = createSelector(
+  [
+    getNodeIDs,
+    getNodeType,
+    getNodeModularPipelines,
+    getModularPipelinesTree,
+    getFocusedModularPipeline,
+    getDisabledModularPipeline,
+  ],
+  (
+    nodeIDs,
+    nodeType,
+    nodeModularPipelines,
+    modularPipelinesTree,
+    focusedModularPipeline,
+    disabledModularPipeline
+  ) =>
+    arrayToObject(nodeIDs, (id) => {
+      let isDisabledViaModularPipeline =
+        disabledModularPipeline[nodeModularPipelines[id]];
+
+      let isDisabledViaFocusedModularPipeline = false;
+
+      if (focusedModularPipeline) {
+        const inputOutputNodeIDs = [
+          ...modularPipelinesTree[focusedModularPipeline.id].inputs,
+          ...modularPipelinesTree[focusedModularPipeline.id].outputs,
+        ];
+
+        if (nodeType[id] === 'modularPipeline') {
+          return (
+            id !== focusedModularPipeline.id &&
+            !id.startsWith(`${focusedModularPipeline.id}.`)
+          );
+        }
+
+        isDisabledViaFocusedModularPipeline =
+          !nodeModularPipelines[id].includes(focusedModularPipeline.id) &&
+          !inputOutputNodeIDs.includes(id);
+      }
+
+      return [
+        isDisabledViaFocusedModularPipeline,
+        isDisabledViaModularPipeline,
+      ].some(Boolean);
+    })
+);
+
 /**
  * Set disabled status if the node is specifically hidden, and/or via a tag/view/type/modularPipeline
  */
@@ -72,11 +123,9 @@ export const getNodeDisabled = createSelector(
     getNodeDisabledNode,
     getNodeDisabledTag,
     getNodeDisabledPipeline,
+    getnodesDisabledViaModularPipeline,
     getNodeType,
     getNodeTypeDisabled,
-    getNodeModularPipelines,
-    getModularPipelinesTree,
-    getFocusedModularPipeline,
     getVisibleSidebarNodes,
     getVisibleModularPipelineInputsOutputs,
     getDisabledModularPipeline,
@@ -88,21 +137,16 @@ export const getNodeDisabled = createSelector(
     nodeDisabledNode,
     nodeDisabledTag,
     nodeDisabledPipeline,
+    nodesDisabledViaModularPipeline,
     nodeType,
     typeDisabled,
-    nodeModularPipelines,
-    modularPipelinesTree,
-    focusedModularPipeline,
     visibleSidebarNodes,
     visibleModularPipelineInputsOutputs,
     disabledModularPipeline,
     slicedPipeline,
     isSliceApplied
   ) =>
     arrayToObject(nodeIDs, (id) => {
-      let isDisabledViaModularPipeline =
-        disabledModularPipeline[nodeModularPipelines[id]];
-
       let isDisabledViaSlicedPipeline = false;
       if (isSliceApplied && slicedPipeline.length > 0) {
         isDisabledViaSlicedPipeline = !slicedPipeline.includes(id);
@@ -112,22 +156,8 @@ export const getNodeDisabled = createSelector(
         !visibleSidebarNodes[id] &&
         !visibleModularPipelineInputsOutputs.has(id);
 
-      let isDisabledViaFocusedModularPipeline = false;
-      if (focusedModularPipeline) {
-        const inputOutputNodeIDs = [
-          ...modularPipelinesTree[focusedModularPipeline.id].inputs,
-          ...modularPipelinesTree[focusedModularPipeline.id].outputs,
-        ];
-        if (nodeType[id] === 'modularPipeline') {
-          isDisabledViaFocusedModularPipeline =
-            id !== focusedModularPipeline.id &&
-            !id.startsWith(`${focusedModularPipeline.id}.`);
-        } else {
-          isDisabledViaFocusedModularPipeline =
-            !nodeModularPipelines[id].includes(focusedModularPipeline.id) &&
-            !inputOutputNodeIDs.includes(id);
-        }
-      }
+      const isDisabledViaModularPipeline = nodesDisabledViaModularPipeline[id];
+
       return [
         nodeDisabledNode[id],
         nodeDisabledTag[id],
@@ -136,7 +166,6 @@ export const getNodeDisabled = createSelector(
         typeDisabled[nodeType[id]],
         isDisabledViaSidebar,
         isDisabledViaModularPipeline,
-        isDisabledViaFocusedModularPipeline,
         isDisabledViaSlicedPipeline,
       ].some(Boolean);
     })