Repository navigation
fixed all npm package version mismatch - #151
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughRepository-wide dependency/tooling version bumps, Vitest/tsconfig config tweaks, many expanded/added Vitest specs and mock refinements across API/services, one exported pipe, and a small null-safe cursor change in the scheduler runner. ChangesDependency Upgrade & Test Coverage Expansion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/modules/identity/auth/auth.controller.spec.ts (1)
353-389: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winHoist
vi.mock()to the top level or usevi.doMock()for clarity.The
vi.mock('better-auth/node', ...)call at line 359 is inside a test case. While Vitest automatically hoistsvi.mock()calls, placing them inside tests is confusing and non-idiomatic because:
- The hoisting behavior is implicit and not obvious to readers
- The mock applies globally to all tests, not just this one
- Future maintainers may not realize the mock scope
♻️ Recommended approach
Move the mock to the top level with other mocks, or use
vi.doMock()if runtime mocking is specifically needed:+// At top of file with other mocks +vi.mock('better-auth/node', () => ({ + toNodeHandler: vi + .fn() + .mockImplementation((_h: any) => (_req: any, res: any) => res.end()), +})); + describe('AuthController', () => { // ... existing setup ... describe('betterAuth', () => { it('should delegate to toNodeHandler when handler is a function', async () => { const mockHandler = vi.fn(); mockAuthService.getHandler.mockReturnValue(mockHandler); - vi.mock('better-auth/node', () => ({ - toNodeHandler: vi - .fn() - .mockImplementation((_h: any) => (_req: any, res: any) => res.end()), - })); - const { toNodeHandler } = await import('better-auth/node');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/modules/identity/auth/auth.controller.spec.ts` around lines 353 - 389, The vi.mock('better-auth/node', ...) call inside the test should be moved or converted: either hoist that mock to the top-level test module alongside other mocks so it's explicit and visible to all tests, or replace the in-test vi.mock with vi.doMock('better-auth/node', ...) inside this specific test to ensure runtime-only mocking; update references to toNodeHandler import usage in the test accordingly (the mock targets better-auth/node and the controller.betterAuth test that calls mockAuthService.getHandler and uses toNodeHandler).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/package.json`:
- Line 101: The package.json dependency for Prettier uses a broad range spec
("prettier": ">=3.0.0") which can allow accidental upgrades; update the Prettier
spec in apps/api/package.json to match the resolved version in pnpm-lock.yaml
(e.g., change to "^3.7.4" or "3.7.4") so installs remain consistent with the
lockfile and prevent unintended bumps of the Prettier version.
In `@apps/api/src/common/pipes/lazy-zod-validation.pipe.spec.ts`:
- Around line 1-15: The file-level ESLint disables are too broad; narrow them
and remove the need for `@typescript-eslint/restrict-plus-operands` and
`@typescript-eslint/no-unsafe-assignment` by changing the MockZodPipe.transform
implementation to coerce the input to a string (e.g., use String(value) +
'-transformed') and then remove or limit the top-of-file eslint-disable comments
to only the specific line(s) that still require exemption (or delete them
entirely once coercion removes the errors). Update the mocked class name
MockZodPipe and its transform method in lazy-zod-validation.pipe.spec.ts
accordingly so the test no longer relies on unsafe addition.
In `@apps/api/src/modules/connections/connections/connectors.controller.spec.ts`:
- Around line 719-756: The test suite lacks a case verifying fallback to stored
credentials when both clientId and clientSecret are missing; add a new it(...)
in connectors.controller.spec.ts that mirrors the existing reconnect test but
sets reconnectBody to have clientId: undefined and clientSecret: undefined with
dataSourceId set, arrange mocks the same way (mockOauthStateService.verifyState,
mockConnectorsService.exchangeCodeForTokens, mockDb.where returning existing
slug and a stored encrypted blob, mockEncryptionService.decrypt resolving to
JSON with clientId and clientSecret), call controller.exchangeCode(mockCtx,
reconnectBody) and assert mockConnectorsService.exchangeCodeForTokens was called
using the decrypted stored clientId and stored-secret (and same other args) to
ensure full stored-credential fallback.
In `@apps/api/src/modules/stitches/stitches-metadata.controller.spec.ts`:
- Around line 316-335: Add explicit boundary tests for the 256-character object
name limit in stitches-metadata.controller.spec.ts by exercising the same
pipe.transform used in the existing tests: add one test that constructs a
256-char valid name (e.g., 'A'.repeat(256)) and asserts pipe.transform returns
it, and another test that constructs a 257-char name (e.g., 'A'.repeat(257)) and
asserts that pipe.transform(...) throws BadRequestException; reference the
existing pipe.transform calls and BadRequestException in your new tests so they
mirror the style of the current suite.
In `@apps/api/vitest.config.mts`:
- Around line 74-75: Replace the broad "// `@ts-ignore`" above the swc.vite(...)
call with a scoped "// `@ts-expect-error`" immediately on the swc.vite(...) line
(or alternatively perform a narrow type cast of the swc.vite(...) result to the
expected Vite plugin type) so the mismatch is flagged as expected rather than
silencing other TS errors; target the swc.vite symbol in the vitest
configuration and ensure the expect-error comment sits directly above that
expression or apply a specific cast to the plugin return value.
In `@apps/mock-gateway/package.json`:
- Line 14: package.json declares express at major version 4 but `@types/express`
at major version 5; update the dependency versions so their majors match (for
example change "`@types/express`" to a 4.x release) by editing
apps/mock-gateway/package.json and aligning the "`@types/express`" entry with
"express" (or vice versa) to ensure correct type-checking for Express
middleware/handlers.
In `@apps/tenant-provisioner/vitest.config.ts`:
- Around line 29-30: Replace the broad `@ts-ignore` before swc.vite(...) with a
scoped suppression: either use `@ts-expect-error` with a short reason comment
describing the Vite/unplugin-swc version mismatch, or create a small typed
wrapper/cast for swc.vite to assert the expected Plugin type and call that
instead; also consider aligning the shared Vite/Vitest/unplugin-swc versions or
centralizing this typed SWC Vite plugin wrapper so other vitest.config.* files
can reuse the same typed fix instead of repeating broad ignores.
In `@apps/web/src/test/environments/jsdom-msw.ts`:
- Line 1: Remove the file-wide eslint-disable and strongly type the
setup/teardown signatures by importing the Vitest Environment types and using
them for the params; e.g., import type { Environment } from 'vitest' and change
async setup(global: any, options: any) and teardown(global: any) to use the
Environment types (for example using Parameters<Environment['setup']>[0] and
Parameters<Environment['setup']>[1] for setup and
Parameters<Environment['teardown']>[0] for teardown) so you no longer need
`@typescript-eslint/no-explicit-any` for the file while keeping typings tied to
Vitest's Environment.
In `@apps/web/vitest.config.ts`:
- Around line 22-24: The Vitest config currently sets environment: 'jsdom' so
setupFiles (./src/test/setup-env.ts) run after JSDOM initializes, preventing
pre-JSDOM removal of native fetch/Headers/Request/Response and causing MSW
mismatches; change the Vitest environment to the custom jsdom-msw environment
(or implement/register a custom environment that deletes native global
fetch/Headers/Request/Response before JSDOM initializes) instead of 'jsdom', and
ensure setupFiles remain for other setup tasks while the jsdom-msw environment
(or custom environment initializer) performs the pre-JSDOM cleanup so MSW
intercepts the expected fetch implementation.
- Around line 10-12: The `@ts-ignore` in apps/web/vitest.config.ts hides a
dependency version skew between Vitest, Vite and `@vitejs/plugin-react`; remove
the comment and align versions so types match. Fix by updating devDependencies:
either upgrade `@vitejs/plugin-react` and/or vitest so both resolve to a Vite
major that they support (or add a pnpm/npm/Yarn override to force a single Vite
version), run install to regenerate the lockfile, then remove the "//
`@ts-ignore`" line above the react() plugin call and run typecheck/tests to verify
no type errors remain; ensure the react() plugin entry in vitest.config.ts
compiles without suppression.
In `@apps/worker/package.json`:
- Line 98: The dependency range "prettier": ">=3.0.0" is unbounded and can cause
sudden major upgrades; replace it with a pinned or semver-safe range (e.g., a
specific version "3.x.y" or a caret range like "^3.0.0") in the package.json
entries that declare "prettier" (notably the "prettier" key currently set to
">=3.0.0" in the manifests referenced) to prevent silent major bumps; update
each package.json that contains the "prettier" entry (apps/worker, apps/api,
packages/auth, packages/identity, packages/eslint-config, packages/infra,
packages/queue) to use the chosen pinned/semver range and run install/lockfile
update so the lockfile reflects the new constraint.
In `@apps/worker/vitest.config.mts`:
- Line 68: Replace the inline TypeScript suppression comment "// `@ts-ignore` -
Type mismatch between vitest/config vite version and local vite plugin version"
with "// `@ts-expect-error` - Type mismatch between vitest/config vite version and
local vite plugin version" so TypeScript will surface the suppression if the
type mismatch is later resolved; update the single-line comment in the
vitest.config.mts configuration block where that suppression appears and keep
the explanatory text unchanged.
In `@packages/identity/package.json`:
- Around line 33-35: Update the dependency ranges in
packages/identity/package.json so `@nestjs/core` matches `@nestjs/common` (e.g.,
change the `@nestjs/core` version spec to the same caret range as `@nestjs/common`
such as ^11.1.11) in both peerDependencies and devDependencies; search for the
dependency keys "`@nestjs/common`" and "`@nestjs/core`" in the file and make them
identical to prevent future skew (also apply the same alignment pattern to other
packages like packages/queue and packages/infra if present).
In `@packages/identity/src/adapters/better-auth.adapter.ts`:
- Around line 184-186: Replace the raw console.error in the catch block that
currently reads console.error("VERIFY ERR", e) with a safe, redacting log:
remove sensitive fields (e.g., recipient, token, verificationLink, any headers)
from the error object and only log non-sensitive metadata such as error.name and
a truncated error.message, or call a small helper like sanitizeError(error)
before logging; use the module's logger (not console) if available (e.g.,
processLogger) and then rethrow the original error. Locate the catch containing
console.error("VERIFY ERR", e) in better-auth.adapter.ts and implement either a
sanitizeError function or explicit deletion of sensitive properties before
logging to avoid emitting tokenized links or recipient data.
In `@packages/infra/vitest.config.ts`:
- Around line 27-28: Replace the broad TypeScript suppression on the SWC plugin
call by changing the comment above the swc.vite({ module: { type: 'es6' } })
invocation from // `@ts-ignore` to // `@ts-expect-error` so only the known
vitest/vite vs SWC typing mismatch is suppressed (match the pattern used in
packages/identity/vitest.config.ts).
---
Outside diff comments:
In `@apps/api/src/modules/identity/auth/auth.controller.spec.ts`:
- Around line 353-389: The vi.mock('better-auth/node', ...) call inside the test
should be moved or converted: either hoist that mock to the top-level test
module alongside other mocks so it's explicit and visible to all tests, or
replace the in-test vi.mock with vi.doMock('better-auth/node', ...) inside this
specific test to ensure runtime-only mocking; update references to toNodeHandler
import usage in the test accordingly (the mock targets better-auth/node and the
controller.betterAuth test that calls mockAuthService.getHandler and uses
toNodeHandler).
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 36a137bd-11c3-4b64-a92b-aa9fb5a64673
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (66)
apps/api/package.jsonapps/api/src/common/pipes/lazy-zod-validation.pipe.spec.tsapps/api/src/constants.spec.tsapps/api/src/core/shutdown.service.spec.tsapps/api/src/db/database-manager.spec.tsapps/api/src/modules/connections/connections/connectors.controller.spec.tsapps/api/src/modules/identity/auth/auth.controller.spec.tsapps/api/src/modules/identity/roles/roles.controller.spec.tsapps/api/src/modules/scheduler/connection-sync-runner.spec.tsapps/api/src/modules/scheduler/connection-sync-runner.tsapps/api/src/modules/stitches/stitches-metadata.controller.spec.tsapps/api/src/modules/stitches/stitches-metadata.controller.tsapps/api/src/modules/trace/data-explorer.service.spec.tsapps/api/src/modules/trigger/trigger-executor.service.spec.tsapps/api/src/modules/workspaces/workspace-connections.controller.spec.tsapps/api/test/vitest-e2e.config.mtsapps/api/tsconfig.jsonapps/api/vitest.config.mtsapps/mock-gateway/package.jsonapps/tenant-provisioner/package.jsonapps/tenant-provisioner/src/modules/provisioner/tenant-provision.worker.spec.tsapps/tenant-provisioner/tsconfig.jsonapps/tenant-provisioner/vitest.config.tsapps/web/package.jsonapps/web/src/test/environments/jsdom-msw.tsapps/web/tsconfig.app.jsonapps/web/vitest.config.tsapps/worker/package.jsonapps/worker/src/db/database-manager.spec.tsapps/worker/tsconfig.jsonapps/worker/vitest.config.mtsengine/ai/core/package.jsonengine/ai/core/src/tools/tool-zod.wrapper.tsengine/sync/application/mapping/package.jsonengine/sync/application/salesforce/revenova/package.jsonengine/sync/platform/core/package.jsonpackage.jsonpackages/auth/package.jsonpackages/auth/tsconfig.build.jsonpackages/cache/package.jsonpackages/credentials/package.jsonpackages/database/package.jsonpackages/dbmanager/package.jsonpackages/domain/tms/package.jsonpackages/eslint-config/package.jsonpackages/identity/package.jsonpackages/identity/src/adapters/better-auth.adapter.spec.tspackages/identity/src/adapters/better-auth.adapter.tspackages/identity/src/adapters/drizzle-tenant.adapter.spec.tspackages/identity/tsconfig.jsonpackages/identity/vitest.config.tspackages/infra/package.jsonpackages/infra/vitest.config.tspackages/mcp/package.jsonpackages/metadata-engine/package.jsonpackages/pieces/application/salesforce/revenova/package.jsonpackages/pieces/platform/framework/package.jsonpackages/pieces/platform/quickbooks/package.jsonpackages/pieces/platform/registry/package.jsonpackages/pieces/platform/registry/src/pieces/execution-worker.service.spec.tspackages/pieces/platform/registry/src/pieces/plugin-manager.service.spec.tspackages/pieces/platform/registry/vitest.config.mtspackages/pieces/platform/salesforce/package.jsonpackages/queue/package.jsonpackages/queue/vitest.config.tspackages/transformer/package.json
💤 Files with no reviewable changes (3)
- engine/ai/core/src/tools/tool-zod.wrapper.ts
- apps/api/tsconfig.json
- apps/worker/tsconfig.json
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 19 file(s) based on 15 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 19 file(s) based on 15 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
packages/identity/src/adapters/better-auth.adapter.ts (1)
185-188:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRedact verification error messages before logging.
Line 186 still logs raw provider error text (even truncated), which can expose recipient/token/link data in auth logs.
Suggested minimal fix
- } catch (e) { - const errorName = e instanceof Error ? e.name : 'UnknownError'; - const errorMessage = e instanceof Error ? e.message.substring(0, 100) : 'Unknown error'; - console.error("Email verification error:", { errorName, errorMessage }); - throw e; + } catch (e) { + const errorName = e instanceof Error ? e.name : "UnknownError"; + // Do not log raw message from downstream email providers in auth flows. + console.error("Email verification error:", { + errorName, + errorMessage: "verification send failed", + }); + throw e; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/identity/src/adapters/better-auth.adapter.ts` around lines 185 - 188, The catch block that currently builds errorName/errorMessage and calls console.error("Email verification error:", { errorName, errorMessage }) should stop logging raw/truncated provider messages; in the error handling inside the verification flow (the catch that creates errorName/errorMessage in better-auth.adapter.ts) replace errorMessage with a redacted or generic string (e.g., 'REDACTED' or 'Provider error') and remove any use of e.message or other potentially sensitive fields when constructing log data—keep errorName (or map to a safe enum) and a generic message, log only non-sensitive fields, then rethrow the original error. Ensure the symbols referenced (errorName, errorMessage and the console.error call) are updated accordingly.apps/web/src/test/environments/jsdom-msw.ts (1)
15-15:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winReplace
as anycast to fix pipeline failure.Line 15 uses
global as any, which violates@typescript-eslint/no-explicit-anyand is currently causing the pipeline to fail. Sinceglobalis already typed asParameters<Environment['setup']>[0], cast it toRecord<string, unknown>instead to safely delete the fetch-related properties.🛠️ Proposed fix
- const globalAny = global as any; + const globalAny = global as Record<string, unknown>;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/test/environments/jsdom-msw.ts` at line 15, Replace the explicit any cast used for the global helper by casting to Record<string, unknown> instead of any: change the declaration of the helper variable (currently "const globalAny = global as any") to cast to Record<string, unknown> so you can still delete the fetch-related properties (fetch, Headers, Request, Response) safely; update uses of globalAny in the teardown logic (the same variable name) to delete those properties without using any.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@apps/web/src/test/environments/jsdom-msw.ts`:
- Line 15: Replace the explicit any cast used for the global helper by casting
to Record<string, unknown> instead of any: change the declaration of the helper
variable (currently "const globalAny = global as any") to cast to Record<string,
unknown> so you can still delete the fetch-related properties (fetch, Headers,
Request, Response) safely; update uses of globalAny in the teardown logic (the
same variable name) to delete those properties without using any.
In `@packages/identity/src/adapters/better-auth.adapter.ts`:
- Around line 185-188: The catch block that currently builds
errorName/errorMessage and calls console.error("Email verification error:", {
errorName, errorMessage }) should stop logging raw/truncated provider messages;
in the error handling inside the verification flow (the catch that creates
errorName/errorMessage in better-auth.adapter.ts) replace errorMessage with a
redacted or generic string (e.g., 'REDACTED' or 'Provider error') and remove any
use of e.message or other potentially sensitive fields when constructing log
data—keep errorName (or map to a safe enum) and a generic message, log only
non-sensitive fields, then rethrow the original error. Ensure the symbols
referenced (errorName, errorMessage and the console.error call) are updated
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5a70dd52-9112-4b46-b757-1dbbc3eb6ca5
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (18)
apps/api/package.jsonapps/api/src/common/pipes/lazy-zod-validation.pipe.spec.tsapps/api/src/modules/connections/connections/connectors.controller.spec.tsapps/api/src/modules/stitches/stitches-metadata.controller.spec.tsapps/api/vitest.config.mtsapps/mock-gateway/package.jsonapps/tenant-provisioner/vitest.config.tsapps/web/src/test/environments/jsdom-msw.tsapps/web/vitest.config.tsapps/worker/package.jsonapps/worker/vitest.config.mtspackages/auth/package.jsonpackages/eslint-config/package.jsonpackages/identity/package.jsonpackages/identity/src/adapters/better-auth.adapter.tspackages/infra/package.jsonpackages/infra/vitest.config.tspackages/queue/package.json
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
Chores