Repository navigation
chore(api): enable vitest, refactor nodemailer, and improve coverage - #35
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughReplaces Jest with Vitest and adds SWC/Vitest configs; updates CI and Node to 20; refactors email module to inject a Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant EmailModule as Email Module
participant Config as ConfigService
participant TransportProv as MAILER_TRANSPORTER Provider
participant ConsoleSvc as ConsoleEmailService
participant NodemailerSvc as NodemailerService
participant Transporter as nodemailer.Transporter
Note over TransportProv: Factory reads MAIL_MOCK / SMTP_* and returns transporter or null
Caller->>EmailModule: sendEmail(options)
EmailModule->>TransportProv: request transporter
TransportProv-->>EmailModule: transporter or null
alt transporter == null
EmailModule->>ConsoleSvc: delegate sendEmail(options)
ConsoleSvc-->>Caller: logs/ack
else transporter available
EmailModule->>NodemailerSvc: delegate sendEmail(options)
NodemailerSvc->>Transporter: transporter.sendMail(payload)
Transporter-->>NodemailerSvc: resolve / reject
NodemailerSvc-->>EmailModule: result / logs error
EmailModule-->>Caller: success or logged failure
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/api/src/modules/identity/auth/auth.controller.spec.ts (2)
1-9: Missing explicit import ofvifrom Vitest.This file uses
vi.fn()andvi.clearAllMocks()without explicitly importingvifromvitest. In contrast,users.controller.spec.ts(line 9) explicitly imports{ vi }fromvitest.This works only if Vitest globals are enabled in the config. For consistency across the codebase and to avoid potential issues if globals configuration changes, consider adding the explicit import.
♻️ Proposed fix to add explicit import
import { Test, TestingModule } from '@nestjs/testing'; import { AuthController } from './auth.controller'; import { AuthService } from './auth.service'; import { USER_PROVIDER, TENANT_PROVIDER } from '@nexiom/identity'; import { InvitationsService } from '../invitations/invitations.service'; import { Request, Response } from 'express'; import { CompleteInvite } from '../users/users.validation'; +import { describe, it, expect, beforeEach, vi } from 'vitest';
263-288: Address the skipped test.The
betterAuthtest is marked as skipped (it.skip). Skipped tests can accumulate and degrade test suite reliability over time. Consider either:
- Fixing and re-enabling the test
- Removing it if no longer relevant
- Adding a TODO comment explaining why it's skipped and when it should be re-enabled
apps/api/src/modules/email/nodemailer.service.ts (1)
32-35: Silent error handling may hide email delivery failures.The catch block logs the error but doesn't re-throw or return a failure indicator. Callers of
sendEmailhave no way to know if the email was actually sent. This could mask critical issues like invitation emails not being delivered.Consider either:
- Re-throwing the error to let callers handle it
- Returning a boolean/result object indicating success/failure
- At minimum, make this a configurable behavior
🛠️ Option 1: Re-throw the error
} catch (error) { this.logger.error(`Failed to send email to ${options.to}`, error); - // Don't throw, just log. Or throw if you want to fail the action. + throw error; }🛠️ Option 2: Return success indicator
- async sendEmail(options: SendEmailOptions): Promise<void> { + async sendEmail(options: SendEmailOptions): Promise<boolean> { this.logger.log(`Sending email to ${options.to} via Nodemailer...`); try { const from = this.configService.get<string>( 'SMTP_FROM', '"No Reply" <noreply@example.com>', ); await this.transporter.sendMail({ from, to: options.to, subject: options.subject, text: options.text, html: options.html, }); this.logger.log(`Email sent successfully to ${options.to}`); + return true; } catch (error) { this.logger.error(`Failed to send email to ${options.to}`, error); - // Don't throw, just log. Or throw if you want to fail the action. + return false; } }
🤖 Fix all issues with AI agents
In `@apps/api/package.json`:
- Around line 64-65: The package.json currently lists both coverage providers
"@vitest/coverage-istanbul" and "@vitest/coverage-v8"; remove the unused one
(either delete the "@vitest/coverage-istanbul" or "@vitest/coverage-v8" entry)
or, if both are intentionally kept, add a brief comment in package.json or the
repo README explaining the rationale and when each provider should be used;
update any CI/test scripts (e.g., vitest config) to reference the chosen
provider (look for vitest config or scripts referencing coverage provider names)
so the dependencies and configuration remain consistent.
- Line 25: Update the "test:debug" npm script to remove the deprecated Vitest
flag and use the new pool option: replace the deprecated "--threads=false" with
"--pool=forks" in the "test:debug" script so the entry "test:debug" uses "vitest
--inspect-brk --inspect --logHeapUsage --pool=forks".
In `@apps/api/src/app.service.spec.ts`:
- Line 3: The test file imports Vitest globals (describe, it, expect,
beforeEach) explicitly which is redundant if vitest.config.ts enables globals:
true; either remove these imports from apps/api/src/app.service.spec.ts to rely
on global vitest bindings or keep them for explicitness—decide based on project
convention and update the file to remove the import line or keep as-is to match
your vitest.config.ts globals setting (verify globals in vitest.config.ts and
adjust accordingly).
In `@apps/api/src/modules/email/email.module.ts`:
- Around line 14-27: The MAILER_TRANSPORTER provider is always created even when
MAIL_MOCK=true; change the provider factory in email.module.ts to first read
configService.get<boolean>('MAIL_MOCK', false) and only create
nodemailer.createTransport(...) when MAIL_MOCK is false (otherwise return
null/undefined or don't register the transporter), and update the EmailService
factory/constructor (the factory that injects MAILER_TRANSPORTER and the
ConsoleEmailService fallback) to accept and handle a nullable transporter (or
branch to ConsoleEmailService when transporter is null) so startup won’t attempt
to connect to SMTP when mocking is enabled.
In `@apps/api/src/modules/email/nodemailer.service.spec.ts`:
- Around line 66-84: The test creates a spy on Logger.prototype.error (logSpy)
but never restores it, which can leak into other tests; update the spec to
restore the spy after the test (e.g., call logSpy.mockRestore() or
vi.restoreAllMocks()) or add an afterEach hook that calls vi.restoreAllMocks()
so Logger.prototype.error is returned to its original implementation after
service.sendEmail(...) is asserted.
In `@apps/api/src/modules/identity/auth/system-admin.guard.spec.ts`:
- Around line 118-135: The test "should throw ForbiddenException if user has
undefined systemRole" is using user data with systemRole: 'tenant_user' so it
never hits the undefined branch; update the mocked session returned by
authService.getSessionFromHeaders in this spec (the test block using
authService.getSessionFromHeaders and mockContext) so the user object has no
systemRole (either remove the systemRole property or set it to undefined) before
calling guard.canActivate; keep the rest of the mockContext and the
expect(...).rejects.toThrow(ForbiddenException) assertion unchanged.
- Around line 96-115: Rename the test description to reflect the actual role
being asserted: change the it(...) title from "should allow access if user is
system_admin" to "should allow access if user is platform_admin" in the spec
that sets mockUser.systemRole = 'platform_admin' and calls guard.canActivate;
this keeps the test name consistent with the mockUser and the guard behavior
(refer to the it(...) block, mockUser.systemRole and guard.canActivate in
system-admin.guard.spec.ts).
In `@apps/api/src/modules/identity/invitations/invitations.service.spec.ts`:
- Around line 9-14: The module-scoped mockAuthProvider (with createInvitation,
acceptInvitation, getInvitation, listInvitations) can leak call/return state
across tests; add a beforeEach hook in invitations.service.spec.ts that resets
the Vitest mocks (e.g., call vi.resetAllMocks() or vi.clearAllMocks()) so each
test starts with fresh mock state for mockAuthProvider's functions.
In `@apps/api/src/modules/identity/users/users.controller.spec.ts`:
- Around line 26-41: Remove the redundant vi.clearAllMocks() call from the
beforeEach block in the test file: since userProvider and tenantProvider are
recreated as fresh mocks in that beforeEach, the vi.clearAllMocks() invocation
(referenced by vi.clearAllMocks) is unnecessary—delete that line and keep the
mock declarations (userProvider.create, userProvider.findAll,
tenantProvider.findAllForUser, etc.) as-is to ensure each test gets new mock
instances.
In `@apps/api/src/shared/utils/headers.util.spec.ts`:
- Around line 1-2: Remove the redundant explicit imports of the Vitest globals
in the test: delete "describe", "it", and "expect" from the import statement so
the file only imports toWebHeaders from './headers.util' and relies on
vitest/globals; update the import line accordingly in
apps/api/src/shared/utils/headers.util.spec.ts (leave toWebHeaders and its usage
unchanged).
In `@apps/api/test/mocks/identity.mock.ts`:
- Around line 1-2: The import of vi at the top of identity.mock.ts is redundant
because vi is provided globally via the "vitest/globals" type in tsconfig;
remove the line "import { vi } from 'vitest';" from identity.mock.ts (or
alternatively, if you prefer explicit imports across tests, add the same
explicit import to other test files like auth.service.spec.ts for consistency) —
locate the reference to vi in identity.mock.ts and either delete the import
statement or standardize imports across the test suite accordingly.
In `@apps/api/test/vitest-e2e.config.ts`:
- Line 13: Remove the unnecessary placeholder comment and empty array by
deleting the setupFiles: [], // Add setups if needed entry from the config;
locate the setupFiles property in the vitest-e2e.config (the setupFiles array)
and remove that line so the config contains only actual settings.
- Around line 1-11: The config uses CommonJS-only __dirname when setting alias
'@src' with path.resolve(__dirname, '../src'); replace __dirname with an
ESM-safe dirname derived from import.meta.url (e.g., compute file path via
fileURLToPath(import.meta.url) and path.dirname(...) or use import.meta.dirname
on Node 20.11+), importing fileURLToPath from 'url' if needed, and update the
path.resolve call in the Vitest config so alias '@src' points to the correct
directory without using __dirname.
In `@apps/api/vitest.config.ts`:
- Around line 8-10: Make the alias entry consistent with the E2E config by
replacing the relative string './src' for the '@src' alias with a resolved
absolute path using path.resolve(__dirname, '../src') and ensure the top of the
file imports or requires Node's path (e.g., import path or import { resolve }
from 'path') so the alias uses path.resolve(__dirname, '../src') rather than the
literal './src'; update the alias object (key '@src') accordingly and keep the
rest of the config unchanged.
Comment on lines
+1
to
+11
| import { defineConfig } from 'vitest/config'; | ||
| import swc from 'unplugin-swc'; | ||
| import path from 'path'; | ||
|
|
||
| export default defineConfig({ | ||
| test: { | ||
| include: ['**/*.e2e-spec.ts'], | ||
| globals: true, | ||
| root: './', | ||
| alias: { | ||
| '@src': path.resolve(__dirname, '../src'), |
Contributor
There was a problem hiding this comment.
__dirname is not available in ESM modules.
This configuration file uses ES module syntax but references __dirname on line 11, which is only available in CommonJS modules. This will cause a ReferenceError: __dirname is not defined at runtime when Vitest loads the config.
🐛 Proposed fix using import.meta helpers
import { defineConfig } from 'vitest/config';
import swc from 'unplugin-swc';
-import path from 'path';
+import path, { dirname } from 'path';
+import { fileURLToPath } from 'url';
+
+const __dirname = dirname(fileURLToPath(import.meta.url));
export default defineConfig({Alternatively, if targeting Node.js 20.11+, you can use import.meta.dirname directly:
- '@src': path.resolve(__dirname, '../src'),
+ '@src': path.resolve(import.meta.dirname, '../src'),📝 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.
Suggested change
| import { defineConfig } from 'vitest/config'; | |
| import swc from 'unplugin-swc'; | |
| import path from 'path'; | |
| export default defineConfig({ | |
| test: { | |
| include: ['**/*.e2e-spec.ts'], | |
| globals: true, | |
| root: './', | |
| alias: { | |
| '@src': path.resolve(__dirname, '../src'), | |
| import { defineConfig } from 'vitest/config'; | |
| import swc from 'unplugin-swc'; | |
| import path, { dirname } from 'path'; | |
| import { fileURLToPath } from 'url'; | |
| const __dirname = dirname(fileURLToPath(import.meta.url)); | |
| export default defineConfig({ | |
| test: { | |
| include: ['**/*.e2e-spec.ts'], | |
| globals: true, | |
| root: './', | |
| alias: { | |
| '@src': path.resolve(__dirname, '../src'), |
🤖 Prompt for AI Agents
In `@apps/api/test/vitest-e2e.config.ts` around lines 1 - 11, The config uses
CommonJS-only __dirname when setting alias '@src' with path.resolve(__dirname,
'../src'); replace __dirname with an ESM-safe dirname derived from
import.meta.url (e.g., compute file path via fileURLToPath(import.meta.url) and
path.dirname(...) or use import.meta.dirname on Node 20.11+), importing
fileURLToPath from 'url' if needed, and update the path.resolve call in the
Vitest config so alias '@src' points to the correct directory without using
__dirname.
| alias: { | ||
| '@src': path.resolve(__dirname, '../src'), | ||
| }, | ||
| setupFiles: [], // Add setups if needed |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Placeholder comment can be removed.
The empty setupFiles array with the comment // Add setups if needed is unnecessary boilerplate. Consider removing it until setup files are actually needed.
🧹 Proposed cleanup
alias: {
'@src': path.resolve(__dirname, '../src'),
},
- setupFiles: [], // Add setups if needed
},📝 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.
Suggested change
| setupFiles: [], // Add setups if needed | |
| alias: { | |
| '@src': path.resolve(__dirname, '../src'), | |
| }, | |
| }, |
🤖 Prompt for AI Agents
In `@apps/api/test/vitest-e2e.config.ts` at line 13, Remove the unnecessary
placeholder comment and empty array by deleting the setupFiles: [], // Add
setups if needed entry from the config; locate the setupFiles property in the
vitest-e2e.config (the setupFiles array) and remove that line so the config
contains only actual settings.
Comment on lines
+8
to
+10
| alias: { | ||
| '@src': './src', | ||
| }, |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor inconsistency with E2E config alias pattern.
This config uses a relative path string './src' for the alias, while the E2E config at apps/api/test/vitest-e2e.config.ts uses path.resolve(__dirname, '../src'). Both approaches work, but using consistent patterns across config files improves maintainability.
🤖 Prompt for AI Agents
In `@apps/api/vitest.config.ts` around lines 8 - 10, Make the alias entry
consistent with the E2E config by replacing the relative string './src' for the
'@src' alias with a resolved absolute path using path.resolve(__dirname,
'../src') and ensure the top of the file imports or requires Node's path (e.g.,
import path or import { resolve } from 'path') so the alias uses
path.resolve(__dirname, '../src') rather than the literal './src'; update the
alias object (key '@src') accordingly and keep the rest of the config unchanged.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/api/src/modules/email/email.module.ts`:
- Line 26: The SMTP secure flag is read via
configService.get<boolean>('SMTP_SECURE', false) which does not coerce strings
to booleans; update the code where the transport option `secure` is set (the
call using ConfigService.get for 'SMTP_SECURE') to explicitly parse the env var
as a string and convert it to a boolean (e.g., treat 'true' or '1' as true,
otherwise false), preserving the default when the var is unset; use
ConfigService.get<string>('SMTP_SECURE') (or read raw and coerce) and assign the
resulting boolean to the `secure` option.
In `@apps/api/vitest.config.mts`:
- Around line 42-47: The branch coverage threshold (branches: 80) in the
thresholds object should be aligned with team expectations; either raise
branches to match the other thresholds (e.g., 90) or add a short comment next to
the thresholds object explaining why branches are intentionally lower, and
update the thresholds object (statements/functions/lines/branches) accordingly
to reflect the chosen enforcement strategy; locate and edit the thresholds
object in vitest.config.mts to make this change.
| return nodemailer.createTransport({ | ||
| host: configService.get<string>('SMTP_HOST'), | ||
| port: configService.get<number>('SMTP_PORT'), | ||
| secure: configService.get<boolean>('SMTP_SECURE', false), |
Contributor
There was a problem hiding this comment.
Environment variable SMTP_SECURE may not parse as expected.
ConfigService.get<string>() returns environment variables as strings. The generic type <boolean> is only a TypeScript annotation and doesn't coerce the value. If SMTP_SECURE is set to the string 'false', it will be truthy in JavaScript, potentially enabling TLS unexpectedly.
🛠️ Proposed fix to handle string-to-boolean conversion
- secure: configService.get<boolean>('SMTP_SECURE', false),
+ secure: configService.get<string>('SMTP_SECURE', 'false') === 'true',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| secure: configService.get<boolean>('SMTP_SECURE', false), | |
| return nodemailer.createTransport({ | |
| host: configService.get<string>('SMTP_HOST'), | |
| port: configService.get<number>('SMTP_PORT'), | |
| secure: configService.get<string>('SMTP_SECURE', 'false') === 'true', | |
| auth: { | |
| user: configService.get<string>('SMTP_USER'), | |
| pass: configService.get<string>('SMTP_PASS'), | |
| }, | |
| }); |
🤖 Prompt for AI Agents
In `@apps/api/src/modules/email/email.module.ts` at line 26, The SMTP secure flag
is read via configService.get<boolean>('SMTP_SECURE', false) which does not
coerce strings to booleans; update the code where the transport option `secure`
is set (the call using ConfigService.get for 'SMTP_SECURE') to explicitly parse
the env var as a string and convert it to a boolean (e.g., treat 'true' or '1'
as true, otherwise false), preserving the default when the var is unset; use
ConfigService.get<string>('SMTP_SECURE') (or read raw and coerce) and assign the
resulting boolean to the `secure` option.
…nt coverage thresholds
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@apps/api/src/modules/email/email.module.ts`:
- Around line 16-21: The current logic treats an undefined MAIL_MOCK as opting
into mocking; change it so mocking is only enabled when MAIL_MOCK is explicitly
set to 'true'. Update the expression that defines useMock (currently using
mailMockEnv === 'true' || mailMockEnv === undefined) to only be true when
mailMockEnv === 'true' (i.e., remove the undefined check) so that
ConsoleEmailService is used only when MAIL_MOCK is explicitly 'true' and real
email remains the default; the relevant symbols to update are mailMockEnv,
useMock and the surrounding if (useMock) return null in email.module.ts where
configService.get is called.
- Around line 24-26: The SMTP port value from ConfigService.get('SMTP_PORT') is
currently passed as a string to nodemailer; update the email module so the port
is explicitly parsed to a number (e.g., via Number(...) or parseInt(..., 10))
before passing to nodemailer in the configuration where host/port/secure are set
(refer to the port entry using configService.get('SMTP_PORT')), and add a
fallback or validation to handle NaN/missing values so nodemailer always
receives a valid numeric port.
In `@apps/api/vitest.config.mts`:
- Around line 9-55: Add an explicit runtime environment to the Vitest config by
setting the test.environment property to 'node' inside the defineConfig({ test:
{ ... } }) block (i.e., add environment: 'node' alongside globals and root) so
the runtime context is explicit even though Vitest defaults to node.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.