chore(shared): Migrate tests to vitest#7014
Conversation
🦋 Changeset detectedLatest commit: 5fa2f97 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughRemoved Jest-based test infrastructure and migrated the package tests in Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant CI as CI / Local runner
participant Vitest as Vitest
participant Setup as `packages/shared/vitest.setup.mts`
participant Tests as test files
Dev->>CI: run `pnpm -w --filter shared test` (now Vitest)
CI->>Vitest: start runner
Vitest->>Setup: load setup file
Setup->>Vitest: initialize globalThis.crypto & JS_PACKAGE_VERSION
Vitest->>Tests: execute test suites (uses vi APIs)
Note right of Tests #DDEEFF: Many tests migrated from Jest to Vitest\n(Some with mocks/timers using `vi`)
Tests-->>Vitest: report results
Vitest-->>CI: exit code / summary
Note: Deleted Jest-specific environment/config files mean no JSDOM custom env is loaded in this flow. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (3)
packages/shared/src/utils/__tests__/createDeferredPromise.spec.ts (1)
14-23: Test could pass even if promise doesn't reject.The try-catch block lacks an assertion failure path. If the promise resolves instead of rejecting, the test will pass silently because no error is caught and no assertion fails.
Apply this diff to ensure the test fails when the promise doesn't reject:
test('rejects with correct error', async () => { const { promise, reject } = createDeferredPromise(); const expectedError = new Error('something went wrong'); reject(expectedError); - try { - await promise; - } catch (error) { - expect(error).toBe(expectedError); - } + await expect(promise).rejects.toBe(expectedError); });packages/shared/src/__tests__/jwtPayloadParser.spec.ts (1)
116-116: Fix typo in test description.The test description contains a typo: "arrat" should be "array".
Apply this diff to fix the typo:
- test('if there is no o.fpm and o.per org permissions should be empty arrat', () => { + test('if there is no o.fpm and o.per org permissions should be empty array', () => {packages/shared/src/__tests__/loadClerkJsScript.spec.ts (1)
142-142: Replacefail()with Vitest-compatible assertion.The
fail()function is not available in Vitest. This will cause a runtime error.Apply this diff to use a Vitest-compatible approach:
- fail('Should have thrown error'); + throw new Error('Should have thrown error');Alternatively, you can restructure the test to avoid the try-catch and use
expect().rejects:- try { - await loadPromise; - fail('Should have thrown error'); - } catch (error) { - expect(error).toBeInstanceOf(ClerkRuntimeError); - expect((error as Error).message).toContain('Clerk: Failed to load Clerk'); - // The malformed Clerk object should still be there since it was set - expect((window as any).Clerk).toEqual({ status: 'ready' }); - } + await expect(loadPromise).rejects.toThrow(ClerkRuntimeError); + await expect(loadPromise).rejects.toThrow('Clerk: Failed to load Clerk'); + // The malformed Clerk object should still be there since it was set + expect((window as any).Clerk).toEqual({ status: 'ready' });
🧹 Nitpick comments (6)
packages/shared/src/__tests__/retry.spec.ts (1)
40-67: Consider removing redundant timer calls.Lines 41 and 66 contain redundant
vi.useFakeTimers()andvi.useRealTimers()calls, as these are already handled by thebeforeEachandafterEachhooks. While not harmful, removing them improves clarity and reduces duplication.Apply this diff to remove the redundant calls:
test('maxDelayBetweenRetries prevents delays from growing beyond the limit', async () => { - vi.useFakeTimers(); let attempts = 0; retry( () => { attempts++; throw new Error('failed'); }, { maxDelayBetweenRetries: 300, initialDelay: 100, factor: 3, jitter: false, shouldRetry: (_, count) => count <= 4, }, ).catch(e => { expect(e.message).toBe('failed'); }); // Run all timer advances before testing the promise await vi.advanceTimersByTimeAsync(100); await vi.advanceTimersByTimeAsync(300); await vi.advanceTimersByTimeAsync(300); await vi.advanceTimersByTimeAsync(300); expect(attempts).toBe(1 + 4); - vi.useRealTimers(); });packages/shared/src/__tests__/devbrowser.spec.ts (1)
40-42: Consider using a more specific type instead ofas any.While the current
as anyassertion works, you could improve type safety by using a more specific type:- const mockHistory = { + const mockHistory: Pick<History, 'replaceState'> = { replaceState: replaceStateMock, - } as any; + };This aligns with the coding guideline to avoid
anytypes and implement proper mock types that match interfaces.As per coding guidelines.
packages/shared/src/router/__tests__/router.spec.ts (1)
9-13: Mock migration is correct.The migration from
jest.fn()tovi.fn()is properly done and preserves the test functionality.For improved type safety, consider adding explicit type parameters to the mock functions:
const mockRouter = { name: 'mockRouter', mode: 'path' as const, - pathname: vi.fn(), - searchParams: vi.fn(), - push: vi.fn(), - shallowPush: vi.fn(), - replace: vi.fn(), + pathname: vi.fn<[], string>(), + searchParams: vi.fn<[], URLSearchParams>(), + push: vi.fn<[string], void>(), + shallowPush: vi.fn<[string], void>(), + replace: vi.fn<[string], void>(), };This makes the mock types explicit and helps catch type errors at compile time.
packages/shared/src/__tests__/keys.spec.ts (1)
48-51: Optional: Consider improving type safety in parameterized tests.The
@ts-ignorecomments at lines 48, 50, and 187 suppress TypeScript errors in parameterized tests. While these pre-date the Vitest migration, consider properly typing the test cases to improve type safety:// Example for lines 48-51 const cases: Array<[string | null | undefined, ReturnType<typeof parsePublishableKey>]> = [ // ... cases ]; test.each(cases)('given %p as a publishable key string, returns %p', (publishableKeyStr, expectedPublishableKey) => { const result = parsePublishableKey(publishableKeyStr); expect(result).toEqual(expectedPublishableKey); });Similarly, for line 187, properly type the mixed array to avoid suppressing TypeScript errors.
Also applies to: 187-187
packages/shared/src/react/__tests__/useReverification.spec.ts (1)
57-61: Consider removing unused variable.The
mockFetcherInnervariable is declared and cleared inbeforeEach, but it's never used in any test. This appears to be dead code.Apply this diff to remove the unused variable:
- const mockFetcherInner = vi.fn().mockResolvedValue({ ok: true }); - beforeEach(() => { - mockFetcherInner.mockClear(); });packages/shared/src/__tests__/telemetry.logs.spec.ts (1)
12-17: Consider using proper Vitest spy types instead ofany.The type annotations have been changed from Jest-specific types to
any, which removes type safety. Consider importing and using Vitest'sSpyInstancetype:+import type { SpyInstance } from 'vitest'; + describe('TelemetryCollector.recordLog', () => { - let fetchSpy: any; - let windowSpy: any; + let fetchSpy: SpyInstance; + let windowSpy: SpyInstance;This maintains type safety while completing the migration to Vitest.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (37)
packages/shared/customJSDOMEnvironment.ts(0 hunks)packages/shared/jest.config.js(0 hunks)packages/shared/jest.setup.ts(0 hunks)packages/shared/package.json(1 hunks)packages/shared/src/__tests__/apiUrlFromPublishableKey.spec.ts(1 hunks)packages/shared/src/__tests__/browser.spec.ts(3 hunks)packages/shared/src/__tests__/buildAccountsBaseUrl.spec.ts(1 hunks)packages/shared/src/__tests__/color.spec.ts(1 hunks)packages/shared/src/__tests__/date.spec.ts(1 hunks)packages/shared/src/__tests__/deprecated.spec.ts(9 hunks)packages/shared/src/__tests__/deriveState.spec.ts(1 hunks)packages/shared/src/__tests__/devbrowser.spec.ts(2 hunks)packages/shared/src/__tests__/error.spec.ts(1 hunks)packages/shared/src/__tests__/eventBus.spec.ts(17 hunks)packages/shared/src/__tests__/fastDeepMerge.spec.ts(1 hunks)packages/shared/src/__tests__/handleValueOrFn.spec.ts(1 hunks)packages/shared/src/__tests__/jwtPayloadParser.spec.ts(1 hunks)packages/shared/src/__tests__/keys.spec.ts(1 hunks)packages/shared/src/__tests__/loadClerkJsScript.spec.ts(8 hunks)packages/shared/src/__tests__/localStorageBroadcastChannel.spec.ts(3 hunks)packages/shared/src/__tests__/logger.spec.ts(2 hunks)packages/shared/src/__tests__/netlifyCacheHandler.spec.ts(1 hunks)packages/shared/src/__tests__/organization.spec.ts(1 hunks)packages/shared/src/__tests__/pathMatcher.spec.ts(1 hunks)packages/shared/src/__tests__/proxy.spec.ts(1 hunks)packages/shared/src/__tests__/retry.spec.ts(8 hunks)packages/shared/src/__tests__/telemetry.logs.spec.ts(11 hunks)packages/shared/src/__tests__/telemetry.test.ts(0 hunks)packages/shared/src/__tests__/underscore.spec.ts(1 hunks)packages/shared/src/__tests__/url.spec.ts(1 hunks)packages/shared/src/__tests__/versionSelector.spec.ts(1 hunks)packages/shared/src/react/__tests__/useReverification.spec.ts(3 hunks)packages/shared/src/router/__tests__/router.spec.ts(1 hunks)packages/shared/src/telemetry/events/__tests__/theme-usage.spec.ts(1 hunks)packages/shared/src/utils/__tests__/createDeferredPromise.spec.ts(1 hunks)packages/shared/src/utils/__tests__/instance.spec.ts(1 hunks)packages/shared/vitest.setup.mts(1 hunks)
💤 Files with no reviewable changes (4)
- packages/shared/jest.setup.ts
- packages/shared/jest.config.js
- packages/shared/src/tests/telemetry.test.ts
- packages/shared/customJSDOMEnvironment.ts
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
**/*.{js,jsx,ts,tsx}: All code must pass ESLint checks with the project's configuration
Follow established naming conventions (PascalCase for components, camelCase for variables)
Maintain comprehensive JSDoc comments for public APIs
Use dynamic imports for optional features
All public APIs must be documented with JSDoc
Provide meaningful error messages to developers
Include error recovery suggestions where applicable
Log errors appropriately for debugging
Lazy load components and features when possible
Implement proper caching strategies
Use efficient data structures and algorithms
Profile and optimize critical paths
Validate all inputs and sanitize outputs
Implement proper logging with different levels
Files:
packages/shared/src/__tests__/error.spec.tspackages/shared/src/__tests__/netlifyCacheHandler.spec.tspackages/shared/src/__tests__/eventBus.spec.tspackages/shared/src/__tests__/fastDeepMerge.spec.tspackages/shared/src/__tests__/telemetry.logs.spec.tspackages/shared/src/__tests__/browser.spec.tspackages/shared/src/__tests__/proxy.spec.tspackages/shared/src/__tests__/keys.spec.tspackages/shared/src/__tests__/localStorageBroadcastChannel.spec.tspackages/shared/src/__tests__/organization.spec.tspackages/shared/src/__tests__/loadClerkJsScript.spec.tspackages/shared/src/__tests__/retry.spec.tspackages/shared/src/utils/__tests__/createDeferredPromise.spec.tspackages/shared/src/__tests__/color.spec.tspackages/shared/src/__tests__/logger.spec.tspackages/shared/src/__tests__/buildAccountsBaseUrl.spec.tspackages/shared/src/router/__tests__/router.spec.tspackages/shared/src/telemetry/events/__tests__/theme-usage.spec.tspackages/shared/src/utils/__tests__/instance.spec.tspackages/shared/src/__tests__/underscore.spec.tspackages/shared/src/__tests__/versionSelector.spec.tspackages/shared/src/__tests__/pathMatcher.spec.tspackages/shared/src/__tests__/apiUrlFromPublishableKey.spec.tspackages/shared/src/__tests__/jwtPayloadParser.spec.tspackages/shared/src/__tests__/date.spec.tspackages/shared/src/__tests__/devbrowser.spec.tspackages/shared/src/__tests__/url.spec.tspackages/shared/src/__tests__/deriveState.spec.tspackages/shared/src/react/__tests__/useReverification.spec.tspackages/shared/src/__tests__/handleValueOrFn.spec.tspackages/shared/src/__tests__/deprecated.spec.ts
**/*.{js,jsx,ts,tsx,json,css,scss,md,yaml,yml}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use Prettier for consistent code formatting
Files:
packages/shared/src/__tests__/error.spec.tspackages/shared/src/__tests__/netlifyCacheHandler.spec.tspackages/shared/src/__tests__/eventBus.spec.tspackages/shared/src/__tests__/fastDeepMerge.spec.tspackages/shared/src/__tests__/telemetry.logs.spec.tspackages/shared/src/__tests__/browser.spec.tspackages/shared/src/__tests__/proxy.spec.tspackages/shared/src/__tests__/keys.spec.tspackages/shared/src/__tests__/localStorageBroadcastChannel.spec.tspackages/shared/src/__tests__/organization.spec.tspackages/shared/src/__tests__/loadClerkJsScript.spec.tspackages/shared/src/__tests__/retry.spec.tspackages/shared/src/utils/__tests__/createDeferredPromise.spec.tspackages/shared/src/__tests__/color.spec.tspackages/shared/package.jsonpackages/shared/src/__tests__/logger.spec.tspackages/shared/src/__tests__/buildAccountsBaseUrl.spec.tspackages/shared/src/router/__tests__/router.spec.tspackages/shared/src/telemetry/events/__tests__/theme-usage.spec.tspackages/shared/src/utils/__tests__/instance.spec.tspackages/shared/src/__tests__/underscore.spec.tspackages/shared/src/__tests__/versionSelector.spec.tspackages/shared/src/__tests__/pathMatcher.spec.tspackages/shared/src/__tests__/apiUrlFromPublishableKey.spec.tspackages/shared/src/__tests__/jwtPayloadParser.spec.tspackages/shared/src/__tests__/date.spec.tspackages/shared/src/__tests__/devbrowser.spec.tspackages/shared/src/__tests__/url.spec.tspackages/shared/src/__tests__/deriveState.spec.tspackages/shared/src/react/__tests__/useReverification.spec.tspackages/shared/src/__tests__/handleValueOrFn.spec.tspackages/shared/src/__tests__/deprecated.spec.ts
packages/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
TypeScript is required for all packages
Files:
packages/shared/src/__tests__/error.spec.tspackages/shared/src/__tests__/netlifyCacheHandler.spec.tspackages/shared/src/__tests__/eventBus.spec.tspackages/shared/src/__tests__/fastDeepMerge.spec.tspackages/shared/src/__tests__/telemetry.logs.spec.tspackages/shared/src/__tests__/browser.spec.tspackages/shared/src/__tests__/proxy.spec.tspackages/shared/src/__tests__/keys.spec.tspackages/shared/src/__tests__/localStorageBroadcastChannel.spec.tspackages/shared/src/__tests__/organization.spec.tspackages/shared/src/__tests__/loadClerkJsScript.spec.tspackages/shared/src/__tests__/retry.spec.tspackages/shared/src/utils/__tests__/createDeferredPromise.spec.tspackages/shared/src/__tests__/color.spec.tspackages/shared/src/__tests__/logger.spec.tspackages/shared/src/__tests__/buildAccountsBaseUrl.spec.tspackages/shared/src/router/__tests__/router.spec.tspackages/shared/src/telemetry/events/__tests__/theme-usage.spec.tspackages/shared/src/utils/__tests__/instance.spec.tspackages/shared/src/__tests__/underscore.spec.tspackages/shared/src/__tests__/versionSelector.spec.tspackages/shared/src/__tests__/pathMatcher.spec.tspackages/shared/src/__tests__/apiUrlFromPublishableKey.spec.tspackages/shared/src/__tests__/jwtPayloadParser.spec.tspackages/shared/src/__tests__/date.spec.tspackages/shared/src/__tests__/devbrowser.spec.tspackages/shared/src/__tests__/url.spec.tspackages/shared/src/__tests__/deriveState.spec.tspackages/shared/src/react/__tests__/useReverification.spec.tspackages/shared/src/__tests__/handleValueOrFn.spec.tspackages/shared/src/__tests__/deprecated.spec.ts
packages/**/*.{ts,tsx,d.ts}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Packages should export TypeScript types alongside runtime code
Files:
packages/shared/src/__tests__/error.spec.tspackages/shared/src/__tests__/netlifyCacheHandler.spec.tspackages/shared/src/__tests__/eventBus.spec.tspackages/shared/src/__tests__/fastDeepMerge.spec.tspackages/shared/src/__tests__/telemetry.logs.spec.tspackages/shared/src/__tests__/browser.spec.tspackages/shared/src/__tests__/proxy.spec.tspackages/shared/src/__tests__/keys.spec.tspackages/shared/src/__tests__/localStorageBroadcastChannel.spec.tspackages/shared/src/__tests__/organization.spec.tspackages/shared/src/__tests__/loadClerkJsScript.spec.tspackages/shared/src/__tests__/retry.spec.tspackages/shared/src/utils/__tests__/createDeferredPromise.spec.tspackages/shared/src/__tests__/color.spec.tspackages/shared/src/__tests__/logger.spec.tspackages/shared/src/__tests__/buildAccountsBaseUrl.spec.tspackages/shared/src/router/__tests__/router.spec.tspackages/shared/src/telemetry/events/__tests__/theme-usage.spec.tspackages/shared/src/utils/__tests__/instance.spec.tspackages/shared/src/__tests__/underscore.spec.tspackages/shared/src/__tests__/versionSelector.spec.tspackages/shared/src/__tests__/pathMatcher.spec.tspackages/shared/src/__tests__/apiUrlFromPublishableKey.spec.tspackages/shared/src/__tests__/jwtPayloadParser.spec.tspackages/shared/src/__tests__/date.spec.tspackages/shared/src/__tests__/devbrowser.spec.tspackages/shared/src/__tests__/url.spec.tspackages/shared/src/__tests__/deriveState.spec.tspackages/shared/src/react/__tests__/useReverification.spec.tspackages/shared/src/__tests__/handleValueOrFn.spec.tspackages/shared/src/__tests__/deprecated.spec.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use proper TypeScript error types
**/*.{ts,tsx}: Always define explicit return types for functions, especially public APIs
Use proper type annotations for variables and parameters where inference isn't clear
Avoidanytype - preferunknownwhen type is uncertain, then narrow with type guards
Useinterfacefor object shapes that might be extended
Usetypefor unions, primitives, and computed types
Preferreadonlyproperties for immutable data structures
Useprivatefor internal implementation details
Useprotectedfor inheritance hierarchies
Usepublicexplicitly for clarity in public APIs
Preferreadonlyfor properties that shouldn't change after construction
Prefer composition and interfaces over deep inheritance chains
Use mixins for shared behavior across unrelated classes
Implement dependency injection for loose coupling
Let TypeScript infer when types are obvious
Useconst assertionsfor literal types:as const
Usesatisfiesoperator for type checking without widening
Use mapped types for transforming object types
Use conditional types for type-level logic
Leverage template literal types for string manipulation
Use ES6 imports/exports consistently
Use default exports sparingly, prefer named exports
Use type-only imports:import type { ... } from ...
Noanytypes without justification
Proper error handling with typed errors
Consistent use ofreadonlyfor immutable data
Proper generic constraints
No unused type parameters
Proper use of utility types instead of manual type construction
Type-only imports where possible
Proper tree-shaking friendly exports
No circular dependencies
Efficient type computations (avoid deep recursion)
Files:
packages/shared/src/__tests__/error.spec.tspackages/shared/src/__tests__/netlifyCacheHandler.spec.tspackages/shared/src/__tests__/eventBus.spec.tspackages/shared/src/__tests__/fastDeepMerge.spec.tspackages/shared/src/__tests__/telemetry.logs.spec.tspackages/shared/src/__tests__/browser.spec.tspackages/shared/src/__tests__/proxy.spec.tspackages/shared/src/__tests__/keys.spec.tspackages/shared/src/__tests__/localStorageBroadcastChannel.spec.tspackages/shared/src/__tests__/organization.spec.tspackages/shared/src/__tests__/loadClerkJsScript.spec.tspackages/shared/src/__tests__/retry.spec.tspackages/shared/src/utils/__tests__/createDeferredPromise.spec.tspackages/shared/src/__tests__/color.spec.tspackages/shared/src/__tests__/logger.spec.tspackages/shared/src/__tests__/buildAccountsBaseUrl.spec.tspackages/shared/src/router/__tests__/router.spec.tspackages/shared/src/telemetry/events/__tests__/theme-usage.spec.tspackages/shared/src/utils/__tests__/instance.spec.tspackages/shared/src/__tests__/underscore.spec.tspackages/shared/src/__tests__/versionSelector.spec.tspackages/shared/src/__tests__/pathMatcher.spec.tspackages/shared/src/__tests__/apiUrlFromPublishableKey.spec.tspackages/shared/src/__tests__/jwtPayloadParser.spec.tspackages/shared/src/__tests__/date.spec.tspackages/shared/src/__tests__/devbrowser.spec.tspackages/shared/src/__tests__/url.spec.tspackages/shared/src/__tests__/deriveState.spec.tspackages/shared/src/react/__tests__/useReverification.spec.tspackages/shared/src/__tests__/handleValueOrFn.spec.tspackages/shared/src/__tests__/deprecated.spec.ts
packages/**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/monorepo.mdc)
Unit tests should use Jest or Vitest as the test runner.
Files:
packages/shared/src/__tests__/error.spec.tspackages/shared/src/__tests__/netlifyCacheHandler.spec.tspackages/shared/src/__tests__/eventBus.spec.tspackages/shared/src/__tests__/fastDeepMerge.spec.tspackages/shared/src/__tests__/telemetry.logs.spec.tspackages/shared/src/__tests__/browser.spec.tspackages/shared/src/__tests__/proxy.spec.tspackages/shared/src/__tests__/keys.spec.tspackages/shared/src/__tests__/localStorageBroadcastChannel.spec.tspackages/shared/src/__tests__/organization.spec.tspackages/shared/src/__tests__/loadClerkJsScript.spec.tspackages/shared/src/__tests__/retry.spec.tspackages/shared/src/utils/__tests__/createDeferredPromise.spec.tspackages/shared/src/__tests__/color.spec.tspackages/shared/src/__tests__/logger.spec.tspackages/shared/src/__tests__/buildAccountsBaseUrl.spec.tspackages/shared/src/router/__tests__/router.spec.tspackages/shared/src/telemetry/events/__tests__/theme-usage.spec.tspackages/shared/src/utils/__tests__/instance.spec.tspackages/shared/src/__tests__/underscore.spec.tspackages/shared/src/__tests__/versionSelector.spec.tspackages/shared/src/__tests__/pathMatcher.spec.tspackages/shared/src/__tests__/apiUrlFromPublishableKey.spec.tspackages/shared/src/__tests__/jwtPayloadParser.spec.tspackages/shared/src/__tests__/date.spec.tspackages/shared/src/__tests__/devbrowser.spec.tspackages/shared/src/__tests__/url.spec.tspackages/shared/src/__tests__/deriveState.spec.tspackages/shared/src/react/__tests__/useReverification.spec.tspackages/shared/src/__tests__/handleValueOrFn.spec.tspackages/shared/src/__tests__/deprecated.spec.ts
**/*.{js,ts,tsx,jsx}
📄 CodeRabbit inference engine (.cursor/rules/monorepo.mdc)
Support multiple Clerk environment variables (CLERK_, NEXT_PUBLIC_CLERK_, etc.) for configuration.
Files:
packages/shared/src/__tests__/error.spec.tspackages/shared/src/__tests__/netlifyCacheHandler.spec.tspackages/shared/src/__tests__/eventBus.spec.tspackages/shared/src/__tests__/fastDeepMerge.spec.tspackages/shared/src/__tests__/telemetry.logs.spec.tspackages/shared/src/__tests__/browser.spec.tspackages/shared/src/__tests__/proxy.spec.tspackages/shared/src/__tests__/keys.spec.tspackages/shared/src/__tests__/localStorageBroadcastChannel.spec.tspackages/shared/src/__tests__/organization.spec.tspackages/shared/src/__tests__/loadClerkJsScript.spec.tspackages/shared/src/__tests__/retry.spec.tspackages/shared/src/utils/__tests__/createDeferredPromise.spec.tspackages/shared/src/__tests__/color.spec.tspackages/shared/src/__tests__/logger.spec.tspackages/shared/src/__tests__/buildAccountsBaseUrl.spec.tspackages/shared/src/router/__tests__/router.spec.tspackages/shared/src/telemetry/events/__tests__/theme-usage.spec.tspackages/shared/src/utils/__tests__/instance.spec.tspackages/shared/src/__tests__/underscore.spec.tspackages/shared/src/__tests__/versionSelector.spec.tspackages/shared/src/__tests__/pathMatcher.spec.tspackages/shared/src/__tests__/apiUrlFromPublishableKey.spec.tspackages/shared/src/__tests__/jwtPayloadParser.spec.tspackages/shared/src/__tests__/date.spec.tspackages/shared/src/__tests__/devbrowser.spec.tspackages/shared/src/__tests__/url.spec.tspackages/shared/src/__tests__/deriveState.spec.tspackages/shared/src/react/__tests__/useReverification.spec.tspackages/shared/src/__tests__/handleValueOrFn.spec.tspackages/shared/src/__tests__/deprecated.spec.ts
**/__tests__/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/typescript.mdc)
**/__tests__/**/*.{ts,tsx}: Create type-safe test builders/factories
Use branded types for test isolation
Implement proper mock types that match interfaces
Files:
packages/shared/src/__tests__/error.spec.tspackages/shared/src/__tests__/netlifyCacheHandler.spec.tspackages/shared/src/__tests__/eventBus.spec.tspackages/shared/src/__tests__/fastDeepMerge.spec.tspackages/shared/src/__tests__/telemetry.logs.spec.tspackages/shared/src/__tests__/browser.spec.tspackages/shared/src/__tests__/proxy.spec.tspackages/shared/src/__tests__/keys.spec.tspackages/shared/src/__tests__/localStorageBroadcastChannel.spec.tspackages/shared/src/__tests__/organization.spec.tspackages/shared/src/__tests__/loadClerkJsScript.spec.tspackages/shared/src/__tests__/retry.spec.tspackages/shared/src/utils/__tests__/createDeferredPromise.spec.tspackages/shared/src/__tests__/color.spec.tspackages/shared/src/__tests__/logger.spec.tspackages/shared/src/__tests__/buildAccountsBaseUrl.spec.tspackages/shared/src/router/__tests__/router.spec.tspackages/shared/src/telemetry/events/__tests__/theme-usage.spec.tspackages/shared/src/utils/__tests__/instance.spec.tspackages/shared/src/__tests__/underscore.spec.tspackages/shared/src/__tests__/versionSelector.spec.tspackages/shared/src/__tests__/pathMatcher.spec.tspackages/shared/src/__tests__/apiUrlFromPublishableKey.spec.tspackages/shared/src/__tests__/jwtPayloadParser.spec.tspackages/shared/src/__tests__/date.spec.tspackages/shared/src/__tests__/devbrowser.spec.tspackages/shared/src/__tests__/url.spec.tspackages/shared/src/__tests__/deriveState.spec.tspackages/shared/src/react/__tests__/useReverification.spec.tspackages/shared/src/__tests__/handleValueOrFn.spec.tspackages/shared/src/__tests__/deprecated.spec.ts
packages/*/package.json
📄 CodeRabbit inference engine (.cursor/rules/global.mdc)
All publishable packages should be placed under the packages/ directory
packages/*/package.json: All publishable packages must be located in the 'packages/' directory.
All packages must be published under the @clerk namespace on npm.
Semantic versioning must be used across all packages.
Files:
packages/shared/package.json
🧬 Code graph analysis (3)
packages/shared/src/__tests__/telemetry.logs.spec.ts (1)
packages/shared/src/telemetry/collector.ts (1)
window(280-318)
packages/shared/src/__tests__/browser.spec.ts (1)
packages/clerk-js/src/utils/runtime.ts (1)
inBrowser(1-3)
packages/shared/src/__tests__/loadClerkJsScript.spec.ts (1)
packages/shared/src/loadClerkJsScript.ts (1)
setClerkJsLoadingErrorPackageName(27-29)
🪛 ast-grep (0.39.6)
packages/shared/src/__tests__/pathMatcher.spec.ts
[warning] 5-5: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${pattern.replace('(.*)', '.*')}$)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🪛 Gitleaks (8.28.0)
packages/shared/src/__tests__/loadClerkJsScript.spec.ts
[high] 26-26: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build Packages
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (56)
packages/shared/src/__tests__/fastDeepMerge.spec.ts (1)
1-1: Vitest migration is correctly applied.The import statement successfully migrates from Jest to Vitest, replacing Jest testing utilities with their Vitest equivalents. The
describe,expect, anditAPIs are properly imported and consistently used throughout the test file.packages/shared/src/utils/__tests__/createDeferredPromise.spec.ts (1)
1-1: LGTM! Vitest imports correctly added.The explicit imports from Vitest align with the migration objective and follow best practices for explicit test framework dependencies.
packages/shared/src/__tests__/handleValueOrFn.spec.ts (1)
1-1: LGTM! Vitest migration is correct.The import statement has been properly updated to use Vitest testing utilities. The rest of the test file requires no changes since Vitest's API is compatible with Jest's syntax (
describe,it.each,expect,.toBe()).packages/shared/src/__tests__/jwtPayloadParser.spec.ts (1)
1-1: LGTM! Vitest import is correct.The migration from Jest to Vitest is properly implemented. All imported functions (
describe,expect,test) are standard Vitest APIs and are used correctly throughout the test file.packages/shared/src/__tests__/underscore.spec.ts (1)
1-1: LGTM! Clean Vitest migration.The import change correctly migrates from Jest to Vitest. All test APIs used throughout the file (
describe,expect,it, and matchers liketoBe,toStrictEqual,toBeFalsy,toHaveProperty) are fully compatible between Jest and Vitest, so the tests should work without modification.packages/shared/src/__tests__/url.spec.ts (1)
1-1: LGTM! Clean migration to Vitest.The import statement has been correctly updated to use Vitest. All the test APIs used in this file (
describe,it,test,expect,it.each) are fully compatible between Jest and Vitest, so no other changes are required. The migration is complete for this file.packages/shared/src/__tests__/retry.spec.ts (4)
1-12: LGTM! Migration to Vitest APIs is correct.The imports and timer setup/teardown hooks are properly migrated from Jest to Vitest equivalents (
vi.useFakeTimers(),vi.useRealTimers()).
14-38: LGTM! Timer advance API correctly migrated.The test correctly uses
vi.advanceTimersByTimeAsync()which is the Vitest equivalent for async timer advancement. This properly processes microtasks and promises.
69-173: LGTM! Timer-based retry tests properly migrated.All tests correctly use
vi.advanceTimersByTimeAsync()to validate retry delays and backoff behavior. The test logic accurately validates the retry functionality with various configurations.
175-209: LGTM! Math.random spy correctly migrated.The spy correctly uses
vi.spyOn()which is the Vitest equivalent of Jest'sjest.spyOn(). The jitter test logic and timer advances are accurate.packages/shared/src/__tests__/devbrowser.spec.ts (2)
1-1: LGTM! Vitest imports are correct.The migration from Jest to Vitest imports is complete and accurate. All necessary test APIs (
describe,expect,it,test,vi,beforeEach,afterEach) are properly imported.
37-37: LGTM! Mock creation correctly migrated to Vitest.The mock function is properly updated from
jest.fn()tovi.fn(), which is the correct Vitest equivalent.packages/shared/src/__tests__/error.spec.ts (2)
3-4: Type imports are properly used.The type-only import on line 3 follows TypeScript best practices and aligns with the coding guidelines for efficient tree-shaking.
1-1: Vitest migration is correct and complete.All imports are properly switched from Jest to Vitest. No remaining Jest references detected. All test APIs used (
describe,it,expect,toThrow,toEqual,toBe,toMatch) are fully compatible with Vitest. Test structure and type annotations follow best practices.packages/shared/src/router/__tests__/router.spec.ts (2)
1-1: LGTM! Vitest imports are correct.The import statement properly brings in all necessary Vitest test utilities, replacing the Jest globals. This is the standard approach for Vitest migration.
17-17: LGTM! Mock cleanup properly migrated.The replacement of
jest.clearAllMocks()withvi.clearAllMocks()is correct and maintains proper test isolation by resetting mocks between test runs.packages/shared/src/__tests__/keys.spec.ts (1)
1-1: LGTM! Migration to Vitest completed successfully.The import change correctly migrates from Jest to Vitest. The APIs used (
describe,expect,it,test) are fully compatible between the two frameworks, and no changes to the test logic are required.packages/shared/src/__tests__/loadClerkJsScript.spec.ts (4)
1-2: LGTM! Clean Vitest migration.The imports are correctly updated from Jest globals to explicit Vitest imports, including the
Mocktype for proper type annotations.
14-14: LGTM! Proper mock and timer setup.The mock configuration and timer management are correctly migrated to Vitest APIs:
- Module mock uses
vi.mock()- Mock clearing with
vi.clearAllMocks()- Fake timers properly set up and torn down
Also applies to: 28-36, 38-40
65-65: LGTM! Timer advancement correctly migrated.All calls to advance timers are properly using
vi.advanceTimersByTime(), which is the correct Vitest API.Also applies to: 87-87, 110-110, 124-124, 138-138
26-26: Static analysis false positive: This is a test mock key, not a real secret.The Gitleaks warning about line 26 is a false positive. The value
pk_test_Zm9vLWJhci0xMy5jbGVyay5hY2NvdW50cy5kZXYkis clearly a test fixture (note thepk_test_prefix and the base64-encoded "foo-bar-13.clerk.accounts.dev$" domain) used throughout the test suite, not a real API key that poses a security risk.packages/shared/src/react/__tests__/useReverification.spec.ts (3)
3-3: LGTM! Vitest imports are correct.The import statement properly includes all necessary Vitest utilities for this test file.
8-16: Mock migration completed correctly.The
jest.mockcall has been properly converted tovi.mock, and all function mocks now usevi.fn(). The mock structure and behavior are preserved.
64-73: Vitest migration correctly implemented; manual test execution required.Static analysis confirms the code follows Vitest and React Testing Library conventions correctly:
- Lines 64-73: Proper use of
vi.fn(),renderHook(),rerender(), and stable reference comparison with.toBe()- Lines 76-91: Correct inline fetcher pattern,
act()wrapper for async operations, andtoHaveBeenCalledWith()assertion- All imports, mocks, and type annotations are properly structured
Since the sandbox environment cannot execute Vitest, please manually run
pnpm test packages/shared -- useReverification.spec.tsto confirm all tests pass as part of the PR validation.packages/shared/src/__tests__/organization.spec.ts (1)
1-1: LGTM! Clean migration to Vitest.The import statement correctly switches from Jest to Vitest, maintaining all test functionality.
packages/shared/src/utils/__tests__/instance.spec.ts (1)
1-1: LGTM! Vitest migration successful.The import correctly switches to Vitest, and the test patterns (including
it.each) are fully compatible.packages/shared/src/__tests__/deriveState.spec.ts (1)
1-1: LGTM! Import updated correctly.The Vitest imports are properly configured for this test suite.
packages/shared/src/__tests__/buildAccountsBaseUrl.spec.ts (1)
1-1: LGTM! Vitest imports configured correctly.The test file successfully migrates to Vitest with proper imports.
packages/shared/src/__tests__/apiUrlFromPublishableKey.spec.ts (1)
1-1: LGTM! Vitest migration complete.The imports are correctly updated to use Vitest utilities.
packages/shared/src/telemetry/events/__tests__/theme-usage.spec.ts (1)
1-1: LGTM! Test imports updated successfully.The migration to Vitest is complete and correct.
packages/shared/src/__tests__/pathMatcher.spec.ts (2)
1-1: LGTM! Vitest imports correctly configured.The import statement properly switches to Vitest utilities including
vifor mocking.
5-7: Mock syntax updated correctly; static analysis warning can be ignored.The
vi.mocksyntax correctly replaces Jest's mocking approach. The static analysis warning about RegExp construction from variable input is a false positive in this test context, as the mock uses controlled patterns for testing purposes, not user input.Note: The simplified mock implementation may not fully represent the behavior of the actual
pathToRegexpfunction. If tests fail or behave unexpectedly, consider using a more accurate mock or the real implementation.packages/shared/src/__tests__/netlifyCacheHandler.spec.ts (1)
2-2: LGTM! Vitest imports complete.The imports correctly include all necessary Vitest utilities, including
beforeEachfor test setup.packages/shared/src/__tests__/versionSelector.spec.ts (1)
2-3: LGTM! Clean migration to Vitest.The import statement has been correctly updated to use Vitest instead of Jest, maintaining the same test API surface (describe, expect, it, test).
packages/shared/vitest.setup.mts (1)
7-7: Verify the hardcoded global version value.The hardcoded value
'5.0.0'forglobalThis.JS_PACKAGE_VERSIONmay not match expectations in all tests. For example,versionSelector.spec.tstests set this value to different versions (e.g., '2.0.0', '2.0.0-next.0', '2.0.0-snapshot.0') within individual test cases usingglobal.JS_PACKAGE_VERSION = ....Confirm that this global default doesn't interfere with tests that override it locally, and consider whether a more neutral default (e.g., '0.0.0-test') would be more appropriate to avoid confusion.
packages/shared/src/__tests__/proxy.spec.ts (1)
1-2: LGTM! Vitest migration complete.The import statement correctly includes all necessary Vitest utilities (afterEach, beforeEach, describe, expect, it) for the test lifecycle hooks and assertions used in this file.
packages/shared/src/__tests__/localStorageBroadcastChannel.spec.ts (3)
1-2: LGTM! Vitest imports correctly added.The import statement includes
vifor mocking, which is properly used throughout the file to replacejest.fn.
13-17: LGTM! Mock functions migrated to Vitest.The migration from
jest.fntovi.fnis correct and maintains the same functionality for mocking localStorage methods.
27-29: LGTM! Test listeners migrated to Vitest mocks.The test listeners correctly use
vi.fn()instead ofjest.fn(), maintaining consistency with the Vitest migration.packages/shared/src/__tests__/logger.spec.ts (2)
1-2: LGTM! Vitest migration complete.All necessary Vitest utilities imported correctly for the test lifecycle and mocking needs.
7-7: LGTM! Console spies migrated to Vitest.The migration from
jest.spyOntovi.spyOnis correct. ThemockImplementation(() => {})properly suppresses console output during tests while maintaining spy functionality.Also applies to: 22-22
packages/shared/src/__tests__/telemetry.logs.spec.ts (3)
3-4: LGTM! Vitest imports correctly added.All necessary Vitest utilities are imported for the test lifecycle, mocking, and timer control.
7-7: LGTM! Timer control migrated to Vitest.The migration from Jest's fake timers to Vitest's
vi.useFakeTimers()is correct and maintains the same test behavior for controlling time-based operations.
36-36: LGTM! Timer control migrated throughout the file.All calls to
jest.runAllTimers()have been correctly updated tovi.runAllTimers(), ensuring consistent timer control across all test cases.Also applies to: 68-68, 76-76, 86-86, 102-102, 111-111, 120-120, 135-135, 162-162, 196-196, 220-220
packages/shared/src/__tests__/browser.spec.ts (3)
1-2: LGTM! Vitest imports correctly added.All necessary Vitest utilities imported for the test needs including the
vimocking utility.
7-7: LGTM! Mock restoration migrated to Vitest.The migration from
jest.restoreAllMocks()tovi.restoreAllMocks()is correct and maintains proper cleanup between tests.Also applies to: 38-38
112-131: Consistent defensive pattern for browser properties.The defensive property definitions for both
webdriverandconnectionare consistent with the pattern established earlier in the file. This is good for maintainability.However, as mentioned in the previous comment, consider consolidating these definitions in the global test setup if they're needed across multiple test files.
packages/shared/package.json (1)
151-153: LGTM! Test scripts fully migrated to Vitest.The test scripts have been correctly updated to:
- Remove Jest from the test pipeline (previously ran both Jest and Vitest)
- Use Vitest for CI with appropriate worker limits
- Use Vitest for coverage collection
This aligns with the complete migration from Jest to Vitest across the test suite.
packages/shared/src/__tests__/date.spec.ts (1)
1-1: LGTM! Clean migration to Vitest.The Vitest import correctly replaces Jest globals. The test suite's basic structure (describe/it/expect) is compatible between Jest and Vitest, so no other changes are needed.
packages/shared/src/__tests__/eventBus.spec.ts (2)
1-1: LGTM! Correct Vitest imports.All necessary Vitest utilities are properly imported, including
vifor mocking.
23-272: LGTM! Consistent mock migration.All
jest.fn()calls have been correctly replaced withvi.fn()throughout the file. The migration is thorough and consistent.packages/shared/src/__tests__/deprecated.spec.ts (4)
1-8: LGTM! Proper Vitest module mocking setup.The module mock correctly uses
vi.mock()withvi.fn()to mock the runtime environment utilities. This is the standard Vitest approach for module mocking.
10-10: LGTM! Correct type import.The
Mocktype from Vitest is properly imported for type casting mock functions throughout the test suite.
19-19: LGTM! Correct spy setup.The console.warn spy is correctly set up using Vitest's
vi.spyOn()API with mock implementation.
142-145: LGTM! Proper mock manipulation.The mock return value manipulations use the correct Vitest pattern with type casting to
Mockand callingmockReturnValue().Also applies to: 164-167, 264-267, 292-295, 348-351, 370-373
packages/shared/src/__tests__/color.spec.ts (1)
1-73: LGTM! Well-structured color utility tests.This new test file demonstrates good testing practices:
- Comprehensive test coverage for color parsing and formatting utilities
- Parameterized tests using
it.eachfor clarity and maintainability- Tests edge cases including empty strings, invalid inputs, and various color formats (hex, rgb/rgba, hsl/hsla, transparent)
- Properly uses Vitest APIs
| // Define webdriver property if it doesn't exist | ||
| if (!('webdriver' in window.navigator)) { | ||
| Object.defineProperty(window.navigator, 'webdriver', { | ||
| configurable: true, | ||
| get: () => false, | ||
| }); | ||
| } | ||
| webdriverGetter = vi.spyOn(window.navigator, 'webdriver', 'get'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Good defensive coding for JSDOM compatibility.
The code checks if the webdriver property exists on window.navigator before defining it. This suggests that Vitest's JSDOM environment may not include this property by default, unlike Jest's environment.
However, verify whether this defensive check is necessary. If Vitest consistently lacks this property, consider defining it unconditionally in the global test setup (vitest.setup.mts) rather than in individual test files to reduce duplication.
Run the following script to check if this pattern appears in other test files:
🏁 Script executed:
#!/bin/bash
# Search for similar defensive property definitions in test files
rg -A 5 "if \(!'.*' in window\.navigator\)" --type ts --glob '**/__tests__/**'Length of output: 0
I need to try different search approaches since the regex pattern didn't match. Let me search more broadly:
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for webdriver property usage in test files
rg -n "webdriver" --type ts --glob '**/__tests__/**' -B 2 -A 2
Length of output: 9467
🏁 Script executed:
#!/bin/bash
# Search for Object.defineProperty usage in test files
rg -n "Object\.defineProperty" --type ts --glob '**/__tests__/**' -B 2 -A 2Length of output: 39064
🏁 Script executed:
#!/bin/bash
# Look for vitest setup files
fd -e mts -e ts --glob '**/vitest*' -o --glob '**/setup*' 2>/dev/null | head -20Length of output: 0
Consider extracting the repeated webdriver property setup into a helper function to avoid duplication.
The defensive webdriver property check appears twice in the same file:
- Lines 28–33 (in
isValidBrowserdescribe block) - Lines 113–118 (in
isValidBrowserOnlinedescribe block)
Since the logic is identical and JSDOM appears to consistently lack this property in both test suites, extract this into a shared setup helper to reduce duplication and improve maintainability:
const setupWebdriverProperty = () => {
if (!('webdriver' in window.navigator)) {
Object.defineProperty(window.navigator, 'webdriver', {
configurable: true,
get: () => false,
});
}
};Then call setupWebdriverProperty() in each beforeEach instead of repeating the full block.
🤖 Prompt for AI Agents
In packages/shared/src/__tests__/browser.spec.ts around lines 27 to 34, the
defensive webdriver property check is duplicated elsewhere; extract the repeated
block into a small helper function (e.g., setupWebdriverProperty) that checks
for 'webdriver' on window.navigator and defines it when missing, then call that
helper from each beforeEach instead of repeating the Object.defineProperty
block; update imports/locals as needed and ensure the vi.spyOn call still
references window.navigator.webdriver after the helper runs.
@clerk/agent-toolkit
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@clerk/elements
@clerk/clerk-expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/clerk-react
@clerk/react-router
@clerk/remix
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/themes
@clerk/types
@clerk/upgrade
@clerk/vue
commit: |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/shared/vitest.setup.mts (1)
11-15: Replace@ts-ignorewith a type assertion for better type safety.While the Web Crypto polyfill logic is sound, using
@ts-ignoresuppresses all type checking. Consider using a type assertion instead to maintain some type safety while acknowledging the intentional type mismatch.Apply this diff to improve type safety:
// Setup Web Crypto API for tests if (!globalThis.crypto) { - // @ts-ignore - globalThis.crypto = webcrypto; + globalThis.crypto = webcrypto as unknown as Crypto; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.changeset/breezy-flowers-float.md(1 hunks)packages/shared/vitest.setup.mts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
.changeset/**
📄 CodeRabbit inference engine (.cursor/rules/monorepo.mdc)
Automated releases must use Changesets.
Files:
.changeset/breezy-flowers-float.md
🪛 LanguageTool
.changeset/breezy-flowers-float.md
[grammar] ~1-~1: Hier könnte ein Fehler sein.
Context: --- ---
(QB_NEW_DE)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (26)
- GitHub Check: Integration Tests (react-router, chrome)
- GitHub Check: Integration Tests (nextjs, chrome, 14)
- GitHub Check: Integration Tests (nextjs, chrome, 15)
- GitHub Check: Integration Tests (custom, chrome)
- GitHub Check: Integration Tests (expo-web, chrome)
- GitHub Check: Integration Tests (billing, chrome)
- GitHub Check: Integration Tests (nuxt, chrome)
- GitHub Check: Integration Tests (machine, chrome)
- GitHub Check: Integration Tests (tanstack-react-start, chrome)
- GitHub Check: Integration Tests (sessions:staging, chrome)
- GitHub Check: Integration Tests (handshake, chrome)
- GitHub Check: Integration Tests (vue, chrome)
- GitHub Check: Integration Tests (express, chrome)
- GitHub Check: Integration Tests (sessions, chrome)
- GitHub Check: Integration Tests (astro, chrome)
- GitHub Check: Integration Tests (handshake:staging, chrome)
- GitHub Check: Integration Tests (elements, chrome)
- GitHub Check: Integration Tests (localhost, chrome)
- GitHub Check: Integration Tests (ap-flows, chrome)
- GitHub Check: Integration Tests (quickstart, chrome)
- GitHub Check: Integration Tests (generic, chrome)
- GitHub Check: Publish with pkg-pr-new
- GitHub Check: Unit Tests (22, **)
- GitHub Check: Static analysis
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (2)
packages/shared/vitest.setup.mts (2)
1-4: LGTM! Imports are correct for Vitest migration.The imports properly set up the test environment by bringing in Node.js Web Crypto support and the Vitest test lifecycle hooks.
6-9: Version constants are already well-distinguished by their naming and usage; no clarification comments needed.The two version constants serve distinct, intentional purposes:
PACKAGE_VERSIONrepresents each package's own version (e.g.,@clerk/clerk-react@0.0.0-test). It's used in SDK_METADATA and userAgent strings across all packages.JS_PACKAGE_VERSIONrepresents the@clerk/jscore library version. It's only defined in packages that depend on@clerk/js(react, remix, react-router, elements, expo, etc.) and is specifically used inpackages/shared/src/versionSelector.tsfor version selection logic (e.g., determining which JS library version to load).The distinction is self-evident from the naming convention (
JS_prefix) and the usage patterns in the codebase, particularly inversionSelector.tswhereJS_PACKAGE_VERSIONis a default parameter that gets returned for snapshot versions. Adding comments would be redundant.
nikosdouvlis
force-pushed
the
nikos/migrate-share
branch
from
d588d6e to
5fa2f97
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.changeset/breezy-flowers-float.md (1)
1-2: Already flagged: Changeset is incomplete.This issue was identified in a previous review. The changeset requires package metadata (
@clerk/shared: patch) and a description documenting the migration to Vitest.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
.changeset/breezy-flowers-float.md(1 hunks)packages/shared/src/__tests__/color.spec.ts(1 hunks)packages/shared/src/__tests__/deriveState.spec.ts(1 hunks)packages/shared/src/__tests__/organization.spec.ts(1 hunks)packages/shared/vitest.setup.mts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/shared/vitest.setup.mts
- packages/shared/src/tests/deriveState.spec.ts
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
**/*.{js,jsx,ts,tsx}: All code must pass ESLint checks with the project's configuration
Follow established naming conventions (PascalCase for components, camelCase for variables)
Maintain comprehensive JSDoc comments for public APIs
Use dynamic imports for optional features
All public APIs must be documented with JSDoc
Provide meaningful error messages to developers
Include error recovery suggestions where applicable
Log errors appropriately for debugging
Lazy load components and features when possible
Implement proper caching strategies
Use efficient data structures and algorithms
Profile and optimize critical paths
Validate all inputs and sanitize outputs
Implement proper logging with different levels
Files:
packages/shared/src/__tests__/organization.spec.tspackages/shared/src/__tests__/color.spec.ts
**/*.{js,jsx,ts,tsx,json,css,scss,md,yaml,yml}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use Prettier for consistent code formatting
Files:
packages/shared/src/__tests__/organization.spec.tspackages/shared/src/__tests__/color.spec.ts
packages/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
TypeScript is required for all packages
Files:
packages/shared/src/__tests__/organization.spec.tspackages/shared/src/__tests__/color.spec.ts
packages/**/*.{ts,tsx,d.ts}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Packages should export TypeScript types alongside runtime code
Files:
packages/shared/src/__tests__/organization.spec.tspackages/shared/src/__tests__/color.spec.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use proper TypeScript error types
**/*.{ts,tsx}: Always define explicit return types for functions, especially public APIs
Use proper type annotations for variables and parameters where inference isn't clear
Avoidanytype - preferunknownwhen type is uncertain, then narrow with type guards
Useinterfacefor object shapes that might be extended
Usetypefor unions, primitives, and computed types
Preferreadonlyproperties for immutable data structures
Useprivatefor internal implementation details
Useprotectedfor inheritance hierarchies
Usepublicexplicitly for clarity in public APIs
Preferreadonlyfor properties that shouldn't change after construction
Prefer composition and interfaces over deep inheritance chains
Use mixins for shared behavior across unrelated classes
Implement dependency injection for loose coupling
Let TypeScript infer when types are obvious
Useconst assertionsfor literal types:as const
Usesatisfiesoperator for type checking without widening
Use mapped types for transforming object types
Use conditional types for type-level logic
Leverage template literal types for string manipulation
Use ES6 imports/exports consistently
Use default exports sparingly, prefer named exports
Use type-only imports:import type { ... } from ...
Noanytypes without justification
Proper error handling with typed errors
Consistent use ofreadonlyfor immutable data
Proper generic constraints
No unused type parameters
Proper use of utility types instead of manual type construction
Type-only imports where possible
Proper tree-shaking friendly exports
No circular dependencies
Efficient type computations (avoid deep recursion)
Files:
packages/shared/src/__tests__/organization.spec.tspackages/shared/src/__tests__/color.spec.ts
packages/**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/monorepo.mdc)
Unit tests should use Jest or Vitest as the test runner.
Files:
packages/shared/src/__tests__/organization.spec.tspackages/shared/src/__tests__/color.spec.ts
**/*.{js,ts,tsx,jsx}
📄 CodeRabbit inference engine (.cursor/rules/monorepo.mdc)
Support multiple Clerk environment variables (CLERK_, NEXT_PUBLIC_CLERK_, etc.) for configuration.
Files:
packages/shared/src/__tests__/organization.spec.tspackages/shared/src/__tests__/color.spec.ts
**/__tests__/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/typescript.mdc)
**/__tests__/**/*.{ts,tsx}: Create type-safe test builders/factories
Use branded types for test isolation
Implement proper mock types that match interfaces
Files:
packages/shared/src/__tests__/organization.spec.tspackages/shared/src/__tests__/color.spec.ts
.changeset/**
📄 CodeRabbit inference engine (.cursor/rules/monorepo.mdc)
Automated releases must use Changesets.
Files:
.changeset/breezy-flowers-float.md
🪛 LanguageTool
.changeset/breezy-flowers-float.md
[grammar] ~1-~1: Hier könnte ein Fehler sein.
Context: --- ---
(QB_NEW_DE)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Build Packages
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (3)
packages/shared/src/__tests__/organization.spec.ts (1)
2-2: LGTM! Migration to Vitest is correctly implemented.The import statement correctly switches from Jest globals to Vitest's named imports. The test code itself is already compatible with Vitest since it uses the standard
describe,it, andexpectAPIs that work identically in both frameworks.packages/shared/src/__tests__/color.spec.ts (2)
1-5: LGTM! Clean Vitest migration setup.The imports correctly use Vitest testing utilities and follow the project's migration pattern from Jest to Vitest.
6-31: LGTM! Well-structured parameterized tests.Both test suites use
it.eachappropriately for case-driven assertions and provide good coverage of different input formats.
| ['rgb(100,100,100)', { r: 100, g: 100, b: 100, a: undefined }], | ||
| ['rgba(100,100,100,0.5)', { r: 100, g: 100, b: 100, a: 0.5 }], | ||
| ['rgb(100,100,100)', { r: 100, g: 100, b: 100, a: undefined }], | ||
| ['rgba(100,100,100,0.5)', { r: 100, g: 100, b: 100, a: 0.5 }], |
There was a problem hiding this comment.
Remove duplicate test cases.
Lines 43-44 are exact duplicates of lines 41-42 (testing the same rgb/rgba inputs). Remove the redundant entries.
Apply this diff:
['rgb(100,100,100)', { r: 100, g: 100, b: 100, a: undefined }],
['rgba(100,100,100,0.5)', { r: 100, g: 100, b: 100, a: 0.5 }],
- ['rgb(100,100,100)', { r: 100, g: 100, b: 100, a: undefined }],
- ['rgba(100,100,100,0.5)', { r: 100, g: 100, b: 100, a: 0.5 }],
['hsl(244,66%,33%)', { h: 244, s: 0.66, l: 0.33, a: undefined }],📝 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.
| ['rgb(100,100,100)', { r: 100, g: 100, b: 100, a: undefined }], | |
| ['rgba(100,100,100,0.5)', { r: 100, g: 100, b: 100, a: 0.5 }], | |
| ['rgb(100,100,100)', { r: 100, g: 100, b: 100, a: undefined }], | |
| ['rgba(100,100,100,0.5)', { r: 100, g: 100, b: 100, a: 0.5 }], | |
| ['rgb(100,100,100)', { r: 100, g: 100, b: 100, a: undefined }], | |
| ['rgba(100,100,100,0.5)', { r: 100, g: 100, b: 100, a: 0.5 }], |
🤖 Prompt for AI Agents
In packages/shared/src/__tests__/color.spec.ts around lines 41 to 44 there are
duplicated test entries for 'rgb(100,100,100)' and 'rgba(100,100,100,0.5)';
remove the redundant lines (the second occurrence on lines 43-44) so each input
case appears only once in the test array.
| const cases: Array<[Color, string]> = [ | ||
| ['', ''], | ||
| ['invalid', ''], | ||
| ['#12ff12', '#12ff12'], | ||
| ['#12ff12', '#12ff12'], | ||
| ['#1ff', '#1ff'], |
There was a problem hiding this comment.
Fix type annotation and remove duplicate test case.
Two issues:
-
Type mismatch: The type annotation
Array<[Color, string]>indicates the first element should be aColorobject, but lines 58-62 use string literals. IfcolorToSameTypeStringaccepts bothColorobjects and strings, update the type annotation to reflect this (e.g.,Array<[Color | string, string]>). -
Duplicate test case: Lines 60-61 both test
'#12ff12'.
Apply this diff to fix the type annotation and remove the duplicate:
- const cases: Array<[Color, string]> = [
+ const cases: Array<[Color | string, string]> = [
['', ''],
['invalid', ''],
['#12ff12', '#12ff12'],
- ['#12ff12', '#12ff12'],
['#1ff', '#1ff'],📝 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.
| const cases: Array<[Color, string]> = [ | |
| ['', ''], | |
| ['invalid', ''], | |
| ['#12ff12', '#12ff12'], | |
| ['#12ff12', '#12ff12'], | |
| ['#1ff', '#1ff'], | |
| const cases: Array<[Color | string, string]> = [ | |
| ['', ''], | |
| ['invalid', ''], | |
| ['#12ff12', '#12ff12'], | |
| ['#1ff', '#1ff'], |
🤖 Prompt for AI Agents
In packages/shared/src/__tests__/color.spec.ts around lines 57 to 62, the test
cases array is annotated as Array<[Color, string]> but uses string literals and
contains a duplicate '#12ff12' entry; update the type annotation to Array<[Color
| string, string]> (or Array<[string | Color, string]>) so strings are allowed,
and remove the duplicate '#12ff12' test case so each case is unique.
1 participant
Description
Title :)
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit