fix(clerk-js): Signal iframe context in CHIPS build #6758
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 1b80c5e The changes in this PR will be included in the next version bump. This PR includes changesets to release 22 packages
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 |
WalkthroughAdds optional iframeContext to SignIn/SignUp types and runtime payloads; detects iframe via inIframe() and, when BUILD_VARIANT_CHIPS is true and running inside an iframe, sets iframeContext = true on create/update requests; updates payload construction, tests, and a changeset. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App
participant Runtime as inIframe()
participant Client as SignIn/SignUp
participant API as Backend
App->>Client: create()/update()/authenticateWithRedirectOrPopup()
Client->>Runtime: inIframe()
Runtime-->>Client: true/false
alt __BUILD_VARIANT_CHIPS__ && inIframe() == true
rect rgb(220,240,255)
Client->>Client: clone params -> createParams/finalParams
Client->>Client: set createParams.iframeContext = true
end
end
Client->>API: POST/PATCH with (maybe) iframeContext
API-->>Client: response
Client-->>App: result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (3)
✨ Finishing touches
🧪 Generate unit tests
Comment |
@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: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/types/src/signUpCommon.ts (1)
103-106: Make the property optional for readability (already optional via Partial)The field is inside a Partial<…>, so it’s effectively optional. For consistency with SignIn types, consider adding ?.
Apply:
- iframeContext: boolean; + iframeContext?: boolean;packages/clerk-js/src/core/resources/SignUp.ts (2)
146-149: Correctly signaling iframe context on createGated by BUILD_VARIANT_CHIPS and inIframe(), which matches the PR goal.
Avoid overriding a pre-set value (even if internal) by only setting when undefined:
- if (__BUILD_VARIANT_CHIPS__ && inIframe()) { - finalParams.iframeContext = true; - } + if (__BUILD_VARIANT_CHIPS__ && inIframe() && finalParams.iframeContext === undefined) { + finalParams.iframeContext = true; + }Please confirm normalizeUnsafeMetadata preserves unknown top-level keys like iframeContext.
434-442: Propagating iframe context on update as well — good coverageMirrors create() behavior so both create/update payloads carry the signal.
Extract a small helper to DRY this logic across create/update:
+const withIframeContext = <T extends Record<string, unknown>>(params: T): T => + (__BUILD_VARIANT_CHIPS__ && inIframe() && params.iframeContext === undefined + ? ({ ...params, iframeContext: true } as T) + : params); ... - const finalParams = { ...params }; - if (__BUILD_VARIANT_CHIPS__ && inIframe()) { - finalParams.iframeContext = true; - } + const finalParams = withIframeContext({ ...params });Do we also need this flag when using the SignUpFuture API paths (create/sso) in CHIPS? If so, consider similar handling there.
packages/clerk-js/src/core/resources/SignIn.ts (1)
302-314: Adds iframeContext during create for OAuth flows — correct, but consider centralizingThis fixes the affected flow. To prevent future misses, consider doing this in SignIn.create() with a safe guard.
Apply:
create = (params: SignInCreateParams): Promise<SignInResource> => { debugLogger.debug('SignIn.create', { id: this.id, strategy: 'strategy' in params ? params.strategy : undefined }); - return this._basePost({ - path: this.pathRoot, - body: params, - }); + const body = + __BUILD_VARIANT_CHIPS__ && + inIframe() && + 'strategy' in params && + (params.strategy === 'saml' || params.strategy === 'enterprise_sso' || typeof params.strategy === 'string') + ? ({ ...params, iframeContext: true } as SignInCreateParams) + : params; + return this._basePost({ path: this.pathRoot, body }); };This keeps the flag constrained to strategy-based creates and avoids touching identifier-only creates.
If you prefer not to centralize, audit other create paths (e.g., web3, email_code bootstrap) to ensure the flag isn’t required there.
📜 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)
packages/clerk-js/src/core/resources/SignIn.ts(3 hunks)packages/clerk-js/src/core/resources/SignUp.ts(3 hunks)packages/types/src/signInCommon.ts(1 hunks)packages/types/src/signInFuture.ts(1 hunks)packages/types/src/signUpCommon.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{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/types/src/signInFuture.tspackages/clerk-js/src/core/resources/SignUp.tspackages/types/src/signUpCommon.tspackages/types/src/signInCommon.tspackages/clerk-js/src/core/resources/SignIn.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/types/src/signInFuture.tspackages/clerk-js/src/core/resources/SignUp.tspackages/types/src/signUpCommon.tspackages/types/src/signInCommon.tspackages/clerk-js/src/core/resources/SignIn.ts
packages/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
TypeScript is required for all packages
Files:
packages/types/src/signInFuture.tspackages/clerk-js/src/core/resources/SignUp.tspackages/types/src/signUpCommon.tspackages/types/src/signInCommon.tspackages/clerk-js/src/core/resources/SignIn.ts
packages/**/*.{ts,tsx,d.ts}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Packages should export TypeScript types alongside runtime code
Files:
packages/types/src/signInFuture.tspackages/clerk-js/src/core/resources/SignUp.tspackages/types/src/signUpCommon.tspackages/types/src/signInCommon.tspackages/clerk-js/src/core/resources/SignIn.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/types/src/signInFuture.tspackages/clerk-js/src/core/resources/SignUp.tspackages/types/src/signUpCommon.tspackages/types/src/signInCommon.tspackages/clerk-js/src/core/resources/SignIn.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/types/src/signInFuture.tspackages/clerk-js/src/core/resources/SignUp.tspackages/types/src/signUpCommon.tspackages/types/src/signInCommon.tspackages/clerk-js/src/core/resources/SignIn.ts
🧬 Code graph analysis (2)
packages/clerk-js/src/core/resources/SignUp.ts (1)
packages/clerk-js/src/utils/runtime.ts (1)
inIframe(9-19)
packages/clerk-js/src/core/resources/SignIn.ts (3)
packages/types/src/signInCommon.ts (1)
SignInCreateParams(123-168)packages/clerk-js/src/utils/runtime.ts (1)
inIframe(9-19)packages/types/src/signInFuture.ts (1)
SignInFutureCreateParams(6-16)
⏰ 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). (2)
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
packages/types/src/signInFuture.ts (1)
12-15: iframeContext type surface addition looks goodOptional flag aligns with runtime usage in SignInFuture.create. Keep it marked @internal to avoid leaking in public docs.
If you generate API docs, ensure @internal is excluded by your Typedoc config so this field doesn’t appear publicly.
packages/clerk-js/src/core/resources/SignUp.ts (1)
56-56: New inIframe import — OKImport location and usage are consistent with utils/runtime.
packages/types/src/signInCommon.ts (1)
131-135: Targeted addition for OAuth/SAML/Enterprise SSO variant — OKField is scoped to the variant that’s relevant for OAuth/SSO flows. Looks correct.
Double-check all call sites that construct this variant (e.g., authenticateWithRedirect/Popup) so none miss the flag.
packages/clerk-js/src/core/resources/SignIn.ts (2)
57-57: inIframe import — OKConsistent with utils usage elsewhere.
637-646: SignInFuture.create includes iframeContext — goodCovers the programmatic future API. No further issues.
Ensure SignInFuture.sso indeed routes through this.create so the flag propagates (it does in the current code).
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
.changeset/violet-badgers-change.md (1)
1-6: Tighten release note; call out CHIPS gating and linked issueClarify optionality and gating to reduce ambiguity in release notes and generated changelog.
Apply:
-Adding iframeContext to SignIn and SignUp params when a CHIPS build is running in an iframe context +CHIPS: Signal iframe context for OAuth flows. + +- Adds optional `iframeContext` to `SignInCreateParams` and `SignUpCreateParams`. +- Only sent when `__BUILD_VARIANT_CHIPS__` is true and `inIframe()` returns true. +- Backwards compatible (no-op for non-CHIPS builds). +- Fixes USER-3277.packages/clerk-js/src/core/resources/__tests__/SignIn.spec.ts (4)
10-12: Avoid module-scope mutation of global build flagSetting
global.__BUILD_VARIANT_CHIPS__at import time can leak across suites. Prefer per-test or per-beforeEachsetup.Apply:
-const originalBuildVariant = global.__BUILD_VARIANT_CHIPS__; -global.__BUILD_VARIANT_CHIPS__ = true; +const originalBuildVariant = global.__BUILD_VARIANT_CHIPS__;And initialize a known default in
beforeEach:beforeEach(() => { vi.clearAllMocks(); + global.__BUILD_VARIANT_CHIPS__ = false; });Finally, make CHIPS-enabled tests explicit (example for the first test):
it('should set iframeContext to true when CHIPS build and in iframe', async () => { + global.__BUILD_VARIANT_CHIPS__ = true; vi.mocked(inIframe).mockReturnValue(true);
31-35: Use a getter spy forfapiClientto auto-restore
Object.definePropertywon’t be reset byvi.clearAllMocks(), risking leakage. Spy on the getter and restore inafterEach.Apply:
- Object.defineProperty(SignIn, 'fapiClient', { - get: () => mockFapiClient, - configurable: true, - }); + vi.spyOn(SignIn as any, 'fapiClient', 'get').mockReturnValue(mockFapiClient);Also restore spies:
afterEach(() => { global.__BUILD_VARIANT_CHIPS__ = originalBuildVariant; + vi.restoreAllMocks(); });
21-24: Remove redundant reassignment ofbuildUrlWithAuthYou already set
buildUrlWithAuthonmockClerk. Reassigning it again is unnecessary.Apply:
- mockBuildUrlWithAuth = vi.fn((url: string) => `https://clerk.example.com${url}`); - SignIn.clerk.buildUrlWithAuth = mockBuildUrlWithAuth; + mockBuildUrlWithAuth = mockClerk.buildUrlWithAuth;Also applies to: 48-50
78-85: Avoid asserting undefined properties; use partial matchAsserting keys with
undefinedmakes tests brittle if the implementation omits the key. Preferexpect.objectContaining.Apply:
- expect(mockCreate).toHaveBeenCalledWith({ - strategy: 'oauth_google', - identifier: 'test@example.com', - redirectUrl: 'https://clerk.example.com/callback', - actionCompleteRedirectUrl: undefined, - iframeContext: true, - }); + expect(mockCreate).toHaveBeenCalledWith( + expect.objectContaining({ + strategy: 'oauth_google', + identifier: 'test@example.com', + redirectUrl: 'https://clerk.example.com/callback', + iframeContext: true, + }), + );Replicate this pattern for similar assertions in this file.
packages/clerk-js/src/core/resources/__tests__/SignUp.spec.ts (4)
10-12: Avoid module-scope mutation of global build flagMatch the SignIn.spec.ts pattern: don’t set CHIPS at import time; set a default in
beforeEach, toggle per test.Apply:
-const originalBuildVariant = global.__BUILD_VARIANT_CHIPS__; -global.__BUILD_VARIANT_CHIPS__ = true; +const originalBuildVariant = global.__BUILD_VARIANT_CHIPS__;And in
beforeEach:beforeEach(() => { vi.clearAllMocks(); + global.__BUILD_VARIANT_CHIPS__ = false; });
42-46: Spy onfapiClientgetter instead of redefining propertyEnsures clean teardown with
vi.restoreAllMocks().Apply:
- Object.defineProperty(SignUp, 'fapiClient', { - get: () => mockFapiClient, - configurable: true, - }); + vi.spyOn(SignUp as any, 'fapiClient', 'get').mockReturnValue(mockFapiClient);Also restore spies and static replacements:
afterEach(() => { global.__BUILD_VARIANT_CHIPS__ = originalBuildVariant; + vi.restoreAllMocks(); });
40-41: RestoreSignUp.clerkafter testsStatic assignment can leak across suites. Save original and restore in
afterEach.Apply:
- SignUp.clerk = mockClerk as any; + const originalClerk = SignUp.clerk; + SignUp.clerk = mockClerk as any;And:
afterEach(() => { global.__BUILD_VARIANT_CHIPS__ = originalBuildVariant; + // Restore static + // @ts-expect-error test-only restoration + SignUp.clerk = originalClerk; + vi.restoreAllMocks(); });Also applies to: 81-83
238-250: Avoid asserting undefined properties; use partial matchPrefer partial matches over explicit
undefinedkeys to reduce brittleness.Apply:
- expect(mockCreate).toHaveBeenCalledWith({ - path: '/client/sign_ups', - body: { - strategy: 'oauth_google', - redirectUrl: 'https://clerk.example.com/callback', - actionCompleteRedirectUrl: undefined, - unsafeMetadata: undefined, - emailAddress: undefined, - legalAccepted: undefined, - oidcPrompt: undefined, - iframeContext: true, - }, - }); + expect(mockCreate).toHaveBeenCalledWith({ + path: '/client/sign_ups', + body: expect.objectContaining({ + strategy: 'oauth_google', + redirectUrl: 'https://clerk.example.com/callback', + iframeContext: true, + }), + });Replicate for the update flow below.
📜 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 (3)
.changeset/violet-badgers-change.md(1 hunks)packages/clerk-js/src/core/resources/__tests__/SignIn.spec.ts(1 hunks)packages/clerk-js/src/core/resources/__tests__/SignUp.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (10)
**/*.{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/clerk-js/src/core/resources/__tests__/SignUp.spec.tspackages/clerk-js/src/core/resources/__tests__/SignIn.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/clerk-js/src/core/resources/__tests__/SignUp.spec.tspackages/clerk-js/src/core/resources/__tests__/SignIn.spec.ts
packages/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
TypeScript is required for all packages
Files:
packages/clerk-js/src/core/resources/__tests__/SignUp.spec.tspackages/clerk-js/src/core/resources/__tests__/SignIn.spec.ts
packages/**/*.{ts,tsx,d.ts}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Packages should export TypeScript types alongside runtime code
Files:
packages/clerk-js/src/core/resources/__tests__/SignUp.spec.tspackages/clerk-js/src/core/resources/__tests__/SignIn.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/clerk-js/src/core/resources/__tests__/SignUp.spec.tspackages/clerk-js/src/core/resources/__tests__/SignIn.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/clerk-js/src/core/resources/__tests__/SignUp.spec.tspackages/clerk-js/src/core/resources/__tests__/SignIn.spec.ts
packages/{clerk-js,elements,themes}/**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/monorepo.mdc)
Visual regression testing should be performed for UI components.
Files:
packages/clerk-js/src/core/resources/__tests__/SignUp.spec.tspackages/clerk-js/src/core/resources/__tests__/SignIn.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/clerk-js/src/core/resources/__tests__/SignUp.spec.tspackages/clerk-js/src/core/resources/__tests__/SignIn.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/clerk-js/src/core/resources/__tests__/SignUp.spec.tspackages/clerk-js/src/core/resources/__tests__/SignIn.spec.ts
.changeset/**
📄 CodeRabbit inference engine (.cursor/rules/monorepo.mdc)
Automated releases must use Changesets.
Files:
.changeset/violet-badgers-change.md
🧬 Code graph analysis (2)
packages/clerk-js/src/core/resources/__tests__/SignUp.spec.ts (1)
packages/clerk-js/src/utils/runtime.ts (1)
inIframe(9-19)
packages/clerk-js/src/core/resources/__tests__/SignIn.spec.ts (1)
packages/clerk-js/src/utils/runtime.ts (1)
inIframe(9-19)
⏰ 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). (25)
- GitHub Check: Integration Tests (nextjs, chrome, 15)
- GitHub Check: Integration Tests (nextjs, chrome, 14)
- GitHub Check: Integration Tests (react-router, chrome)
- GitHub Check: Integration Tests (sessions, chrome)
- GitHub Check: Integration Tests (custom, chrome)
- GitHub Check: Integration Tests (billing, chrome)
- GitHub Check: Integration Tests (machine, chrome)
- GitHub Check: Integration Tests (tanstack-react-router, chrome)
- GitHub Check: Integration Tests (vue, chrome)
- GitHub Check: Integration Tests (nuxt, chrome)
- GitHub Check: Integration Tests (tanstack-react-start, chrome)
- GitHub Check: Integration Tests (ap-flows, chrome)
- GitHub Check: Integration Tests (elements, chrome)
- GitHub Check: Integration Tests (expo-web, chrome)
- GitHub Check: Integration Tests (quickstart, chrome)
- GitHub Check: Integration Tests (express, chrome)
- GitHub Check: Integration Tests (localhost, chrome)
- GitHub Check: Integration Tests (astro, chrome)
- GitHub Check: Integration Tests (generic, chrome)
- GitHub Check: Publish with pkg-pr-new
- GitHub Check: Unit Tests (22, **)
- GitHub Check: Unit Tests (18, --filter=@clerk/astro --filter=@clerk/backend --filter=@clerk/express --filter=@c...
- GitHub Check: Static analysis
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (11)
packages/clerk-js/src/core/resources/__tests__/SignIn.spec.ts (11)
10-12: Avoid module-scoped mutation of BUILD_VARIANT_CHIPS (order-dependent tests).Setting the flag at module load makes test outcomes depend on execution order. Prefer per-test or per-suite setup.
Apply this diff to stop setting it at module scope:
- (globalThis as any).__BUILD_VARIANT_CHIPS__ = true; + // Set __BUILD_VARIANT_CHIPS__ explicitly in beforeEach or per test
18-20: Set the CHIPS flag in beforeEach for deterministic defaults.Ensure every test starts from the same baseline; tests that need a different value can override locally.
beforeEach(() => { vi.clearAllMocks(); + (globalThis as any).__BUILD_VARIANT_CHIPS__ = true; });
14-17: Prevent static leakage: capture originals to restore after the suite.Stubbing SignIn.clerk and the fapiClient getter without restoring can leak across suites.
let signIn: SignIn; let mockCreate: any; - let mockBuildUrlWithAuth: any; + let mockBuildUrlWithAuth: any; + + // Capture originals to restore after the suite + const originalClerk = SignIn.clerk; + const originalFapiDescriptor = Object.getOwnPropertyDescriptor(SignIn, 'fapiClient');
52-55: Restore globals and statics after the suite.Complements the per-test setup and avoids cross-file interference.
afterEach(() => { (globalThis as any).__BUILD_VARIANT_CHIPS__ = originalBuildVariant; }); + + afterAll(() => { + // Restore SignIn statics + SignIn.clerk = originalClerk as any; + if (originalFapiDescriptor) { + Object.defineProperty(SignIn, 'fapiClient', originalFapiDescriptor); + } else { + // Ensure we don't leave a test-only getter behind + // eslint-disable-next-line @typescript-eslint/no-explicit-any + delete (SignIn as any).fapiClient; + } + });
21-35: Minor: prefer vi.stubGlobal for globals, and vi.restoreAllMocks for spies.Not mandatory, but vi.stubGlobal/vi.unstubAllGlobals provide cleaner lifecycle for globals, and restoreAllMocks reverts spy replacements (beyond clearAllMocks).
48-50: Drop redundant reassignment of buildUrlWithAuth.You already set SignIn.clerk in beforeEach; reassigning buildUrlWithAuth here is unnecessary and can trip ESLint (unused variable).
- mockBuildUrlWithAuth = vi.fn((url: string) => `https://clerk.example.com${url}`); - SignIn.clerk.buildUrlWithAuth = mockBuildUrlWithAuth; + // buildUrlWithAuth already mocked via mockClerk above
56-85: Make the expectation resilient; avoid asserting undefined keys.Asserting actionCompleteRedirectUrl: undefined is brittle if the implementation omits undefined fields. Use objectContaining and assert iframeContext specifically.
- expect(mockCreate).toHaveBeenCalledWith({ - strategy: 'oauth_google', - identifier: 'test@example.com', - redirectUrl: 'https://clerk.example.com/callback', - actionCompleteRedirectUrl: undefined, - iframeContext: true, - }); + expect(mockCreate).toHaveBeenCalledWith( + expect.objectContaining({ + strategy: 'oauth_google', + identifier: 'test@example.com', + redirectUrl: 'https://clerk.example.com/callback', + iframeContext: true, + }), + ); + // Optional: assert navigation target if applicable in this path + // expect(mockNavigate).toHaveBeenCalledWith('https://oauth.provider.com/auth');
87-116: Clarify test intent and reduce false positives.Explicitly set CHIPS=true so this test validates the “not in iframe” condition rather than passing because CHIPS might be falsy from a prior test. Also avoid asserting undefined keys.
it('should not set iframeContext when not in iframe', async () => { - vi.mocked(inIframe).mockReturnValue(false); + vi.mocked(inIframe).mockReturnValue(false); + (globalThis as any).__BUILD_VARIANT_CHIPS__ = true; @@ - expect(mockCreate).toHaveBeenCalledWith({ - strategy: 'oauth_google', - identifier: 'test@example.com', - redirectUrl: 'https://clerk.example.com/callback', - actionCompleteRedirectUrl: undefined, - }); + expect(mockCreate).toHaveBeenCalledWith( + expect.objectContaining({ + strategy: 'oauth_google', + identifier: 'test@example.com', + redirectUrl: 'https://clerk.example.com/callback', + }), + ); expect(mockCreate).toHaveBeenCalledWith(expect.not.objectContaining({ iframeContext: true }));
117-141: Optional: also assert redirectUrl building under non-CHIPS.Verifies we still use Clerk’s URL builder regardless of CHIPS.
await signIn.authenticateWithRedirectOrPopup(params, mockNavigate); expect(mockCreate).toHaveBeenCalledWith(expect.not.objectContaining({ iframeContext: true })); + expect(mockCreate).toHaveBeenCalledWith( + expect.objectContaining({ redirectUrl: 'https://clerk.example.com/callback' }), + );
143-167: Strengthen “continueSignIn” behavior check.Also assert that navigation occurs to the provider URL, since create is skipped.
await signIn.authenticateWithRedirectOrPopup(params, mockNavigate); expect(mockCreate).not.toHaveBeenCalled(); + expect(mockNavigate).toHaveBeenCalledWith('https://oauth.provider.com/auth');
198-223: Clarify test baseline for Future path (set CHIPS).Not required for correctness here, but setting CHIPS=true removes any ambiguity about why iframeContext is omitted.
it('should not set iframeContext when not in iframe', async () => { - vi.mocked(inIframe).mockReturnValue(false); + vi.mocked(inIframe).mockReturnValue(false); + (globalThis as any).__BUILD_VARIANT_CHIPS__ = true;
📜 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)
packages/clerk-js/src/core/resources/__tests__/SignIn.spec.ts(1 hunks)packages/clerk-js/src/core/resources/__tests__/SignUp.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/clerk-js/src/core/resources/tests/SignUp.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/clerk-js/src/core/resources/__tests__/SignIn.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/clerk-js/src/core/resources/__tests__/SignIn.spec.ts
packages/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
TypeScript is required for all packages
Files:
packages/clerk-js/src/core/resources/__tests__/SignIn.spec.ts
packages/**/*.{ts,tsx,d.ts}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Packages should export TypeScript types alongside runtime code
Files:
packages/clerk-js/src/core/resources/__tests__/SignIn.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/clerk-js/src/core/resources/__tests__/SignIn.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/clerk-js/src/core/resources/__tests__/SignIn.spec.ts
packages/{clerk-js,elements,themes}/**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/monorepo.mdc)
Visual regression testing should be performed for UI components.
Files:
packages/clerk-js/src/core/resources/__tests__/SignIn.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/clerk-js/src/core/resources/__tests__/SignIn.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/clerk-js/src/core/resources/__tests__/SignIn.spec.ts
🧬 Code graph analysis (1)
packages/clerk-js/src/core/resources/__tests__/SignIn.spec.ts (1)
packages/clerk-js/src/utils/runtime.ts (1)
inIframe(9-19)
⏰ 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). (4)
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Build Packages
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
packages/clerk-js/src/core/resources/__tests__/SignIn.spec.ts (2)
6-8: Mocking inIframe is clean and focused.Good isolation of the runtime check; keeps tests deterministic.
169-196: LGTM: Future path sets iframeContext correctly under CHIPS+iframe.Good coverage of the internal basePost payload.
| if (__BUILD_VARIANT_CHIPS__ && inIframe()) { | ||
| createParams.iframeContext = true; | ||
| } |
There was a problem hiding this comment.
This is the core change. When in CHIPS build and running in an iframe we want to send extra params to FAPI that indicate we are using partitioned cookies in an iframe. FAPI will skip validating the client_id in this scenario
| if (__BUILD_VARIANT_CHIPS__ && inIframe()) { | ||
| createParams.iframeContext = true; | ||
| } |
There was a problem hiding this comment.
Same as above, this is the core logical change
| if (__BUILD_VARIANT_CHIPS__ && inIframe()) { | ||
| finalParams.iframeContext = true; | ||
| } |
There was a problem hiding this comment.
Same as the SignIn change
| /** | ||
| * @internal Used to indicate the request is coming from an iframe context. | ||
| */ | ||
| iframeContext?: boolean; |
There was a problem hiding this comment.
Any thoughts on this param naming? It's meant to communicate partitioned cookies in an iframe, so perhaps iframeContext isn't descriptive anough
Description
When the CHIPS build of clerk-js runs inside an iframe, OAuth sign-in/sign-up flows fail because the backend attempts to validate the client_id against registered OAuth applications. The client_id differs between the iframe context and the parent window context, causing validation failures.
Fixes: USER-3277
Related: https://github.com/clerk/clerk_go/pull/13638
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit
New Features
Improvements
Types
Tests