fix(theme): sidebar drawer through tablet, persistent rail only at xl#61
Merged
Conversation
…ew cards to ai page Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…y at xl Move all sidebar breakpoints from md (768px) / lg (1024px) to xl (1280px) so the drawer overlay is used on mobile and tablet, and the persistent rail only appears at true desktop widths (≥1280px). - use-mobile.tsx: MOBILE_BREAKPOINT 768 → 1280 - sidebar.tsx: all rail-gating md: utilities → xl: (block, flex, inset variants, hit-area after:hidden, menu-action opacity-0) - ShowcaseLayout.tsx: SidebarTrigger lg:hidden → xl:hidden; main content lg:px-10 → xl:px-10 so tablet content uses sm:px-6 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
SidebarMenuAction's after:-inset-2 hit-area expansion was missed in the md→xl migration because its 8-space indentation didn't match the replace_all pattern used on SidebarGroupAction (10-space). Both rail handle hit-areas now consistently hide only at xl (≥1280px), matching the drawer boundary. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe PR realigns responsive layout breakpoints by raising the mobile detection threshold from 768px to 1280px, then updates sidebar, header, and layout components to use ChangesResponsive breakpoint alignment and overview content update
Possibly Related PRs
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
🚥 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)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/hooks/use-mobile.tsx (1)
3-18:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInitialize
isMobilefrom the current media query.With the breakpoint moved to
1280px, widths768-1279pxnow mount in the desktop sidebar path until the effect runs, then flip to the drawer. Seed the state frommatchMedia(...).matchesand reusemql.matchesin the listener so tablets start in the correct mode.Suggested fix
const MOBILE_BREAKPOINT = 1280; export function useIsMobile() { - const [isMobile, setIsMobile] = React.useState<boolean | undefined>(undefined); + const getIsMobile = () => + typeof window !== "undefined" && + window.matchMedia(`(max-width: ${MOBILE_BREAKPOINT - 1}px)`).matches; + + const [isMobile, setIsMobile] = React.useState(getIsMobile); React.useEffect(() => { const mql = window.matchMedia(`(max-width: ${MOBILE_BREAKPOINT - 1}px)`); - const onChange = () => { - setIsMobile(window.innerWidth < MOBILE_BREAKPOINT); + const onChange = (event: MediaQueryListEvent) => { + setIsMobile(event.matches); }; mql.addEventListener("change", onChange); - setIsMobile(window.innerWidth < MOBILE_BREAKPOINT); + setIsMobile(mql.matches); return () => mql.removeEventListener("change", onChange); }, []); - return !!isMobile; + return isMobile; }🤖 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/hooks/use-mobile.tsx` around lines 3 - 18, The useIsMobile hook currently seeds isMobile from window.innerWidth and uses window.innerWidth in the listener, causing a flash; change initialization to seed from the media query (use const mql = window.matchMedia(...); initialize state from mql.matches) and update the onChange handler to use mql.matches rather than window.innerWidth; keep MOBILE_BREAKPOINT and the addEventListener/removeEventListener on mql and still return a boolean from useIsMobile.
🤖 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/components/ui/sidebar.tsx`:
- Line 176: The shadcn primitive in Sidebar has app-specific breakpoint classes
(e.g., className="group peer hidden text-sidebar-foreground xl:block" and other
occurrences with "xl:" at the same file) which must be removed; restore the
primitive to baseline by stripping all "xl:" breakpoint utility classes from the
Sidebar primitive JSX (search for the exact class strings like "xl:block" in
this file), then create a composed wrapper component (e.g., SidebarResponsive or
a CVA variant) that imports the original primitive and applies the xl breakpoint
behavior (adds the xl:block/other xl: classes) so app-specific responsiveness
lives outside the ui primitive; update usages to use the new wrapper where the
xl behavior is required.
---
Outside diff comments:
In `@src/hooks/use-mobile.tsx`:
- Around line 3-18: The useIsMobile hook currently seeds isMobile from
window.innerWidth and uses window.innerWidth in the listener, causing a flash;
change initialization to seed from the media query (use const mql =
window.matchMedia(...); initialize state from mql.matches) and update the
onChange handler to use mql.matches rather than window.innerWidth; keep
MOBILE_BREAKPOINT and the addEventListener/removeEventListener on mql and still
return a boolean from useIsMobile.
🪄 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: 0c1270f3-dbe8-443e-8bd6-15ac62cd0e57
📒 Files selected for processing (4)
src/components/ShowcaseLayout.tsxsrc/components/ui/sidebar.tsxsrc/hooks/use-mobile.tsxsrc/pages/OverviewPage.tsx
| <div | ||
| ref={ref} | ||
| className="group peer hidden text-sidebar-foreground md:block" | ||
| className="group peer hidden text-sidebar-foreground xl:block" |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Keep the xl behavior out of the shadcn primitive.
These breakpoint changes fork src/components/ui/sidebar.tsx for app-specific behavior. Please move them into a wrapper/composed sidebar variant so future shadcn updates stay mergeable.
As per coding guidelines, "Never directly modify src/components/ui/ files (shadcn/ui primitives) — extend them via CVA variants instead. Compose ui/ primitives into atoms/molecules; never rebuild primitives from scratch"
Also applies to: 195-195, 278-278, 386-386, 493-499
🤖 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/sidebar.tsx` at line 176, The shadcn primitive in Sidebar
has app-specific breakpoint classes (e.g., className="group peer hidden
text-sidebar-foreground xl:block" and other occurrences with "xl:" at the same
file) which must be removed; restore the primitive to baseline by stripping all
"xl:" breakpoint utility classes from the Sidebar primitive JSX (search for the
exact class strings like "xl:block" in this file), then create a composed
wrapper component (e.g., SidebarResponsive or a CVA variant) that imports the
original primitive and applies the xl breakpoint behavior (adds the
xl:block/other xl: classes) so app-specific responsiveness lives outside the ui
primitive; update usages to use the new wrapper where the xl behavior is
required.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
md(768px) /lg(1024px) toxl(1280px) — tablets now get an overlay drawer instead of a cramped persistent railuseIsMobile()updated (MOBILE_BREAKPOINT768 → 1280) so shadcn's internal drawer/persistent decision aligns with the new boundarysidebar.tsxmigrated (md:block,md:flex, inset variants, both hit-areaafter:md:hiddenoccurrences,showOnHoveropacity)SidebarTriggervisibility changedlg:hidden→xl:hidden; main content paddinglg:px-10→xl:px-10so the roomier gutter only kicks in when the rail is presentBreakpoint behaviour after this PR
Test plan
xl:px-10padding🤖 Generated with Claude Code
Summary by CodeRabbit