feat: Add Email Trigger Node for IMAP email monitoring#26
Conversation
- Add EmailTriggerNode with full IMAP support - Implement EmailTriggerService with connection management - Add comprehensive configuration options and email parsing - Include unit tests and integration tests - Add detailed README documentation - Update node registry and workflow types - Add required dependencies (imap-simple, mailparser)
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary by CodeRabbit
WalkthroughAdds a server-only Email Trigger node: types, schema, React Flow node UI, IMAP-backed EmailTriggerService, tests, README, package deps, registry registration and export, webpack client fallbacks disabled, and ESLint/.mjs parser override. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Registry as Node Registry
participant UI as EmailTriggerNode (React)
participant Engine as Workflow Engine
participant Service as EmailTriggerService
participant IMAP as IMAP Server
User->>Registry: load node definitions
Registry-->>User: includes EMAIL_TRIGGER_NODE_DEFINITION (serverSideOnly)
User->>UI: render node with config
Engine->>Service: connect(config)
Service->>IMAP: establish TLS connection & open mailbox
IMAP-->>Service: connected
loop trigger cycle
Engine->>Service: fetchEmails(config, lastMessageId?)
Service->>IMAP: search UNSEEN + custom rules
IMAP-->>Service: messages
Service->>Service: parse messages (mailparser), download attachments?
alt postProcessAction == read
Service->>IMAP: mark as Seen
end
Service-->>Engine: return emails[], lastMessageId
end
Engine->>Service: disconnect()
Service->>IMAP: close mailbox + end
IMAP-->>Service: closed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/integration/node-registry.test.ts (2)
101-127: Defaults validation will fail for Email Trigger; update the allowlistEmail trigger defaults intentionally omit credentials; current test wrongly expects validation to pass. Include TriggerType.EMAIL among nodes that require user config.
- const nodesRequiringConfig = [ - ActionType.EMAIL, - ActionType.HTTP, - ActionType.DATABASE, - ActionType.TRANSFORM, - LogicType.IF, - LogicType.FILTER - ] + const nodesRequiringConfig = [ + // Actions + ActionType.EMAIL, + ActionType.HTTP, + ActionType.DATABASE, + ActionType.TRANSFORM, + // Logic + LogicType.IF, + LogicType.FILTER, + // Triggers requiring credentials or external setup + TriggerType.EMAIL + ] if (!nodesRequiringConfig.includes(node.subType as ActionType | LogicType)) { expect(errors).toEqual([]) }
199-206: Cover TriggerType.EMAIL in enum coverage testAdd EMAIL to enum coverage to match the new node.
it('should cover all TriggerType enum values', () => { const registeredTriggers = getNodesByType(NodeType.TRIGGER).map(n => n.subType) // Check that all important trigger types are covered expect(registeredTriggers).toContain(TriggerType.MANUAL) expect(registeredTriggers).toContain(TriggerType.SCHEDULE) expect(registeredTriggers).toContain(TriggerType.WEBHOOK) + expect(registeredTriggers).toContain(TriggerType.EMAIL) })
🧹 Nitpick comments (16)
nodes/index.ts (1)
126-129: Align getNodeDefinition signature with other helpersOther helpers accept string | number for subtype; widen this one for consistency.
-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) }nodes/EmailTriggerNode/index.ts (1)
1-4: Keep service export here; avoid re-export from root to prevent client bundling issuesThis barrel is fine. The fix is to stop re-exporting this barrel from nodes/index.ts (see suggested change there).
If using Next.js, add a server-only guard at the top of EmailTriggerNode.service.ts:
import 'server-only'nodes/EmailTriggerNode/README.md (1)
46-77: Update examples if you keep forceReconnect (or rename consistently)Examples should reflect the chosen option name. If keeping forceReconnect, add it explicitly (or remove reconnectIntervalMinutes if present elsewhere).
"mailbox": "INBOX", "postProcessAction": "read", - "downloadAttachments": true + "downloadAttachments": true, + "forceReconnect": 0nodes/EmailTriggerNode/EmailTriggerNode.tsx (1)
34-41: Avoid showing undefined port; fall back to defaults in description.If port is not set yet in config, the UI will render
host:undefined. Use a sensible fallback (e.g., 993 whensecure, 143 otherwise).- parts.push(`${config.host}:${config.port}`) + const port = typeof config.port === 'number' ? config.port : (secure ? 993 : 143) + parts.push(`${config.host}:${port}`)nodes/EmailTriggerNode/EmailTriggerNode.schema.ts (1)
121-124: ClarifyshowIfsemantics when providing multiple conditions.If
showIfis ANDed,downloadAttachmentswill never show unless format equals both 'simple' and 'resolved'. If it’s meant to be OR, encode accordingly (e.g., a single condition that accepts an array, or adjust renderer).I can update the renderer or schema to express OR explicitly if needed.
nodes/EmailTriggerNode/EmailTriggerNode.service.ts (3)
73-83: Optional: de-duplicate search criteria.If custom rules include
UNSEEN, you’ll push it twice. Not harmful, but easy to avoid.- const customRules = JSON.parse(config.customEmailRules) as (string | string[])[] - searchCriteria.push(...customRules) + const customRules = JSON.parse(config.customEmailRules) as (string | string[])[] + for (const rule of customRules) { + if (rule === 'UNSEEN' && searchCriteria.includes('UNSEEN')) continue + searchCriteria.push(rule) + }
23-29: Prefer a project logger over console logging.Use your centralized logger to capture
onmail/onupdateevents with levels and context.
220-231: Potential performance: avoid fetching all UIDs when only the last is needed.Large mailboxes make
search(['ALL'])expensive. Consider['UID', '*']or['ALL']withfetchOptions: { bodies: [], markSeen: false }and limit result set if the library supports it.nodes/EmailTriggerNode/EmailTriggerNode.types.ts (8)
5-9: Support OAuth2/XOAUTH2 in addition to basic auth.Many IMAP providers (notably Gmail) require OAuth2. Consider adding explicit auth fields while keeping backward compatibility.
Apply this diff:
host: string port: number - secure: boolean - user: string - password: string + secure: boolean + user: string + password?: string + authMethod?: 'basic' | 'xoauth2' + xoauth2Token?: string
25-28: Clarify TLS settings; avoid double-negatives and add room for custom options.
allowUnauthorizedCertsis easy to misuse;enabledis ambiguous. Prefer explicittlsOptionswithrejectUnauthorized, and addstartTLSif supported.Apply this diff:
- // TLS Settings - allowUnauthorizedCerts?: boolean - enabled?: boolean + // TLS Settings + // NOTE: `secure` above maps to IMAP TLS-on-connect. Add StartTLS and explicit options. + startTLS?: boolean + tlsOptions?: { + rejectUnauthorized?: boolean // default true + minVersion?: string + ca?: string + cert?: string + key?: string + } + /** @deprecated Use tlsOptions.rejectUnauthorized */ + allowUnauthorizedCerts?: boolean + /** @deprecated Use `secure` or `startTLS` */ + enabled?: boolean
17-21: TypecustomEmailRulesas structured criteria, not JSON string.Treating this as JSON text loses type-safety and defers errors to runtime.
Apply this diff:
- customEmailRules?: string // JSON array of IMAP search criteria + // IMAP search criteria (e.g., ['UNSEEN', ['SINCE', '01-Aug-2025']]) + customEmailRules?: string[]If you must accept JSON text from UI, keep a second field
customEmailRulesJson?: stringand parse/validate intostring[]at the service boundary.
35-41: Use ISO string for timestamp for transport safety.
Dateserializes to string anyway and can be inconsistent across boundaries (web workers, JSON).Apply this diff:
- timestamp: Date + timestamp: string // ISO 8601
43-57: Tighten message typing: headers multiplicity and id/uid semantics.
- Headers can have multiple values; current
Record<string, string>loses data.- Consider distinguishing RFC 2822
Message-IDfrom internalid.Apply this diff:
- id: string + /** Internal id for de-duplication (e.g., Message-ID or synthesized) */ + id: string + /** RFC 2822 Message-ID header, if available */ + messageId?: string @@ - headers: Record<string, string> + headers: Record<string, string | string[]>Additionally, consider a discriminated union based on
formatto makerawrequired/others optional:// outside this interface export type EmailMessage = | ({ format: 'raw' } & BaseEmailMessage & { raw: string }) | ({ format: 'resolved' | 'simple' } & BaseEmailMessage);
64-69: AvoidBufferin shared types; use transport-friendly content.
Bufferties types to Node and can bloat payloads. PreferUint8Arrayor base64.Apply this diff:
- content: Buffer + // Prefer one of these; service can map between them as needed + content?: Uint8Array + contentBase64?: stringIf you keep
Bufferinternally, define a server-only type and map to a front-end-safe DTO.
15-16: Makeformata discriminator for the result payload.Today
rawis optional even whenformat === 'raw'. Using a discriminated union prevents misuse at compile time.I can provide a small refactor PR snippet to introduce
BaseEmailMessage+ union if you’d like.Also applies to: 43-56
12-16: Post-processing action enum: allow future-safe values.Consider an enum to centralize allowed actions and ease future additions (e.g., move/archive).
Apply this diff:
- postProcessAction: 'read' | 'nothing' + postProcessAction: 'read' | 'nothing' // consider enum PostProcessAction
📜 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 selected for processing (10)
nodes/EmailTriggerNode/EmailTriggerNode.schema.ts(1 hunks)nodes/EmailTriggerNode/EmailTriggerNode.service.ts(1 hunks)nodes/EmailTriggerNode/EmailTriggerNode.test.ts(1 hunks)nodes/EmailTriggerNode/EmailTriggerNode.tsx(1 hunks)nodes/EmailTriggerNode/EmailTriggerNode.types.ts(1 hunks)nodes/EmailTriggerNode/README.md(1 hunks)nodes/EmailTriggerNode/index.ts(1 hunks)nodes/index.ts(3 hunks)package.json(2 hunks)tests/integration/node-registry.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
nodes/EmailTriggerNode/EmailTriggerNode.types.ts (1)
types/workflow.ts (1)
TriggerNodeData(56-59)
tests/integration/node-registry.test.ts (1)
nodes/index.ts (2)
getNodesByType(135-137)NodeType(185-185)
nodes/EmailTriggerNode/EmailTriggerNode.service.ts (1)
nodes/EmailTriggerNode/EmailTriggerNode.types.ts (4)
EmailTriggerConfig(3-28)EmailMessage(43-57)EmailAttachment(64-69)EmailAddress(59-62)
nodes/EmailTriggerNode/EmailTriggerNode.test.ts (2)
nodes/EmailTriggerNode/EmailTriggerNode.service.ts (2)
EmailTriggerService(8-240)connect(12-53)nodes/EmailTriggerNode/EmailTriggerNode.types.ts (1)
EmailTriggerConfig(3-28)
nodes/EmailTriggerNode/EmailTriggerNode.schema.ts (2)
nodes/index.ts (3)
ParameterDefinition(32-46)NodeDefinition(50-72)NodeType(185-185)nodes/EmailTriggerNode/EmailTriggerNode.types.ts (1)
EmailTriggerConfig(3-28)
nodes/EmailTriggerNode/EmailTriggerNode.tsx (3)
nodes/EmailTriggerNode/EmailTriggerNode.types.ts (1)
EmailTriggerData(30-33)types/workflow.ts (1)
WorkflowNodeData(71-71)components/workflow/nodes/base-node.tsx (1)
BaseNode(22-146)
nodes/index.ts (1)
nodes/EmailTriggerNode/EmailTriggerNode.schema.ts (1)
EMAIL_TRIGGER_NODE_DEFINITION(29-256)
🪛 LanguageTool
nodes/EmailTriggerNode/README.md
[grammar] ~23-~23: There might be a mistake here.
Context: ...ype | Required | Default | Description | |-----------|------|----------|---------...
(QB_NEW_EN)
[grammar] ~24-~24: There might be a mistake here.
Context: ...----|----------|---------|-------------| | host | string | Yes | - | IMAP serve...
(QB_NEW_EN)
[grammar] ~25-~25: There might be a mistake here.
Context: ...- | IMAP server hostname or IP address | | port | number | Yes | - | IMAP serve...
(QB_NEW_EN)
[grammar] ~26-~26: There might be a mistake here.
Context: ...(usually 993 for SSL, 143 for non-SSL) | | secure | boolean | Yes | - | Whether...
(QB_NEW_EN)
[grammar] ~27-~27: There might be a mistake here.
Context: ... - | Whether to use SSL/TLS encryption | | user | string | Yes | - | Email acco...
(QB_NEW_EN)
[grammar] ~28-~28: There might be a mistake here.
Context: ...| Email account username/email address | | password | string | Yes | - | Email ...
(QB_NEW_EN)
[grammar] ~29-~29: There might be a mistake here.
Context: ...ount password or app-specific password | | mailbox | string | No | "INBOX" | Ma...
(QB_NEW_EN)
[grammar] ~188-~188: There might be a mistake here.
Context: ...simple: IMAP client library for Node.js - mailparser`: Email parsing and attachment extractio...
(QB_NEW_EN)
[grammar] ~189-~189: There might be a mistake here.
Context: ... Email parsing and attachment extraction - @types/imap-simple: TypeScript definitions for imap-simple...
(QB_NEW_EN)
[grammar] ~190-~190: There might be a mistake here.
Context: ...: TypeScript definitions for imap-simple - @types/mailparser: TypeScript definitions for mailparser ...
(QB_NEW_EN)
[grammar] ~195-~195: There might be a mistake here.
Context: ...Node**: Send emails as part of workflows - HTTP Node: Make API calls based on ema...
(QB_NEW_EN)
[grammar] ~196-~196: There might be a mistake here.
Context: ...*: Make API calls based on email content - Logic Nodes: Process email data and ma...
(QB_NEW_EN)
[grammar] ~197-~197: There might be a mistake here.
Context: ...*: Process email data and make decisions - Action Nodes: Perform actions based on...
(QB_NEW_EN)
🔇 Additional comments (6)
package.json (1)
56-57: Verify that @types/imap-simple (v4) aligns with imap-simple v5 API
Compare the exported signatures (connect, ImapSimpleOptions, Message) in node_modules/@types/imap-simple against the runtime library used in nodes/EmailTriggerNode.service.ts (lines 1–2) and the test imports in EmailTriggerNode.test.ts. If you detect any mismatches, either pin imap-simple to a known v4 release or provide local ambient type overrides.tests/integration/node-registry.test.ts (1)
19-21: LGTM: node count updated to 11The registry size expectation reflects the new Email Trigger node.
nodes/index.ts (2)
160-160: LGTM: imports Email Trigger definition for registrationImport path and naming align with the feature barrel.
173-173: LGTM: registers Email Trigger definitionRegistration order looks consistent with other nodes.
nodes/EmailTriggerNode/EmailTriggerNode.test.ts (1)
137-151: Awaitend()indisconnect()and update test to catch unhandled rejections
Makedisconnect()anasyncmethod thatawaitsthis.client.end(), handling any errors. Then enhance the existing test to assert no unhandled promise rejections—e.g. spy onqueueMicrotask(or registerprocess.on('unhandledRejection')) and verify it’s never called when runningawait service.disconnect().nodes/EmailTriggerNode/EmailTriggerNode.types.ts (1)
1-76: Overall: solid, idiomatic types; good separation of config/result/message.Nice coverage of the node’s data surface and alignment with TriggerNodeData.
| await this.connection.closeBox(false) | ||
| this.connection.end() | ||
| } catch (error) { | ||
| console.error('Error disconnecting from IMAP:', error) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Await end() to avoid unhandled rejections during disconnect.
If end() returns a Promise (or is mocked as such), not awaiting it can surface unhandled rejections.
- this.connection.end()
+ await this.connection.end()📝 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.
| await this.connection.closeBox(false) | |
| this.connection.end() | |
| } catch (error) { | |
| console.error('Error disconnecting from IMAP:', error) | |
| await this.connection.closeBox(false) | |
| await this.connection.end() | |
| } catch (error) { | |
| console.error('Error disconnecting from IMAP:', error) |
🤖 Prompt for AI Agents
In nodes/EmailTriggerNode/EmailTriggerNode.service.ts around lines 58 to 61, the
code calls this.connection.end() without awaiting it, which can cause unhandled
promise rejections if end() returns a Promise; change the call to await
this.connection.end() and ensure it's inside the existing try/catch (or add a
dedicated try/catch) so any rejection from end() is caught and logged
consistently with the existing error handling.
| // Filter by last message ID if tracking is enabled | ||
| if (config.trackLastMessageId !== false && lastMessageId) { | ||
| searchCriteria.push(['UID', `${lastMessageId}:*`]) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
UID range should start after the last processed UID to prevent reprocessing.
lastMessageId:* can include the last processed message again (especially if you don’t mark as read). Increment to lastMessageId + 1.
- if (config.trackLastMessageId !== false && lastMessageId) {
- searchCriteria.push(['UID', `${lastMessageId}:*`])
+ if (config.trackLastMessageId !== false && lastMessageId) {
+ searchCriteria.push(['UID', `${lastMessageId + 1}:*`])
}📝 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.
| // Filter by last message ID if tracking is enabled | |
| if (config.trackLastMessageId !== false && lastMessageId) { | |
| searchCriteria.push(['UID', `${lastMessageId}:*`]) | |
| } | |
| // Filter by last message ID if tracking is enabled | |
| if (config.trackLastMessageId !== false && lastMessageId) { | |
| searchCriteria.push(['UID', `${lastMessageId + 1}:*`]) | |
| } |
🤖 Prompt for AI Agents
In nodes/EmailTriggerNode/EmailTriggerNode.service.ts around lines 85 to 88, the
UID range filter currently uses `${lastMessageId}:*` which can include the last
processed message; change the range to start at lastMessageId + 1 (e.g.,
`${lastMessageId + 1}:*`) so the search excludes the already-processed UID and
prevents reprocessing; ensure you convert the incremented value to a string and
handle numeric/string types safely when building the UID range.
| const messages = await this.connection.search(searchCriteria, { | ||
| bodies: ['HEADER', 'TEXT'], | ||
| markSeen: config.postProcessAction === 'read', | ||
| }) | ||
|
|
||
| const emails: EmailMessage[] = [] | ||
|
|
||
| for (const message of messages) { |
There was a problem hiding this comment.
Body fetch doesn’t match parsing; results in “No message body found”.
You search with bodies: ['HEADER','TEXT'] but later look for part.which === ''. That will always fail. Fetch the full body with [''] (or align parser to use the fetched parts).
- const messages = await this.connection.search(searchCriteria, {
- bodies: ['HEADER', 'TEXT'],
+ const messages = await this.connection.search(searchCriteria, {
+ bodies: [''],
markSeen: config.postProcessAction === 'read',
})📝 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 messages = await this.connection.search(searchCriteria, { | |
| bodies: ['HEADER', 'TEXT'], | |
| markSeen: config.postProcessAction === 'read', | |
| }) | |
| const emails: EmailMessage[] = [] | |
| for (const message of messages) { | |
| const messages = await this.connection.search(searchCriteria, { | |
| bodies: [''], | |
| markSeen: config.postProcessAction === 'read', | |
| }) |
🤖 Prompt for AI Agents
In nodes/EmailTriggerNode/EmailTriggerNode.service.ts around lines 90 to 97, the
IMAP fetch requests use bodies: ['HEADER','TEXT'] but later parsing expects
parts with part.which === '' (full body), so the body lookup always fails;
change the fetch to request the full body (bodies: ['']) or alternatively update
the parser to inspect the same part identifiers you fetch (e.g., check for
part.which === 'TEXT' or 'HEADER'), and keep markSeen behavior unchanged.
| const attachments: EmailAttachment[] = [] | ||
| if (config.downloadAttachments && parsed.attachments) { | ||
| for (const attachment of parsed.attachments) { | ||
| if (attachment.type === 'attachment') { | ||
| attachments.push({ | ||
| filename: attachment.filename || 'attachment', | ||
| contentType: attachment.contentType || 'application/octet-stream', | ||
| size: attachment.size || 0, | ||
| content: attachment.content as Buffer, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Attachment filter uses a non-existent property; attachments won’t be captured.
mailparser attachments expose contentDisposition ('attachment' | 'inline'), not type. Filter on contentDisposition === 'attachment'.
- if (config.downloadAttachments && parsed.attachments) {
+ if (config.downloadAttachments && parsed.attachments) {
for (const attachment of parsed.attachments) {
- if (attachment.type === 'attachment') {
+ if ((attachment as any).contentDisposition === 'attachment' || !(attachment as any).contentDisposition) {
attachments.push({
filename: attachment.filename || 'attachment',
contentType: attachment.contentType || 'application/octet-stream',
size: attachment.size || 0,
content: attachment.content as Buffer,
})
}
}
}📝 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 attachments: EmailAttachment[] = [] | |
| if (config.downloadAttachments && parsed.attachments) { | |
| for (const attachment of parsed.attachments) { | |
| if (attachment.type === 'attachment') { | |
| attachments.push({ | |
| filename: attachment.filename || 'attachment', | |
| contentType: attachment.contentType || 'application/octet-stream', | |
| size: attachment.size || 0, | |
| content: attachment.content as Buffer, | |
| }) | |
| } | |
| const attachments: EmailAttachment[] = [] | |
| if (config.downloadAttachments && parsed.attachments) { | |
| for (const attachment of parsed.attachments) { | |
| if ((attachment as any).contentDisposition === 'attachment' || !(attachment as any).contentDisposition) { | |
| attachments.push({ | |
| filename: attachment.filename || 'attachment', | |
| contentType: attachment.contentType || 'application/octet-stream', | |
| size: attachment.size || 0, | |
| content: attachment.content as Buffer, | |
| }) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In nodes/EmailTriggerNode/EmailTriggerNode.service.ts around lines 142 to 152,
the attachment filter incorrectly checks attachment.type which doesn't exist on
mailparser attachments; change the filter to check attachment.contentDisposition
=== 'attachment' (or treat undefined as not an attachment) so actual attachments
are captured, leaving the rest of the attachment mapping intact (filename,
contentType, size, content).
| expect(mockConnection.search).toHaveBeenCalledWith( | ||
| ['UNSEEN', 'UNSEEN', ['SINCE', '1-Jan-2024']], | ||
| { | ||
| bodies: ['HEADER', 'TEXT'], | ||
| markSeen: true, | ||
| } | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Test currently asserts duplicated UNSEEN; prefer deduped criteria and full-body fetch.
Service should not push a second UNSEEN, and should fetch '' body to satisfy parsing. Update expectations accordingly.
- expect(mockConnection.search).toHaveBeenCalledWith(
- ['UNSEEN', 'UNSEEN', ['SINCE', '1-Jan-2024']],
- {
- bodies: ['HEADER', 'TEXT'],
- markSeen: true,
- }
- )
+ expect(mockConnection.search).toHaveBeenCalledWith(
+ ['UNSEEN', ['SINCE', '1-Jan-2024']],
+ {
+ bodies: [''],
+ markSeen: true,
+ }
+ )📝 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.
| expect(mockConnection.search).toHaveBeenCalledWith( | |
| ['UNSEEN', 'UNSEEN', ['SINCE', '1-Jan-2024']], | |
| { | |
| bodies: ['HEADER', 'TEXT'], | |
| markSeen: true, | |
| } | |
| ) | |
| expect(mockConnection.search).toHaveBeenCalledWith( | |
| ['UNSEEN', ['SINCE', '1-Jan-2024']], | |
| { | |
| bodies: [''], | |
| markSeen: true, | |
| } | |
| ) |
🤖 Prompt for AI Agents
In nodes/EmailTriggerNode/EmailTriggerNode.test.ts around lines 178 to 184, the
test incorrectly expects duplicated 'UNSEEN' and requests 'TEXT' body; update
the expectation to dedupe the search criteria and request the full message body
by asserting mockConnection.search was called with ['UNSEEN', ['SINCE',
'1-Jan-2024']] (no duplicate UNSEEN) and options { bodies: ['HEADER', ''],
markSeen: true } so the service only sends one UNSEEN and fetches the full body
for parsing.
| | Parameter | Type | Required | Default | Description | | ||
| |-----------|------|----------|---------|-------------| | ||
| | `enabled` | boolean | No | true | Whether the trigger is active | | ||
| | `postProcessAction` | string | No | "nothing" | Action after processing: "read" or "nothing" | | ||
| | `downloadAttachments` | boolean | No | false | Whether to download email attachments | | ||
| | `format` | string | No | "simple" | Output format: "simple", "resolved", or "raw" | | ||
| | `attachmentPrefix` | string | No | "attachment_" | Prefix for attachment filenames | | ||
| | `trackLastMessageId` | boolean | No | true | Whether to track the last processed message ID | | ||
| | `allowUnauthorizedCerts` | boolean | No | false | Allow self-signed or invalid SSL certificates | | ||
| | `reconnectIntervalMinutes` | number | No | 5 | Minutes between reconnection attempts | | ||
| | `customEmailRules` | string | No | - | JSON string of custom IMAP search criteria | | ||
|
|
There was a problem hiding this comment.
Docs/schema mismatches: defaults and option names diverge
The README lists defaults/option names that don’t match the schema:
- postProcessAction default "nothing" vs schema default "read"
- reconnectIntervalMinutes vs schema parameter name forceReconnect (no default in getDefaults)
- secure marked Required: Yes with no default, but schema has required: false with defaultValue: true
Please align docs to the code.
- | `secure` | boolean | Yes | - | Whether to use SSL/TLS encryption |
+ | `secure` | boolean | No | true | Whether to use SSL/TLS encryption |
...
- | `postProcessAction` | string | No | "nothing" | Action after processing: "read" or "nothing" |
+ | `postProcessAction` | string | No | "read" | Action after processing: "read" or "nothing" |
...
- | `reconnectIntervalMinutes` | number | No | 5 | Minutes between reconnection attempts |
+ | `forceReconnect` | number | No | 0 | Force reconnection every N minutes (0 to disable)If you prefer the reconnectIntervalMinutes name and default=5, update the schema instead:
- Rename parameter name to reconnectIntervalMinutes
- Add defaultValue: 5 in getDefaults()
- Adjust validation message accordingly
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In nodes/EmailTriggerNode/README.md around lines 34 to 45, the README defaults
and option names must match the schema: change the postProcessAction default
from "nothing" to "read"; replace the reconnectIntervalMinutes row with the
schema parameter name forceReconnect (no default value) and adjust its Type to
match the schema; update the secure row to Required: No and Default: true to
reflect schema defaultValue; ensure any validation text or descriptions
reference the schema names and defaults exactly so docs and code are aligned.
|
|
||
| // Import all nodes | ||
| export * from './EmailNode' | ||
| export * from './EmailTriggerNode' |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Do not re-export server-only EmailTrigger service via the central barrel
Re-exporting the EmailTrigger barrel from the root nodes index can cause client bundles to import Node-only deps (imap-simple/mailparser). Curate the exports to UI/schema only here; keep the service export confined to the feature barrel for server-side imports.
-export * from './EmailTriggerNode'
+// Avoid leaking server-only service into client bundles from the root barrel.
+export { EmailTriggerNode } from './EmailTriggerNode'
+export { EMAIL_TRIGGER_NODE_DEFINITION } from './EmailTriggerNode'
+// If you need types, re-export them explicitly:
+// export type { EmailTriggerConfig, EmailMessage, EmailAttachment } from './EmailTriggerNode'📝 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 * from './EmailTriggerNode' | |
| // nodes/index.ts | |
| // Avoid leaking server-only service into client bundles from the root barrel. | |
| // Export only the definitions needed by the UI/schema. | |
| export { EmailTriggerNode } from './EmailTriggerNode' | |
| export { EMAIL_TRIGGER_NODE_DEFINITION } from './EmailTriggerNode' | |
| // If you need types in the client, re-export them explicitly: | |
| // export type { EmailTriggerConfig, EmailMessage, EmailAttachment } from './EmailTriggerNode' |
🤖 Prompt for AI Agents
In nodes/index.ts around line 8, the file currently re-exports the server-only
EmailTrigger service which pulls in imap-simple/mailparser into client bundles;
remove the blanket "export * from './EmailTriggerNode'" line and instead export
only the UI/schema exports required by the client (e.g. the node's UI component
and schema types) from this barrel, and keep the server-only service
implementation export confined to the feature's server-side barrel (move or
re-add the full EmailTriggerNode export in the server/feature barrel).
| "imap-simple": "^5.1.0", | ||
| "lucide-react": "^0.370.0", | ||
| "mailparser": "^3.7.4", |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure imap-simple/mailparser remain server-only to avoid Next client bundling errors
Because nodes/index.ts re-exports the EmailTrigger barrel, client bundles that import from '@/nodes' may try to resolve Node built-ins (net, tls, fs) via imap-simple/mailparser and fail. I’ve proposed a registry export change in nodes/index.ts to prevent leaking server-only modules into client code. Please apply that to be safe.
Run to confirm no client component imports '@/nodes' in a way that drags server-only code:
🏁 Script executed:
#!/bin/bash
# Find client components importing from '@/nodes' or the trigger barrel
rg -nP -C2 '^[\'"]use client[\'"]' --type=ts --type=tsx | awk -F: '{print $1}' | sort -u | while read f; do
echo "---- $f"; rg -nP "from ['\"]@/nodes(?:/EmailTriggerNode)?['\"]" "$f" -C2 || true
doneLength of output: 271
Use a simpler grep-based check to avoid shell‐regex quoting issues:
🏁 Script executed:
#!/bin/bash
# Find TS/TSX files with a "use client" directive and check for imports from @/nodes
grep -Rnl --include='*.ts' --include='*.tsx' "^[[:space:]]*['\"]use client['\"]" . | sort -u | while read -r f; do
echo "---- $f"
grep -nE "from [\"']@/nodes" "$f" || true
doneLength of output: 8190
🏁 Script executed:
#!/bin/bash
# List directory tree under nodes/ and inspect nodes/index.ts
if [ -d nodes ]; then
echo "Directory tree under nodes/:"
ls -R nodes
else
echo "No 'nodes' directory found."
fi
echo
# Inspect the contents of nodes/index.ts if it exists
if [ -f nodes/index.ts ]; then
echo "Contents of nodes/index.ts:"
sed -n '1,200p' nodes/index.ts
else
echo "nodes/index.ts not found."
fiLength of output: 8449
Scope server-only exports to avoid client bundling errors
In nodes/index.ts, remove the line
export * from './EmailTriggerNode'and relocate the EmailTriggerNode export (and its imap-simple/mailparser imports) into a separate server-only entrypoint so client bundles importing from '@/nodes' won’t pull in Node-built-in dependencies.
🤖 Prompt for AI Agents
In package.json around lines 39 to 41 (and in nodes/index.ts), avoid exporting
EmailTriggerNode from the shared nodes barrel so client bundles don't pull in
Node-only deps: remove the line `export * from './EmailTriggerNode'` from
nodes/index.ts and create a new server-only entrypoint (e.g.,
nodes/server-only.ts or nodes/email-node.server.ts) that imports imap-simple and
mailparser and re-exports EmailTriggerNode; update any server-side imports to
import from that new server-only path and do not add the server-only file to the
client bundle (use explicit import paths or conditional exports in
package.json/tsconfig paths to ensure the client barrel '@/nodes' remains free
of Node built-ins).
| expect(triggerNodes).toHaveLength(4) // MANUAL, SCHEDULE, WEBHOOK, EMAIL | ||
| expect(actionNodes).toHaveLength(5) // EMAIL, HTTP, DATABASE, TRANSFORM, DELAY | ||
| expect(logicNodes).toHaveLength(2) // IF, FILTER | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Also assert EMAIL trigger is present
You updated the trigger count to 4 but didn’t assert EMAIL’s presence below. Add explicit expectations for robustness.
- expect(triggerNodes).toHaveLength(4) // MANUAL, SCHEDULE, WEBHOOK, EMAIL
+ expect(triggerNodes).toHaveLength(4) // MANUAL, SCHEDULE, WEBHOOK, EMAIL…and in the trigger test below (Lines 32–41):
it('should have all trigger nodes registered', () => {
expect(isNodeRegistered(NodeType.TRIGGER, TriggerType.MANUAL)).toBe(true)
expect(isNodeRegistered(NodeType.TRIGGER, TriggerType.SCHEDULE)).toBe(true)
expect(isNodeRegistered(NodeType.TRIGGER, TriggerType.WEBHOOK)).toBe(true)
+ expect(isNodeRegistered(NodeType.TRIGGER, TriggerType.EMAIL)).toBe(true)
expect(getNodeDefinition(NodeType.TRIGGER, TriggerType.MANUAL)).toBeDefined()
expect(getNodeDefinition(NodeType.TRIGGER, TriggerType.SCHEDULE)).toBeDefined()
expect(getNodeDefinition(NodeType.TRIGGER, TriggerType.WEBHOOK)).toBeDefined()
+ expect(getNodeDefinition(NodeType.TRIGGER, TriggerType.EMAIL)).toBeDefined()
})📝 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.
| expect(triggerNodes).toHaveLength(4) // MANUAL, SCHEDULE, WEBHOOK, EMAIL | |
| expect(actionNodes).toHaveLength(5) // EMAIL, HTTP, DATABASE, TRANSFORM, DELAY | |
| expect(logicNodes).toHaveLength(2) // IF, FILTER | |
| }) | |
| expect(triggerNodes).toHaveLength(4) // MANUAL, SCHEDULE, WEBHOOK, EMAIL | |
| expect(actionNodes).toHaveLength(5) // EMAIL, HTTP, DATABASE, TRANSFORM, DELAY | |
| expect(logicNodes).toHaveLength(2) // IF, FILTER | |
| }) | |
| it('should have all trigger nodes registered', () => { | |
| expect(isNodeRegistered(NodeType.TRIGGER, TriggerType.MANUAL)).toBe(true) | |
| expect(isNodeRegistered(NodeType.TRIGGER, TriggerType.SCHEDULE)).toBe(true) | |
| expect(isNodeRegistered(NodeType.TRIGGER, TriggerType.WEBHOOK)).toBe(true) | |
| expect(isNodeRegistered(NodeType.TRIGGER, TriggerType.EMAIL)).toBe(true) | |
| expect(getNodeDefinition(NodeType.TRIGGER, TriggerType.MANUAL)).toBeDefined() | |
| expect(getNodeDefinition(NodeType.TRIGGER, TriggerType.SCHEDULE)).toBeDefined() | |
| expect(getNodeDefinition(NodeType.TRIGGER, TriggerType.WEBHOOK)).toBeDefined() | |
| expect(getNodeDefinition(NodeType.TRIGGER, TriggerType.EMAIL)).toBeDefined() | |
| }) |
🤖 Prompt for AI Agents
In tests/integration/node-registry.test.ts around lines 27–30 (and the related
trigger test at lines 32–41), you updated the trigger count to 4 but did not
assert the EMAIL trigger is present; modify the assertions to explicitly check
that triggerNodes includes an EMAIL trigger (e.g., find a node with type or id
'EMAIL' and expect it toBeDefined/true) and add a matching expectation in the
trigger-specific test block so the EMAIL trigger’s presence is asserted directly
for robustness.
- Move EmailTriggerService to server-side only location - Update Next.js webpack config to exclude Node.js modules - Add ESLint configuration for .mjs files - Fix server-side import handling with error catching - Ensure EmailTriggerNode is properly server-side only - Update all tests and integrations to work with new structure
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
nodes/index.ts (1)
8-8: Do not re-export server-only EmailTrigger barrel from the public indexPrevents accidental client import of server-only deps (imap-simple/mailparser). Export only UI/schema/types.
-export * from './EmailTriggerNode' +// Avoid leaking server-only service into client bundles from the root barrel. +export { EMAIL_TRIGGER_NODE_DEFINITION } from './EmailTriggerNode/EmailTriggerNode.schema' +export type { + EmailTriggerConfig, + EmailMessage, + EmailAttachment, +} from './EmailTriggerNode/EmailTriggerNode.types'
🧹 Nitpick comments (13)
.eslintrc.json (1)
39-53: Broaden the .mjs override and set Node env to match typical config filesMost .mjs/.cjs files (e.g., next.config.mjs, tailwind.config.cjs) run in Node. Include .cjs and enable the Node environment to avoid false positives (globals, module APIs). Optionally, you could later dedupe by merging the JS and MJS overrides, but this keeps scope minimal.
Apply this diff within this override:
- "files": ["*.mjs"], + "files": ["*.mjs", "*.cjs"], "parser": "espree", "parserOptions": { "ecmaVersion": "latest", "sourceType": "module" }, + "env": { + "node": true + }, "rules": {Then run eslint against representative config files to confirm no new errors (e.g., next.config.mjs, tailwind.config.cjs).
next.config.mjs (1)
16-29: Consider aliasing server-only libs instead of blanket core fallbacksIf the intent is to keep imap-simple/mailparser out of client bundles, alias those packages to false on the client to reduce blast radius; other libs might rely on crypto/buffer in the browser.
- config.resolve.fallback = { + config.resolve.alias = { + ...(config.resolve.alias ?? {}), + 'imap-simple': false, + mailparser: false, + } + config.resolve.fallback = { ...(config.resolve.fallback ?? {}), net: false, tls: false, fs: false, - crypto: false, - stream: false, - buffer: false, - util: false, - url: false, - path: false, - os: false, - child_process: false, + // Keep only those you know you need to block + child_process: false, }server/services/email-trigger.service.ts (3)
1-4: Deduplicate imports from imap-simpleMinor cleanup.
-import { ImapSimple, ImapSimpleOptions, Message } from 'imap-simple' -import { connect } from 'imap-simple' -import { simpleParser, ParsedMail } from 'mailparser' +import { ImapSimple, ImapSimpleOptions, Message, connect } from 'imap-simple' +import { simpleParser, ParsedMail } from 'mailparser'
220-231: Avoid O(n) mailbox scans to get last UIDFor large mailboxes, searching ALL to compute max UID is expensive. Prefer STATUS to read UIDNEXT (then last UID = UIDNEXT - 1) or a targeted UID search.
I can propose a STATUS-based implementation tailored to imap-simple/node-imap if desired.
55-66: Disconnect path: ensure end() completes before clearing stateRare, but calling end() is async-evented; consider awaiting a promise around 'end' or listening for 'end/close' to avoid races if reconnecting immediately.
nodes/index.ts (1)
175-177: Order-independent, but consider gated registration in client buildsIf this module is used on the client, guard registration of server-only nodes to avoid any side-effects.
-registerNode(EMAIL_TRIGGER_NODE_DEFINITION) +// Only register server-only nodes on the server +if (typeof window === 'undefined') { + registerNode(EMAIL_TRIGGER_NODE_DEFINITION) +}nodes/EmailTriggerNode/EmailTriggerNode.schema.ts (7)
3-3: Drop unused import.
ParameterDefinitionisn't referenced here.-import type { NodeDefinition, ParameterDefinition } from '@/nodes' +import type { NodeDefinition } from '@/nodes'
20-20: Strongly type the node definition with its config.This tightens type-checking for defaults/validate.
-export const EMAIL_TRIGGER_NODE_DEFINITION: NodeDefinition = { +export const EMAIL_TRIGGER_NODE_DEFINITION: NodeDefinition<EmailTriggerConfig> = {
159-165: Hide “Allow Unauthorized Certificates” unless TLS is enabled.Prevents confusing/non-applicable config when
secure=false.{ name: 'allowUnauthorizedCerts', label: 'Allow Unauthorized Certificates', type: 'boolean', required: false, defaultValue: false, - description: 'Allow connections to servers with invalid/expired SSL certificates (not recommended for production)' + description: 'Allow connections to servers with invalid/expired SSL certificates (not recommended for production)', + showIf: [{ name: 'secure', equals: true }] },
186-189: Stricter port validation (integer 1–65535).Covers NaN and non-integers.
- if (!typed.port || typeof typed.port !== 'number' || typed.port < 1 || typed.port > 65535) { + if (!Number.isInteger(typed.port) || typed.port < 1 || typed.port > 65535) { errors.push('Valid port number is required (1-65535)') }
216-223: Validate customEmailRules shape (must be a JSON array).Currently only checks JSON validity; enforce array contract.
- if (typed.customEmailRules && typeof typed.customEmailRules === 'string') { - try { - JSON.parse(typed.customEmailRules) - } catch { - errors.push('Custom email rules must be valid JSON') - } - } + if (typed.customEmailRules && typeof typed.customEmailRules === 'string') { + try { + const parsed = JSON.parse(typed.customEmailRules) + if (!Array.isArray(parsed)) { + errors.push('Custom email rules must be a JSON array') + } + } catch { + errors.push('Custom email rules must be valid JSON') + } + }
119-129: Add attachmentPrefix validation and tighten forceReconnect.
- Ensure prefix is a string when downloads are enabled.
- Require integer minutes for forceReconnect.
+ // Validate attachmentPrefix + if (typed.downloadAttachments && typed.attachmentPrefix !== undefined && typeof typed.attachmentPrefix !== 'string') { + errors.push('Attachment prefix must be a string') + } @@ - if (typed.forceReconnect !== undefined) { - if (typeof typed.forceReconnect !== 'number' || typed.forceReconnect < 0) { - errors.push('Force reconnect interval must be a positive number or 0 to disable') - } - } + if (typed.forceReconnect !== undefined) { + if (!Number.isInteger(typed.forceReconnect) || typed.forceReconnect < 0) { + errors.push('Force reconnect interval must be an integer >= 0 (minutes)') + } + }Also applies to: 225-230
94-104: Optionally clarify “Resolved (With Attachments)”.Service currently includes attachments based on
downloadAttachments, notformat. Consider clarifying the label/description or wiringresolvedto imply downloads.
📜 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 (8)
.eslintrc.json(1 hunks)next.config.mjs(1 hunks)nodes/EmailTriggerNode/EmailTriggerNode.schema.ts(1 hunks)nodes/EmailTriggerNode/EmailTriggerNode.test.ts(1 hunks)nodes/EmailTriggerNode/EmailTriggerNode.types.ts(1 hunks)nodes/EmailTriggerNode/index.ts(1 hunks)nodes/index.ts(4 hunks)server/services/email-trigger.service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- nodes/EmailTriggerNode/index.ts
- nodes/EmailTriggerNode/EmailTriggerNode.test.ts
- nodes/EmailTriggerNode/EmailTriggerNode.types.ts
🧰 Additional context used
🧬 Code graph analysis (3)
server/services/email-trigger.service.ts (1)
nodes/EmailTriggerNode/EmailTriggerNode.types.ts (4)
EmailTriggerConfig(6-31)EmailMessage(46-60)EmailAttachment(67-72)EmailAddress(62-65)
nodes/index.ts (1)
nodes/EmailTriggerNode/EmailTriggerNode.schema.ts (1)
EMAIL_TRIGGER_NODE_DEFINITION(20-249)
nodes/EmailTriggerNode/EmailTriggerNode.schema.ts (3)
server/services/email-trigger.service.ts (1)
EmailTriggerService(8-240)nodes/index.ts (2)
NodeDefinition(50-75)NodeType(188-188)nodes/EmailTriggerNode/EmailTriggerNode.types.ts (2)
EMAIL_TRIGGER_NODE_IS_SERVER_ONLY(4-4)EmailTriggerConfig(6-31)
🪛 GitHub Actions: CI
nodes/EmailTriggerNode/EmailTriggerNode.schema.ts
[error] 11-11: Cannot find module '@/server/services/email-trigger.service' required by EmailTriggerNode.schema.ts.
🔇 Additional comments (5)
server/services/email-trigger.service.ts (1)
23-29: Verify imap-simple supports onmail/onupdate in connect optionsEnsure these callbacks are actually wired by the library; otherwise subscribe on the underlying imap instance after connect.
Would you like me to check the latest imap-simple docs and propose the correct event wiring?
nodes/index.ts (1)
72-75: LGTM: serverSideOnly flag is a clean way to gate UI/runtime behaviorThis metadata will help the UI filter nodes for client use.
nodes/EmailTriggerNode/EmailTriggerNode.schema.ts (3)
27-68: IMAP connection parameter set looks good.Clear labels, sensible defaults, and required markers align with expected UX.
1-2: Heads-up: path alias consistency.You import from
@/types/workflowsuccessfully, but the dynamic require for@/server/...failed in CI. Ensure test/build resolvers (ts-jest/moduleNameMapper/tsconfig paths) handle all@/*imports if you reintroduce server imports later.
92-105: showIf uses OR semantics by default—no change needed.Likely an incorrect or invalid review comment.
| if (!isServer) { | ||
| // Exclude Node.js-specific packages from client-side bundling | ||
| config.resolve.fallback = { | ||
| ...config.resolve.fallback, | ||
| net: false, | ||
| tls: false, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent potential crash when resolve.fallback is undefined
Guard against undefined and avoid spreading a non-object in older/edge configs.
if (!isServer) {
// Exclude Node.js-specific packages from client-side bundling
- config.resolve.fallback = {
- ...config.resolve.fallback,
+ config.resolve = config.resolve || {}
+ config.resolve.fallback = {
+ ...(config.resolve.fallback ?? {}),
net: false,
tls: false,
fs: false,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!isServer) { | |
| // Exclude Node.js-specific packages from client-side bundling | |
| config.resolve.fallback = { | |
| ...config.resolve.fallback, | |
| net: false, | |
| tls: false, | |
| if (!isServer) { | |
| // Exclude Node.js-specific packages from client-side bundling | |
| config.resolve = config.resolve || {} | |
| config.resolve.fallback = { | |
| ...(config.resolve.fallback ?? {}), | |
| net: false, | |
| tls: false, | |
| fs: false, | |
| } | |
| } |
🤖 Prompt for AI Agents
In next.config.mjs around lines 14 to 19, the code spreads
config.resolve.fallback without guarding against resolve or fallback being
undefined; ensure you first create or normalize config.resolve and
config.resolve.fallback to objects (e.g., set config.resolve = config.resolve ||
{} and use fallback = { ...(config.resolve.fallback || {}), net: false, tls:
false }) so you never spread a non-object and avoid crashes in older/edge
configs.
| // Import service only on server side to avoid client-side bundling | ||
| let EmailTriggerService: typeof import('@/server/services/email-trigger.service').EmailTriggerService | undefined | ||
| if (typeof window === 'undefined') { | ||
| try { | ||
| // Server-side only | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment | ||
| const serviceModule = require('@/server/services/email-trigger.service') | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-unsafe-assignment | ||
| EmailTriggerService = serviceModule.EmailTriggerService | ||
| } catch (error) { | ||
| // Service not available (e.g., in test environment) | ||
| console.warn('EmailTriggerService not available:', error) | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix CI: remove unused server-only require (breaks on path alias).
The dynamic require is unused in this file and triggers the CI error “Cannot find module '@/server/services/email-trigger.service'.” Remove it to unblock the pipeline. If you truly need it later, prefer a lazy relative import inside an execute function and ensure path alias resolution in test/tsconfig.
-// Import service only on server side to avoid client-side bundling
-let EmailTriggerService: typeof import('@/server/services/email-trigger.service').EmailTriggerService | undefined
-if (typeof window === 'undefined') {
- try {
- // Server-side only
- // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
- const serviceModule = require('@/server/services/email-trigger.service')
- // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-unsafe-assignment
- EmailTriggerService = serviceModule.EmailTriggerService
- } catch (error) {
- // Service not available (e.g., in test environment)
- console.warn('EmailTriggerService not available:', error)
- }
-}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Import service only on server side to avoid client-side bundling | |
| let EmailTriggerService: typeof import('@/server/services/email-trigger.service').EmailTriggerService | undefined | |
| if (typeof window === 'undefined') { | |
| try { | |
| // Server-side only | |
| // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment | |
| const serviceModule = require('@/server/services/email-trigger.service') | |
| // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-unsafe-assignment | |
| EmailTriggerService = serviceModule.EmailTriggerService | |
| } catch (error) { | |
| // Service not available (e.g., in test environment) | |
| console.warn('EmailTriggerService not available:', error) | |
| } | |
| } |
🧰 Tools
🪛 GitHub Actions: CI
[error] 11-11: Cannot find module '@/server/services/email-trigger.service' required by EmailTriggerNode.schema.ts.
🤖 Prompt for AI Agents
In nodes/EmailTriggerNode/EmailTriggerNode.schema.ts around lines 5-18, remove
the unused server-only dynamic require block that attempts to
require('@/server/services/email-trigger.service') (the block initializing
EmailTriggerService and the try/catch) because it breaks CI due to path-alias
resolution; if server-side usage is later required, replace it with a lazy
relative import inside the node's execute runtime (e.g., import or require from
a relative path within the function) and/or ensure tests resolve the alias by
updating test/tsconfig paths so the module can be found.
|
|
||
| // Auto-register nodes | ||
| import { EMAIL_NODE_DEFINITION } from './EmailNode' | ||
| import { EMAIL_TRIGGER_NODE_DEFINITION } from './EmailTriggerNode' |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Import schema directly to avoid pulling feature barrel (and service) into client
Keep the registry import lean.
-import { EMAIL_TRIGGER_NODE_DEFINITION } from './EmailTriggerNode'
+import { EMAIL_TRIGGER_NODE_DEFINITION } from './EmailTriggerNode/EmailTriggerNode.schema'📝 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.
| import { EMAIL_TRIGGER_NODE_DEFINITION } from './EmailTriggerNode' | |
| import { EMAIL_TRIGGER_NODE_DEFINITION } from './EmailTriggerNode/EmailTriggerNode.schema' |
🤖 Prompt for AI Agents
In nodes/index.ts at line 163, the import currently pulls from the
EmailTriggerNode feature barrel which drags services into the client; replace
that barrel import with a direct import of the node schema/definition module
(the specific file that only exports EMAIL_TRIGGER_NODE_DEFINITION) so the
registry only loads the schema. Update the import path to point to the
schema-only file and remove any side-effecting exports from the barrel usage.
| const searchCriteria: (string | string[])[] = ['UNSEEN'] | ||
|
|
||
| // Parse custom email rules if provided | ||
| if (config.customEmailRules) { | ||
| try { | ||
| const customRules = JSON.parse(config.customEmailRules) as (string | string[])[] | ||
| searchCriteria.push(...customRules) | ||
| } catch (error) { | ||
| throw new Error('Invalid custom email rules JSON') | ||
| } | ||
| } |
There was a problem hiding this comment.
Custom rules should replace, not intersect with default UNSEEN
Docs/UX imply custom IMAP criteria fully override defaults. Intersecting with UNSEEN yields unexpected results (e.g., ["ALL"] still filters to unseen).
- const searchCriteria: (string | string[])[] = ['UNSEEN']
-
- // Parse custom email rules if provided
- if (config.customEmailRules) {
- try {
- const customRules = JSON.parse(config.customEmailRules) as (string | string[])[]
- searchCriteria.push(...customRules)
- } catch (error) {
- throw new Error('Invalid custom email rules JSON')
- }
- }
+ const searchCriteria: (string | string[])[] = []
+ if (config.customEmailRules) {
+ try {
+ const customRules = JSON.parse(config.customEmailRules) as (string | string[])[]
+ searchCriteria.push(...customRules)
+ } catch {
+ throw new Error('Invalid custom email rules JSON')
+ }
+ }
+ if (searchCriteria.length === 0) {
+ searchCriteria.push('UNSEEN')
+ }📝 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 searchCriteria: (string | string[])[] = ['UNSEEN'] | |
| // Parse custom email rules if provided | |
| if (config.customEmailRules) { | |
| try { | |
| const customRules = JSON.parse(config.customEmailRules) as (string | string[])[] | |
| searchCriteria.push(...customRules) | |
| } catch (error) { | |
| throw new Error('Invalid custom email rules JSON') | |
| } | |
| } | |
| const searchCriteria: (string | string[])[] = [] | |
| if (config.customEmailRules) { | |
| try { | |
| const customRules = JSON.parse(config.customEmailRules) as (string | string[])[] | |
| searchCriteria.push(...customRules) | |
| } catch { | |
| throw new Error('Invalid custom email rules JSON') | |
| } | |
| } | |
| if (searchCriteria.length === 0) { | |
| searchCriteria.push('UNSEEN') | |
| } |
🤖 Prompt for AI Agents
In server/services/email-trigger.service.ts around lines 73 to 83, the code
currently initializes searchCriteria to ['UNSEEN'] and then pushes parsed
customEmailRules onto it, which intersects with the default; change the logic so
that if config.customEmailRules is provided and parses successfully, assign
searchCriteria to the parsed array (replacing the default) instead of pushing;
keep the try/catch for JSON.parse and throw the same or clearer error on parse
failure.
| if (config.trackLastMessageId !== false && lastMessageId) { | ||
| searchCriteria.push(['UID', `${lastMessageId}:*`]) | ||
| } |
There was a problem hiding this comment.
Off-by-one and truthy check on lastMessageId causes duplicates/skips
Use explicit number check and start from lastMessageId + 1.
- if (config.trackLastMessageId !== false && lastMessageId) {
- searchCriteria.push(['UID', `${lastMessageId}:*`])
- }
+ if (config.trackLastMessageId !== false && typeof lastMessageId === 'number') {
+ const start = Math.max(1, lastMessageId + 1)
+ searchCriteria.push(['UID', `${start}:*`])
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (config.trackLastMessageId !== false && lastMessageId) { | |
| searchCriteria.push(['UID', `${lastMessageId}:*`]) | |
| } | |
| if (config.trackLastMessageId !== false && typeof lastMessageId === 'number') { | |
| const start = Math.max(1, lastMessageId + 1) | |
| searchCriteria.push(['UID', `${start}:*`]) | |
| } |
🤖 Prompt for AI Agents
In server/services/email-trigger.service.ts around lines 86 to 88, the current
truthy check for lastMessageId can cause off-by-one duplicates or skipped
messages; change the check to an explicit numeric validation (e.g., typeof
lastMessageId === 'number' && Number.isFinite(lastMessageId)) and build the UID
searchCriteria to start from lastMessageId + 1 (not lastMessageId) so the query
uses the correct inclusive range and avoids reprocessing the last message;
ensure you also handle cases where lastMessageId is undefined/null by omitting
the UID range.
| const messages = await this.connection.search(searchCriteria, { | ||
| bodies: ['HEADER', 'TEXT'], | ||
| markSeen: config.postProcessAction === 'read', | ||
| }) | ||
|
|
||
| const emails: EmailMessage[] = [] | ||
|
|
||
| for (const message of messages) { |
There was a problem hiding this comment.
Fix fetch: parser expects whole message ('') but search fetches only HEADER/TEXT
parseEmailMessage looks for part.which === '' and will throw “No message body found” with current fetch options.
- const messages = await this.connection.search(searchCriteria, {
- bodies: ['HEADER', 'TEXT'],
- markSeen: config.postProcessAction === 'read',
- })
+ const messages = await this.connection.search(searchCriteria, {
+ bodies: [''],
+ markSeen: config.postProcessAction === 'read',
+ })📝 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 messages = await this.connection.search(searchCriteria, { | |
| bodies: ['HEADER', 'TEXT'], | |
| markSeen: config.postProcessAction === 'read', | |
| }) | |
| const emails: EmailMessage[] = [] | |
| for (const message of messages) { | |
| const messages = await this.connection.search(searchCriteria, { | |
| bodies: [''], | |
| markSeen: config.postProcessAction === 'read', | |
| }) | |
| const emails: EmailMessage[] = [] | |
| for (const message of messages) { | |
| // …rest of processing… | |
| } |
🤖 Prompt for AI Agents
In server/services/email-trigger.service.ts around lines 90 to 97, the code
fetches only HEADER/TEXT but parseEmailMessage expects the full message
(part.which === ''), causing a “No message body found” error; update the search
fetch options to request the entire raw message (e.g., use bodies: [''] or the
equivalent RFC822/full-body fetch) so parseEmailMessage receives the whole
message body, and keep markSeen logic intact.
| const parseAddresses = (addresses: ParsedMail['from'] | ParsedMail['to'] | ParsedMail['cc'] | ParsedMail['bcc']): EmailAddress[] => { | ||
| if (!addresses) return [] | ||
|
|
||
| // Handle different address formats from mailparser | ||
| if (Array.isArray(addresses)) { | ||
| return addresses.map((addr): EmailAddress => ({ | ||
| name: typeof addr === 'object' && addr && 'name' in addr ? String(addr.name || '') : undefined, | ||
| address: typeof addr === 'object' && addr && 'address' in addr ? String(addr.address || '') : '', | ||
| })).filter((addr) => addr.address) | ||
| } | ||
|
|
||
| if (typeof addresses === 'object' && addresses && 'address' in addresses) { | ||
| return [{ | ||
| name: 'name' in addresses ? String(addresses.name || '') : undefined, | ||
| address: String(addresses.address || ''), | ||
| }] | ||
| } | ||
|
|
||
| return [] | ||
| } |
There was a problem hiding this comment.
Address parsing is incorrect for mailparser AddressObject
mailparser provides AddressObject with a .value array; current logic returns empty arrays.
- const parseAddresses = (addresses: ParsedMail['from'] | ParsedMail['to'] | ParsedMail['cc'] | ParsedMail['bcc']): EmailAddress[] => {
- if (!addresses) return []
-
- // Handle different address formats from mailparser
- if (Array.isArray(addresses)) {
- return addresses.map((addr): EmailAddress => ({
- name: typeof addr === 'object' && addr && 'name' in addr ? String(addr.name || '') : undefined,
- address: typeof addr === 'object' && addr && 'address' in addr ? String(addr.address || '') : '',
- })).filter((addr) => addr.address)
- }
-
- if (typeof addresses === 'object' && addresses && 'address' in addresses) {
- return [{
- name: 'name' in addresses ? String(addresses.name || '') : undefined,
- address: String(addresses.address || ''),
- }]
- }
-
- return []
- }
+ const parseAddresses = (
+ addresses: ParsedMail['from'] | ParsedMail['to'] | ParsedMail['cc'] | ParsedMail['bcc']
+ ): EmailAddress[] => {
+ if (!addresses) return []
+ // mailparser: AddressObject => { value: Array<{ name?: string; address: string }> }
+ const list =
+ Array.isArray(addresses)
+ ? addresses
+ : (typeof addresses === 'object' && 'value' in (addresses as any) && Array.isArray((addresses as any).value))
+ ? (addresses as any).value
+ : (typeof addresses === 'object' && 'address' in (addresses as any))
+ ? [addresses as any]
+ : []
+ return list
+ .map((addr: any): EmailAddress => ({ name: addr.name || undefined, address: addr.address || '' }))
+ .filter(a => a.address)
+ }📝 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 parseAddresses = (addresses: ParsedMail['from'] | ParsedMail['to'] | ParsedMail['cc'] | ParsedMail['bcc']): EmailAddress[] => { | |
| if (!addresses) return [] | |
| // Handle different address formats from mailparser | |
| if (Array.isArray(addresses)) { | |
| return addresses.map((addr): EmailAddress => ({ | |
| name: typeof addr === 'object' && addr && 'name' in addr ? String(addr.name || '') : undefined, | |
| address: typeof addr === 'object' && addr && 'address' in addr ? String(addr.address || '') : '', | |
| })).filter((addr) => addr.address) | |
| } | |
| if (typeof addresses === 'object' && addresses && 'address' in addresses) { | |
| return [{ | |
| name: 'name' in addresses ? String(addresses.name || '') : undefined, | |
| address: String(addresses.address || ''), | |
| }] | |
| } | |
| return [] | |
| } | |
| const parseAddresses = ( | |
| addresses: ParsedMail['from'] | ParsedMail['to'] | ParsedMail['cc'] | ParsedMail['bcc'] | |
| ): EmailAddress[] => { | |
| if (!addresses) return [] | |
| // mailparser: AddressObject => { value: Array<{ name?: string; address: string }> } | |
| const list = | |
| Array.isArray(addresses) | |
| ? addresses | |
| : (typeof addresses === 'object' && 'value' in (addresses as any) && Array.isArray((addresses as any).value)) | |
| ? (addresses as any).value | |
| : (typeof addresses === 'object' && 'address' in (addresses as any)) | |
| ? [addresses as any] | |
| : [] | |
| return list | |
| .map((addr: any): EmailAddress => ({ name: addr.name || undefined, address: addr.address || '' })) | |
| .filter(a => a.address) | |
| } |
🤖 Prompt for AI Agents
In server/services/email-trigger.service.ts around lines 157 to 176, the
parseAddresses function fails to handle mailparser's AddressObject (which uses a
.value array) and therefore returns empty arrays; update the function to detect
when addresses is an object containing a .value array and map that array into
EmailAddress entries (extracting name and address with safe null/undefined
checks), retain the existing handling for Array and single address object cases,
and ensure you filter out entries with empty address strings and preserve the
name as undefined when empty.
- Convert @/ alias imports to relative paths for better module resolution - Fix EmailNode test failures by implementing proper email provider mocking - Resolve TypeScript/ESLint errors in email-providers.ts - Install @types/nodemailer for proper type definitions - All tests now passing (280/280)
- Restructure EmailNode.service.ts to use dynamic imports for email providers - Add server-side only checks to prevent nodemailer loading in browser - Maintain test functionality while ensuring build compatibility - All tests passing (15/15) and build successful
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nodes/EmailNode/email-providers.ts (1)
27-33: Attachments in config are ignored by the SMTP path.EmailNode.types.ts exposes attachments: string[], but they aren’t sent. This is a functional gap for users expecting attachments to be delivered.
You can extend mailOptions like:
const mailOptions: MailOptions = { from: from || emailService.auth.user, to: to.join(', '), subject, text: body, attachments: config.attachments?.map(path => ({ path })) ?? [] }If attachments are intentionally deferred, please document that in the node’s README and schema validation.
🧹 Nitpick comments (12)
nodes/EmailNode/email-providers.ts (4)
3-5: Type-safe nodemailer options; drop unused type and remove any/ESLint disables.Import TransportOptions from nodemailer, remove the unused Transporter from local types, and avoid the any-cast on options.
Apply this diff:
-import { Transporter, MailOptions, SendMailResult } from './nodemailer-types' -import type { Transporter as NodemailerTransporter } from 'nodemailer' +import { MailOptions, SendMailResult } from './nodemailer-types' +import type { Transporter as NodemailerTransporter, TransportOptions } from 'nodemailer' @@ - // eslint-disable-next-line @typescript-eslint/no-explicit-any - } as any) + })Also applies to: 24-25
117-128: ESM-friendly dynamic import for nodemailer + clearer error; type the transporter factory.Using require can break in ESM builds and obscures the failure mode when nodemailer isn’t installed. Prefer dynamic import with a helpful error and proper typing.
Apply this diff:
-// eslint-disable-next-line @typescript-eslint/no-require-imports -let nodemailer: typeof import('nodemailer') | null = null +let nodemailerMod: typeof import('nodemailer') | null = null @@ -// eslint-disable-next-line @typescript-eslint/no-explicit-any -async function getNodemailerTransporter(options: any): Promise<NodemailerTransporter> { - if (!nodemailer) { - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment - nodemailer = require('nodemailer') as typeof import('nodemailer') - } - // eslint-disable-next-line @typescript-eslint/no-unsafe-argument - return nodemailer.createTransport(options) +async function getNodemailerTransporter(options: TransportOptions): Promise<NodemailerTransporter> { + try { + if (!nodemailerMod) { + // Support both CJS and ESM builds of nodemailer + const mod = await import('nodemailer') + // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment + nodemailerMod = (mod as any).default ?? (mod as any) + } + return nodemailerMod.createTransport(options) + } catch (e) { + throw new Error('Nodemailer is required to send emails. Install "nodemailer" (and types).', { cause: e as Error }) + } }
36-37: Remove unnecessary ESLint disable; keep explicit cast.The cast is already explicit; the disable can be dropped.
Apply this diff:
- // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment - const info: SendMailResult = await transporter.sendMail(mailOptions) as SendMailResult + const info = (await transporter.sendMail(mailOptions)) as SendMailResult
13-25: Stale comment about “graceful fallback.”The header says there’s a fallback when packages aren’t installed, but sendWithNodemailer now hard-requires nodemailer. Update the comment or wire simulateEmailSending as a fallback.
nodes/EmailNode/EmailNode.test.ts (1)
27-28: Remove duplicate comment.This repeats the intent stated above.
Apply this diff:
-// Mock the email provider functions to avoid actual email sending in tests -nodes/EmailNode/nodemailer-types.ts (1)
48-52: Avoid self-recursivedefaulttyping; use a non-recursive interop shape.
default?: NodemailerModulemakes the type recursively reference itself and can confuse tooling. Model ESM/CJS interop explicitly.Apply:
export interface NodemailerModule { createTransporter: (options: TransportOptions) => Transporter createTransport?: (options: TransportOptions) => Transporter - default?: NodemailerModule + default?: { + createTransport?: (options: TransportOptions) => Transporter + createTransporter?: (options: TransportOptions) => Transporter + } }nodes/EmailTriggerNode/EmailTriggerNode.schema.ts (6)
1-4: Switch to relative type imports; avoid@alias here.Stays consistent with other PR changes and fixes test/SSR path issues.
-import { NodeType, TriggerType } from '@/types/workflow' +import { NodeType, TriggerType } from '../../types/workflow' -import type { NodeDefinition, ParameterDefinition } from '@/nodes' +import type { NodeDefinition, ParameterDefinition } from '../index'
5-17: Remove unused EmailTriggerService type scaffold from schema file.Types and constructors for the service don’t belong in the schema module and are unused. Keep schema/UI purely declarative.
-// Import service only on server side to avoid client-side bundling -interface EmailTriggerServiceType { - new (): { - connect(config: EmailTriggerConfig): Promise<void> - disconnect(): Promise<void> - fetchEmails(config: EmailTriggerConfig, lastMessageId?: number): Promise<unknown[]> - markAsRead(messageId: number): Promise<void> - getLastMessageId(): Promise<number | undefined> - isConnectedToServer(): boolean - getConnection(): unknown - } -} - -let EmailTriggerService: EmailTriggerServiceType | undefined
20-36: Drop dynamic require from schema; load the service where execution happens.Even with guards and relative paths, pulling server code into a schema file couples client/UI bundles to server modules. Do the dynamic import inside the server executor or a server-only
executeNode.-// Try to load EmailTriggerService dynamically, but don't fail if it's not available -try { - if (typeof window === 'undefined') { - // Server-side only - use relative path for better test compatibility - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment - const serviceModule = require('../../server/services/email-trigger.service') - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-unsafe-assignment - EmailTriggerService = serviceModule.EmailTriggerService - } -} catch (error) { - // Service not available (e.g., in test environment or when dependencies are missing) - // This is expected and shouldn't cause test failures - if (process.env.NODE_ENV !== 'test') { - console.warn('EmailTriggerService not available:', error) - } - EmailTriggerService = undefined -} +// Note: service is intentionally not imported here; this module remains UI/schema-only.
71-78: Avoid constraining IMAP username to an email format.Many IMAP servers use non-email usernames. Use
textto prevent false negatives.- type: 'email', + type: 'text',
131-135: ConfirmshowIfsemantics (OR vs AND).Two entries for
format === 'simple'andformat === 'resolved'imply OR. If your UI uses AND, the control will never show.If OR isn’t supported, consider a single negative condition or split into two controls behind separate conditions.
234-241: ValidatecustomEmailRulesshape, not just JSON parse.Ensure it’s an array of strings/tuples accepted by imap-simple; reject objects/other types.
- try { - JSON.parse(typed.customEmailRules) - } catch { + try { + const v = JSON.parse(typed.customEmailRules) + if (!Array.isArray(v)) { + errors.push('Custom email rules must be a JSON array') + } + } catch { errors.push('Custom email rules must be valid JSON') - } + }
📜 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 (8)
lib/node-definitions.ts(1 hunks)nodes/EmailNode/EmailNode.test.ts(1 hunks)nodes/EmailNode/email-providers.ts(3 hunks)nodes/EmailNode/nodemailer-types.ts(1 hunks)nodes/EmailTriggerNode/EmailTriggerNode.schema.ts(1 hunks)nodes/EmailTriggerNode/EmailTriggerNode.test.ts(1 hunks)package.json(2 hunks)server/services/workflow-executor.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- nodes/EmailTriggerNode/EmailTriggerNode.test.ts
🧰 Additional context used
🧬 Code graph analysis (3)
nodes/EmailNode/EmailNode.test.ts (1)
nodes/EmailNode/EmailNode.types.ts (1)
EmailNodeConfig(3-24)
nodes/EmailTriggerNode/EmailTriggerNode.schema.ts (3)
nodes/EmailTriggerNode/EmailTriggerNode.types.ts (2)
EmailTriggerConfig(6-31)EMAIL_TRIGGER_NODE_IS_SERVER_ONLY(4-4)server/services/email-trigger.service.ts (1)
EmailTriggerService(8-240)nodes/index.ts (2)
NodeDefinition(50-75)NodeType(188-188)
nodes/EmailNode/email-providers.ts (2)
nodes/EmailNode/EmailNode.types.ts (2)
EmailNodeConfig(3-24)EmailExecutionResult(31-39)nodes/EmailNode/nodemailer-types.ts (1)
SendMailResult(19-29)
🔇 Additional comments (4)
nodes/EmailNode/EmailNode.test.ts (1)
7-25: Ensure mocks are hoisted before module import to avoid real code paths.Depending on Vitest config, vi.mock is hoisted, but being explicit avoids flakiness. Move the mock above imports that transitively load ./email-providers, or dynamically import the SUT after the mock.
Minimal pattern:
vi.mock('./email-providers', /* same factory */) const { executeEmailNode } = await import('./EmailNode.service')lib/node-definitions.ts (1)
7-10: Good move to relative imports; reduces alias brittleness in tests/SSR.No behavioral change; this should improve reliability in environments without path aliases.
server/services/workflow-executor.ts (1)
14-19: Revert to path-alias imports
The project’s tsconfig.json defines the"@/*"alias and both runtime and tests rely on it, so mixing in relative imports will break module resolution. Revert these inserver/services/workflow-executor.ts:-import { executeEmailNode } from '../../nodes/EmailNode' +import { executeEmailNode } from '@/nodes/EmailNode' -import { executeHttpNode } from '../../nodes/HttpNode' +import { executeHttpNode } from '@/nodes/HttpNode' -import { executeManualNode } from '../../nodes/ManualNode' +import { executeManualNode } from '@/nodes/ManualNode' -import { executeIfNode } from '../../nodes/IfNode' +import { executeIfNode } from '@/nodes/IfNode' -import { executeFilterNode } from '../../nodes/FilterNode' +import { executeFilterNode } from '@/nodes/FilterNode' -import { executeDatabaseNode } from '../../nodes/DatabaseNode' +import { executeDatabaseNode } from '@/nodes/DatabaseNode' -import { executeTransformNode } from '../../nodes/TransformNode' +import { executeTransformNode } from '@/nodes/TransformNode' -import { executeDelayNode } from '../../nodes/DelayNode' +import { executeDelayNode } from '@/nodes/DelayNode'Likely an incorrect or invalid review comment.
nodes/EmailTriggerNode/EmailTriggerNode.schema.ts (1)
38-45: Server-only flag: LGTM.
serverSideOnlywired to the constant is correct and matches the node’s intent.
| name: 'password', | ||
| label: 'Password', | ||
| type: 'password', | ||
| required: true, | ||
| description: 'Password or app password for IMAP authentication' | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Do not collect raw secrets in node config; prefer credentials store.
Storing passwords in node config risks leakage in logs/exports. Use a credential field tied to a CredentialType (e.g., IMAP) and fetch secrets server-side.
If supported, change parameters (and import CredentialType) like:
+import type { CredentialType } from '../../types/credentials'
...
-{
- name: 'user',
- label: 'Username',
- type: 'email',
- required: true,
- description: 'Email address for IMAP authentication',
- placeholder: 'your-email@example.com'
-},
-{
- name: 'password',
- label: 'Password',
- type: 'password',
- required: true,
- description: 'Password or app password for IMAP authentication'
-},
+{
+ name: 'credentials',
+ label: 'IMAP Credentials',
+ type: 'credential',
+ required: true,
+ credentialType: 'IMAP' as unknown as CredentialType, // adjust to actual enum/string
+ description: 'Stored credentials used server-side for IMAP authentication'
+},Check available credential types:
🏁 Script executed:
#!/bin/bash
rg -nP -C2 'enum\s+CredentialType|export\s+type\s+CredentialType|CredentialType\.' typesLength of output: 322
🏁 Script executed:
#!/bin/bash
# Search for the definition of CREDENTIAL_TYPES
rg -nP -n "CREDENTIAL_TYPES" -n .Length of output: 346
Replace raw secrets with credentials store
In nodes/EmailTriggerNode/EmailTriggerNode.schema.ts (around lines 80–85), remove the standalone password field and instead add a single credentials entry of type credential. Import type { CredentialType } from '../../types/credentials' and set credentialType: 'email' as CredentialType (or extend CREDENTIAL_TYPES in types/credentials.ts to include 'imap' if you need a distinct IMAP credential). This change ensures passwords are fetched server-side and not exposed in node configs.
🤖 Prompt for AI Agents
In nodes/EmailTriggerNode/EmailTriggerNode.schema.ts around lines 80 to 85,
replace the raw password field with a credentials entry: remove the standalone
password property and add a field with type 'credential' using credentialType:
'email' as CredentialType; also add the import line for type { CredentialType }
from '../../types/credentials'. If you require a distinct IMAP credential,
extend CREDENTIAL_TYPES in types/credentials.ts to include 'imap' and use
credentialType: 'imap' as CredentialType instead. Ensure the new credentials
field retains the original label/description context and that server-side code
retrieves the secret from the credentials store rather than from node config.
| import { WebhookNodeService } from '../../nodes/WebhookNode/WebhookNode.service' | ||
| import { NodeExecutionContext } from '../../nodes/types' | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Wire up the new Email Trigger; current EMAIL case is a stub.
With the IMAP EmailTriggerNode added, the TriggerType.EMAIL case still returns a mock. Either delegate to the node’s executeNode (if provided) or to a server-side EmailTriggerService-backed executor.
Suggested direction (outside this hunk):
- case TriggerType.EMAIL:
- // In a real implementation, this would monitor an email inbox
- return { triggered: true, from: 'test@example.com' }
+ case TriggerType.EMAIL: {
+ // Delegate to server-side service-backed executor
+ const { executeEmailTrigger } = await import('../../nodes/EmailTriggerNode') // e.g., service wrapper
+ const result = await executeEmailTrigger(context)
+ if (!result.success) {
+ throw new Error(result.error || 'Email trigger execution failed')
+ }
+ return result.output
+ }Verification script to find/confirm an exported executor:
🏁 Script executed:
#!/bin/bash
rg -nP -C2 'executeEmailTrigger\s*\(' nodes/EmailTriggerNodeLength of output: 62
🏁 Script executed:
#!/bin/bash
# Search for EmailTriggerNodeService class
rg -nP -C2 'class\s+.*EmailTriggerNodeService' nodes
# Search for all references to EmailTrigger within nodes
rg -n 'EmailTrigger' -C2 nodesLength of output: 8632
🏁 Script executed:
#!/bin/bash
# Show the EMAIL trigger case in the workflow executor
rg -nP -C5 'case TriggerType\\.EMAIL' server/services/workflow-executor.tsLength of output: 76
🏁 Script executed:
#!/bin/bash
# Locate the workflow executor file
fd workflow-executor.ts .
# Fallback: locate any executor file under server/services
fd executor.ts server/servicesLength of output: 136
🏁 Script executed:
#!/bin/bash
# Print the first 200 lines of the workflow executor to locate the EMAIL case
sed -n '1,200p' server/services/workflow-executor.tsLength of output: 7170
🏁 Script executed:
#!/bin/bash
# Locate EMAIL trigger handling in WorkflowExecutor
rg -nP -C3 'case TriggerType\\.EMAIL' server/services/workflow-executor.tsLength of output: 76
🏁 Script executed:
#!/bin/bash
# Print lines 200–350 to locate executeTriggerNode and EMAIL case
sed -n '200,350p' server/services/workflow-executor.tsLength of output: 4852
Implement EMAIL trigger delegation
In WorkflowExecutor.executeTriggerNode (server/services/workflow-executor.ts), the case TriggerType.EMAIL still returns a hard-coded mock. Replace it with a real executor call—e.g. import your EmailTriggerNodeService (or directly use EmailTriggerService from server/services/email-trigger.service.ts) and invoke its execution method with context, throwing on failure.
🤖 Prompt for AI Agents
In server/services/workflow-executor.ts around lines 27 to 29, the
TriggerType.EMAIL branch currently returns a hard-coded mock; replace it by
importing the Email trigger service (either EmailTriggerNodeService from the
node implementation or EmailTriggerService from
server/services/email-trigger.service.ts) and call its execute (or equivalent)
method with the existing context; await the call, check its result and throw an
error if execution fails instead of returning the mock. Ensure you add the
import at the top, remove the mock return, await the real executor invocation
with context, and propagate errors by throwing when the executor indicates
failure.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
nodes/EmailNode/email-providers.ts (6)
21-31: Tighten Nodemailer typing; drop any-castsUse Nodemailer’s SMTPTransport.Options for the transporter config, remove the eslint disables and any-casts.
Apply:
@@ -import type { Transporter as NodemailerTransporter } from 'nodemailer' +import type { Transporter as NodemailerTransporter } from 'nodemailer' +import type SMTPTransport from 'nodemailer/lib/smtp-transport' @@ - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const transporter: NodemailerTransporter = await getNodemailerTransporter({ + const transporter: NodemailerTransporter = await getNodemailerTransporter({ host: emailService.host, port: emailService.port || 587, secure: emailService.secure || false, auth: { user: emailService.auth.user, pass: emailService.auth.pass } - // eslint-disable-next-line @typescript-eslint/no-explicit-any - } as any) + }) @@ -// eslint-disable-next-line @typescript-eslint/no-explicit-any -async function getNodemailerTransporter(options: any): Promise<NodemailerTransporter> { +async function getNodemailerTransporter(options: SMTPTransport.Options): Promise<NodemailerTransporter> { @@ - // eslint-disable-next-line @typescript-eslint/no-unsafe-argument return nodemailer.createTransport(options)Also applies to: 130-147, 4-4
33-38: Pass recipients as array instead of joined stringNodemailer accepts arrays; avoids formatting surprises and lets Nodemailer handle address parsing.
- const mailOptions: MailOptions = { + const mailOptions: MailOptions = { from: from || emailService.auth.user, - to: to.join(', '), + to, subject, text: body }
41-43: Align result typing with Nodemailer’s SentMessageInfoCasting to a custom SendMailResult plus disabling linting hides type issues. Prefer Nodemailer’s own type.
- // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment - const info: SendMailResult = await transporter.sendMail(mailOptions) as SendMailResult + const info = await transporter.sendMail(mailOptions)If you need strong typing, import
import type { SentMessageInfo } from 'nodemailer'and annotateconst info: SentMessageInfo = ....
58-61: Thread AbortSignal and add a timeout to SendGrid fetchHook into upstream
AbortSignaland fail fast. Improves cancellation and prevents hung requests.-export async function sendWithSendGrid(config: EmailNodeConfig): Promise<EmailExecutionResult> { +export async function sendWithSendGrid(config: EmailNodeConfig, signal?: AbortSignal): Promise<EmailExecutionResult> { @@ - const response = await fetch('https://api.sendgrid.com/v3/mail/send', { + const controller = !signal ? new AbortController() : undefined + const timeout = setTimeout(() => controller?.abort(), 15000) + try { + const response = await fetch('https://api.sendgrid.com/v3/mail/send', { method: 'POST', headers: { 'Authorization': `Bearer ${emailService.apiKey}`, 'Content-Type': 'application/json' }, + signal: signal ?? controller?.signal, body: JSON.stringify({ personalizations: [{ to: to.map(email => ({ email })) }], from: { email: from || emailService.auth.user }, subject, content: [{ type: 'text/plain', value: body }] }) - }) + }) + if (!response.ok) { + const errBody = await response.text().catch(() => '') + throw new Error(`SendGrid API error: ${response.status} ${response.statusText}${errBody ? ` - ${errBody}` : ''}`) + } + } finally { + clearTimeout(timeout) + }Follow-up: pass
signalfrom the service (see corresponding comment there).Also applies to: 69-87, 4-4
106-125: Reduce noisy console logs in simulationGate logs behind a debug flag or use a logger with levels to avoid polluting output in production/test.
141-143: Preserve underlying load error messageA generic message hampers diagnostics. Include the original error message.
- } catch (error) { - throw new Error('Failed to load nodemailer module') + } catch (error) { + throw new Error(`Failed to load nodemailer module: ${error instanceof Error ? error.message : 'unknown error'}`) }nodes/EmailNode/EmailNode.service.ts (2)
35-45: Propagate AbortSignal to SendGrid pathAllows cancellation/timeouts to flow into the HTTP request.
- const { sendWithNodemailer, sendWithSendGrid } = await import('./email-providers') + const { sendWithNodemailer, sendWithSendGrid } = await import('./email-providers') @@ - if (emailConfig.emailService.type === 'sendgrid') { - result = await sendWithSendGrid(emailConfig) + if (emailConfig.emailService.type === 'sendgrid') { + result = await sendWithSendGrid(emailConfig, signal) } else { // Default to nodemailer for SMTP/Gmail/Outlook result = await sendWithNodemailer(emailConfig, emailConfig.emailService.type) }
35-45: Attachments not wired throughConfig exposes
attachments, but neither SendGrid nor Nodemailer paths use them yet. If intentional, ignore; if not, we can add basic support (paths or base64).Happy to draft attachment mapping for both providers.
Also applies to: 19-31
📜 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 selected for processing (2)
nodes/EmailNode/EmailNode.service.ts(1 hunks)nodes/EmailNode/email-providers.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
nodes/EmailNode/EmailNode.service.ts (3)
nodes/types.ts (1)
NodeExecutionContext(5-13)nodes/EmailNode/EmailNode.types.ts (2)
EmailExecutionResult(31-39)EmailNodeConfig(3-24)nodes/EmailNode/email-providers.ts (2)
sendWithSendGrid(57-104)sendWithNodemailer(10-55)
nodes/EmailNode/email-providers.ts (2)
nodes/EmailNode/EmailNode.types.ts (2)
EmailNodeConfig(3-24)EmailExecutionResult(31-39)nodes/EmailNode/nodemailer-types.ts (1)
SendMailResult(19-29)
🔇 Additional comments (3)
nodes/EmailNode/email-providers.ts (1)
11-14: Good server-only guardPrevents client-side execution paths early. LGTM.
nodes/EmailNode/EmailNode.service.ts (2)
7-10: Abort-aware early exit looks goodCleanly bails out when the signal is already aborted.
47-55: Confirm client-side “success” semanticsReturning sent: true on the client (simulation) can mislead downstream nodes/UX into assuming a real email was sent. Consider marking
success: falsewith a clear simulation flag, orsent: falseandprovider: 'Simulated'.Would you like a follow-up patch to flip this to an explicit dry-run result?
| // Validate config | ||
| if (!config) { | ||
| return { success: false, error: 'Configuration is required' } | ||
| } | ||
|
|
||
| private static async sendWithSMTP(config: EmailNodeConfig): Promise<EmailExecutionResult> { | ||
| return await sendWithNodemailer(config, 'SMTP') | ||
| } | ||
| } | ||
| const emailConfig = config as EmailNodeConfig | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate emailService and auth before execution
Missing checks can yield runtime errors (e.g., accessing properties of undefined) or opaque provider failures. Add minimal guards.
const emailConfig = config as EmailNodeConfig
// Validate required fields
if (!emailConfig.to || emailConfig.to.length === 0) {
return { success: false, error: 'At least one recipient is required' }
}
if (!emailConfig.subject || emailConfig.subject.trim() === '') {
return { success: false, error: 'Subject is required' }
}
if (!emailConfig.body || emailConfig.body.trim() === '') {
return { success: false, error: 'Email body is required' }
}
+ if (!emailConfig.emailService || !emailConfig.emailService.type) {
+ return { success: false, error: 'Email service configuration is required' }
+ }
+ if (emailConfig.emailService.type === 'sendgrid') {
+ if (!emailConfig.emailService.apiKey) {
+ return { success: false, error: 'SendGrid API key is required' }
+ }
+ } else {
+ if (!emailConfig.emailService.auth?.user || !emailConfig.emailService.auth?.pass) {
+ return { success: false, error: 'SMTP credentials (user/pass) are required' }
+ }
+ }Also applies to: 19-31
1 participant
Summary
This pull request introduces a comprehensive Email Trigger Node that enables workflows to be automatically triggered by incoming emails from IMAP servers. The node provides robust email monitoring capabilities with extensive configuration options, making it ideal for automating email-based workflows such as support ticket processing, order confirmations, and any email-triggered automation.
Key Features
Technical Implementation
The EmailTriggerNode follows the established project architecture:
EmailTriggerServicehandles all IMAP operations usingimap-simpleandmailparserBaseNodepatternConfiguration Options
The node supports 15+ configuration parameters including:
Dependencies Added
imap-simple: IMAP client library for Node.jsmailparser: Email parsing and attachment extraction@types/imap-simple: TypeScript definitions@types/mailparser: TypeScript definitionsScreenshots / Videos (if UI changes)
The EmailTriggerNode integrates seamlessly with the existing workflow editor UI, displaying connection status, mailbox information, and configuration details through color-coded indicators and descriptive text.
Checklist
npm run typecheck- All TypeScript errors resolvednpm run lint- All ESLint errors resolvednpm test- All unit and integration tests passnpm run build- Build completes successfullyREADME.md/docs/) as needed - Comprehensive README addedRelated Issues
This feature addresses the need for email-based workflow triggers, enabling users to automate processes based on incoming emails without requiring external webhook services.
Files Changed
New Files
nodes/EmailTriggerNode/EmailTriggerNode.types.ts- TypeScript interfaces and typesnodes/EmailTriggerNode/EmailTriggerNode.service.ts- Core IMAP service implementationnodes/EmailTriggerNode/EmailTriggerNode.schema.ts- Node definition and configuration schemanodes/EmailTriggerNode/EmailTriggerNode.tsx- React component for workflow editornodes/EmailTriggerNode/EmailTriggerNode.test.ts- Comprehensive unit testsnodes/EmailTriggerNode/README.md- Detailed documentation and usage examplesnodes/EmailTriggerNode/index.ts- Barrel export fileModified Files
types/workflow.ts- Added EMAIL to TriggerType enumnodes/node-definitions.ts- Registered EmailTriggerNode in central registrynodes/index.ts- Added EmailTriggerNode to main nodes exporttests/integration/node-registry.test.ts- Updated test expectations for new nodepackage.json- Added required dependenciesTesting
Usage Examples
The node supports various email provider configurations:
Security Considerations
This implementation provides a robust foundation for email-based workflow automation while maintaining the project's high standards for code quality, testing, and documentation.