Repository navigation
Feature/admin role visibility - #55
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds requester-aware role visibility, membership-derived permission resolution, DB permission inspection methods and CLI commands, expands adapter logic to eagerly load members/roles/permissions with fallbacks, and adds/updates tests and identity exports for role normalization and visibility. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant BetterAuthAdapter
participant Database
participant RoleUtils
Client->>BetterAuthAdapter: findById / login / validateSession
activate BetterAuthAdapter
BetterAuthAdapter->>Database: Query user with USER_WITH_MEMBERS (eager members→roles→permissions)
activate Database
Database-->>BetterAuthAdapter: user + members + role + permissions
deactivate Database
BetterAuthAdapter->>RoleUtils: normalizeRole(member.role) / aggregate permissions / (filterRolesForRequester when listing)
activate RoleUtils
RoleUtils-->>BetterAuthAdapter: Normalized role + filtered roles/permissions
deactivate RoleUtils
BetterAuthAdapter-->>Client: Return mapped user (memberRole, permissions, hasTenant)
deactivate BetterAuthAdapter
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (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: 20
🤖 Fix all issues with AI agents
In @.claude/agent-memory/code-reviewer/MEMORY.md:
- Around line 1-28: The Markdown file violates MD022 by not having blank lines
around headings; update .claude/agent-memory/code-reviewer/MEMORY.md to ensure
each level-2 heading (e.g., "## Project Architecture", "## Key Patterns", "##
Code Quality Notes", "## Test Patterns") has a blank line above and below it so
there is a single empty line separating the heading from preceding and following
content; adjust the surrounding lines accordingly (insert or remove newlines) to
satisfy the blank-line-before-and-after rule without changing the heading text.
In `@apps/api/src/db/database-manager.spec.ts`:
- Around line 324-399: Add unit tests for manager.debugPermissions(): write one
test where drizzleMocks.query.role.findFirst.mockResolvedValue(null) and assert
console.error was called with a message containing the missing role id, and
another where drizzleMocks.query.role.findFirst returns a role object and
drizzleMocks.query.rolePermission.findMany returns permission rows; spy on
console.log and assert it logs the role info, lists returned permission IDs and
marks critical permissions (check for strings containing "system_users:create"
and "users:create" and their enabled/checked indicator). Use the existing mocks
(role.findFirst and rolePermission.findMany), vi.spyOn(console, ...) and call
manager.debugPermissions(roleId) to exercise the behavior.
In `@apps/api/src/db/database-manager.ts`:
- Around line 352-353: The console.log in checkUserPermissions prints the raw
identifier (which may be an email) and can expose PII; remove the direct logging
or replace it with a non-sensitive alternative such as logging a
hashed/partially masked identifier or a generic message via your safe logger
(e.g., use identifier obfuscation or logger.info("Checking permissions for user:
[REDACTED]") instead of console.log(`...${identifier}`)) so the function
checkUserPermissions no longer outputs raw PII.
- Around line 386-389: These console.log calls print sensitive user.email and
legacy user.role in plaintext; restrict them to CLI-only by gating the logs
behind a runtime check (e.g., an isCli/isLocal flag or NODE_ENV check) or move
the printing into a CLI-only helper so they are never reachable from API code.
Locate the console.log lines referencing user.email and user.role in
database-manager.ts and wrap them with the CLI-only guard (or replace them with
a sanitized/debug logger that only emits when running the CLI) to ensure no API
path can call these prints.
- Around line 394-427: The duplicated string-vs-object narrowing for member.role
should be extracted into a shared helper and reused from both this debug block
and BetterAuthAdapter.mapUser(); implement a function (e.g., parseRole or
normalizeRole) that accepts the raw role value and returns a typed object { id:
string, name: string, permissions?: { permissionId: string }[] } (and/or derived
roleName/roleId), update the debug loop here to call that helper and add any
permissionIds to allPermissions from the normalized.permissions, and update
BetterAuthAdapter.mapUser() to call the same helper so the narrowing logic is
centralized and consistent.
- Around line 336-343: The critical permission lists are inconsistent between
debugPermissions and checkUserPermissions; extract the shared array into a
single exported constant (e.g., CRITICAL_PERMISSIONS) and replace the inline
arrays in both debugPermissions and checkUserPermissions with that constant so
both functions reference the same source of truth (ensuring the list contains
'system_users:create', 'users:create', and 'dashboard:view' as required).
In `@apps/api/src/modules/identity/auth/auth.controller.coverage.spec.ts`:
- Around line 36-38: The current mockResponse only stubs setHeader which can
cause confusing "not a function" errors if auth.controller.completeInvite or
other controller methods call additional Response methods; extend the
mockResponse used in
apps/api/src/modules/identity/auth/auth.controller.coverage.spec.ts to include
common Express Response methods (e.g., status, json, send, end, cookie,
clearCookie, setHeader) as vi.fn() stubs or replace it with a reusable test
helper that returns a full Response-like mock, then update assertions to verify
the expected response method calls from completeInvite (or other handlers).
- Around line 98-103: The test's mockInviteData uses a very short expiration
window (expiresAt: new Date(Date.now() + 10000).toISOString()), which can cause
flakiness; update mockInviteData.expiresAt to a much larger offset (e.g., new
Date(Date.now() + 3600_000).toISOString() or add one hour) so the completeInvite
tests (that depend on mockInviteData) reliably treat the invite as valid under
load.
- Around line 143-156: The test for completeInvite is missing assertions for the
unverified-user flow: ensure mocks exist for authService.setPassword and
userProvider.forceVerifyEmail (in addition to authService.login which is already
mocked) and add expect assertions that authService.setPassword,
userProvider.forceVerifyEmail and authService.login were called when
controller.completeInvite is invoked; reference the controller.completeInvite
test, authService.setPassword, authService.login, userProvider.forceVerifyEmail,
userProvider.update and invitationsService.accept to locate where to add the
mocks and expectations.
In `@apps/api/src/modules/identity/roles/roles.controller.spec.ts`:
- Around line 87-103: The test uses lowercase role names while another test uses
capitalized names; update the mock roles in roles.controller.spec.ts so the role
name for the owner matches the canonical casing (e.g., change { id: 'owner',
name: 'owner' } to { id: 'owner', name: 'Owner' }) to match
roles.controller.visibility.spec.ts and real data; ensure roleProvider.findAll
and the expected result reflect the same capitalization so
filterRolesForRequester and controller.findAll assertions remain consistent.
In `@apps/api/src/modules/identity/roles/roles.controller.ts`:
- Around line 37-48: The GET /roles/:id handler (findById) currently returns
role details without applying the visibility filter used in findAll; update the
findById method to run the returned role through filterRolesForRequester (or an
equivalent single-role check) using the RequestAuthContext (ctx.user?.role) and
return 404 or deny access if the role is filtered out, and apply the same check
to update and delete handlers (or add a clear code comment in
findById/update/delete explaining that PermissionsGuard intentionally enforces
Owner visibility if that is the design decision) so visibility for the Owner
role cannot be bypassed.
- Around line 45-47: The controller is using the deprecated global user.role for
authorization; update the return to pass the organization-scoped role by using
ctx.user?.memberRole (fall back to '' when undefined) into
filterRolesForRequester instead of ctx.user?.role, i.e. replace references to
ctx.user?.role with ctx.user?.memberRole ?? '' so authorization uses the correct
member role for filterRolesForRequester.
In `@apps/api/src/modules/identity/roles/roles.controller.visibility.spec.ts`:
- Around line 53-85: The tests in roles.controller.visibility.spec.ts duplicate
visibility assertions already present in roles.controller.spec.ts; keep only the
unique member-requester case here and remove the Owner/Admin duplicate tests
(the it blocks titled 'should return ALL roles (including Owner) for an Owner
requester' and 'should filter out Owner role for an Admin requester'), or
alternatively remove the owner/admin cases from the other spec—ensure
controller.findAll visibility behavior is covered in exactly one spec file;
update the remaining test file to assert the Owner is filtered out for a member
requester and retain references to RequestAuthContext and result.data
expectations.
- Around line 53-62: The test for "should return ALL roles (including Owner) for
an Owner requester" creates a fragile mock RequestAuthContext with only
user.role; update the mock used in this spec so it includes minimal stubs for
headers and session (matching the pattern used in roles.controller.spec.ts) to
avoid runtime errors if controller.findAll or middleware accesses ctx.headers or
ctx.session; specifically expand mockContext to include headers: {} and session:
{ id: 'test' } (or similar minimal values) while still typing it as
RequestAuthContext and keeping user.role: 'owner'.
In `@lint-output.txt`:
- Around line 54-125: The lint run fails across the API with many unsafe `any`
usages and one switch-case lexical declaration; fix by (1) replacing unsafe
`any` in the offending modules with proper TypeScript types or narrow with type
guards instead of `any` (e.g., annotate returned values and variables in
database-manager.ts and the tests in auth.controller.coverage.spec.ts and
roles.controller.visibility.spec.ts, or cast mocks as jest.Mocked<Type> /
unknown as Type where appropriate), (2) avoid unsafe member access/calls by
typing the objects (e.g., ensure objects used with .name, .id, .permissions,
.get, .findByEmail, .update, .login, .delete, etc. have explicit interfaces or
runtime guards), and (3) fix the no-case-declarations in db-cli.ts by wrapping
lexical declarations inside a block (case { const x = …; break; }) or moving
declarations above the switch. Run the linter locally and iterate until all
`@typescript-eslint` no-unsafe-* and no-case-declarations errors are resolved.
In `@packages/identity/src/adapters/better-auth.adapter.spec.ts`:
- Around line 685-727: Add two unit tests in better-auth.adapter.spec.ts
covering the two defensive branches: (1) a test where the rehydration mock for
db.query.user.findFirst returns a user with members: undefined and you then mock
the adapter's lazy member/role fetch (the DB call the adapter makes when members
are missing) so createUser still resolves with correct role/permissions and does
not call db.update; (2) a test where the rehydration returns members whose role
is a string id (e.g., role: "admin") and you mock the supplementary permission
lookup query the adapter issues (the DB query used by mapUser to expand string
role IDs) to return the expected permissions; assert createUser returns the
combined permissions, role is set correctly, and db.update is not called. Ensure
you reference BetterAuthAdapter.createUser and mapUser behaviors and reuse the
existing db mock hooks (db.query.user.findFirst and the appropriate db.query.*
mocks) to simulate those flows.
In `@packages/identity/src/adapters/better-auth.adapter.ts`:
- Around line 786-804: The current guard (if (!permissionsPreloaded &&
hasMembership)) can skip the supplementary DB lookup when some memberships have
eager role objects and others have legacy string role IDs; compute the string
role IDs from dbUser.members first (using the existing roleIds mapping/filter),
then run the supplementary rolePermission.findMany whenever roleIds.length > 0
regardless of permissionsPreloaded (so legacy string-role permissions aren’t
dropped); keep the uniqueness step (uniqueRoleIds) and the loop that adds
rp.permissionId to permissions, but move the roleIds extraction above the
permissionsPreloaded check and only bail out early when there are no string role
IDs to look up.
- Around line 806-809: Replace the hardcoded string "dashboard:read" with a
named constant (e.g. PERMISSION_FALLBACK_DASHBOARD_READ) declared near the top
of the module in better-auth.adapter.ts and use that constant when adding to the
permissions Set; update any unit tests or references to use the constant so the
fallback permission is centrally defined and easy to change if RBAC names
change. Ensure the constant name is exported if used elsewhere and referenced in
the same code block that checks permissions.size === 0 and calls
permissions.add(...).
- Around line 738-753: The loop over dbUser.members (in better-auth.adapter.ts)
currently overwrites role on each membership while still accumulating
permissions; fix by enforcing a single-membership invariant or explicitly
resolving which membership's role to return: either (A) if single-tenant, assert
or throw when dbUser.members.length > 1 (or filter the incoming query to return
only the primary/org-scoped membership) so hasMembership, role and
permissionsPreloaded reflect one membership, or (B) if multi-tenant is allowed,
compute and select a deterministic role (e.g., highest-privilege by comparing
role names/permission counts) and set role once while continuing to union
permissions into permissionsPreloaded and permissions set; update the code
around the dbUser.members loop, hasMembership, role, and permissionsPreloaded to
apply one of these strategies and document the chosen behavior.
In `@packages/identity/src/utils/role-visibility.ts`:
- Around line 14-37: The two identical branches in filterRolesForRequester (the
falsy requesterRole guard and the !isOwner branch) should be collapsed: compute
isOwner by first guarding requesterRole and comparing
requesterRole.toLowerCase() to Role.Owner.toLowerCase(), then if !isOwner return
roles.filter(r => r.name.toLowerCase() !== Role.Owner.toLowerCase()), otherwise
return roles; this deduplicates the filter logic while preserving the
case-insensitive comparison and safe handling of undefined requesterRole.
Comment on lines
+1
to
+28
| # Code Reviewer Memory - Nexiom | ||
|
|
||
| ## Project Architecture | ||
| - Monorepo: `apps/api` (NestJS), `apps/web` (React/Vite), `packages/identity` (shared identity package) | ||
| - ORM: Drizzle ORM with PostgreSQL | ||
| - Auth: better-auth library with custom adapters | ||
| - RBAC: Database-driven, role -> rolePermission -> permission tables | ||
| - Member table holds organization-scoped roles (FK to role table); `user.role` is legacy/deprecated | ||
|
|
||
| ## Key Patterns | ||
| - Adapters: `BetterAuthAdapter` (IAuthProvider), `DrizzleUserAdapter` (IUserProvider), `DrizzleTenantAdapter`, `DrizzleRoleAdapter` | ||
| - `mapUser()` in BetterAuthAdapter resolves permissions from member records (async) | ||
| - `mapUser()` in DrizzleUserAdapter is synchronous and does NOT resolve permissions (just maps `user.role` directly) | ||
| - NestJS DI tokens in `packages/identity/src/constants.ts` | ||
| - Role enum: `Role.Owner`, `Role.Admin`, `Role.Member` (lowercase values) | ||
| - Schema types exported from `packages/identity/src/schema.ts` | ||
|
|
||
| ## Code Quality Notes | ||
| - Debug `console.log` statements have appeared in production code in adapters -- flag these | ||
| - `as any` casts used frequently to work around Drizzle's deep relation type inference | ||
| - `DrizzleUserAdapter.findById` delegates to `AuthProvider.findById` for permission resolution | ||
| - `findById` in BetterAuthAdapter does NOT eager-load members, causing lazy-fetch fallback every time | ||
| - `AuthService.getEnrichedSession` independently resolves permissions via PermissionProvider -- this duplicates/conflicts with mapUser permission resolution | ||
|
|
||
| ## Test Patterns | ||
| - Vitest used for all packages | ||
| - Mocks use `vi.fn()` and `vi.mock()` | ||
| - Test DB mock is `mkDb()` factory returning chainable query mock |
Contributor
There was a problem hiding this comment.
Markdown headings need surrounding blank lines (MD022).
Static analysis flags missing blank lines around headings at Lines 3, 10, 18, and 25. Each ## heading should have a blank line both above and below it.
🔧 Proposed fix
# Code Reviewer Memory - Nexiom
+
## Project Architecture
+
- Monorepo: `apps/api` (NestJS), `apps/web` (React/Vite), `packages/identity` (shared identity package)
...
- Member table holds organization-scoped roles (FK to role table); `user.role` is legacy/deprecated
+
## Key Patterns
+
- Adapters: `BetterAuthAdapter` (IAuthProvider), `DrizzleUserAdapter` (IUserProvider), `DrizzleTenantAdapter`, `DrizzleRoleAdapter`
...
- `AuthService.getEnrichedSession` independently resolves permissions via PermissionProvider -- this duplicates/conflicts with mapUser permission resolution
+
## Code Quality Notes
+
- Debug `console.log` statements have appeared in production code in adapters -- flag these
...
+
## Test Patterns
+
- Vitest used for all packages📝 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
| # Code Reviewer Memory - Nexiom | |
| ## Project Architecture | |
| - Monorepo: `apps/api` (NestJS), `apps/web` (React/Vite), `packages/identity` (shared identity package) | |
| - ORM: Drizzle ORM with PostgreSQL | |
| - Auth: better-auth library with custom adapters | |
| - RBAC: Database-driven, role -> rolePermission -> permission tables | |
| - Member table holds organization-scoped roles (FK to role table); `user.role` is legacy/deprecated | |
| ## Key Patterns | |
| - Adapters: `BetterAuthAdapter` (IAuthProvider), `DrizzleUserAdapter` (IUserProvider), `DrizzleTenantAdapter`, `DrizzleRoleAdapter` | |
| - `mapUser()` in BetterAuthAdapter resolves permissions from member records (async) | |
| - `mapUser()` in DrizzleUserAdapter is synchronous and does NOT resolve permissions (just maps `user.role` directly) | |
| - NestJS DI tokens in `packages/identity/src/constants.ts` | |
| - Role enum: `Role.Owner`, `Role.Admin`, `Role.Member` (lowercase values) | |
| - Schema types exported from `packages/identity/src/schema.ts` | |
| ## Code Quality Notes | |
| - Debug `console.log` statements have appeared in production code in adapters -- flag these | |
| - `as any` casts used frequently to work around Drizzle's deep relation type inference | |
| - `DrizzleUserAdapter.findById` delegates to `AuthProvider.findById` for permission resolution | |
| - `findById` in BetterAuthAdapter does NOT eager-load members, causing lazy-fetch fallback every time | |
| - `AuthService.getEnrichedSession` independently resolves permissions via PermissionProvider -- this duplicates/conflicts with mapUser permission resolution | |
| ## Test Patterns | |
| - Vitest used for all packages | |
| - Mocks use `vi.fn()` and `vi.mock()` | |
| - Test DB mock is `mkDb()` factory returning chainable query mock | |
| # Code Reviewer Memory - Nexiom | |
| ## Project Architecture | |
| - Monorepo: `apps/api` (NestJS), `apps/web` (React/Vite), `packages/identity` (shared identity package) | |
| - ORM: Drizzle ORM with PostgreSQL | |
| - Auth: better-auth library with custom adapters | |
| - RBAC: Database-driven, role -> rolePermission -> permission tables | |
| - Member table holds organization-scoped roles (FK to role table); `user.role` is legacy/deprecated | |
| ## Key Patterns | |
| - Adapters: `BetterAuthAdapter` (IAuthProvider), `DrizzleUserAdapter` (IUserProvider), `DrizzleTenantAdapter`, `DrizzleRoleAdapter` | |
| - `mapUser()` in BetterAuthAdapter resolves permissions from member records (async) | |
| - `mapUser()` in DrizzleUserAdapter is synchronous and does NOT resolve permissions (just maps `user.role` directly) | |
| - NestJS DI tokens in `packages/identity/src/constants.ts` | |
| - Role enum: `Role.Owner`, `Role.Admin`, `Role.Member` (lowercase values) | |
| - Schema types exported from `packages/identity/src/schema.ts` | |
| ## Code Quality Notes | |
| - Debug `console.log` statements have appeared in production code in adapters -- flag these | |
| - `as any` casts used frequently to work around Drizzle's deep relation type inference | |
| - `DrizzleUserAdapter.findById` delegates to `AuthProvider.findById` for permission resolution | |
| - `findById` in BetterAuthAdapter does NOT eager-load members, causing lazy-fetch fallback every time | |
| - `AuthService.getEnrichedSession` independently resolves permissions via PermissionProvider -- this duplicates/conflicts with mapUser permission resolution | |
| ## Test Patterns | |
| - Vitest used for all packages | |
| - Mocks use `vi.fn()` and `vi.mock()` | |
| - Test DB mock is `mkDb()` factory returning chainable query mock |
🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
[warning] 3-3: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 10-10: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 18-18: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 25-25: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
In @.claude/agent-memory/code-reviewer/MEMORY.md around lines 1 - 28, The
Markdown file violates MD022 by not having blank lines around headings; update
.claude/agent-memory/code-reviewer/MEMORY.md to ensure each level-2 heading
(e.g., "## Project Architecture", "## Key Patterns", "## Code Quality Notes",
"## Test Patterns") has a blank line above and below it so there is a single
empty line separating the heading from preceding and following content; adjust
the surrounding lines accordingly (insert or remove newlines) to satisfy the
blank-line-before-and-after rule without changing the heading text.
Comment on lines
+352
to
+353
| async checkUserPermissions(identifier: string): Promise<void> { | ||
| console.log(`🔍 Checking permissions for user: ${identifier}...`); |
Contributor
There was a problem hiding this comment.
User identifier (potentially an email) is logged to console.
Line 353 logs the raw identifier, which may be an email address. This is a local CLI debug tool so it's likely acceptable, but be aware if this code path is ever reused in a server context.
🤖 Prompt for AI Agents
In `@apps/api/src/db/database-manager.ts` around lines 352 - 353, The console.log
in checkUserPermissions prints the raw identifier (which may be an email) and
can expose PII; remove the direct logging or replace it with a non-sensitive
alternative such as logging a hashed/partially masked identifier or a generic
message via your safe logger (e.g., use identifier obfuscation or
logger.info("Checking permissions for user: [REDACTED]") instead of
console.log(`...${identifier}`)) so the function checkUserPermissions no longer
outputs raw PII.
Comment on lines
+394
to
+427
| if (user.members && user.members.length > 0) { | ||
| console.log(` Memberships (${user.members.length}):`); | ||
| for (const member of user.members) { | ||
| const memberRole = member.role as unknown; | ||
|
|
||
| let roleName = | ||
| typeof memberRole === 'string' ? memberRole : 'unknown'; | ||
| let roleId = typeof memberRole === 'string' ? memberRole : 'unknown'; | ||
|
|
||
| if ( | ||
| memberRole && | ||
| typeof memberRole === 'object' && | ||
| 'name' in memberRole && | ||
| 'id' in memberRole | ||
| ) { | ||
| const roleObj = memberRole as { | ||
| id: string; | ||
| name: string; | ||
| permissions: { permissionId: string }[]; | ||
| }; | ||
| roleName = roleObj.name; | ||
| roleId = roleObj.id; | ||
|
|
||
| if (Array.isArray(roleObj.permissions)) { | ||
| roleObj.permissions.forEach((p) => | ||
| allPermissions.add(p.permissionId), | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| console.log( | ||
| ` - Org: ${member.organizationId}, Role: ${roleName} (${roleId})`, | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Type-narrowing logic for member.role duplicates the pattern in BetterAuthAdapter.mapUser().
The string-vs-object check for member.role (Lines 397–422) mirrors the logic in BetterAuthAdapter.mapUser(). For a debug tool this is fine, but if this pattern evolves further, consider extracting a shared helper.
🧰 Tools
🪛 Biome (2.3.14)
[error] 419-419: This callback passed to forEach() iterable method should not return a value.
Either remove this return or remove the returned value.
(lint/suspicious/useIterableCallbackReturn)
🤖 Prompt for AI Agents
In `@apps/api/src/db/database-manager.ts` around lines 394 - 427, The duplicated
string-vs-object narrowing for member.role should be extracted into a shared
helper and reused from both this debug block and BetterAuthAdapter.mapUser();
implement a function (e.g., parseRole or normalizeRole) that accepts the raw
role value and returns a typed object { id: string, name: string, permissions?:
{ permissionId: string }[] } (and/or derived roleName/roleId), update the debug
loop here to call that helper and add any permissionIds to allPermissions from
the normalized.permissions, and update BetterAuthAdapter.mapUser() to call the
same helper so the narrowing logic is centralized and consistent.
pramodnarayana
marked this pull request as draft
February 13, 2026 15:34
pramodnarayana
marked this pull request as ready for review
February 13, 2026 15:34
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/identity/src/adapters/better-auth.adapter.ts (1)
777-798: 🧹 Nitpick | 🔵 TrivialSupplementary permission query runs on every
mapUsercall, even when permissions are already fully loaded.When
USER_WITH_MEMBERSsuccessfully eager-loads roles with their permissions (the happy path), the supplementary query at Lines 780–797 still fires an extrarolePermission.findManyagainst the DB. SincemapUseris called on login, session validation,findById, and user creation, this adds a redundant query per auth operation.Consider skipping the supplementary query when permissions were already preloaded from the eager-loaded role object:
Suggested optimization
+ // 2. Supplementary permission query + // Only needed when member role was a string ID (no preloaded permissions). if (hasMembership) { const roleIds = members! .map((m) => { const normalized = normalizeRole(m.role); - return normalized.id !== "unknown" ? normalized.id : undefined; + // Only fetch supplementary permissions for string-based roles + // (object roles already had their permissions loaded inline) + return typeof m.role === "string" && normalized.id !== "unknown" + ? normalized.id + : undefined; }) .filter((id): id is string => !!id);
🤖 Fix all issues with AI agents
In `@apps/api/src/db/database-manager.spec.ts`:
- Around line 373-398: Rename the test in database-manager.spec.ts to reflect
that no fallback query occurs (e.g., change the it(...) description from "should
handle legacy string roles in membership causing fallback query" to "should
handle legacy string roles in membership without performing fallback query and
yield zero permissions"), and remove or condense the implementation-level block
comments inside that test (the multi-line comment about role
extraction/BetterAuthAdapter) to keep the spec focused; keep the existing
assertions and the use of manager.checkUserPermissions and logSpy unchanged.
In `@apps/api/src/db/database-manager.ts`:
- Around line 429-436: Replace the Array.forEach usages with for...of loops to
satisfy the linter: iterate over sortedPerms with "for (const p of sortedPerms)"
instead of sortedPerms.forEach(...) and iterate over
DatabaseManager.CRITICAL_PERMISSIONS (assigned to the local variable critical)
with "for (const c of critical)" instead of critical.forEach(...); preserve the
same console.log behavior (including the checked emoji and use of
allPermissions.has(c)) and keep sortedPerms logging logic unchanged.
- Around line 411-415: The guard "if (normalized.permissions)" is misleading
because normalizeRole() always returns a permissions array; update the code in
database-manager.ts to either remove the conditional and directly iterate
normalized.permissions.forEach(...) (since forEach on an empty array is a no-op)
or change the guard to check normalized.permissions.length > 0 if you explicitly
want to skip when empty; ensure you keep the same call to
allPermissions.add(p.permissionId) inside the loop and reference normalizeRole()
and the normalized.permissions variable when making the change.
- Around line 343-350: The linter flags the use of Array.forEach with
console.log; replace the two forEach usages with for...of loops: iterate over
permIds using "for (const p of permIds)" to log each permission and iterate over
DatabaseManager.CRITICAL_PERMISSIONS (or the local const critical) using "for
(const c of critical)" and compute "const has = permIds.includes(c)" before
logging the check; keep the exact console.log messages and variable names
(permIds, critical, c, p, has) so behavior is unchanged.
In `@apps/api/src/modules/identity/auth/auth.controller.coverage.spec.ts`:
- Around line 90-99: Add a happy-path unit test for resendVerification: call
controller.resendVerification with a valid payload, mock
authService.resendVerificationEmail to resolve (e.g.,
mockResolvedValue(undefined) or a success value), assert the call resolves
without throwing and verify authService.resendVerificationEmail was called with
the expected email; reference the existing test suite helpers and the methods
authService.resendVerificationEmail and controller.resendVerification to locate
where to add this assertion.
- Around line 166-178: Add a happy-path unit test that mirrors the rollback test
but asserts success: mock invitationsService.get to resolve mockInviteData,
userProvider.findByEmail to resolve null, authService.createUser to resolve a
new user ({ id: 'u1' }), invitationsService.accept to resolve successfully, and
authService.login to resolve a token/session; call
controller.completeInvite(mockCompleteInvite, mockResponse) and assert it
returns the expected login result (or calls/resolves via authService.login) and
that userProvider.delete is NOT called; reference controller.completeInvite,
invitationsService.get, userProvider.findByEmail, authService.createUser,
invitationsService.accept, and authService.login when locating code to update.
In `@apps/api/src/modules/identity/roles/roles.controller.spec.ts`:
- Around line 56-60: The test's mock context (mockCtx cast to
RequestAuthContext) omits user.memberRole which causes the controller to use the
empty-string fallback (ctx.user?.memberRole ?? '')—make the intent explicit by
adding memberRole: 'admin' to mockCtx.user so role-filtering in the controller
uses the correct member role; update the mockCtx object used in
roles.controller.spec.ts (the mockCtx variable) to include memberRole: 'admin'
on the user to prevent reliance on the fallback.
In `@apps/api/src/modules/identity/roles/roles.controller.visibility.spec.ts`:
- Around line 53-65: The test is mocking user.role but the controller reads
ctx.user?.memberRole, so update the mock in roles.controller.visibility.spec.ts
to set mockContext.user.memberRole = 'member' (or the appropriate member role
string) instead of/in addition to user.role so the controller's memberRole-based
branch in controller.findAll (which reads ctx.user?.memberRole ?? '') is
actually exercised; ensure the mockContext shape still matches
RequestAuthContext so the visibility filter logic that excludes 'Owner' for
non-owner users runs via the memberRole path.
In `@packages/identity/src/adapters/better-auth.adapter.ts`:
- Around line 718-719: Remove the stray comment artifact "// ... (imports)" that
was accidentally left in packages/identity/src/adapters/better-auth.adapter.ts;
locate the comment text and delete it so the top of the file contains only real
import statements and no placeholder comments, then run the project's
TypeScript/ESLint checks to ensure no formatting or lint issues remain.
Comment on lines
+373
to
+398
| it('should handle legacy string roles in membership causing fallback query', async () => { | ||
| const logSpy = vi.spyOn(console, 'log'); | ||
|
|
||
| // Mock user with legacy string role in member | ||
| const mockUser = { | ||
| id: 'u2', | ||
| email: 'legacy@example.com', | ||
| role: 'admin', | ||
| members: [{ organizationId: 'org2', role: 'legacy-role-id' }], | ||
| // logic in manager handles string roles by printing them but typically expects object for permission extraction unless resolved | ||
| // Actually current implementation ONLY extracts permissions if role is object and has permissions array. | ||
| // It does NOT perform extra query for permissions in checkUserPermissions (unlike BetterAuthAdapter). | ||
| // It just prints the role ID. | ||
| }; | ||
|
|
||
| drizzleMocks.query.user.findFirst.mockResolvedValue(mockUser); | ||
|
|
||
| await manager.checkUserPermissions('u2'); | ||
|
|
||
| expect(logSpy).toHaveBeenCalledWith( | ||
| expect.stringContaining('Role: legacy-role-id (legacy-role-id)'), | ||
| ); | ||
| expect(logSpy).toHaveBeenCalledWith( | ||
| expect.stringContaining('Effective Permissions (0)'), | ||
| ); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Misleading test title: no "fallback query" actually occurs.
The test name says "causing fallback query" but the assertions confirm zero effective permissions — i.e., no fallback query is made. Rename to match actual behavior.
Proposed fix
- it('should handle legacy string roles in membership causing fallback query', async () => {
+ it('should handle legacy string roles in membership with zero effective permissions', async () => {Also, the block comments on lines 382–386 are implementation-level notes that clutter the test. Consider removing or condensing them.
🤖 Prompt for AI Agents
In `@apps/api/src/db/database-manager.spec.ts` around lines 373 - 398, Rename the
test in database-manager.spec.ts to reflect that no fallback query occurs (e.g.,
change the it(...) description from "should handle legacy string roles in
membership causing fallback query" to "should handle legacy string roles in
membership without performing fallback query and yield zero permissions"), and
remove or condense the implementation-level block comments inside that test (the
multi-line comment about role extraction/BetterAuthAdapter) to keep the spec
focused; keep the existing assertions and the use of
manager.checkUserPermissions and logSpy unchanged.
Comment on lines
+166
to
+178
| it('should rollback user creation if accept fails', async () => { | ||
| invitationsService.get.mockResolvedValue(mockInviteData); | ||
| userProvider.findByEmail.mockResolvedValue(null); | ||
| authService.createUser.mockResolvedValue({ id: 'u1' }); | ||
| invitationsService.accept.mockRejectedValue(new Error('Accept failed')); | ||
|
|
||
| await expect( | ||
| controller.completeInvite(mockCompleteInvite, mockResponse), | ||
| ).rejects.toThrow(BadRequestException); | ||
|
|
||
| expect(userProvider.delete).toHaveBeenCalledWith('u1'); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Missing happy-path test for new-user invite completion.
The suite covers the rollback scenario for new users but not the successful path (findByEmail → null → createUser → accept → login). This leaves the main new-user flow untested.
♻️ Suggested test to add
+ it('should create and login a new user when invite is valid', async () => {
+ invitationsService.get.mockResolvedValue(mockInviteData);
+ userProvider.findByEmail.mockResolvedValue(null);
+ authService.createUser.mockResolvedValue({ id: 'u-new' });
+ invitationsService.accept.mockResolvedValue(undefined);
+ authService.login.mockResolvedValue({ session: 's' });
+
+ await controller.completeInvite(mockCompleteInvite, mockResponse);
+
+ expect(authService.createUser).toHaveBeenCalled();
+ expect(invitationsService.accept).toHaveBeenCalled();
+ expect(authService.login).toHaveBeenCalled();
+ });🤖 Prompt for AI Agents
In `@apps/api/src/modules/identity/auth/auth.controller.coverage.spec.ts` around
lines 166 - 178, Add a happy-path unit test that mirrors the rollback test but
asserts success: mock invitationsService.get to resolve mockInviteData,
userProvider.findByEmail to resolve null, authService.createUser to resolve a
new user ({ id: 'u1' }), invitationsService.accept to resolve successfully, and
authService.login to resolve a token/session; call
controller.completeInvite(mockCompleteInvite, mockResponse) and assert it
returns the expected login result (or calls/resolves via authService.login) and
that userProvider.delete is NOT called; reference controller.completeInvite,
invitationsService.get, userProvider.findByEmail, authService.createUser,
invitationsService.accept, and authService.login when locating code to update.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/identity/src/adapters/better-auth.adapter.ts (2)
778-795: 🧹 Nitpick | 🔵 TrivialRedundant DB round-trip when permissions are already eagerly loaded.
When
USER_WITH_MEMBERSsuccessfully eager-loads the role object with permissions (the common path),normalizeRoleat line 762 already extracts allpermissionIdvalues into thepermissionsSet. The supplementary query at lines 786-794 then fetches the same permission IDs byroleId, adding duplicates that the Set silently discards.Since
mapUseris called on every login, session validation, and user lookup, this is an avoidable DB call on the hot path.♻️ Skip supplementary query when permissions were already resolved from the eager-loaded object
if (hasMembership) { + // Only query for permissions if the role was a legacy string (not eagerly loaded as object). + const needsSupplementaryLookup = members!.some((m) => typeof m.role === 'string'); + if (!needsSupplementaryLookup) { + // Permissions were fully resolved from the eager-loaded role object — skip DB call. + } else { const roleIds = members! .map((m) => { const normalized = normalizeRole(m.role); return normalized.id !== "unknown" ? normalized.id : undefined; }) .filter((id): id is string => !!id); if (roleIds.length > 0) { const uniqueRoleIds = [...new Set(roleIds)]; const rolePerms = await this.db.query.rolePermission.findMany({ where: (rp, { inArray }) => inArray(rp.roleId, uniqueRoleIds), columns: { permissionId: true }, }); for (const rp of rolePerms) { permissions.add(rp.permissionId); } } + } }
354-356:⚠️ Potential issue | 🟡 MinorDuplicate comment.
// Using Better Auth APIappears on both lines 354 and 356.Proposed fix
// Using Better Auth API - - // Using Better Auth API const apiResponse = (await this.api.signInEmail({
🤖 Fix all issues with AI agents
In `@apps/api/package.json`:
- Around line 70-71: Remove the unused dependency entry
"@vitest/coverage-istanbul" from apps/api's package.json since coverage is using
the v8 provider; locate the dependency in the dependencies/devDependencies block
and delete the entire line containing "@vitest/coverage-istanbul": "^2.1.9",
then run the package manager's install command (npm/yarn/pnpm) to update
lockfile and ensure no leftover references to `@vitest/coverage-istanbul` remain
in package.json or package-lock.json/pnpm-lock.yaml/yarn.lock.
In `@apps/api/src/db/database-manager.ts`:
- Around line 313-354: Extract the repeated dynamic-import + client setup into a
private helper like withDrizzle that does the await
import('drizzle-orm/node-postgres'), await import('./schema'), calls
this.getPgClient(), creates db via drizzle(client, { schema }), invokes a passed
async callback with (db, schema) and ensures await client.end() in a finally
block; then refactor debugPermissions and checkUserPermissions to call
withDrizzle and run their query logic inside the callback using the provided db
and schema (preserve existing behavior and error handling).
- Line 3: Change the value import of Client to a type-only import: replace the
current import of Client from 'pg' with a TypeScript type import (import type {
Client } from 'pg') so the module isn't pulled in at runtime; keep the existing
dynamic runtime import usage (await import('pg')) and ensure all references to
Client remain in type positions (e.g., function parameter and return type
annotations such as in functions that accept or return Client instances).
In `@apps/api/src/modules/identity/auth/auth.controller.coverage.spec.ts`:
- Around line 36-41: The mockResponse object (mockResponse) is created once at
describe scope so its vi.fn() properties (setHeader, status, json, cookie)
accumulate calls across tests; fix by either moving the mockResponse
construction into a beforeEach block so each test gets fresh vi.fn() instances,
or call vi.clearAllMocks() (or individually call
mockResponse.setHeader.mockClear() etc.) in beforeEach to reset call history;
update test setup around mockResponse usage to ensure tests assert against fresh
mocks.
In `@apps/api/src/modules/identity/roles/roles.controller.spec.ts`:
- Around line 20-22: The test-level mockContext only includes user and is
brittle if controllers access headers or session; update mockContext (used by
tests for findById, update, delete) to mirror the more complete mockCtx by
adding headers and session properties (e.g., headers: {} and session: {}) so
tests don't throw if controller code reads ctx.headers or ctx.session; ensure
the same shape/typing as RequestAuthContext is used to prevent regressions.
In `@packages/identity/src/adapters/better-auth.adapter.ts`:
- Around line 742-749: The lazy-loaded members result is being force-cast with
"as any", masking type mismatches with the expected mapUser parameter; replace
the unsafe cast by making the query return a properly typed result that matches
the same shared type used by the eager path (or reuse the type of the mapUser
parameter). Update the call to this.db.query.member.findMany to use that
shared/interface type (instead of casting), ensure its relation shape matches
role.with.permissions, and remove the "as any" on members so TypeScript will
surface mismatches if schema changes (reference symbols: members, mapUser,
this.db.query.member.findMany, schema.member.userId, dbUser.id).
- Around line 753-759: Before throwing the multi-membership Error in the
BetterAuthAdapter branch (when hasMembership is true and members!.length > 1),
emit a log with the user identifier and the membership count to aid debugging;
locate the check around hasMembership and members and insert a console.error (or
use the module's logger) that includes the user id variable in scope (e.g.,
userId or user.id) and members!.length, then throw the existing Error as before.
| import { execSync } from 'node:child_process'; | ||
| import path from 'node:path'; | ||
| import type { Client } from 'pg'; | ||
| import { Client } from 'pg'; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Client is only used as a type — consider keeping it as a type-only import.
Client from pg is referenced only in type positions (parameter types, return types). The constructors are obtained via await import('pg'). A value import works but import type { Client } would be more precise and avoids pulling in the module at load time.
♻️ Suggested fix
-import { Client } from 'pg';
+import type { Client } from 'pg';📝 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
| import { Client } from 'pg'; | |
| import type { Client } from 'pg'; |
🤖 Prompt for AI Agents
In `@apps/api/src/db/database-manager.ts` at line 3, Change the value import of
Client to a type-only import: replace the current import of Client from 'pg'
with a TypeScript type import (import type { Client } from 'pg') so the module
isn't pulled in at runtime; keep the existing dynamic runtime import usage
(await import('pg')) and ensure all references to Client remain in type
positions (e.g., function parameter and return type annotations such as in
functions that accept or return Client instances).
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/identity/roles/roles.controller.spec.ts (1)
178-195: 🧹 Nitpick | 🔵 TrivialMissing not-found test for
update.
findByIdanddeleteboth have tests for theNotFoundExceptionpath when the role doesn't exist, butupdatelacks this coverage. If the controller checks visibility before updating (viafindById), the null/not-visible case should be tested here too.♻️ Suggested test to add
+ it('should throw NotFoundException if role not found', async () => { + roleProvider.findById.mockResolvedValue(null); + + await expect( + controller.update('unknown', { description: 'x' }, mockContext), + ).rejects.toThrow(NotFoundException); + expect(roleProvider.update).not.toHaveBeenCalled(); + });packages/identity/src/adapters/better-auth.adapter.ts (1)
782-816: 🧹 Nitpick | 🔵 TrivialRedundant
normalizeRolecall and unconditional supplementary DB query on every user mapping.
normalizeRole(m.role)is invoked at line 783 and again at line 802 on the same (single, enforced) member. More importantly, when the role is eagerly loaded as an object, permissions are added at lines 789-793 and then the supplementary query at lines 809-815 fetches the same role's permissions from the DB again. Since both the eager and lazy paths resolve the fullrole → permissionsrelation, the supplementary query is a redundant round trip on everymapUsercall with an object role.Consider caching the normalization result and skipping the supplementary query when permissions were already extracted from the eagerly-loaded object.
♻️ Proposed refactor
const m = members![0]; const normalized = normalizeRole(m.role); if (normalized.name !== "unknown") { role = normalized.name; } + let permissionsPreloaded = false; if (normalized.permissions && normalized.permissions.length > 0) { for (const rp of normalized.permissions) { permissions.add(rp.permissionId); } + permissionsPreloaded = true; } - } - // 2. Supplementary permission query - // We check for string role IDs and fetch their permissions if needed. - // We do this regardless of preloaded permissions to ensure we don't drop legacy role data. - if (hasMembership) { - const roleIds = members! - .map((m) => { - const normalized = normalizeRole(m.role); - return normalized.id !== "unknown" ? normalized.id : undefined; - }) - .filter((id): id is string => !!id); - - if (roleIds.length > 0) { + // Supplementary query: only needed when the role was a bare string (no eagerly loaded permissions) + if (!permissionsPreloaded && normalized.id !== "unknown") { + const roleIds = [normalized.id]; const uniqueRoleIds = [...new Set(roleIds)]; const rolePerms = await this.db.query.rolePermission.findMany({ where: (rp, { inArray }) => inArray(rp.roleId, uniqueRoleIds), columns: { permissionId: true }, }); for (const rp of rolePerms) { permissions.add(rp.permissionId); } } }
🤖 Fix all issues with AI agents
In `@apps/api/src/db/database-manager.ts`:
- Around line 428-435: The supplementary-lookup block is dead/redundant (it logs
a warning and contains only a placeholder comment) and the condition typeof
member.role === 'string' is confusing because normalizeRole already handles
string-vs-object dispatch; remove the entire if (!member.role?.permissions &&
typeof member.role === 'string') { ... } block (including the console.log and
placeholder comment) to avoid misleading future readers, or if you actually need
fallback behavior implement it by converting the legacy string via the existing
normalizeRole function or by querying the roles store (e.g., resolve member.role
string to a Role object and populate permissions) so that the code path uses
normalizeRole/member.role resolution consistently.
- Around line 413-415: The code silently drops extra memberships by using
user.members[0]; instead, detect when user.members.length > 1 and emit a
warning-level log that includes identifying info (e.g., user.id or user.email)
and the count/list of skipped memberships before continuing to use the first
member; update the block around the member selection (the user.members check and
the `const member = user.members[0];` line) to call the existing logger (e.g.,
logger or processLogger in scope) with a clear message about discarded
memberships for diagnostics.
In `@apps/api/src/db/db-cli.ts`:
- Line 27: Replace the COMMANDS.forEach usage with a for...of loop to satisfy
the Biome lint rule: iterate over COMMANDS using "for (const cmd of COMMANDS)"
and call console.error(` ${cmd}`) inside the loop (look for the COMMANDS symbol
in db-cli.ts and the existing line "COMMANDS.forEach((cmd) => console.error(`
${cmd}`));" to modify).
- Around line 11-18: The CLI currently lists COMMANDS without exposing
DatabaseManager.debugPermissions(roleName), causing inconsistency with
checkUserPermissions; either wire it into the CLI by adding a new command (e.g.,
add 'debug-role' to COMMANDS and handle parsing/invocation similar to the
existing 'check-user' flow to call DatabaseManager.debugPermissions(roleName)),
or make DatabaseManager.debugPermissions private (rename/remove its public
export) so it isn't part of the public API; update any command dispatch logic
that maps command strings to DatabaseManager methods accordingly to keep
behavior consistent.
In `@packages/identity/src/adapters/better-auth.adapter.ts`:
- Around line 819-822: When the permissions Set is empty and you add
PERMISSION_FALLBACK_DASHBOARD_READ (the fallback block where permissions.size
=== 0), emit a warning log so misconfigured roles are visible in production;
update the fallback branch in better-auth.adapter.ts to log a warning that
includes identifying info (role id or name available in scope) and the fact
you're applying PERMISSION_FALLBACK_DASHBOARD_READ, using the adapter's existing
logger instance (or processLogger) so operators can find and fix zero-permission
roles.
- Around line 832-838: The returned user object is inconsistent because role is
lowercased but memberRole uses the original casing; update the assignment so
both use the same normalized casing (e.g., use the already lowercased value or a
new normalized variable) by changing memberRole to the lowercased form of role
(ensure the logic around role normalization that produces the role variable is
respected) so that both role and memberRole are lowercase and consistent with
downstream expectations.
Comment on lines
+413
to
+415
| if (user.members && user.members.length > 0) { | ||
| // Enforce Single-Tenant Rule: Use only the first member record | ||
| const member = user.members[0]; |
Contributor
There was a problem hiding this comment.
Single-tenant assumption silently discards additional memberships.
user.members[0] is taken with a comment about "Single-Tenant Rule," but if a user has multiple memberships, the rest are silently ignored. For a debug/diagnostic tool, this is misleading — an operator investigating permissions wouldn't know other memberships exist.
♻️ Proposed fix — log a warning when additional memberships are skipped
if (user.members && user.members.length > 0) {
- // Enforce Single-Tenant Rule: Use only the first member record
const member = user.members[0];
+ if (user.members.length > 1) {
+ console.log(
+ ` ⚠️ User has ${user.members.length} memberships; showing first only`,
+ );
+ }
const normalized = normalizeRole(member.role);🤖 Prompt for AI Agents
In `@apps/api/src/db/database-manager.ts` around lines 413 - 415, The code
silently drops extra memberships by using user.members[0]; instead, detect when
user.members.length > 1 and emit a warning-level log that includes identifying
info (e.g., user.id or user.email) and the count/list of skipped memberships
before continuing to use the first member; update the block around the member
selection (the user.members check and the `const member = user.members[0];`
line) to call the existing logger (e.g., logger or processLogger in scope) with
a clear message about discarded memberships for diagnostics.
Comment on lines
+819
to
+822
| // 3. Fallback / Default Permissions | ||
| if (permissions.size === 0) { | ||
| permissions.add(PERMISSION_FALLBACK_DASHBOARD_READ); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Note: zero-permission roles silently receive the fallback dashboard:read.
If a role exists in the DB but has no permissions assigned (configuration oversight), this fallback masks the issue. Consider logging a warning when falling back so misconfigurations are visible in production.
Proposed improvement
// 3. Fallback / Default Permissions
if (permissions.size === 0) {
+ console.warn(
+ `[BetterAuthAdapter] No permissions resolved for user ${dbUser.id}; applying fallback: ${PERMISSION_FALLBACK_DASHBOARD_READ}`,
+ );
permissions.add(PERMISSION_FALLBACK_DASHBOARD_READ);
}📝 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
| // 3. Fallback / Default Permissions | |
| if (permissions.size === 0) { | |
| permissions.add(PERMISSION_FALLBACK_DASHBOARD_READ); | |
| } | |
| // 3. Fallback / Default Permissions | |
| if (permissions.size === 0) { | |
| console.warn( | |
| `[BetterAuthAdapter] No permissions resolved for user ${dbUser.id}; applying fallback: ${PERMISSION_FALLBACK_DASHBOARD_READ}`, | |
| ); | |
| permissions.add(PERMISSION_FALLBACK_DASHBOARD_READ); | |
| } |
🤖 Prompt for AI Agents
In `@packages/identity/src/adapters/better-auth.adapter.ts` around lines 819 -
822, When the permissions Set is empty and you add
PERMISSION_FALLBACK_DASHBOARD_READ (the fallback block where permissions.size
=== 0), emit a warning log so misconfigured roles are visible in production;
update the fallback branch in better-auth.adapter.ts to log a warning that
includes identifying info (role id or name available in scope) and the fact
you're applying PERMISSION_FALLBACK_DASHBOARD_READ, using the adapter's existing
logger instance (or processLogger) so operators can find and fix zero-permission
roles.
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 (2)
packages/identity/src/adapters/better-auth.adapter.ts (2)
796-817: 🧹 Nitpick | 🔵 TrivialRedundant
normalizeRolecall and unconditional DB round-trip.
normalizeRole(m.role)is called twice for the same member — once at Line 783 and again at Line 802. Since the single-tenant invariant guarantees exactly one member, you can reuse the result from the first call.More significantly, the supplementary
rolePermission.findManyquery at Line 809 always fires when a role ID exists, even when permissions were already fully resolved from the eagerly-loaded relation. In the common path (eager load succeeds), this is a wasted DB round-trip that returns duplicates theSetsilently absorbs.♻️ Proposed refactor: reuse normalizeRole result, skip query when permissions were eagerly loaded
if (hasMembership) { const m = members![0]; const normalized = normalizeRole(m.role); if (normalized.name !== "unknown") { role = normalized.name; } + let permissionsPreloaded = false; if (normalized.permissions && normalized.permissions.length > 0) { for (const rp of normalized.permissions) { permissions.add(rp.permissionId); } + permissionsPreloaded = true; } - } - // 2. Supplementary permission query - // We check for string role IDs and fetch their permissions if needed. - // We do this regardless of preloaded permissions to ensure we don't drop legacy role data. - if (hasMembership) { - const roleIds = members! - .map((m) => { - const normalized = normalizeRole(m.role); - return normalized.id !== "unknown" ? normalized.id : undefined; - }) - .filter((id): id is string => !!id); - - if (roleIds.length > 0) { - const uniqueRoleIds = [...new Set(roleIds)]; + // Supplementary query only when eager-load didn't resolve permissions + if (!permissionsPreloaded && normalized.id !== "unknown") { const rolePerms = await this.db.query.rolePermission.findMany({ - where: (rp, { inArray }) => inArray(rp.roleId, uniqueRoleIds), + where: (rp, { eq }) => eq(rp.roleId, normalized.id), columns: { permissionId: true }, }); for (const rp of rolePerms) { permissions.add(rp.permissionId); } } }
719-728:⚠️ Potential issue | 🟡 MinorType mismatch between parameter and internal
MemberWithRoletype needs clarification.The parameter type (lines 722–726) defines
roleasstring | (schema.Role & {...})(nonull), while the internalMemberWithRoletype (lines 744–749) includes| null. This mismatch stems from Drizzle's type inference for the lazy-fetch path, not from DB nullability—themember.rolecolumn is declared as.notNull()in the schema.normalizeRolehandlesnullinput gracefully (returnsid: "unknown", name: "unknown"), so there's no runtime risk, but theas unknown as typeof memberscast on line 766 masks this type inconsistency. Explicitly aligning the parameter type withMemberWithRoleor adding a null check before normalizing would make the intent clearer.
🤖 Fix all issues with AI agents
In `@packages/identity/src/adapters/better-auth.adapter.ts`:
- Around line 835-841: The returned user object has inconsistent lowercasing:
`role` uses role.toLowerCase() while `memberRole` uses role.toLocaleLowerCase(),
which can produce different values for locale-sensitive characters; update the
`memberRole` assignment in the same object (the block that sets role,
permissions, banned, banReason, banExpires, hasTenant, memberRole) to use
role.toLowerCase() so both `role` and `memberRole` are normalized identically.
Comment on lines
+835
to
+841
| role: role.toLowerCase(), | ||
| permissions: Array.from(permissions), | ||
| banned: dbUser.banned || false, | ||
| banReason: dbUser.banReason || null, | ||
| banExpires: dbUser.banExpires || null, | ||
| hasTenant: hasMembership, | ||
| memberRole: role.toLocaleLowerCase(), |
Contributor
There was a problem hiding this comment.
Inconsistent lowercasing: toLowerCase() vs toLocaleLowerCase().
role (Line 835) uses toLowerCase() while memberRole (Line 841) uses toLocaleLowerCase(). For ASCII role names they behave identically, but with locale-sensitive characters (e.g., Turkish İ) they diverge — producing mismatched values on the same returned object.
Use toLowerCase() for both, since role identifiers should be locale-independent.
Proposed fix
- memberRole: role.toLocaleLowerCase(),
+ memberRole: role.toLowerCase(),🤖 Prompt for AI Agents
In `@packages/identity/src/adapters/better-auth.adapter.ts` around lines 835 -
841, The returned user object has inconsistent lowercasing: `role` uses
role.toLowerCase() while `memberRole` uses role.toLocaleLowerCase(), which can
produce different values for locale-sensitive characters; update the
`memberRole` assignment in the same object (the block that sets role,
permissions, banned, banReason, banExpires, hasTenant, memberRole) to use
role.toLowerCase() so both `role` and `memberRole` are normalized identically.
This was referenced Feb 15, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation