fix: wf ai assistant more accurate steps #3615
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@coderabbitai review |
Kiryous
changed the title
📝 WalkthroughWalkthroughThis pull request introduces changes to the workflow management system. A new safe node insertion method, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AddStepUI
participant WorkflowStore
participant ErrorToast
User ->> AddStepUI: Initiate add step action
AddStepUI ->> WorkflowStore: Call addNodeBetweenSafe(nodeOrEdgeId, step, type)
alt Insertion succeeds
WorkflowStore -->> AddStepUI: Return new node/edge ID
else Error occurs
WorkflowStore -->> AddStepUI: Throw error
AddStepUI ->> ErrorToast: Display error message
end
sequenceDiagram
participant User
participant BuilderChat
participant ProtectedChecker
participant WorkflowStore
User ->> BuilderChat: Request removal of a step
BuilderChat ->> ProtectedChecker: isProtectedStep(stepId)
alt Step is protected
ProtectedChecker -->> BuilderChat: Deny removal
BuilderChat -->> User: Show protected step error
else
ProtectedChecker -->> BuilderChat: Allow removal
BuilderChat ->> WorkflowStore: removeStepNode(stepId)
WorkflowStore -->> BuilderChat: Updated workflow state
BuilderChat -->> User: Confirm removal
end
Possibly related PRs
Suggested labels
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
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: 0
🧹 Nitpick comments (10)
keep-ui/features/workflows/builder/ui/BuilderChat/builder-chat.tsx (5)
107-114: Consider externalizing special step IDs.
Implementation is correct, but for better maintainability and clarity, you could store these protected IDs in a dedicated constant or enum.
116-130: Optional unification of Skeleton components.
Defining your own skeleton placeholder is fine, but it might be worth unifying the pattern across the app for consistent loading states.
293-319: Improve user flow and validation for removeStepNode.
Usingconfirm()may interrupt the user experience. Consider a custom confirmation modal and verify that the node exists before attempting deletion.
335-365: DRY up logic between removeStepNode and removeTriggerNode.
They share similar behaviors. Consolidating them into one reusable function could avoid code duplication.
692-803: Opportunity to unify addStep and addAction logic.
They share a similar approach; refactoring could minimize duplication and improve maintainability.keep-ui/features/workflows/builder/ui/workflow-status.tsx (1)
23-34: Switching from<div>to<span>could affect layout.
Inline elements might behave differently in various containers. Confirm this doesn't break existing styling or introduce wrapping issues.keep-ui/features/workflows/builder/ui/BuilderChat/AddStepUI.tsx (1)
5-58: Typed approach for step addition is clearer.
Splitting props into “complete” vs. “executing” states improves code clarity. Consider further consolidating these patterns across the codebase to reduce duplication.keep-ui/features/workflows/builder/ui/BuilderChat/StepPreview.tsx (2)
19-26: Simplify redundant conditions.The function contains duplicate conditions that can be simplified.
Apply this diff to remove redundancy:
function getStepIconUrl(data: V2Step | V2StepTrigger) { const { type } = data || {}; - if (type === "alert" || type === "workflow" || type === "trigger" || !type) - return "/keep.png"; - if (type === "incident" || type === "workflow" || type === "trigger" || !type) + if (!type || ["alert", "incident", "workflow", "trigger"].includes(type)) return "/keep.png"; return `/icons/${normalizeStepType(type)}-icon.png`; }
28-48: LGTM! Well-structured YAML generation with proper error handling.The function effectively consolidates YAML generation logic for different step types with proper error handling and type guards.
Address the TODO comment about other types.
The TODO comment indicates that support for additional types needs to be implemented.
Would you like me to help identify the missing types and implement the necessary logic?
keep-ui/entities/workflows/model/workflow-store.ts (1)
268-289: Address the TODO comment for better error handling.The error handling can be improved by extracting more meaningful error messages from ZodError. Currently, there's a TODO comment indicating this need.
Consider implementing a custom error message extractor:
- // TODO: extract meaningful error from ZodError - const validationError = fromError(error); - showErrorToast(validationError); + const validationError = fromError(error); + const errorMessage = validationError.details + .map(detail => detail.message) + .join('. '); + showErrorToast(errorMessage);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
keep-ui/entities/workflows/model/__tests__/workflow-store.test.tsx(2 hunks)keep-ui/entities/workflows/model/types.ts(3 hunks)keep-ui/entities/workflows/model/workflow-store.ts(3 hunks)keep-ui/features/workflows/builder/ui/BuilderChat/AddStepUI.tsx(2 hunks)keep-ui/features/workflows/builder/ui/BuilderChat/AddTriggerUI.tsx(1 hunks)keep-ui/features/workflows/builder/ui/BuilderChat/StepPreview.tsx(1 hunks)keep-ui/features/workflows/builder/ui/BuilderChat/builder-chat.tsx(11 hunks)keep-ui/features/workflows/builder/ui/WorkflowToolbox.tsx(1 hunks)keep-ui/features/workflows/builder/ui/workflow-status.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: tests-e2e (postgres)
- GitHub Check: tests-e2e (mysql)
🔇 Additional comments (14)
keep-ui/features/workflows/builder/ui/BuilderChat/builder-chat.tsx (7)
10-17: New imports for step and condition schemas look good.
They align well with the subsequent usage throughout the file.
47-48: Loading skeleton and AddStepUI imports look fine.
No issues identified, implementation is straightforward.
376-394: AddManualTrigger flow is consistent.
The loading and complete states match the established pattern. Implementation looks good.
419-422: Possibility of a loading fallback for alert properties.
Reading alert properties is straightforward, but ensure you have a fallback UI if data fetching takes longer.
424-485: Validate data types for alertFilters.
Currently, everything is treated as a string. If numeric or boolean values are needed, confirm the system can handle them properly.
669-689: Verify uniqueness of actionName.
Implementation looks fine, but ensure 'actionName' is unique or properly handled to avoid collisions.
805-903: Condition addition logic looks solid.
Implementation is straightforward, with clear error handling.keep-ui/features/workflows/builder/ui/BuilderChat/AddTriggerUI.tsx (1)
59-63: LGTM! Improved error handling with addNodeBetweenSafe.The change to use
addNodeBetweenSafealigns with the safer node addition approach, and the error handling inhandleAddTriggeris comprehensive.keep-ui/features/workflows/builder/ui/WorkflowToolbox.tsx (1)
26-30: LGTM! Consistent use of addNodeBetweenSafe.The change to use
addNodeBetweenSafemaintains consistency with the safer node addition approach across the codebase.keep-ui/entities/workflows/model/types.ts (2)
173-178: LGTM! Well-structured condition type definitions.The new union type properly combines condition schemas and provides a clear type definition.
376-376:❓ Verification inconclusive
Verify the impact of deleteNodes return type change.
The return type of
deleteNodeshas been changed fromvoidtostring[]. Ensure all callers handle the returned array of deleted node IDs correctly.
🏁 Script executed:
#!/bin/bash # Search for deleteNodes usage rg -A 5 'deleteNodes\(' --type tsLength of output: 3122
Action: Verify that all callers are intentionally handling (or intentionally ignoring) the new
string[]return value fromdeleteNodes.
- In the tests (
workflow-store.test.tsx), the call todeleteNodesignores the returned array. Confirm that no additional validation of the deleted node IDs is needed there.- In
BuilderChat(builder-chat.tsx), the returned array is captured and checked, which is the expected handling.- In
NodeMenu.tsx, the function is invoked without using its return value. Ensure this is acceptable given the new API behavior.Please review these usages to make sure that the impact of the return type change is fully intentional and does not lead to any overlooked side effects.
keep-ui/entities/workflows/model/__tests__/workflow-store.test.tsx (1)
69-70: LGTM! Test cases updated to use the new safe method.The test cases have been correctly updated to use
addNodeBetweenSafeinstead ofaddNodeBetween, maintaining the same test coverage and validation logic.Also applies to: 112-113
keep-ui/entities/workflows/model/workflow-store.ts (2)
259-267: LGTM! Improved implementation ofaddNodeBetween.The method has been enhanced with better return type handling and state management.
486-487: LGTM! Consistent return type indeleteNodes.The method now consistently returns an array in all cases, improving type safety and predictability.
Also applies to: 491-492, 573-574
Closes #3614
todo