Files extract2 #1075
Files extract2 #1075
Conversation
YashJuspay626
requested review from
devesh-juspay,
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. WalkthroughThis pull request introduces comprehensive script execution capabilities, review and trigger workflow steps, and new frontend components for code editing. It includes infrastructure updates (Dockerfile multi-stage optimization, environment configuration), backend APIs for script language enumeration and workflow step operations, and frontend components for code editing, script configuration, and review/trigger workflows. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ScriptSidebar as ScriptSidebar Component
participant CodeEditor as CodeEditor
participant API
participant Backend
User->>ScriptSidebar: Open ScriptSidebar
activate ScriptSidebar
ScriptSidebar->>API: Fetch available script languages
API-->>ScriptSidebar: Return language list
User->>ScriptSidebar: Select language
ScriptSidebar->>ScriptSidebar: Save previous language code/config<br/>Load selected language data
User->>ScriptSidebar: Click Edit Code
ScriptSidebar->>CodeEditor: Open modal (isOpen=true)
activate CodeEditor
User->>CodeEditor: Edit script
User->>CodeEditor: Ctrl/Cmd+S or Save button
CodeEditor->>ScriptSidebar: onSave(value)
ScriptSidebar->>ScriptSidebar: Update currentValue
deactivate CodeEditor
User->>ScriptSidebar: Validate config & Save
ScriptSidebar->>ScriptSidebar: Validate JSON config
alt Create New Script
ScriptSidebar->>API: createWorkflowStep(language, code)
API-->>Backend: Store step
else Update Existing Script
ScriptSidebar->>API: updateTool(toolId, code, config)
API-->>Backend: Update tool
end
API-->>ScriptSidebar: Success
ScriptSidebar->>ScriptSidebar: onCodeSaved(language, code)
deactivate ScriptSidebar
sequenceDiagram
participant User
participant Builder as WorkflowBuilder
participant ReviewUI as ReviewExecutionUI
participant API
participant Backend
User->>Builder: Execute workflow
Builder->>Backend: Trigger execution
Backend-->>Builder: Execution started
loop Polling
Builder->>API: GetWorkflowExecutionStatus
API-->>Builder: currentStep, requiresUserInput
alt Step is Review Tool
Builder->>ReviewUI: Render review UI
ReviewUI->>ReviewUI: Display content (files, previous results)
User->>ReviewUI: Approve/Reject
ReviewUI->>API: ReviewWorkflowStepApi (approved/rejected)
API->>Backend: Update review decision
Backend->>Backend: Activate next step based on decision
API-->>ReviewUI: Success
ReviewUI->>Builder: onReviewSubmitted()
Builder->>Builder: Resume polling
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
Summary of ChangesHello @YashJuspay626, 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 significant new features and improvements across the application, primarily focusing on enhancing workflow automation capabilities and developer experience. Key additions include a robust API Key management system with fine-grained control, and powerful new workflow steps for script execution, manual review, and manual triggers. The deployment infrastructure has been updated to support these new features, and the frontend UI has received several quality-of-life improvements, including a new code editor and better chat scrolling. Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
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 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a substantial set of new features and deployment enhancements. Key additions include a script execution engine supporting Python, JavaScript, and R, complete with sandboxing via bubblewrap. It also adds database backup and restore scripts, and a comprehensive API key management system with configurable scopes. The deployment process is significantly refactored, with improvements to Docker configurations, Prometheus monitoring setup, and permissions handling. On the frontend, new components have been added for a code editor, workflow steps (review, trigger, script), and API key management. While these are powerful additions, there are several critical security concerns in the current implementation, such as hardcoded credentials in the database scripts and the use of a privileged Docker container, which must be addressed.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 28
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
frontend/src/components/workflow/api/ApiHandlers.ts (2)
464-468: Remove or properly implement linkSteps method.This method always throws an error, making it effectively dead code. If the API endpoint doesn't exist:
- Remove the method entirely and update any calling code, or
- Implement it using
updateStepas the comment suggests, or- If keeping for interface compatibility, use TypeScript to deprecate it properly
If the method should be removed, apply this diff:
- /** - * Link a step to another step (add to nextStepIds) - */ - async linkSteps(_sourceStepId: string, _targetStepId: string): Promise<any> { - // Note: This specific endpoint doesn't exist in current routes - // You may need to use updateStep to modify nextStepIds instead - throw new Error("linkSteps endpoint not available in current API. Use updateStep to modify nextStepIds.") - },Alternatively, implement it using
updateStep:/** * Link a step to another step (add to nextStepIds) */ async linkSteps(sourceStepId: string, targetStepId: string): Promise<any> { - // Note: This specific endpoint doesn't exist in current routes - // You may need to use updateStep to modify nextStepIds instead - throw new Error("linkSteps endpoint not available in current API. Use updateStep to modify nextStepIds.") + // Fetch current step to get existing nextStepIds + const sourceStep = await this.fetchStepById(sourceStepId) + const currentNextSteps = sourceStep.nextStepIds || [] + + // Add target step if not already present + if (!currentNextSteps.includes(targetStepId)) { + return this.updateStep(sourceStepId, { + nextStepIds: [...currentNextSteps, targetStepId] + }) + } + + return sourceStep },Note: The implementation alternative assumes a
fetchStepByIdmethod exists or needs to be added.
231-235: templatesAPI.fetchAll should return WorkflowTemplateSimplified[] instead of ApiTemplate[]
Update the method signature and itsextractResponseDatageneric toWorkflowTemplateSimplified[]to match the endpoint’s actual response.
File: frontend/src/components/workflow/api/ApiHandlers.ts:231server/api/workflow.ts (1)
2763-2872: Tool ID mapping has fragile fallback logic.The tool mapping logic uses multiple strategies including content-based matching (comparing JSON-stringified tool values). This is fragile and could match the wrong tool if multiple tools have similar or empty values.
The fallback logic at lines 2864-2871 compares stringified tool values:
const matchingTool = createdTools.find(t => t.type === tool.type && JSON.stringify(t.value) === JSON.stringify(tool.value || {}) )Issues:
- JSON.stringify order isn't guaranteed, so identical objects might not match
- Empty objects
{}would match any tool with empty value- Large objects might have minor differences that prevent matching
Consider:
- Requiring frontend tool IDs for all tools in the workflow builder
- Using a more robust matching strategy (e.g., hash of sorted keys/values)
- Logging warnings when fallback matching is used
- Adding validation to ensure each step's tools are correctly mapped
Add this validation after mapping:
if (stepToolIds.length !== correspondingNode.data.tools.length) { Logger.warn(`Tool mapping mismatch for step ${step.name}: expected ${correspondingNode.data.tools.length}, got ${stepToolIds.length}`); }frontend/src/components/workflow/WhatHappensNextUI.tsx (1)
136-141: Inconsistent action handling.The onClick handler at line 136 checks for specific action IDs (
ai_agent,review,trigger) but the newly addedrun_scriptaction (line 76) is not included in this check. This means clicking "Run Script/Code" will do nothing.Add
run_scriptto the action handler:onClick={() => { - if (action.id === "ai_agent" || action.id === "email" || action.id === "review" || action.id === "trigger") { + if (["ai_agent", "email", "review", "trigger", "run_script"].includes(action.id)) { // For AI Agent and Email, trigger custom event to open respective ConfigUI onSelectAction(action.id) onClose() // Close WhatHappensNextUI } }}frontend/src/components/workflow/WorkflowBuilder.tsx (2)
36-138: Unify WorkflowTemplate type and fix field typo (toolExecIds).This file redefines WorkflowTemplate, diverging from Types.ts and routes/_authenticated/workflow.tsx. Also, stepExecutions element declares toolExecId (singular) but code uses step.toolExecIds (plural). This causes type drift and runtime confusion.
- Replace the inline interface with an extension of the shared type from Types.ts.
- Correct toolExecIds spelling.
Apply:
-// Import WorkflowTemplate type -interface WorkflowTemplate { +// Use shared WorkflowTemplate, extend for execution views +import type { WorkflowTemplate as BaseWorkflowTemplate } from "./Types" +type WorkflowTemplate = BaseWorkflowTemplate & { ... - stepExecutions?: Array<{ + stepExecutions?: Array<{ id: string workflowExecutionId: string workflowStepTemplateId: string name: string type: string status: string parentStepId: string | null prevStepIds: string[] nextStepIds: string[] - toolExecId: string[] + toolExecIds: string[] timeEstimate: number metadata: any completedBy: string | null createdAt: string updatedAt: string completedAt: string | null }> toolExecutions?: Array<{ id: string workflowToolId: string workflowExecutionId: string status: string result: any startedAt: string | null completedAt: string | null createdAt: string updatedAt: string }> -} +}Also update the import list accordingly (remove duplicated lucide import spacing if needed). Based on relevant code in Types.ts and routes/_authenticated/workflow.tsx (Based on learnings)
Also applies to: 119-119
2603-2624: Honor source/target handles on manual connections.Hardcoding sourceHandle: "bottom" breaks labeled connections from review handles ("approved"/"rejected") and any custom handles.
- sourceHandle: "bottom", - targetHandle: "top", + sourceHandle: params.sourceHandle ?? "bottom", + targetHandle: params.targetHandle ?? "top",Also applies to: 2617-2619
♻️ Duplicate comments (4)
Dockerfile (1)
93-93: Security risk: SUID bit on bwrap acknowledged.This comment acknowledges the existing review feedback about setting the SUID bit on
bwrap. As noted in the previous review, this configuration should be carefully managed to ensure it doesn't introduce privilege escalation vulnerabilities.Ensure that:
- All uses of
bwrapin the application properly configure sandboxing and drop privileges- The necessity of SUID is documented
- Regular security audits are performed on bwrap usage patterns
deployment/portable/docker-compose.app-build.yml (1)
26-26: Dropprivileged: true(unacceptable security risk).Granting full host privileges to the
appcontainer guts isolation and violates least-privilege. Bubblewrap or other tooling should be enabled by adding only the specific capabilities you need (e.g., viacap_add) instead of flipping the privileged bit. Please revert this and scope the capability set explicitly.backup-database.sh (1)
18-21: Externalize database credentials instead of hardcoding.Embedding the DB name/user/password directly in the script leaks credentials and prevents secure rotation. Please source these from environment variables with sane defaults instead.
-DB_NAME="xyne" -DB_USER="xyne" -DB_PASSWORD="xyne" -DB_HOST="localhost" -DB_PORT="5432" +DB_NAME="${DB_NAME:-xyne}" +DB_USER="${DB_USER:-xyne}" +DB_PASSWORD="${DB_PASSWORD:?Set DB_PASSWORD}" +DB_HOST="${DB_HOST:-localhost}" +DB_PORT="${DB_PORT:-5432}"restore-database.sh (1)
18-21: Remove hardcoded DB credentials.These values must come from environment/config to avoid leaking secrets and to support rotation. Please switch to parameterizing them just like the backup script should.
-DB_NAME="xyne" -DB_USER="xyne" -DB_PASSWORD="xyne" -DB_HOST="localhost" -DB_PORT="5432" +DB_NAME="${DB_NAME:-xyne}" +DB_USER="${DB_USER:-xyne}" +DB_PASSWORD="${DB_PASSWORD:?Set DB_PASSWORD}" +DB_HOST="${DB_HOST:-localhost}" +DB_PORT="${DB_PORT:-5432}"
🧹 Nitpick comments (19)
frontend/src/components/workflow/api/ApiHandlers.ts (3)
52-79: Consider consolidating template type definitions.The codebase has both
ApiTemplateandWorkflowTemplateSimplifiedinterfaces with overlapping fields but different structures. If these represent the same domain entity, consider unifying them to reduce maintenance burden and type confusion.
262-278: Consider using extractResponseData for consistency.The
fetchAllmethod uses custom response handling instead of theextractResponseDatahelper used by other methods. While the comment explains why (needing complete response structure), this inconsistency reduces code reusability.Consider refactoring
extractResponseDatato optionally preserve wrapper properties when needed, or create a dedicated helper likeextractResponseDataWithMeta.
314-342: Simplify FormData construction logic.The current implementation has redundant checks for whether
document_filewas added to FormData. The logic iterates through form entries to check for existence, which is inefficient.Apply this diff to simplify the logic:
const formData = new FormData() + let hasAddedFile = false // Add required fields matching the curl command formData.append("name", executionData.name) formData.append("description", executionData.description) // Add additional form data fields (excluding name and description to avoid duplicates) Object.entries(executionData.formData).forEach(([key, value]) => { if (key !== "name" && key !== "description") { if (value instanceof File) { formData.append("document_file", value) + hasAddedFile = true } else { formData.append(key, String(value)) } } }) - // Additional fallback: If no file has been added yet, check if we need to add the main file as document_file - let hasDocumentFile = false - for (const [key] of formData.entries()) { - if (key === "document_file") { - hasDocumentFile = true - break - } - } - - if (!hasDocumentFile && executionData.file) { + if (!hasAddedFile && executionData.file) { formData.append("document_file", executionData.file) }server/.env.default (1)
29-33: Consider removing unnecessary quotes from database configuration values.The database configuration values are wrapped in quotes, which is typically unnecessary for
.envfiles and can sometimes cause issues with certain parsers.Apply this diff to remove the quotes:
-DB_NAME="xyne" -DB_USER="xyne" -DB_PASSWORD="xyne" -DB_HOST="localhost" -DB_PORT="5432" +DB_NAME=xyne +DB_USER=xyne +DB_PASSWORD=xyne +DB_HOST=localhost +DB_PORT=5432start.sh (1)
11-14: LGTM with optional enhancement.Adding version checks for script execution languages is good practice for debugging and early detection of missing dependencies.
Consider adding labels to make the output clearer:
# Verify script execution languages are available -python3 --version -node --version -Rscript --version +echo "Python version: $(python3 --version 2>&1)" +echo "Node version: $(node --version 2>&1)" +echo "R version: $(Rscript --version 2>&1)"server/api/workflow.ts (3)
220-227: Remove or make debug logging conditional.This debug logging should be removed before merging to production, or made conditional on a debug flag. Production logs will be cluttered with detailed toolIds information for every workflow template fetch.
Consider removing these lines:
- // Debug logging to check toolIds - Logger.info(`GetWorkflowTemplateApi: Found ${steps.length} steps for template ${templateId}`) - steps.forEach((step, index) => { - Logger.info(`Step ${index + 1} (${step.name}): toolIds = ${JSON.stringify(step.toolIds)}`) - }) - const toolIds = steps.flatMap((s) => s.toolIds || []) - Logger.info(`GetWorkflowTemplateApi: Flattened toolIds = ${JSON.stringify(toolIds)}`)
1988-2033: Document return behavior and consider consistent error handling.The function now returns
nullfor various error conditions (property not found, no step data, invalid path). This is more consistent than returning error messages, but the mix of null returns and early string returns for invalid paths (line 1992) is inconsistent.Consider making all error cases return null and log errors for debugging:
): string | null => { try { if (!contentPath.startsWith("input.")) { - return `Invalid path: ${contentPath}. Only paths starting with 'input.' are supported.` + Logger.warn(`Invalid path: ${contentPath}. Only paths starting with 'input.' are supported.`) + return null }Also consider adding JSDoc to document the return behavior:
/** * Extracts content from previous step results using a simplified input path. * @param previousStepResults - Results from previous workflow steps * @param contentPath - Path in format "input.property.subproperty" * @returns Extracted content as string, or null if not found */
2186-2204: Consider simplifying email content extraction with a fallback array.The cascading if-checks for email content extraction work but could be more maintainable as an array of paths to try.
Consider refactoring to:
// Try extraction paths in priority order const contentPaths = contentPath ? [contentPath, "input.aiOutput", "input.output.body", "input.output"] : ["input.aiOutput", "input.output.body", "input.output"]; for (const path of contentPaths) { const extracted = extractContentFromPath(previousStepResults, path); if (extracted) { emailBody = extracted; break; } } if (!emailBody) { emailBody = "No content available from previous step"; }This makes the fallback order explicit and easier to modify.
frontend/src/components/workflow/PlusButton.tsx (1)
41-48: Consider using React patterns instead of custom events.The component dispatches a window-level custom event
openWhatHappensNext, which bypasses React's component communication patterns. While this works, it has drawbacks:
- Makes component dependencies implicit (harder to trace data flow)
- No TypeScript type safety for the event payload
- Requires manual cleanup to prevent memory leaks
- Testing requires mocking window.dispatchEvent
Consider using a callback prop instead:
interface PlusButtonProps { hasNext: boolean | undefined nodeId: string isConnectable?: boolean showHandles?: boolean onAddNext?: (nodeId: string) => void // Add this prop } export function PlusButton({ hasNext, nodeId, isConnectable, showHandles = false, onAddNext }: PlusButtonProps) { // ... onClick={(e) => { e.stopPropagation() e.preventDefault() onAddNext?.(nodeId) // Call the prop directly }}Then the parent component (WorkflowBuilder) can pass the handler directly, making the data flow explicit and type-safe.
frontend/src/components/workflow/executedWorkflowRenderer.tsx (2)
2592-2616: Direct DOM manipulation is fragile.Using
document.querySelector(".react-flow__viewport")and MutationObserver to track zoom changes is brittle and can break if ReactFlow's internal DOM structure changes. ReactFlow provides hooks for this purpose.Replace with ReactFlow's built-in
onMovecallback:- useEffect(() => { - const handleViewportChange = () => { - const viewport = getViewport() - const newZoomLevel = Math.round(viewport.zoom * 100) - setZoomLevel(newZoomLevel) - } - - const reactFlowWrapper = document.querySelector(".react-flow__viewport") - if (reactFlowWrapper) { - const observer = new MutationObserver(handleViewportChange) - observer.observe(reactFlowWrapper, { - attributes: true, - attributeFilter: ["style"], - }) - - const handleWheel = (e: Event) => { - const wheelEvent = e as WheelEvent - if (wheelEvent.ctrlKey || wheelEvent.metaKey) { - setTimeout(handleViewportChange, 10) - } - } - - reactFlowWrapper.addEventListener("wheel", handleWheel, { passive: true }) - - return () => { - observer.disconnect() - reactFlowWrapper.removeEventListener("wheel", handleWheel) - } - } - }, [getViewport])Add to ReactFlow component props:
<ReactFlow nodes={nodes} edges={edges} onNodesChange={onNodesChange} onEdgesChange={onEdgesChange} onConnect={onConnect} onNodeClick={onNodeClick} onNodesDelete={onNodesDelete} onEdgesDelete={onEdgesDelete} + onMove={() => { + const viewport = getViewport() + setZoomLevel(Math.round(viewport.zoom * 100)) + }} nodeTypes={nodeTypes}
2074-2419: Excessive console logging in production.Multiple console.log statements throughout the component will impact performance and clutter production console. These should be behind a development flag or removed.
Wrap debug logs with environment checks:
- console.log("📝 selectedTemplate changed, updating workflowData:", { - selectedTemplate - }) + if (process.env.NODE_ENV === 'development') { + console.log("📝 selectedTemplate changed, updating workflowData:", { + selectedTemplate + }) + }Or create a debug utility:
const debug = process.env.NODE_ENV === 'development' ? console.log.bind(console) : () => {} // Then use: debug("📝 selectedTemplate changed, updating workflowData:", { selectedTemplate })server/workflowScriptExecutorTool.ts (1)
36-77: JSON validation logic is overly complex and fragile.The
validateJsonfunction tries to handle escape sequences manually with complex regex replacements. This is error-prone and can miss edge cases. The native JSON parser already handles these correctly.Simplify:
function validateJson(obj: any): string | null { try { if (typeof obj === 'string') { - let processedJson = obj.trim() - - const escapeCharRegex = /\\[nrtfb"\\]/g - - if (escapeCharRegex.test(processedJson)) { - processedJson = processedJson - .replace(/\\n/g, '\\\\n') // \n -> \\n - .replace(/\\r/g, '\\\\r') // \r -> \\r - .replace(/\\t/g, '\\\\t') // \t -> \\t - .replace(/\\f/g, '\\\\f') // \f -> \\f - .replace(/\\b/g, '\\\\b') // \b -> \\b - .replace(/\\"/g, '\\\\"') // \" -> \\" - .replace(/\\\\/g, '\\\\\\\\') // \\ -> \\\\ - } - - // Try to parse the processed JSON - const parsed = JSON.parse(processedJson) + const parsed = JSON.parse(obj.trim()) // Return the object if it's a valid object type, otherwise null if (typeof parsed === 'object' && parsed !== null) { - return processedJson + return obj.trim() } else { return null } } else { if (typeof obj === 'object' && obj !== null) { - try { - return JSON.stringify(obj) - } catch (stringifyError) { - return null - } + return JSON.stringify(obj) } else { return null } } } catch (error) { - // If any parsing or validation fails, return null return null } }server/api/script.ts (1)
10-33: Consider adding response caching.The available script languages rarely change. Consider caching the response to avoid regenerating the code blocks on every request.
+let cachedLanguages: any = null + export const GetScriptLanguagesApi = async (c: Context) => { try { + if (cachedLanguages) { + return c.json(cachedLanguages) + } + const availableLanguages = Object.values(ScriptLanguage) const languagesWithCodeBlocks = availableLanguages.map(language => { const codeWritingBlock = getCodeWritingBlock(language, {}, {}) return { language, codeWritingBlock } }) - return c.json({ + cachedLanguages = { availableLanguages: languagesWithCodeBlocks, totalCount: availableLanguages.length - }) + } + + return c.json(cachedLanguages) } catch (error) {frontend/src/components/workflow/TriggerExecutionUI.tsx (2)
46-85: Race condition in state updates.If the component unmounts while
handleTriggeris executing (e.g., user navigates away), the state updates at lines 62 and 76 will attempt to update unmounted component state, causing React warnings.Add cleanup with abort controller:
const TriggerExecutionUI: React.FC<TriggerExecutionUIProps> = ({ // ... props }) => { + const isMountedRef = React.useRef(true) const [isTriggering, setIsTriggering] = useState(false) const [triggerError, setTriggerError] = useState<string | null>(null) const [triggerStatus, setTriggerStatus] = useState<'pending' | 'triggered' | 'error'>('pending') + + React.useEffect(() => { + return () => { + isMountedRef.current = false + } + }, []) const handleTrigger = async () => { console.log("🔍 Trigger button clicked:", { stepExecutionId }) setIsTriggering(true) setTriggerError(null) try { console.log("🔍 Making trigger API call to:", `/api/v1/workflow/steps/${stepExecutionId}/trigger`) const response = await api.workflow.steps[":stepId"].trigger.$post({ param: { stepId: stepExecutionId } }) console.log("🔍 Trigger API response:", response) const data = await response.json() console.log("🔍 Trigger API response data:", data) if (data.success) { - setTriggerStatus('triggered') + if (isMountedRef.current) { + setTriggerStatus('triggered') + } // Call the callback to restart workflow polling if (onTriggerSubmitted) { onTriggerSubmitted() } // Auto-close the sidebar after successful trigger setTimeout(() => { - if (onClose) { + if (onClose && isMountedRef.current) { onClose() } }, 2000) } else { throw new Error(data.message || 'Failed to trigger step') } } catch (error: any) { console.error('Error triggering step:', error) - setTriggerError(error.response?.data?.message || error.message || 'Failed to trigger step') - setTriggerStatus('error') + if (isMountedRef.current) { + setTriggerError(error.response?.data?.message || error.message || 'Failed to trigger step') + setTriggerStatus('error') + } } finally { - setIsTriggering(false) + if (isMountedRef.current) { + setIsTriggering(false) + } } }
70-74: Hardcoded auto-close delay lacks user control.The 2-second auto-close after successful trigger gives users no control. Consider making this configurable or adding a manual close option earlier.
// Auto-close the sidebar after successful trigger setTimeout(() => { if (onClose && isMountedRef.current) { onClose() } - }, 2000) + }, 3000) // Increase to 3 seconds to give users more time to see the success messagefrontend/src/components/workflow/WorkflowBuilder.tsx (4)
1160-1195: Use PlusButton for Script nodes for consistency.This inlines a custom “+” button while other nodes use PlusButton. Prefer the shared component to reduce duplication and keep behavior uniform.
- {/* Add Next Step Button */} - {hasNext && ( - <div - className="absolute left-1/2 transform -translate-x-1/2 flex flex-col items-center cursor-pointer z-50 pointer-events-auto" - style={{ top: "calc(100% + 8px)" }} - onClick={(e) => { - e.stopPropagation() - e.preventDefault() - console.log("Plus button clicked for node:", id) - const event = new CustomEvent("openWhatHappensNext", { - detail: { nodeId: id }, - }) - window.dispatchEvent(event) - }} - > - <div className="w-0.5 h-6 bg-gray-300 dark:bg-gray-600 mb-2"></div> - <div - className="bg-black hover:bg-gray-800 rounded-full flex items-center justify-center transition-colors shadow-lg" - style={{ - width: "28px", - height: "28px", - }} - > - <svg - className="w-4 h-4 text-white" - viewBox="0 0 24 24" - fill="none" - stroke="currentColor" - strokeWidth="2" - > - <line x1="12" y1="5" x2="12" y2="19"></line> - <line x1="5" y1="12" x2="19" y2="12"></line> - </svg> - </div> - </div> - )} + <PlusButton hasNext={hasNext} nodeId={id} isConnectable={isConnectable} showHandles={true} />
2997-3013: Strip or gate console logging in production.There are many console.log traces in execution and node-creation paths. Wrap behind a debug flag or remove to avoid noisy logs and potential PII leakage.
Example pattern:
-console.log("🔍 Full execution response:", response) +if (process.env.NODE_ENV !== "production") console.log("🔍 Full execution response:", response)Apply similarly across added logs in this file.
Also applies to: 3040-3067, 3092-3118, 3343-3348, 3490-3510, 3612-3636, 4731-4749
2419-2463: Edge labeling limited to templates; consider execution mode (optional).Edge labels for review paths are derived only when !isExecution. If stepExecutions carry sufficient metadata, consider labeling in execution view too for clarity.
Optionally mirror the template logic by resolving the originating review tool config via workflowStepTemplateId or execution metadata and setting sourceHandle/label accordingly.
Also applies to: 2485-2490
4213-4222: Save button enablement logic looks good; minor simplification optional.The closure computes hasActualSteps each render. You could hoist this into a memo for readability, but current logic is fine.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
frontend/bun.lockbis excluded by!**/bun.lockbserver/bun.lockbis excluded by!**/bun.lockb
📒 Files selected for processing (35)
Dockerfile(2 hunks)backup-database.sh(1 hunks)deployment/portable/.env.default(1 hunks)deployment/portable/deploy.sh(3 hunks)deployment/portable/docker-compose.app-build.yml(2 hunks)deployment/portable/prometheus-selfhosted.yml(2 hunks)frontend/package.json(2 hunks)frontend/src/components/CodeEditor.tsx(1 hunks)frontend/src/components/ScriptSidebar.tsx(1 hunks)frontend/src/components/workflow/AIAgentConfigUI.tsx(1 hunks)frontend/src/components/workflow/EmailConfigUI.tsx(1 hunks)frontend/src/components/workflow/PlusButton.tsx(1 hunks)frontend/src/components/workflow/ReviewExecutionUI.tsx(1 hunks)frontend/src/components/workflow/TriggerExecutionUI.tsx(1 hunks)frontend/src/components/workflow/WhatHappensNextUI.tsx(3 hunks)frontend/src/components/workflow/WorkflowBuilder.tsx(40 hunks)frontend/src/components/workflow/WorkflowIcons.tsx(1 hunks)frontend/src/components/workflow/api/ApiHandlers.ts(2 hunks)frontend/src/components/workflow/executedWorkflowRenderer.tsx(35 hunks)frontend/src/routes/_authenticated/workflow.tsx(12 hunks)global-packages.json(1 hunks)requirements.R(1 hunks)requirements.txt(1 hunks)restore-database.sh(1 hunks)server/.env.default(1 hunks)server/api/chat/agents.ts(2 hunks)server/api/script.ts(1 hunks)server/api/workflow.ts(32 hunks)server/db/schema/workflows.ts(2 hunks)server/server.ts(3 hunks)server/shared/types.ts(1 hunks)server/test-script-executor.js(1 hunks)server/types/workflowTypes.ts(1 hunks)server/workflowScriptExecutorTool.ts(1 hunks)start.sh(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (14)
frontend/src/components/workflow/api/ApiHandlers.ts (1)
frontend/src/api.ts (1)
api(5-5)
frontend/src/components/workflow/executedWorkflowRenderer.tsx (4)
frontend/src/routes/_authenticated/workflow.tsx (1)
WorkflowTemplate(17-89)frontend/src/components/workflow/Types.ts (2)
Step(64-92)Tool(56-61)server/db/schema/workflows.ts (1)
workflowTool(104-117)frontend/src/api.ts (1)
api(5-5)
server/test-script-executor.js (1)
server/workflowScriptExecutorTool.ts (1)
executeScript(638-859)
server/api/script.ts (2)
server/logger/index.ts (2)
getLoggerWithChild(192-200)Subsystem(15-15)server/workflowScriptExecutorTool.ts (2)
ScriptLanguage(861-861)getCodeWritingBlock(863-873)
server/workflowScriptExecutorTool.ts (1)
server/logger/index.ts (3)
getLoggerWithChild(192-200)Subsystem(15-15)getLogger(36-93)
backup-database.sh (1)
restore-database.sh (8)
print_info(28-30)print_success(32-34)print_error(40-42)list_backups(64-72)check_docker_container(45-51)check_local_postgres(54-61)show_usage(403-425)main(280-400)
frontend/src/components/workflow/ReviewExecutionUI.tsx (2)
frontend/src/api.ts (1)
api(5-5)frontend/src/components/workflow/api/ApiHandlers.ts (1)
workflowToolsAPI(376-407)
server/server.ts (3)
server/api/workflow.ts (4)
reviewSubmissionSchema(66-66)ReviewWorkflowStepApi(3732-3942)TriggerWorkflowStepApi(3944-4101)DownloadReviewFilesApi(4233-4442)server/db/schema/workflows.ts (1)
reviewSubmissionSchema(395-397)server/api/script.ts (1)
GetScriptLanguagesApi(10-33)
frontend/src/routes/_authenticated/workflow.tsx (3)
frontend/src/components/workflow/Types.ts (1)
WorkflowTemplate(227-276)frontend/src/components/workflow/api/ApiHandlers.ts (2)
fetchWorkflows(193-197)userWorkflowsAPI(189-224)server/db/schema/workflows.ts (1)
workflowTemplate(60-76)
server/api/workflow.ts (3)
server/db/schema/workflows.ts (6)
workflowStepExecution(143-169)workflowTool(104-117)workflowStepTemplate(79-101)workflowExecution(120-140)toolExecution(172-189)workflowTemplate(60-76)server/services/emailService.ts (1)
emailService(124-124)server/workflowScriptExecutorTool.ts (2)
ScriptLanguage(861-861)executeScript(638-859)
frontend/src/components/ScriptSidebar.tsx (3)
frontend/src/api.ts (1)
api(5-5)frontend/src/components/workflow/api/ApiHandlers.ts (2)
workflowToolsAPI(376-407)workflowStepsAPI(410-469)frontend/src/components/CodeEditor.tsx (1)
CodeEditor(28-177)
frontend/src/components/workflow/TriggerExecutionUI.tsx (1)
frontend/src/api.ts (1)
api(5-5)
server/db/schema/workflows.ts (1)
server/api/workflow.ts (1)
reviewSubmissionSchema(66-66)
frontend/src/components/workflow/WorkflowBuilder.tsx (4)
frontend/src/components/workflow/PlusButton.tsx (1)
PlusButton(10-73)frontend/src/routes/_authenticated/workflow.tsx (1)
WorkflowTemplate(17-89)frontend/src/components/workflow/Types.ts (2)
WorkflowTemplate(227-276)Tool(56-61)frontend/src/components/ScriptSidebar.tsx (1)
ScriptSidebar(29-514)
🪛 dotenv-linter (3.3.0)
server/.env.default
[warning] 29-29: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 30-30: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 31-31: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 31-31: [UnorderedKey] The DB_PASSWORD key should go before the DB_USER key
(UnorderedKey)
[warning] 32-32: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 32-32: [UnorderedKey] The DB_HOST key should go before the DB_NAME key
(UnorderedKey)
[warning] 33-33: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 33-33: [UnorderedKey] The DB_PORT key should go before the DB_USER key
(UnorderedKey)
🪛 Shellcheck (0.11.0)
deployment/portable/deploy.sh
[warning] 129-129: NO_CACHE_FLAG appears unused. Verify use (or export if used externally).
(SC2034)
backup-database.sh
[warning] 70-70: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 107-107: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 147-147: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 159-159: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
restore-database.sh
[warning] 109-109: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 338-338: Declare and assign separately to avoid masking return values.
(SC2155)
🔇 Additional comments (11)
deployment/portable/.env.default (1)
22-23: LGTM!The addition of
USE_BUBBLEWRAP=truefor portable deployments is appropriate for sandboxing script execution and aligns with the broader Bubblewrap integration in this PR.server/.env.default (1)
24-25: LGTM!The addition of the Bubblewrap configuration section with
USE_BUBBLEWRAP=falseis appropriate for the server environment, providing flexibility in sandbox configuration.Dockerfile (1)
71-71: Verify the npm global installation command handles missing dependencies gracefully.The command parses
global-packages.jsonwithjqand installs packages globally. If the JSON structure changes or packages are unavailable, the build will fail without clear error messages.Consider adding error handling or using a more robust approach:
-RUN npm install -g $(cat /usr/src/app/global-packages.json | jq -r '.dependencies | keys[]') +RUN npm install -g $(cat /usr/src/app/global-packages.json | jq -r '.dependencies | keys[]') || \ + { echo "Failed to install global npm packages"; exit 1; }Alternatively, iterate over packages individually for better error reporting:
RUN jq -r '.dependencies | keys[]' /usr/src/app/global-packages.json | \ xargs -I {} sh -c 'npm install -g {} || { echo "Failed to install {}"; exit 1; }'global-packages.json (1)
3-3: No action needed: axios@^1.5.0 resolves to v1.12.2 with all known HIGH-severity vulnerabilities patched.server/types/workflowTypes.ts (1)
23-25: LGTM! New tool types align with PR objectives.The addition of REVIEW, TRIGGER, and SCRIPT tool types is well-structured and consistent with the existing enum pattern. These new types are properly integrated throughout the codebase (workflow APIs, UI components, and database schemas).
server/db/schema/workflows.ts (1)
395-397: LGTM! Schema is well-defined and type-safe.The review submission schema is properly structured with Zod validation ensuring only "approved" or "rejected" values are accepted. The TypeScript type inference via
z.infermaintains type safety throughout the codebase.frontend/src/components/workflow/AIAgentConfigUI.tsx (1)
341-361: LGTM! Header styling is now consistent with other workflow components.The refactor from inline styles to Tailwind utility classes improves maintainability and ensures consistent styling across the workflow UI components (matches EmailConfigUI.tsx, etc.).
Note: The previous review comment about the Gemini model filter at line 110 is still relevant and should be addressed separately.
frontend/src/components/workflow/EmailConfigUI.tsx (1)
201-221: LGTM! Consistent styling and enhanced close behavior.The changes improve consistency:
- Header styling now matches AIAgentConfigUI.tsx pattern
- New
onCloseprop provides flexibility for parent components- Fallback to
onBackmaintains backward compatibilityThe implementation correctly prioritizes
onCloseoveronBackfor the close button (line 216).server/api/workflow.ts (1)
359-369: Verify and document trigger-root-step behavior
- Audit all existing workflow templates to ensure they handle optional
rootStepInputwhenToolType.TRIGGERis used.- Update the API docs to state that
rootStepInputis no longer required for manual trigger root steps.- Add or update integration tests to cover the new trigger-root-step execution path.
frontend/package.json (1)
19-19: Verify Monaco Editor version compatibility.Version 4.6.0 of @monaco-editor/react is current based on learnings, but ensure compatibility with React 19.1.0 that's being used. Monaco Editor v4 requires React 16.8+, so this should work, but test thoroughly.
Based on learnings
Run integration tests to confirm Monaco Editor works correctly with React 19.1.0 and the build system (Vite).
frontend/src/components/workflow/WorkflowIcons.tsx (1)
68-85: LGTM!The ReviewIcon component follows the same pattern as other icon components in the file and uses appropriate SVG paths for an eye/review icon.
YashJuspay626
force-pushed
the
files-extract2
branch
2 times, most recently
from
8808fcd to
dff44df
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (25)
server/api/chat/agents.ts (1)
3579-3579: Duplicate: Unused variablefolderIdsalready flagged in previous review.As noted in the previous review,
folderIdsis extracted but never used in the subsequent code. Please refer to the existing review comment for detailed remediation options (remove the declaration or implement folder-level context handling).frontend/src/components/workflow/executedWorkflowRenderer.tsx (5)
1-35: Critical: MissingbotLogoimport causes runtime error.Line 953 references
botLogobut this import is missing. This will cause a runtime error when rendering python_script nodes.Add the missing import:
import React, { useCallback, useState, useEffect, useRef } from "react" import { Bot, Mail, Settings, X, FileTextIcon , FileText, Code} from "lucide-react" import ReviewExecutionUI from "./ReviewExecutionUI" import TriggerExecutionUI from "./TriggerExecutionUI" +import botLogo from "../../assets/bot-logo.svg"
1490-1494: Critical: Fix property name typo.Line 1493 uses
workflow_tool_idsbut the type definition usestoolExecIds. This property mismatch will cause the previous step output logic to fail.const prevStepTools = workflowData.toolExecutions?.filter((toolExec: any) => - prevStep.workflow_tool_ids?.includes(toolExec.workflowToolId), + prevStep.toolExecIds?.includes(toolExec.id), ) || []
2043-2043: Fix polling timeout ref type for browser compatibility.The
NodeJS.Timeouttype is incorrect for browsersetTimeout, which returnsnumber. This type mismatch can cause issues.- const pollingTimeoutRef = useRef<NodeJS.Timeout | null>(null) + const pollingTimeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null)
2058-2063: Add cleanup for polling timeout to prevent memory leaks.The useEffect at line 2058 starts polling but has no cleanup function. This causes memory leaks and stale closures when the component unmounts or when dependencies change.
useEffect(() => { if (selectedTemplate?.id) { console.log("🚀 Component mounted, fetching initial workflow status for:", selectedTemplate.id) fetchWorkflowStatus(selectedTemplate.id) } - }, []) + return () => { + if (pollingTimeoutRef.current) { + clearTimeout(pollingTimeoutRef.current) + pollingTimeoutRef.current = null + } + } + }, [selectedTemplate?.id])Note: The dependency array should include
selectedTemplate?.idso polling restarts when the template changes.
2620-2782: Improve polling resilience and security.The polling implementation has several issues:
- No abort controller to cancel in-flight requests on unmount
- No exponential backoff on errors
- No maximum retry limit (infinite loop risk)
- Logs sensitive execution data to console in production
Add abort controller, exponential backoff, retry limits, and production-safe logging:
+ const abortControllerRef = useRef<AbortController | null>(null) + const retryCountRef = useRef(0) + const MAX_RETRIES = 10 + const fetchWorkflowStatus = (async (executionId: string) => { + // Cancel previous request if still pending + if (abortControllerRef.current) { + abortControllerRef.current.abort() + } + + abortControllerRef.current = new AbortController() + try { - console.log("📊 Fetching execution data for:", executionId) + if (process.env.NODE_ENV === 'development') { + console.log("📊 Fetching execution data for:", executionId) + } - const response = await api.workflow.executions[executionId].$get() + const response = await api.workflow.executions[executionId].$get({ + signal: abortControllerRef.current.signal + }) if (!response.ok) { throw new Error(`HTTP ${response.status}: ${response.statusText}`) } const executionData = await response.json() - console.log("📊 Raw execution data:", executionData) + if (process.env.NODE_ENV === 'development') { + console.log("📊 Raw execution data:", executionData) + } // ... rest of processing ... + // Reset retry count on success + retryCountRef.current = 0 + // Continue polling after 5 seconds pollingTimeoutRef.current = setTimeout(() => { fetchWorkflowStatus(executionId) }, 5000) return extractedData } catch (error) { + // Don't retry if request was aborted + if (error instanceof DOMException && error.name === 'AbortError') { + return null + } + console.error('❌ Failed to fetch execution data:', error) - // Continue polling on error after 5 seconds + retryCountRef.current++ + + if (retryCountRef.current >= MAX_RETRIES) { + console.error('Max retries reached, stopping polling') + return null + } + + // Exponential backoff: 5s, 10s, 20s, 40s, capped at 60s + const backoffDelay = Math.min(5000 * Math.pow(2, retryCountRef.current - 1), 60000) + console.log(`⏰ Scheduling retry ${retryCountRef.current}/${MAX_RETRIES} in ${backoffDelay}ms`) pollingTimeoutRef.current = setTimeout(() => { fetchWorkflowStatus(executionId) - }, 5000) + }, backoffDelay) return null } })Also update the cleanup in useEffect:
useEffect(() => { if (selectedTemplate?.id) { fetchWorkflowStatus(selectedTemplate.id) } return () => { if (pollingTimeoutRef.current) { clearTimeout(pollingTimeoutRef.current) pollingTimeoutRef.current = null } + if (abortControllerRef.current) { + abortControllerRef.current.abort() + abortControllerRef.current = null + } + retryCountRef.current = 0 } - }, []) + }, [selectedTemplate?.id])requirements.txt (1)
1-1: Security vulnerability in requests version.As noted in a previous review, requests 2.31.0 has known vulnerabilities (CVE-2024-35195 for .netrc credentials leak and session-verify bypass). Update to ≥2.32.5.
Apply this diff:
-requests==2.31.0 +requests==2.32.5requirements.R (1)
6-14: Duplicate package entries in install.packages call.As noted in a previous review, several packages appear multiple times in the installation vector:
httr(lines 8, 11)stringr(lines 10, 12)curl(lines 9, 11)Apply this diff to remove duplicates:
install.packages(c( "jsonlite", # JSON parsing/generation "httr", # HTTP requests for API calls "curl", # Basic HTTP/network operations - "methods", "datasets", "stringr", "lubridate", "dplyr", - "httr", "RCurl", "curl","digest","tidyverse","glue","bigrquery", - "readr","readxl","googlesheets4","stringr","data.table", + "methods", "datasets", "stringr", "lubridate", "dplyr", + "RCurl", "digest","tidyverse","glue","bigrquery", + "readr","readxl","googlesheets4","data.table", "countrycode","foreach","doParallel","googledrive" ), dependencies=TRUE, quiet=FALSE)Dockerfile (2)
70-70: Avoid using--break-system-packagesflag.This issue was previously flagged and remains unaddressed. The
--break-system-packagesflag bypasses Python 3.11+ safety checks that prevent conflicts with system-managed packages.Consider using a virtual environment instead:
-RUN python3 -m pip install --break-system-packages -r /usr/src/app/requirements.txt +RUN python3 -m venv /opt/venv && \ + /opt/venv/bin/pip install -r /usr/src/app/requirements.txt +ENV PATH="/opt/venv/bin:$PATH"
93-93: Review SUID bit on bwrap for security.This issue was previously flagged. Setting the SUID bit on
bwrapallows unprivileged users to execute it with root privileges, which poses security risks if not managed carefully.Ensure this is strictly necessary and that all uses of
bwrapare secure and properly configured to drop privileges as soon as possible.deployment/portable/docker-compose.app-build.yml (1)
26-26: Avoidprivileged: truefor security.This issue was previously flagged. Running with
privileged: truegrants the container all host capabilities, disabling container isolation.If specific capabilities are needed (e.g., for bubblewrap), grant them explicitly using
cap_addinstead:- privileged: true + cap_add: + - SYS_ADMIN # Only if needed for bubblewrapfrontend/src/components/workflow/TriggerExecutionUI.tsx (1)
32-44: Fix useEffect syntax and add dependency array.This issue was previously flagged. The useEffect has a misplaced closing parenthesis and no dependency array, causing it to run on every render.
Apply this fix:
const [triggerError, setTriggerError] = useState<string | null>(null) const [triggerStatus, setTriggerStatus] = useState<'pending' | 'triggered' | 'error'>('pending') + React.useEffect(() => { - console.log("🔍 TriggerExecutionUI props changed:", { isVisible, stepExecutionId, stepName, builder, path }) - // Debug logging - console.log("🔍 TriggerExecutionUI render:", { - isVisible, - builder, - stepExecutionId, - triggerStatus - }) - - } - -) + if (process.env.NODE_ENV === 'development') { + console.log("🔍 TriggerExecutionUI props changed:", { + isVisible, + stepExecutionId, + stepName, + builder, + path, + triggerStatus + }) + } + }, [isVisible, stepExecutionId, stepName, builder, path, triggerStatus])frontend/src/components/workflow/ReviewExecutionUI.tsx (2)
50-53: Reset submission status when panel re-opens.This issue was previously flagged. After one approval/rejection,
submissionStatusremains at 'approved'/'rejected', so action buttons don't render when the panel reopens.Add a useEffect to reset state:
const [submissionStatus, setSubmissionStatus] = useState<'pending' | 'approved' | 'rejected' | 'error'>('pending') + + React.useEffect(() => { + if (isVisible) { + setSubmissionStatus("pending") + setSubmitError(null) + setDownloadError(null) + } + }, [isVisible, stepExecutionId])
137-140: Stop inferring execution ID from step ID.This issue was previously flagged. The fallback
stepExecutionId.split('-')[0]produces an invalid truncated UUID and breaks downloads.Remove the invalid fallback:
- const executionId = workflowExecutionId || - previousStepResult?.workflowExecutionId || - stepExecutionId.split('-')[0] + const executionId = + workflowExecutionId ?? previousStepResult?.workflowExecutionId + + if (!executionId) { + throw new Error("Workflow execution ID not available") + }frontend/src/components/workflow/WorkflowBuilder.tsx (3)
2241-2510: Execution layout still uses template IDs.
performSimpleDFSseeds/recurses with template IDs (rootWorkflowStepTemplateId,nextStepIds) even when rendering executions. Execution nodes therefore fall back to(400,100)while template root is positioned, exactly as noted earlier. Build a template→execution map, translate IDs (root + children), and look up nodes by execution ID before assigning positions.
3342-3468: Honor the selected review path when wiring next steps.
selectedReviewPathis captured from What Happens Next but the AI Agent, Email, and Script handlers still auto-pick the first free path. That regresses the UX called out previously—connecting can jump to the wrong branch. Please preferselectedReviewPathwhen choosingsourceHandle/labels and fall back only if that path is already populated (apply the same fix across all three handlers).Also applies to: 3605-3784, 4637-4759
1018-1025: Fix PlusButton nodeId for trigger nodes (still using data.id).
data.idis unset for trigger nodes created from templates, so the plus button dispatchesnodeId: "undefined". Use the node’s actualid(already in scope) and the derivedhasNextflag. Same bug as the earlier review—please apply the suggested fix.- {!isAwaitingTrigger && ( - <PlusButton - hasNext={Boolean(data.hasNext)} - nodeId={String(data.id)} - isConnectable={isConnectable} - showHandles={false} - /> - )} + {!isAwaitingTrigger && ( + <PlusButton + hasNext={Boolean(hasNext)} + nodeId={id} + isConnectable={isConnectable} + showHandles={false} + /> + )}server/workflowScriptExecutorTool.ts (6)
30-30: Timeout value remains excessive.The 40-minute timeout persists from prior reviews and allows scripts to consume resources for extended periods.
Based on past review comments.
33-33: Sandbox toggle still forced to true.The
|| trueexpression continues to forceUSE_BUBBLEWRAPto always be true, making the environment variable ineffective.Based on past review comments.
229-343: Security validation remains bypassable.The regex-based import and keyword checks persist from prior reviews and can still be circumvented via string concatenation, dynamic evaluation, or indirect access patterns.
Based on past review comments.
346-409: Input size validation still missing.The function continues to accept arbitrarily large
script,input, andconfigparameters without size limits, which could cause memory exhaustion or denial of service.Based on past review comments.
577-636: Path traversal vulnerability remains.The function still lacks validation for
exportDirandimportDirpaths, allowing potential access to unintended filesystem locations.Based on past review comments.
638-859: Resource cleanup and validation gaps persist.While script file cleanup has improved, several issues from past reviews remain:
workflowIdlacks validation for safe filesystem use (line 640)exportDircleanup only occurs in the success path (lines 760-761), not in error cases- No validation that export/import directories are within expected bounds
Based on past review comments.
server/api/workflow.ts (2)
897-927: Leaf node logic needs refinement for conditional workflows.The completion logic from prior reviews still lacks explicit branch tracking. Steps with multiple next steps (e.g., approve/reject paths) may be incorrectly marked as leaf nodes if one branch remains unexecuted but was intentionally not taken.
Based on past review comments.
4260-4268: CRITICAL: Access control remains disabled.The access control check is still commented out, allowing any authenticated user to download files from any workflow execution. This is a critical security vulnerability flagged in previous reviews.
Based on past review comments.
🧹 Nitpick comments (14)
frontend/src/components/workflow/executedWorkflowRenderer.tsx (3)
64-73: Avoidanytype fortoolConfig.The
toolConfigfield usesany, which bypasses type safety and can hide errors. Consider defining a proper type or usingunknownwith type guards.Define a proper type for toolConfig:
+interface ToolConfig { + [key: string]: unknown +} + toolExecutions?: Array<{ id: string type: string toolType: string status: string result?: any createdAt: string updatedAt: string - toolConfig?: any + toolConfig?: ToolConfig }>
894-1155: Eliminate duplication between python_script and script tool rendering.The rendering logic for
python_script(lines 894-1021) andscript(lines 1023-1155) tools is nearly identical, differing only in icon and default description. This duplication makes maintenance harder.Extract the shared rendering logic into a helper function:
const renderScriptNode = ( step: any, tools: any[], selected: boolean, isCompleted: boolean, isConnectable: boolean, iconComponent: React.ReactNode, defaultDescription: string ) => { const hasFailedToolExecution = tools && tools.some((tool) => (tool as any).status === "failed") const isFailed = step.status === "failed" || hasFailedToolExecution const isActive = step.status === "active" return ( <div className="relative cursor-pointer hover:shadow-lg transition-shadow" style={{ // ... shared styles }} > {/* Header with icon and title */} <div className="flex items-center gap-3 text-left w-full px-4 pt-4 mb-3"> <div className="flex justify-center items-center flex-shrink-0" style={{ // ... icon container styles }} > {iconComponent} </div> {/* ... rest of shared JSX */} </div> </div> ) }Then replace both blocks with:
// For python_script if (step.type === "python_script" || hasPythonScriptTool) { return renderScriptNode( step, tools, selected, isCompleted, isConnectable, <img src={botLogo} alt="Bot" width={16} height={16} />, "Execute Python script to process data and generate results." ) } // For script if (step.type === "script" || hasScriptTool) { const scriptTool = hasScriptTool ? tools[0] : null const scriptData = (scriptTool as any)?.result || (scriptTool as any)?.config || {} const language = scriptData?.language || (scriptTool as any)?.toolType || "script" const scriptDescription = step.description || `Execute ${language} script with custom code` return renderScriptNode( step, tools, selected, isCompleted, isConnectable, <Code width={16} height={16} color="#10B981" />, scriptDescription ) }
1693-1707: Reduce duplication in script tool output handling.The script tool type check
tool.type === "script"appears in multiple locations (lines 1694, 1849, 1897, 1947) with similar logic. Consider extracting this into a helper function.Create a helper to check if a tool is a script type:
const isScriptTool = (tool: any, step?: any) => { return tool.type === "script" || tool.toolType === "script" || step?.type === "script" }Then simplify the conditionals:
// In input display if (isScriptTool(null, step)) { return ( <pre className="whitespace-pre-wrap"> {typeof output === "object" ? JSON.stringify(output, null, 2) : String(output)} </pre> ) } // In button rendering if (isScriptTool(tool)) { return ( <button onClick={() => onResultClick?.(tool.result)} className="text-xs px-2 py-1 bg-blue-100 hover:bg-blue-200 text-blue-700 rounded transition-colors" > View Full </button> ) }Also applies to: 1848-1858, 1896-1900, 1946-1955
frontend/src/components/ScriptSidebar.tsx (3)
319-327: Inconsistent error handling between create and update paths.Lines 323-326 silently ignore API errors during step creation, similar to the update path. However, the comment "Continue with UI update even if API call fails" suggests intentional optimistic updates. This pattern should be consistently documented and implemented across both paths.
Extract the error-handling logic into a shared helper function to ensure consistency:
const handleAPIError = (operation: string, error: unknown) => { console.error(`Failed to ${operation}:`, error) // TODO: Add toast notification // Continue with UI update (optimistic update pattern) }Then use it in both the update (line 269) and create (line 324) paths.
228-241: Duplicate value and val fields in scriptToolPayload.Lines 228-241 assign both
valueandvalwith identicalscriptData. The comment "Ensure both value and val are updated for consistency" suggests backward compatibility, but maintaining two identical fields increases maintenance burden and potential for inconsistency.If
valis for backward compatibility only, consider documenting which field is canonical and adding a deprecation notice. Once migration is complete, remove the deprecated field.const scriptToolPayload = { type: "script", value: scriptData, // Canonical field val: scriptData, // @deprecated - For backward compatibility with older nodes config: { language: selectedLanguage, timeout: 300, } }
181-188: JSON validation helper can be simplified.The
validateJSONfunction (lines 181-188) uses a try-catch to validate JSON. This is fine, but consider extracting it to a shared utility if JSON validation is needed elsewhere in the codebase.If JSON validation is used in multiple components, move it to a shared utility file:
// utils/validation.ts export const isValidJSON = (str: string): boolean => { try { JSON.parse(str) return true } catch { return false } }frontend/src/components/CodeEditor.tsx (1)
121-121: Hard-coded light theme may not match user preference.Line 121 hard-codes
theme="light"for the Monaco editor. This ignores the dark mode classes visible elsewhere in the component (e.g.,dark:bg-gray-900at line 92), potentially creating a jarring UX when users are in dark mode.Detect the user's theme preference and pass it to Monaco:
// Add at the top of the component const isDarkMode = document.documentElement.classList.contains('dark') // Then use it: theme={isDarkMode ? "vs-dark" : "light"}deployment/portable/.env.default (1)
23-23: USE_BUBBLEWRAP flag lacks inline documentation.Line 23 adds
USE_BUBBLEWRAP=truewithout a comment explaining its purpose. Given that other environment variables have descriptive comments or are self-explanatory, this new flag should also be documented.Add an inline comment:
USER_SECRET= +# Enable bubblewrap sandboxing for script execution USE_BUBBLEWRAP=truefrontend/package.json (1)
19-19: Upgrade Monaco Editor dependencies to latest stable versions. Bump@monaco-editor/reactfrom ^4.6.0 to ^4.7.0 andmonaco-editorfrom ^0.52.2 to ^0.54.0.frontend/src/routes/_authenticated/workflow.tsx (1)
17-89: Reuse the canonical WorkflowTemplate type.We already define
WorkflowTemplateinfrontend/src/components/workflow/Types.ts. Duplicating it here invites drift (e.g. mismatched fields liketoolExecIds). Please import the shared type instead of redeclaring.server/workflowScriptExecutorTool.ts (2)
36-77: Consider using a robust JSON validation library.The custom escape character handling (lines 44-51) with multiple replace operations is fragile and may miss edge cases like mixed escape sequences or nested structures. Consider using a battle-tested JSON validation library instead of reimplementing parsing logic.
439-575: Output formatters use fragile string manipulation.The language-specific formatters (Python, JavaScript, R) rely on regex replacements that may fail on edge cases:
replace(/'/g, '"')doesn't handle escaped quotes correctly- Nested structures with mixed quote types could break
- No size limits on output being processed
Consider using language-specific parsers (e.g., ast.literal_eval for Python) or enforcing that scripts must return valid JSON.
server/api/workflow.ts (2)
1067-1152: Add email safeguards for review notifications.The notification function lacks:
- Rate limiting to prevent email spam if the function is called multiple times
- Validation that email addresses are well-formed
- Deduplication check to avoid sending duplicate notifications if the step is reactivated
Consider tracking sent notifications in step metadata and validating email formats before sending.
2763-2769: Avoid mutating original tool objects.Line 2768 adds
_tempIddirectly to the tool object, which mutates the original. If the tool object is used elsewhere, this could cause unexpected behavior. Consider storing the tempId mapping separately or using a WeakMap to avoid mutation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
frontend/bun.lockbis excluded by!**/bun.lockbserver/bun.lockbis excluded by!**/bun.lockb
📒 Files selected for processing (28)
Dockerfile(2 hunks)deployment/portable/.env.default(1 hunks)deployment/portable/docker-compose.app-build.yml(2 hunks)frontend/package.json(2 hunks)frontend/src/components/CodeEditor.tsx(1 hunks)frontend/src/components/ScriptSidebar.tsx(1 hunks)frontend/src/components/workflow/AIAgentConfigUI.tsx(1 hunks)frontend/src/components/workflow/EmailConfigUI.tsx(1 hunks)frontend/src/components/workflow/PlusButton.tsx(1 hunks)frontend/src/components/workflow/ReviewExecutionUI.tsx(1 hunks)frontend/src/components/workflow/TriggerExecutionUI.tsx(1 hunks)frontend/src/components/workflow/WhatHappensNextUI.tsx(3 hunks)frontend/src/components/workflow/WorkflowBuilder.tsx(40 hunks)frontend/src/components/workflow/WorkflowIcons.tsx(1 hunks)frontend/src/components/workflow/api/ApiHandlers.ts(2 hunks)frontend/src/components/workflow/executedWorkflowRenderer.tsx(35 hunks)frontend/src/routes/_authenticated/workflow.tsx(12 hunks)requirements.R(1 hunks)requirements.txt(1 hunks)server/.env.default(1 hunks)server/api/chat/agents.ts(2 hunks)server/api/script.ts(1 hunks)server/api/workflow.ts(32 hunks)server/db/schema/workflows.ts(2 hunks)server/server.ts(3 hunks)server/types/workflowTypes.ts(1 hunks)server/workflowScriptExecutorTool.ts(1 hunks)start.sh(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (12)
server/workflowScriptExecutorTool.ts (1)
server/logger/index.ts (3)
getLoggerWithChild(192-200)Subsystem(15-15)getLogger(36-93)
frontend/src/components/ScriptSidebar.tsx (3)
frontend/src/api.ts (1)
api(5-5)frontend/src/components/workflow/api/ApiHandlers.ts (2)
workflowToolsAPI(376-407)workflowStepsAPI(410-469)frontend/src/components/CodeEditor.tsx (1)
CodeEditor(28-177)
frontend/src/components/workflow/executedWorkflowRenderer.tsx (3)
frontend/src/routes/_authenticated/workflow.tsx (1)
WorkflowTemplate(17-89)frontend/src/components/workflow/Types.ts (2)
Step(64-92)Tool(56-61)frontend/src/api.ts (1)
api(5-5)
frontend/src/components/workflow/ReviewExecutionUI.tsx (2)
frontend/src/api.ts (1)
api(5-5)frontend/src/components/workflow/api/ApiHandlers.ts (1)
workflowToolsAPI(376-407)
server/api/script.ts (2)
server/logger/index.ts (2)
getLoggerWithChild(192-200)Subsystem(15-15)server/workflowScriptExecutorTool.ts (2)
ScriptLanguage(861-861)getCodeWritingBlock(863-873)
server/api/workflow.ts (3)
server/db/schema/workflows.ts (6)
workflowStepExecution(143-169)workflowTool(104-117)workflowStepTemplate(79-101)workflowExecution(120-140)toolExecution(172-189)workflowTemplate(60-76)server/services/emailService.ts (1)
emailService(124-124)server/workflowScriptExecutorTool.ts (1)
executeScript(638-859)
server/server.ts (3)
server/api/workflow.ts (7)
reviewSubmissionSchema(66-66)ReviewWorkflowStepApi(3732-3942)TriggerWorkflowStepApi(3944-4101)GetFormDefinitionApi(4107-4161)SubmitFormStepApi(4104-4104)ServeWorkflowFileApi(4216-4230)DownloadReviewFilesApi(4233-4442)server/db/schema/workflows.ts (1)
reviewSubmissionSchema(395-397)server/api/script.ts (1)
GetScriptLanguagesApi(10-33)
frontend/src/routes/_authenticated/workflow.tsx (3)
frontend/src/components/workflow/Types.ts (1)
WorkflowTemplate(227-276)frontend/src/components/workflow/api/ApiHandlers.ts (2)
fetchWorkflows(193-197)userWorkflowsAPI(189-224)server/db/schema/workflows.ts (1)
workflowTemplate(60-76)
frontend/src/components/workflow/TriggerExecutionUI.tsx (1)
frontend/src/api.ts (1)
api(5-5)
server/db/schema/workflows.ts (1)
server/api/workflow.ts (1)
reviewSubmissionSchema(66-66)
frontend/src/components/workflow/WorkflowBuilder.tsx (4)
frontend/src/components/workflow/PlusButton.tsx (1)
PlusButton(10-73)frontend/src/components/workflow/api/ApiHandlers.ts (1)
workflowExecutionsAPI(240-373)frontend/src/components/workflow/Types.ts (2)
WorkflowTemplate(227-276)Tool(56-61)frontend/src/components/ScriptSidebar.tsx (1)
ScriptSidebar(29-514)
frontend/src/components/workflow/api/ApiHandlers.ts (1)
frontend/src/api.ts (1)
api(5-5)
🪛 dotenv-linter (3.3.0)
server/.env.default
[warning] 29-29: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 30-30: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 31-31: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 31-31: [UnorderedKey] The DB_PASSWORD key should go before the DB_USER key
(UnorderedKey)
[warning] 32-32: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 32-32: [UnorderedKey] The DB_HOST key should go before the DB_NAME key
(UnorderedKey)
[warning] 33-33: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 33-33: [UnorderedKey] The DB_PORT key should go before the DB_USER key
(UnorderedKey)
🔇 Additional comments (19)
frontend/src/components/workflow/executedWorkflowRenderer.tsx (1)
1679-1690: Good: Script input display logic is correct.The special handling for script nodes showing complete input data is appropriate, as scripts may need access to the full previous step output structure.
frontend/src/components/ScriptSidebar.tsx (2)
82-94: Verify state reset timing in cleanup.The reset in the
elseblock (lines 86-93) clears state whenisOpenbecomes false. If the sidebar closes while async operations are in-flight (e.g.,fetchLanguages), the cleanup may reset state prematurely, leading to unexpected UI behavior.Consider moving the reset into a cleanup function that runs after any pending async operations complete, or add guards in
fetchLanguagesto check if the component is still open before updating state.
97-122: Potential stale-closure issue in initialData useEffect.Lines 97-122 depend on
isOpen,languages, andinitialData, but the effect does not includeselectedLanguagein the dependency array. IfinitialDatachanges while the sidebar is open and a different language is already selected, the effect may overwrite the user's current selection unexpectedly.Verify that
initialDatachanges during editing sessions do not conflict with user actions. Consider addingselectedLanguageto the dependency array or using a ref to track whether initial data has already been loaded.frontend/src/components/workflow/WorkflowIcons.tsx (1)
68-85: LGTM! ReviewIcon follows existing patterns.The new ReviewIcon component is consistent with other icons in the file, using the same props pattern and SVG structure.
frontend/src/components/CodeEditor.tsx (1)
15-26: Mapping matches backend languages; no changes needed. Frontend’s getMonacoLanguage covers all ScriptLanguage enum values (python, javascript, r), so no unsupported languages are returned.server/db/schema/workflows.ts (1)
395-397: LGTM! Clean schema addition for review submissions.The
reviewSubmissionSchemaandReviewSubmissionRequesttype are well-defined, following existing patterns in the file. The enum constraint on theinputfield properly restricts values to "approved" or "rejected".Also applies to: 434-434
server/api/script.ts (1)
10-33: LGTM! Well-structured API endpoint.The
GetScriptLanguagesApiimplementation is clean and follows best practices:
- Proper error handling with try-catch
- Contextual logging
- Returns structured JSON response with language and code block information
frontend/src/components/workflow/WhatHappensNextUI.tsx (1)
49-80: LGTM! Actions extended for new workflow tooling.The additions for review, trigger, and run_script actions are well-integrated:
- Icons are appropriate for each action
- Click handling correctly triggers
onSelectActionfor review and trigger- Follows existing UI patterns
Also applies to: 136-136
deployment/portable/docker-compose.app-build.yml (1)
67-114: LGTM! app-sync service is well-configured.The new
app-syncservice follows Docker Compose best practices:
- Proper dependencies with
depends_onconditions- Appropriate healthcheck configuration
- Consistent volume and network setup with the main app service
server/server.ts (1)
221-222: LGTM! API routes properly wired.The new workflow endpoints are correctly integrated:
- Review and trigger routes use appropriate validation middleware
- Download endpoint supports both GET and POST methods
- Script languages endpoint follows existing patterns
Also applies to: 226-227, 237-240, 1085-1090, 1094-1095, 1098-1098
frontend/src/components/workflow/TriggerExecutionUI.tsx (1)
46-85: LGTM! Trigger handling is well-implemented.The
handleTriggerfunction properly:
- Manages loading state
- Calls the API with correct parameters
- Invokes callback to restart polling
- Auto-closes on success with appropriate delay
frontend/src/components/workflow/ReviewExecutionUI.tsx (2)
236-261: LGTM! Robust email validation.The
validateEmailfunction includes comprehensive checks:
- Length validation (max 254 characters)
- Regex pattern matching
- Consecutive dots check
- Leading/trailing dot validation
316-361: LGTM! Configuration save with proper error handling.The
handleSaveEmailConfigfunction:
- Updates tool via API when toolId is present
- Falls back to parent handler even if API fails
- Includes helpful debug logging
server/api/workflow.ts (6)
315-369: LGTM: Trigger step validation.The logic correctly identifies trigger-type root steps and bypasses input validation for them while providing empty input objects. This allows workflows to start with manual trigger steps that don't require user input.
1155-1203: LGTM: Downstream step activation logic.The function correctly identifies steps ready to activate by checking prerequisite completion. Integration with review notifications is clean and properly separated.
2087-2160: LGTM: Review and trigger tool handling.The implementation correctly handles both tool types:
- Review tools send optional email notifications and await user decisions
- Trigger tools await manual completion
- Both return appropriate status and messages for workflow coordination
2442-2540: Verify sandbox enforcement for script execution.While the script execution logic is sound, confirm that
USE_BUBBLEWRAPinworkflowScriptExecutorTool.tsis properly configured in production. The current implementation forces it totrue(flagged separately), which provides sandboxing but makes the environment variable meaningless.Based on review of server/workflowScriptExecutorTool.ts line 33.
2892-2921: LGTM: Review tool config mapping.The third-pass update correctly translates frontend step IDs to backend step IDs in review tool configurations, ensuring approved/rejected paths reference the correct step templates.
4346-4385: LGTM: Path traversal protection.The file path validation correctly prevents path traversal attacks:
- Normalizes paths and rejects those containing
..- Validates absolute paths contain the execution ID
- Ensures paths stay within the intended directory scope
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
frontend/src/components/workflow/api/ApiHandlers.ts (1)
119-119: Inconsistent field name: should betoolExecIds(plural).The field is named
toolExecIdbut is used as an array throughout the codebase. For consistency with similar fields (prevStepIds,nextStepIds,toolIds), this should betoolExecIds.Apply this diff:
- toolExecId: string[] + toolExecIds: string[]Note: This change will require updating all usages of this field (e.g., in WorkflowBuilder.tsx line 2337 where
step.toolExecIdsis referenced).frontend/src/components/workflow/WorkflowBuilder.tsx (1)
3355-3525: Extract duplicated review path determination logic.The logic to determine which review path to use (approved/rejected) is duplicated across three handlers:
handleAIAgentConfigSave,handleEmailConfigSave, and the scriptonStepCreatedcallback. This violates DRY principles and makes maintenance harder.Extract into a shared helper function:
type ReviewPathResult = { sourceHandle: string edgeLabel?: string reviewPath: "approved" | "rejected" | null } const determineReviewPath = ( sourceNode: Node | undefined, selectedNodeForNext: string ): ReviewPathResult => { let sourceHandle = "bottom" let edgeLabel = undefined let reviewPath: "approved" | "rejected" | null = null if (sourceNode?.data?.tools && Array.isArray(sourceNode.data.tools)) { const reviewTool = sourceNode.data.tools.find((tool: any) => tool.type === "review") if (reviewTool) { const config = reviewTool.config || {} console.log("Found review step with config:", config) // Use approved path if it's not used, otherwise use rejected path if (!config.approved) { sourceHandle = "approved" edgeLabel = "Approved" reviewPath = "approved" } else if (!config.rejected) { sourceHandle = "rejected" edgeLabel = "Rejected" reviewPath = "rejected" } else { console.warn("Both approved and rejected paths are already used for review step:", selectedNodeForNext) } } } return { sourceHandle, edgeLabel, reviewPath } } // Usage example: const { sourceHandle, edgeLabel, reviewPath } = determineReviewPath( sourceNode, selectedNodeForNext )This reduces duplication and ensures consistent behavior across all step creation handlers.
Also applies to: 3612-3776, 4636-4783
frontend/src/components/workflow/executedWorkflowRenderer.tsx (2)
2025-2031: PropshouldStartPollingis defined but not used.Parent passes this to control polling; destructure and use it.
-const WorkflowBuilderInternal: React.FC<WorkflowBuilderProps> = ({ - onStepClick, - onBackToWorkflows, - selectedTemplate, - isLoadingTemplate, - onTemplateUpdate, -}) => { +const WorkflowBuilderInternal: React.FC<WorkflowBuilderProps> = ({ + onStepClick, + onBackToWorkflows, + selectedTemplate, + isLoadingTemplate, + onTemplateUpdate, + shouldStartPolling, +}) => {
285-289: Failure status detection checks the wrong collection.These branches check
tools(config) forstatus. UsetoolExecutionsto reflect execution results.- const hasFailedToolExecution = - tools && tools.some((tool) => (tool as any).status === "failed") + const hasFailedToolExecution = + Array.isArray(toolExecutions) && toolExecutions.some((te: any) => te.status === "failed")Apply the same change in Email and Form branches.
Also applies to: 487-491, 671-675
frontend/src/components/workflow/WhatHappensNextUI.tsx (1)
136-141: run_script action is not handled on click.You added the action but don’t invoke
onSelectActionfor it, so users can’t select it.- if (action.id === "ai_agent" || action.id === "email" || action.id === "review" || action.id === "trigger") { + if (["ai_agent", "email", "review", "trigger", "run_script"].includes(action.id)) { // For AI Agent and Email, trigger custom event to open respective ConfigUI onSelectAction(action.id) onClose() // Close WhatHappensNextUI }
♻️ Duplicate comments (22)
server/api/chat/agents.ts (1)
3579-3579: Unused variable:folderIdsis still not used.This issue was previously flagged. The
folderIdsvariable remains declared but never referenced in the code. Please address the original review comment by either removing this line or implementing the intended folder-based context handling.server/workflowScriptExecutorTool.ts (3)
30-34: Reduce the hardcoded 40‑minute timeout and make it configurable.
timeout_valueis fixed at 40 (⇒ 40 minutes), yet the comment still says “1 minute.” That’s an excessive default and gives untrusted scripts far too much runway. Please drop the default to something sane (e.g., 5–10 minutes), move it behind aSCRIPT_TIMEOUT_MINUTESenv var, and clamp parsed values to an allowed range before using them.
Tags:-const timeout_value = 40 +const DEFAULT_TIMEOUT_MINUTES = 5 +const timeoutEnv = process.env.SCRIPT_TIMEOUT_MINUTES +const parsedTimeout = Number.parseInt(timeoutEnv ?? "", 10) +const timeoutMinutes = + Number.isFinite(parsedTimeout) && parsedTimeout >= 1 && parsedTimeout <= 30 + ? parsedTimeout + : DEFAULT_TIMEOUT_MINUTES
32-34: Fix the sandbox toggle –|| trueforces bubblewrap on regardless of env.
process.env.USE_BUBBLEWRAP === 'true' || truealways evaluates totrue, so the environment flag is meaningless. Please remove the|| trueand pick an explicit default policy (e.g., default to true but allowfalse, or mandate production).
Tags:-const USE_BUBBLEWRAP = process.env.USE_BUBBLEWRAP === 'true' || true +const USE_BUBBLEWRAP = + process.env.NODE_ENV === 'production' + ? true + : process.env.USE_BUBBLEWRAP !== 'false'
577-632: ValidateexportDir/importDirbefore binding into bubblewrap.
createBubblewrapArgsaccepts arbitrary paths and binds them into the sandbox without any normalization or whitelisting, so a craftedworkflowId(or other caller input) can escape the intended workspace. Please resolve the paths relative to a known base (e.g.,<cwd>/script_executor_utils) and reject any path that traverses outside.
Tags:function createBubblewrapArgs(scriptPath: string, exportDir?: string,importDir?:string): string[] { const path = require('path') const fs = require('fs') + + const baseDir = path.resolve(process.cwd(), 'script_executor_utils') + const validatePath = (candidate: string | undefined, purpose: string) => { + if (!candidate) return undefined + const resolved = path.resolve(candidate) + if (!resolved.startsWith(baseDir)) { + throw new Error(`Invalid ${purpose} path: ${candidate}`) + } + return resolved + } + + exportDir = validatePath(exportDir, 'export') + importDir = validatePath(importDir, 'import') const baseArgs = [server/api/workflow.ts (1)
4259-4280: Re-enable the ownership check before serving artifacts.The access-control block is still commented out, so any authenticated user can pull files from any workflow execution. That’s a critical data leak. Restore the
workflow.createdBy === subcheck (or implement the appropriate ACL) before shipping.
Tags:- // TODO: Re-enable access control in production - // Temporarily allow any user to access files for development/testing - // if (workflow.createdBy !== sub) { - // Logger.warn(`Access denied: user ${sub} tried to access workflow execution ${executionId} created by ${workflow.createdBy}`) - // throw new HTTPException(403, { - // message: "Access denied to this workflow execution", - // }) - // } + if (workflow.createdBy !== sub) { + Logger.warn( + `Access denied: user ${sub} tried to access workflow execution ${executionId} created by ${workflow.createdBy}`, + ) + throw new HTTPException(403, { + message: "Access denied to this workflow execution", + }) + }requirements.txt (1)
1-1: Bumprequeststo the patched release.2.31.0 predates the .netrc credential leak (fixed in 2.32.4) and the CA verification bypass (fixed in 2.32.0). Please upgrade to ≥ 2.32.5 (latest stable) and re-lock any dependency artifacts.
Tags:-requests==2.31.0 +requests==2.32.3frontend/src/components/workflow/AIAgentConfigUI.tsx (1)
109-111: Past review comment remains unaddressed.The hardcoded filter for only 'gemini' models (line 110) was previously flagged as potentially limiting flexibility. This restriction is still present in the code. Consider addressing the previous feedback about making this filter configurable or documenting why it's necessary.
frontend/src/components/workflow/api/ApiHandlers.ts (1)
210-223: Remove commented code.This commented-out function was already flagged for removal in a previous review. Commented code reduces readability and should be removed, relying on version control for history.
frontend/src/components/workflow/WorkflowBuilder.tsx (3)
1017-1025: Fix PlusButton nodeId for trigger nodes (currently undefined).This issue was already flagged in a previous review. Using
data.idyields "undefined" for trigger nodes. Useidfrom NodeProps instead.
2240-2312: DFS layout: execution steps not mapped to template IDs.This issue was already flagged in a previous review. When rendering executions (
stepExecutions),performSimpleDFSusesrootWorkflowStepTemplateIddirectly without mapping to execution IDs, which breaks layout positioning.
2896-2913: Respect user-selected review path when adding next steps.This issue was already flagged in a previous review. The
selectedReviewPathis captured from the UI event but creation handlers (AI Agent, Email, Script) ignore it and auto-pick the first free path, which can connect to the wrong branch.Also applies to: 3360-3380, 3612-3637, 4650-4670
Dockerfile (2)
93-94: SUID on bubblewrap is risky; gate and harden.Setting
u+sgrants root on exec. Ensure it’s required and gated by a build arg/env. At minimum, conditionally set and document.- RUN chmod u+s $(which bwrap) + ARG ENABLE_BWRAP_SUID=false + RUN if [ "$ENABLE_BWRAP_SUID" = "true" ]; then chmod u+s "$(command -v bwrap)"; fiAlso consider:
- Restricting execution to a dedicated group (chgrp + chmod 4750).
- Using user namespaces to avoid SUID where possible.
- Adding runtime checks to ensure bwrap drops privileges and uses a minimal whitelist.
70-70: Avoid--break-system-packages; install into a venv.This flag weakens safety checks and can destabilize the image.
-RUN python3 -m pip install --break-system-packages -r /usr/src/app/requirements.txt +RUN python3 -m venv /opt/venv && \ + /opt/venv/bin/pip install --upgrade pip && \ + /opt/venv/bin/pip install -r /usr/src/app/requirements.txt +ENV PATH="/opt/venv/bin:${PATH}"requirements.R (1)
10-13: Duplicate packages in install list (wastes build time).
httr,stringr, andcurlappear twice.- "methods", "datasets", "stringr", "lubridate", "dplyr", - "httr", "RCurl", "curl","digest","tidyverse","glue","bigrquery", - "readr","readxl","googlesheets4","stringr","data.table", + "methods", "datasets", "stringr", "lubridate", "dplyr", + "RCurl","digest","tidyverse","glue","bigrquery", + "readr","readxl","googlesheets4","data.table",frontend/src/components/workflow/executedWorkflowRenderer.tsx (5)
953-954: Missing import for botLogo causes runtime error.
botLogois used but not imported; rendering python_script nodes will crash.Add an asset import near the top:
+import botLogo from "@/assets/bot-logo.svg"
1491-1494: Wrong property used to fetch previous step outputs (breaks input panel).
prevStep.workflow_tool_idsis not part of the step execution shape you defined; usetoolExecIdsand match tool execution IDs.- const prevStepTools = - workflowData.toolExecutions?.filter((toolExec: any) => - prevStep.workflow_tool_ids?.includes(toolExec.workflowToolId), - ) || [] + const prevStepTools = + workflowData.toolExecutions?.filter((toolExec: any) => + prevStep.toolExecIds?.includes(toolExec.id), + ) || []
2043-2043: Type NodeJS.Timeout is incorrect in browsers.Use a portable type for
setTimeoutresults.- const pollingTimeoutRef = useRef<NodeJS.Timeout | null>(null) + const pollingTimeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null)
2058-2063: Mount effect starts polling unconditionally and lacks cleanup.Respect the parent’s control and clear timers on unmount or template change.
- useEffect(() => { - if (selectedTemplate?.id) { - console.log("🚀 Component mounted, fetching initial workflow status for:", selectedTemplate.id) - fetchWorkflowStatus(selectedTemplate.id) - } - }, []) // Empty dependency array means this runs only on mount + useEffect(() => { + if (shouldStartPolling && selectedTemplate?.id) { + if (process.env.NODE_ENV === "development") { + console.log("🚀 Starting polling for:", selectedTemplate.id) + } + fetchWorkflowStatus(selectedTemplate.id) + } + return () => { + if (pollingTimeoutRef.current) { + clearTimeout(pollingTimeoutRef.current) + pollingTimeoutRef.current = null + } + } + }, [shouldStartPolling, selectedTemplate?.id])
2619-2783: Polling lacks abort/backoff/limits; prints raw payloads.Add AbortController, exponential backoff with a cap, a max retry count, and gate detailed logs to dev. Abort on unmount.
If helpful, I can provide a ready-to-paste patch implementing controller refs, retries, backoff, and cleanup.
frontend/src/components/workflow/TriggerExecutionUI.tsx (1)
32-44: useEffect runs every render and logs noisily. Add deps and gate logs.Fix the effect and reduce noise in production.
- React.useEffect(() => { - console.log("🔍 TriggerExecutionUI props changed:", { isVisible, stepExecutionId, stepName, builder, path }) - // Debug logging - console.log("🔍 TriggerExecutionUI render:", { - isVisible, - builder, - stepExecutionId, - triggerStatus - }) - - } - -) + React.useEffect(() => { + if (process.env.NODE_ENV === "development") { + console.log("🔍 TriggerExecutionUI props changed:", { + isVisible, stepExecutionId, stepName, builder, path, triggerStatus, + }) + } + }, [isVisible, stepExecutionId, stepName, builder, path, triggerStatus])frontend/src/components/workflow/ReviewExecutionUI.tsx (2)
50-64: Reset submission state when the panel re-opens.
submissionStatus(and related errors) stick around after an approval/rejection, so reopening the panel hides the CTA buttons. Add an effect that watchesisVisible(andstepExecutionId) to resetsubmissionStatus,submitError, anddownloadErrorwhen the panel is shown.
131-144: Remove the truncated execution ID fallback.Falling back to
stepExecutionId.split("-")[0]produces an invalid UUID, so downloads fail wheneverworkflowExecutionIdis missing. Drop that fallback and bail out with an error if neitherworkflowExecutionIdnorpreviousStepResult?.workflowExecutionIdis present.
🧹 Nitpick comments (11)
start.sh (1)
11-14: Add error handling for optional language support.The preflight checks will cause the startup script to exit if any of python3, node, or Rscript is unavailable (due to
set -euo pipefailon line 2). If these languages are optional dependencies or if the system should continue without certain script execution capabilities, consider:
- Adding informative error messages explaining what's missing
- Using conditional checks that only warn instead of failing
- Or documenting that all three languages are required dependencies
Example with conditional checks:
# Verify script execution languages are available -python3 --version -node --version -Rscript --version +if ! python3 --version 2>/dev/null; then + echo "WARNING: Python 3 not available - script execution will be limited" +fi +if ! node --version 2>/dev/null; then + echo "WARNING: Node.js not available - script execution will be limited" +fi +if ! Rscript --version 2>/dev/null; then + echo "WARNING: R not available - script execution will be limited" +fiserver/.env.default (1)
24-25: Add documentation for the Bubblewrap configuration.The new
USE_BUBBLEWRAPsetting lacks context for users who may not be familiar with Bubblewrap. Consider adding a comment explaining what Bubblewrap provides (script execution sandboxing) and when it should be enabled.#BUBBLEWRAP SUPPORT +# Enable Bubblewrap sandboxing for script execution (Python/Node/R) +# Provides additional security isolation for user scripts +# Set to true in production environments for enhanced security USE_BUBBLEWRAP=falseserver/db/schema/workflows.ts (1)
395-397: Consider future extensibility for the review schema.The schema is minimal and functional for basic approve/reject flows. As the review feature evolves, you may want to add:
- Optional comment/feedback field for reviewers
- Reviewer identification
- Timestamp information
- Review metadata
For now, the simple enum is appropriate.
server/api/script.ts (1)
10-33: Consider caching the language list response.The endpoint generates the same static response on every request (available languages and their code blocks don't change at runtime). This is inefficient, especially if
getCodeWritingBlockis computationally expensive.Consider:
- Computing this data once at server startup and caching it
- Or adding response caching middleware
Example with module-level cache:
// Compute once at module load const cachedLanguages = (() => { const availableLanguages = Object.values(ScriptLanguage) return availableLanguages.map(language => ({ language, codeWritingBlock: getCodeWritingBlock(language, {}, {}) })) })() export const GetScriptLanguagesApi = async (c: Context) => { try { return c.json({ availableLanguages: cachedLanguages, totalCount: cachedLanguages.length }) } catch (error) { loggerWithChild().error(error, "Failed to get script languages") throw new HTTPException(500, { message: getErrorMessage(error), }) } }frontend/src/components/workflow/WorkflowBuilder.tsx (3)
2499-2549: Remove large block of commented code.This commented-out edge-fixing logic should be removed. It appears to be an alternative implementation that was left behind. Version control preserves the history if needed later.
Apply this diff:
- - // // Fix existing edges for review steps - update sourceHandle based on node configuration - // setTimeout(() => { - // setEdges((prevEdges) => - // prevEdges.map((edge) => { - // const sourceNode = templateNodes.find(n => n.id === edge.source) - // if (sourceNode && sourceNode.data && sourceNode.data.step && sourceNode.data.step.type === "review") { - // // Check if target is connected via approved or rejected path - // if (sourceNode.data.approved === edge.target) { - // return { - // ...edge, - // sourceHandle: "approved", - // label: "Approved", - // labelStyle: { - // fill: '#6B7280', - // fontWeight: 600, - // fontSize: '12px', - // fontFamily: 'Inter, system-ui, sans-serif' - // }, - // labelBgStyle: { - // fill: '#FFFFFF', - // fillOpacity: 0.9, - // stroke: '#E5E7EB', - // strokeWidth: 1, - // rx: 4 - // } - // } - // } else if (sourceNode.data.rejected === edge.target) { - // return { - // ...edge, - // sourceHandle: "rejected", - // label: "Rejected", - // labelStyle: { - // fill: '#6B7280', - // fontWeight: 600, - // fontSize: '12px', - // fontFamily: 'Inter, system-ui, sans-serif' - // }, - // labelBgStyle: { - // fill: '#FFFFFF', - // fillOpacity: 0.9, - // stroke: '#E5E7EB', - // strokeWidth: 1, - // rx: 4 - // } - // } - // } - // } - // return edge - // }) - // ) - // }, 100)
3046-3123: Extract form requirement detection and execution logic into helper functions.The nested logic to determine if a form is required (lines 3046-3068) and the direct execution block (lines 3074-3123) are deeply nested and complex, reducing readability.
Extract into helper functions:
// Helper to determine if root step requires a form const requiresFormSubmission = (template: WorkflowTemplate): boolean => { const rootStep = template.steps?.find(step => step.id === template.rootWorkflowStepTemplateId ) if (!rootStep || rootStep.type !== "manual") { return false } if (rootStep.toolIds && rootStep.toolIds.length > 0) { const rootStepTools = template.workflow_tools?.filter(tool => rootStep.toolIds?.includes(tool.id) ) return rootStepTools?.some(tool => tool.type === "form") || false } return false } // Helper to execute workflow directly const executeWorkflowDirectly = async (template: WorkflowTemplate) => { const formData: Record<string, any> = { name: `${template?.name || "Workflow"} - ${new Date().toLocaleString()}`, description: `Execution of ${template?.name || "workflow"}`, } const executionData = { name: formData.name, description: formData.description, formData: formData, } const response = await workflowExecutionsAPI.executeTemplate( template.id, executionData, ) // Handle response... return response } // Then use in main logic: if (requiresFormSubmission(currentTemplate)) { setShowExecutionModal(true) } else { try { await executeWorkflowDirectly(currentTemplate) // Handle success... } catch (error) { // Handle error... } }This improves testability and readability of the main execution flow.
4177-4181: Potential data mismatch: manual object construction vs. API response.The code manually constructs
newCreatedTemplateusing spread syntax fromcreateResult.data, but then addsid: createdTemplateIdseparately. SincecreateResult.data.idshould already exist, this could lead to confusion or potential bugs if the API response structure changes.Use the API response data directly without manual reconstruction:
- // Use the full response data from the API instead of manually creating the object const newCreatedTemplate = { - ...createResult.data, - id: createdTemplateId, - } as WorkflowTemplate + ...createResult.data + } as WorkflowTemplate + // Verify the ID is present in the response + if (!newCreatedTemplate.id) { + throw new Error("Created template is missing ID in API response") + }This ensures we use the API's data structure exactly as provided, reducing the risk of data inconsistencies.
frontend/src/components/workflow/executedWorkflowRenderer.tsx (2)
2050-2056: Gate verbose console logs behind dev flag.Avoid logging execution payloads in production.
Wrap with:
if (process.env.NODE_ENV === "development") { console.log(...) }Also applies to: 2079-2091, 2408-2411, 2622-2642, 2671-2677, 2767-2771
35-36: Unify type source: import WorkflowTemplate from Types.ts.This file already imports types from
./Types. ImportWorkflowTemplatefrom the same module to avoid duplication with the route file.-// Import WorkflowTemplate from workflow.tsx -import type { WorkflowTemplate } from "../../routes/_authenticated/workflow" +import type { WorkflowTemplate } from "./Types"frontend/src/components/workflow/PlusButton.tsx (1)
50-69: Minor a11y: add aria-label to the plus control.Improves screen reader usability without changing visuals.
- <div + <div className="bg-black hover:bg-gray-800 rounded-full flex items-center justify-center transition-colors shadow-lg" style={{ width: "28px", height: "28px", }} + aria-label="Add next step" + role="button" >frontend/src/routes/_authenticated/workflow.tsx (1)
17-90: DuplicateWorkflowTemplateinterface; prefer single source of truth.This interface exists in
frontend/src/components/workflow/Types.ts. Duplicating invites drift and type mismatches.Replace this block with:
- export interface WorkflowTemplate { ... } + import type { WorkflowTemplate } from "@/components/workflow/Types"If other modules import it from this route, re-export:
export type { WorkflowTemplate }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
frontend/bun.lockbis excluded by!**/bun.lockbserver/bun.lockbis excluded by!**/bun.lockb
📒 Files selected for processing (28)
Dockerfile(2 hunks)deployment/portable/.env.default(1 hunks)deployment/portable/docker-compose.app-build.yml(2 hunks)frontend/package.json(2 hunks)frontend/src/components/CodeEditor.tsx(1 hunks)frontend/src/components/ScriptSidebar.tsx(1 hunks)frontend/src/components/workflow/AIAgentConfigUI.tsx(1 hunks)frontend/src/components/workflow/EmailConfigUI.tsx(1 hunks)frontend/src/components/workflow/PlusButton.tsx(1 hunks)frontend/src/components/workflow/ReviewExecutionUI.tsx(1 hunks)frontend/src/components/workflow/TriggerExecutionUI.tsx(1 hunks)frontend/src/components/workflow/WhatHappensNextUI.tsx(3 hunks)frontend/src/components/workflow/WorkflowBuilder.tsx(40 hunks)frontend/src/components/workflow/WorkflowIcons.tsx(1 hunks)frontend/src/components/workflow/api/ApiHandlers.ts(2 hunks)frontend/src/components/workflow/executedWorkflowRenderer.tsx(35 hunks)frontend/src/routes/_authenticated/workflow.tsx(12 hunks)requirements.R(1 hunks)requirements.txt(1 hunks)server/.env.default(1 hunks)server/api/chat/agents.ts(2 hunks)server/api/script.ts(1 hunks)server/api/workflow.ts(32 hunks)server/db/schema/workflows.ts(2 hunks)server/server.ts(3 hunks)server/types/workflowTypes.ts(1 hunks)server/workflowScriptExecutorTool.ts(1 hunks)start.sh(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (12)
server/workflowScriptExecutorTool.ts (1)
server/logger/index.ts (3)
getLoggerWithChild(192-200)Subsystem(15-15)getLogger(36-93)
server/api/workflow.ts (3)
server/db/schema/workflows.ts (6)
workflowStepExecution(143-169)workflowTool(104-117)workflowStepTemplate(79-101)workflowExecution(120-140)toolExecution(172-189)workflowTemplate(60-76)server/services/emailService.ts (1)
emailService(124-124)server/workflowScriptExecutorTool.ts (1)
executeScript(638-859)
server/server.ts (3)
server/api/workflow.ts (4)
reviewSubmissionSchema(66-66)ReviewWorkflowStepApi(3732-3942)TriggerWorkflowStepApi(3944-4101)DownloadReviewFilesApi(4233-4442)server/db/schema/workflows.ts (1)
reviewSubmissionSchema(395-397)server/api/script.ts (1)
GetScriptLanguagesApi(10-33)
frontend/src/components/workflow/ReviewExecutionUI.tsx (2)
frontend/src/api.ts (1)
api(5-5)frontend/src/components/workflow/api/ApiHandlers.ts (1)
workflowToolsAPI(376-407)
frontend/src/components/workflow/executedWorkflowRenderer.tsx (3)
frontend/src/routes/_authenticated/workflow.tsx (1)
WorkflowTemplate(17-89)frontend/src/components/workflow/Types.ts (2)
Step(64-92)Tool(56-61)frontend/src/api.ts (1)
api(5-5)
server/api/script.ts (2)
server/logger/index.ts (2)
getLoggerWithChild(192-200)Subsystem(15-15)server/workflowScriptExecutorTool.ts (2)
ScriptLanguage(861-861)getCodeWritingBlock(863-873)
frontend/src/components/workflow/WorkflowBuilder.tsx (5)
frontend/src/components/workflow/PlusButton.tsx (1)
PlusButton(10-73)frontend/src/components/workflow/api/ApiHandlers.ts (1)
workflowExecutionsAPI(240-373)frontend/src/routes/_authenticated/workflow.tsx (1)
WorkflowTemplate(17-89)frontend/src/components/workflow/Types.ts (2)
WorkflowTemplate(227-276)Tool(56-61)frontend/src/components/ScriptSidebar.tsx (1)
ScriptSidebar(29-514)
frontend/src/components/workflow/api/ApiHandlers.ts (1)
frontend/src/api.ts (1)
api(5-5)
frontend/src/routes/_authenticated/workflow.tsx (3)
frontend/src/components/workflow/Types.ts (1)
WorkflowTemplate(227-276)frontend/src/components/workflow/api/ApiHandlers.ts (2)
fetchWorkflows(193-197)userWorkflowsAPI(189-224)server/db/schema/workflows.ts (1)
workflowTemplate(60-76)
frontend/src/components/workflow/TriggerExecutionUI.tsx (1)
frontend/src/api.ts (1)
api(5-5)
frontend/src/components/ScriptSidebar.tsx (3)
frontend/src/api.ts (1)
api(5-5)frontend/src/components/workflow/api/ApiHandlers.ts (2)
workflowToolsAPI(376-407)workflowStepsAPI(410-469)frontend/src/components/CodeEditor.tsx (1)
CodeEditor(28-177)
server/db/schema/workflows.ts (1)
server/api/workflow.ts (1)
reviewSubmissionSchema(66-66)
🪛 dotenv-linter (3.3.0)
server/.env.default
[warning] 29-29: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 30-30: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 31-31: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 31-31: [UnorderedKey] The DB_PASSWORD key should go before the DB_USER key
(UnorderedKey)
[warning] 32-32: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 32-32: [UnorderedKey] The DB_HOST key should go before the DB_NAME key
(UnorderedKey)
[warning] 33-33: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 33-33: [UnorderedKey] The DB_PORT key should go before the DB_USER key
(UnorderedKey)
🔇 Additional comments (7)
frontend/src/components/workflow/WorkflowIcons.tsx (1)
68-85: LGTM!The ReviewIcon component follows the established pattern for icon components in this file. The implementation is consistent, uses the standard IconProps interface, and properly supports theming via
currentColor.server/types/workflowTypes.ts (1)
23-25: LGTM!The new ToolType enum members (REVIEW, TRIGGER, SCRIPT) are consistent with the existing naming convention and align with the PR's multi-language script execution and review workflow features.
frontend/src/components/workflow/AIAgentConfigUI.tsx (1)
341-361: LGTM - Header styling refactor looks good.The migration from inline styles to Tailwind utility classes improves maintainability and consistency with the rest of the codebase.
frontend/src/components/workflow/EmailConfigUI.tsx (1)
201-221: LGTM - Header styling refactor looks good.The migration from inline styles to Tailwind utility classes improves maintainability and consistency with the codebase. The use of
onClose || onBackon line 216 properly supports both closing behaviors.frontend/src/components/workflow/WorkflowBuilder.tsx (2)
4213-4222: Save button logic correctly excludes placeholder trigger selector.The logic to determine if the save button should be enabled properly checks for actual workflow steps by excluding the
trigger_selectorplaceholder. This ensures users can't save an empty workflow with just the placeholder node.The implementation correctly:
- Checks for the presence of actual step types (not just trigger_selector)
- Enables the save button only when changes have been made and actual steps exist
- Provides clear user feedback through the disabled state
2573-2591: hasActualSteps already counts manual review steps
The existingnodes.somecheck excludes onlytrigger_selector, so any step withtype: "manual"(including review steps) is already counted—no change required.Likely an incorrect or invalid review comment.
frontend/src/routes/_authenticated/workflow.tsx (1)
942-944: Polling control wired but defaulted off; confirm intent.You pass
shouldStartPollingtoExecutedWorkflowRendereras false when viewing executions and reset it on back. That’s fine if polling is triggered explicitly elsewhere (e.g., after manual actions). Ensure the child now respects this prop after applying fixes in executedWorkflowRenderer.Also applies to: 961-982
YashJuspay626
force-pushed
the
files-extract2
branch
from
6e23368 to
6eb6705
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/workflow/WorkflowBuilder.tsx (1)
1031-1199: Replace manual plus button with PlusButton component.The script node uses the old manual plus button implementation (lines 1161-1195) instead of the PlusButton component used by AI Agent, Email, and Form nodes. This creates inconsistency and duplicates code.
Replace lines 1138-1195 with:
- {/* ReactFlow Handles - invisible but functional */} - <Handle - type="target" - position={Position.Top} - id="top" - isConnectable={isConnectable} - className="opacity-0" - /> - - <Handle - type="source" - position={Position.Bottom} - id="bottom" - isConnectable={isConnectable} - className="opacity-0" - /> - - {/* Bottom center connection point - visual only */} - <div className="absolute -bottom-1.5 left-1/2 transform -translate-x-1/2"> - <div className="w-3 h-3 bg-gray-400 dark:bg-gray-500 rounded-full border-2 border-white dark:border-gray-900 shadow-sm"></div> - </div> - - {/* Add Next Step Button */} - {hasNext && ( - <div - className="absolute left-1/2 transform -translate-x-1/2 flex flex-col items-center cursor-pointer z-50 pointer-events-auto" - style={{ top: "calc(100% + 8px)" }} - onClick={(e) => { - e.stopPropagation() - e.preventDefault() - console.log("Plus button clicked for node:", id) - const event = new CustomEvent("openWhatHappensNext", { - detail: { nodeId: id }, - }) - window.dispatchEvent(event) - }} - > - <div className="w-0.5 h-6 bg-gray-300 dark:bg-gray-600 mb-2"></div> - <div - className="bg-black hover:bg-gray-800 rounded-full flex items-center justify-center transition-colors shadow-lg" - style={{ - width: "28px", - height: "28px", - }} - > - <svg - className="w-4 h-4 text-white" - viewBox="0 0 24 24" - fill="none" - stroke="currentColor" - strokeWidth="2" - > - <line x1="12" y1="5" x2="12" y2="19"></line> - <line x1="5" y1="12" x2="19" y2="12"></line> - </svg> - </div> - </div> - )} + <PlusButton hasNext={hasNext} nodeId={id} isConnectable={isConnectable} showHandles={true} />
♻️ Duplicate comments (18)
frontend/src/components/workflow/WorkflowBuilder.tsx (5)
1017-1025: Fix PlusButton nodeId for trigger nodes (currently undefined).Using
data.idyields "undefined" for trigger nodes. Useidfrom NodeProps and thehasNextvariable already in scope.Apply this diff:
{!isAwaitingTrigger && ( <PlusButton - hasNext={Boolean(data.hasNext)} - nodeId={String(data.id)} + hasNext={Boolean(hasNext)} + nodeId={id} isConnectable={isConnectable} showHandles={false} /> )}
1168-1168: Remove debug console.log statement.Debug logging should be removed from production code.
Apply this diff:
- console.log("Plus button clicked for node:", id)
2240-2312: DFS layout: execution steps not mapped to template IDs; root and children resolution incorrect.When rendering executions (stepExecutions),
performSimpleDFSusesrootWorkflowStepTemplateIddirectly andcurrentNode.nextStepIdswithout mapping to execution IDs. This positions only the template root and leaves executions at fallback (400,100), breaking layout.The fix requires:
- Building a map from
workflowStepTemplateIdto execution step IDs- Resolving the root execution node using this map
- Translating child template IDs in
nextStepIdsto execution IDs before recursingSee the detailed fix in the previous review comment.
2997-3006: Remove debug console.log statements.Multiple debug logging statements should be removed from production code.
Remove or replace with structured logging at lines 2997-3007, 3037-3042, 3064-3065, 3092, 3101-3107, and 3343-3347.
3360-3380: Respect user-selected review path (approved/rejected) when adding next steps.
selectedReviewPathis captured from the UI event but not used when determining which path to connect. The code auto-picks the first free path, which can connect to the wrong branch.Use
selectedReviewPathpreferentially before falling back to first free path:// Use approved path if it's not used, otherwise use rejected path - if (!config.approved) { + if (selectedReviewPath === "approved" && !config.approved) { sourceHandle = "approved" edgeLabel = "Approved" reviewPath = "approved" - } else if (!config.rejected) { + } else if (selectedReviewPath === "rejected" && !config.rejected) { sourceHandle = "rejected" edgeLabel = "Rejected" reviewPath = "rejected" + } else if (!config.approved) { + sourceHandle = "approved" + edgeLabel = "Approved" + reviewPath = "approved" + } else if (!config.rejected) { + sourceHandle = "rejected" + edgeLabel = "Rejected" + reviewPath = "rejected" } else {Apply the same pattern at lines 3612-3637 (handleEmailConfigSave) and lines 4650-4670 (ScriptSidebar onStepCreated callback).
Also applies to: 3612-3637, 4650-4670, 4731-4749
frontend/src/components/workflow/executedWorkflowRenderer.tsx (5)
2043-2043: Browser-incompatible timeout type persists.The
pollingTimeoutRefis still typed asNodeJS.Timeout | null, which is incorrect for browser contexts wheresetTimeoutreturns anumber. This was previously flagged.Apply the previously suggested fix:
- const pollingTimeoutRef = useRef<NodeJS.Timeout | null>(null) + const pollingTimeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null)
2058-2063: Missing cleanup for polling timeout on unmount.The
useEffectthat starts polling on mount has no cleanup function. When the component unmounts orselectedTemplate?.idchanges, the timeout continues running, leading to memory leaks and potential stale updates. This was previously flagged.Add cleanup as previously suggested:
useEffect(() => { if (selectedTemplate?.id) { console.log("🚀 Component mounted, fetching initial workflow status for:", selectedTemplate.id) fetchWorkflowStatus(selectedTemplate.id) } + return () => { + if (pollingTimeoutRef.current) { + clearTimeout(pollingTimeoutRef.current) + pollingTimeoutRef.current = null + } + } - }, []) + }, [selectedTemplate?.id])
2620-2782: Polling lacks request cancellation, backoff, and retry limits.The polling implementation still has the issues flagged in the previous review:
- No
AbortControllerto cancel in-flight requests on unmount- No exponential backoff on errors (retries every 5s indefinitely)
- No maximum retry limit (could loop forever)
- Logs sensitive execution data in production
Refer to the comprehensive fix in the previous review comment which addresses all these concerns by adding:
abortControllerRefto cancel pending requests- Exponential backoff with a cap
MAX_RETRIESlimit- Conditional logging based on
NODE_ENV
1493-1493: Property name mismatch persists.Line 1493 uses
prevStep.workflow_tool_ids?.includes(toolExec.workflowToolId), but theExecutionWorkflowTemplateinterface definestoolExecIdson step executions, notworkflow_tool_ids. This was previously flagged.Apply the previously suggested fix to use the correct property name:
- prevStep.workflow_tool_ids?.includes(toolExec.workflowToolId), + prevStep.toolExecIds?.includes(toolExec.id),
2198-2206: Inconsistent property naming across execution code.The code uses
workflow_tool_idsthroughout the execution rendering logic (lines 2203-2205, 2297-2302, 2478-2482, 2550-2552, 2731-2737), but theExecutionWorkflowTemplateinterface definestoolExecIdson step executions. This naming inconsistency was previously flagged and needs to be resolved across the entire file.Follow the previous recommendation to normalize property naming. Either:
- Update the API response mapping to use consistent names (
workflowToolIds,toolExecIds), or- Update the interface to match the actual API shape
Then update all
.filtercalls to use the unified properties consistently.Also applies to: 2296-2313, 2476-2491, 2550-2553, 2731-2737
server/workflowScriptExecutorTool.ts (6)
30-33: Critical configuration issues remain unaddressed.Both of these configuration problems were flagged in previous reviews but are still present:
Line 30: The 40-minute timeout is dangerously long and could allow malicious scripts to consume resources. This should be reduced to 5-10 minutes and made configurable.
Line 33: The
|| trueforcesUSE_BUBBLEWRAPto always be true, making the environment variable meaningless. This prevents disabling the sandbox in development or overriding the default.Apply this diff to fix both issues:
-const timeout_value = 40 +const DEFAULT_TIMEOUT_MINUTES = 5 +const timeout_value = parseInt(process.env.SCRIPT_TIMEOUT_MINUTES || String(DEFAULT_TIMEOUT_MINUTES), 10) // Bubblewrap sandbox toggle from environment -const USE_BUBBLEWRAP = process.env.USE_BUBBLEWRAP === 'true' || true +// Mandatory in production for security +const USE_BUBBLEWRAP = process.env.NODE_ENV === 'production' || process.env.USE_BUBBLEWRAP === 'true'
90-227: Network and filesystem access in allowlists creates security risks.Previous reviews flagged that allowing network libraries (
requests,urllib,fetch,axios,httr) and filesystem access (fs) enables:
- Data exfiltration to external servers
- SSRF attacks against internal services
- Unauthorized file system access (JavaScript
fsmodule)If scripts originate from untrusted sources, remove these libraries from
allowedImportsor run scripts with--unshare-netin bubblewrap to block network access entirely.
229-343: Regex-based security validation is insufficient.Previous reviews noted that keyword and import validation using regex can be bypassed:
- Python:
__import__viagetattr(__builtins__, '__import__')orexec()- JavaScript:
requireviaglobal.requireor computed property access- String concatenation:
eval('imp'+'ort os')bypasses keyword checksSwitch to AST-based import whitelisting that parses the script and only allows Import/Require/Call nodes referencing modules in
allowedImports. Reject dynamic or computed imports.
346-409: Missing input size validation.Previous reviews noted that
generateScriptaccepts arbitrary-sizedscript,input, andconfigwith no size limits, which could cause memory exhaustion or DOS.Add size validation:
async function generateScript(script: string, language: ScriptLanguage, input: Record<string, any>, config?: Record<string, any>): Promise<{ success: boolean; filePath?: string; error?: string }> { const fs = require('fs').promises const path = require('path') const { randomUUID } = require('crypto') + + const MAX_SCRIPT_SIZE = 100000 // 100KB + const MAX_INPUT_SIZE = 1000000 // 1MB JSON + + if (script.length > MAX_SCRIPT_SIZE) { + return { success: false, error: `Script exceeds ${MAX_SCRIPT_SIZE} bytes` } + } + + const inputJson = JSON.stringify(input) + const configJson = JSON.stringify(config || {}) + + if (inputJson.length > MAX_INPUT_SIZE || configJson.length > MAX_INPUT_SIZE) { + return { success: false, error: `Input/config data exceeds ${MAX_INPUT_SIZE} bytes` } + } // Use empty objects as defaults and validate if input is provided
577-640: Path traversal vulnerability and insufficient sandbox hardening.Two critical issues from previous reviews remain unaddressed:
Path traversal:
exportDirandimportDirare not validated before mounting into the sandbox. Malicious paths could access unintended locations.Weak sandbox: Missing security hardening such as seccomp profiles,
--no-new-privs, capability dropping, and environment sanitization.Apply this diff to add path validation:
function createBubblewrapArgs(scriptPath: string, exportDir?: string, importDir?: string): string[] { const path = require('path') const fs = require('fs') + + // Validate paths to prevent traversal + const safePath = (p: string | undefined, purpose: string): string | undefined => { + if (!p) return undefined + const resolved = path.resolve(p) + const base = path.resolve(process.cwd(), 'script_executor_utils') + if (!resolved.startsWith(base)) { + throw new Error(`Invalid ${purpose} path: must be within script_executor_utils`) + } + return resolved + } + + exportDir = safePath(exportDir, 'export') + importDir = safePath(importDir, 'import') const baseArgs = [For sandbox hardening, add to
baseArgs:'--seccomp', '9', // Use restrictive seccomp profile '--no-new-privs', // Prevent privilege escalation '--cap-drop', 'ALL', // Drop all capabilities
642-863: Incomplete resource cleanup and missing workflowId validation.Previous reviews flagged:
Cleanup: Script file and export directory are not always cleaned up (missing
finallyblock). Lines 771-776 delete the script only in the main flow, but early returns (lines 664-669, 690-696) skip cleanup.workflowId validation: Line 679 uses
workflowIdin a filesystem path without validation. It could contain..or other traversal characters.Add workflowId validation at the start:
export async function executeScript( executionInput: ScriptExecutionInput, workflowId?: string ): Promise<ScriptExecutionOutput> { const { spawn } = require('child_process') const fs = require('fs').promises const fsSync = require('fs') const path = require('path') let scriptPath = '' let exportDir = '' let importDir = '' + + // Validate workflowId to prevent path traversal + if (workflowId && !/^[a-zA-Z0-9_-]+$/.test(workflowId)) { + return { + success: false, + output: {}, + error: 'Invalid workflow ID format', + exitCode: -1 + } + }Wrap execution in
try-finallyto ensure cleanup:try { // ... script generation and execution ... } catch (error) { // ... error handling ... + } finally { + // Always cleanup + if (scriptPath) { + try { + await fs.unlink(scriptPath) + } catch (e) { + Logger.warn(`Failed to delete script: ${scriptPath}`) + } + } + if (exportDir) { + try { + await fs.rm(exportDir, { recursive: true, force: true }) + } catch (e) { + Logger.warn(`Failed to cleanup export dir: ${exportDir}`) + } + } } }server/api/workflow.ts (2)
3896-3919: Wrong argument passed to executeWorkflowChain.This issue was flagged in previous reviews but remains unfixed. Line 3912 passes
tools(the full workflow tool array) as the third argument toexecuteWorkflowChain, but the function signature (line 1209) expectspreviousResults(a result map). Downstream automated steps will receive an array instead of the expected result map and fail to resolve prior outputs.Apply this diff:
executeWorkflowChain( currentStep.workflowExecutionId, nextStep.id, - tools + currentResults ).catch((error) => {
4260-4269: CRITICAL: Access control is disabled.This critical issue was flagged in previous reviews but the access control remains commented out. The TODO comment (line 4262) indicates this is temporary for development/testing, but it allows any authenticated user to download files from any workflow execution, which is a severe security vulnerability.
Remove the TODO and re-enable access control before merging to production:
Logger.info(`Download access check: executionId=${executionId}, user=${sub}, createdBy=${workflow.createdBy}`) - // TODO: Re-enable access control in production - // Temporarily allow any user to access files for development/testing - // if (workflow.createdBy !== sub) { - // Logger.warn(`Access denied: user ${sub} tried to access workflow execution ${executionId} created by ${workflow.createdBy}`) - // throw new HTTPException(403, { - // message: "Access denied to this workflow execution", - // }) - // } + // Verify user owns this workflow execution + if (workflow.createdBy !== sub) { + Logger.warn(`Access denied: user ${sub} tried to access workflow execution ${executionId} created by ${workflow.createdBy}`) + throw new HTTPException(403, { + message: "Access denied to this workflow execution", + }) + }If workspace-level sharing is needed, implement proper authorization checks based on workspace membership instead of disabling the per-owner check.
🧹 Nitpick comments (5)
frontend/src/components/workflow/WhatHappensNextUI.tsx (2)
48-68: Consider using lucide-react icon for consistency.The review action uses an inline SVG (lines 53-66) while other actions use lucide-react icons (Play, Code). For consistency and maintainability, consider importing a suitable icon from lucide-react or creating a reusable ReviewIcon component.
Example using lucide-react:
- icon: ( - <svg className="w-5 h-5" fill="none" stroke="currentColor" viewBox="0 0 24 24"> - <path - strokeLinecap="round" - strokeLinejoin="round" - strokeWidth={2} - d="M15 12a3 3 0 11-6 0 3 3 0 016 0z" - /> - <path - strokeLinecap="round" - strokeLinejoin="round" - strokeWidth={2} - d="M2.458 12C3.732 7.943 7.523 5 12 5c4.478 0 8.268 2.943 9.542 7-1.274 4.057-5.064 7-9.542 7-4.477 0-8.268-2.943-9.542-7z" - /> - </svg> - ), + icon: <Eye className="w-5 h-5" />,Add the import:
import { Bot, Mail, + Eye, Globe,
136-136: Simplify the action ID conditional check.The repetitive
||chain can be simplified using an array and.includes()for better readability and maintainability.Apply this diff:
- if (action.id === "ai_agent" || action.id === "email" || action.id === "review" || action.id === "trigger" || action.id === "run_script") { + const enabledActions = ["ai_agent", "email", "review", "trigger", "run_script"] + if (enabledActions.includes(action.id)) {frontend/src/components/workflow/WorkflowBuilder.tsx (2)
2573-2600: Extract hasActualSteps check to a helper function.The logic to check for actual workflow steps (excluding trigger_selector) is duplicated at lines 2573-2577, 2579-2591, and 4213-4222. Extract to a reusable helper function.
Add this helper function:
const hasActualWorkflowSteps = useCallback(() => { return nodes.some(node => { const nodeData = node.data as any return nodeData?.step?.type && nodeData.step.type !== "trigger_selector" }) }, [nodes])Then use it:
- const hasActualSteps = nodes.some(node => { - const nodeData = node.data as any - return nodeData?.step?.type && nodeData.step.type !== "trigger_selector" - }) + const hasActualSteps = hasActualWorkflowSteps()
4213-4222: Reuse hasActualWorkflowSteps helper here.This is another location with the duplicated hasActualSteps check. After extracting the helper function (see comment on lines 2573-2600), use it here as well.
- isSaveDisabled={(() => { - // Check if we have actual workflow steps (not just trigger selector) - const hasActualSteps = nodes.some(node => { - const nodeData = node.data as any - return nodeData?.step?.type && nodeData.step.type !== "trigger_selector" - }) - - // Enable save button if we have actual steps and changes have been made - return !hasActualSteps || (!hasWorkflowChanged && isWorkflowSaved) - })()} + isSaveDisabled={!hasActualWorkflowSteps() || (!hasWorkflowChanged && isWorkflowSaved)}frontend/src/components/workflow/executedWorkflowRenderer.tsx (1)
2018-2022: UnusedonTemplateUpdateprop.The
onTemplateUpdateprop is defined inWorkflowBuilderProps(lines 2020, 2030) but intentionally not called infetchWorkflowStatus(see comment at line 2643: "Removed onTemplateUpdate call to prevent parent component interference"). This adds API surface that's not used.Consider either:
- Remove the prop if parent component update is intentionally avoided, or
- Document why it exists if it's reserved for future use
selectedTemplate?: ExecutionWorkflowTemplate | null isLoadingTemplate?: boolean - onTemplateUpdate?: (template: ExecutionWorkflowTemplate) => void shouldStartPolling?: booleanAlso applies to: 2030-2030
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
frontend/src/components/workflow/WhatHappensNextUI.tsx(5 hunks)frontend/src/components/workflow/WorkflowBuilder.tsx(40 hunks)frontend/src/components/workflow/executedWorkflowRenderer.tsx(35 hunks)requirements.txt(1 hunks)server/api/workflow.ts(32 hunks)server/workflowScriptExecutorTool.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- requirements.txt
🧰 Additional context used
🧬 Code graph analysis (4)
frontend/src/components/workflow/WorkflowBuilder.tsx (3)
frontend/src/components/workflow/PlusButton.tsx (1)
PlusButton(10-73)frontend/src/routes/_authenticated/workflow.tsx (1)
WorkflowTemplate(17-89)frontend/src/components/ScriptSidebar.tsx (1)
ScriptSidebar(29-514)
server/api/workflow.ts (3)
server/db/schema/workflows.ts (6)
workflowStepExecution(143-169)workflowTool(104-117)workflowStepTemplate(79-101)workflowExecution(120-140)toolExecution(172-189)workflowTemplate(60-76)server/services/emailService.ts (1)
emailService(124-124)server/workflowScriptExecutorTool.ts (2)
ScriptLanguage(865-865)executeScript(642-863)
server/workflowScriptExecutorTool.ts (1)
server/logger/index.ts (3)
getLoggerWithChild(192-200)Subsystem(15-15)getLogger(36-93)
frontend/src/components/workflow/executedWorkflowRenderer.tsx (3)
frontend/src/routes/_authenticated/workflow.tsx (1)
WorkflowTemplate(17-89)frontend/src/components/workflow/Types.ts (2)
Step(64-92)Tool(56-61)frontend/src/api.ts (1)
api(5-5)
🔇 Additional comments (8)
frontend/src/components/workflow/executedWorkflowRenderer.tsx (1)
1681-1690: Approve script tool input handling.The special handling for script nodes at lines 1681-1690 correctly shows the complete input data for script steps, which is appropriate since scripts may need to process the full data structure.
server/api/workflow.ts (7)
1989-2033: LGTM: Improved error handling in extractContentFromPath.The change to return
nullinstead of error strings allows callers to implement fallback logic without treating error messages as valid content. This is appropriate for the use case in the email tool (lines 2187-2205) where multiple fallback paths are tried.
2443-2541: Verify script execution sandbox configuration.The script tool execution logic correctly extracts the latest step's result (lines 2475-2499) and handles success/error cases (lines 2509-2541). However, it relies on
executeScriptfromworkflowScriptExecutorTool.ts, which has multiple unresolved security issues flagged in this review:
- Weak timeout (40 minutes)
- Sandbox toggle always true
- Network/filesystem libraries in allowlists
- Regex-based validation (bypassable)
- Path traversal risks
- Incomplete cleanup
Ensure the sandbox (
USE_BUBBLEWRAP) is enabled in production and properly hardened before deploying this feature. See the review comments onworkflowScriptExecutorTool.tsfor details.
1156-1204: ExportupdateDownstreamStepStatusesfunction.This function is used at line 494 (in
ExecuteWorkflowWithInputApi), line 1403 (inexecuteWorkflowChain), line 3718 (inCompleteWorkflowStepExecutionApi), line 3876 (inReviewWorkflowStepApi), and line 4046 (inTriggerWorkflowStepApi), but it's not exported from the file.Add it to the exports if other modules need to use it, or keep it internal if it's only used within this file. Since it's used extensively within the file, the current implementation is fine, but verify no external usage is expected.
2893-2922: LGTM: Review tool config updates.The third-pass logic (lines 2893-2922) correctly updates review tool configurations with backend step IDs after all steps are created. This ensures that the
approvedandrejectedfields in review tool configs reference the actual database step IDs rather than frontend temporary IDs.The logic properly:
- Checks if the tool is a review type (line 2895)
- Maps frontend step IDs to backend IDs (lines 2900-2909)
- Only updates the database if changes were made (line 2912)
2945-2950: LGTM: Re-querying updated steps.Re-querying the step templates from the database after all updates (lines 2945-2950) ensures that the response includes the correct
toolIdsarrays with the actual database UUIDs. This is good practice and prevents returning stale data with temporary IDs.
3667-3729: LGTM: Previous step validation in CompleteWorkflowStepExecutionApi.The enhancement (lines 3679-3704) correctly validates that all previous steps are completed before allowing a step to be marked as complete. This prevents workflow steps from completing out of order and maintains workflow integrity.
The error message (lines 3699-3702) clearly indicates which steps must be completed first, providing good user feedback.
3945-4102: LGTM: TriggerWorkflowStepApi implementation.The new trigger step API (lines 3945-4102) is well-implemented:
- Validates the step is a trigger type (lines 4001-4005)
- Creates a tool execution record with trigger metadata (lines 4007-4024)
- Marks the step as completed (lines 4026-4043)
- Updates downstream steps (line 4046)
- Continues workflow execution with next automated steps (lines 4048-4078)
The error handling and logging are appropriate.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
server/workflowScriptExecutorTool.ts (6)
30-30: Timeout value remains hardcoded.The 40-minute timeout is still hardcoded. The past review comment suggested making this configurable via environment variable with a safer default (5-10 minutes).
33-33: Sandbox toggle logic is still broken.The
|| truemakesUSE_BUBBLEWRAPalwaystrue, rendering the environment variable meaningless. This was flagged in a previous review but remains unaddressed.
90-227: Network libraries remain in allowedImports.The allowedImports lists still include network libraries (Python:
requests,urllib; JavaScript:fetch,axios,https,http; R:httr,RCurl,curl) that enable SSRF and data exfiltration. This was flagged in previous reviews but remains unaddressed.
229-343: Security validation remains bypassable.The regex-based import and keyword validation can be circumvented through string concatenation, dynamic access patterns, or encoding. Previous reviews recommended AST-based validation, but the implementation still relies on pattern matching.
593-660: Path validation and sandbox hardening still missing.Two issues flagged in previous reviews remain unaddressed:
- No path validation for
exportDirandimportDirparameters (path traversal risk)- Missing sandbox hardening: no seccomp profile,
--no-new-privs, capability dropping, or environment sanitization
680-877: Missing workflowId validation and cleanup structure.Two issues from previous reviews remain unaddressed:
No workflowId validation (lines 717, 783): The
workflowIdparameter is used directly in filesystem paths without validation. A malicious workflowId like../../etccould enable path traversal attacks.Cleanup not guaranteed (lines 804, 854, 868): While
cleanTempFilesis called in multiple locations, it's not in a finally block. If an unexpected error occurs (e.g., in file extraction logic lines 787-797), temporary files may not be cleaned up.Additionally, line 758 logs the full command, which includes filesystem paths. This could leak sensitive information about the server's directory structure in production logs.
Apply this diff to validate workflowId:
export async function executeScript( executionInput: ScriptExecutionInput, workflowId?: string ): Promise<ScriptExecutionOutput> { + // Validate workflowId to prevent path traversal + if (workflowId && !/^[a-zA-Z0-9_-]+$/.test(workflowId)) { + return { + success: false, + output: {}, + error: 'Invalid workflow ID format', + exitCode: -1 + } + } + const { spawn } = require('child_process')For the cleanup structure, consider wrapping the entire execution in try-finally:
let scriptPath = '' let exportDir = '' let importDir = '' try { // ... existing execution logic ... } catch (error) { - cleanTempFiles(scriptPath, exportDir) return { success: false, output: {}, error: error instanceof Error ? error.message : 'Unknown error', exitCode: -1, extractedFiles: [] } + } finally { + cleanTempFiles(scriptPath, exportDir) } }Remove or conditionally guard the logging at line 758:
} else { - Logger.info(`Running without sandbox: ${command} ${args.join(' ')}`) + if (process.env.NODE_ENV !== 'production') { + Logger.info(`Running without sandbox: ${command} ${args.join(' ')}`) + } }
🧹 Nitpick comments (2)
server/workflowScriptExecutorTool.ts (2)
396-405: Script template assumes user returns a value.The template wraps user code in a function and expects
script_result_to be defined (line 402), but if the user's script doesn't explicitly return a value,script_result_will beundefined(JavaScript),None(Python), orNULL(R). Consider documenting this requirement in the code-writing block or adding a default return value.
455-591: Output formatters rely on format assumptions.The language-specific formatters use regex to parse output into JSON, with fallback to raw output if parsing fails. While the fallback logic is good, the formatters assume specific output structures (e.g., Python dicts with single quotes, JavaScript object literals). Edge cases like nested structures with mixed quotes or complex R list outputs may not parse correctly. Ensure comprehensive testing of these formatters.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Dockerfile(2 hunks)js-requirements.json(1 hunks)server/workflowScriptExecutorTool.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Dockerfile
🧰 Additional context used
🧬 Code graph analysis (1)
server/workflowScriptExecutorTool.ts (1)
server/logger/index.ts (3)
getLoggerWithChild(192-200)Subsystem(15-15)getLogger(36-93)
🔇 Additional comments (1)
server/workflowScriptExecutorTool.ts (1)
351-366: Good addition of input size limits.The size validation for script, input, and config addresses the previous review comment about missing size limits. This helps prevent DoS attacks via large inputs.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
server/workflowScriptExecutorTool.ts (4)
30-30: Timeout value remains hardcoded at 40 minutes.This concern was previously raised. The hardcoded 40-minute timeout is still present and could allow resource exhaustion.
Based on past review comments.
36-77: Escape sequence normalization still corrupts valid JSON.This issue was previously flagged. The cascading regex replacements (lines 44-51) will double-escape already-valid JSON strings. For example, a valid JSON string
"hello\nworld"would be transformed to"hello\\nworld", changing its meaning. Line 51's\\→\\\\replacement compounds earlier transformations.Based on past review comments.
90-227: Allowed imports still enable network and filesystem access.This concern was previously raised. The whitelist permits network libraries (Python:
requests,urllib; JavaScript:axios,fetch; R:httr) and filesystem access (JavaScript: line 142 listshttps,http). If scripts originate from untrusted sources, this creates data exfiltration and SSRF risks.Based on past review comments.
229-343: Security validation remains regex-based and bypassable.This concern was extensively documented in previous reviews. The regex-based approach can be circumvented through:
- Python:
getattr(__builtins__, '__import__'),eval('imp'+'ort os')- JavaScript:
global.require,this.require, string concatenation- Dynamic imports and encoded strings
Based on past review comments.
🧹 Nitpick comments (3)
server/workflowScriptExecutorTool.ts (3)
593-660: Consider additional sandbox hardening.The bubblewrap configuration provides basic isolation but could be strengthened with:
- Network isolation: Add
'--unshare-net'to prevent network access (addresses the network library concerns in allowedImports)- Privilege dropping: Add
'--no-new-privs'to prevent privilege escalation- Seccomp filtering: Add explicit seccomp profile to restrict syscalls
- Capability dropping: Use
'--cap-drop', 'ALL'to drop all capabilities- Environment sanitization: Clear environment variables and inject only a minimal allowlist
These additions would significantly strengthen the sandbox, especially given the concerns about network libraries and import bypass risks.
662-679: Fire-and-forget cleanup is intentional but should be documented.The cleanup operations (lines 667-674) use
.catch()instead ofawait, creating fire-and-forget deletions. While this is acceptable for cleanup operations to avoid blocking the response, consider adding a brief comment explaining this design choice.
680-877: Resource cleanup improved; minor issues remain.The cleanup logic has been improved with
cleanTempFilescalled in error paths. However:
- Line 810: Timeout message uses singular "minute" but should handle plural:
timeout_value > 1 ? 'minutes' : 'minute'- Line 717:
workflowIdis used to construct filesystem paths without validation. While past review feedback indicated path traversal validation was "not required," at minimum, validate thatworkflowIdcontains only safe characters (alphanumeric, dash, underscore) to prevent unexpected behavior.- error: `Script execution timed out after ${timeout_value} minute`, + error: `Script execution timed out after ${timeout_value} ${timeout_value > 1 ? 'minutes' : 'minute'}`,For workflowId validation, add at the start of the function:
if (workflowId && !/^[a-zA-Z0-9_-]+$/.test(workflowId)) { return { success: false, output: {}, error: 'Invalid workflow ID format', exitCode: -1 } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/workflowScriptExecutorTool.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
server/workflowScriptExecutorTool.ts (1)
server/logger/index.ts (3)
getLoggerWithChild(192-200)Subsystem(15-15)getLogger(36-93)
🔇 Additional comments (3)
server/workflowScriptExecutorTool.ts (3)
346-425: Input size limits successfully added.The function now validates script and input sizes (lines 351-366), addressing the previous concern about unbounded inputs. The script generation logic correctly validates security, formats inputs, and assembles the final script.
✅ Addressed in recent commits as noted in past review comments.
879-891: LGTM!The exported functions provide clean public APIs. The
getCodeWritingBlockfunction appropriately generates language-specific documentation for users, including allowed imports, input/config access patterns, and file handling instructions.
33-33: Code change is correct and defaults to the safer state.The logic
process.env.USE_BUBBLEWRAP !== 'false'correctly defaults to enabled while allowing explicit opt-out. This is safer than the previous approach. However, the suggestion to enforce production-specific hardening via NODE_ENV checks is not currently implemented in the codebase. Verify whether your deployment architecture requires production-specific overrides; the current implementation is safe as-is.
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
server/api/chat/agents.ts (2)
142-143: Clarify redundantTraceEventimport.The code imports
TraceEventtwice from the same module:
- Line 142:
type TraceEvent as JAFTraceEvent- Line 143:
type TraceEventThis creates ambiguity. Based on usage at line 1830 where
TraceEventis referenced directly, it appears line 143 is the intended import. Consider removing the aliased import on line 142 ifJAFTraceEventis unused, or use consistent naming throughout.Apply this diff to remove the redundant aliased import:
type RunState as JAFRunState, type RunResult as JAFRunResult, - type TraceEvent as JAFTraceEvent, type TraceEvent, } from "@xynehq/jaf"
1535-1535: Fix variable naming convention.The variable
gatheredFragmentskeysshould follow camelCase convention:gatheredFragmentsKeys(capital 'K').Apply this diff:
- const gatheredFragmentskeys = new Set<string>() // to track uniqueness + const gatheredFragmentsKeys = new Set<string>() // to track uniquenessAnd update all references at lines 1973, 1975, 1988, 1990.
frontend/src/components/workflow/TriggerExecutionUI.tsx (2)
8-8: Remove unused props.The
onBack(line 8) andpath(lines 13, 26) props are declared but never used in the component. Thebuilderprop (line 12) already controls the rendering mode on line 101.Apply this diff to clean up the interface:
interface TriggerExecutionUIProps { isVisible: boolean - onBack: () => void onClose?: () => void stepExecutionId: string stepName?: string builder?: boolean // true for builder mode, false for execution mode - path?: string // "builder" or "execution" onTriggerSubmitted?: () => void // Callback to restart polling isStepActive?: boolean // Whether the step is currently active and can be triggered }And update the component signature:
const TriggerExecutionUI: React.FC<TriggerExecutionUIProps> = ({ isVisible, - onBack, onClose, stepExecutionId, stepName = "Trigger Step", builder = true, onTriggerSubmitted, - path="", isStepActive = true, }) => {Also applies to: 13-13, 26-26
96-103: Remove unnecessary empty lines.The empty lines at 98-99 between the two early return conditions are unnecessary and reduce code readability.
Apply this diff:
if (!isVisible) return null - - // In builder mode, don't show any sidebar (return null or empty div) if (builder) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/src/components/workflow/TriggerExecutionUI.tsx(1 hunks)frontend/src/components/workflow/api/ApiHandlers.ts(2 hunks)server/api/chat/agents.ts(13 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
frontend/src/components/workflow/TriggerExecutionUI.tsx (1)
frontend/src/api.ts (1)
api(5-5)
server/api/chat/agents.ts (6)
server/shared/types.ts (2)
AttachmentEntity(39-39)ImageCitation(114-114)server/utils/index.ts (1)
getDateForAI(3-29)server/api/chat/types.ts (2)
MinimalAgentFragment(88-94)ImageCitation(80-86)server/ai/types.ts (3)
TemporalClassifier(279-282)QueryRouterResponse(461-461)ConverseResponse(311-323)server/api/chat/utils.ts (1)
collectFollowupContext(79-144)server/api/chat/chat.ts (2)
UnderstandMessageAndAnswerForGivenContext(3870-3921)UnderstandMessageAndAnswer(3733-3868)
frontend/src/components/workflow/api/ApiHandlers.ts (1)
frontend/src/api.ts (1)
api(5-5)
🔇 Additional comments (3)
frontend/src/components/workflow/api/ApiHandlers.ts (3)
193-197: LGTM!The type parameter update correctly reflects the interface rename and maintains internal consistency with the
WorkflowTemplateResponseinterface.
202-205: LGTM!The return type and extraction type are correctly updated to use
WorkflowTemplateSimplified, maintaining consistency throughout the function.
6-50: No breaking changes detected; changes are safe.The
WorkflowTemplateSimplifiedinterface is defined and used only withinApiHandlers.tsfor API response typing. The oldWorkflowTemplateinterface remains defined separately in other files (Types.ts, WorkflowBuilder.tsx, executedWorkflowRenderer.tsx, workflow.tsx) and is unchanged. The code consuming the API responses already uses defensive patterns likedescription: wf.description || ""and optional chaining, so making the description field optional does not introduce any issues.
YashJuspay626
force-pushed
the
files-extract2
branch
from
e3ca03c to
bb06385
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/workflow/api/ApiHandlers.ts (1)
52-79: Consolidate duplicate template types to prevent inconsistency.Both
ApiTemplate(line 56:description: string- required) andWorkflowTemplateSimplified(line 12:description?: string- optional) represent workflow template data from the same endpoint (api.workflow.templates.$get()). However, they have conflicting requirements for thedescriptionfield:
templatesAPI.fetchAll()(line 213-217) returnsApiTemplate[]with requireddescriptionuserWorkflowsAPI.fetchWorkflows()(line 193-197) returnsWorkflowTemplateSimplified[]with optionaldescriptionThis inconsistency creates type-safety issues and confusion for consumers.
Consider one of these approaches:
- Remove
ApiTemplateand useWorkflowTemplateSimplifiedeverywhere (preferred if the backend can omit description):export const templatesAPI = { async fetchAll(): Promise<ApiTemplate[]> { const response = await api.workflow.templates.$get() - return extractResponseData<ApiTemplate[]>(response) + return extractResponseData<WorkflowTemplateSimplified[]>(response) }, }Then update the return type signature:
- async fetchAll(): Promise<ApiTemplate[]> { + async fetchAll(): Promise<WorkflowTemplateSimplified[]> {
- Or make
ApiTemplate.descriptionoptional if keeping both types:interface ApiTemplate { id: number workspaceId: string name: string - description: string + description?: string version: string // ... }Also applies to: 209-218
♻️ Duplicate comments (4)
server/api/chat/agents.ts (4)
1629-1668: Fragment ID collision remains unresolved.This issue was previously flagged: when gathering fragments from multiple sources (root.children, chatContexts, threadContexts), each uses its own local
idxstarting from 0. The fallback ID generation invespaResultToMinimalAgentFragment(line 537) creates IDs likeFrangment_id_${idx}, which can collide across different sources if documents lack adocId.The suggested fix from the previous review (using a running counter across all sources) has not been applied.
1749-1749: RedundantdateForAIdeclaration remains unresolved.Line 1749 redeclares
dateForAIinside theagentInstructions()function, shadowing the outer-scope variable declared at line 943. This unnecessarily recalculates the date on each function call.The suggested fix (remove the inner declaration and use the outer variable) has not been applied.
1957-1993: Off-by-one error inbestDocIndexesbounds check remains unresolved.Line 1970 checks
idx >= 0 && idx < contexts.length(0-based validation), but line 1971 accessescontexts[idx - 1](1-based indexing). This mismatch means:
idx = 0would pass the check but accesscontexts[-1](undefined)idx = contexts.lengthwould correctly fail the checkThe suggested fix (change condition to
idx >= 1 && idx <= contexts.length) has not been applied.
4478-4478: MissingstartIdxparameter incollectFollowupContextcall remains unresolved.Line 4478 calls
collectFollowupContext(filteredMessages)with only one argument, but the function signature requires(messages: SelectMessage[], startIdx: number, maxHops = 12). This will cause a TypeScript compilation error.The suggested fix (pass
filteredMessages.length - 1as the second argument) has not been applied.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/src/components/workflow/TriggerExecutionUI.tsx(1 hunks)frontend/src/components/workflow/api/ApiHandlers.ts(2 hunks)requirements.R(1 hunks)server/api/chat/agents.ts(13 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/components/workflow/TriggerExecutionUI.tsx
- requirements.R
🧰 Additional context used
🧬 Code graph analysis (2)
server/api/chat/agents.ts (5)
server/utils/index.ts (1)
getDateForAI(3-29)server/api/chat/types.ts (2)
MinimalAgentFragment(88-94)ImageCitation(80-86)server/ai/types.ts (3)
TemporalClassifier(279-282)QueryRouterResponse(461-461)ConverseResponse(311-323)server/api/chat/utils.ts (1)
collectFollowupContext(79-144)server/api/chat/chat.ts (2)
UnderstandMessageAndAnswerForGivenContext(3870-3921)UnderstandMessageAndAnswer(3733-3868)
frontend/src/components/workflow/api/ApiHandlers.ts (1)
frontend/src/api.ts (1)
api(5-5)
🔇 Additional comments (3)
frontend/src/components/workflow/api/ApiHandlers.ts (3)
6-8: LGTM: WorkflowTemplateResponse type updated consistently.The interface correctly reflects the simplified template type.
193-197: LGTM: fetchWorkflows correctly uses WorkflowTemplateSimplified.The generic type parameter is correctly updated.
202-205: LGTM: fetchTemplateById correctly returns WorkflowTemplateSimplified.Both the return type annotation and the extraction are properly updated.
YashJuspay626
force-pushed
the
files-extract2
branch
10 times, most recently
from
bb9e5ac to
bd4101a
Compare
YashJuspay626
force-pushed
the
files-extract2
branch
from
bd4101a to
465c421
Compare
YashJuspay626
force-pushed
the
files-extract2
branch
from
465c421 to
d895bb7
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/workflow/WorkflowBuilder.tsx (1)
2474-2489: Execution tool mapping uses toolExecIds but interface defines toolExecId. Handle both.Current filter reads step.toolExecIds; with toolExecId it fails and hides execution results.
- if (isExecution) { - // For executions, get tool executions from toolExecIds - toolExecutions = - templateToUse.toolExecutions?.filter((toolExec) => - step.toolExecIds?.includes(toolExec.id), - ) || [] + if (isExecution) { + // Support both legacy toolExecIds[] and newer toolExecId (string or string[]) + const execIds: string[] = Array.isArray((step as any).toolExecIds) + ? (step as any).toolExecIds + : Array.isArray((step as any).toolExecId) + ? (step as any).toolExecId + : (step as any).toolExecId + ? [ (step as any).toolExecId ] + : [] + toolExecutions = + templateToUse.toolExecutions?.filter((toolExec) => + execIds.includes(toolExec.id), + ) || []
♻️ Duplicate comments (25)
deployment/portable/docker-compose.app-build.yml (1)
26-26: Remove privileged mode and use explicit capabilities.Setting
privileged: truecompletely defeats container isolation and contradicts the security goals of using Bubblewrap for sandboxing. Bubblewrap can operate with specific Linux capabilities instead of full privileges.Apply this diff to use explicit capabilities:
- privileged: true + cap_add: + - SYS_ADMIN + - SYS_PTRACEfrontend/src/components/CodeEditor.tsx (1)
51-57: No stale closure issue - past review comment not applicable.The past review incorrectly identified a stale closure problem with
handleSavein a useEffect. However,handleKeyDown(lines 51-57) is a regular event handler function, not wrapped in useEffect. It's passed directly asonKeyDown={handleKeyDown}on line 65, which means React recreates it on each render with fresh closures over the currentcurrentValuestate. No dependency array or useCallback is needed here.frontend/src/components/workflow/api/ApiHandlers.ts (1)
9-12: Verify consumers handle optional description field.Making
descriptionoptional inWorkflowTemplateSimplifiedis a breaking change. Past review identified that consumers at WorkflowBuilder.tsx line 4869 and TemplateCard.tsx line 56 may not handleundefinedproperly.Run this script to check how description is used:
#!/bin/bash # Check description field usage in workflow components rg -n "template\.description" --type ts --type tsx -C 2 | head -50frontend/src/components/workflow/TriggerExecutionUI.tsx (4)
34-46: Remove debug console statements.Multiple debug
console.logstatements remain in the code. These should be removed or wrapped in development-only guards to avoid cluttering production logs.Apply this diff:
const handleTrigger = async () => { - console.log("🔍 Trigger button clicked:", { stepExecutionId }) setIsTriggering(true) setTriggerError(null) try { - console.log("🔍 Making trigger API call to:", `/api/v1/workflow/steps/${stepExecutionId}/trigger`) const response = await api.workflow.steps[":stepId"].trigger.$post({ param: { stepId: stepExecutionId } }) - console.log("🔍 Trigger API response:", response) const data = await response.json() - console.log("🔍 Trigger API response data:", data) if (data.success) { setTriggerStatus('triggered') // Call the callback to restart workflow polling if (onTriggerSubmitted) { onTriggerSubmitted() } // Auto-close the sidebar after successful trigger setTimeout(() => { if (onClose) { onClose() } }, 2000) } else { throw new Error(data.message || 'Failed to trigger step') } } catch (error: any) { - console.error('Error triggering step:', error) setTriggerError(error.response?.data?.message || error.message || 'Failed to trigger step') setTriggerStatus('error') } finally {Also applies to: 66-66
57-61: Clean up setTimeout to prevent memory leaks.The
setTimeoutat lines 57-61 is not cleaned up if the component unmounts before completion, which can invokeonCloseon an unmounted component.Add cleanup:
const [isTriggering, setIsTriggering] = useState(false) const [triggerError, setTriggerError] = useState<string | null>(null) const [triggerStatus, setTriggerStatus] = useState<'pending' | 'triggered' | 'error'>('pending') + const timeoutRef = React.useRef<NodeJS.Timeout>() + + React.useEffect(() => { + return () => { + if (timeoutRef.current) { + clearTimeout(timeoutRef.current) + } + } + }, []) const handleTrigger = async () => { // ... // Auto-close the sidebar after successful trigger - setTimeout(() => { + timeoutRef.current = setTimeout(() => { if (onClose) { onClose() } }, 2000)
38-68: Fix error handling to match hono client response structure.The error handling assumes axios-style errors (
error.response?.data?.message), but the hono client returns standard Web APIResponseobjects. The code should checkresponse.okbefore parsing, and errors should be thrown explicitly rather than caught from response structure.Apply this pattern:
try { const response = await api.workflow.steps[":stepId"].trigger.$post({ param: { stepId: stepExecutionId } }) + if (!response.ok) { + const errorData = await response.json().catch(() => ({ message: "Network error" })) + throw new Error(errorData.message || `HTTP ${response.status}: ${response.statusText}`) + } + const data = await response.json() if (data.success) { setTriggerStatus('triggered') if (onTriggerSubmitted) { onTriggerSubmitted() } timeoutRef.current = setTimeout(() => { if (onClose) { onClose() } }, 2000) } else { throw new Error(data.message || 'Failed to trigger step') } } catch (error: any) { - setTriggerError(error.response?.data?.message || error.message || 'Failed to trigger step') + setTriggerError(error.message || 'Failed to trigger step') setTriggerStatus('error') } finally {
195-211: Tooltip may not display reliably on disabled button.The tooltip at lines 206-209 relies on
group-hover, but browsers may not fire hover events on disabled elements. The button should be wrapped in a non-disabled container to ensure the tooltip displays.Apply this fix:
) : ( - <div className="relative group"> + <div className="relative group cursor-not-allowed"> <Button disabled={true} - className="w-full bg-gray-400 hover:bg-gray-400 text-gray-600 cursor-not-allowed" + className="w-full bg-gray-400 hover:bg-gray-400 text-gray-600 pointer-events-none" > <div className="flex items-center space-x-2"> <Play className="w-4 h-4" /> <span>Trigger</span> </div> </Button> <div className="absolute bottom-full left-1/2 transform -translate-x-1/2 mb-2 px-3 py-2 bg-gray-900 text-white text-sm rounded-lg opacity-0 group-hover:opacity-100 transition-opacity duration-200 whitespace-nowrap z-10">frontend/src/components/workflow/ReviewExecutionUI.tsx (3)
50-70: Reset submission state when panel reopens.The
submissionStatusstate persists across opens, which means after one approval/rejection, the action buttons won't render when the panel is opened again. Add a useEffect to reset state when the panel opens.Also remove the debug console.log statements:
const [isSubmitting, setIsSubmitting] = useState(false) const [submitError, setSubmitError] = useState<string | null>(null) const [submissionStatus, setSubmissionStatus] = useState<'pending' | 'approved' | 'rejected' | 'error'>('pending') - // Debug logging - React.useEffect(() => { - console.log("🔍 ReviewExecutionUI props changed:", { - isVisible, - stepExecutionId, - stepName, - builder, - submissionStatus: submissionStatus - }) - }, [isVisible, stepExecutionId, stepName, builder, submissionStatus]) - - console.log("🔍 ReviewExecutionUI render:", { - isVisible, - builder, - stepExecutionId, - submissionStatus - }) + + // Reset state when panel reopens + React.useEffect(() => { + if (isVisible) { + setSubmissionStatus('pending') + setSubmitError(null) + setDownloadError(null) + setIsSubmitting(false) + } + }, [isVisible, stepExecutionId]) + const [isDownloading, setIsDownloading] = useState(false)
137-143: Remove invalid execution ID fallback.The fallback
stepExecutionId.split('-')[0]at line 139 produces a truncated, invalid UUID. Remove this fallback and rely only on the providedworkflowExecutionIdorpreviousStepResult?.workflowExecutionId.Apply this diff:
// Use the provided workflowExecutionId prop or try to extract from previousStepResult - const executionId = workflowExecutionId || - previousStepResult?.workflowExecutionId || - stepExecutionId.split('-')[0] + const executionId = + workflowExecutionId ?? previousStepResult?.workflowExecutionId if (!executionId) { throw new Error('Workflow execution ID not available') }
145-208: Remove debug log from download handler.The file download logic is well-implemented with proper error handling for the hono client. Just remove the debug console.log at line 201.
// Cleanup window.URL.revokeObjectURL(url) document.body.removeChild(link) - console.log('Files downloaded successfully') } catch (error: any) {frontend/src/components/ScriptSidebar.tsx (1)
243-277: API update errors are silently swallowed after visual update.At lines 246-252, the visual node is updated immediately. If the API call fails (lines 268-272), the error is only logged to console, but the UI shows success. This creates a data-consistency gap where the visual state doesn't match the backend.
Consider showing user feedback:
} catch (error) { console.error("Failed to update script tool in API:", error) + // TODO: Show toast notification to inform user + // Example: toast.error("Visual update succeeded but failed to sync with server. Changes may be lost.") // Don't return early - visual update already happened } }Better approach: Either revert the visual update on API failure, or implement a retry/sync mechanism.
frontend/src/components/workflow/executedWorkflowRenderer.tsx (3)
2299-2299: Fix browser-incompatible timeout type.Past review flagged this:
NodeJS.Timeoutis incorrect for browsersetTimeout, which returnsnumber. This type mismatch was marked as addressed but remains unfixed.- const pollingTimeoutRef = useRef<NodeJS.Timeout | null>(null) + const pollingTimeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null)
1001-1001: Replace commented-out icon with actual icon.This line was commented out to address a missing
botLogoimport, but now the icon container renders empty. Replace with theCodeicon that's already imported.- {/* <img src={botLogo} alt="Bot" width={16} height={16} /> */} + <Code width={16} height={16} color="#2563EB" />
2896-3058: Implement proper polling with request cancellation and backoff.Past review flagged multiple issues that remain unaddressed:
- No
AbortControllerto cancel in-flight requests on unmount (race conditions)- No exponential backoff on errors (could hammer server)
- No retry limit (infinite loop on persistent errors)
- Logs sensitive data in production (lines 2907, 2922, 2947, etc.)
+ const abortControllerRef = useRef<AbortController | null>(null) + const retryCountRef = useRef(0) + const MAX_RETRIES = 10 const fetchWorkflowStatus = (async (executionId: string) => { + // Cancel previous request + if (abortControllerRef.current) { + abortControllerRef.current.abort() + } + abortControllerRef.current = new AbortController() + try { - console.log("📊 Fetching execution data for:", executionId) + if (process.env.NODE_ENV === 'development') { + console.log("📊 Fetching execution data for:", executionId) + } - const response = await api.workflow.executions[executionId].$get() + const response = await api.workflow.executions[executionId].$get({ + signal: abortControllerRef.current.signal + }) if (!response.ok) { throw new Error(`HTTP ${response.status}: ${response.statusText}`) } const executionData = await response.json() - console.log("📊 Raw execution data:", executionData) + if (process.env.NODE_ENV === 'development') { + console.log("📊 Raw execution data:", executionData) + } // ... rest of processing ... + // Reset retry count on success + retryCountRef.current = 0 + // Continue polling after 5 seconds for automated steps - console.log("⏰ Scheduling next poll in 5 seconds") + if (process.env.NODE_ENV === 'development') { + console.log("⏰ Scheduling next poll in 5 seconds") + } pollingTimeoutRef.current = setTimeout(() => { fetchWorkflowStatus(executionId) }, 5000) return extractedData } catch (error) { + // Don't retry if aborted + if (error instanceof DOMException && error.name === 'AbortError') { + return null + } + console.error('❌ Failed to fetch execution data:', error) + retryCountRef.current++ + + if (retryCountRef.current >= MAX_RETRIES) { + console.error('Max retries reached, stopping polling') + return null + } + + // Exponential backoff: 5s, 10s, 20s, capped at 60s + const backoffDelay = Math.min(5000 * Math.pow(2, retryCountRef.current - 1), 60000) + console.log(`⏰ Retry ${retryCountRef.current}/${MAX_RETRIES} in ${backoffDelay}ms`) - console.log("⏰ Scheduling retry in 5 seconds after error") pollingTimeoutRef.current = setTimeout(() => { fetchWorkflowStatus(executionId) - }, 5000) + }, backoffDelay) return null } })Also update cleanup in useEffect:
useEffect(() => { return () => { if (pollingTimeoutRef.current) { clearTimeout(pollingTimeoutRef.current) pollingTimeoutRef.current = null } + if (abortControllerRef.current) { + abortControllerRef.current.abort() + abortControllerRef.current = null + } + retryCountRef.current = 0 } }, [])frontend/src/components/workflow/WorkflowBuilder.tsx (5)
3212-3227: Remove verbose debug logs from execution flow.Console dumps are noisy and may leak data. Keep errors; drop debug logs.
- console.log("🔍 Full execution response:", response) ... - console.log("🔍 Extracted execution ID:", executionId) - console.log("🔍 Response structure:", { - hasData: !!response.data, - hasExecution: !!response.data?.execution, - hasDirectId: !!response.id, - responseKeys: Object.keys(response) - })- console.log("🔍 Full execution response (direct):", response) ... - console.log("🔍 Extracted execution ID (direct):", executionId) - console.log("🔍 Response structure (direct):", { - hasData: !!response.data, - hasExecution: !!response.data?.execution, - hasDirectId: !!response.id, - responseKeys: Object.keys(response) - })Also applies to: 3300-3315
1019-1027: Fix PlusButton nodeId/hasNext for trigger nodes (still undefined).Use id from NodeProps and the hasNext variable; data.id is undefined for trigger nodes, breaking “add next step”.
- {!isAwaitingTrigger && ( - <PlusButton - hasNext={Boolean(data.hasNext)} - nodeId={String(data.id)} - isConnectable={isConnectable} - showHandles={false} - /> - )} + {!isAwaitingTrigger && ( + <PlusButton + hasNext={Boolean(hasNext)} + nodeId={id} + isConnectable={isConnectable} + showHandles={false} + /> + )}
1168-1171: Remove debug console.log in Script node “+” handler.Leftover debug noise; keep event dispatch only.
- console.log("Plus button clicked for node:", id)
2380-2451: DFS layout breaks for executions: map template IDs → execution IDs.performSimpleDFS uses templateToUse.rootWorkflowStepTemplateId and nextStepIds directly, so execution nodes fall back to (400,100).
const performSimpleDFS = (steps: any[]) => { - const rootStepId = templateToUse.rootWorkflowStepTemplateId + const rootStepId = templateToUse.rootWorkflowStepTemplateId + const isExec = !!(templateToUse as any).stepExecutions + // Map: template step ID -> execution step ID + const templateToExec = new Map<string, string>() + if (isExec) { + steps.forEach((s: any) => { + if (s.workflowStepTemplateId) templateToExec.set(s.workflowStepTemplateId, s.id) + }) + } ... - const dfs = (nodeId: string, parentX: number, parentY: number, siblingIds: string[] = [], myIndex: number = 0) => { + const dfs = (nodeId: string, parentX: number, parentY: number, siblingIds: string[] = [], myIndex: number = 0) => { ... - const currentNode = steps.find(s => s.id === nodeId) + const currentNode = steps.find((s: any) => s.id === nodeId) ... - const nextStepIds = currentNode.nextStepIds || [] + const rawNext = currentNode.nextStepIds || [] + const nextStepIds = isExec + ? rawNext.map((tid: string) => templateToExec.get(tid)).filter(Boolean) as string[] + : rawNext ... } - console.log(`Starting simple DFS from root: ${rootStepId}`) - nodePositions.set(rootStepId, { x: 400, y: 100 }) + // Resolve correct root (execution ID when rendering executions) + let rootId = rootStepId + if (isExec) rootId = templateToExec.get(rootStepId) || rootStepId + console.log(`Starting simple DFS from root: ${rootId}`) + nodePositions.set(rootId, { x: 400, y: 100 }) - const rootNode = steps.find(s => s.id === rootStepId) - if (rootNode?.nextStepIds) { - rootNode.nextStepIds.forEach((childId: string, index: number) => { - dfs(childId, 400, 100, rootNode.nextStepIds, index) - }) - } + const rootNode = steps.find((s: any) => s.id === rootId) + || steps.find((s: any) => isExec && s.workflowStepTemplateId === rootStepId) + if (rootNode?.nextStepIds) { + const children = isExec + ? (rootNode.nextStepIds as string[]).map((tid: string) => templateToExec.get(tid)).filter(Boolean) as string[] + : rootNode.nextStepIds + children.forEach((childId: string, index: number) => { + dfs(childId, 400, 100, children, index) + }) + } }
3570-3596: Respect user-selected review path when wiring next steps.Handlers ignore selectedReviewPath and auto-pick first free path → connections can land on wrong branch.
- // Use approved path if it's not used, otherwise use rejected path - if (!config.approved) { + // Prefer user-selected path, then fall back + if (selectedReviewPath === "approved" && !config.approved) { sourceHandle = "approved" edgeLabel = "Approved" reviewPath = "approved" - } else if (!config.rejected) { + } else if (selectedReviewPath === "rejected" && !config.rejected) { sourceHandle = "rejected" edgeLabel = "Rejected" reviewPath = "rejected" + } else if (!config.approved) { + sourceHandle = "approved" + edgeLabel = "Approved" + reviewPath = "approved" + } else if (!config.rejected) { + sourceHandle = "rejected" + edgeLabel = "Rejected" + reviewPath = "rejected" } else { console.warn("Both approved and rejected paths are already used for review step:", selectedNodeForNext) }Apply the same preference block in:
- handleEmailConfigSave (lines 3831-3855)
- ScriptSidebar onStepCreated (lines 4914-4933)
Also clear selection on close (already done at lines 4570-4580).
Also applies to: 3831-3855, 4914-4933, 4570-4580
server/workflowScriptExecutorTool.ts (2)
30-30: Timeout too high; make configurable with sane default.40 minutes risks resource abuse. Default 5–10m; allow override via env with bounds.
-const timeout_value = 40 +const DEFAULT_TIMEOUT_MINUTES = 5 +const MAX_TIMEOUT_MINUTES = 30 +const parsed = parseInt(process.env.SCRIPT_TIMEOUT_MINUTES || `${DEFAULT_TIMEOUT_MINUTES}`, 10) +const timeout_value = Number.isFinite(parsed) + ? Math.min(Math.max(parsed, 1), MAX_TIMEOUT_MINUTES) + : DEFAULT_TIMEOUT_MINUTES
689-719: Validate workflowId before using in filesystem paths.Prevent path traversal/odd chars; early-reject invalid IDs.
let scriptPath = '' let exportDir = '' let importDir = '' try { + if (workflowId && !/^[a-zA-Z0-9_-]+$/.test(workflowId)) { + return { success: false, output: {}, error: 'Invalid workflow ID', exitCode: -1, extractedFiles: [] } + }server/api/workflow.ts (4)
2650-2716: Verify script execution sandboxing is enabled.The script execution implementation correctly passes the
executionIdfor file extraction, but sandboxing configuration should be verified before production use.Please confirm:
USE_BUBBLEWRAPenvironment variable is enabled in production- Sandboxing includes resource limits (CPU, memory, disk)
- File system access is restricted to workflow-specific directories only
Refer to
server/workflowScriptExecutorTool.tsaround lines 577-640 for sandbox configuration details.
4167-4177: CRITICAL: Pass review results to executeWorkflowChain.Line 4170 passes
tools(the workflow tool array) as the third argument toexecuteWorkflowChain, but the function expectspreviousResults(a result map). Downstream automated steps will fail to resolve prior outputs.Apply this diff:
executeWorkflowChain( currentStep.workflowExecutionId, nextStep.id, - tools + currentResults ).catch((error) => {
4517-4527: CRITICAL: Re-enable access control for file downloads.The access control check verifying that
workflow.userId === user.idis commented out (lines 4520-4527), allowing any authenticated user to download files from any workflow execution. This is a security vulnerability that must be fixed before merging.Logger.info(`Download access check: executionId=${executionId}, user=${sub}, userId=${workflow.userId}`) - // TODO: Re-enable access control in production - // Temporarily allow any user to access files for development/testing - // if (workflow.createdBy !== sub) { - // Logger.warn(`Access denied: user ${sub} tried to access workflow execution ${executionId} created by ${workflow.createdBy}`) - // throw new HTTPException(403, { - // message: "Access denied to this workflow execution", - // }) - // } + // Verify user owns this workflow execution + const [userRecord] = await db + .select() + .from(users) + .where(eq(users.email, sub)) + + if (!userRecord || userRecord.id !== workflow.userId) { + Logger.warn(`Access denied: user ${sub} tried to access workflow execution ${executionId}`) + throw new HTTPException(403, { + message: "Access denied to this workflow execution", + }) + }If workspace-level sharing is required, implement proper workspace membership checks instead of disabling authorization.
1042-1073: Refine leaf node detection for conditional workflows.The current logic treats a step as complete if all its next steps are unexecuted (draft). This works for linear workflows but may incorrectly mark conditional workflows as complete when only one branch is taken and others remain unexecuted.
Recommendation:
- Introduce explicit branch metadata (e.g.,
branchTakenflag on step executions or next step references)- Only consider next steps that are on the active branch when checking for leaf nodes
- Add test cases for conditional workflows with approval/rejection branches
Example test scenario:
- Step A completes with next steps: B (approved) and C (rejected)
- Only B executes (approved path taken)
- C remains draft (rejected path not taken)
- B completes and becomes a leaf
- Workflow should complete correctly ✓
However, without explicit branch tracking, edge cases might fail.
🧹 Nitpick comments (13)
frontend/src/components/workflow/WhatHappensNextUI.tsx (1)
143-143: Consider using array includes for cleaner code.The long
||chain works but could be more maintainable using array includes.- if (action.id === "ai_agent" || action.id === "email" || action.id === "review" || action.id === "trigger" || action.id === "run_script"||action.id === "select_agents") { + const clickableActions = ["ai_agent", "email", "review", "trigger", "run_script", "select_agents"] + if (clickableActions.includes(action.id)) {frontend/src/components/workflow/executedWorkflowRenderer.tsx (5)
43-43: Consider tighter typing for config field.The
configfield is typed asany, which reduces type safety for a public interface. If the config structure is well-defined, consider creating a proper type or usingRecord<string, unknown>as a safer alternative.- config?: any + config?: Record<string, unknown>
1557-1590: Gate debug logging behind development environment check.These console.log statements will run in production, potentially logging sensitive execution data and impacting performance.
- console.log("Debug getCurrentStepOutputData: No step or workflowData", { step: !!step, workflowData: !!workflowData }) + if (process.env.NODE_ENV === 'development') { + console.log("Debug getCurrentStepOutputData: No step or workflowData", { step: !!step, workflowData: !!workflowData }) + }Apply this pattern to all console.log calls in lines 1557-1628.
1807-1833: Extract duplicated script output display logic into helper function.The logic for displaying script output is duplicated between the input section (lines 1807-1833) and output section (lines 1950-1975). Extract into a reusable helper to reduce maintenance burden.
// Helper function at top of ExecutionSidebar component const renderScriptOrToolOutput = (output: any, step: any, tools?: any[]) => { // Check if current step is a script node - if so, show whole output if (step.type === "script" || (tools && tools.some((tool: any) => tool.type === "script"))) { return ( <pre className="whitespace-pre-wrap"> {typeof output === "object" ? JSON.stringify(output, null, 2) : String(output)} </pre> ) } if (typeof output === "object" && output) { // Check for script tool output - show raw data if (output.toolType === "script" || output.type === "script") { return ( <div className="space-y-2"> <div> <span className="font-medium text-gray-600">Script Output:</span> <div className="mt-1 text-gray-900 whitespace-pre-wrap font-mono text-xs bg-gray-50 p-2 rounded border"> {typeof output === "object" ? JSON.stringify(output, null, 2) : String(output)} </div> </div> </div> ) } // ... rest of display logic } // ... fallback }Then replace both occurrences with calls to this helper.
Also applies to: 1950-1975
2355-2367: Gate debug logging behind development checks.Multiple debug console.log statements will execute in production, potentially logging sensitive workflow data and degrading performance.
Wrap all debug logs with environment checks:
- console.log("🔍 Workflow data structure:", { - workflowData, - stepExecutions: dataToRender?.stepExecutions?.slice(0, 2), - workflow_tools: dataToRender?.workflow_tools?.slice(0, 3) - }) + if (process.env.NODE_ENV === 'development') { + console.log("🔍 Workflow data structure:", { + workflowData, + stepExecutions: dataToRender?.stepExecutions?.slice(0, 2), + workflow_tools: dataToRender?.workflow_tools?.slice(0, 3) + }) + }Apply this pattern to all debug logs in the component (lines 2355-2367, 2534-2542, 2684-2687, 2761-2767, 2773-2817, 2907, 2922, 2947, 2997-3005, 3023, 3033-3038, 3043, 3052).
Also applies to: 2534-2542, 2684-2687
2267-3226: Consider breaking down this massive component.The
WorkflowBuilderInternalcomponent is extremely large (~950 lines) within an already massive file (3237 lines total). This makes the code hard to review, test, and maintain. Consider extracting:
- Node creation logic into a separate hook or utility
- Edge creation logic into a separate function
- Polling logic into a custom hook
- Review/trigger sidebar logic into separate components
Example structure:
// hooks/useWorkflowPolling.ts export function useWorkflowPolling(executionId: string | undefined, options: PollingOptions) { // All polling logic here return { workflowData, isPolling, error } } // hooks/useWorkflowNodes.ts export function useWorkflowNodes(workflowData: ExecutionWorkflowTemplate) { // Node creation logic return { nodes, edges } } // Then in component: const { workflowData, isPolling } = useWorkflowPolling(selectedTemplate?.id, {...}) const { nodes, edges } = useWorkflowNodes(workflowData)frontend/src/components/workflow/WorkflowBuilder.tsx (1)
2-4: Deduplicate lucide-react imports.Import FileText with the first lucide-react import.
-import { Bot, Mail, Plus, Play, Clock } from "lucide-react" +import { Bot, Mail, Plus, Play, Clock, FileText } from "lucide-react" -import { FileText } from "lucide-react"server/workflowScriptExecutorTool.ts (4)
597-621: Harden sandbox: disable network in bubblewrap.Add --unshare-net to cut egress/SSRF from scripts. Consider making it configurable.
const baseArgs = [ // Create new namespace for security '--unshare-user', - '--unshare-ipc', - '--unshare-pid', - '--unshare-uts', - '--unshare-cgroup-try', + '--unshare-ipc', + '--unshare-pid', + '--unshare-uts', + '--unshare-cgroup-try', + '--unshare-net',
3-4: Remove unused loggerWithChild.Declared but not used.
-import { getLogger, getLoggerWithChild } from "@/logger" +import { getLogger } from "@/logger" -import { Subsystem } from "@/types" -const loggerWithChild = getLoggerWithChild(Subsystem.WorkflowApi) +import { Subsystem } from "@/types"
101-104: Duplicate entries in allowedImports (Python).requests listed twice; minor cleanup.
- "csv","pandas","requests","time" + "csv","pandas","time"
804-813: Pluralize timeout message.User-facing copy nit.
- error: `Script execution timed out after ${timeout_value} minute`, + error: `Script execution timed out after ${timeout_value} minute${timeout_value === 1 ? "" : "s"}`,frontend/src/routes/_authenticated/workflow.tsx (2)
276-284: Consider making debug logging conditional.These debug logs will execute in production and may clutter logs. Consider wrapping them in a debug flag check or removing them after debugging is complete.
- // Debug logging to check toolIds - Logger.info(`GetWorkflowTemplateApi: Found ${steps.length} steps for template ${templateId}`) - steps.forEach((step, index) => { - Logger.info(`Step ${index + 1} (${step.name}): toolIds = ${JSON.stringify(step.toolIds)}`) - }) - + if (process.env.NODE_ENV === 'development') { + Logger.debug(`Found ${steps.length} steps for template ${templateId}`) + steps.forEach((step, index) => { + Logger.debug(`Step ${index + 1} (${step.name}): toolIds = ${JSON.stringify(step.toolIds)}`) + }) + }
338-338: Restore type safety for template icon functions.Widening the parameter types to
anyloses type safety. Consider usingLocalWorkflowTemplateor a union type of the expected shapes instead.- const getTemplateIcon = (workflowTemplate: any): string => { + const getTemplateIcon = (workflowTemplate: LocalWorkflowTemplate | WorkflowTemplate): string => { // Use the icon from rootStep metadata if available if (workflowTemplate.rootStep?.metadata?.icon) { return workflowTemplate.rootStep.metadata.icon- const getTemplateIconBgColor = (workflowTemplate:any): string => { + const getTemplateIconBgColor = (workflowTemplate: LocalWorkflowTemplate | WorkflowTemplate): string => { if (workflowTemplate.config.allowed_file_types?.includes('pdf')) { return '#E8F5E8'Also applies to: 357-357
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
Dockerfile(4 hunks)deployment/portable/.env.default(1 hunks)deployment/portable/docker-compose.app-build.yml(2 hunks)frontend/package.json(3 hunks)frontend/src/components/CodeEditor.tsx(1 hunks)frontend/src/components/ScriptSidebar.tsx(1 hunks)frontend/src/components/workflow/AIAgentConfigUI.tsx(1 hunks)frontend/src/components/workflow/EmailConfigUI.tsx(1 hunks)frontend/src/components/workflow/PlusButton.tsx(1 hunks)frontend/src/components/workflow/ReviewExecutionUI.tsx(1 hunks)frontend/src/components/workflow/TriggerExecutionUI.tsx(1 hunks)frontend/src/components/workflow/Types.ts(1 hunks)frontend/src/components/workflow/WhatHappensNextUI.tsx(5 hunks)frontend/src/components/workflow/WorkflowBuilder.tsx(40 hunks)frontend/src/components/workflow/WorkflowIcons.tsx(1 hunks)frontend/src/components/workflow/api/ApiHandlers.ts(2 hunks)frontend/src/components/workflow/executedWorkflowRenderer.tsx(37 hunks)frontend/src/routes/_authenticated/workflow.tsx(13 hunks)js-requirements.json(1 hunks)requirements.R(1 hunks)requirements.txt(1 hunks)server/.env.default(1 hunks)server/api/script.ts(1 hunks)server/api/workflow.ts(43 hunks)server/db/schema/workflows.ts(2 hunks)server/server.ts(3 hunks)server/types/workflowTypes.ts(1 hunks)server/workflowScriptExecutorTool.ts(1 hunks)start.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- requirements.txt
- frontend/src/components/workflow/AIAgentConfigUI.tsx
- frontend/src/components/workflow/WorkflowIcons.tsx
- frontend/src/components/workflow/EmailConfigUI.tsx
- frontend/src/components/workflow/Types.ts
- requirements.R
- start.sh
- server/types/workflowTypes.ts
🧰 Additional context used
🧠 Learnings (11)
📚 Learning: 2025-09-02T16:41:31.729Z
Learnt from: devesh-juspay
PR: xynehq/xyne#758
File: server/db/schema/workflowTools.ts:9-14
Timestamp: 2025-09-02T16:41:31.729Z
Learning: In the workflowTools.ts schema, the user prefers to add notNull() constraint to the type field but does not want onDelete cascade behavior for the workflowTemplateId foreign key reference.
Applied to files:
server/db/schema/workflows.tsfrontend/src/components/workflow/api/ApiHandlers.tsfrontend/src/routes/_authenticated/workflow.tsxfrontend/src/components/workflow/executedWorkflowRenderer.tsxserver/api/workflow.ts
📚 Learning: 2025-09-04T09:29:29.879Z
Learnt from: devesh-juspay
PR: xynehq/xyne#758
File: server/db/schema/workflowServiceConfig.ts:51-53
Timestamp: 2025-09-04T09:29:29.879Z
Learning: In the workflowServiceConfig.ts schema, the user prefers to keep currentWorkflowExeId as a simple uuid reference without adding foreign key constraints or onDelete behaviors to the workflowExe table.
Applied to files:
server/db/schema/workflows.tsfrontend/src/components/workflow/api/ApiHandlers.tsfrontend/src/components/workflow/executedWorkflowRenderer.tsxserver/api/workflow.ts
📚 Learning: 2025-10-16T13:11:07.070Z
Learnt from: debajyoti0606
PR: xynehq/xyne#1075
File: Dockerfile:70-70
Timestamp: 2025-10-16T13:11:07.070Z
Learning: In the xyne repository, using the `--break-system-packages` flag for pip installations in Dockerfiles is considered safe and expected behavior within container contexts.
Applied to files:
Dockerfile
📚 Learning: 2025-10-28T07:51:03.144Z
Learnt from: Sithaarth24
PR: xynehq/xyne#1127
File: server/db/schema/workflows.ts:320-320
Timestamp: 2025-10-28T07:51:03.144Z
Learning: In the workflow template architecture, the API layer handles conversion between external UUIDs (used by frontend) and internal integer IDs (used in database). Frontend types expect UUID strings for workflow template IDs, while internal schemas like createWorkflowStepTemplateSchema expect integers. The publicWorkflowTemplateSchema maps external_id → id for API responses.
Applied to files:
frontend/src/components/workflow/api/ApiHandlers.tsfrontend/src/routes/_authenticated/workflow.tsxfrontend/src/components/workflow/executedWorkflowRenderer.tsxserver/api/workflow.ts
📚 Learning: 2025-10-17T08:54:59.756Z
Learnt from: debajyoti0606
PR: xynehq/xyne#1075
File: server/workflowScriptExecutorTool.ts:36-77
Timestamp: 2025-10-17T08:54:59.756Z
Learning: In server/workflowScriptExecutorTool.ts, the validateJson function's escape sequence normalization logic (lines 41-52 that replace \n, \r, \t, \f, \b, \", and \\ before parsing) is intentionally designed to handle edge cases and should not be flagged as potentially corrupting valid JSON. The double-escaping behavior is expected.
Applied to files:
server/workflowScriptExecutorTool.tsserver/api/workflow.ts
📚 Learning: 2025-10-17T10:33:37.098Z
Learnt from: debajyoti0606
PR: xynehq/xyne#1075
File: server/workflowScriptExecutorTool.ts:480-532
Timestamp: 2025-10-17T10:33:37.098Z
Learning: In server/workflowScriptExecutorTool.ts, the formatJavaScriptOutput function's regex pattern for quoting property names (using `/([{,]\s*)([a-zA-Z_$][a-zA-Z0-9_$]*)\s*:/g`) is intentionally designed as a best-effort approach, even though it may match inside string values in edge cases. This is expected behavior with fallback handling via try-catch blocks.
Applied to files:
server/workflowScriptExecutorTool.ts
📚 Learning: 2025-06-06T11:02:33.505Z
Learnt from: naSim087
PR: xynehq/xyne#506
File: server/scripts/clear-vespa-data.ts:652-653
Timestamp: 2025-06-06T11:02:33.505Z
Learning: Testing scripts in the `server/scripts/` directory often contain intentionally empty or placeholder values (like empty email strings) to prevent accidental execution against real data. This is a safety practice and should not be flagged as an issue when the user clarifies it's a testing script.
Applied to files:
server/workflowScriptExecutorTool.tsserver/api/workflow.ts
📚 Learning: 2025-09-29T06:32:31.923Z
Learnt from: MayankBansal2004
PR: xynehq/xyne#914
File: frontend/src/hooks/useMermaidRenderer.tsx:78-101
Timestamp: 2025-09-29T06:32:31.923Z
Learning: In frontend/src/hooks/useMermaidRenderer.tsx, the mermaidPatterns array intentionally requires trailing newlines (\n) for diagram header detection (e.g., /^graph\s+(TD|TB|BT|RL|LR)\s*\n/i). MayankBansal2004 confirmed this implementation is working well and should not be changed.
Applied to files:
server/workflowScriptExecutorTool.ts
📚 Learning: 2025-06-16T08:36:57.488Z
Learnt from: MayankBansal2004
PR: xynehq/xyne#532
File: frontend/src/routes/_authenticated/chat.tsx:1238-1242
Timestamp: 2025-06-16T08:36:57.488Z
Learning: MayankBansal2004 prefers to keep working implementations even when security improvements are suggested, but appreciates being made aware of potential security considerations for future reference.
Applied to files:
server/workflowScriptExecutorTool.ts
📚 Learning: 2025-09-24T08:03:32.428Z
Learnt from: MayankBansal2004
PR: xynehq/xyne#948
File: frontend/src/services/callNotifications.ts:514-525
Timestamp: 2025-09-24T08:03:32.428Z
Learning: MayankBansal2004 plans to address the security concern about access tokens in URLs by changing from query parameters to router params in the calling functionality implementation.
Applied to files:
server/workflowScriptExecutorTool.ts
📚 Learning: 2025-09-12T13:28:43.782Z
Learnt from: devesh-juspay
PR: xynehq/xyne#821
File: server/server.ts:771-837
Timestamp: 2025-09-12T13:28:43.782Z
Learning: All workflow API endpoints should be protected with AuthMiddleware to prevent unauthorized access to template creation, execution, tool management, and other sensitive workflow operations.
Applied to files:
server/api/workflow.ts
🧬 Code graph analysis (12)
server/db/schema/workflows.ts (1)
server/api/workflow.ts (1)
reviewSubmissionSchema(70-70)
server/api/script.ts (2)
server/logger/index.ts (2)
getLoggerWithChild(192-200)Subsystem(15-15)server/workflowScriptExecutorTool.ts (2)
ScriptLanguage(879-879)getCodeWritingBlock(881-891)
server/server.ts (3)
server/api/workflow.ts (4)
reviewSubmissionSchema(70-70)ReviewWorkflowStepApi(3991-4201)TriggerWorkflowStepApi(4203-4360)DownloadReviewFilesApi(4492-4701)server/db/schema/workflows.ts (1)
reviewSubmissionSchema(409-411)server/api/script.ts (1)
GetScriptLanguagesApi(10-33)
frontend/src/components/ScriptSidebar.tsx (2)
frontend/src/api.ts (1)
api(5-5)frontend/src/components/workflow/api/ApiHandlers.ts (2)
workflowToolsAPI(359-390)workflowStepsAPI(393-452)
frontend/src/components/workflow/api/ApiHandlers.ts (1)
frontend/src/api.ts (1)
api(5-5)
frontend/src/components/workflow/ReviewExecutionUI.tsx (2)
frontend/src/api.ts (1)
api(5-5)frontend/src/components/workflow/api/ApiHandlers.ts (1)
workflowToolsAPI(359-390)
frontend/src/routes/_authenticated/workflow.tsx (3)
frontend/src/components/workflow/Types.ts (1)
WorkflowTemplate(227-280)frontend/src/components/workflow/api/ApiHandlers.ts (2)
fetchWorkflows(194-198)userWorkflowsAPI(190-207)server/db/schema/workflows.ts (1)
workflowTemplate(63-87)
frontend/src/components/workflow/executedWorkflowRenderer.tsx (3)
frontend/src/components/workflow/Types.ts (2)
Step(64-92)Tool(56-61)server/db/schema/workflows.ts (1)
workflowTool(115-133)frontend/src/api.ts (1)
api(5-5)
server/workflowScriptExecutorTool.ts (1)
server/logger/index.ts (3)
getLoggerWithChild(192-200)Subsystem(15-15)getLogger(36-93)
server/api/workflow.ts (4)
server/db/user.ts (1)
getUserFromJWT(475-487)server/db/schema/workflows.ts (6)
workflowTemplate(63-87)workflowStepExecution(164-190)workflowTool(115-133)workflowStepTemplate(90-112)workflowExecution(136-161)toolExecution(193-210)server/services/emailService.ts (1)
emailService(124-124)server/workflowScriptExecutorTool.ts (2)
ScriptLanguage(879-879)executeScript(680-877)
frontend/src/components/workflow/TriggerExecutionUI.tsx (1)
frontend/src/api.ts (1)
api(5-5)
frontend/src/components/workflow/WorkflowBuilder.tsx (4)
frontend/src/components/workflow/PlusButton.tsx (1)
PlusButton(10-73)frontend/src/components/workflow/api/ApiHandlers.ts (1)
workflowExecutionsAPI(223-356)frontend/src/components/workflow/Types.ts (2)
WorkflowTemplate(227-280)Tool(56-61)frontend/src/components/ScriptSidebar.tsx (1)
ScriptSidebar(29-514)
🪛 dotenv-linter (4.0.0)
server/.env.default
[warning] 29-29: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 30-30: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 31-31: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 31-31: [UnorderedKey] The DB_PASSWORD key should go before the DB_USER key
(UnorderedKey)
[warning] 32-32: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 32-32: [UnorderedKey] The DB_HOST key should go before the DB_NAME key
(UnorderedKey)
[warning] 33-33: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 33-33: [UnorderedKey] The DB_PORT key should go before the DB_USER key
(UnorderedKey)
🔇 Additional comments (25)
deployment/portable/.env.default (1)
23-23: LGTM!The
USE_BUBBLEWRAP=truedefault enables sandboxing for script execution in portable deployments, which is a sensible security posture. This aligns with the broader Bubblewrap integration across the PR.frontend/package.json (1)
19-19: LGTM!The Monaco Editor dependencies are recent versions that will enable code editing functionality. The React wrapper (
@monaco-editor/react^4.6.0) is compatible with the core Monaco editor (monaco-editor^0.52.2).Also applies to: 52-52
Dockerfile (3)
1-44: LGTM! Well-structured dependency layer with proper caching.The first stage effectively separates system dependencies, language runtimes, and package installations into cached layers. The installation of Python, Node.js, R, and Bubblewrap supports the script execution capabilities being introduced.
Note: The
--break-system-packagesflag on line 42 is acceptable per established learnings for this repository.Based on learnings
97-98: LGTM! Runtime directory creation includes script execution support.The creation of the
script_executor_utilsdirectory supports the script execution workflow tooling being introduced in this PR.
43-43: Update npm installation to respect version constraints if versioning is required.The verification confirms the review concern: the current command installs only package names globally, ignoring version constraints from
js-requirements.json. Foraxios: "^1.5.0", npm will install the latest version instead.If version pinning is important for these global packages, update the installation to respect versions:
RUN npm install -g $(cat /usr/src/app/js-requirements.json | jq -r '.dependencies | to_entries[] | "\(.key)@\(.value)"')If global packages should always use latest versions, consider removing version constraints from
js-requirements.jsonto avoid confusion.server/.env.default (1)
24-25: LGTM! Bubblewrap support added with appropriate default.The
USE_BUBBLEWRAP=falsedefault for server development environments is reasonable, as Bubblewrap sandboxing may not be necessary in all deployment contexts. This can be overridden in production deployments where sandboxing is desired.server/api/script.ts (1)
10-33: LGTM! Clean API implementation with proper error handling.The endpoint correctly enumerates available script languages and generates code writing blocks for each. The error handling with contextual logging follows the established patterns in the codebase.
Minor note: Line 15 passes empty objects
{}for input and config togetCodeWritingBlock. Consider adding a comment explaining that these are default/empty values for the language template generation, to clarify the intent.server/server.ts (2)
240-241: LGTM! Proper imports for new workflow and script APIs.All new API handlers and schemas are correctly imported from their respective modules.
Also applies to: 245-245, 256-256, 259-259
1122-1135: LGTM! Workflow step routes properly configured.The new routes are well-structured:
- Review step endpoint validates input against
reviewSubmissionSchema- Trigger step endpoint (no validation suggests it's parameter-based only)
- Download endpoint supports both GET (all files) and POST (specific files) methods
- Script languages endpoint provides available language enumerations
All routes follow established patterns in the codebase.
deployment/portable/docker-compose.app-build.yml (1)
75-122: Verify concurrent volume access between app and app-sync services.The review concerns about port 3010 and the
/healthendpoint are resolved:
- Port 3010 doesn't conflict (app uses 3000, app-sync uses 3010)
/healthendpoint is implemented in sync-server.tsHowever, two concerns remain:
dev:sync command uses
--watchflag – The scriptbun run --watch sync-server.tsindicates development/watch mode, which may not be suitable for a production compose configuration. Verify whether this is intentional or should be changed to a production-ready equivalent without--watch.Shared volume access – Both app and app-sync mount the identical volume paths (app-uploads, app-logs, app-assets, app-migrations, app-downloads). While
app-syncdepends_onappwithservice_healthy, verify there are no race conditions when both services write to shared volumes concurrently (e.g., migrations, logs, downloads directories).server/db/schema/workflows.ts (1)
409-448: LGTM! Clean validation schema for review submissions.The new
reviewSubmissionSchemaandReviewSubmissionRequesttype are well-structured. The enum constraint properly limits review decisions to "approved" or "rejected", and the schema follows the established patterns in this file.frontend/src/components/workflow/PlusButton.tsx (1)
1-73: LGTM! Well-structured component for workflow node connections.The
PlusButtoncomponent cleanly handles React Flow integration with invisible functional handles and a visible connection point. The CustomEvent pattern for "openWhatHappensNext" is appropriate for cross-component communication in the workflow builder context.frontend/src/components/CodeEditor.tsx (1)
1-133: LGTM! Clean Monaco editor integration.The CodeEditor component is well-implemented with:
- Proper state synchronization between
initialValueandcurrentValue- Clean keyboard shortcut handling via
onKeyDownprop- Monaco editor with sensible defaults
- Good UX with modal overlay and save/close actions
frontend/src/components/workflow/WhatHappensNextUI.tsx (2)
1-34: LGTM! Correct prop handling.The removal of
selectedNodeIdfrom the destructuring (line 34) aligns with the interface definition, which doesn't include this prop. The added imports (React, Play) support the new actions being introduced.
49-88: LGTM! Well-structured new workflow actions.The three new actions (review, trigger, run_script) are properly structured with clear descriptions and appropriate icons. They follow the same pattern as existing actions and integrate cleanly with the available actions list.
frontend/src/components/workflow/api/ApiHandlers.ts (1)
194-206: LGTM! API functions correctly typed.The
fetchWorkflowsandfetchTemplateByIdfunctions are properly updated to useWorkflowTemplateSimplified. The type parameters forextractResponseDataare correct.frontend/src/components/ScriptSidebar.tsx (3)
1-94: LGTM! Clean state management and initialization.The component setup is well-structured with:
- Clear prop interfaces
- Proper state initialization
- Language fetching with error handling
- State reset on sidebar close
279-334: LGTM! Appropriate error handling for create flow.The create workflow step logic properly prioritizes visual updates (line 314) and gracefully degrades when API calls fail (lines 323-326). This is intentional for blank mode workflows where backend sync is optional.
336-514: LGTM! Clean UI implementation.The render logic is well-structured with:
- Proper conditional rendering for loading/error states
- Clean language selection dropdown
- Monaco editor integration for code and config
- Consistent styling with dark mode support
server/workflowScriptExecutorTool.ts (2)
36-77: Note on JSON escaping and JS key-quoting. No action.validateJson escape normalization and formatJavaScriptOutput regex are intentionally implemented with fallbacks; leaving as-is.
Based on learnings
Also applies to: 480-532
367-373: Input is coerced to string; scripts receive wrong types (double-JSON).validateJson returns a JSON string; passing that into getDataParser then JSON.stringify() double-quotes it. Parse back to object or skip stringify for object inputs.
- const validatedInput = validateJson(input) - const inputData = validatedInput || {} - const configData = config || {} - if (input && !validatedInput) { - return { success: false, error: "Invalid input: must be a valid JSON object" } - } + let inputData: Record<string, any> = {} + if (typeof input === 'string') { + const v = validateJson(input) + if (!v) return { success: false, error: "Invalid input: must be a valid JSON object" } + try { inputData = JSON.parse(v) } catch { return { success: false, error: "Invalid input JSON" } } + } else if (input && typeof input === 'object') { + inputData = input + } + + let configData: Record<string, any> = {} + if (typeof config === 'string') { + const v = validateJson(config) + if (!v) return { success: false, error: "Invalid config: must be a valid JSON object" } + try { configData = JSON.parse(v) } catch { return { success: false, error: "Invalid config JSON" } } + } else if (config && typeof config === 'object') { + configData = config + }⛔ Skipped due to learnings
Learnt from: debajyoti0606 PR: xynehq/xyne#1075 File: server/workflowScriptExecutorTool.ts:36-77 Timestamp: 2025-10-17T08:54:59.756Z Learning: In server/workflowScriptExecutorTool.ts, the validateJson function's escape sequence normalization logic (lines 41-52 that replace \n, \r, \t, \f, \b, \", and \\ before parsing) is intentionally designed to handle edge cases and should not be flagged as potentially corrupting valid JSON. The double-escaping behavior is expected.frontend/src/routes/_authenticated/workflow.tsx (1)
983-1001: LGTM! Reasonable defaults for display purposes.The conversion from execution data to
LocalWorkflowTemplateuses sensible defaults (userId: 0, workspaceId: 0, isPublic: false) for fields not available in execution data. Since this is used for display inExecutedWorkflowRendererand not for write operations, these defaults are acceptable.server/api/workflow.ts (3)
2163-2211: LGTM! Improved error handling with nullable return type.Changing the return type to
string | nulland returningnullfor error cases (instead of error messages) allows calling code to implement graceful fallbacks. This is well-utilized in the email tool (lines 2364-2382) with multiple fallback content paths.
4203-4360: Verify trigger step doesn't need prerequisite checks.
TriggerWorkflowStepApiallows completing a trigger step without checking if previous steps are complete, unlikeCompleteWorkflowStepExecutionApi(lines 3938-3962) which validates all previous steps are completed first.Is this intentional? If trigger steps are meant to be manual gates that can be triggered independently of workflow order, this is correct. Otherwise, consider adding the same prerequisite validation.
If prerequisite validation is needed, add this logic before line 4265:
// Check if all previous steps are completed const prevStepIds = currentStep.prevStepIds || [] if (prevStepIds.length > 0) { const allSteps = await db .select() .from(workflowStepExecution) .where(eq(workflowStepExecution.workflowExecutionId, currentStep.workflowExecutionId)) const previousSteps = allSteps.filter(step => prevStepIds.includes(step.workflowStepTemplateId) ) const incompletePrevSteps = previousSteps.filter(step => step.status !== WorkflowStatus.COMPLETED ) if (incompletePrevSteps.length > 0) { const incompleteStepNames = incompletePrevSteps.map(step => step.name).join(", ") throw new HTTPException(400, { message: `Cannot trigger step. Previous steps must be completed first: ${incompleteStepNames}`, }) } }
4606-4627: Good security checks for path traversal.The file path validation properly handles both absolute and relative paths, checking for
..sequences to prevent directory traversal attacks (lines 4621-4627). The additional check ensuring absolute paths contain theexecutionId(line 4615) adds another layer of protection.
Summary by CodeRabbit
Release Notes
New Features
Infrastructure Improvements