feat: enhance email node with security improvements and type safety#23
feat: enhance email node with security improvements and type safety#23Justin322322 wants to merge 7 commits intomainfrom
Conversation
- Add comprehensive email provider types and configurations - Implement security status component for workflow validation - Enhance EmailNode with better type definitions and schema validation - Add security utilities for workflow safety checks - Update workflow components with improved error handling - Add email provider configurations with proper TypeScript types - Improve node configuration panel and execution logging - Update package dependencies for enhanced functionality
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded@Justin322322 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 2 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 (3)
WalkthroughImplements client-side credential encryption and status UI; migrates workflow drafts to sessionStorage; adds collapsible Execution Log panel; introduces mobile swipe-to-close sheets; overhauls Email node (schema, service, providers, types) with multi-provider sending; updates node-definition types; adjusts editor loading logic; and updates build scripts/dependencies. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as EditorInner
participant Store as useWorkflowStore
participant Sess as sessionStorage
participant Local as localStorage
UI->>Sess: read workflowDraft, lastOpenedWorkflowId
alt session draft applicable
UI->>Store: setWorkflow(parse(draft)) (decrypt in store)
else workflowId present
UI->>Local: read workflows[]
UI->>Store: setWorkflow(found by id) (decrypt in store)
else
UI->>Store: createNewWorkflow()
end
Store-->>Sess: schedule draft save (encrypt) on changes
sequenceDiagram
participant Node as EmailNode
participant Svc as EmailService
participant Prov as email-providers
participant SendGrid as SendGrid API
participant SMTP as Nodemailer/SMTP
Node->>Svc: sendEmail(config)
alt type == sendgrid
Svc->>Prov: sendWithSendGrid(config)
Prov->>SendGrid: POST /mail/send
SendGrid-->>Prov: response
else type in (gmail|outlook|smtp)
Svc->>Prov: sendWithNodemailer(config, provider)
Prov->>SMTP: transporter.sendMail()
SMTP-->>Prov: result
end
Prov-->>Svc: EmailExecutionResult
Svc-->>Node: EmailExecutionResult
sequenceDiagram
participant User as User (mobile)
participant Sheet as MobileSheet/DialogContent
User->>Sheet: touchstart
User->>Sheet: touchmove (drag Y)
Sheet->>Sheet: update swipeDistance
User->>Sheet: touchend
alt distance > threshold
Sheet->>Sheet: onOpenChange(false)
else
Sheet->>Sheet: reset transform
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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 (
|
- Update test expectations to reflect EmailNode now being properly implemented - Add required emailService configuration to valid test case - Update test description to reflect new modular node definition system - All tests now pass (157/157)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (3)
hooks/use-workflow-store.ts (3)
158-179: Draft persistence uses localStorage here; elsewhere you’ve migrated to sessionStorage.This inconsistency breaks the new session-draft loading path in EditorInner. Persist new workflow drafts and lastOpenedWorkflowId to sessionStorage instead.
try { - localStorage.setItem('lastOpenedWorkflowId', newWorkflow.id) - localStorage.setItem('workflowDraft', JSON.stringify(newWorkflow)) + sessionStorage.setItem('lastOpenedWorkflowId', newWorkflow.id) + sessionStorage.setItem('workflowDraft', JSON.stringify(newWorkflow)) } catch (err) { console.debug('create draft failed', err) }
112-156: Normalize createdAt/updatedAt to Date inside setWorkflow.Workflows loaded from storage contain ISO strings. Centralize date normalization here to avoid scattered callers converting.
setWorkflow: (workflow) => { - // Decrypt credentials when loading workflow + // Normalize date fields + const normalizedWorkflow: Workflow = { + ...workflow, + createdAt: new Date((workflow as any).createdAt ?? Date.now()), + updatedAt: new Date((workflow as any).updatedAt ?? Date.now()), + } + + // Decrypt credentials when loading workflow - const decryptedNodes = workflow.nodes.map(node => { + const decryptedNodes = normalizedWorkflow.nodes.map(node => { if (node.data.config && typeof node.data.config === 'object') { return { ...node, data: { ...node.data, config: decryptEmailConfig(node.data.config as Record<string, unknown>) } } } return node }) set({ - workflow, + workflow: normalizedWorkflow, nodes: decryptedNodes, - edges: workflow.edges, + edges: normalizedWorkflow.edges, }) try { - sessionStorage.setItem('lastOpenedWorkflowId', workflow.id) + sessionStorage.setItem('lastOpenedWorkflowId', normalizedWorkflow.id) // initialize draft on load to ensure refresh survival until first change const { nodes, edges } = get() // Encrypt before storing const encryptedNodes = nodes.map(node => { if (node.data.config && typeof node.data.config === 'object') { return { ...node, data: { ...node.data, config: encryptEmailConfig(node.data.config as Record<string, unknown>) } } } return node }) - const draft = { ...workflow, nodes: encryptedNodes, edges, updatedAt: new Date() } + const draft = { ...normalizedWorkflow, nodes: encryptedNodes, edges, updatedAt: new Date() } sessionStorage.setItem('workflowDraft', JSON.stringify(draft))
253-263: Autosave doesn’t trigger on new connections.onConnect updates edges but doesn’t schedule a draft save, so edge connections aren’t persisted in the session draft until another change happens.
onConnect: (connection) => { // Prevent self-connections (nodes connecting to themselves) if (connection.source === connection.target) { console.warn('Cannot connect a node to itself') return } set({ edges: addEdge(connection, get().edges), }) + ;(get() as unknown as { _scheduleDraftSave: () => void })._scheduleDraftSave() },
🧹 Nitpick comments (31)
nodes/EmailNode/EmailNode.types.ts (2)
11-23: ModelemailServiceas a discriminated union for provider-specific requirementsRight now
auth.passis required for all providers andapiKeyis optional, which doesn’t align with SendGrid (API key only) or potential OAuth flows. A discriminated union enforces the right fields per provider and improves type safety across the app.Apply this change within the selected range:
- emailService: { - type: 'smtp' | 'gmail' | 'outlook' | 'sendgrid' - // SMTP Configuration - host?: string - port?: number - secure?: boolean // true for 465, false for other ports - auth: { - user: string - pass: string // App password or API key - } - // SendGrid specific - apiKey?: string - } + emailService: EmailServiceConfigAdd these supporting types (place near the top of this file, before EmailNodeConfig):
type SmtpConfig = { type: 'smtp' host: string port: number secure?: boolean auth: { user: string; pass: string } // app password } type GmailConfig = { type: 'gmail' auth: { user: string; pass: string } // app password (or consider OAuth in future) from?: string } type OutlookConfig = { type: 'outlook' auth: { user: string; pass: string } // app password (or consider OAuth) from?: string } type SendgridConfig = { type: 'sendgrid' apiKey: string from?: string } export type EmailServiceConfig = SmtpConfig | GmailConfig | OutlookConfig | SendgridConfig
31-39: MakemessageIdoptional for failure cases; consider serializable timestampsExecution can fail (
sent: false,errorset). In those casesmessageIdmay not exist. Also, if this result is serialized across the wire, a string timestamp is safer thanDate.export interface EmailExecutionResult { sent: boolean to: string[] subject: string - messageId: string - timestamp: Date + messageId?: string + timestamp: string | Date provider?: string error?: string }nodes/EmailNode/nodemailer-types.ts (3)
6-17: Broadentotype to match nodemailer expectationsNodemailer accepts
string | string[](and richer variants). Restricting tostringmakes adapters do extra coercion.export interface MailOptions { from?: string - to: string + to: string | string[] subject: string text?: string html?: string attachments?: Array<{ filename?: string content?: Buffer | string path?: string }> }
48-51: Align API surface with nodemailer: usecreateTransportUsing
createTransporterdiverges from the real module and invites drift. PrefercreateTransportconsistently.export interface NodemailerModule { - createTransporter: (options: TransportOptions) => Transporter - createTransport?: (options: TransportOptions) => Transporter + createTransport: (options: TransportOptions) => Transporter }If you rely on an internal wrapper that exposes
createTransporter, rename it tocreateTransportto reduce cognitive load and improve compatibility.
57-63: Avoid augmentingNodeJS.Global; prefer globalThis for ambientrequireAugmenting
NodeJS.Globalis brittle in modern TS/Node setups and can bleed into client builds. If you only need types for an ambientrequire, use globalThis instead.-declare global { - namespace NodeJS { - interface Global { - require?: NodeRequire - } - } -} +declare global { + // Optional ambient `require` (e.g., in CommonJS or when polyfilled) + // This avoids depending on NodeJS.Global types and works in modern TS targets. + // Do not rely on this on the client. + // eslint-disable-next-line no-var + var require: NodeRequire | undefined +}Also consider avoiding any global augmentation and instead pass a loader function explicitly where needed.
lib/security.ts (1)
64-68: UseisEncryptedhelper instead of base64 heuristicsThe current check can misclassify. Centralize with
isEncryptedto reduce false negatives.- // Check if value looks encrypted (base64 format from CryptoJS) - if (!encryptedValue.includes('/') && !encryptedValue.includes('+') && !encryptedValue.includes('=')) { - // Likely plaintext, return as-is for backward compatibility - return encryptedValue - } + if (!isEncrypted(encryptedValue)) { + // Likely plaintext, return as-is for backward compatibility + return encryptedValue + }package.json (2)
33-38: Move@types/crypto-jsto devDependenciesType packages are only needed at build/typecheck time. Keeping them under dependencies bloats the production bundle/install surface.
"@reactflow/minimap": "^11.7.14", - "@types/crypto-js": "^4.2.2", "@types/three": "^0.179.0", "class-variance-authority": "^0.7.0", "clsx": "^2.1.1", "crypto-js": "^4.2.0",Apply the addition under devDependencies (see next comment).
52-68: Add@types/crypto-jsunder devDependencies"devDependencies": { "@types/node": "^22.7.4", "@types/react": "^19.0.2", "@types/react-dom": "^19.0.2", "@types/uuid": "^9.0.8", + "@types/crypto-js": "^4.2.2", "@typescript-eslint/eslint-plugin": "^8.39.1", "@typescript-eslint/parser": "^8.39.1", "autoprefixer": "^10.4.20",components/workflow/node-config-panel.tsx (3)
14-18: Remove unused imports to satisfylint:strict
EMAIL_NODE_DEFINITIONandgetSecurityStatusare not used in this file. With--max-warnings 0, these will fail CI.-import { EMAIL_NODE_DEFINITION, EmailNodeConfig } from '@/nodes/EmailNode' +import { EmailNodeConfig } from '@/nodes/EmailNode' @@ -import { SECURITY_WARNINGS, getSecurityStatus } from '@/lib/security' +import { SECURITY_WARNINGS } from '@/lib/security'
171-188: Use centralized security messages to avoid duplication/driftYou already import
SECURITY_WARNINGS. Render those constants instead of hardcoding text.{selectedNode.data.nodeType === NodeType.ACTION && (selectedNode.data as { actionType: ActionType }).actionType === ActionType.EMAIL && ( <div className="mb-4 p-3 border border-gray-300 rounded-md"> <div className="flex items-start gap-2"> <ShieldCheck className="w-4 h-4 text-gray-600 mt-0.5 flex-shrink-0" /> <div className="text-sm text-gray-700"> <p className="font-medium mb-1">Security Notice</p> - <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="space-y-1 text-xs"> + <li>• {SECURITY_WARNINGS.CREDENTIAL_STORAGE}</li> + <li>• {SECURITY_WARNINGS.APP_PASSWORD}</li> + <li>• {SECURITY_WARNINGS.DATA_PROTECTION}</li> + <li>• {SECURITY_WARNINGS.TRUSTED_DEVICE}</li> + </ul> </div> </div> </div> )}
578-626: Remove legacy email config UI to avoid divergenceWith schema-driven parameters in place, this manual Email configuration path is redundant and risks drifting out of sync. Let the
def.parameterspath drive EmailNode entirely.- // Email configuration - using new modular EmailNode - if (data.nodeType === NodeType.ACTION && data.actionType === ActionType.EMAIL) { - const config = data.config as unknown as EmailNodeConfig - - return ( - <> - <div className="space-y-2"> - <Label>To (comma separated)</Label> - <Input - value={config.to?.join(', ') || ''} - onChange={(e) => handleConfigChange('to', e.target.value.split(',').map(s => s.trim()).filter(s => s.length > 0))} - placeholder="user@example.com, another@example.com" - className="bg-white text-gray-900 placeholder:text-gray-400 border-gray-300" - /> - </div> - - <div className="space-y-2"> - <Label>Subject</Label> - <Input - value={config.subject || ''} - onChange={(e) => handleConfigChange('subject', e.target.value)} - placeholder="Email subject" - className="bg-white text-gray-900 placeholder:text-gray-400 border-gray-300" - /> - </div> - - <div className="space-y-2"> - <Label>Body</Label> - <textarea - className="w-full p-2 border rounded-md bg-white text-gray-900 border-gray-300" - rows={6} - value={config.body || ''} - onChange={(e) => handleConfigChange('body', e.target.value)} - placeholder="Email body content..." - /> - </div> - - <div className="space-y-2"> - <Label>From (optional)</Label> - <Input - value={config.from || ''} - onChange={(e) => handleConfigChange('from', e.target.value)} - placeholder="sender@example.com" - className="bg-white text-gray-900 placeholder:text-gray-400 border-gray-300" - /> - </div> - </> - ) - } + // (Removed) Legacy Email configuration UI now superseded by schema-driven parameterscomponents/workflow/workflow-toolbar.tsx (1)
224-227: Good placement of SecurityStatus; consider mobile visibility.Placing SecurityStatus next to the Run-from-node button is sensible. It’s hidden on mobile—if security state is critical, consider showing a compact badge in the MobileActionSheet too to keep parity across breakpoints.
lib/node-definitions.ts (3)
20-47: Duplicate parameter schema types across modules; centralize to avoid drift.This file defines ParameterType/ParameterDefinition while nodes/index.ts defines its own ParameterDefinition with a slightly different shape (e.g., includes
name, lackspath), which can lead to inconsistencies. Consider exporting a single shared type (e.g., from nodes/index.ts) and importing it here, or vice versa.
67-92: Dead helpers (setValueAtPath/buildDefaultsFromParameters) can be removed or reused.These utilities aren’t used in this module anymore. Either remove them to reduce maintenance, or export and reuse where defaults are built.
1-8: Minor: Unused importScheduleNodeConfig.If not used elsewhere, remove to keep the surface tidy.
lib/node-definitions.test.ts (1)
129-149: Great: asserts modular resolution for Email nodes; add a non-Email resolution test once registry fallback is used.After refactoring findNodeDefinition to delegate to the central registry, add a test ensuring HTTP nodes also resolve via the registry.
Example test to add:
it('should find HTTP node definition via registry', () => { const httpNode: WorkflowNode = { id: 'test-http-node', type: 'action', position: { x: 0, y: 0 }, data: { label: 'HTTP Request', nodeType: NodeType.ACTION, actionType: ActionType.HTTP, config: {} } } const definition = findNodeDefinition(httpNode) expect(definition).toBeDefined() expect(definition?.nodeType).toBe(NodeType.ACTION) })nodes/EmailNode/EmailNode.test.ts (1)
120-122: Execution tests cover happy path, validation failures, abort, and multiple recipients.Good breadth. If executeEmailNode ever serializes dates (e.g., across workers), consider relaxing
timestampassertion to accept ISO strings by parsing withnew Date(...).Also applies to: 138-140, 149-151, 159-161, 172-175, 185-189
nodes/index.ts (1)
67-70: Broaden getNodeDefinition to accept numeric subTypes (enum values).Your NodeDefinition.subType allows string | number; adjust the getter accordingly to avoid casting at call sites.
-export function getNodeDefinition(nodeType: NodeType, subType: string): NodeDefinition | undefined { +export function getNodeDefinition(nodeType: NodeType, subType: string | number): NodeDefinition | undefined { const key = `${nodeType}-${subType}` return NODE_REGISTRY.get(key) }app/editor/page.tsx (1)
31-39: Harden localStorage parsing to avoid runtime errors on malformed data.A corrupted or manually-edited 'workflows' entry will throw during JSON.parse. Add a try/catch and fallback to an empty array.
if (workflowId) { // Load from persistent storage (localStorage) - these are encrypted - const workflows = JSON.parse(localStorage.getItem('workflows') || '[]') as Workflow[] + let workflows: Workflow[] = [] + try { + workflows = JSON.parse(localStorage.getItem('workflows') || '[]') as Workflow[] + } catch { + workflows = [] + } const workflow = workflows.find((w: Workflow) => w.id === workflowId) if (workflow) { setWorkflow(workflow) // setWorkflow will handle decryption } else { createNewWorkflow() } } else {components/workflow/execution-log.tsx (3)
98-113: Add a11y attributes to the collapse/expand control.Expose the expanded state and the controlled panel region for assistive tech.
<Button variant="ghost" size="icon" onClick={() => setLogsPanelCollapsed(!isLogsPanelCollapsed)} className="w-8 h-8 rounded-full bg-white/10 hover:bg-white/20 transition-all duration-200 text-white border-0 flex-shrink-0 group" title={isLogsPanelCollapsed ? "Expand execution logs" : "Collapse execution logs"} + aria-expanded={!isLogsPanelCollapsed} + aria-controls="desktop-execution-log-panel" >
116-130: Expose a stable region id for aria-controls.Tie the toggle button to the panel content for better accessibility.
- {!isLogsPanelCollapsed && ( - <> + {!isLogsPanelCollapsed && ( + <> {hasAny ? ( - <div className="flex-1 overflow-y-auto p-4">{renderLogsList()}</div> + <div id="desktop-execution-log-panel" className="flex-1 overflow-y-auto p-4">{renderLogsList()}</div> ) : ( - <div className="h-full flex items-center justify-center text-white/50"> + <div id="desktop-execution-log-panel" className="h-full flex items-center justify-center text-white/50"> <div className="text-center"> <Info className="w-12 h-12 mx-auto mb-2 text-white/30" /> <p>No execution logs yet</p> <p className="text-sm text-white/40">Run a workflow to see logs here</p> </div> </div> )} </> )}
20-28: Consider using an Info icon for info logs.Visual consistency: info-level logs could use Info instead of CheckCircle.
const getLogIcon = (level: string) => { switch (level) { case 'error': return <XCircle className="w-4 h-4 text-red-500" /> case 'warning': return <AlertCircle className="w-4 h-4 text-yellow-500" /> default: - return <CheckCircle className="w-4 h-4 text-green-500" /> + return <Info className="w-4 h-4 text-blue-500" /> } }nodes/EmailNode/email-providers.ts (2)
41-47: Support attachments when sending via Nodemailer.If config.attachments contains file paths, pass them through to transporter.
const mailOptions: MailOptions = { from: from || emailService.auth.user, to: to.join(', '), subject, - text: body + text: body, + attachments: Array.isArray(config.attachments) + ? config.attachments.map((path) => ({ path })) + : undefined }
109-127: Avoid logging PII in simulated sending.Printing full recipient list and body can leak sensitive data. Consider masking.
console.warn('📧 Email package not installed - simulating email sending') - console.log(`Provider: ${provider}`) - console.log(`From: ${from || emailService.auth.user}`) - console.log(`To: ${to.join(', ')}`) - console.log(`Subject: ${subject}`) - console.log(`Body: ${body}`) + const mask = (s: string) => s.replace(/(^.).*(@.*$)/, '$1***$2') + console.log(`Provider: ${provider}`) + console.log(`From: ${from || emailService.auth.user}`) + console.log(`To: ${to.map(mask).join(', ')}`) + console.log(`Subject: ${subject}`) + console.log(`Body length: ${body?.length ?? 0} chars`) console.log('💡 To send real emails, install: npm install nodemailer @types/nodemailer')nodes/EmailNode/EmailNode.service.ts (1)
114-128: Avoid over-validating SendGrid API key prefix.Enforcing the "SG." prefix can reject valid keys if SendGrid changes formats. Checking presence is sufficient; rely on send attempt errors for invalid keys.
if (config.emailService.type === 'sendgrid') { if (!config.emailService.apiKey) { return { success: false, error: 'SendGrid API key is required' } } - if (!config.emailService.apiKey.startsWith('SG.')) { - return { - success: false, - error: 'SendGrid API key should start with "SG."' - } - } } else {components/ui/security-status.tsx (2)
20-30: Prefer event-driven updates over 1s polling to reduce work and avoid unnecessary wakeupsPolling every second is wasteful on mobile and background tabs. You can update on focus/visibilitychange and keep a low-frequency fallback timer.
Apply this diff to make updates event-driven and reduce the fallback interval:
useEffect(() => { setMounted(true) const updateStatus = () => { setSecurityState(getSecurityStatus()) } - - updateStatus() - // Update status periodically - const interval = setInterval(updateStatus, 1000) - return () => clearInterval(interval) + + updateStatus() + + const onFocus = () => updateStatus() + const onVisibilityChange = () => { + if (document.visibilityState === 'visible') updateStatus() + } + window.addEventListener('focus', onFocus) + document.addEventListener('visibilitychange', onVisibilityChange) + + // Fallback: periodic refresh (less frequent) + const interval = setInterval(updateStatus, 5000) + return () => { + window.removeEventListener('focus', onFocus) + document.removeEventListener('visibilitychange', onVisibilityChange) + clearInterval(interval) + } }, [])
39-46: Expose status semantics to assistive techAdd role/status semantics so screen readers announce updates when encryption status changes.
- return ( - <div className={`inline-flex items-center gap-2 ${className}`}> + return ( + <div role="status" aria-live="polite" className={`inline-flex items-center gap-2 ${className}`}> <StatusIcon className={`w-4 h-4 ${isSecure ? 'text-green-600' : 'text-yellow-600'}`} /> <span className={`text-sm font-medium ${isSecure ? 'text-green-700' : 'text-yellow-700'}`}> {isSecure ? 'Credentials Encrypted' : 'Security Active'} </span>components/ui/mobile-sheet.tsx (3)
87-93: Ensure internal transform style isn’t unintentionally overridden by consumer propsBecause {...props} comes after style, any style passed by the caller will override your drag transform. Flip the order to keep drag behavior reliable.
onOpenAutoFocus={(e) => e.preventDefault()} // Prevent auto-focus on mobile - style={{ - transform: isDragging ? `translateY(${swipeDistance}px)` : undefined, - transition: isDragging ? 'none' : 'transform 0.2s ease-out' - }} - {...props} + {...props} + style={{ + transform: isDragging ? `translateY(${swipeDistance}px)` : undefined, + transition: isDragging ? 'none' : 'transform 0.2s ease-out' + }} >
164-169: Same style precedence issue in ActionSheet variantMirror the style/props order here as well to avoid losing drag transform when a custom style is provided.
onOpenAutoFocus={(e) => e.preventDefault()} // Prevent auto-focus on mobile - style={{ - transform: isDragging ? `translateY(${swipeDistance}px)` : undefined, - transition: isDragging ? 'none' : 'transform 0.2s ease-out' - }} - {...props} + {...props} + style={{ + transform: isDragging ? `translateY(${swipeDistance}px)` : undefined, + transition: isDragging ? 'none' : 'transform 0.2s ease-out' + }} >
95-102: Prevent page scroll interference during dragAdding touch-action: none to the drag handle reduces unintended page scroll while swiping to close. This improves gesture reliability on mobile.
- <div - className="py-3 flex-shrink-0 sm:hidden cursor-grab active:cursor-grabbing" + <div + className="py-3 flex-shrink-0 sm:hidden cursor-grab active:cursor-grabbing touch-none" onTouchStart={handleTouchStart} onTouchMove={handleTouchMove} onTouchEnd={handleTouchEnd} >- <div - className="py-3 flex-shrink-0 sm:hidden cursor-grab active:cursor-grabbing" + <div + className="py-3 flex-shrink-0 sm:hidden cursor-grab active:cursor-grabbing touch-none" onTouchStart={handleTouchStart} onTouchMove={handleTouchMove} onTouchEnd={handleTouchEnd} >Also applies to: 170-178
nodes/EmailNode/EmailNode.schema.ts (1)
135-166: Harden validation with trimming and type guards for provider credentialsCurrent checks don’t trim whitespace, so inputs like " user@example.com " or " SG.abc " will fail or pass incorrectly. Also, guard against non-string values.
Apply this diff to normalize and validate safely:
// Validate email service configuration if (!typed.emailService) { errors.push('Email service configuration is required') } else { if (!typed.emailService.type) { errors.push('Email service type is required') } - if (!typed.emailService.auth?.user) { - errors.push('Email address is required') - } else if (!isValidEmail(typed.emailService.auth.user)) { - errors.push('Invalid email address format') - } + const userVal = typeof typed.emailService.auth?.user === 'string' + ? typed.emailService.auth.user.trim() + : '' + if (!userVal) { + errors.push('Email address is required') + } else if (!isValidEmail(userVal)) { + errors.push('Invalid email address format') + } if (typed.emailService.type === 'sendgrid') { - if (!typed.emailService.apiKey) { + const apiKeyVal = typeof (typed.emailService as any).apiKey === 'string' + ? (typed.emailService as any).apiKey.trim() + : '' + if (!apiKeyVal) { errors.push('SendGrid API key is required') } } else { - if (!typed.emailService.auth?.pass) { + const passVal = typeof typed.emailService.auth?.pass === 'string' + ? typed.emailService.auth.pass.trim() + : '' + if (!passVal) { errors.push('Email password/app password is required') } } if (typed.emailService.type === 'smtp') { - if (!typed.emailService.host) { + const hostVal = typeof (typed.emailService as any).host === 'string' + ? (typed.emailService as any).host.trim() + : '' + if (!hostVal) { errors.push('SMTP host is required') } } }Also applies to: 175-196
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
app/editor/page.tsx(2 hunks)components/ui/dialog.tsx(0 hunks)components/ui/mobile-sheet.tsx(3 hunks)components/ui/security-status.tsx(1 hunks)components/workflow/execution-log.tsx(2 hunks)components/workflow/node-config-panel.tsx(4 hunks)components/workflow/workflow-toolbar.tsx(2 hunks)hooks/use-workflow-store.ts(7 hunks)lib/node-definitions.test.ts(3 hunks)lib/node-definitions.ts(3 hunks)lib/security.ts(1 hunks)nodes/EmailNode/EmailNode.schema.ts(7 hunks)nodes/EmailNode/EmailNode.service.ts(3 hunks)nodes/EmailNode/EmailNode.test.ts(7 hunks)nodes/EmailNode/EmailNode.types.ts(2 hunks)nodes/EmailNode/email-providers.ts(1 hunks)nodes/EmailNode/nodemailer-types.ts(1 hunks)nodes/index.ts(2 hunks)package.json(2 hunks)
💤 Files with no reviewable changes (1)
- components/ui/dialog.tsx
🧰 Additional context used
🧬 Code Graph Analysis (11)
nodes/EmailNode/email-providers.ts (2)
nodes/EmailNode/EmailNode.types.ts (2)
EmailNodeConfig(3-24)EmailExecutionResult(31-39)nodes/EmailNode/nodemailer-types.ts (5)
NodemailerModule(48-51)Transporter(42-46)MailOptions(6-17)SendMailResult(19-29)NodeRequire(53-55)
components/ui/mobile-sheet.tsx (2)
components/ui/dialog.tsx (4)
Dialog(122-122)DialogContent(123-123)DialogHeader(124-124)DialogTitle(126-126)lib/utils.ts (1)
cn(4-6)
components/workflow/workflow-toolbar.tsx (1)
components/ui/security-status.tsx (1)
SecurityStatus(12-66)
hooks/use-workflow-store.ts (2)
lib/security.ts (3)
encryptEmailConfig(90-116)decryptEmailConfig(121-147)clearSensitiveData(152-178)types/workflow.ts (1)
Workflow(77-87)
components/workflow/execution-log.tsx (2)
types/workflow.ts (1)
ExecutionLog(101-107)lib/utils.ts (1)
cn(4-6)
nodes/EmailNode/EmailNode.test.ts (3)
nodes/EmailNode/EmailNode.types.ts (1)
EmailNodeConfig(3-24)nodes/EmailNode/EmailNode.schema.ts (1)
EMAIL_NODE_DEFINITION(26-214)nodes/types.ts (1)
createTestContext(24-34)
nodes/EmailNode/EmailNode.service.ts (3)
nodes/EmailNode/EmailNode.types.ts (2)
EmailNodeConfig(3-24)EmailExecutionResult(31-39)nodes/EmailNode/email-providers.ts (2)
sendWithSendGrid(65-107)sendWithNodemailer(15-63)nodes/types.ts (2)
NodeExecutionContext(5-13)NodeExecutionResult(15-19)
components/ui/security-status.tsx (1)
lib/security.ts (1)
getSecurityStatus(194-204)
lib/node-definitions.ts (2)
nodes/index.ts (2)
NodeType(118-118)NodeDefinition(31-53)nodes/EmailNode/EmailNode.schema.ts (1)
EMAIL_NODE_DEFINITION(26-214)
components/workflow/node-config-panel.tsx (2)
nodes/index.ts (1)
NodeType(118-118)components/ui/input.tsx (1)
Input(24-24)
app/editor/page.tsx (1)
types/workflow.ts (1)
Workflow(77-87)
🪛 Biome (2.1.2)
nodes/EmailNode/email-providers.ts
[error] 141-141: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
[error] 148-148: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
🔇 Additional comments (19)
package.json (1)
7-9: Build pipeline tightening LGTMRunning typecheck and strict linting before build is a good guardrail for the new types and UI changes.
components/workflow/node-config-panel.tsx (1)
396-447: New parameter input types LGTMAdding explicit cases for email/password/url/text improves UX and validation fidelity. The placeholders and type-specific behaviors are consistent with the existing pattern.
components/workflow/workflow-toolbar.tsx (1)
10-10: Import looks correct and aligns with the new security UI.The named import for SecurityStatus matches the new component and should tree-shake fine.
lib/node-definitions.ts (4)
28-32: Type surface expansion is appropriate.Adding 'text' | 'email' | 'url' | 'password' to ParameterType is consistent with the new Email node schema and future UI controls.
41-41: Addingplaceholderto ParameterDefinition is useful for UX.This aligns with the Email node parameters using placeholders.
127-165: Default routing implementation looks good.Explicitly delegating to per-node getDefaults avoids the stale legacy array issue.
167-211: Validation routing is clear and aligns with the modular design.Good use of per-node validate functions with typed narrowing by subtype.
lib/node-definitions.test.ts (1)
45-53: Tests correctly cover the new emailService shape.Good to assert a valid Gmail-like config with auth.user/pass; this guards schema regressions.
nodes/EmailNode/EmailNode.test.ts (2)
7-24: Solid test helper abstraction for email configs.This reduces duplication and keeps all tests aligned with the required emailService shape.
106-114: Default config assertion matches schema defaults.Including emailService with empty credentials and type 'gmail' is correct and future-proof.
nodes/index.ts (2)
20-20: Including 'password' in parameter types is appropriate.This aligns with credential fields in Email/SMTP providers.
26-26: AddingshowIfenables conditional UI—good addition.This unblocks dynamic forms like SMTP-specific fields.
app/editor/page.tsx (2)
60-62: Smooth collapsible logs panel integration looks good.Clean, responsive transition and width toggle via store state is well-implemented and keeps layout concerns localized to the editor shell.
21-29: Good call using sessionStorage for drafts in the current session.Reading sessionDraft first and normalizing dates before setWorkflow is sound and reduces stale/cross-session leakage. Once setWorkflow normalizes dates internally (see store comment), this block can be simplified further.
nodes/EmailNode/email-providers.ts (1)
65-107: Ensure global fetch is available in your runtimeThe
sendWithSendGridfunction innodes/EmailNode/email-providers.ts(lines 65–107) relies on the globalfetchAPI (as do other parts of your server, e.g.server/services/http-client.ts). However:
- There is no
"engines"field inpackage.jsonto enforce Node 18+- There are no polyfill dependencies (
undici,node-fetch,cross-fetch, etc.)Please choose one of the following:
• Constrain your runtime to Node 18+ by adding to
package.json:"engines": { "node": ">=18" }• Or install and initialize a fetch polyfill. For example:
npm install undici- At server startup:
import { fetch } from 'undici'; global.fetch = fetch;hooks/use-workflow-store.ts (1)
181-225: Encrypt-before-persist and keep decrypted in-memory: solid pattern.Good separation of concerns: encrypted persistence to localStorage, decrypted state for UI, and a post-save sensitive-fields scrub.
nodes/EmailNode/EmailNode.service.ts (1)
25-53: Provider-specific SMTP presets are cleanly factored.The Gmail/Outlook/SMTP routing through the shared Nodemailer path keeps provider branching minimal and testable.
nodes/EmailNode/EmailNode.schema.ts (2)
200-213: Defaults look consistent with new schemaInclusion of
attachments: []andemailServicedefaults aligns with validation paths and provider options. No issues spotted.
32-40: Verify Multi-Recipient Handling for the “to” Email Field
- In nodes/EmailNode/EmailNode.schema.ts (lines 32–40), the
toparameter is defined withtype: 'email'yet hasdefaultValue: []and is validated as an array.- The core
ParameterDefinition.type(in nodes/index.ts) only permits scalar types and offers nomultipleflag or array‐specific option.- No form‐renderer code was found that explicitly treats
type: 'email'as a multi‐value (e.g., chips/list) input.Please confirm whether the UI form renderer can handle an array of emails for
type: 'email'. If it doesn’t, consider:
- Adding a
multiple: trueproperty toParameterDefinition- Introducing a distinct parameter type, e.g.
'emailList', for multi-recipient fields
| export function findNodeDefinition(node: WorkflowNode): NodeDefinition | undefined { | ||
| const data = node.data as WorkflowNode['data'] | ||
|
|
||
| // Special case for EmailNode - use the new definition | ||
| if (data.nodeType === NodeType.ACTION && (data as { actionType: ActionType }).actionType === ActionType.EMAIL) { | ||
| return EMAIL_NODE_DEFINITION as unknown as NodeDefinition | ||
| } | ||
|
|
||
| // Use legacy system for other nodes |
There was a problem hiding this comment.
🛠️ Refactor suggestion
findNodeDefinition fallback will return undefined for non-Email nodes; route to the central registry.
NODE_DEFINITIONS is empty, so HTTP/Trigger/Logic nodes will fail to resolve here. Parts of the app that call findNodeDefinition for non-email nodes will break.
Refactor to delegate to the node registry (nodes/index.ts) for all node types, using the email special-case only if truly necessary.
Apply this diff within the function:
export function findNodeDefinition(node: WorkflowNode): NodeDefinition | undefined {
const data = node.data as WorkflowNode['data']
-
- // Special case for EmailNode - use the new definition
- if (data.nodeType === NodeType.ACTION && (data as { actionType: ActionType }).actionType === ActionType.EMAIL) {
- return EMAIL_NODE_DEFINITION as unknown as NodeDefinition
- }
-
- // Use legacy system for other nodes
- switch (data.nodeType) {
- case NodeType.ACTION:
- return NODE_DEFINITIONS.find((d) => d.nodeType === NodeType.ACTION && d.subType === (data as { actionType: ActionType }).actionType)
- case NodeType.TRIGGER:
- return NODE_DEFINITIONS.find((d) => d.nodeType === NodeType.TRIGGER && d.subType === (data as { triggerType: TriggerType }).triggerType)
- case NodeType.LOGIC:
- return NODE_DEFINITIONS.find((d) => d.nodeType === NodeType.LOGIC && d.subType === (data as { logicType: LogicType }).logicType)
- default:
- return undefined
- }
+ // Prefer the central registry for resolution to avoid drift
+ try {
+ const { getNodeDefinition } = require('@/nodes') as {
+ getNodeDefinition: (nodeType: NodeType, subType: string | number) => NodeDefinition | undefined
+ }
+ if (data.nodeType === NodeType.ACTION) {
+ return getNodeDefinition(NodeType.ACTION, (data as { actionType: ActionType }).actionType)
+ }
+ if (data.nodeType === NodeType.TRIGGER) {
+ return getNodeDefinition(NodeType.TRIGGER, (data as { triggerType: TriggerType }).triggerType)
+ }
+ if (data.nodeType === NodeType.LOGIC) {
+ return getNodeDefinition(NodeType.LOGIC, (data as { logicType: LogicType }).logicType)
+ }
+ } catch {}
+ return undefined
}Additionally add the static import near the top if your build allows it (preferred over require); otherwise, the dynamic require above avoids circular import initialization issues:
import { getNodeDefinition } from '@/nodes'🤖 Prompt for AI Agents
In lib/node-definitions.ts around lines 106 to 114, the function currently
special-cases Email nodes and returns undefined for all other node types which
breaks resolution because NODE_DEFINITIONS is empty; change it to delegate node
lookup to the central registry (nodes/index.ts) by calling
getNodeDefinition(node) for all nodes and only fall back to the
EMAIL_NODE_DEFINITION when the registry lookup fails or when an Email action
truly requires the legacy definition; add a static import at the top: import {
getNodeDefinition } from '@/nodes' if your build supports it, otherwise keep the
existing dynamic require approach to avoid circular-init issues, and ensure the
function returns the registry result (or the email fallback) instead of
undefined.
| export function clearSensitiveData(): void { | ||
| // Clear password input fields | ||
| const passwordFields = document.querySelectorAll('input[type="password"]') | ||
| passwordFields.forEach((field) => { | ||
| if (field instanceof HTMLInputElement) { | ||
| field.value = '' | ||
| } | ||
| }) | ||
|
|
||
| // Clear any sensitive data from forms | ||
| const sensitiveSelectors = [ | ||
| 'input[name*="pass"]', | ||
| 'input[name*="password"]', | ||
| 'input[name*="secret"]', | ||
| 'input[name*="key"]', | ||
| 'input[name*="apiKey"]' | ||
| ] | ||
|
|
||
| sensitiveSelectors.forEach(selector => { | ||
| const fields = document.querySelectorAll(selector) | ||
| fields.forEach((field) => { | ||
| if (field instanceof HTMLInputElement) { | ||
| field.value = '' | ||
| } | ||
| }) | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard DOM access in clearSensitiveData to avoid SSR crashes
document is undefined in SSR/RSC and some test environments.
export function clearSensitiveData(): void {
+ if (typeof document === 'undefined') return
// Clear password input fields
const passwordFields = document.querySelectorAll('input[type="password"]')
passwordFields.forEach((field) => {
if (field instanceof HTMLInputElement) {
field.value = ''
}
})
// Clear any sensitive data from forms
const sensitiveSelectors = [
'input[name*="pass"]',
'input[name*="password"]',
'input[name*="secret"]',
'input[name*="key"]',
'input[name*="apiKey"]'
]
sensitiveSelectors.forEach(selector => {
const fields = document.querySelectorAll(selector)
fields.forEach((field) => {
if (field instanceof HTMLInputElement) {
field.value = ''
}
})
})
}📝 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.
| export function clearSensitiveData(): void { | |
| // Clear password input fields | |
| const passwordFields = document.querySelectorAll('input[type="password"]') | |
| passwordFields.forEach((field) => { | |
| if (field instanceof HTMLInputElement) { | |
| field.value = '' | |
| } | |
| }) | |
| // Clear any sensitive data from forms | |
| const sensitiveSelectors = [ | |
| 'input[name*="pass"]', | |
| 'input[name*="password"]', | |
| 'input[name*="secret"]', | |
| 'input[name*="key"]', | |
| 'input[name*="apiKey"]' | |
| ] | |
| sensitiveSelectors.forEach(selector => { | |
| const fields = document.querySelectorAll(selector) | |
| fields.forEach((field) => { | |
| if (field instanceof HTMLInputElement) { | |
| field.value = '' | |
| } | |
| }) | |
| }) | |
| } | |
| export function clearSensitiveData(): void { | |
| if (typeof document === 'undefined') return | |
| // Clear password input fields | |
| const passwordFields = document.querySelectorAll('input[type="password"]') | |
| passwordFields.forEach((field) => { | |
| if (field instanceof HTMLInputElement) { | |
| field.value = '' | |
| } | |
| }) | |
| // Clear any sensitive data from forms | |
| const sensitiveSelectors = [ | |
| 'input[name*="pass"]', | |
| 'input[name*="password"]', | |
| 'input[name*="secret"]', | |
| 'input[name*="key"]', | |
| 'input[name*="apiKey"]' | |
| ] | |
| sensitiveSelectors.forEach(selector => { | |
| const fields = document.querySelectorAll(selector) | |
| fields.forEach((field) => { | |
| if (field instanceof HTMLInputElement) { | |
| field.value = '' | |
| } | |
| }) | |
| }) | |
| } |
🤖 Prompt for AI Agents
In lib/security.ts around lines 152 to 178, clearSensitiveData() currently
accesses document directly which will throw in SSR/RSC or some test
environments; wrap the DOM access in a guard (e.g., if (typeof document ===
'undefined' || typeof window === 'undefined') return) or check for
document.querySelectorAll existence before proceeding, then keep the existing
logic to select and clear inputs only when the DOM is available; ensure the
function returns early when running server-side to avoid crashes.
| export function getSecurityStatus(): { | ||
| encrypted: boolean | ||
| sessionBased: boolean | ||
| deviceKey: boolean | ||
| } { | ||
| return { | ||
| encrypted: !!sessionStorage.getItem('deviceKey'), | ||
| sessionBased: true, // Using sessionStorage | ||
| deviceKey: !!sessionStorage.getItem('deviceKey') | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make getSecurityStatus SSR-safe and resilient to storage failures
Direct sessionStorage access can throw.
export function getSecurityStatus(): {
encrypted: boolean
sessionBased: boolean
deviceKey: boolean
} {
- return {
- encrypted: !!sessionStorage.getItem('deviceKey'),
- sessionBased: true, // Using sessionStorage
- deviceKey: !!sessionStorage.getItem('deviceKey')
- }
+ if (typeof window === 'undefined') {
+ return { encrypted: false, sessionBased: false, deviceKey: false }
+ }
+ let hasKey = false
+ try {
+ hasKey = !!window.sessionStorage?.getItem('deviceKey')
+ } catch {
+ hasKey = false
+ }
+ return { encrypted: hasKey, sessionBased: true, deviceKey: hasKey }
}📝 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.
| export function getSecurityStatus(): { | |
| encrypted: boolean | |
| sessionBased: boolean | |
| deviceKey: boolean | |
| } { | |
| return { | |
| encrypted: !!sessionStorage.getItem('deviceKey'), | |
| sessionBased: true, // Using sessionStorage | |
| deviceKey: !!sessionStorage.getItem('deviceKey') | |
| } | |
| } | |
| export function getSecurityStatus(): { | |
| encrypted: boolean | |
| sessionBased: boolean | |
| deviceKey: boolean | |
| } { | |
| if (typeof window === 'undefined') { | |
| return { encrypted: false, sessionBased: false, deviceKey: false } | |
| } | |
| let hasKey = false | |
| try { | |
| hasKey = !!window.sessionStorage?.getItem('deviceKey') | |
| } catch { | |
| hasKey = false | |
| } | |
| return { encrypted: hasKey, sessionBased: true, deviceKey: hasKey } | |
| } |
🤖 Prompt for AI Agents
In lib/security.ts around lines 194 to 204, calling sessionStorage.getItem
directly is unsafe for SSR and can throw; make the function check that window
and sessionStorage exist (typeof window !== 'undefined' && typeof sessionStorage
!== 'undefined'), then wrap the getItem call(s) in a try/catch and read the
value into a local variable once; set encrypted and deviceKey based on the
retrieved value (fallback to false on error), and set sessionBased to true only
when sessionStorage is available, returning those values. Ensure no direct
sessionStorage access occurs outside the availability check and catch block so
the function is SSR-safe and resilient to storage failures.
| const transporter: Transporter = nodemailer.createTransporter({ | ||
| host: emailService.host, | ||
| port: emailService.port || 587, | ||
| secure: emailService.secure || false, | ||
| auth: { | ||
| user: emailService.auth.user, | ||
| pass: emailService.auth.pass | ||
| } | ||
| }) |
There was a problem hiding this comment.
Incorrect Nodemailer API usage will crash when nodemailer is present.
Nodemailer exposes createTransport, not createTransporter. This will throw “createTransporter is not a function” in real environments.
- const transporter: Transporter = nodemailer.createTransporter({
+ const createTransport =
+ (nodemailer as unknown as { createTransport?: (opts: unknown) => Transporter }).createTransport ??
+ (nodemailer as unknown as { createTransporter?: (opts: unknown) => Transporter }).createTransporter
+ if (typeof createTransport !== 'function') {
+ throw new Error('Invalid nodemailer module: missing createTransport')
+ }
+ const transporter: Transporter = createTransport({
host: emailService.host,
port: emailService.port || 587,
secure: emailService.secure || false,
auth: {
user: emailService.auth.user,
pass: emailService.auth.pass
}
})📝 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 transporter: Transporter = nodemailer.createTransporter({ | |
| host: emailService.host, | |
| port: emailService.port || 587, | |
| secure: emailService.secure || false, | |
| auth: { | |
| user: emailService.auth.user, | |
| pass: emailService.auth.pass | |
| } | |
| }) | |
| const createTransport = | |
| (nodemailer as unknown as { createTransport?: (opts: unknown) => Transporter }).createTransport ?? | |
| (nodemailer as unknown as { createTransporter?: (opts: unknown) => Transporter }).createTransporter | |
| if (typeof createTransport !== 'function') { | |
| throw new Error('Invalid nodemailer module: missing createTransport') | |
| } | |
| const transporter: Transporter = createTransport({ | |
| host: emailService.host, | |
| port: emailService.port || 587, | |
| secure: emailService.secure || false, | |
| auth: { | |
| user: emailService.auth.user, | |
| pass: emailService.auth.pass | |
| } | |
| }) |
🤖 Prompt for AI Agents
In nodes/EmailNode/email-providers.ts around lines 31 to 39, the code calls
nodemailer.createTransporter which does not exist; change the call to
nodemailer.createTransport with the same config object (host, port, secure,
auth) so it uses the correct Nodemailer API; keep or adjust the TypeScript type
for the transporter (Transporter) as needed to match
nodemailer.createTransport's return type.
| async function loadNodemailer(): Promise<NodemailerModule> { | ||
| // Try different methods to load nodemailer without causing build issues | ||
|
|
||
| // Check if we're in Node.js environment | ||
| if (typeof window !== 'undefined') { | ||
| throw new Error('Nodemailer only works in Node.js environment') | ||
| } | ||
|
|
||
| try { | ||
| // Method 1: Try require if available (Node.js environment) | ||
| const globalAny = globalThis as Record<string, unknown> | ||
| const nodeRequire = (globalAny.require as NodeRequire) || (eval('require') as NodeRequire) | ||
| const nodemailerModule = nodeRequire('nodemailer') as NodemailerModule | ||
| return nodemailerModule | ||
| } catch (requireError) { | ||
| try { | ||
| // Method 2: Try dynamic import with string variable to avoid TypeScript checking | ||
| const moduleName = 'nodemailer' | ||
| const importResult = await eval(`import('${moduleName}')`) as NodemailerModule | ||
| return importResult | ||
| } catch (importError) { | ||
| // All methods failed | ||
| throw new Error('Nodemailer not available') | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Replace eval-based module loading (security lint) with createRequire/dynamic import.
Biome flags eval usage; it's also unnecessary. Use module.createRequire and a standard dynamic import as fallback.
async function loadNodemailer(): Promise<NodemailerModule> {
// Try different methods to load nodemailer without causing build issues
// Check if we're in Node.js environment
if (typeof window !== 'undefined') {
throw new Error('Nodemailer only works in Node.js environment')
}
try {
- // Method 1: Try require if available (Node.js environment)
- const globalAny = globalThis as Record<string, unknown>
- const nodeRequire = (globalAny.require as NodeRequire) || (eval('require') as NodeRequire)
- const nodemailerModule = nodeRequire('nodemailer') as NodemailerModule
- return nodemailerModule
+ // Method 1: Node.js require using createRequire in ESM environments
+ const { createRequire } = await import('module')
+ const req = createRequire(import.meta.url)
+ const mod = req('nodemailer') as unknown as NodemailerModule
+ return (mod as any).default ?? mod
} catch (requireError) {
try {
- // Method 2: Try dynamic import with string variable to avoid TypeScript checking
- const moduleName = 'nodemailer'
- const importResult = await eval(`import('${moduleName}')`) as NodemailerModule
- return importResult
+ // Method 2: Dynamic import
+ const importResult = (await import('nodemailer')) as unknown as NodemailerModule
+ return (importResult as any).default ?? importResult
} catch (importError) {
// All methods failed
throw new Error('Nodemailer not available')
}
}
}📝 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 function loadNodemailer(): Promise<NodemailerModule> { | |
| // Try different methods to load nodemailer without causing build issues | |
| // Check if we're in Node.js environment | |
| if (typeof window !== 'undefined') { | |
| throw new Error('Nodemailer only works in Node.js environment') | |
| } | |
| try { | |
| // Method 1: Try require if available (Node.js environment) | |
| const globalAny = globalThis as Record<string, unknown> | |
| const nodeRequire = (globalAny.require as NodeRequire) || (eval('require') as NodeRequire) | |
| const nodemailerModule = nodeRequire('nodemailer') as NodemailerModule | |
| return nodemailerModule | |
| } catch (requireError) { | |
| try { | |
| // Method 2: Try dynamic import with string variable to avoid TypeScript checking | |
| const moduleName = 'nodemailer' | |
| const importResult = await eval(`import('${moduleName}')`) as NodemailerModule | |
| return importResult | |
| } catch (importError) { | |
| // All methods failed | |
| throw new Error('Nodemailer not available') | |
| } | |
| } | |
| } | |
| async function loadNodemailer(): Promise<NodemailerModule> { | |
| // Try different methods to load nodemailer without causing build issues | |
| // Check if we're in Node.js environment | |
| if (typeof window !== 'undefined') { | |
| throw new Error('Nodemailer only works in Node.js environment') | |
| } | |
| try { | |
| // Method 1: Node.js require using createRequire in ESM environments | |
| const { createRequire } = await import('module') | |
| const req = createRequire(import.meta.url) | |
| const mod = req('nodemailer') as unknown as NodemailerModule | |
| return (mod as any).default ?? mod | |
| } catch (requireError) { | |
| try { | |
| // Method 2: Dynamic import | |
| const importResult = (await import('nodemailer')) as unknown as NodemailerModule | |
| return (importResult as any).default ?? importResult | |
| } catch (importError) { | |
| // All methods failed | |
| throw new Error('Nodemailer not available') | |
| } | |
| } | |
| } |
🧰 Tools
🪛 Biome (2.1.2)
[error] 141-141: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
[error] 148-148: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
🤖 Prompt for AI Agents
In nodes/EmailNode/email-providers.ts around lines 130 to 155, replace the
eval-based require/import logic with a safe createRequire + standard dynamic
import fallback: use createRequire from the 'module' package to synchronously
require('nodemailer') (e.g. const require = createRequire(import.meta.url)) and
if that fails, perform a normal dynamic import('nodemailer') without eval; keep
the Node.js environment check and throw the same error if both methods fail.
Ensure no eval is used and use standard APIs (module.createRequire and import())
as described.
| path: 'emailService.auth.pass', | ||
| label: 'Password/App Password', | ||
| type: 'password', | ||
| required: true, | ||
| description: 'Your email password or app-specific password', | ||
| placeholder: 'Enter your app password' | ||
| }, |
There was a problem hiding this comment.
Password field is marked required but conflicts with SendGrid flow
The UI marks emailService.auth.pass as required globally, but your validation logic makes it optional for sendgrid. This creates UX inconsistency and can block valid SendGrid configs.
Minimal fix: make the field not required and let the validator enforce it for non-SendGrid types.
{
path: 'emailService.auth.pass',
label: 'Password/App Password',
type: 'password',
- required: true,
+ required: false,
description: 'Your email password or app-specific password',
placeholder: 'Enter your app password'
},Optional (if your form engine supports conditional visibility or “any-of” conditions): show this field only when provider is gmail, outlook, or smtp.
📝 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.
| path: 'emailService.auth.pass', | |
| label: 'Password/App Password', | |
| type: 'password', | |
| required: true, | |
| description: 'Your email password or app-specific password', | |
| placeholder: 'Enter your app password' | |
| }, | |
| path: 'emailService.auth.pass', | |
| label: 'Password/App Password', | |
| type: 'password', | |
| required: false, | |
| description: 'Your email password or app-specific password', | |
| placeholder: 'Enter your app password' | |
| }, |
🤖 Prompt for AI Agents
In nodes/EmailNode/EmailNode.schema.ts around lines 91 to 97, the password field
is declared required globally which conflicts with your validator that allows
omission for SendGrid; change the schema to make path 'emailService.auth.pass'
not required (required: false or remove the required flag) so the UI won't block
SendGrid configs, and rely on the existing validation logic to enforce the
password for non-sendgrid providers; optionally, if supported by the form
engine, add conditional visibility or an any-of rule to display the field only
for gmail, outlook, or smtp providers.
| export interface ParameterDefinition { | ||
| name: string | ||
| label: string | ||
| type: 'text' | 'textarea' | 'select' | 'number' | 'boolean' | 'email' | 'url' | 'json' | ||
| type: 'text' | 'textarea' | 'select' | 'number' | 'boolean' | 'email' | 'url' | 'json' | 'password' | ||
| required?: boolean | ||
| defaultValue?: unknown | ||
| options?: Array<{ label: string; value: string }> | ||
| placeholder?: string | ||
| description?: string | ||
| showIf?: Array<{ path: string; equals: string | number | boolean }> | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
ParameterDefinition likely needs path?: string for path-based configs used by Email node.
The new Email schema uses path-style parameters (e.g., 'emailService.auth.user'). This interface only exposes name. Add path?: string (and keep name for backwards compatibility) to prevent type friction.
Apply this diff:
export interface ParameterDefinition {
- name: string
+ name: string
+ // New path-based access into config (preferred). Keep `name` for legacy nodes.
+ path?: string
label: string
type: 'text' | 'textarea' | 'select' | 'number' | 'boolean' | 'email' | 'url' | 'json' | 'password'
required?: boolean
defaultValue?: unknown
options?: Array<{ label: string; value: string }>
placeholder?: string
description?: string
showIf?: Array<{ path: string; equals: string | number | boolean }>
}📝 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.
| export interface ParameterDefinition { | |
| name: string | |
| label: string | |
| type: 'text' | 'textarea' | 'select' | 'number' | 'boolean' | 'email' | 'url' | 'json' | |
| type: 'text' | 'textarea' | 'select' | 'number' | 'boolean' | 'email' | 'url' | 'json' | 'password' | |
| required?: boolean | |
| defaultValue?: unknown | |
| options?: Array<{ label: string; value: string }> | |
| placeholder?: string | |
| description?: string | |
| showIf?: Array<{ path: string; equals: string | number | boolean }> | |
| } | |
| export interface ParameterDefinition { | |
| name: string | |
| // New path-based access into config (preferred). Keep `name` for legacy nodes. | |
| path?: string | |
| label: string | |
| type: 'text' | 'textarea' | 'select' | 'number' | 'boolean' | 'email' | 'url' | 'json' | 'password' | |
| required?: boolean | |
| defaultValue?: unknown | |
| options?: Array<{ label: string; value: string }> | |
| placeholder?: string | |
| description?: string | |
| showIf?: Array<{ path: string; equals: string | number | boolean }> | |
| } |
🤖 Prompt for AI Agents
In nodes/index.ts around lines 17 to 27, the ParameterDefinition interface lacks
an optional path property required by path-style configs (e.g.,
"emailService.auth.user"); update the interface to include path?: string while
retaining the existing name field for backward compatibility, and ensure any
consumers/types that expect ParameterDefinition accept the new optional path
property (no required runtime changes).
|
|
||
| // Register all nodes on module load | ||
| registerNode(EMAIL_NODE_DEFINITION) | ||
| // EMAIL_NODE_DEFINITION is handled directly in findNodeDefinition for now |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reconsider not registering EMAIL_NODE_DEFINITION; it may disappear from discovery UIs.
If any UI enumerates getAllNodeDefinitions() (e.g., node palette), Email will be missing. Prefer registering Email like other nodes; remove the special-casing elsewhere by making lib/node-definitions.ts consult this registry.
-// EMAIL_NODE_DEFINITION is handled directly in findNodeDefinition for now
+// Register Email as well for discovery; resolution can still go via the registry
+registerNode(EMAIL_NODE_DEFINITION)🤖 Prompt for AI Agents
In nodes/index.ts around line 109, the comment indicates EMAIL_NODE_DEFINITION
is special-cased instead of being registered; update the code to register the
Email node like other nodes so getAllNodeDefinitions() will include it. Remove
or refactor the special-case in lib/node-definitions.ts so it consults the
central registry (or call the registry from there) rather than relying on
findNodeDefinition; ensure registration follows the same pattern and naming as
other node registrations and that any lookup code uses the registry API so
discovery UIs can enumerate Email normally.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 participant
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 #
Summary by CodeRabbit
New Features
Style
Tests
Chores