experiment: revert PR 8035 to test e2e impact#8423
Conversation
This reverts commit 9b57986.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@clerk/expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/hono
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/react
@clerk/react-router
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/ui
@clerk/upgrade
@clerk/vue
commit: |
📝 WalkthroughWalkthroughThis pull request removes the automatic proxy URL detection and derivation feature across the Clerk monorepo. The changes eliminate environment-based auto-proxy logic from multiple layers: the backend authentication context, the clerk-js core initialization, the nextjs middleware, utility functions, and the shared proxy utilities. Additionally, relative proxy URL path handling in script loading is removed, and Vercel-specific environment variables are removed from Turbo's global task dependencies. Corresponding test coverage for auto-proxy behavior is deleted across all affected packages. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Review rate limit: 4/5 reviews remaining, refill in 12 minutes. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/shared/src/loadClerkJsScript.ts (1)
283-293:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve relative proxy URL handling here.
proxyUrlstill accepts relative values, but this branch now routes them through browser-only normalization and then concatenates the result directly. That breaks server-sideclerkJSScriptUrl/clerkUIScriptUrlcallers and can emit malformed.../proxy//npm/...URLs when the proxy path has a trailing slash.Please restore the previous relative-proxy path or normalize the proxy path before building the script host.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/loadClerkJsScript.ts` around lines 283 - 293, buildScriptHost currently converts proxyUrl via proxyUrlToAbsoluteURL then strips the protocol, which breaks server-side callers and produces malformed paths when proxyUrl is a relative path (especially with trailing slash). Update the proxyUrl branch in buildScriptHost to detect relative proxy URLs and either (a) return the original relative path (preserving behavior for server-side callers) or (b) normalize the proxy path by removing a trailing slash before concatenation; use the existing helpers (isValidProxyUrl, proxyUrlToAbsoluteURL) but ensure relative URLs bypass proxyUrlToAbsoluteURL or are normalized (trim trailing slash) so buildScriptHost returns a correct host string for both server and browser usage. Reference: function buildScriptHost, helper proxyUrlToAbsoluteURL, and isValidProxyUrl.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/shared/src/loadClerkJsScript.ts`:
- Around line 283-293: buildScriptHost currently converts proxyUrl via
proxyUrlToAbsoluteURL then strips the protocol, which breaks server-side callers
and produces malformed paths when proxyUrl is a relative path (especially with
trailing slash). Update the proxyUrl branch in buildScriptHost to detect
relative proxy URLs and either (a) return the original relative path (preserving
behavior for server-side callers) or (b) normalize the proxy path by removing a
trailing slash before concatenation; use the existing helpers (isValidProxyUrl,
proxyUrlToAbsoluteURL) but ensure relative URLs bypass proxyUrlToAbsoluteURL or
are normalized (trim trailing slash) so buildScriptHost returns a correct host
string for both server and browser usage. Reference: function buildScriptHost,
helper proxyUrlToAbsoluteURL, and isValidProxyUrl.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 1560bd88-b204-41af-ad90-7a1bce2428e1
📒 Files selected for processing (14)
packages/backend/src/tokens/__tests__/authenticateContext.test.tspackages/backend/src/tokens/authenticateContext.tspackages/clerk-js/src/core/__tests__/clerk.test.tspackages/clerk-js/src/core/clerk.tspackages/nextjs/src/app-router/server/__tests__/DynamicClerkScripts.test.tsxpackages/nextjs/src/server/__tests__/clerkMiddleware.test.tspackages/nextjs/src/server/clerkMiddleware.tspackages/nextjs/src/utils/__tests__/mergeNextClerkPropsWithEnv.test.tspackages/nextjs/src/utils/mergeNextClerkPropsWithEnv.tspackages/shared/src/__tests__/loadClerkJsScript.spec.tspackages/shared/src/__tests__/proxy.spec.tspackages/shared/src/loadClerkJsScript.tspackages/shared/src/proxy.tsturbo.json
💤 Files with no reviewable changes (8)
- turbo.json
- packages/nextjs/src/app-router/server/tests/DynamicClerkScripts.test.tsx
- packages/backend/src/tokens/authenticateContext.ts
- packages/nextjs/src/server/tests/clerkMiddleware.test.ts
- packages/clerk-js/src/core/tests/clerk.test.ts
- packages/backend/src/tokens/tests/authenticateContext.test.ts
- packages/nextjs/src/utils/tests/mergeNextClerkPropsWithEnv.test.ts
- packages/shared/src/tests/loadClerkJsScript.spec.ts
|
Closing — the same 5 |
1 participant
Do not merge. Sandbox to verify whether reverting #8035 (auto-proxy for eligible hosts) flips the consistently-failing express / hono / generic / nextjs / billing / machine integration suites back to green.
If the integration matrix on this branch goes mostly green compared to recent runs on `main`, that confirms #8035 is the regression source. Will be closed regardless of outcome.
Related: #8422 (the actual fix PR currently open).