Repository navigation
fix(test): stabilize unit tests and platform_user role - #24
Merged
Merged
Conversation
- Fixed API tests: Updated mocks for better-auth, invitations controller, and validation schemas. - Fixed Web tests: Refactored SignupPage.spec.tsx to use MemoryRouter. - Refactor: Finalized platform_user system role and admin access logic. - Feat: Added admin reset script. - Chore: Applied granular lint suppressions for mocks.
Contributor
📝 WalkthroughWalkthroughThreads request headers through auth and invitation flows, moves invitation acceptance to a DB-backed transactional path with rollback for newly created users, renames role Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,200,255,0.5)
participant Client as Client
participant AdminCtrl as SystemAdminController
participant InvSvc as InvitationsService
participant IdProvider as IdentityProvider (BetterAuth)
participant DB as Database
end
Client->>AdminCtrl: POST /admin/invitations {email, role, headers}
AdminCtrl->>IdProvider: getSessionFromHeaders(headers)
IdProvider->>DB: Query session/user by headers
DB-->>IdProvider: Session + User
IdProvider-->>AdminCtrl: Admin user
AdminCtrl->>InvSvc: create(dto, adminId, headers)
InvSvc->>IdProvider: createInvitation(payload, headers)
IdProvider->>DB: Insert invitation or call external API
DB-->>IdProvider: Created invitation
IdProvider-->>InvSvc: Invitation
InvSvc-->>AdminCtrl: Invitation created
AdminCtrl-->>Client: 200 {invitation}
sequenceDiagram
rect rgba(200,255,200,0.5)
participant Client as Client
participant AuthCtrl as AuthController
participant IdProvider as IdentityProvider (BetterAuth)
participant DB as Database
end
Client->>AuthCtrl: POST /auth/complete-invite {invitationId, userData, headers}
AuthCtrl->>IdProvider: getInvitation(invitationId, headers)
IdProvider->>DB: Query invitation
DB-->>IdProvider: Invitation record
IdProvider-->>AuthCtrl: Invitation
AuthCtrl->>IdProvider: createUser(userData, headers)
IdProvider->>DB: Create user record / sign-up API
DB-->>IdProvider: New user
IdProvider-->>AuthCtrl: User created
AuthCtrl->>IdProvider: acceptInvitation(invitationId, userId, headers)
IdProvider->>DB: Transaction -> update invitation, add membership or update role
DB-->>IdProvider: Transaction success
IdProvider-->>AuthCtrl: Accepted
AuthCtrl-->>Client: 200 {session, user}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
apps/api/src/modules/system-admin/system-admin.validation.spec.ts (1)
78-83: Rename the test to match the new default.
The test title still says “user” even though the default is now platform_user.✏️ Suggested rename
- it('should default systemRole to user', () => { + it('should default systemRole to platform_user', () => {apps/api/src/modules/auth/providers/better-auth/better-auth.provider.spec.ts (1)
211-234: Minor formatting inconsistency.Lines 213-214 and 228-229 have an awkward blank line before
.mockResolvedValue(). This appears to be accidental formatting.Suggested fix
const findFirstSpy = jest .spyOn(provider['db'].query.session, 'findFirst') - .mockResolvedValue(null as any);apps/api/src/modules/invitations/invitations.controller.ts (2)
29-35: Minor style inconsistency withacceptmethod.Line 30 assigns
req.headersto a variable before passing it, while line 53 passesreq.headersdirectly. Consider applying the same pattern for consistency.♻️ Optional: Use consistent header passing style
// Pass headers to propagate auth context to BetterAuth client - const headers = req.headers; return this.invitationsService.create( createInvitation, req.user.id, - headers, + req.headers, );
38-42: Existing TODO: Consider guarding thegetendpoint.The comment indicates this endpoint may need protection or ID validation. While public access may be intentional for invitation links, exposing invitation details without authentication could leak information (inviter, organization, email).
Would you like me to help address this, or should this be tracked as a separate issue?
apps/web/src/pages/SignupPage.tsx (1)
34-37: CallingsetEmailduring render is a React anti-pattern.Setting state directly in the component body (outside an effect) can trigger warnings and potential infinite re-renders. Move this logic into a
useEffecthook.🐛 Proposed fix
+import { useState, useEffect } from 'react'; -import { useState } from 'react';Then replace lines 34-37 with:
- // Pre-fill email if provided - if (emailParam && !email) { - setEmail(emailParam); - } + // Pre-fill email if provided + useEffect(() => { + if (emailParam) { + setEmail(emailParam); + } + }, [emailParam]);
🤖 Fix all issues with AI agents
In `@apps/api/package.json`:
- Line 57: Remove the deprecated dependency entry "@types/dotenv" from
package.json (the dependency key in apps/api package.json) and update the
lockfile by running the package manager to reinstall (e.g., npm install or yarn
install) so types come from the bundled dotenv package; ensure no other code
imports/@types references remain and remove any explicit references to
"@types/dotenv" from CI or build scripts.
In `@apps/api/src/modules/auth/auth.controller.ts`:
- Around line 94-97: Define a concrete Invitation interface and update the
IdentityProvider.getInvitation (or invitationsService.get) return type from
Promise<unknown> to Promise<Invitation | null>, then replace the inline
assertion in auth.controller.ts where invitationsService.get is called (the
variable named invitation) so it receives the typed result; update any
implementations of IdentityProvider.getInvitation (or invitationsService.get) to
return the new Invitation shape to keep types consistent across
InvitationsService and callers.
In `@apps/api/src/modules/auth/identity-provider.abstract.ts`:
- Around line 91-95: The abstract method acceptInvitation has a misleading
parameter name; rename the second parameter from inviterId to userId in the
acceptInvitation declaration (method signature in identity-provider.abstract.ts)
and update all implementing classes, overrides, and call sites (e.g., where
auth.controller.ts calls acceptInvitation with user.id) to use the new userId
name, preserving the same type and behavior; also update any JSDoc/comments and
exported types/interfaces that reference inviterId to userId to keep signatures
consistent.
In `@apps/api/src/modules/auth/providers/better-auth/better-auth.provider.ts`:
- Around line 476-484: The public getInvitation currently returns any invite by
id; update getInvitation to filter out invites that are expired or already
accepted before returning. Modify the query in getInvitation (the call to
this.db.query.invitation.findFirst) to include conditions on the invitation
status (e.g., not "accepted") and on the expiry timestamp (e.g., expiresAt >
now) using the schema fields (schema.invitation.status,
schema.invitation.expiresAt) or, alternately, fetch and then check those fields
and return null if expired/accepted; ensure the function returns null for
expired or accepted invites instead of the record.
- Around line 486-545: acceptInvitation currently grants membership or
systemRole without verifying the acceptor's identity; before creating the member
or updating schema.user, fetch the user by inviterId (e.g.,
tx.query.user.findFirst or equivalent) and compare the user's email to
invitation.email (handle casing/trim); if they do not match, throw an
Authorization/Validation error and abort the transaction; only proceed to insert
into schema.member or update schema.user.systemRole when the email check passes.
- Around line 133-143: Remove the raw console.error debug prints in the
BetterAuth provider constructor that log Object.keys(this.auth) and
Object.keys((this.auth as any).api) (and the commented console.dir) and replace
them with Nest's Logger debug/info calls; gate those Logger.debug calls behind a
debug flag or environment check (e.g., a provider-level debug config or NODE_ENV
!== 'production') so they don't run in production. Locate the constructor in
better-auth.provider.ts (references: this.auth, (this.auth as any).api) and swap
console.error usages for this.logger.debug(...) or inject/use Logger, ensuring
all debug output is conditional on the chosen debug flag.
In `@apps/api/src/modules/auth/system-admin.guard.ts`:
- Around line 32-38: This guard currently allows platform_user and thus opens
admin endpoints; decide intent and act accordingly: if unintentional, modify
SystemAdminGuard to authorize only user.systemRole === 'platform_admin' (remove
the platform_user branch) and ensure the thrown ForbiddenException message
remains "Requires Platform Admin Privileges"; if intentional, rename
SystemAdminGuard and the error message to reflect platform_user access (e.g.,
PlatformUserGuard or update message to indicate platform user access) and update
any related documentation; in either case add/adjust unit tests to cover both
platform_admin and platform_user scenarios (and reference
better-auth.provider.ts which sets the default role) so this behavior cannot
regress.
In `@apps/api/src/modules/system-admin/system-admin.controller.ts`:
- Around line 72-95: Add a proper input validation schema and standardize the
default role to 'platform_user': create a Zod schema named
CreateSystemInvitationValidation (email: z.string().email(), role:
z.enum(['platform_admin','platform_user']).default('platform_user')) and use it
to validate/parse the request body at the start of createSystemInvitation;
replace any hardcoded default role 'user' in this controller (e.g., in
createSystemInvitation and the other places in this controller that set role
defaults) to use the validated value or the 'platform_user' default so invalid
roles are rejected and the default is consistent.
In `@apps/api/src/scripts/force-reset-admin.ts`:
- Around line 47-61: Remove the redundant conditional checks around schema
tables in the cleanup block: delete the extra `if (schema.session)`, `if
(schema.account)`, `if (schema.member)`, and `if (schema.invitation)` guards and
invoke the deletion calls directly using
`db.delete(schema.session).where(eq(schema.session.userId, userId))`,
`db.delete(schema.account).where(eq(schema.account.userId, userId))`,
`db.delete(schema.member).where(eq(schema.member.userId, userId))`, and
`db.delete(schema.invitation).where(eq(schema.invitation.inviterId, userId))` so
the code matches the style in `bootstrap-admin.ts`/`reset-db.ts` and relies on
the imported `schema` tables being defined.
- Around line 88-101: The process exits on error paths (the !res.ok branch using
res.ok and the catch that logs 'Failed to contact API' and calls
process.exit(1)) without closing the DB client opened earlier; update both error
paths to first await closing the DB connection (use the actual DB client
variable created earlier—e.g., prisma, dbClient, or whatever was instantiated on
line 28—and call its cleanup method such as prisma.$disconnect() or
dbClient.close()/disconnect()), then call process.exit(1); ensure the cleanup is
awaited so the connection is closed before exiting.
- Around line 12-20: Replace hardcoded EMAIL and PASSWORD with environment
variables (e.g., process.env.ADMIN_EMAIL and process.env.ADMIN_PASSWORD) and
validate they exist like dbUrl does; update any code using EMAIL or PASSWORD
(refer to the constants EMAIL and PASSWORD) to read from these env vars. Remove
or change any plain-text logging of the password (any console.log or logger
usage that prints PASSWORD) to either omit the value or print a masked version
(e.g., show only last 2 chars or fixed asterisks) and ensure error messages do
not echo credentials.
In `@apps/api/test/invitations.e2e-spec.ts`:
- Around line 80-83: The afterAll hook currently only calls app.close(); update
the afterAll in invitations.e2e-spec.ts to delete the test user created with
testEmail before closing the app: locate the afterAll block and add an awaited
call to the application’s cleanup/DB API (e.g., UserService.deleteByEmail or the
test Prisma client’s user.deleteMany({ where: { email: testEmail } })) using the
same test helpers/clients used elsewhere in the spec, ensure the deletion runs
before await app.close(), and guard it so it doesn’t throw if the user is
already absent.
- Around line 47-77: Replace the runtime require('./../src/db/schema') with a
static ES import of the schema at the top of the test file so TypeScript can
type-check schema usage (references: schema, db, eq, invitation, user, insert,
delete). Update the file to import the schema module via an ES import statement
and remove the require call in the cleanup/setup block; ensure the test
tsconfig/jest/Vite config supports TS imports so the imported schema types are
available to the db.delete/db.insert calls.
In `@apps/web/src/layouts/AdminLayout.tsx`:
- Around line 147-150: Add a test that verifies users with systemRole
'platform_user' are allowed into the admin area just like 'platform_admin';
locate the AdminLayout access check (the conditional that checks user.systemRole
and calls navigate('/dashboard')) and create a unit/integration test that
renders AdminLayout (or the same test harness used for the existing
'platform_admin' test), stubs/mocks navigate, supplies a user object with
systemRole: 'platform_user', and asserts that navigate('/dashboard') is NOT
called (or that the admin content is rendered) to confirm access is permitted.
In `@apps/web/src/lib/auth/types.ts`:
- Line 14: Remove the stale 'user' literal from the systemRole union in the auth
type so it matches backend schema; update the declaration for systemRole in the
types file (systemRole?: 'platform_admin' | 'platform_user') and then search for
any usages of the 'user' value (checks, switches, defaults, tests, or
serializers) and replace or remove those branches to align with the two valid
roles ('platform_admin' and 'platform_user'), ensuring any runtime assumptions
or fallbacks are adjusted accordingly.
In `@apps/web/src/pages/public/AcceptInvitePage.tsx`:
- Around line 74-89: The CardHeader/CardDescription currently always shows
"Validating your invitation…" even when status === 'error' or inviteId is falsy;
update the rendering logic in AcceptInvitePage so CardDescription is conditional
based on the same check used for the error panel (use the status and inviteId
variables) and display a matching error header like "Invitation invalid or
expired" when showing the error UI, otherwise keep the original "Validating your
invitation…" text; locate the CardHeader/CardDescription JSX in the
AcceptInvitePage component to implement the conditional text.
In `@lint_report.txt`:
- Around line 116-127: The lint failure is caused by untyped/incorrect mock
implementations and ignored ESLint directives; remove blanket /* eslint-disable
*/ from better-auth.mock.ts and invitations.e2e-spec.ts, then properly type the
mocks and test helpers (e.g., give the mock object in better-auth.mock.ts the
same interface/type as the real auth provider, add explicit return types and
parameter types to async test helpers to eliminate require-await, and fix unsafe
member access in better-auth.provider.ts by typing the object/methods referenced
around the disabled block), update invitations.e2e-spec.ts and
system-admin.controller.spec.ts to import and use the correct types for mocked
functions/args, and finally if disable comments still don’t take effect, check
project ESLint config (parser, overrides, and ignore patterns) to ensure inline
eslint-disable directives are honored.
Comment on lines
94
to
97
| const invitation = (await this.invitationsService.get( | ||
| body.invitationId, | ||
| req.headers, | ||
| )) as { status: string; expiresAt: Date } | null; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider defining a typed return for getInvitation.
The inline type assertion works but is fragile. Consider defining a proper Invitation interface and updating the getInvitation return type in IdentityProvider from Promise<unknown> to Promise<Invitation | null> for better type safety.
🤖 Prompt for AI Agents
In `@apps/api/src/modules/auth/auth.controller.ts` around lines 94 - 97, Define a
concrete Invitation interface and update the IdentityProvider.getInvitation (or
invitationsService.get) return type from Promise<unknown> to Promise<Invitation
| null>, then replace the inline assertion in auth.controller.ts where
invitationsService.get is called (the variable named invitation) so it receives
the typed result; update any implementations of IdentityProvider.getInvitation
(or invitationsService.get) to return the new Invitation shape to keep types
consistent across InvitationsService and callers.
Comment on lines
91
to
95
| abstract acceptInvitation( | ||
| invitationId: string, | ||
| inviterId: string, | ||
| headers?: Headers | Record<string, any>, | ||
| ): Promise<unknown>; |
Contributor
There was a problem hiding this comment.
Misleading parameter name: inviterId should be userId.
The second parameter represents the user accepting the invitation, not the inviter. The call site in auth.controller.ts passes user.id (the accepting user). Rename for clarity.
✏️ Suggested fix
abstract acceptInvitation(
invitationId: string,
- inviterId: string,
+ userId: string,
headers?: Headers | Record<string, any>,
): Promise<unknown>;📝 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
| abstract acceptInvitation( | |
| invitationId: string, | |
| inviterId: string, | |
| headers?: Headers | Record<string, any>, | |
| ): Promise<unknown>; | |
| abstract acceptInvitation( | |
| invitationId: string, | |
| userId: string, | |
| headers?: Headers | Record<string, any>, | |
| ): Promise<unknown>; |
🤖 Prompt for AI Agents
In `@apps/api/src/modules/auth/identity-provider.abstract.ts` around lines 91 -
95, The abstract method acceptInvitation has a misleading parameter name; rename
the second parameter from inviterId to userId in the acceptInvitation
declaration (method signature in identity-provider.abstract.ts) and update all
implementing classes, overrides, and call sites (e.g., where auth.controller.ts
calls acceptInvitation with user.id) to use the new userId name, preserving the
same type and behavior; also update any JSDoc/comments and exported
types/interfaces that reference inviterId to userId to keep signatures
consistent.
- feat: restrict SystemAdminGuard to 'platform_admin' only - fix(input-validation): add strict Zod schema for new admin invitations - fix(security): harden force-reset-admin script with env vars & masking - fix(e2e): improve invitations.e2e-spec.ts cleanup and strict types - refactor: extract Invitation interface to resolve circular dependency - refactor(web): strict systemRole typing (platform_admin | platform_user) - feat(web): add AdminLayout access tests for platform_user - feat(web): improve AcceptInvitePage UI for invalid vs missing links
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/api/src/modules/system-admin/system-admin.controller.ts (1)
145-159: Inconsistent default role - should be'platform_user'.Line 151 uses
'user'as the defaultsystemRole, but this is inconsistent with:
- Line 64 which uses
'platform_user'- The DB schema (
user.schema.ts) which defaultssystemRoleto'platform_user'- The PR's objective to standardize on
'platform_user'Proposed fix
const [user] = await this.db .insert(schema.user) .values({ id: uuidv4(), name: input.name, email: input.email, - systemRole: input.systemRole || 'user', + systemRole: input.systemRole || 'platform_user', emailVerified: false, createdAt: new Date(), updatedAt: new Date(), }) .returning();apps/api/src/modules/auth/auth.controller.ts (1)
111-159: Rollback deletes pre‑existing users and logs PII.If accept fails for a provisioned user, the current rollback deletes that existing account. Also, debug logs emit
user.email. Gate deletion to newly created users and remove/mask PII logs.🔧 Suggested fix
- let user = await this.authProvider.getUserByEmail(body.email); + const existingUser = await this.authProvider.getUserByEmail(body.email); + const isNewUser = !existingUser; + let user = existingUser; @@ - console.log( - 'DEBUG: completeInvite - accepting invitation for user:', - user.id, - user.email, - ); await this.invitationsService.accept( body.invitationId, user.id, req.headers, ); } catch (e) { - console.error('DEBUG: acceptInvitation failed:', e); - // Rollback: Delete the user if acceptance fails to prevent orphans - console.log('DEBUG: Rolling back user creation:', user.id); - await this.authProvider.deleteUser(user.id); - throw new BadRequestException( - 'Failed to accept invitation (User creation rolled back)', - ); + // Rollback only if the user was created in this flow + if (isNewUser) { + await this.authProvider.deleteUser(user.id); + } + throw new BadRequestException('Failed to accept invitation'); }apps/api/src/modules/auth/identity-provider.abstract.ts (1)
3-27: Remove duplicate JSDoc comment blocks.The same comment "Abstract Class defining the contract for Identity Providers..." appears four times (lines 3-9, 13-16, 19-22, 24-27). This appears to be a copy-paste error from merging or refactoring.
🧹 Suggested cleanup
import { CreateUser } from '../users/users.validation'; - -/** - * Abstract Class defining the contract for Identity Providers. - * This allows swapping Better Auth with Auth0/Keycloak without changing business logic. - * - * We use an abstract class instead of an interface so it can be used - * as a Dependency Injection token in NestJS. - */ import { User } from '../users/user.schema'; import { Session } from './auth.schema'; - -/** - * Abstract Class defining the contract for Identity Providers. - * ... - */ import { Invitation } from '../invitations/invitation.interface'; /** * Abstract Class defining the contract for Identity Providers. - * ... - */ - -/** - * Abstract Class defining the contract for Identity Providers. - * ... + * This allows swapping Better Auth with Auth0/Keycloak without changing business logic. + * + * We use an abstract class instead of an interface so it can be used + * as a Dependency Injection token in NestJS. */ export abstract class IdentityProvider {
🤖 Fix all issues with AI agents
In `@apps/api/src/modules/auth/providers/better-auth/better-auth.provider.ts`:
- Around line 133-148: Remove the duplicated initialization log by keeping only
one occurrence of this.logger.log('Better Auth Initialized (with Injected DB)')
and deleting the redundant identical call; ensure the remaining log stays before
the NODE_ENV debug block so the existing debug statements (that reference
this.auth and (this.auth as any).api) remain unchanged.
- Around line 278-307: The fallback session sync in the BetterAuthProvider (the
dbSession check and synthetic sessionData creation) can mask real bugs; modify
the code in the block that builds sessionData and inserts into schema.session so
that when the environment is not a test (e.g., process.env.NODE_ENV !== 'test'
or using an injected config flag) you both emit a higher-severity alert
(this.logger.error or this.logger.warn with clear context including truncated
result.token and userId) and record a metric (e.g.,
this.metrics.increment('auth.session.sync_fallback')) before inserting;
additionally consider gating the synthetic insert behind an explicit test-only
flag to avoid automatic production writes and keep the existing synthetic
behaviour only for test/E2E flows.
In `@apps/api/src/modules/invitations/invitations.controller.ts`:
- Around line 29-35: Remove the duplicated comment above the call to
invitationsService.create in invitations.controller.ts: keep a single instance
of the comment that explains passing headers to propagate auth context to the
BetterAuth client and delete the repeated line so the code block around
invitationsService.create(createInvitation, req.user.id, req.headers) contains
only one explanatory comment.
In `@apps/api/src/modules/system-admin/system-admin.validation.spec.ts`:
- Around line 78-84: The controller currently falls back to the wrong default
systemRole value; update the default assignment used at the systemRole fallback
in system-admin.controller.ts (the code around the systemRole/default handling
at the current line ~151) from 'user' to 'platform_user' so it matches
CreateUserSchema; then update all tests/mocks that expect 'user' to instead use
'platform_user' (system-admin.controller.spec.ts occurrences,
system-admin.guard.spec.ts mock, and better-auth.provider.spec.ts mocks at the
lines called out) so implementation and test fixtures are consistent with the
validation schema.
In `@apps/api/src/scripts/force-reset-admin.ts`:
- Around line 12-26: Replace the real-looking fallback credentials in the top of
the script: update the EMAIL and PASSWORD constants (currently defined as EMAIL
and PASSWORD) to either remove hardcoded fallbacks and require
process.env.ADMIN_EMAIL/process.env.ADMIN_PASSWORD (throw/exit if missing) or
change them to clearly fake/dev-only values like 'admin@localhost' and
'changeme' and keep the existing warning; ensure API_URL remains unchanged and
that the script enforces use of env vars by checking
process.env.ADMIN_EMAIL/process.env.ADMIN_PASSWORD before proceeding (adjust the
conditional that currently logs the warning to exit when you choose to enforce).
- Around line 131-134: The top-level catch on reset() can leak the DB connection
if an error is thrown after client.connect(); update reset() so the database
client is always closed by either wrapping the entire reset() function body in a
try/finally that calls client.end()/client.close() in the finally block, or by
ensuring the top-level catch calls client.end()/client.close() before
process.exit(1); locate the DB client usage around client.connect() and the
role-elevation block (lines mentioning client.connect() and the elevation code)
and add the guaranteed cleanup there.
In `@apps/api/src/test/mocks/better-auth.mock.ts`:
- Around line 21-40: The signUpEmail mock creates a dynamic user id but leaves
session.userId hardcoded, causing a mismatch between user.id and session.userId;
update the signUpEmail implementation so that the generated id (the variable id)
is reused for session.userId (i.e., set session.userId to the same id value) so
user.id and session.userId always match when signUpEmail returns.
- Around line 82-90: The getInvitation mock returns role: 'user' which is now
renamed to 'platform_user' across the codebase; update the mock inside the
getInvitation function (signature: getInvitation(opts: MockOptions)) to return
role: 'platform_user' instead of 'user' so tests and consumers of this mock use
the new role name consistently.
In `@apps/api/test/invitations.e2e-spec.ts`:
- Around line 29-43: The environment variables are being set inside beforeAll
after AppModule is imported which can miss module-level reads; move the
process.env assignments so they run before AppModule is imported (e.g., top of
this test file before any imports) or place them in a Jest global setup file and
reference it via setupFiles/setupFilesAfterEnv; update the test to ensure
process.env.BETTER_AUTH_URL, ALLOWED_ORIGINS, FRONTEND_URL, and
BETTER_AUTH_SECRET are initialized prior to creating TestingModule (used in
createTestingModule/overrideGuard/overrideProvider) so AppModule and its
providers see the correct values at import/initialization time.
In `@apps/web/src/pages/public/AcceptInvitePage.tsx`:
- Around line 88-95: The conditional rendering in AcceptInvitePage.tsx is
unreachable because status === 'error' is only set when !inviteId, making the
inviteId branch impossible; either (A) if you expect server-side invite
validation failures, set status = 'error' in the server-validation failure path
(e.g., inside the fetch/validateInvite logic or useEffect that checks the
invite) so the "Invitation Invalid" branch can run, referencing the status and
inviteId variables, or (B) if you don't plan server validation, simplify the JSX
by removing the status === 'error' check and base the message solely on
!inviteId (update the conditional around the <p> elements accordingly) to
eliminate dead code.
♻️ Duplicate comments (1)
apps/api/src/scripts/force-reset-admin.ts (1)
119-121: Password masking still reveals partial credentials.The current masking
${PASSWORD.substring(0, 2)}***${PASSWORD.substring(PASSWORD.length - 2)}shows the first and last two characters (e.g.,pa***23), which reveals password structure. For consistency withbootstrap-admin.tswhich uses********, consider full masking.Suggested fix
- console.log( - ` Password: ${PASSWORD.substring(0, 2)}***${PASSWORD.substring(PASSWORD.length - 2)}`, - ); + console.log(` Password: ********`);
Comment on lines
+278
to
307
| let dbSession = await this.db.query.session.findFirst({ | ||
| where: eq(schema.session.token, result.token), | ||
| }); | ||
|
|
||
| if (!dbSession) { | ||
| // Fallback or retry? If BetterAuth just created it, it should be there. | ||
| // If not found, maybe result.token IS the session ID? | ||
| // BetterAuth usually uses token as the lookup. | ||
| this.logger.warn( | ||
| `Session for token ${result.token.substring(0, 10)}... not found in DB. Syncing...`, | ||
| ); | ||
|
|
||
| // Force Sync Session (Critical for E2E mocks) | ||
| const sessionData = { | ||
| id: result.session?.id || uuidv4(), | ||
| token: result.token, | ||
| userId: result.user.id, | ||
| expiresAt: new Date(result.session?.expiresAt || Date.now() + 86400000), | ||
| createdAt: new Date(), | ||
| updatedAt: new Date(), | ||
| userAgent: 'system-sync', | ||
| ipAddress: '127.0.0.1', | ||
| }; | ||
|
|
||
| await this.db.insert(schema.session).values(sessionData); | ||
| dbSession = await this.db.query.session.findFirst({ | ||
| where: eq(schema.session.token, result.token), | ||
| }); | ||
| } | ||
|
|
||
| if (!dbSession) { | ||
| throw new Error('Login succeeded but session record not found in DB'); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Defensive session synchronization may mask underlying issues.
The fallback session creation (lines 288-302) uses synthetic data (userAgent: 'system-sync', ipAddress: '127.0.0.1') when a session isn't found in DB. While this stabilizes E2E tests with mocks, it could hide real synchronization bugs in production.
Consider adding a metric or more prominent warning when this fallback path is hit in non-test environments.
💡 Optional: Add environment-specific warning
if (!dbSession) {
- this.logger.warn(
- `Session for token ${result.token.substring(0, 10)}... not found in DB. Syncing...`,
- );
+ const isTestEnv = process.env.NODE_ENV === 'test';
+ if (!isTestEnv) {
+ this.logger.error(
+ `Session for token ${result.token.substring(0, 10)}... not found in DB. Forcing sync - this may indicate a DB replication issue.`,
+ );
+ } else {
+ this.logger.warn(
+ `Session for token ${result.token.substring(0, 10)}... not found in DB. Syncing (test env)...`,
+ );
+ }🤖 Prompt for AI Agents
In `@apps/api/src/modules/auth/providers/better-auth/better-auth.provider.ts`
around lines 278 - 307, The fallback session sync in the BetterAuthProvider (the
dbSession check and synthetic sessionData creation) can mask real bugs; modify
the code in the block that builds sessionData and inserts into schema.session so
that when the environment is not a test (e.g., process.env.NODE_ENV !== 'test'
or using an injected config flag) you both emit a higher-severity alert
(this.logger.error or this.logger.warn with clear context including truncated
result.token and userId) and record a metric (e.g.,
this.metrics.increment('auth.session.sync_fallback')) before inserting;
additionally consider gating the synthetic insert behind an explicit test-only
flag to avoid automatic production writes and keep the existing synthetic
behaviour only for test/E2E flows.
Comment on lines
+78
to
84
| it('should default systemRole to platform_user', () => { | ||
| const result = CreateUserSchema.parse({ | ||
| name: 'Standard User', | ||
| email: 'user@nexiom.com', | ||
| }); | ||
| expect(result.systemRole).toBe('user'); | ||
| expect(result.systemRole).toBe('platform_user'); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "systemRole.*['\"]user['\"]" --type ts -g '!node_modules/**'Repository: pramodnarayana/nexiom
Length of output: 1104
🏁 Script executed:
rg -A 5 "CreateUserSchema.*=" apps/api/src/modules/system-admin/ --type ts -g '!*.spec.ts'Repository: pramodnarayana/nexiom
Length of output: 666
🏁 Script executed:
sed -n '145,160p' apps/api/src/modules/system-admin/system-admin.controller.tsRepository: pramodnarayana/nexiom
Length of output: 408
Update system-admin.controller.ts line 151 to use the correct default 'platform_user'.
The validation schema correctly defaults systemRole to 'platform_user', and the test validates this. However, the controller implementation at line 151 still falls back to 'user' instead of 'platform_user'. This mismatch causes the implementation to contradict the schema contract.
Additionally, multiple test files use 'user' in mock data and should be updated for consistency:
system-admin.controller.spec.ts(lines 278, 292, 301, 308, 598)system-admin.guard.spec.ts(line 59)better-auth.provider.spec.ts(lines 198, 249, 262)
🤖 Prompt for AI Agents
In `@apps/api/src/modules/system-admin/system-admin.validation.spec.ts` around
lines 78 - 84, The controller currently falls back to the wrong default
systemRole value; update the default assignment used at the systemRole fallback
in system-admin.controller.ts (the code around the systemRole/default handling
at the current line ~151) from 'user' to 'platform_user' so it matches
CreateUserSchema; then update all tests/mocks that expect 'user' to instead use
'platform_user' (system-admin.controller.spec.ts occurrences,
system-admin.guard.spec.ts mock, and better-auth.provider.spec.ts mocks at the
lines called out) so implementation and test fixtures are consistent with the
validation schema.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/api/src/modules/auth/system-admin.guard.spec.ts (1)
75-92: Remove or repurpose the duplicate Forbidden test.This repeats the previous platform_user scenario; consider deleting it or changing it to cover a distinct non-admin variant.
🧹 Suggested cleanup
- it('should throw ForbiddenException if user is platform_user (insufficient privileges)', async () => { - mockIdentityProvider.getSessionFromHeaders.mockResolvedValue({ - session: { token: 'valid' }, - user: { id: 'u2', systemRole: 'platform_user' }, - }); - - const mockContext = { - switchToHttp: () => ({ - getRequest: () => ({ - headers: {}, - }), - }), - } as Partial<ExecutionContext>; - - await expect( - guard.canActivate(mockContext as ExecutionContext), - ).rejects.toThrow(ForbiddenException); - });apps/api/src/modules/auth/auth.controller.ts (1)
111-138: Useinvitation.rolewhen creating new user, but default to'user'not'platform_user'.The hardcoded
role: 'user'ignores the invitation's role and applies it only later during the accept flow, creating unnecessary inconsistency. While'user'is the correct tenant-level default, the invitation role should take precedence if provided. However,'platform_user'is not a valid fallback—it's a system-level role (systemRole), not a tenant role.✅ Correct fix using invitation.role
- role: 'user', + role: invitation.role || 'user',
🤖 Fix all issues with AI agents
In `@apps/api/src/modules/auth/providers/better-auth/better-auth.provider.ts`:
- Around line 549-563: The getInvitation method currently filters out only
'accepted' invitations but should return only pending, non-expired invites:
update the db.query.invitation.findFirst where clause in getInvitation to
replace the ne(inv.status, 'accepted') predicate with eq(inv.status, 'pending')
so it strictly enforces pending status (keep the gt(inv.expiresAt, new Date())
check intact), ensuring behavior matches acceptInvitation and other status
checks.
In `@apps/api/src/scripts/force-reset-admin.ts`:
- Around line 127-131: The script currently logs an error when the created admin
user is not found but then exits with a zero status; update the failure branch
in apps/api/src/scripts/force-reset-admin.ts (the else branch that prints '❌
CRITICAL: User not found in DB after creation!') to terminate with a non‑zero
exit code (e.g., call process.exit(1) or set process.exitCode = 1 and return) so
CI and callers detect the failure; ensure any necessary cleanup or open handles
are handled before exiting if present in the surrounding function.
- Around line 124-126: Replace the partial-password display in
force-reset-admin.ts (the console.log that references PASSWORD and uses
substring) with the same full-masking behavior used in bootstrap-admin.ts;
locate the console.log that prints `Password: ${PASSWORD.substring...}` and
change it to print a fixed masked string (e.g., "Password: ********") or call
the shared masking utility if one exists so the output is consistent and does
not reveal any characters.
- Around line 85-86: The admin's firstName/lastName fields in
force-reset-admin.ts are hardcoded; change them to read from environment
variables (e.g., process.env.ADMIN_FIRST_NAME and process.env.ADMIN_LAST_NAME)
with sensible defaults so the script is reusable. Update the object where
firstName and lastName are set (in the create/update admin logic in
force-reset-admin.ts) to use those env vars and ensure validation/trim is
applied so empty values fall back to current literals.
- Line 34: The console.log in force-reset-admin.ts prints dbUrl which may
contain credentials; update the logging in the script (remove or redact the
sensitive dbUrl) by changing the console.log that references dbUrl to either
omit the URL entirely or replace it with a masked value (e.g., show only the
host or a fixed label) so the variable dbUrl is not logged; locate the
console.log(` Target DB: ${dbUrl}`) instance and modify it accordingly.
In `@apps/web/src/lib/auth/AuthProvider.tsx`:
- Around line 72-85: The retry of the enriched fetch currently calls
retryRes.json() unguarded and may hydrate invalid data; update the block that
does the POST to `${API_URL}/auth/refresh-session` so you first check
retryRes.ok before parsing JSON and calling hydrateUser — if retryRes.ok is
false (or parsing fails) call hydrateUser(enrichedData) or
hydrateUser(data.session) as a fallback; touch the function/variables handling
the retry (the fetch to /auth/refresh-session, the retryRes variable, and the
hydrateUser calls) to implement the ok-guard and fallback.
♻️ Duplicate comments (1)
apps/api/src/modules/auth/providers/better-auth/better-auth.provider.ts (1)
565-646: Transactional accept + email identity verification looks good.Nice hardening of invitation acceptance and privilege elevation flows.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@apps/api/src/modules/auth/providers/better-auth/better-auth.provider.ts`:
- Around line 681-691: The deleteUser transaction should first fetch the user's
email (e.g., query schema.user by userId to get the email) and then, before
deleting the user row, delete or mark as cancelled any pending invitations that
target that email (e.g., delete from schema.invitation where
eq(schema.invitation.email, userEmail) and eq(schema.invitation.status,
'pending') or the equivalent pending flag) all inside the same transaction in
deleteUser so no stale invitations remain if the user is removed; keep existing
deletes for session/account/member and ensure you log the email-aware deletion
in the existing logger.warn message.
- Around line 194-207: The insert in the BetterAuth sync block (where
existingDbUser is checked) can fail under concurrent/replicated conditions;
modify the this.db.insert(schema.user).values({...}) call to use
onConflictDoNothing() (or equivalent conflict handler) so the insert becomes
idempotent and won’t throw on unique constraint violations (apply to the same
insertion path that sets id, email, name, emailVerified, createdAt, updatedAt,
systemRole).
In `@apps/api/src/scripts/force-reset-admin.ts`:
- Around line 13-43: The script currently validates required env vars using the
requiredEnv array but checks DATABASE_URL separately via dbUrl; add
'DATABASE_URL' to the requiredEnv array so missingEnv covers it, remove the
standalone if (!dbUrl) block (or keep only a typed const dbUrl =
process.env.DATABASE_URL as string after validation), and ensure you still
assign const dbUrl = process.env.DATABASE_URL as string after validation; update
references to requiredEnv, missingEnv, and dbUrl accordingly.
- Around line 68-84: The cleanup deletes invitations where inviterId equals the
userId but misses invitations where the user is the recipient; update the
deletion logic in the block using existing[0] to also remove invitations
matching schema.invitation.recipientEmail === existing[0].email (in addition to
the existing inviterId delete) before deleting the user, i.e., fetch userEmail
from existing[0].email and call
db.delete(schema.invitation).where(eq(schema.invitation.recipientEmail,
userEmail)) (or combine conditions) so both inviter and recipient invitation
rows are removed.
In `@apps/web/src/lib/auth/AuthProvider.tsx`:
- Around line 85-92: The retry branch currently calls await retryRes.json()
without guarding against invalid/empty JSON which can throw and prevent
hydration; wrap the parsing in a try/catch inside the retryRes.ok block (around
the call to retryRes.json()), log/parrot the parsing error, and on any parse
failure call hydrateUser(enrichedData) as a fallback (keeping the existing
console.warn for non-ok responses). Ensure you update the block that references
retryRes, hydrateUser, and enrichedData so a thrown JSON parse error doesn't
skip user hydration.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/api/src/modules/auth/providers/better-auth/better-auth.provider.ts`:
- Around line 617-633: The code updates user.systemRole directly from
invitation.role in the system-level invite branch; validate and sanitize
invitation.role before the tx.update call (in the block handling
invitation.role) by enforcing an allow-list of valid platform roles (e.g.,
platform_admin, super_admin, etc.) or mapping tenant roles (like "user") to an
explicit platform default, and fallback to a safe default or throw if invalid;
locate the logic around the tx.update(schema.user).set({ systemRole:
invitation.role }) and replace the direct assignment with the validated/mapped
value.
In `@apps/web/src/lib/auth/AuthProvider.tsx`:
- Line 144: Validate apiUser.systemRole against the allowed set before asserting
its type: replace the direct assertion in AuthProvider (the assignment using
systemRole: typeof apiUser.systemRole === 'string' ? (apiUser.systemRole as
'platform_admin' | 'platform_user') : undefined) with a guard that checks
membership in ['platform_admin','platform_user'] and only then assigns the typed
value; apply the same guarded check in the login callback where a similar
assertion occurs (locate the login callback in the same AuthProvider.tsx) so
unexpected strings yield undefined instead of a false type assertion.
♻️ Duplicate comments (3)
apps/api/src/scripts/force-reset-admin.ts (1)
144-152:process.exit(1)bypasses thefinallyblock, leaking the DB connection.In Node.js,
process.exit()terminates immediately without executingfinallyblocks. Theclient.end()at line 151 will not be called when the user is not found after creation.Replace
process.exit(1)withthrowto let the error propagate through the finally block for proper cleanup.Proposed fix
} else { console.error( '❌ CRITICAL: User not found in DB after creation! Something is wrong.', ); - process.exit(1); // Non-zero exit code + throw new Error('User not found in DB after creation'); }apps/web/src/lib/auth/AuthProvider.tsx (1)
48-103: Well-structured provisioning guards and error handling.The platform user check correctly prevents auto-provisioning for
platform_adminandplatform_userroles, and the JSON parsing is now properly guarded with try/catch as previously requested.apps/api/src/modules/auth/providers/better-auth/better-auth.provider.ts (1)
150-206: Make the DB user sync insert idempotent to avoid unique‑constraint races.
Concurrent signups or replication lag can still trip a unique constraint. ConsideronConflictDoNothing()to make the sync safe. Please confirm the drizzle‑orm version supports this API.🔧 Proposed fix
- await this.db.insert(schema.user).values({ + await this.db + .insert(schema.user) + .values({ id: result.user.id, email: result.user.email, name: result.user.name || '', emailVerified: result.user.emailVerified, createdAt: new Date(result.user.createdAt), updatedAt: new Date(result.user.updatedAt), systemRole: 'platform_user', // Default - }); + }) + .onConflictDoNothing();
pramodnarayana
marked this pull request as draft
January 21, 2026 16:58
pramodnarayana
marked this pull request as ready for review
January 21, 2026 16:59
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @.agent/reviewer.md:
- Around line 21-24: Update the "Tool Compliance" checklist entries (the lines
containing "Ask: \"Has Qodo generated tests for this?\"" and "Ask: \"Are there
active SonarLint warnings?\"") to be tool‑agnostic: replace explicit vendor
names with conditional wording like "If applicable/available, has
project‑approved tooling (e.g., test generator or static analysis) been run?"
and change the blocking guidance to "block only if tooling is applicable and
indicates blocking issues" so the "Tool Compliance" and "⚡ Performance" checks
reference project‑approved tooling rather than hardcoding Qodo or SonarLint.
In
`@apps/api/src/modules/auth/providers/better-auth/better-auth.provider.spec.ts`:
- Around line 4-6: The test file currently uses broad file-level disables for
`@typescript-eslint/no-unsafe-return` and `@typescript-eslint/unbound-method`;
remove those two top-level /* eslint-disable ... */ comments and instead add
targeted inline disables immediately above the specific offending statements in
better-auth.provider.spec.ts (for example add // eslint-disable-next-line
`@typescript-eslint/no-unsafe-return` before assertions or returns that trigger
that rule, and // eslint-disable-next-line `@typescript-eslint/unbound-method`
before any stubs/spies that reference unbound methods), so only the specific
lines are suppressed rather than the whole file.
In `@apps/api/src/modules/auth/providers/better-auth/better-auth.provider.ts`:
- Around line 393-397: getSessionFromHeaders and createInvitation are
incorrectly passing plain objects (casting to Record<string, string>) to
BetterAuth, which expects a Web Headers API object; update both functions to
convert incoming headers to a Web Headers instance before calling
this.auth.api.getSession and this.auth.api.createInvitation — e.g., in
Node/Express use BetterAuth's fromNodeHeaders(req.headers) and in Next.js use
await headers() (or the library's helper) to produce the proper Headers object
and pass that instead of the raw/cast record.
In `@apps/api/src/scripts/force-reset-admin.ts`:
- Around line 32-39: The ADMIN_ROLE env var is declared but then ignored because
the elevation step hardcodes systemRole: 'platform_admin'; either remove
ADMIN_ROLE from requiredEnv and consistently hardcode 'platform_admin' when
calling the signup API (where EMAIL/PASSWORD are passed) and in the elevation
step, or keep the env var and replace the hardcoded value with the ROLE variable
so the signup call (the API signup call around where EMAIL/PASSWORD are passed)
and the elevation step (where systemRole: 'platform_admin' is set) both use
ROLE; also update the requiredEnv list to match whichever approach you choose.
Comment on lines
+21
to
+24
| * **✅ Tool Compliance:** | ||
| * Ask: *"Has Qodo generated tests for this?"* (If not, block it). | ||
| * Ask: *"Are there active SonarLint warnings?"* (If yes, block it). | ||
| * **⚡ Performance:** Flag O(n^2) loops or heavy computations on the main thread. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Make tooling checks tool‑agnostic to avoid hard process lock‑ins.
Mandating a block if Qodo tests aren’t generated or if SonarLint warnings exist will create false blockers in environments where those tools aren’t required or used. Consider rephrasing to “if applicable/available” or referencing “project‑approved tooling” instead of specific vendors.
♻️ Suggested wording
-* **✅ Tool Compliance:**
- * Ask: *"Has Qodo generated tests for this?"* (If not, block it).
- * Ask: *"Are there active SonarLint warnings?"* (If yes, block it).
+* **✅ Tool Compliance:**
+ * Ask: *"Have required tests (per project policy) been generated or updated?"* (If not, block it).
+ * Ask: *"Are there active lint/static‑analysis warnings from the project‑approved tooling?"* (If yes, block it).📝 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
| * **✅ Tool Compliance:** | |
| * Ask: *"Has Qodo generated tests for this?"* (If not, block it). | |
| * Ask: *"Are there active SonarLint warnings?"* (If yes, block it). | |
| * **⚡ Performance:** Flag O(n^2) loops or heavy computations on the main thread. | |
| * **✅ Tool Compliance:** | |
| * Ask: *"Have required tests (per project policy) been generated or updated?"* (If not, block it). | |
| * Ask: *"Are there active lint/static‑analysis warnings from the project‑approved tooling?"* (If yes, block it). | |
| * **⚡ Performance:** Flag O(n^2) loops or heavy computations on the main thread. |
🤖 Prompt for AI Agents
In @.agent/reviewer.md around lines 21 - 24, Update the "Tool Compliance"
checklist entries (the lines containing "Ask: \"Has Qodo generated tests for
this?\"" and "Ask: \"Are there active SonarLint warnings?\"") to be
tool‑agnostic: replace explicit vendor names with conditional wording like "If
applicable/available, has project‑approved tooling (e.g., test generator or
static analysis) been run?" and change the blocking guidance to "block only if
tooling is applicable and indicates blocking issues" so the "Tool Compliance"
and "⚡ Performance" checks reference project‑approved tooling rather than
hardcoding Qodo or SonarLint.
Comment on lines
+4
to
+6
| /* eslint-disable @typescript-eslint/no-unsafe-return */ | ||
|
|
||
| /* eslint-disable @typescript-eslint/unbound-method */ |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider narrowing file-level ESLint suppressions.
If possible, prefer targeted inline disables to avoid masking future test issues.
🤖 Prompt for AI Agents
In `@apps/api/src/modules/auth/providers/better-auth/better-auth.provider.spec.ts`
around lines 4 - 6, The test file currently uses broad file-level disables for
`@typescript-eslint/no-unsafe-return` and `@typescript-eslint/unbound-method`;
remove those two top-level /* eslint-disable ... */ comments and instead add
targeted inline disables immediately above the specific offending statements in
better-auth.provider.spec.ts (for example add // eslint-disable-next-line
`@typescript-eslint/no-unsafe-return` before assertions or returns that trigger
that rule, and // eslint-disable-next-line `@typescript-eslint/unbound-method`
before any stubs/spies that reference unbound methods), so only the specific
lines are suppressed rather than the whole file.
left a comment
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/modules/auth/providers/better-auth/better-auth.provider.ts (1)
393-410: UsefromNodeHeaders()helper for proper header conversion with BetterAuth.The code manually converts
Headersto a plain object viaObject.fromEntries(), but BetterAuth's documentation explicitly recommends using thefromNodeHeaders()helper from"better-auth/node"to convert Node.js headers into the formatauth.api.getSession()expects. Replace the manual conversion with:import { fromNodeHeaders } from "better-auth/node";Then use
fromNodeHeaders(headerObj)instead of the manual conversion.
♻️ Duplicate comments (1)
apps/api/src/modules/auth/providers/better-auth/better-auth.provider.spec.ts (1)
1-11: ESLint suppressions documented with rationale.The descriptive comment block (lines 1-6) explains why the file-level suppressions are necessary. While targeted inline disables would be ideal per the past review comment, the extensive mocking patterns in this test file make file-level suppression a pragmatic choice.
left a comment
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/modules/auth/providers/better-auth/better-auth.provider.ts (1)
488-502: Same unsafe header conversion pattern.Line 491 has the same static analysis warning as line 399. Apply the same fix pattern using explicit
IncomingHttpHeaderstyping to satisfy thefromNodeHeadersparameter type.🔧 Suggested fix
+import { IncomingHttpHeaders } from 'http'; + // In createInvitation method: const headerObj = payload.headers instanceof Headers - ? fromNodeHeaders(Object.fromEntries(payload.headers.entries()) as any) - : payload.headers; + ? fromNodeHeaders( + Object.fromEntries(payload.headers.entries()) as IncomingHttpHeaders, + ) + : payload.headers + ? fromNodeHeaders(payload.headers as IncomingHttpHeaders) + : undefined;
🤖 Fix all issues with AI agents
In `@apps/api/src/modules/auth/providers/better-auth/better-auth.provider.ts`:
- Around line 394-403: In getSessionFromHeaders, ensure both branches convert
headers using fromNodeHeaders: when headers is a Fetch Headers convert to a
plain object then pass it through fromNodeHeaders, and when headers is already a
Record use fromNodeHeaders directly instead of bypassing it; also replace the
"as any" cast with the correct IncomingHttpHeaders type (use IncomingHttpHeaders
from http) so the auth.api.getSession call receives headers typed as
Record<string,string> after conversion by fromNodeHeaders.
left a comment
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/api/src/modules/auth/providers/better-auth/better-auth.provider.ts (3)
491-509: Inconsistent header handling withgetSessionFromHeaders.In
getSessionFromHeaders(lines 398-403), both branches usefromNodeHeadersfor proper conversion. However, here increateInvitation, plainRecord<string, any>objects bypassfromNodeHeadersand are passed directly. For consistency and to ensure BetterAuth receives headers in the expected format, consider applyingfromNodeHeadersto both branches.♻️ Suggested fix for consistent header handling
// Use Better Auth's helper for Headers objects, pass plain objects directly const headerObj = payload.headers instanceof Headers ? fromNodeHeaders( Object.fromEntries( payload.headers.entries(), ) as IncomingHttpHeaders, ) - : payload.headers; + : payload.headers + ? fromNodeHeaders(payload.headers as IncomingHttpHeaders) + : undefined;
781-781: Consider usingas unknown asfor type cast consistency.This line uses
as any as Userwhile other similar casts in the file (lines 223, 340, 584) useas unknown as X. Usingunknowninstead ofanyis generally safer as it requires explicit type handling.♻️ Suggested fix
- return updated as any as User; + return updated as unknown as User;
789-789: Same type cast inconsistency.Same recommendation as above - consider using
as unknown as Userfor consistency with the rest of the file.
This was referenced Jan 22, 2026
This was referenced Jan 30, 2026
This was referenced Feb 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.