chore: address remaining PR feedback for types, ESM resolution, and tech debt - #84
Merged
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR makes ESM compatibility edits, converts several runtime imports to type-only, introduces a shared EncryptionService contract and updates AesEncryptionService and token manager to use it, exposes package type entrypoints, hardens tests, and updates TECHNICAL_DEBT.md to recommend a 30-day scheduled PII cleanup approach. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
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/services/permission-seeder.spec.ts (1)
196-302:⚠️ Potential issue | 🟠 MajorWrap each module mock in
try/finallyto guarantee cleanup.If the dynamic import,
seedSystemRbac(...), or the assertion throws beforevi.doUnmock("../constants.js"), the mock persists and poisons later tests.vi.restoreAllMocks()does not cover module-level cleanup; onlyvi.doUnmock()removes the mock from the registry. Usetry/finallyto ensure cleanup always runs.Suggested fix
it("seed skips rolePermission insertion if all exist", async () => { vi.resetModules(); vi.doMock("../constants.js", async (importOriginal) => { const actual = await importOriginal<typeof import("../constants.js")>(); return { ...actual, // Include all permissions referenced by the member role in rbac-seeding ALL_PERMISSIONS: [ "users:read", "tenants:read", "dashboard:read", "admin_dashboard:view", "system_users:read", "system_tenants:read", ], isSystemPermission: (p: string) => p.startsWith("system_") || p.startsWith("admin_dashboard:"), }; }); - - // Re-import to pickup mock - const { seedSystemRbac } = await import("../utils/rbac-seeding.js"); - - const dbMock = mkDb(); + try { + // Re-import to pickup mock + const { seedSystemRbac } = await import("../utils/rbac-seeding.js"); + + const dbMock = mkDb(); - // Return existing rows covering every permission that seedSystemRbac would generate - // for owner, admin, and member roles so the deduplication sees them all as existing. - const existingRows = [ - // member base perms (organizationId: null) - { roleId: "member", permissionId: "users:read", organizationId: null }, - { roleId: "member", permissionId: "tenants:read", organizationId: null }, - { - roleId: "member", - permissionId: "dashboard:read", - organizationId: null, - }, - // member system perms (organizationId: "sys") - { - roleId: "member", - permissionId: "admin_dashboard:view", - organizationId: "sys", - }, - { - roleId: "member", - permissionId: "system_users:read", - organizationId: "sys", - }, - { - roleId: "member", - permissionId: "system_tenants:read", - organizationId: "sys", - }, - // admin & owner — all perms (null + sys scoped) - ...["admin", "owner"].flatMap((role) => [ - { roleId: role, permissionId: "users:read", organizationId: null }, - { roleId: role, permissionId: "tenants:read", organizationId: null }, - { roleId: role, permissionId: "dashboard:read", organizationId: null }, - { - roleId: role, - permissionId: "admin_dashboard:view", - organizationId: "sys", - }, - { - roleId: role, - permissionId: "system_users:read", - organizationId: "sys", - }, - { - roleId: role, - permissionId: "system_tenants:read", - organizationId: "sys", - }, - ]), - ]; - dbMock.select.mockReturnValue(mockChainedQuery(existingRows)); + // Return existing rows covering every permission that seedSystemRbac would generate + // for owner, admin, and member roles so the deduplication sees them all as existing. + const existingRows = [ + // member base perms (organizationId: null) + { roleId: "member", permissionId: "users:read", organizationId: null }, + { roleId: "member", permissionId: "tenants:read", organizationId: null }, + { + roleId: "member", + permissionId: "dashboard:read", + organizationId: null, + }, + // member system perms (organizationId: "sys") + { + roleId: "member", + permissionId: "admin_dashboard:view", + organizationId: "sys", + }, + { + roleId: "member", + permissionId: "system_users:read", + organizationId: "sys", + }, + { + roleId: "member", + permissionId: "system_tenants:read", + organizationId: "sys", + }, + // admin & owner — all perms (null + sys scoped) + ...["admin", "owner"].flatMap((role) => [ + { roleId: role, permissionId: "users:read", organizationId: null }, + { roleId: role, permissionId: "tenants:read", organizationId: null }, + { roleId: role, permissionId: "dashboard:read", organizationId: null }, + { + roleId: role, + permissionId: "admin_dashboard:view", + organizationId: "sys", + }, + { + roleId: role, + permissionId: "system_users:read", + organizationId: "sys", + }, + { + roleId: role, + permissionId: "system_tenants:read", + organizationId: "sys", + }, + ]), + ]; + dbMock.select.mockReturnValue(mockChainedQuery(existingRows)); - const loggerMock = { log: vi.fn(), error: vi.fn() } as unknown as Logger; - const optionsMock = mkOptions(); + const loggerMock = { log: vi.fn(), error: vi.fn() } as unknown as Logger; + const optionsMock = mkOptions(); - await seedSystemRbac(dbMock, optionsMock.constants, loggerMock); + await seedSystemRbac(dbMock, optionsMock.constants, loggerMock); - expect(loggerMock.log).toHaveBeenCalledWith( - "No new role permissions to insert.", - ); - - vi.doUnmock("../constants.js"); + expect(loggerMock.log).toHaveBeenCalledWith( + "No new role permissions to insert.", + ); + } finally { + vi.doUnmock("../constants.js"); + vi.resetModules(); + } });Apply the same pattern to the test starting at line 283.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/identity/src/services/permission-seeder.spec.ts` around lines 196 - 302, The test blocks that call vi.doMock("../constants.js") must ensure module mock cleanup even if imports or assertions throw; wrap the dynamic import of seedSystemRbac, the calls to seedSystemRbac, and the expect assertions in a try/finally and move vi.doUnmock("../constants.js") into the finally block so the mock is always removed. Specifically, for the two tests that mock constants (the one verifying "No new role permissions to insert." and the "seed throws error on invalid permission format" test), keep vi.resetModules() and vi.doMock(...) as-is, then perform the await import("../utils/rbac-seeding.js") and subsequent calls to seedSystemRbac/dbMock/loggerMock/optionsMock and assertions inside a try, and call vi.doUnmock("../constants.js") in the finally to guarantee cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/connectors/src/crypto/encryption.service.ts`:
- Around line 3-7: The current AesEncryptionService imports the base
EncryptionService from TokenManagerService, coupling crypto to the OAuth module;
create a new standalone abstraction (e.g., export an abstract class or interface
named EncryptionService in a new crypto-level file) and move the
EncryptionService declaration there, then update AesEncryptionService to import
EncryptionService from that new module and also update TokenManagerService to
import the same EncryptionService; ensure the new file exports the same symbol
name (EncryptionService) so AesEncryptionService and TokenManagerService
reference identical types and remove the original export from
token-manager.service.
In `@packages/database/package.json`:
- Line 45: The file ends with a closing brace '}' but lacks a trailing newline;
open the package.json that ends with that '}' and add a single newline character
after it (ensuring the file ends with '\n') so it conforms to POSIX/newline
conventions and linters.
In `@packages/identity/src/adapters/drizzle-user.adapter.spec.ts`:
- Line 2: Remove the file-wide "/* eslint-disable
`@typescript-eslint/unbound-method` */" and instead add inline eslint-disable
comments only on the specific assertion lines that provoke the Vitest false
positive (the expect(...) calls in drizzle-user.adapter.spec.ts that reference
unbound methods). Locate the offending expect(...) statements in the spec and
append an inline comment like // eslint-disable-next-line
`@typescript-eslint/unbound-method` to each such line, leaving the rest of the
file linting intact.
In `@TECHNICAL_DEBT.md`:
- Around line 25-26: The doc currently recommends implementing runPIICleanup
with `@nestjs/schedule` which will execute in each replica and produce duplicate
cleanup/audit events; update the guidance in the background/jobs section to
explicitly warn against using an in-process cron without a singleton guarantee
and list safe alternatives: runPIICleanup only from a single dedicated worker
process, schedule it via an external orchestrator (e.g., Kubernetes CronJob), or
protect the in-process job with a distributed lock/leader election (e.g., Redis
Redlock, Consul/etcd leader election) and provide a short note to ensure audit
events are emitted exactly once per cleanup pass.
---
Outside diff comments:
In `@packages/identity/src/services/permission-seeder.spec.ts`:
- Around line 196-302: The test blocks that call vi.doMock("../constants.js")
must ensure module mock cleanup even if imports or assertions throw; wrap the
dynamic import of seedSystemRbac, the calls to seedSystemRbac, and the expect
assertions in a try/finally and move vi.doUnmock("../constants.js") into the
finally block so the mock is always removed. Specifically, for the two tests
that mock constants (the one verifying "No new role permissions to insert." and
the "seed throws error on invalid permission format" test), keep
vi.resetModules() and vi.doMock(...) as-is, then perform the await
import("../utils/rbac-seeding.js") and subsequent calls to
seedSystemRbac/dbMock/loggerMock/optionsMock and assertions inside a try, and
call vi.doUnmock("../constants.js") in the finally to guarantee cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f13c53ac-67d6-4edd-a1f2-46f01ef6321e
📒 Files selected for processing (13)
TECHNICAL_DEBT.mdapps/api/src/db/database-manager.spec.tsapps/api/src/db/reset-e2e.tsapps/api/src/main.tsapps/api/src/modules/connections/connections/callback.controller.spec.tsapps/api/src/modules/identity/users/users.controller.tsapps/api/src/modules/trigger/trigger.module.tsapps/api/src/scripts/admin-bootstrap.tspackages/connectors/src/crypto/encryption.service.tspackages/database/package.jsonpackages/identity/src/adapters/drizzle-user.adapter.spec.tspackages/identity/src/services/permission-seeder.spec.tspackages/pieces/salesforce/tsconfig.json
Comment on lines
+3
to
+7
| import { EncryptionService } from '../oauth/token-manager.service.js'; | ||
| import { createCipheriv, createDecipheriv, randomBytes } from 'node:crypto'; | ||
|
|
||
| @Injectable() | ||
| export class AesEncryptionService { | ||
| export class AesEncryptionService extends EncryptionService { |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Decouple the crypto contract from token-manager.service.
Line 3 now pulls the base abstraction from packages/connectors/src/oauth/token-manager.service.ts:8-11, so the crypto implementation depends on the OAuth module just to inherit EncryptionService. That boundary is backwards and makes reuse/circular-import regressions easier. Please move EncryptionService into its own file under crypto or a shared abstractions module, then have both AesEncryptionService and TokenManagerService import it from there.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/connectors/src/crypto/encryption.service.ts` around lines 3 - 7, The
current AesEncryptionService imports the base EncryptionService from
TokenManagerService, coupling crypto to the OAuth module; create a new
standalone abstraction (e.g., export an abstract class or interface named
EncryptionService in a new crypto-level file) and move the EncryptionService
declaration there, then update AesEncryptionService to import EncryptionService
from that new module and also update TokenManagerService to import the same
EncryptionService; ensure the new file exports the same symbol name
(EncryptionService) so AesEncryptionService and TokenManagerService reference
identical types and remove the original export from token-manager.service.
| "typescript": "^5.7.3" | ||
| } | ||
| } | ||
| } No newline at end of file |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor: Missing trailing newline.
The file ends without a trailing newline. POSIX convention and most linters expect files to end with a newline character.
🔧 Add trailing newline
}
-}
+}
+📝 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
| } | |
| } | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/database/package.json` at line 45, The file ends with a closing
brace '}' but lacks a trailing newline; open the package.json that ends with
that '}' and add a single newline character after it (ensuring the file ends
with '\n') so it conforms to POSIX/newline conventions and linters.
| @@ -1,4 +1,5 @@ | |||
| /* eslint-disable @typescript-eslint/no-unsafe-return */ | |||
| /* eslint-disable @typescript-eslint/unbound-method */ | |||
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Scope the unbound-method suppression to the offending assertions.
Disabling @typescript-eslint/unbound-method for the whole spec hides real unbound calls in the rest of the file. Keep the suppression inline on the specific expect(...) lines that trigger the Vitest false positive instead of broadening it to file scope.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/identity/src/adapters/drizzle-user.adapter.spec.ts` at line 2,
Remove the file-wide "/* eslint-disable `@typescript-eslint/unbound-method` */"
and instead add inline eslint-disable comments only on the specific assertion
lines that provoke the Vitest false positive (the expect(...) calls in
drizzle-user.adapter.spec.ts that reference unbound methods). Locate the
offending expect(...) statements in the spec and append an inline comment like
// eslint-disable-next-line `@typescript-eslint/unbound-method` to each such line,
leaving the rest of the file linting intact.
Comment on lines
+23
to
+26
| - Keep raw IP/user-agent on insert for security auditing. | ||
| - Remove or rename the legacy `anonymizeIp` and `anonymizeUserAgent` pre-insert helpers if they exist. | ||
| - Implement `runPIICleanup` inside the `background`/`jobs` module using `@nestjs/schedule` to run daily. | ||
| - This job will find sessions older than 30 days and anonymize their PII (nullify or hash), emitting audit logs. |
Contributor
There was a problem hiding this comment.
Specify one irreversible cleanup strategy.
nullify or hash is too ambiguous for a compliance-sensitive path. Those options have different privacy properties, and a hash can still leave the data linkable. Please document a single approved transformation and explicitly forbid raw IP/user-agent from being copied into the audit logs produced by this job.
Suggested doc change
-- This job will find sessions older than 30 days and anonymize their PII (nullify or hash), emitting audit logs.
+- This job will find sessions older than 30 days and apply a single approved irreversible cleanup strategy to their PII.
+- Prefer nulling the raw `ipAddress` and `userAgent` fields unless a reviewed retention requirement explicitly requires a non-reversible derived value.
+- Audit logs must record counts/record IDs/timestamps only and must not persist raw `ipAddress` or `userAgent`.
Comment on lines
+25
to
+26
| - Implement `runPIICleanup` inside the `background`/`jobs` module using `@nestjs/schedule` to run daily. | ||
| - This job will find sessions older than 30 days and anonymize their PII (nullify or hash), emitting audit logs. |
Contributor
There was a problem hiding this comment.
Avoid documenting this as an in-process cron without a singleton guarantee.
Putting runPIICleanup behind @nestjs/schedule in the app module will run it once per replica in a scaled deployment. That creates duplicate cleanup passes and duplicate audit events unless you also define leader election or a distributed lock.
Suggested doc change
-- Implement `runPIICleanup` inside the `background`/`jobs` module using `@nestjs/schedule` to run daily.
+- Implement `runPIICleanup` as a singleton scheduled task.
+- If `@nestjs/schedule` is used, guard execution with leader election or a distributed lock; otherwise run it from a dedicated worker/queue so only one instance performs the daily cleanup.📝 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
| - Implement `runPIICleanup` inside the `background`/`jobs` module using `@nestjs/schedule` to run daily. | |
| - This job will find sessions older than 30 days and anonymize their PII (nullify or hash), emitting audit logs. | |
| - Implement `runPIICleanup` as a singleton scheduled task. | |
| - If `@nestjs/schedule` is used, guard execution with leader election or a distributed lock; otherwise run it from a dedicated worker/queue so only one instance performs the daily cleanup. | |
| - This job will find sessions older than 30 days and anonymize their PII (nullify or hash), emitting audit logs. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@TECHNICAL_DEBT.md` around lines 25 - 26, The doc currently recommends
implementing runPIICleanup with `@nestjs/schedule` which will execute in each
replica and produce duplicate cleanup/audit events; update the guidance in the
background/jobs section to explicitly warn against using an in-process cron
without a singleton guarantee and list safe alternatives: runPIICleanup only
from a single dedicated worker process, schedule it via an external orchestrator
(e.g., Kubernetes CronJob), or protect the in-process job with a distributed
lock/leader election (e.g., Redis Redlock, Consul/etcd leader election) and
provide a short note to ensure audit events are emitted exactly once per cleanup
pass.
…up and exclusions
Contributor
There was a problem hiding this comment.
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/drizzle-user.adapter.spec.ts (1)
181-196: 🧹 Nitpick | 🔵 TrivialAssert the full cascade, not just “some delete happened.”
txCalls.length > 0still passes if only one table is deleted. Sincepackages/identity/src/adapters/drizzle-user.adapter.ts:67-80is explicitly cascading through five tables, this test should verify that full sequence so partial cleanup regressions do not slip through.Proposed test strengthening
- const txCalls: string[] = []; + const deletedTables: unknown[] = []; db.transaction.mockImplementation((fn: (tx: MockTx) => unknown) => { const tx = { - delete: vi.fn().mockImplementation(() => { - txCalls.push("delete"); + delete: vi.fn().mockImplementation((table) => { + deletedTables.push(table); return { where: vi.fn().mockReturnThis() }; }), } as unknown as MockTx; return fn(tx); }); @@ await adapter.delete("u1"); expect(db.transaction).toHaveBeenCalled(); - expect(txCalls.length).toBeGreaterThan(0); + expect(deletedTables).toEqual([ + schema.member, + schema.invitation, + schema.session, + schema.account, + schema.user, + ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/identity/src/adapters/drizzle-user.adapter.spec.ts` around lines 181 - 196, The test currently only asserts txCalls.length > 0 which allows partial cascades; update the db.transaction mock and assertions to validate the full cascade invoked by adapter.delete: in the mockImplementation of db.transaction (the tx delete mock that pushes into txCalls), record distinct identifiers for each targeted table (or push an entry when the chained where() is invoked) in the order the adapter cascades through tables referenced in the delete implementation, then assert txCalls equals the exact expected sequence (five entries in the correct order) and maybe verify the exact count equals 5; keep the db.transaction mock, txCalls array and the call to await adapter.delete("u1") but replace the weak expect(txCalls.length).toBeGreaterThan(0) with strict equality and order assertions matching the cascade in the adapter.delete implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/identity/src/adapters/drizzle-user.adapter.spec.ts`:
- Around line 181-196: The test currently only asserts txCalls.length > 0 which
allows partial cascades; update the db.transaction mock and assertions to
validate the full cascade invoked by adapter.delete: in the mockImplementation
of db.transaction (the tx delete mock that pushes into txCalls), record distinct
identifiers for each targeted table (or push an entry when the chained where()
is invoked) in the order the adapter cascades through tables referenced in the
delete implementation, then assert txCalls equals the exact expected sequence
(five entries in the correct order) and maybe verify the exact count equals 5;
keep the db.transaction mock, txCalls array and the call to await
adapter.delete("u1") but replace the weak
expect(txCalls.length).toBeGreaterThan(0) with strict equality and order
assertions matching the cascade in the adapter.delete implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3bbec596-f20b-417a-adc5-5bd5eddcda56
📒 Files selected for processing (6)
packages/connectors/src/crypto/encryption.interface.tspackages/connectors/src/crypto/encryption.service.tspackages/connectors/src/index.tspackages/connectors/src/oauth/token-manager.service.tspackages/identity/src/adapters/drizzle-user.adapter.spec.tspackages/identity/src/services/permission-seeder.spec.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/identity/src/adapters/drizzle-user.adapter.spec.ts`:
- Around line 482-489: The test currently verifies that countChain.innerJoin was
called but not that the tenant filter was applied to the count query; update the
spec that calls adapter.findAll to also assert countChain.where was invoked with
the same tenant predicate used for dataChain (e.g., check countChain.where was
called and/or calledWith matching tenantId predicate), referencing the existing
mocks countChain and dataChain and the adapter.findAll invocation so the
count-side pagination total is validated for tenant scoping.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6d8ea45f-5ae2-4180-9bf7-4dadf5d0ad77
📒 Files selected for processing (1)
packages/identity/src/adapters/drizzle-user.adapter.spec.ts
Comment on lines
482
to
489
| await adapter.findAll({ tenantId: "t1" }); | ||
|
|
||
| expect(dataChain.innerJoin).toHaveBeenCalled(); | ||
|
|
||
| expect(countChain.innerJoin).toHaveBeenCalled(); | ||
|
|
||
| expect(dataChain.where).toHaveBeenCalled(); | ||
| }); |
Contributor
There was a problem hiding this comment.
Assert tenant filtering on the count query too.
This spec verifies innerJoin() on countChain, but it never checks that the tenant predicate is applied there. If the count-side where() is omitted, the test still passes while pagination totals leak across tenants.
Suggested fix
expect(dataChain.innerJoin).toHaveBeenCalled();
expect(countChain.innerJoin).toHaveBeenCalled();
expect(dataChain.where).toHaveBeenCalled();
+ expect(countChain.where).toHaveBeenCalled();📝 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
| await adapter.findAll({ tenantId: "t1" }); | |
| expect(dataChain.innerJoin).toHaveBeenCalled(); | |
| expect(countChain.innerJoin).toHaveBeenCalled(); | |
| expect(dataChain.where).toHaveBeenCalled(); | |
| }); | |
| await adapter.findAll({ tenantId: "t1" }); | |
| expect(dataChain.innerJoin).toHaveBeenCalled(); | |
| expect(countChain.innerJoin).toHaveBeenCalled(); | |
| expect(dataChain.where).toHaveBeenCalled(); | |
| expect(countChain.where).toHaveBeenCalled(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/identity/src/adapters/drizzle-user.adapter.spec.ts` around lines 482
- 489, The test currently verifies that countChain.innerJoin was called but not
that the tenant filter was applied to the count query; update the spec that
calls adapter.findAll to also assert countChain.where was invoked with the same
tenant predicate used for dataChain (e.g., check countChain.where was called
and/or calledWith matching tenantId predicate), referencing the existing mocks
countChain and dataChain and the adapter.findAll invocation so the count-side
pagination total is validated for tenant scoping.
This was referenced Mar 21, 2026
Merged
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
Bug Fixes
Documentation
Chores
Tests