feat(webhook): node integration #974
Conversation
debojyoti-git
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. WalkthroughAdds webhook support end-to-end: new WebhookConfig type and UI, credential management components and API, WorkflowBuilder webhook wiring, server webhook runtime (registry, auth, execution, integration), scripts, and related services; also removes an icon export. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant TriggersSidebar
participant WorkflowBuilder
participant Canvas
participant WebhookConfigUI as "Webhook Configuration UI"
participant credentialsAPI as "credentialsAPI / backend"
User->>TriggersSidebar: Select "On Webhook Call"
TriggersSidebar->>WorkflowBuilder: triggerSelected("webhook")
WorkflowBuilder->>Canvas: create pending webhook node (temp id)
WorkflowBuilder->>WebhookConfigUI: open with initialConfig/toolData
WebhookConfigUI->>credentialsAPI: fetch credential list (if auth)
User->>WebhookConfigUI: edit path/method/auth/headers/query
WebhookConfigUI-->>User: show generated webhook URL (copyable)
User->>WebhookConfigUI: Save
WebhookConfigUI->>WorkflowBuilder: onSave(finalConfig)
alt Creating new webhook
WorkflowBuilder->>Canvas: replace pending node with webhook node (tool type "webhook")
WorkflowBuilder->>credentialsAPI: saveWebhookConfig(...)
else Updating existing webhook
WorkflowBuilder->>Canvas: update node.tool/config
WorkflowBuilder->>credentialsAPI: updateWebhookConfig(...)
end
WorkflowBuilder->>WebhookConfigUI: close UI, reset state
sequenceDiagram
actor ExternalCaller
participant HTTP as "Server /webhook/*"
participant webhookRegistry
participant WebhookAuthService
participant WebhookExecutionService
participant WorkflowEngine
ExternalCaller->>HTTP: POST /webhook/<path> (headers/body)
HTTP->>webhookRegistry: getWebhook(path)
alt webhook found
HTTP->>WebhookAuthService: validateAuthentication(request, config)
WebhookAuthService-->>HTTP: ok / throw
HTTP->>WebhookExecutionService: executeWorkflowFromWebhook(context)
WebhookExecutionService->>WorkflowEngine: create execution & start async
alt responseMode == immediate
HTTP-->>ExternalCaller: 200 accepted (executionId)
else responseMode == wait_for_completion
WebhookExecutionService->>WebhookExecutionService: waitForExecution (poll)
WebhookExecutionService-->>HTTP: final result or timeout
HTTP-->>ExternalCaller: 200/504
end
else
HTTP-->>ExternalCaller: 404
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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 @debojyoti-git, 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 comprehensive webhook integration into the frontend workflow builder. It provides users with the ability to define custom webhook endpoints that can trigger workflows, offering granular control over HTTP methods, paths, authentication mechanisms, and response handling. The changes include a new configuration interface, a dedicated UI panel for setup, and seamless integration within the existing workflow visualization and editing environment. 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 webhook integration for workflows, including a new UI for webhook configuration and updates to the workflow builder to support webhook trigger nodes. The implementation is solid, but there are several opportunities for improvement regarding code duplication, maintainability, and user experience. I've provided suggestions to refactor complex logic, centralize duplicated code, replace legacy UI patterns, and improve data model consistency.
| // Build description from webhook configuration | ||
| if (webhookConfig?.webhookUrl || webhookConfig?.path) { | ||
| const method = webhookConfig?.httpMethod || 'POST' | ||
| const url = webhookConfig?.webhookUrl || `${window.location.origin}/webhook${webhookConfig?.path || ''}` |
There was a problem hiding this comment.
The logic for generating the webhook URL is duplicated from the WebhookConfigurationUI component. Code duplication, especially for business logic like URL generation, is risky and can lead to inconsistencies if one location is updated and the other is not. This logic should be extracted into a shared utility function to ensure a single source of truth.
| useEffect(() => { | ||
| if (initialConfig) { | ||
| setConfig(initialConfig) | ||
| } else if (toolData?.val || toolData?.config) { | ||
| const data = toolData.val || toolData.config | ||
| setConfig({ | ||
| webhookUrl: data.webhookUrl || "", | ||
| httpMethod: data.httpMethod || "POST", | ||
| path: data.path || "", | ||
| authentication: data.authentication || "none", | ||
| responseMode: data.responseMode || "immediately", | ||
| options: data.options || {}, | ||
| headers: data.headers || {}, | ||
| queryParams: data.queryParams || {}, | ||
| }) | ||
| } | ||
| }, [initialConfig, toolData]) |
There was a problem hiding this comment.
The logic inside this useEffect to initialize the component's state is verbose, with the else if block repeating many default values. This can be refactored to be more concise and maintainable by consolidating the data source and then applying defaults, which reduces code repetition.
useEffect(() => {
const data = initialConfig || toolData?.val || toolData?.config
if (data) {
setConfig({
webhookUrl: data.webhookUrl || "",
httpMethod: data.httpMethod || "POST",
path: data.path || "",
authentication: data.authentication || "none",
responseMode: data.responseMode || "immediately",
options: data.options || {},
headers: data.headers || {},
queryParams: data.queryParams || {},
})
}
}, [initialConfig, toolData])
| if (!config.path.trim()) { | ||
| alert("Please enter a webhook path") | ||
| return |
There was a problem hiding this comment.
Using alert() for form validation provides a poor user experience as it is a blocking and disruptive browser dialog. It's recommended to use non-blocking UI feedback, such as a snackbar/toast notification or an inline error message near the input field, to inform the user about the missing path.
| const copyWebhookUrl = async () => { | ||
| const url = config.path ? generateWebhookUrl() : `${window.location.origin}/webhook/your-path` | ||
| try { | ||
| await navigator.clipboard.writeText(url) | ||
| setCopied(true) | ||
| setTimeout(() => setCopied(false), 2000) // Reset after 2 seconds | ||
| } catch (err) { | ||
| console.error('Failed to copy URL:', err) | ||
| // Fallback for browsers that don't support clipboard API | ||
| const textArea = document.createElement('textarea') | ||
| textArea.value = url | ||
| document.body.appendChild(textArea) | ||
| textArea.select() | ||
| document.execCommand('copy') | ||
| document.body.removeChild(textArea) | ||
| setCopied(true) | ||
| setTimeout(() => setCopied(false), 2000) | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic for indicating a successful copy operation (setCopied(true) and the setTimeout) is duplicated in both the try block for the modern Clipboard API and the catch block for the fallback mechanism. This duplication can be avoided by extracting the success handling logic into a separate function, which would improve maintainability.
// Copy webhook URL to clipboard
const handleCopySuccess = () => {
setCopied(true);
setTimeout(() => setCopied(false), 2000); // Reset after 2 seconds
};
const copyWebhookUrl = async () => {
const url = config.path ? generateWebhookUrl() : `${window.location.origin}/webhook/your-path`;
try {
await navigator.clipboard.writeText(url);
handleCopySuccess();
} catch (err) {
console.error('Failed to copy URL:', err);
// Fallback for browsers that don't support clipboard API
const textArea = document.createElement('textarea');
textArea.value = url;
document.body.appendChild(textArea);
textArea.select();
document.execCommand('copy');
document.body.removeChild(textArea);
handleCopySuccess();
}
}
| style={{ | ||
| display: "flex", | ||
| padding: "20px", | ||
| alignItems: "center", | ||
| gap: "10px", | ||
| alignSelf: "stretch", | ||
| borderBottom: "1px solid var(--gray-300, #E4E6E7)", | ||
| }} |
There was a problem hiding this comment.
This component uses several inline style objects for styling. Since the project is already configured with Tailwind CSS, it's a best practice to replace these inline styles with Tailwind's utility classes. This improves code consistency, makes it easier to manage styles, and ensures that theming (like dark mode) is applied correctly.
| {(() => { | ||
| // First try to get title from workflow_tools[index].val.title | ||
| if (hasWebhookTool && tools?.[0]?.val && typeof tools[0].val === 'object' && (tools[0].val as any)?.title) { | ||
| return (tools[0].val as any).title | ||
| } | ||
|
|
||
| // Try to get title from workflow_tools[index].value.title | ||
| if (hasWebhookTool && tools?.[0] && (tools[0] as any)?.value && typeof (tools[0] as any).value === 'object' && (tools[0] as any).value?.title) { | ||
| return (tools[0] as any).value.title | ||
| } | ||
|
|
||
| // Fallback to "Webhook" title | ||
| return "Webhook" | ||
| })()} |
There was a problem hiding this comment.
This IIFE (Immediately Invoked Function Expression) to determine the webhook title contains complex logic that is repeated for determining the description. This pattern reduces readability and maintainability. It would be cleaner to extract this logic into a dedicated helper function. A similar function could be created for the description as well.
| const auth = webhookConfig?.authentication === 'none' ? 'No authentication' : | ||
| webhookConfig?.authentication === 'basic' ? 'Basic authentication' : | ||
| webhookConfig?.authentication === 'bearer' ? 'Bearer token authentication' : | ||
| webhookConfig?.authentication === 'api_key' ? 'API key authentication' : 'No authentication' |
There was a problem hiding this comment.
This long chain of ternary operators for generating the authentication description is hard to read and maintain. A mapping object would be a much cleaner and more scalable solution.
const authMap: Record<string, string> = {
'none': 'No authentication',
'basic': 'Basic authentication',
'bearer': 'Bearer token authentication',
'api_key': 'API key authentication',
};
const auth = authMap[webhookConfig?.authentication] ?? 'No authentication';
| const webhookTool = { | ||
| id: `tool-${newNodeId}`, | ||
| type: "webhook", | ||
| val: webhookConfig, // Use 'val' to match template structure | ||
| value: webhookConfig, // Also include 'value' for compatibility | ||
| config: webhookConfig, | ||
| } |
There was a problem hiding this comment.
The webhookTool object is being created with val, value, and config properties, all holding the same webhookConfig data. This redundancy can lead to confusion and makes the data model harder to manage. It's best to have a single source of truth for the tool's configuration. This change might require adjustments in other parts of the code that consume this tool object, but it will lead to a more robust and clearer data model.
const webhookTool = {
id: `tool-${newNodeId}`,
type: "webhook",
config: webhookConfig,
}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
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)
2328-2358: Include webhook nodes in the “valid trigger” check.
hasValidTriggernever turns true for webhook steps, so workflows that start with the new webhook trigger can’t be saved—the Save button stays disabled forever. Add the"webhook"type to this guard.- return nodeData?.step?.type && - nodeData.step.type !== "trigger_selector" && - (nodeData.step.type === "form_submission" || - nodeData.step.type === "manual" || - nodeData.step.type === "schedule" || - nodeData.step.type === "app_event") + return ( + nodeData?.step?.type && + nodeData.step.type !== "trigger_selector" && + [ + "form_submission", + "manual", + "schedule", + "app_event", + "webhook", + ].includes(nodeData.step.type) + )
🧹 Nitpick comments (1)
frontend/src/components/workflow/Types.ts (1)
303-312: Clarify optionality for webhook authentication settings.Currently
authenticationis always required, yet method-specific fields (e.g.,authdetails for basic, bearer, API key) are undocumented and absent. If the UI can submit “none” without any additional payload, consider either:
- Making
authenticationoptional (defaulting to"none"), or- Introducing dedicated option fields (e.g.,
basicAuth?: { username: string; password: string }) so downstream consumers know what to expect.Without this refinement, implementers must inspect the UI to guess the payload shape, increasing integration risk. Tightening the type here keeps the contract self-describing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/src/components/workflow/Types.ts(1 hunks)frontend/src/components/workflow/WebhookConfigurationUI.tsx(1 hunks)frontend/src/components/workflow/WorkflowBuilder.tsx(16 hunks)frontend/src/components/workflow/WorkflowIcons.tsx(0 hunks)
💤 Files with no reviewable changes (1)
- frontend/src/components/workflow/WorkflowIcons.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
frontend/src/components/workflow/Types.ts (1)
frontend/src/components/workflow/WebhookConfigurationUI.tsx (1)
WebhookConfig(486-486)
frontend/src/components/workflow/WebhookConfigurationUI.tsx (2)
frontend/src/components/workflow/Types.ts (1)
WebhookConfig(303-312)frontend/src/components/workflow/WorkflowIcons.tsx (1)
BackArrowIcon(393-413)
frontend/src/components/workflow/WorkflowBuilder.tsx (3)
frontend/src/components/workflow/WorkflowIcons.tsx (1)
WebhookIcon(166-184)frontend/src/components/workflow/Types.ts (2)
WebhookConfig(303-312)Tool(56-61)frontend/src/components/workflow/WebhookConfigurationUI.tsx (3)
WebhookConfig(486-486)WebhookConfigurationUI(19-483)WebhookConfigurationUI(485-485)
| const generateWebhookUrl = () => { | ||
| const baseUrl = window.location.origin | ||
| const cleanPath = config.path.startsWith('/') ? config.path : `/${config.path}` | ||
| return `${baseUrl}/webhook${cleanPath}` | ||
| } |
There was a problem hiding this comment.
Guard browser globals during render.
Referencing window.location while rendering will throw in SSR/Prerender environments, making this panel unusable there. Please gate the access behind a runtime check and reuse the helper everywhere you build the URL.
+ const getOrigin = () =>
+ typeof window !== "undefined" ? window.location.origin : ""
+
// Generate webhook URL based on the path
const generateWebhookUrl = () => {
- const baseUrl = window.location.origin
+ const baseUrl = getOrigin()
const cleanPath = config.path.startsWith('/') ? config.path : `/${config.path}`
return `${baseUrl}/webhook${cleanPath}`
}
…
- {config.path ? generateWebhookUrl() : `${window.location.origin}/webhook/your-path`}
+ {config.path ? generateWebhookUrl() : `${getOrigin()}/webhook/your-path`}Also applies to: 228-231
🤖 Prompt for AI Agents
In frontend/src/components/workflow/WebhookConfigurationUI.tsx around lines
66-70 (and also apply to lines 228-231), the code reads window.location.origin
directly which will throw during SSR/prerender; replace this with a small
runtime-safe helper that first checks typeof window !== "undefined" before
accessing window.location, returning a sensible fallback (e.g. empty string or a
configurable baseUrl) when window is unavailable, and then use that helper
everywhere the webhook URL is built so both locations reuse the same safe logic.
| const url = webhookConfig?.webhookUrl || `${window.location.origin}/webhook${webhookConfig?.path || ''}` | ||
| const auth = webhookConfig?.authentication === 'none' ? 'No authentication' : | ||
| webhookConfig?.authentication === 'basic' ? 'Basic authentication' : | ||
| webhookConfig?.authentication === 'bearer' ? 'Bearer token authentication' : | ||
| webhookConfig?.authentication === 'api_key' ? 'API key authentication' : 'No authentication' | ||
|
|
||
| return `${method} ${url} • ${auth}` | ||
| } |
There was a problem hiding this comment.
Guard window usage in node rendering.
This JSX executes during render; on an SSR pass it will hit window and crash. Please mirror the getOrigin() helper (or inline the guard) before constructing the fallback URL.
- const url = webhookConfig?.webhookUrl || `${window.location.origin}/webhook${webhookConfig?.path || ''}`
+ const origin =
+ typeof window !== "undefined" ? window.location.origin : ""
+ const url =
+ webhookConfig?.webhookUrl ||
+ `${origin}/webhook${webhookConfig?.path || ""}`📝 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 url = webhookConfig?.webhookUrl || `${window.location.origin}/webhook${webhookConfig?.path || ''}` | |
| const auth = webhookConfig?.authentication === 'none' ? 'No authentication' : | |
| webhookConfig?.authentication === 'basic' ? 'Basic authentication' : | |
| webhookConfig?.authentication === 'bearer' ? 'Bearer token authentication' : | |
| webhookConfig?.authentication === 'api_key' ? 'API key authentication' : 'No authentication' | |
| return `${method} ${url} • ${auth}` | |
| } | |
| const origin = | |
| typeof window !== "undefined" ? window.location.origin : "" | |
| const url = | |
| webhookConfig?.webhookUrl || | |
| `${origin}/webhook${webhookConfig?.path || ""}` | |
| const auth = | |
| webhookConfig?.authentication === "none" | |
| ? "No authentication" | |
| : webhookConfig?.authentication === "basic" | |
| ? "Basic authentication" | |
| : webhookConfig?.authentication === "bearer" | |
| ? "Bearer token authentication" | |
| : webhookConfig?.authentication === "api_key" | |
| ? "API key authentication" | |
| : "No authentication" | |
| return `${method} ${url} • ${auth}` | |
| } |
🤖 Prompt for AI Agents
In frontend/src/components/workflow/WorkflowBuilder.tsx around lines 842 to 849,
the render constructs a fallback URL using window.location.origin which will
crash during SSR; update the code to guard window access by using the existing
getOrigin() helper (or inline a check like typeof window !== 'undefined' ?
window.location.origin : '') before building the fallback URL, so replace
`${window.location.origin}/webhook...` with a call to getOrigin() (or the
guarded expression) to ensure safe server-side rendering.
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (12)
frontend/src/components/workflow/Types.ts (1)
303-313: Type looks fine; align casing and ID semantics across API
- Confirm server expects camelCase (webhookUrl) vs snake_case (webhook_url) used elsewhere in ToolValue/ToolConfig.
- Consider renaming selectedCredential → selectedCredentialId for clarity.
frontend/src/components/workflow/api/ApiHandlers.ts (3)
517-523: Avoid relying onthisbinding inside object literalUsing
this.createToolcan break if the method is detached. Call via the namespace.Apply:
- const response = await this.createTool(toolData) + const response = await workflowToolsAPI.createTool(toolData)
632-634: Avoidthisbinding in update; call via namespaceApply:
- const response = await this.updateTool(toolId, toolData) + const response = await workflowToolsAPI.updateTool(toolId, toolData)
438-546: Scrub verbose console logs (some contain or could contain sensitive config)Drop or guard logs behind a debug flag. Avoid logging payloads entirely in production.
frontend/src/components/workflow/WebhookConfigurationUI.tsx (3)
334-343: Authentication options incomplete vs type.WebhookConfig allows "bearer" and "api_key" but the dropdown exposes only "none" and "basic". Add the missing options for parity.
<Dropdown options={[ { value: "none", label: "None" }, - { value: "basic", label: "Basic Auth" } + { value: "basic", label: "Basic Auth" }, + { value: "bearer", label: "Bearer Token" }, + { value: "api_key", label: "API Key" } ]}
112-116: Avoid blocking alert for validation.Replace alert with inline error or a toast/snackbar for better UX.
I can wire a small pathError state and render a helper text under the input. Want a patch?
61-76: Remove or gate verbose console logs.These logs will spam prod consoles. Wrap with env guard or drop.
- console.log("🔧 Initializing webhook config from toolData:", { valueData, configData }) + if (process.env.NODE_ENV !== "production") { + console.debug("🔧 Initializing webhook config from toolData:", { valueData, configData }) + } … - console.log("🔧 Extracted path from toolData:", valueData.path) + if (process.env.NODE_ENV !== "production") { + console.debug("🔧 Extracted path from toolData:", valueData.path) + } … - console.log("🔧 Setting webhook config:", newConfig) + if (process.env.NODE_ENV !== "production") { + console.debug("🔧 Setting webhook config:", newConfig) + }Also applies to: 91-91
frontend/src/components/CredentialModal.tsx (2)
58-61: Validate before save.Prevent saving empty credentials (e.g., name/user/password) and signal errors inline.
I can add minimal required checks with helper texts and disabled Save until valid. Proceed?
76-94: Use unified Input component (optional).For consistency with the rest of the UI, consider using the shared for the name field too.
frontend/src/components/workflow/WorkflowBuilder.tsx (3)
3480-3514: Use a unique node ID for the webhook trigger.Hardcoding "webhook-trigger" risks collisions; prefer a counter- or timestamp-based ID.
- const newNodeId = "webhook-trigger" + const newNodeId = `webhook-${nodeCounter}`
3580-3583: Replace alert with Snackbar for errors.You already have showSnackbarMessage; use it for consistent UX.
- alert("Failed to save webhook configuration. Please try again.") + showSnackbarMessage("Failed to save webhook configuration. Please try again.", "error")
159-160: Import type from Types to decouple UI.Avoid importing WebhookConfig via the UI module; import the type directly from "./Types".
-import WebhookConfigurationUI, { WebhookConfig } from "./WebhookConfigurationUI" +import WebhookConfigurationUI from "./WebhookConfigurationUI" +import { WebhookConfig } from "./Types"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
frontend/src/components/CredentialExample.tsx(1 hunks)frontend/src/components/CredentialModal.tsx(1 hunks)frontend/src/components/workflow/CredentialSelector.tsx(1 hunks)frontend/src/components/workflow/Types.ts(1 hunks)frontend/src/components/workflow/WebhookConfigurationUI.tsx(1 hunks)frontend/src/components/workflow/WorkflowBuilder.tsx(16 hunks)frontend/src/components/workflow/api/ApiHandlers.ts(3 hunks)server/types/workflowTypes.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
frontend/src/components/workflow/CredentialSelector.tsx (2)
frontend/src/components/workflow/api/ApiHandlers.ts (2)
Credential(5-18)credentialsAPI(703-841)frontend/src/components/CredentialModal.tsx (2)
CredentialData(17-22)CredentialModal(32-207)
frontend/src/components/CredentialExample.tsx (1)
frontend/src/components/CredentialModal.tsx (2)
CredentialData(17-22)CredentialModal(32-207)
frontend/src/components/workflow/WorkflowBuilder.tsx (4)
frontend/src/components/workflow/WorkflowIcons.tsx (1)
WebhookIcon(166-184)frontend/src/components/workflow/Types.ts (2)
WebhookConfig(303-313)Tool(56-61)frontend/src/components/workflow/WebhookConfigurationUI.tsx (3)
WebhookConfig(543-543)WebhookConfigurationUI(20-540)WebhookConfigurationUI(542-542)frontend/src/components/workflow/api/ApiHandlers.ts (1)
workflowToolsAPI(392-635)
frontend/src/components/CredentialModal.tsx (4)
frontend/src/components/ui/dialog.tsx (2)
Dialog(109-109)DialogContent(114-114)frontend/src/components/ui/button.tsx (1)
Button(57-57)frontend/src/components/ui/input.tsx (1)
Input(25-25)frontend/src/components/ui/select.tsx (5)
Select(111-111)SelectTrigger(114-114)SelectValue(113-113)SelectContent(115-115)SelectItem(117-117)
frontend/src/components/workflow/WebhookConfigurationUI.tsx (3)
frontend/src/components/workflow/Types.ts (1)
WebhookConfig(303-313)frontend/src/components/workflow/WorkflowIcons.tsx (1)
BackArrowIcon(393-413)frontend/src/components/workflow/CredentialSelector.tsx (1)
CredentialSelector(13-305)
frontend/src/components/workflow/Types.ts (1)
frontend/src/components/workflow/WebhookConfigurationUI.tsx (1)
WebhookConfig(543-543)
🔇 Additional comments (6)
frontend/src/components/workflow/api/ApiHandlers.ts (1)
494-498: Verify server expects camelCase keys in tool.valueExisting types elsewhere include webhook_url; confirm server contracts.
frontend/src/components/workflow/CredentialSelector.tsx (1)
1-5: Imports and API usage look consistentComponent correctly leverages credentialsAPI and CredentialModal.
server/types/workflowTypes.ts (1)
24-25: WEBHOOK accepted by all validators and DB enums
pgEnum("tool_type", Object.values(ToolType))inserver/db/schema/workflows.tsnow includes"webhook".- Zod schemas (
z.enum(Object.values(ToolType))) automatically permit"webhook".- No hard-coded allowlists or manual checks found in
server/api/workflow.ts.frontend/src/components/workflow/WebhookConfigurationUI.tsx (2)
151-169: Deduplicate copy success handling.Extract the setCopied/setTimeout logic and reuse in both paths.
Apply this diff:
const copyWebhookUrl = async () => { const url = config.path ? generateWebhookUrl() : `${getOrigin()}/webhook/your-path` try { await navigator.clipboard.writeText(url) - setCopied(true) - setTimeout(() => setCopied(false), 2000) // Reset after 2 seconds + handleCopySuccess() } catch (err) { console.error('Failed to copy URL:', err) // Fallback for browsers that don't support clipboard API const textArea = document.createElement('textarea') textArea.value = url document.body.appendChild(textArea) textArea.select() document.execCommand('copy') document.body.removeChild(textArea) - setCopied(true) - setTimeout(() => setCopied(false), 2000) + handleCopySuccess() } }Add this helper (outside the selected range, e.g., above copyWebhookUrl):
const handleCopySuccess = () => { setCopied(true) setTimeout(() => setCopied(false), 2000) }
201-223: Inline styles: prefer Tailwind utilities for consistency.The project uses Tailwind; convert these inline style objects to utility classes.
Also applies to: 228-238, 244-251
frontend/src/components/workflow/WorkflowBuilder.tsx (1)
843-849: SSR crash: window used during render.This executes while rendering StepNode; window is undefined in SSR.
- const url = webhookConfig?.webhookUrl || `${window.location.origin}/webhook${webhookConfig?.path || ''}` + const origin = + typeof window !== "undefined" ? window.location.origin : "" + const url = + webhookConfig?.webhookUrl || + `${origin}/webhook${webhookConfig?.path || ""}`Consider extracting a shared getOrigin()/buildWebhookUrl() util to de-duplicate across components.
| const handleCredentialSave = (data: CredentialData) => { | ||
| console.log("Credential saved:", data) | ||
| } |
There was a problem hiding this comment.
Do not log secrets
This logs user/password from the modal. Remove or redact before merging.
Apply:
- const handleCredentialSave = (data: CredentialData) => {
- console.log("Credential saved:", data)
- }
+ const handleCredentialSave = (_data: CredentialData) => {
+ // Persist securely; avoid logging secrets
+ }📝 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 handleCredentialSave = (data: CredentialData) => { | |
| console.log("Credential saved:", data) | |
| } | |
| const handleCredentialSave = (_data: CredentialData) => { | |
| // Persist securely; avoid logging secrets | |
| } |
🤖 Prompt for AI Agents
In frontend/src/components/CredentialExample.tsx around lines 8 to 10, the
handler currently logs full credential objects (including username and
password); remove the console.log of the raw data and either omit sensitive
fields or redact them before logging (e.g., replace password and any secret with
"***" or log only non-sensitive metadata), or better yet emit a non-logging
confirmation or use a safe telemetry event that excludes credentials; update the
handler to never print user/password to console or logs.
| // Format credential data for backend storage | ||
| const formatCredentialData = async () => { | ||
| console.log("🔐 Formatting credential data...") | ||
| if (webhookConfig.authentication === "none") { | ||
| console.log("ℹ️ No authentication, returning empty array") | ||
| return [] | ||
| } | ||
|
|
||
| try { | ||
| // Fetch all credentials of the required type | ||
| console.log("🔍 Fetching all credentials for auth type:", webhookConfig.authentication) | ||
| const allCredentials = await credentialsAPI.fetchByType(webhookConfig.authentication as "basic" | "bearer" | "api_key") | ||
|
|
||
| if (allCredentials.length === 0) { | ||
| console.log("ℹ️ No credentials found for this auth type") | ||
| return [] | ||
| } | ||
|
|
||
| console.log(`✅ Found ${allCredentials.length} credentials of type ${webhookConfig.authentication}`) | ||
|
|
||
| // Map all credentials with isSelected flag based on selectedCredential | ||
| const credentialData = allCredentials.map(credential => { | ||
| const isSelected = credential.id === webhookConfig.selectedCredential | ||
|
|
||
| // Create base64 encoding of user:password for basic auth | ||
| const basicAuth = btoa(`${credential.user}:${credential.password}`) | ||
|
|
||
| console.log(`📋 Processing credential: ${credential.name}, isSelected: ${isSelected}`) | ||
|
|
||
| return { | ||
| user: credential.user, | ||
| password: credential.password, // Note: In production, this should be encrypted | ||
| basic_auth: basicAuth, | ||
| isSelected, | ||
| name: credential.name, | ||
| allowedDomains: credential.allowedDomains | ||
| } | ||
| }) | ||
|
|
||
| console.log("📦 Formatted credential data with all credentials:", credentialData) | ||
| return credentialData | ||
| } catch (error) { | ||
| console.error("❌ Error formatting credential data:", error) | ||
| return [] | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Critical: Frontend collects and prepares to send secrets (user/password/base64) — remove immediately
Never assemble or transmit credential secrets from the client. Persist only a reference (credential ID); resolve secrets server-side.
Apply:
- // Format credential data for backend storage
- const formatCredentialData = async () => {
- console.log("🔐 Formatting credential data...")
- if (webhookConfig.authentication === "none") {
- console.log("ℹ️ No authentication, returning empty array")
- return []
- }
-
- try {
- // Fetch all credentials of the required type
- console.log("🔍 Fetching all credentials for auth type:", webhookConfig.authentication)
- const allCredentials = await credentialsAPI.fetchByType(webhookConfig.authentication as "basic" | "bearer" | "api_key")
-
- if (allCredentials.length === 0) {
- console.log("ℹ️ No credentials found for this auth type")
- return []
- }
-
- console.log(`✅ Found ${allCredentials.length} credentials of type ${webhookConfig.authentication}`)
-
- // Map all credentials with isSelected flag based on selectedCredential
- const credentialData = allCredentials.map(credential => {
- const isSelected = credential.id === webhookConfig.selectedCredential
-
- // Create base64 encoding of user:password for basic auth
- const basicAuth = btoa(`${credential.user}:${credential.password}`)
-
- console.log(`📋 Processing credential: ${credential.name}, isSelected: ${isSelected}`)
-
- return {
- user: credential.user,
- password: credential.password, // Note: In production, this should be encrypted
- basic_auth: basicAuth,
- isSelected,
- name: credential.name,
- allowedDomains: credential.allowedDomains
- }
- })
-
- console.log("📦 Formatted credential data with all credentials:", credentialData)
- return credentialData
- } catch (error) {
- console.error("❌ Error formatting credential data:", error)
- return []
- }
- }
+ // Persist only a reference to the selected credential; secrets must be resolved server-side
+ const credentialRef = webhookConfig.selectedCredential ?? nullCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/src/components/workflow/api/ApiHandlers.ts around lines 441 to 487,
the client-side code is assembling and potentially sending credential secrets
(user/password/base64); remove that behavior and only return credential
references: map fetched credentials to an object containing the credential id
and selection flag (e.g., { id, isSelected }) and optionally non-secret metadata
like name, and remove any user, password, basic_auth or base64 logic; update any
callers to expect IDs instead and ensure secret resolution (fetching actual
user/password) is performed on the server side only via a secure backend
endpoint.
| } | ||
| } | ||
|
|
||
| const credentialArray = await formatCredentialData() |
There was a problem hiding this comment.
Remove secret array generation
Stop awaiting and using the formatted secret array.
Apply:
- const credentialArray = await formatCredentialData()
+ // No secret material derived on the client📝 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 credentialArray = await formatCredentialData() | |
| // No secret material derived on the client |
🤖 Prompt for AI Agents
In frontend/src/components/workflow/api/ApiHandlers.ts around line 488, remove
the awaited assignment to credentialArray and stop using that formatted secret
array; either delete the line "const credentialArray = await
formatCredentialData()" and any subsequent references to credentialArray, or if
formatCredentialData must run for side-effects call it without awaiting (e.g.,
formatCredentialData()) and remove any code that reads from credentialArray.
| description: `${webhookConfig.httpMethod} ${webhookConfig.webhookUrl} • ${ | ||
| webhookConfig.authentication === 'none' ? 'No authentication' : 'Basic authentication' | ||
| }` |
There was a problem hiding this comment.
Fix auth description for non-basic types
Current text implies all non-none auth is “Basic”. Handle bearer/api_key.
Apply:
- description: `${webhookConfig.httpMethod} ${webhookConfig.webhookUrl} • ${
- webhookConfig.authentication === 'none' ? 'No authentication' : 'Basic authentication'
- }`
+ description: `${webhookConfig.httpMethod} ${webhookConfig.webhookUrl} • ${
+ webhookConfig.authentication === 'none'
+ ? 'No authentication'
+ : webhookConfig.authentication === 'basic'
+ ? 'Basic authentication'
+ : webhookConfig.authentication === 'bearer'
+ ? 'Bearer token'
+ : 'API key'
+ }`📝 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.
| description: `${webhookConfig.httpMethod} ${webhookConfig.webhookUrl} • ${ | |
| webhookConfig.authentication === 'none' ? 'No authentication' : 'Basic authentication' | |
| }` | |
| description: `${webhookConfig.httpMethod} ${webhookConfig.webhookUrl} • ${ | |
| webhookConfig.authentication === 'none' | |
| ? 'No authentication' | |
| : webhookConfig.authentication === 'basic' | |
| ? 'Basic authentication' | |
| : webhookConfig.authentication === 'bearer' | |
| ? 'Bearer token' | |
| : 'API key' | |
| }` |
🤖 Prompt for AI Agents
In frontend/src/components/workflow/api/ApiHandlers.ts around lines 499 to 501,
the description string currently treats any non-'none' authentication as "Basic
authentication"; update the interpolation to branch on
webhookConfig.authentication and produce correct text for each type (e.g., 'No
authentication' for 'none', 'Basic authentication' for 'basic', 'Bearer token
authentication' for 'bearer', 'API key authentication' for 'api_key', and a
sensible fallback like 'Authentication configured' for unknown values) so the
displayed description accurately reflects the auth method.
| config: { | ||
| // Store behavior configuration in config column | ||
| authentication: webhookConfig.authentication, | ||
| responseMode: webhookConfig.responseMode, | ||
| headers: webhookConfig.headers || {}, | ||
| queryParams: webhookConfig.queryParams || {}, | ||
| options: webhookConfig.options || {}, | ||
| credentials: credentialArray, // Array of credential objects with base64 auth | ||
| selectedCredential: webhookConfig.selectedCredential | ||
| } | ||
| } |
There was a problem hiding this comment.
Do not include credentials array in config
Remove credentials from payload; pass only selectedCredential reference.
Apply:
config: {
// Store behavior configuration in config column
authentication: webhookConfig.authentication,
responseMode: webhookConfig.responseMode,
headers: webhookConfig.headers || {},
queryParams: webhookConfig.queryParams || {},
- options: webhookConfig.options || {},
- credentials: credentialArray, // Array of credential objects with base64 auth
- selectedCredential: webhookConfig.selectedCredential
+ options: webhookConfig.options || {},
+ selectedCredential: credentialRef
}📝 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.
| config: { | |
| // Store behavior configuration in config column | |
| authentication: webhookConfig.authentication, | |
| responseMode: webhookConfig.responseMode, | |
| headers: webhookConfig.headers || {}, | |
| queryParams: webhookConfig.queryParams || {}, | |
| options: webhookConfig.options || {}, | |
| credentials: credentialArray, // Array of credential objects with base64 auth | |
| selectedCredential: webhookConfig.selectedCredential | |
| } | |
| } | |
| config: { | |
| // Store behavior configuration in config column | |
| authentication: webhookConfig.authentication, | |
| responseMode: webhookConfig.responseMode, | |
| headers: webhookConfig.headers || {}, | |
| queryParams: webhookConfig.queryParams || {}, | |
| options: webhookConfig.options || {}, | |
| selectedCredential: credentialRef | |
| } | |
| } |
🤖 Prompt for AI Agents
In frontend/src/components/workflow/api/ApiHandlers.ts around lines 503 to 513,
the config payload currently includes a credentials array (credentialArray)
which must be removed; update the config object to not send credentials and
instead only include selectedCredential (webhookConfig.selectedCredential).
Remove any reference to credentialArray from this payload construction and
ensure downstream callers or API accepts only the selectedCredential identifier;
if credentialArray is used elsewhere, keep it locally but do not serialize it
into config.
| description: `${webhookConfig.httpMethod} ${webhookConfig.webhookUrl} • ${ | ||
| webhookConfig.authentication === 'none' ? 'No authentication' : 'Basic authentication' | ||
| }` |
There was a problem hiding this comment.
Fix auth description in update flow
Apply:
- description: `${webhookConfig.httpMethod} ${webhookConfig.webhookUrl} • ${
- webhookConfig.authentication === 'none' ? 'No authentication' : 'Basic authentication'
- }`
+ description: `${webhookConfig.httpMethod} ${webhookConfig.webhookUrl} • ${
+ webhookConfig.authentication === 'none'
+ ? 'No authentication'
+ : webhookConfig.authentication === 'basic'
+ ? 'Basic authentication'
+ : webhookConfig.authentication === 'bearer'
+ ? 'Bearer token'
+ : 'API key'
+ }`📝 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.
| description: `${webhookConfig.httpMethod} ${webhookConfig.webhookUrl} • ${ | |
| webhookConfig.authentication === 'none' ? 'No authentication' : 'Basic authentication' | |
| }` | |
| description: `${webhookConfig.httpMethod} ${webhookConfig.webhookUrl} • ${ | |
| webhookConfig.authentication === 'none' | |
| ? 'No authentication' | |
| : webhookConfig.authentication === 'basic' | |
| ? 'Basic authentication' | |
| : webhookConfig.authentication === 'bearer' | |
| ? 'Bearer token' | |
| : 'API key' | |
| }` |
🤖 Prompt for AI Agents
In frontend/src/components/workflow/api/ApiHandlers.ts around lines 616-618, the
description text hardcodes "Basic authentication" for any non-'none' auth;
change it to reflect the actual webhookConfig.authentication value (e.g., map
'basic' -> 'Basic authentication', 'bearer' or 'token' -> 'Bearer
authentication', etc.) instead of always showing Basic, and handle undefined
values by defaulting to a readable label (or "Unknown authentication"); update
the template string to use that mapped label.
| // Store behavior configuration in config column | ||
| authentication: webhookConfig.authentication, | ||
| responseMode: webhookConfig.responseMode, | ||
| headers: webhookConfig.headers || {}, | ||
| queryParams: webhookConfig.queryParams || {}, | ||
| options: webhookConfig.options || {}, | ||
| credentials: credentialArray, // Array of credential objects with base64 auth | ||
| selectedCredential: webhookConfig.selectedCredential | ||
| } |
There was a problem hiding this comment.
Do not include credentials array in update payload
Apply:
headers: webhookConfig.headers || {},
queryParams: webhookConfig.queryParams || {},
options: webhookConfig.options || {},
- credentials: credentialArray, // Array of credential objects with base64 auth
- selectedCredential: webhookConfig.selectedCredential
+ selectedCredential: credentialRef📝 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.
| // Store behavior configuration in config column | |
| authentication: webhookConfig.authentication, | |
| responseMode: webhookConfig.responseMode, | |
| headers: webhookConfig.headers || {}, | |
| queryParams: webhookConfig.queryParams || {}, | |
| options: webhookConfig.options || {}, | |
| credentials: credentialArray, // Array of credential objects with base64 auth | |
| selectedCredential: webhookConfig.selectedCredential | |
| } | |
| // Store behavior configuration in config column | |
| authentication: webhookConfig.authentication, | |
| responseMode: webhookConfig.responseMode, | |
| headers: webhookConfig.headers || {}, | |
| queryParams: webhookConfig.queryParams || {}, | |
| options: webhookConfig.options || {}, | |
| selectedCredential: credentialRef | |
| } |
🤖 Prompt for AI Agents
In frontend/src/components/workflow/api/ApiHandlers.ts around lines 621 to 629,
the update payload is including the full credentials array (credentials:
credentialArray) which must not be sent; remove the credentials field from the
object constructed for the update and only send non-sensitive fields (e.g.,
selectedCredential if needed), or explicitly delete/omit credentials before
making the API call so no credential objects or base64 auth data are
transmitted.
| const credential = await credentialsAPI.update(editingCredential.id, { | ||
| name: updatedCredential.name, | ||
| user: updatedCredential.user, | ||
| password: updatedCredential.password, | ||
| allowedDomains: updatedCredential.allowedDomains | ||
| }) | ||
|
|
There was a problem hiding this comment.
Avoid wiping password on edit when left blank
Only include password in update if the user provided one.
Apply:
- const credential = await credentialsAPI.update(editingCredential.id, {
- name: updatedCredential.name,
- user: updatedCredential.user,
- password: updatedCredential.password,
- allowedDomains: updatedCredential.allowedDomains
- })
+ const updates: Partial<Credential> = {
+ name: updatedCredential.name,
+ user: updatedCredential.user,
+ allowedDomains: updatedCredential.allowedDomains,
+ }
+ if (updatedCredential.password?.trim()) {
+ updates.password = updatedCredential.password
+ }
+ const credential = await credentialsAPI.update(editingCredential.id, updates)📝 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 credential = await credentialsAPI.update(editingCredential.id, { | |
| name: updatedCredential.name, | |
| user: updatedCredential.user, | |
| password: updatedCredential.password, | |
| allowedDomains: updatedCredential.allowedDomains | |
| }) | |
| const updates: Partial<Credential> = { | |
| name: updatedCredential.name, | |
| user: updatedCredential.user, | |
| allowedDomains: updatedCredential.allowedDomains, | |
| } | |
| if (updatedCredential.password?.trim()) { | |
| updates.password = updatedCredential.password | |
| } | |
| const credential = await credentialsAPI.update(editingCredential.id, updates) |
🤖 Prompt for AI Agents
In frontend/src/components/workflow/CredentialSelector.tsx around lines 117 to
123, the current update call always sends password which wipes it when the field
is left blank; change to build the update payload dynamically (e.g., create an
object with name, user, allowedDomains) and only add the password property if
updatedCredential.password is non-empty/defined before calling
credentialsAPI.update so empty input does not overwrite the stored password.
| // Generate webhook URL based on the path | ||
| const generateWebhookUrl = () => { | ||
| const baseUrl = window.location.origin | ||
| const cleanPath = config.path?.startsWith('/') ? config.path : `/${config.path || ''}` | ||
| return `${baseUrl}/webhook${cleanPath}` | ||
| } |
There was a problem hiding this comment.
Guard window during URL generation (SSR-safe).
Accessing window.location.origin will crash on SSR. Use a small helper and reuse it.
Apply this diff:
const generateWebhookUrl = () => {
- const baseUrl = window.location.origin
+ const baseUrl = getOrigin()
const cleanPath = config.path?.startsWith('/') ? config.path : `/${config.path || ''}`
return `${baseUrl}/webhook${cleanPath}`
}Add this helper (outside the selected range, e.g., above generateWebhookUrl):
const getOrigin = () =>
typeof window !== "undefined" && window.location?.origin ? window.location.origin : ""🤖 Prompt for AI Agents
In frontend/src/components/workflow/WebhookConfigurationUI.tsx around lines 105
to 110, the generateWebhookUrl function directly accesses window.location.origin
which will crash during SSR; add a small SSR-safe helper function (e.g.,
getOrigin) above generateWebhookUrl that returns window.location.origin only
when typeof window !== "undefined" and window.location?.origin exists, otherwise
an empty string, then refactor generateWebhookUrl to call getOrigin() instead of
directly using window.location.origin so URL generation is safe and reusable.
| const copyWebhookUrl = async () => { | ||
| const url = config.path ? generateWebhookUrl() : `${window.location.origin}/webhook/your-path` | ||
| try { |
There was a problem hiding this comment.
Guard window in copy/fallback and UI render.
Replace direct window.location.origin usages.
- const url = config.path ? generateWebhookUrl() : `${window.location.origin}/webhook/your-path`
+ const url = config.path ? generateWebhookUrl() : `${getOrigin()}/webhook/your-path`- {config.path ? generateWebhookUrl() : `${window.location.origin}/webhook/your-path`}
+ {config.path ? generateWebhookUrl() : `${getOrigin()}/webhook/your-path`}Also applies to: 269-269
🤖 Prompt for AI Agents
In frontend/src/components/workflow/WebhookConfigurationUI.tsx around lines
151-153 (and also at line 269), the code uses window.location.origin directly
for the copied URL and fallback UI rendering; guard against window being
undefined (SSR or non-browser env) by deriving origin in a safe way—use typeof
window !== 'undefined' && window.location?.origin ? window.location.origin : an
appropriate fallback (e.g., empty string or process.env.BASE_URL) and update
both copyWebhookUrl and the UI render to use that safe origin value so no
runtime error occurs during server-side rendering or tests.
avirupsinha12
force-pushed
the
webhook-node-integration
branch
from
b364321 to
f5b2ae6
Compare
There was a problem hiding this comment.
Actionable comments posted: 15
♻️ Duplicate comments (11)
frontend/src/components/CredentialExample.tsx (1)
8-10: Remove credential logging (previously raised).This logs sensitive credential data including passwords to the console, which is a security risk.
As noted in the previous review, remove or redact sensitive fields before logging:
- const handleCredentialSave = (data: CredentialData) => { - console.log("Credential saved:", data) - } + const handleCredentialSave = (_data: CredentialData) => { + // Persist securely; avoid logging secrets + }frontend/src/components/workflow/CredentialSelector.tsx (1)
113-123: Avoid wiping password on edit when left blankBuild the update payload dynamically; only include password if provided.
- const credential = await credentialsAPI.update(editingCredential.id, { - name: updatedCredential.name, - user: updatedCredential.user, - password: updatedCredential.password, - allowedDomains: updatedCredential.allowedDomains - }) + const updates: Partial<Credential> = { + name: updatedCredential.name, + user: updatedCredential.user, + allowedDomains: updatedCredential.allowedDomains, + } + if (updatedCredential.password?.trim()) { + updates.password = updatedCredential.password + } + const credential = await credentialsAPI.update(editingCredential.id, updates)frontend/src/components/workflow/api/ApiHandlers.ts (8)
488-488: Remove unused secret array variableEliminate the credentialArray usage entirely.
- const credentialArray = await formatCredentialData() + // No secret material derived on the client
499-501: Accurate auth description textShow correct auth label for basic/bearer/api_key.
- description: `${webhookConfig.httpMethod} ${webhookConfig.webhookUrl} • ${ - webhookConfig.authentication === 'none' ? 'No authentication' : 'Basic authentication' - }` + description: `${webhookConfig.httpMethod} ${webhookConfig.webhookUrl} • ${ + webhookConfig.authentication === 'none' + ? 'No authentication' + : webhookConfig.authentication === 'basic' + ? 'Basic authentication' + : webhookConfig.authentication === 'bearer' + ? 'Bearer token' + : 'API key' + }`
605-605: Remove unused secret array variable (update flow)Delete credentialArray and all uses.
- const credentialArray = await formatCredentialData() + // No secret material derived on the client
616-618: Accurate auth description text (update flow)Use the same mapping for auth labels.
- description: `${webhookConfig.httpMethod} ${webhookConfig.webhookUrl} • ${ - webhookConfig.authentication === 'none' ? 'No authentication' : 'Basic authentication' - }` + description: `${webhookConfig.httpMethod} ${webhookConfig.webhookUrl} • ${ + webhookConfig.authentication === 'none' + ? 'No authentication' + : webhookConfig.authentication === 'basic' + ? 'Basic authentication' + : webhookConfig.authentication === 'bearer' + ? 'Bearer token' + : 'API key' + }`
441-487: Critical: Remove client-side secret handling; never assemble or transmit secrets from the frontendPersist only a credential reference (ID). Delete base64 derivation and plaintext password handling.
- // Format credential data for backend storage - const formatCredentialData = async () => { - console.log("🔐 Formatting credential data...") - if (webhookConfig.authentication === "none") { - console.log("ℹ️ No authentication, returning empty array") - return [] - } - - try { - // Fetch all credentials of the required type - console.log("🔍 Fetching all credentials for auth type:", webhookConfig.authentication) - const allCredentials = await credentialsAPI.fetchByType(webhookConfig.authentication as "basic" | "bearer" | "api_key") - - if (allCredentials.length === 0) { - console.log("ℹ️ No credentials found for this auth type") - return [] - } - - console.log(`✅ Found ${allCredentials.length} credentials of type ${webhookConfig.authentication}`) - - // Map all credentials with isSelected flag based on selectedCredential - const credentialData = allCredentials.map(credential => { - const isSelected = credential.id === webhookConfig.selectedCredential - - // Create base64 encoding of user:password for basic auth - const basicAuth = btoa(`${credential.user}:${credential.password}`) - - console.log(`📋 Processing credential: ${credential.name}, isSelected: ${isSelected}`) - - return { - user: credential.user, - password: credential.password, // Note: In production, this should be encrypted - basic_auth: basicAuth, - isSelected, - name: credential.name, - allowedDomains: credential.allowedDomains - } - }) - - console.log("📦 Formatted credential data with all credentials:", credentialData) - return credentialData - } catch (error) { - console.error("❌ Error formatting credential data:", error) - return [] - } - } + // Persist only a reference to the selected credential; secrets are resolved server-side + const credentialRef = webhookConfig.selectedCredential ?? null
503-513: Do not include credentials in payload; send only selectedCredential referenceRemove credentials array; include only credentialRef.
config: { // Store behavior configuration in config column authentication: webhookConfig.authentication, responseMode: webhookConfig.responseMode, headers: webhookConfig.headers || {}, queryParams: webhookConfig.queryParams || {}, options: webhookConfig.options || {}, - credentials: credentialArray, // Array of credential objects with base64 auth - selectedCredential: webhookConfig.selectedCredential + selectedCredential: credentialRef }
568-605: Critical: Same fix for update flow — no client-side secret handlingMirror the save fix: keep only the ID reference.
- // Format credential data for backend storage - const formatCredentialData = async () => { - if (webhookConfig.authentication === "none") { - return [] - } - - try { - // Fetch all credentials of the required type - const allCredentials = await credentialsAPI.fetchByType(webhookConfig.authentication as "basic" | "bearer" | "api_key") - - if (allCredentials.length === 0) { - return [] - } - - // Map all credentials with isSelected flag based on selectedCredential - const credentialData = allCredentials.map(credential => { - const isSelected = credential.id === webhookConfig.selectedCredential - - // Create base64 encoding of user:password for basic auth - const basicAuth = btoa(`${credential.user}:${credential.password}`) - - return { - user: credential.user, - password: credential.password, // Note: In production, this should be encrypted - basic_auth: basicAuth, - isSelected, - name: credential.name, - allowedDomains: credential.allowedDomains - } - }) - - return credentialData - } catch (error) { - console.error("Error formatting credential data:", error) - return [] - } - } + // Persist only a reference to the selected credential; secrets are resolved server-side + const credentialRef = webhookConfig.selectedCredential ?? null
621-629: Do not include credentials in update payloadKeep only the credentialRef.
headers: webhookConfig.headers || {}, queryParams: webhookConfig.queryParams || {}, options: webhookConfig.options || {}, - credentials: credentialArray, // Array of credential objects with base64 auth - selectedCredential: webhookConfig.selectedCredential + selectedCredential: credentialReffrontend/src/components/workflow/WorkflowBuilder.tsx (1)
842-849: Guard window access for SSR-safe renderingAvoid window in render; compute origin conditionally.
- const url = webhookConfig?.webhookUrl || `${window.location.origin}/webhook${webhookConfig?.path || ''}` + const origin = typeof window !== "undefined" ? window.location.origin : "" + const url = webhookConfig?.webhookUrl || `${origin}/webhook${webhookConfig?.path || ''}`
🧹 Nitpick comments (13)
server/services/webhookExecutionService.ts (1)
63-100: Strengthen type safety.The helper methods use
anytypes which reduces type safety and IntelliSense support.Consider creating explicit types for the template and steps:
import type { InferSelectModel } from 'drizzle-orm' type WorkflowTemplate = InferSelectModel<typeof workflowTemplate> type WorkflowStepTemplate = InferSelectModel<typeof workflowStepTemplate> private async getWorkflowTemplate(templateId: string): Promise<WorkflowTemplate | undefined> { const [template] = await db .select() .from(workflowTemplate) .where(eq(workflowTemplate.id, templateId)) .limit(1) return template } private async createWorkflowExecution(template: WorkflowTemplate, context: WebhookExecutionContext) { // ... implementation } private async getWorkflowSteps(templateId: string): Promise<WorkflowStepTemplate[]> { // ... implementation }server/services/webhookAuthService.ts (1)
211-215: Use static import for crypto module.Dynamic
requirefor the crypto module reduces type safety and prevents tree-shaking.Apply this diff:
+import crypto from 'crypto' + export class WebhookAuthService { // ... generateWebhookSecret(): string { // Generate a secure random secret for webhook signature validation - const crypto = require('crypto') return crypto.randomBytes(32).toString('hex') }server/services/webhookReloadService.ts (1)
45-47: Log the error object for stack/structured loggingPass the error object to Logger.error to preserve stack/details.
- Logger.error(`Failed to trigger webhook reload: ${errorMessage}`) + Logger.error(error, `Failed to trigger webhook reload: ${errorMessage}`)server/services/webhookRegistry.ts (1)
43-44: Normalize paths consistently (leading/trailing slashes)Prevent duplicate registrations for equivalent paths (e.g., "/foo" vs "foo/" vs "//foo").
- const cleanPath = path.startsWith('/') ? path : `/${path}` + const cleanPath = `/${String(path).replace(/^\/+/, "").replace(/\/+$/, "")}`Apply the same normalization in unregisterWebhook and getWebhook.
Also applies to: 62-63, 71-72
server/server.ts (1)
1160-1199: If you switch to the shared registry, adjust these endpoints accordinglyThese use the local Map’s size/entries. With the singleton, return webhookRegistry.getAllWebhooks().length and map of getAllWebhooks().
server/api/workflow.ts (1)
2564-2573: Don’t block critical API paths on webhook reload; use fire-and-forget
In server/api/workflow.ts (lines 2565–2569 and 2609–2613), replace bothawait triggerWebhookReload()blocks with non-blocking calls:- if (hasWebhookTools(createdTools)) { - Logger.info("🔄 Workflow contains webhook tools, triggering webhook reload...") - const reloadResult = await triggerWebhookReload() - if (reloadResult.success) { - Logger.info(`✅ Webhooks reloaded successfully: ${reloadResult.count} webhooks active`) - } else { - Logger.warn(`⚠️ Webhook reload failed: ${reloadResult.error}`) - } - } + if (hasWebhookTools(createdTools)) { + Logger.info("🔄 Workflow contains webhook tools, triggering webhook reload...") + triggerWebhookReload() + .then((res) => { + if (res.success) Logger.info(`✅ Webhooks reloaded: ${res.count} active`) + else Logger.warn(`⚠️ Webhook reload failed: ${res.error}`) + }) + .catch((e) => Logger.warn(`⚠️ Webhook reload threw: ${e instanceof Error ? e.message : String(e)}`)) + }- Logger.info("🔄 Template updated, triggering webhook reload to ensure webhooks are current...") - const reloadResult = await triggerWebhookReload() - if (reloadResult.success) { - Logger.info(`✅ Webhooks reloaded successfully: ${reloadResult.count} webhooks active`) - } else { - Logger.warn(`⚠️ Webhook reload failed: ${reloadResult.error}`) - } + Logger.info("🔄 Template updated, triggering webhook reload...") + triggerWebhookReload() + .then((res) => { + if (res.success) Logger.info(`✅ Webhooks reloaded: ${res.count} active`) + else Logger.warn(`⚠️ Webhook reload failed: ${res.error}`) + }) + .catch((e) => Logger.warn(`⚠️ Webhook reload threw: ${e instanceof Error ? e.message : String(e)}`))frontend/src/components/workflow/CredentialSelector.tsx (1)
94-102: Include only relevant fields per authType in create/update
Only senduser/passwordfor"basic",tokenfor"bearer", andapiKeyfor"api_key"(both in lines 94–102 and 113–123). For example:- const credential = await credentialsAPI.create({ - name: newCredential.name, - type: authType, - user: newCredential.user, - password: newCredential.password, - allowedDomains: newCredential.allowedDomains - }) + const base = { + name: newCredential.name, + type: authType, + allowedDomains: newCredential.allowedDomains + } + const payload = + authType === "basic" + ? { ...base, user: newCredential.user, password: newCredential.password } + : authType === "bearer" + ? { ...base, token: newCredential.token } + : authType === "api_key" + ? { ...base, apiKey: newCredential.apiKey } + : base + const credential = await credentialsAPI.create(payload as any)Apply the same pattern to the update call.
frontend/src/components/CredentialModal.tsx (2)
17-22: Narrow the type of allowedDomains to a unionPrevents invalid values and aligns with Select options.
export interface CredentialData { name: string user: string password: string - allowedDomains: string + allowedDomains: "All" | "Custom" }
169-185: Handle “Custom” domains selectionIf “Custom” is chosen, provide an input to capture allowed domains; otherwise remove the option for now to avoid dead-end UX.
server/api/webhook.ts (2)
36-69: Remove or disable the custom loader to avoid duplicate/incorrect registrationsThis function guesses templateId and may double-register. Prefer the integration service’s registerExistingWebhookTools.
172-179: Optionally include whitelisted headers in requestDataIf downstream needs headers (e.g., signature verification), pass selected headers instead of an empty object.
- headers: {}, + // Consider whitelisting specific headers instead of empty object + headers: Object.fromEntries(new URL(c.req.url).searchParams.entries()) /* replace with a header allowlist */,server/services/webhookIntegrationService.ts (2)
226-245: Optional: Avoid full-table scan to find templateIdFilter by toolId in SQL (array contains) to reduce load.
280-311: Optional: Validate path via DB filter instead of scanning all toolsPrefer querying for matching path in value JSON rather than iterating in JS.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
frontend/src/components/CredentialExample.tsx(1 hunks)frontend/src/components/CredentialModal.tsx(1 hunks)frontend/src/components/workflow/CredentialSelector.tsx(1 hunks)frontend/src/components/workflow/Types.ts(1 hunks)frontend/src/components/workflow/WebhookConfigurationUI.tsx(1 hunks)frontend/src/components/workflow/WorkflowBuilder.tsx(16 hunks)frontend/src/components/workflow/WorkflowIcons.tsx(0 hunks)frontend/src/components/workflow/api/ApiHandlers.ts(3 hunks)server/api/webhook.ts(1 hunks)server/api/workflow.ts(5 hunks)server/manual-webhook-register.ts(1 hunks)server/register-webhook.ts(1 hunks)server/server.ts(2 hunks)server/services/webhookAuthService.ts(1 hunks)server/services/webhookExecutionService.ts(1 hunks)server/services/webhookIntegrationService.ts(1 hunks)server/services/webhookRegistry.ts(1 hunks)server/services/webhookReloadService.ts(1 hunks)server/types/workflowTypes.ts(1 hunks)
💤 Files with no reviewable changes (1)
- frontend/src/components/workflow/WorkflowIcons.tsx
✅ Files skipped from review due to trivial changes (1)
- server/manual-webhook-register.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- server/types/workflowTypes.ts
- frontend/src/components/workflow/WebhookConfigurationUI.tsx
🧰 Additional context used
🧬 Code graph analysis (13)
server/register-webhook.ts (1)
server/services/webhookRegistry.ts (1)
webhookRegistry(88-88)
frontend/src/components/workflow/Types.ts (2)
frontend/src/components/workflow/WebhookConfigurationUI.tsx (1)
WebhookConfig(548-548)server/services/webhookRegistry.ts (1)
WebhookConfig(89-89)
server/services/webhookReloadService.ts (1)
server/logger/index.ts (2)
getLogger(36-93)Subsystem(15-15)
frontend/src/components/workflow/CredentialSelector.tsx (2)
frontend/src/components/workflow/api/ApiHandlers.ts (2)
Credential(5-18)credentialsAPI(703-841)frontend/src/components/CredentialModal.tsx (2)
CredentialData(17-22)CredentialModal(32-207)
server/api/workflow.ts (2)
server/services/webhookReloadService.ts (2)
hasWebhookTools(12-14)triggerWebhookReload(20-53)server/services/webhookRegistry.ts (1)
webhookRegistry(88-88)
server/api/webhook.ts (5)
server/logger/index.ts (2)
getLogger(36-93)Subsystem(15-15)server/db/schema/workflows.ts (1)
workflowTool(104-117)frontend/src/components/workflow/Types.ts (1)
WebhookConfig(303-313)server/services/webhookRegistry.ts (2)
WebhookConfig(89-89)webhookRegistry(88-88)server/services/webhookExecutionService.ts (1)
executeWorkflowFromWebhook(34-61)
server/services/webhookExecutionService.ts (2)
server/logger/index.ts (2)
getLogger(36-93)Subsystem(15-15)server/db/schema/workflows.ts (6)
workflowTemplate(60-76)workflowExecution(120-140)workflowStepTemplate(79-101)workflowStepExecution(143-169)workflowTool(104-117)toolExecution(172-189)
server/services/webhookIntegrationService.ts (3)
server/logger/index.ts (2)
getLogger(36-93)Subsystem(15-15)server/db/schema/workflows.ts (2)
workflowTool(104-117)workflowStepTemplate(79-101)server/services/webhookRegistry.ts (1)
webhookRegistry(88-88)
server/server.ts (5)
server/services/webhookRegistry.ts (1)
webhookRegistry(88-88)server/db/connector.ts (1)
db(3-3)server/db/client.ts (1)
db(15-15)server/db/schema/workflows.ts (5)
workflowTool(104-117)workflowStepTemplate(79-101)workflowTemplate(60-76)workflowExecution(120-140)workflowStepExecution(143-169)server/services/webhookExecutionService.ts (2)
executeWorkflowFromWebhook(34-61)startWorkflowExecution(166-185)
server/services/webhookAuthService.ts (1)
server/logger/index.ts (2)
getLogger(36-93)Subsystem(15-15)
server/services/webhookRegistry.ts (2)
server/logger/index.ts (2)
getLogger(36-93)Subsystem(15-15)frontend/src/components/workflow/Types.ts (1)
WebhookConfig(303-313)
frontend/src/components/workflow/WorkflowBuilder.tsx (4)
frontend/src/components/workflow/WorkflowIcons.tsx (1)
WebhookIcon(166-184)frontend/src/components/workflow/Types.ts (2)
WebhookConfig(303-313)Tool(56-61)frontend/src/components/workflow/WebhookConfigurationUI.tsx (3)
WebhookConfig(548-548)WebhookConfigurationUI(20-545)WebhookConfigurationUI(547-547)frontend/src/components/workflow/api/ApiHandlers.ts (1)
workflowToolsAPI(392-635)
frontend/src/components/CredentialExample.tsx (1)
frontend/src/components/CredentialModal.tsx (2)
CredentialData(17-22)CredentialModal(32-207)
🔇 Additional comments (5)
server/services/webhookExecutionService.ts (3)
1-15: LGTM!Imports and logger setup follow the established patterns in the codebase.
17-32: LGTM!The interface clearly defines webhook execution context, and the singleton pattern is correctly implemented.
34-61: LGTM!The main orchestration flow is well-structured with proper error handling and logging.
server/services/webhookAuthService.ts (1)
1-24: LGTM!Imports, interface definition, and singleton setup follow established patterns.
frontend/src/components/workflow/Types.ts (1)
303-313: LGTM!The
WebhookConfiginterface is well-defined with appropriate types for webhook configuration. The fields align with the webhook functionality across the codebase.
| // Load webhooks on startup | ||
| loadWebhooksFromDatabase() | ||
|
|
There was a problem hiding this comment.
Critical: Replace ad-hoc DB loader; it registers wrong workflowTemplateId
Using tool.id as workflowTemplateId breaks execution. Delegate to the integration service that resolves template relationships and registers properly.
-// Load webhooks on startup
-loadWebhooksFromDatabase()
+// Load webhooks on startup via integration service (resolves template/tool links)
+await webhookIntegrationService.registerExistingWebhookTools()Based on learnings
📝 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.
| // Load webhooks on startup | |
| loadWebhooksFromDatabase() | |
| // Load webhooks on startup via integration service (resolves template/tool links) | |
| await webhookIntegrationService.registerExistingWebhookTools() |
🤖 Prompt for AI Agents
In server/api/webhook.ts around lines 71 to 73 the ad-hoc call
loadWebhooksFromDatabase() is using tool.id as workflowTemplateId and must be
replaced with the integration service that resolves template relationships and
performs registration correctly; remove the direct DB loader call and instead
invoke the integration service method (the service that maps tools to workflow
templates and registers webhooks), ensure you pass the correct identifiers (not
tool.id) so the service can resolve the actual workflowTemplateId, await the
service call, propagate or log errors appropriately, and add/update tests to
cover the registration flow.
| if (method !== webhookData.httpMethod) { | ||
| await webhookAuthService.auditWebhookAccess(c, path, false, `Method ${method} not allowed`) | ||
| throw new HTTPException(405, { | ||
| message: `Method ${method} not allowed. Expected ${webhookData.httpMethod}` | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Method check likely should read from config; also set Allow header
If registry doesn’t expose httpMethod at top-level, read webhookData.config.httpMethod. Return an Allow header for 405.
- if (method !== webhookData.httpMethod) {
+ const expectedMethod = webhookData.config?.httpMethod ?? webhookData.httpMethod
+ if (method !== expectedMethod) {
await webhookAuthService.auditWebhookAccess(c, path, false, `Method ${method} not allowed`)
- throw new HTTPException(405, {
- message: `Method ${method} not allowed. Expected ${webhookData.httpMethod}`
- })
+ c.header("Allow", expectedMethod)
+ throw new HTTPException(405, {
+ message: `Method ${method} not allowed. Expected ${expectedMethod}`
+ })
}Run to confirm registry shapes:
🏁 Script executed:
#!/bin/bash
# Inspect registry API to verify where httpMethod lives and getWebhook shape
fd -a webhookRegistry.ts
rg -n -C3 'registerWebhook\s*\('
rg -n -C3 'getWebhook\s*\('
rg -n -C3 'interface\s+.*Webhook' server/services/webhookRegistry.ts || trueLength of output: 8759
🏁 Script executed:
#!/bin/bash
# Display WebhookRegistrationService implementation and getWebhook return type
echo "=== server/services/webhookRegistry.ts (lines 1-200) ==="
sed -n '1,200p' server/services/webhookRegistry.ts
echo -e "\n=== server/api/webhook.ts (lines 70-120) ==="
sed -n '70,120p' server/api/webhook.tsLength of output: 4510
Add Allow header on 405 response
In server/api/webhook.ts’s handleWebhookRequest, before throwing the 405, set the Allow header using the existing webhookData.httpMethod (which already reflects the configured method).
Example:
await webhookAuthService.auditWebhookAccess(c, path, false, `Method ${method} not allowed`)
c.header("Allow", webhookData.httpMethod)
throw new HTTPException(405, {
message: `Method ${method} not allowed. Expected ${webhookData.httpMethod}`
})🤖 Prompt for AI Agents
In server/api/webhook.ts around lines 88 to 93, before throwing the 405
HTTPException when the request method doesn't match webhookData.httpMethod, set
the response "Allow" header to webhookData.httpMethod and then throw the 405;
keep the existing auditWebhookAccess call, call c.header("Allow",
webhookData.httpMethod) immediately after auditing and before throwing, and
ensure the header value matches the configured method string.
| // If this is a webhook tool, register the webhook | ||
| if (tool.type === ToolType.WEBHOOK && tool.config && tool.value) { | ||
| try { | ||
| const config = tool.config as any | ||
| const value = tool.value as any | ||
|
|
||
| if (config.path || value.path) { | ||
| const webhookConfig = { | ||
| webhookUrl: value.webhookUrl || `http://localhost:3000/webhook${config.path || value.path}`, | ||
| httpMethod: config.httpMethod || 'POST', | ||
| path: config.path || value.path, | ||
| authentication: config.authentication || 'none', | ||
| selectedCredential: config.selectedCredential, | ||
| responseMode: config.responseMode || 'immediately', | ||
| options: config.options || {}, | ||
| headers: config.headers || {}, | ||
| queryParams: config.queryParams || {} | ||
| } | ||
|
|
||
| // Get workflow template ID (simplified - in real implementation you'd get this from context) | ||
| const templateId = requestData.workflowTemplateId || 'default-template' | ||
|
|
||
| await webhookRegistry.registerWebhook( | ||
| webhookConfig.path, | ||
| templateId, | ||
| tool.id, | ||
| webhookConfig | ||
| ) | ||
|
|
||
| Logger.info(`Registered webhook ${webhookConfig.path} for tool ${tool.id}`) | ||
| } | ||
| } catch (webhookError) { | ||
| Logger.error(webhookError, `Failed to register webhook for tool ${tool.id}`) | ||
| // Don't fail the tool creation if webhook registration fails | ||
| } | ||
| } |
There was a problem hiding this comment.
Wrong registry and unsafe default template; unify registration or just reload
This registers into services/webhookRegistry, but runtime uses a different in-memory Map in server/server.ts. Registration here won’t affect live webhook handling. Also, using 'default-template' when workflowTemplateId isn’t provided can misroute executions.
Option A (simplest): drop per-tool registration and trigger a reload.
- // If this is a webhook tool, register the webhook
- if (tool.type === ToolType.WEBHOOK && tool.config && tool.value) {
- try {
- const config = tool.config as any
- const value = tool.value as any
-
- if (config.path || value.path) {
- const webhookConfig = {
- webhookUrl: value.webhookUrl || `http://localhost:3000/webhook${config.path || value.path}`,
- httpMethod: config.httpMethod || 'POST',
- path: config.path || value.path,
- authentication: config.authentication || 'none',
- selectedCredential: config.selectedCredential,
- responseMode: config.responseMode || 'immediately',
- options: config.options || {},
- headers: config.headers || {},
- queryParams: config.queryParams || {}
- }
-
- // Get workflow template ID (simplified - in real implementation you'd get this from context)
- const templateId = requestData.workflowTemplateId || 'default-template'
-
- await webhookRegistry.registerWebhook(
- webhookConfig.path,
- templateId,
- tool.id,
- webhookConfig
- )
-
- Logger.info(`Registered webhook ${webhookConfig.path} for tool ${tool.id}`)
- }
- } catch (webhookError) {
- Logger.error(webhookError, `Failed to register webhook for tool ${tool.id}`)
- // Don't fail the tool creation if webhook registration fails
- }
- }
+ // If this is a webhook tool, trigger a registry reload (runtime source of truth)
+ if (tool.type === ToolType.WEBHOOK) {
+ triggerWebhookReload().catch((e) =>
+ Logger.warn(`Webhook reload after tool create failed: ${e instanceof Error ? e.message : String(e)}`),
+ )
+ }Option B: migrate server/server.ts to use the shared services/webhookRegistry for a single source of truth, then keep per-tool register/unregister. I can draft that refactor.
📝 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.
| // If this is a webhook tool, register the webhook | |
| if (tool.type === ToolType.WEBHOOK && tool.config && tool.value) { | |
| try { | |
| const config = tool.config as any | |
| const value = tool.value as any | |
| if (config.path || value.path) { | |
| const webhookConfig = { | |
| webhookUrl: value.webhookUrl || `http://localhost:3000/webhook${config.path || value.path}`, | |
| httpMethod: config.httpMethod || 'POST', | |
| path: config.path || value.path, | |
| authentication: config.authentication || 'none', | |
| selectedCredential: config.selectedCredential, | |
| responseMode: config.responseMode || 'immediately', | |
| options: config.options || {}, | |
| headers: config.headers || {}, | |
| queryParams: config.queryParams || {} | |
| } | |
| // Get workflow template ID (simplified - in real implementation you'd get this from context) | |
| const templateId = requestData.workflowTemplateId || 'default-template' | |
| await webhookRegistry.registerWebhook( | |
| webhookConfig.path, | |
| templateId, | |
| tool.id, | |
| webhookConfig | |
| ) | |
| Logger.info(`Registered webhook ${webhookConfig.path} for tool ${tool.id}`) | |
| } | |
| } catch (webhookError) { | |
| Logger.error(webhookError, `Failed to register webhook for tool ${tool.id}`) | |
| // Don't fail the tool creation if webhook registration fails | |
| } | |
| } | |
| // If this is a webhook tool, trigger a registry reload (runtime source of truth) | |
| if (tool.type === ToolType.WEBHOOK) { | |
| triggerWebhookReload().catch((e) => | |
| Logger.warn( | |
| `Webhook reload after tool create failed: ${ | |
| e instanceof Error ? e.message : String(e) | |
| }`, | |
| ), | |
| ) | |
| } |
🤖 Prompt for AI Agents
In server/api/workflow.ts around lines 2777-2812, the code registers webhooks
into services/webhookRegistry (which is not the runtime registry) and falls back
to the unsafe 'default-template' id; instead, remove the per-tool call to
webhookRegistry.register and trigger the runtime to reload its live webhook map
(or emit the existing reload/update event) so the live in-memory registry in
server/server.ts picks up the new tool; also stop using 'default-template' —
require requestData.workflowTemplateId (return a 4xx if missing) or pass the
explicit template id from context so webhooks are mapped correctly. Ensure the
try/catch still prevents tool creation failure but uses the reload mechanism or
error response rather than registering in the wrong registry.
| // If this is a webhook tool, update the webhook registration | ||
| if (result.tool.type === ToolType.WEBHOOK && result.tool.config && result.tool.value) { | ||
| try { | ||
| const config = result.tool.config as any | ||
| const value = result.tool.value as any | ||
|
|
||
| if (config.path || value.path) { | ||
| const webhookConfig = { | ||
| webhookUrl: value.webhookUrl || `http://localhost:3000/webhook${config.path || value.path}`, | ||
| httpMethod: config.httpMethod || 'POST', | ||
| path: config.path || value.path, | ||
| authentication: config.authentication || 'none', | ||
| selectedCredential: config.selectedCredential, | ||
| responseMode: config.responseMode || 'immediately', | ||
| options: config.options || {}, | ||
| headers: config.headers || {}, | ||
| queryParams: config.queryParams || {} | ||
| } | ||
|
|
||
| // Get workflow template ID (simplified - in real implementation you'd get this from context) | ||
| const templateId = requestData.workflowTemplateId || 'default-template' | ||
|
|
||
| // Unregister old webhook first (if path changed) | ||
| if (existingTool[0] && existingTool[0].value) { | ||
| const oldValue = existingTool[0].value as any | ||
| if (oldValue.path && oldValue.path !== webhookConfig.path) { | ||
| await webhookRegistry.unregisterWebhook(oldValue.path) | ||
| } | ||
| } | ||
|
|
||
| // Register new/updated webhook | ||
| await webhookRegistry.registerWebhook( | ||
| webhookConfig.path, | ||
| templateId, | ||
| result.tool.id, | ||
| webhookConfig | ||
| ) | ||
|
|
||
| Logger.info(`Updated webhook registration ${webhookConfig.path} for tool ${result.tool.id}`) | ||
| } | ||
| } catch (webhookError) { | ||
| Logger.error(webhookError, `Failed to update webhook for tool ${result.tool.id}`) | ||
| // Don't fail the tool update if webhook registration fails | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Unregister old path robustly and avoid hardcoded base URL
- Only checking oldValue.path misses old config.path. Unregister whichever old path exists.
- Avoid
http://localhost:3000; derive from config.port.
- const config = result.tool.config as any
- const value = result.tool.value as any
+ const config = result.tool.config as any
+ const value = result.tool.value as any
if (config.path || value.path) {
- const webhookConfig = {
- webhookUrl: value.webhookUrl || `http://localhost:3000/webhook${config.path || value.path}`,
+ const webhookConfig = {
+ webhookUrl: value.webhookUrl || `http://127.0.0.1:${config.port}/webhook${config.path || value.path}`,
httpMethod: config.httpMethod || 'POST',
path: config.path || value.path,
authentication: config.authentication || 'none',
selectedCredential: config.selectedCredential,
responseMode: config.responseMode || 'immediately',
options: config.options || {},
headers: config.headers || {},
queryParams: config.queryParams || {}
}
// Get workflow template ID (simplified - in real implementation you'd get this from context)
const templateId = requestData.workflowTemplateId || 'default-template'
// Unregister old webhook first (if path changed)
- if (existingTool[0] && existingTool[0].value) {
- const oldValue = existingTool[0].value as any
- if (oldValue.path && oldValue.path !== webhookConfig.path) {
- await webhookRegistry.unregisterWebhook(oldValue.path)
- }
- }
+ const prevCfg = existingTool[0]?.config as any
+ const prevVal = existingTool[0]?.value as any
+ const oldPath = (prevCfg && prevCfg.path) || (prevVal && prevVal.path)
+ if (oldPath && oldPath !== webhookConfig.path) {
+ await webhookRegistry.unregisterWebhook(oldPath)
+ }Note: consider eliminating the 'default-template' fallback; require a real templateId or skip registration.
📝 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.
| // If this is a webhook tool, update the webhook registration | |
| if (result.tool.type === ToolType.WEBHOOK && result.tool.config && result.tool.value) { | |
| try { | |
| const config = result.tool.config as any | |
| const value = result.tool.value as any | |
| if (config.path || value.path) { | |
| const webhookConfig = { | |
| webhookUrl: value.webhookUrl || `http://localhost:3000/webhook${config.path || value.path}`, | |
| httpMethod: config.httpMethod || 'POST', | |
| path: config.path || value.path, | |
| authentication: config.authentication || 'none', | |
| selectedCredential: config.selectedCredential, | |
| responseMode: config.responseMode || 'immediately', | |
| options: config.options || {}, | |
| headers: config.headers || {}, | |
| queryParams: config.queryParams || {} | |
| } | |
| // Get workflow template ID (simplified - in real implementation you'd get this from context) | |
| const templateId = requestData.workflowTemplateId || 'default-template' | |
| // Unregister old webhook first (if path changed) | |
| if (existingTool[0] && existingTool[0].value) { | |
| const oldValue = existingTool[0].value as any | |
| if (oldValue.path && oldValue.path !== webhookConfig.path) { | |
| await webhookRegistry.unregisterWebhook(oldValue.path) | |
| } | |
| } | |
| // Register new/updated webhook | |
| await webhookRegistry.registerWebhook( | |
| webhookConfig.path, | |
| templateId, | |
| result.tool.id, | |
| webhookConfig | |
| ) | |
| Logger.info(`Updated webhook registration ${webhookConfig.path} for tool ${result.tool.id}`) | |
| } | |
| } catch (webhookError) { | |
| Logger.error(webhookError, `Failed to update webhook for tool ${result.tool.id}`) | |
| // Don't fail the tool update if webhook registration fails | |
| } | |
| } | |
| // If this is a webhook tool, update the webhook registration | |
| if (result.tool.type === ToolType.WEBHOOK && result.tool.config && result.tool.value) { | |
| try { | |
| const config = result.tool.config as any | |
| const value = result.tool.value as any | |
| if (config.path || value.path) { | |
| const webhookConfig = { | |
| webhookUrl: value.webhookUrl || `http://127.0.0.1:${config.port}/webhook${config.path || value.path}`, | |
| httpMethod: config.httpMethod || 'POST', | |
| path: config.path || value.path, | |
| authentication: config.authentication || 'none', | |
| selectedCredential: config.selectedCredential, | |
| responseMode: config.responseMode || 'immediately', | |
| options: config.options || {}, | |
| headers: config.headers || {}, | |
| queryParams: config.queryParams || {} | |
| } | |
| // Get workflow template ID (simplified - in real implementation you'd get this from context) | |
| const templateId = requestData.workflowTemplateId || 'default-template' | |
| // Unregister old webhook first (if path changed) | |
| const prevCfg = existingTool[0]?.config as any | |
| const prevVal = existingTool[0]?.value as any | |
| const oldPath = (prevCfg && prevCfg.path) || (prevVal && prevVal.path) | |
| if (oldPath && oldPath !== webhookConfig.path) { | |
| await webhookRegistry.unregisterWebhook(oldPath) | |
| } | |
| // Register new/updated webhook | |
| await webhookRegistry.registerWebhook( | |
| webhookConfig.path, | |
| templateId, | |
| result.tool.id, | |
| webhookConfig | |
| ) | |
| Logger.info(`Updated webhook registration ${webhookConfig.path} for tool ${result.tool.id}`) | |
| } | |
| } catch (webhookError) { | |
| Logger.error(webhookError, `Failed to update webhook for tool ${result.tool.id}`) | |
| // Don't fail the tool update if webhook registration fails | |
| } | |
| } |
🤖 Prompt for AI Agents
In server/api/workflow.ts around lines 2894 to 2939, the webhook-update block
should unregister the previous path from either the old value or old config (not
just oldValue.path), avoid hardcoding the base URL, and stop using a fallback
'default-template'; instead require a real workflowTemplateId or skip
registration. Fix by checking both existingTool[0].value.path and
existingTool[0].config.path when deciding what to unregister, build webhookUrl
from the runtime config (e.g., derive host/port from config or request
environment) instead of using "http://localhost:3000", and if
requestData.workflowTemplateId is missing skip registration and log a warning
(do not use 'default-template').
| async function registerTestWebhook() { | ||
| try { | ||
| console.log('🔧 Manually registering webhook...') | ||
|
|
||
| const webhookConfig = { | ||
| webhookUrl: "http://localhost:3000/webhook/test", | ||
| httpMethod: "POST" as const, | ||
| path: "/test", | ||
| authentication: "none" as const, | ||
| responseMode: "immediately" as const, | ||
| headers: {}, | ||
| queryParams: {}, | ||
| options: {} | ||
| } | ||
|
|
||
| await webhookRegistry.registerWebhook( | ||
| "/test", | ||
| "manual-template-id", | ||
| "manual-tool-id", | ||
| webhookConfig | ||
| ) | ||
|
|
||
| console.log('✅ Webhook registered successfully!') | ||
| console.log('🔗 Test with: curl -X POST "http://localhost:3000/webhook/test" -H "Content-Type: application/json" -d \'{"test": true}\'') | ||
|
|
||
| // List all webhooks | ||
| const webhooks = webhookRegistry.getAllWebhooks() | ||
| console.log('📋 All registered webhooks:', webhooks) | ||
|
|
||
| } catch (error) { | ||
| console.error('❌ Registration failed:', error) | ||
| } | ||
| } |
There was a problem hiding this comment.
Use valid database IDs or document requirements.
Lines 21-22 use hardcoded placeholder IDs ("manual-template-id" and "manual-tool-id") that don't exist in the database. This will cause runtime failures when the webhook is triggered and the workflow execution service tries to query these entities.
Consider one of these approaches:
- Query for valid IDs dynamically:
async function registerTestWebhook() {
try {
console.log('🔧 Manually registering webhook...')
// Fetch a real workflow template
const [template] = await db
.select()
.from(workflowTemplate)
.limit(1)
if (!template) {
throw new Error('No workflow templates found. Create one first.')
}
// Fetch or create a webhook tool
const [tool] = await db
.select()
.from(workflowTool)
.where(eq(workflowTool.type, ToolType.WEBHOOK))
.limit(1)
if (!tool) {
throw new Error('No webhook tools found. Create one first.')
}
const webhookConfig = {
webhookUrl: "http://localhost:3000/webhook/test",
httpMethod: "POST" as const,
path: "/test",
authentication: "none" as const,
responseMode: "immediately" as const,
headers: {},
queryParams: {},
options: {}
}
await webhookRegistry.registerWebhook(
"/test",
template.id,
tool.id,
webhookConfig
)
console.log('✅ Webhook registered successfully!')
console.log('🔗 Test with: curl -X POST "http://localhost:3000/webhook/test" -H "Content-Type: application/json" -d \'{"test": true}\'')
const webhooks = webhookRegistry.getAllWebhooks()
console.log('📋 All registered webhooks:', webhooks)
} catch (error) {
console.error('❌ Registration failed:', error)
}
}- Or add clear documentation:
// NOTE: Replace these placeholder IDs with actual template and tool IDs from your database:
// 1. Query workflow_template table for a valid template ID
// 2. Query workflow_tool table for a valid tool ID with type='webhook'
await webhookRegistry.registerWebhook(
"/test",
"REPLACE_WITH_REAL_TEMPLATE_ID", // e.g., from workflow_template.id
"REPLACE_WITH_REAL_TOOL_ID", // e.g., from workflow_tool.id where type='webhook'
webhookConfig
)🤖 Prompt for AI Agents
In server/register-webhook.ts around lines 4 to 36: the code registers a test
webhook using hardcoded placeholder IDs ("manual-template-id" and
"manual-tool-id") which will fail at runtime because those entities don't exist;
replace the placeholders with valid IDs by querying the database for a real
workflow template and a webhook-type workflow tool (or throw a clear error if
none exist), then pass template.id and tool.id into
webhookRegistry.registerWebhook, or alternatively add a prominent comment
instructing developers to replace the placeholders with actual IDs from the
workflow_template and workflow_tool tables before running.
| async completeExecution(executionId: string, result?: any) { | ||
| try { | ||
| await db | ||
| .update(workflowExecution) | ||
| .set({ | ||
| status: WorkflowStatus.COMPLETED, | ||
| completedAt: new Date(), | ||
| metadata: result ? { ...result, completedAt: new Date().toISOString() } : undefined, | ||
| updatedAt: new Date() | ||
| }) | ||
| .where(eq(workflowExecution.id, executionId)) | ||
|
|
||
| Logger.info(`Workflow execution ${executionId} completed`) | ||
|
|
||
| } catch (error) { | ||
| Logger.error(`Failed to complete execution ${executionId}: ${error}`) | ||
| throw error | ||
| } | ||
| } |
There was a problem hiding this comment.
Preserve existing metadata when completing execution.
Line 224 completely overwrites the execution's metadata when result is provided, which will lose important trigger information (webhookPath, requestData, triggeredAt) that was stored during creation.
Apply this diff to merge metadata instead:
async completeExecution(executionId: string, result?: any) {
try {
+ // Fetch current execution to preserve existing metadata
+ const [currentExecution] = await db
+ .select()
+ .from(workflowExecution)
+ .where(eq(workflowExecution.id, executionId))
+ .limit(1)
+
await db
.update(workflowExecution)
.set({
status: WorkflowStatus.COMPLETED,
completedAt: new Date(),
- metadata: result ? { ...result, completedAt: new Date().toISOString() } : undefined,
+ metadata: result
+ ? { ...(currentExecution?.metadata || {}), ...result, completedAt: new Date().toISOString() }
+ : { ...(currentExecution?.metadata || {}), completedAt: new Date().toISOString() },
updatedAt: new Date()
})
.where(eq(workflowExecution.id, executionId))
Logger.info(`Workflow execution ${executionId} completed`)
} catch (error) {
Logger.error(`Failed to complete execution ${executionId}: ${error}`)
throw error
}
}📝 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.
| async completeExecution(executionId: string, result?: any) { | |
| try { | |
| await db | |
| .update(workflowExecution) | |
| .set({ | |
| status: WorkflowStatus.COMPLETED, | |
| completedAt: new Date(), | |
| metadata: result ? { ...result, completedAt: new Date().toISOString() } : undefined, | |
| updatedAt: new Date() | |
| }) | |
| .where(eq(workflowExecution.id, executionId)) | |
| Logger.info(`Workflow execution ${executionId} completed`) | |
| } catch (error) { | |
| Logger.error(`Failed to complete execution ${executionId}: ${error}`) | |
| throw error | |
| } | |
| } | |
| async completeExecution(executionId: string, result?: any) { | |
| try { | |
| // Fetch current execution to preserve existing metadata | |
| const [currentExecution] = await db | |
| .select() | |
| .from(workflowExecution) | |
| .where(eq(workflowExecution.id, executionId)) | |
| .limit(1) | |
| await db | |
| .update(workflowExecution) | |
| .set({ | |
| status: WorkflowStatus.COMPLETED, | |
| completedAt: new Date(), | |
| metadata: result | |
| ? { ...(currentExecution?.metadata || {}), ...result, completedAt: new Date().toISOString() } | |
| : { ...(currentExecution?.metadata || {}), completedAt: new Date().toISOString() }, | |
| updatedAt: new Date() | |
| }) | |
| .where(eq(workflowExecution.id, executionId)) | |
| Logger.info(`Workflow execution ${executionId} completed`) | |
| } catch (error) { | |
| Logger.error(`Failed to complete execution ${executionId}: ${error}`) | |
| throw error | |
| } | |
| } |
| async failExecution(executionId: string, error: any) { | ||
| try { | ||
| await db | ||
| .update(workflowExecution) | ||
| .set({ | ||
| status: WorkflowStatus.FAILED, | ||
| metadata: { | ||
| error: error.toString(), | ||
| failedAt: new Date().toISOString() | ||
| }, | ||
| updatedAt: new Date() | ||
| }) | ||
| .where(eq(workflowExecution.id, executionId)) | ||
|
|
||
| Logger.error(`Workflow execution ${executionId} failed: ${error}`) | ||
|
|
||
| } catch (dbError) { | ||
| Logger.error(`Failed to mark execution ${executionId} as failed: ${dbError}`) | ||
| throw dbError | ||
| } | ||
| } |
There was a problem hiding this comment.
Preserve existing metadata when failing execution.
Similar to completeExecution, this method overwrites all metadata instead of merging with existing data.
Apply this diff:
async failExecution(executionId: string, error: any) {
try {
+ // Fetch current execution to preserve existing metadata
+ const [currentExecution] = await db
+ .select()
+ .from(workflowExecution)
+ .where(eq(workflowExecution.id, executionId))
+ .limit(1)
+
await db
.update(workflowExecution)
.set({
status: WorkflowStatus.FAILED,
- metadata: {
+ metadata: {
+ ...(currentExecution?.metadata || {}),
error: error.toString(),
failedAt: new Date().toISOString()
},
updatedAt: new Date()
})
.where(eq(workflowExecution.id, executionId))
Logger.error(`Workflow execution ${executionId} failed: ${error}`)
} catch (dbError) {
Logger.error(`Failed to mark execution ${executionId} as failed: ${dbError}`)
throw dbError
}
}📝 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.
| async failExecution(executionId: string, error: any) { | |
| try { | |
| await db | |
| .update(workflowExecution) | |
| .set({ | |
| status: WorkflowStatus.FAILED, | |
| metadata: { | |
| error: error.toString(), | |
| failedAt: new Date().toISOString() | |
| }, | |
| updatedAt: new Date() | |
| }) | |
| .where(eq(workflowExecution.id, executionId)) | |
| Logger.error(`Workflow execution ${executionId} failed: ${error}`) | |
| } catch (dbError) { | |
| Logger.error(`Failed to mark execution ${executionId} as failed: ${dbError}`) | |
| throw dbError | |
| } | |
| } | |
| async failExecution(executionId: string, error: any) { | |
| try { | |
| // Fetch current execution to preserve existing metadata | |
| const [currentExecution] = await db | |
| .select() | |
| .from(workflowExecution) | |
| .where(eq(workflowExecution.id, executionId)) | |
| .limit(1) | |
| await db | |
| .update(workflowExecution) | |
| .set({ | |
| status: WorkflowStatus.FAILED, | |
| metadata: { | |
| ...(currentExecution?.metadata || {}), | |
| error: error.toString(), | |
| failedAt: new Date().toISOString(), | |
| }, | |
| updatedAt: new Date() | |
| }) | |
| .where(eq(workflowExecution.id, executionId)) | |
| Logger.error(`Workflow execution ${executionId} failed: ${error}`) | |
| } catch (dbError) { | |
| Logger.error(`Failed to mark execution ${executionId} as failed: ${dbError}`) | |
| throw dbError | |
| } | |
| } |
| // Unregister old webhook if path is changing | ||
| if (config.path && config.path !== existingValue?.path) { | ||
| await webhookRegistry.unregisterWebhook(existingValue.path) | ||
| } | ||
|
|
There was a problem hiding this comment.
Guard unregister when old path is undefined
Prevent calling unregister with undefined.
- if (config.path && config.path !== existingValue?.path) {
- await webhookRegistry.unregisterWebhook(existingValue.path)
- }
+ if (config.path && config.path !== existingValue?.path) {
+ if (existingValue?.path) {
+ await webhookRegistry.unregisterWebhook(existingValue.path)
+ }
+ }📝 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.
| // Unregister old webhook if path is changing | |
| if (config.path && config.path !== existingValue?.path) { | |
| await webhookRegistry.unregisterWebhook(existingValue.path) | |
| } | |
| // Unregister old webhook if path is changing | |
| if (config.path && config.path !== existingValue?.path) { | |
| if (existingValue?.path) { | |
| await webhookRegistry.unregisterWebhook(existingValue.path) | |
| } | |
| } |
🤖 Prompt for AI Agents
In server/services/webhookIntegrationService.ts around lines 99 to 103, the code
calls webhookRegistry.unregisterWebhook(existingValue.path) without ensuring
existingValue.path is defined; add a guard so you only call unregisterWebhook
when existingValue?.path is truthy (e.g., change condition to check config.path
&& config.path !== existingValue?.path && existingValue?.path) to prevent
passing undefined to unregisterWebhook.
| // Re-register webhook with new configuration | ||
| if (config.path) { | ||
| const fullConfig = { ...existingConfig, ...updatedConfig, path: config.path } | ||
| // Get workflow template ID from step template | ||
| const templateId = await this.getWorkflowTemplateIdFromTool(toolId) | ||
|
|
||
| if (templateId) { | ||
| await webhookRegistry.registerWebhook( | ||
| config.path, | ||
| templateId, | ||
| toolId, | ||
| fullConfig as WebhookToolConfig | ||
| ) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Update registry even when only config changes (path unchanged)
Currently, changed httpMethod/auth/headers aren’t reflected in the registry unless path changes.
- // Re-register webhook with new configuration
- if (config.path) {
- const fullConfig = { ...existingConfig, ...updatedConfig, path: config.path }
- // Get workflow template ID from step template
- const templateId = await this.getWorkflowTemplateIdFromTool(toolId)
-
- if (templateId) {
- await webhookRegistry.registerWebhook(
- config.path,
- templateId,
- toolId,
- fullConfig as WebhookToolConfig
- )
- }
- }
+ // Re-register (or refresh) webhook with updated configuration
+ const pathToUse: string | undefined = (config.path ?? existingValue?.path)
+ if (pathToUse) {
+ const fullConfig = { ...existingConfig, ...updatedConfig, path: pathToUse }
+ // Get workflow template ID from step template
+ const templateId = await this.getWorkflowTemplateIdFromTool(toolId)
+ if (templateId) {
+ await webhookRegistry.registerWebhook(
+ pathToUse,
+ templateId,
+ toolId,
+ fullConfig as WebhookToolConfig
+ )
+ }
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In server/services/webhookIntegrationService.ts around lines 131 to 146, the
code only calls webhookRegistry.registerWebhook when config.path is present, so
changes to httpMethod/auth/headers are not propagated if the path is unchanged;
update the logic to always compute fullConfig (merge existingConfig and
updatedConfig), determine the effective path (use config.path if present else
existingConfig.path), fetch the workflow template ID as before, and call
webhookRegistry.registerWebhook with the effective path, templateId, toolId and
fullConfig so registry entries are updated even when only non-path config fields
change.
| const response = await fetch('http://localhost:3000/webhook-api/reload', { | ||
| method: 'GET', | ||
| headers: { | ||
| 'Content-Type': 'application/json' | ||
| } | ||
| }) | ||
|
|
||
| if (!response.ok) { | ||
| throw new Error(`HTTP ${response.status}: ${response.statusText}`) | ||
| } |
There was a problem hiding this comment.
Avoid hardcoded localhost and add request timeout (prevents hung request on API path)
This GET uses a fixed port and no timeout. On request threads, a hung call can stall the API. Use config.port and AbortController with a short timeout.
Apply:
- // Make internal HTTP request to the webhook reload endpoint
- const response = await fetch('http://localhost:3000/webhook-api/reload', {
- method: 'GET',
- headers: {
- 'Content-Type': 'application/json'
- }
- })
+ // Make internal HTTP request to the webhook reload endpoint (with timeout)
+ const controller = new AbortController()
+ const timeoutId = setTimeout(() => controller.abort(), 5000)
+ const baseUrl = `http://127.0.0.1:${config.port}`
+ const response = await fetch(`${baseUrl}/webhook-api/reload`, {
+ method: 'GET',
+ signal: controller.signal,
+ })
+ clearTimeout(timeoutId)And add at top of file:
import config from "@/config"🤖 Prompt for AI Agents
In server/services/webhookReloadService.ts around lines 25 to 34, the fetch call
is using a hardcoded localhost:3000 URL and has no timeout; change it to use
config.port (import config from "@/config" at top of the file) and wrap the
fetch with an AbortController that times out (e.g., 2–5s) by calling setTimeout
to abort the controller and clearing the timer after response; build the URL
using `http://localhost:${config.port}/webhook-api/reload`, pass signal to
fetch, and handle AbortError to throw a clear timeout error and ensure the timer
is cleaned up in all code paths.
2 participants
Description
Testing
Additional Notes
Summary by CodeRabbit
New Features
Style