1.18 ports8 #7233
1.18 ports8 #7233
Conversation
* fixed search focus and keyboard/clipboard events separate selectionState reducer * fixed edges
📝 WalkthroughWalkthroughThe pull request introduces several modifications across multiple files in the codebase. Key changes include updates to the Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (17)
designer/client/src/reducers/graph/selectionState.ts (1)
9-17: Add null checks for action payloadsThe reducer doesn't validate action payloads before accessing them. This could lead to runtime errors if the actions are dispatched with missing or invalid data.
Consider adding null checks:
case "DELETE_NODES": - return xor(state, action.ids); + return action.ids ? xor(state, action.ids) : state; case "EXPAND_SELECTION": - return uniq([...state, ...action.nodeIds]); + return action.nodeIds ? uniq([...state, ...action.nodeIds]) : state; case "TOGGLE_SELECTION": - return xor(state, action.nodeIds); + return action.nodeIds ? xor(state, action.nodeIds) : state;designer/client/progressBar.js (1)
Line range hint
6-10: Consider enhancing progress bar configurability.The progress bar configuration could be improved by:
- Externalizing the progress bar characters to configuration, allowing easy customization
- Adding theme support for different visual styles
- Including fallback characters for environments with limited Unicode support
Example configuration structure:
const defaultConfig = { barLength: 40, longTime: 40, almostDone: 0.8, characters: { filled: "◼", empty: "_", fallback: { filled: "#", empty: "-" } } };designer/client/src/common/ClipboardUtils.ts (1)
Line range hint
31-50: Consider adding error handling and improving robustnessWhile the fallback implementation is necessary for HTTP contexts, consider these improvements:
- Add error handling for failed copy operations
- Consider using a more semantic class name for the hidden textarea
- Add timeout handling for the copy operation
const fallbackWriteText: WriteText = (text) => { - return new Promise((resolve) => { + return new Promise((resolve, reject) => { + const timeout = setTimeout(() => { + cleanup(); + reject(new Error("Copy operation timed out")); + }, 5000); + const el = document.createElement("textarea"); el.value = text; el.setAttribute("readonly", ""); el.className = css({ position: "absolute", left: "-9999px", }); + + const cleanup = () => { + clearTimeout(timeout); + document.body.removeChild(el); + }; + el.oncopy = (e) => { e.stopPropagation(); + cleanup(); resolve(text); }; + document.body.appendChild(el); el.select(); - document.execCommand("copy"); - document.body.removeChild(el); + try { + const success = document.execCommand("copy"); + if (!success) { + cleanup(); + reject(new Error("Copy command failed")); + } + } catch (error) { + cleanup(); + reject(error); + } }); };designer/client/src/components/themed/InputWithIcon.tsx (1)
77-80: Consider memoizing the click handler.The click handler function is recreated on every render. Consider memoizing it with
useCallbackto optimize performance.+const handleClear = useCallback(() => { + onClear(); + focus({ preventScroll: true }); +}, [onClear, focus]); -onClick={() => { - onClear(); - focus({ preventScroll: true }); -}} +onClick={handleClear}designer/client/src/reducers/settings.ts (2)
Line range hint
85-91: Add validation for processDefinitionData structureThe reducer directly spreads the incoming processDefinitionData without validating its structure. Consider adding runtime validation to ensure the expected shape, especially after the type change.
case "PROCESS_DEFINITION_DATA": { const { processDefinitionData } = action; + // Ensure the expected structure exists + const safeData = { + edgesForNodes: Array.isArray(processDefinitionData?.edgesForNodes) + ? processDefinitionData.edgesForNodes + : [], + ...processDefinitionData, + }; return { ...state, - processDefinitionData, + processDefinitionData: safeData, }; }
Line range hint
66-70: Address FIXME comment about class in storeThere's a TODO comment about removing class from store. This technical debt should be tracked and addressed.
Would you like me to create a GitHub issue to track this technical debt item? The issue would focus on removing class instances from the Redux store to maintain serializable state.
designer/client/src/containers/BindKeyboardShortcuts.tsx (1)
49-59: Consider extracting clipboard logic to a dedicated hookGiven the broader changes to clipboard functionality across multiple files (ClipboardUtils.ts, SelectionContextProvider.tsx), consider extracting these clipboard event handlers into a dedicated custom hook (e.g.,
useClipboardHandlers). This would:
- Improve separation of concerns
- Make the clipboard functionality more reusable
- Align with the ongoing clipboard-related refactoring
designer/client/src/actions/nk/node.ts (2)
36-38: Well-structured type changes for better node batch handling.The refactoring from single-node operations to batch operations with dedicated layout handling is a good architectural improvement. The separation of node data (
nodes), positioning (layout), and ID mapping (idMapping) provides better organization and flexibility.Consider documenting the structure of
idMappingand its role in node remapping, as this might not be immediately obvious to other developers.Also applies to: 50-51
147-157: Implementation aligns well with type changes, but needs documentation.The code correctly implements the new batch operation pattern and properly handles state access. The
allowIdChange=trueparameter inprepareNewNodesWithLayoutsuggests intentional ID remapping behavior.Consider adding a code comment explaining:
- The significance of
allowIdChange=true- The structure and purpose of the generated
idMapping- The relationship between
nodes,layout, andedges+ // allowIdChange=true enables ID remapping for imported nodes to avoid conflicts + // idMapping tracks the relationship between original and remapped node IDs const { nodes, layout, idMapping } = prepareNewNodesWithLayout(scenarioGraph.nodes, nodesWithPositions, true);designer/client/test/reducer-test.js (2)
112-127: Good refactor ofreduceAll, consider adding error handling.The new implementation properly supports both sync and async actions, following Redux patterns. However, consider adding error handling for better test reliability.
Consider adding try-catch blocks:
const dispatch = (action) => { + try { if (typeof action === "function") { action(dispatch, getState); } else { currentState = reducer(currentState, action); } + } catch (error) { + console.error('Action dispatch failed:', error); + throw error; + } };
Line range hint
172-188: Consider improving test isolation.While the tests are comprehensive, they share state through the
baseStateWithProcess. This could lead to flaky tests if the order of execution changes.Consider resetting the state before each test:
beforeEach(() => { // Reset to initial state currentState = reducer({}, { type: "@@INIT" }); currentState = reducer(currentState, { type: "DISPLAY_PROCESS", scenario: baseProcessState, }); });Also applies to: 193-209
docs/Changelog.md (1)
129-132: LGTM! Consider adding more details about the specific bugs fixed.The changelog entry is well-structured and properly references the PR. However, it would be helpful for users to know which specific clipboard, keyboard, and focus-related bugs were fixed.
Consider expanding the description to include:
-* [#7207](https://github.com/TouK/nussknacker/pull/7207) Fixed minor clipboard, keyboard and focus related bugs +* [#7207](https://github.com/TouK/nussknacker/pull/7207) Fixed minor clipboard, keyboard and focus related bugs: + * <specific bug 1> + * <specific bug 2> + * <specific bug 3>designer/client/src/components/graph/SelectionContextProvider.tsx (4)
1-1: Consider using built-inMath.mininstead oflodash'sminfunctionThe
minfunction fromlodashis imported, but JavaScript's built-inMath.mincan achieve the same result. This can reduce dependencies and improve performance.Apply this diff to replace
lodash'sminwithMath.min:- import { min } from "lodash"; + // Removed import of 'min' from 'lodash' ... - const minNodeX: number = min(selection.nodes.map((node) => node.additionalFields.layoutData.x)); - const minNodeY: number = min(selection.nodes.map((node) => node.additionalFields.layoutData.y)); + const nodePositionsX = selection.nodes.map((node) => node.additionalFields.layoutData.x); + const nodePositionsY = selection.nodes.map((node) => node.additionalFields.layoutData.y); + const minNodeX = Math.min(...nodePositionsX); + const minNodeY = Math.min(...nodePositionsY);
Line range hint
58-93: Ensure consistent return types inuseClipboardPermissionfunctionThe function
useClipboardPermissionis declared to returnboolean | string, but it may return an object ornulldue to thecontentvariable being set fromparse(text). This can lead to type inconsistencies and potential runtime errors.Apply this diff to correct the return type and ensure consistency:
- function useClipboardPermission(): boolean | string { + function useClipboardPermission(): boolean | object { ... - const [content, setContent] = useState(""); + const [content, setContent] = useState(null); ... - return state === "prompt" || content; + return state === "prompt" || !!content;Alternatively, adjust the implementation to return only
booleanvalues:- function useClipboardPermission(): boolean | string { + function useClipboardPermission(): boolean { ... - return state === "prompt" || content; + return state === "prompt" || Boolean(content);
Line range hint
95-107: Improve randomness logic incalculatePastedNodePositionAdding a random offset to node positions may lead to inconsistent user experiences and potential node overlap. Consider using a deterministic approach to offsetting pasted nodes.
Apply this diff to increment positions based on the number of pasted nodes:
- const random = Math.floor(Math.random() * 20) + 1; ... - const randomizedNodePosition = { - x: selectionLayoutNodePosition.x + random, - y: selectionLayoutNodePosition.y + random, - }; + const offset = index * 20; // 'index' is the position of the node in the array + const adjustedNodePosition = { + x: selectionLayoutNodePosition.x + offset, + y: selectionLayoutNodePosition.y + offset, + };
Line range hint
115-131: Handle potential exceptions when accessing clipboardIn the
pastefunction, there's a possibility of exceptions ifClipboardUtils.readTextfails. Ensure that errors are properly caught and handled to prevent the application from crashing.Apply this diff to enhance error handling:
try { const clipboardText = await ClipboardUtils.readText(event); parseInsertNodes(clipboardText); } catch (error) { console.error("Error reading from clipboard:", error); dispatch(error(t("userActions.paste.notAvailable", "Paste button is not available. Try Ctrl+V"))); }designer/client/src/reducers/graph/reducer.ts (1)
242-262: Improve variable naming in edge adjustment logicIn the
adjustedEdgescomputation, usingedgesas both the parameter and accumulator in thereducefunction can be confusing. Consider renaming the accumulator toaccumulatedEdgesorupdatedEdgesfor clarity.Apply this diff to improve variable names:
-const adjustedEdges = edgesWithValidIds.reduce((edges, edge) => { +const adjustedEdges = edgesWithValidIds.reduce((accumulatedEdges, edge) => { const fromNode = nodes.find((n) => n.id === edge.from); const toNode = nodes.find((n) => n.id === edge.to); const currentNodeEdges = NodeUtils.getOutputEdges(fromNode.id, accumulatedEdges); const newEdge = createEdge(fromNode, toNode, edge.edgeType, currentNodeEdges, processDefinitionData); - return edges.concat(newEdge); + return accumulatedEdges.concat(newEdge); }, state.scenario.scenarioGraph.edges);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
designer/client/src/reducers/graph/__snapshots__/utils.test.ts.snapis excluded by!**/*.snapdesigner/client/test/__snapshots__/reducer-test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (14)
designer/client/progressBar.js(1 hunks)designer/client/src/actions/actionTypes.ts(0 hunks)designer/client/src/actions/nk/node.ts(5 hunks)designer/client/src/common/ClipboardUtils.ts(2 hunks)designer/client/src/components/graph/SelectionContextProvider.tsx(3 hunks)designer/client/src/components/themed/InputWithIcon.tsx(1 hunks)designer/client/src/containers/BindKeyboardShortcuts.tsx(2 hunks)designer/client/src/reducers/graph/reducer.ts(3 hunks)designer/client/src/reducers/graph/selectionState.ts(1 hunks)designer/client/src/reducers/graph/utils.test.ts(1 hunks)designer/client/src/reducers/graph/utils.ts(2 hunks)designer/client/src/reducers/settings.ts(1 hunks)designer/client/test/reducer-test.js(5 hunks)docs/Changelog.md(1 hunks)
💤 Files with no reviewable changes (1)
- designer/client/src/actions/actionTypes.ts
🔇 Additional comments (20)
designer/client/src/reducers/graph/utils.test.ts (1)
7-7: Verify the snapshot contents.
The test relies on snapshot testing, but we should verify that the snapshot captures all essential aspects of the node transformation.
✅ Verification successful
The snapshot appears to be comprehensive and well-structured
The snapshot effectively captures all essential aspects of the node transformation including:
- Node ID mappings for uniqueness
- Layout positions for all nodes
- Complete node configurations including:
- Variable nodes with expressions and languages
- Union node with branch parameters and output configurations
- Additional fields like descriptions and layout data
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the snapshot file contents to ensure it captures the node transformation
# Look for the snapshot file
snapshot_file=$(fd -t f -e snap -e snapshot . | grep "utils.test")
if [ -n "$snapshot_file" ]; then
echo "Found snapshot file: $snapshot_file"
# Display the snapshot contents
cat "$snapshot_file"
else
echo "No snapshot file found"
fi
Length of output: 2846
designer/client/src/reducers/graph/selectionState.ts (2)
1-3: LGTM! Imports are appropriate for the functionality.
The imports are well-chosen:
xorfrom lodash provides efficient toggle functionalityuniqensures no duplicate selections- Local
Reducertype maintains type safety
6-9:
Remove handlers for deleted action types
According to the AI summary, NODE_ADDED and NODES_WITH_EDGES_ADDED action types have been removed from actionTypes.ts. These cases should be removed to prevent potential runtime issues.
Let's verify the action types removal:
Apply this diff to remove the obsolete cases:
- case "NODES_WITH_EDGES_ADDED":
- case "NODE_ADDED":
- return action.nodes.map((n) => n.id);designer/client/progressBar.js (1)
22-22: Verify the progress bar rendering in different terminal environments.
The change to simpler characters ("◼" and "_") improves compatibility, but we should ensure consistent rendering across different terminal emulators and environments.
Let's verify the character rendering:
✅ Verification successful
Let me gather more information about the progress bar implementation and its usage to better understand the context.
Let me check the actual usage of the progress bar in the codebase to understand where and how it's being used.
Based on the complete context, I can now provide the final response:
The progress bar character changes are safe and well-implemented
The change to use "◼" (filled square) and "_" (underscore) is actually a good improvement because:
- The code handles both CI and non-TTY environments separately
- The progress bar is used only in webpack build progress visualization
- The characters are wrapped in chalk for colorization, ensuring proper terminal rendering
- The implementation includes proper cursor handling and line clearing for TTY environments
The simpler characters will work reliably across different terminal environments, and the underscore provides better visual alignment as it sits on the baseline.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Test if the new characters render correctly in common terminal environments
# Note: This script helps identify potential rendering issues
# Test Unicode character display
echo "Testing Unicode character display..."
echo -e "New filled character: \u25FC"
echo -e "Previous filled character: \u25FC\uFE0E"
# Check if terminal supports Unicode
echo "Checking terminal Unicode support..."
echo $LANG
echo $LC_ALL
echo $LC_CTYPE
Length of output: 354
Script:
#!/bin/bash
# Search for usage of progressBar.js
rg -l "progressBar" --type js
# Look for any test files related to progress bar
fd -e test.js -e spec.js progress
# Check if there are any related configuration files
fd -e config.js progress
# Search for any documentation about terminal compatibility
fd README -t f
Length of output: 438
Script:
#!/bin/bash
# Search for imports or requires of progressBar
rg -A 2 "require.*progressBar|import.*progressBar"
# Search for actual usage of progress bar rendering
rg "getBar|progressBar" -A 2
# Check the file content to understand the full context
cat designer/client/progressBar.js
Length of output: 3175
designer/client/src/common/ClipboardUtils.ts (1)
20-22: LGTM: Well-defined TypeScript interface
The WriteText interface provides a clear contract for clipboard write operations, ensuring consistent typing across implementations.
designer/client/src/components/themed/InputWithIcon.tsx (1)
75-81: LGTM! Good UX improvement.
The changes improve user experience by maintaining input focus after clearing, while preventing unwanted scrolling during the operation.
designer/client/src/reducers/settings.ts (1)
57-59: Verify the impact of ProcessDefinitionData structure change
The change from an empty object to a structured type with edgesForNodes array could impact existing code that relies on the previous shape.
Let's verify the usage and potential breaking changes:
✅ Verification successful
The processDefinitionData.edgesForNodes change appears safe
Based on the search results, the only direct usage of processDefinitionData.edgesForNodes is in NodeUtils.ts, which aligns with the new structure. Other usages of processDefinitionData access different properties like components and componentGroups. No spread operations were found that might assume the old shape.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct usage of processDefinitionData
rg "processDefinitionData\." -A 3
# Search for spread operations that might assume the old shape
rg "\.\.\.\s*processDefinitionData" -A 3
# Search for type usage
ast-grep --pattern 'type $name = { $$$processDefinitionData$$$}'
Length of output: 4382
designer/client/src/containers/BindKeyboardShortcuts.tsx (1)
3-3: LGTM! Clean import organization.
The consolidated import statement improves code readability.
designer/client/src/actions/nk/node.ts (3)
1-11: LGTM! Imports are well-organized and necessary.
The new imports support the refactored node handling functionality, with flushSync specifically addressing React 18's automatic batching behavior.
125-138: Good handling of React 18's automatic batching.
The use of flushSync with a clear comment explaining its necessity for redux-undo compatibility is excellent.
However, let's verify that this change doesn't affect other parts of the codebase that might rely on the old single-node behavior.
✅ Verification successful
No compatibility issues with the flushSync changes
Based on the codebase analysis, the changes are safe because:
- The
nodeAddedaction creator is only used in two places:Graph.tsxcomponent's direct invocation- Test files that verify basic node addition functionality
- The reducer handlers for
NODE_ADDEDin bothreducer.tsandselectionState.tsalready expect and handle an array of nodes, making them compatible with the current implementation - The action is properly validated through
nodeValidationMiddlewarein the store configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct nodeAdded action dispatches that might expect old behavior
rg -l "dispatch\(nodeAdded\(" | xargs -I {} rg -A 5 "dispatch\(nodeAdded\(" {}
# Search for NODE_ADDED action handling in reducers
ast-grep --pattern 'case "NODE_ADDED": { $$$'
Length of output: 132
Script:
#!/bin/bash
# Search for NODE_ADDED action usage with more context
rg -A 5 "NODE_ADDED"
# Search for nodeAdded action creator usage
rg -A 5 "nodeAdded\("
# Look for reducer handling NODE_ADDED
rg -A 10 'case "NODE_ADDED":'
Length of output: 6398
Line range hint 1-157: Verify undo/redo functionality with these changes.
The refactoring to batch operations and the use of flushSync could impact undo/redo behavior.
✅ Verification successful
Let me gather more specific information about the batch grouping implementation and its interaction with undo/redo.
Let me gather more specific information about the batch grouping implementation and its interaction with redux-undo.
Let me check the BatchGroupBy implementation and its usage in the codebase.
Based on the gathered information, I can now provide a final response about the undo/redo functionality and batching changes:
The batching implementation is safe for undo/redo operations.
The code shows a well-structured approach to handling batched operations with redux-undo:
- The
BatchGroupByclass is properly integrated with redux-undo's groupBy functionality through itsinit()method - The
flushSyncusage in node actions is explicitly documented to prevent conflicts with React 18's automatic batching - The reducer is configured with proper filters and groupBy settings to maintain undo/redo state consistency
- All batch operations are properly wrapped with
startOrContinue()andend()calls
The changes in the file are safe and maintain the integrity of the undo/redo functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for undo/redo related code that might be affected
rg -l "redux-undo|ActionCreators\.undo|ActionCreators\.redo" | xargs -I {} rg -A 5 "redux-undo|ActionCreators\.undo|ActionCreators\.redo" {}
# Look for reducer configuration that might need updates
ast-grep --pattern 'undoable($$$)'
Length of output: 6440
Script:
#!/bin/bash
# Check the batch grouping implementation
rg -A 10 "class BatchGroupBy"
# Look for the reducer configuration where undo/redo is set up
rg -A 10 "undoable\("
# Check for any existing group-by configurations in redux-undo
rg -A 5 "groupBy:|groupBy ="
Length of output: 131965
Script:
#!/bin/bash
# Check the batch grouping implementation
rg -A 10 "batchGroupBy" designer/client/src/reducers/graph/
# Look for redux-undo configuration
rg -A 5 "undoable\(" designer/client/src/reducers/
# Check for any undo/redo related tests
rg -A 5 "test\(.*undo|test\(.*redo" designer/client/src/
Length of output: 2507
Script:
#!/bin/bash
# Check the BatchGroupBy class implementation
rg -A 20 "class BatchGroupBy" designer/client/src/reducers/graph/
# Check where batchGroupBy.startOrContinue is used
rg -A 3 "batchGroupBy\.startOrContinue" designer/client/src/
# Check where batchGroupBy.end is used
rg -A 3 "batchGroupBy\.end" designer/client/src/
Length of output: 5551
designer/client/test/reducer-test.js (3)
1-1: LGTM! Good practice using action creators.
The switch from direct action types to action creators improves maintainability and type safety.
157-159: LGTM! Good test coverage for single node addition.
The tests properly verify both node addition and position handling, with good coverage of ID uniqueness cases.
Also applies to: 164-167
1-1: Verify complete migration from action types.
Since the action types "NODE_ADDED" and "NODES_WITH_EDGES_ADDED" have been removed, let's verify all usages have been updated.
designer/client/src/reducers/graph/utils.ts (2)
55-76: prepareNewNodesWithLayout function updates are appropriate
The changes to prepareNewNodesWithLayout effectively handle the preparation of new nodes with unique IDs and layouts. The logic for generating uniqueIds and mapping them to nodes and layouts is well-implemented.
80-102: addNodesWithLayout function correctly merges state changes
The updates streamline the merging process of nodes, edges, and layouts into the existing state. The destructuring and default assignments ensure that the state remains consistent even if some properties are undefined.
designer/client/src/components/graph/SelectionContextProvider.tsx (2)
31-31: Verify the import path and usage of useInterval
Ensure that the useInterval hook is correctly imported from "../../containers/Interval" and that it functions as intended across all supported environments.
Run the following script to check for the correct export of useInterval:
✅ Verification successful
Import path and usage of useInterval is correct
The verification confirms that:
useIntervalis correctly exported fromdesigner/client/src/containers/Interval.tsx- The import path in
SelectionContextProvider.tsxmatches the export location - The usage follows the correct signature with
refreshTimeanddisabledoptions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that 'useInterval' is correctly exported from 'Interval.tsx' or 'Interval.ts'
# Search for the export statement of 'useInterval'
rg 'export function useInterval' -g 'src/containers/Interval.{ts,tsx}'
Length of output: 221
Script:
#!/bin/bash
# Let's first find the exact location of the Interval file
fd -e ts -e tsx "Interval" designer/client/src
# Then check its content for useInterval export
rg "export.*useInterval" designer/client/src
Length of output: 294
Script:
#!/bin/bash
# Let's verify the usage of useInterval in SelectionContextProvider
rg "useInterval" designer/client/src/components/graph/SelectionContextProvider.tsx -A 2
Length of output: 430
Line range hint 155-162: Verify user actions for nullability and dependencies
Ensure that the userActions object does not contain null functions, which could lead to runtime errors when actions are invoked without proper checks.
Run the following script to check for any null or undefined values in userActions:
designer/client/src/reducers/graph/reducer.ts (2)
236-239: Simplify node addition using addNodesWithLayout
The refactoring to use addNodesWithLayout directly enhances readability and maintains consistency in how nodes are added to the state. This change aligns with the updated structure of node data.
313-313: Integration of selectionState into the reducer
The addition of selectionState into the merged reducers ensures that selection-related state is managed alongside other graph state properties, promoting better state management.
| it("prepareNewNodesWithLayout should update union output expression parameter with an updated node name when new unique node ids created", () => { | ||
| const { scenarioGraph } = state.scenario; | ||
| expect(prepareNewNodesWithLayout(scenarioGraph.nodes, nodesWithPositions, true)).toMatchSnapshot(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing test coverage with additional test cases.
While snapshot testing is useful, the current test suite could benefit from:
- Additional test cases covering error conditions and edge cases
- Explicit assertions alongside snapshot testing to make the test's intentions clearer
- Tests verifying the behavior when unique node IDs are not required (passing
falseas the third parameter)
Consider adding these test cases:
describe("GraphUtils", () => {
it("prepareNewNodesWithLayout should update union output expression parameter with an updated node name when new unique node ids created", () => {
const { scenarioGraph } = state.scenario;
- expect(prepareNewNodesWithLayout(scenarioGraph.nodes, nodesWithPositions, true)).toMatchSnapshot();
+ const result = prepareNewNodesWithLayout(scenarioGraph.nodes, nodesWithPositions, true);
+ expect(result).toMatchSnapshot();
+ // Add explicit assertions for critical behavior
+ expect(result.some(node => node.id !== node.originalId)).toBeTruthy();
});
+
+ it("should handle empty nodes array", () => {
+ const { scenarioGraph } = state.scenario;
+ expect(prepareNewNodesWithLayout(scenarioGraph.nodes, [], true)).toMatchSnapshot();
+ });
+
+ it("should preserve original IDs when uniqueness is not required", () => {
+ const { scenarioGraph } = state.scenario;
+ const result = prepareNewNodesWithLayout(scenarioGraph.nodes, nodesWithPositions, false);
+ expect(result).toMatchSnapshot();
+ expect(result.every(node => node.id === node.originalId)).toBeTruthy();
+ });
});Committable suggestion skipped: line range outside the PR's diff.
| export const selectionState: Reducer<string[]> = (state = [], action) => { | ||
| switch (action.type) { | ||
| case "NODES_WITH_EDGES_ADDED": | ||
| case "NODE_ADDED": | ||
| return action.nodes.map((n) => n.id); | ||
| case "DELETE_NODES": | ||
| return xor(state, action.ids); | ||
| case "EXPAND_SELECTION": | ||
| return uniq([...state, ...action.nodeIds]); | ||
| case "TOGGLE_SELECTION": | ||
| return xor(state, action.nodeIds); | ||
| case "RESET_SELECTION": | ||
| return action.nodeIds ? action.nodeIds : []; | ||
| } | ||
| return state; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add TypeScript types for action payloads
The reducer lacks type safety for action payloads. Consider adding proper TypeScript types to prevent potential runtime errors.
Here's a suggested implementation:
type SelectionAction =
| { type: "DELETE_NODES"; ids: string[] }
| { type: "EXPAND_SELECTION"; nodeIds: string[] }
| { type: "TOGGLE_SELECTION"; nodeIds: string[] }
| { type: "RESET_SELECTION"; nodeIds?: string[] };
export const selectionState: Reducer<string[], SelectionAction> = (state = [], action) => {
switch (action.type) {
case "DELETE_NODES":
return xor(state, action.ids);
case "EXPAND_SELECTION":
return uniq([...state, ...action.nodeIds]);
case "TOGGLE_SELECTION":
return xor(state, action.nodeIds);
case "RESET_SELECTION":
return action.nodeIds ?? [];
}
return state;
};| export const writeText: WriteText = (text) => { | ||
| if (navigator.clipboard?.writeText) { | ||
| return navigator.clipboard.writeText(text).then(() => text); | ||
| } | ||
| return fallbackWriteText(text); | ||
| }; |
There was a problem hiding this comment.
💡 Codebase verification
Add browser compatibility tests for clipboard functionality
The implementation has proper feature detection and fallback handling, but there are no dedicated tests found for the clipboard functionality. While the code structure is sound, consider:
- Adding unit tests to verify the feature detection logic
- Including tests for both modern and fallback clipboard implementations
- Testing error scenarios in both implementations
🔗 Analysis chain
Verify browser compatibility testing
The implementation looks good with proper feature detection and fallback handling. However, ensure that browser compatibility testing is in place.
Let me check the babel configuration to understand the browser compatibility settings.
Let me check for any browser compatibility configurations in package.json as browserslist is commonly defined there.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for browser compatibility testing setup
# Look for browser compatibility tests
rg -l "writeText.*test" --type ts
rg -l "clipboard.*test" --type ts
# Check for browser compatibility configuration
fd -e json -e js "browserslist|babel\.config"
Length of output: 153
Script:
#!/bin/bash
# Check babel configuration for browser targets
cat designer/client/babel.config.json
# Look for any test files related to clipboard functionality
fd -e test.ts -e test.tsx -e spec.ts -e spec.tsx . designer/client/src/common/
Length of output: 1667
Script:
#!/bin/bash
# Check package.json for browserslist configuration
cat designer/client/package.json | jq '.browserslist'
# Also check for any clipboard-related test files in the entire project
fd -e test.ts -e test.tsx -e spec.ts -e spec.tsx . | rg -i "clipboard"
Length of output: 129
| copy: (event) => { | ||
| if (isInputEvent(event)) return; | ||
| userActions.copy ? eventWithStatistics({ selector: EventTrackingSelector.CopyNode }, userActions.copy(event)) : null; | ||
| }, | ||
| paste: (event) => { | ||
| userActions.paste ? eventWithStatistics({ selector: EventTrackingSelector.PasteNode }, userActions.paste(event)) : null; | ||
| }, | ||
| cut: (event) => { | ||
| if (isInputEvent(event)) return; | ||
| userActions.cut ? eventWithStatistics({ selector: EventTrackingSelector.CutNode }, userActions.cut(event)) : null; | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Several improvements needed in clipboard event handlers
- The paste handler is missing an input event check, unlike copy and cut.
- The conditional expressions can be simplified.
- Consider preventing default behavior to avoid duplicate clipboard operations.
Here's a suggested improvement:
copy: (event) => {
if (isInputEvent(event)) return;
- userActions.copy ? eventWithStatistics({ selector: EventTrackingSelector.CopyNode }, userActions.copy(event)) : null;
+ event.preventDefault();
+ userActions.copy?.(event) && eventWithStatistics({ selector: EventTrackingSelector.CopyNode }, true);
},
paste: (event) => {
+ if (isInputEvent(event)) return;
- userActions.paste ? eventWithStatistics({ selector: EventTrackingSelector.PasteNode }, userActions.paste(event)) : null;
+ event.preventDefault();
+ userActions.paste?.(event) && eventWithStatistics({ selector: EventTrackingSelector.PasteNode }, true);
},
cut: (event) => {
if (isInputEvent(event)) return;
- userActions.cut ? eventWithStatistics({ selector: EventTrackingSelector.CutNode }, userActions.cut(event)) : null;
+ event.preventDefault();
+ userActions.cut?.(event) && eventWithStatistics({ selector: EventTrackingSelector.CutNode }, true);
},This refactor:
- Adds input event check for paste operations
- Simplifies the conditional logic using optional chaining
- Prevents default browser behavior
- Makes the event tracking more consistent
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| copy: (event) => { | |
| if (isInputEvent(event)) return; | |
| userActions.copy ? eventWithStatistics({ selector: EventTrackingSelector.CopyNode }, userActions.copy(event)) : null; | |
| }, | |
| paste: (event) => { | |
| userActions.paste ? eventWithStatistics({ selector: EventTrackingSelector.PasteNode }, userActions.paste(event)) : null; | |
| }, | |
| cut: (event) => { | |
| if (isInputEvent(event)) return; | |
| userActions.cut ? eventWithStatistics({ selector: EventTrackingSelector.CutNode }, userActions.cut(event)) : null; | |
| }, | |
| copy: (event) => { | |
| if (isInputEvent(event)) return; | |
| event.preventDefault(); | |
| userActions.copy?.(event) && eventWithStatistics({ selector: EventTrackingSelector.CopyNode }, true); | |
| }, | |
| paste: (event) => { | |
| if (isInputEvent(event)) return; | |
| event.preventDefault(); | |
| userActions.paste?.(event) && eventWithStatistics({ selector: EventTrackingSelector.PasteNode }, true); | |
| }, | |
| cut: (event) => { | |
| if (isInputEvent(event)) return; | |
| event.preventDefault(); | |
| userActions.cut?.(event) && eventWithStatistics({ selector: EventTrackingSelector.CutNode }, true); | |
| }, |
| nodes: zipWith(newNodes, uniqueIds, (node, id) => ({ | ||
| ...node, | ||
| id, | ||
| branchParameters: adjustBranchParameters(node.branchParameters, uniqueIds), | ||
| })), |
There was a problem hiding this comment.
Pass idMapping to adjustBranchParameters for accurate ID mapping
Since idMapping provides a direct mapping from original IDs to unique IDs, passing it to adjustBranchParameters ensures precise ID replacements for branch parameters.
Apply this diff to fix the issue:
nodes: zipWith(newNodes, uniqueIds, (node, id) => ({
...node,
id,
- branchParameters: adjustBranchParameters(node.branchParameters, uniqueIds),
+ branchParameters: adjustBranchParameters(node.branchParameters, idMapping),
})),Committable suggestion skipped: line range outside the PR's diff.
| function adjustBranchParameters(branchParameters: BranchParams[], uniqueIds: string[]) { | ||
| return branchParameters?.map(({ branchId, ...branchParameter }: BranchParams) => ({ | ||
| ...branchParameter, | ||
| branchId: uniqueIds.find((uniqueId) => uniqueId.includes(branchId)), | ||
| })); | ||
| } |
There was a problem hiding this comment.
Use precise ID mapping instead of includes in adjustBranchParameters
Using includes for ID matching can lead to incorrect mappings if multiple IDs contain similar substrings. It's safer to use idMapping for exact ID replacements to ensure accurate branch parameter adjustments.
Apply this diff to fix the issue:
-function adjustBranchParameters(branchParameters: BranchParams[], uniqueIds: string[]) {
+function adjustBranchParameters(branchParameters: BranchParams[], idMapping: Dictionary<string>) {
return branchParameters?.map(({ branchId, ...branchParameter }: BranchParams) => ({
...branchParameter,
- branchId: uniqueIds.find((uniqueId) => uniqueId.includes(branchId)),
+ branchId: idMapping[branchId] || branchId,
}));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function adjustBranchParameters(branchParameters: BranchParams[], uniqueIds: string[]) { | |
| return branchParameters?.map(({ branchId, ...branchParameter }: BranchParams) => ({ | |
| ...branchParameter, | |
| branchId: uniqueIds.find((uniqueId) => uniqueId.includes(branchId)), | |
| })); | |
| } | |
| function adjustBranchParameters(branchParameters: BranchParams[], idMapping: Dictionary<string>) { | |
| return branchParameters?.map(({ branchId, ...branchParameter }: BranchParams) => ({ | |
| ...branchParameter, | |
| branchId: idMapping[branchId] || branchId, | |
| })); | |
| } |
| const adjustedEdges = edgesWithValidIds.reduce((edges, edge) => { | ||
| const fromNode = nodes.find((n) => n.id === edge.from); | ||
| const toNode = nodes.find((n) => n.id === edge.to); | ||
| const currentNodeEdges = NodeUtils.getOutputEdges(fromNode.id, edges); | ||
| const newEdge = createEdge(fromNode, toNode, edge.edgeType, currentNodeEdges, action.processDefinitionData); | ||
| const newEdge = createEdge(fromNode, toNode, edge.edgeType, currentNodeEdges, processDefinitionData); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add checks for undefined nodes when creating new edges
When creating new edges, ensure that both fromNode and toNode are found in the nodes array. If either is undefined, it could lead to runtime errors.
Consider adding validation:
const fromNode = nodes.find((n) => n.id === edge.from);
+if (!fromNode) {
+ throw new Error(`Node with id ${edge.from} not found`);
+}
const toNode = nodes.find((n) => n.id === edge.to);
+if (!toNode) {
+ throw new Error(`Node with id ${edge.to} not found`);
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const adjustedEdges = edgesWithValidIds.reduce((edges, edge) => { | |
| const fromNode = nodes.find((n) => n.id === edge.from); | |
| const toNode = nodes.find((n) => n.id === edge.to); | |
| const currentNodeEdges = NodeUtils.getOutputEdges(fromNode.id, edges); | |
| const newEdge = createEdge(fromNode, toNode, edge.edgeType, currentNodeEdges, action.processDefinitionData); | |
| const newEdge = createEdge(fromNode, toNode, edge.edgeType, currentNodeEdges, processDefinitionData); | |
| const adjustedEdges = edgesWithValidIds.reduce((edges, edge) => { | |
| const fromNode = nodes.find((n) => n.id === edge.from); | |
| if (!fromNode) { | |
| throw new Error(`Node with id ${edge.from} not found`); | |
| } | |
| const toNode = nodes.find((n) => n.id === edge.to); | |
| if (!toNode) { | |
| throw new Error(`Node with id ${edge.to} not found`); | |
| } | |
| const currentNodeEdges = NodeUtils.getOutputEdges(fromNode.id, edges); | |
| const newEdge = createEdge(fromNode, toNode, edge.edgeType, currentNodeEdges, processDefinitionData); |
Describe your changes
Checklist before merge
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes