ref: fix bugs after reggression testing#64
Conversation
liudmylasovetovs
requested review from
AM1007 and
ViktorSvertoka
as code owners
✅ Deploy Preview for develop-devlovers ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughThe PR restructures the shop layout by introducing a new ShopShell wrapper component that consolidates theming and provider setup, simplifies the shop layout to remove duplicate provider imports, updates the main app layout with conditional ShopShell usage and restructured site navigation, improves mobile navigation accessibility, and extends remote image configuration. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/app/[locale]/layout.tsx (1)
37-46: Critical: Double ThemeProvider nesting in shop scope.The outer
ThemeProvider(lines 37-42) wraps the entire layout, including the shop scope. However,ShopShell(line 44) contains its ownShopThemeProvider(seefrontend/components/shop/shop-shell.tsxline 20). This creates nested theme providers in the shop scope:ThemeProvider (outer, line 37) └─ ShopShell (line 44) └─ ShopThemeProvider (inner, in shop-shell.tsx)This nesting can cause:
- Theme context conflicts where the inner provider overrides the outer
- Loss of the
disableTransitionOnChangeprop (present in outer, absent in inner)- Unexpected theme behavior or flickering during navigation
🔎 Recommended fix: Remove ThemeProvider from shop scope
Option 1 (Recommended): Conditionally wrap only the non-shop scope with ThemeProvider:
return ( <NextIntlClientProvider messages={messages}> - <ThemeProvider - attribute="class" - defaultTheme="system" - enableSystem - disableTransitionOnChange - > {scope === 'shop' ? ( + <ThemeProvider + attribute="class" + defaultTheme="system" + enableSystem + disableTransitionOnChange + > <ShopShell showAdminLink={showAdminNavLink}> <main className="mx-auto px-6 min-h-[80vh]">{children}</main> </ShopShell> + <Footer /> + <Toaster position="top-right" richColors expand /> + </ThemeProvider> ) : ( + <ThemeProvider + attribute="class" + defaultTheme="system" + enableSystem + disableTransitionOnChange + > <> {/* site header */} <main className="mx-auto px-6 min-h-[80vh]">{children}</main> </> + <Footer /> + <Toaster position="top-right" richColors expand /> + </ThemeProvider> )} - <Footer /> - <Toaster position="top-right" richColors expand /> - </ThemeProvider> </NextIntlClientProvider> );Option 2: Remove
ShopThemeProviderfromShopShelland rely on the outerThemeProvider. This may conflict with the shop's distinct theming requirements mentioned in the PR objectives.
🧹 Nitpick comments (4)
frontend/components/shop/header/mobile-nav.tsx (3)
17-26: Formatting inconsistency in button element.The button's attributes and content have inconsistent indentation (mixing component-level and extra indentation). Consider aligning the formatting for readability.
🔎 Suggested formatting fix
<button - type="button" - onClick={() => setOpen((prev) => !prev)} - className="flex h-9 w-9 items-center justify-center rounded-md text-muted-foreground transition-colors hover:bg-secondary hover:text-foreground md:hidden" - aria-label="Toggle menu" - aria-expanded={open ? "true" : "false"} - aria-controls="shop-mobile-nav" -> - {open ? <X className="h-5 w-5" /> : <Menu className="h-5 w-5" />} -</button> + type="button" + onClick={() => setOpen((prev) => !prev)} + className="flex h-9 w-9 items-center justify-center rounded-md text-muted-foreground transition-colors hover:bg-secondary hover:text-foreground md:hidden" + aria-label="Toggle menu" + aria-expanded={open ? "true" : "false"} + aria-controls="shop-mobile-nav" + > + {open ? <X className="h-5 w-5" /> : <Menu className="h-5 w-5" />} + </button>
30-37: Consider using a<div>instead of<button>for the click-outside overlay.Using a
<button>for the overlay means screen readers will announce it as an interactive element. A<div>withonClickandrole="presentation"(or no role) would be more semantically appropriate for a dismissal backdrop, as the primary close mechanism is already provided by the toggle button.🔎 Suggested change
- <button - type="button" - aria-label="Close menu" - className="fixed inset-0 z-40 md:hidden" + <div + className="fixed inset-0 z-40 bg-black/20 md:hidden" onClick={() => setOpen(false)} />This also adds a subtle visual backdrop (
bg-black/20) which improves UX by indicating the menu is modal.
31-31: Consider using English for code comments.The comment "overlay для кліку поза меню" is in Ukrainian. For consistency and maintainability across the team, consider using English: "overlay for click outside menu".
frontend/components/shop/shop-shell.tsx (1)
9-9: Consider relocating the theme CSS to the components directory.The CSS import crosses from
components/shop/intoapp/[locale]/shop/, which couples this component to the app directory structure. If the app folder structure changes, this import path will break. Consider movingshop-theme.csstocomponents/shop/or a shared styles directory for better modularity.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
frontend/app/[locale]/layout.tsx(2 hunks)frontend/app/[locale]/shop/admin/products/[id]/edit/page.tsx(3 hunks)frontend/app/[locale]/shop/layout.tsx(1 hunks)frontend/components/shop/header/mobile-nav.tsx(1 hunks)frontend/components/shop/product-card.tsx(2 hunks)frontend/components/shop/shop-header.tsx(3 hunks)frontend/components/shop/shop-shell.tsx(1 hunks)frontend/next.config.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
frontend/components/shop/shop-shell.tsx (1)
frontend/components/shop/cart-provider.tsx (1)
CartProvider(35-137)
frontend/components/shop/header/mobile-nav.tsx (1)
frontend/components/shop/header/nav-links.tsx (1)
NavLinks(31-62)
frontend/app/[locale]/shop/admin/products/[id]/edit/page.tsx (2)
frontend/db/index.ts (1)
db(17-17)frontend/db/schema/shop.ts (1)
products(32-74)
frontend/app/[locale]/layout.tsx (4)
frontend/components/shop/shop-shell.tsx (1)
ShopShell(11-28)frontend/drizzle/schema.ts (1)
user(119-130)frontend/components/auth/logoutButton.tsx (1)
LogoutButton(6-12)frontend/components/shared/LanguageSwitcher.tsx (1)
LanguageSwitcher(9-70)
🔇 Additional comments (9)
frontend/components/shop/product-card.tsx (1)
14-17: LGTM! Good defensive handling for product images.The fallback logic correctly handles missing or empty image values by checking both type and length before using the product image.
frontend/next.config.ts (1)
19-23: LGTM! Cloudinary remote pattern added correctly.This enables product images hosted on Cloudinary to be optimized by Next.js Image component.
frontend/app/[locale]/shop/layout.tsx (1)
3-4: LGTM! Layout correctly simplified after provider extraction to ShopShell.The fragment wrapper is functional. If no shop-specific layout logic is anticipated, this file could potentially be removed, but keeping it as a minimal wrapper is also acceptable for future extensibility.
frontend/app/[locale]/shop/admin/products/[id]/edit/page.tsx (2)
11-17: LGTM! Correct async params pattern for Next.js 15+.The
paramsis now correctly typed asPromise<{ id: string }>and awaited before parsing. This aligns with Next.js 15's async dynamic APIs.
37-55: No action needed. TheinitialValuescorrectly omitsimagePublicIdbecause it is a server-managed field for Cloudinary operations and does not require client-side form handling. The field exists in the schema for backend image management but is intentionally excluded from the form's input type (ProductAdminInput) and validation schema.frontend/components/shop/shop-header.tsx (2)
20-26: LGTM! Clean implementation of the back-to-main-site navigation.The Home link is well-implemented with proper accessibility (
aria-label) and consistent styling matching other header buttons.
40-52: Verify: Search and Account buttons appear to be placeholder implementations.These buttons have no
onClickhandlers. If this is intentional placeholder functionality, consider addingdisabledattribute or a TODO comment to indicate future implementation is needed.frontend/app/[locale]/layout.tsx (2)
13-13: LGTM!The import correctly brings in the new
ShopShellcomponent that consolidates shop theming, cart context, and header rendering.
117-122: LGTM!The main content container uses consistent styling across both shop and non-shop scopes (
mx-auto px-6 min-h-[80vh]), and theFooterandToasterare appropriately rendered globally for both scopes.
| <> | ||
| <header className="bg-white dark:bg-neutral-900 border-b border-gray-200 dark:border-neutral-800 shadow-sm sticky top-0 z-50 transition-colors"> | ||
| <div className="max-w-5xl mx-auto flex items-center justify-between px-6 py-4"> | ||
| <Link href="/" className="flex items-center gap-2"> | ||
| <span className="text-xl font-bold text-blue-600 dark:text-blue-500"> | ||
| DevLovers | ||
| </span> | ||
| </Link> | ||
|
|
||
| {!user ? ( | ||
| <nav className="flex items-center gap-6 text-gray-700 dark:text-gray-300 font-medium"> | ||
| <Link | ||
| href="/login" | ||
| href="/q&a" | ||
| className="hover:text-blue-600 dark:hover:text-blue-400 transition" | ||
| > | ||
| Log in | ||
| Q&A | ||
| </Link> | ||
| <Link | ||
| href="/quiz/react-fundamentals" | ||
| className="hover:text-blue-600 dark:hover:text-blue-400 transition" | ||
| > | ||
| Quiz | ||
| </Link> | ||
| <Link | ||
| href="/leaderboard" | ||
| className="hover:text-blue-600 dark:hover:text-blue-400 transition" | ||
| > | ||
| Leaderboard | ||
| </Link> | ||
| <Link | ||
| href="/blog" | ||
| className="hover:text-blue-600 dark:hover:text-blue-400 transition" | ||
| > | ||
| Blog | ||
| </Link> | ||
| <Link | ||
| href="/about" | ||
| className="hover:text-blue-600 dark:hover:text-blue-400 transition" | ||
| > | ||
| About | ||
| </Link> | ||
| <Link | ||
| href="/contacts" | ||
| className="hover:text-blue-600 dark:hover:text-blue-400 transition" | ||
| > | ||
| Contacts | ||
| </Link> | ||
| <Link | ||
| href="/shop" | ||
| className="hover:text-blue-600 dark:hover:text-blue-400 transition" | ||
| > | ||
| Shop | ||
| </Link> | ||
| ) : ( | ||
| <LogoutButton /> | ||
| )} | ||
|
|
||
| <LanguageSwitcher /> | ||
| </nav> | ||
| </div> | ||
| </header> | ||
| )} | ||
| {!user ? ( | ||
| <Link | ||
| href="/login" | ||
| className="hover:text-blue-600 dark:hover:text-blue-400 transition" | ||
| > | ||
| Log in | ||
| </Link> | ||
| ) : ( | ||
| <LogoutButton /> | ||
| )} | ||
|
|
||
| <main className="mx-auto px-6 min-h-[80vh]">{children}</main> | ||
| <LanguageSwitcher /> | ||
| </nav> | ||
| </div> | ||
| </header> | ||
|
|
||
| <main className="mx-auto px-6 min-h-[80vh]">{children}</main> | ||
| </> | ||
| )} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for the layout.tsx file and check its content
echo "=== Checking layout.tsx file structure ==="
fd -n "layout.tsx" frontend/app
# Check for any responsive classes on nav or header elements
echo -e "\n=== Searching for responsive navigation patterns ==="
rg -n "hidden|flex|md:|lg:|sm:" frontend/app/\[locale\]/layout.tsx -A 3 -B 1
# Search for mobile menu or responsive nav components
echo -e "\n=== Searching for mobile menu components ==="
fd -e tsx -e ts frontend/components | xargs rg -l -i "mobile|burger|menu|drawer|sidebar" 2>/dev/null | head -20Repository: DevLoversTeam/devlovers.net
Length of output: 1902
🏁 Script executed:
#!/bin/bash
# Read the full layout.tsx file
echo "=== Full layout.tsx file content (relevant portion) ==="
cat frontend/app/\[locale\]/layout.tsx | head -150
# Check if mobile-nav or similar exists for site header
echo -e "\n=== Search for mobile nav in site components ==="
find frontend/components -type f \( -name "*mobile*" -o -name "*nav*" \) | grep -v shop | grep -v node_modules
# Check the shop mobile-nav to understand pattern
echo -e "\n=== Shop mobile-nav.tsx file ==="
head -100 frontend/components/shop/header/mobile-nav.tsxRepository: DevLoversTeam/devlovers.net
Length of output: 6076
Add mobile-responsive navigation for the site header.
The site header navigation (line 57) uses a horizontal flex layout without responsive breakpoints. Unlike the shop header which has a mobile-nav component with toggle button (md:hidden), the site header will overflow or wrap awkwardly on mobile devices. Apply one of the following:
- Hide the horizontal nav on mobile with
hidden md:flexand implement a toggle button (like the shop's MobileNav component), or - Add responsive wrapping with breakpoint-aware classes to ensure navigation remains accessible on smaller screens
The structure, localization, conditional user handling, and language switcher are correct. Reference frontend/components/shop/header/mobile-nav.tsx for the responsive mobile navigation pattern used elsewhere in the project.
🤖 Prompt for AI Agents
In frontend/app/[locale]/layout.tsx around lines 48–119, the site header's
horizontal nav is not mobile-responsive and will overflow on small screens;
change the nav container to be hidden on small screens (e.g., replace current
classes with "hidden md:flex") and add a mobile toggle/button (visible on small
screens with "md:hidden") that opens the existing MobileNav pattern used by the
shop; import frontend/components/shop/header/mobile-nav.tsx (or reuse its
logic), render the MobileNav when the toggle is activated, and ensure the mobile
menu includes the same links plus the conditional Login/Logout and
LanguageSwitcher and uses proper accessible attributes (aria-expanded,
aria-controls) and focus management.
| export function ShopShell({ | ||
| children, | ||
| showAdminLink, | ||
| }: { | ||
| children: React.ReactNode | ||
| showAdminLink: boolean | ||
| }) { | ||
| return ( | ||
| <div className="shop-scope min-h-screen"> | ||
| <ShopThemeProvider attribute="class" defaultTheme="system" enableSystem> | ||
| <CartProvider> | ||
| <ShopHeader showAdminLink={showAdminLink} /> | ||
| {children} | ||
| </CartProvider> | ||
| </ShopThemeProvider> | ||
| </div> | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify ThemeProvider usage in parent layout and check for nesting conflicts
# Search for ThemeProvider in layout.tsx
echo "=== Checking for ThemeProvider in parent layout ==="
rg -n -A 10 -B 2 'ThemeProvider' frontend/app/\[locale\]/layout.tsx
echo ""
echo "=== Checking for ShopShell usage in parent layout ==="
# Search for ShopShell usage in layout.tsx to see the nesting
rg -n -A 5 -B 2 'ShopShell' frontend/app/\[locale\]/layout.tsx
echo ""
echo "=== Also check if ShopThemeProvider is specifically used ==="
rg -n 'ShopThemeProvider' frontend/app/\[locale\]/layout.tsxRepository: DevLoversTeam/devlovers.net
Length of output: 2246
🏁 Script executed:
#!/bin/bash
# Check the imports in shop-shell.tsx
head -n 30 frontend/components/shop/shop-shell.tsxRepository: DevLoversTeam/devlovers.net
Length of output: 803
🏁 Script executed:
#!/bin/bash
# Check the shop theme-provider to see if it's using next-themes
cat frontend/components/shop/theme-provider.tsxRepository: DevLoversTeam/devlovers.net
Length of output: 825
🏁 Script executed:
#!/bin/bash
# Check the theme/ThemeProvider to see if it's also using next-themes
cat frontend/components/theme/ThemeProvider.tsxRepository: DevLoversTeam/devlovers.net
Length of output: 362
Remove redundant ShopThemeProvider nesting.
The parent layout already wraps the entire app with a ThemeProvider from next-themes (lines 37-123 in frontend/app/[locale]/layout.tsx). Adding another next-themes ThemeProvider inside ShopShell creates nested theme providers, which is not supported by the next-themes library and can cause context conflicts or unexpected behavior. Use the existing parent theme context instead of creating a duplicate provider in the shop scope.
🤖 Prompt for AI Agents
In frontend/components/shop/shop-shell.tsx around lines 11 to 28, remove the
nested ShopThemeProvider so the component uses the parent next-themes
ThemeProvider from the app layout; wrap only the shop scope div with
CartProvider and ShopHeader/children, delete the ShopThemeProvider element (and
its props) and remove its import if it becomes unused to avoid duplicate theme
contexts.
2 participants
Description
This PR fixes multiple UX and layout issues in the Shop area related to header composition, cart state visibility, mobile navigation behavior, and accessibility warnings.
What was changed and why
Cart item count visibility fixed
Ensured that
CartProviderwraps bothShopHeaderand all shop pages in the same React tree. This resolves the issue where the cart badge count was always0in the shop header due to the header being rendered outside of the cart context.Separated platform header and shop header responsibilities
The platform header remains in the global layout, while the shop header is conditionally rendered for shop routes, avoiding duplicated headers and preserving distinct branding between platform and shop.
Restored shop-specific styling for header and cart button
Wrapped the shop header in the shop-specific scope (
shop-scope) and styles so accent colors and theming are correctly applied, instead of inheriting platform styles.Improved mobile navigation behavior in shop header
Fixed the mobile menu so it opens consistently below the header using
position: fixedinstead of flowing in the header layout. Added an overlay to close the menu when clicking outside.Resolved ARIA false-positive warnings
Updated
aria-expandedto render valid string values ("true"/"false") in the DOM. Verified that the HTML output is correct; remaining warnings are confirmed as false positives from Edge Tools static analysis.Overall, this PR stabilizes the shop header architecture, restores correct cart UX, and improves mobile navigation reliability without changing the existing routing structure.
Related Issue
Issue: #<issue_number>
Changes
CartProviderusage for shop header and pagesDatabase Changes (if applicable)
How Has This Been Tested?
Screenshots (if applicable)
Checklist
Before submitting
Reviewers
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.