fix(ui): input/output router filtering for main workflow #746
Conversation
WalkthroughThe pull request introduces a new Changes
Sequence DiagramsequenceDiagram
participant Editor
participant LeftPanel
participant ActionsList
participant UseApi
Editor->>LeftPanel: Pass isMainWorkflow
LeftPanel->>ActionsList: Forward isMainWorkflow
ActionsList->>UseApi: Request actions with filter
UseApi-->>ActionsList: Return filtered actions
ActionsList->>ActionsList: Render filtered actions
Possibly related PRs
Suggested reviewers
Poem
Finishing Touches
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 canceled.
|
…xrouter-actions-main-workflow
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
ui/src/features/Editor/components/OverlayUI/components/NodePickerDialog/index.tsx (2)
39-44: Consider performance implications of removing search debouncing.The removal of debouncing for the search functionality might cause unnecessary API calls with each keystroke. Consider re-implementing debouncing to optimize performance.
+import { debounce } from "lodash-es"; + const { useGetActionsSegregated } = useAction(i18n.language); +const debouncedSetSearchTerm = debounce(setSearchTerm, 300); const { actions } = useGetActionsSegregated({ isMainWorkflow, searchTerm, type: openedActionType?.nodeType, });
93-93: Update the search handler to use debounced function.Update the onChange handler to use the debounced function for better performance.
- onChange={(e) => setSearchTerm(e.target.value)} + onChange={(e) => debouncedSetSearchTerm(e.target.value)}ui/src/lib/fetch/transformers/useApi.ts (2)
12-45: Add TypeScript types and JSDoc documentation.The
filterActionsfunction would benefit from explicit TypeScript types and documentation explaining its purpose and parameters.+/** + * Filters actions based on main workflow status and search term. + * @param actions - Array of actions to filter + * @param filter - Optional filter parameters + * @returns Filtered array of actions + */ -const filterActions = ( +const filterActions = <T extends Action>( - actions: Action[], + actions: T[], filter?: { isMainWorkflow: boolean; searchTerm?: string; }, -) => { +): T[] => { if (!actions) return [];
64-73: Memoize the filterBySearchTerm function.The
filterBySearchTermfunction could be memoized to prevent unnecessary recalculations for the same action and search term.+import { useMemo } from "react"; + -const filterBySearchTerm = (action: Action, searchTerm?: string) => { +const filterBySearchTerm = useMemo(() => (action: Action, searchTerm?: string) => { return Object.values(action).some((value) => { const strValue = Array.isArray(value) ? value.join(" ") : typeof value === "string" ? value : String(value); return strValue.toLowerCase().includes(searchTerm?.toLowerCase() ?? ""); }); -}; +}, []);ui/src/features/Editor/components/LeftPanel/components/Actions/index.tsx (1)
Line range hint
82-95: Consider improving node positioning UXWhen adding a new node on double-click, it's positioned at screen center which might not be optimal if the user has scrolled or zoomed. Consider:
- Using the click position as the node's initial position
- Ensuring the new node doesn't overlap with existing nodes
- position: screenToFlowPosition({ - x: window.innerWidth / 2, - y: window.innerHeight / 2, - }), + // Use click position and adjust for scroll/zoom + position: screenToFlowPosition({ + x: event.clientX, + y: event.clientY, + }),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
ui/src/features/Editor/components/LeftPanel/components/Actions/index.tsx(4 hunks)ui/src/features/Editor/components/LeftPanel/index.tsx(2 hunks)ui/src/features/Editor/components/OverlayUI/components/NodePickerDialog/index.tsx(6 hunks)ui/src/features/Editor/components/OverlayUI/components/Toolbox/index.tsx(0 hunks)ui/src/features/Editor/components/OverlayUI/index.tsx(1 hunks)ui/src/features/Editor/index.tsx(1 hunks)ui/src/lib/fetch/transformers/useApi.ts(2 hunks)
💤 Files with no reviewable changes (1)
- ui/src/features/Editor/components/OverlayUI/components/Toolbox/index.tsx
🔇 Additional comments (6)
ui/src/features/Editor/index.tsx (1)
65-65: LGTM! Clean prop addition.The
isMainWorkflowprop is correctly passed to theLeftPanelcomponent.Also applies to: 65-65
ui/src/features/Editor/components/LeftPanel/index.tsx (1)
103-109: LGTM! Clean implementation of ActionsList with isMainWorkflow prop.The prop is correctly passed to the ActionsList component.
ui/src/features/Editor/components/LeftPanel/components/Actions/index.tsx (2)
29-32: LGTM: Props type updated to support workflow context filteringThe addition of
isMainWorkflowprop aligns with the PR objective to filter actions based on workflow context.
47-55: LGTM: Action fetching with workflow contextGood implementation of action filtering using
isMainWorkflowin both hooks. The search term is properly debounced and synchronized between the hooks.ui/src/features/Editor/components/OverlayUI/index.tsx (2)
77-79: LGTM: NodePickerDialog integration with workflow contextThe
isMainWorkflowprop is correctly passed to NodePickerDialog, enabling proper action filtering.
Line range hint
1-85: Verify isMainWorkflow usage in child componentsThe
isMainWorkflowprop is passed to both NodePickerDialog and Toolbox. Let's verify its implementation in these components.✅ Verification successful
isMainWorkflow prop is correctly implemented across components
The isMainWorkflow prop is properly implemented:
- In Toolbox: Controls visibility of reader/writer tools
- In NodePickerDialog: Affects available actions through useGetActionsSegregated
- Implementation is consistent and forms a logical pattern for differentiating main workflow from sub-workflows
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check how isMainWorkflow is used in NodePickerDialog and Toolbox # Search for isMainWorkflow prop usage in component files echo "Checking NodePickerDialog implementation:" rg -A 5 "isMainWorkflow" "ui/src/features/Editor/components/OverlayUI/components/NodePickerDialog" echo "Checking Toolbox implementation:" rg -A 5 "isMainWorkflow" "ui/src/features/Editor/components/OverlayUI/components/Toolbox"Length of output: 3908
Script:
#!/bin/bash # Check the hook implementation that uses isMainWorkflow in NodePickerDialog echo "Searching for hook implementation in NodePickerDialog:" rg -B 5 -A 10 "useAvailableNodes" "ui/src/features/Editor/components/OverlayUI/components/NodePickerDialog" # Also check if there are any related hooks in the feature directory echo -e "\nSearching for related hooks in the feature directory:" rg -B 5 -A 10 "useAvailableNodes" "ui/src/features/Editor"Length of output: 404
Script:
#!/bin/bash # Check imports in NodePickerDialog echo "Checking imports in NodePickerDialog:" rg "^import" "ui/src/features/Editor/components/OverlayUI/components/NodePickerDialog" # Search for hook files in the feature directory echo -e "\nSearching for hook files:" fd "hooks?" "ui/src/features/Editor" # If we find any hook files, check their content echo -e "\nChecking content of any hook files for available nodes:" rg -B 5 -A 10 "availableNodes" "ui/src/features/Editor"Length of output: 2074
Script:
#!/bin/bash # Check useAction implementation echo "Checking useAction implementation:" rg -B 5 -A 10 "useAction" "ui/src/features/Editor" # Check Canvas hooks content echo -e "\nChecking Canvas hooks content:" cat "ui/src/features/Editor/components/Canvas/hooks.ts" # Check main hooks file echo -e "\nChecking main hooks file:" cat "ui/src/features/Editor/hooks.ts"Length of output: 18257
|
|
||
| import { useFetch } from "./useFetch"; | ||
|
|
||
| const filterActions = ( |
There was a problem hiding this comment.
Having functions used inside a custom hook below the custom hook has become kind of a convention. IF you can move these below that would be awesome 🙏🏻
| searchTerm?: string; | ||
| }, | ||
| ) => { | ||
| if (!actions) return []; |
There was a problem hiding this comment.
Actions will never be undefined/null. Do you mean if actions.length < 1 or something?
There was a problem hiding this comment.
My bad! Yeah I meant if the length is under 1 you are correct!
| if (filter?.isMainWorkflow) { | ||
| result = result.filter( | ||
| (action) => !action.name.toLowerCase().includes("router"), | ||
| ); | ||
| } | ||
|
|
||
| if (filter?.searchTerm) { | ||
| result = result.filter((action) => | ||
| Object.values(action).some((value) => { | ||
| const strValue = Array.isArray(value) | ||
| ? value.join(" ") | ||
| : typeof value === "string" | ||
| ? value | ||
| : String(value); | ||
| return strValue | ||
| .toLowerCase() | ||
| .includes(filter.searchTerm?.toLowerCase() ?? ""); | ||
| }), | ||
| ); | ||
| } | ||
|
|
||
| return result; | ||
| }; |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ui/src/lib/fetch/transformers/useApi.ts (2)
51-79: Optimize the filtering logic in useGetActionsSegregated.The current implementation creates multiple intermediate objects. Consider simplifying:
const filteredData = useMemo(() => { - if (!data) return data; + if (!data?.byCategory || !data?.byType) return data; - let result = { ...data }; - - result = { + const result = { byCategory: filterActionsByPredicate( - result.byCategory, + data.byCategory, (action) => combinedFilter(action, filter), !!filter?.searchTerm, ), byType: filterActionsByPredicate( - result.byType, + data.byType, (action) => combinedFilter(action, filter), !!filter?.searchTerm, ), }; if (filter?.type && result.byType) { return { ...result, byType: { [filter.type]: result.byType[filter.type], }, }; } return result; }, [data, filter]);
147-156: Optimize search term filtering performance.The current implementation:
- Converts all values to strings, including non-searchable properties
- Performs case conversion on every iteration
- Might search through irrelevant properties
const filterBySearchTerm = (action: Action, searchTerm?: string) => { + if (!searchTerm) return true; + + const normalizedTerm = searchTerm.toLowerCase(); + const searchableFields = ['name', 'description', 'category', 'type']; + return Object.entries(action).some(([key, value]) => { + if (!searchableFields.includes(key)) return false; + const strValue = Array.isArray(value) ? value.join(" ") : typeof value === "string" ? value : String(value); - return strValue.toLowerCase().includes(searchTerm?.toLowerCase() ?? ""); + return strValue.toLowerCase().includes(normalizedTerm); }); };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ui/src/features/Editor/components/LeftPanel/index.tsx(2 hunks)ui/src/lib/fetch/transformers/useApi.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ui/src/features/Editor/components/LeftPanel/index.tsx
🔇 Additional comments (3)
ui/src/lib/fetch/transformers/useApi.ts (3)
25-28: Refine the null check condition.Based on previous discussions, actions will never be undefined/null. Consider using a more specific check:
- if (!data) return data; + if (!data?.length) return [];
93-156: Code organization looks good!The utility functions are correctly placed below the custom hook, following the established convention as suggested in the previous review.
19-22: Verify the filter implementation across components.The filter implementation looks good and addresses the PR objectives. However, let's verify that all components using these hooks are passing the correct
isMainWorkflowvalue:Also applies to: 44-48
✅ Verification successful
Implementation verified: isMainWorkflow filter is correctly integrated
The filter implementation is properly integrated across all components:
- Both hooks (useGetActions and useGetActionsSegregated) correctly implement the filter
- All components pass the isMainWorkflow prop consistently
- Router filtering logic is properly applied when isMainWorkflow is true
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of useGetActions and useGetActionsSegregated # to ensure isMainWorkflow is correctly passed echo "Checking hook usage patterns..." rg -A 3 "useGetActions|useGetActionsSegregated" --type ts --type tsx echo "\nChecking isMainWorkflow prop declarations..." ast-grep --pattern 'interface $interface { $$$ isMainWorkflow: boolean $$$ }'Length of output: 360
Script:
#!/bin/bash # Search in both .ts and .tsx files with more context echo "Searching in TypeScript files..." rg -t ts "useGetActions|useGetActionsSegregated" -B 2 -A 5 echo "\nSearching for isMainWorkflow usage..." rg "isMainWorkflow" -t ts -B 2 -A 2 echo "\nChecking for interface declarations..." ast-grep --pattern 'interface $_ { $$$ isMainWorkflow: $_ $$$ }'Length of output: 18845
| if (filter?.isMainWorkflow && action.name.toLowerCase().includes("router")) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make the router check more robust.
The current implementation has several potential issues:
- Hardcoded string check for "router"
- Case sensitivity might miss some cases
- No type checking to ensure it's specifically an input/output router
- if (filter?.isMainWorkflow && action.name.toLowerCase().includes("router")) {
+ const ROUTER_TYPES = ["input_router", "output_router"];
+ if (filter?.isMainWorkflow &&
+ (ROUTER_TYPES.includes(action.type) ||
+ action.name.toLowerCase().includes("router"))) {
return false;
}Committable suggestion skipped: line range outside the PR's diff.
Overview
input and output routers were available in main workflow but should only be available on subwork flows.
What I've done
Added some helper functions to filter accordingly whether it is the main workflow or a subwork flow.
What I haven't done
How I tested
Manually
Screenshot
Which point I want you to review particularly
Memo
Summary by CodeRabbit
Release Notes
New Features
Improvements
Technical Updates