test(AlertDialog): lazy mount dialog content (#1847)#2068
Conversation
📝 WalkthroughWalkthroughA new test file is added for ChangesAlertDialog Lazy Mount Tests
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 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 |
|
Align lazy mount coverage with portal/theme test setup used elsewhere.
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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/components/ui/AlertDialog/tests/AlertDialog.lazyMount.test.tsx (1)
6-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNormalize
matchMediaeven when it already exists.The early return makes this suite depend on whatever
jsdomprovides. SinceThemecallsaddEventListener/removeEventListener, patch missing methods (or always override in this suite) to keep tests deterministic across environments.Suggested change
const mockMatchMedia = () => { - if ('matchMedia' in window && typeof window.matchMedia === 'function') return; - Object.defineProperty(window, 'matchMedia', { + const existing = window.matchMedia?.bind(window); + Object.defineProperty(window, 'matchMedia', { writable: true, - value: jest.fn().mockImplementation(() => ({ - matches: false, - addEventListener: jest.fn(), - removeEventListener: jest.fn() - })) + value: jest.fn().mockImplementation((query: string) => { + const base = existing?.(query) ?? ({ matches: false } as MediaQueryList); + return { + ...base, + addEventListener: base.addEventListener ?? jest.fn(), + removeEventListener: base.removeEventListener ?? 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/components/ui/AlertDialog/tests/AlertDialog.lazyMount.test.tsx` around lines 6 - 16, The mockMatchMedia helper in AlertDialog.lazyMount.test.tsx currently returns early when window.matchMedia already exists, which leaves the suite dependent on the jsdom implementation. Update mockMatchMedia to always normalize matchMedia for this test suite, or extend the existing window.matchMedia so it always provides addEventListener and removeEventListener for Theme, keeping the AlertDialog tests deterministic across environments.
🤖 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.
Nitpick comments:
In `@src/components/ui/AlertDialog/tests/AlertDialog.lazyMount.test.tsx`:
- Around line 6-16: The mockMatchMedia helper in AlertDialog.lazyMount.test.tsx
currently returns early when window.matchMedia already exists, which leaves the
suite dependent on the jsdom implementation. Update mockMatchMedia to always
normalize matchMedia for this test suite, or extend the existing
window.matchMedia so it always provides addEventListener and removeEventListener
for Theme, keeping the AlertDialog tests deterministic across environments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f0759ef8-eebc-4c17-b2d5-f47ed8f42090
📒 Files selected for processing (1)
src/components/ui/AlertDialog/tests/AlertDialog.lazyMount.test.tsx
Code reviewReviewed and pushed follow-up fixes addressing handler composition, test patterns ( CI was green before fixes; please re-run checks on latest commit. |
Adds tests verifying alert dialog content is not mounted until opened.\n\nRelated to #1847
Summary by CodeRabbit