Repository navigation
Implement direct email sending with fallback and streamline tests - #18
Conversation
…d environment variables for concurrency and cooldown
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR updates the email delivery flow to prefer direct SMTP sending (with throttling and failover) while keeping queue-based delivery as a fallback, and adjusts env validation and tests accordingly.
Changes:
- Add direct-send gating (concurrency limit + failover cooldown) and refactor queueing into a helper.
- Extend environment schema with new email delivery tuning variables.
- Update email queue tests and adjust Next.js route types import path.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| tests/email-queue.test.ts | Updates tests to cover direct-send and queueing behavior with mocked nodemailer. |
| next-env.d.ts | Changes the generated routes types import path. |
| lib/env.ts | Adds validation/defaults for new email delivery env vars. |
| lib/email.ts | Adds direct SMTP sending preference with concurrency/cooldown and shared queueing helper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async function queueEmailJob(job: EmailQueueJob): Promise<boolean> { | ||
| if (isRabbitMqConfigured()) { | ||
| try { | ||
| const enqueued = await publishRabbitMqMessage(EMAIL_QUEUE_NAME, { job, attempts: 0 }); | ||
| if (enqueued) return true; | ||
| } catch (error) { | ||
| console.error('RabbitMQ enqueue failed, falling back to Redis', error); | ||
| } | ||
| } | ||
|
|
||
| const redis = getRedis(); | ||
| if (!redis) return false; | ||
|
|
||
| await redis.rpush('email:queue', JSON.stringify({ job, attempts: 0 })); | ||
| return true; | ||
| } |
| const redis = getRedis(); | ||
| if (!redis) { | ||
| // fallback to immediate send | ||
| const queued = await queueEmailJob({ job, attempts: 0 }); |
| const canSendDirectly = hasSmtpConfiguration() && Date.now() >= directEmailFailoverUntil && activeDirectEmailSends < env.EMAIL_DIRECT_CONCURRENCY_LIMIT; | ||
|
|
||
| if (canSendDirectly) { | ||
| activeDirectEmailSends += 1; |
| it('sends directly when SMTP is available and the queue is enabled', async () => { | ||
| Object.assign(process.env, baseEnv); | ||
|
|
||
| vi.resetModules(); | ||
| const { queueEmail } = await import('../lib/email'); | ||
|
|
||
| await queueEmail('test-template', '[email protected]', 'Subject', { name: 'NexDrop' }); | ||
|
|
||
| expect(createTransportMock).toHaveBeenCalledTimes(1); | ||
| expect(sendMailMock).toHaveBeenCalledTimes(1); | ||
| expect(publishMock).not.toHaveBeenCalled(); | ||
| expect(rpushMock).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('queues when direct delivery fails', async () => { | ||
| Object.assign(process.env, baseEnv); | ||
|
|
||
| sendMailMock.mockRejectedValueOnce(new Error('smtp unavailable')); | ||
|
|
||
| vi.resetModules(); | ||
| const { queueEmail } = await import('../lib/email'); | ||
|
|
||
| await queueEmail('test-template', '[email protected]', 'Subject', { name: 'NexDrop' }); | ||
|
|
||
| expect(sendMailMock).toHaveBeenCalledTimes(1); | ||
| expect(publishMock).toHaveBeenCalledTimes(1); | ||
| expect(rpushMock).toHaveBeenCalledTimes(1); | ||
| expect(rpushMock).toHaveBeenCalledWith( | ||
| 'email:queue', | ||
| expect.stringContaining('[email protected]') | ||
| ); | ||
| expect(rpushMock).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
Code Review
This pull request introduces a direct-send optimization for emails when SMTP is configured, with fallback mechanisms to RabbitMQ or Redis queues when direct delivery fails or concurrency limits are exceeded. The review identified a critical bug in queueEmailJob where the email job payload is double-wrapped, which would cause worker failures. Additionally, there is a recommendation to restore test coverage for the Redis fallback mechanism when RabbitMQ publishing fails.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| async function queueEmailJob(job: EmailQueueJob): Promise<boolean> { | ||
| if (isRabbitMqConfigured()) { | ||
| try { | ||
| const enqueued = await publishRabbitMqMessage(EMAIL_QUEUE_NAME, { job, attempts: 0 }); | ||
| if (enqueued) return true; | ||
| } catch (error) { | ||
| console.error('RabbitMQ enqueue failed, falling back to Redis', error); | ||
| } | ||
| } | ||
|
|
||
| const redis = getRedis(); | ||
| if (!redis) return false; | ||
|
|
||
| await redis.rpush('email:queue', JSON.stringify({ job, attempts: 0 })); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
There is a critical bug where the email job payload is double-wrapped when enqueued.
The queueEmailJob function receives a parameter job of type EmailQueueJob (which already has the structure { job: Mail, attempts: number }). By wrapping it again as { job, attempts: 0 } when publishing to RabbitMQ or pushing to Redis, the final payload structure becomes { job: { job: Mail, attempts: number }, attempts: 0 }.
When the email worker processes this job, it expects payload.job to be of type Mail, but it will instead receive the nested { job: Mail, attempts: number } object. This will cause sendEmail to receive undefined for to, subject, and html, failing to send the email.
We should pass the job parameter directly without re-wrapping it.
| async function queueEmailJob(job: EmailQueueJob): Promise<boolean> { | |
| if (isRabbitMqConfigured()) { | |
| try { | |
| const enqueued = await publishRabbitMqMessage(EMAIL_QUEUE_NAME, { job, attempts: 0 }); | |
| if (enqueued) return true; | |
| } catch (error) { | |
| console.error('RabbitMQ enqueue failed, falling back to Redis', error); | |
| } | |
| } | |
| const redis = getRedis(); | |
| if (!redis) return false; | |
| await redis.rpush('email:queue', JSON.stringify({ job, attempts: 0 })); | |
| return true; | |
| } | |
| async function queueEmailJob(job: EmailQueueJob): Promise<boolean> { | |
| if (isRabbitMqConfigured()) { | |
| try { | |
| const enqueued = await publishRabbitMqMessage(EMAIL_QUEUE_NAME, job); | |
| if (enqueued) return true; | |
| } catch (error) { | |
| console.error('RabbitMQ enqueue failed, falling back to Redis', error); | |
| } | |
| } | |
| const redis = getRedis(); | |
| if (!redis) return false; | |
| await redis.rpush('email:queue', JSON.stringify(job)); | |
| return true; | |
| } |
| it('queues when direct delivery fails', async () => { | ||
| Object.assign(process.env, baseEnv); | ||
|
|
||
| sendMailMock.mockRejectedValueOnce(new Error('smtp unavailable')); | ||
|
|
||
| vi.resetModules(); | ||
| const { queueEmail } = await import('../lib/email'); | ||
|
|
||
| await queueEmail('test-template', '[email protected]', 'Subject', { name: 'NexDrop' }); | ||
|
|
||
| expect(sendMailMock).toHaveBeenCalledTimes(1); | ||
| expect(publishMock).toHaveBeenCalledTimes(1); | ||
| expect(rpushMock).toHaveBeenCalledTimes(1); | ||
| expect(rpushMock).toHaveBeenCalledWith( | ||
| 'email:queue', | ||
| expect.stringContaining('[email protected]') | ||
| ); | ||
| expect(rpushMock).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
The test suite no longer verifies the fallback behavior to Redis when RabbitMQ publishing fails. This is a regression in test coverage compared to the previous version of the tests.
We should add a test case to ensure that if both direct SMTP delivery and RabbitMQ publishing fail, the system correctly falls back to Redis queueing.
it('queues when direct delivery fails', async () => {
Object.assign(process.env, baseEnv);
sendMailMock.mockRejectedValueOnce(new Error('smtp unavailable'));
vi.resetModules();
const { queueEmail } = await import('../lib/email');
await queueEmail('test-template', '[email protected]', 'Subject', { name: 'NexDrop' });
expect(sendMailMock).toHaveBeenCalledTimes(1);
expect(publishMock).toHaveBeenCalledTimes(1);
expect(rpushMock).not.toHaveBeenCalled();
});
it('falls back to Redis when RabbitMQ publish fails', async () => {
Object.assign(process.env, baseEnv);
sendMailMock.mockRejectedValueOnce(new Error('smtp unavailable'));
publishMock.mockRejectedValueOnce(new Error('rabbit unavailable'));
vi.resetModules();
const { queueEmail } = await import('../lib/email');
await queueEmail('test-template', '[email protected]', 'Subject', { name: 'NexDrop' });
expect(sendMailMock).toHaveBeenCalledTimes(1);
expect(publishMock).toHaveBeenCalledTimes(1);
expect(rpushMock).toHaveBeenCalledTimes(1);
});
This pull request introduces significant improvements to the email delivery and queueing logic, focusing on robust direct SMTP delivery with concurrency limits, intelligent failover to queue backends (RabbitMQ/Redis), and enhanced test coverage for these behaviors. The changes ensure more reliable email handling and clearer fallback mechanisms when direct delivery fails.
Email delivery and queueing improvements:
hasSmtpConfigurationand logic to send emails directly via SMTP if configuration is available, within a configurable concurrency limit (EMAIL_DIRECT_CONCURRENCY_LIMIT). If direct delivery fails, the system now enters a cooldown period (EMAIL_QUEUE_FAILOVER_COOLDOWN_MS) before attempting direct sends again. If direct delivery is not possible, jobs are enqueued to RabbitMQ or Redis as available. [1] [2]queueEmailJobfunction to abstract queueing logic, prioritizing RabbitMQ and falling back to Redis if needed.Configuration enhancements:
EMAIL_DIRECT_CONCURRENCY_LIMITandEMAIL_QUEUE_FAILOVER_COOLDOWN_MSto control direct send concurrency and cooldown behavior, with validation in the environment schema.Testing improvements:
email-queue.test.tsto mock SMTP, RabbitMQ, and Redis, verifying direct send, queue fallback, and error scenarios. Tests now ensure that direct delivery is preferred when possible and that queueing occurs only when necessary. [1] [2] [3]