fixed search focus and keyboard/clipboard events #7207
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several modifications across multiple files to enhance functionality related to node management, clipboard interactions, and user actions within the application. In Possibly related PRs
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: 3
🧹 Outside diff range and nitpick comments (10)
designer/client/src/components/themed/InputWithIcon.tsx (1)
75-81: LGTM! Consider adding error handling for the focus operation.The change improves user experience by maintaining focus after clearing the input while preventing unwanted scrolling. This is a good enhancement to the component's behavior.
Consider wrapping the focus call in a try-catch block to handle potential errors, as focus operations can fail in certain edge cases (e.g., when the input is removed from DOM):
<div className={addonStyles} onClick={() => { onClear(); - focus({ preventScroll: true }); + try { + focus({ preventScroll: true }); + } catch (error) { + console.warn('Failed to focus input after clear:', error); + } }} >designer/client/src/containers/BindKeyboardShortcuts.tsx (1)
49-59: Refactor clipboard handlers to reduce code duplication.The clipboard handlers follow a similar pattern and could be refactored to improve maintainability.
Consider this refactoring:
-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; -}, +const createClipboardHandler = ( + action: keyof typeof userActions, + selector: EventTrackingSelector, + skipInputCheck?: boolean +) => (event: ClipboardEvent) => { + if (!skipInputCheck && isInputEvent(event)) return; + userActions[action]?.((e) => eventWithStatistics({ selector }, e))(event); +}; + +copy: createClipboardHandler('copy', EventTrackingSelector.CopyNode), +paste: createClipboardHandler('paste', EventTrackingSelector.PasteNode, true), +cut: createClipboardHandler('cut', EventTrackingSelector.CutNode),designer/client/src/actions/nk/node.ts (2)
Line range hint
119-131: Consider enhancing the batching behavior documentationWhile the comment explains why
flushSyncis needed, it would be helpful to document:
- The specific Redux undo issue that this solves
- Why batch grouping is still needed outside of
flushSync
Line range hint
1-158: Consider documenting the node management lifecycleThe changes introduce more sophisticated node management with proper batching, validation, and selection handling. Consider adding documentation or a sequence diagram that illustrates:
- The lifecycle of node operations (add/delete/connect)
- The batching strategy and its interaction with React's rendering
- The validation rules for node connections
This would help future maintainers understand the system's behavior and constraints.
designer/client/src/common/ClipboardUtils.ts (2)
20-23: Consider using a type alias instead of an interface for function typesIn TypeScript, it's more idiomatic to use a type alias when defining function types. Changing
interface WriteTextto atypecan improve code readability.Suggested change:
-interface WriteText { - (text: string): Promise<string>; -} +type WriteText = (text: string) => Promise<string>;
Line range hint
31-50: Ensure the promise always resolves infallbackWriteTextRelying on the
oncopyevent to resolve the promise may lead to issues if the event doesn't fire as expected. To guarantee the promise resolves, consider resolving it immediately after executing the copy command.Suggested change:
const fallbackWriteText: WriteText = (text) => { return new Promise((resolve) => { const el = document.createElement("textarea"); el.value = text; el.setAttribute("readonly", ""); el.className = css({ position: "absolute", left: "-9999px", }); - el.oncopy = (e) => { - // Skip event triggered by writing selection to the clipboard. - e.stopPropagation(); - resolve(text); - }; document.body.appendChild(el); el.select(); document.execCommand("copy"); + resolve(text); document.body.removeChild(el); }); };designer/client/src/components/graph/SelectionContextProvider.tsx (4)
1-1: Consider using nativeMath.minfor minimum value calculationInstead of importing
minfrom "lodash", you can use the nativeMath.minfunction along with the spread operator for basic minimum value calculations. This reduces dependencies and may improve performance.
Line range hint
104-106: Clarify the return type ofuseClipboardPermissionThe hook returns
state === "prompt" || content;, which can be a boolean or a string. This might cause confusion when the return value is used in boolean contexts.Refactor the return statement to consistently return a boolean indicating whether the clipboard can be accessed. For example:
return state === "granted";If you need to return the parsed content separately, consider returning an object with explicit properties.
Line range hint
149-156: Avoid using random offsets for pasted node positionsIn
calculatePastedNodePosition, adding a random offset to node positions can lead to unpredictable placement, which may confuse users.Replace the random offset with a fixed offset to ensure consistent positioning. For example:
- const random = Math.floor(Math.random() * 20) + 1; const randomizedNodePosition = { - x: selectionLayoutNodePosition.x + random, - y: selectionLayoutNodePosition.y + random, + x: selectionLayoutNodePosition.x + FIXED_OFFSET_X, + y: selectionLayoutNodePosition.y + FIXED_OFFSET_Y, };Define
FIXED_OFFSET_XandFIXED_OFFSET_Yas needed.
Line range hint
155-165: Handle potentialundefinedvalues fromminfunctionThe
minfunction from "lodash" returnsundefinedwhen provided an empty array. This can lead toNaNvalues in position calculations ifselection.nodesis empty.Add checks to ensure
selection.nodesis not empty before proceeding with calculations. For example:if (selection.nodes.length === 0) { dispatch(error(t("userActions.paste.failed", "No nodes to paste from clipboard"))); return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
designer/client/src/actions/nk/node.ts(2 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)
🔇 Additional comments (7)
designer/client/src/containers/BindKeyboardShortcuts.tsx (2)
3-3: LGTM! Clean import statement organization.
The consolidated import statement improves code organization and readability.
49-59: Verify clipboard permissions and handle errors.
The clipboard operations should verify permissions and handle potential errors.
Let's check if clipboard permissions are handled elsewhere in the codebase:
designer/client/src/actions/nk/node.ts (3)
1-1: LGTM: New imports serve clear purposes
The addition of flushSync and toggleSelection imports aligns well with the implementation needs:
flushSynchandles React 18's automatic batching behaviortoggleSelectionsupports the enhanced node deletion workflow
Also applies to: 9-9
Line range hint 89-117: LGTM: Robust validation for node connections
The implementation properly validates node connection capabilities before attempting connections, preventing invalid graph states. The batch grouping ensures atomic operations.
56-58: Verify toggleSelection's argument handling
The spread operator usage with toggleSelection(...ids) assumes the function accepts multiple arguments. Let's verify this implementation.
✅ Verification successful
toggleSelection correctly handles spread arguments
The implementation in designer/client/src/actions/nk/selection.ts confirms that toggleSelection is correctly defined to accept variable arguments using rest parameters:
export function toggleSelection(...nodeIds: string[]): ThunkActionThe spread operator usage in the review code matches this implementation perfectly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for toggleSelection implementation to verify argument handling
ast-grep --pattern 'export function toggleSelection($$$) {
$$$
}'
Length of output: 69
Script:
#!/bin/bash
# Let's try searching for toggleSelection with ripgrep to find its implementation
rg "toggleSelection" -A 5
Length of output: 6243
designer/client/src/components/graph/SelectionContextProvider.tsx (2)
Line range hint 78-97: Ensure cross-browser compatibility for navigator.permissions API
The navigator.permissions API and the "clipboard-read" permission name are experimental features and may not be supported in all browsers. Relying on them could lead to inconsistent behavior across different user environments.
Implement feature detection and provide fallbacks for browsers that do not support this API. For example:
useEffect(() => {
if (navigator.permissions && navigator.permissions.query) {
navigator.permissions
.query({ name: "clipboard-read" as PermissionName })
.then(/* ... */)
.catch(/* ... */);
} else {
// Fallback logic or default state
}
}, []);Line range hint 201-212: Verify that text selection state updates correctly
The selectionchange event listener updates the hasSelection state using hasTextSelection, but this might not reflect the latest selection state accurately due to closure over the initial value.
Ensure that hasTextSelection is called within the event handler to get the current selection:
useDocumentListeners(
useMemo(
() => ({
- selectionchange: () => setHasSelection(hasTextSelection),
+ selectionchange: () => setHasSelection(hasTextSelection()),
}),
[],
),
);
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
designer/client/src/components/graph/SelectionContextProvider.tsx (2)
Line range hint
156-166: Add input validation and document magic numbersThe position calculation function needs defensive programming:
- Validate required properties (layoutData) before access
- Document or extract the magic number (20) used in random calculation
Consider this safer implementation:
function calculatePastedNodePosition(node, pasteX, minNodeX, pasteY, minNodeY, random) { + if (!node?.additionalFields?.layoutData) { + throw new Error("Node layout data is required for position calculation"); + } const currentNodePosition = node.additionalFields.layoutData; const pasteNodePosition = { x: currentNodePosition.x + pasteX, y: currentNodePosition.y + pasteY }; const selectionLayoutNodePosition = { x: pasteNodePosition.x - minNodeX, y: pasteNodePosition.y - minNodeY }; + // Add slight randomization (1-20 pixels) to prevent exact overlay of nodes const randomizedNodePosition = { x: selectionLayoutNodePosition.x + random, y: selectionLayoutNodePosition.y + random, }; return randomizedNodePosition; }
Line range hint
168-184: Improve error handling and add cleanupThe node insertion implementation has two areas for improvement:
- The empty catch block silently swallows errors
- The debounced callback should be cleaned up on unmount
Consider these improvements:
const [parseInsertNodes] = useDebouncedCallback((clipboardText) => { const selection = parse(clipboardText); if (selection) { const { x, y } = props.pastePosition(); - const minNodeX: number = min(selection.nodes.map((node) => node.additionalFields.layoutData.x)); - const minNodeY: number = min(selection.nodes.map((node) => node.additionalFields.layoutData.y)); + // Handle potential undefined layoutData + const minNodeX: number = min(selection.nodes.map((node) => node?.additionalFields?.layoutData?.x ?? 0)); + const minNodeY: number = min(selection.nodes.map((node) => node?.additionalFields?.layoutData?.y ?? 0)); const random = Math.floor(Math.random() * 20) + 1; const nodesWithPositions = selection.nodes.map((node) => ({ node, position: calculatePastedNodePosition(node, x, minNodeX, y, minNodeY, random), })); dispatch(nodesWithEdgesAdded(nodesWithPositions, selection.edges)); } else { dispatch(error(t("userActions.paste.failed", "Cannot paste content from clipboard"))); } -}, 250); +}, 250, { maxWait: 1000 }); + +useEffect(() => { + return () => { + parseInsertNodes.cancel(); + }; +}, [parseInsertNodes]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
designer/client/src/actions/nk/node.ts(2 hunks)designer/client/src/components/graph/SelectionContextProvider.tsx(3 hunks)engine/flink/components/base-tests/src/test/scala/pl/touk/nussknacker/engine/flink/SpelTemplateLazyParameterTest.scala(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- engine/flink/components/base-tests/src/test/scala/pl/touk/nussknacker/engine/flink/SpelTemplateLazyParameterTest.scala
🚧 Files skipped from review as they are similar to previous changes (1)
- designer/client/src/actions/nk/node.ts
🔇 Additional comments (1)
designer/client/src/components/graph/SelectionContextProvider.tsx (1)
Line range hint 186-199: Handle potential race condition in paste operation
The paste operation might encounter a race condition when multiple paste events occur rapidly, as the clipboard read and parse operations are asynchronous.
Let's verify the clipboard event handling:
Consider adding a loading state to prevent concurrent paste operations:
+const [isPasting, setIsPasting] = useState(false);
+
const paste = useCallback(
async (event?: Event) => {
if (isInputEvent(event)) {
return;
}
+ if (isPasting) {
+ return;
+ }
try {
+ setIsPasting(true);
const clipboardText = await ClipboardUtils.readText(event);
parseInsertNodes(clipboardText);
} catch {
dispatch(error(t("userActions.paste.notAvailable", "Paste button is not available. Try Ctrl+V")));
+ } finally {
+ setIsPasting(false);
}
},
- [dispatch, parseInsertNodes, t],
+ [dispatch, parseInsertNodes, t, isPasting],
);
JulianWielga
force-pushed
the
keyboard-events
branch
from
171aa39
to
84033e5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
designer/client/src/common/ClipboardUtils.ts (2)
Line range hint
31-50: Add error handling and cleanup guaranteesThe fallback implementation should handle errors and ensure cleanup in all scenarios.
Consider this more robust implementation:
const fallbackWriteText: WriteText = (text) => { - return new Promise((resolve) => { + return new Promise((resolve, reject) => { const el = document.createElement("textarea"); + const cleanup = () => { + if (document.body.contains(el)) { + document.body.removeChild(el); + } + }; el.value = text; el.setAttribute("readonly", ""); el.className = css({ position: "absolute", left: "-9999px", }); el.oncopy = (e) => { // Skip event triggered by writing selection to the clipboard. 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); + } + // Cleanup after a timeout in case the copy event never fires + setTimeout(cleanup, 1000); }); };
Line range hint
1-57: Consider architectural improvements for better maintainabilityA few suggestions to enhance the codebase:
- Add type guards for clipboard API availability
- Extract magic numbers (like timeout) to constants
- Export error types for better error handling across the application
Example type guard:
const isClipboardWriteSupported = (): boolean => { return !!(navigator.clipboard?.writeText); };Example constants:
export const CLIPBOARD_CONSTANTS = { CLEANUP_TIMEOUT_MS: 1000, HIDDEN_POSITION: "-9999px", } as const;designer/client/src/components/graph/SelectionContextProvider.tsx (3)
Line range hint
164-173: Enhance node position calculation robustnessThe position calculation function could benefit from several improvements:
- Add input validation to prevent NaN or undefined values
- Consider extracting the random offset range (20) to a named constant
- Consider making the random offset optional or configurable
+const POSITION_RANDOM_OFFSET_RANGE = 20; + function calculatePastedNodePosition(node, pasteX, minNodeX, pasteY, minNodeY, random) { + if (!node?.additionalFields?.layoutData) { + throw new Error("Invalid node layout data"); + } const currentNodePosition = node.additionalFields.layoutData; + if (typeof currentNodePosition.x !== "number" || typeof currentNodePosition.y !== "number") { + throw new Error("Invalid position coordinates"); + } const pasteNodePosition = { x: currentNodePosition.x + pasteX, y: currentNodePosition.y + pasteY }; const selectionLayoutNodePosition = { x: pasteNodePosition.x - minNodeX, y: pasteNodePosition.y - minNodeY }; const randomizedNodePosition = {
Line range hint
175-208: Improve paste operation UX and error handlingConsider the following improvements:
- Extract the debounce delay to a named constant
- Provide more specific error messages based on failure reason
- Add loading state during paste operation
+const PASTE_DEBOUNCE_DELAY = 250; +const PASTE_ERROR_TYPES = { + INVALID_FORMAT: "paste.invalidFormat", + PERMISSION_DENIED: "paste.permissionDenied", + NETWORK_ERROR: "paste.networkError", +}; + const [parseInsertNodes] = useDebouncedCallback((clipboardText) => { const selection = parse(clipboardText); if (selection) { const { x, y } = props.pastePosition(); 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 random = Math.floor(Math.random() * 20) + 1; const nodesWithPositions = selection.nodes.map((node) => ({ node, position: calculatePastedNodePosition(node, x, minNodeX, y, minNodeY, random), })); dispatch(nodesWithEdgesAdded(nodesWithPositions, selection.edges)); } else { - dispatch(error(t("userActions.paste.failed", "Cannot paste content from clipboard"))); + dispatch(error(t(`userActions.${PASTE_ERROR_TYPES.INVALID_FORMAT}`, "Invalid content format in clipboard"))); } -}, 250); +}, PASTE_DEBOUNCE_DELAY);
Line range hint
210-223: Add type safety for event handlingConsider improving type safety by:
- Using a more specific type for the event parameter
- Adding type guards for event handling
-type UserAction = ((e: Event) => unknown) | null; +type ClipboardEvent = Event & { clipboardData?: DataTransfer }; +type UserAction = ((e: ClipboardEvent) => unknown) | null; +function isClipboardEvent(event: Event): event is ClipboardEvent { + return 'clipboardData' in event; +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
designer/client/src/actions/nk/node.ts(2 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)
🚧 Files skipped from review as they are similar to previous changes (3)
- designer/client/src/actions/nk/node.ts
- designer/client/src/components/themed/InputWithIcon.tsx
- designer/client/src/containers/BindKeyboardShortcuts.tsx
🔇 Additional comments (3)
designer/client/src/common/ClipboardUtils.ts (2)
20-22: LGTM: Well-defined TypeScript interface
The WriteText interface is clear, concise, and properly typed.
52-57: Previous review comment about error handling is still valid
The implementation should handle clipboard API errors and attempt fallback as suggested in the previous review.
designer/client/src/components/graph/SelectionContextProvider.tsx (1)
Line range hint 76-81: Consider event-driven clipboard access instead of polling
The current implementation polls the clipboard every 500ms, which could impact performance and may not work reliably due to browser security restrictions.
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (11)
designer/client/src/reducers/graph/utils.ts (1)
60-63: Consider handling emptynewNodesarrayIf
newNodesis empty, functions likemapandreducemay behave unexpectedly. Ensure that the code correctly handles cases wherenewNodesWithPositionsmight be empty.You might want to add a check at the beginning:
if (newNodes.length === 0) { return { nodes: [], layout: [], idMapping: {}, }; }designer/client/src/reducers/graph/reducer.ts (1)
34-34: Consider initializing 'selectionState' to an empty objectSetting
selectionStatetonullmay lead to null reference errors when accessing its properties. Initializing it to an empty object{}ensures safer access and aligns with standard practices.Apply this diff to initialize
selectionStateas an empty object:const emptyGraphState: GraphState = { scenarioLoading: false, scenario: null, layout: [], testCapabilities: null, testFormParameters: null, - selectionState: null, + selectionState: {}, processCounts: {}, testResults: null, unsavedNewName: null, };designer/client/src/reducers/graph/utils.test.ts (1)
7-7: Document the boolean parameter's purposeThe boolean parameter
truepassed toprepareNewNodesWithLayoutlacks context. Consider:
- Adding a comment explaining its purpose
- Using a named constant instead of a magic boolean
- expect(prepareNewNodesWithLayout(scenarioGraph.nodes, nodesWithPositions, true)).toMatchSnapshot(); + const PRESERVE_NODE_POSITIONS = true; // or whatever this boolean controls + expect(prepareNewNodesWithLayout(scenarioGraph.nodes, nodesWithPositions, PRESERVE_NODE_POSITIONS)).toMatchSnapshot();Additionally, consider adding explicit assertions alongside the snapshot test to make the test's intentions clearer:
const result = prepareNewNodesWithLayout(scenarioGraph.nodes, nodesWithPositions, PRESERVE_NODE_POSITIONS); // Add specific assertions for critical behavior expect(result.nodes).toBeDefined(); expect(result.idMapping).toBeDefined(); // Then use snapshot for complete verification expect(result).toMatchSnapshot();designer/client/src/reducers/graph/selectionState.ts (2)
1-4: Consider optimizing lodash importsThe current implementation imports both
uniqandxorfrom lodash. Sinceuniqis only used once, consider using native Set for uniqueness to reduce bundle size.-import { uniq, xor } from "lodash"; +import { xor } from "lodash";Then update the EXPAND_SELECTION case to use Set:
-return uniq([...state, ...action.nodeIds]); +return [...new Set([...state, ...action.nodeIds])];
4-19: Consider adding TypeScript improvements for better type safetyThe reducer could benefit from stricter typing:
- Use a discriminated union type for actions
- Make the switch statement exhaustive
type SelectionAction = | { type: "NODES_WITH_EDGES_ADDED"; nodes: Array<{ id: string }> } | { type: "NODE_ADDED"; nodes: Array<{ id: string }> } | { 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) => { // ... rest of the implementationdesigner/client/src/reducers/settings.ts (1)
57-59: LGTM! Consider adding documentation for the edgesForNodes array.The initialization of
processDefinitionDatawith an emptyedgesForNodesarray is a good practice to ensure consistent state structure. However, consider adding a comment explaining its purpose and relationship with node management.Add documentation like this:
processDefinitionData: { + // Stores edges information for nodes in the process definition edgesForNodes: [], },designer/client/src/actions/nk/node.ts (2)
125-138: Consider extracting the node preparation logicWhile the implementation is solid, the node preparation logic within the flushSync block could be extracted to a separate function for better testability and reuse. This would also make it easier to modify the preparation logic without dealing with React's batching concerns.
Consider refactoring to:
export function nodeAdded(node: NodeType, position: Position): ThunkAction { return (dispatch, getState) => { batchGroupBy.startOrContinue(); + const prepareNode = () => { + const scenarioGraph = getScenarioGraph(getState()); + return prepareNewNodesWithLayout(scenarioGraph.nodes, [{ node, position }], false); + }; // We need to disable automatic React batching https://react.dev/blog/2022/03/29/react-v18#new-feature-automatic-batching // since it breaks redux undo in this case flushSync(() => { - const scenarioGraph = getScenarioGraph(getState()); - const { nodes, layout } = prepareNewNodesWithLayout(scenarioGraph.nodes, [{ node, position }], false); + const { nodes, layout } = prepareNode(); dispatch({ type: "NODE_ADDED", nodes, layout, }); dispatch(layoutChanged()); }); batchGroupBy.end(); }; }
147-157: Consider adding error handling for edge validationWhile the implementation is good, it might benefit from validating the edges before dispatching the action. Invalid edges could lead to inconsistent graph state.
Consider adding validation:
export function nodesWithEdgesAdded(nodesWithPositions: NodesWithPositions, edges: Edge[]): ThunkAction { return (dispatch, getState) => { const state = getState(); const processDefinitionData = getProcessDefinitionData(state); const scenarioGraph = getScenarioGraph(state); const { nodes, layout, idMapping } = prepareNewNodesWithLayout(scenarioGraph.nodes, nodesWithPositions, true); + // Validate edges before dispatching + const validEdges = edges.filter(edge => { + const fromNode = nodes.find(n => n.id === edge.from); + const toNode = nodes.find(n => n.id === edge.to); + return fromNode && toNode && NodeUtils.canConnect(fromNode, toNode, processDefinitionData); + }); + batchGroupBy.startOrContinue(); dispatch({ type: "NODES_WITH_EDGES_ADDED", nodes, layout, idMapping, - edges, + edges: validEdges, processDefinitionData, }); dispatch(layoutChanged()); batchGroupBy.end(); }; }designer/client/test/reducer-test.js (3)
112-127: Excellent implementation of the test reducer!The refactored
reduceAllfunction is a significant improvement:
- Properly handles both synchronous and asynchronous actions
- Maintains state through closure
- Follows Redux patterns with
dispatchandgetStateConsider adding error handling for failed dispatches to make the tests more robust.
Line range hint
171-188: Enhance test coverage for multiple nodes.While the test verifies node IDs, it should also verify:
- Node positions are correctly set
- Original node remains unchanged
Consider adding these assertions:
expect(result.graphReducer.layout.find((n) => n.id === "kafka-transaction (copy 1)").position).toEqual(testPosition); expect(result.graphReducer.layout.find((n) => n.id === "kafka-transaction (copy 2)").position).toEqual(testPosition); expect(NodeUtils.getNodeById("kafka-transaction", result.graphReducer.scenario.scenarioGraph)).toEqual(originalNode);
Based on the findings from NodeUtils-test.js, I can now generate the final response:
Test should include validation rules for edge connections
The current test is missing important edge cases that are already validated in the codebase:
- Connections to/from sink nodes (a sink can only receive connections)
- Connections to/from source nodes (a source can only send connections)
- Maximum number of output connections (e.g., Filter nodes have a limit of two outputs)
- Edge uniqueness between same nodes
Add test cases to verify these scenarios when adding nodes and edges to ensure the graph maintains its integrity constraints.
🔗 Analysis chain
Line range hint
192-213: Consider additional edge cases for node-edge connections.The test should verify:
- Edge cases for invalid connections
- Proper handling of duplicate edge IDs
Let's check if there are any edge validation rules in the codebase:
Let's search for more specific validation rules and edge connection logic:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for edge validation logic rg -A 5 "validateEdge|isValidConnection|canConnect"Length of output: 3186
Script:
#!/bin/bash # Search for canMakeLink implementation and related validation logic rg -A 10 "canMakeLink" # Search for edge-related tests rg -l "test.*edge|edge.*test" --type=testLength of output: 9764
📜 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 (8)
designer/client/src/actions/actionTypes.ts(0 hunks)designer/client/src/actions/nk/node.ts(5 hunks)designer/client/src/reducers/graph/reducer.ts(4 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)
💤 Files with no reviewable changes (1)
- designer/client/src/actions/actionTypes.ts
🔇 Additional comments (10)
designer/client/src/reducers/graph/reducer.ts (3)
2-15: New imports are appropriate and necessary
The added imports from "lodash" and local modules (batchGroupBy, correctFetchedDetails, NestedKeyOf, selectionState) enhance the functionality of the reducer and are correctly used in the code.
236-239: Simplification of 'NODE_ADDED' action enhances readability
Refactoring the 'NODE_ADDED' case to directly use addNodesWithLayout with action.nodes and action.layout simplifies the code and reduces complexity, improving maintainability.
313-313: Integration of 'selectionState' reducer promotes modularity
Including selectionState in mergeReducers allows for better state management by delegating selection-specific logic to its own reducer, enhancing code organization.
designer/client/src/reducers/graph/utils.test.ts (1)
5-5: Clarify test description to match implementation
The test description mentions "union output expression parameter" but the test implementation doesn't explicitly verify this behavior. Consider either:
- Adding explicit assertions to verify the union output expression update, or
- Updating the description to better match what's being tested
Let's verify the union output expression handling in the codebase:
designer/client/src/reducers/graph/selectionState.ts (1)
4-19: Verify consistent usage of selection actions
Let's verify that all components dispatching these selection actions are using the correct payload structure.
✅ Verification successful
All selection actions are being used correctly with proper payload structure
Based on the verification results, the selection actions are being dispatched consistently across the codebase:
TOGGLE_SELECTIONandEXPAND_SELECTIONactions inactions/nk/selection.tsusenodeIdspayloadRESET_SELECTIONinactions/nk/selection.tsuses optionalnodeIdspayloadNODE_ADDEDandNODES_WITH_EDGES_ADDEDinactions/nk/node.tsusenodespayloadDELETE_NODESinactions/nk/node.tsusesidspayload
All dispatched actions match the expected payload structure handled by the reducer, ensuring type safety and consistency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for dispatch calls of selection actions
echo "Searching for selection action dispatches..."
rg -A 2 "dispatch\(.*(?:NODES_WITH_EDGES_ADDED|NODE_ADDED|DELETE_NODES|EXPAND_SELECTION|TOGGLE_SELECTION|RESET_SELECTION)"
# Search for action creator usage
echo "Searching for action creators..."
ast-grep --pattern 'dispatch({ type: $_, $$$})'
Length of output: 20559
designer/client/src/reducers/settings.ts (1)
57-59: Verify the impact on node-related operations.
Since this initialization is connected to node management functionality, let's verify its usage across the codebase.
✅ Verification successful
Based on the search results, I can now provide a final response:
The edgesForNodes initialization is safe and correctly used across the codebase
The empty array initialization is appropriate as this property is populated by the server-side DefinitionsService and consumed by client-side components, primarily in NodeUtils.ts. The usage pattern shows:
- Server defines edges via
EdgeTypesPreparer.prepareEdgeTypes - Client uses it for node connection validation and edge type determination
- Test files confirm the expected structure with empty arrays as valid initial states
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how edgesForNodes is used in node-related operations
# Find direct references to edgesForNodes
echo "Direct references to edgesForNodes:"
rg "edgesForNodes" -A 3
# Find node-related actions that might depend on this initialization
echo "\nNode-related actions:"
ast-grep --pattern 'type = "NODES_WITH_EDGES_ADDED"'
Length of output: 15911
designer/client/src/actions/nk/node.ts (3)
36-38: LGTM! Improved separation of concerns
The restructuring of NodesWithEdgesAddedAction to separate node data from layout information is a good architectural improvement. The addition of idMapping provides better support for node identification and potential duplication scenarios.
50-51: LGTM! Consistent type structure
The NodeAddedAction type now follows the same pattern as NodesWithEdgesAddedAction, supporting batch operations and maintaining a clear separation between node data and layout information.
1-6: Verify the necessity of flushSync usage
The addition of flushSync suggests a workaround for React 18's automatic batching. While it's used to prevent issues with redux undo, this approach might indicate an underlying architectural issue.
Let's check if this is a common pattern in the codebase:
✅ Verification successful
flushSync usage appears to be necessary for Redux state synchronization
The flushSync usage is justified in this case. It's used in the node actions to ensure synchronous state updates when adding nodes to the scenario graph. This is important because:
- The code needs to get the current scenario graph state
- Prepare new nodes with layout
- Dispatch multiple actions in sequence (NODE_ADDED and layoutChanged)
Without flushSync, React 18's automatic batching could cause race conditions between these dependent state updates.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other flushSync usages to understand the pattern
rg "flushSync" --type ts --type tsx
Length of output: 63
Script:
#!/bin/bash
# Search for flushSync usages with correct ripgrep syntax
rg "flushSync"
# Also search for react-dom imports to understand the context
rg "from ['\"]react-dom['\"]"
Length of output: 131122
Script:
#!/bin/bash
# Search for flushSync usage in the codebase
ast-grep --pattern 'flushSync($$$)'
# Also search for any imports of flushSync
ast-grep --pattern 'import { $$$flushSync$$$ } from "react-dom"'
Length of output: 3285
designer/client/test/reducer-test.js (1)
1-1: LGTM! Good practice using action creators.
The change to import action creators instead of manually constructing action objects improves maintainability and consistency.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
designer/client/src/reducers/graph/reducer.ts (1)
313-313: Good architectural change: Selection state extractionMoving selection state management to a separate reducer improves code organization and follows the Single Responsibility Principle. This change:
- Improves maintainability by isolating selection logic
- Makes the code more modular
- Follows better separation of concerns
Consider applying similar pattern to other complex state management areas in the reducer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
designer/client/src/reducers/graph/reducer.ts(3 hunks)
🔇 Additional comments (3)
designer/client/src/reducers/graph/reducer.ts (3)
242-262: Previous review comments about edge handling remain valid
The code still needs improvements in robustness:
- Missing null checks for fromNode/toNode before edge creation
- No fallback handling for missing IDs in idMapping
Referring to previous review comments about:
- Adding null checks before edge creation
- Ensuring robustness in ID mapping for edges
15-15: Verify selectionState initialization consistency
There appears to be an inconsistency in how selectionState is initialized and handled:
- emptyGraphState initializes it as an empty array
[] - The reducer composition suggests different handling via imported
selectionState
This could lead to unexpected behavior in selection state management.
Also applies to: 313-313
✅ Verification successful
No inconsistency found in selectionState initialization and handling
The selectionState implementation is consistent across the codebase:
- The
emptyGraphStatecorrectly initializesselectionStateas[] - The imported
selectionStatereducer fromselectionState.tsalso uses[]as its default state - The reducer composition at line 313 properly combines these consistent implementations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check selectionState initialization and usage patterns
rg "selectionState.*[=:]" --type typescript
Length of output: 80
Script:
#!/bin/bash
# Check selectionState initialization and usage patterns with ripgrep
rg "selectionState.*[=:]"
# Also check the specific file content
cat designer/client/src/reducers/graph/reducer.ts
# Check the selectionState file
cat designer/client/src/reducers/graph/selectionState.ts
Length of output: 15088
236-239: Verify NODE_ADDED action payload structure
The simplified node addition logic assumes the action payload contains properly structured nodes and layout. Ensure that all code paths creating this action provide these fields correctly.
✅ Verification successful
Action payload structure is correctly handled
The verification confirms that:
- The
NODE_ADDEDaction is dispatched with propernodesandlayoutfields, which are obtained fromprepareNewNodesWithLayoututility. - The
prepareNewNodesWithLayoutfunction properly structures the data, returning bothnodesandlayoutin the expected format. - The reducer's
addNodesWithLayouthandler correctly processes these fields as seen in the reducer implementation.
The action payload structure is consistent across the codebase and properly validated through TypeScript types and utility functions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all dispatches of NODE_ADDED action
ast-grep --pattern 'dispatch({ type: "NODE_ADDED", $$$})'
Length of output: 396
Script:
#!/bin/bash
# Let's examine the context around the NODE_ADDED dispatch to understand how nodes and layout are structured
rg -B 10 -A 5 'type: "NODE_ADDED",' designer/client/src/actions/nk/node.ts
# Also check the type definitions for the action
ast-grep --pattern 'interface $_ { type: "NODE_ADDED"; $$$ }'
Length of output: 778
Script:
#!/bin/bash
# Let's check the prepareNewNodesWithLayout function to understand how nodes and layout are structured
ast-grep --pattern 'function prepareNewNodesWithLayout($$$) { $$$ }'
# Also check the type definition of the state to understand the expected structure
ast-grep --pattern 'type $_ = { nodes: $_; layout: $_; $$$ }'
Length of output: 134
Script:
#!/bin/bash
# Let's try a different approach to find the prepareNewNodesWithLayout implementation
rg -B 2 -A 10 "prepareNewNodesWithLayout"
# Also check for type definitions in the reducer file
cat designer/client/src/reducers/graph/reducer.ts
Length of output: 19548
|
created: #7226 |
JulianWielga
force-pushed
the
keyboard-events
branch
from
822087a
to
f080e3e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (11)
designer/client/src/reducers/graph/utils.test.ts (1)
5-5: Improve test description clarityThe current test description is lengthy and could be more concise. Consider breaking it down into multiple test cases for better clarity and maintainability.
- it("prepareNewNodesWithLayout should update union output expression parameter with an updated node name when new unique node ids created", () => { + it("should update node names in union output expressions when creating unique IDs", () => {designer/client/src/actions/nk/node.ts (3)
1-6: Add documentation for flushSync usage.The imports look good and align with the new functionality. However, consider adding a more detailed comment explaining why
flushSyncis necessary for handling React 18's automatic batching behavior in this specific case.
50-51: Document multi-node addition scenarios.The type now supports adding multiple nodes at once. Consider adding documentation to explain scenarios where multiple nodes would be added in a single action.
147-157: LGTM! Consider adding batch grouping tests.The changes improve consistency and make state access more explicit. The batch grouping ensures atomic operations, but consider adding tests to verify this behavior.
Would you like me to help create test cases for the batch grouping behavior?
designer/client/test/reducer-test.js (3)
112-127: Consider adding error handling to the dispatch function.While the implementation correctly handles both synchronous and asynchronous actions, it could benefit from error handling to catch and report dispatch failures.
Consider this enhancement:
const dispatch = (action) => { + try { if (typeof action === "function") { - action(dispatch, getState); + return action(dispatch, getState); } else { - currentState = reducer(currentState, action); + currentState = reducer(currentState, action); + return action; } + } catch (error) { + console.error('Action dispatch failed:', error); + throw error; + } };
Line range hint
157-209: Consider adding error case test coverage.While the happy path scenarios are well covered, consider adding tests for:
- Invalid node data
- Duplicate edge creation
- Invalid edge references
- Position validation
Example test case:
it("should handle invalid node data", () => { expect(() => reduceAll([ nodeAdded({ id: "invalid" }, testPosition) ]) ).toThrow(/Invalid node type/); });
159-160: Consider grouping related assertions.The test assertions could be organized better for readability by grouping related checks together.
Example refactor:
it("should add single node", () => { const result = reduceAll([nodeAdded(testNode, testPosition)]); // Node structure validation const addedNode = NodeUtils.getNodeById(testNode.id, result.graphReducer.scenario.scenarioGraph); expect(addedNode).toMatchSnapshot(); // Layout validation const nodeLayout = result.graphReducer.layout.find((n) => n.id === testNode.id); expect(nodeLayout.position).toEqual(testPosition); });Also applies to: 166-167, 187-188, 208-209
designer/client/src/components/graph/SelectionContextProvider.tsx (3)
Line range hint
165-174: Enhance type safety and predictability of node positioningThe function could benefit from TypeScript types and more predictable positioning logic.
Consider these improvements:
- function calculatePastedNodePosition(node, pasteX, minNodeX, pasteY, minNodeY, random) { + interface LayoutData { + x: number; + y: number; + } + + interface Node { + additionalFields: { + layoutData: LayoutData; + }; + } + + function calculatePastedNodePosition( + node: Node, + targetPosition: LayoutData, + selectionBounds: LayoutData, + offset: number = 10 + ): LayoutData { const currentNodePosition = node.additionalFields.layoutData; - const pasteNodePosition = { x: currentNodePosition.x + pasteX, y: currentNodePosition.y + pasteY }; - const selectionLayoutNodePosition = { x: pasteNodePosition.x - minNodeX, y: pasteNodePosition.y - minNodeY }; - const randomizedNodePosition = { - x: selectionLayoutNodePosition.x + random, - y: selectionLayoutNodePosition.y + random, - }; - return randomizedNodePosition; + return { + x: currentNodePosition.x + targetPosition.x - selectionBounds.x + offset, + y: currentNodePosition.y + targetPosition.y - selectionBounds.y + offset, + }; }
Line range hint
176-195: Improve paste operation robustness and error handlingThe paste operation could benefit from more specific error handling and consistent node positioning.
Consider these improvements:
const [parseInsertNodes] = useDebouncedCallback((clipboardText) => { const selection = parse(clipboardText); if (selection) { const { x, y } = props.pastePosition(); 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 random = Math.floor(Math.random() * 20) + 1; + const GRID_SPACING = 10; // Use consistent spacing instead of random const nodesWithPositions = selection.nodes.map((node) => ({ node, - position: calculatePastedNodePosition(node, x, minNodeX, y, minNodeY, random), + position: calculatePastedNodePosition( + node, + { x, y }, + { x: minNodeX, y: minNodeY }, + GRID_SPACING + ), })); dispatch(nodesWithEdgesAdded(nodesWithPositions, selection.edges)); } else { - dispatch(error(t("userActions.paste.failed", "Cannot paste content from clipboard"))); + dispatch(error(t("userActions.paste.failed", "Invalid content format in clipboard. Expected a valid node selection."))); } - }, 250); + }, 100); // Reduce debounce time for better responsiveness
Line range hint
197-208: Enhance error handling and debugging capabilitiesThe current error handling is too generic and might hide important issues from developers.
Consider improving error handling:
const paste = useCallback( async (event?: Event) => { if (isInputEvent(event)) { return; } try { const clipboardText = await ClipboardUtils.readText(event); parseInsertNodes(clipboardText); - } catch { - dispatch(error(t("userActions.paste.notAvailable", "Paste button is not available. Try Ctrl+V"))); + } catch (e) { + console.error("Paste operation failed:", e); + if (e instanceof Error) { + if (e.name === "NotAllowedError") { + dispatch(error(t("userActions.paste.notAllowed", "Clipboard access denied. Please use Ctrl+V"))); + } else if (e.name === "DataError") { + dispatch(error(t("userActions.paste.invalidData", "Invalid clipboard content"))); + } else { + dispatch(error(t("userActions.paste.failed", "Failed to paste: {{message}}", { message: e.message }))); + } + } } }, [dispatch, parseInsertNodes, t], );designer/client/src/reducers/graph/reducer.ts (1)
234-246: Consider functional approach for edge creationThe edge creation logic could be more functional and concise.
Consider this alternative implementation:
-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, processDefinitionData); - return edges.concat(newEdge); -}, state.scenario.scenarioGraph.edges); +const nodesMap = new Map(nodes.map(node => [node.id, node])); +const adjustedEdges = state.scenario.scenarioGraph.edges.concat( + edgesWithValidIds + .filter(edge => nodesMap.has(edge.from) && nodesMap.has(edge.to)) + .map(edge => { + const fromNode = nodesMap.get(edge.from); + const toNode = nodesMap.get(edge.to); + const currentNodeEdges = NodeUtils.getOutputEdges(fromNode.id, state.scenario.scenarioGraph.edges); + return createEdge(fromNode, toNode, edge.edgeType, currentNodeEdges, processDefinitionData); + }) +);
📜 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 (12)
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)
💤 Files with no reviewable changes (1)
- designer/client/src/actions/actionTypes.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- designer/client/src/common/ClipboardUtils.ts
- designer/client/src/components/themed/InputWithIcon.tsx
- designer/client/src/containers/BindKeyboardShortcuts.tsx
- designer/client/src/reducers/graph/selectionState.ts
- designer/client/src/reducers/settings.ts
🔇 Additional comments (10)
designer/client/src/reducers/graph/utils.test.ts (1)
4-7: Verify snapshot testing implementation
Let's verify if snapshot testing is properly set up and if there are any existing snapshots.
✅ Verification successful
Let me check the actual snapshot content to verify if it's properly implemented.
Snapshot testing is properly implemented
The snapshot testing implementation is correct and follows best practices:
- Snapshot file exists and contains valid serialized test data
- The snapshot captures complex graph node structures with proper formatting
- The test case has a clear, descriptive name
- Consistent with other snapshot tests in the codebase
- Jest snapshot configuration is present (detected in earlier search)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing snapshot files and snapshot testing setup
# Check for snapshot file
fd --type f -e snap -e snapshot
# Check for snapshot testing configuration
rg -l "toMatchSnapshot|jest.config"
# Check if other tests are using snapshots (for consistency)
rg "toMatchSnapshot" --type ts
Length of output: 1346
Script:
#!/bin/bash
# Check the content of the snapshot file
cat designer/client/src/reducers/graph/__snapshots__/utils.test.ts.snap
# Check the test file implementation
cat designer/client/src/reducers/graph/utils.test.ts
Length of output: 2965
designer/client/src/actions/nk/node.ts (2)
36-38: LGTM! Improved type structure.
The separation of nodes and layout data, along with the addition of idMapping, provides better organization and supports node duplication scenarios.
125-138: Enhance documentation and verify redux-undo integration.
The changes look good, particularly the use of prepareNewNodesWithLayout. However:
- Consider expanding the comment to explain specifically how automatic batching affects redux-undo in this case.
- We should verify that this change properly integrates with redux-undo.
✅ Verification successful
Let me gather more information about the batching and redux state updates to better understand the integration.
Let me try another search pattern to find the NODE_ADDED action and batching behavior.
Based on the search results, I can now provide a final response since I have enough context about the batching behavior and flushSync usage:
The flushSync usage is correct and necessary for redux-undo compatibility.
The code shows a well-implemented solution where:
- The
batchGroupByutility is properly integrated with redux-undo to group related actions - The
flushSyncusage is necessary because:- It prevents React 18's automatic batching from interfering with redux-undo's history tracking
- It's wrapped around the specific dispatch calls that need to be processed immediately
- The comment accurately explains the reason for disabling automatic batching
The implementation ensures that node addition operations are properly tracked in the undo history while maintaining compatibility with React 18's batching behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for redux-undo usage and potential batching issues
rg -A 3 "redux-undo|undoable"
Length of output: 5791
Script:
#!/bin/bash
# Search for relevant state updates and batching patterns
rg -A 5 "NODE_ADDED|flushSync"
# Also check for any redux batch operations
ast-grep --pattern 'dispatch({ type: "NODE_ADDED", $$$})'
Length of output: 130388
Script:
#!/bin/bash
# Search for NODE_ADDED and flushSync usage
rg -A 5 "NODE_ADDED|flushSync|batchGroupBy" designer/client/src/actions/nk/node.ts
# Also check for redux batching related code
rg -A 5 "batchGroupBy|redux" designer/client/src/reducers/graph/
Length of output: 5537
designer/client/test/reducer-test.js (1)
1-3: LGTM: Import changes align with action creator updates.
The imports have been properly reorganized to use the new action creators, maintaining a clean separation of concerns.
designer/client/src/reducers/graph/utils.ts (3)
44-49:
Critical: Unsafe ID matching in adjustBranchParameters
The current implementation using uniqueIds.find((uniqueId) => uniqueId.includes(branchId)) is unsafe as it can lead to incorrect mappings when a uniqueId contains the branchId as a substring. For example, if uniqueIds contains "node10" and branchId is "node1", it would incorrectly match.
The previous review suggestion to use idMapping is still valid. Additionally, consider adding input validation:
-function adjustBranchParameters(branchParameters: BranchParams[], uniqueIds: string[]) {
+function adjustBranchParameters(branchParameters: BranchParams[] | undefined, idMapping: Dictionary<string>) {
+ if (!branchParameters?.length) return [];
return branchParameters?.map(({ branchId, ...branchParameter }: BranchParams) => ({
...branchParameter,
- branchId: uniqueIds.find((uniqueId) => uniqueId.includes(branchId)),
+ branchId: idMapping[branchId] || branchId,
}));
}80-100:
Fix potential loss of existing edges
The current implementation directly assigns changes.edges which could lead to loss of existing edges when changes.edges is undefined.
The previous review suggestion to merge edges is still valid:
return {
...state,
scenario: {
...state.scenario,
scenarioGraph: {
...state.scenario.scenarioGraph,
nodes: [...currentNodes, ...changes.nodes],
- edges: changes.edges,
+ edges: changes.edges
+ ? [...(state.scenario.scenarioGraph.edges || []), ...changes.edges]
+ : state.scenario.scenarioGraph.edges,
},
},
layout: [...currentLayout, ...changes.layout],
};52-76:
Update adjustBranchParameters call to use the generated idMapping
The function generates an idMapping but doesn't use it for adjustBranchParameters. Instead, it passes uniqueIds which is used unsafely. Since idMapping is already being created (line 76), it should be used for branch parameter adjustment.
return {
nodes: zipWith(newNodes, uniqueIds, (node, id) => ({
...node,
id,
- branchParameters: adjustBranchParameters(node.branchParameters, uniqueIds),
+ branchParameters: adjustBranchParameters(node.branchParameters, zipObject(initialIds, uniqueIds)),
})),
layout: zipWith(newPositions, uniqueIds, (position, id) => ({
id,
position,
})),
idMapping: zipObject(initialIds, uniqueIds),
};Likely invalid or redundant comment.
designer/client/src/reducers/graph/reducer.ts (3)
12-15: LGTM! Good architectural separation
The separation of concerns through new imports (selectionState, batchGroupBy, etc.) improves modularity and maintainability.
303-303: LGTM! Good separation of selection state
Moving selection state to a separate reducer improves code organization and maintainability.
226-229: Verify layout computation before node addition
The simplified node addition logic assumes correct layout computation. Let's verify where the layout is being computed.
✅ Verification successful
Layout computation is correctly handled before node addition
The code shows that layout computation is properly handled:
- Layout is computed by
prepareNewNodesWithLayoutinreducers/graph/utils.tsbefore dispatching the NODE_ADDED action - The computed layout is passed through the action and correctly used in
addNodesWithLayoutreducer - This pattern is consistently used in both single node addition and batch node operations
The simplified node addition logic in the reducer is safe as it relies on pre-computed layout from the action creators.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find where node layout is computed before NODE_ADDED action
# Look for layout computation in action creators and utils
rg -t typescript -t javascript "NODE_ADDED.*layout" -B 3 -A 3
ast-grep --pattern 'export function $_($$_): $_ { $$_ layout $$_ NODE_ADDED $$_ }'
Length of output: 183
Script:
#!/bin/bash
# Search for NODE_ADDED action type definition and usage
rg --type ts --type js "NODE_ADDED" -B 3 -A 3
# Search for layout computation related to nodes
rg --type ts --type js "layout.*nodes|nodes.*layout" -B 3 -A 3
# Find functions that might compute layout
ast-grep --pattern 'function $name($params) {
$$$
layout
$$$
}'
Length of output: 133875
Script:
#!/bin/bash
# Search for layout computation in action creators and graph reducer
rg --type ts --type js "addNodesWithLayout|prepareNewNodesWithLayout" -B 3 -A 3
# Search for layout computation in node-related files
rg --type ts --type js "layout.*nodes|nodes.*layout" -B 3 -A 3 designer/client/src/actions/nk/node.ts
Length of output: 6231
| const { scenarioGraph } = state.scenario; | ||
| expect(prepareNewNodesWithLayout(scenarioGraph.nodes, nodesWithPositions, true)).toMatchSnapshot(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider expanding test coverage
While snapshot testing is a valid approach, consider adding more specific test cases to verify edge scenarios and explicit expectations. This would make the test more maintainable and easier to debug when failures occur.
Here's a suggested expansion:
describe("GraphUtils", () => {
describe("prepareNewNodesWithLayout", () => {
const { scenarioGraph } = state.scenario;
it("should match snapshot for basic node layout", () => {
expect(prepareNewNodesWithLayout(scenarioGraph.nodes, nodesWithPositions, true)).toMatchSnapshot();
});
it("should handle empty current nodes", () => {
expect(prepareNewNodesWithLayout([], nodesWithPositions, true)).toMatchSnapshot();
});
it("should handle empty nodes with positions", () => {
expect(prepareNewNodesWithLayout(scenarioGraph.nodes, [], true)).toMatchSnapshot();
});
it("should verify specific node name updates", () => {
const result = prepareNewNodesWithLayout(scenarioGraph.nodes, nodesWithPositions, true);
// Add specific assertions for node name updates
expect(result.nodes[0].name).toBeDefined();
expect(result.idMapping).toBeDefined();
});
});
});
separate selectionState reducer
JulianWielga
force-pushed
the
keyboard-events
branch
from
f080e3e
to
632fd1c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
designer/client/src/actions/nk/node.ts (1)
125-138: LGTM! Good handling of React 18 batching issues.The changes correctly handle React 18's automatic batching behavior and provide consistent node preparation. The comment explaining the
flushSyncusage is particularly helpful.Consider extracting the node preparation logic into a separate function for better reusability:
return (dispatch, getState) => { batchGroupBy.startOrContinue(); + const prepareNode = (scenarioGraph) => { + return prepareNewNodesWithLayout(scenarioGraph.nodes, [{ node, position }], false); + }; flushSync(() => { const scenarioGraph = getScenarioGraph(getState()); - const { nodes, layout } = prepareNewNodesWithLayout(scenarioGraph.nodes, [{ node, position }], false); + const { nodes, layout } = prepareNode(scenarioGraph);designer/client/src/reducers/graph/utils.ts (1)
51-100: Consider adding input validation and error handlingThe graph manipulation functions could benefit from:
- Input validation to ensure required properties exist
- Error handling for edge cases (e.g., duplicate IDs, invalid edges)
- TypeScript strict null checks for better type safety
Example improvements:
function validateNode(node: NodeType): void { if (!node.id) { throw new Error("Node must have an ID"); } // Add more validation as needed } // Usage in prepareNewNodesWithLayout: newNodes.forEach(validateNode);designer/client/src/components/graph/SelectionContextProvider.tsx (2)
Line range hint
156-166: Add TypeScript types to improve maintainabilityThe position calculation logic is sound, but the function could benefit from explicit TypeScript types for better maintainability and type safety.
Consider this improvement:
-function calculatePastedNodePosition(node, pasteX, minNodeX, pasteY, minNodeY, random) { +interface NodePosition { + x: number; + y: number; +} + +interface NodeWithLayout { + additionalFields: { + layoutData: NodePosition; + }; +} + +function calculatePastedNodePosition( + node: NodeWithLayout, + pasteX: number, + minNodeX: number, + pasteY: number, + minNodeY: number, + random: number +): NodePosition {
Line range hint
168-186: Extract magic numbers into named constantsThe paste handling logic is well-implemented with debouncing and proper error handling. However, consider extracting magic numbers into named constants for better maintainability.
Consider this improvement:
+const DEBOUNCE_DELAY_MS = 250; +const MIN_RANDOM_OFFSET = 1; +const MAX_RANDOM_OFFSET = 20; const [parseInsertNodes] = useDebouncedCallback((clipboardText) => { const selection = parse(clipboardText); if (selection) { const { x, y } = props.pastePosition(); 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 random = Math.floor(Math.random() * 20) + 1; + const random = Math.floor(Math.random() * (MAX_RANDOM_OFFSET - MIN_RANDOM_OFFSET + 1)) + MIN_RANDOM_OFFSET; const nodesWithPositions = selection.nodes.map((node) => ({ node, position: calculatePastedNodePosition(node, x, minNodeX, y, minNodeY, random), })); dispatch(nodesWithEdgesAdded(nodesWithPositions, selection.edges)); } else { dispatch(error(t("userActions.paste.failed", "Cannot paste content from clipboard"))); } -}, 250); +}, DEBOUNCE_DELAY_MS);designer/client/src/reducers/graph/reducer.ts (1)
303-303: LGTM: Good architectural separation of selection stateMoving selection state to a separate reducer follows the Single Responsibility Principle and improves code organization.
Consider applying similar separation of concerns to other aspects of the graph state in future refactors, such as layout management or validation state.
docs/Changelog.md (1)
112-112: Consider expanding the changelog entry with more details.The current entry "Fixed minor clipboard, keyboard and focus related bugs" is quite vague. Consider providing more specific details about:
- What search focus issues were fixed
- What keyboard/clipboard event bugs were addressed
- The impact of these fixes on user experience
This would help users and developers better understand the changes and their implications.
📜 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 (13)
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
🚧 Files skipped from review as they are similar to previous changes (6)
- designer/client/src/common/ClipboardUtils.ts
- designer/client/src/components/themed/InputWithIcon.tsx
- designer/client/src/containers/BindKeyboardShortcuts.tsx
- designer/client/src/reducers/graph/selectionState.ts
- designer/client/src/reducers/settings.ts
- designer/client/test/reducer-test.js
🔇 Additional comments (11)
designer/client/src/actions/nk/node.ts (4)
1-6: LGTM! Imports are well-organized and support the new functionality.
The new imports, particularly flushSync, are necessary to handle React 18's automatic batching behavior correctly.
147-157: LGTM! Well-structured state management.
The changes follow good practices:
- State selectors are called at the start
- Consistent use of
prepareNewNodesWithLayout - Proper batch grouping maintenance
50-51: Document the new batch capability of NodeAddedAction.
The type now supports batch operations with arrays of nodes and layout. While this provides more flexibility, it's a breaking change that needs documentation.
Consider adding JSDoc comments to document this new batch capability.
✅ Verification successful
Let me check the reducer implementations to understand how the batch operation is handled.
Let me check the implementation of addNodesWithLayout to ensure it properly handles the batch operation.
Add JSDoc comments to document the batch operation capability
The verification confirms that both the reducers (selectionState.ts and reducer.ts) and the utility function addNodesWithLayout properly handle arrays of nodes and layout positions. The code is functionally correct, but lacks documentation about this batch capability.
designer/client/src/actions/nk/node.ts: Add JSDoc comments toNodeAddedActiontype to document:- The batch operation capability
- The requirement that
nodesandlayoutarrays must have matching indices
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct usage of NodeAddedAction type
# and potential switch statements handling this action
# Look for type usage
rg "NodeAddedAction" --type ts
# Look for reducer cases handling this action type
rg "case\s+['\"]NODE_ADDED['\"]" --type ts
Length of output: 359
Script:
#!/bin/bash
# Check the reducer implementations
rg -A 10 'case "NODE_ADDED"' --type ts
Length of output: 1938
Script:
#!/bin/bash
# Look for addNodesWithLayout implementation
rg -A 15 "function addNodesWithLayout" --type ts
Length of output: 1201
36-38: Verify consumers of NodesWithEdgesAddedAction type.
The type has been restructured to use separate nodes, layout, and idMapping properties instead of nodesWithPositions. While this provides better type safety, it's a breaking change.
✅ Verification successful
Breaking change impact is localized and properly handled
The type restructuring is safely handled in both consumers:
selectionStatereducer only uses thenodesarray to map IDsgraph/reducer.tscorrectly destructures all new properties (nodes,layout,idMapping) and uses them appropriately
No other direct consumers of this type were found in the codebase, and the existing ones have been updated to handle the new structure correctly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any direct usage of NodesWithEdgesAddedAction type
# and potential switch statements handling this action
# Look for type usage
rg "NodesWithEdgesAddedAction" --type ts
# Look for reducer cases handling this action type
rg "case\s+['\"]NODES_WITH_EDGES_ADDED['\"]" --type ts
Length of output: 425
Script:
#!/bin/bash
# Let's examine the reducer implementations to understand how they handle this action
rg -B 2 -A 5 'case\s+"NODES_WITH_EDGES_ADDED"' --type ts
# Also check for any tests that might reveal usage patterns
rg "NODES_WITH_EDGES_ADDED" --type test
Length of output: 1530
Script:
#!/bin/bash
# Let's check for test files with different extensions
rg "NODES_WITH_EDGES_ADDED" -g "*.test.*" -g "*.spec.*"
# Let's also see more context of the reducer implementation
rg -B 2 -A 15 'case\s+"NODES_WITH_EDGES_ADDED"' designer/client/src/reducers/graph/reducer.ts
Length of output: 1015
designer/client/src/reducers/graph/utils.ts (3)
44-49:
Critical: Unsafe branch ID mapping logic
The current implementation using uniqueIds.find((uniqueId) => uniqueId.includes(branchId)) is unsafe as it can lead to incorrect mappings if one ID is a substring of another (e.g., "node1" would match with "node10").
The previous review suggestion to use idMapping dictionary remains valid. Please implement the suggested fix:
-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,
}));
}80-100:
Prevent potential edge loss during node addition
The current implementation directly assigns changes.edges, which could lead to loss of existing edges when changes.edges is undefined.
The previous review suggestion to merge edges remains valid. Please implement the suggested fix:
const currentNodes = state.scenario.scenarioGraph.nodes || [];
+const currentEdges = state.scenario.scenarioGraph.edges || [];
const currentLayout = state.layout;
return {
...state,
scenario: {
...state.scenario,
scenarioGraph: {
...state.scenario.scenarioGraph,
nodes: [...currentNodes, ...changes.nodes],
- edges: changes.edges,
+ edges: changes.edges ? [...currentEdges, ...changes.edges] : currentEdges,
},
},
layout: [...currentLayout, ...changes.layout],
};51-77: Good improvement in function signature, but needs follow-up
The change to accept currentNodes instead of full state reduces coupling and makes the function more focused. The addition of idMapping in the return type is also beneficial for tracking ID transformations.
However, once the adjustBranchParameters function is updated to use idMapping, this call needs to be updated. Let's verify the current usage:
✅ Verification successful
Function implementation is correct, no changes needed
The adjustBranchParameters function is already correctly using the uniqueIds parameter to map branch IDs, and the current call in prepareNewNodesWithLayout passes the correct arguments. The implementation shows that idMapping is not needed as the function uses uniqueIds to find corresponding new IDs for branches.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all calls to adjustBranchParameters
rg "adjustBranchParameters\(" --type ts
Length of output: 306
Script:
#!/bin/bash
# Let's check the implementation of adjustBranchParameters
ast-grep --pattern 'function adjustBranchParameters($$$) {
$$$
}'
Length of output: 644
designer/client/src/components/graph/SelectionContextProvider.tsx (2)
1-1: LGTM: Import additions are appropriate
The new imports for min from lodash and useInterval are correctly added and necessary for the new paste position calculation and clipboard monitoring functionality.
Also applies to: 31-31
Line range hint 77-82: Consider using Clipboard API events instead of polling
This comment was previously raised and is still valid. The polling approach could impact performance and battery life.
designer/client/src/reducers/graph/reducer.ts (2)
226-229: LGTM: Clean refactor of node addition logic
The changes simplify the node addition by using the addNodesWithLayout utility function, improving code maintainability.
232-252: Skipping duplicate comments about edge handling
The previous review comments about edge ID mapping and null checks are still valid and applicable to this code segment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
designer/client/src/reducers/graph/utils.ts (2)
52-59: Add input validation for defensive programming.The function should validate its inputs to handle edge cases gracefully.
Consider adding these checks:
export function prepareNewNodesWithLayout( currentNodes: NodeType[] = [], newNodesWithPositions: NodesWithPositions, isCopy: boolean, ): { layout: NodePosition[]; nodes: NodeType[]; idMapping: Dictionary<string>; } { + if (!newNodesWithPositions?.length) { + return { nodes: [], layout: [], idMapping: {} }; + }
80-87: Add type guards for the changes parameter.The function should validate the structure of the input object to prevent runtime errors.
Consider adding these checks:
export function addNodesWithLayout( state: GraphState, changes: { nodes: NodeType[]; layout: NodePosition[]; edges?: Edge[]; }, ): GraphState { + if (!changes?.nodes || !changes?.layout) { + throw new Error("Invalid changes object: nodes and layout are required"); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
designer/client/src/reducers/graph/utils.ts(2 hunks)
🔇 Additional comments (1)
designer/client/src/reducers/graph/utils.ts (1)
1-1: Overall structure looks good!
The file maintains good organization, immutability, and type safety. The utility functions are well-designed and follow functional programming principles.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
Style