-
Notifications
You must be signed in to change notification settings - Fork 435
feat(clerk-js): Portal option #7241
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -3,6 +3,8 @@ import { FloatingFocusManager, FloatingNode, FloatingPortal } from '@floating-ui | |||||||||||||||||
| import type { PropsWithChildren } from 'react'; | ||||||||||||||||||
| import React from 'react'; | ||||||||||||||||||
|
|
||||||||||||||||||
| type PortalConfig = boolean | (() => HTMLElement | null); | ||||||||||||||||||
|
||||||||||||||||||
| type PortalConfig = boolean | (() => HTMLElement | null); | |
| import type { FloatingContext, ReferenceType } from '@floating-ui/react'; | |
| import { FloatingFocusManager, FloatingNode, FloatingPortal } from '@floating-ui/react'; | |
| import type { PortalConfig } from '@clerk/shared/types'; | |
| import type { PropsWithChildren } from 'react'; | |
| import React from 'react'; | |
| type PopoverProps = PropsWithChildren<{ |
🤖 Prompt for AI Agents
In packages/clerk-js/src/ui/elements/Popover.tsx around line 6, the local type
alias "type PortalConfig = boolean | (() => HTMLElement | null);" duplicates a
shared type; remove this local definition and instead import PortalConfig from
"@clerk/shared/types" at the top of the file, update any usages to reference the
imported name, and ensure the import is correctly added to the existing import
block (or create one) and that TypeScript builds without additional type
conflicts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
Verify portal logic consistency with documented defaults.
The portal resolution logic (lines 41-52) results in shouldPortal = false when portal={true} is specified without an explicit root element, because resolveRoot() returns null when only a boolean is passed.
However, the JSDoc for UserProfileProps.portal and UserButtonProps.portal documents @default true, suggesting portalling should be enabled by default. With the current logic, portal={true} without a root disables portalling, which seems inconsistent with the documented behavior.
Please verify:
- Should
portal={true}use a default container (e.g.,document.body)? - Or should the JSDoc be updated to clarify that
portal={true}requires an explicitroot?
I need to verify the review comment by examining the actual codebase. Let me gather the necessary information.
🏁 Script executed:
#!/bin/bash
# Find and read the Popover.tsx file
fd -t f "Popover.tsx" packages/clerk-js/src/ui/elementsLength of output: 45
🏁 Script executed:
#!/bin/bash
# Search for UserProfileProps and UserButtonProps definitions
rg -n "interface UserProfileProps|interface UserButtonProps|type UserProfileProps|type UserButtonProps" -A 10 packages/clerk-js/srcLength of output: 0
🏁 Script executed:
#!/bin/bash
# Read the Popover.tsx file to see full context
cat packages/clerk-js/src/ui/elements/Popover.tsxLength of output: 2090
I found the Popover.tsx file. Now I need to search for UserProfileProps and UserButtonProps to verify the JSDoc claims in the review.
🏁 Script executed:
#!/bin/bash
# Search more broadly for UserProfileProps and UserButtonProps
rg -n "UserProfileProps|UserButtonProps" packages/clerk-js/src --type=ts --type=tsx | head -20Length of output: 27
🏁 Script executed:
#!/bin/bash
# Search for portal prop documentation in prop interfaces
rg -n "portal\?" packages/clerk-js/src -B 5 -A 2 --type=ts --type=tsx | grep -E "(portal|@default)" | head -30Length of output: 27
🏁 Script executed:
#!/bin/bash
# Find where these props might be defined
fd -t f -e "ts" -e "tsx" packages/clerk-js/src | xargs grep -l "UserProfileProps\|UserButtonProps" | head -5Length of output: 389
Let me fix the command syntax and search again:
🏁 Script executed:
#!/bin/bash
# Search for UserProfileProps and UserButtonProps without type specification
rg -n "UserProfileProps|UserButtonProps" packages/clerk-js/src | head -20Length of output: 1856
🏁 Script executed:
#!/bin/bash
# Search for files containing these types
fd . packages/clerk-js/src -type f | xargs grep -l "UserProfileProps\|UserButtonProps" 2>/dev/null | head -10Length of output: 170
🏁 Script executed:
#!/bin/bash
# Search for portal prop in all files
rg -n "portal\?" packages/clerk-js/src -B 3 -A 1 | head -50Length of output: 2112
I can see UserProfileProps and UserButtonProps are imported from @clerk/shared/types. Let me find those definitions:
🏁 Script executed:
#!/bin/bash
# Find @clerk/shared package
find . -type d -name "shared" | grep -E "clerk|packages" | head -10Length of output: 167
🏁 Script executed:
#!/bin/bash
# Search for the actual type definitions in the repo
rg -n "export.*type.*UserProfileProps|export.*interface.*UserProfileProps" . | head -10Length of output: 292
🏁 Script executed:
#!/bin/bash
# Look for @clerk/shared in package.json files
rg -n "@clerk/shared" packages/clerk-js | head -5Length of output: 512
Now let me read the UserProfileProps definition and check the portal property documentation:
🏁 Script executed:
#!/bin/bash
# Read the UserProfileProps definition with context
sed -n '1540,1600p' packages/shared/src/types/clerk.tsLength of output: 1960
🏁 Script executed:
#!/bin/bash
# Search for portal property documentation in the shared types
rg -n "portal" packages/shared/src/types/clerk.ts -B 2 -A 1Length of output: 3073
🏁 Script executed:
#!/bin/bash
# Find UserButtonProps definition
rg -n "export.*type.*UserButtonProps|export.*interface.*UserButtonProps" packages/shared/src/types/clerk.ts -A 30 | head -60Length of output: 1249
Portal default behavior in Popover contradicts JSDoc documentation.
The code behavior is inconsistent with the documented defaults. When portal={true} (the default) is used without an explicit root, the shouldPortal flag becomes false, disabling portalling entirely. However, the JSDoc for both UserProfileProps.portal and UserButtonProps.portal in packages/shared/src/types/clerk.ts documents @default true, implying portalling should be enabled by default.
Additionally, the Popover's root prop JSDoc claims @default document.body, but the resolveRoot() function returns null when root is undefined, not using document.body.
Fix required: Either:
- Update the implementation to use
document.bodyas the default root when not explicitly provided (matching the JSDoc), or - Update the JSDoc to clarify that
portal={true}requires an explicitrootelement to function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PortalContext fallback is effectively disabled for OrganizationSwitcher.
portalwith a default (portal: portalProp = true), soportalPropis neverundefined.portalasportalProp !== undefined ? portalProp : (portalFromContext ?? true), which means theportalFromContextand?? truefallbacks are dead code.PortalProvider/usePortalContextcannot influence this Popover at all; only the OrganizationSwitcher context prop can, and even that defaults totrue. The comment on line 17 also no longer matches the behavior.This breaks the intent of wiring global portal configuration through
PortalContextinto OrganizationSwitcher.You can fix both the behavior and the comment with something like:
This lets:
portaloverride the global context when provided.PortalProviderportal config apply when the component doesn’t specifyportal.truewhen neither is set.Also applies to: 28-29, 46-46
🤖 Prompt for AI Agents