fix(ui): Navigation arrow logic node history added. #716
Conversation
WalkthroughThis pull request introduces a new custom React hook Changes
Sequence DiagramsequenceDiagram
participant User
participant ParamEditor
participant RightPanel
participant useNodeHistory
User->>ParamEditor: Click Navigate Buttons
ParamEditor->>RightPanel: Call onNavigateNodeHistory
RightPanel->>useNodeHistory: Trigger handleNavigateNodeHistory
useNodeHistory-->>RightPanel: Update Selected Node
RightPanel-->>ParamEditor: Update Node History Position
ParamEditor->>User: Render Updated Node
Possibly related PRs
Suggested reviewers
Poem
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: 1
🧹 Nitpick comments (4)
ui/src/features/Editor/components/Canvas/useNodeHistory.ts (1)
5-22: Types could be more strictly definedThe type definitions look good overall, but could benefit from some improvements:
- The
Nodetype is imported but its shape is unclearanytype insetSelectedNodecould be more specificConsider updating the types:
type NavigateNodeHistoryProps = { direction: "prev" | "next"; history: HistoryState; // Use HistoryState type instead of object literal - setSelectedNode: (node: Node) => void; + setSelectedNode: React.Dispatch<React.SetStateAction<Node | undefined>>; };ui/src/features/Editor/components/RightPanel/index.tsx (1)
Line range hint
26-40: Consider extracting click event dispatch logicThe click event dispatch logic for node unlocking is marked as "hacky" in the comments.
Consider extracting this into a utility function:
const dispatchPaneClick = () => { const paneElement = document.getElementsByClassName("react-flow__pane")[0]; if (!paneElement) return; const clickEvent = new Event("click", { bubbles: true, cancelable: true }); paneElement.dispatchEvent(clickEvent); };ui/src/features/Editor/components/RightPanel/components/ParamEditor/index.tsx (2)
15-19: Consider type improvements and add JSDoc documentationThe new props would benefit from better type definitions and documentation:
+/** Direction type for node history navigation */ +type NavigationDirection = "prev" | "next"; + +/** Position state for node history navigation */ +type NodeHistoryPosition = { + canGoBack: boolean; + canGoForward: boolean; +}; + type Props = { nodeId: string; nodeMeta: NodeData; nodeType: string; nodeParameters?: unknown; // TODO: define type onSubmit: (nodeId: string, data: any) => void; - onNavigateNodeHistory: (direction: "prev" | "next") => void; + /** Callback for navigating through node history */ + onNavigateNodeHistory: (direction: NavigationDirection) => void; - nodeHistoryPosition: { - canGoBack: boolean; - canGoForward: boolean; - }; + /** Current position state in node history */ + nodeHistoryPosition: NodeHistoryPosition; };
44-56: Enhance accessibility and optimize performanceWhile the navigation implementation is functionally correct, consider these improvements:
+const ParamEditor: React.FC<Props> = memo(({ nodeId, nodeMeta, onSubmit, onNavigateNodeHistory, nodeHistoryPosition, }) => { + const handlePrevClick = useCallback(() => onNavigateNodeHistory("prev"), [onNavigateNodeHistory]); + const handleNextClick = useCallback(() => onNavigateNodeHistory("next"), [onNavigateNodeHistory]); return ( <div> <div className="mb-3 flex justify-between gap-4"> <div className="flex gap-2"> <IconButton className={actionButtonClasses} icon={<ArrowLeft />} tooltipText="Previous selection" - onClick={() => onNavigateNodeHistory("prev")} + onClick={handlePrevClick} disabled={!nodeHistoryPosition.canGoBack} + aria-label="Go to previous node" /> <IconButton className={actionButtonClasses} icon={<ArrowRight />} tooltipText="Next selection" - onClick={() => onNavigateNodeHistory("next")} + onClick={handleNextClick} disabled={!nodeHistoryPosition.canGoForward} + aria-label="Go to next node" /> </div> </div>This improves:
- Accessibility with proper aria-labels
- Performance by memoizing event handlers
- Component re-render efficiency by wrapping the entire component in memo
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ui/src/features/Editor/components/Canvas/useNodeHistory.ts(1 hunks)ui/src/features/Editor/components/RightPanel/components/ParamEditor/index.tsx(3 hunks)ui/src/features/Editor/components/RightPanel/index.tsx(4 hunks)ui/src/features/Editor/index.tsx(1 hunks)
🔇 Additional comments (7)
ui/src/features/Editor/components/Canvas/useNodeHistory.ts (2)
24-40: Navigation handler implementation is robust
The navigation logic correctly:
- Handles direction-based index calculation
- Performs bounds checking
- Updates both history and selected node
49-73: Verify history cleanup logic
The history cleanup logic in useEffect looks correct but complex. Let's verify the implementation:
- Filters history to only include valid nodes
- Checks for duplicate entries
- Updates history state
✅ Verification successful
The executed scripts show that there are no similar patterns of history cleanup logic in the codebase. The history cleanup implementation in useNodeHistory.ts is unique and follows a clear pattern:
- Creates a Set of valid node IDs from current nodes
- Filters history to only include valid nodes
- Checks for duplicates before adding new entries
- Updates the history state with proper indices
The implementation is correct and follows best practices
- The cleanup logic efficiently uses Set for O(1) lookups of valid nodes
- Properly handles edge cases by checking for duplicates
- Maintains history integrity by slicing at the current index when adding new entries
- Memory efficient by removing invalid nodes that no longer exist
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar patterns of history cleanup in the codebase
rg -U "validNodeIds.*filter.*has.*id"
# Look for potential memory leaks in useEffect dependencies
ast-grep --pattern 'useEffect($$$, [$$$])'
Length of output: 35109
ui/src/features/Editor/index.tsx (1)
102-102: Prop addition looks correct
The nodes prop is correctly passed to RightPanel, enabling node history functionality.
ui/src/features/Editor/components/RightPanel/index.tsx (2)
71-79: Verify ParamEditor prop updates
The changes to ParamEditor props look correct, using the new selectedNode state.
✅ Verification successful
The verification is complete based on the shell script results. I'll generate the final response:
ParamEditor props are correctly typed and match usage
The provided props in the code match exactly with the ParamEditor component's type definition:
nodeId(string)nodeMeta(NodeData)nodeType(string)onSubmit(callback)onNavigateNodeHistory(direction callback)nodeHistoryPosition(object with canGoBack/canGoForward)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check ParamEditor prop types
ast-grep --pattern 'type Props = {
$$$
onNavigateNodeHistory: $$$;
nodeHistoryPosition: $$$;
$$$
}'
Length of output: 1400
17-22: Hook integration looks good but verify prop types
The useNodeHistory hook is correctly integrated, but let's verify the prop types match.
✅ Verification successful
Let me analyze the prop types from the output. I can see that the useNodeHistory hook in Canvas/useNodeHistory.ts has matching prop types with the RightPanel component:
// useNodeHistory.ts
type Props = {
selected?: Node;
nodes: Node[];
};
// RightPanel/index.tsx
type Props = {
selected?: Node;
onParamsSubmit: (nodeId: string, data: any) => void;
nodes: Node[];
};Props match correctly with useNodeHistory hook
The prop types in RightPanel perfectly align with the requirements of useNodeHistory hook. The selected and nodes props are correctly typed and match the hook's interface, while onParamsSubmit is an additional prop specific to RightPanel's functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistent prop types across components
ast-grep --pattern 'type Props = {
$$$
nodes: Node[];
$$$
}'
Length of output: 5763
ui/src/features/Editor/components/RightPanel/components/ParamEditor/index.tsx (2)
30-31: LGTM!
Props destructuring is clean and follows consistent patterns.
Line range hint 8-14: Consider improving type safety
There are two areas that could benefit from better type definitions:
- The
nodeParametersprop has a TODO comment about defining its type - The
onSubmitcallback usesanytype for its data parameter
Let's check if we can find the correct types from usage:
| const memoizedNodeHistory = useCallback( | ||
| (direction: "prev" | "next") => { | ||
| handleNavigateNodeHistory({ | ||
| direction, | ||
| history, | ||
| setHistory, | ||
| setSelectedNode, | ||
| }); | ||
| }, | ||
| [history], | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Memoization could be improved
The memoized callback only depends on history but uses setHistory and setSelectedNode inside.
Add missing dependencies to useCallback:
const memoizedNodeHistory = useCallback(
(direction: "prev" | "next") => {
handleNavigateNodeHistory({
direction,
history,
setHistory,
setSelectedNode,
});
},
- [history],
+ [history, setHistory, setSelectedNode],
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const memoizedNodeHistory = useCallback( | |
| (direction: "prev" | "next") => { | |
| handleNavigateNodeHistory({ | |
| direction, | |
| history, | |
| setHistory, | |
| setSelectedNode, | |
| }); | |
| }, | |
| [history], | |
| ); | |
| const memoizedNodeHistory = useCallback( | |
| (direction: "prev" | "next") => { | |
| handleNavigateNodeHistory({ | |
| direction, | |
| history, | |
| setHistory, | |
| setSelectedNode, | |
| }); | |
| }, | |
| [history, setHistory, setSelectedNode], | |
| ); |
Overview
In the right panel for the parameter menu on a selected node the next and prev arrows did not
What I've done
What I haven't done
How I tested
manually
Screenshot
Which point I want you to review particularly
Memo
Summary by CodeRabbit
New Features
ParamEditorcomponent with navigation buttons for node history.RightPanelcomponent.Bug Fixes
Documentation