Conversation
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive unit tests for the useIsMobile React hook using bun:test framework and @testing-library/react. The tests introduce happy-dom to provide a DOM environment for testing browser APIs like window.matchMedia and window.innerWidth, which are essential for the mobile detection logic implemented in the hook.
Changes:
- Added test file
src/hooks/use-mobile.test.tsxwith 161 lines covering mobile detection scenarios - Added
happy-domand@happy-dom/global-registratoras dev dependencies
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/hooks/use-mobile.test.tsx | New comprehensive test suite for useIsMobile hook covering desktop/mobile detection, resize events, and cleanup |
| package.json | Added happy-dom dependencies for DOM environment in tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| value: mock((query: string) => ({ | ||
| matches: false, | ||
| media: query, | ||
| onchange: null, | ||
| addListener: mock(), // Deprecated | ||
| removeListener: mock(), // Deprecated | ||
| addEventListener: mock((type: string, listener: (e: MediaQueryListEvent) => void) => { | ||
| if (!listeners[type]) listeners[type] = []; | ||
| listeners[type].push(listener); | ||
| }), | ||
| removeEventListener: mock((type: string, listener: (e: MediaQueryListEvent) => void) => { | ||
| if (listeners[type]) { | ||
| listeners[type] = listeners[type].filter((l) => l !== listener); | ||
| } | ||
| }), | ||
| dispatchEvent: mock(), | ||
| })), |
There was a problem hiding this comment.
The mock implementation of matchMedia always returns matches: false regardless of the query or window size. This doesn't accurately reflect how matchMedia works. The matches property should be dynamically computed based on the query and current window.innerWidth. For example, when mocking a query like "(max-width: 767px)" and window.innerWidth is 500, the matches property should be true.
| value: mock((query: string) => ({ | |
| matches: false, | |
| media: query, | |
| onchange: null, | |
| addListener: mock(), // Deprecated | |
| removeListener: mock(), // Deprecated | |
| addEventListener: mock((type: string, listener: (e: MediaQueryListEvent) => void) => { | |
| if (!listeners[type]) listeners[type] = []; | |
| listeners[type].push(listener); | |
| }), | |
| removeEventListener: mock((type: string, listener: (e: MediaQueryListEvent) => void) => { | |
| if (listeners[type]) { | |
| listeners[type] = listeners[type].filter((l) => l !== listener); | |
| } | |
| }), | |
| dispatchEvent: mock(), | |
| })), | |
| value: mock((query: string) => { | |
| const width = window.innerWidth; | |
| // Basic support for (max-width: Npx), (min-width: Npx) and combined queries | |
| const maxMatch = query.match(/\(max-width:\s*(\d+)px\)/); | |
| const minMatch = query.match(/\(min-width:\s*(\d+)px\)/); | |
| const maxWidth = maxMatch ? parseInt(maxMatch[1], 10) : undefined; | |
| const minWidth = minMatch ? parseInt(minMatch[1], 10) : undefined; | |
| let matches = true; | |
| if (typeof maxWidth === "number") { | |
| matches = matches && width <= maxWidth; | |
| } | |
| if (typeof minWidth === "number") { | |
| matches = matches && width >= minWidth; | |
| } | |
| // If no constraints parsed, default to false to avoid accidental matches | |
| if (typeof maxWidth === "undefined" && typeof minWidth === "undefined") { | |
| matches = false; | |
| } | |
| return { | |
| matches, | |
| media: query, | |
| onchange: null, | |
| addListener: mock(), // Deprecated | |
| removeListener: mock(), // Deprecated | |
| addEventListener: mock((type: string, listener: (e: MediaQueryListEvent) => void) => { | |
| if (!listeners[type]) listeners[type] = []; | |
| listeners[type].push(listener); | |
| }), | |
| removeEventListener: mock((type: string, listener: (e: MediaQueryListEvent) => void) => { | |
| if (listeners[type]) { | |
| listeners[type] = listeners[type].filter((l) => l !== listener); | |
| } | |
| }), | |
| dispatchEvent: mock(), | |
| }; | |
| }), |
src/hooks/use-mobile.test.tsx
Outdated
| Object.defineProperty(window, "matchMedia", { | ||
| writable: true, | ||
| value: originalMatchMedia, | ||
| }); | ||
| } | ||
|
|
||
| if (originalInnerWidth !== undefined) { | ||
| Object.defineProperty(window, "innerWidth", { | ||
| writable: true, | ||
| value: originalInnerWidth, | ||
| }); |
There was a problem hiding this comment.
Inconsistent indentation in the afterEach block. Lines 56-59 and 63-66 use 8 spaces instead of the 4-space indentation used elsewhere in the file. This should be standardized to match the rest of the codebase.
| Object.defineProperty(window, "matchMedia", { | |
| writable: true, | |
| value: originalMatchMedia, | |
| }); | |
| } | |
| if (originalInnerWidth !== undefined) { | |
| Object.defineProperty(window, "innerWidth", { | |
| writable: true, | |
| value: originalInnerWidth, | |
| }); | |
| Object.defineProperty(window, "matchMedia", { | |
| writable: true, | |
| value: originalMatchMedia, | |
| }); | |
| } | |
| if (originalInnerWidth !== undefined) { | |
| Object.defineProperty(window, "innerWidth", { | |
| writable: true, | |
| value: originalInnerWidth, | |
| }); |
| const { result } = renderHook(() => useIsMobile()); | ||
| expect(result.current).toBe(true); | ||
| }); | ||
|
|
There was a problem hiding this comment.
The test suite is missing edge case tests for the exact breakpoint boundaries. The hook checks window.innerWidth < 768, so values of 767px (should be mobile) and 768px (should be desktop) should be explicitly tested to ensure the boundary condition is handled correctly.
| test("should return true when window width is exactly one pixel below MOBILE_BREAKPOINT", () => { | |
| // Set width to just below the breakpoint (mobile) | |
| Object.defineProperty(window, "innerWidth", { | |
| writable: true, | |
| value: MOBILE_BREAKPOINT - 1, | |
| }); | |
| const { result } = renderHook(() => useIsMobile()); | |
| expect(result.current).toBe(true); | |
| }); | |
| test("should return false when window width is exactly MOBILE_BREAKPOINT", () => { | |
| // Set width to the breakpoint (desktop according to '< MOBILE_BREAKPOINT') | |
| Object.defineProperty(window, "innerWidth", { | |
| writable: true, | |
| value: MOBILE_BREAKPOINT, | |
| }); | |
| const { result } = renderHook(() => useIsMobile()); | |
| expect(result.current).toBe(false); | |
| }); |
| import { useIsMobile } from "./use-mobile"; | ||
|
|
||
| // The hook uses 768 as the breakpoint internally. | ||
| const MOBILE_BREAKPOINT = 768; |
There was a problem hiding this comment.
Unused variable MOBILE_BREAKPOINT.
…ilwindcss-4.1.18 chore(deps): bump tailwindcss from 3.4.17 to 4.1.18
Added comprehensive unit tests for the
useIsMobilehook usingbun:testand@testing-library/react. The tests mockwindow.matchMediaandwindow.innerWidthto verify mobile detection logic across different screen sizes and resize events.happy-domis used to provide the DOM environment for testing.PR created automatically by Jules for task 1209804375518948078 started by @prem22k