refactor(users,auth): extract password hashing to lib/helpers to break auth↔users cycle - #3864
Conversation
|
Warning Review limit reached
More reviews will be available in 55 minutes and 25 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughA new shared ChangesShared bcrypt helper extraction and service wiring
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 #3864 +/- ##
=======================================
Coverage 92.41% 92.41%
=======================================
Files 162 163 +1
Lines 5394 5394
Branches 1735 1735
=======================================
Hits 4985 4985
Misses 328 328
Partials 81 81
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR breaks the circular dependency between users.service and auth.service by extracting the bcrypt wrappers (hashPassword, comparePassword) into a new leaf helper at lib/helpers/password.js, then sourcing both services from that helper while keeping AuthService.hashPassword/comparePassword available via re-export.
Changes:
- Added
lib/helpers/password.js(bcrypt wrappers) and unit tests for it. - Updated
users.service.jsto hash passwords via the new helper instead of importingauth.service.js. - Updated
auth.service.jsto import wrappers from the helper and re-export them; adjusted related unit tests and mocks.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
lib/helpers/password.js |
New shared bcrypt wrapper helper used by both auth and users services. |
lib/helpers/tests/password.unit.tests.js |
New unit tests validating hashing/comparison behavior and default export shape. |
modules/auth/services/auth.service.js |
Imports bcrypt wrappers from the helper and re-exports them on AuthService. |
modules/auth/tests/auth.unit.tests.js |
Updates tests to validate re-exported helper behavior. |
modules/users/services/users.service.js |
Uses the helper for password hashing during user creation (removes auth-service dependency). |
modules/users/tests/users.service.count.unit.tests.js |
Updates mocks to target the new helper module. |
modules/users/tests/users.service.remove.cascade.unit.tests.js |
Removes now-unneeded auth-service mock. |
modules/users/tests/users.service.remove.pendingSweep.unit.tests.js |
Removes now-unneeded auth-service mock. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/helpers/password.js`:
- Around line 9-11: Add JSDoc documentation blocks for both the hashPassword and
comparePassword functions in lib/helpers/password.js. For hashPassword, include
a one-line description, a `@param` tag documenting the password parameter, and a
`@returns` tag indicating it returns a Promise resolving to the hashed password
string. For comparePassword, include a one-line description, `@param` tags for
both userPassword and storedPassword parameters, and a `@returns` tag indicating
it returns a Promise resolving to a boolean result of the comparison. Follow the
existing JSDoc format used elsewhere in the codebase to maintain consistency.
🪄 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: 79e87059-7b91-4a70-b7ba-cce9300f9cc8
📒 Files selected for processing (8)
lib/helpers/password.jslib/helpers/tests/password.unit.tests.jsmodules/auth/services/auth.service.jsmodules/auth/tests/auth.unit.tests.jsmodules/users/services/users.service.jsmodules/users/tests/users.service.count.unit.tests.jsmodules/users/tests/users.service.remove.cascade.unit.tests.jsmodules/users/tests/users.service.remove.pendingSweep.unit.tests.js
💤 Files with no reviewable changes (2)
- modules/users/tests/users.service.remove.cascade.unit.tests.js
- modules/users/tests/users.service.remove.pendingSweep.unit.tests.js
Summary
hashPasswordandcomparePassword(two bcrypt wrappers) fromauth.service.jsinto a new dependency-free leaf modulelib/helpers/password.js.users.service.jsnow imports them from the helper directly;auth.service.jssources them from the same helper and re-exports them on its default export (transparent re-export — zero API surface change).users.servicewas importingauth.servicesolely for these two helpers, creating a circular import path (users.service→auth.service→users.service). The leaf helper breaks the cycle structurally, with no behavioral change.Scope
lib/helpers/password.js(new),src/modules/auth/auth.service.js,src/modules/users/users.service.js, related unit testsnone—auth.servicere-exports the helpers on its default export, so any existing caller ofauth.service.hashPassword/auth.service.comparePasswordcontinues to work unchangedlowValidation
npm run lintnpm testGuardrails check
.env*,secrets/**, keys, tokens)Verification
1994/1994 unit tests green, lint clean. Cycle structurally proven:
users.service.jsno longer imports anything fromauth.service.Notes for reviewers
auth.servicedefault means downstream callers need zero migration.Reviewer notes (accepted)
Two pre-reviewed nits — please do not re-raise:
(a)
lib/helpers/password.jsexports both named (export { hashPassword, comparePassword }) and default (export default { hashPassword, comparePassword }). This is intentional — it mirrors the existing convention inlib/helpers/emailVerification.js, which uses the same dual-export pattern.(b) The
users.service.counttest mocks both the named and default exports of the password helper even though only the default export is consumed by the service. This is harmless; the extra mock is kept for structural symmetry with the other test files.Summary by CodeRabbit