DIAG: instrument userButton mount path (do not merge)#8436
DIAG: instrument userButton mount path (do not merge)#8436jacekradko wants to merge 1 commit intomainfrom
Conversation
🦋 Changeset detectedLatest commit: e9d68a7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types 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.
|
@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 adds diagnostic instrumentation across the Clerk codebase to aid in debugging UserButton mounting behavior. A changeset entry file is created, and console.log statements are added to four locations: the Clerk.emit method to report listener counts and current identifiers, the useUserBase hook snapshot function to log loading status and fallback conditions with a conditional early return based on a Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Review rate limit: 2/5 reviews remaining, refill in 33 minutes and 3 seconds. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/clerk-js/src/core/clerk.ts`:
- Around line 3238-3253: The DIAG console logs in the Clerk emit path leak
internal identifiers and run on a hot path; wrap the diagnostic logging in a
non-production/explicit diagnostic flag check (e.g., only log when a process/env
flag like CLERK_DIAG_ENABLED or an internal config boolean is true) and stop
logging raw IDs—either remove userId/sessionId/clientId or redact them (e.g.,
boolean presence or truncated/sanitized values) before logging; apply the same
gated/sanitized change to the other diagnostic console.log sites referenced in
useUserBase and withCoreUserGuard and ensure the guard references the same
flag/config accessor used here (the block around the console.log in the method
that iterates this.#listeners).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 88fefe42-c3f3-46cf-8334-5b13748c4d1f
📒 Files selected for processing (4)
.changeset/diag-userbutton-mount.mdpackages/clerk-js/src/core/clerk.tspackages/shared/src/react/hooks/base/useUserBase.tspackages/ui/src/contexts/CoreUserContext.tsx
| // DIAG: instrument userButton mount regression | ||
| // eslint-disable-next-line no-console | ||
| console.log('[CLERK_DIAG] clerk.#emit', { | ||
| listeners: this.#listeners.length, | ||
| userId: this.user?.id ?? null, | ||
| sessionId: this.session?.id ?? null, | ||
| clientId: this.client?.id ?? null, | ||
| sessionsOnClient: this.client?.sessions?.length ?? 0, | ||
| }); | ||
| for (const listener of this.#listeners) { | ||
| listener(resources); | ||
| } | ||
| } else { | ||
| // DIAG: emit was skipped because client is unset | ||
| // eslint-disable-next-line no-console | ||
| console.log('[CLERK_DIAG] clerk.#emit skipped (no client)'); |
There was a problem hiding this comment.
Remove or hard-gate [CLERK_DIAG] logs before merge
Line 3240 logs identifiers (userId, sessionId, clientId) on every emit. This is a merge blocker: accidental merge would leak internal identifiers into browser/CI logs and add noisy hot-path logging. Please gate these diagnostics behind an explicit non-production flag (or remove before merge), and apply the same guard to the matching [CLERK_DIAG] logs in useUserBase and withCoreUserGuard.
As per coding guidelines: "Implement proper logging with different levels" and "validate all inputs and sanitize outputs."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/clerk-js/src/core/clerk.ts` around lines 3238 - 3253, The DIAG
console logs in the Clerk emit path leak internal identifiers and run on a hot
path; wrap the diagnostic logging in a non-production/explicit diagnostic flag
check (e.g., only log when a process/env flag like CLERK_DIAG_ENABLED or an
internal config boolean is true) and stop logging raw IDs—either remove
userId/sessionId/clientId or redact them (e.g., boolean presence or
truncated/sanitized values) before logging; apply the same gated/sanitized
change to the other diagnostic console.log sites referenced in useUserBase and
withCoreUserGuard and ensure the guard references the same flag/config accessor
used here (the block around the console.log in the method that iterates
this.#listeners).
|
Closing — the userButton mount regression we set out to instrument resolved itself externally between runs at 17:52 and 18:25 UTC today, with no JS-side code change in that window. That's confirmation the cause was environmental (FAPI deploy / instance config / runner image), not in this repo's source. Diagnostic isn't needed since the symptom is gone. |
Do not merge. Diagnostic patch to capture exactly what state `useUserBase` and `withCoreUserGuard` see at the moment the userButton fails to paint.
Adds three `[CLERK_DIAG]` console.log lines:
When the express/hono userButton tests fail, the playwright trace now contains the diagnostic logs and we can definitively see whether the issue is:
Will be closed after we read the next CI run's traces.