Skip to content

feat: failure detection #6026

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
254 changes: 232 additions & 22 deletions packages/nextjs/src/utils/clerk-js-script.tsx
Original file line number Diff line number Diff line change
@@ -1,20 +1,166 @@
import { useClerk } from '@clerk/clerk-react';
import { buildClerkJsScriptAttributes, clerkJsScriptUrl } from '@clerk/clerk-react/internal';
import Head from 'next/head';
import NextScript from 'next/script';
import React from 'react';
import React, { useCallback, useEffect, useRef, useState } from 'react';

import { useClerkNextOptions } from '../client-boundary/NextOptionsContext';

// TODO: This will work once the exports are properly set up
// For now, we'll use the blocking coordinator pattern with inline script

type LoadingState = 'idle' | 'loading' | 'loaded' | 'error';

type ClerkJSScriptProps = {
router: 'app' | 'pages';
onLoad?: () => void;
onError?: (error: Error) => void;
onLoadingStateChange?: (state: LoadingState) => void;
};

// Inline blocking coordinator script (until we can import properly)
function getBlockingCoordinatorScript(): string {
return `
(function() {
'use strict';

if (window.__clerkJSBlockingCoordinator) {
return;
}

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();
}
Comment on lines +22 to +128
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Security concern: Global prototype modification

The inline script modifies global DOM prototypes (Node.prototype.appendChild), which could:

  1. Conflict with other libraries that also modify these prototypes
  2. Break third-party scripts that expect standard DOM behavior
  3. 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.tsx and consume the shared coordinator implementation from packages/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 touching Node.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.


// Hook to get the current ClerkJS loading state from the blocking coordinator
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 };
}
Comment on lines +131 to +148
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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.


/**
* Enhanced ClerkJS Script component with bulletproof load detection.
*
* This component ensures the blocking coordinator is loaded in the document head
* before any ClerkJS scripts, regardless of the router type.
*/
function ClerkJSScript(props: ClerkJSScriptProps) {
const { publishableKey, clerkJSUrl, clerkJSVersion, clerkJSVariant, nonce } = useClerkNextOptions();
const { domain, proxyUrl } = useClerk();
const scriptRef = useRef<HTMLScriptElement>(null);
const coordinatorInjected = useRef(false);

/**
* If no publishable key, avoid appending an invalid script in the DOM.
* If no publishable key, avoid appending invalid scripts in the DOM.
*/
if (!publishableKey) {
return null;
Expand All @@ -31,26 +177,90 @@ function ClerkJSScript(props: ClerkJSScriptProps) {
};
const scriptUrl = clerkJsScriptUrl(options);

/**
* Notes:
* `next/script` in 13.x.x when used with App Router will fail to pass any of our `data-*` attributes, resulting in errors
* Nextjs App Router will automatically move inline scripts inside `<head/>`
* Using the `nextjs/script` for App Router with the `beforeInteractive` strategy will throw an error because our custom script will be mounted outside the `html` tag.
*/
const Script = props.router === 'app' ? 'script' : NextScript;

return (
<Script
src={scriptUrl}
data-clerk-js-script
async
// `nextjs/script` will add defer by default and does not get removed when we async is true
defer={props.router === 'pages' ? false : undefined}
crossOrigin='anonymous'
strategy={props.router === 'pages' ? 'beforeInteractive' : undefined}
{...buildClerkJsScriptAttributes(options)}
/>
// Inject coordinator script into head manually to ensure it's there first
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;
}, []);
Comment on lines +181 to +203
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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.


// Handle state changes from the blocking coordinator
const handleLoad = useCallback(() => {
props.onLoad?.();
}, [props]);

const handleError = useCallback(
(error: Error) => {
props.onError?.(error);
},
[props],
);

// Subscribe to blocking coordinator state changes
useEffect(() => {
if (typeof window === 'undefined') return;

const coordinator = (window as any).__clerkJSBlockingCoordinator;
if (coordinator) {
coordinator.registerCallback({
onLoad: handleLoad,
onError: handleError,
onStateChange: (state: LoadingState) => {
props.onLoadingStateChange?.(state);
},
});
}
}, [handleLoad, handleError, props]);

const scriptAttributes = buildClerkJsScriptAttributes(options);

if (props.router === 'app') {
// For App Router, use Next.js Head component to ensure script goes to head
return (
<Head>
<script
ref={scriptRef}
src={scriptUrl}
data-clerk-js-script='true'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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`.

async
crossOrigin='anonymous'
{...scriptAttributes}
/>
</Head>
);
} else {
// For Pages Router, use Next.js Script components with beforeInteractive
return (
<NextScript
src={scriptUrl}
data-clerk-js-script='true'
async
defer={false}
crossOrigin='anonymous'
strategy='beforeInteractive'
{...scriptAttributes}
/>
);
}
}

export { ClerkJSScript };
export { ClerkJSScript, useClerkJSLoadingState };
export type { ClerkJSScriptProps, LoadingState };
Loading
Loading