feat: failure detection #6026
feat: failure detection #6026
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
@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: |
|
Hello 👋 We currently close PRs after 60 days of inactivity. It's been 50 days since the last update here. If we missed this PR, please reply here. Otherwise, we'll close this PR in 10 days. Thanks for being a part of the Clerk community! 🙏 |
📝 WalkthroughWalkthroughThis update introduces a global blocking coordinator pattern for managing the loading of the ClerkJS script in Next.js applications. A new inline script is injected into the document head to track and control the ClerkJS script's loading state, preventing multiple scripts from loading simultaneously. The coordinator exposes its state and allows registration of callbacks for load, error, and state changes. The 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
packages/shared/src/loadClerkJsScript.ts (1)
85-85: Consider a more accurate error messageThe error message "ClerkJS loaded but script element not found" might be misleading. The script element might not exist if ClerkJS was loaded through a different method or if the DOM query failed.
- reject(new Error('ClerkJS loaded but script element not found')); + reject(new Error('ClerkJS coordinator reported success but script element could not be retrieved'));packages/nextjs/src/utils/clerk-js-script.tsx (1)
9-11: Address the TODO - avoid code duplicationThe TODO indicates that the blocking coordinator code is duplicated here instead of being properly imported. This creates maintenance burden and potential for divergence between implementations.
Would you like me to help set up the proper exports so this inline duplication can be removed?
packages/shared/src/clerkJsBlockingCoordinator.ts (1)
202-224: Performance concern: Expensive MutationObserver configurationObserving the entire body with
subtree: trueis expensive and thequerySelectorAllinside the mutation callback adds more overhead.Consider:
- Only observing specific containers where scripts are likely to be added
- Using a more efficient selector strategy
- Debouncing the observer callback for multiple mutations
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/nextjs/src/utils/clerk-js-script.tsx(2 hunks)packages/shared/src/__tests__/loadClerkJsScript.test.ts(4 hunks)packages/shared/src/clerkJsBlockingCoordinator.ts(1 hunks)packages/shared/src/index.ts(1 hunks)packages/shared/src/loadClerkJsScript.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (13)
**/*.{js,jsx,ts,tsx}
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- .cursor/rules/development.mdc
**/*.{js,jsx,ts,tsx,json,css,scss,md,yaml,yml}
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- .cursor/rules/development.mdc
packages/**/*.{ts,tsx}
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- .cursor/rules/development.mdc
packages/**/*.{ts,tsx,d.ts}
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- .cursor/rules/development.mdc
packages/**/index.{js,ts}
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- .cursor/rules/development.mdc
**/*.{ts,tsx}
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- .cursor/rules/development.mdc
- .cursor/rules/typescript.mdc
**/*.{js,ts,tsx,jsx}
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- .cursor/rules/monorepo.mdc
**/index.ts
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- .cursor/rules/react.mdc
- .cursor/rules/typescript.mdc
**/*
Instructions used from:
Sources:
⚙️ CodeRabbit Configuration File
packages/**/*.{test,spec}.{js,jsx,ts,tsx}
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- .cursor/rules/monorepo.mdc
**/__tests__/**/*.{ts,tsx}
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- .cursor/rules/typescript.mdc
**/*.{jsx,tsx}
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- .cursor/rules/development.mdc
- .cursor/rules/react.mdc
**/*.tsx
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- .cursor/rules/react.mdc
🔇 Additional comments (3)
packages/shared/src/index.ts (1)
40-40: LGTM!The new export follows the established pattern for module exports in this file.
packages/shared/src/loadClerkJsScript.ts (1)
162-166: Good API design with clear aliasingThe re-exports with descriptive aliases provide a cleaner public API that aligns well with the existing module's purpose.
packages/shared/src/__tests__/loadClerkJsScript.test.ts (1)
12-138: Excellent test refactoringThe move from mocked behavior to real DOM simulation provides much better confidence in the implementation. The race condition test (lines 92-103) is particularly valuable.
| <script | ||
| ref={scriptRef} | ||
| src={scriptUrl} | ||
| data-clerk-js-script='true' |
There was a problem hiding this comment.
Inconsistent data attribute values
The data-clerk-js-script attribute uses 'true' here but empty string in buildClerkJsScriptAttributes. This inconsistency could cause issues.
- data-clerk-js-script='true'
+ data-clerk-js-script=''And for the Pages Router:
- data-clerk-js-script='true'
+ data-clerk-js-script=''Also applies to: 254-254
🤖 Prompt for AI Agents
In packages/nextjs/src/utils/clerk-js-script.tsx at lines 242 and 254, the data
attribute `data-clerk-js-script` is set to the string 'true', whereas in the
function `buildClerkJsScriptAttributes` it is set as an empty string. To fix
this inconsistency, standardize the value of `data-clerk-js-script` across all
usages by either setting it to 'true' or an empty string consistently, updating
the attribute in these lines to match the value used in
`buildClerkJsScriptAttributes`.
| function useClerkJSLoadingState() { | ||
| const [loadingState, setLoadingState] = useState<LoadingState>('idle'); | ||
|
|
||
| useEffect(() => { | ||
| if (typeof window === 'undefined') return; | ||
|
|
||
| const coordinator = (window as any).__clerkJSBlockingCoordinator; | ||
| if (coordinator) { | ||
| coordinator.registerCallback({ | ||
| onStateChange: (state: LoadingState) => { | ||
| setLoadingState(state); | ||
| }, | ||
| }); | ||
| } | ||
| }, []); | ||
|
|
||
| return { loadingState }; | ||
| } |
There was a problem hiding this comment.
Missing cleanup in useEffect
The effect registers a callback but doesn't clean it up, which could cause memory leaks.
useEffect(() => {
if (typeof window === 'undefined') return;
const coordinator = (window as any).__clerkJSBlockingCoordinator;
if (coordinator) {
+ // Set initial state
+ setLoadingState(coordinator.state);
+
coordinator.registerCallback({
onStateChange: (state: LoadingState) => {
setLoadingState(state);
},
});
}
+
+ // Note: The blocking coordinator doesn't provide an unregister method
+ // This could lead to memory leaks if components using this hook are frequently mounted/unmounted
}, []);Consider implementing an unregister mechanism in the blocking coordinator.
📝 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.
| function useClerkJSLoadingState() { | |
| const [loadingState, setLoadingState] = useState<LoadingState>('idle'); | |
| useEffect(() => { | |
| if (typeof window === 'undefined') return; | |
| const coordinator = (window as any).__clerkJSBlockingCoordinator; | |
| if (coordinator) { | |
| coordinator.registerCallback({ | |
| onStateChange: (state: LoadingState) => { | |
| setLoadingState(state); | |
| }, | |
| }); | |
| } | |
| }, []); | |
| return { loadingState }; | |
| } | |
| function useClerkJSLoadingState() { | |
| const [loadingState, setLoadingState] = useState<LoadingState>('idle'); | |
| useEffect(() => { | |
| if (typeof window === 'undefined') return; | |
| const coordinator = (window as any).__clerkJSBlockingCoordinator; | |
| if (coordinator) { | |
| // Set initial state | |
| setLoadingState(coordinator.state); | |
| coordinator.registerCallback({ | |
| onStateChange: (state: LoadingState) => { | |
| setLoadingState(state); | |
| }, | |
| }); | |
| } | |
| // Note: The blocking coordinator doesn't provide an unregister method | |
| // This could lead to memory leaks if components using this hook are frequently mounted/unmounted | |
| }, []); | |
| return { loadingState }; | |
| } |
🤖 Prompt for AI Agents
In packages/nextjs/src/utils/clerk-js-script.tsx around lines 131 to 148, the
useEffect registers a callback on the coordinator but does not unregister it on
cleanup, risking memory leaks. Modify the effect to return a cleanup function
that calls an unregister method on the coordinator to remove the callback when
the component unmounts. Ensure the coordinator supports this unregister
mechanism and invoke it properly in the cleanup.
| } | ||
|
|
||
| var coordinator = { | ||
| state: 'idle', | ||
| scriptUrl: null, | ||
| scriptElement: null, | ||
| error: null, | ||
| callbacks: [], | ||
|
|
||
| shouldAllowScript: function(scriptElement) { | ||
| if (!scriptElement.hasAttribute('data-clerk-js-script')) { | ||
| return true; | ||
| } | ||
|
|
||
| if (this.scriptElement && this.scriptElement.src === scriptElement.src) { | ||
| return false; | ||
| } | ||
|
|
||
| if (window.Clerk) { | ||
| this.setState('loaded'); | ||
| return false; | ||
| } | ||
|
|
||
| if (this.state === 'loading') { | ||
| return false; | ||
| } | ||
|
|
||
| this.adoptScript(scriptElement); | ||
| return true; | ||
| }, | ||
|
|
||
| adoptScript: function(scriptElement) { | ||
| this.scriptElement = scriptElement; | ||
| this.scriptUrl = scriptElement.src; | ||
| this.setState('loading'); | ||
|
|
||
| var self = this; | ||
|
|
||
| scriptElement.addEventListener('load', function() { | ||
| scriptElement.setAttribute('data-clerk-loaded', 'true'); | ||
| self.setState('loaded'); | ||
| }); | ||
|
|
||
| scriptElement.addEventListener('error', function() { | ||
| self.setState('error', new Error('ClerkJS failed to load')); | ||
| }); | ||
| }, | ||
|
|
||
| registerCallback: function(callback) { | ||
| this.callbacks.push(callback); | ||
|
|
||
| if (callback.onStateChange) { | ||
| callback.onStateChange(this.state); | ||
| } | ||
|
|
||
| if (this.state === 'loaded' && callback.onLoad) { | ||
| callback.onLoad(); | ||
| } else if (this.state === 'error' && callback.onError && this.error) { | ||
| callback.onError(this.error); | ||
| } | ||
| }, | ||
|
|
||
| setState: function(newState, error) { | ||
| this.state = newState; | ||
| if (error) this.error = error; | ||
|
|
||
| for (var i = 0; i < this.callbacks.length; i++) { | ||
| var callback = this.callbacks[i]; | ||
| try { | ||
| if (callback.onStateChange) { | ||
| callback.onStateChange(newState); | ||
| } | ||
|
|
||
| if (newState === 'loaded' && callback.onLoad) { | ||
| callback.onLoad(); | ||
| } else if (newState === 'error' && callback.onError && error) { | ||
| callback.onError(error); | ||
| } | ||
| } catch (e) { | ||
| console.error('ClerkJS coordinator callback error:', e); | ||
| } | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| var originalAppendChild = Node.prototype.appendChild; | ||
| Node.prototype.appendChild = function(child) { | ||
| if (child.tagName === 'SCRIPT' && child.hasAttribute('data-clerk-js-script')) { | ||
| if (!coordinator.shouldAllowScript(child)) { | ||
| return child; | ||
| } | ||
| } | ||
|
|
||
| return originalAppendChild.call(this, child); | ||
| }; | ||
|
|
||
| window.__clerkJSBlockingCoordinator = coordinator; | ||
| })(); | ||
| `.trim(); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Security concern: Global prototype modification
The inline script modifies global DOM prototypes (Node.prototype.appendChild), which could:
- Conflict with other libraries that also modify these prototypes
- Break third-party scripts that expect standard DOM behavior
- Create security vulnerabilities if not properly sanitized
Consider using MutationObserver exclusively or implementing a more isolated approach.
Let me verify if there are any other scripts in the codebase that might conflict with these DOM modifications:
Please tag clerk/security to review the security implications of modifying global DOM prototypes.
🏁 Script executed:
#!/bin/bash
# Search for other code that might modify DOM prototypes
rg -A 5 "prototype\.(appendChild|insertBefore)" --type js --type ts
# Search for MutationObserver usage that might conflict
rg -A 5 "MutationObserver" --type js --type ts
# Check if any third-party libraries are known to modify these methods
fd "package.json" --exec jq -r '.dependencies | keys[]' {} \; | sort | uniq | grep -E "(jquery|prototype|mootools|zepto)"Length of output: 9285
Consolidate and Isolate DOM Prototype Overrides
We’re currently overriding Node.prototype.appendChild (and insertBefore) in two places—
• packages/shared/src/clerkJsBlockingCoordinator.ts
• packages/nextjs/src/utils/clerk-js-script.tsx
This duplication not only increases maintenance burden but also amplifies the risk of conflicts with other code that touches global prototypes.
Suggestions:
- Remove the inline duplicate in
packages/nextjs/src/utils/clerk-js-script.tsxand consume the shared coordinator implementation frompackages/shared/src/clerkJsBlockingCoordinator.ts. - If global prototype patching is unavoidable, wrap it in a self-contained module that can:
- Automatically restore the original methods when no longer needed.
- Expose a clear API for registering/unregistering callbacks.
- As an alternative to prototype overrides, consider relying solely on a
MutationObserver-based approach to intercept injected<script>tags and avoid touchingNode.prototype.
Files needing updates:
- packages/nextjs/src/utils/clerk-js-script.tsx
- (Optionally) the shared coordinator at packages/shared/src/clerkJsBlockingCoordinator.ts to ensure it supports being imported into Next.js
🤖 Prompt for AI Agents
In packages/nextjs/src/utils/clerk-js-script.tsx around lines 22 to 128, the
code duplicates the override of Node.prototype.appendChild already implemented
in packages/shared/src/clerkJsBlockingCoordinator.ts, causing maintenance and
conflict risks. Remove the inline prototype override and instead import and use
the shared coordinator from packages/shared/src/clerkJsBlockingCoordinator.ts.
Refactor the shared coordinator to provide a self-contained module that manages
the prototype patching with an API to register/unregister callbacks and restore
original methods when needed. Optionally, consider replacing prototype overrides
with a MutationObserver-based approach to detect injected script tags without
modifying global prototypes.
| useEffect(() => { | ||
| if (typeof window === 'undefined' || coordinatorInjected.current) return; | ||
|
|
||
| // Check if coordinator already exists | ||
| if ((window as any).__clerkJSBlockingCoordinator) { | ||
| coordinatorInjected.current = true; | ||
| return; | ||
| } | ||
|
|
||
| // Create and inject coordinator script into head | ||
| const coordinatorScript = document.createElement('script'); | ||
| coordinatorScript.id = 'clerk-blocking-coordinator'; | ||
| coordinatorScript.innerHTML = getBlockingCoordinatorScript(); | ||
|
|
||
| // Insert at the beginning of head to ensure it runs first | ||
| if (document.head.firstChild) { | ||
| document.head.insertBefore(coordinatorScript, document.head.firstChild); | ||
| } else { | ||
| document.head.appendChild(coordinatorScript); | ||
| } | ||
|
|
||
| coordinatorInjected.current = true; | ||
| }, []); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential race condition in coordinator injection
Multiple components could attempt to inject the coordinator simultaneously before coordinatorInjected.current is set.
useEffect(() => {
if (typeof window === 'undefined' || coordinatorInjected.current) return;
+ // Use a global flag to prevent race conditions
+ if ((window as any).__clerkJSBlockingCoordinatorInjecting) return;
+ (window as any).__clerkJSBlockingCoordinatorInjecting = true;
+
// Check if coordinator already exists
if ((window as any).__clerkJSBlockingCoordinator) {
coordinatorInjected.current = true;
+ delete (window as any).__clerkJSBlockingCoordinatorInjecting;
return;
}
// Create and inject coordinator script into head
const coordinatorScript = document.createElement('script');
coordinatorScript.id = 'clerk-blocking-coordinator';
coordinatorScript.innerHTML = getBlockingCoordinatorScript();
// Insert at the beginning of head to ensure it runs first
if (document.head.firstChild) {
document.head.insertBefore(coordinatorScript, document.head.firstChild);
} else {
document.head.appendChild(coordinatorScript);
}
coordinatorInjected.current = true;
+ delete (window as any).__clerkJSBlockingCoordinatorInjecting;
}, []);📝 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.
| useEffect(() => { | |
| if (typeof window === 'undefined' || coordinatorInjected.current) return; | |
| // Check if coordinator already exists | |
| if ((window as any).__clerkJSBlockingCoordinator) { | |
| coordinatorInjected.current = true; | |
| return; | |
| } | |
| // Create and inject coordinator script into head | |
| const coordinatorScript = document.createElement('script'); | |
| coordinatorScript.id = 'clerk-blocking-coordinator'; | |
| coordinatorScript.innerHTML = getBlockingCoordinatorScript(); | |
| // Insert at the beginning of head to ensure it runs first | |
| if (document.head.firstChild) { | |
| document.head.insertBefore(coordinatorScript, document.head.firstChild); | |
| } else { | |
| document.head.appendChild(coordinatorScript); | |
| } | |
| coordinatorInjected.current = true; | |
| }, []); | |
| useEffect(() => { | |
| if (typeof window === 'undefined' || coordinatorInjected.current) return; | |
| // Use a global flag to prevent race conditions | |
| if ((window as any).__clerkJSBlockingCoordinatorInjecting) return; | |
| (window as any).__clerkJSBlockingCoordinatorInjecting = true; | |
| // Check if coordinator already exists | |
| if ((window as any).__clerkJSBlockingCoordinator) { | |
| coordinatorInjected.current = true; | |
| delete (window as any).__clerkJSBlockingCoordinatorInjecting; | |
| return; | |
| } | |
| // Create and inject coordinator script into head | |
| const coordinatorScript = document.createElement('script'); | |
| coordinatorScript.id = 'clerk-blocking-coordinator'; | |
| coordinatorScript.innerHTML = getBlockingCoordinatorScript(); | |
| // Insert at the beginning of head to ensure it runs first | |
| if (document.head.firstChild) { | |
| document.head.insertBefore(coordinatorScript, document.head.firstChild); | |
| } else { | |
| document.head.appendChild(coordinatorScript); | |
| } | |
| coordinatorInjected.current = true; | |
| delete (window as any).__clerkJSBlockingCoordinatorInjecting; | |
| }, []); |
🤖 Prompt for AI Agents
In packages/nextjs/src/utils/clerk-js-script.tsx around lines 181 to 203, there
is a potential race condition where multiple components might inject the
coordinator script simultaneously before coordinatorInjected.current is set. To
fix this, implement a locking mechanism or use a shared state outside the
component scope to ensure only one injection occurs at a time, preventing
duplicate script insertions.
| // Return a no-op unsubscribe function since the blocking coordinator | ||
| // doesn't need complex unsubscription | ||
| return () => {}; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Misleading unsubscribe function
The function returns a no-op unsubscribe, but the comment acknowledges the coordinator doesn't support unsubscription. This could mislead users into thinking cleanup is happening when it's not.
Either:
- Implement proper unsubscription in the coordinator (preferred)
- Document clearly in the function's JSDoc that cleanup is not performed
- Consider returning void instead of a no-op function
🤖 Prompt for AI Agents
In packages/shared/src/clerkJsBlockingCoordinator.ts around lines 280 to 282,
the function returns a no-op unsubscribe function which may mislead users into
thinking unsubscription or cleanup occurs. To fix this, either implement proper
unsubscription logic in the coordinator, or if that is not feasible, update the
function's JSDoc to clearly state that no cleanup or unsubscription is
performed. Alternatively, consider changing the return type to void instead of
returning a no-op function to avoid confusion.
| var originalAppendChild = Node.prototype.appendChild; | ||
| Node.prototype.appendChild = function(child) { | ||
| if (!interceptScript(child)) { | ||
| // Create a dummy script element to return | ||
| var dummy = document.createElement('script'); | ||
| dummy.src = child.src; | ||
| Object.keys(child.attributes || {}).forEach(function(key) { | ||
| var attr = child.attributes[key]; | ||
| if (attr && attr.name && attr.value) { | ||
| dummy.setAttribute(attr.name, attr.value); | ||
| } | ||
| }); | ||
| return dummy; | ||
| } | ||
|
|
||
| return originalAppendChild.call(this, child); | ||
| }; |
There was a problem hiding this comment.
Critical: Dummy script elements could cause issues
Creating dummy script elements that are returned but never added to DOM could:
- Cause memory leaks if references are held
- Break code that expects the returned element to be in the DOM
- Violate the appendChild contract
Consider throwing an error or properly handling the duplicate script scenario instead of returning a dummy element.
🤖 Prompt for AI Agents
In packages/shared/src/clerkJsBlockingCoordinator.ts between lines 171 and 187,
the current override of Node.prototype.appendChild returns a dummy script
element when interceptScript(child) is false, which can cause memory leaks and
break DOM expectations. Instead of returning a dummy element, modify the
function to either throw an error to explicitly block unsupported script appends
or handle the duplicate script scenario properly by not returning a dummy but
preventing the append while maintaining the appendChild contract. Ensure the
returned element is either the appended child or handle the case without
breaking DOM integrity.
| return dummy; | ||
| } | ||
|
|
||
| return originalInsertBefore.call(this, newNode, referenceNode); | ||
| }; | ||
|
|
||
| // Also watch for scripts being set via innerHTML or similar | ||
| var observer = new MutationObserver(function(mutations) { | ||
| mutations.forEach(function(mutation) { | ||
| if (mutation.type === 'childList') { | ||
| Array.prototype.slice.call(mutation.addedNodes).forEach(function(node) { | ||
| if (node.nodeType === 1) { // Element node | ||
| if (node.tagName === 'SCRIPT' && node.hasAttribute('data-clerk-js-script')) { | ||
| if (!coordinator.shouldAllowScript(node)) { | ||
| node.remove(); | ||
| } | ||
| } | ||
|
|
||
| // Also check children in case scripts are added in bulk | ||
| var scripts = node.querySelectorAll ? node.querySelectorAll('script[data-clerk-js-script]') : []; | ||
| for (var i = 0; i < scripts.length; i++) { | ||
| if (!coordinator.shouldAllowScript(scripts[i])) { | ||
| scripts[i].remove(); | ||
| } | ||
| } | ||
| } | ||
| }); | ||
| } | ||
| }); | ||
| }); | ||
|
|
||
| // Start observing | ||
| if (document.body) { | ||
| observer.observe(document.body, { | ||
| childList: true, | ||
| subtree: true | ||
| }); | ||
| } else { | ||
| // If body doesn't exist yet, wait for it | ||
| document.addEventListener('DOMContentLoaded', function() { | ||
| observer.observe(document.body, { | ||
| childList: true, | ||
| subtree: true | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| // Also observe head if it exists | ||
| if (document.head) { | ||
| observer.observe(document.head, { | ||
| childList: true, | ||
| subtree: true | ||
| }); | ||
| } | ||
|
|
||
| // Expose the coordinator globally | ||
| window.__clerkJSBlockingCoordinator = coordinator; | ||
| })(); | ||
| `.trim(); | ||
| } | ||
|
|
||
| /** | ||
| * Get the current state from the blocking coordinator. | ||
| */ | ||
| export function getBlockingCoordinatorState(): LoadingState { | ||
| if (typeof window === 'undefined') return 'idle'; | ||
| const coordinator = (window as any).__clerkJSBlockingCoordinator; | ||
| return coordinator ? coordinator.state : 'idle'; | ||
| } | ||
|
|
||
| /** | ||
| * Register callbacks with the blocking coordinator. | ||
| */ | ||
| export function registerWithBlockingCoordinator(callback: { | ||
| onLoad?: () => void; | ||
| onError?: (error: Error) => void; | ||
| onStateChange?: (state: LoadingState) => void; | ||
| }): () => void { | ||
| if (typeof window === 'undefined') return () => {}; | ||
|
|
||
| const coordinator = (window as any).__clerkJSBlockingCoordinator; | ||
| if (coordinator) { | ||
| coordinator.registerCallback(callback); | ||
| } | ||
|
|
||
| // Return a no-op unsubscribe function since the blocking coordinator | ||
| // doesn't need complex unsubscription | ||
| return () => {}; | ||
| } | ||
|
|
||
| /** | ||
| * Check if ClerkJS is loaded according to the blocking coordinator. | ||
| */ | ||
| export function isClerkJSLoadedBlocking(): boolean { | ||
| if (typeof window === 'undefined') return false; | ||
| const coordinator = (window as any).__clerkJSBlockingCoordinator; | ||
| return coordinator ? coordinator.state === 'loaded' : false; | ||
| } | ||
|
|
||
| export type { LoadingState, BlockingCoordinator }; |
There was a problem hiding this comment.
Security Review Required
This implementation modifies global DOM prototypes which has significant security implications. Please tag clerk/security for review.
Key concerns:
- Global prototype pollution
- Potential for breaking third-party scripts
- No mechanism to restore original behavior
- Intercepting all script insertions could be exploited
🤖 Prompt for AI Agents
In packages/shared/src/clerkJsBlockingCoordinator.ts lines 1 to 294, the code
modifies global DOM prototypes like Node.prototype.appendChild and insertBefore,
which poses security risks and can break third-party scripts. To address this,
tag the code for clerk/security review as requested, and implement a mechanism
to restore original prototype methods if needed. Additionally, consider limiting
the interception scope to only trusted contexts or scripts to reduce
exploitation risk, and document these security considerations clearly.
| registerCallback: function(callback) { | ||
| this.callbacks.push(callback); | ||
|
|
||
| // Call immediately if we have a current state | ||
| if (callback.onStateChange) { | ||
| callback.onStateChange(this.state); | ||
| } | ||
|
|
||
| if (this.state === 'loaded' && callback.onLoad) { | ||
| callback.onLoad(); | ||
| } else if (this.state === 'error' && callback.onError && this.error) { | ||
| callback.onError(this.error); | ||
| } | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Memory leak: Callbacks array grows indefinitely
The callbacks array only grows and never shrinks, causing a memory leak in long-running applications.
registerCallback: function(callback) {
- this.callbacks.push(callback);
+ var callbackId = Date.now() + Math.random();
+ callback._id = callbackId;
+ this.callbacks.push(callback);
// Call immediately if we have a current state
if (callback.onStateChange) {
callback.onStateChange(this.state);
}
if (this.state === 'loaded' && callback.onLoad) {
callback.onLoad();
} else if (this.state === 'error' && callback.onError && this.error) {
callback.onError(this.error);
}
+
+ // Return unsubscribe function
+ var self = this;
+ return function() {
+ self.callbacks = self.callbacks.filter(function(cb) {
+ return cb._id !== callbackId;
+ });
+ };
},📝 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.
| registerCallback: function(callback) { | |
| this.callbacks.push(callback); | |
| // Call immediately if we have a current state | |
| if (callback.onStateChange) { | |
| callback.onStateChange(this.state); | |
| } | |
| if (this.state === 'loaded' && callback.onLoad) { | |
| callback.onLoad(); | |
| } else if (this.state === 'error' && callback.onError && this.error) { | |
| callback.onError(this.error); | |
| } | |
| }, | |
| registerCallback: function(callback) { | |
| // Generate a unique ID for this callback and attach it | |
| var callbackId = Date.now() + Math.random(); | |
| callback._id = callbackId; | |
| this.callbacks.push(callback); | |
| // Call immediately if we have a current state | |
| if (callback.onStateChange) { | |
| callback.onStateChange(this.state); | |
| } | |
| if (this.state === 'loaded' && callback.onLoad) { | |
| callback.onLoad(); | |
| } else if (this.state === 'error' && callback.onError && this.error) { | |
| callback.onError(this.error); | |
| } | |
| // Return an unsubscribe function to remove this callback | |
| var self = this; | |
| return function() { | |
| self.callbacks = self.callbacks.filter(function(cb) { | |
| return cb._id !== callbackId; | |
| }); | |
| }; | |
| }, |
🤖 Prompt for AI Agents
In packages/shared/src/clerkJsBlockingCoordinator.ts around lines 121 to 134,
the callbacks array keeps growing without removing callbacks after they are
called, leading to a memory leak. Modify the registerCallback function to remove
or clean up callbacks from the array once they have been invoked or are no
longer needed, ensuring the array does not grow indefinitely in long-running
applications.
Description
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests