Repository navigation
Feat/invitation flow rbac - #47
Merged
Merged
Conversation
…ole field\n\n- Added 'role: user' to CreateUserValidation payloads in system-admin.controller.spec.ts\n- Resolves TypeScript errors blocking build
- Fixed all TypeScript lint errors in test files (drizzle-role, drizzle-user, pii-cleanup specs) - Added proper type definitions for mock objects using vitest Mock type - Implemented PII cleanup background job with accurate row counting via .returning() - Improved type safety by replacing double type assertions with proper imports - Enhanced regex patterns to avoid overmatching (e.g., 'user not found' vs 'user session not found') - Extracted magic numbers into named constants (BATCH_DELAY_MS) - Fixed unsafe type assignments and member access violations - Added proper error handling and iteration limits to prevent infinite loops - Fixed TypeScript type errors in test mocks (uuid, provider tokens) - All tests passing (70/70) with 86.29% coverage - Zero lint errors across all packages
Contributor
📝 WalkthroughWalkthroughThis PR introduces role-based access control infrastructure, refactors authentication and invitation flows with enhanced error handling and header propagation, adds database utilities and background tasks, implements frontend invitation UI improvements, and updates the database schema to support role management with cascading deletes and constraint enforcement. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Web as Web App
participant API as API Server
participant AuthProvider as Auth Provider<br/>(Better Auth)
participant DB as Database
User->>Web: Access invite link /<br/>complete-invite
Web->>Web: Parse inviteId from URL
Web->>API: GET /auth/me (check session)
API-->>Web: Session or 401
alt Not Logged In
Web->>Web: Redirect to signup<br/>with invite context
User->>Web: Submit signup form<br/>(email, firstName,<br/>lastName, password)
Web->>API: POST /auth/complete-invite<br/>(email, password, invite)
else Logged In
Web->>API: POST /auth/complete-invite<br/>(silent accept, token)
end
API->>API: Extract x-request-id,<br/>x-forwarded-for headers
API->>AuthProvider: createUser or verify existing<br/>(firstName, lastName,<br/>email, role, headers)
AuthProvider->>DB: Insert/fetch user
AuthProvider-->>API: User object
alt New User from Signup
API->>API: Proceed with acceptance<br/>if user created
else Existing User
API->>API: Skip creation,<br/>proceed to acceptance
end
API->>AuthProvider: acceptInvitation<br/>(inviteId, userId)
AuthProvider->>DB: Update invitation status<br/>to accepted
AuthProvider->>DB: Force email verification
AuthProvider-->>API: Session/login token
API->>DB: Return login session
API-->>Web: Set-Cookie + Redirect
Web->>Web: Navigate to /dashboard
Web-->>User: Dashboard loaded
sequenceDiagram
participant Admin as Admin User
participant Web as Web App
participant API as API Server
participant AuthService as AuthService
participant AuthProvider as Auth Provider
participant TenantProvider as Tenant Provider
Admin->>Web: Click "Invite User"
Web->>API: POST /invitations<br/>(email, role,<br/>headers: x-request-id)
API->>AuthService: invitationsService.create<br/>(createInvitation, userId,<br/>forwardedHeaders)
AuthService->>AuthProvider: createInvitation<br/>(email, role, headers)
AuthProvider-->>AuthService: Invitation object
AuthService-->>API: Invitation saved
API-->>Web: { data: invitation }
Web->>Web: Show "Invitation Sent"<br/>success toast
Web->>API: GET /users?scope=org<br/>(refresh list)
API->>AuthService: userProvider.findAll<br/>(tenantId)
AuthService->>AuthProvider: Query users
AuthProvider-->>AuthService: Users + role info
API->>AuthService: invitationsService.list<br/>(tenantId)
AuthService-->>API: Pending invitations
API->>API: Merge users +<br/>pending invitations
API-->>Web: { data: [users,<br/>invitations], total }
Web->>Web: Update user list<br/>with new invitee
Web-->>Admin: List updated
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 50
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (17)
apps/web/src/shared/components/layout/Sidebar.tsx (1)
72-85:⚠️ Potential issue | 🟠 MajorNested interactive elements:
<button>inside<a>is invalid HTML.Wrapping a
<Button>(which renders a<button>element) inside a<Link>(which renders an<a>) produces invalid HTML per the spec and breaks keyboard accessibility (two tab stops, ambiguous screen-reader semantics).Use
asChildon theButtonso it merges into theLink, or make theLinkthe child withasChildonButton:♻️ Proposed fix — use Button's `asChild` prop
- <Link key={item.href} to={item.href}> - <Button - variant="ghost" - className={cn( - "w-full justify-start transition-all duration-200 font-medium", - isActive - ? "bg-primary/10 text-primary border-l-4 border-primary rounded-l-none" - : "text-muted-foreground hover:text-foreground hover:bg-muted/50" - )} - > - <item.icon className={cn("mr-3 h-4 w-4", isActive ? "text-primary" : "text-muted-foreground")} /> - {item.label} - </Button> - </Link> + <Button + key={item.href} + variant="ghost" + asChild + className={cn( + "w-full justify-start transition-all duration-200 font-medium", + isActive + ? "bg-primary/10 text-primary border-l-4 border-primary rounded-l-none" + : "text-muted-foreground hover:text-foreground hover:bg-muted/50" + )} + > + <Link to={item.href}> + <item.icon className={cn("mr-3 h-4 w-4", isActive ? "text-primary" : "text-muted-foreground")} /> + {item.label} + </Link> + </Button>apps/api/src/modules/identity/system-admin/system-admin.controller.ts (2)
280-281: 🧹 Nitpick | 🔵 TrivialTODO: Safety check for deleting the last platform admin is missing.
This is a critical safety gap — accidentally deleting the last admin would lock everyone out of the system admin panel. Consider prioritizing this before release.
Would you like me to open an issue to track implementing the last-admin deletion guard?
68-71:⚠️ Potential issue | 🟡 Minor
createSystemInvitationandinviteUserboth throwBadRequestExceptionfor unauthorized — considerUnauthorizedException(401) instead.Both
createSystemInvitation(line 97) andinviteUser(line 70) throwBadRequestExceptionwhen the session is missing. A401 Unauthorized(UnauthorizedException) would be semantically correct and more helpful for API consumers, since the issue is a missing/invalid session, not malformed input.Proposed fix (apply similarly on line 97)
-import { +import { ... BadRequestException, + UnauthorizedException, ... } from '@nestjs/common'; if (!session?.user) { - throw new BadRequestException('Unauthorized'); + throw new UnauthorizedException('Valid session required'); }apps/web/src/shared/lib/auth/AuthProvider.spec.tsx (2)
109-145:⚠️ Potential issue | 🟡 MinorTest doesn't differentiate
/users/meand/tenantsresponses.The mock on line 125 sets
apiClient.getto return tenant data for all GET calls, including the/users/mecall introduced in the AuthProvider. The test passes coincidentally because the profile merge ({ ...apiUser, ...fullUser }) doesn't break whenfullUseris an array. However, this makes the test fragile and semantically inaccurate.Use a URL-aware mock to return the correct response shape per endpoint:
♻️ Proposed fix
- (apiClient.get as Mock).mockResolvedValue({ - data: [ - { id: 'orgA', name: 'Org A', createdAt: '2023-01-01' }, - { id: 'orgB', name: 'Latest Org', createdAt: '2024-01-01' } // Should pick this one - ] - }); + (apiClient.get as Mock).mockImplementation((url: string) => { + if (url === '/tenants') { + return Promise.resolve({ + data: [ + { id: 'orgA', name: 'Org A', createdAt: '2023-01-01' }, + { id: 'orgB', name: 'Latest Org', createdAt: '2024-01-01' } + ] + }); + } + return Promise.resolve({ data: {} }); // /users/me fallback + });
46-51: 🧹 Nitpick | 🔵 TrivialConsider adding test coverage for new error paths.
The AuthProvider now has several new branches that lack test coverage:
/users/mereturning a 401 (should clear auth state)./users/mefailing with a non-401 error (should fall back to session data).- Empty tenant list (should hydrate with base user, no org).
- The
logincallback name derivation logic.These paths have explicit fallback behavior that would benefit from regression tests.
apps/web/src/shared/lib/auth/AuthProvider.tsx (1)
8-18:⚠️ Potential issue | 🟡 MinorLocal
Tenantinterface diverges from canonical package type.The local
Tenantinterface in AuthProvider.tsx differs frompackages/identity/src/interfaces/types.ts:
status: local uses"archived"but canonical uses"disabled"slug: local requires it, but canonical makes it optional (string | null | undefined)isSystem: local requires it, but canonical makes it optional (boolean | undefined)The API and other parts of the codebase (e.g.,
apps/web/src/modules/identity/tenants/types.ts) use"disabled"for the status. Consider importing the canonicalTenanttype from the identity package to maintain consistency.apps/api/src/modules/identity/auth/auth.service.ts (1)
195-212:⚠️ Potential issue | 🟡 MinorResolve the uncertain error-swallowing comment — this reads as an open question in production code.
Lines 207-208 contain
// Swallow error to preserve user account? // Or we could rollback. For now, we swallow as per original logic.— this is fine as a design choice, but the question-mark framing signals unresolved intent. If the decision is to keep the user on tenant-provisioning failure, replace with a definitive comment explaining why (e.g., "User can manually create a tenant later" or "Tenant provisioning is retried asynchronously"). Otherwise, implement a compensating rollback.The current behavior means a user could end up in a state with no tenant and no obvious recovery path from the UI.
apps/web/src/modules/identity/pages/SignupPage.tsx (2)
136-140: 🧹 Nitpick | 🔵 Trivial
alert()is a poor UX fallback.Line 138 uses
alert()when auto-login fails after invite completion. This blocks the UI thread and is inconsistent with the polished error handling elsewhere (e.g., thesetErrorJSX pattern on lines 176-198). Consider usingsetErrorwith an appropriate message here as well.Proposed fix
} else { - // Fallback (Should not happen with new endpoint) - alert("Account created, but auto-login failed. Please log in."); - navigate('/login'); + // Fallback (Should not happen with new endpoint) + setError( + <div className="flex flex-col gap-2 items-center"> + <span>Account created, but auto-login failed.</span> + <Button + variant="outline" + size="sm" + className="w-full mt-1" + onClick={() => navigate('/login')} + > + Go to Login + </Button> + </div> + ); }
60-67: 🧹 Nitpick | 🔵 TrivialName derivation logic vs. required fields.
firstNameandlastNameare markedrequiredin the form (lines 234, 247), yet the submission still falls back to deriving names from the email (lines 62-63). If both fields are truly required, the fallback is dead code for normal form submissions. If the fallback is intentional (e.g., for programmatic/test usage), consider adding a brief comment explaining the rationale to avoid confusion.apps/web/src/modules/identity/pages/SignupPage.spec.tsx (1)
32-34:⚠️ Potential issue | 🟡 MinorUse
vi.stubEnv()instead ofvi.stubGlobal('import.meta', ...)at lines 13-15, and remove debug console.log statements.
vi.stubGlobal('import.meta', ...)is ineffective—import.metais a compile-time construct handled by Vite, not a runtime global. The correct approach isvi.stubEnv(), which is already used insrc/test/setup.tsandLoginPage.spec.tsx. Replace lines 13-15 withvi.stubEnv('VITE_API_URL', 'http://test-api.com');. Additionally, remove the debugconsole.logstatements at lines 115, 130, 137, and 140.packages/identity/src/adapters/better-auth.adapter.ts (1)
707-866: 🛠️ Refactor suggestion | 🟠 Major
mapUseris excessively complex (~160 LOC) with multiple fallback paths.The method has high cyclomatic complexity: global role lookup → eager-loaded members → fallback member query → supplementary permission query → fail-safe defaults, all with extensive inline comments explaining design decisions. Consider extracting role/permission resolution into a dedicated helper (e.g.,
resolveUserPermissions).apps/api/src/modules/identity/invitations/invitations.controller.spec.ts (1)
46-66: 🧹 Nitpick | 🔵 TrivialConsider adding a test case where
x-request-idandx-forwarded-forare present.The current test only validates the
undefinedcase. A second test with actual header values would confirm that the controller correctly extracts and forwards them.apps/api/src/modules/identity/auth/auth.service.spec.ts (1)
245-276:⚠️ Potential issue | 🟡 MinorStale
describeblock name: should be'registerUser'instead of'createUser'.The method was renamed to
registerUserbut thedescribelabel on Line 245 still reads'createUser'.Proposed fix
- describe('createUser', () => { + describe('registerUser', () => {packages/identity/src/interfaces/user-provider.interface.ts (1)
41-53:⚠️ Potential issue | 🟡 MinorJSDoc
@returnsis stale — doesn't match the new return type.Line 48 still says
@returns true if deleted, false if user was the last admin, but the return type is nowPromise<{ success: boolean; hardDeleted?: boolean }>. Update the doc to describe the structured result.Proposed fix
- * `@returns` true if deleted, false if user was the last admin + * `@returns` An object with `success` (true if deleted, false if user was the last admin) and optional `hardDeleted` flagapps/api/src/modules/identity/auth/auth.controller.ts (1)
192-204:⚠️ Potential issue | 🟡 MinorPassword change for existing provisioned users is not rolled back on invitation acceptance failure.
When
isNewUseris false (existing provisioned user), line 176 callssetPassword, but ifinvitationsService.acceptfails on line 193, only new users are cleaned up (line 196-198). The existing user's password has already been changed but the invitation wasn't accepted, leaving the user in an inconsistent state (new password set, but not a member of the org).This may be acceptable if the user can retry, but worth documenting or handling.
apps/api/src/modules/identity/users/users.controller.ts (1)
106-123:⚠️ Potential issue | 🟠 MajorNo pagination support — returns only the first page of users plus all invitations.
findAlldelegates tothis.userProvider.findAll({ tenantId })without forwardingpage/limitfrom query parameters. The provider defaults to page 1, limit 10. Meanwhile,invitationsService.list(tenantId)fetches all invitations unbounded. For organizations with many users/invitations, this will:
- Silently truncate the user list to 10 items.
- Return all invitations regardless of size.
- Return a misleading
total(from the provider) that doesn't match the truncateddata.Consider accepting
@Query()pagination parameters and forwarding them to the provider, or documenting that this endpoint is intentionally unpaginated.packages/identity/src/schema.ts (1)
269-282:⚠️ Potential issue | 🟡 MinorAdd foreign key constraint to
invitation.rolereferencingrole.id.
invitation.role(line 275) stores role identifiers (e.g., "user", "member") that correspond torole.idvalues, but has no foreign key reference. This allows invitations to reference deleted or non-existent roles. Add.references(() => role.id)with an appropriateonDeletestrategy (e.g.,"cascade"or"restrict") to enforce referential integrity.
🤖 Fix all issues with AI agents
In `@apps/api/package.json`:
- Around line 79-81: Remove the unused "ts-node" devDependency from package.json
and update the outdated usage message in manage.ts to reference "tsx manage.ts"
instead of "ts-node manage.ts"; specifically edit the devDependencies entry for
"ts-node" in package.json to delete it and update the string in the usage output
inside the manage.ts file (the message around the existing line that currently
suggests "ts-node manage.ts") so all scripts and messages consistently reference
"tsx".
In `@apps/api/src/modules/identity/auth/auth.controller.ts`:
- Around line 130-133: The INFO-level log in CompleteInvite that logs body.email
is logging PII; change it so the email is not logged at INFO—either remove it
entirely or downgrade it to DEBUG and/or mask the value before logging; update
the calls in auth.controller.ts (the this.logger.log(...) and the
this.logger.debug(...) lines within the CompleteInvite handler) so that no raw
email is emitted at INFO and only non-identifying or masked fields are logged at
DEBUG.
In `@apps/api/src/modules/identity/invitations/invitations.controller.ts`:
- Around line 25-32: Remove the duplicated organization enforcement block in the
invitations controller: there are two identical if (req.user.organizationId) {
createInvitation.organizationId = req.user.organizationId; } blocks; keep a
single instance (inside the controller method handling invitation creation) and
delete the redundant copy so createInvitation.organizationId is set only once.
In `@apps/api/src/modules/identity/roles/roles.controller.ts`:
- Around line 49-54: The findById endpoint currently returns { data: null } when
roleProvider.findById(id) yields null; update the RolesController.findById
method to check the returned value and throw a NotFoundException (e.g., new
NotFoundException(`Role with id ${id} not found`)) when null, otherwise return {
data: role }; ensure NotFoundException is imported from `@nestjs/common` if not
already present.
- Around line 42-47: Create and attach Zod validation for the create/update
endpoints: define Zod schemas for CreateRoleInput and UpdateRoleInput (e.g.,
createRoleSchema, updateRoleSchema) and apply them to the controller
actions—either by decorating the DTOs or by adding `@UsePipes`(new
ZodValidationPipe(createRoleSchema)) on the create method and `@UsePipes`(new
ZodValidationPipe(updateRoleSchema)) on the update method in
roles.controller.ts; ensure you reference the existing CreateRoleInput and
UpdateRoleInput types and keep the roleProvider.create and roleProvider.update
calls intact so validated data is passed through.
In `@apps/api/src/modules/identity/system-admin/system-admin.controller.spec.ts`:
- Line 188: The test in system-admin.controller.spec.ts uses an invalid role
string ('user') that doesn't match CreateUserSchema's enum; replace occurrences
of role: 'user' (around the test blocks including the ones noted at lines ~188,
204, 211) with a valid role id such as getRequiredOwnerRoleId() (or
getRequiredAdminRoleId()) so the test payloads match buildCreateUserSchema()'s
z.enum and align with other tests like createSystemInvitation; update all
instances to reference the helper function instead of the literal 'user'.
In `@apps/api/src/modules/identity/system-admin/system-admin.controller.ts`:
- Around line 141-154: The createUser method currently unsafely asserts
CreateUserValidation to CreateUserDto; remove the manual cast and enforce
validated typing by applying the same validation pipe used for
createSystemInvitation: add `@UsePipes`(new
LazyZodValidationPipe(buildCreateUserSchema)) to the createUser handler and
change the parameter type from CreateUserValidation to CreateUserDto (or accept
CreateUserDto directly), so the body is validated/typed correctly before you
call this.userProvider.create; alternatively, if you must keep the DTO class,
remove the "as CreateUserDto" cast and rely on an appropriate validation pipe to
produce a safe CreateUserDto instance.
In `@apps/api/src/modules/identity/system-admin/system-admin.validation.ts`:
- Around line 57-64: The module-level cache _cachedCreateUserSchema causes test
isolation issues because getCreateUserSchema() can return a schema built with
mocked role IDs that persists across tests; fix by either removing the cache and
always returning buildCreateUserSchema(), or add and export a
resetCreateUserSchemaCache() function that sets _cachedCreateUserSchema = null
and update tests to call it in teardown; target the symbols
_cachedCreateUserSchema, getCreateUserSchema, and buildCreateUserSchema when
making the change.
- Around line 77-79: The function buildCreateSystemInvitationSchema currently
calls buildCreateUserSchema() directly and so bypasses the cached schema; change
it to call getCreateUserSchema() and then .pick({ email: true, role: true })
instead, ensuring you import or use the existing getCreateUserSchema accessor;
keep the same return shape but rely on the cached accessor to avoid creating a
fresh schema each call.
- Around line 66-71: Replace the z.lazy wrapper so createZodDto receives the
concrete schema: change CreateUserSchema to directly call getCreateUserSchema()
(e.g., const CreateUserSchema = getCreateUserSchema()) and keep exporting
CreateUserValidation as class CreateUserValidation extends
createZodDto(CreateUserSchema) {}; update any dependent code (e.g., remove the
manual cast in system-admin.controller where input is cast to CreateUserDto) so
the DTO type is inferred correctly by TypeScript.
In `@apps/api/src/modules/identity/users/users.controller.ts`:
- Around line 30-50: UserListItem uses two discriminators (kind and
isInvitation) which is redundant; make the union a clean discriminated union by
removing isInvitation from the invitation variant and making kind a required
literal on both branches (set user variant to { kind: 'user' } and invitation
variant to { kind: 'invitation' }), then update any consumers that check
isInvitation to instead check item.kind === 'invitation' (or adjust tests/uses
of UserListItem accordingly).
- Around line 148-155: The total returned for pagination is using the unfiltered
invitations array (invitations.length) which mismatches the merged data
(combinedData built from users and pendingInvitations); update the return to
compute total using userTotal + pendingInvitations.length instead of
invitations.length so pagination metadata matches combinedData (refer to
combinedData, users, invitedUsers, userTotal, invitations, pendingInvitations).
In `@apps/api/src/scripts/check-user-permissions.ts`:
- Line 1: The file contains an unnecessary side-effect import "reflect-metadata"
that isn't used; remove the import statement (the line import
'reflect-metadata';) from the script so the module isn't pulled in unnecessarily
and no decorators/reflection code is referenced (check
apps/api/src/scripts/check-user-permissions.ts and delete that import).
- Around line 41-64: The logs in check-user-permissions.ts print full emails
(e.g., targetEmail, user.email, users[0].email) which leaks PII; update the
script to reuse the existing email-masking helper (or extract one if missing)
and replace plain email usage in all console.log statements (the initial lookup
log, the defaulting-to-first-user log, and the "Checking permissions for" and
"User Record" logs) to show maskedEmail unless an explicit --unmask flag is
provided; ensure masking is applied to variables user.email and targetEmail and
that the flag controls whether maskedEmail or the real email is printed.
In `@apps/api/src/scripts/manage.ts`:
- Around line 166-179: Duplicate admin env-resolution logic in bootstrapAdmin
and forceResetAdmin should be extracted into a single helper (e.g., getAdminEnv)
that returns { EMAIL, PASSWORD, FIRST_NAME, LAST_NAME }; implement getAdminEnv
to encapsulate the existing precedence: EMAIL from ADMIN_EMAIL or
BOOTSTRAP_ADMIN_EMAIL, PASSWORD from ADMIN_PASSWORD or BOOTSTRAP_ADMIN_PASSWORD,
FULL_NAME from ADMIN_NAME or BOOTSTRAP_ADMIN_NAME or 'Platform Admin', then
derive FIRST_NAME as ADMIN_FIRST_NAME or first token of FULL_NAME and LAST_NAME
as ADMIN_LAST_NAME or remaining tokens or 'User'. Replace the duplicated blocks
in bootstrapAdmin and forceResetAdmin with a call const { EMAIL, PASSWORD,
FIRST_NAME, LAST_NAME } = getAdminEnv(); and ensure getAdminEnv is colocated or
exported where both functions can import it.
- Around line 305-312: Add a startup cleanup that reverts stale "archived_*"
renames: implement a function (e.g., cleanupStaleArchivedEmails) that queries
users where email LIKE 'archived_%' and updated_at is older than a short
threshold, reconstructs the original email by stripping the "archived_" prefix
(since tempEmail is set as `archived_${original}` in the rename code using
db.update(schema.user).set({ email: tempEmail }).where(eq(schema.user.id,
userId))), and updates the row back to the original; call this function at
script startup before performing new rename/signup work so a crashed run can be
recovered instead of leaving orphaned archived_* emails and ensure it targets
the same records your existing rollback/catch-block logic touches.
In `@apps/api/src/scripts/rename-column.ts`:
- Around line 8-57: The standalone rename script (main, Client usage in
rename-column.ts) must be converted into a proper Drizzle SQL migration so the
column change is tracked and executed by db:migrate; create a new migration file
in your Drizzle migrations folder containing the same idempotent SQL (an ALTER
TABLE "user" RENAME COLUMN "image_url" TO "image" guarded by an IF EXISTS or an
equivalent safe check), commit that migration so it runs in order with other
migrations, remove or disable the standalone rename-column.ts invocation, and
ensure the migration is picked up by the existing migration runner (so
db:migrate applies it automatically).
In `@apps/web/src/modules/identity/pages/AcceptInvitePage.tsx`:
- Line 31: The code in AcceptInvitePage reads import.meta.env.VITE_API_URL
inside the effect which can be undefined; extract const API_URL =
import.meta.env.VITE_API_URL to module scope (top of AcceptInvitePage.tsx) and
add a guard like if (!API_URL) throw new Error("VITE_API_URL is missing") so any
missing env is a clear error; then remove the local declaration inside the
component/effect and reference the module-level API_URL in the fetch call within
the useEffect (or AcceptInvitePage component) to avoid silent invalid fetch
URLs.
In `@apps/web/src/modules/identity/pages/SignupPage.spec.tsx`:
- Around line 115-116: Remove debug console.log statements from the test file
SignupPage.spec.tsx: delete the console.log("Starting invite flow test") and any
other console.log calls left in the spec (they are debugging artifacts within
the invite flow tests). Search for console.log in SignupPage.spec.tsx and remove
those lines so tests no longer emit noisy debug output.
In `@apps/web/src/modules/identity/pages/SignupPage.tsx`:
- Around line 175-198: The current fragile string-match in the SignupPage error
handling should be replaced with structured error-code checking: when handling
the signup response (where errorMessage is derived), prefer inspecting a
structured field like error.code or response.error.code (e.g., "USER_EXISTS")
and branch on that to call setError(...) with the existing JSX redirect block;
keep a fallback to the existing toLowerCase string checks for backward
compatibility. Update the handler that calls setError (referencing errorMessage,
setError and navigate) to first check for error?.code === "USER_EXISTS" (or
similar backend contract) before falling back to message substring checks, and
update corresponding backend contract/typing where applicable.
- Around line 168-169: The catch block in the signup flow (catch in
SignupPage.tsx, likely inside the submit/handleSubmit function) currently calls
console.error(err) unguarded; change it to only log in development by wrapping
the log with the same import.meta.env.DEV check used elsewhere in this file
(e.g., lines near existing DEV guards) so raw error details are not emitted in
production, and retain any user-facing error handling/notification logic
unchanged.
- Around line 74-82: The catch block inside SignupPage's handleSubmit is using
catch (e) which shadows the handleSubmit event parameter named e; rename the
catch variable to something non-conflicting (e.g., err or parseError) so it no
longer shadows the outer event parameter used by handleSubmit, update the
console.error call to use the new name, and ensure this change is applied within
the try/catch that parses redirectUrl using new URL in SignupPage.
In `@apps/web/src/modules/identity/users/InviteUserDialog.tsx`:
- Around line 82-90: The dialog keeps stale form state because form.reset() is
only called after submit; update the InviteUserDialog useEffect that runs on
open (the one currently defaulting the role) to reset the form when the dialog
opens: call form.reset() (or form.reset({ role: def }) to set the default role
immediately) as soon as open is true and roles are available, then apply the
existing logic that sets a default role via form.setValue when appropriate;
reference the InviteUserDialog component, the useEffect that reads
open/roles/form, and the form.reset, form.getValues, form.setValue helpers.
In `@apps/web/src/shared/lib/auth/AuthProvider.tsx`:
- Around line 109-114: The test's single apiClient.get mock is shared by both
refreshSession calls to '/users/me' and '/tenants', causing tenant overrides to
also affect the user call; update the test to use (apiClient.get as
Mock).mockImplementation that checks the url argument and returns a user-shaped
response for '/users/me', tenant array for '/tenants', and a safe default for
other endpoints so refreshSession sees correct data for both calls (targets:
refreshSession, apiClient.get, '/users/me', '/tenants').
- Around line 86-98: The catch block in AuthProvider that logs profileError in
development exposes sensitive request/response data; instead, change the logging
to emit only the sanitized details already computed (errorMessage and status)
and avoid printing the full profileError object. In the catch for profileError
(inside AuthProvider), keep the 401 re-throw logic, and replace the console.warn
that currently logs profileError with a message that includes errorMessage and
the status variable (and optionally mark status as "unknown" if undefined) so
dev logs match production and do not leak tokens/headers/PII.
- Around line 82-98: The current code mutates the session object from
authClient.getSession by assigning to data.user (and similar in the
organizationId fast path and fallbacks); instead, stop mutating that external
object — keep apiUser (or a new local variable like userToHydrate) locally and
pass that explicit user object into hydrateUser (and any downstream callers)
rather than setting data.user; remove any assignments to data.user and ensure
branches that currently write into data (organizationId fast path and fallback
paths) use the local user variable when calling hydrateUser or returning values.
- Around line 184-186: The constructed fullName can become an empty string when
apiUser.name, firstName, and lastName are absent; update the logic that computes
fullName (the variable fullName in AuthProvider using apiUser, firstName,
lastName) so that if the composed value.trim() === '' you assign undefined
instead of an empty string, and ensure the object property you pass into
hydrateUser uses that undefined value for name so hydrateUser's typeof check
will not keep a semantic empty name.
In `@packages/identity/drizzle/0001_workable_santa_claus.sql`:
- Around line 12-15: The migration adds ON DELETE CASCADE FKs
(member_organizationId_organization_id_fk,
role_permission_organizationId_organization_id_fk, member_userId_user_id_fk) but
you must prevent accidental hard deletes: before applying, add a migration
pre-check that queries for orphaned member or role_permission rows referencing
non-existent organization or user IDs and aborts if any exist; update deletion
flows to use soft-delete (honor the deletedAt column) and enforce it in the
organization/user delete handlers (create a protected delete endpoint with
confirmation, audit logging, and a system-org check using isSystem), and ensure
existing logic like deleteIfNotLastAdmin still runs to block removing last
admins; only deploy the cascade FKs after these safeguards and the pre-check
pass.
In `@packages/identity/src/adapters/better-auth.adapter.spec.ts`:
- Around line 54-57: Add a test exercising the permission-resolution happy path
by mocking mapUser-related DB calls to return a real role and permissions: in
better-auth.adapter.spec.ts, update the test setup for mapUser to mock
member.findMany (or member.findFirst as used), role.findFirst to return a role
object (with id/name), and rolePermission.findMany to return permission entries
including "dashboard:view" and at least one other permission; then assert
mapUser (or the function under test) resolves with the expected aggregated
permissions and does not fall back to the default-only path. Ensure you
reference the existing mocks role.findFirst, rolePermission.findMany, and
member.findMany in the new test to simulate the full permission-resolution flow.
In `@packages/identity/src/adapters/better-auth.adapter.ts`:
- Around line 513-517: The validation currently only checks role === undefined
while other fields use (value === undefined || value === null || value === ""),
so update the role validation in the createInvitation/validation block to reject
null and empty-string values as well; ensure the same error is thrown (e.g.,
`Invalid response from createInvitation: Missing field "role"`) when role is
null or "" so role is validated as a non-empty string like the other fields.
- Around line 318-325: Post-signup role assignment uses this.db.update on
schema.user with result.user.id and is a separate non-transactional write, so if
it fails the user is created without a role; fix by performing the create + role
assignment in a single transaction (use your DB client's transaction API to
insert the user and set role in one atomic operation) or, if transaction support
is unavailable, catch errors from the this.db.update call and log the failure
(include context: result.user.id and input.role) and/or retry the update to
avoid silent role omission.
- Around line 828-830: The warning in BetterAuthAdapter's mapUser currently logs
dbUser.email, which leaks PII; update the console.warn call in the mapUser
implementation to remove dbUser.email and log only the user identifier
(dbUser.id) and any non-PII context. Locate the console.warn in
better-auth.adapter.ts inside the mapUser function and change the message to
reference only dbUser.id (e.g., "...admin user ${dbUser.id}...") so no email or
other personal data is included in logs.
- Around line 455-475: The catch block in the createInvitation flow logs the
entire error object (console.error("[BetterAuthAdapter] api.createInvitation
failed:", error)) which can leak sensitive payload details; change the logging
to only record non-sensitive information such as the error message and a minimal
context string (e.g., log (error as Error).message or String(error)) instead of
the whole error object, keeping the throw error behavior and leaving
validateInvitationResponse and this.api.createInvitation untouched.
- Around line 824-848: The catch block in BetterAuthAdapter.mapUser currently
swallows the thrown admin permission-resolution error; update the error handling
so admin failures are not masked—either move the admin-empty-permissions guard
outside the try/catch that resolves permissions, or inside the catch detect if
role === "admin" and re-throw the caught error before falling back to
permissions.add("dashboard:view"); reference mapUser, the admin guard that
throws `Permission resolution failed for admin user`, and the outer catch that
currently logs and adds "dashboard:view".
- Around line 789-800: The if block checking "if (roleIds.length > 0 &&
permissions.size === 0)" is dead code containing only comments; remove the
entire conditional (including its comment-only body) or replace it with the
intended supplementary query logic that populates "permissions" when "roleIds"
exist but "permissions" is empty—locate the check around the variables roleIds
and permissions in better-auth.adapter.ts (function handling member/role
permission aggregation) and either delete the empty block to avoid confusion or
implement the fallback permission fetch so permissions are populated when
missing.
In `@packages/identity/src/adapters/drizzle-role.adapter.ts`:
- Around line 50-61: The create method in function create currently inserts into
schema.role and lets DB unique-violation errors surface; wrap the insert in a
try/catch inside create, detect the Postgres/Drizzle unique constraint error for
"role_name_unique" (or check error.code === '23505' and constraint ===
'role_name_unique'), and throw a ConflictException with a clear message (e.g.,
role name already exists) instead of letting the raw error bubble up; rethrow
any other errors unchanged so other failures are preserved.
- Around line 63-77: Build an updates object from the incoming UpdateRoleInput
(in the update method of drizzle-role.adapter.ts) instead of spreading directly
into .set(); if the updates object has no keys (Object.keys(updates).length ===
0) abort before calling this.db.update(schema.role) — e.g., throw a
BadRequestException like "No fields to update" — otherwise pass the updates
object to .set(...) and proceed with the existing where(eq(schema.role.id,
id)).returning() logic, preserving the current NotFoundException handling for a
missing role.
In `@packages/identity/src/adapters/drizzle-tenant.adapter.ts`:
- Line 3: Replace the hardcoded enum Role.Owner used when inserting/updating
member.role with the environment-derived role ID by calling getOwnerRoleId();
locate where Role.Owner is passed to member.role (in this adapter's
create/update logic) and change it to getOwnerRoleId() so the value matches
role.id seeded from env vars and avoids the foreign key constraint violation.
In `@packages/identity/src/adapters/drizzle-user.adapter.spec.ts`:
- Around line 296-313: The test creates a thenable by adding a then property to
a plain object and branches behavior by inspecting
chain.innerJoin.mock.calls.length and chain.limit.mock.calls.length, which lints
as suspicious and makes the test fragile; replace the thenable trick and
call-count branching by providing explicit mockResolvedValueOnce (or separate
mock implementations) on the mocked query chain methods used by the adapter
(e.g., set sequential return values for the function that returns the chain or
use mockResolvedValueOnce on the promise-returning method) or split the spec
into separate test cases so each scenario directly stubs the specific call
sequence instead of reading chain.innerJoin and chain.limit call counts; target
the mocked chain object and the adapter calls (chain.innerJoin, chain.limit, and
the promise resolution path) and sequence their resolved values per-case rather
than using a then property.
In `@packages/identity/src/adapters/drizzle-user.adapter.ts`:
- Around line 94-126: The current catch in DrizzleUserAdapter.findById relies on
a brittle regex to detect "not found" from provider messages; update the handler
to first check well-defined protocol signals (e.g., if error instanceof
UserNotFoundError OR if (error as any).code === 'NOT_FOUND' or check a
documented error.name like 'NotFoundError') and only use the regex as a
documented fallback, keeping the existing logger/rethrow behavior for anything
else; also add a short inline comment in the catch explaining that providers
should throw UserNotFoundError or set error.code = 'NOT_FOUND' (the regex
remains an interim last-resort check).
- Around line 343-358: The cascade hard-delete logic duplicated in
deleteIfNotLastAdmin (the tx.delete sequence deleting invitation, session,
account, user) should be extracted into a single private helper (e.g., private
async cascadeHardDeleteUser(tx, userId) or similar) and then invoked from both
delete() and deleteIfNotLastAdmin; ensure the helper encapsulates the tx.delete
calls for schema.invitation, schema.session, schema.account, and schema.user,
and make it idempotent or skip member deletion since deleteIfNotLastAdmin
already removed the member row—replace the inline deletes in
deleteIfNotLastAdmin with a call to this helper so both paths stay in sync.
In `@packages/identity/src/background/pii-cleanup.spec.ts`:
- Around line 22-29: The test setup's mock logger used in beforeEach is missing
the warn method which runPIICleanup invokes; update the mock in the beforeEach
block to include warn: vi.fn() so calls to logger.warn(...) in runPIICleanup
succeed, and then add a unit test exercising the MAX_ITERATIONS branch of
runPIICleanup to assert logger.warn was called and behavior matches
expectations.
In `@packages/identity/src/background/pii-cleanup.ts`:
- Around line 31-43: Add a DB migration to create a partial index to speed up
the sessionsToUpdate query: create an index (e.g., idx_session_pii_cleanup) on
the session.createdAt column with a WHERE clause matching the query predicate
(ipAddress IS NOT NULL OR userAgent IS NOT NULL) so the SELECT in pii-cleanup.ts
(the sessionsToUpdate query using schema.session.createdAt,
schema.session.ipAddress, schema.session.userAgent and BATCH_SIZE) can use the
index; include the index name in the migration, add the corresponding
down/rollback to drop the index, and run the migration before deploying the
cleanup job.
In `@packages/identity/src/interfaces/auth-provider.interface.ts`:
- Line 46: Change the inconsistent nullability for findById to match
getInvitation by making findById return Promise<User | null> instead of
throwing; update the interface signature for findById(userId: string) to return
Promise<User | null>, and then update all implementations of the interface and
call sites that assumed an exception (e.g., remove try/catch expecting
UserNotFoundError and add null checks) to handle a null return; keep
getInvitation as-is so both methods follow the same “nullable if not found”
convention.
In `@packages/identity/src/interfaces/role-provider.interface.ts`:
- Around line 37-47: The JSDoc on the RoleProvider interface is
framework-coupled: update(id: string, input: UpdateRoleInput):
Promise<RoleEntity> and delete(id: string): Promise<void> currently mention
"Throws NotFoundException"; change those comments to a framework-agnostic
statement such as "Throws if the role does not exist" (or "Throws an error if
the role does not exist") so the interface (role-provider.interface.ts)
references only the condition, not NestJS's NotFoundException, leaving concrete
implementations to map to framework-specific exceptions.
- Around line 13-17: The CreateRoleInput interface currently exposes isSystem
allowing any caller with roles:create to mark roles as system; remove isSystem
from CreateRoleInput (or if you prefer to keep the shape for internal use, make
it optional-only in internal DTOs) and enforce in the controller (where
CreateRoleInput is consumed) that isSystem is never accepted from API requests
by stripping/ignoring that field and defaulting to false before creating roles;
update any usage sites that construct CreateRoleInput (and relevant tests) to
stop passing isSystem, and ensure seeding code that must create system roles
uses an internal API or a separate seed-only creator that can set isSystem.
In `@packages/identity/src/services/permission-seeder.spec.ts`:
- Around line 19-29: The mockChainedQuery helper creates a plain object with a
then property which makes it a thenable and triggers Biome's noThenProperty
rule; update mockChainedQuery to avoid adding a plain then property—instead
either create an actual Promise (e.g., const promise = Promise.resolve(result))
and attach the chain methods to that Promise, or make the chain object expose an
explicit async method like execute() that returns Promise.resolve(result);
modify the mockChainedQuery function (and the methods array assignment) to use
one of these approaches so tests remain awaitable without producing a thenable
plain object.
In `@packages/identity/src/services/permission-seeder.ts`:
- Around line 41-66: The outer seed() function creates a transaction and passes
tx into seedSystemRbac, but seedSystemRbac wraps that tx in its own
db.transaction, creating an unnecessary nested transaction; fix by removing the
inner db.transaction in seedSystemRbac (rbac-seeding.ts) and use the passed tx
directly for all queries/operations (preserve seedSystemRbac signature that
accepts tx or add an overload to accept tx), ensuring all calls (from seed()
where tx is passed) operate on the same tx so the System Tenant upsert and RBAC
seeding run in one atomic transaction.
In `@packages/identity/src/utils/rbac-seeding.ts`:
- Around line 36-188: The code currently always calls db.transaction(...) inside
seedSystemRbac causing nested transactions/savepoints when the caller already
passes a transaction; change seedSystemRbac to accept an optional transaction
parameter (e.g., tx?: DBTransaction) or an options flag (e.g.,
useSavepoint=false) and then: if a tx is provided, run the body directly using
that tx, otherwise call db.transaction(...) as before; update all internal
references to use the provided tx and ensure callers (like permission-seeder.ts)
pass their transaction handle into seedSystemRbac to avoid unintended nesting.
- Around line 163-178: The dedupe key currently uses the string sentinel
"__NULL__" for null organizationId which can collide; update the key
construction in rbac-seeding.ts (where existingSet is built and inside the
rolePermissionsToInsert.filter callback) to use a structured serialization
instead—e.g. JSON.stringify([e.roleId, e.permissionId, e.organizationId]) or
another unambiguous tuple format—so null, undefined and string values cannot
collide; ensure both the existing.map and the key variable in the filter use the
exact same serialization and keep the internal existingSet.add(key) logic
unchanged to preserve batch deduping.
Comment on lines
79
to
+81
| "ts-node": "^10.9.2", | ||
| "tsconfig-paths": "^4.2.0", | ||
| "tsx": "^4.21.0", |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if ts-node is still referenced anywhere in the api app
rg -n 'ts-node' apps/api/ --type-not jsonRepository: pramodnarayana/nexiom
Length of output: 154
🏁 Script executed:
#!/bin/bash
# Check the scripts section of package.json
cd apps/api && cat package.json | jq '.scripts' || grep -A 50 '"scripts"' package.json | head -60Repository: pramodnarayana/nexiom
Length of output: 1032
🏁 Script executed:
#!/bin/bash
# Look at context around line 462 in manage.ts
sed -n '455,470p' apps/api/src/scripts/manage.tsRepository: pramodnarayana/nexiom
Length of output: 508
🏁 Script executed:
#!/bin/bash
# Check for ts-node references in configuration files
rg -l 'ts-node' apps/api/ -t ts -t js -t json --ignore-case | grep -E '\.(ts|js|json|config)$'Repository: pramodnarayana/nexiom
Length of output: 117
ts-node is unused and should be removed from devDependencies.
All scripts have been migrated to tsx (e.g., db:seed, db:reset, db:admin:reset), and no configuration files (drizzle-kit, vitest) reference ts-node. The only remaining reference is an outdated usage message in manage.ts (line 462) that incorrectly suggests ts-node manage.ts—this should also be updated to tsx manage.ts for consistency.
🤖 Prompt for AI Agents
In `@apps/api/package.json` around lines 79 - 81, Remove the unused "ts-node"
devDependency from package.json and update the outdated usage message in
manage.ts to reference "tsx manage.ts" instead of "ts-node manage.ts";
specifically edit the devDependencies entry for "ts-node" in package.json to
delete it and update the string in the usage output inside the manage.ts file
(the message around the existing line that currently suggests "ts-node
manage.ts") so all scripts and messages consistently reference "tsx".
Comment on lines
+130
to
+133
| this.logger.log(`[CompleteInvite] Received request for ${body.email}`); | ||
| this.logger.debug( | ||
| `[CompleteInvite] Payload: invitationId=${body.invitationId}, firstName=${body.firstName}, lastName=${body.lastName}`, | ||
| ); |
Contributor
There was a problem hiding this comment.
PII (email) logged at INFO level — compliance risk.
Line 130 logs the user's email via logger.log (INFO level). INFO logs are typically retained long-term and shipped to centralized logging systems, creating GDPR/CCPA exposure. Line 132 also logs firstName and lastName at DEBUG level.
Consider removing or masking PII from log messages, or at minimum downgrading the email log to DEBUG so it can be suppressed in production.
Proposed fix
- this.logger.log(`[CompleteInvite] Received request for ${body.email}`);
- this.logger.debug(
- `[CompleteInvite] Payload: invitationId=${body.invitationId}, firstName=${body.firstName}, lastName=${body.lastName}`,
- );
+ this.logger.log(`[CompleteInvite] Received request for invitationId=${body.invitationId}`);
+ this.logger.debug(
+ `[CompleteInvite] Payload: invitationId=${body.invitationId}`,
+ );📝 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
| this.logger.log(`[CompleteInvite] Received request for ${body.email}`); | |
| this.logger.debug( | |
| `[CompleteInvite] Payload: invitationId=${body.invitationId}, firstName=${body.firstName}, lastName=${body.lastName}`, | |
| ); | |
| this.logger.log(`[CompleteInvite] Received request for invitationId=${body.invitationId}`); | |
| this.logger.debug( | |
| `[CompleteInvite] Payload: invitationId=${body.invitationId}`, | |
| ); |
🤖 Prompt for AI Agents
In `@apps/api/src/modules/identity/auth/auth.controller.ts` around lines 130 -
133, The INFO-level log in CompleteInvite that logs body.email is logging PII;
change it so the email is not logged at INFO—either remove it entirely or
downgrade it to DEBUG and/or mask the value before logging; update the calls in
auth.controller.ts (the this.logger.log(...) and the this.logger.debug(...)
lines within the CompleteInvite handler) so that no raw email is emitted at INFO
and only non-identifying or masked fields are logged at DEBUG.
Comment on lines
+25
to
+32
| // If specific organization context exists (Tenant Admin), enforce it. | ||
| if (req.user.organizationId) { | ||
| createInvitation.organizationId = req.user.organizationId; | ||
| } | ||
| // If specific organization context exists (Tenant Admin), enforce it. | ||
| if (req.user.organizationId) { | ||
| createInvitation.organizationId = req.user.organizationId; | ||
| } |
Contributor
There was a problem hiding this comment.
Duplicate code block: organizationId enforcement is repeated.
Lines 26–28 and 30–32 are identical. This is a copy-paste error. Remove one of the two blocks.
Proposed fix
// If specific organization context exists (Tenant Admin), enforce it.
if (req.user.organizationId) {
createInvitation.organizationId = req.user.organizationId;
}
- // If specific organization context exists (Tenant Admin), enforce it.
- if (req.user.organizationId) {
- createInvitation.organizationId = req.user.organizationId;
- }📝 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
| // If specific organization context exists (Tenant Admin), enforce it. | |
| if (req.user.organizationId) { | |
| createInvitation.organizationId = req.user.organizationId; | |
| } | |
| // If specific organization context exists (Tenant Admin), enforce it. | |
| if (req.user.organizationId) { | |
| createInvitation.organizationId = req.user.organizationId; | |
| } | |
| // If specific organization context exists (Tenant Admin), enforce it. | |
| if (req.user.organizationId) { | |
| createInvitation.organizationId = req.user.organizationId; | |
| } |
🤖 Prompt for AI Agents
In `@apps/api/src/modules/identity/invitations/invitations.controller.ts` around
lines 25 - 32, Remove the duplicated organization enforcement block in the
invitations controller: there are two identical if (req.user.organizationId) {
createInvitation.organizationId = req.user.organizationId; } blocks; keep a
single instance (inside the controller method handling invitation creation) and
delete the redundant copy so createInvitation.organizationId is set only once.
| controller.createUser({ | ||
| name: 'Test', | ||
| email: 'taken@example.com', | ||
| role: 'user', |
Contributor
There was a problem hiding this comment.
Test uses role: 'user' which is not a valid enum value per CreateUserSchema.
buildCreateUserSchema() defines role as z.enum([getRequiredOwnerRoleId(), getRequiredAdminRoleId()]), which in tests resolves to ['owner-role-id', 'admin-role-id']. The value 'user' would fail Zod validation at runtime. While unit tests may bypass the validation pipe, this makes the tests unrepresentative of real request payloads.
Use a valid role ID consistent with the schema and the other tests in this file (e.g., createSystemInvitation uses getRequiredOwnerRoleId()).
Proposed fix
await expect(
controller.createUser({
name: 'Test',
email: 'taken@example.com',
- role: 'user',
+ role: getRequiredAdminRoleId(),
}),
).rejects.toThrow(BadRequestException); const result = await controller.createUser({
name: 'Test',
email: 'new@example.com',
- role: 'user',
+ role: getRequiredAdminRoleId(),
});
expect(result).toEqual(mockUser);
expect(mockUserProvider.create).toHaveBeenCalledWith({
name: 'Test',
email: 'new@example.com',
- role: 'user',
+ role: getRequiredAdminRoleId(),
});Also applies to: 204-204, 211-211
🤖 Prompt for AI Agents
In `@apps/api/src/modules/identity/system-admin/system-admin.controller.spec.ts`
at line 188, The test in system-admin.controller.spec.ts uses an invalid role
string ('user') that doesn't match CreateUserSchema's enum; replace occurrences
of role: 'user' (around the test blocks including the ones noted at lines ~188,
204, 211) with a valid role id such as getRequiredOwnerRoleId() (or
getRequiredAdminRoleId()) so the test payloads match buildCreateUserSchema()'s
z.enum and align with other tests like createSystemInvitation; update all
instances to reference the helper function instead of the literal 'user'.
Comment on lines
141
to
+154
| async createUser(@Body() input: CreateUserValidation) { | ||
| // Type the validated input properly | ||
| const data: CreateUserDto = input as CreateUserDto; | ||
|
|
||
| // Check if email already exists | ||
| const existing = await this.userProvider.findByEmail(input.email); | ||
| const existing = await this.userProvider.findByEmail(data.email); | ||
|
|
||
| if (existing) { | ||
| throw new BadRequestException('User with this email already exists'); | ||
| } | ||
|
|
||
| // Now uses single-step creation via Adapter logic | ||
| const user = await this.userProvider.create({ | ||
| ...input, | ||
| ...data, |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Unsafe type assertion — prefer proper typing or a validation pipe instead.
The as CreateUserDto cast on line 143 bypasses type safety. If CreateUserValidation and CreateUserDto diverge (e.g., schema changes), the compiler won't catch it.
Consider either:
- Using
@UsePipes(new LazyZodValidationPipe(buildCreateUserSchema))(consistent withcreateSystemInvitationon line 86) and typing the body asCreateUserDtodirectly. - Or, if the DTO class must stay, rely on its inferred type without the manual cast.
Option 1: Use LazyZodValidationPipe consistently
`@Post`('users')
`@RequirePermission`('system_users', 'manage')
- async createUser(`@Body`() input: CreateUserValidation) {
- // Type the validated input properly
- const data: CreateUserDto = input as CreateUserDto;
-
+ `@UsePipes`(new LazyZodValidationPipe(buildCreateUserSchema))
+ async createUser(`@Body`() data: CreateUserDto) {
// Check if email already exists
const existing = await this.userProvider.findByEmail(data.email);🤖 Prompt for AI Agents
In `@apps/api/src/modules/identity/system-admin/system-admin.controller.ts` around
lines 141 - 154, The createUser method currently unsafely asserts
CreateUserValidation to CreateUserDto; remove the manual cast and enforce
validated typing by applying the same validation pipe used for
createSystemInvitation: add `@UsePipes`(new
LazyZodValidationPipe(buildCreateUserSchema)) to the createUser handler and
change the parameter type from CreateUserValidation to CreateUserDto (or accept
CreateUserDto directly), so the body is validated/typed correctly before you
call this.userProvider.create; alternatively, if you must keep the DTO class,
remove the "as CreateUserDto" cast and rely on an appropriate validation pipe to
produce a safe CreateUserDto instance.
| setPassword?(userId: string, password: string): Promise<void>; | ||
|
|
||
| resendVerificationEmail?(email: string): Promise<void>; | ||
| findById?(userId: string): Promise<User>; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Inconsistent nullability convention: findById returns Promise<User> while getInvitation returns Promise<Invitation | null>.
getInvitation(id) on line 37 returns | null for not-found, but findById throws instead. This inconsistency forces callers to handle not-found via two different patterns (null-check vs try/catch) within the same interface. Consider aligning on one convention — either return User | null here, or document that this method throws UserNotFoundError.
🤖 Prompt for AI Agents
In `@packages/identity/src/interfaces/auth-provider.interface.ts` at line 46,
Change the inconsistent nullability for findById to match getInvitation by
making findById return Promise<User | null> instead of throwing; update the
interface signature for findById(userId: string) to return Promise<User | null>,
and then update all implementations of the interface and call sites that assumed
an exception (e.g., remove try/catch expecting UserNotFoundError and add null
checks) to handle a null return; keep getInvitation as-is so both methods follow
the same “nullable if not found” convention.
Comment on lines
+19
to
29
| const mockChainedQuery = (result: unknown) => { | ||
| const chain: Record<string, any> = { | ||
| then: (onfulfilled: (value: unknown) => unknown) => | ||
| Promise.resolve(result).then(onfulfilled), | ||
| }; | ||
| const methods = ["from", "where", "innerJoin", "select", "limit", "offset"]; | ||
| methods.forEach((m) => { | ||
| chain[m] = vi.fn().mockReturnValue(chain); | ||
| }); | ||
| return chain; | ||
| }; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Biome: then property on plain object makes it a thenable.
Adding then to the returned object turns it into a thenable, which can cause subtle issues if the object is passed to Promise.resolve() or awaited in an unexpected context. This is flagged by Biome (noThenProperty). Since the intent is to simulate an awaitable query chain, consider using a proper mock Promise or wrapping the result differently:
♻️ Suggested alternative
const mockChainedQuery = (result: unknown) => {
- const chain: Record<string, any> = {
- then: (onfulfilled: (value: unknown) => unknown) =>
- Promise.resolve(result).then(onfulfilled),
- };
- const methods = ["from", "where", "innerJoin", "select", "limit", "offset"];
- methods.forEach((m) => {
- chain[m] = vi.fn().mockReturnValue(chain);
- });
- return chain;
+ const promise = Promise.resolve(result);
+ const chain: Record<string, any> = {};
+ const methods = ["from", "where", "innerJoin", "select", "limit", "offset"];
+ methods.forEach((m) => {
+ chain[m] = vi.fn().mockReturnValue(chain);
+ });
+ // Make the chain awaitable without adding a `then` own-property
+ Object.setPrototypeOf(chain, { then: promise.then.bind(promise) });
+ return chain;
};🧰 Tools
🪛 Biome (2.3.13)
[error] 21-21: Do not add then to an object.
(lint/suspicious/noThenProperty)
🤖 Prompt for AI Agents
In `@packages/identity/src/services/permission-seeder.spec.ts` around lines 19 -
29, The mockChainedQuery helper creates a plain object with a then property
which makes it a thenable and triggers Biome's noThenProperty rule; update
mockChainedQuery to avoid adding a plain then property—instead either create an
actual Promise (e.g., const promise = Promise.resolve(result)) and attach the
chain methods to that Promise, or make the chain object expose an explicit async
method like execute() that returns Promise.resolve(result); modify the
mockChainedQuery function (and the methods array assignment) to use one of these
approaches so tests remain awaitable without producing a thenable plain object.
Comment on lines
+41
to
+66
| await this.db.transaction(async (tx) => { | ||
| // Ensure System Tenant exists to avoid FK violations | ||
| const systemTenant = await tx.query.organization.findFirst({ | ||
| where: (org, { eq }) => eq(org.id, systemTenantId), | ||
| }); | ||
|
|
||
| if (!systemTenant) { | ||
| this.logger.log(`Creating System Tenant (${systemTenantId})...`); | ||
| await tx | ||
| .insert(schema.organization) | ||
| .values({ | ||
| id: systemTenantId, | ||
| name: "Nexiom Platform", | ||
| slug: "system", | ||
| isSystem: true, | ||
| status: "active", | ||
| }) | ||
| .onConflictDoNothing(); | ||
| } | ||
|
|
||
| await seedSystemRbac( | ||
| tx, | ||
| { ownerRoleId, adminRoleId, memberRoleId, systemTenantId }, | ||
| this.logger, | ||
| ); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/identity/src/utils/rbac-seeding.ts | head -100Repository: pramodnarayana/nexiom
Length of output: 3447
🏁 Script executed:
# Check the permission-seeder.ts to see exactly how tx is passed
cat -n packages/identity/src/services/permission-seeder.ts | sed -n '35,70p'Repository: pramodnarayana/nexiom
Length of output: 1251
🏁 Script executed:
# Check package.json for drizzle-orm version
grep -A 2 -B 2 'drizzle-orm' package.json packages/*/package.jsonRepository: pramodnarayana/nexiom
Length of output: 367
🏁 Script executed:
cat -n packages/identity/src/utils/rbac-seeding.ts | sed -n '35,180p'Repository: pramodnarayana/nexiom
Length of output: 5398
🏁 Script executed:
cat -n packages/identity/src/utils/rbac-seeding.ts | sed -n '180,220p'Repository: pramodnarayana/nexiom
Length of output: 645
Remove or flatten nested transaction wrapping in seedSystemRbac.
seed() starts a transaction at line 41 and passes tx to seedSystemRbac (line 62). However, seedSystemRbac immediately wraps that transaction handle in another db.transaction() call (rbac-seeding.ts line 37), creating a nested transaction that degrades to a savepoint with drizzle-orm + node-postgres. This means the System Tenant upsert (lines 49–58) and RBAC operations run in separate transaction scopes, even though they're logically a single atomic unit.
While error propagation (rbac-seeding.ts lines 191–194) ensures the outer transaction will roll back on RBAC failure, the pattern is confusing and adds unnecessary nesting. Either:
- Remove the outer transaction here and let
seedSystemRbacown transaction atomicity (moving System Tenant upsert intoseedSystemRbac), or - Remove the inner
db.transaction()inseedSystemRbacand use the passedtxdirectly without wrapping.
🤖 Prompt for AI Agents
In `@packages/identity/src/services/permission-seeder.ts` around lines 41 - 66,
The outer seed() function creates a transaction and passes tx into
seedSystemRbac, but seedSystemRbac wraps that tx in its own db.transaction,
creating an unnecessary nested transaction; fix by removing the inner
db.transaction in seedSystemRbac (rbac-seeding.ts) and use the passed tx
directly for all queries/operations (preserve seedSystemRbac signature that
accepts tx or add an overload to accept tx), ensuring all calls (from seed()
where tx is passed) operate on the same tx so the System Tenant upsert and RBAC
seeding run in one atomic transaction.
Comment on lines
+36
to
+188
| // Wrap entire operation in a transaction for atomicity | ||
| await db.transaction(async (tx) => { | ||
| // 1. Ensure Roles | ||
| const roles = [ | ||
| { | ||
| id: ownerRoleId, | ||
| name: "Owner", | ||
| isSystem: true, | ||
| description: "Full access", | ||
| }, | ||
| { | ||
| id: adminRoleId, | ||
| name: "Admin", | ||
| isSystem: true, | ||
| description: "Manage users and settings", | ||
| }, | ||
| { | ||
| id: memberRoleId, | ||
| name: "Member", | ||
| isSystem: true, | ||
| description: "Read only access", | ||
| }, | ||
| ]; | ||
|
|
||
| await tx | ||
| .insert(schema.role) | ||
| .values( | ||
| roles.map((r) => ({ | ||
| ...r, | ||
| createdAt: now, | ||
| })), | ||
| ) | ||
| .onConflictDoNothing(); | ||
|
|
||
| // 2. Ensure Permissions | ||
| const perms = ALL_PERMISSIONS; | ||
| const permissionsToInsert = perms.map((p) => { | ||
| const separatorIndex = p.indexOf(":"); | ||
| if (separatorIndex === -1) { | ||
| throw new Error(`Invalid permission format: ${p}`); | ||
| } | ||
| return { | ||
| id: p, | ||
| resource: p.substring(0, separatorIndex), | ||
| action: p.substring(separatorIndex + 1), | ||
| createdAt: now, | ||
| })), | ||
| ) | ||
| .onConflictDoNothing(); | ||
|
|
||
| // 2. Ensure Permissions | ||
| const perms = ALL_PERMISSIONS; | ||
| const permissionsToInsert = perms.map((p) => { | ||
| const separatorIndex = p.indexOf(":"); | ||
| if (separatorIndex === -1) { | ||
| throw new Error(`Invalid permission format: ${p}`); | ||
| } | ||
| return { | ||
| id: p, | ||
| resource: p.substring(0, separatorIndex), | ||
| action: p.substring(separatorIndex + 1), | ||
| createdAt: now, | ||
| }; | ||
| }); | ||
|
|
||
| await tx | ||
| .insert(schema.permission) | ||
| .values(permissionsToInsert) | ||
| .onConflictDoNothing(); | ||
|
|
||
| // 3. Build Role-Permission Mappings | ||
| const rolePermissionsToInsert: { | ||
| id: string; | ||
| roleId: string; | ||
| permissionId: string; | ||
| organizationId?: string | null; | ||
| }[] = []; | ||
|
|
||
| const addPermissionsForRole = ( | ||
| roleId: string, | ||
| permissions: readonly string[], | ||
| scopedOrganizationId: string, | ||
| ) => { | ||
| for (const p of permissions) { | ||
| // Deterministically decide scope based on permission type | ||
| const isSystem = isSystemPermission(p); | ||
| const orgId = isSystem ? scopedOrganizationId : null; | ||
|
|
||
| // Push to list | ||
| rolePermissionsToInsert.push({ | ||
| id: uuidv4(), | ||
| roleId, | ||
| permissionId: p, | ||
| organizationId: orgId, | ||
| }); | ||
| } | ||
| }; | ||
| }); | ||
|
|
||
| await db | ||
| .insert(schema.permission) | ||
| .values(permissionsToInsert) | ||
| .onConflictDoNothing(); | ||
|
|
||
| // 3. Assign Permissions | ||
| const rolePermissionsToInsert: { | ||
| id: string; | ||
| roleId: string; | ||
| permissionId: string; | ||
| organizationId?: string | null; | ||
| }[] = []; | ||
|
|
||
| const addPermissionsForRole = ( | ||
| roleId: string, | ||
| permissions: readonly string[], | ||
| scopedOrganizationId: string, | ||
| ) => { | ||
| for (const p of permissions) { | ||
| // Deterministically decide scope based on permission type | ||
| const isSystem = isSystemPermission(p); | ||
| const orgId = isSystem ? scopedOrganizationId : null; | ||
|
|
||
| // Push to list | ||
| rolePermissionsToInsert.push({ | ||
| id: uuidv4(), | ||
| roleId, | ||
| permissionId: p, | ||
| organizationId: orgId, | ||
| }); | ||
| } | ||
| }; | ||
| // Admin | ||
| addPermissionsForRole(adminRoleId, perms, systemTenantId); | ||
|
|
||
| // Member: curated safe subset | ||
| const memberPerms: PermissionType[] = ["users:read", "tenants:read"]; | ||
|
|
||
| // Admin | ||
| addPermissionsForRole(adminRoleId, perms, systemTenantId); | ||
| // Validate configuration fail-fast | ||
| const invalidPerms = memberPerms.filter((p) => !perms.includes(p)); | ||
| if (invalidPerms.length > 0) { | ||
| throw new Error( | ||
| `Invalid member permissions configured: ${invalidPerms.join(", ")}. Must be in ALL_PERMISSIONS.`, | ||
| ); | ||
| } | ||
|
|
||
| // Member: curated safe subset | ||
| const memberPerms: PermissionType[] = ["users:read", "tenants:read"]; | ||
| // Member role always receives organizationId: null per RBAC design (see ADR) | ||
| for (const p of memberPerms) { | ||
| if (perms.includes(p)) { | ||
| // Member role always receives organizationId: null per RBAC design (see ADR) | ||
| for (const p of memberPerms) { | ||
| rolePermissionsToInsert.push({ | ||
| id: uuidv4(), | ||
| roleId: memberRoleId, | ||
| permissionId: p, | ||
| organizationId: null, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| // Owner (Same as Admin) | ||
| addPermissionsForRole(ownerRoleId, perms, systemTenantId); | ||
|
|
||
| await db | ||
| .insert(schema.rolePermission) | ||
| .values(rolePermissionsToInsert) | ||
| .onConflictDoNothing({ | ||
| target: [ | ||
| schema.rolePermission.roleId, | ||
| schema.rolePermission.permissionId, | ||
| schema.rolePermission.organizationId, | ||
| ], | ||
| }); | ||
|
|
||
| // Owner (Same as Admin) | ||
| addPermissionsForRole(ownerRoleId, perms, systemTenantId); | ||
|
|
||
| // 4. Assign Permissions (Read-Filter-Insert to avoid ON CONFLICT issues) | ||
| if (rolePermissionsToInsert.length > 0) { | ||
| // Fetch existing logic: restricted to the relevant system roles | ||
| const existing = await tx | ||
| .select({ | ||
| roleId: schema.rolePermission.roleId, | ||
| permissionId: schema.rolePermission.permissionId, | ||
| organizationId: schema.rolePermission.organizationId, | ||
| }) | ||
| .from(schema.rolePermission) | ||
| .where( | ||
| inArray(schema.rolePermission.roleId, [ | ||
| ownerRoleId, | ||
| adminRoleId, | ||
| memberRoleId, | ||
| ]), | ||
| ); | ||
|
|
||
| const existingSet = new Set( | ||
| existing.map( | ||
| (e) => | ||
| `${e.roleId}|${e.permissionId}|${e.organizationId ?? "__NULL__"}`, | ||
| ), | ||
| ); | ||
|
|
||
| const toInsert = rolePermissionsToInsert.filter((rp) => { | ||
| const key = `${rp.roleId}|${rp.permissionId}|${rp.organizationId ?? "__NULL__"}`; | ||
| if (existingSet.has(key)) { | ||
| return false; | ||
| } | ||
| // Also dedupe internally within the batch | ||
| existingSet.add(key); | ||
| return true; | ||
| }); | ||
|
|
||
| if (toInsert.length > 0) { | ||
| // Batch insert using transaction | ||
| await tx.insert(schema.rolePermission).values(toInsert); | ||
| logger.log(`Inserted ${toInsert.length} new role permissions.`); | ||
| } else { | ||
| logger.log("No new role permissions to insert."); | ||
| } | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
Transaction-within-transaction: this function is also called with an existing tx handle.
As noted in permission-seeder.ts, seedSystemRbac is invoked with a transaction handle (tx) from the caller's this.db.transaction(). Calling db.transaction() here (line 37) on that handle creates a savepoint. Consider accepting an optional flag or checking whether db is already a transaction to avoid unnecessary nesting, or document that savepoint semantics are intentional.
🤖 Prompt for AI Agents
In `@packages/identity/src/utils/rbac-seeding.ts` around lines 36 - 188, The code
currently always calls db.transaction(...) inside seedSystemRbac causing nested
transactions/savepoints when the caller already passes a transaction; change
seedSystemRbac to accept an optional transaction parameter (e.g., tx?:
DBTransaction) or an options flag (e.g., useSavepoint=false) and then: if a tx
is provided, run the body directly using that tx, otherwise call
db.transaction(...) as before; update all internal references to use the
provided tx and ensure callers (like permission-seeder.ts) pass their
transaction handle into seedSystemRbac to avoid unintended nesting.
Comment on lines
+163
to
+178
| const existingSet = new Set( | ||
| existing.map( | ||
| (e) => | ||
| `${e.roleId}|${e.permissionId}|${e.organizationId ?? "__NULL__"}`, | ||
| ), | ||
| ); | ||
|
|
||
| const toInsert = rolePermissionsToInsert.filter((rp) => { | ||
| const key = `${rp.roleId}|${rp.permissionId}|${rp.organizationId ?? "__NULL__"}`; | ||
| if (existingSet.has(key)) { | ||
| return false; | ||
| } | ||
| // Also dedupe internally within the batch | ||
| existingSet.add(key); | ||
| return true; | ||
| }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Sentinel "__NULL__" for null organizationId in dedup set could theoretically collide.
The composite key uses "__NULL__" as a stand-in for null. While an organizationId of "__NULL__" is astronomically unlikely, a structured key avoids the ambiguity entirely:
♻️ Minimal fix
- `${e.roleId}|${e.permissionId}|${e.organizationId ?? "__NULL__"}`,
+ JSON.stringify([e.roleId, e.permissionId, e.organizationId]),- const key = `${rp.roleId}|${rp.permissionId}|${rp.organizationId ?? "__NULL__"}`;
+ const key = JSON.stringify([rp.roleId, rp.permissionId, rp.organizationId]);📝 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
| const existingSet = new Set( | |
| existing.map( | |
| (e) => | |
| `${e.roleId}|${e.permissionId}|${e.organizationId ?? "__NULL__"}`, | |
| ), | |
| ); | |
| const toInsert = rolePermissionsToInsert.filter((rp) => { | |
| const key = `${rp.roleId}|${rp.permissionId}|${rp.organizationId ?? "__NULL__"}`; | |
| if (existingSet.has(key)) { | |
| return false; | |
| } | |
| // Also dedupe internally within the batch | |
| existingSet.add(key); | |
| return true; | |
| }); | |
| const existingSet = new Set( | |
| existing.map( | |
| (e) => | |
| JSON.stringify([e.roleId, e.permissionId, e.organizationId]), | |
| ), | |
| ); | |
| const toInsert = rolePermissionsToInsert.filter((rp) => { | |
| const key = JSON.stringify([rp.roleId, rp.permissionId, rp.organizationId]); | |
| if (existingSet.has(key)) { | |
| return false; | |
| } | |
| // Also dedupe internally within the batch | |
| existingSet.add(key); | |
| return true; | |
| }); |
🤖 Prompt for AI Agents
In `@packages/identity/src/utils/rbac-seeding.ts` around lines 163 - 178, The
dedupe key currently uses the string sentinel "__NULL__" for null organizationId
which can collide; update the key construction in rbac-seeding.ts (where
existingSet is built and inside the rolePermissionsToInsert.filter callback) to
use a structured serialization instead—e.g. JSON.stringify([e.roleId,
e.permissionId, e.organizationId]) or another unambiguous tuple format—so null,
undefined and string values cannot collide; ensure both the existing.map and the
key variable in the filter use the exact same serialization and keep the
internal existingSet.add(key) logic unchanged to preserve batch deduping.
Comment on lines
+42
to
+47
| @Post() | ||
| @RequirePermission('roles', 'create') | ||
| async create(@Body() body: CreateRoleInput) { | ||
| const role = await this.roleProvider.create(body); | ||
| return { data: role }; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for CreateRoleInput and UpdateRoleInput definitions and usage
rg -n "CreateRoleInput|UpdateRoleInput" --type=ts -g '!node_modules' -g '!dist' -A 3Repository: pramodnarayana/nexiom
Length of output: 3777
🏁 Script executed:
#!/bin/bash
# Check for validation decorators or schemas associated with these types
rg -n "class CreateRoleInput|interface CreateRoleInput|type CreateRoleInput" --type=ts -g '!node_modules' -g '!dist' -B 2 -A 5Repository: pramodnarayana/nexiom
Length of output: 671
🏁 Script executed:
#!/bin/bash
# Look for ZodValidationPipe or other validation pipes in the codebase
rg -n "ZodValidationPipe|ValidationPipe|class.*Pipe" --type=ts -g '!node_modules' -g '!dist' | head -30Repository: pramodnarayana/nexiom
Length of output: 856
🏁 Script executed:
#!/bin/bash
# Search for Zod schema definitions for roles
rg -n "CreateRole.*[Ss]chema|UpdateRole.*[Ss]chema" --type=ts -g '!node_modules' -g '!dist' -B 2 -A 5Repository: pramodnarayana/nexiom
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Check the roles module structure for any schema files
find apps/api/src/modules/identity/roles -type f -name "*.ts" | head -20Repository: pramodnarayana/nexiom
Length of output: 233
🏁 Script executed:
#!/bin/bash
# Look at the full roles.controller.ts to see if there are any validation decorators
cat -n apps/api/src/modules/identity/roles/roles.controller.ts | head -80Repository: pramodnarayana/nexiom
Length of output: 2254
Add request body validation for CreateRoleInput and UpdateRoleInput using Zod schemas.
These are plain TypeScript interfaces without validation decorators or Zod schemas. Although ZodValidationPipe is registered globally, it requires Zod schema definitions to validate input. Without schema definitions, these request bodies pass through unvalidated.
Create Zod schemas for both types and decorate the DTOs, or add explicit @UsePipes(new ZodValidationPipe(schema)) decorators to the create and update endpoints.
🤖 Prompt for AI Agents
In `@apps/api/src/modules/identity/roles/roles.controller.ts` around lines 42 -
47, Create and attach Zod validation for the create/update endpoints: define Zod
schemas for CreateRoleInput and UpdateRoleInput (e.g., createRoleSchema,
updateRoleSchema) and apply them to the controller actions—either by decorating
the DTOs or by adding `@UsePipes`(new ZodValidationPipe(createRoleSchema)) on the
create method and `@UsePipes`(new ZodValidationPipe(updateRoleSchema)) on the
update method in roles.controller.ts; ensure you reference the existing
CreateRoleInput and UpdateRoleInput types and keep the roleProvider.create and
roleProvider.update calls intact so validated data is passed through.
Comment on lines
+49
to
+54
| @Get(':id') | ||
| @RequirePermission('roles', 'read') | ||
| async findById(@Param('id') id: string) { | ||
| const role = await this.roleProvider.findById(id); | ||
| return { data: role }; | ||
| } |
Contributor
There was a problem hiding this comment.
Missing NotFoundException when role is not found.
roleProvider.findById returns Promise<RoleEntity | null>. If null, the endpoint will return { data: null } with a 200 status, which is incorrect REST semantics.
Proposed fix
+ import { NotFoundException } from '@nestjs/common';
...
`@Get`(':id')
`@RequirePermission`('roles', 'read')
async findById(`@Param`('id') id: string) {
const role = await this.roleProvider.findById(id);
+ if (!role) {
+ throw new NotFoundException(`Role ${id} not found`);
+ }
return { data: role };
}📝 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
| @Get(':id') | |
| @RequirePermission('roles', 'read') | |
| async findById(@Param('id') id: string) { | |
| const role = await this.roleProvider.findById(id); | |
| return { data: role }; | |
| } | |
| `@Get`(':id') | |
| `@RequirePermission`('roles', 'read') | |
| async findById(`@Param`('id') id: string) { | |
| const role = await this.roleProvider.findById(id); | |
| if (!role) { | |
| throw new NotFoundException(`Role ${id} not found`); | |
| } | |
| return { data: role }; | |
| } |
🤖 Prompt for AI Agents
In `@apps/api/src/modules/identity/roles/roles.controller.ts` around lines 49 -
54, The findById endpoint currently returns { data: null } when
roleProvider.findById(id) yields null; update the RolesController.findById
method to check the returned value and throw a NotFoundException (e.g., new
NotFoundException(`Role with id ${id} not found`)) when null, otherwise return {
data: role }; ensure NotFoundException is imported from `@nestjs/common` if not
already present.
Comment on lines
+30
to
+50
| // Union type to support both real Users and Pending Invitations in the same list | ||
| export type UserListItem = | ||
| | (User & { kind?: 'user' }) // Optional discriminator for backwards compat if needed, or strict: { kind: 'user' } | ||
| | { | ||
| kind: 'invitation'; | ||
| id: string; | ||
| email: string; | ||
| name: string; | ||
| role: string; | ||
| status: 'pending'; | ||
| emailVerified: boolean; | ||
| createdAt: Date; | ||
| updatedAt: Date; | ||
| isInvitation: true; | ||
| // Optional fields from User to satisfy strict typing if needed by consumers | ||
| permissions?: string[]; | ||
| image?: string; | ||
| banned?: boolean; | ||
| banReason?: string | null; | ||
| banExpires?: Date | null; | ||
| }; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
UserListItem union has redundant discriminators.
The invitation variant has both kind: 'invitation' and isInvitation: true. The user variant has kind?: 'user' (optional). This means consumers must check isInvitation or kind inconsistently. Consider making kind required on both variants for a clean discriminated union, or drop one of the two discriminator fields.
🤖 Prompt for AI Agents
In `@apps/api/src/modules/identity/users/users.controller.ts` around lines 30 -
50, UserListItem uses two discriminators (kind and isInvitation) which is
redundant; make the union a clean discriminated union by removing isInvitation
from the invitation variant and making kind a required literal on both branches
(set user variant to { kind: 'user' } and invitation variant to { kind:
'invitation' }), then update any consumers that check isInvitation to instead
check item.kind === 'invitation' (or adjust tests/uses of UserListItem
accordingly).
Comment on lines
+148
to
+155
| // Merge: Users first, then Pending Invites (or sort by date) | ||
| const combinedData = [...users, ...invitedUsers]; | ||
|
|
||
| // Return Refine-compatible pagination structure | ||
| return { | ||
| data: combinedData, | ||
| total: userTotal + invitations.length, | ||
| }; |
Contributor
There was a problem hiding this comment.
Bug: total uses unfiltered invitations.length instead of pendingInvitations.length.
Line 149 correctly merges users with pendingInvitations (filtered), but line 154 uses invitations.length (unfiltered) for the total. This causes total to be greater than data.length, breaking pagination metadata for consumers.
Proposed fix
return {
data: combinedData,
- total: userTotal + invitations.length,
+ total: userTotal + pendingInvitations.length,
};📝 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
| // Merge: Users first, then Pending Invites (or sort by date) | |
| const combinedData = [...users, ...invitedUsers]; | |
| // Return Refine-compatible pagination structure | |
| return { | |
| data: combinedData, | |
| total: userTotal + invitations.length, | |
| }; | |
| // Merge: Users first, then Pending Invites (or sort by date) | |
| const combinedData = [...users, ...invitedUsers]; | |
| // Return Refine-compatible pagination structure | |
| return { | |
| data: combinedData, | |
| total: userTotal + pendingInvitations.length, | |
| }; |
🤖 Prompt for AI Agents
In `@apps/api/src/modules/identity/users/users.controller.ts` around lines 148 -
155, The total returned for pagination is using the unfiltered invitations array
(invitations.length) which mismatches the merged data (combinedData built from
users and pendingInvitations); update the return to compute total using
userTotal + pendingInvitations.length instead of invitations.length so
pagination metadata matches combinedData (refer to combinedData, users,
invitedUsers, userTotal, invitations, pendingInvitations).
Comment on lines
+63
to
+77
| async update(id: string, input: UpdateRoleInput): Promise<RoleEntity> { | ||
| const [role] = await this.db | ||
| .update(schema.role) | ||
| .set({ | ||
| ...(input.name !== undefined ? { name: input.name } : {}), | ||
| ...(input.description !== undefined | ||
| ? { description: input.description } | ||
| : {}), | ||
| }) | ||
| .where(eq(schema.role.id, id)) | ||
| .returning(); | ||
|
|
||
| if (!role) throw new NotFoundException("Role not found"); | ||
| return role; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, run the reviewer's suggested verification
rg -n '\.update\(' --type=ts -A10 -g '**/*adapter*' | head -100Repository: pramodnarayana/nexiom
Length of output: 9109
🏁 Script executed:
# Also check the UpdateRoleInput type definition
rg -n 'UpdateRoleInput' --type=ts | head -20Repository: pramodnarayana/nexiom
Length of output: 711
🏁 Script executed:
# Look for the role schema definition to understand constraints
rg -n 'schema\.role' --type=ts -B2 -A2 | head -80Repository: pramodnarayana/nexiom
Length of output: 6381
🏁 Script executed:
# Check if there are any tests for the update method
find . -name "*.spec.ts" -o -name "*.test.ts" | xargs rg -l "drizzle-role" 2>/dev/null | head -5Repository: pramodnarayana/nexiom
Length of output: 126
🏁 Script executed:
# Look for how Drizzle ORM is used throughout the codebase with .set()
rg -n '\.set\(' --type=ts -B2 -A2 | head -120Repository: pramodnarayana/nexiom
Length of output: 8441
🏁 Script executed:
# Get the UpdateRoleInput interface definition
cat -n packages/identity/src/interfaces/role-provider.interface.ts | sed -n '19,30p'Repository: pramodnarayana/nexiom
Length of output: 443
🏁 Script executed:
# Check the drizzle-role adapter tests for edge cases
cat -n packages/identity/src/adapters/drizzle-role.adapter.spec.tsRepository: pramodnarayana/nexiom
Length of output: 8495
🏁 Script executed:
# Look at how the user adapter builds updates to understand the pattern
sed -n '40,65p' packages/identity/src/adapters/drizzle-user.adapter.tsRepository: pramodnarayana/nexiom
Length of output: 746
🏁 Script executed:
# Search for Drizzle ORM documentation or comments about empty .set() behavior
rg -B5 -A5 'updates.*=.*{}' --type=ts packages/identity/src/adapters/Repository: pramodnarayana/nexiom
Length of output: 174
🏁 Script executed:
# Verify tenant adapter pattern to confirm consistency
sed -n '100,145p' packages/identity/src/adapters/drizzle-tenant.adapter.tsRepository: pramodnarayana/nexiom
Length of output: 1266
🏁 Script executed:
# Check if there's any Drizzle ORM version or known behavior about empty .set()
rg -i 'drizzle' package.json | head -20Repository: pramodnarayana/nexiom
Length of output: 47
🏁 Script executed:
# Check the exact Drizzle version to understand potential behavior
rg 'drizzle-orm' package.json -A2Repository: pramodnarayana/nexiom
Length of output: 47
🏁 Script executed:
# Verify the role adapter has no updatedAt field like the user/tenant adapters do
sed -n '63,77p' packages/identity/src/adapters/drizzle-role.adapter.tsRepository: pramodnarayana/nexiom
Length of output: 536
🏁 Script executed:
# Check if there are any calls to role adapter update to understand real-world usage
rg 'roleProvider\.update\|adapter\.update' --type=ts -B2 -A2 | head -60Repository: pramodnarayana/nexiom
Length of output: 47
🏁 Script executed:
# Verify if the role schema has any auto-updated timestamp fields
rg -B5 -A20 'export const role' packages/identity/src/schema.ts | head -40Repository: pramodnarayana/nexiom
Length of output: 1480
Guard against empty .set() which produces invalid SQL.
If update(id, {}) is called or all input fields are undefined, the spread operators produce set({}), which is a SQL syntax error. The user adapter in this codebase demonstrates the proper pattern: check Object.keys(updates).length > 0 before executing the update query. The role schema lacks an updatedAt field (unlike user/tenant), making it vulnerable to this case.
Proposed fix
async update(id: string, input: UpdateRoleInput): Promise<RoleEntity> {
+ const updates: Record<string, unknown> = {};
+ if (input.name !== undefined) updates.name = input.name;
+ if (input.description !== undefined) updates.description = input.description;
+
+ if (Object.keys(updates).length === 0) {
+ const existing = await this.findById(id);
+ if (!existing) throw new NotFoundException("Role not found");
+ return existing;
+ }
+
const [role] = await this.db
.update(schema.role)
- .set({
- ...(input.name !== undefined ? { name: input.name } : {}),
- ...(input.description !== undefined
- ? { description: input.description }
- : {}),
- })
+ .set(updates)
.where(eq(schema.role.id, id))
.returning();
if (!role) throw new NotFoundException("Role not found");
return role;
}📝 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
| async update(id: string, input: UpdateRoleInput): Promise<RoleEntity> { | |
| const [role] = await this.db | |
| .update(schema.role) | |
| .set({ | |
| ...(input.name !== undefined ? { name: input.name } : {}), | |
| ...(input.description !== undefined | |
| ? { description: input.description } | |
| : {}), | |
| }) | |
| .where(eq(schema.role.id, id)) | |
| .returning(); | |
| if (!role) throw new NotFoundException("Role not found"); | |
| return role; | |
| } | |
| async update(id: string, input: UpdateRoleInput): Promise<RoleEntity> { | |
| const updates: Record<string, unknown> = {}; | |
| if (input.name !== undefined) updates.name = input.name; | |
| if (input.description !== undefined) updates.description = input.description; | |
| if (Object.keys(updates).length === 0) { | |
| const existing = await this.findById(id); | |
| if (!existing) throw new NotFoundException("Role not found"); | |
| return existing; | |
| } | |
| const [role] = await this.db | |
| .update(schema.role) | |
| .set(updates) | |
| .where(eq(schema.role.id, id)) | |
| .returning(); | |
| if (!role) throw new NotFoundException("Role not found"); | |
| return role; | |
| } |
🤖 Prompt for AI Agents
In `@packages/identity/src/adapters/drizzle-role.adapter.ts` around lines 63 - 77,
Build an updates object from the incoming UpdateRoleInput (in the update method
of drizzle-role.adapter.ts) instead of spreading directly into .set(); if the
updates object has no keys (Object.keys(updates).length === 0) abort before
calling this.db.update(schema.role) — e.g., throw a BadRequestException like "No
fields to update" — otherwise pass the updates object to .set(...) and proceed
with the existing where(eq(schema.role.id, id)).returning() logic, preserving
the current NotFoundException handling for a missing role.
Comment on lines
+296
to
+313
| then: (resolve: (val: any) => void) => { | ||
| // Differentiate queries based on chain structure (stable detection) | ||
| // Admin Count Query: .select({ count }).from(member).innerJoin(role)... | ||
| // Remaining Memberships: .select({ count }).from(member).where(...) -> NO innerJoin | ||
|
|
||
| const hasInnerJoin = chain.innerJoin.mock.calls.length > 0; | ||
| const hasLimit = chain.limit.mock.calls.length > 0; | ||
|
|
||
| if (hasInnerJoin) { | ||
| resolve(adminCount); | ||
| } else if (hasLimit) { | ||
| // Fallback for logic that uses limit with count (unlikely in current adapter but safe) | ||
| resolve(membership); | ||
| } else { | ||
| // Remaining memberships query (no join, no limit) | ||
| resolve(remainingMemberships); | ||
| } | ||
| }, |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Thenable mock object is a linter violation and a fragile test pattern.
Adding a then property to a plain object makes it a thenable, which Biome correctly flags (lint/suspicious/noThenProperty). While this is an intentional trick to simulate promise resolution in mock chains, it's fragile — the dispatch logic (lines 301–312) differentiates queries by inspecting innerJoin.mock.calls.length and limit.mock.calls.length, coupling tests tightly to the adapter's internal query construction order.
Consider replacing this with individual mock implementations per call order (e.g., using mockResolvedValueOnce sequencing) or splitting scenarios into separate test cases with narrower mocks. This would eliminate the thenable hack and the brittle call-count detection.
🧰 Tools
🪛 Biome (2.3.13)
[error] 296-296: Do not add then to an object.
(lint/suspicious/noThenProperty)
🤖 Prompt for AI Agents
In `@packages/identity/src/adapters/drizzle-user.adapter.spec.ts` around lines 296
- 313, The test creates a thenable by adding a then property to a plain object
and branches behavior by inspecting chain.innerJoin.mock.calls.length and
chain.limit.mock.calls.length, which lints as suspicious and makes the test
fragile; replace the thenable trick and call-count branching by providing
explicit mockResolvedValueOnce (or separate mock implementations) on the mocked
query chain methods used by the adapter (e.g., set sequential return values for
the function that returns the chain or use mockResolvedValueOnce on the
promise-returning method) or split the spec into separate test cases so each
scenario directly stubs the specific call sequence instead of reading
chain.innerJoin and chain.limit call counts; target the mocked chain object and
the adapter calls (chain.innerJoin, chain.limit, and the promise resolution
path) and sequence their resolved values per-case rather than using a then
property.
Comment on lines
+94
to
+126
| } catch (error) { | ||
| // Only return null for "not found" errors from authProvider | ||
| // All other errors (connection, query syntax, etc.) should propagate | ||
|
|
||
| if (error instanceof UserNotFoundError) { | ||
| return null; | ||
| } | ||
|
|
||
| // Check for provider-specific "not found" error properties if available | ||
| // Using strict regex matching to avoid swallowing unrelated errors | ||
| const errorMessage = | ||
| error instanceof Error ? error.message : String(error); | ||
|
|
||
| // Match user/entity not-found errors while avoiding infrastructure errors | ||
| // Accepts: "User not found", "Entity not found", "Record not found", "Not found" | ||
| // Rejects: "User session not found", "User token not found", "connection not found" | ||
| // Only allow whitespace between entity type and "not found" | ||
| const isUserNotFoundError = | ||
| /\b(user|entity|record)\s+not\s+found\b/i.test(errorMessage) || | ||
| /^not found$/i.test(errorMessage); | ||
| if (isUserNotFoundError) { | ||
| return null; | ||
| } | ||
|
|
||
| // Log and rethrow unexpected errors | ||
| // NestJS Logger.error(message, stack, context) | ||
| this.logger.error( | ||
| `findById failed for user ${id}: ${errorMessage}`, | ||
| error instanceof Error ? error.stack : undefined, | ||
| "DrizzleUserAdapter.findById", | ||
| ); | ||
| throw error; | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Regex-based "not found" detection is fragile.
The fallback regex matching (/\b(user|entity|record)\s+not\s+found\b/i) silently swallows errors from auth providers based on message text. A provider message like "Could not find user" or "No such user" would escape this check and propagate as an unexpected error.
Prefer a protocol-based approach: have auth providers throw UserNotFoundError (already supported at line 98) or decorate errors with a code property (e.g., error.code === 'NOT_FOUND'). The regex path is a reasonable interim fallback, but document it as such and consider tightening the contract with IAuthProvider.
🤖 Prompt for AI Agents
In `@packages/identity/src/adapters/drizzle-user.adapter.ts` around lines 94 -
126, The current catch in DrizzleUserAdapter.findById relies on a brittle regex
to detect "not found" from provider messages; update the handler to first check
well-defined protocol signals (e.g., if error instanceof UserNotFoundError OR if
(error as any).code === 'NOT_FOUND' or check a documented error.name like
'NotFoundError') and only use the regex as a documented fallback, keeping the
existing logger/rethrow behavior for anything else; also add a short inline
comment in the catch explaining that providers should throw UserNotFoundError or
set error.code = 'NOT_FOUND' (the regex remains an interim last-resort check).
Comment on lines
+343
to
+358
| // 6. If no memberships remain, HARD DELETE the user account (Orphan Cleanup) | ||
| if (membershipCount === 0) { | ||
| // Cascade delete user data (same as delete method) | ||
| await tx | ||
| .delete(schema.invitation) | ||
| .where(eq(schema.invitation.inviterId, userId)); | ||
| await tx | ||
| .delete(schema.session) | ||
| .where(eq(schema.session.userId, userId)); | ||
| await tx | ||
| .delete(schema.account) | ||
| .where(eq(schema.account.userId, userId)); | ||
| await tx.delete(schema.user).where(eq(schema.user.id, userId)); | ||
|
|
||
| hardDeleted = true; | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Hard-delete cascade duplicates delete() logic.
The cascade sequence here (invitation → session → account → user) is identical to delete() on lines 67–80. Extracting a shared private method would keep the two paths in sync and reduce maintenance risk.
Proposed refactor
+ private async cascadeDeleteUser(
+ tx: Parameters<Parameters<typeof this.db.transaction>[0]>[0],
+ userId: string,
+ ): Promise<void> {
+ await tx.delete(schema.member).where(eq(schema.member.userId, userId));
+ await tx.delete(schema.invitation).where(eq(schema.invitation.inviterId, userId));
+ await tx.delete(schema.session).where(eq(schema.session.userId, userId));
+ await tx.delete(schema.account).where(eq(schema.account.userId, userId));
+ await tx.delete(schema.user).where(eq(schema.user.id, userId));
+ }
async delete(id: string): Promise<void> {
await this.db.transaction(async (tx) => {
- await tx.delete(schema.member).where(eq(schema.member.userId, id));
- await tx.delete(schema.invitation).where(eq(schema.invitation.inviterId, id));
- await tx.delete(schema.session).where(eq(schema.session.userId, id));
- await tx.delete(schema.account).where(eq(schema.account.userId, id));
- await tx.delete(schema.user).where(eq(schema.user.id, id));
+ await this.cascadeDeleteUser(tx, id);
});
}Then in deleteIfNotLastAdmin, replace lines 346–355 with:
- await tx.delete(schema.invitation).where(eq(schema.invitation.inviterId, userId));
- await tx.delete(schema.session).where(eq(schema.session.userId, userId));
- await tx.delete(schema.account).where(eq(schema.account.userId, userId));
- await tx.delete(schema.user).where(eq(schema.user.id, userId));
+ // Member already deleted above (step 4), so only cascade remaining tables
+ await tx.delete(schema.invitation).where(eq(schema.invitation.inviterId, userId));
+ await tx.delete(schema.session).where(eq(schema.session.userId, userId));
+ await tx.delete(schema.account).where(eq(schema.account.userId, userId));
+ await tx.delete(schema.user).where(eq(schema.user.id, userId));Note: In deleteIfNotLastAdmin, the member row is already deleted at step 4, so the cascade helper would need to account for that (either skip member deletion or make it idempotent). Adjust accordingly.
🤖 Prompt for AI Agents
In `@packages/identity/src/adapters/drizzle-user.adapter.ts` around lines 343 -
358, The cascade hard-delete logic duplicated in deleteIfNotLastAdmin (the
tx.delete sequence deleting invitation, session, account, user) should be
extracted into a single private helper (e.g., private async
cascadeHardDeleteUser(tx, userId) or similar) and then invoked from both
delete() and deleteIfNotLastAdmin; ensure the helper encapsulates the tx.delete
calls for schema.invitation, schema.session, schema.account, and schema.user,
and make it idempotent or skip member deletion since deleteIfNotLastAdmin
already removed the member row—replace the inline deletes in
deleteIfNotLastAdmin with a call to this helper so both paths stay in sync.
Comment on lines
+13
to
+17
| export interface CreateRoleInput { | ||
| name: string; | ||
| description?: string; | ||
| isSystem?: boolean; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "isSystem" --type=ts -g '!node_modules' -g '!dist' -C 3Repository: pramodnarayana/nexiom
Length of output: 22902
🏁 Script executed:
find . -path ./node_modules -prune -o -path ./dist -prune -o -name "*roles.controller*" -type f -printRepository: pramodnarayana/nexiom
Length of output: 185
🏁 Script executed:
cat -n ./apps/api/src/modules/identity/roles/roles.controller.tsRepository: pramodnarayana/nexiom
Length of output: 2254
🏁 Script executed:
cat -n ./apps/api/src/modules/identity/roles/roles.controller.spec.ts | head -100Repository: pramodnarayana/nexiom
Length of output: 3694
🏁 Script executed:
cat -n ./apps/api/src/modules/identity/roles/roles.controller.spec.ts | sed -n '95,130p'Repository: pramodnarayana/nexiom
Length of output: 1526
🏁 Script executed:
# Check if there's any service layer or additional validation
find . -path ./node_modules -prune -o -path ./dist -prune -o -type f -name "*.ts" -path "*/roles/*" -print | head -20Repository: pramodnarayana/nexiom
Length of output: 239
🏁 Script executed:
cat -n ./apps/api/src/modules/identity/roles/roles.module.tsRepository: pramodnarayana/nexiom
Length of output: 284
🏁 Script executed:
# Check if isSystem is used in any permission checks or authorization logic
rg "isSystem" --type=ts -g '!node_modules' -g '!dist' -B 2 -A 2 | grep -A 5 -B 5 "permission\|auth\|guard\|check"Repository: pramodnarayana/nexiom
Length of output: 3602
Remove or restrict isSystem from CreateRoleInput.
Any caller with roles:create permission can currently set isSystem: true when creating a role. System roles should only be created during seeding, not via the API. Either remove isSystem from CreateRoleInput entirely or add explicit validation in the controller to strip it and default to false.
🤖 Prompt for AI Agents
In `@packages/identity/src/interfaces/role-provider.interface.ts` around lines 13
- 17, The CreateRoleInput interface currently exposes isSystem allowing any
caller with roles:create to mark roles as system; remove isSystem from
CreateRoleInput (or if you prefer to keep the shape for internal use, make it
optional-only in internal DTOs) and enforce in the controller (where
CreateRoleInput is consumed) that isSystem is never accepted from API requests
by stripping/ignoring that field and defaulting to false before creating roles;
update any usage sites that construct CreateRoleInput (and relevant tests) to
stop passing isSystem, and ensure seeding code that must create system roles
uses an internal API or a separate seed-only creator that can set isSystem.
Comment on lines
+37
to
+47
| /** | ||
| * Updates a role. | ||
| * Throws NotFoundException if role does not exist. | ||
| */ | ||
| update(id: string, input: UpdateRoleInput): Promise<RoleEntity>; | ||
|
|
||
| /** | ||
| * Deletes a role. | ||
| * Throws NotFoundException if role does not exist. | ||
| */ | ||
| delete(id: string): Promise<void>; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
JSDoc references NotFoundException — avoid framework coupling in a package-level interface.
The @throws NotFoundException JSDoc ties this interface to NestJS semantics. Since this lives in packages/identity (shared library), document it as "throws if the role does not exist" and let concrete implementations choose the appropriate exception type.
Proposed doc update
/**
* Updates a role.
- * Throws NotFoundException if role does not exist.
+ * Throws if the role does not exist.
*/
update(id: string, input: UpdateRoleInput): Promise<RoleEntity>;
/**
* Deletes a role.
- * Throws NotFoundException if role does not exist.
+ * Throws if the role does not exist.
*/
delete(id: string): Promise<void>;🤖 Prompt for AI Agents
In `@packages/identity/src/interfaces/role-provider.interface.ts` around lines 37
- 47, The JSDoc on the RoleProvider interface is framework-coupled: update(id:
string, input: UpdateRoleInput): Promise<RoleEntity> and delete(id: string):
Promise<void> currently mention "Throws NotFoundException"; change those
comments to a framework-agnostic statement such as "Throws if the role does not
exist" (or "Throws an error if the role does not exist") so the interface
(role-provider.interface.ts) references only the condition, not NestJS's
NotFoundException, leaving concrete implementations to map to framework-specific
exceptions.
This was referenced Feb 10, 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
Release Notes
New Features
Improvements
Bug Fixes