feat(clerk-js): Add initial protect integration #7227
Conversation
Introduces the ProtectConfig resource and related types to support protect configuration in the environment. Adds a Protect class that loads a protect script based on environment config, and integrates it into Clerk initialization and environment loading.
🦋 Changeset detectedLatest commit: 4dce932 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdd a Protect runtime that reads Environment.protectConfig at startup to optionally inject configured DOM loaders; introduce ProtectConfig resource, shared types/snapshots and re-exports; add Playwright integration tests; raise headless bundle size threshold; wire Protect into Clerk initialization. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Clerk as Clerk
participant Protect as Protect
participant Env as Environment
participant Doc as Document
App->>Clerk: instantiate
activate Clerk
Clerk->>Protect: new Protect()
Clerk->>Protect: `#protect.load`(env)
activate Protect
Protect->>Env: read env.protectConfig
alt no protectConfig or not browser or already initialized
Protect-->>Protect: return (no-op)
else loaders present
Protect->>Protect: set `#initialized`
loop for each loader
Protect->>Doc: createElement(type || 'script')
Protect->>Doc: set attributes (stringify values)
Protect->>Doc: set textContent (if provided)
alt target == 'head'
Protect->>Doc: append to head
else target == 'body'
Protect->>Doc: append to body
else target starts with '#'
Protect->>Doc: append to element by id (or warn if missing)
end
end
end
deactivate Protect
deactivate Clerk
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)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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). (32)
Comment |
The 'enabled' property was removed from ProtectConfig interfaces and class. This simplifies the configuration and avoids redundant checks, relying on the presence of the loader to determine enablement.
@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: 1
🧹 Nitpick comments (2)
packages/clerk-js/src/core/clerk.ts (1)
522-522: Type cast may be unnecessary.The cast
this.environment as Environmentmight not be needed since theProtect.load()method already handles null/undefined environment gracefully with its guardif (!config?.loader). However, if this is to ensure TypeScript understands the type for the specific Environment class methods, it's acceptable.Consider whether the cast is necessary:
- this.#protect?.load(this.environment as Environment); + this.#protect?.load(this.environment);If the type system requires it due to the EnvironmentResource vs Environment distinction, the current approach is fine.
packages/clerk-js/src/core/protect.ts (1)
23-38: Add error handling for DOM operations.The code creates DOM elements and sets attributes from server configuration without error handling. While
createElementandsetAttributeare generally safe, they can throw exceptions for malformed input (e.g., invalid element types or attribute names).Consider wrapping the DOM operations in a try-catch block:
if (config.loader.type) { + try { const element = document.createElement(config.loader.type); if (config.loader.attributes) { Object.entries(config.loader.attributes).forEach(([key, value]) => element.setAttribute(key, String(value))); } switch (config.loader.target) { case 'head': document.head.appendChild(element); break; case 'body': document.body.appendChild(element); break; default: break; } + } catch (error) { + // Log or handle error appropriately + console.error('Failed to load protect config:', error); + } }Note on security: The code allows arbitrary element creation and attribute setting from server config. This is acceptable if the config source is fully trusted (server-controlled), but ensure that the protect config API has appropriate access controls.
📜 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 (10)
packages/clerk-js/src/core/clerk.ts(4 hunks)packages/clerk-js/src/core/protect.ts(1 hunks)packages/clerk-js/src/core/resources/Environment.ts(4 hunks)packages/clerk-js/src/core/resources/ProtectConfig.ts(1 hunks)packages/clerk-js/src/core/resources/internal.ts(1 hunks)packages/shared/src/types/environment.ts(2 hunks)packages/shared/src/types/index.ts(1 hunks)packages/shared/src/types/json.ts(2 hunks)packages/shared/src/types/protectConfig.ts(1 hunks)packages/shared/src/types/snapshots.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
packages/clerk-js/src/core/resources/Environment.ts (2)
packages/shared/src/types/protectConfig.ts (1)
ProtectConfigResource(17-22)packages/clerk-js/src/core/resources/ProtectConfig.ts (1)
ProtectConfig(10-41)
packages/shared/src/types/protectConfig.ts (1)
packages/shared/src/types/snapshots.ts (1)
ProtectConfigJSONSnapshot(123-123)
packages/shared/src/types/json.ts (1)
packages/shared/src/types/protectConfig.ts (1)
ProtectConfigJSON(10-15)
packages/shared/src/types/environment.ts (1)
packages/shared/src/types/protectConfig.ts (1)
ProtectConfigResource(17-22)
packages/clerk-js/src/core/resources/ProtectConfig.ts (2)
packages/shared/src/types/protectConfig.ts (3)
ProtectConfigResource(17-22)ProtectLoader(4-8)ProtectConfigJSON(10-15)packages/shared/src/types/snapshots.ts (1)
ProtectConfigJSONSnapshot(123-123)
packages/clerk-js/src/core/protect.ts (1)
packages/clerk-js/src/core/resources/Environment.ts (1)
Environment(18-104)
packages/shared/src/types/snapshots.ts (1)
packages/shared/src/types/protectConfig.ts (1)
ProtectConfigJSON(10-15)
packages/clerk-js/src/core/clerk.ts (2)
packages/clerk-js/src/core/protect.ts (1)
Protect(3-40)packages/clerk-js/src/core/resources/Environment.ts (1)
Environment(18-104)
⏰ 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: Build Packages
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (11)
packages/shared/src/types/protectConfig.ts (1)
1-22: LGTM! Type definitions are well-structured.The ProtectConfig type definitions follow the established patterns in the codebase. The three-tier structure (ProtectLoader → ProtectConfigJSON → ProtectConfigResource) provides good separation of concerns.
packages/shared/src/types/index.ts (1)
49-49: LGTM! Export correctly added.The re-export of protectConfig types is properly positioned in alphabetical order and follows the established pattern.
packages/shared/src/types/environment.ts (1)
6-6: LGTM! ProtectConfig integrated into EnvironmentResource.The protectConfig field is correctly added as a required field, consistent with other configuration resources in the Environment (authConfig, displayConfig, etc.).
Also applies to: 18-18
packages/clerk-js/src/core/resources/internal.ts (1)
32-32: LGTM! ProtectConfig export added to barrel.The export is correctly positioned alphabetically and follows the established pattern for resource exports.
packages/shared/src/types/snapshots.ts (1)
34-34: LGTM! Snapshot type correctly defined.The ProtectConfigJSONSnapshot alias follows the same pattern as other configuration snapshots (AuthConfigJSONSnapshot, DisplayConfigJSONSnapshot) that don't require type transformations.
Also applies to: 123-124
packages/clerk-js/src/core/clerk.ts (1)
161-161: LGTM! Protect initialization properly integrated.The Protect class is correctly integrated into the Clerk initialization flow:
- Instantiated in the constructor
- Called after environment and debug logging are set up
Also applies to: 231-231, 434-434
packages/shared/src/types/json.ts (1)
28-28: LGTM! ProtectConfig added to EnvironmentJSON.The protect_config field is correctly added to the EnvironmentJSON interface, following the snake_case naming convention and consistency with other configuration fields.
Also applies to: 94-94
packages/clerk-js/src/core/resources/Environment.ts (3)
9-9: LGTM! ProtectConfig resource properly declared.The protectConfig field is correctly added to the Environment class, following the same pattern as other configuration resources (authConfig, displayConfig, etc.).
Also applies to: 15-15, 30-30
59-59: LGTM! ProtectConfig correctly initialized from JSON.The fromJSON method properly instantiates ProtectConfig with the protect_config data, consistent with how other configuration resources are initialized.
101-101: LGTM! ProtectConfig included in environment snapshot.The protect_config field is correctly included in the snapshot serialization, maintaining consistency with the rest of the environment state.
packages/clerk-js/src/core/resources/ProtectConfig.ts (1)
1-41: LGTM! Clean resource implementation.The
ProtectConfigclass follows the establishedBaseResourcepattern used throughout the codebase (similar toAuthConfig,DisplayConfig, etc. in Environment.ts). The implementation correctly:
- Handles nullable data in
fromJSON- Uses
withDefaultfor safe property assignment- Implements the
ProtectConfigResourceinterface- Provides snapshot serialization via
__internal_toSnapshotThe optional
loaderfield is handled appropriately, and the types align with the shared type definitions.
zourzouvillys
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/clerk-js/src/core/protect.ts (1)
6-18: Check theenabledflag before proceeding.The code checks
!config?.loaderat line 9 but never verifiesconfig.enabled. According to theProtectConfigResourceinterface, there's anenabled: booleanfield that should gate whether the loader executes. The comment at line 10 claims "not enabled" but the code doesn't actually check this flag.Apply this diff to add the enabled check:
load(env: Environment): void { const config = env?.protectConfig; - if (!config?.loader) { - // not enabled or no protect config available + if (!config?.enabled || !config?.loader) { + // protection disabled or no loader config available return;
🧹 Nitpick comments (1)
packages/clerk-js/src/core/protect.ts (1)
23-38: Consider adding error handling for DOM operations.The DOM manipulation code lacks error handling. If
createElement,appendChild, or accessingdocument.head/document.bodythrows an exception, it will propagate uncaught. Since line 21 marks the instance as initialized before these operations, a failure would prevent any retry attempts.Additionally, note that the default case (lines 35-36) silently does nothing when
targetis neither'head'nor'body', meaning the element is created but never appended to the DOM.Consider wrapping the DOM operations in a try-catch block:
this.#initialized = true; + try { if (config.loader.type) { const element = document.createElement(config.loader.type); if (config.loader.attributes) { Object.entries(config.loader.attributes).forEach(([key, value]) => element.setAttribute(key, String(value))); } switch (config.loader.target) { case 'head': document.head.appendChild(element); break; case 'body': document.body.appendChild(element); break; default: break; } } + } catch (error) { + // Log or handle the error appropriately + console.error('Failed to load protect configuration:', error); + } } }
📜 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)
packages/clerk-js/src/core/protect.ts(1 hunks)packages/clerk-js/src/core/resources/ProtectConfig.ts(1 hunks)packages/shared/src/types/protectConfig.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/clerk-js/src/core/resources/ProtectConfig.ts
- packages/shared/src/types/protectConfig.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/clerk-js/src/core/protect.ts (1)
packages/clerk-js/src/core/resources/Environment.ts (1)
Environment(18-104)
⏰ 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 (1)
packages/clerk-js/src/core/protect.ts (1)
1-4: LGTM!The imports and class structure are clean. The private
#initializedflag appropriately uses modern JavaScript private field syntax.
Introduces tests to verify script creation based on protect_config.loader settings. Ensures the loader script is added when configured and absent otherwise.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 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 (1)
integration/tests/protect-service.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
integration/tests/protect-service.test.ts (3)
packages/shared/src/types/protectConfig.ts (1)
ProtectConfigJSON(10-14)integration/testUtils/index.ts (2)
testAgainstRunningApps(88-88)createTestUtils(24-86)integration/presets/index.ts (1)
appConfigs(15-32)
⏰ 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). (33)
- GitHub Check: Integration Tests (nextjs, chrome, 15)
- GitHub Check: Integration Tests (nextjs, chrome, 15, RQ)
- GitHub Check: Integration Tests (nextjs, chrome, 16)
- GitHub Check: Integration Tests (nextjs, chrome, 14)
- GitHub Check: Integration Tests (billing, chrome)
- GitHub Check: Integration Tests (quickstart, chrome, 16)
- GitHub Check: Integration Tests (billing, chrome, RQ)
- GitHub Check: Integration Tests (quickstart, chrome, 15)
- GitHub Check: Integration Tests (react-router, chrome)
- GitHub Check: Integration Tests (handshake:staging, chrome)
- GitHub Check: Integration Tests (custom, chrome)
- GitHub Check: Integration Tests (machine, chrome)
- GitHub Check: Integration Tests (vue, chrome)
- GitHub Check: Integration Tests (tanstack-react-start, chrome)
- GitHub Check: Integration Tests (nuxt, chrome)
- GitHub Check: Integration Tests (astro, chrome)
- GitHub Check: Integration Tests (handshake, chrome)
- GitHub Check: Integration Tests (expo-web, chrome)
- GitHub Check: Integration Tests (localhost, chrome)
- GitHub Check: Integration Tests (sessions, chrome)
- GitHub Check: Integration Tests (elements, chrome)
- GitHub Check: Integration Tests (sessions:staging, chrome)
- GitHub Check: Integration Tests (ap-flows, chrome)
- GitHub Check: Integration Tests (express, chrome)
- GitHub Check: Integration Tests (generic, chrome)
- GitHub Check: Publish with pkg-pr-new
- GitHub Check: Unit Tests (22, shared, clerk-js, RQ)
- GitHub Check: Unit Tests (22, **)
- GitHub Check: Static analysis
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (2)
integration/tests/protect-service.test.ts (2)
8-18: LGTM!The mocking helper correctly intercepts the environment API endpoint and overrides the
protect_configfield. The implementation properly fetches the original response and modifies the JSON before fulfilling the route.
27-41: LGTM!The test correctly verifies that a script element is created when
protect_config.loaderis provided. The setup, mocking, and assertion are all appropriate.
Adds type checking for loader attributes in Protect class to ensure only string, number, or boolean values are set as element attributes. Prevents illegal attribute values from being assigned.
Renamed the test from 'Hello World Div @xgeneric' to 'Clerk Protect checks' for clarity. Updated mockProtectSettings to accept an optional config parameter.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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 (1)
.changeset/beige-sloths-provide.md(1 hunks)
⏰ 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). (11)
- GitHub Check: Integration Tests (nextjs, chrome, 15)
- GitHub Check: Integration Tests (nextjs, chrome, 16)
- GitHub Check: Integration Tests (nextjs, chrome, 15, RQ)
- GitHub Check: Integration Tests (billing, chrome)
- GitHub Check: Integration Tests (billing, chrome, RQ)
- GitHub Check: Integration Tests (nextjs, chrome, 14)
- GitHub Check: Integration Tests (astro, chrome)
- GitHub Check: Integration Tests (machine, chrome)
- GitHub Check: Integration Tests (sessions:staging, chrome)
- GitHub Check: Integration Tests (sessions, chrome)
- GitHub Check: Integration Tests (generic, chrome)
Revised the changeset description to specify the introduction of the ProtectConfig resource and Protect loader integration for improved clarity.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.changeset/beige-sloths-provide.md (1)
6-6: Description could be slightly more user-focused.The description has been improved from the previous placeholder and now clearly identifies what was added. However, it could be marginally enhanced to explain the user-facing benefit—specifically that this enables protect configuration support in the environment with automatic script loader injection.
Consider revising to emphasize the user impact:
-Introduce ProtectConfig resource and Protect loader integration. +Introduce ProtectConfig resource and Protect loader integration to support protect configuration through the environment.Alternatively, if more context on the automatic loading behavior is valuable for users:
-Introduce ProtectConfig resource and Protect loader integration. +Introduce ProtectConfig resource and Protect loader integration. Enables protect configuration in the environment with automatic script loader injection.
📜 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 (1)
.changeset/beige-sloths-provide.md(1 hunks)
⏰ 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). (33)
- GitHub Check: Integration Tests (vue, chrome)
- GitHub Check: Integration Tests (custom, chrome)
- GitHub Check: Integration Tests (billing, chrome, RQ)
- GitHub Check: Integration Tests (quickstart, chrome, 16)
- GitHub Check: Integration Tests (nextjs, chrome, 16)
- GitHub Check: Integration Tests (quickstart, chrome, 15)
- GitHub Check: Integration Tests (nextjs, chrome, 14)
- GitHub Check: Integration Tests (nextjs, chrome, 15)
- GitHub Check: Integration Tests (billing, chrome)
- GitHub Check: Integration Tests (nuxt, chrome)
- GitHub Check: Integration Tests (react-router, chrome)
- GitHub Check: Integration Tests (nextjs, chrome, 15, RQ)
- GitHub Check: Integration Tests (machine, chrome)
- GitHub Check: Integration Tests (tanstack-react-start, chrome)
- GitHub Check: Integration Tests (localhost, chrome)
- GitHub Check: Integration Tests (sessions, chrome)
- GitHub Check: Unit Tests (22, shared, clerk-js, RQ)
- GitHub Check: Integration Tests (expo-web, chrome)
- GitHub Check: Integration Tests (handshake:staging, chrome)
- GitHub Check: Integration Tests (elements, chrome)
- GitHub Check: Integration Tests (ap-flows, chrome)
- GitHub Check: Integration Tests (astro, chrome)
- GitHub Check: Integration Tests (handshake, chrome)
- GitHub Check: Integration Tests (sessions:staging, chrome)
- GitHub Check: Integration Tests (generic, chrome)
- GitHub Check: Integration Tests (express, chrome)
- GitHub Check: Static analysis
- GitHub Check: Publish with pkg-pr-new
- GitHub Check: Unit Tests (22, **)
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
zourzouvillys
changed the title
zourzouvillys
changed the title
Replaces direct document type check with the inBrowser utility to determine browser environment in Protect class initialization.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/clerk-js/src/core/protect.ts (1)
24-49: Consider validating target before creating the element.The element is created on line 25 before checking if
config.loader.targetis valid (lines 40-49). If the target is neither'head'nor'body', the element is created but never appended to the DOM, which is wasteful.Consider checking the target first:
this.#initialized = true; - if (config.loader.type) { + if (config.loader.type && (config.loader.target === 'head' || config.loader.target === 'body')) { const element = document.createElement(config.loader.type); if (config.loader.attributes) { Object.entries(config.loader.attributes).forEach(([key, value]) => { switch (typeof value) { case 'string': case 'number': case 'boolean': element.setAttribute(key, String(value)); break; default: // illegal to set. break; } }); } switch (config.loader.target) { case 'head': document.head.appendChild(element); break; case 'body': document.body.appendChild(element); break; - default: - break; } }
📜 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 (1)
packages/clerk-js/src/core/protect.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/clerk-js/src/core/protect.ts (2)
packages/clerk-js/src/core/resources/Environment.ts (1)
Environment(18-104)packages/clerk-js/src/utils/runtime.ts (1)
inBrowser(1-3)
⏰ 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). (32)
- GitHub Check: Integration Tests (quickstart, chrome, 15)
- GitHub Check: Integration Tests (quickstart, chrome, 16)
- GitHub Check: Integration Tests (nextjs, chrome, 15)
- GitHub Check: Integration Tests (handshake:staging, chrome)
- GitHub Check: Integration Tests (custom, chrome)
- GitHub Check: Integration Tests (nextjs, chrome, 14)
- GitHub Check: Integration Tests (nextjs, chrome, 15, RQ)
- GitHub Check: Integration Tests (nextjs, chrome, 16)
- GitHub Check: Integration Tests (billing, chrome, RQ)
- GitHub Check: Integration Tests (tanstack-react-start, chrome)
- GitHub Check: Integration Tests (sessions, chrome)
- GitHub Check: Integration Tests (machine, chrome)
- GitHub Check: Integration Tests (expo-web, chrome)
- GitHub Check: Integration Tests (billing, chrome)
- GitHub Check: Integration Tests (nuxt, chrome)
- GitHub Check: Integration Tests (sessions:staging, chrome)
- GitHub Check: Integration Tests (vue, chrome)
- GitHub Check: Integration Tests (react-router, chrome)
- GitHub Check: Integration Tests (handshake, chrome)
- GitHub Check: Integration Tests (astro, chrome)
- GitHub Check: Integration Tests (elements, chrome)
- GitHub Check: Integration Tests (generic, chrome)
- GitHub Check: Integration Tests (localhost, chrome)
- GitHub Check: Integration Tests (ap-flows, chrome)
- GitHub Check: Integration Tests (express, chrome)
- GitHub Check: Publish with pkg-pr-new
- GitHub Check: Unit Tests (22, shared, clerk-js, RQ)
- GitHub Check: Static analysis
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (3)
packages/clerk-js/src/core/protect.ts (3)
1-6: LGTM! Clean imports and class structure.The use of
inBrowser()from@clerk/shared/browserfollows the recommendation from previous feedback, and the private#initializedfield appropriately encapsulates state.
7-19: LGTM! Early return guards are well-structured.The checks appropriately handle missing config, duplicate initialization, and non-browser environments. The clarification that presence of
loaderacts as the enablement flag is correctly implemented.
27-38: LGTM! Attribute validation correctly implemented.The type-checking switch statement properly filters attribute values to only allow primitives (string, number, boolean), addressing the previous review feedback. Invalid types are silently ignored, which is appropriate behavior.
Introduces a 'rollout' property to ProtectConfig, enabling percentage-based rollouts for loader initialization. Adds validation and debug logging for rollout values, loader types, attributes, and targets to improve error handling and observability.
Swapped out usage of debugLogger for logger from @clerk/shared/logger in protect.ts. Updated warning calls to use logger.warnOnce for improved logging consistency.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/clerk-js/src/core/protect.ts (1)
22-23: Add error handling to prevent silent failures.Setting
#initialized = truebefore DOM operations (line 23) means that ifdocument.createElement(line 43) orappendChild(lines 62, 65) throw an exception, the Protect instance will be marked as initialized but the protection element will never be loaded. This leaves no opportunity for retry and no observability into the failure.Wrap the DOM operations in a try-catch block and only set
#initialized = trueafter successful completion, or reset it tofalsein the catch block and log the error for debugging.- // here rather than at end to mark as initialized even if load fails. - this.#initialized = true; - // we use rollout for percentage based rollouts (as the environment file is cached) if (config.rollout) { if (typeof config.rollout !== 'number' || config.rollout < 0 || config.rollout > 1) { // invalid rollout percentage - do nothing logger.warnOnce(`[protect] loader rollout value is invalid: ${config.rollout}`); return; } if (Math.random() > config.rollout) { // not in rollout percentage - do nothing return; } } if (!config.loader.type) { logger.warnOnce(`[protect] loader type is missing`); return; } + try { const element = document.createElement(config.loader.type); if (config.loader.attributes) { Object.entries(config.loader.attributes).forEach(([key, value]) => { switch (typeof value) { case 'string': case 'number': case 'boolean': element.setAttribute(key, String(value)); break; default: // illegal to set. logger.warnOnce(`[protect] loader attribute is invalid type: ${key}=${value}`); break; } }); } switch (config.loader.target) { case 'head': document.head.appendChild(element); break; case 'body': document.body.appendChild(element); break; default: logger.warnOnce(`[protect] loader target is invalid: ${config.loader.target}`); break; } + // Mark as initialized only after successful load + this.#initialized = true; + } catch (error) { + logger.warnOnce(`[protect] failed to load: ${error}`); + }
📜 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)
packages/clerk-js/src/core/protect.ts(1 hunks)packages/clerk-js/src/core/resources/ProtectConfig.ts(1 hunks)packages/shared/src/types/protectConfig.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/clerk-js/src/core/resources/ProtectConfig.ts
🧰 Additional context used
🧬 Code graph analysis (2)
packages/shared/src/types/protectConfig.ts (1)
packages/shared/src/types/snapshots.ts (1)
ProtectConfigJSONSnapshot(123-123)
packages/clerk-js/src/core/protect.ts (2)
packages/clerk-js/src/core/resources/Environment.ts (1)
Environment(18-104)packages/clerk-js/src/utils/runtime.ts (1)
inBrowser(1-3)
⏰ 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: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (8)
packages/clerk-js/src/core/protect.ts (5)
1-7: LGTM! Good use of shared utilities.The imports correctly leverage shared utilities from
@clerk/shared, and the private field syntax is appropriate for tracking initialization state.
8-20: LGTM! Guards are well-structured.The early return guards are in a logical order and properly handle all edge cases: missing config, duplicate initialization, and non-browser environments.
25-36: LGTM! Rollout logic is correct.The rollout validation properly checks for a number in the range [0, 1], and the random comparison logic correctly implements percentage-based rollouts.
38-41: LGTM! Type validation is appropriate.Validating that
loader.typeexists before callingdocument.createElementprevents potential exceptions and provides clear logging.
43-59: LGTM! Attribute type validation is robust.The type checking for attributes properly restricts values to primitives (string, number, boolean) and logs warnings for invalid types, which addresses previous review feedback.
packages/shared/src/types/protectConfig.ts (3)
1-2: LGTM! Imports are appropriate.The imports follow the codebase's standard patterns for resource types and snapshots.
10-15: LGTM! JSON interface is well-structured.The interface follows the codebase's standard pattern with an object discriminator field, and optional
rolloutandloaderfields match how they're checked inprotect.ts.
17-22: LGTM! Resource interface follows codebase patterns.The interface extends
ClerkResourceappropriately, and the__internal_toSnapshotproperty matches the serialization pattern used in other resources (e.g.,Environment).
Updated ProtectConfig and related types to use an array of loaders instead of a single loader. Refactored Protect class logic to apply each loader individually, supporting rollout percentages, custom targets (including element IDs), and text content. This change enables more flexible and extensible protect configuration.
Introduces a test to verify that the loader script is not added when the protect_config loader's rollout is set to 0. Ensures correct behavior for conditional loader injection based on rollout value.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/clerk-js/src/core/protect.ts (3)
46-51: Consider using local variables instead of mutating the loader parameter.Mutating the incoming
loaderobject may have unintended side effects if the loader configuration is reused or stored elsewhere. Consider assigning defaults to local variables instead.- if (!loader.type) { - loader.type = 'script'; - } - if (!loader.target) { - loader.target = 'body'; - } - - const element = document.createElement(loader.type); + const type = loader.type || 'script'; + const target = loader.target || 'body'; + + const element = document.createElement(type);And update the references to
loader.targetin the switch statement below to use thetargetvariable.
53-53: Add error handling for DOM element creation.
document.createElement()can throw aDOMExceptionifloader.typecontains invalid characters or unsupported tag names. While the design appears to mark initialization complete regardless of failures (per line 23), unhandled exceptions could still disrupt the load sequence for subsequent loaders.Consider wrapping the DOM operations in try-catch:
+ try { const element = document.createElement(loader.type); if (loader.attributes) { // ... attribute setting code } if (loader.textContent && typeof loader.textContent === 'string') { element.textContent = loader.textContent; } switch (loader.target) { // ... target handling code } + } catch (error) { + logger.warnOnce(`[protect] Failed to create loader element: ${error}`); + return; + }This ensures that a malformed loader configuration won't prevent subsequent loaders from being applied.
75-94: Consider adding null checks for document.head and document.body.While the
inBrowser()check at line 18 makes this unlikely, in rare edge cases (very early page load or non-standard document objects),document.headordocument.bodymight benull, which would causeappendChildto throw a TypeError.switch (loader.target) { case 'head': + if (!document.head) { + logger.warnOnce(`[protect] document.head is not available`); + return; + } document.head.appendChild(element); break; case 'body': + if (!document.body) { + logger.warnOnce(`[protect] document.body is not available`); + return; + } document.body.appendChild(element); break;
📜 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 (4)
integration/tests/protect-service.test.ts(1 hunks)packages/clerk-js/src/core/protect.ts(1 hunks)packages/clerk-js/src/core/resources/ProtectConfig.ts(1 hunks)packages/shared/src/types/protectConfig.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/clerk-js/src/core/resources/ProtectConfig.ts
🧰 Additional context used
🧬 Code graph analysis (3)
packages/shared/src/types/protectConfig.ts (1)
packages/shared/src/types/snapshots.ts (1)
ProtectConfigJSONSnapshot(123-123)
packages/clerk-js/src/core/protect.ts (3)
packages/clerk-js/src/core/resources/Environment.ts (1)
Environment(18-104)packages/clerk-js/src/utils/runtime.ts (1)
inBrowser(1-3)packages/shared/src/types/protectConfig.ts (1)
ProtectLoader(4-10)
integration/tests/protect-service.test.ts (3)
packages/shared/src/types/protectConfig.ts (1)
ProtectConfigJSON(12-16)integration/testUtils/index.ts (1)
createTestUtils(24-86)integration/presets/index.ts (1)
appConfigs(15-32)
⏰ 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 (7)
packages/shared/src/types/protectConfig.ts (1)
1-22: LGTM! Type definitions are well-structured.The interfaces are cleanly defined with appropriate types. The past review comment regarding optional
attributeshas been properly addressed.packages/clerk-js/src/core/protect.ts (2)
9-29: Load method logic is sound.The early-return guards are well-structured, checking for missing loaders, initialization state, and browser context before proceeding. The decision to mark as initialized before applying loaders (line 24) appears intentional per the inline comment, preventing repeated attempts even if individual loaders fail.
32-44: Rollout logic is correctly implemented.The validation ensures rollout is a number between 0 and 1, with appropriate warning logging for invalid values. The Math.random() comparison correctly implements percentage-based rollout.
integration/tests/protect-service.test.ts (4)
8-18: Mock helper is well-implemented.The
mockProtectSettingshelper correctly intercepts and modifies the environment API response, allowing tests to inject custom protect configurations. The conditional handling for config presence/absence is appropriate.
27-45: Test correctly verifies loader creation.The test properly configures a protect loader with
rollout: 1.0to ensure deterministic behavior and validates that the expected DOM element is created with the correct attributes.
47-65: Test correctly verifies rollout behavior.The test properly validates that a loader with
rollout: 0is never applied, using the appropriate Playwright assertiontoHaveCount(0)to verify the element's absence.
67-75: Test correctly verifies behavior when config is absent.The test properly validates that no loader is created when
protect_configis not provided. The inline comment explaining Playwright locator behavior is helpful for maintainability.
Wraps loader application in a try-catch block and logs a warning if a loader fails to apply, improving robustness during initialization by continuing to load other loaders if present.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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 (1)
packages/clerk-js/src/core/protect.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/clerk-js/src/core/protect.ts (3)
packages/clerk-js/src/core/resources/Environment.ts (1)
Environment(18-104)packages/clerk-js/src/utils/runtime.ts (1)
inBrowser(1-3)packages/shared/src/types/protectConfig.ts (1)
ProtectLoader(4-10)
⏰ 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). (31)
- GitHub Check: Integration Tests (billing, chrome)
- GitHub Check: Integration Tests (quickstart, chrome, 16)
- GitHub Check: Integration Tests (nextjs, chrome, 16)
- GitHub Check: Integration Tests (machine, chrome)
- GitHub Check: Integration Tests (nextjs, chrome, 15)
- GitHub Check: Integration Tests (custom, chrome)
- GitHub Check: Integration Tests (quickstart, chrome, 15)
- GitHub Check: Integration Tests (nextjs, chrome, 14)
- GitHub Check: Integration Tests (react-router, chrome)
- GitHub Check: Integration Tests (handshake:staging, 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 (expo-web, chrome)
- GitHub Check: Integration Tests (astro, chrome)
- GitHub Check: Integration Tests (sessions, chrome)
- GitHub Check: Integration Tests (localhost, chrome)
- GitHub Check: Integration Tests (handshake, chrome)
- GitHub Check: Integration Tests (elements, chrome)
- GitHub Check: Integration Tests (sessions:staging, chrome)
- GitHub Check: Integration Tests (ap-flows, chrome)
- GitHub Check: Integration Tests (express, chrome)
- GitHub Check: Integration Tests (generic, chrome)
- GitHub Check: Unit Tests (22, shared, clerk-js, RQ)
- GitHub Check: Unit Tests (22, **)
- GitHub Check: Publish with pkg-pr-new
- GitHub Check: Static analysis
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (5)
packages/clerk-js/src/core/protect.ts (5)
9-21: LGTM! Well-structured guard clauses.The early return logic correctly validates the environment config, initialization state, and browser context before proceeding. The checks are comprehensive and in the right order.
23-33: LGTM! Solid error handling strategy.Setting
#initializedtotruebefore applying loaders prevents retry loops, and the per-loader try-catch ensures one failing loader doesn't block others. The error logging provides appropriate observability.
36-48: LGTM! Proper rollout validation and application.The rollout logic correctly validates the range [0,1] and uses client-side randomness to determine loader activation. The validation and logging provide appropriate safeguards.
57-77: LGTM! Safe element creation and attribute handling.The element creation and attribute setting logic is well-implemented with proper type validation. The switch statement correctly filters attribute types, and the textContent validation adds safety. Any DOM exceptions will be caught by the try-catch in the
load()method.
79-98: LGTM! Comprehensive target handling.The target switch correctly handles
head,body, and element-by-ID cases. The validation for missing elements and logging for invalid targets provide appropriate safeguards.
Replaces direct mutation of loader.type and loader.target with local variables, improving code clarity and preventing side effects. Updates element creation and target selection logic to use these local variables.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/clerk-js/src/core/protect.ts (2)
38-51: Consider documenting rollout behavior and aligning type definitions.Two minor observations:
Rollout randomness (line 44): Using
Math.random()means rollout decisions are made per page load, not per user or session. This could result in inconsistent protection for the same user across page loads. If this is intentional for canary testing, consider documenting it; otherwise, you might want to use a deterministic approach based on a user/session identifier.Type definition alignment (lines 50-51): The
ProtectLoaderinterface definestypeandtargetas required fields, but the code provides fallbacks as if they're optional. Consider making these fields optional in the interface to match the implementation:export interface ProtectLoader { rollout?: number; target?: 'head' | 'body' | `#${string}`; // Make optional type?: string; // Make optional attributes?: Record<string, string | number | boolean>; textContent?: string; }This change should be made in
packages/shared/src/types/protectConfig.ts.
53-95: LGTM! Safe DOM manipulation with good validation.The implementation is solid:
- Attribute type validation (lines 57-67) correctly restricts to primitives and logs invalid types
- Using
textContent(line 72) instead ofinnerHTMLprevents XSS risks- Target element validation (lines 84-87) handles missing elements gracefully
Optional enhancement: The custom selector support (line 83) is limited to
#idformat. If you later need more flexibility (e.g., class selectors or data attributes), consider usingdocument.querySelector(target)instead of justgetElementById. However, the current approach is simpler and perfectly adequate for the initial implementation.
📜 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 (1)
packages/clerk-js/src/core/protect.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/clerk-js/src/core/protect.ts (3)
packages/clerk-js/src/core/resources/Environment.ts (1)
Environment(18-104)packages/clerk-js/src/utils/runtime.ts (1)
inBrowser(1-3)packages/shared/src/types/protectConfig.ts (1)
ProtectLoader(4-10)
⏰ 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). (32)
- GitHub Check: Integration Tests (custom, chrome)
- GitHub Check: Integration Tests (nextjs, chrome, 16)
- GitHub Check: Integration Tests (quickstart, chrome, 16)
- GitHub Check: Integration Tests (nuxt, chrome)
- GitHub Check: Integration Tests (quickstart, chrome, 15)
- GitHub Check: Integration Tests (react-router, chrome)
- GitHub Check: Integration Tests (billing, chrome)
- GitHub Check: Integration Tests (nextjs, chrome, 15)
- GitHub Check: Integration Tests (billing, chrome, RQ)
- GitHub Check: Integration Tests (nextjs, chrome, 15, RQ)
- GitHub Check: Integration Tests (nextjs, chrome, 14)
- GitHub Check: Integration Tests (vue, chrome)
- GitHub Check: Integration Tests (machine, chrome)
- GitHub Check: Integration Tests (sessions:staging, chrome)
- GitHub Check: Integration Tests (tanstack-react-start, chrome)
- GitHub Check: Integration Tests (astro, chrome)
- GitHub Check: Integration Tests (handshake, chrome)
- GitHub Check: Integration Tests (sessions, chrome)
- GitHub Check: Integration Tests (expo-web, chrome)
- GitHub Check: Integration Tests (handshake:staging, chrome)
- GitHub Check: Integration Tests (localhost, chrome)
- GitHub Check: Integration Tests (express, chrome)
- GitHub Check: Integration Tests (elements, chrome)
- GitHub Check: Integration Tests (generic, chrome)
- GitHub Check: Integration Tests (ap-flows, chrome)
- GitHub Check: Publish with pkg-pr-new
- GitHub Check: Unit Tests (22, shared, clerk-js, RQ)
- GitHub Check: Static analysis
- GitHub Check: Unit Tests (22, **)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (2)
packages/clerk-js/src/core/protect.ts (2)
1-8: LGTM! Clean imports and initialization.The imports are well-organized, correctly using
inBrowserfrom@clerk/shared/browseras suggested in previous reviews. The private#initializedflag provides a simple and effective guard against duplicate initialization.
9-33: LGTM! Robust guard conditions and error handling.The method correctly validates the configuration, checks initialization state, and verifies browser environment before proceeding. The per-loader try-catch (lines 27-31) is excellent—it ensures one misconfigured loader doesn't prevent others from loading, which is critical for a protection system.
Updated loader script element IDs in integration tests for clarity and uniqueness. Improved rollout logic in Protect class to handle 0% rollout and invalid values more robustly, ensuring loader is not applied when rollout is zero or invalid.
Introduces intial Clerk Protect dynamic loader and related types to support dynamically enabling and rollout out protect in the environment.
Description
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit
New Features
Types
Tests
Chores