feat(ui): Hide Reader and Writer options in toolbox if subworkflow #693
Conversation
WalkthroughThe changes introduce a new import of Changes
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
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: 0
🧹 Outside diff range and nitpick comments (2)
ui/src/features/Editor/components/OverlayUI/components/Toolbox/index.tsx (2)
40-41: Add error handling for workflow contextWhile the hook usage is correct, consider adding error handling for cases where the workflow context might be undefined.
- const [workflowId] = useCurrentWorkflowId(); + const [workflowId = "main"] = useCurrentWorkflowId();
75-78: Extract magic string and improve filtering logicThe implementation correctly hides reader and writer nodes in subworkflows, but could be more maintainable.
Consider these improvements:
+ const MAIN_WORKFLOW_ID = "main"; + const EXCLUDED_SUBWORKFLOW_TOOLS: NodeType[] = ["reader", "writer"]; const availableTools = - workflowId === "main" + workflowId === MAIN_WORKFLOW_ID ? allTools - : allTools.filter((tool) => tool.id !== "reader" && tool.id !== "writer"); + : allTools.filter((tool) => !EXCLUDED_SUBWORKFLOW_TOOLS.includes(tool.id));This makes the code more maintainable and easier to update if the list of excluded tools changes in the future.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ui/src/features/Editor/components/OverlayUI/components/Toolbox/index.tsx(3 hunks)
🔇 Additional comments (2)
ui/src/features/Editor/components/OverlayUI/components/Toolbox/index.tsx (2)
17-17: LGTM: Clean import of workflow context hook
The addition of useCurrentWorkflowId hook follows React best practices for managing workflow context.
75-78: Verify impact on existing workflow functionality
While the implementation correctly achieves the PR objective, let's verify the integration points.
✅ Verification successful
No impact on existing functionality detected
The implementation is isolated to the Toolbox component and the condition is specific to subworkflow behavior. The search results show:
- The Toolbox component is only imported for UI purposes in WorkspaceSettings and WorkspaceLeftPanel
- No other components rely on the workflowId === "main" logic
- The change doesn't affect any existing tests as there are no direct Toolbox component tests
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other components that might be affected by this change
rg -l "useCurrentWorkflowId|workflowId.*main" --type tsx
# Look for any tests that might need updating
fd -e test.tsx -e spec.tsx
Length of output: 115
Script:
#!/bin/bash
# Search for other components that might be affected by this change, using TypeScript extensions
rg -l "useCurrentWorkflowId|workflowId.*main" -g "*.ts" -g "*.tsx"
# Look for any tests that might need updating
fd . -e .test.tsx -e .test.ts -e .spec.tsx -e .spec.ts
# Search for toolbox-related tests and components
rg -l "Toolbox" -g "*.test.tsx" -g "*.test.ts" -g "*.spec.tsx" -g "*.spec.ts"
# Search for workflow-related hooks and utilities
rg -l "useCurrentWorkflow" -g "*.ts" -g "*.tsx"
Length of output: 1062
Script:
#!/bin/bash
# Let's check the Toolbox component's test coverage and related workflow tests
rg -g "*.test.{ts,tsx}" -g "*.spec.{ts,tsx}" -C 5 "Toolbox|workflowId.*main"
# Check for any workflow-related components that might interact with the Toolbox
rg -g "*.{ts,tsx}" -C 3 "import.*Toolbox|from.*Toolbox"
# Look for any workflow ID related logic in the codebase
rg -g "*.{ts,tsx}" -C 3 "workflowId === 'main'"
Length of output: 1543
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
ui/src/stores/index.ts (2)
5-5: Consider using a more specific type for workflow IDWhile the current type
string | undefinedworks, consider using a union type to explicitly define allowed values since it appears the workflow ID can only be "main" or undefined based on the PR context.-const currentWorkflowId = atom<string | undefined>("main"); +type WorkflowId = "main" | undefined; +const currentWorkflowId = atom<WorkflowId>("main");
5-6: Add documentation for the workflow ID atomPlease add JSDoc comments to document:
- The purpose of this atom
- The significance of the "main" value
- How it affects the toolbox visibility
+/** + * Atom to track the current workflow context. + * - "main": Indicates the main workflow where all tools are visible + * - undefined: Indicates a subworkflow where reader/writer tools are hidden + */ const currentWorkflowId = atom<string | undefined>("main"); export const useCurrentWorkflowId = () => useAtom(currentWorkflowId);
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
ui/src/features/Editor/components/OverlayUI/components/Toolbox/index.tsx (1)
76-80: Consider improving filtering logic readability and performanceWhile the logic is correct, consider these improvements for better maintainability:
- const availableTools = - workflowId === DEFAULT_ENTRY_GRAPH_ID - ? allTools - : allTools.filter((tool) => tool.id !== "reader" && tool.id !== "writer"); + // Hide reader and writer nodes in subworkflows + const isMainWorkflow = workflowId === DEFAULT_ENTRY_GRAPH_ID; + const restrictedToolTypes = new Set(["reader", "writer"]); + const availableTools = isMainWorkflow + ? allTools + : allTools.filter((tool) => !restrictedToolTypes.has(tool.id));This refactor:
- Adds a descriptive comment explaining the business logic
- Introduces a clearer variable name
isMainWorkflow- Uses Set for potentially better performance with larger tool sets
- Improves readability of the filtering condition
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ui/src/features/Editor/components/OverlayUI/components/Toolbox/index.tsx(3 hunks)
🔇 Additional comments (4)
ui/src/features/Editor/components/OverlayUI/components/Toolbox/index.tsx (4)
17-18: LGTM: Clean import additions
The new imports are well-organized and necessary for the workflow context functionality.
Line range hint 43-75: LGTM: Clear separation of tool definitions
The renaming from availableTools to allTools improves code clarity by clearly separating the complete set of tools from the filtered subset.
41-41: Verify hook behavior in edge cases
The hook usage looks correct, but let's ensure it handles all scenarios properly:
- Initial render with undefined workflowId
- Transitions between main workflow and subworkflows
✅ Verification successful
Hook implementation and usage verified as correct
The hook is properly implemented and safely handles all scenarios:
- Initialized with a default value of "main" in the atom
- Used consistently across components for workflow management
- Properly handles transitions between workflows as evidenced by the WorkflowTabs component implementation
- Safely destructured with only the state value where the setter isn't needed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for hook usage and initialization to verify proper handling
ast-grep --pattern 'const currentWorkflowId = atom<$_>($_)'
ast-grep --pattern 'export const useCurrentWorkflowId = $_'
Length of output: 303
Script:
#!/bin/bash
# Let's check for all usages of useCurrentWorkflowId to understand the patterns
rg "useCurrentWorkflowId" -A 2
# Also check for any transitions or updates to currentWorkflowId
rg "currentWorkflowId" -A 2
Length of output: 5932
76-80: Verify UI behavior matches provided screenshots
The implementation logic looks correct, but please ensure:
- The toolbox UI updates immediately when switching between main workflow and subworkflows
- The transition is smooth without visual glitches
- The layout adjusts properly when tools are hidden
Overview
I made if the current tab is subworkflow, reader and writer node in the toolbox are not visible.
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