feat(ui): improve canvas navigation panel #663
Conversation
WalkthroughThis pull request introduces several enhancements across multiple components in the editor's UI. Key modifications include the implementation of a locking mechanism for node interactions, improvements in the Changes
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
Documentation and Community
|
✅ Deploy Preview for reearth-flow ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![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 (16)
ui/src/features/Editor/components/LeftPanel/index.tsx (4)
203-214: Simplify event handling by directly passing the item toonDoubleClickCurrently,
idContaineris used to store the selected item's ID between theonSelectChangeandonDoubleClickhandlers. This can cause issues if the user double-clicks without first selecting an item or if the state is not updated promptly. To make the code more reliable and user-friendly, consider modifying theTreecomponent to pass the clicked item directly to theonDoubleClickhandler.Here's how you can adjust the code:
- Update the
Treecomponent to pass the clicked item to theonDoubleClickhandler.- Modify the
onDoubleClickhandler to accept the item.-<Tree +<Tree data={treeContent} className="w-full shrink-0 truncate rounded px-1" - onSelectChange={(item) => { ... }} - onDoubleClick={() => { ... }} + onSelectChange={(item) => { ... }} + onDoubleClick={(item) => { + if (item?.id) { + const node = nodes.find((n) => n.id === item.id); + if (node) { + if (interactionLockedNodes.some((n) => n.id === node.id)) { + unlockNodeInteraction(node); + } else { + lockNodeInteraction(node); + } + } + } + }} />This refactoring:
- Eliminates the need for
idContainer.- Ensures that the correct node is accessed when double-clicking.
- Enhances code readability and maintainability.
81-158: Refactor repetitive code intreeContentgenerationThe code for generating
treeContentcontains repetitive patterns for each node type (transformer,note,subworkflow,batch). This repetition can make the code harder to maintain and prone to errors. Consider creating a helper function to generate these sections dynamically.Here's an example of how you might refactor the code:
function createNodeSection(type: string, icon: any, title: string) { const filteredNodes = nodes?.filter((n) => n.type === type); if (filteredNodes && filteredNodes.length > 0) { return { id: type, name: t(title), icon: icon, children: filteredNodes.map((n) => ({ id: n.id, name: n.data.name ?? "untitled", icon: isNodeLocked(n.id) ? Lock : icon, // Include any additional properties specific to this node type })), }; } return null; } const treeContent: TreeDataItem[] = [ // Reader nodes ...(nodes ?.filter((n) => n.type === "reader") .map((n) => ({ id: n.id, name: n.data.name ?? "untitled", icon: isNodeLocked(n.id) ? Lock : Database, })) ?? []), // Writer nodes ...(nodes ?.filter((n) => n.type === "writer") .map((n) => ({ id: n.id, name: n.data.name ?? "untitled", icon: isNodeLocked(n.id) ? Lock : Disc, })) ?? []), // Other node types createNodeSection("transformer", Lightning, "Transformers"), createNodeSection("note", Note, "Note"), createNodeSection("subworkflow", Plus, "Subworkflow"), createNodeSection("batch", RectangleDashed, "Batch Node"), ].filter(Boolean); // Remove null entriesThis refactoring:
- Reduces code duplication.
- Makes it easier to add or modify node types in the future.
- Improves readability and maintainability.
161-176: Ensure default case handles unexpected node types ingetNodeIconThe
getNodeIconfunction provides icons based on the node type. The default case uses theCircleicon. Confirm that theCircleicon is appropriate for unexpected or new node types. If not, consider adding handling for additional node types or updating the default icon.
218-222: Adjust the positioning of the "Unlock All" button for responsivenessThe "Unlock All" button is positioned absolutely at the bottom-right corner. On smaller screens or in cases where the content overflows, the button might overlap with other elements or become inaccessible. Consider using flex properties or relative positioning to ensure the button remains accessible and the layout is responsive.
Example adjustment:
-<button - onClick={unlockAllNodes} - className="absolute bottom-2 right-2 w-24 h-8 rounded-lg bg-red-500 text-white text-xs flex items-center justify-center shadow-md hover:bg-red-400"> +<div className="mt-2 flex justify-end"> + <button + onClick={unlockAllNodes} + className="w-24 h-8 rounded-lg bg-red-500 text-white text-xs flex items-center justify-center shadow-md hover:bg-red-400"> Unlock All - </button> -</button> +</button> +</div>This change places the button within the flow of the document, making it adapt better to different screen sizes and content lengths.
ui/src/utils/index.ts (1)
4-4: Consider a more domain-specific approach for node identificationInstead of adding a generic
randomIDshortutility, consider creating a more domain-specific utility likegenerateNodeIdthat encapsulates the node identification logic. This would make the code more maintainable and self-documenting.Example implementation:
// nodeUtils.ts export function generateNodeId(nodeName: string): string { // You can include node-specific logic here // e.g., prefix, validation, etc. return `${nodeName}-${randomIDshort()}`; }ui/src/utils/randomIDshort.ts (1)
1-12: Consider using a more robust random ID generationWhile Math.random() is acceptable for UI purposes, consider using a more robust solution if this function might be used for other purposes in the future.
Some alternatives to consider:
- UUID v4 for guaranteed uniqueness
- nanoid for shorter, URL-friendly unique IDs
- crypto.getRandomValues() for cryptographically secure randomness
Would you like me to provide implementation examples for any of these alternatives?
ui/src/features/Editor/useInteractionLocker.tsx (3)
22-38: Consider improving the fitView implementation.The zoom functionality could be enhanced in several ways:
- Consider making the duration and padding configurable via props
- Add debouncing to prevent jarring effects during rapid locks
- Add error handling for invalid node IDs
+ import { debounce } from 'lodash'; + + type LockerProviderProps = { + children: React.ReactNode; + zoomDuration?: number; + zoomPadding?: number; + }; + - export const LockerProvider: React.FC<{ children: React.ReactNode }> = ({ + export const LockerProvider: React.FC<LockerProviderProps> = ({ children, + zoomDuration = 500, + zoomPadding = 2, }) => { // ... existing state code ... + const debouncedFitView = useCallback( + debounce((nodeId: string) => { + fitView({ + nodes: [{ id: nodeId }], + duration: zoomDuration, + padding: zoomPadding, + }); + }, 250), + [fitView, zoomDuration, zoomPadding] + ); const lockNodeInteraction = useCallback( (node: Node) => { + if (!node?.id) { + console.warn('Invalid node provided to lockNodeInteraction'); + return; + } setInteractionLockedNodes((currentLockedNodes) => { if (currentLockedNodes.some((n) => n.id === node.id)) return currentLockedNodes; const updatedLockedNodes = [...currentLockedNodes, node]; - fitView({ - nodes: [{ id: node.id }], - duration: 500, - padding: 2, - }); + debouncedFitView(node.id); return updatedLockedNodes; }); }, - [fitView], + [debouncedFitView], );
63-69: Consider enhancing the error message for better debugging.While the error handling is good, the error message could be more specific to help developers understand where the Provider should be placed.
if (!context) { - throw new Error("useLocker must be used within a LockerProvider"); + throw new Error( + "useLocker must be used within a LockerProvider. " + + "Please ensure LockerProvider is present in the component tree above this component." + ); }
17-19: Consider handling concurrent lock operations.The current implementation might be susceptible to race conditions if multiple lock/unlock operations occur simultaneously. Consider using a more robust state update mechanism.
- const [interactionLockedNodes, setInteractionLockedNodes] = useState<Node[]>( - [], - ); + const [interactionLockedNodes, setInteractionLockedNodes] = useState<Map<string, Node>>( + new Map(), + );This would provide atomic operations and prevent potential race conditions:
- Use Map for O(1) lookups
- Ensure atomic updates with single state transitions
- Prevent duplicate entries automatically
ui/src/features/Editor/index.tsx (2)
39-39: Consider adding error boundariesSince LockerProvider manages critical UI state, consider wrapping it with an error boundary to gracefully handle potential rendering failures.
+import { ErrorBoundary } from 'react-error-boundary'; return ( + <ErrorBoundary fallback={<div>Something went wrong with the editor.</div>}> <LockerProvider> {/* existing content */} </LockerProvider> + </ErrorBoundary> );
Remove commented out lockedNodeIds line
The
lockedNodeIdsis actively used in the codebase:
- It's a state variable in
useNodeLocker.ts- It's properly consumed in
hooks.ts- Only in
Editor/index.tsxit appears as a commented-out line, which is inconsistent with its active usage elsewhereThe commented line should be either restored or removed completely to maintain consistency with the rest of the implementation.
🔗 Analysis chain
Line range hint
17-17: Verify removal of lockedNodeIdsThe commented out lockedNodeIds suggests a transition in the locking implementation. Let's verify if any cleanup is needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to lockedNodeIds rg -t typescript -t javascript "lockedNodeIds" --glob "!node_modules"Length of output: 104
Script:
#!/bin/bash # Search for any remaining references to lockedNodeIds using ripgrep # Note: Using just the file extension patterns instead of file type rg "lockedNodeIds" --glob "*.ts" --glob "*.tsx" --glob "*.js" --glob "*.jsx"Length of output: 462
Script:
#!/bin/bash # Let's check the context around these usages to understand if the commented line is stale rg "lockedNodeIds" --glob "*.ts" --glob "*.tsx" --glob "*.js" --glob "*.jsx" -B 3 -A 3Length of output: 2181
ui/src/features/Editor/components/Canvas/components/Nodes/NoteNode/index.tsx (1)
Line range hint
45-83: Clean up commented resize-related codeThere are multiple blocks of commented code related to resizing functionality. This makes the code harder to maintain.
Consider either:
- Implementing the resize functionality if it's needed
- Removing the commented code if it's no longer required
- Adding a TODO comment with a ticket reference if it's planned for future implementation
Would you like assistance with implementing the resize functionality?
ui/src/features/Editor/components/Canvas/useDnd.ts (2)
74-74: Remove debug console.log statementDebug logging statements should be removed before merging to production.
- console.log("nodetype = " + nodeTypes);
81-81: Consider adding name length and format validationThe addition of random IDs to node names is a good solution for ensuring uniqueness. However, consider adding these safeguards:
- Validate the total length of the generated name
- Sanitize the base name before concatenation to ensure valid characters
- name: d + "-" + randomIDshort(), + name: sanitizeName(d).slice(0, 30) + "-" + randomIDshort(),Consider adding this utility function:
function sanitizeName(name: string): string { return name.replace(/[^a-zA-Z0-9-_]/g, ''); }ui/src/features/Editor/components/Canvas/index.tsx (2)
74-80: Consider performance optimizations for node updatesThe logic is correct, but consider these optimizations to prevent unnecessary recalculations:
- Memoize the isNodeDraggable function
- Use useMemo for updatedNodes transformation
+ const isNodeDraggable = useCallback( (nodeId: string) => !interactionLockedNodes.some((lockedNode) => lockedNode.id === nodeId), + [interactionLockedNodes] + ); + const updatedNodes = useMemo( () => nodes.map((node) => ({ ...node, draggable: isNodeDraggable(node.id), })), + [nodes, isNodeDraggable] + );
72-85: Consider extracting locking logic to a separate hookThe current implementation mixes node transformation logic with the component. Consider extracting this into a custom hook (e.g.,
useLockedNodes) to improve modularity and reusability. This would also make it easier to add the missing zoom functionality.Example structure:
const useLockedNodes = (nodes: Node[]) => { const { interactionLockedNodes } = useLocker(); const isNodeDraggable = useCallback(/*...*/); const updatedNodes = useMemo(/*...*/); const zoomToLockedNode = useCallback(/*...*/); return { updatedNodes, zoomToLockedNode }; };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (13)
ui/src/features/Editor/components/Canvas/components/Nodes/NoteNode/index.tsx(3 hunks)ui/src/features/Editor/components/Canvas/index.tsx(2 hunks)ui/src/features/Editor/components/Canvas/useDnd.ts(2 hunks)ui/src/features/Editor/components/LeftPanel/index.tsx(3 hunks)ui/src/features/Editor/index.tsx(2 hunks)ui/src/features/Editor/useInteractionLocker.tsx(1 hunks)ui/src/lib/i18n/locales/en.json(1 hunks)ui/src/lib/i18n/locales/es.json(1 hunks)ui/src/lib/i18n/locales/fr.json(1 hunks)ui/src/lib/i18n/locales/ja.json(1 hunks)ui/src/lib/i18n/locales/zh.json(1 hunks)ui/src/utils/index.ts(1 hunks)ui/src/utils/randomIDshort.ts(1 hunks)
🔇 Additional comments (20)
ui/src/features/Editor/components/LeftPanel/index.tsx (1)
34-34: Ensure consistency with updated onNodeLocking prop
The onNodeLocking prop in the Props type has been updated to include an optional options parameter. Verify that all usages of onNodeLocking within this component are updated to pass the options parameter where necessary. Additionally, ensure that any parent components using LeftPanel are aware of this change and pass the appropriate arguments.
ui/src/utils/index.ts (1)
4-4: Verify the necessity of a separate randomIDshort utility
Given that there's already a randomID utility, let's verify if we need both utilities or if we can consolidate them.
✅ Verification successful
Let me gather more information about the randomID implementation since the previous command didn't show it.
Both randomID utilities serve different purposes and lengths - keep them separate
The verification shows that both utilities are justified:
randomIDgenerates 24-character IDs by default, used for core entity identifiers (workflows, nodes)randomIDshortgenerates 6-character IDs, used specifically for appending to node names for visual differentiation
The implementations and use cases are distinct:
randomIDis used in 8 files for generating primary identifiersrandomIDshortis only used inuseDnd.tsfor creating visually manageable node name suffixes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Compare implementations of randomID and randomIDshort
echo "Checking randomID implementation:"
ast-grep --pattern 'function randomID' -A 10
echo "\nChecking randomIDshort implementation:"
ast-grep --pattern 'function randomIDshort' -A 10
echo "\nChecking usage patterns:"
rg -A 2 'randomID(?!short)' --type ts
rg -A 2 'randomIDshort' --type ts
Length of output: 1486
Script:
#!/bin/bash
# Search for randomID implementation and its usages with PCRE2 support
echo "Checking randomID implementation and file:"
rg -p 'function randomID|const randomID' --type ts
echo "\nChecking randomID usages:"
rg -p 'randomID[^s]' --type ts
echo "\nChecking randomIDshort implementation:"
cat ui/src/utils/randomIDshort.ts
Length of output: 2536
ui/src/utils/randomIDshort.ts (1)
7-9: Consider collision probability for generated IDs
With the current implementation using a 6-character string from 62 possible characters, there's a chance of ID collisions when generating many IDs. For the canvas node use case mentioned in the PR, this might be acceptable if the number of concurrent nodes is relatively small.
Let's analyze the usage to ensure this won't be a problem:
✅ Verification successful
Based on the search results, I can generate the final response as we have sufficient information about the usage of randomIDshort:
Short IDs are used appropriately with minimal collision risk
The function is only used in the canvas drag-and-drop functionality (useDnd.ts) to generate names for individual nodes when they're created. There's no evidence of bulk node creation or patterns that would generate multiple IDs simultaneously. The IDs are only generated one at a time during user-initiated drag operations, making the collision probability negligible for this use case.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how these IDs are used in the codebase
# Look for patterns where multiple nodes might be created simultaneously
# Search for usage of randomIDshort
rg "randomIDshort" -A 5
# Search for patterns indicating bulk node creation
ast-grep --pattern 'new Array($$$).map($$$)'
ast-grep --pattern 'Array.from($$$).map($$$)'
Length of output: 1754
ui/src/features/Editor/useInteractionLocker.tsx (2)
1-10: LGTM! Well-structured type definitions and appropriate imports.
The type definitions and imports are clean, well-organized, and provide good type safety for the locking mechanism.
12-12: LGTM! Context creation follows best practices.
The context is properly typed and initialized with undefined, enforcing the use of the Provider.
ui/src/features/Editor/index.tsx (3)
8-8: LGTM: Clean import addition
The LockerProvider import follows proper conventions and is well-organized with other imports.
39-85: LGTM: Clean implementation of locking feature
The LockerProvider integration and panel visibility conditions are well-structured and maintain proper component hierarchy.
63-63: Verify locking implementation completeness
The PR objectives mention that some interactions are still possible with locked nodes. The current boolean implementation might be too simplistic.
Let's check the implementation details:
✅ Verification successful
Implementation appears complete and well-structured
The locking implementation is comprehensive and properly restricts interactions:
- The canvasLock prop correctly disables all canvas interactions when a node is locked (dragging, connecting, focusing, selecting, panning, zooming)
- Node locking is managed through useNodeLocker hook with proper state management
- Interaction locking is handled via useInteractionLocker with clear lock/unlock methods
- UI reflects locked state and provides controls to unlock nodes
- The boolean implementation is appropriate since it's used as a simple toggle for enabling/disabling interactions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all node interaction handlers to verify locking checks
rg -t typescript -t javascript "function.*handle.*Node" --glob "!node_modules"
# Search for usages of canvasLock prop
rg -t typescript -t javascript "canvasLock.*=" --glob "!node_modules"
Length of output: 222
Script:
#!/bin/bash
# Search for all node interaction handlers to verify locking checks
rg "function.*handle.*Node"
# Search for usages of canvasLock prop
rg "canvasLock"
# Search for locking related code
rg "lock(ed)?" -g "*.{ts,tsx,js,jsx}"
Length of output: 131157
Script:
#!/bin/bash
# Search for all node interaction handlers and locking related code
rg -g "*.{ts,tsx,js,jsx}" "function.*handle.*Node|lock"
# Search for canvasLock usage
rg -g "*.{ts,tsx,js,jsx}" "canvasLock"
Length of output: 5725
ui/src/features/Editor/components/Canvas/components/Nodes/NoteNode/index.tsx (2)
84-85: LGTM! Good switch to controlled component
The change from defaultValue to controlled value prop with onChange handler is a good practice.
2-2: Implement locking feature as per PR objectives
The PR objectives mention that users should be able to lock nodes, but the locking functionality is not implemented in this component. Consider adding:
- A lock state
- Lock/unlock handlers
- Visual indicators for locked state
Let's verify if locking-related code exists elsewhere:
Would you like help implementing the locking feature in this component?
Also applies to: 34-34
ui/src/features/Editor/components/Canvas/useDnd.ts (2)
13-13: LGTM: Import statement is correctly placed
The addition of randomIDshort import alongside randomID maintains good code organization.
Line range hint 19-23: Verify locking feature integration
The onNodeLocking callback is properly integrated into the component props. Let's verify its usage across the codebase to ensure consistent behavior.
✅ Verification successful
Let me try a different approach to verify the locking feature implementation.
Let me try one final search to find the Node type specific to the workflow editor and any locking-related state.
Node locking feature is properly implemented and integrated
The verification reveals a complete and consistent implementation of the node locking feature:
- Node type includes a
lockedproperty inui/src/types/node.ts - Locking state management is handled via
useNodeLockerhook with proper state tracking for both local and collaborative editing - The locking callback is properly propagated through the component hierarchy and consistently used in node creation and interactions
- The UI properly respects the locked state, showing/hiding controls appropriately
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation and usage of node locking feature
# Expected: Find all references to node locking to ensure consistent implementation
# Search for locking-related code
echo "Searching for node locking implementations..."
rg -l "locked|locking|unlock" --type ts --type tsx
# Search specifically for onNodeLocking usage
echo "Searching for onNodeLocking usage..."
ast-grep --pattern 'onNodeLocking={$_}'
Length of output: 306
Script:
#!/bin/bash
# Search for locking-related code with proper file type
echo "Searching for node locking implementations..."
rg -l "locked|locking|unlock" --type typescript
# Search for onNodeLocking function definitions and usages
echo "Searching for onNodeLocking patterns..."
rg -A 3 "onNodeLocking"
# Search for Node interface or type definitions that might contain locking-related properties
echo "Searching for Node type definitions..."
ast-grep --pattern 'type Node = {
$$$
}'
ast-grep --pattern 'interface Node {
$$$
}'
Length of output: 10390
Script:
#!/bin/bash
# Search for workflow-specific Node type and any locking properties
echo "Searching for workflow Node type..."
rg -A 10 "type.*Node.*=.*{|interface.*Node.*{" --type ts
# Search for handleNodeLocking implementation
echo "Searching for handleNodeLocking implementation..."
rg -A 5 "handleNodeLocking.*=" --type ts
# Search for any lock-related state management
echo "Searching for lock-related state..."
rg -A 3 "lock.*State|isLocked|locked" --type ts
Length of output: 6859
ui/src/features/Editor/components/Canvas/index.tsx (2)
20-21: LGTM: Clean integration of the locking functionality
The useLocker hook is properly imported and integrated into the component.
84-85: Verify zoom-to-locked-item functionality
The PR objectives mention that "When an item is locked, the canvas will zoom in on the selected item". However, this functionality appears to be missing in the current implementation.
Let's verify if this functionality exists elsewhere:
ui/src/lib/i18n/locales/en.json (2)
230-230: Verify the "Reload" functionality implementation.
The "Reload" entry has been added but contains an empty string value. Please ensure this matches the actual text displayed in the UI.
✅ Verification successful
The "Reload" translation is properly implemented and used.
The "Reload" translation key is correctly used in the error page component where it serves as the button text for reloading the page when there's an error fetching user data. The empty string in the translation file allows for flexible translation while defaulting to "Reload" in English.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for "Reload" usage in the codebase to verify implementation
# Search for React components using the "Reload" translation key
rg -t tsx -t jsx "('|\")Reload('|\")" -A 3
# Search for translation key usage
ast-grep --pattern 't("Reload")'
Length of output: 202
Script:
#!/bin/bash
# Let's get more context around the Reload usage in the routes/index.tsx file
rg -t js -t jsx -t ts -t tsx "Reload" -B 3 -A 3 ui/src/routes/index.tsx
# Also check for any related error or loading states in the same file
rg -t js -t jsx -t ts -t tsx "error|loading" -B 2 -A 2 ui/src/routes/index.tsx
Length of output: 208
Script:
#!/bin/bash
# Let's try without file type restrictions since rg doesn't recognize tsx/jsx
# Get context around the Reload usage
rg "Reload" -B 3 -A 3 ui/src/routes/index.tsx
# Look for any error handling or loading states
cat ui/src/routes/index.tsx
Length of output: 1787
231-231: Potential confusion between "Subworkflow" and "Subworkflow Node".
There are now two similar entries: "Subworkflow" and "Subworkflow Node" (line 77). This might lead to confusion in the UI. Consider:
- Using just one consistent term
- Adding a comment to clarify the different use cases
- Using more distinct names if they serve different purposes
✅ Verification successful
Based on the results, I can now generate a final response as we have clear evidence of how these terms are used in different contexts.
Both terms serve distinct purposes in different UI components
The terms are used in different contexts:
- "Subworkflow" is used in the LeftPanel component, likely as a category or section name
- "Subworkflow Node" is used in the Toolbox component, specifically for node creation/selection
Since these terms serve different UI purposes (panel vs. node toolbox), having distinct names is actually beneficial for clarity and should be maintained.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage contexts of both translation keys
# Search for components using either translation
rg -t tsx -t jsx "('|\")Subworkflow('|\")" -A 3
rg -t tsx -t jsx "('|\")Subworkflow Node('|\")" -A 3
# Search for translation key usage patterns
ast-grep --pattern 't("Subworkflow")'
ast-grep --pattern 't("Subworkflow Node")'
Length of output: 458
ui/src/lib/i18n/locales/zh.json (1)
230-230: LGTM: Correct translation for "Reload"
The Chinese translation "重新加载" is accurate and appropriate for the "Reload" action.
ui/src/lib/i18n/locales/ja.json (1)
230-231: LGTM! Translations are accurate and consistent.
The translations for both "Reload" (リロード) and "Subworkflow" (サブワークフロー) are:
- Accurate and natural in Japanese
- Consistent with existing terminology in the file
- Follow proper technical term conventions in Japanese
✅ Verification successful
Translations verified and JSON structure is valid
The translations for "Reload" (リロード) and "Subworkflow" (サブワークフロー) are correctly placed at the end of the file, properly formatted with commas, and maintain consistency with the existing translations. The JSON structure is valid and there are no duplicate keys. The translations follow Japanese technical terminology conventions, matching the style used throughout the file.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify JSON validity and consistent formatting
# Note: Using jq to validate JSON structure and check for duplicate keys
# Test 1: Validate JSON structure
jq '.' ui/src/lib/i18n/locales/ja.json > /dev/null
# Test 2: Check for duplicate keys
jq -r 'to_entries | .[] | .key' ui/src/lib/i18n/locales/ja.json | sort | uniq -d
Length of output: 121
Script:
#!/bin/bash
# Let's try a different approach to verify the JSON structure and content
# First, check if the file exists and show its content
cat ui/src/lib/i18n/locales/ja.json
# Then check specifically for the entries we're reviewing
grep -A 1 '"Reload"' ui/src/lib/i18n/locales/ja.json
grep -A 1 '"Subworkflow"' ui/src/lib/i18n/locales/ja.json
# Also check for any potential duplicate keys around these entries
grep -C 3 '"Reload"\|"Subworkflow"' ui/src/lib/i18n/locales/ja.json
Length of output: 9065
ui/src/lib/i18n/locales/es.json (1)
230-230: LGTM: "Reload" translation is correct
The Spanish translation "Recargar" for "Reload" is accurate and commonly used.
ui/src/lib/i18n/locales/fr.json (1)
230-230: LGTM: "Reload" translation is properly localized
The French translation "Recharger" for "Reload" is accurate and appropriate.
| var idContainer = ""; | ||
|
|
There was a problem hiding this comment.
Use useRef instead of var for idContainer to persist value across renders
The variable idContainer is declared using var, which can lead to unexpected behavior due to hoisting and scope issues. In React functional components, variables declared directly inside the component do not persist between renders. To maintain the value of idContainer across renders and avoid potential bugs, consider using the useRef hook.
Apply the following changes:
-import { memo, useEffect, useState } from "react";
+import { memo, useEffect, useState, useRef } from "react";
- var idContainer = "";
+ const idContainer = useRef("");
...
- onSelectChange={(item) => {
+ onSelectChange={(item: TreeDataItem | null) => {
setContent(item?.name ?? "");
if (typeof item?.id === "string") {
- idContainer = item.id;
+ idContainer.current = item.id;
}
}}
- onDoubleClick={() => {
- if (idContainer) {
- const node = nodes.find((n) => n.id === idContainer);
+ onDoubleClick={() => {
+ if (idContainer.current) {
+ const node = nodes.find((n) => n.id === idContainer.current);
if (node) {
if (interactionLockedNodes.some((n) => n.id === node.id)) {
unlockNodeInteraction(node);
} else {
lockNodeInteraction(node);
}
}
}
}}This ensures that idContainer retains its value between renders and behaves as expected during user interactions.
Committable suggestion skipped: line range outside the PR's diff.
| nodes={nodes} | ||
| edges={edges} | ||
| canvasLock={!!locallyLockedNode} | ||
| onWorkflowAdd={handleWorkflowAdd} | ||
| onNodesUpdate={handleNodesUpdate} | ||
| onNodeHover={handleNodeHover} | ||
| onNodeLocking={handleNodeLocking} | ||
| onNodePickerOpen={handleNodePickerOpen} | ||
| onEdgesUpdate={handleEdgesUpdate} | ||
| onEdgeHover={handleEdgeHover} | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using TypeScript discriminated union for lock states
Instead of a boolean canvasLock, consider using a more expressive type to handle different locking states and permitted interactions.
type LockState = {
type: 'unlocked'
} | {
type: 'locked'
allowedInteractions: Array<'select' | 'pan' | 'zoom'>
lockedNodeId: string
};
// Usage in props
interface CanvasProps {
// ... other props
lockState: LockState;
}| const handleContentChange = useCallback( | ||
| (event: React.ChangeEvent<HTMLTextAreaElement>) => { | ||
| const newValue = event.target.value; | ||
| setContent(newValue); | ||
| data.content = newValue; | ||
| }, | ||
| [data], | ||
| ); |
There was a problem hiding this comment.
Avoid direct mutation of props
The handleContentChange function directly mutates data.content, which is an anti-pattern in React. Props should be treated as immutable.
Consider lifting the state up or using a callback prop:
-const handleContentChange = useCallback(
- (event: React.ChangeEvent<HTMLTextAreaElement>) => {
- const newValue = event.target.value;
- setContent(newValue);
- data.content = newValue;
- },
- [data],
-);
+const handleContentChange = useCallback(
+ (event: React.ChangeEvent<HTMLTextAreaElement>) => {
+ const newValue = event.target.value;
+ setContent(newValue);
+ if (data.onChange) {
+ data.onChange(newValue);
+ }
+ },
+ [data],
+);Also update the Node type to include the onChange callback:
type Node = {
// ... existing properties
onChange?: (content: string) => void;
};| @@ -227,5 +227,6 @@ | |||
| "Member has been successfully removed from the workspace.": "成员已成功从工作区中移除。", | |||
| "Empty workflow detected": "检测到空工作流", | |||
| "You cannot create a deployment without a workflow.": "没有工作流，您无法创建部署。", | |||
| "Reload": "重新加载" | |||
| "Reload": "重新加载", | |||
| "Subworkflow": "" | |||
There was a problem hiding this comment.
Missing translation for "Subworkflow"
The "Subworkflow" key has an empty translation value. Since subworkflows are a key feature in this PR's canvas navigation improvements, this missing translation will impact Chinese users' experience.
Suggested translation: "Subworkflow": "子工作流"
Note: This suggestion is based on common terminology in Chinese software localization, where "子" (meaning "sub/child") is prefixed to "工作流" (meaning "workflow").
| @@ -227,5 +227,6 @@ | |||
| "Member has been successfully removed from the workspace.": "El miembro ha sido eliminado exitosamente del espacio de trabajo.", | |||
| "Empty workflow detected": "Se detectó un flujo de trabajo vacío", | |||
| "You cannot create a deployment without a workflow.": "No puedes crear un despliegue sin un flujo de trabajo.", | |||
| "Reload": "Recargar" | |||
| "Reload": "Recargar", | |||
| "Subworkflow": "" | |||
There was a problem hiding this comment.
Add Spanish translation for "Subworkflow"
The PR introduces subworkflow functionality as a key feature, but the Spanish translation is missing. This could impact the user experience for Spanish-speaking users.
Suggested translation: "Subworkflow": "Subflujo de trabajo" or "Subworkflow": "Subflujo"
| @@ -227,5 +227,6 @@ | |||
| "Member has been successfully removed from the workspace.": "Le membre a été supprimé avec succès de l'espace de travail.", | |||
| "Empty workflow detected": "Flux de travail vide détecté", | |||
| "You cannot create a deployment without a workflow.": "Vous ne pouvez pas créer de déploiement sans flux de travail.", | |||
| "Reload": "Recharger" | |||
| "Reload": "Recharger", | |||
| "Subworkflow": "" | |||
There was a problem hiding this comment.
Missing French translation for "Subworkflow"
The "Subworkflow" key has an empty translation string. Since this is part of the new canvas navigation panel improvements, having missing translations could impact French users' experience. Consider adding an appropriate French translation:
- "Subworkflow": ""
+ "Subworkflow": "Sous-workflow"Note: The related key "Subworkflow Node" also has an empty translation and should be addressed.
📝 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.
| "Subworkflow": "" | |
| "Subworkflow": "Sous-workflow" |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
ui/src/features/Editor/components/LeftPanel/index.tsx (6)
34-34: Add JSDoc documentation for the onNodeLocking options parameterThe
optionsparameter with itssourceproperty lacks documentation explaining its purpose and possible values.Add JSDoc documentation like this:
/** * @param nodeId - The ID of the node to lock/unlock * @param options - Additional options for node locking * @param options.source - The source triggering the lock (e.g., 'user', 'system') */ onNodeLocking: (nodeId: string, options?: { source: string }) => void;
57-62: Add cleanup function to useEffectThe effect that resets the selected tab should clean up when the component unmounts.
useEffect(() => { if (!isOpen && selectedTab) { setSelectedTab(undefined); } + return () => { + // Clean up any pending state updates + setSelectedTab(undefined); + }; }, [isOpen, selectedTab]);
109-111: Extract locked node styles to a constant or CSS classThe inline styles for locked nodes should be moved to a constant or CSS class for better maintainability and consistency.
+const LOCKED_NODE_STYLES = { color: "red", cursor: "not-allowed" } as const; -style: isNodeLocked(n.id) - ? { color: "red", cursor: "not-allowed" } - : undefined, +style: isNodeLocked(n.id) ? LOCKED_NODE_STYLES : undefined,
161-176: Improve type safety of getNodeIcon functionThe function accepts any string as type, which could lead to runtime errors.
-function getNodeIcon(type: string | undefined) { +type NodeType = "note" | "subworkflow" | "transformer" | "reader" | "writer"; + +function getNodeIcon(type: NodeType | undefined) { switch (type) { case "note": return Note; case "subworkflow": return Plus; case "transformer": return Lightning; case "reader": return Database; case "writer": return Disc; default: + console.warn(`Unknown node type: ${type}`); return Circle; } }
203-214: Simplify double-click handler logicThe double-click handler can be simplified by extracting the node finding logic.
-onDoubleClick={() => { - if (idContainer) { - const node = nodes.find((n) => n.id === idContainer); - if (node) { - if (interactionLockedNodes.some((n) => n.id === node.id)) { - unlockNodeInteraction(node); - } else { - lockNodeInteraction(node); - } - } - } -}} +onDoubleClick={() => { + const node = idContainer.current && nodes.find((n) => n.id === idContainer.current); + if (!node) return; + + const isLocked = interactionLockedNodes.some((n) => n.id === node.id); + isLocked ? unlockNodeInteraction(node) : lockNodeInteraction(node); +}}
218-223: Consider moving Unlock All button to a more prominent locationThe "Unlock All" button's position at the bottom-right corner might not be immediately visible to users.
Consider moving it to the top of the panel or adding it to the context menu for better discoverability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
ui/src/features/Editor/components/LeftPanel/index.tsx(4 hunks)
🧰 Additional context used
🪛 GitHub Check: ci-ui / ci
ui/src/features/Editor/components/LeftPanel/index.tsx
[failure] 178-178:
Unexpected var, use let or const instead
🔇 Additional comments (1)
ui/src/features/Editor/components/LeftPanel/index.tsx (1)
178-179:
Replace var with useRef for idContainer
Using var for storing selected item ID can lead to issues with React's rendering lifecycle.
This issue was previously identified. Please implement the suggested fix using useRef.
🧰 Tools
🪛 GitHub Check: ci-ui / ci
[failure] 178-178:
Unexpected var, use let or const instead
| ...(nodes?.some((n) => n.type === "transformer") | ||
| ? [ | ||
| { | ||
| id: "transformer", | ||
| name: t("Transformers"), | ||
| icon: Lightning, | ||
| children: nodes | ||
| ?.filter((n) => n.type === "transformer") | ||
| .map((n) => ({ | ||
| id: n.id, | ||
| name: n.data.name ?? "untitled", | ||
| icon: isNodeLocked(n.id) ? Lock : Lightning, | ||
| })), | ||
| }, | ||
| ] | ||
| : []), | ||
| ...(nodes?.some((n) => n.type === "note") | ||
| ? [ | ||
| { | ||
| id: "note", | ||
| name: t("Note"), | ||
| icon: Note, | ||
| children: nodes | ||
| ?.filter((n) => n.type === "note") | ||
| .map((n) => ({ | ||
| id: n.id, | ||
| name: n.data.name ?? "untitled", | ||
| icon: isNodeLocked(n.id) ? Lock : Note, | ||
| style: isNodeLocked(n.id) | ||
| ? { color: "red", cursor: "not-allowed" } | ||
| : undefined, // 잠긴 노드는 회색 및 커서 변경 | ||
| children: { | ||
| id: n.id, | ||
| name: n.data.content ?? "untitled", | ||
| }, | ||
| })), | ||
| }, | ||
| ] | ||
| : []), | ||
| ...(nodes?.some((n) => n.type === "subworkflow") | ||
| ? [ | ||
| { | ||
| id: "subworkflow", | ||
| name: t("Subworkflow"), | ||
| icon: Plus, | ||
| children: nodes | ||
| ?.filter((n) => n.type === "subworkflow") | ||
| .map((n) => ({ | ||
| id: n.id, | ||
| name: n.data.name ?? "untitled", | ||
| icon: isNodeLocked(n.id) ? Lock : Plus, | ||
| })), | ||
| }, | ||
| ] | ||
| : []), | ||
| ...(nodes?.some((n) => n.type === "batch") | ||
| ? [ | ||
| { | ||
| id: "batch", | ||
| name: t("Batch Node"), | ||
| icon: RectangleDashed, | ||
| children: nodes | ||
| ?.filter((n) => n.type === "batch") | ||
| .map((n) => ({ | ||
| id: n.id, | ||
| name: n.data.name ?? "untitled", | ||
| icon: isNodeLocked(n.id) ? Lock : RectangleDashed, | ||
| children: nodes | ||
| ?.filter((d) => d.parentId === n.id) | ||
| .map((d) => ({ | ||
| id: d.id, | ||
| name: d.data.name ?? "untitled", | ||
| icon: getNodeIcon(d.type), | ||
| })), | ||
| })), | ||
| }, | ||
| ] | ||
| : []), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor repeated node mapping pattern
The code repeats similar mapping logic for different node types. Consider extracting this to a reusable function.
type NodeType = "transformer" | "note" | "subworkflow" | "batch";
function createNodeGroup(
nodes: Node[],
type: NodeType,
groupName: string,
groupIcon: Icon,
options?: { includeChildren?: boolean }
): TreeDataItem | null {
const typeNodes = nodes?.filter((n) => n.type === type);
if (!typeNodes?.length) return null;
return {
id: type,
name: t(groupName),
icon: groupIcon,
children: typeNodes.map((n) => ({
id: n.id,
name: n.data.name ?? "untitled",
icon: isNodeLocked(n.id) ? Lock : groupIcon,
...(options?.includeChildren && {
children: nodes
?.filter((d) => d.parentId === n.id)
.map((d) => ({
id: d.id,
name: d.data.name ?? "untitled",
icon: getNodeIcon(d.type),
})),
}),
})),
};
}
// Usage:
const treeContent: TreeDataItem[] = [
createNodeGroup(nodes, "transformer", "Transformers", Lightning),
createNodeGroup(nodes, "note", "Note", Note),
createNodeGroup(nodes, "subworkflow", "Subworkflow", Plus),
createNodeGroup(nodes, "batch", "Batch Node", RectangleDashed, { includeChildren: true }),
].filter(Boolean);
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
ui/src/features/Editor/components/LeftPanel/index.tsx (4)
34-34: Consider adding TypeScript types for the options parameterThe
optionsparameter inonNodeLockingcould benefit from a proper type definition for better type safety and documentation.- onNodeLocking: (nodeId: string, options?: { source: string }) => void; + type NodeLockingOptions = { + source: string; + }; + onNodeLocking: (nodeId: string, options?: NodeLockingOptions) => void;
158-173: Optimize node icon mapping with a lookup objectThe switch statement could be replaced with a more maintainable and performant lookup object.
- function getNodeIcon(type: string | undefined) { - switch (type) { - case "note": - return Note; - case "subworkflow": - return Plus; - case "transformer": - return Lightning; - case "reader": - return Database; - case "writer": - return Disc; - default: - return Circle; - } - } + const NODE_ICON_MAP = { + note: Note, + subworkflow: Plus, + transformer: Lightning, + reader: Database, + writer: Disc, + } as const; + + function getNodeIcon(type: string | undefined) { + return NODE_ICON_MAP[type as keyof typeof NODE_ICON_MAP] ?? Circle; + }
200-211: Extract double-click handler to a separate functionThe double-click handler logic is complex enough to warrant extraction into a named function for better readability and maintainability.
+ const handleNodeLockToggle = (nodeId: string) => { + const node = nodes.find((n) => n.id === nodeId); + if (node) { + if (interactionLockedNodes.some((n) => n.id === node.id)) { + unlockNodeInteraction(node); + } else { + lockNodeInteraction(node); + } + } + }; + onDoubleClick={() => { - if (idContainer) { - const node = nodes.find((n) => n.id === idContainer); - if (node) { - if (interactionLockedNodes.some((n) => n.id === node.id)) { - unlockNodeInteraction(node); - } else { - lockNodeInteraction(node); - } - } - } + if (idContainer) handleNodeLockToggle(idContainer); }}
215-219: Consider accessibility and positioning of the Unlock All buttonThe absolute positioning of the unlock button might overlap with content in the tree view. Also, it should have proper accessibility attributes.
<button onClick={unlockAllNodes} - className="absolute bottom-2 right-2 w-24 h-8 rounded-lg bg-red-500 text-white text-xs flex items-center justify-center shadow-md hover:bg-red-400"> + className="mt-auto mx-2 mb-2 w-24 h-8 rounded-lg bg-red-500 text-white text-xs flex items-center justify-center shadow-md hover:bg-red-400" + aria-label="Unlock all nodes" + title="Unlock all nodes"> Unlock All </button>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
ui/src/features/Editor/components/LeftPanel/index.tsx(4 hunks)
🔇 Additional comments (2)
ui/src/features/Editor/components/LeftPanel/index.tsx (2)
175-175: Use useRef instead of var for idContainer to persist value across renders
Using a mutable variable directly in a React component can lead to unexpected behavior.
81-155: Refactor repeated node mapping pattern
The code contains repeated patterns for different node types which could be extracted into a reusable function.
There was a problem hiding this comment.
This isn't necessary. We already have randomID. randomID's length prop is set to 24 as a default value, but if a value is passed in to the function it'll make an id of that length.
Overview
I made LeftPanel instantly reflect the nodes on the current canvas, and I created a new lock that works with LeftPanel.
What I've done
What I haven't done
Lock still allows certain interactions on some nodes
How I tested
Manually
Screenshot
Which point I want you to review particularly
Memo
Summary by CodeRabbit
Release Notes
New Features
LockerProviderto manage locking state across components.Localization
Bug Fixes
Chores