fix: footer sticks to bottom and remove duplicate nav #2160
Conversation
Summary by CodeRabbit
WalkthroughIntroduces a main wrapper around page children in layout, and refactors Header to simplify desktop links, overhaul mobile navigation/actions, and remove NavDropdown/NavButton usage. No exported/public APIs changed. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
|
@kasya Fixed footer layout issue |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/src/components/Header.tsx (1)
106-114: Remove leftover NavButton block (undefined symbol)
NavButtonisn’t imported anymore; leaving this in will fail at build-time. It’s also redundant with the new Desktop Actions.Apply this diff:
- <NavButton - href="https://github.com/OWASP/Nest" - defaultIcon={faRegularStar} - hoverIcon={faSolidStar} - defaultIconColor="#FDCE2D" - hoverIconColor="#FDCE2D" - text="Star" - className="hidden" - /> +frontend/src/app/layout.tsx (1)
70-79: Apply flex layout to body (not just main) for a true sticky footerRight now
mainhasflex-1but the parent (body) isn’t a flex column, so the footer may still float on tall viewports. Moveflex min-h-screen flex-coltobodyand drop the inline style.Apply this diff:
- <body - className={`${geistSans.variable} ${geistMono.variable} antialiased`} - style={{ minHeight: '100vh' }} - > + <body + className={`${geistSans.variable} ${geistMono.variable} antialiased flex min-h-screen flex-col`} + > @@ - <BreadCrumbs /> - <main className="flex min-w-0 flex-1 flex-col">{children}</main> - + <BreadCrumbs /> + <main className="flex min-w-0 flex-1 flex-col">{children}</main>
🧹 Nitpick comments (3)
frontend/src/components/Header.tsx (3)
119-128: Open external links safely in a new tabAdd target and rel for security and UX.
Apply this diff:
<Button as="a" href="https://github.com/OWASP/Nest" + target="_blank" + rel="noopener noreferrer" variant="secondary" size="sm" className="hidden text-yellow-300 hover:text-yellow-200 md:flex" > @@ <Button as="a" href="https://owasp.org/donate/?reponame=www-project-nest&title=OWASP+Nest" + target="_blank" + rel="noopener noreferrer" variant="secondary" size="sm" className="hidden text-pink-300 hover:text-pink-200 md:flex" >Also applies to: 129-138
143-153: Improve mobile menu toggle a11y (aria-expanded, controls, dynamic label)Adds correct state and target wiring.
Apply this diff:
<div className="md:hidden"> <Button onPress={toggleMobileMenu} + aria-expanded={mobileMenuOpen} + aria-controls="mobile-drawer" className="bg-transparent text-slate-300 hover:bg-transparent hover:text-slate-100 focus:outline-hidden" > - <span className="sr-only">Open main menu</span> + <span className="sr-only">{mobileMenuOpen ? 'Close main menu' : 'Open main menu'}</span> {mobileMenuOpen ? ( <FontAwesomeIcon icon={faTimes} className="h-6 w-6" /> ) : ( <FontAwesomeIcon icon={faBars} className="h-6 w-6" /> )} </Button> </div> @@ - <div + <div + id="mobile-drawer" className={cn(Also applies to: 158-162
2-6: Remove unused solid icon importsThey’re not referenced after the refactor; drop to keep tree clean.
Apply this diff:
import { faBars, faTimes } from '@fortawesome/free-solid-svg-icons' -import { faHeart as faSolidHeart } from '@fortawesome/free-solid-svg-icons' -import { faStar as faSolidStar } from '@fortawesome/free-solid-svg-icons'
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
frontend/src/app/layout.tsx(1 hunks)frontend/src/components/Header.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/components/Header.tsx (5)
frontend/src/types/link.ts (1)
Link(1-7)frontend/src/utils/constants.ts (1)
headerLinks(5-27)frontend/src/utils/utility.ts (1)
cn(16-18)frontend/src/components/UserMenu.tsx (1)
UserMenu(13-119)frontend/src/components/ModeToggle.tsx (1)
ModeToggle(7-40)
🪛 Biome (2.1.2)
frontend/src/components/Header.tsx
[error] 82-82: Expected corresponding JSX closing tag for 'nav'.
Opening tag
closing tag
(parse)
[error] 261-261: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 263-264: Unexpected token. Did you mean {'}'} or }?
(parse)
[error] 264-264: expected < but instead the file ends
the file ends here
(parse)
🔇 Additional comments (2)
frontend/src/components/Header.tsx (1)
23-51: desktopViewMinWidth matches Tailwind’s default md breakpoint
desktopViewMinWidth is set to 768 and no custom screens override is defined in tailwind.config.mjs, so Tailwind’s default md breakpoint of 768px applies—no change needed.frontend/src/app/layout.tsx (1)
68-80: Confirm content offset for fixed headerHeader is fixed (
top-0 h-16). Ensure the first content below it has enough top offset (e.g., globalpt-16or equivalent) to avoid overlap. If not already handled elsewhere, addpt-16on the main container.Option (if needed):
- <main className="flex min-w-0 flex-1 flex-col">{children}</main> + <main className="flex min-w-0 flex-1 flex-col pt-16">{children}</main>
| {/* Desktop Links */} | ||
| <nav className="hidden flex-1 justify-start pl-6 font-medium md:flex"> | ||
| <div className="flex space-x-1"> | ||
| {headerLinks | ||
| .filter((link) => { | ||
| if (link.requiresGitHubAuth) { | ||
| return isGitHubAuthEnabled | ||
| } | ||
| if (link.requiresGitHubAuth) return isGitHubAuthEnabled | ||
| return true | ||
| }) | ||
| .map((link, i) => { | ||
| return link.submenu ? ( | ||
| <NavDropdown link={link} pathname={pathname} key={i} /> | ||
| ) : ( | ||
| <Link | ||
| key={link.text} | ||
| href={link.href || '/'} | ||
| className={cn( | ||
| 'navlink px-3 py-2 text-slate-700 hover:text-slate-800 dark:text-slate-300 dark:hover:text-slate-200', | ||
| pathname === link.href && 'font-bold text-blue-800 dark:text-white' | ||
| )} | ||
| aria-current="page" | ||
| > | ||
| {link.text} | ||
| </Link> | ||
| ) | ||
| })} | ||
| .map((link) => ( | ||
| <Link | ||
| key={link.text} | ||
| href={link.href || '/'} | ||
| className={cn( | ||
| 'navlink px-3 py-2 text-sm text-slate-700 hover:text-slate-800 dark:text-slate-300 dark:hover:text-slate-200', | ||
| pathname === link.href && 'font-bold text-blue-800 dark:text-white' | ||
| )} | ||
| aria-current={pathname === link.href ? 'page' : undefined} | ||
| > | ||
| {link.text} | ||
| </Link> | ||
| ))} | ||
| </div> | ||
|
|
||
| </div> |
There was a problem hiding this comment.
Fix missing closing tag: use instead of
This is causing the parse error Biome reported. Close the opened nav element properly.
Apply this diff:
- </div>
+ </nav>📝 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.
| {/* Desktop Links */} | |
| <nav className="hidden flex-1 justify-start pl-6 font-medium md:flex"> | |
| <div className="flex space-x-1"> | |
| {headerLinks | |
| .filter((link) => { | |
| if (link.requiresGitHubAuth) { | |
| return isGitHubAuthEnabled | |
| } | |
| if (link.requiresGitHubAuth) return isGitHubAuthEnabled | |
| return true | |
| }) | |
| .map((link, i) => { | |
| return link.submenu ? ( | |
| <NavDropdown link={link} pathname={pathname} key={i} /> | |
| ) : ( | |
| <Link | |
| key={link.text} | |
| href={link.href || '/'} | |
| className={cn( | |
| 'navlink px-3 py-2 text-slate-700 hover:text-slate-800 dark:text-slate-300 dark:hover:text-slate-200', | |
| pathname === link.href && 'font-bold text-blue-800 dark:text-white' | |
| )} | |
| aria-current="page" | |
| > | |
| {link.text} | |
| </Link> | |
| ) | |
| })} | |
| .map((link) => ( | |
| <Link | |
| key={link.text} | |
| href={link.href || '/'} | |
| className={cn( | |
| 'navlink px-3 py-2 text-sm text-slate-700 hover:text-slate-800 dark:text-slate-300 dark:hover:text-slate-200', | |
| pathname === link.href && 'font-bold text-blue-800 dark:text-white' | |
| )} | |
| aria-current={pathname === link.href ? 'page' : undefined} | |
| > | |
| {link.text} | |
| </Link> | |
| ))} | |
| </div> | |
| </div> | |
| {/* Desktop Links */} | |
| <nav className="hidden flex-1 justify-start pl-6 font-medium md:flex"> | |
| <div className="flex space-x-1"> | |
| {headerLinks | |
| .filter((link) => { | |
| if (link.requiresGitHubAuth) return isGitHubAuthEnabled | |
| return true | |
| }) | |
| .map((link) => ( | |
| <Link | |
| key={link.text} | |
| href={link.href || '/'} | |
| className={cn( | |
| 'navlink px-3 py-2 text-sm text-slate-700 hover:text-slate-800 dark:text-slate-300 dark:hover:text-slate-200', | |
| pathname === link.href && 'font-bold text-blue-800 dark:text-white' | |
| )} | |
| aria-current={pathname === link.href ? 'page' : undefined} | |
| > | |
| {link.text} | |
| </Link> | |
| ))} | |
| </div> | |
| </nav> |
🧰 Tools
🪛 Biome (2.1.2)
[error] 82-82: Expected corresponding JSX closing tag for 'nav'.
Opening tag
closing tag
(parse)
🤖 Prompt for AI Agents
In frontend/src/components/Header.tsx around lines 81 to 104, the opened <nav>
element is incorrectly closed with </div>, causing an HTML/JSX parse error;
replace the incorrect closing tag with </nav> to properly close the nav element
and restore valid JSX structure.
| .map((link) => ( | ||
| <Link | ||
| key={link.text} | ||
| href={link.href || '/'} | ||
| className={cn( | ||
| 'navlink px-3 py-2 text-sm text-slate-700 hover:text-slate-800 dark:text-slate-300 dark:hover:text-slate-200', | ||
| pathname === link.href && 'font-bold text-blue-800 dark:text-white' | ||
| )} | ||
| aria-current={pathname === link.href ? 'page' : undefined} | ||
| > | ||
| {link.text} | ||
| </Link> | ||
| ))} |
There was a problem hiding this comment.
Desktop nav regression: submenu parents link to “/”
On desktop, links with a submenu (e.g., “Community”) currently resolve to “/”, removing access to submenu pages. Use the first submenu item as the fallback href and improve active matching.
Apply this diff:
- .map((link) => (
- <Link
- key={link.text}
- href={link.href || '/'}
- className={cn(
- 'navlink px-3 py-2 text-sm text-slate-700 hover:text-slate-800 dark:text-slate-300 dark:hover:text-slate-200',
- pathname === link.href && 'font-bold text-blue-800 dark:text-white'
- )}
- aria-current={pathname === link.href ? 'page' : undefined}
- >
- {link.text}
- </Link>
- ))}
+ .map((link) => {
+ const href = link.href ?? link.submenu?.[0]?.href ?? '/'
+ const isActive = href !== '/' && pathname.startsWith(href)
+ return (
+ <Link
+ key={link.text}
+ href={href}
+ className={cn(
+ 'navlink px-3 py-2 text-sm text-slate-700 hover:text-slate-800 dark:text-slate-300 dark:hover:text-slate-200',
+ isActive && 'font-bold text-blue-800 dark:text-white'
+ )}
+ aria-current={isActive ? 'page' : undefined}
+ >
+ {link.text}
+ </Link>
+ )
+ })}📝 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.
| .map((link) => ( | |
| <Link | |
| key={link.text} | |
| href={link.href || '/'} | |
| className={cn( | |
| 'navlink px-3 py-2 text-sm text-slate-700 hover:text-slate-800 dark:text-slate-300 dark:hover:text-slate-200', | |
| pathname === link.href && 'font-bold text-blue-800 dark:text-white' | |
| )} | |
| aria-current={pathname === link.href ? 'page' : undefined} | |
| > | |
| {link.text} | |
| </Link> | |
| ))} | |
| .map((link) => { | |
| const href = link.href ?? link.submenu?.[0]?.href ?? '/' | |
| const isActive = href !== '/' && pathname.startsWith(href) | |
| return ( | |
| <Link | |
| key={link.text} | |
| href={href} | |
| className={cn( | |
| 'navlink px-3 py-2 text-sm text-slate-700 hover:text-slate-800 dark:text-slate-300 dark:hover:text-slate-200', | |
| isActive && 'font-bold text-blue-800 dark:text-white' | |
| )} | |
| aria-current={isActive ? 'page' : undefined} | |
| > | |
| {link.text} | |
| </Link> | |
| ) | |
| })} |
🤖 Prompt for AI Agents
In frontend/src/components/Header.tsx around lines 89 to 101, links that have
submenus currently fall back to "/" which breaks access to submenu pages; change
the fallback href to the first submenu item's href (e.g., link.href ||
link.items?.[0]?.href || '/') and update the active matching so the navitem is
considered active when the current pathname matches the link.href or any submenu
href (or startsWith the link href if appropriate) — adjust the aria-current and
className condition to use this improved check (e.g., pathname === link.href ||
link.items?.some(i => i.href === pathname) or pathname.startsWith(link.href)).
| </div> | ||
| </div> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
Remove stray “)}” after the mobile drawer
This unmatched token breaks parsing/compilation.
Apply this diff:
- )}
+ 📝 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.
| )} |
🧰 Tools
🪛 Biome (2.1.2)
[error] 261-261: Unexpected token. Did you mean {'}'} or }?
(parse)
🤖 Prompt for AI Agents
In frontend/src/components/Header.tsx around line 261, there is a stray closing
token ")}" left after the mobile drawer which breaks compilation; remove that
unmatched ")}" so the JSX/TSX returns and component structure remain balanced,
then run TypeScript/JSX compile to verify no other mismatched braces remain.
|
Hi @OUMIMANDAL ! Sorry, but this doesn't work. I can tell that you haven't even ran the code, because it doesn't compile right now - there are missing closing tags for some items and missed imports. Btw, CodeRabbit told you so in this PR too, so you could have fixed it quickly. I appreciate your work on this one, but that's not the quality of PRs we are looking for. I'm going to close this PR and reassign the issue. |
|
Hi @kasya Thanks for the feedback — I apologize for the errors. I should’ve tested the code properly before submitting. My monitor has a limited width , which makes it hard to preview larger layouts. Still, I should’ve used dev tools to check responsiveness — I’ll do that going forward. I’m committed to improving and would love to collaborate on future tasks. Thanks again for your guidance. Best regards, |
2 participants
This PR fixes:
Uses
flex min-h-screen flex-colin layout.tsx for proper layout.Fixes #2033