Add list of webhook functions as dropdown in call a webhook node #172
Conversation
|
Warning Rate limit exceeded@AmishaBisht has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 52 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdds function-based webhook selection to the WebhookRouterForm: introduces Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WebhookForm as WebhookRouterForm
participant Completion as Completion Endpoint
participant State as FormState
rect rgba(200,220,255,0.18)
Note over WebhookForm: componentDidMount -> fetch completion options
WebhookForm->>Completion: GET config.endpoints.completion
Completion-->>WebhookForm: options[]
WebhookForm->>State: set webhookOptions
end
rect rgba(220,255,220,0.18)
Note over User,WebhookForm: user selects FUNCTION and picks a function
User->>WebhookForm: set httpMethod = FUNCTION
User->>WebhookForm: select webhookFunction
WebhookForm->>WebhookForm: handleWebhookFunctionChanged()
WebhookForm->>State: update webhookFunction, url, body
end
rect rgba(255,245,205,0.18)
Note over WebhookForm: render & validation
WebhookForm->>WebhookForm: render TembaSelectElement (FUNCTION)
WebhookForm->>WebhookForm: skip empty-body JSON validation when FUNCTION
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/flow/routers/webhook/WebhookRouterForm.tsx (1)
272-281: Require function selection when method is FUNCTIONCurrently you can save without selecting a function. Add a guard and use the enum for comparison.
- let valid = false; - if (this.state.method.value.name === 'FUNCTION') { - valid = this.handleUpdate({ resultName: this.state.resultName.value }, true); + let valid = false; + if (this.state.method.value.value === Methods.FUNCTION) { + if (!this.state.webhookFunction.value) { + const updated = mergeForm(this.state, { + webhookFunction: { + value: this.state.webhookFunction.value, + validationFailures: [{ message: i18n.t('forms.function_required', 'Select a function') }] + } + }) as WebhookRouterFormState; + this.setState(updated); + return; + } + valid = this.handleUpdate({ resultName: this.state.resultName.value }, true); } else { valid = this.handleUpdate( { url: this.state.url.value, resultName: this.state.resultName.value }, true ); }
🧹 Nitpick comments (7)
src/components/flow/routers/webhook/helpers.ts (1)
60-68: Avoid duplicating defaults for webhookFunctionnodeToState seeds webhookFunction here, and WebhookRouterForm also re-seeds it in the constructor. Keep the default in one place (prefer this helper) to prevent drift.
src/components/flow/routers/webhook/WebhookRouterForm.tsx (6)
52-54: Tighten types for webhookFunction and webhookOptionsPrefer explicit types to reduce any-casts and runtime surprises.
-export interface WebhookRouterFormState extends FormState { +type WebhookOption = { name: string; body?: string }; +export interface WebhookRouterFormState extends FormState { headers: HeaderEntry[]; method: MethodEntry; url: StringEntry; body: StringEntry; resultName: StringEntry; - webhookFunction: FormEntry; - webhookOptions: { name: string; body?: string }[]; + webhookFunction: FormEntry /* value: WebhookOption | null */; + webhookOptions: WebhookOption[]; }
60-63: Legacy contextTypes accepted, but consider modern Context APINo blocker; optional migration to static contextType or useContext for future refactors.
67-72: Remove redundant initialization of webhook defaultsnodeToState already sets webhookFunction; avoid re-declaring it here.
- this.state = { - ...nodeToState(this.props.nodeSettings), - webhookFunction: { value: null }, - webhookOptions: [] - }; + this.state = { ...nodeToState(this.props.nodeSettings), webhookOptions: [] };
78-90: Harden fetch: guard config, handle errors, and preselect existing functionAdd try/catch and avoid setState after unmount; also preselect based on existing url when method is FUNCTION.
- async componentDidMount() { - const endpoint = this.context.config.endpoints.completion; - const response = await fetch(endpoint); - const data = await response.json(); - - const webhookOptions = (data.webhook || []).map((webhook: any) => ({ - name: webhook.name, - body: webhook.body - })); - - this.setState({ webhookOptions }); - } + private _unmounted = false; + async componentDidMount() { + try { + const endpoint = this.context?.config?.endpoints?.completion; + if (!endpoint) return; + const res = await fetch(endpoint, { credentials: 'same-origin' }); + if (!res.ok) throw new Error(`Fetch failed: ${res.status}`); + const data = await res.json(); + const webhookOptions = (data.webhook || []).map((w: any) => ({ name: w.name, body: w.body })); + if (this._unmounted) return; + const updates: Partial<WebhookRouterFormState> = { webhookOptions }; + // Preselect if editing existing FUNCTION using saved url as the key + if (this.state.method.value.value === Methods.FUNCTION && this.state.url.value) { + const sel = webhookOptions.find(w => w.name === this.state.url.value); + if (sel) updates.webhookFunction = { value: sel }; + } + this.setState(updates as WebhookRouterFormState); + } catch { + if (!this._unmounted) this.setState({ webhookOptions: [] } as any); + } + } + componentWillUnmount() { + this._unmounted = true; + }Please confirm the endpoint key is correct (completion vs completions/functions) and that the payload indeed provides
data.webhookwith{ name, body }.
199-207: Body validation: tiny copy tweak and behavior LGTMLogic to skip JSON validation for empty FUNCTION body looks good. Consider generic label.
- updates.body = validate('POST body', keys.body, [isValidJson()]); + updates.body = validate(i18n.t('forms.request_body', 'Request body'), keys.body, [isValidJson()]);
396-421: Use enum in conditionals to avoid string literalsImproves robustness and refactor-safety.
- {method === 'FUNCTION' ? ( + {method === Methods.FUNCTION ? ( <TembaSelectElement key="webhook_function_select" name={i18n.t('forms.function', 'Function')}Also update the method constant upstream:
- const method = this.state.method.value.value; + const method = this.state.method.value.value as Methods;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/flow/routers/webhook/WebhookRouterForm.tsx(7 hunks)src/components/flow/routers/webhook/helpers.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/flow/routers/webhook/WebhookRouterForm.tsx (7)
src/store/nodeEditor.ts (2)
FormEntry(22-25)mergeForm(63-156)src/components/flow/routers/llm/LLMForm.tsx (1)
WebhookRouterForm(24-174)src/config/ConfigProvider.tsx (1)
fakePropType(4-4)src/components/flow/routers/webhook/helpers.ts (2)
nodeToState(55-109)getDefaultBody(155-157)src/store/validators.ts (1)
validate(118-132)src/temba/TembaSelectElement.tsx (1)
TembaSelectElement(13-26)src/components/form/textinput/TextInputElement.tsx (1)
TextInputElement(35-94)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Coverage
🔇 Additional comments (4)
src/components/flow/routers/webhook/WebhookRouterForm.tsx (4)
19-19: Import looks goodTembaSelectElement import is appropriate for the new dropdown.
36-36: Context import OKfakePropType matches existing pattern in the codebase.
157-160: Default body logic LGTMSkipping default body for FUNCTION avoids unintended payloads.
91-127: Refactor to store single selected item, not array, when multi={false}The TembaSelect component with
multi={false}(line 403) invokes onChange with a single item:resolved[0](TembaSelect.tsx lines 175). The current handler defensively wraps this into an array, creating a type/shape mismatch. Whenmulti={false}, store the selected item directly, not wrapped in an array.For the URL coupling: confirm with your backend that storing the webhook function name as the URL value is the expected behavior.
There was a problem hiding this comment.
@AmishaBisht , there are a lot of compiler issues content?.default won’t work since we’re using Node 16. You’ll need to explicitly check if a value exists before using it, and skip it if it’s not needed.
even after removing the optional chaining getting errors
|
I am using node version 16 only and its working fine for me😅 were you in the correct BE branch when you tested the PR? |
it working now , leave it thanks |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/components/flow/routers/webhook/WebhookRouterForm.tsx (3)
53-53: Remove the empty comment or add meaningful documentation.The trailing
//comment is empty and provides no value.Apply this diff to remove the empty comment:
- webhookOptions: any[]; // + webhookOptions: any[];Additionally, consider defining a proper type for webhook options instead of
any[]:interface WebhookOption { name: string; value: string; id: string; label: string; body: string; }Then use
webhookOptions: WebhookOption[];
82-88: Add defensive checks for webhook properties.The code directly accesses
webhook.nameandwebhook.bodywithout verifying they exist. If the API returns malformed data, this could create invalid options withundefinedvalues.Apply this diff to add validation:
- const webhookOptions = (data.webhook || []).map((webhook: any) => ({ - name: webhook.name, - value: webhook.name, - id: webhook.name, - label: webhook.name, - body: webhook.body - })); + const webhookOptions = (data.webhook || []) + .filter((webhook: any) => webhook && webhook.name) + .map((webhook: any) => ({ + name: webhook.name, + value: webhook.name, + id: webhook.name, + label: webhook.name, + body: webhook.body || '' + }));
67-70: Redundant initialization of webhookOptions.The constructor explicitly sets
webhookOptions: [], but according to thenodeToStatehelper inhelpers.ts(line 63),webhookOptionsis already initialized to[]. This override is redundant but harmless.Apply this diff to simplify:
this.state = { - ...nodeToState(this.props.nodeSettings), - webhookOptions: [] + ...nodeToState(this.props.nodeSettings) };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/flow/routers/webhook/WebhookRouterForm.tsx(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/flow/routers/webhook/WebhookRouterForm.tsx (6)
src/store/nodeEditor.ts (2)
FormEntry(22-25)mergeForm(63-156)src/config/ConfigProvider.tsx (1)
fakePropType(4-4)src/components/flow/routers/webhook/helpers.ts (3)
nodeToState(55-110)getDefaultBody(156-158)isValidJson(160-170)src/store/validators.ts (1)
validate(118-132)src/temba/TembaSelectElement.tsx (1)
TembaSelectElement(13-26)src/components/form/textinput/TextInputElement.tsx (1)
TextInputElement(35-94)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Coverage
🔇 Additional comments (6)
src/components/flow/routers/webhook/WebhookRouterForm.tsx (6)
103-125: LGTM! Webhook function handler correctly manages state.The handler properly clears the webhook function, URL, and body when deselected, and updates them appropriately when a function is selected. The use of optional chaining for accessing
selected?.name,selected?.value, andselected?.bodyis appropriate sinceselectedcan be null or undefined.
155-158: LGTM! Correct conditional logic for default body.The change appropriately skips setting a default body when switching to the FUNCTION method, allowing the body to be populated from the selected webhook function instead.
272-279: LGTM! Appropriate validation logic for FUNCTION method.The save handler correctly skips URL validation for the FUNCTION method since the URL is auto-populated from the selected webhook function. This prevents unnecessary validation errors.
394-414: LGTM! Clean conditional rendering for function selection.The conditional rendering correctly displays a searchable dropdown for webhook functions when the FUNCTION method is selected, and falls back to the standard URL input for other methods. The props configuration is appropriate for single-selection function picker use case.
60-62: Using legacy Context API (consistent with codebase patterns).The code uses the legacy
contextTypespattern to access configuration. While React 16.3+ offers the newer Context API, this approach is consistent with the existing codebase patterns (as evidenced by thefakePropTypeutility). No changes needed.
198-204: JSON validation for non-empty FUNCTION method bodies is intentional and correct.The code design allows FUNCTION methods to have optional JSON body parameters: empty bodies skip validation to support "no body" scenarios, while non-empty bodies must be valid JSON. This is clearly indicated by the comment and the explicit conditional check. When switching methods to FUNCTION, the body is preserved from the previous method, allowing users to retain or edit it. The
stateToNodefunction passes the body through directly, and the CallWebhook action accepts it as-is, making this behavior semantically sound.
| async componentDidMount() { | ||
| const endpoint = this.context.config.endpoints.completion; | ||
| const response = await fetch(endpoint); | ||
| const data = await response.json(); | ||
|
|
||
| const webhookOptions = (data.webhook || []).map((webhook: any) => ({ | ||
| name: webhook.name, | ||
| value: webhook.name, | ||
| id: webhook.name, | ||
| label: webhook.name, | ||
| body: webhook.body | ||
| })); | ||
|
|
||
| if (this.state.method.value.value === Methods.FUNCTION && this.state.url.value) { | ||
| const functionName = this.state.url.value; | ||
| const selectedOption = webhookOptions.find(opt => opt.name === functionName); | ||
|
|
||
| this.setState({ | ||
| webhookOptions, | ||
| webhookFunction: { value: selectedOption || null } | ||
| }); | ||
| } else { | ||
| this.setState({ webhookOptions }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add error handling for the async fetch operation.
The fetch call has no error handling, which could lead to unhandled promise rejections if the network request fails or the endpoint is unavailable.
Apply this diff to add error handling:
async componentDidMount() {
- const endpoint = this.context.config.endpoints.completion;
- const response = await fetch(endpoint);
- const data = await response.json();
-
- const webhookOptions = (data.webhook || []).map((webhook: any) => ({
- name: webhook.name,
- value: webhook.name,
- id: webhook.name,
- label: webhook.name,
- body: webhook.body
- }));
-
- if (this.state.method.value.value === Methods.FUNCTION && this.state.url.value) {
- const functionName = this.state.url.value;
- const selectedOption = webhookOptions.find(opt => opt.name === functionName);
-
- this.setState({
- webhookOptions,
- webhookFunction: { value: selectedOption || null }
- });
- } else {
- this.setState({ webhookOptions });
- }
+ try {
+ const endpoint = this.context.config.endpoints.completion;
+ const response = await fetch(endpoint);
+
+ if (!response.ok) {
+ console.error('Failed to fetch webhook options:', response.statusText);
+ return;
+ }
+
+ const data = await response.json();
+
+ const webhookOptions = (data.webhook || []).map((webhook: any) => ({
+ name: webhook.name,
+ value: webhook.name,
+ id: webhook.name,
+ label: webhook.name,
+ body: webhook.body
+ }));
+
+ if (this.state.method.value.value === Methods.FUNCTION && this.state.url.value) {
+ const functionName = this.state.url.value;
+ const selectedOption = webhookOptions.find(opt => opt.name === functionName);
+
+ this.setState({
+ webhookOptions,
+ webhookFunction: { value: selectedOption || null }
+ });
+ } else {
+ this.setState({ webhookOptions });
+ }
+ } catch (error) {
+ console.error('Error loading webhook options:', 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 componentDidMount() { | |
| const endpoint = this.context.config.endpoints.completion; | |
| const response = await fetch(endpoint); | |
| const data = await response.json(); | |
| const webhookOptions = (data.webhook || []).map((webhook: any) => ({ | |
| name: webhook.name, | |
| value: webhook.name, | |
| id: webhook.name, | |
| label: webhook.name, | |
| body: webhook.body | |
| })); | |
| if (this.state.method.value.value === Methods.FUNCTION && this.state.url.value) { | |
| const functionName = this.state.url.value; | |
| const selectedOption = webhookOptions.find(opt => opt.name === functionName); | |
| this.setState({ | |
| webhookOptions, | |
| webhookFunction: { value: selectedOption || null } | |
| }); | |
| } else { | |
| this.setState({ webhookOptions }); | |
| } | |
| } | |
| async componentDidMount() { | |
| try { | |
| const endpoint = this.context.config.endpoints.completion; | |
| const response = await fetch(endpoint); | |
| if (!response.ok) { | |
| console.error('Failed to fetch webhook options:', response.statusText); | |
| return; | |
| } | |
| const data = await response.json(); | |
| const webhookOptions = (data.webhook || []).map((webhook: any) => ({ | |
| name: webhook.name, | |
| value: webhook.name, | |
| id: webhook.name, | |
| label: webhook.name, | |
| body: webhook.body | |
| })); | |
| if (this.state.method.value.value === Methods.FUNCTION && this.state.url.value) { | |
| const functionName = this.state.url.value; | |
| const selectedOption = webhookOptions.find(opt => opt.name === functionName); | |
| this.setState({ | |
| webhookOptions, | |
| webhookFunction: { value: selectedOption || null } | |
| }); | |
| } else { | |
| this.setState({ webhookOptions }); | |
| } | |
| } catch (error) { | |
| console.error('Error loading webhook options:', error); | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/components/flow/routers/webhook/WebhookRouterForm.tsx (1)
77-101: Add error handling for the async fetch operation.The fetch call lacks error handling, which could lead to unhandled promise rejections if the network request fails or the endpoint is unavailable. This concern was previously flagged and remains unaddressed.
🧹 Nitpick comments (1)
src/components/flow/routers/webhook/WebhookRouterForm.tsx (1)
52-53: Consider adding proper typing forwebhookOptions.The
webhookOptionsarray usesany[], which loses type safety. Based on the mapping logic incomponentDidMount(lines 82-88), consider defining an interface for the webhook option structure.Define an interface and apply it:
+interface WebhookOption { + name: string; + value: string; + id: string; + label: string; + body: string; +} + export interface WebhookRouterFormState extends FormState { headers: HeaderEntry[]; method: MethodEntry; url: StringEntry; body: StringEntry; resultName: StringEntry; webhookFunction: FormEntry; - webhookOptions: any[]; + webhookOptions: WebhookOption[]; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/flow/routers/webhook/WebhookRouterForm.tsx(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/flow/routers/webhook/WebhookRouterForm.tsx (7)
src/store/nodeEditor.ts (2)
FormEntry(22-25)mergeForm(63-156)src/components/flow/props.ts (1)
RouterFormProps(35-47)src/config/ConfigProvider.tsx (1)
fakePropType(4-4)src/components/flow/routers/webhook/helpers.ts (3)
nodeToState(55-110)getDefaultBody(156-158)isValidJson(160-170)src/store/validators.ts (1)
validate(118-132)src/temba/TembaSelectElement.tsx (1)
TembaSelectElement(13-26)src/components/form/textinput/TextInputElement.tsx (1)
TextInputElement(35-94)
🪛 GitHub Actions: Build
src/components/flow/routers/webhook/WebhookRouterForm.tsx
[error] 92-92: TypeScript error in WebhookRouterForm.tsx(92,50): Parameter 'opt' implicitly has an 'any' type. TS7006
🔇 Additional comments (2)
src/components/flow/routers/webhook/WebhookRouterForm.tsx (2)
155-158: LGTM! Proper handling of FUNCTION method validation.The changes correctly:
- Skip default body initialization for FUNCTION methods (lines 155-158)
- Skip JSON validation for empty FUNCTION body while maintaining validation for non-empty bodies (lines 198-204)
This aligns well with the feature requirements where FUNCTION methods may have empty or pre-populated bodies.
Also applies to: 198-204
394-414: LGTM! Clean conditional rendering for FUNCTION vs URL input.The conditional rendering correctly switches between:
TembaSelectElementfor function selection when method is FUNCTIONTextInputElementfor URL input for other HTTP methodsThe props are properly wired to their respective handlers.
| private handleWebhookFunctionChanged(selected: any): boolean { | ||
| if (!selected) { | ||
| const updates: Partial<WebhookRouterFormState> = { | ||
| webhookFunction: { value: null }, | ||
| url: { value: '' }, | ||
| body: { value: '' } | ||
| }; | ||
| const updated = mergeForm(this.state, updates) as WebhookRouterFormState; | ||
| this.setState(updated); | ||
| return updated.valid; | ||
| } | ||
|
|
||
| const updates: Partial<WebhookRouterFormState> = { | ||
| webhookFunction: { value: selected }, | ||
| url: { value: selected?.name || selected?.value || '' }, | ||
| body: { value: selected?.body || '' } | ||
| }; | ||
|
|
||
| const updated = mergeForm(this.state, updates) as WebhookRouterFormState; | ||
| this.setState(updated); | ||
|
|
||
| return updated.valid; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify optional chaining compatibility with the project's Node version.
The PR comments mention concerns about optional chaining (?.) compatibility with Node 16. While one contributor confirmed it works, ensure your TypeScript target and build configuration properly support this syntax.
Run the following to verify the TypeScript configuration:
🏁 Script executed:
#!/bin/bash
# Check TypeScript configuration for target compatibility
if [ -f "tsconfig.json" ]; then
echo "=== TypeScript Configuration ==="
jq '.compilerOptions.target, .compilerOptions.lib' tsconfig.json
fi
# Check Node version requirements
if [ -f "package.json" ]; then
echo -e "\n=== Node Engine Requirements ==="
jq '.engines' package.json
fiLength of output: 119
Critical: Optional chaining syntax incompatible with TypeScript target ES5.
Lines 117-118 use optional chaining (selected?.name, selected?.value, selected?.body), but the project's TypeScript configuration targets ES5, which does not support this syntax. Optional chaining was introduced in ES2020.
Either:
- Update
tsconfig.jsonto target ES2020 or later, or - Replace optional chaining with traditional checks (e.g.,
selected && selected.name)
🤖 Prompt for AI Agents
In src/components/flow/routers/webhook/WebhookRouterForm.tsx around lines 103 to
125, the optional chaining usage (selected?.name, selected?.value,
selected?.body) is incompatible with the project's TS target ES5; replace those
with explicit null/undefined checks so the code works under ES5 — for example,
set url to use (selected && (selected.name || selected.value) || '') and body to
(selected && selected.body || ''), update the webhookFunction assignment to use
selected directly as before, then call mergeForm/setState as-is; alternatively,
if you prefer to use optional chaining project-wide, change tsconfig.json target
to "ES2020" or later (but do only one approach, not both).
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/flow/routers/webhook/WebhookRouterForm.tsx (1)
270-279: Add validation for function selection and use constant instead of string literal.Two issues here:
- When the method is
FUNCTION, the validation only checksresultNamebut doesn't verify that a function was actually selected. This allows saving with an empty function selection.- Line 272 uses a string literal
'FUNCTION'instead of theMethods.FUNCTIONconstant.Apply this diff to fix both issues:
private handleSave(): void { let valid = false; - if (this.state.method.value.name === 'FUNCTION') { + if (this.state.method.value.value === Methods.FUNCTION) { + // Validate that a function is selected + if (!this.state.webhookFunction.value) { + const updates: Partial<WebhookRouterFormState> = { + webhookFunction: { + value: null, + validationFailures: [{ message: 'Please select a function' }] + } + }; + this.setState(mergeForm(this.state, updates) as WebhookRouterFormState); + return; + } valid = this.handleUpdate({ resultName: this.state.resultName.value }, true); } else { valid = this.handleUpdate( { url: this.state.url.value, resultName: this.state.resultName.value }, true ); } if (valid) { this.props.updateRouter(stateToNode(this.props.nodeSettings, this.state)); this.props.onClose(false); } }
♻️ Duplicate comments (2)
src/components/flow/routers/webhook/WebhookRouterForm.tsx (2)
103-125: Replace optional chaining with explicit checks for ES5 compatibility.This issue was flagged in previous reviews but remains unaddressed. Lines 117-118 use optional chaining (
selected?.name,selected?.value,selected?.body), which is incompatible with the project's TypeScript target of ES5. Optional chaining requires ES2020 or later.Apply this diff to replace optional chaining with explicit checks:
private handleWebhookFunctionChanged(selected: any): boolean { if (!selected) { const updates: Partial<WebhookRouterFormState> = { webhookFunction: { value: null }, url: { value: '' }, body: { value: '' } }; const updated = mergeForm(this.state, updates) as WebhookRouterFormState; this.setState(updated); return updated.valid; } const updates: Partial<WebhookRouterFormState> = { webhookFunction: { value: selected }, - url: { value: selected?.name || selected?.value || '' }, - body: { value: selected?.body || '' } + url: { value: (selected && (selected.name || selected.value)) || '' }, + body: { value: (selected && selected.body) || '' } }; const updated = mergeForm(this.state, updates) as WebhookRouterFormState; this.setState(updated); return updated.valid; }
77-101: Add error handling and abort capability for the async fetch operation.This issue was flagged in previous reviews but remains unaddressed. The fetch call lacks error handling, which could lead to unhandled promise rejections. Additionally, if the component unmounts before the fetch completes, the setState call may trigger warnings.
Apply this diff to add error handling and cleanup:
+ private abortController: AbortController | null = null; + + componentWillUnmount() { + if (this.abortController) { + this.abortController.abort(); + } + } + async componentDidMount() { + this.abortController = new AbortController(); + + try { const endpoint = this.context.config.endpoints.completion; - const response = await fetch(endpoint); + const response = await fetch(endpoint, { signal: this.abortController.signal }); + + if (!response.ok) { + console.error('Failed to fetch webhook options:', response.statusText); + return; + } + const data = await response.json(); const webhookOptions = (data.webhook || []).map((webhook: any) => ({ name: webhook.name, value: webhook.name, id: webhook.name, label: webhook.name, body: webhook.body })); if (this.state.method.value.value === Methods.FUNCTION && this.state.url.value) { const functionName = this.state.url.value; const selectedOption = webhookOptions.find((opt: any) => opt.name === functionName); this.setState({ webhookOptions, webhookFunction: { value: selectedOption || null } }); } else { this.setState({ webhookOptions }); } + } catch (error) { + if (error.name !== 'AbortError') { + console.error('Error loading webhook options:', error); + } + } }
🧹 Nitpick comments (2)
src/components/flow/routers/webhook/WebhookRouterForm.tsx (2)
52-53: Consider defining a type for webhook options.The
webhookOptions: any[]could benefit from a more specific type definition to improve type safety and code maintainability.Define an interface for the webhook option structure:
+export interface WebhookOption { + name: string; + value: string; + id: string; + label: string; + body: string; +} + export interface WebhookRouterFormState extends FormState { headers: HeaderEntry[]; method: MethodEntry; url: StringEntry; body: StringEntry; resultName: StringEntry; webhookFunction: FormEntry; - webhookOptions: any[]; + webhookOptions: WebhookOption[]; }
394-414: Use constant instead of string literal for method comparison.Line 394 uses the string literal
'FUNCTION'instead of theMethods.FUNCTIONconstant, which reduces maintainability if the method name changes.Apply this diff:
- {method === 'FUNCTION' ? ( + {method === Methods.FUNCTION ? ( <TembaSelectElement
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/flow/routers/webhook/WebhookRouterForm.tsx(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/flow/routers/webhook/WebhookRouterForm.tsx (4)
src/store/nodeEditor.ts (2)
FormEntry(22-25)mergeForm(63-156)src/config/ConfigProvider.tsx (1)
fakePropType(4-4)src/components/flow/routers/webhook/helpers.ts (3)
nodeToState(55-110)getDefaultBody(156-158)isValidJson(160-170)src/temba/TembaSelectElement.tsx (1)
TembaSelectElement(13-26)
🔇 Additional comments (1)
src/components/flow/routers/webhook/WebhookRouterForm.tsx (1)
155-158: LGTM! Body handling logic is correct.The changes appropriately handle the body field for FUNCTION methods:
- Default body is only set for non-FUNCTION methods (lines 155-158)
- Empty bodies in FUNCTION mode skip JSON validation while non-empty bodies are still validated (lines 198-204)
This is the expected behavior for function-based webhooks.
Also applies to: 198-204
Target issue: #3200
Summary by CodeRabbit
New Features
Bug Fixes / Improvements