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: 52c7174 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.
|
WalkthroughAdds a Protect runtime that reads Environment.protectConfig to optionally inject DOM loaders at runtime, introduces ProtectConfig resource and shared types/snapshots, re-exports the resource, includes integration tests for loader behavior, and increases the headless bundle size threshold. 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 loader(s) present
Protect->>Protect: set `#initialized`
loop for each loader
Protect->>Doc: createElement(loader.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)
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)
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.
| if (config.loader.type) { | ||
| 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; | ||
| } | ||
| } |
There was a problem hiding this comment.
Add error handling around DOM operations.
The document.createElement(config.loader.type) call on line 25 can throw a DOMException if config.loader.type contains invalid characters or is not a valid tag name. Since #initialized is set to true before attempting to create the element (line 22), any thrown error will prevent retry attempts, potentially leaving the protect functionality in a broken state with no observability.
Consider wrapping the DOM operations in a try-catch block:
// here rather than at end to mark as initialized even if load fails.
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]) => {
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;
}
}
+ } catch (error) {
+ // Consider logging the error for debugging
+ console.error('Failed to load protect element:', error);
+ }
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/clerk-js/src/core/protect.ts around lines 24 to 50, the DOM
operations (document.createElement and appending to head/body) can throw (e.g.,
DOMException) but #initialized is set before these ops; wrap the entire DOM
creation/attribute-setting/append sequence in a try-catch, only set #initialized
to true after successful completion (or reset it to false in the catch), and in
the catch log the error (console.error or the module logger) including the
thrown exception so callers can observe failures and retries remain possible.
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).
| 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; | ||
| } |
There was a problem hiding this comment.
Add null checks for document.head and document.body.
In rare cases (e.g., very early page load or non-standard document objects), document.head or document.body might be null, which would cause appendChild to throw a TypeError. Add null checks before appending.
Example fix (assuming error handling is added per previous comment):
switch (config.loader.target) {
case 'head':
+ if (!document.head) {
+ logger.warnOnce(`[protect] document.head is not available`);
+ break;
+ }
document.head.appendChild(element);
break;
case 'body':
+ if (!document.body) {
+ logger.warnOnce(`[protect] document.body is not available`);
+ break;
+ }
document.body.appendChild(element);
break;
default:
logger.warnOnce(`[protect] loader target is invalid: ${config.loader.target}`);
break;
}📝 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.
| 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; | |
| } | |
| switch (config.loader.target) { | |
| case 'head': | |
| if (!document.head) { | |
| logger.warnOnce(`[protect] document.head is not available`); | |
| break; | |
| } | |
| document.head.appendChild(element); | |
| break; | |
| case 'body': | |
| if (!document.body) { | |
| logger.warnOnce(`[protect] document.body is not available`); | |
| break; | |
| } | |
| document.body.appendChild(element); | |
| break; | |
| default: | |
| logger.warnOnce(`[protect] loader target is invalid: ${config.loader.target}`); | |
| break; | |
| } |
🤖 Prompt for AI Agents
In packages/clerk-js/src/core/protect.ts around lines 60-70, the switch appends
to document.head/body without checking for null; update each case to first test
that the target node exists (e.g., if (document.head) {
document.head.appendChild(element); } else { logger.warnOnce(`[protect]
document.head is null, falling back to document.documentElement or skipping`);
if (document.documentElement) document.documentElement.appendChild(element); })
and similarly for document.body; if neither fallback exists, log and skip append
to avoid a TypeError. Ensure checks satisfy TypeScript strictNullChecks if
enabled and keep the existing logger.warnOnce usage for diagnostics.
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.
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