Workflow Builder UI Integration in Xyne Dashboard #745
Conversation
avirupsinha12
requested review from
junaid-shirur,
kalpadhwaryu,
shivamashtikar and
zereraz
as code owners
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a workflow feature: new /workflow authenticated route and sidebar entry; React Flow-based WorkflowBuilder, ActionBar, icons, types, utils, API handlers, and a FlowProvider with localStorage persistence. Also adds dependency Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant WB as WorkflowBuilder
participant TAPI as workflowTemplatesAPI
participant WAPI as workflowsAPI
participant RF as ReactFlowState
U->>WB: Click "Execute Node"
WB->>TAPI: instantiate(templateId, {name,...})
alt instantiate OK
TAPI-->>WB: { workflowId, rootStepId }
WB->>WAPI: run(workflowId)
alt run OK
WB->>WAPI: completeStep(rootStepId)
WAPI-->>WB: 200
WB->>WB: start polling loop
loop until terminal
WB->>WAPI: fetchById(workflowId)
WAPI-->>WB: Flow state
WB->>RF: update nodes (isActive/isCompleted)
end
else run error
WAPI-->>WB: error
WB->>RF: surface error
end
else instantiate error
TAPI-->>WB: error
WB->>RF: surface error
end
sequenceDiagram
autonumber
actor U as User
participant WB as WorkflowBuilder
participant RF as ReactFlowState
rect rgba(220,245,220,0.12)
note right of U: Authoring interactions
U->>WB: Add node / Add first step
WB->>RF: add node(s)
U->>WB: Connect nodes
WB->>RF: add edge (onConnect)
U->>WB: Select / Delete
WB->>RF: update nodes/edges
U->>WB: Change zoom
WB->>RF: zoomTo(percent)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @avirupsinha12, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces a significant new feature: a Workflow Builder UI within the Xyne Dashboard. The primary goal is to provide users with a visual, interactive tool to design and manage automated workflows. This involves integrating a specialized React library for node-based diagrams, defining the underlying data structures for workflows, setting up API communication for persistence, and creating a dedicated user interface accessible through the main application sidebar. The changes enable users to visually construct workflows by connecting various steps and triggers, enhancing the dashboard's automation capabilities.
Highlights
- Integration of
@xyflow/react: The core of this feature is the integration of@xyflow/react, a powerful library for creating interactive node-based diagrams. This enables the visual representation and manipulation of workflow steps and their connections. - New Workflow Page and Navigation: A new dedicated page at
/workflowhas been added, accessible via a new link in the sidebar. This page serves as the central hub for managing and building workflows, offering both a list view and a visual builder. - Defined Workflow Data Models: Comprehensive TypeScript types have been introduced for all workflow-related entities, including
Step,Flow,WorkflowTemplate, and various utility interfaces. This ensures strong typing and better code maintainability for the workflow domain. - Backend API Integration: New API handlers and endpoints have been implemented for interacting with workflow templates and workflows on the backend. This provides the necessary data persistence and retrieval mechanisms for the new feature.
- Interactive Workflow Builder UI Components: The Workflow Builder UI includes components for an action bar (with zoom and execute functionality), a custom
StepNodefor visual representation of workflow steps, and aTriggersSidebarfor selecting how workflows are initiated.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a new workflow builder feature, integrating it into the dashboard. It adds a new route, sidebar icon, and the main builder UI using @xyflow/react. The implementation includes components for the builder canvas, nodes, action bar, and sidebars, along with API handlers and type definitions. The overall structure is good, but there are several areas for improvement. I've found some critical type mismatches in the workflow builder's data structures, unused code, and hardcoded data in the workflow list page that should be replaced with API calls. Addressing these points will improve the correctness and maintainability of the new feature.
| contents: [{ | ||
| type: 'TRIGGER', | ||
| value: 'When chat message received' | ||
| }] |
There was a problem hiding this comment.
The structure of the objects in the contents array does not match the Content type defined in Types.ts. The Content type requires properties like id, display, and content_type, but here { type: 'TRIGGER', value: '...' } is used. This type mismatch is a critical issue that will likely cause runtime errors when this data is consumed.
| flow: Flow | ||
| setFlow: (flow: Flow) => void |
There was a problem hiding this comment.
The FlowContextProps interface defines flow as Flow and setFlow as (flow: Flow) => void. However, in WorkflowProvider.tsx, the state and setter handle Flow | null. This inconsistency can lead to type errors. The types here should be updated to allow for null values to match the provider's implementation.
| flow: Flow | |
| setFlow: (flow: Flow) => void | |
| flow: Flow | null | |
| setFlow: (flow: Flow | null) => void |
| data: { | ||
| step: { | ||
| id: '1', | ||
| name: 'Select trigger from the sidebar', | ||
| status: 'PENDING', | ||
| contents: [] | ||
| }, | ||
| isActive: false, | ||
| isCompleted: false | ||
| }, |
There was a problem hiding this comment.
The Step object being created here is missing the required type property, which is defined as a non-optional string in the Step type definition. This will cause TypeScript errors and potential runtime issues. Please add the type property to ensure the object conforms to the Step interface.
| data: { | |
| step: { | |
| id: '1', | |
| name: 'Select trigger from the sidebar', | |
| status: 'PENDING', | |
| contents: [] | |
| }, | |
| isActive: false, | |
| isCompleted: false | |
| }, | |
| data: { | |
| step: { | |
| id: '1', | |
| type: 'trigger', | |
| name: 'Select trigger from the sidebar', | |
| status: 'PENDING', | |
| contents: [] | |
| }, | |
| isActive: false, | |
| isCompleted: false | |
| }, |
| <div className="grid grid-cols-1 md:grid-cols-4 gap-4 mb-8"> | ||
| <Card className="dark:bg-slate-800"> | ||
| <CardContent className="p-4"> | ||
| <div className="flex items-center justify-between"> | ||
| <div> | ||
| <p className="text-sm text-gray-600 dark:text-gray-400">Total Workflows</p> | ||
| <p className="text-2xl font-bold text-gray-900 dark:text-gray-100">12</p> | ||
| </div> | ||
| <Workflow className="h-8 w-8 text-blue-600" /> | ||
| </div> | ||
| </CardContent> | ||
| </Card> | ||
|
|
||
| <Card className="dark:bg-slate-800"> | ||
| <CardContent className="p-4"> | ||
| <div className="flex items-center justify-between"> | ||
| <div> | ||
| <p className="text-sm text-gray-600 dark:text-gray-400">Active</p> | ||
| <p className="text-2xl font-bold text-green-600">8</p> | ||
| </div> | ||
| <Play className="h-8 w-8 text-green-600" /> | ||
| </div> | ||
| </CardContent> | ||
| </Card> | ||
|
|
||
| <Card className="dark:bg-slate-800"> | ||
| <CardContent className="p-4"> | ||
| <div className="flex items-center justify-between"> | ||
| <div> | ||
| <p className="text-sm text-gray-600 dark:text-gray-400">Shared</p> | ||
| <p className="text-2xl font-bold text-purple-600">5</p> | ||
| </div> | ||
| <Users className="h-8 w-8 text-purple-600" /> | ||
| </div> | ||
| </CardContent> | ||
| </Card> | ||
|
|
||
| <Card className="dark:bg-slate-800"> | ||
| <CardContent className="p-4"> | ||
| <div className="flex items-center justify-between"> | ||
| <div> | ||
| <p className="text-sm text-gray-600 dark:text-gray-400">Scheduled</p> | ||
| <p className="text-2xl font-bold text-orange-600">3</p> | ||
| </div> | ||
| <Clock className="h-8 w-8 text-orange-600" /> | ||
| </div> | ||
| </CardContent> | ||
| </Card> | ||
| </div> | ||
|
|
||
| {/* Workflow List */} | ||
| <div className="space-y-4"> | ||
| <h2 className="text-xl font-semibold text-gray-700 dark:text-gray-100"> | ||
| Your Workflows | ||
| </h2> | ||
|
|
||
| {/* Sample Workflow Cards */} | ||
| {[ | ||
| { | ||
| name: "Data Processing Pipeline", | ||
| description: "Automated data extraction and processing from multiple sources", | ||
| status: "active", | ||
| lastRun: "2 hours ago", | ||
| }, | ||
| { | ||
| name: "Report Generation", | ||
| description: "Weekly analytics report generation and distribution", | ||
| status: "scheduled", | ||
| lastRun: "1 day ago", | ||
| }, | ||
| { | ||
| name: "Customer Onboarding", | ||
| description: "Automated workflow for new customer setup and welcome sequence", | ||
| status: "active", | ||
| lastRun: "30 minutes ago", | ||
| }, | ||
| ].map((workflow, index) => ( | ||
| <Card key={index} className="dark:bg-slate-800 hover:shadow-md transition-shadow cursor-pointer"> | ||
| <CardHeader className="pb-3"> | ||
| <div className="flex items-center justify-between"> | ||
| <CardTitle className="text-lg font-medium text-gray-900 dark:text-gray-100"> | ||
| {workflow.name} | ||
| </CardTitle> | ||
| <div className="flex items-center gap-2"> | ||
| <span className={`px-2 py-1 text-xs font-medium rounded-full ${ | ||
| workflow.status === 'active' | ||
| ? 'bg-green-100 text-green-800 dark:bg-green-900 dark:text-green-200' | ||
| : 'bg-orange-100 text-orange-800 dark:bg-orange-900 dark:text-orange-200' | ||
| }`}> | ||
| {workflow.status} | ||
| </span> | ||
| <Button variant="ghost" size="icon" className="h-8 w-8"> | ||
| <Settings size={16} /> | ||
| </Button> | ||
| </div> | ||
| </div> | ||
| </CardHeader> | ||
| <CardContent className="pt-0"> | ||
| <p className="text-gray-600 dark:text-gray-400 text-sm mb-2"> | ||
| {workflow.description} | ||
| </p> | ||
| <p className="text-xs text-gray-500 dark:text-gray-500"> | ||
| Last run: {workflow.lastRun} | ||
| </p> | ||
| </CardContent> | ||
| </Card> | ||
| ))} | ||
| </div> |
There was a problem hiding this comment.
This component displays hardcoded data for workflow statistics and the list of workflows. To make this page functional, this static data should be replaced with dynamic data fetched from the backend using the workflowsAPI handlers that have been implemented.
| <svg className="w-4 h-4 text-slate-500" viewBox="0 0 24 24" fill="none" stroke="currentColor" strokeWidth="2"> | ||
| <circle cx="11" cy="11" r="8"></circle> | ||
| <line x1="21" y1="21" x2="16.65" y2="16.65"></line> | ||
| </svg> |
There was a problem hiding this comment.
The SVG used for the zoom control appears to be a search icon, which could be confusing for users. For consistency and clarity, consider replacing the hardcoded SVGs in this component with icons from the lucide-react library, which is already a project dependency. You could use ZoomIn for this control, Play for the execute button, and ChevronDown for the select dropdown.
| if (response.error) { | ||
| console.error('Error fetching workflow templates:', response.error); | ||
| setTemplatesError(response.error); | ||
| } else if (response.data) { |
There was a problem hiding this comment.
The useEffect hook correctly catches errors from the API and updates the templatesError state. However, this state variable is not used to provide any feedback to the user. If fetching templates fails, the user will be unaware of the problem. Consider displaying this error in the UI, for example, using a toast notification or an error message component.
| const deleteSelectedNodes = useCallback(() => { | ||
| if (selectedNodes.length > 0) { | ||
| const nodeIdsToDelete = selectedNodes.map(node => node.id); | ||
| setNodes((nds) => nds.filter(node => !nodeIdsToDelete.includes(node.id))); | ||
| setEdges((eds) => eds.filter(edge => | ||
| !nodeIdsToDelete.includes(edge.source) && !nodeIdsToDelete.includes(edge.target) | ||
| )); | ||
| } | ||
| }, [selectedNodes, setNodes, setEdges]); |
There was a problem hiding this comment.
The deleteSelectedNodes function appears to be unused. Node deletion is already configured to be handled by React Flow via the deleteKeyCode prop. This function can be removed to simplify the component.
| import { Status, Step, Flow, StepGeneratorData, ComponentListData, SerialComponents } from './Types'; | ||
| import { defaultStep } from './Default'; | ||
|
|
||
| export function padWithZero(value: number): string { | ||
| if (value < 10) { | ||
| return "0" + value.toString() | ||
| } else { | ||
| return value.toString() | ||
| } | ||
| } | ||
|
|
||
| export function parseTime(time: string): [number, number, number] { | ||
| const regex = /^([0-9]+):([0-5][0-9]):([0-5][0-9])$/ | ||
| const result = regex.exec(time) | ||
|
|
||
| const getRegexRes = ( | ||
| regexResult: RegExpExecArray | null, | ||
| index: number, | ||
| ): number | undefined => { | ||
| if (!regexResult || !regexResult[index]) { | ||
| return undefined | ||
| } | ||
| const parsed = parseInt(regexResult[index], 10) | ||
| return isNaN(parsed) ? undefined : parsed | ||
| } | ||
|
|
||
| if (result) { | ||
| const hour = getRegexRes(result, 1) | ||
| const min = getRegexRes(result, 2) | ||
| const sec = getRegexRes(result, 3) | ||
|
|
||
| if (hour !== undefined && min !== undefined && sec !== undefined) { | ||
| return [hour, min, sec] | ||
| } | ||
| } | ||
|
|
||
| return [0, 0, 0] | ||
| } | ||
|
|
||
| export function isExecutable(status: Status): boolean { | ||
| return status === "PENDING" || status === "INCOMPLETE" || status === "OVERDUE" | ||
| } | ||
|
|
||
| export function customCompare(a: number | undefined, b: number | undefined): number { | ||
| if (a === undefined && b === undefined) return 0 | ||
| if (a === undefined) return 1 | ||
| if (b === undefined) return -1 | ||
| return a - b | ||
| } | ||
|
|
||
| export function getFirstActiveSubStepInfo( | ||
| stepPropsArray: StepGeneratorData[], | ||
| stepDict: Record<string, Step>, | ||
| ): Step | undefined { | ||
| const activeStep = stepPropsArray.find((stepProps) => | ||
| isExecutable(stepProps.step.status), | ||
| )?.step | ||
|
|
||
| if (!activeStep) return undefined | ||
|
|
||
| const childSteps = activeStep.child_step_ids | ||
| ?.map((id) => stepDict[id]) | ||
| .filter(Boolean) || [activeStep] | ||
|
|
||
| return childSteps | ||
| .sort((a, b) => customCompare(a.position, b.position)) | ||
| .find((step) => isExecutable(step.status)) | ||
| } | ||
|
|
||
| export function addTime(tB: string, tA: string): string { | ||
| const matchA = parseTime(tA) | ||
| const matchB = parseTime(tB) | ||
|
|
||
| const hrSum = matchA[0] + matchB[0] | ||
| const minSum = matchA[1] + matchB[1] | ||
| const secSum = matchA[2] + matchB[2] | ||
|
|
||
| let carry = 0 | ||
| const totalSec = secSum % 60 | ||
| carry = Math.floor(secSum / 60) | ||
|
|
||
| const totalMin = (minSum + carry) % 60 | ||
| carry = Math.floor((minSum + carry) / 60) | ||
|
|
||
| const totalHr = hrSum + carry | ||
|
|
||
| return ( | ||
| padWithZero(totalHr) + | ||
| ":" + | ||
| padWithZero(totalMin) + | ||
| ":" + | ||
| padWithZero(totalSec) | ||
| ) | ||
| } | ||
|
|
||
| export function fillConnectedChildSteps( | ||
| childStepIds: string[], | ||
| connectedStepList: string[], | ||
| stepDict: Record<string, Step>, | ||
| ): void { | ||
| connectedStepList.push(...childStepIds) | ||
|
|
||
| childStepIds.forEach((childStepId) => { | ||
| const childStep = stepDict[childStepId] | ||
| if (childStep?.child_step_ids) { | ||
| fillConnectedChildSteps( | ||
| childStep.child_step_ids, | ||
| connectedStepList, | ||
| stepDict, | ||
| ) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| export function flowBFS( | ||
| stepDict: Record<string, Step>, | ||
| flow: Flow, | ||
| ): [ComponentListData[], number, number, string] { | ||
| const rootStep = stepDict[flow.root_step_id] || defaultStep | ||
| const traversedArray = [rootStep.id] | ||
| const componentList: ComponentListData[] = [] | ||
| const connectedStepList = [rootStep.id] | ||
|
|
||
| let offSet = 0 | ||
| let stepNumber = 1 | ||
| let doneCount = rootStep.status === "DONE" ? 1 : 0 | ||
| let serialSteps: SerialComponents[] = [ | ||
| { | ||
| type: "Step", | ||
| data: { | ||
| step: rootStep, | ||
| stepNumber, | ||
| isRootStep: true, | ||
| isLastStep: false, | ||
| isConnectedStep: true, | ||
| }, | ||
| }, | ||
| ] | ||
| let etaSum = "00:00:00" | ||
| let queue = [rootStep] | ||
|
|
||
| while (queue.length > 0) { | ||
| const newBlockingStepsArray: Step[] = [] | ||
|
|
||
| queue.forEach((step) => { | ||
| if (step.time_needed) { | ||
| etaSum = addTime(etaSum, step.time_needed) | ||
| } | ||
| if (step.child_step_ids) { | ||
| fillConnectedChildSteps( | ||
| step.child_step_ids, | ||
| connectedStepList, | ||
| stepDict, | ||
| ) | ||
| step.child_step_ids.forEach((id) => { | ||
| const childStep = stepDict[id] | ||
| if (childStep?.time_needed) { | ||
| etaSum = addTime(etaSum, childStep.time_needed) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| const isParallelBlockingLevel = (step.blocking_step_ids || []).length > 1 | ||
|
|
||
| if (isParallelBlockingLevel) { | ||
| componentList.push({ | ||
| level: componentList.length, | ||
| marginLeft: offSet, | ||
| serialComponents: [...serialSteps], | ||
| className: "flex", | ||
| }) | ||
| offSet += Math.floor(serialSteps.length / 2 + 1) * 520 | ||
| serialSteps = [] | ||
| } | ||
|
|
||
| const blockingSteps = step.blocking_step_ids || [] | ||
| blockingSteps.forEach((blockingStepId) => { | ||
| const blockingStep = stepDict[blockingStepId] || defaultStep | ||
|
|
||
| if (!traversedArray.includes(blockingStepId)) { | ||
| stepNumber++ | ||
| traversedArray.push(blockingStepId) | ||
| doneCount += blockingStep.status === "DONE" ? 1 : 0 | ||
|
|
||
| serialSteps.push({ | ||
| type: "Step", | ||
| data: { | ||
| step: blockingStep, | ||
| stepNumber, | ||
| isRootStep: false, | ||
| isLastStep: blockingStepId === flow.last_step_id, | ||
| isConnectedStep: true, | ||
| }, | ||
| }) | ||
|
|
||
| connectedStepList.push(blockingStep.id) | ||
| newBlockingStepsArray.push(blockingStep) | ||
| } | ||
| }) | ||
|
|
||
| if (isParallelBlockingLevel) { | ||
| componentList.push({ | ||
| level: componentList.length, | ||
| marginLeft: offSet - 180, | ||
| serialComponents: [...serialSteps], | ||
| className: "flex flex-col justify-center", | ||
| }) | ||
| offSet += 520 - 180 | ||
| serialSteps = [] | ||
| return | ||
| } | ||
| }) | ||
|
|
||
| queue = newBlockingStepsArray | ||
| } | ||
|
|
||
| componentList.push({ | ||
| level: componentList.length, | ||
| marginLeft: offSet, | ||
| serialComponents: [...serialSteps], | ||
| className: "flex flex-row gap-[146px]", | ||
| }) | ||
|
|
||
| return [componentList, stepNumber, doneCount, etaSum] | ||
| } No newline at end of file |
There was a problem hiding this comment.
This file and its contents, such as flowBFS, do not seem to be imported or used by the new React Flow-based WorkflowBuilder. This suggests it might be legacy code from a previous implementation. If it's no longer needed, consider removing it to reduce code complexity and maintenance overhead.
| </div> | ||
|
|
||
| {/* Empty State (if no workflows) */} | ||
| <div className="text-center py-12 hidden"> |
There was a problem hiding this comment.
The empty state for the workflow list is always hidden because of the hidden CSS class. This class should be removed and the component should be rendered conditionally based on whether the user has any workflows.
| <div className="text-center py-12 hidden"> | |
| <div className="text-center py-12"> |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (17)
frontend/src/components/Sidebar.tsx (1)
174-193: Add accessibility affordances to the Workflow nav item.
Provide an explicit label and aria-current for active state.- <Link - to="/workflow" - className={cn( + <Link + to="/workflow" + aria-label="Workflow Builder" + aria-current={location.pathname.includes("/workflow") ? "page" : undefined} + className={cn( "flex w-8 h-8 items-center justify-center hover:bg-[#D8DFE680] dark:hover:bg-gray-700 rounded-md mt-[10px]", location.pathname.includes("/workflow") && "bg-[#D8DFE680] dark:bg-gray-700", )} >frontend/src/components/workflow/ActionBar.tsx (2)
24-31: Prevent accidental form submission; normalize onClick.
Set button type to "button" and invoke handler safely.- <button - onClick={onExecute} + <button + type="button" + onClick={() => onExecute?.()} className="px-4 py-2 bg-slate-800 hover:bg-slate-700 text-white border-none rounded-md cursor-pointer text-sm font-medium flex items-center gap-1.5 transition-all duration-200" >
38-48: Add an accessible label to the zoom selector.
Improves SR usability without changing visuals.- <select - className="border-none bg-transparent text-sm font-medium text-slate-700 cursor-pointer appearance-none pr-4 outline-none" + <select + aria-label="Zoom level" + className="border-none bg-transparent text-sm font-medium text-slate-700 cursor-pointer appearance-none pr-4 outline-none" value={zoomLevel} onChange={handleZoomChange} >frontend/src/components/workflow/Types.ts (2)
59-64: Type setFlow as React state updater for correctness.
Prevents stale updates and matches Context/State patterns.+import type { Dispatch, SetStateAction } from "react"; @@ export interface FlowContextProps { flow: Flow - setFlow: (flow: Flow) => void + setFlow: Dispatch<SetStateAction<Flow>> activeSubStep: Step | null setActiveSubStep: (step: Step | null) => void }Also applies to: 1-6
43-44: Prefer numeric duration over opaque string.
Using a number (e.g., minutes) simplifies math and validation.- time_needed?: string + // Duration in minutes (consider ISO-8601 or a branded type if API requires) + time_needed_minutes?: numberfrontend/src/routes/_authenticated/workflow.tsx (2)
38-44: Make builder/list state reflect in URL for deep-linkingSwitching modes only sets local state. Consider a query param or sub-route (
/workflow?mode=builderor/workflow/builder) for shareable links and refresh resilience.-<Button - onClick={() => setViewMode("builder")} +<Button + onClick={() => { + setViewMode("builder") + // router.navigate({ to: "/workflow", search: { mode: "builder" } }) + }}
123-153: Use stable keys for list items
indexas a key harms reconciliation when items reorder. Use a stable field likenameor anid.- ].map((workflow, index) => ( - <Card key={index} ... + ].map((workflow) => ( + <Card key={workflow.name} ...frontend/src/components/workflow/WorkflowProvider.tsx (2)
28-53: LocalStorage handling is fine; consider namespacing keys and reducing console noiseNamespace keys to avoid collisions and gate logs in production.
- const savedFlow = localStorage.getItem("flow_data") - const savedActiveSubStep = localStorage.getItem("active_sub_step") + const savedFlow = localStorage.getItem("xyne.workflow.flow") + const savedActiveSubStep = localStorage.getItem("xyne.workflow.active_sub_step") ... - localStorage.removeItem("flow_data") - localStorage.removeItem("active_sub_step") + localStorage.removeItem("xyne.workflow.flow") + localStorage.removeItem("xyne.workflow.active_sub_step")- console.log("FlowProvider: Setting flow to:", newFlow) + if (process.env.NODE_ENV !== "production") console.log("FlowProvider: set flow", newFlow) ... - localStorage.setItem("flow_data", JSON.stringify(newFlow)) + localStorage.setItem("xyne.workflow.flow", JSON.stringify(newFlow))
80-88: Memoize context value to avoid unnecessary re-renders downstreamThe provider value is recreated on every render.
- return ( - <FlowContext.Provider - value={{ - flow, - setFlow: setFlowWithLog, - activeSubStep, - setActiveSubStep: setActiveSubStepWithLog, - }} - > + const ctx = React.useMemo(() => ({ + flow, + setFlow: setFlowWithLog, + activeSubStep, + setActiveSubStep: setActiveSubStepWithLog, + }), [flow, activeSubStep]) + + return ( + <FlowContext.Provider value={ctx}> {children} </FlowContext.Provider> )frontend/src/components/workflow/WorkflowBuilder.tsx (2)
30-49: Type the node data explicitly for better safetyProvide a typed
NodePropsgeneric to avoid unsafeanyondata.-const StepNode: React.FC<NodeProps> = ({ data, isConnectable, selected }) => { - const { step, isActive, isCompleted } = data as { step: Step; isActive?: boolean; isCompleted?: boolean; }; +type StepNodeData = { step: Step; isActive?: boolean; isCompleted?: boolean } +const StepNode: React.FC<NodeProps<StepNodeData>> = ({ data, isConnectable, selected }) => { + const { step, isActive, isCompleted } = data
234-241: Use the provided onClose in TriggersSidebar (currently unused)Add a close affordance to honor the prop.
- <div className="px-6 pt-5 pb-4 border-b border-slate-200"> + <div className="px-6 pt-5 pb-4 border-b border-slate-200 flex items-center justify-between"> <div className="text-sm font-semibold text-gray-700 tracking-wider uppercase mb-1.5"> SELECT TRIGGERS </div> - <div className="text-sm text-slate-500 leading-5 font-normal"> + <div className="text-sm text-slate-500 leading-5 font-normal"> Trigger is an action that will initiate the workflow. </div> + <button aria-label="Close" onClick={onClose} className="ml-3 rounded-md p-1 hover:bg-slate-100"> + <svg width="16" height="16" viewBox="0 0 24 24" stroke="currentColor" fill="none"><line x1="18" y1="6" x2="6" y2="18"/><line x1="6" y1="6" x2="18" y2="18"/></svg> + </button> </div>frontend/src/components/workflow/api/ApiHandlers.ts (1)
46-64: APIs look good; consider surfacing ApiResponse type and adding abort/timeout laterUniform response shape is solid. Optionally export
ApiResponseand accept anAbortSignalinoptionsfor cancellation.Also applies to: 66-110
frontend/src/components/workflow/WorkflowUtils.ts (5)
131-136: Mark root’s last-step status correctly.
Initialize isLastStep using flow.last_step_id.- isLastStep: false, + isLastStep: rootStep.id === flow.last_step_id,
146-161: Clarify eta aggregation semantics; current logic risks under/over-counting.
You add time for each queued step and also for its immediate children, but not for deeper descendants unless they later appear in the queue. Define the contract (e.g., sum of all steps reachable from root via child edges, or only the BFS chain via blocking edges) and compute once over a de-duplicated set.Option (sum unique connected children): build a Set via fillConnectedChildSteps, then fold:
- if (step.child_step_ids) { - fillConnectedChildSteps( - step.child_step_ids, - connectedStepList, - stepDict, - ) - step.child_step_ids.forEach((id) => { - const childStep = stepDict[id] - if (childStep?.time_needed) { - etaSum = addTime(etaSum, childStep.time_needed) - } - }) - } + if (step.child_step_ids) { + fillConnectedChildSteps(step.child_step_ids, connectedStepList, stepDict) + }Then after BFS, fold once:
- return [componentList, stepNumber, doneCount, etaSum] + for (const id of Array.from(new Set(connectedStepList))) { + const s = stepDict[id]; if (s?.time_needed) etaSum = addTime(etaSum, s.time_needed) + } + return [componentList, stepNumber, doneCount, etaSum]If the intended contract is different, please adjust accordingly.
122-123: connectedStepList is write-only in this function.
It’s built but never read within flowBFS; either return it or remove to reduce overhead.Also applies to: 196-196
172-173: Extract magic numbers (520, 180) to named constants.
Improves readability and future tuning.+const LANE_WIDTH = 520 +const CENTER_SHIFT = 180 ... - offSet += Math.floor(serialSteps.length / 2 + 1) * 520 + offSet += Math.floor(serialSteps.length / 2 + 1) * LANE_WIDTH ... - offSet += 520 - 180 + offSet += LANE_WIDTH - CENTER_SHIFTAlso applies to: 208-208
115-121: Fail fast if root step is missing to avoid rendering placeholders.
Using defaultStep may mask upstream data issues; consider returning an empty layout with a clear signal.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
frontend/package.json(2 hunks)frontend/src/components/Sidebar.tsx(2 hunks)frontend/src/components/workflow/ActionBar.tsx(1 hunks)frontend/src/components/workflow/Default.ts(1 hunks)frontend/src/components/workflow/Types.ts(1 hunks)frontend/src/components/workflow/WorkflowBuilder.tsx(1 hunks)frontend/src/components/workflow/WorkflowProvider.tsx(1 hunks)frontend/src/components/workflow/WorkflowUtils.ts(1 hunks)frontend/src/components/workflow/api/ApiHandlers.ts(1 hunks)frontend/src/components/workflow/api/Endpoints.ts(1 hunks)frontend/src/routeTree.gen.ts(13 hunks)frontend/src/routes/_authenticated/workflow.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
frontend/src/components/workflow/Default.ts (1)
frontend/src/components/workflow/Types.ts (1)
Step(28-46)
frontend/src/components/workflow/WorkflowProvider.tsx (1)
frontend/src/components/workflow/Types.ts (3)
FlowContextProps(59-64)Flow(48-57)Step(28-46)
frontend/src/components/Sidebar.tsx (2)
frontend/src/lib/utils.ts (1)
cn(5-7)frontend/src/components/Tooltip.tsx (1)
Tip(3-28)
frontend/src/routes/_authenticated/workflow.tsx (1)
frontend/src/components/Sidebar.tsx (1)
Sidebar(38-344)
frontend/src/components/workflow/WorkflowBuilder.tsx (2)
frontend/src/components/workflow/Types.ts (4)
Step(28-46)Flow(48-57)UserDetail(1-6)WorkflowTemplate(107-115)frontend/src/components/workflow/api/ApiHandlers.ts (1)
workflowTemplatesAPI(47-64)
frontend/src/components/workflow/api/ApiHandlers.ts (2)
frontend/src/components/workflow/Types.ts (2)
WorkflowTemplate(107-115)Flow(48-57)frontend/src/components/workflow/api/Endpoints.ts (1)
API_ENDPOINTS(5-9)
frontend/src/components/workflow/WorkflowUtils.ts (2)
frontend/src/components/workflow/Types.ts (6)
Status(8-19)StepGeneratorData(73-79)Step(28-46)Flow(48-57)ComponentListData(83-88)SerialComponents(81-81)frontend/src/components/workflow/Default.ts (1)
defaultStep(3-9)
🪛 GitHub Actions: TypeScript Build Check
frontend/src/components/workflow/WorkflowBuilder.tsx
[error] 345-345: TS6133: 'workflowTemplates' is declared but its value is never read.
[error] 346-346: TS6133: 'isLoadingTemplates' is declared but its value is never read.
[error] 347-347: TS6133: 'templatesError' is declared but its value is never read.
[error] 592-592: TS6133: 'addNewNode' is declared but its value is never read.
[error] 618-618: TS6133: 'deleteSelectedNodes' is declared but its value is never read.
🔇 Additional comments (10)
frontend/src/components/Sidebar.tsx (1)
15-16: Icon import looks correct and consistent.
The lucide-react Workflow icon usage aligns with existing icon imports.frontend/package.json (1)
31-37: @xyflow/react v12.8.4 supports React 19
Peer deps arereact>=17andreact-dom>=17, and the dist-tags for React/ReactDOM are at19.1.1, so compatibility is confirmed.frontend/src/components/workflow/ActionBar.tsx (1)
1-58: Solid, minimal ActionBar API.
Props are clear and defaults sensible. Styling matches rest of app.frontend/src/components/workflow/Default.ts (1)
3-9: LGTM.
Valid Step shape with minimal sensible defaults.frontend/src/routes/_authenticated/workflow.tsx (1)
10-12: Alias mapping for/workflowalready configured
The generated route tree updates the_authenticated/workflowfile route toid: '/workflow'andpath: '/workflow', so the sidebar’s/workflowlink correctly maps to theWorkflowComponent. No changes needed.frontend/src/routeTree.gen.ts (4)
263-269: Types augmented for '/_authenticated/workflow': LGTM.
FileRoutesByPath and AuthenticatedRoute children interfaces/objects are updated coherently.Also applies to: 392-392, 415-415
449-449: Indices and unions extended for '/workflow' and '/_authenticated/workflow': LGTM.
Mappings across FileRoutesByFullPath/To/Id and FileRouteTypes look consistent.Also applies to: 475-475, 503-503, 532-532, 557-557, 583-583
18-18: Verified: workflow route export exists
The modulefrontend/src/routes/_authenticated/workflow.tsxexportsRouteas expected (line 10); wiring under_authenticatedis correct.
638-639: Route manifest entries verified – file exists at expected pathfrontend/src/components/workflow/WorkflowUtils.ts (1)
4-10: Time helpers are solid and defensive.
Regex guards mm/ss; hours unbounded; sensible [0,0,0] fallback. No issues.Also applies to: 12-38
| const data = await response.json(); | ||
|
|
||
| if (!response.ok) { | ||
| return { | ||
| error: data.message || `HTTP ${response.status}: ${response.statusText}`, | ||
| status: response.status, | ||
| }; | ||
| } | ||
|
|
||
| return { | ||
| data, | ||
| status: response.status, | ||
| }; | ||
| } catch (error) { | ||
| return { | ||
| error: error instanceof Error ? error.message : 'Network error', | ||
| status: 0, | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle empty/non-JSON responses to prevent crashes (DELETE/204, HTML errors, etc.)
Unconditional response.json() throws on 204 or non-JSON bodies. Parse defensively.
- const data = await response.json();
+ const text = await response.text();
+ let data: unknown = undefined;
+ try {
+ data = text ? JSON.parse(text) : undefined;
+ } catch {
+ // Non-JSON response; leave data undefined
+ }
if (!response.ok) {
return {
- error: data.message || `HTTP ${response.status}: ${response.statusText}`,
+ // @ts-expect-error safe optional chaining on unknown
+ error: (data as any)?.message || `HTTP ${response.status}: ${response.statusText}`,
status: response.status,
};
}
return {
- data,
+ data: data as T,
status: response.status,
};📝 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 data = await response.json(); | |
| if (!response.ok) { | |
| return { | |
| error: data.message || `HTTP ${response.status}: ${response.statusText}`, | |
| status: response.status, | |
| }; | |
| } | |
| return { | |
| data, | |
| status: response.status, | |
| }; | |
| } catch (error) { | |
| return { | |
| error: error instanceof Error ? error.message : 'Network error', | |
| status: 0, | |
| }; | |
| } | |
| } | |
| const text = await response.text(); | |
| let data: unknown = undefined; | |
| try { | |
| data = text ? JSON.parse(text) : undefined; | |
| } catch { | |
| // Non-JSON response; leave data undefined | |
| } | |
| if (!response.ok) { | |
| return { | |
| // @ts-expect-error safe optional chaining on unknown | |
| error: (data as any)?.message || `HTTP ${response.status}: ${response.statusText}`, | |
| status: response.status, | |
| }; | |
| } | |
| return { | |
| data: data as T, | |
| status: response.status, | |
| }; |
🤖 Prompt for AI Agents
In frontend/src/components/workflow/api/ApiHandlers.ts around lines 25 to 44,
the code calls response.json() unconditionally which will throw for 204/no-body
or non-JSON responses; update the handler to parse defensively by first checking
response.status (treat 204/205 as empty body) and/or the Content-Type header for
application/json, use response.text() as a fallback, and only call JSON.parse in
a try/catch when the body is non-empty and JSON-like; ensure returned object
uses an empty object/null for empty successful responses and returns the same
error shape for malformed/non-JSON error responses without letting parsing
exceptions bubble up.
| // Base API configuration | ||
| const BASE_URL = 'http://localhost:3009/api/v1'; | ||
|
|
There was a problem hiding this comment.
Replace hard-coded localhost with environment-based and relative-safe base URL
Hard-coding http://localhost:3009 will break non-local builds and is inconsistent with other modules using relative /api/v1. Use env + sane fallback and keep it relative to support reverse proxies.
Apply:
-// Base API configuration
-const BASE_URL = 'http://localhost:3009/api/v1';
+// Base API configuration
+const BASE_URL =
+ // Prefer explicit env
+ (import.meta as any)?.env?.VITE_API_BASE_URL ??
+ // Next.js (if applicable)
+ (process as any)?.env?.NEXT_PUBLIC_API_BASE_URL ??
+ // Fallback to relative path (works behind proxies)
+ '/api/v1';📝 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.
| // Base API configuration | |
| const BASE_URL = 'http://localhost:3009/api/v1'; | |
| // Base API configuration | |
| const BASE_URL = | |
| // Prefer explicit env (Vite) | |
| (import.meta as any)?.env?.VITE_API_BASE_URL ?? | |
| // Next.js (if applicable) | |
| (process as any)?.env?.NEXT_PUBLIC_API_BASE_URL ?? | |
| // Fallback to relative path (works behind proxies) | |
| '/api/v1'; |
🤖 Prompt for AI Agents
In frontend/src/components/workflow/api/Endpoints.ts around lines 1 to 3, the
BASE_URL is hard-coded to 'http://localhost:3009/api/v1'; replace this with an
environment-configurable value that falls back to a relative, reverse-proxy-safe
path. Read the base from an env var (e.g. process.env.REACT_APP_API_BASE or
import.meta.env.VITE_API_BASE depending on the build system) and if not set use
'/api/v1' as the default; ensure you join paths cleanly (no double slashes) and
keep the protocol/host out of defaults so non-local and proxied deployments work
correctly.
| // Base API configuration | ||
| const BASE_URL = 'http://localhost:3009/api/v1'; | ||
|
|
||
| // API endpoints | ||
| export const API_ENDPOINTS = { | ||
| WORKFLOW_TEMPLATES: `${BASE_URL}/workflow-templates`, | ||
| WORKFLOWS: `${BASE_URL}/workflows`, | ||
| WORKFLOW_BY_ID: (id: string) => `${BASE_URL}/workflows/${id}`, | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify consistency of API base usage across the app
Sidebar uses relative /api/v1/... while this module used absolute localhost. Confirm and normalize.
🏁 Script executed:
#!/bin/bash
# Find absolute API URLs and mixed base usages
rg -nP --type=ts --type=tsx -C2 '(https?://[^"'\''\s]+/api/v1|/api/v1/)' frontend | sed -E 's/^(.+)$/\1/g'Length of output: 136
🏁 Script executed:
#!/bin/bash
# Search for absolute API URLs in TS/TSX files
rg -n -g '*.ts' -g '*.tsx' -C2 'https?://[^"'\''\s]+/api/v1' frontend
# Search for relative API URLs in TS/TSX files
rg -n -g '*.ts' -g '*.tsx' -C2 '/api/v1' frontendLength of output: 11167
Normalize API base usage in Endpoints.ts
In frontend/src/components/workflow/api/Endpoints.ts, replace the hard-coded http://localhost:3009/api/v1 with the shared relative API base (e.g. import the /api/v1 prefix from src/api.ts or use authFetch) so it aligns with other modules.
🤖 Prompt for AI Agents
In frontend/src/components/workflow/api/Endpoints.ts around lines 1 to 9, the
file currently hard-codes BASE_URL to 'http://localhost:3009/api/v1'; replace
that with the shared relative API base used across the app by importing the base
(e.g. API_BASE or API_PREFIX) from src/api.ts (or use the shared authFetch
wrapper) and construct endpoints from that imported value so the module uses the
common /api/v1 prefix and environment-agnostic base; update the import statement
accordingly and remove the hard-coded constant.
| export const API_ENDPOINTS = { | ||
| WORKFLOW_TEMPLATES: `${BASE_URL}/workflow-templates`, | ||
| WORKFLOWS: `${BASE_URL}/workflows`, | ||
| WORKFLOW_BY_ID: (id: string) => `${BASE_URL}/workflows/${id}`, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Encode path params to avoid malformed URLs
Ensure id is URL-safe.
export const API_ENDPOINTS = {
WORKFLOW_TEMPLATES: `${BASE_URL}/workflow-templates`,
WORKFLOWS: `${BASE_URL}/workflows`,
- WORKFLOW_BY_ID: (id: string) => `${BASE_URL}/workflows/${id}`,
+ WORKFLOW_BY_ID: (id: string) => `${BASE_URL}/workflows/${encodeURIComponent(id)}`,
};📝 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.
| export const API_ENDPOINTS = { | |
| WORKFLOW_TEMPLATES: `${BASE_URL}/workflow-templates`, | |
| WORKFLOWS: `${BASE_URL}/workflows`, | |
| WORKFLOW_BY_ID: (id: string) => `${BASE_URL}/workflows/${id}`, | |
| }; | |
| export const API_ENDPOINTS = { | |
| WORKFLOW_TEMPLATES: `${BASE_URL}/workflow-templates`, | |
| WORKFLOWS: `${BASE_URL}/workflows`, | |
| WORKFLOW_BY_ID: (id: string) => `${BASE_URL}/workflows/${encodeURIComponent(id)}`, | |
| }; |
🤖 Prompt for AI Agents
In frontend/src/components/workflow/api/Endpoints.ts around lines 5 to 9, the
WORKFLOW_BY_ID path concatenates the id directly which can produce malformed
URLs for ids containing special characters; change the factory to URL-encode the
id (e.g. use encodeURIComponent on the id param), update the function signature
if needed to accept number|string and ensure callers pass the raw id (not
pre-encoded) so all generated endpoints are URL-safe.
| interface FlowContextProps { | ||
| flow: Flow | null | ||
| setFlow: (flow: Flow | null) => void | ||
| activeSubStep: Step | null | ||
| setActiveSubStep: (step: Step | null) => void | ||
| } |
There was a problem hiding this comment.
Type drift from Types.ts: FlowContextProps redefined and nullable here
Types.ts exports FlowContextProps with non-null flow, but this file defines a divergent, nullable variant. This will cause inconsistency across consumers.
Unify by deriving a local widened type from the canonical one:
-import { Flow, Step } from './Types';
+import { Flow, Step, type FlowContextProps as BaseFlowContextProps } from './Types';
-interface FlowContextProps {
- flow: Flow | null
- setFlow: (flow: Flow | null) => void
- activeSubStep: Step | null
- setActiveSubStep: (step: Step | null) => void
-}
+type FlowContextProps = Omit<BaseFlowContextProps, 'flow' | 'setFlow'> & {
+ flow: Flow | null
+ setFlow: (flow: Flow | null) => void
+}Follow up: either adjust Types.ts to reflect nullable flow, or keep it non-null app-wide and initialize flow with a valid default.
📝 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.
| interface FlowContextProps { | |
| flow: Flow | null | |
| setFlow: (flow: Flow | null) => void | |
| activeSubStep: Step | null | |
| setActiveSubStep: (step: Step | null) => void | |
| } | |
| import { Flow, Step, type FlowContextProps as BaseFlowContextProps } from './Types'; | |
| type FlowContextProps = Omit<BaseFlowContextProps, 'flow' | 'setFlow'> & { | |
| flow: Flow | null | |
| setFlow: (flow: Flow | null) => void | |
| } |
🤖 Prompt for AI Agents
In frontend/src/components/workflow/WorkflowProvider.tsx around lines 4 to 9,
the file redefines FlowContextProps with a nullable flow which diverges from the
canonical FlowContextProps in Types.ts (flow is non-null there); to fix, import
the canonical FlowContextProps from Types.ts and either use that type directly
or create a derived widened type (e.g., make flow optional/null via TypeScript
utility types) so the local declaration matches the project-wide contract;
alternatively, if the app design requires nullable flow, update Types.ts to
reflect the nullable type and update all consumers accordingly to keep types
consistent.
| export function getFirstActiveSubStepInfo( | ||
| stepPropsArray: StepGeneratorData[], | ||
| stepDict: Record<string, Step>, | ||
| ): Step | undefined { | ||
| const activeStep = stepPropsArray.find((stepProps) => | ||
| isExecutable(stepProps.step.status), | ||
| )?.step | ||
|
|
||
| if (!activeStep) return undefined | ||
|
|
||
| const childSteps = activeStep.child_step_ids | ||
| ?.map((id) => stepDict[id]) | ||
| .filter(Boolean) || [activeStep] | ||
|
|
||
| return childSteps | ||
| .sort((a, b) => customCompare(a.position, b.position)) | ||
| .find((step) => isExecutable(step.status)) | ||
| } |
There was a problem hiding this comment.
Fix fallback when child_step_ids exist but resolve to empty after mapping.
With the current || [activeStep], an empty array is truthy and you may return undefined even when activeStep is executable. Also tighten TS typing on the filter.
Apply:
- const childSteps = activeStep.child_step_ids
- ?.map((id) => stepDict[id])
- .filter(Boolean) || [activeStep]
-
- return childSteps
+ const childSteps = (activeStep.child_step_ids
+ ?.map((id) => stepDict[id])
+ .filter((s): s is Step => Boolean(s)) ?? [])
+
+ const stepsToSearch = childSteps.length > 0 ? childSteps : [activeStep]
+
+ return stepsToSearch
.sort((a, b) => customCompare(a.position, b.position))
.find((step) => isExecutable(step.status))🤖 Prompt for AI Agents
frontend/src/components/workflow/WorkflowUtils.ts around lines 51 to 68: the
current fallback uses "?.map(...).filter(Boolean) || [activeStep]" which fails
when child_step_ids exists but mapping yields an empty array (empty arrays are
truthy) and the filter lacks a type guard; change the logic to first map then
filter with a TypeScript type predicate (e.g., .filter((s): s is Step => s !=
null) or similar) into a const typed as Step[], then if that resulting array is
empty fallback to [activeStep]; ensure childSteps is typed as Step[] so sorting
and find remain correct.
| const matches = useRouterState({ select: (s) => s.matches }) | ||
| const { user } = matches[matches.length - 1].context | ||
| const [viewMode, setViewMode] = useState<"list" | "builder">("list") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid brittle destructuring of route context; guard or use useRouteContext
matches[matches.length - 1].context can crash if the route tree changes. Prefer useRouteContext (typed) or guard the access.
- const matches = useRouterState({ select: (s) => s.matches })
- const { user } = matches[matches.length - 1].context
+ const matches = useRouterState({ select: (s) => s.matches })
+ const last = matches[matches.length - 1]
+ const user = (last?.context as any)?.user ?? nullAlternatively (preferred, if parent route provides context):
// import { useRouteContext } from "@tanstack/react-router"
// const { user } = useRouteContext({ from: "/_authenticated" })🤖 Prompt for AI Agents
In frontend/src/routes/_authenticated/workflow.tsx around lines 15 to 18, the
code directly accesses matches[matches.length - 1].context which can throw if
the route tree changes; replace this brittle destructuring by using
useRouteContext<{ user: UserType }>({ from: "/_authenticated" }) to obtain a
typed user from the route context (or, if useRouteContext is not available here,
guard the access by checking matches?.length > 0 and matches[matches.length -
1]?.context before reading .user), then update the rest of the component to read
user from that safe/typed value.
| Editor | ||
| </button> | ||
| <button className="px-4 py-1.5 bg-transparent text-slate-500 text-sm font-medium border-none cursor-pointer flex items-center gap-1.5 h-8 min-w-[80px] justify-center"> | ||
| <svg className="w-3.5 h-3.5" viewBox="0 0 24 24" fill="none" stroke="currentColor" strokeWidth="2"> |
There was a problem hiding this comment.
move this svg to different file or use icon if possible
| }, []) | ||
|
|
||
| const setFlowWithLog = (newFlow: Flow | null) => { | ||
| console.log("FlowProvider: Setting flow to:", newFlow) |
| options?: RequestInit | ||
| ): Promise<ApiResponse<T>> { | ||
| try { | ||
| const response = await fetch(url, { |
There was a problem hiding this comment.
don't use fetch, if it is a internal server endpoint use hono api
There was a problem hiding this comment.
Using custom API Handler now with fetch as this Workflow FE can call other system apis too
| @@ -0,0 +1,110 @@ | |||
| import { WorkflowTemplate, Flow } from '../Types'; | |||
There was a problem hiding this comment.
use hono for apis, checkout api.ts for reference
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (6)
frontend/src/components/workflow/ActionBar.tsx (1)
27-31: Replace hardcoded SVGs with lucide-react icons for consistencyUse existing dependency icons and keep visuals consistent across the app. This also addresses the zoom control using a search glyph.
+import { Play, ZoomIn, ChevronDown } from 'lucide-react'; ... - <svg className="w-4 h-4" viewBox="0 0 24 24" fill="none" stroke="currentColor" strokeWidth="2"> - <polygon points="5 3 19 12 5 21 5 3"></polygon> - </svg> + <Play className="w-4 h-4" /> ... - <svg className="w-4 h-4 text-slate-500" viewBox="0 0 24 24" fill="none" stroke="currentColor" strokeWidth="2"> - <circle cx="11" cy="11" r="8"></circle> - <line x1="21" y1="21" x2="16.65" y2="16.65"></line> - </svg> + <ZoomIn className="w-4 h-4 text-slate-500" /> ... - <svg className="w-4 h-4 text-slate-500 ml-1" viewBox="0 0 24 24" fill="none" stroke="currentColor" strokeWidth="2"> - <polyline points="6 9 12 15 18 9"></polyline> - </svg> + <ChevronDown className="w-4 h-4 text-slate-500 ml-1" />Also applies to: 34-37, 49-51, 1-2
frontend/src/components/workflow/api/Endpoints.ts (1)
3-3: Do not hard-code ngrok URL; use env with reverse-proxy-safe fallbackThis will break non-local and expire; align with app-wide base handling.
-const WORKFLOW_SERVICE_URL = 'https://53b79c6d27eb.ngrok-free.app/v1'; +const WORKFLOW_SERVICE_URL = + // Vite + (import.meta as any)?.env?.VITE_WORKFLOW_BASE_URL ?? + // Next.js / other + (process as any)?.env?.NEXT_PUBLIC_WORKFLOW_BASE_URL ?? + // Proxy-safe default + '/api/v1';#!/bin/bash # Verify absolute API bases left in repo rg -nPI --type=ts --type=tsx 'https?://[^"'\''\s]+/v1|ngrok' frontendfrontend/src/components/workflow/Types.ts (1)
178-183: Allow nulls in FlowContextProps to match provider usageexport interface FlowContextProps { - flow: Flow | TemplateFlow | LegacyFlow - setFlow: (flow: Flow | TemplateFlow | LegacyFlow) => void + flow: Flow | TemplateFlow | LegacyFlow | null + setFlow: (flow: Flow | TemplateFlow | LegacyFlow | null) => void activeSubStep: Step | null setActiveSubStep: (step: Step | null) => void }frontend/src/components/workflow/api/ApiHandlers.ts (3)
19-27: Fix header merge bug and default credentials (cookies not sent, headers can be clobbered).Spreading
...optionsafter buildingheaderslets a caller-providedoptions.headersoverride and drop your defaults. Also, credentials aren’t sent, so cookie/session auth will fail. Mergeoptionsfirst, then compose headers, and set a sensible default forcredentials.- const response = await fetch(url, { - headers: { - 'Content-Type': 'application/json', - 'ngrok-skip-browser-warning': 'true', - 'Accept': 'application/json', - ...options?.headers, - }, - ...options, - }); + const response = await fetch(url, { + credentials: options?.credentials ?? 'include', + ...options, + headers: { + Accept: 'application/json', + 'Content-Type': 'application/json', + 'ngrok-skip-browser-warning': 'true', + ...(options?.headers ?? {}), + }, + });
29-41: Don’t call response.json() unconditionally — parse defensively (handles 204/non‑JSON).This will throw on 204/205, empty bodies, or HTML error pages. Parse via text with a guarded JSON parse.
- const data = await response.json(); + const contentType = response.headers.get('content-type') ?? ''; + const hasBody = response.status !== 204 && response.status !== 205; + let data: unknown = undefined; + if (hasBody) { + const text = await response.text(); + if (text) { + try { + data = contentType.includes('application/json') ? JSON.parse(text) : undefined; + } catch { + data = undefined; + } + } + } if (!response.ok) { return { - error: data.message || `HTTP ${response.status}: ${response.statusText}`, + // @ts-expect-error tolerant access to optional "message" + error: (data as any)?.message || `HTTP ${response.status}: ${response.statusText}`, status: response.status, }; } return { - data, + data: data as T, status: response.status, };
14-28: Align with project standard: use the shared Hono API client instead of raw fetch.Previous review notes say to “use hono for apis, checkout api.ts”. If that client exists, route these calls through it to avoid duplicate request logic and to inherit auth/interceptors.
Would you like me to generate a patch after confirming the client module path? Run:
#!/bin/bash # Discover existing API client(s) to integrate with fd -t f -a '(api|client).ts' frontend | sed 's|^|FILE: |' rg -nP -C2 'hono|Hono|new\s+Hono|export\s+const\s+api\b|create.*client' frontend
🧹 Nitpick comments (10)
frontend/src/components/workflow/api/Endpoints.ts (1)
1-13: DRY the base URL with ApiHandlers to avoid driftPrefer a single source of truth (import base from ApiHandlers or export it here).
frontend/src/components/workflow/WorkflowIcons.tsx (1)
10-15: Mark decorative SVGs as presentational and unify stroke caps/joinsImproves accessibility and visual consistency.
- <svg className={className} width={width} height={height} viewBox="0 0 24 24" fill="none" stroke="currentColor" strokeWidth="2"> + <svg className={className} width={width} height={height} viewBox="0 0 24 24" + fill="none" stroke="currentColor" strokeWidth="2" + aria-hidden="true" focusable="false" + strokeLinecap="round" strokeLinejoin="round">Apply similarly across icons or factor a BaseIcon wrapper to reduce duplication.
Also applies to: 17-22, 24-29
frontend/src/components/workflow/WorkflowBuilder.tsx (4)
456-472: Type Node instances with generics where createdKeep Node for strong typing.
- const newNode: Node = { + const newNode: Node<StepNodeData> = { ... - const templateNodes: Node[] = templateWorkflow.template_steps.map((templateStep, index) => { + const templateNodes: Node<StepNodeData>[] = templateWorkflow.template_steps.map((templateStep, index) => { ... - const newNode: Node = { + const newNode: Node<StepNodeData> = {Also applies to: 489-525, 558-582
401-406: Surface template fetch errors to the userShow a toast/banner instead of console-only logging.
I can wire a minimal toast using your existing notification system if you point me to it.
587-599: Remove unused deleteSelectedNodes (and related ‘void’ hack)Dead code increases maintenance; React Flow already handles Delete via prop.
- const deleteSelectedNodes = useCallback(() => { - if (selectedNodes.length > 0) { - const nodeIdsToDelete = selectedNodes.map(node => node.id); - setNodes((nds) => nds.filter(node => !nodeIdsToDelete.includes(node.id))); - setEdges((eds) => eds.filter(edge => - !nodeIdsToDelete.includes(edge.source) && !nodeIdsToDelete.includes(edge.target) - )); - } - }, [selectedNodes, setNodes, setEdges]); - - // Prevent unused variable warning - void deleteSelectedNodes; + // (Removed unused deleteSelectedNodes; React Flow deleteKeyCode handles deletion)
445-450: Stale length check on deleteUse remaining count rather than comparing previous length to deleted length.
- const onNodesDelete = useCallback<OnNodesDelete>((_deleted) => { - if (nodes.length === _deleted.length) { - setShowEmptyCanvas(true); - } - }, [nodes.length]); + const onNodesDelete = useCallback<OnNodesDelete>((_deleted) => { + setShowEmptyCanvas((prev) => (nodes.length - _deleted.length) <= 0 ? true : prev); + }, [nodes.length]);frontend/src/components/workflow/api/ApiHandlers.ts (4)
63-67: Tighten types: avoid any in request payload.Prefer a safer shape for metadata.
- async instantiate(id: string, options: { name: string; metadata?: any }): Promise<ApiResponse<{ workflowId: string; rootStepId: string }>> { + async instantiate( + id: string, + options: { name: string; metadata?: Record<string, unknown> } + ): Promise<ApiResponse<{ workflowId: string; rootStepId: string }>> {
84-88: Avoid any in responses for run/complete; use unknown (or concrete types if available).Keeps unsoundness from leaking across the codebase.
- async run(id: string): Promise<ApiResponse<any>> { - return apiRequest<any>(`${WORKFLOW_BASE_URL}/workflow/${id}/run`, { + async run(id: string): Promise<ApiResponse<unknown>> { + return apiRequest<unknown>(`${WORKFLOW_BASE_URL}/workflow/${encodeURIComponent(id)}/run`, { method: 'POST', }); },- async completeStep(stepId: string): Promise<ApiResponse<any>> { - return apiRequest<any>(`${WORKFLOW_BASE_URL}/workflow/step/${stepId}/complete`, { + async completeStep(stepId: string): Promise<ApiResponse<unknown>> { + return apiRequest<unknown>(`${WORKFLOW_BASE_URL}/workflow/step/${encodeURIComponent(stepId)}/complete`, { method: 'POST', }); },Also applies to: 93-97
14-28: Add a default request timeout.Prevent hung UIs on stalled connections.
- try { - const response = await fetch(url, { + try { + const timeoutSignal = + options?.signal ?? + // Use AbortSignal.timeout when available; otherwise no timeout. + // Adjust the 15000ms default as needed. + (typeof AbortSignal !== 'undefined' && 'timeout' in AbortSignal + ? (AbortSignal as any).timeout(15000) + : undefined); + const response = await fetch(url, { credentials: options?.credentials ?? 'include', - ...options, + ...options, + signal: timeoutSignal, headers: {
3-8: Consider enriching ApiResponse with ok flag and headers when needed.Small ergonomics win for callers doing branching and needing pagination/request IDs.
-interface ApiResponse<T> { +interface ApiResponse<T> { data?: T; error?: string; status: number; + ok?: boolean; + headers?: Headers; }And set on return:
- return { - data: data as T, - status: response.status, - }; + return { data: data as T, status: response.status, ok: true, headers: response.headers };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
frontend/src/components/workflow/ActionBar.tsx(1 hunks)frontend/src/components/workflow/Default.ts(1 hunks)frontend/src/components/workflow/Types.ts(1 hunks)frontend/src/components/workflow/WorkflowBuilder.tsx(1 hunks)frontend/src/components/workflow/WorkflowIcons.tsx(1 hunks)frontend/src/components/workflow/api/ApiHandlers.ts(1 hunks)frontend/src/components/workflow/api/Endpoints.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/workflow/Default.ts
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/src/components/workflow/WorkflowBuilder.tsx (2)
frontend/src/components/workflow/Types.ts (5)
Tool(56-61)Step(64-91)Flow(159-164)TemplateFlow(94-98)UserDetail(1-6)frontend/src/components/workflow/api/ApiHandlers.ts (2)
workflowTemplatesAPI(51-69)workflowsAPI(72-98)
frontend/src/components/workflow/api/ApiHandlers.ts (1)
frontend/src/components/workflow/Types.ts (2)
TemplateFlow(94-98)Flow(159-164)
🪛 GitHub Actions: TypeScript Build Check
frontend/src/components/workflow/WorkflowBuilder.tsx
[error] 632-632: bunx tsc -b: TS2339: Property 'name' does not exist on type '{}'.
[error] 633-633: bunx tsc -b: TS2339: Property 'name' does not exist on type '{}'.
| <select | ||
| className="border-none bg-transparent text-sm font-medium text-slate-700 cursor-pointer appearance-none pr-4 outline-none" | ||
| value={zoomLevel} | ||
| onChange={handleZoomChange} | ||
| > |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix select value type (string expected) and add an accessible label
Avoid React warnings and improve a11y by using a string value and aria-label.
- <select
- className="border-none bg-transparent text-sm font-medium text-slate-700 cursor-pointer appearance-none pr-4 outline-none"
- value={zoomLevel}
- onChange={handleZoomChange}
- >
+ <select
+ className="border-none bg-transparent text-sm font-medium text-slate-700 cursor-pointer appearance-none pr-4 outline-none"
+ aria-label="Zoom level"
+ value={String(zoomLevel)}
+ onChange={handleZoomChange}
+ >📝 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.
| <select | |
| className="border-none bg-transparent text-sm font-medium text-slate-700 cursor-pointer appearance-none pr-4 outline-none" | |
| value={zoomLevel} | |
| onChange={handleZoomChange} | |
| > | |
| <select | |
| className="border-none bg-transparent text-sm font-medium text-slate-700 cursor-pointer appearance-none pr-4 outline-none" | |
| aria-label="Zoom level" | |
| value={String(zoomLevel)} | |
| onChange={handleZoomChange} | |
| > |
🤖 Prompt for AI Agents
In frontend/src/components/workflow/ActionBar.tsx around lines 38 to 42, the
select element uses a non-string value and lacks an accessible label; change the
select's value to a string (e.g., value={String(zoomLevel)}) and add an
aria-label (e.g., aria-label="Zoom level"), and update handleZoomChange to parse
the incoming event value back to number (e.g., const v =
parseInt(e.target.value, 10)) before setting zoomLevel so React receives a
string for the select and state keeps the numeric zoom.
| } | ||
|
|
||
| // Base URL for workflow service | ||
| const WORKFLOW_BASE_URL = 'https://53b79c6d27eb.ngrok-free.app/v1'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Externalize the base URL and gate ngrok-only header.
Avoid hardcoding an ngrok URL in main; use env/config and only send the ngrok header in dev.
-const WORKFLOW_BASE_URL = 'https://53b79c6d27eb.ngrok-free.app/v1';
+// Prefer central config/endpoints or env (adjust to your setup)
+const WORKFLOW_BASE_URL =
+ (import.meta as any)?.env?.VITE_WORKFLOW_BASE_URL ??
+ (typeof process !== 'undefined' ? (process as any).env?.VITE_WORKFLOW_BASE_URL : undefined) ??
+ '/v1';And conditionally include the ngrok header:
- 'ngrok-skip-browser-warning': 'true',
+ ...(WORKFLOW_BASE_URL.includes('ngrok')
+ ? { 'ngrok-skip-browser-warning': 'true' }
+ : {}),Also applies to: 22-22
| return apiRequest<TemplateFlow>(`${WORKFLOW_BASE_URL}/workflow-template/${id}`); | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
URL-encode path params.
Guard against special characters in IDs.
- return apiRequest<TemplateFlow>(`${WORKFLOW_BASE_URL}/workflow-template/${id}`);
+ return apiRequest<TemplateFlow>(`${WORKFLOW_BASE_URL}/workflow-template/${encodeURIComponent(id)}`);- return apiRequest<{ workflowId: string; rootStepId: string }>(`${WORKFLOW_BASE_URL}/workflow-template/${id}/instantiate`, {
+ return apiRequest<{ workflowId: string; rootStepId: string }>(`${WORKFLOW_BASE_URL}/workflow-template/${encodeURIComponent(id)}/instantiate`, {
method: 'POST',
body: JSON.stringify(options),
});- return apiRequest<Flow>(`${WORKFLOW_BASE_URL}/workflow/${id}`);
+ return apiRequest<Flow>(`${WORKFLOW_BASE_URL}/workflow/${encodeURIComponent(id)}`);- return apiRequest<any>(`${WORKFLOW_BASE_URL}/workflow/${id}/run`, {
+ return apiRequest<any>(`${WORKFLOW_BASE_URL}/workflow/${encodeURIComponent(id)}/run`, {
method: 'POST',
});- return apiRequest<any>(`${WORKFLOW_BASE_URL}/workflow/step/${stepId}/complete`, {
+ return apiRequest<any>(`${WORKFLOW_BASE_URL}/workflow/step/${encodeURIComponent(stepId)}/complete`, {
method: 'POST',
});Also applies to: 64-67, 77-79, 85-87, 93-96
🤖 Prompt for AI Agents
In frontend/src/components/workflow/api/ApiHandlers.ts around lines 57-58 (and
also apply the same change at 64-67, 77-79, 85-87, 93-96), the path parameters
are interpolated directly into URLs and need to be URL-encoded to guard against
special characters; update each template literal to wrap IDs and other path
params with encodeURIComponent(...) before interpolation so all generated URLs
are safe and correct when IDs contain slashes, spaces, or reserved characters.
| WORKFLOW_TEMPLATE_BY_ID: (id: string) => `${WORKFLOW_SERVICE_URL}/workflow-template/${id}`, | ||
| WORKFLOW_TEMPLATE_INSTANTIATE: (id: string) => `${WORKFLOW_SERVICE_URL}/workflow-template/${id}/instantiate`, | ||
| WORKFLOWS: `${WORKFLOW_SERVICE_URL}/workflow`, | ||
| WORKFLOW_BY_ID: (id: string) => `${WORKFLOW_SERVICE_URL}/workflow/${id}`, | ||
| WORKFLOW_RUN: (id: string) => `${WORKFLOW_SERVICE_URL}/workflow/${id}/run`, | ||
| WORKFLOW_STEP_COMPLETE: (stepId: string) => `${WORKFLOW_SERVICE_URL}/workflow/step/${stepId}/complete`, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Encode path params to prevent malformed URLs
IDs may contain slashes or spaces; encode them.
- WORKFLOW_TEMPLATE_BY_ID: (id: string) => `${WORKFLOW_SERVICE_URL}/workflow-template/${id}`,
- WORKFLOW_TEMPLATE_INSTANTIATE: (id: string) => `${WORKFLOW_SERVICE_URL}/workflow-template/${id}/instantiate`,
+ WORKFLOW_TEMPLATE_BY_ID: (id: string) => `${WORKFLOW_SERVICE_URL}/workflow-template/${encodeURIComponent(id)}`,
+ WORKFLOW_TEMPLATE_INSTANTIATE: (id: string) => `${WORKFLOW_SERVICE_URL}/workflow-template/${encodeURIComponent(id)}/instantiate`,
...
- WORKFLOW_BY_ID: (id: string) => `${WORKFLOW_SERVICE_URL}/workflow/${id}`,
- WORKFLOW_RUN: (id: string) => `${WORKFLOW_SERVICE_URL}/workflow/${id}/run`,
- WORKFLOW_STEP_COMPLETE: (stepId: string) => `${WORKFLOW_SERVICE_URL}/workflow/step/${stepId}/complete`,
+ WORKFLOW_BY_ID: (id: string) => `${WORKFLOW_SERVICE_URL}/workflow/${encodeURIComponent(id)}`,
+ WORKFLOW_RUN: (id: string) => `${WORKFLOW_SERVICE_URL}/workflow/${encodeURIComponent(id)}/run`,
+ WORKFLOW_STEP_COMPLETE: (stepId: string) => `${WORKFLOW_SERVICE_URL}/workflow/step/${encodeURIComponent(stepId)}/complete`,📝 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.
| WORKFLOW_TEMPLATE_BY_ID: (id: string) => `${WORKFLOW_SERVICE_URL}/workflow-template/${id}`, | |
| WORKFLOW_TEMPLATE_INSTANTIATE: (id: string) => `${WORKFLOW_SERVICE_URL}/workflow-template/${id}/instantiate`, | |
| WORKFLOWS: `${WORKFLOW_SERVICE_URL}/workflow`, | |
| WORKFLOW_BY_ID: (id: string) => `${WORKFLOW_SERVICE_URL}/workflow/${id}`, | |
| WORKFLOW_RUN: (id: string) => `${WORKFLOW_SERVICE_URL}/workflow/${id}/run`, | |
| WORKFLOW_STEP_COMPLETE: (stepId: string) => `${WORKFLOW_SERVICE_URL}/workflow/step/${stepId}/complete`, | |
| WORKFLOW_TEMPLATE_BY_ID: (id: string) => | |
| `${WORKFLOW_SERVICE_URL}/workflow-template/${encodeURIComponent(id)}`, | |
| WORKFLOW_TEMPLATE_INSTANTIATE: (id: string) => | |
| `${WORKFLOW_SERVICE_URL}/workflow-template/${encodeURIComponent(id)}/instantiate`, | |
| WORKFLOWS: `${WORKFLOW_SERVICE_URL}/workflow`, | |
| WORKFLOW_BY_ID: (id: string) => | |
| `${WORKFLOW_SERVICE_URL}/workflow/${encodeURIComponent(id)}`, | |
| WORKFLOW_RUN: (id: string) => | |
| `${WORKFLOW_SERVICE_URL}/workflow/${encodeURIComponent(id)}/run`, | |
| WORKFLOW_STEP_COMPLETE: (stepId: string) => | |
| `${WORKFLOW_SERVICE_URL}/workflow/step/${encodeURIComponent(stepId)}/complete`, |
🤖 Prompt for AI Agents
In frontend/src/components/workflow/api/Endpoints.ts around lines 7 to 12, the
path-building functions currently interpolate raw IDs which can contain slashes
or spaces and produce malformed URLs; update the functions that accept IDs
(WORKFLOW_TEMPLATE_BY_ID, WORKFLOW_TEMPLATE_INSTANTIATE, WORKFLOW_BY_ID,
WORKFLOW_RUN, WORKFLOW_STEP_COMPLETE) to wrap the id/stepId parameters with
encodeURIComponent(...) when constructing the path (leave the WORKFLOWS constant
unchanged), so all path params are safely URL-encoded.
| // Legacy Step type for backward compatibility | ||
| export type Step = { | ||
| id: string | ||
| name?: string | ||
| description?: string | ||
| type?: string | ||
| status?: string | ||
| parentStepId?: string | null | ||
| nextStepIds?: string[] | null | ||
| toolIds?: string[] | null | ||
| timeEstimate?: number | ||
| metadata?: any | null | ||
| createdAt?: string | ||
| updatedAt?: string | ||
| // Legacy fields for backward compatibility | ||
| blocked_by_step_ids?: string[] | ||
| blocking_step_ids?: string[] | ||
| parent_step_id?: string | ||
| child_step_ids?: string[] | ||
| assignee_id?: string | ||
| template_step_id?: string | ||
| unblocked_at?: string | ||
| completed_by?: string | ||
| completed_at?: string | ||
| contents?: Content[] | ||
| fall_back_step_id?: string | ||
| time_needed?: string | ||
| position?: number | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add optional fields used by WorkflowBuilder to Step for compatibility
Builder writes prevStepIds/tool_id on step objects.
export type Step = {
id: string
name?: string
description?: string
type?: string
status?: string
parentStepId?: string | null
nextStepIds?: string[] | null
+ prevStepIds?: string[] | null
+ tool_id?: string | null
toolIds?: string[] | null
timeEstimate?: number
metadata?: any | null📝 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.
| // Legacy Step type for backward compatibility | |
| export type Step = { | |
| id: string | |
| name?: string | |
| description?: string | |
| type?: string | |
| status?: string | |
| parentStepId?: string | null | |
| nextStepIds?: string[] | null | |
| toolIds?: string[] | null | |
| timeEstimate?: number | |
| metadata?: any | null | |
| createdAt?: string | |
| updatedAt?: string | |
| // Legacy fields for backward compatibility | |
| blocked_by_step_ids?: string[] | |
| blocking_step_ids?: string[] | |
| parent_step_id?: string | |
| child_step_ids?: string[] | |
| assignee_id?: string | |
| template_step_id?: string | |
| unblocked_at?: string | |
| completed_by?: string | |
| completed_at?: string | |
| contents?: Content[] | |
| fall_back_step_id?: string | |
| time_needed?: string | |
| position?: number | |
| } | |
| // frontend/src/components/workflow/Types.ts | |
| // Legacy Step type for backward compatibility | |
| export type Step = { | |
| id: string | |
| name?: string | |
| description?: string | |
| type?: string | |
| status?: string | |
| parentStepId?: string | null | |
| nextStepIds?: string[] | null | |
| prevStepIds?: string[] | null | |
| tool_id?: string | null | |
| toolIds?: string[] | null | |
| timeEstimate?: number | |
| metadata?: any | null | |
| createdAt?: string | |
| updatedAt?: string | |
| // Legacy fields for backward compatibility | |
| blocked_by_step_ids?: string[] | |
| blocking_step_ids?: string[] | |
| parent_step_id?: string | |
| child_step_ids?: string[] | |
| assignee_id?: string | |
| template_step_id?: string | |
| unblocked_at?: string | |
| completed_by?: string | |
| completed_at?: string | |
| contents?: Content[] | |
| fall_back_step_id?: string | |
| time_needed?: string | |
| position?: number | |
| } |
🤖 Prompt for AI Agents
In frontend/src/components/workflow/Types.ts around lines 63 to 91, the Step
type is missing optional fields the WorkflowBuilder writes (prevStepIds and
tool_id); update the type to include prevStepIds?: string[] | null and tool_id?:
string | null so builder-produced step objects are compatible, keeping both
fields optional and nullable to match existing legacy field patterns.
| import React, { useCallback, useState, useEffect } from 'react'; | ||
| import { | ||
| ReactFlow, | ||
| ReactFlowProvider, | ||
| Node, | ||
| Edge, | ||
| addEdge, | ||
| ConnectionLineType, | ||
| useNodesState, | ||
| useEdgesState, | ||
| Background, | ||
| BackgroundVariant, | ||
| Handle, | ||
| Position, | ||
| NodeProps, | ||
| Connection, | ||
| useReactFlow, | ||
| Panel, | ||
| OnSelectionChangeParams, | ||
| OnNodesDelete, | ||
| OnEdgesDelete, | ||
| } from '@xyflow/react'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid edge ID collisions and use enum MarkerType
Multiple edges between the same nodes will collide; also prefer MarkerType constant.
import {
ReactFlow,
ReactFlowProvider,
Node,
Edge,
addEdge,
ConnectionLineType,
+ MarkerType,
...
(params: Connection) => {
const newEdge = {
...params,
- id: `${params.source}-${params.target}`,
+ id: params?.id ?? `${params.source}-${params.target}-${crypto?.randomUUID?.() ?? Date.now()}`,
type: 'smoothstep',
animated: true,
style: {
stroke: '#3B82F6',
strokeWidth: 2,
},
markerEnd: {
- type: 'arrowclosed',
+ type: MarkerType.ArrowClosed,
color: '#3B82F6',
},
};And when generating template edges:
- markerEnd: {
- type: 'arrowclosed',
- color: '#3B82F6',
- },
+ markerEnd: { type: MarkerType.ArrowClosed, color: '#3B82F6' },Also applies to: 413-432, 531-543
| const [pollingInterval, setPollingInterval] = useState<NodeJS.Timeout | null>(null); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use browser-safe timer type
ReturnType works in both browser and Node.
- const [pollingInterval, setPollingInterval] = useState<NodeJS.Timeout | null>(null);
+ const [pollingInterval, setPollingInterval] = useState<ReturnType<typeof setInterval> | null>(null);📝 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 [pollingInterval, setPollingInterval] = useState<NodeJS.Timeout | null>(null); | |
| const [pollingInterval, setPollingInterval] = useState<ReturnType<typeof setInterval> | null>(null); |
🤖 Prompt for AI Agents
In frontend/src/components/workflow/WorkflowBuilder.tsx around lines 374 to 375,
the state uses NodeJS.Timeout which ties the type to Node; change the type to
ReturnType<typeof setInterval> so it is compatible with both browser and Node
environments (update the useState generic to use ReturnType<typeof setInterval>
| null and adjust any related usage annotations accordingly).
| // Fetch specific workflow template by ID on component mount | ||
| useEffect(() => { | ||
| const fetchWorkflowTemplate = async () => { | ||
| const templateId = 'a50e8400-e29b-41d4-a716-446655440010'; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Hard-coded templateId will block real usage
Pass via props or route param; fallback from env.
Example:
- const templateId = 'a50e8400-e29b-41d4-a716-446655440010';
+ const templateId =
+ new URLSearchParams(window.location.search).get('templateId') ??
+ (import.meta as any)?.env?.VITE_DEFAULT_TEMPLATE_ID ??
+ '';
+ if (!templateId) return;🏁 Script executed:
#!/bin/bash
# Check if workflow route provides a template id param or selection
rg -nP --type=ts --type=tsx -C2 'useParams\(|templateId|_authenticated/workflow' frontend/srcLength of output: 123
🏁 Script executed:
#!/bin/bash
set -e
# Search for WorkflowBuilder usage
rg -n 'WorkflowBuilder' -C2 frontend/src || true
# Inspect props in WorkflowBuilder component
rg -n 'const WorkflowBuilder' -C2 frontend/src/components/workflow/WorkflowBuilder.tsx || true
# Search for useParams usage across the codebase
rg -n 'useParams' -C2 frontend/src || true
# Search for route definitions with workflow path
rg -n '/workflow' -C2 frontend/src || trueLength of output: 13727
Extract templateId into a prop or route param
Remove the hard-coded templateId at frontend/src/components/workflow/WorkflowBuilder.tsx:387 and accept it via props or via Route.useParams()/URLSearchParams, with an import.meta.env.VITE_DEFAULT_TEMPLATE_ID fallback.
🤖 Prompt for AI Agents
In frontend/src/components/workflow/WorkflowBuilder.tsx around line 387, replace
the hard-coded const templateId = 'a50e8400-e29b-41d4-a716-446655440010' by
reading templateId from component props or from the route/URL (Route.useParams()
or new URLSearchParams(location.search)) and fall back to
import.meta.env.VITE_DEFAULT_TEMPLATE_ID when no value is provided; update the
component's props type to include an optional templateId and use that first,
otherwise read the route param/query, otherwise use the env fallback, and ensure
callers/parent route pass the prop or route includes the param so the value is
no longer hard-coded.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
frontend/src/components/workflow/WorkflowUtils.ts (3)
1-226: Confirm whether this module is actually used; remove if dead code.Previous reviewers flagged it as possibly unused with the new React Flow builder. If unused, delete to reduce maintenance; if used, the essential fixes above apply.
#!/bin/bash # Find imports/usages of WorkflowUtils across the repo. rg -nP -C2 --type=ts --type=tsx '\bWorkflowUtils\b|from\s+["\'].*WorkflowUtils["\']'
51-68: Fix fallback when child_step_ids map to an empty array; add typed filter.Empty arrays are truthy; current code can skip activeStep even when executable. Also remove unnecessary casts by updating isExecutable.
- const activeStep = stepPropsArray.find((stepProps) => - stepProps.step.status && isExecutable(stepProps.step.status as Status), - )?.step + const activeStep = stepPropsArray.find((stepProps) => + isExecutable(stepProps.step.status), + )?.step @@ - const childSteps = activeStep.child_step_ids - ?.map((id) => stepDict[id]) - .filter(Boolean) || [activeStep] + const childSteps: Step[] = (activeStep.child_step_ids + ?.map((id) => stepDict[id]) + .filter((s): s is Step => Boolean(s)) ?? []) + const stepsToSearch = childSteps.length > 0 ? childSteps : [activeStep] @@ - return childSteps + return stepsToSearch .sort((a, b) => customCompare(a.position, b.position)) - .find((step) => step.status && isExecutable(step.status as Status)) + .find((step) => isExecutable(step.status))
96-113: Guard against cycles and duplicates in child traversal.Add a visited set to prevent infinite recursion and bloat.
-export function fillConnectedChildSteps( +export function fillConnectedChildSteps( childStepIds: string[], connectedStepList: string[], stepDict: Record<string, Step>, -): void { - connectedStepList.push(...childStepIds) - - childStepIds.forEach((childStepId) => { - const childStep = stepDict[childStepId] - if (childStep?.child_step_ids) { - fillConnectedChildSteps( - childStep.child_step_ids, - connectedStepList, - stepDict, - ) - } - }) -} + visited: Set<string> = new Set(), +): void { + for (const id of childStepIds) { + if (visited.has(id)) continue + visited.add(id) + if (!connectedStepList.includes(id)) connectedStepList.push(id) + } + for (const childStepId of childStepIds) { + const childStep = stepDict[childStepId] + if (childStep?.child_step_ids?.length) { + fillConnectedChildSteps(childStep.child_step_ids, connectedStepList, stepDict, visited) + } + } +}
🧹 Nitpick comments (5)
frontend/src/components/workflow/WorkflowUtils.ts (5)
141-151: Pass a shared visited set into fillConnectedChildSteps.Avoid re-traversal across iterations.
- let queue = [rootStep] + let queue = [rootStep] + const visitedChildIds = new Set<string>() @@ - if (step.child_step_ids) { - fillConnectedChildSteps( - step.child_step_ids, - connectedStepList, - stepDict, - ) + if (step.child_step_ids) { + fillConnectedChildSteps(step.child_step_ids, connectedStepList, stepDict, visitedChildIds)
125-125: Nit: rename offSet → offset.Minor readability improvement.
- let offSet = 0 + let offset = 0Also update its usages.
202-213: Prefer for..of over Array.forEach when using early continue/return semantics.The inner return only exits the callback, which is easy to misread.
218-223: Extract layout magic numbers into constants.520, 180, and 146px should be named for maintainability.
- className: "flex flex-row gap-[146px]", + className: "flex flex-row gap-[146px]", // TODO: replace 146 with a named constantIf desired, I can send a patch introducing LAYOUT_GAP, COLUMN_WIDTH, CENTER_OFFSET, etc.
4-10: Simplify zero-padding.export function padWithZero(value: number): string { - if (value < 10) { - return "0" + value.toString() - } else { - return value.toString() - } + return value.toString().padStart(2, "0") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
frontend/src/components/workflow/WorkflowBuilder.tsx(1 hunks)frontend/src/components/workflow/WorkflowUtils.ts(1 hunks)frontend/src/components/workflow/api/ApiHandlers.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/components/workflow/WorkflowBuilder.tsx
- frontend/src/components/workflow/api/ApiHandlers.ts
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/components/workflow/WorkflowUtils.ts (2)
frontend/src/components/workflow/Types.ts (7)
Status(8-21)StepGeneratorData(192-198)Step(64-91)Flow(159-164)LegacyFlow(167-176)ComponentListData(202-207)SerialComponents(200-200)frontend/src/components/workflow/Default.ts (1)
defaultStep(3-15)
| export function isExecutable(status: Status): boolean { | ||
| return status === "PENDING" || status === "INCOMPLETE" || status === "OVERDUE" | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make status checks case-insensitive and accept undefined.
Default step uses "pending" (lowercase); current check will miss it.
-export function isExecutable(status: Status): boolean {
- return status === "PENDING" || status === "INCOMPLETE" || status === "OVERDUE"
-}
+export function isExecutable(status: Status | string | undefined): boolean {
+ if (!status) return false
+ const s = status.toUpperCase()
+ return s === "PENDING" || s === "INCOMPLETE" || s === "OVERDUE"
+}📝 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.
| export function isExecutable(status: Status): boolean { | |
| return status === "PENDING" || status === "INCOMPLETE" || status === "OVERDUE" | |
| } | |
| export function isExecutable(status: Status | string | undefined): boolean { | |
| if (!status) return false | |
| const s = status.toUpperCase() | |
| return s === "PENDING" || s === "INCOMPLETE" || s === "OVERDUE" | |
| } |
🤖 Prompt for AI Agents
In frontend/src/components/workflow/WorkflowUtils.ts around lines 40-42, the
isExecutable check currently does a strict equality against uppercase strings
and will miss lowercase values and undefined; update the function to accept
undefined and perform a case-insensitive comparison by normalizing status (e.g.,
convert status to uppercase only if defined) then return true if the normalized
value is "PENDING", "INCOMPLETE" or "OVERDUE", and also treat undefined as
executable per the review (return true when status is undefined).
| export function flowBFS( | ||
| stepDict: Record<string, Step>, | ||
| flow: Flow | LegacyFlow, | ||
| ): [ComponentListData[], number, number, string] { | ||
| const legacyFlow = flow as LegacyFlow; | ||
| const rootStep = stepDict[legacyFlow.root_step_id] || defaultStep | ||
| const traversedArray = [rootStep.id] | ||
| const componentList: ComponentListData[] = [] | ||
| const connectedStepList = [rootStep.id] |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Flow union is not actually handled; only LegacyFlow fields are read.
If callers pass the new Flow, traversal collapses to defaultStep. Either narrow the type or add proper guards to support both.
Would you like a patch that:
- Adds a type guard (isLegacyFlow) and uses nextStepIds/workflow_info for the new Flow, or
- Changes the signature to LegacyFlow to reflect actual support?
🏁 Script executed:
#!/bin/bash
# Find LegacyFlow and Flow definitions to verify union handling
rg -nP "type\s+LegacyFlow" -C2 .
rg -nP "interface\s+LegacyFlow" -C2 .
rg -nP "type\s+Flow\b" -C2 .
rg -nP "interface\s+Flow\b" -C2 .Length of output: 857
🏁 Script executed:
#!/bin/bash
# Extract Flow and LegacyFlow type definitions for inspection
sed -n '150,240p' frontend/src/components/workflow/Types.tsLength of output: 1941
flowBFS must discriminate between LegacyFlow and Flow
flowBFS currently casts flow to LegacyFlow and reads only root_step_id, so passing the new Flow always falls back to defaultStep. Add a type guard (e.g. isLegacyFlow) to handle both variants—using step_exe/workflow_info for Flow—or narrow the signature to LegacyFlow only.
🤖 Prompt for AI Agents
In frontend/src/components/workflow/WorkflowUtils.ts around lines 115 to 123,
flowBFS currently force-casts flow to LegacyFlow and always reads root_step_id,
causing the new Flow variant to fall back to defaultStep; add a type guard (e.g.
isLegacyFlow(flow): flow is LegacyFlow) and use it to pick the correct root id:
if LegacyFlow use flow.root_step_id, otherwise extract the root from the Flow
shape (e.g. flow.workflow_info?.root_step_id or from step_exe metadata per the
Flow interface), then use that id to lookup stepDict (with same defaultStep
fallback) and proceed; update the function signature/types if you prefer to
restrict to LegacyFlow only.
| if (step.time_needed) { | ||
| etaSum = addTime(etaSum, step.time_needed) | ||
| } | ||
| if (step.child_step_ids) { | ||
| fillConnectedChildSteps( | ||
| step.child_step_ids, | ||
| connectedStepList, | ||
| stepDict, | ||
| ) | ||
| step.child_step_ids.forEach((id) => { | ||
| const childStep = stepDict[id] | ||
| if (childStep?.time_needed) { | ||
| etaSum = addTime(etaSum, childStep.time_needed) | ||
| } | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid ETA double-counting.
Add time when a step is first discovered; remove per-child additions here.
- if (step.time_needed) {
- etaSum = addTime(etaSum, step.time_needed)
- }
- if (step.child_step_ids) {
- fillConnectedChildSteps(step.child_step_ids, connectedStepList, stepDict, visitedChildIds)
- }
- const isParallelBlockingLevel = (step.blocking_step_ids || []).length > 1
+ if (step.child_step_ids) {
+ fillConnectedChildSteps(step.child_step_ids, connectedStepList, stepDict, visitedChildIds)
+ }
+ const neighbors = step.blocking_step_ids ?? step.nextStepIds ?? []
+ const isParallelBlockingLevel = neighbors.length > 1Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/src/components/workflow/WorkflowUtils.ts around lines 147–161, the
code is double-counting ETA by adding child step durations here after
fillConnectedChildSteps; remove the per-child additions in this block (delete
the step.child_step_ids.forEach loop that adds childStep.time_needed) and
instead ensure time is added exactly once when a step is first discovered/added
to connectedStepList (e.g., move the addTime call into the logic that adds a
step to connectedStepList or into fillConnectedChildSteps and guard with a
seen/visited check so durations are idempotently summed).
| const blockingSteps = step.blocking_step_ids || [] | ||
| blockingSteps.forEach((blockingStepId) => { | ||
| const blockingStep = stepDict[blockingStepId] || defaultStep | ||
|
|
||
| if (!traversedArray.includes(blockingStepId)) { | ||
| stepNumber++ | ||
| traversedArray.push(blockingStepId) | ||
| doneCount += blockingStep.status === "DONE" ? 1 : 0 | ||
|
|
||
| serialSteps.push({ | ||
| type: "Step", | ||
| data: { | ||
| step: blockingStep, | ||
| stepNumber, | ||
| isRootStep: false, | ||
| isLastStep: blockingStepId === legacyFlow.last_step_id, | ||
| isConnectedStep: true, | ||
| }, | ||
| }) | ||
|
|
||
| connectedStepList.push(blockingStep.id) | ||
| newBlockingStepsArray.push(blockingStep) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use both legacy and new edge fields; add ETA only once when enqueuing.
Currently only blocking_step_ids are traversed and ETA for children is added twice.
- const blockingSteps = step.blocking_step_ids || []
- blockingSteps.forEach((blockingStepId) => {
+ const blockingSteps = step.blocking_step_ids ?? step.nextStepIds ?? []
+ blockingSteps.forEach((blockingStepId) => {
const blockingStep = stepDict[blockingStepId] || defaultStep
-
- if (!traversedArray.includes(blockingStepId)) {
+ if (!traversedArray.includes(blockingStepId)) {
stepNumber++
traversedArray.push(blockingStepId)
doneCount += blockingStep.status === "DONE" ? 1 : 0
+ if (blockingStep.time_needed) {
+ etaSum = addTime(etaSum, blockingStep.time_needed)
+ }📝 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 blockingSteps = step.blocking_step_ids || [] | |
| blockingSteps.forEach((blockingStepId) => { | |
| const blockingStep = stepDict[blockingStepId] || defaultStep | |
| if (!traversedArray.includes(blockingStepId)) { | |
| stepNumber++ | |
| traversedArray.push(blockingStepId) | |
| doneCount += blockingStep.status === "DONE" ? 1 : 0 | |
| serialSteps.push({ | |
| type: "Step", | |
| data: { | |
| step: blockingStep, | |
| stepNumber, | |
| isRootStep: false, | |
| isLastStep: blockingStepId === legacyFlow.last_step_id, | |
| isConnectedStep: true, | |
| }, | |
| }) | |
| connectedStepList.push(blockingStep.id) | |
| newBlockingStepsArray.push(blockingStep) | |
| } | |
| // Use legacy blocking edges first, then fall back to new nextStepIds | |
| const blockingSteps = step.blocking_step_ids ?? step.nextStepIds ?? [] | |
| blockingSteps.forEach((blockingStepId) => { | |
| const blockingStep = stepDict[blockingStepId] || defaultStep | |
| if (!traversedArray.includes(blockingStepId)) { | |
| stepNumber++ | |
| traversedArray.push(blockingStepId) | |
| doneCount += blockingStep.status === "DONE" ? 1 : 0 | |
| // Add ETA only once when enqueuing this step | |
| if (blockingStep.time_needed) { | |
| etaSum = addTime(etaSum, blockingStep.time_needed) | |
| } | |
| serialSteps.push({ | |
| type: "Step", | |
| data: { | |
| step: blockingStep, | |
| stepNumber, | |
| isRootStep: false, | |
| isLastStep: blockingStepId === legacyFlow.last_step_id, | |
| isConnectedStep: true, | |
| }, | |
| }) | |
| connectedStepList.push(blockingStep.id) | |
| newBlockingStepsArray.push(blockingStep) | |
| } |
🤖 Prompt for AI Agents
In frontend/src/components/workflow/WorkflowUtils.ts around lines 177-199,
update the traversal to use both the legacy edge field and the newer edge field
(e.g., keep checking blocking_step_ids and also the newer blocking_steps/ref
field) by merging them into a single deduplicated list before iterating; when
enqueuing child steps ensure ETA is attached only once by adding ETA at the
enqueue point (when pushing into newBlockingStepsArray or connected queue) and
remove any second ETA assignment when building serialSteps/serialSteps entries
so each child receives ETA a single time.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
frontend/src/components/workflow/api/ApiHandlers.ts (5)
13-26: Prefer Hono client for internal APIs (if that’s the project standard)If the repo standardizes on Hono (see prior review), refactor this wrapper to use the shared Hono client in
api.tsfor consistency, interceptors, and auth.Would you like me to draft the Hono-based equivalent of
apiRequest?#!/bin/bash # Verify presence of centralized API client/Hono usage fd -a -t f -E node_modules 'api.ts' rg -nP -C2 "(from\\s+['\"][^'\"]*hono[^'\"]*['\"])|(hono/client)" -g '!**/node_modules/**'
19-26: Fix header merge bug, add credentials, and gate ngrok headerSpreading
...optionsafterheaderscan overwrite your merged headers. Also, default to sending cookies for auth-protected APIs and conditionally add the ngrok header.- const response = await fetch(url, { - headers: { - 'Content-Type': 'application/json', - 'Accept': 'application/json', - ...options?.headers, - }, - ...options, - }); + const response = await fetch(url, { + ...options, + credentials: options?.credentials ?? 'include', + headers: { + Accept: 'application/json', + 'Content-Type': 'application/json', + ...(WORKFLOW_BASE_URL.includes('ngrok') + ? { 'ngrok-skip-browser-warning': 'true' } + : {}), + ...(options?.headers ?? {}), + }, + });
28-40: Parse responses defensively (204/no-body, HTML, non-JSON) to avoid crashes
response.json()throws on 204 and non-JSON. Safely parse and keep uniform error shape.- const data = await response.json(); + const text = await response.text(); + let data: unknown = undefined; + try { + data = text ? JSON.parse(text) : undefined; + } catch { + // Non-JSON response; leave data undefined + } if (!response.ok) { return { - error: data.message || `HTTP ${response.status}: ${response.statusText}`, + // @ts-expect-error best-effort message extraction + error: (data as any)?.message || `HTTP ${response.status}: ${response.statusText}`, status: response.status, }; } return { - data, + data: data as T, status: response.status, };
10-12: Externalize the base URL; avoid hardcoding localhostUse env/config with sensible fallback to relative path.
-// Base URL for workflow service -const WORKFLOW_BASE_URL = 'http://localhost:3000/v1'; +// Base URL for workflow service +const WORKFLOW_BASE_URL = + (import.meta as any)?.env?.VITE_WORKFLOW_BASE_URL ?? + (typeof process !== 'undefined' ? (process as any).env?.VITE_WORKFLOW_BASE_URL : undefined) ?? + '/v1';
56-57: URL-encode path params to handle special characters safelyWrap IDs with
encodeURIComponent(...).- return apiRequest<TemplateFlow>(`${WORKFLOW_BASE_URL}/workflow-template/${id}`); + return apiRequest<TemplateFlow>(`${WORKFLOW_BASE_URL}/workflow-template/${encodeURIComponent(id)}`);- return apiRequest<{ workflowId: string; rootStepId: string }>(`${WORKFLOW_BASE_URL}/workflow-template/${id}/instantiate`, { + return apiRequest<{ workflowId: string; rootStepId: string }>(`${WORKFLOW_BASE_URL}/workflow-template/${encodeURIComponent(id)}/instantiate`, { method: 'POST', body: JSON.stringify(options), });- return apiRequest<Flow>(`${WORKFLOW_BASE_URL}/workflow/${id}`); + return apiRequest<Flow>(`${WORKFLOW_BASE_URL}/workflow/${encodeURIComponent(id)}`);- return apiRequest<any>(`${WORKFLOW_BASE_URL}/workflow/${id}/run`, { + return apiRequest<any>(`${WORKFLOW_BASE_URL}/workflow/${encodeURIComponent(id)}/run`, { method: 'POST', });- return apiRequest<any>(`${WORKFLOW_BASE_URL}/workflow/step/${stepId}/complete`, { + return apiRequest<any>(`${WORKFLOW_BASE_URL}/workflow/step/${encodeURIComponent(stepId)}/complete`, { method: 'POST', });Also applies to: 63-67, 77-78, 84-87, 93-96
🧹 Nitpick comments (2)
frontend/src/components/workflow/api/ApiHandlers.ts (2)
83-87: Avoidanyin API results; use explicit types orunknownReplace
anywith specific response types when known; otherwise preferunknownto force safe narrowing at call sites.- async run(id: string): Promise<ApiResponse<any>> { - return apiRequest<any>(`${WORKFLOW_BASE_URL}/workflow/${encodeURIComponent(id)}/run`, { + async run(id: string): Promise<ApiResponse<unknown>> { + return apiRequest<unknown>(`${WORKFLOW_BASE_URL}/workflow/${encodeURIComponent(id)}/run`, { method: 'POST', }); },- async completeStep(stepId: string): Promise<ApiResponse<any>> { - return apiRequest<any>(`${WORKFLOW_BASE_URL}/workflow/step/${encodeURIComponent(stepId)}/complete`, { + async completeStep(stepId: string): Promise<ApiResponse<unknown>> { + return apiRequest<unknown>(`${WORKFLOW_BASE_URL}/workflow/step/${encodeURIComponent(stepId)}/complete`, { method: 'POST', }); },Also applies to: 92-96
14-17: Optional: add abort/timeout supportConsider
AbortControllerwith a reasonable default (e.g., 20s) and allow callers to pass asignalto override.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
frontend/src/components/workflow/api/ApiHandlers.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/components/workflow/api/ApiHandlers.ts (1)
frontend/src/components/workflow/Types.ts (2)
TemplateFlow(94-98)Flow(159-164)
avirupsinha12
force-pushed
the
Workflow-Builder-UI-Integration
branch
from
6acab84 to
9ab9426
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
frontend/src/components/workflow/WorkflowBuilder.tsx (6)
374-375: Use browser-safe timer typeReturnType works in both browser and Node.
- const [pollingInterval, setPollingInterval] = useState<NodeJS.Timeout | null>(null); + const [pollingInterval, setPollingInterval] = useState<ReturnType<typeof setInterval> | null>(null);
368-370: Surface template loading errors to the userYou set error state but never display it; add a toast or inline banner.
Example inline banner near the header or top-right Panel: show templatesError when non-null and provide a retry button calling fetchWorkflowTemplate().
Also applies to: 401-406
413-432: Make edge IDs unique and use MarkerType to avoid collisionsMultiple edges between the same nodes will overwrite each other; also prefer enum MarkerType over string.
import { ReactFlow, ReactFlowProvider, Node, Edge, addEdge, ConnectionLineType, + MarkerType, useNodesState,const onConnect = useCallback( (params: Connection) => { const newEdge = { ...params, - id: `${params.source}-${params.target}`, + id: params?.id ?? `${params.source}-${params.target}-${crypto?.randomUUID?.() ?? Date.now()}`, type: 'smoothstep', animated: true, style: { stroke: '#3B82F6', strokeWidth: 2, }, - markerEnd: { - type: 'arrowclosed', - color: '#3B82F6', - }, + markerEnd: { type: MarkerType.ArrowClosed, color: '#3B82F6' }, }; setEdges((eds) => addEdge(newEdge, eds)); }, [setEdges] );
526-546: Template edges: same unique IDs + MarkerTypePrevent collisions in prebuilt edges and use enum.
templateWorkflow.template_steps.forEach(templateStep => { templateStep.nextStepIds.forEach(nextStepId => { templateEdges.push({ - id: `${templateStep.id}-${nextStepId}`, + id: `${templateStep.id}-${nextStepId}-${crypto?.randomUUID?.() ?? Date.now()}`, source: templateStep.id, target: nextStepId, type: 'smoothstep', animated: false, style: { stroke: '#3B82F6', strokeWidth: 2, }, - markerEnd: { - type: 'arrowclosed', - color: '#3B82F6', - }, + markerEnd: { type: MarkerType.ArrowClosed, color: '#3B82F6' }, }); }); });
349-354: Remove hard-coded templateId; read from props, URL, or envHard-coding blocks real usage and testing.
interface WorkflowBuilderProps { flow?: Flow | TemplateFlow; activeStepId?: string; onStepClick?: (step: Step) => void; user?: UserDetail; + templateId?: string; }-const WorkflowBuilderInternal: React.FC<WorkflowBuilderProps> = ({ - onStepClick, -}) => { +const WorkflowBuilderInternal: React.FC<WorkflowBuilderProps> = ({ + onStepClick, + templateId, +}) => {useEffect(() => { const fetchWorkflowTemplate = async () => { - const templateId = 'a50e8400-e29b-41d4-a716-446655440010'; + const effectiveTemplateId = + templateId ?? + new URLSearchParams(window.location.search).get('templateId') ?? + (import.meta as any)?.env?.VITE_DEFAULT_TEMPLATE_ID ?? + ''; + if (!effectiveTemplateId) return; setIsLoadingTemplate(true); setTemplateError(null); try { - const response = await workflowTemplatesAPI.fetchById(templateId); + const response = await workflowTemplatesAPI.fetchById(effectiveTemplateId);Also applies to: 356-360, 385-410, 392-392
558-586: Remove dead code: addNewNode/deleteSelectedNodes and nodeCounter usagesBoth helpers are unused (React Flow already handles Delete key). Also drop nodeCounter and its writes that become no-ops.
- const [nodeCounter, setNodeCounter] = useState(1);- const addNewNode = useCallback(() => { - const newNode: Node = { - id: nodeCounter.toString(), - type: 'stepNode', - position: { - x: Math.random() * 400 + 200, - y: Math.random() * 300 + 150 - }, - data: { - step: { - id: nodeCounter.toString(), - name: `New Step ${nodeCounter}`, - status: 'PENDING', - contents: [] - }, - isActive: false, - isCompleted: false - }, - draggable: true, - }; - - setNodes((nds) => [...nds, newNode]); - setNodeCounter(prev => prev + 1); - setShowEmptyCanvas(false); - }, [nodeCounter, setNodes, setShowEmptyCanvas]); - - // Prevent unused variable warning - void addNewNode; + // (removed unused addNewNode)- const deleteSelectedNodes = useCallback(() => { - if (selectedNodes.length > 0) { - const nodeIdsToDelete = selectedNodes.map(node => node.id); - setNodes((nds) => nds.filter(node => !nodeIdsToDelete.includes(node.id))); - setEdges((eds) => eds.filter(edge => - !nodeIdsToDelete.includes(edge.source) && !nodeIdsToDelete.includes(edge.target) - )); - } - }, [selectedNodes, setNodes, setEdges]); - - // Prevent unused variable warning - void deleteSelectedNodes; + // (removed unused deleteSelectedNodes)- setNodeCounter(2);- setNodeCounter(templateWorkflow.template_steps.length + 1);Also applies to: 584-586, 587-599, 597-599, 362-362, 475-475, 550-550
🧹 Nitpick comments (3)
frontend/src/components/workflow/WorkflowBuilder.tsx (3)
45-46: Type Node data (StepNodeData) and use generics to drop castsEliminates repeated type assertions and TS2339 risks.
+type StepNodeData = { + step?: Step; + isActive?: boolean; + isCompleted?: boolean; + tools?: Tool[]; +};-const StepNode: React.FC<NodeProps> = ({ data, isConnectable, selected }) => { - const { step, isActive, isCompleted, tools } = data as { - step: Step; - isActive?: boolean; - isCompleted?: boolean; - tools?: Tool[]; - }; +const StepNode: React.FC<NodeProps<StepNodeData>> = ({ data, isConnectable, selected }) => { + const { step, isActive, isCompleted, tools } = data ?? {};- const [selectedNodes, setSelectedNodes] = useState<Node[]>([]); + const [selectedNodes, setSelectedNodes] = useState<Node<StepNodeData>[]>([]);- const initialNodes: Node[] = []; + const initialNodes: Node<StepNodeData>[] = []; const initialEdges: Edge[] = []; - const [nodes, setNodes, onNodesChange] = useNodesState(initialNodes); + const [nodes, setNodes, onNodesChange] = useNodesState<StepNodeData>(initialNodes);- const onNodeClick = useCallback((_: React.MouseEvent, node: Node) => { + const onNodeClick = useCallback((_: React.MouseEvent, node: Node<StepNodeData>) => { if (onStepClick && node.data?.step) { - onStepClick(node.data.step as Step); + onStepClick(node.data.step); } }, [onStepClick]);- <strong>Step:</strong> {(selectedNodes[0].data.step as Step).name || 'Unnamed'} + <strong>Step:</strong> {selectedNodes[0].data.step?.name || 'Unnamed'}Also applies to: 81-89, 360-362, 377-383, 439-444, 820-823
445-449: Safer empty-canvas check after deletionsAccount for multi-select deletions without relying on exact length equality.
- const onNodesDelete = useCallback<OnNodesDelete>((_deleted) => { - if (nodes.length === _deleted.length) { - setShowEmptyCanvas(true); - } - }, [nodes.length]); + const onNodesDelete = useCallback<OnNodesDelete>((_deleted) => { + const remaining = nodes.length - _deleted.length; + if (remaining <= 0) setShowEmptyCanvas(true); + }, [nodes.length]);
201-201: TriggersSidebar: remove unused onClose prop or implement itSignature exposes onClose but it isn’t used; simplify now or add a close control.
-const TriggersSidebar = ({ isVisible }: { isVisible: boolean; onClose?: () => void }) => { +const TriggersSidebar = ({ isVisible }: { isVisible: boolean }) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
frontend/package.json(2 hunks)frontend/src/components/Sidebar.tsx(2 hunks)frontend/src/components/workflow/ActionBar.tsx(1 hunks)frontend/src/components/workflow/Default.ts(1 hunks)frontend/src/components/workflow/Types.ts(1 hunks)frontend/src/components/workflow/WorkflowBuilder.tsx(1 hunks)frontend/src/components/workflow/WorkflowIcons.tsx(1 hunks)frontend/src/components/workflow/WorkflowProvider.tsx(1 hunks)frontend/src/components/workflow/WorkflowUtils.ts(1 hunks)frontend/src/components/workflow/api/ApiHandlers.ts(1 hunks)frontend/src/routeTree.gen.ts(13 hunks)frontend/src/routes/_authenticated/workflow.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- frontend/src/components/workflow/Default.ts
- frontend/src/components/workflow/WorkflowUtils.ts
- frontend/src/components/workflow/ActionBar.tsx
- frontend/src/components/workflow/WorkflowProvider.tsx
- frontend/src/components/workflow/api/ApiHandlers.ts
- frontend/src/components/workflow/WorkflowIcons.tsx
- frontend/package.json
- frontend/src/components/Sidebar.tsx
- frontend/src/routes/_authenticated/workflow.tsx
- frontend/src/routeTree.gen.ts
- frontend/src/components/workflow/Types.ts
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/components/workflow/WorkflowBuilder.tsx (2)
frontend/src/components/workflow/Types.ts (6)
Tool(56-61)Step(64-91)Flow(159-164)TemplateFlow(94-98)UserDetail(1-6)StepExecution(101-126)frontend/src/components/workflow/api/ApiHandlers.ts (2)
workflowTemplatesAPI(49-67)workflowsAPI(70-96)
| const addFirstStep = useCallback(() => { | ||
| setShowTriggersSidebar(true); | ||
| const newNode: Node = { | ||
| id: '1', | ||
| type: 'stepNode', | ||
| position: { x: 400, y: 200 }, | ||
| data: { | ||
| step: { | ||
| id: '1', | ||
| name: 'Select trigger from the sidebar', | ||
| status: 'PENDING', | ||
| contents: [] | ||
| }, | ||
| isActive: false, | ||
| isCompleted: false | ||
| }, | ||
| draggable: true, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Normalize status casing to match UI logic
The UI maps 'pending' (lowercase). New nodes use 'PENDING', so badges won’t render as intended.
data: {
step: {
id: '1',
name: 'Select trigger from the sidebar',
- status: 'PENDING',
+ status: 'pending',
contents: []
}, step: {
id: nodeCounter.toString(),
name: `New Step ${nodeCounter}`,
- status: 'PENDING',
+ status: 'pending',
contents: []
}, Also applies to: 568-571
🤖 Prompt for AI Agents
In frontend/src/components/workflow/WorkflowBuilder.tsx around lines 455-471
(and also update the similar block at 568-571), the newly created step nodes use
status: 'PENDING' which doesn't match the UI logic that expects lowercase
'pending'; change the status value to 'pending' in both places so the UI badge
rendering and status comparisons work correctly.
2 participants
Description
Workflow Builder UI Integration in Xyne Dashboard
Additional Notes
Summary by CodeRabbit
New Features
UI
Chores