fix(menu): portal via ThemeContext refs only (#1100)#2020
Conversation
Remove querySelector fallbacks and resolve menu portal root from ThemeContext portalRootRef and containerRef. Update portal test helper to wrap renders in Theme. Fixes #1100
|
📝 WalkthroughWalkthrough
ChangesPortal Root Selection and Test Utility
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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. ✨ 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/test-utils/portal.ts`:
- Line 21: The querySelector on the portalRoot variable is force-cast to
HTMLElement without verifying the element exists, which defers failures to
downstream code making them harder to diagnose. After the querySelector call in
the getPortalRoot function, add a null/undefined check and throw a descriptive
error immediately if the element with the data-rad-ui-portal-root attribute is
not found. This ensures failures are caught early at the source with a clear
error message.
- Around line 7-14: The mockMatchMedia function checks if matchMedia exists as a
function and returns early, but doesn't validate that the matchMedia
implementation provides the required methods (addEventListener and
removeEventListener) that are called by Theme. Enhance the validation logic to
not only check if matchMedia is a function, but also verify that it returns an
object with both addEventListener and removeEventListener methods before
deciding to skip the mock. Only skip the mock setup if the complete API shape is
present; otherwise, proceed with the jest.fn mock implementation to ensure all
required methods are available.
🪄 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: bc174966-1418-4883-bd4b-5f57c10305b5
📒 Files selected for processing (2)
src/core/primitives/Menu/fragments/MenuPrimitivePortal.tsxsrc/test-utils/portal.ts
| if ('matchMedia' in window && typeof window.matchMedia === 'function') return; | ||
| Object.defineProperty(window, 'matchMedia', { | ||
| writable: true, | ||
| value: jest.fn().mockImplementation(() => ({ | ||
| matches: false, | ||
| addEventListener: jest.fn(), | ||
| removeEventListener: jest.fn() | ||
| })) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether other test setup files provide an incompatible partial matchMedia mock.
fd -i 'setup*.{ts,tsx,js,jsx}' . | xargs -r rg -n -C2 'matchMedia|addEventListener|removeEventListener|addListener|removeListener'Repository: rad-ui/ui
Length of output: 464
🏁 Script executed:
# Fix 1: Inspect the actual portal.ts file to understand current implementation
cat -n src/test-utils/portal.tsRepository: rad-ui/ui
Length of output: 3008
🏁 Script executed:
# Fix 2: Search for Theme's usage of matchMedia to understand the dependency
rg -n 'matchMedia|addEventListener.*media|removeEventListener.*media' --type ts --type tsx -A 2 -B 2 | head -100Repository: rad-ui/ui
Length of output: 175
🏁 Script executed:
# Fix 3: Find setup files using correct glob patterns
fd -g '**/*setup*' -e ts -e tsx -e js -e jsx . | head -20Repository: rad-ui/ui
Length of output: 167
🏁 Script executed:
# Fix 4: Search for other matchMedia mocks in test files
rg -n 'matchMedia' -A 5 -B 2 --type ts --type tsx --type js --type jsx | head -150Repository: rad-ui/ui
Length of output: 175
🏁 Script executed:
# Search for Theme component to understand matchMedia usage
find . -name "Theme.ts*" -o -name "Theme.js*" | grep -i theme | head -5Repository: rad-ui/ui
Length of output: 183
🏁 Script executed:
# Search for addEventListener/removeEventListener usage with matchMedia
rg -n 'addEventListener|removeEventListener' -B 5 -A 2 | grep -A 7 -B 5 'matchMedia'Repository: rad-ui/ui
Length of output: 147
🏁 Script executed:
# Check setupTests.ts for other matchMedia mocks
cat -n src/setupTests.tsRepository: rad-ui/ui
Length of output: 2876
🏁 Script executed:
# Broader search for all matchMedia references in codebase
rg 'matchMedia' -n -B 2 -A 2Repository: rad-ui/ui
Length of output: 147
🏁 Script executed:
# Read the Theme component to understand matchMedia usage
cat -n ./src/components/ui/Theme/Theme.tsxRepository: rad-ui/ui
Length of output: 4082
🏁 Script executed:
# Try a simpler rg search for matchMedia
rg 'matchMedia' -nRepository: rad-ui/ui
Length of output: 147
🏁 Script executed:
# Check for addEventListener calls in the codebase
rg 'addEventListener' -n -A 1 -B 1 | head -50Repository: rad-ui/ui
Length of output: 147
mockMatchMedia should validate API shape, not only function presence.
Line 7 returns early when matchMedia exists, but Theme (lines 62, 67) calls addEventListener and removeEventListener on the media query object. If the environment provides a partial matchMedia implementation lacking these methods, tests will fail with a runtime error.
Suggested fix
const mockMatchMedia = () => {
- if ('matchMedia' in window && typeof window.matchMedia === 'function') return;
- Object.defineProperty(window, 'matchMedia', {
+ const hasCompatibleMatchMedia = (() => {
+ if (!('matchMedia' in window) || typeof window.matchMedia !== 'function') return false;
+ const media = window.matchMedia('(prefers-color-scheme: dark)');
+ return !!media && typeof media.addEventListener === 'function' && typeof media.removeEventListener === 'function';
+ })();
+
+ if (hasCompatibleMatchMedia) return;
+
+ Object.defineProperty(window, 'matchMedia', {
writable: true,
value: jest.fn().mockImplementation(() => ({
matches: false,
- addEventListener: jest.fn(),
- removeEventListener: jest.fn()
+ addEventListener: jest.fn(),
+ removeEventListener: jest.fn(),
+ addListener: jest.fn(),
+ removeListener: jest.fn()
}))
});
};🤖 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/test-utils/portal.ts` around lines 7 - 14, The mockMatchMedia function
checks if matchMedia exists as a function and returns early, but doesn't
validate that the matchMedia implementation provides the required methods
(addEventListener and removeEventListener) that are called by Theme. Enhance the
validation logic to not only check if matchMedia is a function, but also verify
that it returns an object with both addEventListener and removeEventListener
methods before deciding to skip the mock. Only skip the mock setup if the
complete API shape is present; otherwise, proceed with the jest.fn mock
implementation to ensure all required methods are available.
| const result = render(ui); | ||
| mockMatchMedia(); | ||
| const result = render(React.createElement(Theme, null, ui)); | ||
| const portalRoot = document.querySelector('[data-rad-ui-portal-root]') as HTMLElement; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Fail fast when portal root is not found.
Line 21 force-casts query result to HTMLElement; if missing, downstream failures become harder to diagnose.
Suggested fix
- const portalRoot = document.querySelector('[data-rad-ui-portal-root]') as HTMLElement;
+ const portalRoot = document.querySelector('[data-rad-ui-portal-root]');
+ if (!portalRoot) {
+ throw new Error('renderWithPortal: [data-rad-ui-portal-root] not found. Ensure Theme rendered correctly.');
+ }📝 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.
| const portalRoot = document.querySelector('[data-rad-ui-portal-root]') as HTMLElement; | |
| const portalRoot = document.querySelector('[data-rad-ui-portal-root]'); | |
| if (!portalRoot) { | |
| throw new Error('renderWithPortal: [data-rad-ui-portal-root] not found. Ensure Theme rendered correctly.'); | |
| } |
🤖 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/test-utils/portal.ts` at line 21, The querySelector on the portalRoot
variable is force-cast to HTMLElement without verifying the element exists,
which defers failures to downstream code making them harder to diagnose. After
the querySelector call in the getPortalRoot function, add a null/undefined check
and throw a descriptive error immediately if the element with the
data-rad-ui-portal-root attribute is not found. This ensures failures are caught
early at the source with a clear error message.
Code reviewLGTM. Matches project patterns for portal Theme refs, Floating UI prop merge, controlled-switch/lazy-mount/RTL tests, or focused bug fixes. No changes requested. |
1 participant
Summary
Fixes #1100
Test plan
Summary by CodeRabbit