fix(ui): Initial pass for adding RHC guards to ui package#7983
Merged
fix(ui): Initial pass for adding RHC guards to ui package#7983
Conversation
🦋 Changeset detectedLatest commit: c4100d3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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.
|
royanger
changed the title
tmilewski
approved these changes
Mar 5, 2026
Co-authored-by: Tom Milewski <me@tm.codes>
Contributor
📝 WalkthroughWalkthroughThis pull request introduces a new RHC-disabled variant of the 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/search-for-rhc.mjs (1)
27-37:⚠️ Potential issue | 🔴 CriticalRHC scan can fail open, so this new check may silently pass.
Because of current scanner behavior, Line 30 can report success even when the scan errored: Line 17 runs
grepon directories without recursion, and Line 20 treats any non-zero exit code (including grep errors) as "not found". This makes the no-RHC guard unsafe to rely on.🐛 Proposed fix
async function asyncSearchRHC(name, search, regex = false) { const flag = regex ? 'E' : 'F'; const cmd = () => targetType === 'directory' - ? $`grep -${flag}q --include=\\*.js --include=\\*.mjs ${search} ${target}` + ? $`grep -r -${flag}q --include=\\*.js --include=\\*.mjs ${search} ${target}` : $`grep -${flag}q ${search} ${target}`; - if ((await cmd().exitCode) === 0) { + const exitCode = await cmd().exitCode; + if (exitCode === 0) { throw new Error(`Found ${name} related RHC in build output. (Search: \`${search}\`)`); } + if (exitCode > 1) { + throw new Error(`Failed scanning for ${name}. grep exited with code ${exitCode}.`); + } return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/search-for-rhc.mjs` around lines 27 - 37, The asyncSearchRHC-based scan can "fail open" because grep is run non-recursively and any grep error is treated like "not found"; update asyncSearchRHC to invoke grep with recursion (e.g., -R) and change its child-process handling so that grep exit code 2 (or any stderr output / execution error) causes the promise to reject (only exit code 1 should be interpreted as “no match”), then ensure the Promise.allSettled consumer interprets rejected promises as scan failures rather than successes. Specifically modify the asyncSearchRHC implementation and its child_process callbacks to propagate execution errors (reject on code 2 or error), use recursive grep, and adjust how results from Promise.allSettled are evaluated so errors don’t silently pass the no-RHC guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/ui/src/utils/one-tap.ts`:
- Around line 42-44: The branch that returns "undefined as unknown as Google"
when __BUILD_DISABLE_RHC__ is true must be fixed: change the function's return
type to allow undefined (e.g., Google | undefined) or return a safe stub object
instead of force-casting undefined; update the code around the
__BUILD_DISABLE_RHC__ check so it either returns a proper fallback
implementation or an unambiguous undefined/null (no cast) and adjust all callers
of the function (the function that contains the __BUILD_DISABLE_RHC__ branch and
any usages expecting a Google) to handle the optional value accordingly.
---
Outside diff comments:
In `@scripts/search-for-rhc.mjs`:
- Around line 27-37: The asyncSearchRHC-based scan can "fail open" because grep
is run non-recursively and any grep error is treated like "not found"; update
asyncSearchRHC to invoke grep with recursion (e.g., -R) and change its
child-process handling so that grep exit code 2 (or any stderr output /
execution error) causes the promise to reject (only exit code 1 should be
interpreted as “no match”), then ensure the Promise.allSettled consumer
interprets rejected promises as scan failures rather than successes.
Specifically modify the asyncSearchRHC implementation and its child_process
callbacks to propagate execution errors (reject on code 2 or error), use
recursive grep, and adjust how results from Promise.allSettled are evaluated so
errors don’t silently pass the no-RHC guard.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: ecd6d950-d379-4618-a36f-e2aff31b43ed
📒 Files selected for processing (8)
.changeset/five-carrots-laugh.mdpackages/chrome-extension/src/react/ClerkProvider.tsxpackages/chrome-extension/src/utils/__tests__/clerk-client.test.tspackages/chrome-extension/src/utils/clerk-client.tspackages/ui/package.jsonpackages/ui/src/utils/one-tap.tspackages/ui/tsdown.config.mtsscripts/search-for-rhc.mjs
Comment on lines
+42
to
+44
| if (__BUILD_DISABLE_RHC__) { | ||
| return undefined as unknown as Google; | ||
| } |
Contributor
There was a problem hiding this comment.
Do not return undefined cast as Google in no-RHC builds.
Line 43 creates a type/runtime mismatch (undefined masquerading as Google). This can cause runtime crashes when consumers dereference the result.
🐛 Proposed fix
-async function loadGIS() {
+async function loadGIS(): Promise<Google> {
if (__BUILD_DISABLE_RHC__) {
- return undefined as unknown as Google;
+ clerkFailedToLoadThirdPartyScript('Google Identity Services');
}
if (!window.google) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ui/src/utils/one-tap.ts` around lines 42 - 44, The branch that
returns "undefined as unknown as Google" when __BUILD_DISABLE_RHC__ is true must
be fixed: change the function's return type to allow undefined (e.g., Google |
undefined) or return a safe stub object instead of force-casting undefined;
update the code around the __BUILD_DISABLE_RHC__ check so it either returns a
proper fallback implementation or an unambiguous undefined/null (no cast) and
adjust all callers of the function (the function that contains the
__BUILD_DISABLE_RHC__ branch and any usages expecting a Google) to handle the
optional value accordingly.
@clerk/agent-toolkit
@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: |
royanger
deleted the
roy/add-remote-code-exclusion-mechanism-to-ui-package
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
clerk-jsandsharedalready exclude remotely hosted code. Core 3 moved ui to its own package, but it is not correctly excluding remotely hosted code even those some guards were in place in the code. This PR adds the mechanism to exclude remotely hosted code and imports the new@clerk/us/no-rhcinto<ClerkProvider />for@clerk/chrome-extensionChecklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit
Release Notes
New Features
Chores