feat(home): SaaS readiness checklist + admin route#3355
Merged
8 commits merged intoApr 2, 2026
Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 15 minutes and 43 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds a SaaS readiness feature: async startup readiness logging, a protected admin-only Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Router as "Route (/api/admin/readiness)"
participant Auth as "JWT Auth"
participant Policy as "policy.isAllowed"
participant Controller as "home.readiness"
participant Service as "HomeService.getReadinessStatus"
Client->>Router: GET /api/admin/readiness
Router->>Auth: passport.authenticate('jwt')
alt Not authenticated
Auth-->>Client: 401
else Authenticated
Auth->>Policy: isAllowed(req)
Policy->>Policy: deriveSubjectType('/api/admin/readiness') -> 'Readiness'
alt Not authorized
Policy-->>Client: 403
else Authorized
Policy->>Controller: invoke readiness
Controller->>Service: getReadinessStatus()
Service-->>Controller: readiness array
Controller-->>Client: 200 + data
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 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🧪 Generate unit tests (beta)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3355 +/- ##
==========================================
+ Coverage 84.60% 84.84% +0.24%
==========================================
Files 110 110
Lines 2767 2811 +44
Branches 769 789 +20
==========================================
+ Hits 2341 2385 +44
Misses 337 337
Partials 89 89 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds a “SaaS readiness” checklist to the Home module, exposing it via an admin-only API endpoint and printing a summary during server boot to help operators verify production configuration.
Changes:
- Added
GET /api/admin/readinessprotected by JWT + CASL admin ability, returning readiness checks as JSON. - Implemented
HomeService.getReadinessStatus()to compute 7-category readiness results. - Printed readiness summary during startup (skipped in
NODE_ENV=test) and added integration tests for 401/403/200.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
modules/home/tests/home.integration.tests.js |
Adds integration coverage for the new readiness endpoint (auth + response shape). |
modules/home/services/home.service.js |
Implements readiness checks and supporting config helpers. |
modules/home/routes/home.route.js |
Registers the new admin readiness route with JWT + policy middleware. |
modules/home/policies/home.policy.js |
Grants CASL read ability for Readiness to admin users. |
modules/home/controllers/home.controller.js |
Adds controller handler returning readiness checks via standard responses.success. |
lib/middlewares/policy.js |
Maps /api/admin/readiness to CASL subject type Readiness. |
lib/app.js |
Logs readiness table at boot via dynamic import of HomeService (skipping test env). |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/app.js`:
- Around line 89-92: The JSDoc for the newly async function logConfiguration is
missing a `@returns` clause; add a JSDoc header above the logConfiguration
declaration that documents the resolved value (e.g. `@returns` {Promise<void>} if
it returns nothing) so the async return type is explicit — update the comment
block for function logConfiguration to include the appropriate `@returns`
{Promise<...>} reflecting the resolved type (and any `@param` tags if needed).
- Around line 103-107: The app is importing HomeService.getReadinessStatus()
from a feature module; extract the shared readiness builder into a new
lower-level module under lib/helpers or lib/services (e.g., create
lib/helpers/readinessBuilder.js exporting a function getReadinessStatus), update
HomeService (in modules/home/services/home.service.js) to call that new helper
instead of owning the logic, and change lib/app.js to import the new helper for
startup logging (replace call to HomeService.getReadinessStatus with the
helper's function) so the dependency flows from feature -> lib/helpers rather
than core -> feature.
In `@modules/home/controllers/home.controller.js`:
- Around line 99-103: Add a `@returns` {void} tag to the JSDoc block above the
SaaS readiness checks handler (the comment starting "@desc Endpoint to return
SaaS readiness checks (admin only)") so the function's return contract is
explicitly documented; update the JSDoc for the handler in
modules/home/controllers/home.controller.js (the readiness-checks endpoint
comment) to include "@returns {void}" alongside the existing `@param` entries.
In `@modules/home/policies/home.policy.js`:
- Around line 15-17: Replace the bespoke readiness rule with the shared
platform-admin short‑circuit: remove the Array.isArray(user?.roles) &&
user.roles.includes('admin') branch that only grants can('read','Readiness') and
instead implement the standard admin check using user.roles.includes('admin') to
call can('manage','all') and immediately return; locate the check around the
current can('read','Readiness') usage to modify the policy accordingly.
In `@modules/home/services/home.service.js`:
- Around line 128-134: The security check currently only marks jwtDefault true
for one exact literal; update the logic that computes jwtDefault (the variable
referencing config.jwt?.secret) to treat missing/falsy secrets, empty strings,
the known placeholder 'WaosSecretKeyExampleToChnageAbsolutely', and any
env-placeholder patterns like strings starting with 'DEVKIT_NODE_' as defaults.
Then keep pushing the same checks entry into checks (category 'security') but
set status to 'warning' and the warning message when jwtDefault is true;
otherwise mark 'ok' and the custom message. This ensures the checks array entry
for the JWT secret correctly flags empty/placeholder secrets as warnings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 46316307-48c2-4801-9b08-7554ce08e30c
📒 Files selected for processing (7)
lib/app.jslib/middlewares/policy.jsmodules/home/controllers/home.controller.jsmodules/home/policies/home.policy.jsmodules/home/routes/home.route.jsmodules/home/services/home.service.jsmodules/home/tests/home.integration.tests.js
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
lib/app.js (1)
104-118: 🛠️ Refactor suggestion | 🟠 MajorDependency direction concern:
lib/imports frommodules/.The core bootstrapping code (
lib/app.js) dynamically imports from a feature module (modules/home/services/home.service.js). This inverts the expected dependency direction where feature modules should depend on shared lib code, not vice versa.Consider extracting the readiness logic into
lib/helpers/readiness.jsorlib/services/readiness.jsso both startup logging and the Home module can depend on the same lower-level utility.As per coding guidelines,
lib/**/*.js: Shared code goes inlib/helpers/orlib/services/with explicit justification.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/app.js` around lines 104 - 118, Move the readiness-check logic out of modules/home/service into a shared lib helper (e.g., create lib/helpers/readiness.js or lib/services/readiness.js) that exports a function like getReadinessStatus(); update HomeService (modules/home/services/home.service.js) to call/import that shared getReadinessStatus instead of providing the logic itself; then change lib/app.js to dynamically import the shared helper (or call its exported function) instead of importing HomeService so the dependency flows from modules -> lib, not lib -> modules; ensure exports/imports and any returned structure (array of checks with category, status, message) remain the same.modules/home/services/home.service.js (1)
113-180: 🧹 Nitpick | 🔵 TrivialOverall
getReadinessStatusimplementation is solid.The function covers all seven readiness categories mentioned in the PR objectives, uses consistent check patterns, and returns a well-typed array. The checks are appropriately non-blocking warnings as designed.
One minor observation: the JWT check (lines 129-130) doesn't use
isSetfor theDEVKIT_NODE_placeholder detection, while other checks do. This is acceptable since the JWT check has unique requirements (specific default value comparison), but consider addingDEVKIT_NODE_prefix detection for consistency:Optional: Add DEVKIT_NODE_ detection to JWT check
const jwtSecret = config.jwt?.secret; - const jwtInsecure = !jwtSecret || jwtSecret.trim() === '' || jwtSecret === 'WaosSecretKeyExampleToChnageAbsolutely'; + const jwtInsecure = !jwtSecret || jwtSecret.trim() === '' || jwtSecret.startsWith('DEVKIT_NODE_') || jwtSecret === 'WaosSecretKeyExampleToChnageAbsolutely';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/home/services/home.service.js` around lines 113 - 180, The JWT placeholder detection in getReadinessStatus should also treat values that are environment placeholders (start with "DEVKIT_NODE_") as insecure; update the jwtInsecure logic in getReadinessStatus (where jwtSecret and jwtInsecure are defined) to consider jwtSecret empty/blank, equal to the default example string, or jwtSecret.startsWith('DEVKIT_NODE_') as insecure so the status/message remain a warning for those placeholder values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/app.js`:
- Around line 115-117: Replace the silent catch block that uses catch (_err)
around the readiness check with a catch (err) that logs the failure (e.g.,
logger.warn or processLogger.warn / console.warn) including a clear message like
"readiness check failed" and the error details, keeping the behavior
non-blocking (do not rethrow); update the catch in lib/app.js that currently
reads catch (_err) to capture the error and emit a log entry so readiness
failures are visible for debugging.
In `@modules/home/services/home.service.js`:
- Around line 121-126: Cache the result of isSet(config.domain) into a local
variable before pushing the check to avoid calling it twice; for example,
compute something like const domainSet = isSet(config.domain) and then use
domainSet to set status and message inside the object you push to checks. Update
the block that builds the config — domain check (the code that currently calls
isSet(config.domain) twice) to use this single variable so only one evaluation
of isSet occurs.
---
Duplicate comments:
In `@lib/app.js`:
- Around line 104-118: Move the readiness-check logic out of
modules/home/service into a shared lib helper (e.g., create
lib/helpers/readiness.js or lib/services/readiness.js) that exports a function
like getReadinessStatus(); update HomeService
(modules/home/services/home.service.js) to call/import that shared
getReadinessStatus instead of providing the logic itself; then change lib/app.js
to dynamically import the shared helper (or call its exported function) instead
of importing HomeService so the dependency flows from modules -> lib, not lib ->
modules; ensure exports/imports and any returned structure (array of checks with
category, status, message) remain the same.
In `@modules/home/services/home.service.js`:
- Around line 113-180: The JWT placeholder detection in getReadinessStatus
should also treat values that are environment placeholders (start with
"DEVKIT_NODE_") as insecure; update the jwtInsecure logic in getReadinessStatus
(where jwtSecret and jwtInsecure are defined) to consider jwtSecret empty/blank,
equal to the default example string, or jwtSecret.startsWith('DEVKIT_NODE_') as
insecure so the status/message remain a warning for those placeholder values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7651bec0-f888-455c-9a0d-b96aa7e4c68f
📒 Files selected for processing (2)
lib/app.jsmodules/home/services/home.service.js
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
lib/app.js (1)
105-108: 🛠️ Refactor suggestion | 🟠 MajorKeep readiness construction out of the feature-module service layer here.
lib/app.jsnow pullsmodules/home/services/home.service.jsinto core bootstrap just to print readiness. Extract the readiness builder intolib/helpers/orlib/services/, then have bothHomeServiceandlogConfiguration()call that shared utility instead of reversing the dependency flow.As per coding guidelines,
lib/**/*.js: Shared code goes inlib/helpers/orlib/services/with explicit justification.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/app.js` around lines 105 - 108, lib/app.js currently imports HomeService to build readiness checks for logConfiguration(), which reverses the dependency flow; extract the readiness construction logic into a shared helper (e.g., new function buildReadinessStatus in lib/helpers/readiness.js or lib/services/readiness.js) and have both HomeService.getReadinessStatus() delegate to that shared buildReadinessStatus, then change logConfiguration() to call the shared helper instead of importing HomeService; update HomeService (modules/home/services/home.service.js) to call the new helper and remove any readiness-building code from the module so core bootstrap no longer depends on feature-module code.modules/home/services/home.service.js (1)
129-136:⚠️ Potential issue | 🟠 MajorStill warn on
DEVKIT_NODE_*JWT placeholders.This branch now catches blank and one hard-coded default, but it still reports placeholder secrets as custom. Reuse
isSet()here so the security check doesn't return a falseokwhen the base config is still carrying aDEVKIT_NODE_...value.Proposed fix
const jwtSecret = config.jwt?.secret; - const jwtInsecure = !jwtSecret || jwtSecret.trim() === '' || jwtSecret === 'WaosSecretKeyExampleToChnageAbsolutely'; + const normalizedJwtSecret = typeof jwtSecret === 'string' ? jwtSecret.trim() : ''; + const jwtInsecure = !isSet(normalizedJwtSecret) || normalizedJwtSecret === 'WaosSecretKeyExampleToChnageAbsolutely'; checks.push({ category: 'security', status: jwtInsecure ? 'warning' : 'ok', - message: jwtInsecure ? 'JWT secret is missing or default — change it before production' : 'JWT secret is custom', + message: jwtInsecure ? 'JWT secret is missing, placeholder, or default — change it before production' : 'JWT secret is custom', });Based on learnings, in
config/index.jsthe development defaults are loaded for everyNODE_ENV, soDEVKIT_NODE_*placeholders are a real runtime state until overridden.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/home/services/home.service.js` around lines 129 - 136, The JWT check currently treats placeholder DEVKIT_NODE_* values as custom; update the jwtInsecure calculation to reuse the existing isSet() helper so placeholders are considered unset. Specifically, import or reference isSet and change the jwtInsecure expression (the jwtSecret and jwtInsecure variables used to build the checks array) to include !isSet(jwtSecret) (e.g. jwtInsecure = !isSet(jwtSecret) || jwtSecret.trim() === '' || jwtSecret === 'WaosSecretKeyExampleToChnageAbsolutely') so the security check emits a warning for DEVKIT_NODE_* placeholders.modules/home/policies/home.policy.js (1)
15-17: 🛠️ Refactor suggestion | 🟠 MajorUse the standard platform-admin CASL short-circuit.
Granting
manageon onlyReadinessfixes this endpoint, but admins still need future subjects added one by one in this file. Switch to the sharedmanage allearly return here.Proposed fix
export function homeAbilities(user, membership, { can }) { - can('read', 'Home'); - if (Array.isArray(user?.roles) && user.roles.includes('admin')) { - can('manage', 'Readiness'); - } + if (user.roles.includes('admin')) { + can('manage', 'all'); + return; + } + can('read', 'Home'); }As per coding guidelines,
**/modules/*/policies/*.js: Platform admin checks must useif (user.roles.includes('admin')) { can('manage', 'all'); return; }pattern in policy files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/home/policies/home.policy.js` around lines 15 - 17, Replace the current admin check that grants only Readiness (the Array.isArray(user?.roles) && user.roles.includes('admin') block that calls can('manage','Readiness')) with the platform-admin short-circuit: detect admin via user.roles.includes('admin'), call can('manage','all') and immediately return so admins automatically get full privileges without per-subject additions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@modules/home/services/home.service.js`:
- Around line 148-154: The current health check uses mailer.isConfigured(),
which only verifies config.mailer.from and can falsely report 'ok'; replace this
with a readiness check that inspects the actual transport setup or call a new
mailer readiness helper (e.g., mailer.isReady() or mailer.isTransportReady())
that verifies the configured transport (Resend credentials present or Nodemailer
transport usable) and implements the "Resend suppresses Nodemailer warning"
logic; update the checks entry (category: 'mail') to use that readiness helper
so the status/message reflect the real transport readiness rather than just the
sender being set.
---
Duplicate comments:
In `@lib/app.js`:
- Around line 105-108: lib/app.js currently imports HomeService to build
readiness checks for logConfiguration(), which reverses the dependency flow;
extract the readiness construction logic into a shared helper (e.g., new
function buildReadinessStatus in lib/helpers/readiness.js or
lib/services/readiness.js) and have both HomeService.getReadinessStatus()
delegate to that shared buildReadinessStatus, then change logConfiguration() to
call the shared helper instead of importing HomeService; update HomeService
(modules/home/services/home.service.js) to call the new helper and remove any
readiness-building code from the module so core bootstrap no longer depends on
feature-module code.
In `@modules/home/policies/home.policy.js`:
- Around line 15-17: Replace the current admin check that grants only Readiness
(the Array.isArray(user?.roles) && user.roles.includes('admin') block that calls
can('manage','Readiness')) with the platform-admin short-circuit: detect admin
via user.roles.includes('admin'), call can('manage','all') and immediately
return so admins automatically get full privileges without per-subject
additions.
In `@modules/home/services/home.service.js`:
- Around line 129-136: The JWT check currently treats placeholder DEVKIT_NODE_*
values as custom; update the jwtInsecure calculation to reuse the existing
isSet() helper so placeholders are considered unset. Specifically, import or
reference isSet and change the jwtInsecure expression (the jwtSecret and
jwtInsecure variables used to build the checks array) to include
!isSet(jwtSecret) (e.g. jwtInsecure = !isSet(jwtSecret) || jwtSecret.trim() ===
'' || jwtSecret === 'WaosSecretKeyExampleToChnageAbsolutely') so the security
check emits a warning for DEVKIT_NODE_* placeholders.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e109e766-dbd0-4164-adb3-46cf394fe63b
📒 Files selected for processing (5)
lib/app.jsmodules/home/controllers/home.controller.jsmodules/home/policies/home.policy.jsmodules/home/services/home.service.jsmodules/home/tests/home.integration.tests.js
PierreBrisorgueil
force-pushed
the
feat/saas-readiness
branch
2 times, most recently
from
April 2, 2026 06:16
e6d540f to
6a97954
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@modules/home/tests/home.integration.tests.js`:
- Around line 202-205: The test "should return 401 for unauthenticated user"
currently only checks that result.body exists; update it to assert the error
shape like the other auth tests by verifying specific fields—e.g., after
agent.get('/api/admin/readiness').expect(401) add assertions such as
expect(result.body).toMatchObject({ error: expect.any(String) }) and/or
expect(result.body.message).toBeDefined() (or whatever keys the other auth tests
use) so the response structure is validated consistently with the tests around
line 209.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: babaeeb9-45c8-44fc-844d-de27fbacd307
📒 Files selected for processing (1)
modules/home/tests/home.integration.tests.js
PierreBrisorgueil
force-pushed
the
feat/saas-readiness
branch
from
April 2, 2026 06:24
d7a2157 to
12a0e0e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
modules/home/services/home.service.js (2)
129-136:⚠️ Potential issue | 🟠 MajorFlag
DEVKIT_NODE_*JWT secrets as insecure.At Line 131, placeholder secrets like
DEVKIT_NODE_*are currently treated as custom. This can misreport security readiness asok.Proposed fix
const jwtSecret = config.jwt?.secret; - const jwtInsecure = !jwtSecret || jwtSecret.trim() === '' || jwtSecret === 'WaosSecretKeyExampleToChnageAbsolutely'; + const jwtInsecure = !isSet(jwtSecret) || jwtSecret === 'WaosSecretKeyExampleToChnageAbsolutely'; checks.push({ category: 'security', status: jwtInsecure ? 'warning' : 'ok',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/home/services/home.service.js` around lines 129 - 136, The current jwtInsecure check (using jwtSecret, jwtInsecure and the checks.push block) treats placeholder secrets like DEVKIT_NODE_* as custom; update the jwtInsecure condition to also mark any jwtSecret that equals or starts with the placeholder pattern (e.g., 'DEVKIT_NODE_' or exactly 'DEVKIT_NODE_*') as insecure. Modify the boolean expression that defines jwtInsecure to include a test for jwtSecret.startsWith('DEVKIT_NODE_') or a strict equality to 'DEVKIT_NODE_*' so the checks.push entry will report 'warning' for those placeholder values.
148-154:⚠️ Potential issue | 🟠 MajorMail readiness currently checks sender presence, not transport readiness.
At Line 149,
mailer.isConfigured()only validatesconfig.mailer.from, so mail can be markedokwhile neither Resend nor Nodemailer is actually usable. It also does not encode the “Resend suppresses Nodemailer warning” behavior.Proposed fix
- const mailConfigured = mailer.isConfigured(); + const resendConfigured = isSet(config.mailer?.options?.apiKey); + const nodemailerConfigured = isSet(config.mailer?.options?.service) + && isSet(config.mailer?.options?.auth?.user) + && isSet(config.mailer?.options?.auth?.pass); + const mailConfigured = isSet(config.mailer?.from) && (resendConfigured || nodemailerConfigured); checks.push({ category: 'mail', status: mailConfigured ? 'ok' : 'warning', - message: mailConfigured ? 'Mail provider configured' : 'No mail provider configured', + message: resendConfigured + ? 'Resend configured' + : nodemailerConfigured + ? 'Nodemailer configured' + : 'No mail provider configured', });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/home/services/home.service.js` around lines 148 - 154, The mail readiness check currently calls mailer.isConfigured() (which only checks config.mailer.from) so update this block to test actual transport availability and implement the "Resend suppresses Nodemailer warning" rule: query the mailer for transport state (e.g. add/use methods like mailer.hasResendProvider() and mailer.isNodemailerReady() or a single mailer.getTransportStatus()) and set checks entry category 'mail' to status 'ok' only if a usable transport exists; if Resend is available mark 'ok' even if Nodemailer is not ready, and if only Nodemailer is configured but not ready mark 'warning' with a message explaining the transport is not ready. Ensure you replace the mailer.isConfigured() usage and update the message text accordingly so the check reflects real transport readiness.modules/home/tests/home.integration.tests.js (1)
202-205:⚠️ Potential issue | 🟡 MinorStrengthen the 401 assertion to validate the error contract.
At Line 204,
toBeDefined()is too weak and can pass with an invalid error payload.Proposed fix
test('should return 401 for unauthenticated user', async () => { const result = await agent.get('/api/admin/readiness').expect(401); - expect(result.body).toBeDefined(); + expect(result.body.type).toBe('error'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/home/tests/home.integration.tests.js` around lines 202 - 205, The test "should return 401 for unauthenticated user" currently uses expect(result.body).toBeDefined(), which is too weak; update the assertion for the agent.get('/api/admin/readiness').expect(401) response to validate the API's error contract (e.g., check specific fields like status/code/message or an error key and message) and ensure the body shape and values match the expected 401 error payload returned by the server (replace the toBeDefined() with assertions that check the exact properties and types).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@modules/home/tests/home.integration.tests.js`:
- Around line 234-267: The readiness test "should report ok status when config
values are properly set" mutates shared config and a mailer spy without
guaranteed cleanup; wrap the setup, assertions and HTTP call in a try/finally so
you always restore config values (origDomain, origJwt, origOAuth, origStripe,
origPosthog, origSentry) and call mailerSpy.mockRestore() in the finally block;
apply the same try/finally pattern to the other readiness tests that mutate
config (the tests around the other readiness cases) to prevent test-state
leakage.
---
Duplicate comments:
In `@modules/home/services/home.service.js`:
- Around line 129-136: The current jwtInsecure check (using jwtSecret,
jwtInsecure and the checks.push block) treats placeholder secrets like
DEVKIT_NODE_* as custom; update the jwtInsecure condition to also mark any
jwtSecret that equals or starts with the placeholder pattern (e.g.,
'DEVKIT_NODE_' or exactly 'DEVKIT_NODE_*') as insecure. Modify the boolean
expression that defines jwtInsecure to include a test for
jwtSecret.startsWith('DEVKIT_NODE_') or a strict equality to 'DEVKIT_NODE_*' so
the checks.push entry will report 'warning' for those placeholder values.
- Around line 148-154: The mail readiness check currently calls
mailer.isConfigured() (which only checks config.mailer.from) so update this
block to test actual transport availability and implement the "Resend suppresses
Nodemailer warning" rule: query the mailer for transport state (e.g. add/use
methods like mailer.hasResendProvider() and mailer.isNodemailerReady() or a
single mailer.getTransportStatus()) and set checks entry category 'mail' to
status 'ok' only if a usable transport exists; if Resend is available mark 'ok'
even if Nodemailer is not ready, and if only Nodemailer is configured but not
ready mark 'warning' with a message explaining the transport is not ready.
Ensure you replace the mailer.isConfigured() usage and update the message text
accordingly so the check reflects real transport readiness.
In `@modules/home/tests/home.integration.tests.js`:
- Around line 202-205: The test "should return 401 for unauthenticated user"
currently uses expect(result.body).toBeDefined(), which is too weak; update the
assertion for the agent.get('/api/admin/readiness').expect(401) response to
validate the API's error contract (e.g., check specific fields like
status/code/message or an error key and message) and ensure the body shape and
values match the expected 401 error payload returned by the server (replace the
toBeDefined() with assertions that check the exact properties and types).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 013b4309-46d0-469a-8482-c9f2215afd89
📒 Files selected for processing (2)
modules/home/services/home.service.jsmodules/home/tests/home.integration.tests.js
- GET /api/admin/readiness (admin-only) returns startup checklist as JSON - validateSaaSReadiness() prints status table at boot (skipped in test) - 7 categories: Config, Security, Auth, Mail, Billing, Analytics, Monitoring - Smart dependencies: Resend configured → skip Nodemailer warning - Non-blocking: warnings only, never prevents startup
…tion - Add proper JSDoc with @returns on logConfiguration - Fix isSet JSDoc to match implementation (non-empty string check) - JWT check now catches missing/empty secret, not just default value - Remove mail provider name from response to avoid leaking placeholders
… cache isSet - Add @returns {void} to readiness controller JSDoc - Use can('manage', 'Readiness') matching standard CASL pattern - Log readiness check failures at boot instead of swallowing - Cache isSet(config.domain) to avoid duplicate calls
Cover all getReadinessStatus branches (ok/warning paths for each check category) and the readiness controller error handler to improve patch and project coverage.
- Wrap config mutation tests with try/finally to prevent state leakage - Strengthen 401 assertion with status check - Clarify isSet JSDoc wording
PierreBrisorgueil
force-pushed
the
feat/saas-readiness
branch
from
April 2, 2026 06:59
ea46745 to
6eec090
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
lib/app.js (1)
105-108: 🛠️ Refactor suggestion | 🟠 MajorKeep readiness building out of
lib/app.js.
lib/app.jsnow reaches intomodules/home/services/home.service.jsjust to reuse readiness assembly. That reverses the dependency direction and pulls feature-module wiring into core bootstrapping. Extract the readiness builder intolib/helpers/orlib/services/, then have bothHomeService.getReadinessStatus()andlogConfiguration()call that lower-level helper.As per coding guidelines,
lib/**/*.js: Shared code goes inlib/helpers/orlib/services/with explicit justification.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/app.js` around lines 105 - 108, The app currently imports HomeService in lib/app.js to build readiness via HomeService.getReadinessStatus(), which reverses dependencies; extract the readiness assembly logic into a new lower-level helper module (e.g., lib/helpers/readiness.js or lib/services/readiness.js) that exposes a function like buildReadinessStatus(), move the readiness-building code from HomeService.getReadinessStatus() into that helper, update HomeService.getReadinessStatus() to delegate to buildReadinessStatus(), and modify lib/app.js (and logConfiguration()) to call the new buildReadinessStatus() helper instead of importing HomeService so core bootstrapping no longer depends on feature modules.modules/home/policies/home.policy.js (1)
13-17: 🛠️ Refactor suggestion | 🟠 MajorUse the repo-standard platform-admin short-circuit here.
Granting
manageonly onReadinessworks for this endpoint, but it still drifts from the shared admin policy model and forces future admin-only Home subjects to be added one by one.Proposed fix
export function homeAbilities(user, membership, { can }) { + if (user.roles.includes('admin')) { + can('manage', 'all'); + return; + } can('read', 'Home'); - if (Array.isArray(user?.roles) && user.roles.includes('admin')) { - can('manage', 'Readiness'); - } }As per coding guidelines,
**/modules/*/policies/*.js: Platform admin checks must useif (user.roles.includes('admin')) { can('manage', 'all'); return; }pattern in policy files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/home/policies/home.policy.js` around lines 13 - 17, Update the platform-admin short-circuit in the homeAbilities function: instead of granting manage only on 'Readiness', add a top-level admin check inside homeAbilities that uses user.roles.includes('admin') to call can('manage', 'all') and return immediately; ensure you reference the existing function signature homeAbilities(user, membership, { can }) and replace the current admin branch so future admin-only subjects are covered by the shared policy pattern.modules/home/services/home.service.js (1)
130-135:⚠️ Potential issue | 🟠 MajorFlag
DEVKIT_NODE_*JWT secrets as insecure too.This branch still bypasses
isSet(), so placeholder secrets likeDEVKIT_NODE_JWT_SECRETare reported as custom. That makes the highest-risk readiness item look green while the rest of the checks treat the same placeholder pattern as unset.Proposed fix
const jwtSecret = config.jwt?.secret; - const jwtInsecure = !jwtSecret || jwtSecret.trim() === '' || jwtSecret === 'WaosSecretKeyExampleToChnageAbsolutely'; + const jwtInsecure = !isSet(jwtSecret) || jwtSecret === 'WaosSecretKeyExampleToChnageAbsolutely'; checks.push({ category: 'security', status: jwtInsecure ? 'warning' : 'ok',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/home/services/home.service.js` around lines 130 - 135, Update the jwtInsecure check so placeholder env-style values are considered insecure: modify the condition that computes jwtInsecure (which currently reads the config.jwt?.secret into jwtSecret and checks emptiness and the example default) to also treat values that match the DEVKIT_NODE_* placeholder pattern (e.g., start with "DEVKIT_NODE_") as insecure; update the logic used when pushing into checks (the jwtInsecure variable and its use in the checks.push block) so that any jwtSecret that is empty, the example default, or matches the DEVKIT_NODE_* pattern is reported as 'warning' rather than 'ok'.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@modules/home/tests/home.integration.tests.js`:
- Around line 22-23: The test suite fails to cleanup or pre-delete the
regular-readiness user fixture; update the beforeAll to explicitly remove any
user with email "regular-readiness@test.com" before creating it (mirror how
adminUser is handled) and store its identifier (e.g., regularReadinessUser or
userToken) so it can be removed later; then modify afterAll to delete that
regular readiness user alongside adminUser and restore
originalOrganizationsEnabled so the fixture is always cleaned up and no leftover
record breaks future runs.
---
Duplicate comments:
In `@lib/app.js`:
- Around line 105-108: The app currently imports HomeService in lib/app.js to
build readiness via HomeService.getReadinessStatus(), which reverses
dependencies; extract the readiness assembly logic into a new lower-level helper
module (e.g., lib/helpers/readiness.js or lib/services/readiness.js) that
exposes a function like buildReadinessStatus(), move the readiness-building code
from HomeService.getReadinessStatus() into that helper, update
HomeService.getReadinessStatus() to delegate to buildReadinessStatus(), and
modify lib/app.js (and logConfiguration()) to call the new
buildReadinessStatus() helper instead of importing HomeService so core
bootstrapping no longer depends on feature modules.
In `@modules/home/policies/home.policy.js`:
- Around line 13-17: Update the platform-admin short-circuit in the
homeAbilities function: instead of granting manage only on 'Readiness', add a
top-level admin check inside homeAbilities that uses
user.roles.includes('admin') to call can('manage', 'all') and return
immediately; ensure you reference the existing function signature
homeAbilities(user, membership, { can }) and replace the current admin branch so
future admin-only subjects are covered by the shared policy pattern.
In `@modules/home/services/home.service.js`:
- Around line 130-135: Update the jwtInsecure check so placeholder env-style
values are considered insecure: modify the condition that computes jwtInsecure
(which currently reads the config.jwt?.secret into jwtSecret and checks
emptiness and the example default) to also treat values that match the
DEVKIT_NODE_* placeholder pattern (e.g., start with "DEVKIT_NODE_") as insecure;
update the logic used when pushing into checks (the jwtInsecure variable and its
use in the checks.push block) so that any jwtSecret that is empty, the example
default, or matches the DEVKIT_NODE_* pattern is reported as 'warning' rather
than 'ok'.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4bb08cce-8ae5-4f8b-a31f-22a88c10c669
📒 Files selected for processing (7)
lib/app.jslib/middlewares/policy.jsmodules/home/controllers/home.controller.jsmodules/home/policies/home.policy.jsmodules/home/routes/home.route.jsmodules/home/services/home.service.jsmodules/home/tests/home.integration.tests.js
Pre-clean regular-readiness@test.com before creating it to prevent unique index failures on re-runs, hoist to suite scope, and delete in afterAll.
PierreBrisorgueil
enabled auto-merge
April 2, 2026 07:55
PierreBrisorgueil
closed this pull request by merging all changes
into
master
in
c8bd3a0
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
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
GET /api/admin/readinessroute (JWT + CASL admin-only) returning startup checklist as JSONvalidateSaaSReadiness(config)prints status table at boot (skipped in test env)Test plan
GET /api/admin/readinessreturns 401 without auth[{ category, status, message }]for adminSummary by CodeRabbit
New Features
Security
Bug Fixes
Tests