-
Notifications
You must be signed in to change notification settings - Fork 6
Refactor: Webhook function #184
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughAdded an Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (2)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/components/flow/routers/webhook/__snapshots__/WebhookRouterForm.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
src/components/flow/routers/webhook/WebhookRouterForm.tsx(4 hunks)src/components/flow/routers/webhook/helpers.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/flow/routers/webhook/WebhookRouterForm.tsx (1)
src/__mocks__/axios.ts (1)
axios(3-3)
⏰ 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: Tests (16.x)
🔇 Additional comments (5)
src/components/flow/routers/webhook/helpers.ts (1)
67-68: LGTM!The
isLoading: trueinitialization is appropriate since the component always fetches webhook options on mount, regardless of whether there's an existing action.src/components/flow/routers/webhook/WebhookRouterForm.tsx (4)
55-55: LGTM!The
isLoadingboolean field is a clean addition to the form state interface.
70-71: LGTM!The initialization of
webhookOptionsandisLoadingis appropriate for the component's loading flow.
83-110: Good error handling structure.The try/catch/finally pattern correctly ensures
isLoadingis set tofalseregardless of success or failure, preventing the UI from being stuck in a loading state.
431-435: LGTM!The dynamic placeholder appropriately communicates the loading state to users, showing "Loading functions…" during the fetch operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/flow/routers/webhook/WebhookRouterForm.tsx (2)
54-55: Consider defining a specific type for webhook options.Using
any[]forwebhookOptionsreduces type safety. Consider defining an interface for the webhook option structure based on the shape used throughout the component (with properties likename,value,id,label,body).+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[]; isLoading: boolean; }
79-109: Add cleanup to prevent setState on unmounted component.The async
componentDidMountdoesn't track whether the component is still mounted before callingsetStatein thefinallyblock. If the component unmounts before the fetch completes, this will trigger a warning and could lead to memory leaks.Add a mounted flag to track component lifecycle:
export default class WebhookRouterForm extends React.Component< RouterFormProps, WebhookRouterFormState > { public static contextTypes = { config: fakePropType }; + + private _isMounted = false; constructor(props: RouterFormProps) { super(props); this.state = { ...nodeToState(this.props.nodeSettings), webhookOptions: [], isLoading: true }; bindCallbacks(this, { include: [/^handle/] }); } async componentDidMount() { + this._isMounted = true; const endpoint = this.context.config.endpoints.completion; try { const response = await axios.get(endpoint); const data = response.data; 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 } - }); + if (this._isMounted) { + this.setState({ + webhookOptions, + webhookFunction: { value: selectedOption || null } + }); + } } else { - this.setState({ webhookOptions }); + if (this._isMounted) { + this.setState({ webhookOptions }); + } } } catch (error) { console.error('Error fetching webhook options:', error); } finally { - this.setState({ isLoading: false }); + if (this._isMounted) { + this.setState({ isLoading: false }); + } } } + + componentWillUnmount() { + this._isMounted = false; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/flow/routers/webhook/WebhookRouterForm.tsx(4 hunks)
⏰ 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 (1)
src/components/flow/routers/webhook/WebhookRouterForm.tsx (1)
429-433: Well-implemented loading state feedback.The conditional placeholder provides clear user feedback during the webhook options fetch, improving the user experience by indicating that the function list is loading.
akanshaaa19
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, just small change
use this line after componentDidMount to have delay in api response to test the loading state
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.