feat: Modular node configuration system with UI improvements#27
Merged
Justin322322 merged 1 commit intomainfrom Sep 2, 2025
Merged
feat: Modular node configuration system with UI improvements#27Justin322322 merged 1 commit intomainfrom
Justin322322 merged 1 commit intomainfrom
Conversation
- Modularized node-config-panel.tsx (1,229 lines 25+ components) - Created parameter handlers for 11+ parameter types (text, select, json, etc.) - Implemented node-specific configurations (HTTP, Email, Schedule, Webhook) - Enhanced UI with better spacing, hierarchy, and visual design - Removed all emojis from codebase for professional appearance - Updated documentation to reflect current project state - Fixed React infinite loop with useCallback optimization - Added comprehensive workflow documentation - Improved TypeScript type safety throughout BREAKING CHANGES: - Refactored node-config-panel.tsx to use modular architecture - Updated component interfaces for better type safety - Changed parameter handling to use individual handler components FEATURES: - Modular parameter handling system - Enhanced visual hierarchy and spacing - Professional emoji-free codebase - Comprehensive documentation updates - Improved error handling and validation PERFORMANCE: - Reduced bundle size with modular components - Optimized React re-renders with useCallback - Better TypeScript compilation performance TESTS: All 280 tests passing BUILD: Clean TypeScript compilation LINT: Zero linting errors
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Summary by CodeRabbit
WalkthroughRefactors NodeConfigPanel to a modular node-config system with dedicated configuration components, shared parameter handlers, and hooks. Adds utilities, a barrel export, and documentation. Updates messaging in scripts and logs. Introduces safe deep-set config updates and conditional parameter rendering. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant NCP as NodeConfigPanel
participant H as useNodeConfig
participant WS as WorkflowStore
participant PR as ParameterRenderer
participant NC as Node Config Component
participant PH as Parameter Handler(s)
U->>NCP: Open selected node
NCP->>H: useNodeConfig(selectedNodeId)
H->>WS: read selected node
WS-->>H: selectedNode
H-->>NCP: { selectedNode, isMobile, handleConfigChange }
alt Parameter-driven node
NCP->>PR: Render(parameters, config, onConfigChange)
PR->>PH: Render handler by type
PH-->>PR: onChange(path, value)
PR-->>NCP: onConfigChange(path, value)
NCP->>H: handleConfigChange(path, value)
H->>WS: updateNode(config with safe deep-set)
else Dedicated node config
NCP->>NC: Render(config, onConfigChange)
NC-->>NCP: onConfigChange(path, value)
NCP->>H: handleConfigChange(path, value)
H->>WS: updateNode(config with safe deep-set)
end
note over NCP,NC: SecurityWarning rendered for Email Action nodes (conditional)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
nodes/EmailNode/email-providers.ts (2)
63-81: Potential crash when emailService.auth is undefined (SendGrid “from” email).Accessing
emailService.auth.usercan throw ifauthis missing. Guard and validate a sender.- const { emailService, to, subject, body, from } = config + const { emailService, to, subject, body, from } = config + const fromEmail = from ?? emailService?.auth?.user + if (!fromEmail) { + throw new Error('SendGrid "from" email is required (provide config.from or emailService.auth.user)') + } ... - from: { email: from || emailService.auth.user }, + from: { email: fromEmail },
69-93: Add network timeouts to SendGrid API call.External calls need bounded latency. Add AbortController-based timeout.
- try { - const response = await fetch('https://api.sendgrid.com/v3/mail/send', { + try { + const controller = new AbortController() + const timeout = setTimeout(() => controller.abort(), 10_000) + const response = await fetch('https://api.sendgrid.com/v3/mail/send', { method: 'POST', headers: { 'Authorization': `Bearer ${emailService.apiKey}`, 'Content-Type': 'application/json' }, + signal: controller.signal, body: JSON.stringify({ personalizations: [{ to: to.map(email => ({ email })) }], - from: { email: from || emailService.auth.user }, + from: { email: fromEmail }, subject, content: [{ type: 'text/plain', value: body }] }) - }) + }) + clearTimeout(timeout)
🧹 Nitpick comments (48)
nodes/EmailNode/email-providers.ts (2)
109-116: Emoji slipped through; align with “emoji-free codebase”.Remove the envelope emoji from the warning to match the PR objective.
- console.warn('📧 Email package not installed - simulating email sending') + console.warn('Email package not installed - simulating email sending')
52-55: Implement the documented “graceful fallback” for nodemailer.If nodemailer isn’t available, simulate instead of erroring to match the file’s contract.
- } catch (error) { - throw new Error(`${provider} error: ${error instanceof Error ? error.message : 'Unknown error'}`) - } + } catch (error) { + if (error instanceof Error && error.message === 'Failed to load nodemailer module') { + return simulateEmailSending(config, provider) + } + throw new Error(`${provider} error: ${error instanceof Error ? error.message : 'Unknown error'}`) + }components/workflow/node-config/README.md (1)
106-118: Tighten “Before/After” formatting for readability.Convert the hyphenated run-on lines into proper bullet lists.
-**Before**: 1 monolithic file with 1229 lines -- Hard to maintain and navigate -- Complex parameter handling logic mixed with UI -- Difficult to test individual components -- No code reusability +**Before** (1 monolithic file, 1229 lines) +- Hard to maintain and navigate +- Complex parameter handling logic mixed with UI +- Difficult to test individual components +- Little code reusability -**After**: 25+ modular files with clear separation of concerns -- Each component has a single responsibility -- Easy to maintain and extend -- Fully testable components -- High code reusability -- Clear architecture patterns +**After** (25+ modular files, clear separation of concerns) +- Each component has a single responsibility +- Easier to maintain and extend +- Fully testable components +- Higher code reusability +- Clear architecture patternscomponents/workflow/node-config/components/node-configurations/WebhookNodeConfiguration.tsx (3)
11-20: Remove misleading comment or add actual validation.The “Assuming validateWorkflowId…” comment references a non-existent import.
- // Assuming validateWorkflowId is imported from utils return encodeURIComponent(workflowId || '<workflowId>')
87-97: Clamp and parse response status safely.Ensure values stay within 100–599 and avoid NaN.
- <Input + <Input type="number" - value={config.responseCode || 200} - onChange={(e) => onConfigChange('responseCode', parseInt(e.target.value) || 200)} + value={config.responseCode ?? 200} + onChange={(e) => { + const n = parseInt(e.target.value, 10) + const safe = Number.isFinite(n) ? Math.min(599, Math.max(100, n)) : 200 + onConfigChange('responseCode', safe) + }} placeholder="200" min="100" max="599" className="bg-white text-gray-900 placeholder:text-gray-400 border-gray-300" />
100-111: Optional: validate JSON response body inline.Provide quick feedback if the body isn’t valid JSON.
- <textarea + <textarea value={config.responseBody || '{"success": true, "message": "Webhook received"}'} - onChange={(e) => onConfigChange('responseBody', e.target.value)} + onChange={(e) => { + const val = e.target.value + try { + JSON.parse(val) + onConfigChange('responseBody', val) + } catch { + // Optionally set a local error state or surface a helper text + onConfigChange('responseBody', val) // keep permissive if preferred + } + }} placeholder='{"success": true, "message": "Webhook received"}' className="w-full h-20 px-3 py-2 bg-white text-gray-900 placeholder:text-gray-400 border border-gray-300 rounded-md focus:outline-none focus:ring-2 focus:ring-blue-500 focus:border-transparent resize-y" />components/workflow/node-config/hooks/useParameterState.ts (2)
29-36: Add per-path clear to prevent stale editor state growthExpose a way to remove a path’s cached state; otherwise maps can grow indefinitely during long editing sessions.
export function useParameterState() { const [jsonTextByPath, setJsonTextByPath] = useState<Record<string, string>>({}) const [kvStateByPath, setKvStateByPath] = useState<Record<string, KvRow[]>>({}) const updateJsonText = useCallback((path: string, text: string) => { setJsonTextByPath(prev => ({ ...prev, [path]: text })) }, []) const updateKvState = useCallback((path: string, rows: KvRow[]) => { setKvStateByPath(prev => ({ ...prev, [path]: rows })) }, []) const resetStates = useCallback(() => { setJsonTextByPath({}) setKvStateByPath({}) }, []) + const clearPath = useCallback((path: string) => { + setJsonTextByPath(prev => { + const { [path]: _omit, ...rest } = prev + return rest + }) + setKvStateByPath(prev => { + const { [path]: _omit, ...rest } = prev + return rest + }) + }, []) return { jsonTextByPath, kvStateByPath, updateJsonText, updateKvState, - resetStates + resetStates, + clearPath, } }
1-1: Stabilize returned object to reduce re-rendersMemoize the returned object to avoid unnecessary downstream updates when only unrelated state changes.
-import { useState, useCallback } from 'react' +import { useState, useCallback, useMemo } from 'react' @@ - return { - jsonTextByPath, - kvStateByPath, - updateJsonText, - updateKvState, - resetStates - } + return useMemo(() => ({ + jsonTextByPath, + kvStateByPath, + updateJsonText, + updateKvState, + resetStates, + clearPath, + }), [jsonTextByPath, kvStateByPath, updateJsonText, updateKvState, resetStates, clearPath])Also applies to: 29-36
docs/workflow.md (2)
92-99: Clarify webhook placeholder and improve exampleMake it explicit that the placeholder must be replaced to avoid copy/paste errors during setup.
-```bash -# Example webhook trigger -curl -X POST http://localhost:3000/api/webhooks/[workflowId] \ +```bash +# Example webhook trigger +# Replace <workflowId> with your actual workflow ID +curl -X POST http://localhost:3000/api/webhooks/<workflowId> \ -H "Content-Type: application/json" \ -d '{"event": "user_signup", "data": {"email": "user@example.com"}}'--- `58-64`: **Disambiguate JSON vs Key-Value editor behavior** Readers may assume JSON is only freeform text. Clarify dual modes to match the new parameter handlers. ```diff -- **JSON**: Structured data with key-value editor +- **JSON**: Structured data with text editor (with optional key–value helper)docs/README.md (1)
35-37: Link out “Features” and “API docs” for quicker navigationTurn plain items into links to existing docs sections.
-2. [workflow.md](workflow.md) - Complete workflow creation and management guide -3. Project features and capabilities -4. API documentation (webhooks, workflows) +2. [workflow.md](workflow.md) - Complete workflow creation and management guide +3. [Features](../README.md) - Project features and capabilities +4. API docs: [Webhooks](workflow.md#webhook-integration) and [Workflows](workflow.md#creating-workflows)components/workflow/node-config/hooks/useNodeConfig.ts (3)
30-36: MemoizehandleConfigChangeto avoid unnecessary re-renders/loopsThis handler props-drills into many components; stabilize its identity.
-import { useEffect, useRef, useState } from 'react' +import { useEffect, useRef, useState, useCallback } from 'react' @@ - const handleConfigChange = (path: string, value: unknown) => { + const handleConfigChange = useCallback((path: string, value: unknown) => { if (!selectedNodeId || !selectedNode) return @@ - updateNode(selectedNodeId, { - config: nextConfig, - }) - } + updateNode(selectedNodeId, { config: nextConfig }) + }, [selectedNodeId, selectedNode, updateNode])Also applies to: 85-89, 1-1
33-43: Prefer plain objects over null-prototype clones to avoid ecosystem edge-casesNull-prototype objects can break code that relies on
instanceof Object,obj.constructor === Object, or library guards. Path validation already mitigates pollution risk.- // Create a safe clone using Object.create(null) for the root to avoid prototype chain - const clone = Object.create(null) as Record<string, unknown> - - // Safely copy properties from the original object - for (const [key, val] of Object.entries(obj)) { - if (Object.prototype.hasOwnProperty.call(obj, key)) { - clone[key] = val - } - } + // Shallow clone root; path validation prevents prototype pollution + const clone: Record<string, unknown> = { ...obj } @@ - if (!isValidObject(next)) { - cur[key] = Object.create(null) as Record<string, unknown> + if (!isValidObject(next)) { + cur[key] = {} } else { - // Safely clone the nested object - const nestedClone = Object.create(null) as Record<string, unknown> - for (const [nestedKey, nestedVal] of Object.entries(next)) { - if (Object.prototype.hasOwnProperty.call(next, nestedKey)) { - nestedClone[nestedKey] = nestedVal - } - } - cur[key] = nestedClone + // Shallow clone the nested object + cur[key] = { ...(next as Record<string, unknown>) } }Also applies to: 49-57, 66-75
33-43: Validate empty/invalid paths earlyAvoid creating keys like "" or segments with whitespace.
const parts = p.split('.') + if (parts.length === 0 || parts.some(seg => seg.trim() === '')) { + throw new Error('Invalid empty path or segment') + }components/workflow/node-config/components/node-configurations/HttpNodeConfiguration.tsx (2)
89-105: Make JSON editors actually editable; keep local text state and validate.With
value={JSON.stringify(...)}and parse-or-noop onChange, users can’t type intermediate invalid JSON. Keep local text state, parse on blur or when valid, and show the latest text in the textarea.+import { useEffect, useState } from 'react' import { Label } from '@/components/ui/label' import { Input } from '@/components/ui/input' import { Select, SelectContent, SelectItem, SelectTrigger, SelectValue } from '@/components/ui/select' import { HttpNodeConfig } from '@/types/workflow' @@ - <textarea + <textarea className="w-full p-2 border rounded-md text-sm font-mono bg-white text-gray-900 border-gray-300" rows={4} - value={JSON.stringify(config.headers || {}, null, 2)} - onChange={(e) => { - try { - const headers = JSON.parse(e.target.value) as Record<string, string> - onConfigChange('headers', headers) - } catch { - // no-op - } - }} + id="http-headers" + value={headersText} + onChange={(e) => setHeadersText(e.target.value)} + onBlur={() => { + try { + const headers = JSON.parse(headersText) as Record<string, string> + onConfigChange('headers', headers) + } catch { + /* keep text; consider surfacing an error state */ + } + }} placeholder='{"Content-Type": "application/json"}' />Add supporting state inside the component (place near top, after
const method):+ const [headersText, setHeadersText] = useState(JSON.stringify(config.headers || {}, null, 2)) + useEffect(() => { + setHeadersText(JSON.stringify(config.headers || {}, null, 2)) + }, [config.headers])Do the same for Body:
- <textarea + <textarea className="w-full p-2 border rounded-md text-sm font-mono bg-white text-gray-900 border-gray-300" rows={6} - value={JSON.stringify(config.body || {}, null, 2)} - onChange={(e) => { - try { - const body = JSON.parse(e.target.value) as unknown - onConfigChange('body', body) - } catch { - // no-op - } - }} + id="http-body" + value={bodyText} + onChange={(e) => setBodyText(e.target.value)} + onBlur={() => { + try { + const body = JSON.parse(bodyText) as unknown + onConfigChange('body', body) + } catch { + /* keep text; consider surfacing an error state */ + } + }} placeholder='{}' />And state:
+ const [bodyText, setBodyText] = useState(JSON.stringify(config.body || {}, null, 2)) + useEffect(() => { + setBodyText(JSON.stringify(config.body || {}, null, 2)) + }, [config.body])Also applies to: 110-124, 1-4
34-41: A11y: associate URL label with input and use URL-friendly attributes.- <Label>URL</Label> + <Label htmlFor="http-url">URL</Label> <Input + id="http-url" + type="url" + autoComplete="off" + autoCorrect="off" + autoCapitalize="none" + inputMode="url" + spellCheck={false} value={config.url || ''} onChange={(e) => onConfigChange('url', e.target.value)} placeholder="https://api.example.com/endpoint" className="bg-white text-gray-900 placeholder:text-gray-400 border-gray-300" />scripts/setup-branch-protection.sh (1)
11-11: LGTM on emoji removal; consider hardening the script.-set -e +set -Eeuo pipefail +IFS=$'\n\t' + +trap 'echo "Error on line $LINENO"; exit 1' ERR @@ -echo "Applying protection to main branch..." +echo "Applying protection to main branch..." +# Validate repo exists and token scopes before applying rules +gh repo view "$REPO_OWNER/$REPO_NAME" >/dev/null @@ -echo "Branch protection rules applied successfully!" +echo "Branch protection rules applied successfully!"Optional: add explicit headers for API versioning to future-proof:
- gh api repos/$REPO_OWNER/$REPO_NAME/branches/$branch/protection \ + gh api repos/$REPO_OWNER/$REPO_NAME/branches/$branch/protection \ + -H "Accept: application/vnd.github+json" \ + -H "X-GitHub-Api-Version: 2022-11-28" \Also applies to: 26-26, 50-52, 64-66
components/workflow/node-config/utils/parameter-utils.ts (3)
31-33: Avoid returning empty paths from getParamPath.Returning
''hides mistakes and can cause hard-to-trace bugs downstream. Throw early.-export function getParamPath(param: ExtendedParameterDefinition): string { - return param.path || param.name || '' -} +export function getParamPath(param: ExtendedParameterDefinition): string { + const path = param.path ?? param.name + if (!path) { + throw new Error('Parameter is missing "path" or "name"') + } + return path +}
53-78: Confirm intended showIf semantics (OR vs AND).
shouldShowParameteruses OR (some). If the intent was “all conditions must match,” switch toevery. Otherwise, document OR semantics to avoid confusion.
83-96: Optional: support bracket/index paths in getValueAtPath.Current dot-only paths don’t handle arrays (e.g.,
headers[0].name). Consider minimal bracket support.-export function getValueAtPath(obj: Record<string, unknown>, path: string): unknown { - const parts = path.split('.') +export function getValueAtPath(obj: Record<string, unknown>, path: string): unknown { + // supports foo.bar[0].baz + const parts = path + .replace(/\[(\d+)\]/g, '.$1') + .split('.') let current: unknown = objcomponents/workflow/node-config/components/parameter-handlers/TextareaParameterHandler.tsx (1)
1-4: Add placeholder support and proper label association; drop unnecessary key.-import { FieldLabel } from '../shared/FieldLabel' -import { ExtendedParameterDefinition, getParamPath } from '../../utils/parameter-utils' -import { getParamValue, getParameterDescription } from '../../utils/config-utils' +import { FieldLabel } from '../shared/FieldLabel' +import { ExtendedParameterDefinition, getParamPath } from '../../utils/parameter-utils' +import { getParamValue, getParameterDescription, getParameterPlaceholder } from '../../utils/config-utils' @@ const value = getParamValue(config, paramPath, 'string', param.default) const description = getParameterDescription(param.description) + const placeholder = getParameterPlaceholder(param.placeholder) ?? description @@ - return ( - <div key={paramPath} className="space-y-2"> + return ( + <div className="space-y-2"> <FieldLabel text={param.label} description={description} htmlFor={paramPath} /> <textarea + id={paramPath} + name={paramPath} className="w-full p-3 border rounded-md bg-white text-gray-900 border-gray-300 hover:border-gray-400 focus:border-blue-500 focus:ring-1 focus:ring-blue-500 resize-y" rows={4} value={String(value)} onChange={handleChange} - placeholder={description} + placeholder={placeholder} /> </div>Also applies to: 11-15, 20-30
components/workflow/node-config/components/parameter-handlers/UrlParameterHandler.tsx (1)
22-31: A11y + UX: connect label, add URL-friendly attributes.return ( <div key={paramPath} className="space-y-2"> <FieldLabel text={param.label} description={description} htmlFor={paramPath} /> <Input type="url" + id={paramPath} + name={paramPath} + autoComplete="off" + autoCorrect="off" + autoCapitalize="none" + inputMode="url" + spellCheck={false} value={String(value)} onChange={handleChange} placeholder={placeholder || 'Enter URL'} className="bg-white text-gray-900 placeholder:text-gray-400 border-gray-300 hover:border-gray-400 focus:border-blue-500 focus:ring-1 focus:ring-blue-500" /> </div>components/workflow/node-config/components/shared/SecurityWarning.tsx (3)
11-14: Avoid unsafe cast; use a type guard for actionType.Prevents accessing a possibly-missing property and improves type safety.
- if (node.data.nodeType !== NodeType.ACTION || - (node.data as { actionType: ActionType }).actionType !== ActionType.EMAIL) { - return null - } + if (node.data?.nodeType !== NodeType.ACTION) return null + if (!('actionType' in node.data) || (node.data as any).actionType !== ActionType.EMAIL) { + return null + }
19-19: Mark decorative icon as hidden from assistive tech.Improves a11y.
- <ShieldCheck className="w-4 h-4 text-gray-600 mt-0.5 flex-shrink-0" /> + <ShieldCheck aria-hidden="true" className="w-4 h-4 text-gray-600 mt-0.5 flex-shrink-0" />
22-27: Use semantic list styling instead of literal bullets.Removes redundant "•" characters and lets CSS handle bullets.
- <ul className="space-y-1 text-xs"> - <li>• Your credentials are encrypted and stored locally on your device only</li> - <li>• Use app-specific passwords instead of your main email password</li> - <li>• Data is automatically cleared when you close the browser</li> - <li>• Only use on trusted devices for maximum security</li> - </ul> + <ul className="list-disc pl-4 space-y-1 text-xs"> + <li>Your credentials are encrypted and stored locally on your device only</li> + <li>Use app-specific passwords instead of your main email password</li> + <li>Data is automatically cleared when you close the browser</li> + <li>Only use on trusted devices for maximum security</li> + </ul>components/workflow/node-config/components/shared/FieldLabel.tsx (1)
15-24: Don’t use a focus-suppressed button for a passive tooltip icon.Use a non-interactive element and hide the icon from screen readers.
- <button - type="button" - className="inline-flex items-center text-gray-400 hover:text-gray-600 focus:outline-none" - title={description} - aria-label="Info" - tabIndex={-1} - > - <Info className="w-4 h-4" /> - </button> + <span + className="inline-flex items-center text-gray-400 hover:text-gray-600 cursor-help" + title={description} + aria-hidden="true" + > + <Info className="w-4 h-4" /> + </span>components/workflow/node-config/components/parameter-handlers/CredentialParameterHandler.tsx (2)
24-26: Remove unnecessary key on a non-list element.No list mapping here.
- <div key={paramPath} className="space-y-1.5 sm:space-y-2"> + <div className="space-y-1.5 sm:space-y-2">
27-34: Label isn’t programmatically associated with the control.CredentialSelector doesn’t expose id/aria props; expose them and forward to the trigger for a11y.
Proposed minimal API in components/ui/credential-selector.tsx:
- export function CredentialSelector({ - value, - onChange, - credentialType = 'generic', - placeholder = "Select a credential", - className = "" - }: CredentialSelectorProps) { + export function CredentialSelector({ + value, + onChange, + credentialType = 'generic', + placeholder = "Select a credential", + className = "", + id, + ariaLabelledby, + }: CredentialSelectorProps & { id?: string; ariaLabelledby?: string }) { ... - <Select value={value} onValueChange={onChange}> - <SelectTrigger className="flex-1 h-9"> + <Select value={value} onValueChange={onChange}> + <SelectTrigger id={id} aria-labelledby={ariaLabelledby} className="flex-1 h-9">Then in this handler:
const labelId = `label-${paramPath.replace(/[^a-zA-Z0-9_-]/g, '_')}` <FieldLabel text={param.label} description={description} htmlFor={labelId} /> <CredentialSelector id={labelId} ariaLabelledby={labelId} ... />Want me to open a follow-up PR for this?
components/workflow/node-config/components/parameter-handlers/EmailParameterHandler.tsx (1)
23-23: Drop the key on a non-list wrapper.- <div key={paramPath} className="space-y-2"> + <div className="space-y-2">components/workflow/node-config/components/parameter-handlers/NumberParameterHandler.tsx (2)
4-4: Use placeholder helper for consistency.Aligns with other handlers.
-import { getParamValue, getParameterDescription } from '../../utils/config-utils' +import { getParamValue, getParameterDescription, getParameterPlaceholder } from '../../utils/config-utils'
23-23: Drop the key on a non-list wrapper.- <div key={paramPath} className="space-y-2"> + <div className="space-y-2">components/workflow/node-config/components/parameter-handlers/StringListParameterHandler.tsx (2)
17-21: Avoid parse-on-every-keystroke; preserve user typing and commit on blur/Enter.Current round-trip via
arrayValue.joincan collapse spaces and trailing commas, causing cursor jumps.Apply:
+import { useState } from 'react' @@ export function StringListParameterHandler({ param, config, onConfigChange }: StringListParameterHandlerProps) { const paramPath = getParamPath(param) const arrayValue = getArrayValue<string>(config, paramPath, []) const description = getParameterDescription(param.description) - const handleChange = (e: React.ChangeEvent<HTMLInputElement>) => { - const value = e.target.value - const items = value.split(',').map(s => s.trim()).filter(Boolean) - onConfigChange(paramPath, items) - } + const [inputValue, setInputValue] = useState<string>(arrayValue.map(String).join(', ')) + const handleChange = (e: React.ChangeEvent<HTMLInputElement>) => setInputValue(e.target.value) + const commit = () => { + const items = inputValue.split(',').map(s => s.trim()).filter(Boolean) + onConfigChange(paramPath, items) + } @@ - <Input - value={arrayValue.map(String).join(', ')} - onChange={handleChange} + <Input + value={inputValue} + onChange={handleChange} + onBlur={commit} + onKeyDown={(e) => (e.key === 'Enter' ? (e.preventDefault(), commit()) : undefined)}Also applies to: 27-31
24-24: Remove unnecessary key on a non-list container.
key={paramPath}here has no effect and can be dropped.Apply:
- <div key={paramPath} className="space-y-2"> + <div className="space-y-2">components/workflow/node-config/components/node-configurations/EmailActionNodeConfiguration.tsx (1)
16-20: Unify string-list parsing with the shared handler.The “To” parsing duplicates the new
StringListParameterHandler. Prefer reusing it (orParameterRendererwithtype: 'stringList') to avoid drift.components/workflow/node-config/components/parameter-handlers/SelectParameterHandler.tsx (2)
26-33: Label association and placeholder for empty selection.Bind the label and provide a sensible placeholder.
Apply:
- <FieldLabel text={param.label} description={description} /> + <FieldLabel text={param.label} description={description} htmlFor={paramPath || undefined} /> <Select - value={value} + value={value} onValueChange={handleValueChange} > - <SelectTrigger className="bg-white text-gray-900 border-gray-300 hover:border-gray-400 focus:border-blue-500 focus:ring-1 focus:ring-blue-500"> - <SelectValue /> + <SelectTrigger id={paramPath || undefined} className="bg-white text-gray-900 border-gray-300 hover:border-gray-400 focus:border-blue-500 focus:ring-1 focus:ring-blue-500"> + <SelectValue placeholder={typeof param.placeholder === 'string' ? (param.placeholder as string) : 'Select...'} /> </SelectTrigger>
35-37: Avoid recomputing dynamic options on every render.Memoize when
param.optionsis a function if it’s non-trivial.Example:
const options = useMemo( () => (typeof param.options === 'function' ? param.options() : param.options || []), [param.options] )components/workflow/node-config/components/node-configurations/ScheduleNodeConfiguration.tsx (1)
15-20: Optional: validate cron/timezone to prevent invalid schedules.Add lightweight client-side checks (e.g., cron format guard; IANA TZ list select) or defer to backend with inline error display.
Also applies to: 27-33
components/workflow/node-config/components/shared/ParameterRenderer.tsx (3)
38-42: Use a single stable key for all handler cases.Avoid duplicating key logic and reduce risk of duplicate keys by precomputing one stable key per param.
Apply:
- {validParameters.map((param) => { + {validParameters.map((param) => { + const stableKey = `${param.type}:${param.path || param.name}` if (!shouldShowParameter(param, config)) return null switch (param.type) { case 'select': return ( <SelectParameterHandler - key={param.path || param.name} + key={stableKey} param={param} config={config} onConfigChange={onConfigChange} /> ) case 'string': case 'text': return ( <TextParameterHandler - key={param.path || param.name} + key={stableKey} param={param} config={config} onConfigChange={onConfigChange} /> ) case 'textarea': return ( <TextareaParameterHandler - key={param.path || param.name} + key={stableKey} param={param} config={config} onConfigChange={onConfigChange} /> ) case 'boolean': return ( <BooleanParameterHandler - key={param.path || param.name} + key={stableKey} param={param} config={config} onConfigChange={onConfigChange} /> ) case 'json': return ( <JsonParameterHandler - key={param.path || param.name} + key={stableKey} param={param} config={config} onConfigChange={onConfigChange} jsonTextByPath={jsonTextByPath} kvStateByPath={kvStateByPath} onJsonTextChange={onJsonTextChange} onKvStateChange={onKvStateChange} /> ) case 'number': return ( <NumberParameterHandler - key={param.path || param.name} + key={stableKey} param={param} config={config} onConfigChange={onConfigChange} /> ) case 'email': return ( <EmailParameterHandler - key={param.path || param.name} + key={stableKey} param={param} config={config} onConfigChange={onConfigChange} /> ) case 'password': return ( <PasswordParameterHandler - key={param.path || param.name} + key={stableKey} param={param} config={config} onConfigChange={onConfigChange} /> ) case 'url': return ( <UrlParameterHandler - key={param.path || param.name} + key={stableKey} param={param} config={config} onConfigChange={onConfigChange} /> ) case 'credential': return ( <CredentialParameterHandler - key={param.path || param.name} + key={stableKey} param={param} config={config} onConfigChange={onConfigChange} /> ) case 'stringList': return ( <StringListParameterHandler - key={param.path || param.name} + key={stableKey} param={param} config={config} onConfigChange={onConfigChange} /> )Also applies to: 45-46, 55-56, 64-65, 73-74, 82-83, 95-96, 104-105, 113-114, 121-122, 131-132, 140-141
29-33: Avoid new object defaults each render.Defaulting to {} creates new references and can cause unnecessary child re-renders. Prefer undefined defaults and handle inside JsonParameterHandler.
15-23: Consider tightening prop types.If all callers pass validated params, switch
parameters: unknown[]toExtendedParameterDefinition[]and drop the runtime filter, or memoize the filter:+import { useMemo } from 'react' ... - const validParameters = parameters.filter(isValidParameter) as ExtendedParameterDefinition[] + const validParameters = useMemo( + () => parameters.filter(isValidParameter) as ExtendedParameterDefinition[], + [parameters] + )components/workflow/node-config/components/parameter-handlers/PasswordParameterHandler.tsx (1)
12-20: Don’t echo existing secrets back into the DOM if avoidable.Controlled password inputs place the secret in the DOM value. Consider masking stored values (e.g., show placeholder bullets and only overwrite on change).
Would you like a small wrapper that renders '••••••••' when a secret exists and only writes on change?
components/workflow/node-config/components/parameter-handlers/JsonParameterHandler.tsx (2)
1-7: Import React types explicitly.Avoid relying on the global React namespace for types.
+import type React from 'react' import { Copy } from 'lucide-react'
169-179: Add id to textarea for accessibility.Provide an id and wire htmlFor in FieldLabel when possible.
- <FieldLabel text={param.label} description={description} /> + <FieldLabel text={param.label} description={description} htmlFor={paramPath} /> <textarea + id={paramPath} className="w-full p-2 border rounded-md text-sm font-mono bg-white text-gray-900 border-gray-300" rows={6} value={displayText} onChange={handleJsonChange} placeholder={description || '{}'} />components/workflow/node-config-panel.tsx (2)
296-309: Ensure unique checkbox ids.Static id "continueOnFail" can collide; key it by nodeId.
- <input - id="continueOnFail" + <input + id={`continueOnFail-${nodeId}`} type="checkbox" ... - <Label htmlFor="continueOnFail" className="text-sm text-gray-700 cursor-pointer"> + <Label htmlFor={`continueOnFail-${nodeId}`} className="text-sm text-gray-700 cursor-pointer">
112-114: Simplify nodeId inference.Given the early return when pendingDeleteNodeId is set, the non-null assertion on it is unnecessary.
-const nodeId = selectedNodeId ?? pendingDeleteNodeId! +const nodeId = selectedNodeId!components/workflow/node-config/utils/config-utils.ts (2)
46-48: Fix placeholder return type: current impl never returns undefinedThe function advertises
string | undefinedbut always returns a string viagetSafePlaceholder. Returnundefinedfor non-strings to match the declared type.-export function getParameterPlaceholder(placeholder: unknown): string | undefined { - return getSafePlaceholder(placeholder) -} +export function getParameterPlaceholder(placeholder: unknown): string | undefined { + return typeof placeholder === 'string' ? placeholder : undefined +}Also remove the now-unused import:
-import { getTypedParameterValue, getSafeDescription, getSafePlaceholder, getSafeDefaultValue } from '@/lib/type-safe-utils' +import { getTypedParameterValue, getSafeDescription, getSafeDefaultValue } from '@/lib/type-safe-utils'
7-34: Simplify getParamValue; try/catch is unnecessary
getTypedParameterValueis already safe; the try/catch and branching can be collapsed.export function getParamValue( config: Record<string, unknown>, path: string, paramType: 'string' | 'number' | 'boolean', defaultVal?: unknown ): string | number | boolean { - try { - if (paramType === 'string') { - return getTypedParameterValue(config, path, defaultVal, 'string') - } else if (paramType === 'number') { - return getTypedParameterValue(config, path, defaultVal, 'number') - } else { - return getTypedParameterValue(config, path, defaultVal, 'boolean') - } - } catch { - // Fallback for type safety - switch (paramType) { - case 'string': - return '' - case 'number': - return 0 - case 'boolean': - return false - default: - return '' - } - } + return getTypedParameterValue(config, path, defaultVal, paramType) }components/workflow/node-config/index.ts (1)
1-4: Consider limiting wildcard exports to protect public API surfaceIf API stability is a priority, explicitly re-export only intended public utilities/types from
parameter-utilsto prevent accidental export growth.
components/workflow/node-config/components/node-configurations/EmailActionNodeConfiguration.tsx
Show resolved
Hide resolved
components/workflow/node-config/components/node-configurations/HttpNodeConfiguration.tsx
Show resolved
Hide resolved
components/workflow/node-config/components/node-configurations/HttpNodeConfiguration.tsx
Show resolved
Hide resolved
components/workflow/node-config/components/node-configurations/ScheduleNodeConfiguration.tsx
Show resolved
Hide resolved
components/workflow/node-config/components/parameter-handlers/SelectParameterHandler.tsx
Show resolved
Hide resolved
components/workflow/node-config/components/parameter-handlers/StringListParameterHandler.tsx
Show resolved
Hide resolved
components/workflow/node-config/components/parameter-handlers/TextParameterHandler.tsx
Show resolved
Hide resolved
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
BREAKING CHANGES:
FEATURES:
PERFORMANCE:
TESTS: All 280 tests passing
BUILD: Clean TypeScript compilation
LINT: Zero linting errors
Summary
Describe the changes and the problem they solve.
Screenshots / Videos (if UI changes)
Checklist
npm run typechecknpm run lintnpm testnpm run buildREADME.md/docs/) as neededRelated Issues
Closes #