Repository navigation
identity hexagonal refactor - #159
Merged
Merged
Conversation
Contributor
|
Warning Review limit reached
More reviews will be available in 30 minutes and 27 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ 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 (7)
📝 WalkthroughWalkthroughThis PR migrates identity outbound contracts from provider to repository ports, renames adapters and tokens, adds user orchestration use-cases and tests, reshapes package exports, and wires a global EncryptionModule into API and worker apps. ChangesProvider-to-Repository Pattern Migration with Use Cases
Encryption Module Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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 |
![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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/identity/src/index.spec.ts (1)
55-66: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winCover the newly exported barrel surface here.
This smoke test still only asserts adapter symbols, so it will not catch regressions in the new root exports added in
index.tsfor the outbound ports and user use cases. Add a few representative assertions for those new exports so the package-surface refactor is actually guarded.🤖 Prompt for 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. In `@packages/identity/src/index.spec.ts` around lines 55 - 66, The test only asserts adapter symbols; update the spec to also assert the new root exports for the outbound ports and user use cases so the package surface change is covered: open the package's index.ts, identify the newly exported symbols (e.g., the outbound port interfaces and user use case classes/functions such as CreateUserUseCase, UserOutboundPort — replace with the exact names you find) and add expect((IdentityPackage as any).<ExportName>).toBeDefined() assertions for each representative export alongside the existing adapter assertions (reference IdentityPackage and the adapter names already in the test to locate where to add them).packages/identity/src/adapters/outbound/drizzle-user.repository.ts (1)
300-302:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThrow the repository contract’s
NotFoundExceptionhere.
IUserRepository.deleteIfNotLastAdmin()documents aNotFoundExceptionwhen the user is missing or not a member of the tenant, but this branch now throws a genericError. That turns a normal not-found/masked-access path into an untyped failure for every repository consumer behind the new token wiring.Suggested fix
-import { Inject, Injectable, Logger } from "`@nestjs/common`"; +import { Inject, Injectable, Logger, NotFoundException } from "`@nestjs/common`"; ... if (!membershipWithRole.length) { - throw new Error("User is not a member of this organization"); + throw new NotFoundException("User not found"); }🤖 Prompt for 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. In `@packages/identity/src/adapters/outbound/drizzle-user.repository.ts` around lines 300 - 302, Replace the generic Error thrown when membershipWithRole is empty with the repository contract's NotFoundException so callers receive the documented not-found behavior; locate the check in deleteIfNotLastAdmin (the branch that currently does if (!membershipWithRole.length) throw new Error("User is not a member of this organization")) and throw NotFoundException (or construct the repository's NotFoundException type) with an appropriate message instead of Error.apps/api/src/modules/identity/system-admin/system-admin.controller.spec.ts (1)
207-219:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStub the role lookup in the create-user happy path.
SystemAdminController.createUser()now rejects any truthyrolewhenroleProvider.findById()returns nothing, but this test passes"member"without arranging that lookup. The current expectation fails beforemockUserProvider.create()is reached.Suggested fix
it('should create user via provider', async () => { mockUserProvider.findByEmail.mockResolvedValue(null); + mockRoleProvider.findById.mockResolvedValue({ id: 'member' }); const mockUser = { id: 'u1', email: 'new@example.com', };🤖 Prompt for 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. In `@apps/api/src/modules/identity/system-admin/system-admin.controller.spec.ts` around lines 207 - 219, The test calls SystemAdminController.createUser with role 'member' but doesn't stub the role lookup, causing createUser to reject when roleProvider.findById returns undefined; update the test to arrange/mock the role lookup before calling controller.createUser by having the mock role provider's findById (e.g., mockRoleProvider.findById) return a valid role object (an object containing at least id/name) so the happy path continues to mockUserProvider.create and the test exercises the create flow.
🤖 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 `@apps/api/src/app/app.module.ts`:
- Around line 52-60: The EncryptionModule.forRootAsync useFactory is missing the
KMS region parameter; update the factory (the useFactory function that receives
ConfigService / cfg) to include region: cfg.get('KMS_REGION') alongside kmsKeyId
and kmsEndpoint so the returned config object supplies region to the shared
EncryptionModule; make this change in the app.module where
EncryptionModule.forRootAsync is registered for both the API and the worker.
In `@packages/identity/src/core/ports/outbound/types.ts`:
- Around line 60-78: Update the UserListItem union's invitation variant to make
the name property optional: change the invitation branch (the object with kind:
"invitation") so that name?: string instead of name: string; this keeps all
other fields (id, email, role, status, emailVerified, createdAt, updatedAt,
isInvitation, permissions, image, banned, banReason, banExpires) intact and
aligns the invitation shape with cases that map name to an empty string.
In `@packages/identity/src/core/use-cases/users/create-user.use-case.ts`:
- Around line 14-16: The execute method in CreateUserUseCase is a pure
pass-through to userRepository.create(input) so either add meaningful logic
(validation, error mapping, and side-effects) or remove the indirection: if you
keep it, validate CreateUserInput inside execute (e.g., required fields, email
format), call this.userRepository.create(input), wrap repository errors into
domain errors, and emit a UserCreated event via the event bus (e.g.,
eventBus.publish('UserCreated', user)); if you remove it, delete
CreateUserUseCase and update callers/controllers to inject and call
userRepository.create(...) directly, removing the thin wrapper.
In `@packages/identity/src/core/use-cases/users/get-user-by-id.use-case.ts`:
- Around line 30-31: Replace the expensive tenant list load in
get-user-by-id.use-case: instead of calling
tenantRepository.findAllForUser(user.id) and using userTenants.some(...) to set
isMember, call tenantRepository.findOneForUser(user.id, tenantId) and set
isMember = Boolean(result) (or check for null/undefined). Update the code paths
that reference userTenants/isMember to use the new lookup so authorization is
based on the single tenant-scoped query.
In
`@packages/identity/src/core/use-cases/users/get-user-profile.use-case.spec.ts`:
- Around line 16-24: Add a unit test that covers the NotFoundException path:
mock userRepository.findById to resolve to null and assert that
useCase.execute("user-id") rejects/throws a NotFoundException. Locate the spec
file testing GetUserProfileUseCase (references: GetUserProfileUseCase,
useCase.execute, userRepository.findById) and add a new it/test block that
verifies the thrown error is an instance (or has the expected name/message) of
NotFoundException when findById returns null.
In
`@packages/identity/src/core/use-cases/users/list-users-with-invitations.use-case.ts`:
- Around line 33-48: The invitedUsers array maps pendingInvitations into
UserListItem objects and currently sets banned: false which is incorrect for
invitations; update the mapping (in the invitedUsers creation where
pendingInvitations.map is used) to set banned: undefined so the optional banned
field correctly reflects that invitees have no ban status (leave all other
fields unchanged).
In `@packages/identity/src/core/use-cases/users/remove-user.use-case.ts`:
- Around line 38-44: The current catch in remove-user.use-case.ts relies on
matching an English error string from deleteIfNotLastAdmin(), which is brittle;
instead either (A) add an explicit membership check before deletion (call a
repository method like usersRepository.isMember or
organizationRepository.hasMember with the userId and organizationId and throw
NotFoundException if false) or (B) change the repository contract so
deleteIfNotLastAdmin() throws a typed/unique error (e.g. MembershipError or
NotMemberError) and catch that specific class here to rethrow NotFoundException;
update the catch to test the specific error type
(MembershipError/NotMemberError) rather than error.message.includes(...).
In `@packages/identity/src/identity.module.spec.ts`:
- Around line 3-9: The test is missing the ROLE_REPOSITORY token in its wiring
assertions; update the import list and the assertions in identity.module.spec.ts
to include ROLE_REPOSITORY so the suite verifies that IdentityModule
registers/exports the role repository and that RolesController can inject it;
specifically add ROLE_REPOSITORY to the imported constants and include it in the
same assertion blocks where AUTH_PROVIDER, USER_REPOSITORY, TENANT_REPOSITORY,
PERMISSION_REPOSITORY, and IDENTITY_OPTIONS are checked.
In `@packages/identity/src/identity.module.ts`:
- Around line 43-48: registerAsync() is currently ignoring the
eventPublisherToken returned by the async options factory (so callers that set
eventPublisherToken in the resolved IdentityModuleOptions silently get the
default IdentityEventPublisher), causing inconsistent behavior vs register();
update the registerAsync implementation (the function named registerAsync and
its options handling) to read and honor eventPublisherToken from the resolved
options object returned by useFactory (and from any passed-in AsyncOptions
interface), and wire that token into the providers the same way register() does
(also update the other async-registration branches where options are mapped —
the blocks around the other async provider builds that currently only check
top-level tokens — to select options.eventPublisherToken when present). Ensure
you reference IdentityModuleOptions.eventPublisherToken and the
eventPublisherToken variable used in provider registration so the async path
exposes the same contract as the sync path.
---
Outside diff comments:
In `@apps/api/src/modules/identity/system-admin/system-admin.controller.spec.ts`:
- Around line 207-219: The test calls SystemAdminController.createUser with role
'member' but doesn't stub the role lookup, causing createUser to reject when
roleProvider.findById returns undefined; update the test to arrange/mock the
role lookup before calling controller.createUser by having the mock role
provider's findById (e.g., mockRoleProvider.findById) return a valid role object
(an object containing at least id/name) so the happy path continues to
mockUserProvider.create and the test exercises the create flow.
In `@packages/identity/src/adapters/outbound/drizzle-user.repository.ts`:
- Around line 300-302: Replace the generic Error thrown when membershipWithRole
is empty with the repository contract's NotFoundException so callers receive the
documented not-found behavior; locate the check in deleteIfNotLastAdmin (the
branch that currently does if (!membershipWithRole.length) throw new Error("User
is not a member of this organization")) and throw NotFoundException (or
construct the repository's NotFoundException type) with an appropriate message
instead of Error.
In `@packages/identity/src/index.spec.ts`:
- Around line 55-66: The test only asserts adapter symbols; update the spec to
also assert the new root exports for the outbound ports and user use cases so
the package surface change is covered: open the package's index.ts, identify the
newly exported symbols (e.g., the outbound port interfaces and user use case
classes/functions such as CreateUserUseCase, UserOutboundPort — replace with the
exact names you find) and add expect((IdentityPackage as
any).<ExportName>).toBeDefined() assertions for each representative export
alongside the existing adapter assertions (reference IdentityPackage and the
adapter names already in the test to locate where to add them).
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9e925e46-c16b-4fe3-9d8f-be2b22437698
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (60)
apps/api/src/app/app.module.tsapps/api/src/modules/identity/auth/auth.controller.coverage.spec.tsapps/api/src/modules/identity/auth/auth.controller.spec.tsapps/api/src/modules/identity/auth/auth.controller.tsapps/api/src/modules/identity/auth/platform.guard.spec.tsapps/api/src/modules/identity/roles/roles.controller.spec.tsapps/api/src/modules/identity/roles/roles.controller.tsapps/api/src/modules/identity/roles/roles.controller.visibility.spec.tsapps/api/src/modules/identity/system-admin/system-admin.controller.spec.tsapps/api/src/modules/identity/system-admin/system-admin.controller.tsapps/api/src/modules/identity/tenants/tenants.controller.spec.tsapps/api/src/modules/identity/tenants/tenants.controller.tsapps/api/src/modules/identity/users/users.controller.spec.tsapps/api/src/modules/identity/users/users.controller.tsapps/worker/package.jsonapps/worker/src/app.module.tspackages/auth/src/services/auth.service.tspackages/credentials/src/index.tspackages/credentials/src/oauth/token-manager.service.tspackages/credentials/src/oauth/token-refresh.service.tspackages/identity/src/adapters/outbound/better-auth.abac.spec.tspackages/identity/src/adapters/outbound/better-auth.adapter.spec.tspackages/identity/src/adapters/outbound/better-auth.adapter.tspackages/identity/src/adapters/outbound/drizzle-permission.repository.spec.tspackages/identity/src/adapters/outbound/drizzle-permission.repository.tspackages/identity/src/adapters/outbound/drizzle-role.repository.spec.tspackages/identity/src/adapters/outbound/drizzle-role.repository.tspackages/identity/src/adapters/outbound/drizzle-tenant.repository.spec.tspackages/identity/src/adapters/outbound/drizzle-tenant.repository.tspackages/identity/src/adapters/outbound/drizzle-user.repository.spec.tspackages/identity/src/adapters/outbound/drizzle-user.repository.tspackages/identity/src/better-auth.config.tspackages/identity/src/constants.tspackages/identity/src/core/ports/outbound/auth-provider.port.tspackages/identity/src/core/ports/outbound/better-auth-config.port.tspackages/identity/src/core/ports/outbound/email-provider.port.tspackages/identity/src/core/ports/outbound/errors.tspackages/identity/src/core/ports/outbound/event-publisher.port.tspackages/identity/src/core/ports/outbound/index.tspackages/identity/src/core/ports/outbound/permission-repository.port.tspackages/identity/src/core/ports/outbound/role-repository.port.tspackages/identity/src/core/ports/outbound/tenant-repository.port.tspackages/identity/src/core/ports/outbound/types.tspackages/identity/src/core/ports/outbound/user-repository.port.tspackages/identity/src/core/use-cases/users/create-user.use-case.spec.tspackages/identity/src/core/use-cases/users/create-user.use-case.tspackages/identity/src/core/use-cases/users/get-user-by-id.use-case.spec.tspackages/identity/src/core/use-cases/users/get-user-by-id.use-case.tspackages/identity/src/core/use-cases/users/get-user-profile.use-case.spec.tspackages/identity/src/core/use-cases/users/get-user-profile.use-case.tspackages/identity/src/core/use-cases/users/list-users-with-invitations.use-case.spec.tspackages/identity/src/core/use-cases/users/list-users-with-invitations.use-case.tspackages/identity/src/core/use-cases/users/remove-user.use-case.spec.tspackages/identity/src/core/use-cases/users/remove-user.use-case.tspackages/identity/src/identity.module.spec.tspackages/identity/src/identity.module.tspackages/identity/src/index.spec.tspackages/identity/src/index.tspackages/identity/src/interfaces/index.tspackages/identity/src/services/identity-event-publisher.service.ts
💤 Files with no reviewable changes (1)
- packages/identity/src/interfaces/index.ts
Comment on lines
+14
to
+16
| async execute(input: CreateUserInput) { | ||
| return this.userRepository.create(input); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider whether this thin wrapper adds sufficient value.
The execute method is a pure pass-through to userRepository.create(input) with no validation, orchestration, or error transformation. If future orchestration (e.g., event publishing, multi-step workflows) is planned, this structure is appropriate. Otherwise, controllers could inject the repository directly, reducing indirection.
🤖 Prompt for 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.
In `@packages/identity/src/core/use-cases/users/create-user.use-case.ts` around
lines 14 - 16, The execute method in CreateUserUseCase is a pure pass-through to
userRepository.create(input) so either add meaningful logic (validation, error
mapping, and side-effects) or remove the indirection: if you keep it, validate
CreateUserInput inside execute (e.g., required fields, email format), call
this.userRepository.create(input), wrap repository errors into domain errors,
and emit a UserCreated event via the event bus (e.g.,
eventBus.publish('UserCreated', user)); if you remove it, delete
CreateUserUseCase and update callers/controllers to inject and call
userRepository.create(...) directly, removing the thin wrapper.
Comment on lines
+38
to
+44
| } catch (error) { | ||
| if ( | ||
| error instanceof Error && | ||
| error.message.includes("not a member of this organization") | ||
| ) { | ||
| throw new NotFoundException("User not found in this organization"); | ||
| } |
Contributor
There was a problem hiding this comment.
Don’t couple the not-found path to an adapter error string.
This branch only returns NotFoundException when deleteIfNotLastAdmin() throws an English message containing "not a member of this organization". Any adapter wording change will turn the tenant-scoped 404 into a 500. Move this to a structured repository contract (for example a typed failure reason) or perform an explicit membership lookup before delete.
🤖 Prompt for 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.
In `@packages/identity/src/core/use-cases/users/remove-user.use-case.ts` around
lines 38 - 44, The current catch in remove-user.use-case.ts relies on matching
an English error string from deleteIfNotLastAdmin(), which is brittle; instead
either (A) add an explicit membership check before deletion (call a repository
method like usersRepository.isMember or organizationRepository.hasMember with
the userId and organizationId and throw NotFoundException if false) or (B)
change the repository contract so deleteIfNotLastAdmin() throws a typed/unique
error (e.g. MembershipError or NotMemberError) and catch that specific class
here to rethrow NotFoundException; update the catch to test the specific error
type (MembershipError/NotMemberError) rather than error.message.includes(...).
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 8 file(s) based on 9 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 8 file(s) based on 9 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/identity/src/identity.module.ts (1)
201-212:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHonor
eventPublisherTokeninregisterAsync().
register()supports a customIdentityModuleOptions.eventPublisherToken, but Lines 205-209 ignore the resolved option and always returndefaultPublisher. That silently changes behavior for async callers that provide a custom publisher fromuseFactory.Suggested fix
IdentityEventPublisher, { provide: IDENTITY_EVENT_PUBLISHER, - useFactory: (identityOptions: IdentityModuleOptions, defaultPublisher: IdentityEventPublisher) => { - // Honor eventPublisherToken from resolved options - // If a custom token was provided, it should be injected at index 2 - // Otherwise, use the default IdentityEventPublisher - return defaultPublisher; - }, - inject: [IDENTITY_OPTIONS, IdentityEventPublisher], + useFactory: ( + identityOptions: IdentityModuleOptions, + moduleRef: ModuleRef, + defaultPublisher: IdentityEventPublisher, + ) => { + if (!identityOptions.eventPublisherToken) { + return defaultPublisher; + } + + return moduleRef.get(identityOptions.eventPublisherToken, { + strict: false, + }); + }, + inject: [IDENTITY_OPTIONS, ModuleRef, IdentityEventPublisher], },🤖 Prompt for 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. In `@packages/identity/src/identity.module.ts` around lines 201 - 212, The provider for IDENTITY_EVENT_PUBLISHER currently ignores IdentityModuleOptions.eventPublisherToken and always returns defaultPublisher; update the provider's useFactory to accept (identityOptions: IdentityModuleOptions, defaultPublisher: IdentityEventPublisher, moduleRef: ModuleRef) and, if identityOptions.eventPublisherToken is set, resolve and return moduleRef.get(identityOptions.eventPublisherToken, { strict: false }) otherwise return defaultPublisher; also update the inject array to [IDENTITY_OPTIONS, IdentityEventPublisher, ModuleRef] and keep the provider name IDENTITY_EVENT_PUBLISHER so async register callers who supply a custom token are honored.
🤖 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 `@packages/identity/src/identity.module.ts`:
- Around line 6-7: The file imports unused symbols Inject and ModuleRef which
cause lint failures; remove Inject and ModuleRef from the import list in
identity.module.ts (keep Global and Module) unless you later use ModuleRef for
the async publisher fix—update the import statement to only import the actually
used symbols so the linter passes.
---
Duplicate comments:
In `@packages/identity/src/identity.module.ts`:
- Around line 201-212: The provider for IDENTITY_EVENT_PUBLISHER currently
ignores IdentityModuleOptions.eventPublisherToken and always returns
defaultPublisher; update the provider's useFactory to accept (identityOptions:
IdentityModuleOptions, defaultPublisher: IdentityEventPublisher, moduleRef:
ModuleRef) and, if identityOptions.eventPublisherToken is set, resolve and
return moduleRef.get(identityOptions.eventPublisherToken, { strict: false })
otherwise return defaultPublisher; also update the inject array to
[IDENTITY_OPTIONS, IdentityEventPublisher, ModuleRef] and keep the provider name
IDENTITY_EVENT_PUBLISHER so async register callers who supply a custom token are
honored.
🪄 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: 67382ae0-ad1e-412f-8953-60547c485387
📒 Files selected for processing (8)
apps/api/src/app/app.module.tsapps/worker/src/app.module.tspackages/identity/src/core/ports/outbound/types.tspackages/identity/src/core/use-cases/users/get-user-by-id.use-case.tspackages/identity/src/core/use-cases/users/get-user-profile.use-case.spec.tspackages/identity/src/core/use-cases/users/list-users-with-invitations.use-case.tspackages/identity/src/identity.module.spec.tspackages/identity/src/identity.module.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/identity/src/core/use-cases/users/get-user-by-id.use-case.spec.ts (2)
42-49: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd mock call assertions to verify repository method invocations.
The test verifies the user is returned but doesn't confirm that
findByIdandfindOneForUserwere called with the expected arguments. Following the pattern inget-user-profile.use-case.spec.ts(line 24), add assertions to ensure the use case invokes repositories correctly.🧪 Suggested assertions
const result = await useCase.execute("user1", "tenant1"); expect(result).toEqual(user); + expect(userRepository.findById).toHaveBeenCalledWith("user1"); + expect(tenantRepository.findOneForUser).toHaveBeenCalledWith("user1", "tenant1"); });🤖 Prompt for 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. In `@packages/identity/src/core/use-cases/users/get-user-by-id.use-case.spec.ts` around lines 42 - 49, The test is missing verifications that the repositories were called with the expected arguments; after calling useCase.execute("user1", "tenant1") add assertions that userRepository.findById was invoked with "user1" and tenantRepository.findOneForUser was invoked with { userId: "user1", tenantId: "tenant1" } (or the exact shape your use case passes) to mirror the pattern in get-user-profile.use-case.spec.ts; use expect(userRepository.findById).toHaveBeenCalledWith("user1") and expect(tenantRepository.findOneForUser).toHaveBeenCalledWith(/* args your useCase uses */) and also consider toHaveBeenCalledTimes(1) where appropriate.
32-40: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd mock call assertions to verify repository method invocations.
The test verifies the exception is thrown but doesn't confirm that
findByIdandfindOneForUserwere called with the expected arguments. Following the pattern inget-user-profile.use-case.spec.ts(lines 24, 31), add assertions to strengthen test confidence.🧪 Suggested assertions
await expect(useCase.execute("user1", "tenant1")).rejects.toThrow( NotFoundException, ); + expect(userRepository.findById).toHaveBeenCalledWith("user1"); + expect(tenantRepository.findOneForUser).toHaveBeenCalledWith("user1", "tenant1"); });🤖 Prompt for 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. In `@packages/identity/src/core/use-cases/users/get-user-by-id.use-case.spec.ts` around lines 32 - 40, The test "should throw NotFoundException if user is not member of tenant" currently asserts the error but not that repository methods were invoked; add mock call assertions after the expect to verify userRepository.findById was called with "user1" and tenantRepository.findOneForUser was called with the returned user and "tenant1" (or with the user id per your implementation), using the mocked methods' toHaveBeenCalledWith/toHaveBeenCalledTimes assertions so the test confirms both calls occurred as expected when useCase.execute("user1", "tenant1") is invoked.apps/api/src/modules/identity/system-admin/system-admin.controller.spec.ts (1)
207-230: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAssert that the new role-repository dependency is actually used.
This test now stubs
mockRoleProvider.findById(), but it never verifies thatSystemAdminController.createUser()consultedROLE_REPOSITORYbefore creating the user. If that validation call disappears, the test still passes.✅ Tighten the happy-path assertion
const result = await controller.createUser({ name: 'Test', email: 'new@example.com', role: 'member', }); expect(result).toEqual(mockUser); + expect(mockRoleProvider.findById).toHaveBeenCalled(); expect(mockUserProvider.create).toHaveBeenCalledWith({ name: 'Test', email: 'new@example.com', role: 'member', });🤖 Prompt for 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. In `@apps/api/src/modules/identity/system-admin/system-admin.controller.spec.ts` around lines 207 - 230, The test currently stubs mockRoleProvider.findById but never asserts it was used; update the test for SystemAdminController.createUser to explicitly assert that mockRoleProvider.findById was called with the requested role (e.g., expect(mockRoleProvider.findById).toHaveBeenCalledWith('member')) before asserting mockUserProvider.create was invoked, so the test fails if createUser stops consulting ROLE_REPOSITORY (check references to createUser, mockRoleProvider.findById, and mockUserProvider.create).packages/identity/src/identity.module.spec.ts (1)
95-115: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAssert the concrete adapter classes, not just that
useClassexists.These checks still pass if the wrong adapter is bound to a token. For this migration, the spec should compare each provider’s
useClassto the expected repository/auth adapter so cross-token wiring regressions fail loudly.✅ Tighten the wiring assertions
import { AUTH_PROVIDER, USER_REPOSITORY, TENANT_REPOSITORY, PERMISSION_REPOSITORY, ROLE_REPOSITORY, IDENTITY_OPTIONS, } from "./constants.js"; +import { BetterAuthAdapter } from "./adapters/outbound/better-auth.adapter.js"; +import { DrizzleUserRepositoryAdapter } from "./adapters/outbound/drizzle-user.repository.js"; +import { DrizzleTenantRepositoryAdapter } from "./adapters/outbound/drizzle-tenant.repository.js"; +import { DrizzlePermissionRepositoryAdapter } from "./adapters/outbound/drizzle-permission.repository.js"; +import { DrizzleRoleRepositoryAdapter } from "./adapters/outbound/drizzle-role.repository.js"; ... - expect(authProv.useClass).toBeDefined(); + expect(authProv.useClass).toBe(BetterAuthAdapter); ... - expect(userProv.useClass).toBeDefined(); + expect(userProv.useClass).toBe(DrizzleUserRepositoryAdapter); ... - expect(tenantProv.useClass).toBeDefined(); + expect(tenantProv.useClass).toBe(DrizzleTenantRepositoryAdapter); ... - expect(permProv.useClass).toBeDefined(); + expect(permProv.useClass).toBe(DrizzlePermissionRepositoryAdapter); ... - expect(roleProv.useClass).toBeDefined(); + expect(roleProv.useClass).toBe(DrizzleRoleRepositoryAdapter);🤖 Prompt for 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. In `@packages/identity/src/identity.module.spec.ts` around lines 95 - 115, The tests currently only assert useClass is defined; change them to assert the exact concrete adapter classes are wired by replacing the loose checks in the assertClassProvider results (for AUTH_PROVIDER, USER_REPOSITORY, TENANT_REPOSITORY, PERMISSION_REPOSITORY, ROLE_REPOSITORY) with strict equality checks against the expected adapter classes (e.g. the concrete Auth adapter and the concrete repository adapter classes used in your module) so that assertClassProvider(...).useClass is compared to the specific class (use expect(...useClass).toBe(ExpectedAuthAdapter / ExpectedUserRepositoryAdapter / ExpectedTenantRepositoryAdapter / ExpectedPermissionRepositoryAdapter / ExpectedRoleRepositoryAdapter).
🤖 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 `@packages/identity/src/adapters/outbound/drizzle-user.repository.ts`:
- Around line 300-303: The empty membershipWithRole result is being mapped to
UserNotFoundError even when the tenantId does not exist; update the logic in the
repository (around the FOR UPDATE select and the membershipWithRole check) to
first verify the tenant/organization exists (e.g., inspect the result of the FOR
UPDATE select or run an existence check for tenantId) and throw a
TenantNotFoundError (or another tenant-specific error) when the tenant is
missing, otherwise keep throwing UserNotFoundError when the tenant exists but
the membershipWithRole array is empty; reference membershipWithRole,
UserNotFoundError, tenantId and the earlier FOR UPDATE select to locate where to
add the tenant existence check and adjust the thrown error accordingly.
---
Outside diff comments:
In `@apps/api/src/modules/identity/system-admin/system-admin.controller.spec.ts`:
- Around line 207-230: The test currently stubs mockRoleProvider.findById but
never asserts it was used; update the test for SystemAdminController.createUser
to explicitly assert that mockRoleProvider.findById was called with the
requested role (e.g.,
expect(mockRoleProvider.findById).toHaveBeenCalledWith('member')) before
asserting mockUserProvider.create was invoked, so the test fails if createUser
stops consulting ROLE_REPOSITORY (check references to createUser,
mockRoleProvider.findById, and mockUserProvider.create).
In `@packages/identity/src/core/use-cases/users/get-user-by-id.use-case.spec.ts`:
- Around line 42-49: The test is missing verifications that the repositories
were called with the expected arguments; after calling useCase.execute("user1",
"tenant1") add assertions that userRepository.findById was invoked with "user1"
and tenantRepository.findOneForUser was invoked with { userId: "user1",
tenantId: "tenant1" } (or the exact shape your use case passes) to mirror the
pattern in get-user-profile.use-case.spec.ts; use
expect(userRepository.findById).toHaveBeenCalledWith("user1") and
expect(tenantRepository.findOneForUser).toHaveBeenCalledWith(/* args your
useCase uses */) and also consider toHaveBeenCalledTimes(1) where appropriate.
- Around line 32-40: The test "should throw NotFoundException if user is not
member of tenant" currently asserts the error but not that repository methods
were invoked; add mock call assertions after the expect to verify
userRepository.findById was called with "user1" and
tenantRepository.findOneForUser was called with the returned user and "tenant1"
(or with the user id per your implementation), using the mocked methods'
toHaveBeenCalledWith/toHaveBeenCalledTimes assertions so the test confirms both
calls occurred as expected when useCase.execute("user1", "tenant1") is invoked.
In `@packages/identity/src/identity.module.spec.ts`:
- Around line 95-115: The tests currently only assert useClass is defined;
change them to assert the exact concrete adapter classes are wired by replacing
the loose checks in the assertClassProvider results (for AUTH_PROVIDER,
USER_REPOSITORY, TENANT_REPOSITORY, PERMISSION_REPOSITORY, ROLE_REPOSITORY) with
strict equality checks against the expected adapter classes (e.g. the concrete
Auth adapter and the concrete repository adapter classes used in your module) so
that assertClassProvider(...).useClass is compared to the specific class (use
expect(...useClass).toBe(ExpectedAuthAdapter / ExpectedUserRepositoryAdapter /
ExpectedTenantRepositoryAdapter / ExpectedPermissionRepositoryAdapter /
ExpectedRoleRepositoryAdapter).
🪄 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: bbfa3012-2ba0-41f6-865c-95cb03b8e825
📒 Files selected for processing (8)
apps/api/src/modules/identity/system-admin/system-admin.controller.spec.tspackages/identity/src/adapters/outbound/drizzle-user.repository.tspackages/identity/src/core/use-cases/users/get-user-by-id.use-case.spec.tspackages/identity/src/core/use-cases/users/get-user-by-id.use-case.tspackages/identity/src/core/use-cases/users/get-user-profile.use-case.spec.tspackages/identity/src/identity.module.spec.tspackages/identity/src/identity.module.tspackages/identity/src/index.spec.ts
This was referenced Jun 13, 2026
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
New Features
Bug Fixes
Chores