feat(imap) IMAP plugin #310
Conversation
Reviewer's GuideThis PR revives and restructures the IMAP integration plugin by refactoring the core utilities, wiring up an Express route, adding message broker handlers, integrating with redlock-based listeners, updating GraphQL resolvers, and bumping required dependencies. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a full IMAP integration: new init function run at server start, GraphQL mutation support, an IMAP message broker for create/update/remove/listen operations, a complete IMAP utils rewrite (connection, syncing, persistence, locks, distribution), an attachment download route, and new runtime dependencies. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Server
participant IMAPApp as IMAP App
participant Orgs as Orgs/SaaS
participant Utils as IMAP Utils
participant Broker as IMAP Broker
Server->>IMAPApp: initImapApp(app)
IMAPApp->>Orgs: get organizations or use 'os'
loop per subdomain
IMAPApp->>Utils: setupMessageConsumers(subdomain)
Utils->>Broker: sendImapMessage({_id})
Broker->>Utils: ImapListen({_id})
Utils->>Utils: obtain lock → connect → sync → listen
end
note right of Utils: ready / mail / error handlers\nupdate health/status and persist state
sequenceDiagram
autonumber
participant GQL as GraphQL Resolvers
participant Broker as IMAP Broker
participant Models as IMAP Models
participant Utils as IMAP Utils
GQL->>Broker: imapCreateIntegrations({subdomain,data})
Broker->>Models: insert ImapIntegrations
Broker->>Utils: listenIntegration({_id})
Broker-->>GQL: {status:'success'}/error
GQL->>Broker: imapUpdateIntegrations({subdomain,data})
Broker->>Models: update + fetch integration
Broker->>Utils: reattach listener
Broker-->>GQL: {status:'success'}/error
GQL->>Broker: imapRemoveIntegrations({subdomain,data})
Broker->>Models: remove ImapMessages/ImapCustomers/ImapIntegrations
Broker-->>GQL: {status:'success'}/error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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 (
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
Blocking issues:
- Detected possible user input going into a
path.joinorpath.resolvefunction. This could possibly lead to a path traversal vulnerability, where the attacker can access arbitrary files stored in the file system. Instead, be sure to sanitize or validate user input first. (link) - Detected that function argument
attrshas entered the fs module. An attacker could potentially control the location of this file, to include going backwards in the directory with '../'. To address this, ensure that user-controlled variables in file paths are validated. (link) - Detected that function argument
attrshas entered the fs module. An attacker could potentially control the location of this file, to include going backwards in the directory with '../'. To address this, ensure that user-controlled variables in file paths are validated. (link)
General comments:
- In imapUpdateIntegrations you’re passing the original
integrationobject into listenIntegration rather than the freshly fetchedupdatedIntegration, so new settings won’t be applied—swap in the updated document. - The searchMessages promise uses bare
throwstatements inside callbacks, which won’t reject the promise properly—replace them withreject(err)to propagate errors correctly. - You’ve duplicated a lot of IMAP connection, event binding, and cleanup logic across utils and the messageBroker/route setup—consider extracting a shared helper to reduce repetition and centralize lifecycle management.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In imapUpdateIntegrations you’re passing the original `integration` object into listenIntegration rather than the freshly fetched `updatedIntegration`, so new settings won’t be applied—swap in the updated document.
- The searchMessages promise uses bare `throw` statements inside callbacks, which won’t reject the promise properly—replace them with `reject(err)` to propagate errors correctly.
- You’ve duplicated a lot of IMAP connection, event binding, and cleanup logic across utils and the messageBroker/route setup—consider extracting a shared helper to reduce repetition and centralize lifecycle management.
## Security Issues
### Issue 1
<location> `backend/plugins/frontline_api/src/modules/integrations/imap/configs.ts:99` </location>
<issue_to_address>
**security (javascript.lang.security.audit.path-traversal.path-join-resolve-traversal):** Detected possible user input going into a `path.join` or `path.resolve` function. This could possibly lead to a path traversal vulnerability, where the attacker can access arbitrary files stored in the file system. Instead, be sure to sanitize or validate user input first.
*Source: opengrep*
</issue_to_address>
### Issue 2
<location> `backend/plugins/frontline_api/src/modules/integrations/imap/configs.ts:109` </location>
<issue_to_address>
**security (javascript.lang.security.audit.detect-non-literal-fs-filename):** Detected that function argument `attrs` has entered the fs module. An attacker could potentially control the location of this file, to include going backwards in the directory with '../'. To address this, ensure that user-controlled variables in file paths are validated.
*Source: opengrep*
</issue_to_address>
### Issue 3
<location> `backend/plugins/frontline_api/src/modules/integrations/imap/configs.ts:122` </location>
<issue_to_address>
**security (javascript.lang.security.audit.detect-non-literal-fs-filename):** Detected that function argument `attrs` has entered the fs module. An attacker could potentially control the location of this file, to include going backwards in the directory with '../'. To address this, ensure that user-controlled variables in file paths are validated.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| const tempPath = path.join( | ||
| os.tmpdir(), | ||
| `${Date.now()}-${attachment.params.name}`, |
There was a problem hiding this comment.
security (javascript.lang.security.audit.path-traversal.path-join-resolve-traversal): Detected possible user input going into a path.join or path.resolve function. This could possibly lead to a path traversal vulnerability, where the attacker can access arbitrary files stored in the file system. Instead, be sure to sanitize or validate user input first.
Source: opengrep
|
|
||
| fetcher.on('message', (msg) => { | ||
| msg.on('body', (stream) => { | ||
| const writeStream = fs.createWriteStream(tempPath); |
There was a problem hiding this comment.
security (javascript.lang.security.audit.detect-non-literal-fs-filename): Detected that function argument attrs has entered the fs module. An attacker could potentially control the location of this file, to include going backwards in the directory with '../'. To address this, ensure that user-controlled variables in file paths are validated.
Source: opengrep
| tempPath, | ||
| attachment.params.name, | ||
| (err) => { | ||
| fs.unlink(tempPath, () => {}); |
There was a problem hiding this comment.
security (javascript.lang.security.audit.detect-non-literal-fs-filename): Detected that function argument attrs has entered the fs module. An attacker could potentially control the location of this file, to include going backwards in the directory with '../'. To address this, ensure that user-controlled variables in file paths are validated.
Source: opengrep
| if (Array.isArray(struct[i])) { | ||
| findAttachmentParts(struct[i], attachments); | ||
| } else { | ||
| if ( | ||
| struct[i].disposition && | ||
| ['INLINE', 'ATTACHMENT'].indexOf(toUpper(struct[i].disposition.type)) > | ||
| -1 | ||
| ) { | ||
| attachments.push(struct[i]); | ||
| } | ||
| } |
There was a problem hiding this comment.
suggestion (code-quality): Merge else clause's nested if statement into else if (merge-else-if)
| if (Array.isArray(struct[i])) { | |
| findAttachmentParts(struct[i], attachments); | |
| } else { | |
| if ( | |
| struct[i].disposition && | |
| ['INLINE', 'ATTACHMENT'].indexOf(toUpper(struct[i].disposition.type)) > | |
| -1 | |
| ) { | |
| attachments.push(struct[i]); | |
| } | |
| } | |
| if (Array.isArray(struct[i])) { | |
| findAttachmentParts(struct[i], attachments); | |
| } | |
| else if (struct[i].disposition && | |
| ['INLINE', 'ATTACHMENT'].indexOf(toUpper(struct[i].disposition.type)) > | |
| -1) { | |
| attachments.push(struct[i]); | |
| } | |
Explanation
Flattening if statements nested within else clauses generates code that iseasier to read and expand upon.
| while (true) { | ||
| try { | ||
| await distributeJob(); | ||
| // try doing it every 10 minutes | ||
| await new Promise((resolve) => setTimeout(resolve, 10 * 60 * 1000)); | ||
| } catch (e) { | ||
| console.log('distributeWork error', e); | ||
| } | ||
| } |
There was a problem hiding this comment.
issue (code-quality): Avoid loops with missing or constant end conditions. (avoid-infinite-loops)
Explanation
Loops with a constant condition, or no terminator, can be useful in certain contexts, but it's easy to forget to break out of them. This rule highlights loops which may never end.![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to cc0e94a in 1 minute and 53 seconds. Click for details.
- Reviewed
1582lines of code in7files - Skipped
0files when reviewing. - Skipped posting
7draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. backend/plugins/frontline_api/package.json:15
- Draft comment:
New dependencies (dotenv, ioredis, mailparser, node-imap) have been added and redlock updated. Please ensure these versions are compatible with the rest of the codebase and have been adequately tested. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is about dependency changes and asks the PR author to ensure compatibility and testing, which violates the rules. It doesn't provide a specific suggestion or point out a specific issue with the code.
2. backend/plugins/frontline_api/src/main.ts:11
- Draft comment:
IMAP plugin initialization has been introduced. Confirm that any errors within initImapApp are properly handled and that its initialization order (before call app) does not introduce conflicts. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to confirm that errors are handled and that the initialization order does not introduce conflicts. This is asking for confirmation of intention and ensuring behavior, which violates the rules.
3. backend/plugins/frontline_api/src/modules/inbox/graphql/resolvers/mutations/integrations.ts:68
- Draft comment:
IMAP integration cases have been added for create, update, and remove. Verify that the payload structure and error handling for IMAP integrations are consistent with other integrations. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to verify the payload structure and error handling, which is against the rules as it asks for confirmation and verification. It does not provide a specific suggestion or point out a specific issue.
4. backend/plugins/frontline_api/src/modules/integrations/imap/redlock.ts:7
- Draft comment:
Ensure that the Redis environment variables (REDIS_HOST, REDIS_PORT, REDIS_PASSWORD) are properly set and validated, as they are critical for the redlock initialization. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. backend/plugins/frontline_api/src/modules/integrations/imap/utils.ts:54
- Draft comment:
In the searchMessages function, errors in the imap.search callback are thrown instead of rejecting the promise. Use reject(err) to properly propagate asynchronous errors. - Reason this comment was not posted:
Comment was on unchanged code.
6. backend/plugins/frontline_api/src/modules/integrations/imap/utils.ts:359
- Draft comment:
The cleanupLock function uses 'lockExtendInterval' which is defined later. Consider declaring lockExtendInterval in an outer scope before cleanupLock to avoid potential hoisting issues. - Reason this comment was not posted:
Comment looked like it was already resolved.
7. backend/plugins/frontline_api/src/modules/inbox/graphql/resolvers/mutations/integrations.ts:30
- Draft comment:
Typo: 'callRemoveIntergration' appears to be misspelled. It should likely be 'callRemoveIntegration'. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_Pez2r7MeGrhTICHo
You can customize 
| app.use(express.json({ limit: '15mb' })); | ||
|
|
||
| // ====== Read Mail Attachment Route ====== | ||
| app.get( |
There was a problem hiding this comment.
The '/read-mail-attachment' endpoint does not appear to enforce authentication or authorization. Confirm that public access is acceptable or add proper security checks.
| } | ||
| } | ||
|
|
||
| export async function imapUpdateIntegrations({ |
There was a problem hiding this comment.
In imapUpdateIntegrations, the code calls listenIntegration with the original integration object rather than the updated one. Additionally, the error message in the catch block refers to 'create integration' instead of 'update'. Consider adjusting these for clarity and correctness.
| healthStatus: 'healthy', | ||
| }); | ||
| for (const integration of integrations) { | ||
| console.log(integrations, '&&&&&&&&&'); |
There was a problem hiding this comment.
A debug log printing '&&&&&&&&&' is present in startDistributingJobs. Remove or refine such debug statements before merging to production.
| console.log(integrations, '&&&&&&&&&'); |
| } catch (e) { | ||
| return { | ||
| status: 'error', | ||
| errorMessage: `Failed to create integration: ${e.message}`, |
There was a problem hiding this comment.
Typo in the error message in imapUpdateIntegrations: It currently says 'Failed to create integration' but should likely be 'Failed to update integration'.
| errorMessage: `Failed to create integration: ${e.message}`, | |
| errorMessage: `Failed to update integration: ${e.message}`, |
| } catch (e) { | ||
| return { | ||
| status: 'error', | ||
| errorMessage: `Failed to create integration: ${e.message}`, |
There was a problem hiding this comment.
Typo in the error message in imapRemoveIntegrations: It currently says 'Failed to create integration' but should likely be 'Failed to remove integration'.
| errorMessage: `Failed to create integration: ${e.message}`, | |
| errorMessage: `Failed to remove integration: ${e.message}`, |
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 73ef46f in 29 seconds. Click for details.
- Reviewed
35lines of code in2files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. backend/plugins/frontline_api/src/modules/integrations/imap/messageBroker.ts:81
- Draft comment:
Correct error message is now consistent with update action. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. backend/plugins/frontline_api/src/modules/integrations/imap/messageBroker.ts:109
- Draft comment:
Error message now correctly reflects the remove context. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. backend/plugins/frontline_api/src/modules/integrations/imap/utils.ts:435
- Draft comment:
Removed unnecessary debug log; using proper logging (if needed) is preferred. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_XJD0FAivhTEaxZy2
You can customize
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/plugins/frontline_api/src/main.ts (1)
30-41: Prevent plugin startup from failing if IMAP initialization throws.
initImapApp(app)currently runs outside the existing try/catch. Any error will crash startup. Wrap it so the service can still boot and report actionable logs.onServerInit: async (app) => { - await initImapApp(app); - await initCallApp(app); - try { + try { + await initImapApp(app); + await initCallApp(app); if (getEnv({ name: 'CALL_DASHBOARD_ENABLED' })) { await initializeCallQueueMonitoring(); } } catch (error) { - console.error('Failed to initialize call queue monitoring:', error); + console.error('Failed to initialize startup tasks (IMAP/Call/CALL_DASHBOARD):', error); } },
🧹 Nitpick comments (19)
backend/plugins/frontline_api/package.json (1)
16-27: Dependencies sanity-checked: all clear to merge
- [email protected]
• Matches default ESM import (import Redlock from 'redlock') in
–backend/plugins/frontline_api/src/modules/integrations/imap/redlock.ts
–backend/plugins/frontline_api/src/modules/integrations/call/redlock.ts
• Latest non-beta release, no GitHub advisories found ([securityVulnerabilities nodes empty])- node-imap@^0.9.6 & mailparser@^3.7.4
• Imports (import Imap from 'node-imap'andimport { simpleParser } from 'mailparser') in
–backend/plugins/frontline_api/src/modules/integrations/imap/utils.ts
• Both at latest stable versions, no advisories detected- dotenv@^17.2.1
• Usesimport * as dotenv from 'dotenv'; dotenv.config();unchanged from prior patterns
• Latest stable release, no advisories detectedAll other packages (ioredis@^5.7.0, base64-stream@^1.0.0) are at their latest stable versions with no known security issues.
You can proceed with merging.
Optional refactor recommendation:
- Evaluate switching to imapflow for more robust idle handling and built-in TypeScript typings in long-running IMAP listeners.
backend/plugins/frontline_api/src/modules/integrations/imap/redlock.ts (1)
2-15: Default import switch for redlock is correct for 4.x; consider resiliency options.Instantiation looks fine with ioredis. For robustness in distributed job locking, set retry and jitter parameters to avoid thundering herd under Redis hiccups.
-export const redlock = new Redlock([redis]); +export const redlock = new Redlock([redis], { + // backoff strategy to reduce lock contention and avoid spikes + retryCount: 10, + retryDelay: 200, // ms + retryJitter: 100, // ms + // driftFactor kept default; tune if you observe frequent clock drift issues +});If your tsconfig does not enable esModuleInterop, ensure the default import compiles as expected; otherwise switch to
import Redlock = require('redlock').backend/plugins/frontline_api/src/modules/integrations/imap/configs.ts (4)
12-17: Use absolute path aliases for local imports to match repo guidelines.Switch
./utilsand./messageBrokerto absolute aliases for consistency and easier refactors.-import startDistributingJobs, { - findAttachmentParts, - createImap, - toUpper, - routeErrorHandling, -} from './utils'; +import startDistributingJobs, { + findAttachmentParts, + createImap, + toUpper, + routeErrorHandling, +} from '~/modules/integrations/imap/utils'; @@ -import { +import { imapCreateIntegrations, imapUpdateIntegrations, imapRemoveIntegrations, ImapListen, sendImapMessage, -} from './messageBroker'; +} from '~/modules/integrations/imap/messageBroker';Also applies to: 21-27
63-65: Gmail-specific folder assumption will break on non-Gmail IMAP servers.Hard-coding
[Gmail]/Sent Maillimits attachment retrieval on most providers. Prefer a provider-agnostic approach (e.g., try the message’s original mailbox if stored, a configured “sentMailbox” on the integration, or probe common sent folders).-const folderType = sentMessage ? '[Gmail]/Sent Mail' : 'INBOX'; +const folderType = + sentMessage + ? integration.sentMailbox || 'Sent' // e.g., configurable per integration + : 'INBOX';If
integrationor your message schema keeps the mailbox/folder, use that directly to avoid guessing.
34-51: Input validation: add basic normalization for messageId and filename.Presence checks are good; add normalization to avoid subtle mismatches (Message-ID angle brackets) and sanitize the download filename used in headers.
-const { messageId, integrationId, filename } = req.query as Record<string, string>; +let { messageId, integrationId, filename } = req.query as Record<string, string>; +// Normalize Message-ID to include angle brackets if omitted +if (messageId && !/^<.*>$/.test(messageId)) { + messageId = `<${messageId}>`; +} +// Prevent header injection / odd characters in Content-Disposition +const safeFilename = path.basename(filename || ''); @@ - if (!messageId || !integrationId || !filename) { + if (!messageId || !integrationId || !safeFilename) { return res.status(400).send('Missing required query parameters'); }And replace
attachment.params.nameinres.downloadwithsafeFilename.
179-179: Replace console.log with a structured logger.Follow project guideline to avoid console logs.
-console.log('IMAP plugin initialized successfully.'); +debugError('IMAP plugin initialized successfully.');backend/plugins/frontline_api/src/modules/integrations/imap/messageBroker.ts (6)
33-38: Improve error message consistency (create path)Message says "Failed to create integration" which is fine here, but consider reusing a shared
toErrorhelper and include a safe code in addition tomessage.- return { - status: 'error', - errorMessage: `Failed to create integration: ${e.message}`, - }; + return { status: 'error', errorMessage: `imapCreateIntegrations failed: ${e instanceof Error ? e.message : String(e)}` };
79-84: Wrong error message (update path)This path is update, but the message says "Failed to create integration." Correct the copy.
- return { - status: 'error', - errorMessage: `Failed to create integration: ${e.message}`, - }; + return { status: 'error', errorMessage: `imapUpdateIntegrations failed: ${e instanceof Error ? e.message : String(e)}` };
107-112: Wrong error message (remove path)Same copy/paste issue in remove flow.
- return { - status: 'error', - errorMessage: `Failed to create integration: ${e.message}`, - }; + return { status: 'error', errorMessage: `imapRemoveIntegrations failed: ${e instanceof Error ? e.message : String(e)}` };
23-23: Avoid unhandled promise rejections when starting listeners
listenIntegrationis async and can reject before the lock is obtained. Fire-and-forget is fine, but explicitly detach withvoidand add.catchto avoid unhandled rejections.- listenIntegration(subdomain, integration, models); + void listenIntegration(subdomain, integration, models).catch((err) => + models.ImapLogs.createLog({ type: 'error', message: `listenIntegration failed: ${err.message}`, errorStack: err.stack }), + );Replicate the same pattern where you reattach or explicitly start listeners.
Also applies to: 73-73, 128-128, 144-144
4-10: Add explicit TypeScript types for broker function inputs/outputsMissing types reduce safety. Define shared arg types and return type once and reuse.
interface SendImapMessageArgs { subdomain: string; action?: string; data: { _id: string; }; } + +interface ImapUpdateArgs { + subdomain: string; + data: { integrationId: string; doc: { data: string | Record<string, any> } }; +} + +interface ImapRemoveArgs { + subdomain: string; + data: { integrationId: string }; +} + +type BrokerResult = { status: 'success' } | { status: 'error'; errorMessage: string };Then annotate the exported functions with these types and return
Promise<BrokerResult>.Also applies to: 41-45, 87-91, 115-121, 133-136
121-126: Use ImapLogs instead of console.log for operational signalsPer guidelines ("Avoid console logs"), write into your existing
ImapLogsto aid observability.- if (!integration) { - console.log(`Queue: imap:listen. Integration not found ${_id}`); - return; - } + if (!integration) { + await models.ImapLogs.createLog({ + type: 'warn', + message: `Queue(imap:listen): integration not found ${_id}`, + }); + return; + }Do the same in
ImapListen.Also applies to: 137-141
backend/plugins/frontline_api/src/modules/integrations/imap/utils.ts (7)
75-79: ESLint: use const for buffers
buffersis never reassigned.- let buffers: Buffer[] = []; + const buffers: Buffer[] = [];
229-235: Guard against missing attachments and provide content fallbacks
msg.attachmentsmay be undefined. Also,msg.htmlcan be empty; fall back totextAsHtmlortext.- attachments: msg.attachments.map(({ filename, contentType, size }) => ({ - filename, - type: contentType, - size, - })), + attachments: + (msg.attachments?.map(({ filename, contentType, size }) => ({ + filename, + type: contentType, + size, + })) as any[]) || [], + body: msg.html || msg.textAsHtml || msg.text || '',
335-337: Replace console logs with structured logsMultiple
console.log/errorcalls should be replaced with a centralized logger orImapLogsto match the rest of the module and project guidelines (avoid console logs).Example:
- console.log('IMAP ready for integration:', integration._id); + await models.ImapLogs.createLog({ type: 'info', message: `IMAP ready for integration: ${integration._id}` });Do similarly for sync success, errors, and catch blocks.
Also applies to: 303-304, 318-333, 350-356, 410-411
358-365: ESLint: empty catch blocks – handle or document intentionallyEmpty
catch {}blocks causeno-emptyviolations and swallow errors. Either log at debug level or add comments to explain intentional suppression.- try { - clearInterval(lockExtendInterval); - } catch {} + try { + clearInterval(lockExtendInterval); + } catch (e) { + // noop: interval may already be cleared + } @@ - try { - await lock.unlock(); - } catch {} + try { + await lock.unlock(); + } catch (e) { + // noop: lock may have expired + }Also applies to: 367-377
11-16: Circular dependency between utils.ts and messageBroker.ts
utils.tsimportssendImapMessagefrommessageBroker.ts, andmessageBroker.tsimportslistenIntegrationfromutils.ts. This is fragile and can lead to partially initialized exports at runtime.
- Extract shared listener start/dispatch functions into a small module (e.g.,
imap/runtime.ts) and have both files depend on it.- Alternatively, swap the import in
utils.tsfor a dynamic import right before callingsendImapMessageto break the cycle:// right before use const { sendImapMessage } = await import('~/modules/integrations/imap/messageBroker');
16-48: Style and typing cleanups to match guidelines
- Prefer
functiondeclarations for pure helpers (toUpper,findAttachmentParts,createImap,searchMessages,saveMessages,listenIntegration,startDistributingJobs,routeErrorHandling).- Add TypeScript types to parameters and return values instead of
any.- Keep absolute imports (already done) and avoid console logs (see other comments).
Also applies to: 50-99, 101-109, 245-249, 416-466, 467-481
467-481: routeErrorHandling: return types and loggingUse Express typings and avoid
console.log. Also consider returning a standardized error body.-export const routeErrorHandling = (fn, callback?: any) => { - return async (req, res, next) => { +import type { Request, Response, NextFunction } from 'express'; +export const routeErrorHandling = ( + fn: (req: Request, res: Response, next: NextFunction) => Promise<unknown>, + callback?: (res: Response, e: any, next: NextFunction) => unknown, +) => { + return async (req: Request, res: Response, next: NextFunction) => { try { await fn(req, res, next); } catch (e) { - console.log(e.message); + await (await generateModels(req.headers['x-subdomain'] as string)).ImapLogs.createLog({ + type: 'error', + message: e instanceof Error ? e.message : String(e), + errorStack: e instanceof Error ? e.stack : undefined, + }); if (callback) { return callback(res, e, next); } return next(e); } }; };
📜 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 (7)
backend/plugins/frontline_api/package.json(1 hunks)backend/plugins/frontline_api/src/main.ts(2 hunks)backend/plugins/frontline_api/src/modules/inbox/graphql/resolvers/mutations/integrations.ts(4 hunks)backend/plugins/frontline_api/src/modules/integrations/imap/configs.ts(1 hunks)backend/plugins/frontline_api/src/modules/integrations/imap/messageBroker.ts(1 hunks)backend/plugins/frontline_api/src/modules/integrations/imap/redlock.ts(2 hunks)backend/plugins/frontline_api/src/modules/integrations/imap/utils.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{ts,tsx}: Use functional and declarative programming patterns; avoid classes.
Use TypeScript for all code; prefer interfaces over types.
Avoid enums; use maps instead.
Files:
backend/plugins/frontline_api/src/modules/integrations/imap/redlock.tsbackend/plugins/frontline_api/src/modules/integrations/imap/messageBroker.tsbackend/plugins/frontline_api/src/modules/inbox/graphql/resolvers/mutations/integrations.tsbackend/plugins/frontline_api/src/modules/integrations/imap/configs.tsbackend/plugins/frontline_api/src/main.tsbackend/plugins/frontline_api/src/modules/integrations/imap/utils.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{ts,tsx,js,jsx}: Use descriptive variable names with auxiliary verbs (e.g., isLoading, hasError).
Avoid console logs.
Always use absolute paths when importing.
Use the "function" keyword for pure functions.
Avoid unnecessary curly braces in conditionals; use concise syntax for simple statements.
**/*.{ts,tsx,js,jsx}: Use descriptive variable names with auxiliary verbs (e.g., isLoading, hasError).
Use the "function" keyword for pure functions.
Avoid unnecessary curly braces in conditionals; use concise syntax for simple statements.
Use absolute path when import.
Files:
backend/plugins/frontline_api/src/modules/integrations/imap/redlock.tsbackend/plugins/frontline_api/src/modules/integrations/imap/messageBroker.tsbackend/plugins/frontline_api/src/modules/inbox/graphql/resolvers/mutations/integrations.tsbackend/plugins/frontline_api/src/modules/integrations/imap/configs.tsbackend/plugins/frontline_api/src/main.tsbackend/plugins/frontline_api/src/modules/integrations/imap/utils.ts
🧬 Code graph analysis (4)
backend/plugins/frontline_api/src/modules/integrations/imap/messageBroker.ts (1)
backend/plugins/frontline_api/src/modules/integrations/imap/utils.ts (1)
listenIntegration(245-414)
backend/plugins/frontline_api/src/modules/inbox/graphql/resolvers/mutations/integrations.ts (1)
backend/plugins/frontline_api/src/modules/integrations/imap/messageBroker.ts (3)
imapCreateIntegrations(12-39)imapUpdateIntegrations(41-85)imapRemoveIntegrations(87-113)
backend/plugins/frontline_api/src/modules/integrations/imap/configs.ts (5)
backend/plugins/frontline_api/src/modules/integrations/imap/utils.ts (4)
routeErrorHandling(467-481)createImap(39-48)findAttachmentParts(20-37)toUpper(16-18)backend/erxes-api-shared/src/utils/utils.ts (2)
getSubdomain(31-36)getEnv(9-29)backend/plugins/frontline_api/src/connectionResolvers.ts (2)
generateModels(281-281)IModels(121-151)backend/plugins/frontline_api/src/modules/integrations/imap/messageBroker.ts (1)
ImapListen(133-145)backend/erxes-api-shared/src/utils/saas/saas-mongo-connection.ts (1)
getSaasOrganizations(119-127)
backend/plugins/frontline_api/src/modules/integrations/imap/utils.ts (6)
backend/erxes-api-shared/src/utils/trpc/index.ts (2)
err(103-117)sendTRPCMessage(42-68)backend/plugins/frontline_api/src/connectionResolvers.ts (2)
IModels(121-151)generateModels(281-281)backend/plugins/frontline_api/src/modules/inbox/receiveMessage.ts (1)
receiveInboxMessage(21-227)backend/plugins/frontline_api/src/modules/inbox/graphql/resolvers/mutations/widget.ts (1)
pConversationClientMessageInserted(4-66)backend/plugins/frontline_api/src/modules/integrations/imap/redlock.ts (1)
redlock(15-15)backend/plugins/frontline_api/src/modules/integrations/imap/messageBroker.ts (1)
sendImapMessage(115-131)
🪛 ESLint
backend/plugins/frontline_api/src/modules/integrations/imap/configs.ts
[error] 122-122: Unexpected empty arrow function.
(@typescript-eslint/no-empty-function)
backend/plugins/frontline_api/src/modules/integrations/imap/utils.ts
[error] 75-75: 'buffers' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 361-361: Empty block statement.
(no-empty)
[error] 364-364: Empty block statement.
(no-empty)
[error] 371-371: Empty block statement.
(no-empty)
[error] 374-374: Empty block statement.
(no-empty)
[error] 451-452: Expected an assignment or function call and instead saw an expression.
(@typescript-eslint/no-unused-expressions)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
backend/plugins/frontline_api/src/modules/inbox/graphql/resolvers/mutations/integrations.ts (1)
23-27: IMAP message broker wiring verified
- Call sites correctly invoke the broker functions:
- Create:
imapCreateIntegrations({ subdomain, data })at line 69 inbackend/plugins/frontline_api/src/modules/inbox/graphql/resolvers/mutations/integrations.ts- Update:
imapUpdateIntegrations({ subdomain, data })at line 100- Remove:
imapRemoveIntegrations({ subdomain, data })at line 129- Broker definitions in
backend/plugins/frontline_api/src/modules/integrations/imap/messageBroker.ts:
imapCreateIntegrationsat line 12imapUpdateIntegrationsat line 41imapRemoveIntegrationsat line 87Next steps:
- Verify end-to-end payload shapes match broker signatures (stringify
datafor creates; use nesteddoc.datafor updates) and confirm that update flows restart any active listeners on the integration record.- Consider consolidating unsupported integration cases (
mobinetSms) under a single TODO or emit a telemetry event to gauge demand.
| const fetcher = imap.fetch(attrs.uid, { | ||
| bodies: [attachment.partID], | ||
| struct: true, | ||
| }); | ||
|
|
There was a problem hiding this comment.
Bug: second fetch uses UID but does not enable UID mode.
node-imap treats the message set as sequence numbers unless uid: true is set. You're passing attrs.uid, so this can fetch the wrong message or nothing.
-const fetcher = imap.fetch(attrs.uid, {
- bodies: [attachment.partID],
- struct: true,
-});
+const fetcher = imap.fetch(attrs.uid, {
+ bodies: [attachment.partID],
+ struct: true,
+ uid: true,
+});Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In backend/plugins/frontline_api/src/modules/integrations/imap/configs.ts around
lines 102 to 106, the second imap.fetch call passes attrs.uid but doesn't set
uid: true, so node-imap treats the argument as a sequence number; update the
fetch options to include uid: true (e.g., add uid: true to the options object)
so the fetch uses the UID mode and retrieves the correct message.
| writeStream.on('finish', () => { | ||
| res.download( | ||
| tempPath, | ||
| attachment.params.name, | ||
| (err) => { | ||
| fs.unlink(tempPath, () => {}); | ||
| imap.end(); | ||
| if (err) return next(err); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Fix empty arrow function and ensure reliable temp file cleanup.
ESLint flags the empty callback; also prefer promises and log cleanup failures via the existing debug utility.
- writeStream.on('finish', () => {
+ writeStream.on('finish', () => {
res.download(
tempPath,
attachment.params.name,
- (err) => {
- fs.unlink(tempPath, () => {});
- imap.end();
- if (err) return next(err);
- },
+ async (err) => {
+ try {
+ await fs.promises.unlink(tempPath);
+ } catch (unlinkErr) {
+ debugError(
+ `Failed to cleanup temp file ${tempPath}: ${unlinkErr?.message}`,
+ );
+ } finally {
+ imap.end();
+ }
+ if (err) return next(err);
+ },
);
});📝 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.
| writeStream.on('finish', () => { | |
| res.download( | |
| tempPath, | |
| attachment.params.name, | |
| (err) => { | |
| fs.unlink(tempPath, () => {}); | |
| imap.end(); | |
| if (err) return next(err); | |
| }, | |
| ); | |
| writeStream.on('finish', () => { | |
| res.download( | |
| tempPath, | |
| attachment.params.name, | |
| async (err) => { | |
| try { | |
| await fs.promises.unlink(tempPath); | |
| } catch (unlinkErr) { | |
| debugError( | |
| `Failed to cleanup temp file ${tempPath}: ${unlinkErr?.message}`, | |
| ); | |
| } finally { | |
| imap.end(); | |
| } | |
| if (err) return next(err); | |
| }, | |
| ); | |
| }); |
🧰 Tools
🪛 ESLint
[error] 122-122: Unexpected empty arrow function.
(@typescript-eslint/no-empty-function)
🤖 Prompt for AI Agents
In backend/plugins/frontline_api/src/modules/integrations/imap/configs.ts around
lines 117-126, the writeStream 'finish' handler uses an empty fs.unlink callback
and doesn't surface unlink errors; change this to use the promise-based
fs.promises.unlink and handle errors via the module's debug logger, and ensure
imap.end() is always called. Specifically, in the res.download callback wrap
fs.promises.unlink(tempPath) in a try/catch (or call an async IIFE) and on
failure log the error with the existing debug utility; call imap.end()
unconditionally before returning next(err) for the download error, and avoid
leaving empty arrow callbacks. Ensure all branches (successful download,
download error, unlink error) are properly awaited/handled and logged.
| const integration = await models.ImapIntegrations.create({ | ||
| inboxId: data.integrationId, | ||
| healthStatus: 'healthy', | ||
| error: '', | ||
| ...JSON.parse(data), | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix: Inconsistent data shape and unsafe JSON.parse in imapCreateIntegrations
You access data.integrationId (object-like) but also spread ...JSON.parse(data) (string-like). This will either throw (if data is an object) or produce integrationId: undefined (if data is a JSON string). Align the API with imapUpdateIntegrations by accepting { integrationId, doc } and parsing only doc.
Apply:
-export async function imapCreateIntegrations({ subdomain, data }) {
+interface ImapCreateArgs {
+ subdomain: string;
+ data: { integrationId: string; doc: string | Record<string, any> };
+}
+
+type BrokerResult = { status: 'success' } | { status: 'error'; errorMessage: string };
+
+export async function imapCreateIntegrations({ subdomain, data }: ImapCreateArgs): Promise<BrokerResult> {
try {
const models = await generateModels(subdomain);
- const integration = await models.ImapIntegrations.create({
- inboxId: data.integrationId,
- healthStatus: 'healthy',
- error: '',
- ...JSON.parse(data),
- });
+ const detail = typeof data.doc === 'string' ? JSON.parse(data.doc) : data.doc;
+ const integration = await models.ImapIntegrations.create({
+ inboxId: data.integrationId,
+ healthStatus: 'healthy',
+ error: '',
+ ...detail,
+ });🤖 Prompt for AI Agents
In backend/plugins/frontline_api/src/modules/integrations/imap/messageBroker.ts
around lines 16 to 21, the code mixes object and string handling by reading
data.integrationId while also spreading JSON.parse(data), which is unsafe and
inconsistent with imapUpdateIntegrations; change the handler to accept an object
shaped { integrationId, doc }, JSON.parse only the doc, then call
models.ImapIntegrations.create using inboxId: integrationId, healthStatus:
'healthy', error: '', and spread the parsed doc (not the entire data), and
update any callers to pass integrationId and doc accordingly.
| await models.ImapIntegrations.updateOne( | ||
| { inboxId: integrationId }, | ||
| { $set: detail }, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden update: whitelist fields before $set
Blindly setting detail allows clients to overwrite internal fields or inject unexpected structure. Pick allowed keys only (e.g., host, user, mainUser, password).
- await models.ImapIntegrations.updateOne(
- { inboxId: integrationId },
- { $set: detail },
- );
+ const allowed = (({ host, user, mainUser, password }) => ({ host, user, mainUser, password }))(detail || {});
+ await models.ImapIntegrations.updateOne(
+ { inboxId: integrationId },
+ { $set: { ...allowed, healthStatus: detail.healthStatus, error: detail.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.
| await models.ImapIntegrations.updateOne( | |
| { inboxId: integrationId }, | |
| { $set: detail }, | |
| ); | |
| // Harden update: only allow specific fields from `detail` | |
| const allowed = (({ host, user, mainUser, password }) => | |
| ({ host, user, mainUser, password }))(detail || {}); | |
| await models.ImapIntegrations.updateOne( | |
| { inboxId: integrationId }, | |
| { | |
| $set: { | |
| ...allowed, | |
| healthStatus: detail.healthStatus, | |
| error: detail.error, | |
| }, | |
| }, | |
| ); |
🤖 Prompt for AI Agents
In backend/plugins/frontline_api/src/modules/integrations/imap/messageBroker.ts
around lines 63 to 66, the code currently passes the whole detail object into
$set which allows clients to overwrite internal fields; instead whitelist
allowed keys (for example: host, user, mainUser, password — adjust to actual
allowed schema) and build a sanitized object containing only those keys present
in detail (omitting undefined/null), then call models.ImapIntegrations.updateOne
with { $set: sanitized } so only permitted fields are updated and no unexpected
structure is injected.
| if (updatedIntegration) { | ||
| listenIntegration(subdomain, integration, models); | ||
| } |
There was a problem hiding this comment.
Bug: Listener reattached with stale integration document
You fetch updatedIntegration (Lines 68-70) but call listenIntegration(subdomain, integration, models). This uses the stale pre-update doc and may miss updated credentials/fields.
Apply:
- if (updatedIntegration) {
- listenIntegration(subdomain, integration, models);
- }
+ if (updatedIntegration) {
+ void listenIntegration(subdomain, updatedIntegration, models).catch((err) =>
+ models.ImapLogs.createLog({ type: 'error', message: `listenIntegration failed: ${err.message}`, errorStack: err.stack }),
+ );
+ }📝 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 (updatedIntegration) { | |
| listenIntegration(subdomain, integration, models); | |
| } | |
| if (updatedIntegration) { | |
| void listenIntegration(subdomain, updatedIntegration, models).catch((err) => | |
| models.ImapLogs.createLog({ type: 'error', message: `listenIntegration failed: ${err.message}`, errorStack: err.stack }), | |
| ); | |
| } |
🤖 Prompt for AI Agents
In backend/plugins/frontline_api/src/modules/integrations/imap/messageBroker.ts
around lines 72 to 74, the code checks for updatedIntegration but then calls
listenIntegration(subdomain, integration, models), which reattaches the listener
using the stale pre-update integration document; change the call to pass the
freshly fetched updatedIntegration (i.e., listenIntegration(subdomain,
updatedIntegration, models)) and ensure updatedIntegration is non-null before
calling to avoid runtime errors.
| export async function ImapListen({ | ||
| subdomain, | ||
| data: { _id }, | ||
| }: SendImapMessageArgs) { | ||
| const models = await generateModels(subdomain); | ||
| const integration = await models.ImapIntegrations.findById(_id); | ||
| if (!integration) { | ||
| console.log(`Queue: imap:listen. Integration not found ${_id}`); | ||
| return; | ||
| } | ||
|
|
||
| listenIntegration(subdomain, integration, models); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Naming: prefer camelCase for functions
ImapListen starts with a capital letter and reads like a type/constructor. Please rename to imapListen for consistency with the rest of the module.
Run to spot call sites:
🏁 Script executed:
#!/bin/bash
# Find usages of ImapListen to update call sites if you rename it.
rg -nP --type=ts -C2 '\bImapListen\s*\('Length of output: 1044
Rename ImapListen to imapListen and update all call sites
To keep function names in camelCase and avoid confusion with constructors/types:
• In backend/plugins/frontline_api/src/modules/integrations/imap/messageBroker.ts
– Change
export async function ImapListen({ … }: SendImapMessageArgs) { … }to
export async function imapListen({ … }: SendImapMessageArgs) { … }• In backend/plugins/frontline_api/src/modules/integrations/imap/configs.ts (around line 158)
– Update the call
await ImapListen({ subdomain, data: { _id: … } });to
await imapListen({ subdomain, data: { _id: … } });• Also update any import in configs.ts (or other files) from
import { ImapListen } from './messageBroker';to
import { imapListen } from './messageBroker';This ensures consistency and prevents runtime errors due to mismatched names.
🤖 Prompt for AI Agents
In backend/plugins/frontline_api/src/modules/integrations/imap/messageBroker.ts
around lines 133 to 145, the exported function ImapListen should be renamed to
camelCase imapListen; update the function declaration/export to imapListen and
then update all call sites and imports accordingly (e.g., in
backend/plugins/frontline_api/src/modules/integrations/imap/configs.ts around
line 158 change the import to import { imapListen } from './messageBroker' and
change any calls from ImapListen(...) to imapListen(...)); ensure the file
exports and all references across the codebase use the new imapListen identifier
to avoid runtime import/name errors.
| imap.search(criteria, (err, results) => { | ||
| if (err) { | ||
| throw err; | ||
| } | ||
|
|
||
| let f: Imap.ImapFetch; | ||
|
|
||
| try { | ||
| f = imap.fetch(results, { bodies: '', struct: true }); | ||
| f.on('error', (error: any) => { | ||
| throw error; | ||
| }); | ||
| } catch (e) { | ||
| if (e.message?.includes('Nothing to fetch')) { | ||
| return resolve([]); | ||
| } | ||
| throw e; | ||
| } |
There was a problem hiding this comment.
Critical: Throwing inside async callbacks – resolve/reject the Promise instead
Inside imap.search callback and f.on('error') you throw, which escapes the Promise chain and can crash the process. Reject the Promise instead. Also, short-circuit on empty results.
- imap.search(criteria, (err, results) => {
- if (err) {
- throw err;
- }
+ imap.search(criteria, (err, results) => {
+ if (err) {
+ return reject(err);
+ }
+
+ if (!results || results.length === 0) {
+ return resolve([]);
+ }
@@
- f = imap.fetch(results, { bodies: '', struct: true });
- f.on('error', (error: any) => {
- throw error;
- });
+ f = imap.fetch(results, { bodies: '', struct: true });
+ f.on('error', (error: any) => {
+ return reject(error);
+ });
} catch (e) {
if (e.message?.includes('Nothing to fetch')) {
return resolve([]);
}
- throw e;
+ return reject(e);
}Also applies to: 63-66
🤖 Prompt for AI Agents
In backend/plugins/frontline_api/src/modules/integrations/imap/utils.ts around
lines 54 to 71, the code throws errors inside the imap.search callback and
inside the f.on('error') handler which can escape the Promise and crash the
process; change these to call the Promise's reject instead, and short-circuit
when search returns no results by resolving([]) before calling imap.fetch.
Specifically: replace "if (err) { throw err; }" with "if (err) { return
reject(err); }", add an early check "if (!results || results.length === 0) {
return resolve([]); }" before fetch, and replace "f.on('error', (error) => {
throw error; });" with "f.on('error', (error) => { return reject(error); });" so
all asynchronous errors are handled via the Promise.
| try { | ||
| const criteria: any = [ | ||
| 'UNSEEN', | ||
| ['SINCE', lastFetchDate.toISOString()], | ||
| ]; | ||
| const nextLastFetchDate = new Date(); | ||
| await saveMessages( | ||
| subdomain, | ||
| imap, | ||
| updatedIntegration, | ||
| criteria, | ||
| models, | ||
| ); | ||
| lastFetchDate = nextLastFetchDate; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Risk of missing emails with time-based SINCE filtering
Using ['SINCE', lastFetchDate.toISOString()] can skip messages when server/client clocks drift or when a message has an old Date header but arrives after you advance lastFetchDate. Prefer tracking highest UID and fetching by UID range.
Outline:
- Store
lastSeenUidper integration. - Replace the search criteria with
['UID',${lastSeenUid + 1}:*]. - Update
lastSeenUidfromuidof the last processed message. - Only rely on 'UNSEEN' if you intentionally want to ignore already-seen mail.
I can draft the UID-based implementation if you want.
🤖 Prompt for AI Agents
In backend/plugins/frontline_api/src/modules/integrations/imap/utils.ts around
lines 283 to 297, the current SINCE-based search using lastFetchDate can miss
messages; replace time-based filtering with UID-based tracking: persist a
lastSeenUid per integration (in the integration record or models), build IMAP
search criteria as ['UID', `${lastSeenUid + 1}:*`] (optionally keep 'UNSEEN'
only if desired), pass that criteria to saveMessages, and after processing
update and persist lastSeenUid to the highest UID seen; ensure lastSeenUid is
initialized (0 if none) and handle race conditions by updating lastSeenUid only
after successful saveMessages.
| const startDistributingJobs = async (subdomain: string) => { | ||
| const models = await generateModels(subdomain); | ||
| const distributeJob = async () => { | ||
| let lock; | ||
| try { | ||
| lock = await redlock.lock(`${subdomain}:imap:work_distributor`, 60000); | ||
| } catch (e) { | ||
| // 1 other pod or container is already working on job distribution | ||
| return; | ||
| } | ||
| try { | ||
| await models.ImapLogs.createLog({ | ||
| type: 'info', | ||
| message: `Distributing imap sync jobs`, | ||
| }); | ||
|
|
||
| const integrations = await models.ImapIntegrations.find({ | ||
| healthStatus: 'healthy', | ||
| }); | ||
| for (const integration of integrations) { | ||
| console.log(integrations, '&&&&&&&&&'); | ||
|
|
||
| sendImapMessage({ | ||
| subdomain, | ||
| action: 'listen', | ||
| data: { | ||
| _id: integration._id as string, | ||
| }, | ||
| }); | ||
| } | ||
| } catch (e) { | ||
| await lock.unlock(); | ||
| } | ||
| }; | ||
| // wait for other containers to start up | ||
| NODE_ENV === 'production' && | ||
| (await new Promise((resolve) => setTimeout(resolve, 60000))); | ||
|
|
||
| while (true) { | ||
| try { | ||
| await distributeJob(); | ||
| // try doing it every 10 minutes | ||
| await new Promise((resolve) => setTimeout(resolve, 10 * 60 * 1000)); | ||
| } catch (e) { | ||
| console.log('distributeWork error', e); | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
Work distributor: always unlock in finally; remove debug log; fix production delay expression
- Lock is only unlocked on error; on success it relies on TTL. Always unlock in a
finally. - Remove
console.log(integrations, '&&&&&&&&&'). NODE_ENV === 'production' && await ...triggers lint; useif.
const startDistributingJobs = async (subdomain: string) => {
const models = await generateModels(subdomain);
const distributeJob = async () => {
let lock;
try {
lock = await redlock.lock(`${subdomain}:imap:work_distributor`, 60000);
} catch (e) {
// 1 other pod or container is already working on job distribution
return;
}
- try {
+ try {
await models.ImapLogs.createLog({
type: 'info',
message: `Distributing imap sync jobs`,
});
const integrations = await models.ImapIntegrations.find({
healthStatus: 'healthy',
});
- for (const integration of integrations) {
- console.log(integrations, '&&&&&&&&&');
+ for (const integration of integrations) {
sendImapMessage({
subdomain,
action: 'listen',
data: {
_id: integration._id as string,
},
});
}
- } catch (e) {
- await lock.unlock();
- }
+ } catch (e) {
+ // log or handle error if needed
+ } finally {
+ try {
+ await lock.unlock();
+ } catch (e) {
+ // noop: lock may have expired
+ }
+ }
};
// wait for other containers to start up
- NODE_ENV === 'production' &&
- (await new Promise((resolve) => setTimeout(resolve, 60000)));
+ if (NODE_ENV === 'production') {
+ await new Promise((resolve) => setTimeout(resolve, 60000));
+ }Also applies to: 451-452
🧰 Tools
🪛 ESLint
[error] 451-452: Expected an assignment or function call and instead saw an expression.
(@typescript-eslint/no-unused-expressions)
🤖 Prompt for AI Agents
In backend/plugins/frontline_api/src/modules/integrations/imap/utils.ts around
lines 416-463 (also note the related 451-452), the distributeJob currently only
releases the redlock on error, leaves a stray console.log debug statement, and
uses a lint-triggering conditional await expression for the production delay;
change the try/catch so the lock.unlock() is invoked in a finally block
(guarding unlock with a truthy lock check), remove the console.log(integrations,
...) line entirely, and replace the short-circuit NODE_ENV === 'production' &&
await ... with an explicit if (NODE_ENV === 'production') await new
Promise(...). Ensure no other early returns skip the finally so the lock is
always released.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (6)
backend/plugins/frontline_api/src/modules/integrations/imap/messageBroker.ts (3)
68-74: Bug: listener reattached with stale pre-update doc; pass updatedIntegration and handle rejectionYou fetch
updatedIntegrationbut calllistenIntegrationwithintegration(stale). Also guard the promise.- if (updatedIntegration) { - listenIntegration(subdomain, integration, models); - } + if (updatedIntegration) { + void listenIntegration(subdomain, updatedIntegration, models).catch((err) => + models.ImapLogs.createLog({ + type: 'error', + message: `listenIntegration failed: ${err.message}`, + errorStack: err.stack, + }), + ); + }
133-145: Rename ImapListen to imapListen; it reads like a constructor and breaks camelCase conventionsAlso, this function duplicates
sendImapMessage. Prefer exporting a single entry point or aliasing.-export async function ImapListen({ +export async function imapListen({ subdomain, data: { _id }, }: SendImapMessageArgs) { const models = await generateModels(subdomain); const integration = await models.ImapIntegrations.findById(_id); if (!integration) { console.log(`Queue: imap:listen. Integration not found ${_id}`); return; } - listenIntegration(subdomain, integration, models); + void listenIntegration(subdomain, integration, models).catch((err) => + models.ImapLogs.createLog({ + type: 'error', + message: `listenIntegration failed: ${err.message}`, + errorStack: err.stack, + }), + ); }Update imports and call sites:
#!/bin/bash # Find and update all ImapListen usages to imapListen rg -nP -C2 '\bImapListen\b'
23-23: Do not fire-and-forget listenIntegration: catch errors and logIf
listenIntegrationrejects, the unhandled rejection will be lost. Wrap invoid ...catch(...)and log via ImapLogs.- listenIntegration(subdomain, integration, models); + void listenIntegration(subdomain, integration, models).catch((err) => + models.ImapLogs.createLog({ + type: 'error', + message: `listenIntegration failed: ${err.message}`, + errorStack: err.stack, + }), + );backend/plugins/frontline_api/src/modules/integrations/imap/utils.ts (3)
20-35: Flatten nested else-if in findAttachmentParts for readabilitySmall readability improvement; behavior unchanged.
- } else { - if ( - struct[i].disposition && - ['INLINE', 'ATTACHMENT'].indexOf(toUpper(struct[i].disposition.type)) > - -1 - ) { - attachments.push(struct[i]); - } - } + } else if ( + struct[i].disposition && + ['INLINE', 'ATTACHMENT'].indexOf(toUpper(struct[i].disposition.type)) > -1 + ) { + attachments.push(struct[i]); + }
281-297: Risk of skipped emails with time-based SINCE filter; consider UID-based syncRelying on
['SINCE', lastFetchDate.toISOString()]can miss messages due to clock drift or backdated Date headers. Track and fetch by UID ranges instead: storelastSeenUid, search['UID',${lastSeenUid + 1}:*], and persist the highest UID processed.I can draft the UID-based flow (schema field + logic) in a follow-up patch.
416-451: Always unlock the distributor redlock in finally; fix production delay expression
- The lock is only unlocked on error; always unlock in a
finally.- Replace
NODE_ENV === 'production' && await ...with an explicitifto satisfy lints and readability.const distributeJob = async () => { let lock; try { lock = await redlock.lock(`${subdomain}:imap:work_distributor`, 60000); } catch (e) { // 1 other pod or container is already working on job distribution return; } - try { + try { await models.ImapLogs.createLog({ type: 'info', message: `Distributing imap sync jobs`, }); @@ - } catch (e) { - await lock.unlock(); - } + } catch (e) { + // optional: log error + } finally { + try { + await lock.unlock(); + } catch { + /* noop: may be expired */ + } + } }; // wait for other containers to start up - NODE_ENV === 'production' && - (await new Promise((resolve) => setTimeout(resolve, 60000))); + if (NODE_ENV === 'production') { + await new Promise((resolve) => setTimeout(resolve, 60000)); + }
🧹 Nitpick comments (5)
backend/plugins/frontline_api/src/modules/integrations/imap/messageBroker.ts (1)
87-103: Optional: wrap multi-collection deletes in a transactionThree independent deletes risk partial cleanup if a later operation fails. If your Mongo deployment supports transactions, run these deletes in a single session/transaction.
I can draft a transaction-based version once you confirm that Mongoose is configured with a replica set and transactions are enabled.
backend/plugins/frontline_api/src/modules/integrations/imap/utils.ts (4)
110-110: Remove noisy console log or downgrade to debug loggerAvoid
console.login production code as per guidelines. If needed, log viaImapLogsor a central logger with an appropriate log level.- console.log(`======== found ${msgs.length} messages`); + // optionally: await models.ImapLogs.createLog({ type: 'info', message: `found ${msgs.length} messages` });
358-365: Silence no-empty catches with explicit comments or minimal loggingEmpty catch blocks trip ESLint and obscure failures. Add intent comments or minimal logging.
- try { - clearInterval(lockExtendInterval); - } catch {} + try { + clearInterval(lockExtendInterval); + } catch { + /* noop: interval may already be cleared */ + } @@ - try { - await lock.unlock(); - } catch {} + try { + await lock.unlock(); + } catch { + /* noop: lock may already be released/expired */ + } @@ - try { - imap.end(); - } catch {} + try { + imap.end(); + } catch { + /* noop */ + } @@ - try { - imap.removeAllListeners(); - } catch {} + try { + imap.removeAllListeners(); + } catch { + /* noop */ + }Also applies to: 371-375
452-460: Optional: add jitter/backoff for distributor loop to avoid thundering herd on repeated failuresThe perpetual loop is fine, but consider exponential backoff with jitter on errors to reduce load when Redis/Mongo are degraded.
I can add a bounded exponential backoff helper if desired.
465-479: Route error handler: avoid console.log for errorsPrefer a centralized logger or
ImapLogs.createLog-like mechanism rather thanconsole.log(e.message)to keep error observability consistent.- console.log(e.message); + // TODO: replace with centralized logger, e.g. logger.error(e) or ImapLogs.createLog
📜 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 (3)
backend/plugins/frontline_api/src/modules/integrations/imap/messageBroker.ts(1 hunks)backend/plugins/frontline_api/src/modules/integrations/imap/redlock.ts(1 hunks)backend/plugins/frontline_api/src/modules/integrations/imap/utils.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/plugins/frontline_api/src/modules/integrations/imap/redlock.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{ts,tsx}: Use functional and declarative programming patterns; avoid classes.
Use TypeScript for all code; prefer interfaces over types.
Avoid enums; use maps instead.
Files:
backend/plugins/frontline_api/src/modules/integrations/imap/messageBroker.tsbackend/plugins/frontline_api/src/modules/integrations/imap/utils.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{ts,tsx,js,jsx}: Use descriptive variable names with auxiliary verbs (e.g., isLoading, hasError).
Avoid console logs.
Always use absolute paths when importing.
Use the "function" keyword for pure functions.
Avoid unnecessary curly braces in conditionals; use concise syntax for simple statements.
**/*.{ts,tsx,js,jsx}: Use descriptive variable names with auxiliary verbs (e.g., isLoading, hasError).
Use the "function" keyword for pure functions.
Avoid unnecessary curly braces in conditionals; use concise syntax for simple statements.
Use absolute path when import.
Files:
backend/plugins/frontline_api/src/modules/integrations/imap/messageBroker.tsbackend/plugins/frontline_api/src/modules/integrations/imap/utils.ts
🧬 Code graph analysis (2)
backend/plugins/frontline_api/src/modules/integrations/imap/messageBroker.ts (1)
backend/plugins/frontline_api/src/modules/integrations/imap/utils.ts (1)
listenIntegration(245-414)
backend/plugins/frontline_api/src/modules/integrations/imap/utils.ts (7)
backend/plugins/frontline_api/src/modules/integrations/imap/models.ts (1)
IIntegrationImapDocument(276-276)backend/erxes-api-shared/src/utils/trpc/index.ts (1)
sendTRPCMessage(42-68)backend/plugins/frontline_api/src/connectionResolvers.ts (2)
IModels(121-151)generateModels(281-281)backend/plugins/frontline_api/src/modules/inbox/receiveMessage.ts (1)
receiveInboxMessage(21-227)backend/plugins/frontline_api/src/modules/inbox/graphql/resolvers/mutations/widget.ts (1)
pConversationClientMessageInserted(4-66)backend/plugins/frontline_api/src/modules/integrations/imap/redlock.ts (1)
redlock(14-14)backend/plugins/frontline_api/src/modules/integrations/imap/messageBroker.ts (1)
sendImapMessage(115-131)
🪛 ESLint
backend/plugins/frontline_api/src/modules/integrations/imap/utils.ts
[error] 75-75: 'buffers' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 361-361: Empty block statement.
(no-empty)
[error] 364-364: Empty block statement.
(no-empty)
[error] 371-371: Empty block statement.
(no-empty)
[error] 374-374: Empty block statement.
(no-empty)
[error] 449-450: Expected an assignment or function call and instead saw an expression.
(@typescript-eslint/no-unused-expressions)
🔇 Additional comments (1)
backend/plugins/frontline_api/src/modules/integrations/imap/utils.ts (1)
342-347: Verify healthStatus vocabularyYou set
healthStatus: 'unHealthy'elsewhere default is'healthy'. Confirm allowed values (e.g.,'unhealthy'vs'unHealthy') and standardize.I can normalize all call sites once you confirm the canonical enum/string values used by ImapIntegrations.
| export async function imapCreateIntegrations({ subdomain, data }) { | ||
| try { | ||
| const models = await generateModels(subdomain); | ||
|
|
||
| const integration = await models.ImapIntegrations.create({ | ||
| inboxId: data.integrationId, | ||
| healthStatus: 'healthy', | ||
| error: '', | ||
| ...JSON.parse(data), | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix unsafe JSON.parse and inconsistent payload shape in imapCreateIntegrations
JSON.parse(data) assumes data is a JSON string, but you also access data.integrationId as if data were an object. This will either throw or silently ignore fields. Parse only the document portion and sanitize allowed fields before persisting.
Apply:
- const integration = await models.ImapIntegrations.create({
- inboxId: data.integrationId,
- healthStatus: 'healthy',
- error: '',
- ...JSON.parse(data),
- });
+ const detail = typeof data.doc === 'string' ? JSON.parse(data.doc) : data.doc;
+ const allowed = (({ host, user, mainUser, password }) => ({ host, user, mainUser, password }))(detail || {});
+ const integration = await models.ImapIntegrations.create({
+ inboxId: data.integrationId,
+ healthStatus: 'healthy',
+ error: '',
+ ...allowed,
+ });Additionally, prefer a typed signature to enforce the shape at the boundary:
// add near the top of the file
type BrokerResult = { status: 'success' } | { status: 'error'; errorMessage: string };
interface ImapCreateArgs {
subdomain: string;
data: { integrationId: string; doc: string | Record<string, any> };
}
export async function imapCreateIntegrations({ subdomain, data }: ImapCreateArgs): Promise<BrokerResult> { … }🤖 Prompt for AI Agents
In backend/plugins/frontline_api/src/modules/integrations/imap/messageBroker.ts
around lines 12 to 22, the function assumes `data` is both an object and a JSON
string which is unsafe; change the function signature to accept a typed
ImapCreateArgs (subdomain: string, data: { integrationId: string; doc: string |
Record<string, any> }) and return a BrokerResult type, parse only `data.doc`
when it's a string (catch JSON errors), build a sanitized payload by
whitelisting allowed fields (e.g., subject, sender, config, etc.), merge with
explicit values inboxId: data.integrationId, healthStatus: 'healthy', error: ''
and any safe parsed fields, then call models.ImapIntegrations.create(...) with
that sanitized object and return success/error structured as BrokerResult.
| export async function imapUpdateIntegrations({ | ||
| subdomain, | ||
| data: { integrationId, doc }, | ||
| }) { | ||
| try { | ||
| const detail = JSON.parse(doc.data); | ||
| const models = await generateModels(subdomain); | ||
|
|
||
| const integration = await models.ImapIntegrations.findOne({ | ||
| inboxId: integrationId, | ||
| }); | ||
|
|
||
| if (!integration) { | ||
| return { | ||
| status: 'error', | ||
| errorMessage: 'Integration not found.', | ||
| }; | ||
| } | ||
|
|
||
| detail.healthStatus = 'healthy'; | ||
| detail.error = ''; | ||
|
|
||
| await models.ImapIntegrations.updateOne( | ||
| { inboxId: integrationId }, | ||
| { $set: detail }, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden update: robust doc parsing and whitelist fields for $set
const detail = JSON.parse(doc.data);assumesdochas a.datastring. Accept both string/object inputs.- Blindly
$set: detailallows overwriting internal fields.
- const detail = JSON.parse(doc.data);
+ const detail = typeof doc === 'string' ? JSON.parse(doc) : doc;
@@
- await models.ImapIntegrations.updateOne(
- { inboxId: integrationId },
- { $set: detail },
- );
+ const allowed = (({ host, user, mainUser, password }) => ({ host, user, mainUser, password }))(detail || {});
+ await models.ImapIntegrations.updateOne(
+ { inboxId: integrationId },
+ { $set: { ...allowed, healthStatus: 'healthy', error: '' } },
+ );Committable suggestion skipped: line range outside the PR's diff.
| export async function sendImapMessage({ | ||
| subdomain, | ||
| data: { _id }, | ||
| }: SendImapMessageArgs) { | ||
| const models = await generateModels(subdomain); | ||
|
|
||
| const integration = await models.ImapIntegrations.findById(_id); | ||
|
|
||
| if (!integration) { | ||
| console.log(`Queue: imap:listen. Integration not found ${_id}`); | ||
| return; | ||
| } | ||
|
|
||
| listenIntegration(subdomain, integration, models); | ||
|
|
||
| return { status: 'success' }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Standardize result and guard listener errors in sendImapMessage
- When integration is not found you currently return
void, which is inconsistent with the success payload. - Catch listener errors to avoid unhandled rejections.
export async function sendImapMessage({
subdomain,
data: { _id },
}: SendImapMessageArgs) {
const models = await generateModels(subdomain);
const integration = await models.ImapIntegrations.findById(_id);
if (!integration) {
console.log(`Queue: imap:listen. Integration not found ${_id}`);
- return;
+ return { status: 'error', errorMessage: `Integration not found: ${_id}` };
}
- listenIntegration(subdomain, integration, models);
+ void listenIntegration(subdomain, integration, models).catch((err) =>
+ models.ImapLogs.createLog({
+ type: 'error',
+ message: `listenIntegration failed: ${err.message}`,
+ errorStack: err.stack,
+ }),
+ );
return { status: 'success' };
}📝 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 async function sendImapMessage({ | |
| subdomain, | |
| data: { _id }, | |
| }: SendImapMessageArgs) { | |
| const models = await generateModels(subdomain); | |
| const integration = await models.ImapIntegrations.findById(_id); | |
| if (!integration) { | |
| console.log(`Queue: imap:listen. Integration not found ${_id}`); | |
| return; | |
| } | |
| listenIntegration(subdomain, integration, models); | |
| return { status: 'success' }; | |
| } | |
| export async function sendImapMessage({ | |
| subdomain, | |
| data: { _id }, | |
| }: SendImapMessageArgs) { | |
| const models = await generateModels(subdomain); | |
| const integration = await models.ImapIntegrations.findById(_id); | |
| if (!integration) { | |
| console.log(`Queue: imap:listen. Integration not found ${_id}`); | |
| return { status: 'error', errorMessage: `Integration not found: ${_id}` }; | |
| } | |
| void listenIntegration(subdomain, integration, models).catch((err) => | |
| models.ImapLogs.createLog({ | |
| type: 'error', | |
| message: `listenIntegration failed: ${err.message}`, | |
| errorStack: err.stack, | |
| }), | |
| ); | |
| return { status: 'success' }; | |
| } |
🤖 Prompt for AI Agents
In backend/plugins/frontline_api/src/modules/integrations/imap/messageBroker.ts
around lines 115 to 131, the function returns void when the integration is not
found and does not guard against errors thrown by listenIntegration; update the
not-found branch to return a consistent payload (e.g., { status: 'not_found',
id: _id }) instead of void, and wrap the listenIntegration call in a try/catch
that logs the error and returns an error payload (e.g., { status: 'error',
error: err.message }) to avoid unhandled rejections and keep return shapes
consistent with the success case.
| const searchMessages = (imap: Imap, criteria) => { | ||
| return new Promise((resolve, reject) => { | ||
| const messages: string[] = []; | ||
|
|
||
| imap.search(criteria, (err, results) => { | ||
| if (err) { | ||
| throw err; | ||
| } | ||
|
|
||
| let f: Imap.ImapFetch; | ||
|
|
||
| try { | ||
| f = imap.fetch(results, { bodies: '', struct: true }); | ||
| f.on('error', (error: any) => { | ||
| throw error; | ||
| }); | ||
| } catch (e) { | ||
| if (e.message?.includes('Nothing to fetch')) { | ||
| return resolve([]); | ||
| } | ||
| throw e; | ||
| } |
There was a problem hiding this comment.
Do not throw inside async callbacks; reject the promise instead and short-circuit on empty results
Current throws inside imap.search and f.on('error') can escape the Promise chain and crash the process. Also handle empty results and fix lint on buffers.
return new Promise((resolve, reject) => {
@@
- imap.search(criteria, (err, results) => {
- if (err) {
- throw err;
- }
+ imap.search(criteria, (err, results) => {
+ if (err) {
+ return reject(err);
+ }
+ if (!results || results.length === 0) {
+ return resolve([]);
+ }
@@
- try {
- f = imap.fetch(results, { bodies: '', struct: true });
- f.on('error', (error: any) => {
- throw error;
- });
- } catch (e) {
+ try {
+ f = imap.fetch(results, { bodies: '', struct: true });
+ f.on('error', (error: any) => {
+ return reject(error);
+ });
+ } catch (e) {
if (e.message?.includes('Nothing to fetch')) {
return resolve([]);
}
- throw e;
+ return reject(e);
}
@@
- msg.on('body', async (stream) => {
- let buffers: Buffer[] = [];
+ msg.on('body', async (stream) => {
+ const buffers: Buffer[] = [];Also applies to: 73-99, 75-83
🤖 Prompt for AI Agents
In backend/plugins/frontline_api/src/modules/integrations/imap/utils.ts around
lines 50 to 71 (and similarly at 73-99 and 75-83), the code throws errors inside
async callbacks which can escape the Promise; change all throw statements inside
imap.search callback and f.on('error') to reject(err) to properly propagate
errors, short-circuit and resolve([]) when results is empty before calling
imap.fetch (avoid calling fetch with empty results), and replace any use of
non-typed or lint-problematic `buffers` with the correctly typed variable or
cast (e.g., Buffer instances) to satisfy linting; ensure the Promise
resolves/rejects exactly once and remove any uncaught throws in these callbacks.
| if ( | ||
| msg.to && | ||
| msg.to.value && | ||
| msg.to.value[0] && | ||
| msg.to.value[0].address !== integration.user | ||
| ) { | ||
| continue; | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Strengthen recipient filter to support aliases and missing fields
- Guard against missing
msg.to/value. - Compare against
integration.mainUser || integration.user.
- if (
- msg.to &&
- msg.to.value &&
- msg.to.value[0] &&
- msg.to.value[0].address !== integration.user
- ) {
- continue;
- }
+ const toAddress = msg?.to?.value?.[0]?.address;
+ const targetUser = integration.mainUser || integration.user;
+ if (toAddress && targetUser && toAddress !== targetUser) {
+ continue;
+ }📝 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 ( | |
| msg.to && | |
| msg.to.value && | |
| msg.to.value[0] && | |
| msg.to.value[0].address !== integration.user | |
| ) { | |
| continue; | |
| } | |
| // strengthen recipient filter: handle missing fields and support aliases | |
| const toAddress = msg?.to?.value?.[0]?.address; | |
| const targetUser = integration.mainUser || integration.user; | |
| if (toAddress && targetUser && toAddress !== targetUser) { | |
| continue; | |
| } |
🤖 Prompt for AI Agents
In backend/plugins/frontline_api/src/modules/integrations/imap/utils.ts around
lines 113 to 121, the recipient check assumes msg.to and msg.to.value[0].address
exist and only compares the first address to integration.user; update it to
first ensure msg.to and msg.to.value are present, then iterate all entries in
msg.to.value (guarding each item's address) and only accept if any address
equals (integration.mainUser || integration.user); otherwise continue. Ensure
null/undefined checks before accessing properties.
| const conversationMessage = await models.ImapMessages.create({ | ||
| inboxIntegrationId: integration.inboxId, | ||
| inboxConversationId: conversationId, | ||
| createdAt: msg.date, | ||
| messageId: msg.messageId, | ||
| inReplyTo: msg.inReplyTo, | ||
| references: msg.references, | ||
| subject: msg.subject, | ||
| body: msg.html, | ||
| to: msg.to && msg.to.value, | ||
| cc: msg.cc && msg.cc.value, | ||
| bcc: msg.bcc && msg.bcc.value, | ||
| from: msg.from && msg.from.value, | ||
| attachments: msg.attachments.map(({ filename, contentType, size }) => ({ | ||
| filename, | ||
| type: contentType, | ||
| size, | ||
| })), | ||
| type: 'INBOX', | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard attachments and content fields to avoid runtime errors
msg.attachments can be undefined; msg.html may be missing. Provide safe fallbacks.
- const conversationMessage = await models.ImapMessages.create({
+ const conversationMessage = await models.ImapMessages.create({
inboxIntegrationId: integration.inboxId,
inboxConversationId: conversationId,
createdAt: msg.date,
messageId: msg.messageId,
inReplyTo: msg.inReplyTo,
references: msg.references,
subject: msg.subject,
- body: msg.html,
+ body: msg.html || msg.textAsHtml || msg.text || '',
to: msg.to && msg.to.value,
cc: msg.cc && msg.cc.value,
bcc: msg.bcc && msg.bcc.value,
from: msg.from && msg.from.value,
- attachments: msg.attachments.map(({ filename, contentType, size }) => ({
- filename,
- type: contentType,
- size,
- })),
+ attachments: (msg.attachments || []).map(({ filename, contentType, size }) => ({
+ filename,
+ type: contentType,
+ size,
+ })),
type: 'INBOX',
});📝 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 conversationMessage = await models.ImapMessages.create({ | |
| inboxIntegrationId: integration.inboxId, | |
| inboxConversationId: conversationId, | |
| createdAt: msg.date, | |
| messageId: msg.messageId, | |
| inReplyTo: msg.inReplyTo, | |
| references: msg.references, | |
| subject: msg.subject, | |
| body: msg.html, | |
| to: msg.to && msg.to.value, | |
| cc: msg.cc && msg.cc.value, | |
| bcc: msg.bcc && msg.bcc.value, | |
| from: msg.from && msg.from.value, | |
| attachments: msg.attachments.map(({ filename, contentType, size }) => ({ | |
| filename, | |
| type: contentType, | |
| size, | |
| })), | |
| type: 'INBOX', | |
| }); | |
| const conversationMessage = await models.ImapMessages.create({ | |
| inboxIntegrationId: integration.inboxId, | |
| inboxConversationId: conversationId, | |
| createdAt: msg.date, | |
| messageId: msg.messageId, | |
| inReplyTo: msg.inReplyTo, | |
| references: msg.references, | |
| subject: msg.subject, | |
| body: msg.html || msg.textAsHtml || msg.text || '', | |
| to: msg.to && msg.to.value, | |
| cc: msg.cc && msg.cc.value, | |
| bcc: msg.bcc && msg.bcc.value, | |
| from: msg.from && msg.from.value, | |
| attachments: (msg.attachments || []).map(({ filename, contentType, size }) => ({ | |
| filename, | |
| type: contentType, | |
| size, | |
| })), | |
| type: 'INBOX', | |
| }); |
🤖 Prompt for AI Agents
In backend/plugins/frontline_api/src/modules/integrations/imap/utils.ts around
lines 216 to 235, the create call assumes msg.attachments and msg.html always
exist which can cause runtime errors; update the fields to use safe fallbacks:
use msg.html || msg.text || '' for body, default attachments to an empty array
before mapping (e.g., (msg.attachments || [])), and ensure any other optional
arrays/objects (to/cc/bcc/from) are safely accessed (e.g., msg.to?.value or the
existing conditional checks) so creation never tries to map or read properties
of undefined.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
backend/plugins/frontline_api/src/modules/integrations/imap/models.ts (4)
100-106: Fix return type mismatch and await the DB write to avoid unhandled rejectionsThe interface declares
createSendMailreturnsPromise<IMessageImapDocument>, but the implementation returns{ info }and does not awaitmodels.ImapMessages.create(...). This is both a type mismatch and a potential unhandled promise rejection if the insert fails after sending the email.Apply the following diff to:
- Align the return type.
- Await the create call and return both the SMTP info and the saved message document.
- Add a minimal SMTPTransport type import.
@@ -import * as nodemailer from 'nodemailer'; +import * as nodemailer from 'nodemailer'; +import type SMTPTransport from 'nodemailer/lib/smtp-transport'; @@ export interface IMessageImapModel extends Model<IMessageImapDocument> { createSendMail( args: any, subdomain: string, models: IModels, - ): Promise<IMessageImapDocument>; + ): Promise<{ info: SMTPTransport.SentMessageInfo; message: IMessageImapDocument }>; } @@ - const info = await transporter.sendMail(mailData); - - models.ImapMessages.create({ + const info = await transporter.sendMail(mailData); + const message = await models.ImapMessages.create({ inboxIntegrationId: integration.inboxId, inboxConversationId: conversationId, createdAt: new Date(), messageId: info.messageId, inReplyTo: replyToMessageId, - references: mailData.references, + references: mailData.references ?? [], subject: mailData.subject, body: mailData.html, - to: (mailData.to || []).map((to) => ({ name: to, address: to })), + to: (Array.isArray(mailData.to) ? mailData.to : [mailData.to].filter(Boolean)).map( + (addr) => ({ name: String(addr), address: String(addr) }) + ), - from: [{ name: mailData.from, address: mailData.from }], + from: [{ name: String(mailData.from), address: String(mailData.from) }], attachments: attachments ? attachments.map(({ name, type, size }) => ({ filename: name, type, size, })) : [], type: 'SENT', }); - return { - info: info, - }; + return { info, message };Also applies to: 236-259
96-98: Use Date.now for defaults; new Date() freezes the default at schema creation time
default: new Date()captures a single timestamp when the process starts. New documents will all receive the same date. UseDate.now.- createdAt: { type: Date, index: true, default: new Date() }, + createdAt: { type: Date, index: true, default: Date.now },
219-232: Normalize 'to' to an array before using; current mapping assumes an array
mailData.toderives fromtoand may be a string or array. The subsequent.map(...)assumes an array and will throw if a string is passed.- const mailData = { - from, - to, + const toList = Array.isArray(to) ? to : (to ? [to] : []); + const mailData = { + from, + to: toList, subject: replyToMessageId ? `Re: ${subject}` : subject, html: body, inReplyTo: replyToMessageId, - references: [replyToMessageId], + references: replyToMessageId ? [replyToMessageId] : undefined, attachments: attachments ? attachments.map((attach) => ({ filename: attach.name, path: attach.url, })) : [], // Default to an empty array if attachments is undefined }; @@ - to: (mailData.to || []).map((to) => ({ name: to, address: to })), + to: mailData.to.map((addr) => ({ name: String(addr), address: String(addr) })),Also applies to: 245-247
270-271: Type smtpPort as a number in the interface and schemaNodemailer expects a number for
port. Persisting it asStringinvites accidental misconfiguration.export interface IIntegrationImap { inboxId: string; host: string; smtpHost: string; - smtpPort: string; + smtpPort: number; @@ export const integrationImapSchema = new Schema({ inboxId: String, host: String, smtpHost: String, - smtpPort: String, + smtpPort: Number,Migration hint: if existing documents store
smtpPortas strings, add a one-off migration to coerce them to numbers.Also applies to: 286-287
🧹 Nitpick comments (8)
backend/plugins/frontline_api/src/modules/integrations/imap/models.ts (8)
219-226: Avoid setting 'references' when replyTo is absent; guard and keep headers clean
references: [replyToMessageId]yields[undefined]whenreplyToMessageIdis falsy, which may produce malformed headers.- inReplyTo: replyToMessageId, - references: [replyToMessageId], + inReplyTo: replyToMessageId, + references: replyToMessageId ? [replyToMessageId] : undefined,
226-231: Mitigate SSRF risk when using attachment URLsPassing arbitrary URLs directly as
pathallows the server to fetch attacker-controlled endpoints. Either pre-fetch and passcontent/buffer, or restrict protocols/hosts.Example hardening:
- attachments: attachments - ? attachments.map((attach) => ({ - filename: attach.name, - path: attach.url, - })) - : [], + attachments: attachments + ? attachments + .filter((a) => typeof a.url === 'string' && /^https?:\/\//i.test(a.url)) + .map((a) => ({ filename: a.name, path: a.url })) + : [],Alternatively, download via your own
/read-mail-attachmentroute on the same origin and passcontentas a Buffer, avoiding outbound fetches entirely.If you prefer host allowlisting, I can generate a patch wired to your config to enforce it.
192-205: Make status updates mutually exclusive and explicitIf both
shouldResolveandshouldOpenare true, the second update overwrites the first. Choose priority or enforce exclusivity, and use$setexplicitly.- if (conversationId) { - if (shouldResolve) { - await models.Conversations.updateOne( - { _id: conversationId }, - { status: 'closed' }, - ); - } - if (shouldOpen) { - await models.Conversations.updateOne( - { _id: conversationId }, - { status: 'new' }, - ); - } - } + if (conversationId) { + const nextStatus = shouldResolve ? 'closed' : (shouldOpen ? 'new' : undefined); + if (nextStatus) { + await models.Conversations.updateOne( + { _id: conversationId }, + { $set: { status: nextStatus } }, + ); + } + }
207-217: Disable verbose SMTP logging in production
logger: trueanddebug: truecan leak PII and credentials to logs. Guard with environment checks or your centralized logger.- const transporter = nodemailer.createTransport({ + const transporter = nodemailer.createTransport({ host: integration.smtpHost, - port: integration.smtpPort, + port: Number(integration.smtpPort), secure: true, - logger: true, - debug: true, + logger: process.env.NODE_ENV !== 'production', + debug: process.env.NODE_ENV !== 'production', auth: { user: integration.mainUser || integration.user, pass: integration.password, }, });Note: casting port to
Numberalso guards againstsmtpPortbeing persisted as a string.
226-254: Preserve mimeType on stored attachmentsThe schema defines both
mimeTypeandtype, yet the persistence only setstype. IfmimeTypeis the canonical field, capture it.- attachments: attachments - ? attachments.map(({ name, type, size }) => ({ - filename: name, - type, - size, - })) + attachments: attachments + ? attachments.map(({ name, type, size, mimeType }) => ({ + filename: name, + type, + mimeType: mimeType ?? type, + size, + })) : [],
333-334: Remove unused placeholder export
export const CreateSendMail = (Model) => {};is dead code and can confuse readers.-export const CreateSendMail = (Model) => {};
310-311: Use primitivestringinstead ofStringin interfacesPrefer
stringfor TypeScript types.- errorStack?: String; + errorStack?: string;
108-116: Prefer schema.statics over classes per code style guidelinesYour guidelines ask to avoid classes in TS/JS. You can attach statics without
loadClass.Example (for Message only):
-export const loadImapMessageClass = (models) => { - class Message { - public static async createSendMail( /* ... */ ) { /* ... */ } - } - messageImapSchema.loadClass(Message); - return messageImapSchema; -}; +export const loadImapMessageClass = (models) => { + messageImapSchema.statics.createSendMail = async function (/* ... */) { + // implementation body unchanged + }; + return messageImapSchema; +};If you want, I can send a full patch converting all three loaders (
Customer,Message,Integration) to statics.Also applies to: 299-304
📜 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 (1)
backend/plugins/frontline_api/src/modules/integrations/imap/models.ts(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{ts,tsx}: Use functional and declarative programming patterns; avoid classes.
Use TypeScript for all code; prefer interfaces over types.
Avoid enums; use maps instead.
Files:
backend/plugins/frontline_api/src/modules/integrations/imap/models.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{ts,tsx,js,jsx}: Use descriptive variable names with auxiliary verbs (e.g., isLoading, hasError).
Avoid console logs.
Always use absolute paths when importing.
Use the "function" keyword for pure functions.
Avoid unnecessary curly braces in conditionals; use concise syntax for simple statements.
**/*.{ts,tsx,js,jsx}: Use descriptive variable names with auxiliary verbs (e.g., isLoading, hasError).
Use the "function" keyword for pure functions.
Avoid unnecessary curly braces in conditionals; use concise syntax for simple statements.
Use absolute path when import.
Files:
backend/plugins/frontline_api/src/modules/integrations/imap/models.ts
🧬 Code graph analysis (1)
backend/plugins/frontline_api/src/modules/integrations/imap/models.ts (2)
backend/erxes-api-shared/src/utils/trpc/index.ts (1)
sendTRPCMessage(42-68)backend/plugins/frontline_api/src/modules/inbox/graphql/resolvers/customResolvers/conversation.ts (1)
integration(26-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
backend/plugins/frontline_api/src/modules/integrations/imap/models.ts (2)
164-174: Verify integration lookup joins; conversation.integrationId vs ImapIntegrations.inboxIdThe code first searches
models.Integrationsby{ user: from }or{ inboxId: integrationId }, then, if still missing andconversationIdexists, it loadsConversationsand queriesmodels.ImapIntegrationsby{ inboxId: conversation.integrationId }. In the inbox resolver,conversation.integrationIdpoints tomodels.Integrations._id(not ImapIntegrations.inboxId). Confirm whetherImapIntegrations.inboxIdis indeed the Integrations_id. If not, this may fail to resolve the correct IMAP integration.I can scan the repo for the
ImapIntegrationsschema usage to confirm the expected join keys and propose a unified lookup function if you’d like.Also applies to: 176-186
135-143: Ensure tenant isolation by passing the subdomain header in TRPC callsWe need to explicitly scope these
sendTRPCMessagecalls to the correct tenant—otherwise they may query or create data in the wrong tenant. I didn’t find any existing uses ofoptions.headersonsendTRPCMessageelsewhere in the repo, so you’ll need to choose the canonical header name for your setup (for example,x-subdomainorerxes-subdomain).Please update both blocks in backend/plugins/frontline_api/src/modules/integrations/imap/models.ts (around lines 135–143 and 148–159) as follows:
customer = await sendTRPCMessage({ pluginName: 'core', method: 'query', module: 'customers', action: 'findOne', input: { selector, }, + options: { headers: { 'x-subdomain': subdomain } }, }); … customer = await sendTRPCMessage({ pluginName: 'core', method: 'mutation', module: 'customers', action: 'createCustomer', input: { doc: { state: 'lead', primaryEmail, }, }, + options: { headers: { 'x-subdomain': subdomain } }, });– Verify that this header key matches the one your API gateway or core service expects.
Summary by Sourcery
Add a full-featured IMAP plugin to the API, including utilities for connecting to and syncing emails, REST route for downloading attachments, background job distribution, message broker handlers, and GraphQL integration management.
New Features:
Enhancements:
Build:
Important
Add IMAP plugin for email integration with new endpoints, job distribution, and improved error handling.
messageBroker.ts./read-mail-attachmentendpoint inconfigs.tsfor downloading email attachments.configs.ts.utils.ts.utils.ts.integrations.tsto support IMAP operations.utils.ts.dotenv,node-imap,mailparser,base64-stream,ioredis,redlockinpackage.json.This description was created by
for 73ef46f. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
Improvements
Chores