Repository navigation
feat: complete rbac implementation and verification >90% coverage - #30
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds RBAC schema (roles, permissions, role_permission) and seeding; replaces Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PermissionAdapter as DrizzlePermissionAdapter
participant DB as Database
participant Context as MemberContext
Client->>PermissionAdapter: can(user, resource, action, tenantId)
PermissionAdapter->>PermissionAdapter: if user.systemRole == "platform_admin" return true
alt no tenantId
PermissionAdapter-->>Client: false
else
PermissionAdapter->>DB: fetchMemberContext(user.id, tenantId)
Note over DB: JOIN member, role, role_permission, permission
DB-->>Context: { roleId, roleName, permissions[] }
PermissionAdapter->>PermissionAdapter: if roleName == "Owner" allow
alt Owner
PermissionAdapter-->>Client: true
else
PermissionAdapter->>PermissionAdapter: match permissions for resource/action
alt permission found
PermissionAdapter-->>Client: true
else
PermissionAdapter-->>Client: false
end
end
end
sequenceDiagram
participant Test
participant BetterAuthAdapter
participant Callbacks as BetterAuthCallbacks
participant Email as EmailService
Test->>BetterAuthAdapter: instantiate adapter (registers callbacks)
BetterAuthAdapter->>Callbacks: register hashing and email callbacks
Callbacks-->>BetterAuthAdapter: hash/verify available
Test->>Callbacks: trigger email verification flow
Callbacks->>Email: send verification email
Email-->>Callbacks: delivered
Test->>Callbacks: trigger invitation via org plugin
Callbacks->>Email: send invitation email
Email-->>Callbacks: delivered
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 8
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/drizzle-tenant.adapter.ts (1)
40-46: Add error handling for missing admin role and ensure RBAC seeding occurs before tenant creation.The hardcoded
roleId: "admin"references a role that must exist in the role table. Themembertable has a foreign key constraint onroleId, so if the "admin" role doesn't exist, the insert will fail with a FK violation (error code 23503). The current error handler only catches slug uniqueness violations (code 23505), leaving FK violations unhandled.The
seedRbacfunction exists but is not invoked in the normal initialization flow (seed.ts has seeding removed as legacy, and bootstrap-admin.ts doesn't call it). This creates a deployment risk: if seedRbac isn't manually run before any tenant creation, the operation will fail.Either ensure the RBAC seeding script is always executed during initialization, or add error handling for FK violations and a fallback mechanism (e.g., query the role first, or create it if missing).
packages/identity/src/adapters/drizzle-permission.adapter.spec.ts (1)
144-173: Consider adding test for platform_admin with tenantId.The current tests cover platform_admin without tenantId and regular user with tenantId. Consider adding a test for platform_admin with tenantId to verify both wildcard and tenant permissions are returned.
// platform_admin with tenantId should get both "*" and tenant perms db.select.mockReturnValue( mockChainedQuery([ { roleId: "admin", roleName: "Admin", permId: "users:read", resource: "users", action: "read" }, ]), ); expect(await adapter.getPermissions(mkUser({ systemRole: "platform_admin" }), "o1")).toEqual([ "*", "role:admin", "users:read", ]);
🤖 Fix all issues with AI agents
In `@packages/identity/src/adapters/better-auth.adapter.spec.ts`:
- Around line 508-555: Remove the unnecessary dynamic import and use the
module-level mocked better-auth instead: delete the line that does const {
betterAuth } = await import("better-auth"); and reference the already-imported
mocked betterAuth when calling vi.mocked(...) in this test that instantiates
BetterAuthAdapter and inspects callArgs, so all uses of betterAuth in the test
come from the top-level mock rather than a dynamic import.
In `@packages/identity/src/adapters/drizzle-permission.adapter.spec.ts`:
- Around line 116-142: The test reveals a naming/semantic mismatch: the test
calls hasRole(user, "admin", "o1") passing a roleName-like string while the
DrizzlePermissionAdapter implementation compares against context.roleId; update
the test and/or adapter to be consistent — either change the test comment to
state it is asserting by role ID (roleId) when calling hasRole, or alter the
adapter’s hasRole implementation to compare the provided roleName to the stored
roleName instead of context.roleId; reference the test function hasRole, the
adapter class DrizzlePermissionAdapter, and the properties roleId/roleName when
making the clarification or code change.
- Around line 8-27: The mockChainedQuery helper currently defines a plain object
with a then property (a thenable), which can trigger lint warnings and cause
surprises in async code; update mockChainedQuery so it returns a real Promise by
using Promise.resolve(result) and attaching the chain methods to that Promise
(e.g., create const p = Promise.resolve(result) and Object.assign(p, { from:
vi.fn().mockReturnValue(p), ... }) ), keeping the same method names
("from","innerJoin","leftJoin","where","select","limit","offset","orderBy") and
preserving mockReturnValue behavior so existing tests that call those methods
still get the promise back.
In `@packages/identity/src/adapters/drizzle-permission.adapter.ts`:
- Around line 35-46: The hasRole method uses a parameter named roleName but
compares it to context?.roleId, which is inconsistent; either rename the
parameter to roleId or compare against the context's display name field. Fix by
updating the signature of hasRole(user: User, roleId: string, tenantId?: string)
and change all references from roleName to roleId (including the tenant branch:
return context?.roleId === roleId), or if the intent is to match display names,
change the tenant check to compare context?.roleName === roleName and ensure
fetchMemberContext returns/contains roleName; update usages accordingly
(hasRole, roleName/roleId variables) to keep naming consistent with
fetchMemberContext and user.systemRole.
- Around line 26-27: The current Owner override uses a brittle display-name
check (context.roleName === "Owner"); change it to check the role identifier
instead (use context.roleId) and compare against a canonical Owner role id or
helper (e.g., OWNER_ROLE_ID or an isOwnerRole(roleId) utility) inside the
drizzle-permission.adapter's permission check so it no longer depends on
case-sensitive display names; ensure any existing callers still pass roleId or
resolve it from the context before applying the override.
In `@packages/identity/src/adapters/drizzle-user.adapter.spec.ts`:
- Around line 180-213: Add assertions to the compensation-failure test so it
verifies that the compensation error is logged and that update was called with
the expected args: spy on console.error (or the logger used) and assert it was
called when the delete compensation fails, and add an assertion like
expect(spyUpdate).toHaveBeenCalledWith("uCompFail", { systemRole: "admin" }) to
verify adapter.update was invoked with the correct parameters; keep the existing
checks for thrown error and spyDelete call.
In `@packages/identity/src/schema.ts`:
- Around line 190-192: Update the mock to match the schema change: replace the
member mock property using role with roleId and map it to 'member.roleId'
instead of 'member.role' (i.e., change the object key from role to roleId in the
identity mocks so it aligns with the schema's roleId reference).
In `@packages/identity/src/scripts/seed-rbac.ts`:
- Around line 43-53: The member role in roleMap currently only grants
"users:read"; update the member entry in roleMap to also include "tenants:read"
so the member role reflects read-only access to tenant info (locate the roleMap
constant and the member array to modify).
Comment on lines
+508
to
+555
| it("configures better-auth callbacks correctly (emails, hashing)", async () => { | ||
| const db = mkDb(); | ||
| const email = mkEmail(); | ||
| const { betterAuth } = await import("better-auth"); | ||
|
|
||
| // Instantiate adapter to trigger betterAuth call | ||
| new BetterAuthAdapter(db, email as any, cfg()); | ||
|
|
||
| const callArgs = vi.mocked(betterAuth).mock.calls[0][0] as any; | ||
| expect(callArgs).toBeDefined(); | ||
|
|
||
| // 1. Password Hashing - trigger hash to cover lines | ||
| // checking that it returns a promise is enough to cover the adapter wrapper | ||
| const hashFn = callArgs.emailAndPassword.password.hash; | ||
| const verifyFn = callArgs.emailAndPassword.password.verify; | ||
| // We expect these to fail since bcrypt isn't mocked/installed deeply, but we catch it or expect promise | ||
| // Actually, let's just assert existence to avoid runtime errors if we don't want to mock bcrypt | ||
| expect(hashFn).toBeDefined(); | ||
| expect(verifyFn).toBeDefined(); | ||
|
|
||
| // 2. Email Verification | ||
| const sendVerify = callArgs.emailVerification.sendVerificationEmail; | ||
| await sendVerify({ | ||
| user: { email: "test@test.com" }, | ||
| url: "http://verify.com", | ||
| }); | ||
| expect(email.sendEmail).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| to: "test@test.com", | ||
| text: expect.stringContaining("http://verify.com"), | ||
| }), | ||
| ); | ||
|
|
||
| // 3. Invitation Email | ||
| const orgPlugin = callArgs.plugins.find((p: any) => p.sendInvitationEmail); | ||
| expect(orgPlugin).toBeDefined(); | ||
| await orgPlugin.sendInvitationEmail({ | ||
| email: "invite@test.com", | ||
| invitation: { id: "inv1" }, | ||
| organization: { name: "Test Org" }, | ||
| }); | ||
| expect(email.sendEmail).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| to: "invite@test.com", | ||
| subject: expect.stringContaining("invited to join"), | ||
| }), | ||
| ); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Good callback coverage test, but dynamic import is unnecessary.
The test validates email verification and invitation email callbacks. However, better-auth is already mocked at the top of the file (line 10), so the dynamic import on line 511 will return the same mock.
♻️ Suggested simplification
it("configures better-auth callbacks correctly (emails, hashing)", async () => {
const db = mkDb();
const email = mkEmail();
- const { betterAuth } = await import("better-auth");
+ const { betterAuth } = require("better-auth");
// Instantiate adapter to trigger betterAuth call
new BetterAuthAdapter(db, email as any, cfg());Or simply use the already-imported mock directly since vi.mocked works with the module-level mock.
📝 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
| it("configures better-auth callbacks correctly (emails, hashing)", async () => { | |
| const db = mkDb(); | |
| const email = mkEmail(); | |
| const { betterAuth } = await import("better-auth"); | |
| // Instantiate adapter to trigger betterAuth call | |
| new BetterAuthAdapter(db, email as any, cfg()); | |
| const callArgs = vi.mocked(betterAuth).mock.calls[0][0] as any; | |
| expect(callArgs).toBeDefined(); | |
| // 1. Password Hashing - trigger hash to cover lines | |
| // checking that it returns a promise is enough to cover the adapter wrapper | |
| const hashFn = callArgs.emailAndPassword.password.hash; | |
| const verifyFn = callArgs.emailAndPassword.password.verify; | |
| // We expect these to fail since bcrypt isn't mocked/installed deeply, but we catch it or expect promise | |
| // Actually, let's just assert existence to avoid runtime errors if we don't want to mock bcrypt | |
| expect(hashFn).toBeDefined(); | |
| expect(verifyFn).toBeDefined(); | |
| // 2. Email Verification | |
| const sendVerify = callArgs.emailVerification.sendVerificationEmail; | |
| await sendVerify({ | |
| user: { email: "test@test.com" }, | |
| url: "http://verify.com", | |
| }); | |
| expect(email.sendEmail).toHaveBeenCalledWith( | |
| expect.objectContaining({ | |
| to: "test@test.com", | |
| text: expect.stringContaining("http://verify.com"), | |
| }), | |
| ); | |
| // 3. Invitation Email | |
| const orgPlugin = callArgs.plugins.find((p: any) => p.sendInvitationEmail); | |
| expect(orgPlugin).toBeDefined(); | |
| await orgPlugin.sendInvitationEmail({ | |
| email: "invite@test.com", | |
| invitation: { id: "inv1" }, | |
| organization: { name: "Test Org" }, | |
| }); | |
| expect(email.sendEmail).toHaveBeenCalledWith( | |
| expect.objectContaining({ | |
| to: "invite@test.com", | |
| subject: expect.stringContaining("invited to join"), | |
| }), | |
| ); | |
| }); | |
| it("configures better-auth callbacks correctly (emails, hashing)", async () => { | |
| const db = mkDb(); | |
| const email = mkEmail(); | |
| const { betterAuth } = require("better-auth"); | |
| // Instantiate adapter to trigger betterAuth call | |
| new BetterAuthAdapter(db, email as any, cfg()); | |
| const callArgs = vi.mocked(betterAuth).mock.calls[0][0] as any; | |
| expect(callArgs).toBeDefined(); | |
| // 1. Password Hashing - trigger hash to cover lines | |
| // checking that it returns a promise is enough to cover the adapter wrapper | |
| const hashFn = callArgs.emailAndPassword.password.hash; | |
| const verifyFn = callArgs.emailAndPassword.password.verify; | |
| // We expect these to fail since bcrypt isn't mocked/installed deeply, but we catch it or expect promise | |
| // Actually, let's just assert existence to avoid runtime errors if we don't want to mock bcrypt | |
| expect(hashFn).toBeDefined(); | |
| expect(verifyFn).toBeDefined(); | |
| // 2. Email Verification | |
| const sendVerify = callArgs.emailVerification.sendVerificationEmail; | |
| await sendVerify({ | |
| user: { email: "test@test.com" }, | |
| url: "http://verify.com", | |
| }); | |
| expect(email.sendEmail).toHaveBeenCalledWith( | |
| expect.objectContaining({ | |
| to: "test@test.com", | |
| text: expect.stringContaining("http://verify.com"), | |
| }), | |
| ); | |
| // 3. Invitation Email | |
| const orgPlugin = callArgs.plugins.find((p: any) => p.sendInvitationEmail); | |
| expect(orgPlugin).toBeDefined(); | |
| await orgPlugin.sendInvitationEmail({ | |
| email: "invite@test.com", | |
| invitation: { id: "inv1" }, | |
| organization: { name: "Test Org" }, | |
| }); | |
| expect(email.sendEmail).toHaveBeenCalledWith( | |
| expect.objectContaining({ | |
| to: "invite@test.com", | |
| subject: expect.stringContaining("invited to join"), | |
| }), | |
| ); | |
| }); |
🤖 Prompt for AI Agents
In `@packages/identity/src/adapters/better-auth.adapter.spec.ts` around lines 508
- 555, Remove the unnecessary dynamic import and use the module-level mocked
better-auth instead: delete the line that does const { betterAuth } = await
import("better-auth"); and reference the already-imported mocked betterAuth when
calling vi.mocked(...) in this test that instantiates BetterAuthAdapter and
inspects callArgs, so all uses of betterAuth in the test come from the top-level
mock rather than a dynamic import.
Comment on lines
+8
to
+27
| const mockChainedQuery = (result: unknown) => { | ||
| const chain: Record<string, any> = { | ||
| then: (onfulfilled: (value: unknown) => unknown) => | ||
| Promise.resolve(result).then(onfulfilled), | ||
| }; | ||
| const methods = [ | ||
| "from", | ||
| "innerJoin", | ||
| "leftJoin", | ||
| "where", | ||
| "select", | ||
| "limit", | ||
| "offset", | ||
| "orderBy", | ||
| ]; | ||
| methods.forEach((m) => { | ||
| chain[m] = vi.fn().mockReturnValue(chain); | ||
| }); | ||
| return chain; | ||
| }; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Refactor thenable mock to avoid lint violation and potential pitfalls.
Biome flags then on plain objects as suspicious because thenables can cause unexpected behavior when passed to Promise.resolve() or used in async contexts. While this works, a cleaner approach wraps the result in an actual Promise.
♻️ Proposed fix
const mockChainedQuery = (result: unknown) => {
- const chain: Record<string, any> = {
- then: (onfulfilled: (value: unknown) => unknown) =>
- Promise.resolve(result).then(onfulfilled),
- };
+ const chain: Record<string, any> = {};
const methods = [
"from",
"innerJoin",
"leftJoin",
"where",
"select",
"limit",
"offset",
"orderBy",
];
methods.forEach((m) => {
chain[m] = vi.fn().mockReturnValue(chain);
});
- return chain;
+ // Return a promise that resolves to result, but also has chainable methods
+ const promise = Promise.resolve(result);
+ methods.forEach((m) => {
+ (promise as any)[m] = vi.fn().mockReturnValue(promise);
+ });
+ return promise;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const mockChainedQuery = (result: unknown) => { | |
| const chain: Record<string, any> = { | |
| then: (onfulfilled: (value: unknown) => unknown) => | |
| Promise.resolve(result).then(onfulfilled), | |
| }; | |
| const methods = [ | |
| "from", | |
| "innerJoin", | |
| "leftJoin", | |
| "where", | |
| "select", | |
| "limit", | |
| "offset", | |
| "orderBy", | |
| ]; | |
| methods.forEach((m) => { | |
| chain[m] = vi.fn().mockReturnValue(chain); | |
| }); | |
| return chain; | |
| }; | |
| const mockChainedQuery = (result: unknown) => { | |
| const chain: Record<string, any> = {}; | |
| const methods = [ | |
| "from", | |
| "innerJoin", | |
| "leftJoin", | |
| "where", | |
| "select", | |
| "limit", | |
| "offset", | |
| "orderBy", | |
| ]; | |
| methods.forEach((m) => { | |
| chain[m] = vi.fn().mockReturnValue(chain); | |
| }); | |
| // Return a promise that resolves to result, but also has chainable methods | |
| const promise = Promise.resolve(result); | |
| methods.forEach((m) => { | |
| (promise as any)[m] = vi.fn().mockReturnValue(promise); | |
| }); | |
| return promise; | |
| }; |
🧰 Tools
🪛 Biome (2.1.2)
[error] 10-10: Do not add then to an object.
(lint/suspicious/noThenProperty)
🤖 Prompt for AI Agents
In `@packages/identity/src/adapters/drizzle-permission.adapter.spec.ts` around
lines 8 - 27, The mockChainedQuery helper currently defines a plain object with
a then property (a thenable), which can trigger lint warnings and cause
surprises in async code; update mockChainedQuery so it returns a real Promise by
using Promise.resolve(result) and attaching the chain methods to that Promise
(e.g., create const p = Promise.resolve(result) and Object.assign(p, { from:
vi.fn().mockReturnValue(p), ... }) ), keeping the same method names
("from","innerJoin","leftJoin","where","select","limit","offset","orderBy") and
preserving mockReturnValue behavior so existing tests that call those methods
still get the promise back.
Comment on lines
+116
to
142
| it("hasRole: checks role id", async () => { | ||
| const db = mkDb(); | ||
| const adapter = new DrizzlePermissionAdapter(db); | ||
| const user = mkUser(); | ||
| expect(await adapter.can(user, "update", "user", undefined)).toBe(false); | ||
| }); | ||
|
|
||
| it("hasRole: tenant and system paths", async () => { | ||
| const db = mkDb(); | ||
| const adapter = new DrizzlePermissionAdapter(db); | ||
| const user = mkUser(); | ||
|
|
||
| db.query.member.findFirst.mockResolvedValueOnce({ role: "admin" }); | ||
| db.select.mockReturnValue( | ||
| mockChainedQuery([ | ||
| { | ||
| roleId: "admin", | ||
| roleName: "Admin", | ||
| permId: null, | ||
| }, | ||
| ]), | ||
| ); | ||
| expect(await adapter.hasRole(user, "admin", "o1")).toBe(true); | ||
|
|
||
| db.query.member.findFirst.mockResolvedValueOnce({ role: "user" }); | ||
| db.select.mockReturnValue( | ||
| mockChainedQuery([ | ||
| { | ||
| roleId: "user", | ||
| roleName: "User", | ||
| permId: null, | ||
| }, | ||
| ]), | ||
| ); | ||
| expect(await adapter.hasRole(user, "admin", "o1")).toBe(false); | ||
|
|
||
| expect(await adapter.hasRole(mkUser(), "platform_admin")).toBe(false); | ||
| expect( | ||
| await adapter.hasRole( | ||
| mkUser({ systemRole: "platform_admin" }), | ||
| "platform_admin", | ||
| ), | ||
| ).toBe(true); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Tests pass but reveal naming inconsistency in the adapter.
The test correctly verifies behavior, but note that hasRole(user, "admin", "o1") passes "admin" as a roleName parameter, yet the adapter compares it against context.roleId. This works because role IDs and names happen to match in lowercase, but the semantics are inconsistent. Consider updating the test comment to clarify this is checking by ID, not display name.
📝 Suggested comment update
- it("hasRole: checks role id", async () => {
+ it("hasRole: checks roleId match (not display name)", async () => {🤖 Prompt for AI Agents
In `@packages/identity/src/adapters/drizzle-permission.adapter.spec.ts` around
lines 116 - 142, The test reveals a naming/semantic mismatch: the test calls
hasRole(user, "admin", "o1") passing a roleName-like string while the
DrizzlePermissionAdapter implementation compares against context.roleId; update
the test and/or adapter to be consistent — either change the test comment to
state it is asserting by role ID (roleId) when calling hasRole, or alter the
adapter’s hasRole implementation to compare the provided roleName to the stored
roleName instead of context.roleId; reference the test function hasRole, the
adapter class DrizzlePermissionAdapter, and the properties roleId/roleName when
making the clarification or code change.
Comment on lines
+190
to
+192
| roleId: text("roleId") | ||
| .notNull() | ||
| .references(() => role.id, { onDelete: "restrict" }), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, verify the schema change at the specified lines
echo "=== Schema at lines 190-192 ==="
sed -n '188,195p' packages/identity/src/schema.ts
echo ""
echo "=== Searching for 'member.' references in mock files ==="
rg -n "member\." --type=ts apps/api/test/mocks/ -A 2 -B 2Repository: pramodnarayana/nexiom
Length of output: 902
🏁 Script executed:
#!/bin/bash
# Check the identity.mock.ts file specifically
echo "=== Checking identity.mock.ts ==="
cat -n apps/api/test/mocks/identity.mock.ts | head -100Repository: pramodnarayana/nexiom
Length of output: 2400
🏁 Script executed:
#!/bin/bash
# Search for usages of member.role in tests
echo "=== Searching for member.role usage in test files ==="
rg "member\.role" --type=ts apps/api/test/ -B 2 -A 2Repository: pramodnarayana/nexiom
Length of output: 400
🏁 Script executed:
#!/bin/bash
# Also check if roleId is used anywhere in tests
echo "=== Checking roleId usage in test files ==="
rg "roleId" --type=ts apps/api/test/ -B 1 -A 1 | head -50Repository: pramodnarayana/nexiom
Length of output: 108
Update mock file to reflect schema change from role to roleId.
The member table schema now uses roleId instead of role. Line 41 in apps/api/test/mocks/identity.mock.ts should be updated from role: 'member.role' to roleId: 'member.roleId' to maintain consistency with the schema.
🤖 Prompt for AI Agents
In `@packages/identity/src/schema.ts` around lines 190 - 192, Update the mock to
match the schema change: replace the member mock property using role with roleId
and map it to 'member.roleId' instead of 'member.role' (i.e., change the object
key from role to roleId in the identity mocks so it aligns with the schema's
roleId reference).
pramodnarayana
marked this pull request as draft
January 27, 2026 09:23
pramodnarayana
marked this pull request as ready for review
January 27, 2026 09:24
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/identity/src/adapters/drizzle-permission.adapter.ts`:
- Around line 69-111: fetchMemberContext currently maps rows to permissions
which may include duplicates if rolePermission has duplicate entries; update
fetchMemberContext to deduplicate by permission id (or by resource+action if id
can be null) before building the permissions array: iterate rows (after
filtering permId !== null), track seen permission ids in a Set (or a Map for
composite keys), only push unique permissions into the permissions result to
ensure stable, de-duplicated output for the permissions field returned by
fetchMemberContext.
In `@packages/identity/src/adapters/drizzle-user.adapter.spec.ts`:
- Around line 180-225: Rename the test case currently declared as it("create:
handles compensation failure (log only)", ...) to a clearer name like
it("create: handles compensation failure without masking original error") and
remove or condense the noisy inline comments about prior review feedback; keep
the assertions that verify adapter.create rejects with "Update failed" and that
spyUpdate was called with ("uCompFail", { systemRole: "admin" }) and spyDelete
was called with ("uCompFail") so behavior and spies (spyUpdate, spyDelete)
remain unchanged.
pramodnarayana
marked this pull request as draft
January 27, 2026 09:37
pramodnarayana
marked this pull request as ready for review
January 27, 2026 09:37
pramodnarayana
marked this pull request as draft
January 27, 2026 09:38
pramodnarayana
marked this pull request as ready for review
January 27, 2026 09:38
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/identity/src/adapters/drizzle-permission.adapter.ts`:
- Around line 99-123: The permissionsMap is created and populated from rows but
never used, causing an O(n²) dedupe via the reduce/acc.find() block; replace the
reduce with a single-pass Map-based dedup: iterate rows, for each r with
r.permId set permissionsMap.set(r.permId, { id: r.permId, resource: r.resource!,
action: r.action! }) only when not already present, then build the final
permissions array from permissionsMap.values(); alternatively remove
permissionsMap entirely and keep the existing reduce, but prefer using
permissionsMap for O(1) deduplication to replace the acc.find() usage and avoid
quadratic behavior.
This was referenced Jan 27, 2026
This was referenced Feb 6, 2026
This was referenced Feb 15, 2026
This was referenced Mar 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.