fix(menu): portal content inside Theme scope (#1100)#2013
Conversation
Use ThemeContext portalRootRef and containerRef with theme container fallbacks instead of querySelector-only resolution. Fixes #1100
|
📝 WalkthroughWalkthrough
ChangesMenuPrimitivePortal – container prop and ThemeContext root resolution
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
CoverageThis report compares the PR with the base branch. "Δ" shows how the PR affects each metric.
Coverage improved or stayed the same. Great job! Run |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/primitives/Menu/fragments/MenuPrimitivePortal.tsx`:
- Around line 24-27: The fallback order in the rootElementRef assignment within
MenuPrimitivePortal.tsx is incorrect for multi-theme documents. Reorder the
checks so that theme-scoped fallbacks (themeContext?.containerRef.current) are
evaluated before global portal queries
(document.querySelector('[data-rad-ui-portal-root]')). Move the
themeContext?.containerRef.current check to execute before the global
data-rad-ui-portal-root selector lookup to ensure theme-scoped containers take
precedence over global portal roots.
- Around line 17-31: The rootElementRef in MenuPrimitivePortal stores the
computed portal root in a ref, which doesn't trigger re-renders when updated.
When container or themeContext dependencies change, the component re-renders and
reads the stale rootElementRef.current value before useLayoutEffect updates it,
causing the portal to render with an outdated root. Instead of using useRef and
useLayoutEffect to compute and store the root element, replace this with useMemo
that depends on container and themeContext, so the computed portal root is
recalculated and available during render whenever dependencies change. Remove
the rootElementRef entirely and compute the portal root value directly inside a
useMemo hook.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1c588d9e-17e6-45a3-8e05-c719c73dba21
📒 Files selected for processing (1)
src/core/primitives/Menu/fragments/MenuPrimitivePortal.tsx
| const rootElementRef = useRef<HTMLElement | null>(null); | ||
| const [isMounted, setIsMounted] = useState(false); | ||
|
|
||
| useEffect(() => { | ||
| if (rootElement) { | ||
| setRootElementFound(true); | ||
| useLayoutEffect(() => { | ||
| if (container) { | ||
| rootElementRef.current = container as HTMLElement; | ||
| } else { | ||
| rootElementRef.current = themeContext?.portalRootRef.current | ||
| || document.querySelector('[data-rad-ui-portal-root]') as HTMLElement | null | ||
| || themeContext?.containerRef.current | ||
| || document.querySelector('#rad-ui-theme-container') as HTMLElement | null | ||
| || document.body; | ||
| } | ||
| }, [rootElement]); | ||
| setIsMounted(true); | ||
| }, [container, themeContext]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/core/primitives/Menu/fragments/MenuPrimitivePortal.tsx | head -60Repository: rad-ui/ui
Length of output: 2315
🏁 Script executed:
fd -t f Floater || rg -l "Floater" src/ | head -5Repository: rad-ui/ui
Length of output: 35
🏁 Script executed:
find src -name "*loater*" -o -name "*Floater*" 2>/dev/null | head -10Repository: rad-ui/ui
Length of output: 81
🏁 Script executed:
ls -la src/core/primitives/Floater/Repository: rad-ui/ui
Length of output: 226
🏁 Script executed:
cat -n src/core/primitives/Floater/index.tsxRepository: rad-ui/ui
Length of output: 1470
🏁 Script executed:
rg -B5 -A5 "MenuPrimitivePortal" src/ --type tsx | head -40Repository: rad-ui/ui
Length of output: 81
🏁 Script executed:
rg -B5 -A5 "MenuPrimitivePortal" src/ | head -60Repository: rad-ui/ui
Length of output: 4702
🏁 Script executed:
cat -n src/core/primitives/Menu/contexts/MenuPrimitiveRootContext.tsxRepository: rad-ui/ui
Length of output: 1515
🏁 Script executed:
cat -n src/core/primitives/Menu/fragments/MenuPrimitiveRoot.tsx | head -80Repository: rad-ui/ui
Length of output: 3522
🏁 Script executed:
rg -B3 -A3 "MenuPrimitive.Portal\|MenuPrimitivePortal" src/ | head -80Repository: rad-ui/ui
Length of output: 35
🏁 Script executed:
find src -type f -name "*.tsx" -o -name "*.ts" | xargs grep -l "MenuPrimitive.Portal\|MenuPrimitivePortal" 2>/dev/nullRepository: rad-ui/ui
Length of output: 448
🏁 Script executed:
cat -n src/core/primitives/Menu/tests/MenuPrimitive.test.tsx | head -100Repository: rad-ui/ui
Length of output: 4096
🏁 Script executed:
rg -A10 -B2 "container\|Portal" src/core/primitives/Menu/tests/MenuPrimitive.test.tsx | head -60Repository: rad-ui/ui
Length of output: 35
🏁 Script executed:
cat -n src/components/ui/DropdownMenu/fragments/DropdownMenuPortal.tsxRepository: rad-ui/ui
Length of output: 1232
🏁 Script executed:
cat -n src/core/primitives/Menu/fragments/MenuPrimitiveContent.tsx | head -80Repository: rad-ui/ui
Length of output: 2297
🏁 Script executed:
python3 << 'PY'
# Let me analyze the timing issue more carefully
print("=== Ref Write vs State Update Timing ===")
print()
print("Current code (using ref):")
print("1. container/themeContext changes")
print("2. Component re-renders")
print("3. During render: rootElementRef.current = OLD value (effect hasn't run yet)")
print("4. Floater.Portal gets OLD root")
print("5. useLayoutEffect runs and updates rootElementRef.current")
print("6. Portal already rendered, won't update (ref changes don't trigger re-renders)")
print()
print("Proposed code (using state):")
print("1. container/themeContext changes")
print("2. Component re-renders")
print("3. During render: rootElement = OLD value (effect hasn't run yet)")
print("4. useLayoutEffect runs and calls setRootElement(nextRoot)")
print("5. setRootElement triggers another re-render")
print("6. Second render uses NEW rootElement value")
print()
print("Conclusion: The ref approach has a 1-render delay in applying root changes.")
print("The state approach ensures correct value is used by triggering re-render.")
PYRepository: rad-ui/ui
Length of output: 880
🏁 Script executed:
# Check if there are any usages showing the actual impact
rg -B5 -A5 "container\s*=" src/core/primitives/Menu/ | head -40Repository: rad-ui/ui
Length of output: 35
Root element can become stale because ref writes don't trigger re-renders.
Lines 17-31 write the computed portal root into a ref, but line 38 reads it during render. When container or themeContext changes after initial mount, the component re-renders but reads the old ref value before useLayoutEffect updates it. The portal then renders with the stale root and never updates since ref mutations don't cause re-renders.
Suggested fix
- const rootElementRef = useRef<HTMLElement | null>(null);
+ const [rootElement, setRootElement] = useState<HTMLElement | null>(null);
const [isMounted, setIsMounted] = useState(false);
useLayoutEffect(() => {
+ const nextRoot = container
+ ? (container as HTMLElement)
+ : (themeContext?.portalRootRef.current
+ || themeContext?.containerRef.current
+ || (document.querySelector('[data-rad-ui-portal-root]') as HTMLElement | null)
+ || (document.querySelector('`#rad-ui-theme-container`') as HTMLElement | null)
+ || document.body);
- if (container) {
- rootElementRef.current = container as HTMLElement;
- } else {
- rootElementRef.current = themeContext?.portalRootRef.current
- || document.querySelector('[data-rad-ui-portal-root]') as HTMLElement | null
- || themeContext?.containerRef.current
- || document.querySelector('`#rad-ui-theme-container`') as HTMLElement | null
- || document.body;
- }
+ setRootElement(nextRoot);
setIsMounted(true);
- }, [container, themeContext]);
+ }, [container, themeContext, context?.isOpen]);
if (!context) return null;
const { isOpen } = context;
- if (!isOpen || !isMounted) return null;
+ if (!isOpen || !isMounted || !rootElement) return null;
return (
- <Floater.Portal root={rootElementRef.current} {...props}>
+ <Floater.Portal root={rootElement} {...props}>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/primitives/Menu/fragments/MenuPrimitivePortal.tsx` around lines 17 -
31, The rootElementRef in MenuPrimitivePortal stores the computed portal root in
a ref, which doesn't trigger re-renders when updated. When container or
themeContext dependencies change, the component re-renders and reads the stale
rootElementRef.current value before useLayoutEffect updates it, causing the
portal to render with an outdated root. Instead of using useRef and
useLayoutEffect to compute and store the root element, replace this with useMemo
that depends on container and themeContext, so the computed portal root is
recalculated and available during render whenever dependencies change. Remove
the rootElementRef entirely and compute the portal root value directly inside a
useMemo hook.
| rootElementRef.current = themeContext?.portalRootRef.current | ||
| || document.querySelector('[data-rad-ui-portal-root]') as HTMLElement | null | ||
| || themeContext?.containerRef.current | ||
| || document.querySelector('#rad-ui-theme-container') as HTMLElement | null |
There was a problem hiding this comment.
Prefer Theme-scoped fallback before global portal query.
On Line 25, the global [data-rad-ui-portal-root] lookup runs before Line 26 (themeContext?.containerRef.current), so in multi-theme documents this can mount into the wrong Theme subtree.
Suggested fix
- rootElementRef.current = themeContext?.portalRootRef.current
- || document.querySelector('[data-rad-ui-portal-root]') as HTMLElement | null
- || themeContext?.containerRef.current
+ rootElementRef.current = themeContext?.portalRootRef.current
+ || themeContext?.containerRef.current
+ || document.querySelector('[data-rad-ui-portal-root]') as HTMLElement | null
|| document.querySelector('`#rad-ui-theme-container`') as HTMLElement | null
|| document.body;📝 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.
| rootElementRef.current = themeContext?.portalRootRef.current | |
| || document.querySelector('[data-rad-ui-portal-root]') as HTMLElement | null | |
| || themeContext?.containerRef.current | |
| || document.querySelector('#rad-ui-theme-container') as HTMLElement | null | |
| rootElementRef.current = themeContext?.portalRootRef.current | |
| || themeContext?.containerRef.current | |
| || document.querySelector('[data-rad-ui-portal-root]') as HTMLElement | null | |
| || document.querySelector('`#rad-ui-theme-container`') as HTMLElement | null | |
| || document.body; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/primitives/Menu/fragments/MenuPrimitivePortal.tsx` around lines 24 -
27, The fallback order in the rootElementRef assignment within
MenuPrimitivePortal.tsx is incorrect for multi-theme documents. Reorder the
checks so that theme-scoped fallbacks (themeContext?.containerRef.current) are
evaluated before global portal queries
(document.querySelector('[data-rad-ui-portal-root]')). Move the
themeContext?.containerRef.current check to execute before the global
data-rad-ui-portal-root selector lookup to ensure theme-scoped containers take
precedence over global portal roots.
|
Closing as duplicate of #2020 |
1 participant
Summary
Fixes #1100
Test plan
Summary by CodeRabbit