Conversation
…ocessing for webhook triggers
…sing for matching triggers
|
@drewradcliff is attempting to deploy a commit to the inbound Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughAdds a workflow automation feature: new Postgres schema, Drizzle ORM schema/migrations, Neon DB entrypoint and Drizzle config; server APIs for workflows, executions, execution logs, webhook handling, and schedule management integration; a Vercel-compatible workflow executor and step implementations (HTTP request, send email, trigger) with step logging and DB persistence; a type-safe API client; extensive client-side workflow UI (editor canvas, nodes, toolbar, runs, selector, context menu, panels) backed by a Jotai store and autosave; removes legacy schedules UI/actions; adds DB tooling, migrations, and updates project tooling and Prettier configuration. Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (19)
packages/web/components/ai-elements/canvas.tsx-1-29 (1)
1-29: Move @xyflow global CSS import to the root layoutImporting
@xyflow/react/dist/style.cssat the component level is not a best practice. In Next.js App Router, while component-level global CSS imports are technically allowed, best practice is to import third-party stylesheets once at the root layout (e.g.,app/layout.tsx) to ensure predictable loading and avoid potential cascade conflicts. If your project uses the Pages Router, this import would be disallowed outside ofpages/_app.js.Move this import to your app's root layout or global entry point and remove it from
Canvas.packages/web/components/workflow/config/condition-config.tsx-24-29 (1)
24-29: Fix inconsistency between placeholder and help text.The help text on Line 29 says "use @ to reference previous node outputs", but the placeholder on Line 24 shows
{{PreviousNode.value}}syntax. These don't match, which will confuse users.Apply this diff to align the help text with the placeholder syntax:
- use @ to reference previous node outputs. + use {{NodeLabel.field}} syntax to reference previous node outputs.packages/web/components/workflow/config/trigger-config.tsx-41-46 (1)
41-46: Add error handling for clipboard operations.The clipboard API can fail (user denied permission, insecure context, etc.). The current code doesn't handle potential errors.
Apply this diff to add error handling:
const handleCopyWebhookUrl = () => { if (webhookUrl) { - navigator.clipboard.writeText(webhookUrl) - toast.success("Webhook URL copied to clipboard") + navigator.clipboard + .writeText(webhookUrl) + .then(() => toast.success("Webhook URL copied to clipboard")) + .catch(() => toast.error("Failed to copy webhook URL")) } }packages/web/components/ai-elements/connection.tsx-19-26 (1)
19-26: Use theme-aware color instead of hardcoded white.The circle marker uses
fill="#fff"which doesn't adapt to dark mode. This creates poor contrast in dark themes.Apply this diff to use a theme-aware color:
cx={toX} cy={toY} - fill="#fff" + fill="var(--background)" r={3} stroke="var(--color-ring)" strokeWidth={1}packages/web/lib/api-client.ts-116-131 (1)
116-131: Type issue:Datefields will be strings after JSON parsing.The
startedAtandcompletedAtfields are typed asDatebut will be ISO strings after JSON parsing. Consider usingstringtype or parsing dates in the response.getExecutions: (id: string) => apiCall< Array<{ id: string; workflowId: string; userId: string; status: string; input: unknown; output: unknown; error: string | null; - startedAt: Date; - completedAt: Date | null; + startedAt: string; + completedAt: string | null; duration: string | null; }> >(`/api/workflows/${id}/executions`),packages/web/lib/api-client.ts-36-53 (1)
36-53: Potential issue with empty response bodies.The
apiCallfunction callsresponse.json()unconditionally, which will throw on 204 No Content or other empty responses. Consider handling empty responses.async function apiCall<T>(endpoint: string, options?: RequestInit): Promise<T> { const response = await fetch(endpoint, { ...options, headers: { "Content-Type": "application/json", ...options?.headers, }, }); if (!response.ok) { const error = await response .json() .catch(() => ({ error: "Unknown error" })); throw new ApiError(response.status, error.error || "Request failed"); } + // Handle empty responses (204 No Content) + const contentLength = response.headers.get("content-length"); + if (response.status === 204 || contentLength === "0") { + return undefined as T; + } + return response.json(); }packages/web/app/api/workflow-log/route.ts-70-72 (1)
70-72: Silent success on missinglogIdmay mask issues.Returning
{ success: true }whenlogIdis missing could hide bugs where the caller failed to pass the ID. Consider logging a warning or returning a distinct response to aid debugging.packages/web/app/workflows/[id]/page.tsx-80-82 (1)
80-82: No user feedback on fetch errors.When the workflow fetch fails (non-404), the error is only logged to the console. The user sees no indication that something went wrong. Consider adding an error state to display a message or retry option.
packages/web/components/workflow/workflow-runs.tsx-162-162 (1)
162-162: Removekeyprop from component's root element.The
key={log.id}on line 162 is inside the component definition. Thekeyprop should be applied by the parent when rendering this component in a list (which is correctly done on line 644), not on the component's internal root element.- <div className="relative flex gap-3" key={log.id}> + <div className="relative flex gap-3">packages/web/components/workflow/config/action-config.tsx-91-96 (1)
91-96: Potential infinite loop ifonUpdateConfigis not stable.The
useEffectdepends ononUpdateConfig. If the parent component doesn't memoize this callback (e.g., withuseCallback), a new function reference on each render will trigger this effect repeatedly, potentially causing an infinite loop of state updates.Consider removing
onUpdateConfigfrom dependencies by using a ref pattern, or ensure the parent memoizes the callback:+import { useRef } from "react" + function HttpRequestFields({ config, onUpdateConfig, disabled, }: {...}) { + const onUpdateConfigRef = useRef(onUpdateConfig) + onUpdateConfigRef.current = onUpdateConfig + useEffect(() => { if (!config?.httpMethod) { - onUpdateConfig("httpMethod", "POST") + onUpdateConfigRef.current("httpMethod", "POST") } - }, [config?.httpMethod, onUpdateConfig]) + }, [config?.httpMethod])Committable suggestion skipped: line range outside the PR's diff.
packages/web/app/api/workflows/[id]/route.ts-67-87 (1)
67-87: Returned workflow may have stalescheduleId.The workflow is returned from the database update on line 67-71, but
createOrUpdateWorkflowSchedule(lines 75-84) updates thescheduleIdin a separate database call. The response will contain the oldscheduleIdvalue, not the newly created one.Consider re-fetching the workflow after schedule management, or returning the
scheduleIdfrom the schedule result:if (nodes !== undefined) { const scheduleResult = await createOrUpdateWorkflowSchedule( id, nodes, existingWorkflow.scheduleId, ) if (!scheduleResult.success) { console.error("Failed to manage schedule:", scheduleResult.error) - // Don't fail the whole request, just log the error + } else if (scheduleResult.scheduleId !== undefined) { + workflow.scheduleId = scheduleResult.scheduleId } }packages/web/components/workflow/node-config-panel.tsx-170-192 (1)
170-192: Missing response status check inhandleDeleteAllRuns.The function doesn't check
response.okbefore treating the operation as successful. A non-2xx response would still execute the success path.const handleDeleteAllRuns = async () => { if (!currentWorkflowId) { return } try { const response = await fetch(`/api/workflows/${currentWorkflowId}/executions`, { method: "DELETE", }) + + if (!response.ok) { + const error = await response.json() + throw new Error(error.message || "Failed to delete runs") + } + clearNodeStatuses() setShowDeleteRunsAlert(false) // Refresh the runs list if (refreshRunsRef.current) { await refreshRunsRef.current() } toast.success("All runs deleted") } catch (error) {packages/web/app/actions/workflow-schedules.ts-97-125 (1)
97-125: Delete-then-create pattern may leave workflow without a schedule on failure.If deleting the existing schedule succeeds but creating the new one fails, the workflow will be left without any schedule. Consider handling this more gracefully, or updating the workflow's
scheduleIdtonullin the catch block.} catch (error) { console.error("Error creating schedule:", error) + // Clear scheduleId since old schedule was deleted but new one failed + if (existingScheduleId) { + await db + .update(workflows) + .set({ scheduleId: null, updatedAt: new Date() }) + .where(eq(workflows.id, workflowId)) + } return { success: false, error: "Failed to create schedule. Please check your cron expression.", } }Committable suggestion skipped: line range outside the PR's diff.
packages/web/components/workflow/node-config-panel.tsx-603-613 (1)
603-613: Delete button shown for trigger nodes but deletion is prevented by store.Based on the
deleteNodeAtomlogic in workflow-store.ts (relevant_code_snippets), trigger nodes cannot be deleted. However, this UI shows a delete button for trigger nodes, which would confuse users when clicking it does nothing.Consider hiding the delete button for trigger nodes:
- {selectedNode.data.type === "trigger" && ( - <div className="shrink-0 border-t p-4"> - <Button - onClick={() => setShowDeleteNodeAlert(true)} - size="icon" - variant="ghost" - > - <Trash2 className="size-4" /> - </Button> - </div> - )}Or update
deleteNodeAtomto allow trigger deletion if that's the intended behavior.Committable suggestion skipped: line range outside the PR's diff.
packages/web/components/workflow/workflow-canvas.tsx-370-370 (1)
370-370: Potential stale closure onedges.The
setEdges([...edges, newEdge])capturesedgesat the time the callback was created. If edges change betweenonConnectStartandonConnectEnd, this could lose concurrent edge changes. Use the functional update form instead.-setEdges([...edges, newEdge]); +setEdges((currentEdges) => [...currentEdges, newEdge]);packages/web/components/workflow/workflow-canvas.tsx-234-234 (1)
234-234: Same stale closure issue withedges.Use functional update to avoid losing concurrent edge changes.
-setEdges([...edges, newEdge]); +setEdges((currentEdges) => [...currentEdges, newEdge]);packages/web/lib/workflow-executor.workflow.ts-371-371 (1)
371-371:Object.values(results).at(-1)may not reflect meaningful ordering.Object iteration order in JavaScript is insertion order, which depends on execution timing of parallel nodes. The "last" result may not be the final step in the workflow DAG.
Consider collecting outputs from leaf nodes (nodes with no outgoing edges) instead, or passing the trigger nodes' results explicitly:
-output: Object.values(results).at(-1)?.data, +output: results, // or extract specific terminal node outputsCommittable suggestion skipped: line range outside the PR's diff.
packages/web/lib/workflow-executor.workflow.ts-156-169 (1)
156-169: Missing validation for required fields before type casting.If
processedConfig.endpointisundefined, theas stringcast will passundefinedtohttpRequestStep, which may cause unexpected behavior. Consider validating required fields.case "HTTP Request": { + const endpoint = processedConfig.endpoint as string | undefined + if (!endpoint) { + return { + success: false, + error: "HTTP Request requires an endpoint URL", + } + } const result = await httpRequestStep({ - endpoint: processedConfig.endpoint as string, + endpoint, httpMethod: (processedConfig.httpMethod as string) || "POST",packages/web/lib/workflow-executor.workflow.ts-170-182 (1)
170-182: Same validation concern for Send Email.
emailToandemailSubjectare required but cast without validation.case "Send Email": { + const emailTo = processedConfig.emailTo as string | undefined + const emailSubject = processedConfig.emailSubject as string | undefined + if (!emailTo || !emailSubject) { + return { + success: false, + error: "Send Email requires 'to' and 'subject' fields", + } + } const result = await sendEmailStep({ - emailTo: processedConfig.emailTo as string, - emailSubject: processedConfig.emailSubject as string, + emailTo, + emailSubject,
🧹 Nitpick comments (46)
packages/web/app/globals.css (1)
1-246: Heavy reliance on custom CSS conflicts with stated styling preference; however, much is justified for React Flow.While the codebase prefers Tailwind CSS over custom CSS, this file deviates significantly—especially lines 163–232 with extensive React Flow overrides using
!importantflags. This is largely justified, as third-party libraries often require custom CSS to override their internals, and animations likedashdraw(lines 154–161) lack Tailwind equivalents.That said, the approach is acceptable given the constraints. If possible, consider:
- Consolidating the two
@layer baseblocks (lines 115–121 and 128–152) into one for cleanliness.- Verifying that all React Flow styling (lines 163–232) cannot be achieved via React Flow configuration or reduced scope.
If you'd like, I can help verify whether the React Flow styling can be reduced or if there are configuration-level alternatives. Consolidating the
@layer baseblocks would be straightforward:@layer base { * { @apply border-border outline-ring/50; } body { @apply bg-background text-foreground; } html, body { @apply overflow-hidden; } + ::after, + ::before, + ::backdrop, + ::file-selector-button { + @apply border-border; + } + * { + @apply min-w-0; + } + html { + text-rendering: optimizelegibility; + } + body { + @apply min-h-dvh; + } + input::placeholder, + textarea::placeholder { + @apply text-muted-foreground; + } + button:not(:disabled), + [role="button"]:not(:disabled) { + @apply cursor-pointer; + } } -@layer base { - ::after, - ::before, - ::backdrop, - ::file-selector-button { - @apply border-border; - } - * { - @apply min-w-0; - } - html { - text-rendering: optimizelegibility; - } - body { - @apply min-h-dvh; - } - input::placeholder, - textarea::placeholder { - @apply text-muted-foreground; - } - button:not(:disabled), - [role="button"]:not(:disabled) { - @apply cursor-pointer; - } -}packages/web/components/ui/spinner.tsx (1)
5-14: Well-implemented loading spinner with good accessibility.The component is clean and functional with proper accessibility attributes (
role="status"andaria-label). The use of Tailwind classes and props forwarding provides good flexibility.As an optional enhancement, consider making the
aria-labelconfigurable for internationalization or custom contexts:-function Spinner({ className, ...props }: React.ComponentProps<"svg">) { +function Spinner({ + className, + "aria-label": ariaLabel = "Loading", + ...props +}: React.ComponentProps<"svg">) { return ( <Loader2Icon role="status" - aria-label="Loading" + aria-label={ariaLabel} className={cn("size-4 animate-spin", className)} {...props} /> ); }packages/web/components/workflow/nodes/trigger-node.tsx (1)
14-27: TightenTriggerNodePropstyping to use the genericNodePropsshape directlyTo better align with
@xyflow/react’s API and avoid intersecting over thedatafield, consider defining the props via the generic type instead ofNodeProps & { data?: WorkflowNodeData }. That keepsdatastrongly typed and avoids any subtle type merging of thedataproperty:-import type { NodeProps } from "@xyflow/react"; +import type { NodeProps as ReactFlowNodeProps } from "@xyflow/react"; @@ -type TriggerNodeProps = NodeProps & { - data?: WorkflowNodeData; -}; +type TriggerNodeProps = ReactFlowNodeProps<WorkflowNodeData>;With this,
datawill already beWorkflowNodeData, and you can rely on the library’s guarantees about its presence (you can keep the runtimeif (!data) return nullif you want the extra safety).packages/web/package.json (1)
5-59: DB tooling scripts and dependencies are wired correctly; consider scoping dotenvThe new
db:*scripts and Drizzle-related deps look consistent withdrizzle.config.tsand Neon/Postgres usage. Sincedotenvappears to be used only in CLI/tooling (e.g.,drizzle.config.ts), you might consider moving it todevDependenciesto keep the runtime dep surface minimal.packages/web/components/ui/animated-border.tsx (1)
1-79: Prefer Tailwind-based utilities over styled-jsx for the animated borderThe visual implementation is nice, but the inline
style jsxblock introduces a custom styling mechanism where our guidelines call for Tailwind-based styling. Consider moving the@property,@keyframes, and.animate-border-maskrules into your Tailwind setup (e.g.,@layer utilitiesin the main CSS) and then reference them via utility classes (e.g.,animate-[border-rotate]or a customborder-animateclass) on the wrapper divs.This keeps all component styling aligned with the Tailwind-first approach while preserving the animation. Based on learnings, this file falls under the “use Tailwind CSS instead of custom CSS” rule.
packages/web/components/workflow/config/action-grid.tsx (1)
17-32: Consider externalizing the actions configuration.The actions array is hardcoded in this component. As the number of available actions grows, consider moving this to a separate configuration file or constant for better maintainability and potential reuse.
For example, create
packages/web/lib/workflow-actions.ts:import { Mail, Zap } from "lucide-react"; export const WORKFLOW_ACTIONS = [ { id: "HTTP Request", label: "HTTP Request", description: "Make an HTTP request to any API", category: "System", icon: Zap, }, { id: "Send Email", label: "Send Email", description: "Send an email via Inbound", category: "Inbound", icon: Mail, }, ] as const;Then import and use it in this component.
packages/web/components/ui/tabs.tsx (1)
44-47: Consider breaking up the long className string for readability.Line 45 has a very long className string that's difficult to read and maintain. Consider breaking it into logical groups.
className={cn( - "data-[state=active]:bg-background dark:data-[state=active]:text-foreground focus-visible:border-ring focus-visible:ring-ring/50 focus-visible:outline-ring dark:data-[state=active]:border-input dark:data-[state=active]:bg-input/30 text-foreground dark:text-muted-foreground inline-flex h-[calc(100%-1px)] flex-1 items-center justify-center gap-1.5 rounded-md border border-transparent px-2 py-1 text-sm font-medium whitespace-nowrap transition-[color,box-shadow] focus-visible:ring-[3px] focus-visible:outline-1 disabled:pointer-events-none disabled:opacity-50 data-[state=active]:shadow-sm [&_svg]:pointer-events-none [&_svg]:shrink-0 [&_svg:not([class*='size-'])]:size-4", + "inline-flex h-[calc(100%-1px)] flex-1 items-center justify-center gap-1.5", + "rounded-md border border-transparent px-2 py-1 text-sm font-medium whitespace-nowrap", + "text-foreground dark:text-muted-foreground", + "transition-[color,box-shadow]", + "data-[state=active]:bg-background data-[state=active]:shadow-sm", + "dark:data-[state=active]:text-foreground dark:data-[state=active]:border-input dark:data-[state=active]:bg-input/30", + "focus-visible:border-ring focus-visible:ring-ring/50 focus-visible:outline-ring focus-visible:ring-[3px] focus-visible:outline-1", + "disabled:pointer-events-none disabled:opacity-50", + "[&_svg]:pointer-events-none [&_svg]:shrink-0 [&_svg:not([class*='size-'])]:size-4", className )}packages/web/components/workflow/nodes/action-node.tsx (1)
54-84: Consider centralizing action metadata.Both
getProviderLogoandgetIntegrationFromActionTypecontain hardcoded mappings that duplicate information fromaction-grid.tsx(Lines 17-32). Consider creating a shared configuration that includes icon, label, description, category, and integration name for each action type.This would improve maintainability and ensure consistency across the UI. You could create a shared
packages/web/lib/workflow-actions.tsmodule that both components import from.Example structure:
export const WORKFLOW_ACTIONS_CONFIG = { "Send Email": { label: "Send Email", description: "Send an email via Inbound", category: "Inbound", integration: "Inbound", icon: Mail, iconColor: "text-purple-400", }, // ... other actions };packages/web/components/ui/button-group.tsx (1)
7-22: Note: Complex CSS selectors for maintainability.The
buttonGroupVariantsuses complex CSS selectors (e.g.,[&>*]:focus-visible:z-10,has-[select[aria-hidden=true]:last-child]:[&>[data-slot=select-trigger]:last-of-type]:rounded-r-md). While functional, these can be fragile if the DOM structure changes. Consider documenting the expected structure or simplifying if possible in future iterations.packages/web/lib/steps/send-email.ts (1)
10-21: Consider exportingSendEmailResulttype for consumers.
SendEmailInputis exported butSendEmailResultis not. Consumers ofsendEmailStepmay need to type the return value. Consider exportingSendEmailResultfor consistency.-type SendEmailResult = +export type SendEmailResult = | { success: true data: { id?: string; messageId?: string; status: string } } | { success: false; error: string }packages/web/components/workflow/config/schema-builder.tsx (1)
186-208: Consider adding a recursion depth limit for nested schemas.The component recursively renders
SchemaBuilderfor nested objects and array items without a depth limit. Deeply nested schemas could cause performance issues or stack overflow.+const MAX_NESTING_DEPTH = 5; + export function SchemaBuilder({ schema, onChange, disabled, level = 0, }: SchemaBuilderProps) { + const isMaxDepth = level >= MAX_NESTING_DEPTH; + // ... existing code ... {field.type === "object" && ( - <div className="mt-2"> + <div className="mt-2"> + {isMaxDepth ? ( + <p className="text-sm text-muted-foreground">Maximum nesting depth reached</p> + ) : ( <Label className="mb-2 block">Object Properties</Label> <SchemaBuilder disabled={disabled} level={level + 1} onChange={(fields) => updateNestedFields(index, fields)} schema={field.fields || []} /> + )} </div> )}packages/web/components/ui/alert-dialog.tsx (1)
121-143: Minor inconsistency:AlertDialogActionandAlertDialogCancelare missingdata-slotattributes.Other components in this file include
data-slotattributes for composition hooks. Consider adding them for consistency.function AlertDialogAction({ className, ...props }: React.ComponentProps<typeof AlertDialogPrimitive.Action>) { return ( <AlertDialogPrimitive.Action + data-slot="alert-dialog-action" className={cn(buttonVariants(), className)} {...props} /> ) } function AlertDialogCancel({ className, ...props }: React.ComponentProps<typeof AlertDialogPrimitive.Cancel>) { return ( <AlertDialogPrimitive.Cancel + data-slot="alert-dialog-cancel" className={cn(buttonVariants({ variant: "outline" }), className)} {...props} /> ) }packages/web/lib/api-client.ts (1)
207-231: Inconsistent return type forautoSaveWorkflow.The function returns
Promise<SavedWorkflow> | undefined, which makes it difficult for callers to handle. Consider always returning a Promise or void for consistency.autoSaveWorkflow: (() => { let autosaveTimeout: NodeJS.Timeout | null = null; const AUTOSAVE_DELAY = 2000; return ( id: string, data: Partial<WorkflowData>, debounce = true - ): Promise<SavedWorkflow> | undefined => { + ): Promise<SavedWorkflow | void> => { if (!debounce) { return workflowApi.update(id, data); } if (autosaveTimeout) { clearTimeout(autosaveTimeout); } - autosaveTimeout = setTimeout(() => { - workflowApi.update(id, data).catch((error) => { - console.error("Auto-save failed:", error); - }); - }, AUTOSAVE_DELAY); + return new Promise((resolve) => { + autosaveTimeout = setTimeout(() => { + workflowApi.update(id, data) + .then(() => resolve()) + .catch((error) => { + console.error("Auto-save failed:", error); + resolve(); + }); + }, AUTOSAVE_DELAY); + }); }; })(),packages/web/components/ai-elements/controls.tsx (2)
26-28: Prefer functional updater when toggling Jotai stateUsing the previous value avoids stale-closure issues if this handler is ever reused in more complex flows:
- const handleToggleMinimap = () => { - setShowMinimap(!showMinimap); - }; + const handleToggleMinimap = () => { + setShowMinimap((prev) => !prev); + };
30-71: Improve accessibility of icon-only buttons and reduce class duplicationAll four controls are icon-only and rely on
title; consider addingaria-label(or visually hidden text) for better screen reader support, e.g.aria-label="Zoom in". Also, the longclassNamestring is duplicated across all buttons; extracting it into a shared constant would reduce repetition and keep future tweaks in one place.packages/web/components/workflow/workflow-context-menu.tsx (1)
58-81: Avoid hardcoding node height when positioning new steps
handleAddStepuses a hardcodednodeHeight = 192to vertically center the new node. If the actual node height changes in the renderer, this will drift.Consider exporting a shared
NODE_HEIGHT(or similar) from the node component module, or at least a shared workflow constants module, and importing it here so pane context placement stays in sync with the rendered node size.packages/web/drizzle/0002_add_schedule_id.sql (1)
1-2: Confirm whetherschedule_idshould be indexed/uniqueIf
schedule_idis used to look up workflows from an external scheduler (QStash, etc.), you may want a (partial) unique index for integrity and performance, e.g.:CREATE UNIQUE INDEX "workflows_schedule_id_idx" ON "workflows" ("schedule_id") WHERE "schedule_id" IS NOT NULL;If multiple workflows can legitimately share a schedule, then the current nullable text column is fine—just confirm the intended cardinality.
packages/web/drizzle/0001_amused_power_man.sql (1)
1-5: Consider enforcing uniqueness onwebhook_idIf
webhook_idis used to route incoming webhooks to a single workflow (e.g./api/webhook/[webhookId]), a unique (or at least indexed) column will help both correctness and lookup performance, for example:CREATE UNIQUE INDEX "workflows_webhook_id_idx" ON "workflows" ("webhook_id") WHERE "webhook_id" IS NOT NULL;If the design allows multiple workflows per webhook, then leaving it as a non-unique nullable text column is fine—just double-check the intended mapping.
packages/web/app/workflows/page.tsx (1)
6-24: Redirect logic is correct; consider limiting the query to a single rowThe page correctly redirects to the most recently updated workflow and shows a clean empty state, but you only ever use
allWorkflows[0]. You can reduce DB load by adding.limit(1)and naming the resultlatestWorkflowto reflect intent.packages/web/app/workflows/create-workflow-button.tsx (1)
21-51: Creation flow is solid; consider minor UX and ID robustness tweaksThe create flow (loading state, default trigger node, navigation on success, finally block) is well-structured. Two minor thoughts:
- On failure you only
console.error; consider surfacing a toast/alert so users know creation failed.- For the default node ID,
Date.now()is probably fine, but if you already have a UUID helper elsewhere, using it would avoid any theoretical collisions and keep IDs consistent.As per coding guidelines, importing
Buttonfrom"@/components/ui/button"is exactly what we want.packages/web/drizzle/meta/0001_snapshot.json (1)
6-262: Consider adding indexes for common workflow/execution access patternsThe schema captures the workflow model well, but there are no secondary indexes. Given the APIs in this PR, you frequently query:
workflow_executionsbyworkflow_idordered bystarted_atworkflow_execution_logsbyexecution_id(and often ordered bystarted_at)At scale, B-tree indexes on
(workflow_id, started_at)and(execution_id, started_at)would significantly improve query performance. These should be added via a new migration (which will in turn update future snapshots), not by editing this JSON directly.packages/web/app/workflows/[id]/page.tsx (1)
52-59: JSON parsing may throw on malformed data.If
workflow.nodesorworkflow.edgesare malformed strings,JSON.parsewill throw an unhandled exception, causing the entireloadWorkflowfunction to fail silently (only logged, not displayed).Consider wrapping in try-catch or validating the shape:
- const parsedNodes: WorkflowNode[] = - typeof workflow.nodes === "string" - ? JSON.parse(workflow.nodes) - : workflow.nodes || [] + let parsedNodes: WorkflowNode[] = [] + try { + parsedNodes = + typeof workflow.nodes === "string" + ? JSON.parse(workflow.nodes) + : workflow.nodes || [] + } catch { + console.error("Failed to parse workflow nodes") + }packages/web/components/workflow/workflow-selector.tsx (2)
118-130: Button insideSelectContentmay cause accessibility issues.Placing a
ButtoninsideSelectContentbreaks expected Select behavior—keyboard navigation may not work correctly, and screen readers may not announce it properly. Consider usingSelectItemwith a special value or moving the "New Workflow" action outside the dropdown.- <Button - variant="ghost" - className="w-full justify-start gap-2 px-2 py-1.5 text-sm font-normal" - onClick={handleCreateWorkflow} - disabled={isCreating} - > - {isCreating ? ( - <Loader2 className="size-4 animate-spin" /> - ) : ( - <Plus className="size-4" /> - )} - New Workflow - </Button> + <SelectItem value="new" disabled={isCreating}> + <span className="flex items-center gap-2"> + {isCreating ? ( + <Loader2 className="size-4 animate-spin" /> + ) : ( + <Plus className="size-4" /> + )} + New Workflow + </span> + </SelectItem>
65-77: Consider using UUID for node ID instead ofDate.now().
Date.now()could produce duplicate IDs if called rapidly. The codebase already usesuuidv4elsewhere—consider usingcrypto.randomUUID()for consistency and guaranteed uniqueness.packages/web/app/api/workflows/executions/[executionId]/logs/route.ts (1)
17-43: Consider combining queries to reduce database round trips.Three separate queries are made sequentially. While functional, this could be optimized using a join or Drizzle's relational queries to fetch execution + workflow + logs in fewer round trips.
packages/web/app/api/workflows/[id]/route.ts (2)
43-65: Consider validatingstatusfield against allowed values.The
statusfield is accepted directly from the request body without validation. Per the schema, valid values are"draft" | "active" | "paused". Invalid values could be persisted to the database.+const VALID_STATUSES = ["draft", "active", "paused"] as const + const updateData: Record<string, unknown> = { updatedAt: new Date(), } if (name !== undefined) updateData.name = name if (description !== undefined) updateData.description = description if (nodes !== undefined) updateData.nodes = nodes if (edges !== undefined) updateData.edges = edges -if (status !== undefined) updateData.status = status +if (status !== undefined) { + if (!VALID_STATUSES.includes(status)) { + return NextResponse.json({ error: "Invalid status value" }, { status: 400 }) + } + updateData.status = status +}
119-129: Unnecessary dynamic import.
deleteWorkflowSchedulecan be statically imported at the top of the file alongsidecreateOrUpdateWorkflowSchedule, avoiding the runtime import overhead.-import { createOrUpdateWorkflowSchedule } from "@/app/actions/workflow-schedules" +import { createOrUpdateWorkflowSchedule, deleteWorkflowSchedule } from "@/app/actions/workflow-schedules"Then simplify the deletion:
if (existingWorkflow.scheduleId) { try { - const { deleteWorkflowSchedule } = await import( - "@/app/actions/workflow-schedules" - ) await deleteWorkflowSchedule(existingWorkflow.scheduleId) } catch (error) { console.error("Failed to delete schedule:", error) } }packages/web/lib/steps/http-request.ts (2)
20-29: Silent failure on malformed JSON headers.
parseHeadersreturns an empty object when JSON parsing fails, which silently ignores user-provided headers. Consider logging a warning or including this in the result to aid debugging.
46-52: Potential unhandled JSON parse error in response.If the server returns
Content-Type: application/jsonbut the body is malformed,response.json()will throw. While caught by the outer try-catch, the error message won't indicate it was a JSON parsing issue.async function parseResponse(response: Response): Promise<unknown> { const contentType = response.headers.get("content-type") if (contentType?.includes("application/json")) { - return response.json() + try { + return await response.json() + } catch { + return response.text() + } } return response.text() }packages/web/components/workflow/config/action-config.tsx (2)
132-154: Consider lazy loading Monaco Editor.Monaco Editor is a heavy dependency that can significantly impact initial bundle size. Consider using dynamic imports with
next/dynamicto lazy-load the editor.import dynamic from 'next/dynamic' const Editor = dynamic(() => import('@monaco-editor/react'), { ssr: false, loading: () => <div className="h-[120px] animate-pulse bg-muted rounded-md" /> })
219-220: Unused atoms can be removed.
currentWorkflowIdAtomandcurrentWorkflowNameAtomare imported and destructured but the values (_workflowId,_workflowName) are not used anywhere in this component. Consider removing these unused imports to keep the code clean.-import { - currentWorkflowIdAtom, - currentWorkflowNameAtom, -} from "@/lib/workflow-store" -import { useAtom } from "jotai" ... export function ActionConfig({ config, onUpdateConfig, disabled, }: ActionConfigProps) { - const [_workflowId] = useAtom(currentWorkflowIdAtom) - const [_workflowName] = useAtom(currentWorkflowNameAtom)packages/web/components/workflow/workflow-runs.tsx (4)
57-59: Redundantnew Date()wrapper.The parameter
dateis already typed asDate, so wrapping it withnew Date(date)is unnecessary.function getRelativeTime(date: Date): string { const now = new Date() - const diff = now.getTime() - new Date(date).getTime() + const diff = now.getTime() - date.getTime()
119-120: Minor: setTimeout cleanup on unmount.The timeout at line 120 isn't cleaned up if the component unmounts before 2 seconds. While React 18+ handles this gracefully, cleaning up is a good practice.
+import { useRef, useEffect } from "react" + function CopyButton({ data, isError = false }: {...}) { const [copied, setCopied] = useState(false) + const timeoutRef = useRef<NodeJS.Timeout>() + + useEffect(() => { + return () => { + if (timeoutRef.current) clearTimeout(timeoutRef.current) + } + }, []) const handleCopy = async (e: React.MouseEvent) => { e.stopPropagation() try { const text = isError ? String(data) : JSON.stringify(data, null, 2) await navigator.clipboard.writeText(text) setCopied(true) - setTimeout(() => setCopied(false), 2000) + timeoutRef.current = setTimeout(() => setCopied(false), 2000) } catch (error) {
446-449: Sequential log refresh could be parallelized.Refreshing logs for expanded runs sequentially with
awaitinside the loop delays polling completion. Consider usingPromise.allfor parallel execution.// Also refresh logs for expanded runs - for (const executionId of expandedRuns) { - await refreshExecutionLogs(executionId) - } + await Promise.all( + Array.from(expandedRuns).map((executionId) => + refreshExecutionLogs(executionId) + ) + )
455-457: Polling interval restarts on every expansion toggle.
expandedRunsin the dependency array causes the interval to be cleared and recreated whenever a run is expanded/collapsed, potentially causing polling gaps. Consider using a ref to access the current expanded runs inside the polling callback.+const expandedRunsRef = useRef(expandedRuns) +expandedRunsRef.current = expandedRuns + useEffect(() => { if (!(isActive && currentWorkflowId)) { return } const pollExecutions = async () => { try { const data = await api.workflow.getExecutions(currentWorkflowId) setExecutions(data as WorkflowExecution[]) - for (const executionId of expandedRuns) { + for (const executionId of expandedRunsRef.current) { await refreshExecutionLogs(executionId) } } catch (error) { console.error("Failed to poll executions:", error) } } const interval = setInterval(pollExecutions, 2000) return () => clearInterval(interval) -}, [isActive, currentWorkflowId, expandedRuns, refreshExecutionLogs]) +}, [isActive, currentWorkflowId, refreshExecutionLogs])packages/web/app/actions/workflow-schedules.ts (2)
8-20: Consider reusingWorkflowNodetype from workflow-store.The
WorkflowNodetype defined here duplicates the type already exported from@/lib/workflow-store. Based on relevant_code_snippets,WorkflowNodeis defined at line 16 ofpackages/web/lib/workflow-store.ts. Consider importing it to maintain consistency and avoid divergence."use server" import { db } from "@/db" import { workflows } from "@/db/schema" import { qstash } from "@/lib/qstash" +import type { WorkflowNode } from "@/lib/workflow-store" import { eq } from "drizzle-orm" type TriggerConfig = { triggerType?: string scheduleCron?: string scheduleTimezone?: string } - -type WorkflowNode = { - id: string - data: { - type: string - config?: TriggerConfig - } -}
85-92: Cron validation is minimal and may allow invalid expressions.The validation only checks if the cron string has at least 5 characters, which doesn't validate the actual cron syntax. Invalid cron expressions will fail at QStash creation time, but this could be caught earlier with proper validation.
Consider using a cron validation library or regex for better UX:
// Validate cron expression const cron = triggerConfig.scheduleCron - if (!cron || cron.trim().length < 5) { + if (!cron || cron.trim() === "") { return { success: false, error: "Cron expression is required for scheduled workflows", } } + + // Basic cron format validation (5 or 6 fields) + const cronParts = cron.trim().split(/\s+/) + if (cronParts.length < 5 || cronParts.length > 6) { + return { + success: false, + error: "Invalid cron expression format. Expected 5 or 6 space-separated fields.", + } + }packages/web/components/workflow/workflow-toolbar.tsx (1)
120-127: Potential stale closure issue withnodesreference.The
updateNodesStatusfunction capturesnodesfrom the component's render scope. If called asynchronously (e.g., in error handler at line 233),nodesmight be stale. Consider reading nodes directly inside the atom or passing it as a parameter.packages/web/db/schema.ts (1)
51-52: Consider using numeric type fordurationMs.
durationMsis stored astext, which prevents numeric operations at the database level (e.g.,AVG,SUM, sorting). Consider usingintegerorbigintfor better queryability.// Duration in milliseconds - durationMs: text("duration_ms"), + durationMs: integer("duration_ms"),Note: This would require a migration if there's existing data.
packages/web/components/workflow/node-config-panel.tsx (1)
213-229: Optimistic update without rollback on failure.
handleUpdateWorkspaceNamesets the local name immediately but doesn't revert it if the API call fails. This could leave the UI showing a name that wasn't persisted.const handleUpdateWorkspaceName = async (newName: string) => { + const previousName = currentWorkflowName setCurrentWorkflowName(newName) // Save to database if workflow exists if (currentWorkflowId) { try { await fetch(`/api/workflows/${currentWorkflowId}`, { method: "PATCH", headers: { "Content-Type": "application/json" }, body: JSON.stringify({ name: newName, nodes, edges }), }) } catch (error) { console.error("Failed to update workflow name:", error) + setCurrentWorkflowName(previousName) // Revert on error toast.error("Failed to update workspace name") } } }packages/web/drizzle/0000_mixed_peter_parker.sql (1)
1-40: Consider adding indexes on foreign key columns for query performance.The foreign key columns
execution_id(inworkflow_execution_logs) andworkflow_id(inworkflow_executions) will be frequently used in JOIN queries and lookups. Adding indexes would improve query performance, especially as data grows.Consider adding these indexes in a follow-up migration:
CREATE INDEX idx_workflow_executions_workflow_id ON workflow_executions(workflow_id); CREATE INDEX idx_workflow_execution_logs_execution_id ON workflow_execution_logs(execution_id);Note: PostgreSQL automatically creates indexes for PRIMARY KEY columns, but not for foreign key columns.
packages/web/components/workflow/workflow-canvas.tsx (4)
121-146: Consider extracting sidebar width constant.The
0.3(30%) sidebar width is hardcoded here and likely duplicated elsewhere. If the sidebar width changes, this logic would break silently.+// Should match the sidebar width in your layout/CSS +const DEFAULT_SIDEBAR_WIDTH_PERCENT = 0.3; + useEffect(() => { if (isTransitioningFromHomepage && !hasPreShiftedRef.current) { // ... - const defaultSidebarPercent = 0.3; + const defaultSidebarPercent = DEFAULT_SIDEBAR_WIDTH_PERCENT;
165-173: Consider usingrequestAnimationFrameinstead ofsetTimeout(..., 0).While
setTimeout(..., 0)works,requestAnimationFrameis more semantically correct for waiting until the next paint/layout cycle and can provide smoother visual results.- setTimeout(() => { - fitView({ maxZoom: 1, minZoom: 0.5, padding: 0.2, duration: 0 }); - fittedViewForWorkflowRef.current = currentWorkflowId; - viewportInitialized.current = true; - setIsCanvasReady(true); - setIsTransitioningFromHomepage(false); - }, 0); + requestAnimationFrame(() => { + fitView({ maxZoom: 1, minZoom: 0.5, padding: 0.2, duration: 0 }); + fittedViewForWorkflowRef.current = currentWorkflowId; + viewportInitialized.current = true; + setIsCanvasReady(true); + setIsTransitioningFromHomepage(false); + });
329-332: Hardcoded node height may drift from actual dimensions.The
192pxheight is derived fromh-48in Tailwind, but this coupling is fragile. Consider extracting this as a constant or measuring dynamically.+// Node height matches h-48 (12rem = 192px) in action-node component +const ACTION_NODE_HEIGHT = 192; + // Center the node vertically at the cursor position -// Node height is 192px (h-48 in Tailwind) -const nodeHeight = 192; +const nodeHeight = ACTION_NODE_HEIGHT; position.y -= nodeHeight / 2;
354-361: Magic timeout values to work around panOnDrag.The
50msand100msdelays are workarounds for React Flow's internal behavior. Consider adding a comment explaining why these specific values were chosen, or investigate if React Flow provides a more robust callback.// Deselect all other nodes and select only the new node -// Need to do this after a delay because panOnDrag will clear selection +// Need to do this after a delay because panOnDrag will clear selection. +// 50ms is sufficient for panOnDrag's selection clear to complete. setTimeout(() => {packages/web/lib/steps/step-handler.ts (1)
100-103: Consider suppressing unused variable warning.The
_contextvariable is intentionally unused (destructured to strip it). Adding an underscore prefix or@ts-ignorewould make this intent clearer.function stripContext<T extends StepInput>(input: T): Omit<T, "_context"> { - const { _context, ...rest } = input + // eslint-disable-next-line @typescript-eslint/no-unused-vars + const { _context: _, ...rest } = input return rest as Omit<T, "_context"> }
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/web/lib/steps/http-request.ts (1)
64-69: Add request timeout to prevent indefinite hangs.The fetch call lacks a timeout, which could cause workflow steps to hang indefinitely on unresponsive endpoints. This was flagged in a previous review.
Apply this diff to add a timeout:
+const controller = new AbortController() +const timeoutId = setTimeout(() => controller.abort(), 30000) // 30s timeout + const response = await fetch(input.endpoint, { method: input.httpMethod, headers: parseHeaders(input.httpHeaders), body: parseBody(input.httpMethod, input.httpBody), + signal: controller.signal, }) + +clearTimeout(timeoutId)packages/web/lib/workflow-executor.workflow.ts (1)
107-130: Prioritize exact label matches over node type matching to avoid unpredictable collisions.The matching logic at lines 115-119 uses
Object.values(outputs).find()with an||operator, so{{Trigger}}could match either a node labeled "Trigger" or one withnodeType: "trigger"— whichever appears first in iteration order. This makes the behavior non-deterministic.Apply this diff to check label matches first, then fall back to nodeType:
const simpleLabelPattern = /\{\{(?!@)([^.}]+)(\.([^}]+))?\}\}/g processedValue = processedValue.replace( simpleLabelPattern, (match, label, _dotPart, fieldPath) => { - // Find output by label (case-insensitive) - // Also match by node type (e.g., "Trigger" matches any trigger node) - const output = Object.values(outputs).find( - (o) => - o.label.toLowerCase() === label.toLowerCase() || - o.nodeType.toLowerCase() === label.toLowerCase(), - ) + // Find output by label first (case-insensitive), then by node type + let output = Object.values(outputs).find( + (o) => o.label.toLowerCase() === label.toLowerCase(), + ) + if (!output) { + output = Object.values(outputs).find( + (o) => o.nodeType.toLowerCase() === label.toLowerCase(), + ) + } if (!output) { return match }
🧹 Nitpick comments (3)
packages/web/lib/steps/http-request.ts (3)
19-28: Silent failure on malformed JSON headers may mask configuration errors.When
JSON.parsefails, returning an empty object silently could cause hard-to-debug issues where users expect headers to be sent but they're quietly ignored.Consider logging or returning an error result when header parsing fails:
function parseHeaders(httpHeaders?: string): Record<string, string> { if (!httpHeaders) { return {} } try { return JSON.parse(httpHeaders) } catch { + console.warn("[httpRequest] Failed to parse headers, using empty headers") return {} } }
30-43: Body parsing handles edge cases but HEAD method should also exclude body.The logic correctly excludes body for GET requests, but per HTTP spec, HEAD requests should also not include a body.
function parseBody(httpMethod: string, httpBody?: string): string | undefined { - if (httpMethod === "GET" || !httpBody) { + if (httpMethod === "GET" || httpMethod === "HEAD" || !httpBody) { return undefined }
45-51: Response parsing should handle JSON parse failures gracefully.If Content-Type is
application/jsonbut the body isn't valid JSON,response.json()will throw. This error would be caught by the outer try-catch inhttpRequest, but the error message won't clearly indicate it was a parse failure.async function parseResponse(response: Response): Promise<unknown> { const contentType = response.headers.get("content-type") if (contentType?.includes("application/json")) { - return response.json() + try { + return await response.json() + } catch { + return response.text() + } } return response.text() }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
packages/web/lib/steps/http-request.ts(1 hunks)packages/web/lib/steps/send-email.ts(1 hunks)packages/web/lib/steps/step-handler.ts(1 hunks)packages/web/lib/steps/trigger.ts(1 hunks)packages/web/lib/workflow-executor.workflow.ts(1 hunks)packages/web/next.config.ts(1 hunks)packages/web/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/web/package.json
- packages/web/lib/steps/send-email.ts
- packages/web/lib/steps/trigger.ts
🧰 Additional context used
📓 Path-based instructions (1)
packages/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (packages/web/.cursor/rules/shadcn-ui.mdc)
Always import shadcn components from '@/components/ui/*'
packages/web/**/*.{ts,tsx}: Define event schemas using Zod for Upstash Realtime in TypeScript/Next.js applications
Create Redis instances by importing from @upstash/redis and configuring with UPSTASH_REDIS_REST_URL and UPSTASH_REDIS_REST_TOKEN environment variables
Use the useRealtime hook from @upstash/realtime/client in client components with proper TypeScript typing via RealtimeEvents type
Files:
packages/web/next.config.tspackages/web/lib/steps/step-handler.tspackages/web/lib/steps/http-request.tspackages/web/lib/workflow-executor.workflow.ts
🧬 Code graph analysis (2)
packages/web/lib/steps/http-request.ts (1)
packages/web/lib/steps/step-handler.ts (2)
StepInput(21-23)withStepLogging(164-210)
packages/web/lib/workflow-executor.workflow.ts (5)
packages/web/lib/workflow-store.ts (2)
WorkflowNode(17-17)WorkflowEdge(18-18)packages/web/lib/steps/step-handler.ts (1)
StepContext(10-15)packages/web/lib/steps/http-request.ts (1)
httpRequestStep(105-110)packages/web/lib/steps/send-email.ts (1)
sendEmailStep(90-95)packages/web/lib/steps/trigger.ts (1)
triggerStep(57-60)
🔇 Additional comments (13)
packages/web/lib/steps/http-request.ts (3)
8-17: Type definitions look good.The discriminated union for
HttpRequestResultwith explicit success/failure branches is well-designed for type-safe error handling. TheHttpRequestInputproperly extendsStepInputfor context propagation.
71-85: Error handling strategy is well-designed.The distinction between 4xx (FatalError, no retry) and 5xx (retry-eligible, returns error result) follows good practices for workflow retry semantics. The error text extraction with fallback is also solid.
105-110: Step wrapper correctly uses "use step" directive with logging.The separation between pure business logic (
httpRequest) and the step wrapper (httpRequestStep) is clean. ThewithStepLoggingwrapper ensures consistent observability.packages/web/lib/steps/step-handler.ts (6)
10-28: Type definitions are well-structured.The
StepContexttype captures necessary execution metadata, andStepInputwith optional_contextprovides a clean extension point for all steps.
33-63: Graceful degradation on logging failures is good, but consider the timing discrepancy.The function correctly handles missing
executionIdand catches DB errors. However,startTimeis set after the DB insert completes, which means duration calculations will exclude the logging overhead. This is likely intentional but worth noting.
98-101:stripContextcorrectly removes internal metadata from logged input.The destructuring approach cleanly separates
_contextfrom the rest of the input. The type cast is necessary due to TypeScript limitations withOmiton generic types.
107-129: Workflow completion logging mirrors step logging patterns.The function follows the same error-resilient pattern as step logging, with
console.errorfallback on failures. This ensures workflow completion is recorded even if individual step logs failed.
164-210:withStepLoggingwrapper is well-designed with proper error semantics.The wrapper:
- Correctly extracts and strips context before logging
- Detects error results via duck-typing (
success === false)- Re-throws caught errors after logging to preserve error propagation
- Handles optional workflow completion logging
One consideration: the duck-typing check on lines 177-181 assumes all error results have a
success: falseshape. This is a reasonable convention but should be documented as a contract for step implementations.
78-93: Schema definesdurationMsas a text field; string conversion is correct.The
durationMsfield in the schema is defined astext("duration_ms"), which stores string values. Converting the numeric duration to a string withduration.toString()is the correct and necessary approach that matches the schema design. No type mismatch exists.packages/web/lib/workflow-executor.workflow.ts (4)
1-30: LGTM!The imports and type definitions are well-structured and appropriate for the workflow executor.
38-106: LGTM!The full pattern template processing is well-implemented with proper null handling and fallback behavior.
144-187: LGTM!The action step execution is well-structured with proper template processing and error handling for unknown action types.
193-196: Confirm team awareness of Workflow DevKit beta status.The codebase uses Vercel's Workflow DevKit at version 4.0.1-beta.29, which is a pre-release. While the library currently offers no stable release (only beta versions), ensure the team has documented and validated this choice for your production environment. Consider establishing a plan to migrate to stable releases once available.
| } catch (error) { | ||
| const errorMessage = | ||
| error instanceof Error ? error.message : "Unknown error" | ||
| await logStepComplete(logInfo, "error", undefined, errorMessage) | ||
| throw error | ||
| } |
There was a problem hiding this comment.
Workflow completion is not logged when step throws an exception.
When an error is thrown (not returned as { success: false }), the workflow completion logging in lines 196-201 is skipped. If the throwing step has _workflowComplete set, the workflow status won't be updated.
Consider logging workflow completion on error path as well:
} catch (error) {
const errorMessage =
error instanceof Error ? error.message : "Unknown error"
await logStepComplete(logInfo, "error", undefined, errorMessage)
+
+ // Also log workflow completion on error if requested
+ if (context?._workflowComplete && context.executionId) {
+ await logWorkflowComplete({
+ executionId: context.executionId,
+ status: "error",
+ error: errorMessage,
+ startTime: context._workflowComplete.startTime,
+ })
+ }
+
throw error
}Committable suggestion skipped: line range outside the PR's diff.
| await triggerStep({ | ||
| triggerData: {}, | ||
| _workflowComplete: { | ||
| executionId, | ||
| status: finalSuccess ? "success" : "error", | ||
| output: Object.values(results).at(-1)?.data, | ||
| error: Object.values(results).find((r) => !r.success)?.error, | ||
| startTime: workflowStartTime, | ||
| }, | ||
| }) |
There was a problem hiding this comment.
Reconsider using .at(-1) for workflow output.
Line 369 uses Object.values(results).at(-1)?.data to determine the final workflow output. Since Object.values() order is not guaranteed and workflows may have parallel execution branches, the "last" result is ambiguous and may not represent the intended final output.
Consider alternatives:
- Use a designated "output" node or terminal node
- Aggregate results from all terminal nodes (nodes with no outgoing edges)
- Make the output selection strategy explicit
🤖 Prompt for AI Agents
In packages/web/lib/workflow-executor.workflow.ts around lines 364–373, the
current use of Object.values(results).at(-1)?.data to pick the workflow output
is unsafe because Object.values order is not reliable and parallel branches can
produce multiple terminal results; change this to explicitly determine output by
(a) identifying terminal nodes in the workflow graph (nodes with no outgoing
edges) and collecting their result entries from the results map, (b) if there is
a single terminal node use its .data as output, otherwise aggregate multiple
terminal results into a deterministic structure (e.g., an array of terminal
outputs or a merged object keyed by node id) and set that as the output, and (c)
ensure errors from any terminal node are surfaced similarly (e.g., set error
field if any terminal result is failing). Finally, replace the .at(-1) usage
with this explicit terminal-node-based selection and update triggerStep’s
_workflowComplete.output assignment accordingly.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
packages/web/components/workflow/config/trigger-config.tsx (1)
49-57: Guard against missing/failed Clipboard API inhandleCopyWebhookUrl
navigator.clipboard.writeTextcan be unavailable (non-HTTPS, some browsers) or reject its Promise. As written, this can throw or silently fail without feedback.You can keep the UX the same while handling these cases:
const handleCopyWebhookUrl = () => { - if (webhookUrl) { - navigator.clipboard.writeText(webhookUrl) - toast.success("Webhook URL copied to clipboard") - } + if (!webhookUrl) return + + if (typeof navigator !== "undefined" && navigator.clipboard?.writeText) { + navigator.clipboard + .writeText(webhookUrl) + .then(() => { + toast.success("Webhook URL copied to clipboard") + }) + .catch(() => { + toast.error("Failed to copy webhook URL") + }) + } else { + toast.error("Clipboard API is not available in this environment") + } }packages/web/lib/workflow-executor.workflow.ts (1)
372-381: Reconsider using.at(-1)for workflow output.Line 377 still uses
Object.values(results).at(-1)?.datato determine the final workflow output. As previously noted,Object.values()iteration order is not guaranteed, and with parallel execution branches (line 346'sPromise.all), the "last" result is arbitrary and may not represent the intended final output.Consider alternatives:
- Use a designated "output" node or terminal node
- Aggregate results from all terminal nodes (nodes with no outgoing edges)
- Make the output selection strategy explicit
#!/bin/bash # Check if there are any explicit terminal node or output node patterns in the workflow schema rg -n --type=ts -C3 'terminal.*node|output.*node|final.*node|workflow.*output' packages/web/libpackages/web/lib/steps/http-request.ts (1)
67-76: SSRF protection still missing—add URL validation before fetch call.The AbortController timeout (30 seconds) addresses the timeout concern. However, the endpoint parameter accepts any URL without validation, allowing requests to localhost, private IP ranges (10.x, 192.168.x, 172.16-31.x), or non-HTTP protocols—classic SSRF vectors.
Add URL validation immediately after the empty-endpoint check:
if (!input.endpoint) { return { success: false, error: "HTTP request failed: URL is required", } } + try { + const url = new URL(input.endpoint) + if (!['http:', 'https:'].includes(url.protocol)) { + return { success: false, error: 'Only HTTP(S) protocols are allowed' } + } + const hostname = url.hostname.toLowerCase() + if ( + hostname === 'localhost' || + hostname.startsWith('127.') || + hostname.startsWith('10.') || + hostname.startsWith('192.168.') || + hostname.startsWith('169.254.') || + /^172\.(1[6-9]|2[0-9]|3[0-1])\./.test(hostname) + ) { + return { success: false, error: 'Internal IP addresses are not allowed' } + } + } catch { + return { success: false, error: 'Invalid URL format' } + } + const controller = new AbortController()
🧹 Nitpick comments (2)
packages/web/components/workflow/config/trigger-config.tsx (2)
18-28: Webhook schema parsing is safe now; consider validating the parsed shapeThe dedicated
parseWebhookSchemahelper fixes the earlier crash risk fromJSON.parseand keeps failures contained with a console warning, which is good.If you want to be a bit more defensive, you could also ensure the parsed value is actually an array before casting:
function parseWebhookSchema(schemaString: unknown): SchemaField[] { if (!schemaString || typeof schemaString !== "string") { return [] } try { - return JSON.parse(schemaString) as SchemaField[] + const parsed = JSON.parse(schemaString) + return Array.isArray(parsed) ? (parsed as SchemaField[]) : [] } catch (error) { console.warn("Failed to parse webhookSchema:", error) return [] } }
30-37: Config typing is very loose; consider a dedicated TriggerConfig type
config: Record<string, unknown>plusonUpdateConfig: (key: string, value: string) => voidworks, but it pushes a lot of casting (as string) and stringly-typed keys through the component.Defining a
TriggerConfiginterface (withtriggerType,webhookSchema,webhookMockRequest,scheduleCron,scheduleTimezone, etc.) and tighteningonUpdateConfigto a key-of-TriggerConfig union would catch typos and type mismatches at compile time and simplify casts in this component.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/web/app/api/workflows/[id]/executions/route.ts(1 hunks)packages/web/app/api/workflows/executions/[executionId]/status/route.ts(1 hunks)packages/web/components/workflow/config/trigger-config.tsx(1 hunks)packages/web/components/workflow/workflow-context-menu.tsx(1 hunks)packages/web/components/workflow/workflow-toolbar.tsx(1 hunks)packages/web/lib/steps/http-request.ts(1 hunks)packages/web/lib/workflow-executor.workflow.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/web/app/api/workflows/[id]/executions/route.ts
- packages/web/app/api/workflows/executions/[executionId]/status/route.ts
- packages/web/components/workflow/workflow-toolbar.tsx
🧰 Additional context used
📓 Path-based instructions (3)
packages/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (packages/web/.cursor/rules/shadcn-ui.mdc)
Always import shadcn components from '@/components/ui/*'
packages/web/**/*.{ts,tsx}: Define event schemas using Zod for Upstash Realtime in TypeScript/Next.js applications
Create Redis instances by importing from @upstash/redis and configuring with UPSTASH_REDIS_REST_URL and UPSTASH_REDIS_REST_TOKEN environment variables
Use the useRealtime hook from @upstash/realtime/client in client components with proper TypeScript typing via RealtimeEvents type
Files:
packages/web/lib/steps/http-request.tspackages/web/components/workflow/config/trigger-config.tsxpackages/web/components/workflow/workflow-context-menu.tsxpackages/web/lib/workflow-executor.workflow.ts
packages/web/**/{components,src/**/components}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (packages/web/.cursor/rules/shadcn-ui.mdc)
Always use shadcn/ui components when adding new UI components
Files:
packages/web/components/workflow/config/trigger-config.tsxpackages/web/components/workflow/workflow-context-menu.tsx
packages/web/**/{components,pages,src/**/components,src/**/pages}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (packages/web/.cursor/rules/shadcn-ui.mdc)
Use Tailwind CSS for styling instead of custom CSS or other styling solutions
Files:
packages/web/components/workflow/config/trigger-config.tsxpackages/web/components/workflow/workflow-context-menu.tsx
🧠 Learnings (3)
📓 Common learnings
Learnt from: drewradcliff
Repo: inboundemail/ehook.app PR: 6
File: packages/web/app/api/workflows/[id]/execute/route.ts:42-70
Timestamp: 2025-12-15T00:39:41.749Z
Learning: When using Vercel Workflows SDK (the `workflow` package) with workflow functions (typically in `.workflow.ts` files), background execution is durable and will complete reliably in serverless environments. The concern about Promise.resolve().then() not completing in serverless doesn't apply to workflow functions, as they have different execution semantics with built-in durability guarantees.
📚 Learning: 2025-11-26T14:29:47.918Z
Learnt from: CR
Repo: inboundemail/ehook.app PR: 0
File: packages/web/.cursor/rules/shadcn-ui.mdc:0-0
Timestamp: 2025-11-26T14:29:47.918Z
Learning: Applies to packages/web/**/{components,src/**/components}/**/*.{ts,tsx} : Always use shadcn/ui components when adding new UI components
Applied to files:
packages/web/components/workflow/workflow-context-menu.tsx
📚 Learning: 2025-12-15T00:39:41.749Z
Learnt from: drewradcliff
Repo: inboundemail/ehook.app PR: 6
File: packages/web/app/api/workflows/[id]/execute/route.ts:42-70
Timestamp: 2025-12-15T00:39:41.749Z
Learning: For code reviews involving Vercel Workflows SDK usage, especially files that define workflow functions (typically named with the .workflow.ts pattern), rely on their durable, serverless-friendly execution guarantees. Do not treat Promise.resolve().then() completion as a concern in these workflow functions, as they have built-in durability guarantees that ensure completion even in constrained serverless environments. This guidance applies to all files implementing workflow functions, not just a single instance.
Applied to files:
packages/web/lib/workflow-executor.workflow.ts
🧬 Code graph analysis (2)
packages/web/components/workflow/config/trigger-config.tsx (5)
packages/web/components/workflow/config/schema-builder.tsx (2)
SchemaField(16-23)SchemaBuilder(32-240)packages/web/components/ui/label.tsx (1)
Label(24-24)packages/web/components/ui/select.tsx (5)
Select(177-177)SelectTrigger(185-185)SelectValue(186-186)SelectContent(178-178)SelectItem(180-180)packages/web/components/ui/input.tsx (1)
Input(21-21)packages/web/components/ui/button.tsx (1)
Button(60-60)
packages/web/components/workflow/workflow-context-menu.tsx (2)
packages/web/lib/workflow-store.ts (6)
nodesAtom(21-21)deleteNodeAtom(306-338)deleteEdgeAtom(340-360)addNodeAtom(180-202)selectedNodeAtom(23-23)WorkflowNode(17-17)packages/web/lib/utils.ts (1)
cn(4-6)
🔇 Additional comments (7)
packages/web/components/workflow/workflow-context-menu.tsx (1)
175-205: MenuItem now correctly uses shadcn Button component.This implementation properly addresses the previous review feedback. The
MenuItemcomponent now uses theButtonprimitive from shadcn/ui with appropriate variant mapping, which aligns with the coding guidelines for using shadcn components.packages/web/lib/steps/http-request.ts (2)
20-52: LGTM! Helper functions are safe and well-structured.The parsing helpers correctly handle edge cases:
parseHeadersandparseBodysafely catch JSON.parse errors,parseBodycorrectly excludes body for GET requests, andparseResponseis content-type aware.
104-110: LGTM! Step wrapper follows the correct pattern.The
"use step"directive withwithStepLoggingwrapper andmaxRetries = 0configuration is appropriate for Vercel Workflows integration.packages/web/lib/workflow-executor.workflow.ts (4)
116-126: LGTM! Label matching now prioritized correctly.The template resolution now searches for exact label matches first (case-insensitive), then falls back to nodeType matching. This resolves the collision concern from the previous review.
38-147: LGTM! Template processing is thorough and safe.The two-pattern template resolution (full
{{@nodeId:Label.field}}and simple{{Label.field}}) with field path traversal and safe stringification handles edge cases correctly.
152-195: LGTM! Action step execution is well-structured.The switch-based dispatch for "HTTP Request" and "Send Email" action types with template processing and standardized result format is clean and extensible.
260-264: Thevisitedset enforces single-execution-per-node, preventing multi-path convergence.The
visitedset (line 260) ensures each node executes at most once across all paths from a given trigger. For workflow DAGs where multiple paths converge at a node (e.g., Trigger → [NodeA, NodeB] → NodeC), whichever branch reaches NodeC first will execute it; the second path will skip execution due to the visited check.The current implementation uses only
edgesBySourcefor outgoing edges and has no incoming edge tracking or predecessor-counting mechanism. If your workflows require a node to wait for results from all upstream branches before executing, the architecture would need to track incoming edges per node and defer execution until all predecessors complete (e.g., using a join-node pattern or counting incoming edges).
| const handleAddStep = useCallback(() => { | ||
| if (menuState?.flowPosition) { | ||
| const nodeHeight = 192 | ||
| const newNode: WorkflowNode = { | ||
| id: nanoid(), | ||
| type: "action", | ||
| position: { | ||
| x: menuState.flowPosition.x, | ||
| y: menuState.flowPosition.y - nodeHeight / 2, | ||
| }, | ||
| data: { | ||
| label: "", | ||
| description: "", | ||
| type: "action", | ||
| config: {}, | ||
| status: "idle", | ||
| }, | ||
| selected: true, | ||
| } | ||
| addNode(newNode) | ||
| setSelectedNode(newNode.id) | ||
| } | ||
| onClose() | ||
| }, [menuState, addNode, setSelectedNode, onClose]) |
There was a problem hiding this comment.
Hardcoded node height may cause positioning misalignment.
The nodeHeight = 192 constant at line 61 is used to center the new node vertically, but if the actual rendered node height differs (due to CSS changes or dynamic content), the positioning will be inaccurate.
Additionally, creating nodes with an empty label (line 70) may not provide clear feedback to users about the newly created step.
Consider:
- Extracting
nodeHeightas a shared constant from where node dimensions are actually defined - Providing a default label like "New Step" for better UX
const handleAddStep = useCallback(() => {
if (menuState?.flowPosition) {
- const nodeHeight = 192
+ const nodeHeight = 192 // TODO: Import from node component constants
const newNode: WorkflowNode = {
id: nanoid(),
type: "action",
position: {
x: menuState.flowPosition.x,
y: menuState.flowPosition.y - nodeHeight / 2,
},
data: {
- label: "",
+ label: "New Step",
description: "",
type: "action",
config: {},
status: "idle",
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleAddStep = useCallback(() => { | |
| if (menuState?.flowPosition) { | |
| const nodeHeight = 192 | |
| const newNode: WorkflowNode = { | |
| id: nanoid(), | |
| type: "action", | |
| position: { | |
| x: menuState.flowPosition.x, | |
| y: menuState.flowPosition.y - nodeHeight / 2, | |
| }, | |
| data: { | |
| label: "", | |
| description: "", | |
| type: "action", | |
| config: {}, | |
| status: "idle", | |
| }, | |
| selected: true, | |
| } | |
| addNode(newNode) | |
| setSelectedNode(newNode.id) | |
| } | |
| onClose() | |
| }, [menuState, addNode, setSelectedNode, onClose]) | |
| const handleAddStep = useCallback(() => { | |
| if (menuState?.flowPosition) { | |
| const nodeHeight = 192 // TODO: Import from node component constants | |
| const newNode: WorkflowNode = { | |
| id: nanoid(), | |
| type: "action", | |
| position: { | |
| x: menuState.flowPosition.x, | |
| y: menuState.flowPosition.y - nodeHeight / 2, | |
| }, | |
| data: { | |
| label: "New Step", | |
| description: "", | |
| type: "action", | |
| config: {}, | |
| status: "idle", | |
| }, | |
| selected: true, | |
| } | |
| addNode(newNode) | |
| setSelectedNode(newNode.id) | |
| } | |
| onClose() | |
| }, [menuState, addNode, setSelectedNode, onClose]) |
| const triggerResult = await triggerStep({ | ||
| triggerData: { | ||
| triggered: true, | ||
| timestamp: Date.now(), | ||
| ...triggerInput, | ||
| }, | ||
| _context: context, | ||
| }) |
There was a problem hiding this comment.
Spread operator order allows triggerInput to override system properties.
The spread order { triggered: true, timestamp: Date.now(), ...triggerInput } means user-provided triggerInput can override the hardcoded triggered and timestamp properties. This could lead to inconsistent system state if external input contains these keys.
Apply this diff to ensure system properties take precedence:
const triggerResult = await triggerStep({
triggerData: {
+ ...triggerInput,
triggered: true,
timestamp: Date.now(),
- ...triggerInput,
},
_context: context,
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const triggerResult = await triggerStep({ | |
| triggerData: { | |
| triggered: true, | |
| timestamp: Date.now(), | |
| ...triggerInput, | |
| }, | |
| _context: context, | |
| }) | |
| const triggerResult = await triggerStep({ | |
| triggerData: { | |
| ...triggerInput, | |
| triggered: true, | |
| timestamp: Date.now(), | |
| }, | |
| _context: context, | |
| }) |
🤖 Prompt for AI Agents
In packages/web/lib/workflow-executor.workflow.ts around lines 286 to 293, the
object spread currently lets caller-supplied triggerInput override system
fields; change the construction so system properties take precedence by
spreading triggerInput first and then setting triggered: true and timestamp:
Date.now() (i.e. { ...triggerInput, triggered: true, timestamp: Date.now() }),
ensuring timestamp is evaluated once at call time and that the explicit
properties overwrite any keys from triggerInput.
1 participant
Description
Demo
ehook-workflow.webm