From f27384ac0e6a461e6ecf0eaeedf5add54f34a584 Mon Sep 17 00:00:00 2001
From: rashidakanchwala <37628668+rashidakanchwala@users.noreply.github.com>
Date: Fri, 15 Nov 2024 13:23:43 +0000
Subject: [PATCH] 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.
---
.../node-list-tree/node-list-tree.js | 36 ++-------
src/components/nodes-panel/nodes-panel.js | 6 +-
.../nodes-panel/nodes-panel.test.js | 4 -
.../nodes-panel/utils/node-list-context.js | 8 +-
src/selectors/disabled.js | 81 +++++++++++++------
5 files changed, 71 insertions(+), 64 deletions(-)
diff --git a/src/components/node-list-tree/node-list-tree.js b/src/components/node-list-tree/node-list-tree.js
index 6aaab1cdb0..fdb5df54d3 100644
--- 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
index 8c845108d7..8a8957cf61 100644
--- 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 }) => {
{
onToggleHoveredFocusMode={handleToggleHoveredFocusMode}
searchValue={searchValue}
slicedPipeline={slicedPipeline}
+ nodesDisabledViaModularPipeline={
+ nodesDisabledViaModularPipeline
+ }
/>
diff --git a/src/components/nodes-panel/nodes-panel.test.js b/src/components/nodes-panel/nodes-panel.test.js
index a136fbc74e..8d56c56adc 100644
--- 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
index f2adc4afd5..a7c4cde3fc 100644
--- 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,7 +209,6 @@ export const NodeListContextProvider = ({ children }) => {
return (
{
modularPipelinesTree,
selectedNodes,
slicedPipeline,
+ nodesDisabledViaModularPipeline,
handleModularPipelineToggleExpanded: onToggleModularPipelineExpanded,
handleNodeListRowClicked,
handleNodeListRowChanged,
diff --git a/src/selectors/disabled.js b/src/selectors/disabled.js
index 2822be9711..f49441b9c3 100644
--- 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,11 +137,9 @@ export const getNodeDisabled = createSelector(
nodeDisabledNode,
nodeDisabledTag,
nodeDisabledPipeline,
+ nodesDisabledViaModularPipeline,
nodeType,
typeDisabled,
- nodeModularPipelines,
- modularPipelinesTree,
- focusedModularPipeline,
visibleSidebarNodes,
visibleModularPipelineInputsOutputs,
disabledModularPipeline,
@@ -100,9 +147,6 @@ export const getNodeDisabled = createSelector(
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);
})