feat: Enhance root layout accessibility #128
Conversation
WalkthroughRoot and general layout files updated to change wrapper elements: the root layout now places children inside a semantic Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| <ErrorBoundary> | ||
| {children} | ||
| <main> | ||
| {children} | ||
| </main> | ||
| <ToastProvider /> | ||
| </ErrorBoundary> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/app/layout.tsxRepository: Authormaton/authormaton
Length of output: 898
🏁 Script executed:
rg -n '<main\b' --type=tsx --type=ts --type=jsx --type=js .Repository: Authormaton/authormaton
Length of output: 94
🏁 Script executed:
rg -i 'skip.*content|skiplink|skip-link' --type=tsx --type=ts --type=jsx --type=js .Repository: Authormaton/authormaton
Length of output: 94
🏁 Script executed:
rg -n '<main' . --glob='*.{tsx,ts,jsx,js}'Repository: Authormaton/authormaton
Length of output: 375
🏁 Script executed:
find src/app -name '*.tsx' -o -name '*.ts' | head -20Repository: Authormaton/authormaton
Length of output: 292
🏁 Script executed:
rg -i 'skip' src/ --type=jsx --type=js -C 2Repository: Authormaton/authormaton
Length of output: 94
🏁 Script executed:
cat -n src/app/\(general\)/layout.tsxRepository: Authormaton/authormaton
Length of output: 945
🏁 Script executed:
cat -n src/components/layouts/GeneralLayout.tsx | head -40Repository: Authormaton/authormaton
Length of output: 1641
🏁 Script executed:
cat -n src/components/ui/sidebar.tsx | sed -n '280,290p'Repository: Authormaton/authormaton
Length of output: 603
🏁 Script executed:
git ls-files src/app | grep -E 'layout|page' | sortRepository: Authormaton/authormaton
Length of output: 262
🏁 Script executed:
cat -n src/app/\(general\)/layout.tsx | grep -A 5 -B 5 'html\|body'Repository: Authormaton/authormaton
Length of output: 728
Critical issue confirmed: Multiple nested <main> elements exist in the codebase.
Your concern about nested <main> landmarks is valid and needs immediate attention. The current structure creates three levels of <main> elements for routes under (general):
- Root layout (
src/app/layout.tsx, line 20):<main> - General route group layout (
src/app/(general)/layout.tsx, line 13):<main>(nested) - GeneralLayout component (
src/components/layouts/GeneralLayout.tsx, line 32):<main>(doubly nested)
This violates HTML semantics—only one <main> landmark should exist per document. Additionally, src/app/(general)/layout.tsx should not re-declare <html> and <body> tags; these belong only in the root layout.
Remove the redundant <main> from the (general) layout and the GeneralLayout component, keeping only the root layout's <main>.
🤖 Prompt for AI Agents
In src/app/layout.tsx around lines 19-24 keep the single root <main> as the
document's only main landmark; remove the redundant <main> elements from
src/app/(general)/layout.tsx (remove any <main> wrapper and ensure it does not
re-declare <html> or <body>) and from src/components/layouts/GeneralLayout.tsx
(replace its <main> with a <div> or fragment), updating imports/props as needed
so children still render in the root <main> without changing semantics.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/app/(general)/layout.tsx (2)
7-7: Rename function to avoid confusion.The function is named
RootLayoutbut resides in a route group ((general)/layout.tsx), making it a nested layout rather than the actual root layout. This naming could mislead developers.Consider renaming to better reflect its purpose:
-export default async function RootLayout({ children }: { children: React.ReactNode }) { +export default async function GeneralRootLayout({ children }: { children: React.ReactNode }) {
11-15: Consider adding ARIA landmark role for better accessibility.Since the PR focuses on accessibility improvements and this container div holds the main content area, adding an appropriate ARIA role would further enhance navigation for assistive technologies. However, verify that the actual root layout (src/app/layout.tsx) provides the
<main>element first to avoid conflicts.If the root layout does NOT provide a
<main>wrapper, consider:- <div className='w-full'> + <div className='w-full' role='main'>Otherwise, the current implementation is acceptable as long as the semantic
<main>is properly placed at the root level.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
src/app/(general)/layout.tsx(1 hunks)src/components/layouts/GeneralLayout.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/app/(general)/layout.tsx (3)
src/components/common/Sidebar/AppSidebar.tsx (1)
AppSidebar(8-45)src/contexts/LoadingContext.tsx (1)
LoadingProvider(12-21)src/components/layouts/GeneralLayout.tsx (1)
GeneralLayout(8-36)
🔇 Additional comments (1)
src/components/layouts/GeneralLayout.tsx (1)
32-32: The change is sound and properly supported by the root layout.The root layout (src/app/layout.tsx) correctly wraps children in a
<main>element at line 20, confirming that the change from<main>to<div>in GeneralLayout.tsx aligns with the PR objective to centralize the<main>landmark at the root level. This prevents duplicate<main>elements and maintains proper semantic HTML structure.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/(general)/layout.tsx (1)
1-1: Remove unused import.The
AppSidebarimport is no longer used after correctly removing the duplicate sidebar render (as noted in the past review comment). Clean up by removing this import.Apply this diff:
-import { AppSidebar } from '@/components/common/Sidebar/AppSidebar'; import { GeneralLayout } from '@/components/layouts/GeneralLayout';
🧹 Nitpick comments (1)
src/app/(general)/layout.tsx (1)
7-7: Consider renaming for clarity.The function is named
RootLayoutbut this file is located atsrc/app/(general)/layout.tsx, which is a route-group layout, not the application root layout. Consider renaming toGeneralGroupLayoutor similar to avoid confusion.Apply this diff:
-export default async function RootLayout({ children }: { children: React.ReactNode }) { +export default async function GeneralGroupLayout({ children }: { children: React.ReactNode }) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app/(general)/layout.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/app/(general)/layout.tsx (3)
src/components/ui/sidebar.tsx (1)
SidebarProvider(672-672)src/contexts/LoadingContext.tsx (1)
LoadingProvider(12-21)src/components/layouts/GeneralLayout.tsx (1)
GeneralLayout(8-36)
🔇 Additional comments (1)
src/app/(general)/layout.tsx (1)
9-15: Structure correctly addresses duplication issue.The removal of
<AppSidebar />correctly resolves the duplication flagged in the past review comment, asGeneralLayoutalready handles conditional sidebar rendering for both mobile and desktop. The use of<div>instead of<main>is appropriate here since the root layout (src/app/layout.tsx) now provides the semantic<main>landmark, and nested<main>elements would violate HTML semantics.
Overview: This change introduces small but impactful accessibility improvements to the root layout.
Changes
<main>landmark role to semantically mark the primary content area insrc/app/layout.tsx.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.