Multi-page design-drift audit: sidebar nav, double-header fix, pricing/tools layout#39
Open
projectamazonph wants to merge 2 commits into
Open
Multi-page design-drift audit: sidebar nav, double-header fix, pricing/tools layout#39projectamazonph wants to merge 2 commits into
projectamazonph wants to merge 2 commits into
Conversation
The Field Manual design spec (docs/stitch-prompts.md) calls for a 240px left sidebar + top bar on every authenticated screen. The app had no such layout: pages under (dashboard) rendered under the same public marketing header used on the landing page (always showing "Sign in / Get started" regardless of session state) plus an inconsistent mobile bottom nav that only 3 of 6 pages actually used. - New (dashboard)/layout.tsx wraps every page in that route group with a sidebar (Dashboard/Courses/Tools/Live classes/Certificates/ Payments) and the existing admin TopBar component, mirroring the pattern /admin already uses. - Generalized NavSidebar to accept item lists instead of hardcoding the admin nav, so both areas share one component. - Fixed TopBar's role label, hardcoded to "Admin" — reusing it for students would have mislabeled every non-admin user. - New SiteHeader component makes the public header hide itself on shell routes (/admin, /dashboard, /courses, /tools, /payments, /certificates, /live-classes). Root layout.tsx previously rendered that header unconditionally, so /admin has been showing a redundant "Sign in / Get started" bar above its own sidebar+TopBar (which correctly shows the logged-in user) this whole time — the same bug the new student sidebar would have inherited unfixed. - Middleware was unconditionally redirecting bare /dashboard to /, intended for legacy /dashboard/<feature> bookmarks but written to also swallow /dashboard itself. That made the dashboard page component permanently unreachable. Narrowed the redirect to /dashboard/* sub-paths only. - Removed the now-redundant BottomNav from tools/courses/dashboard and fixed their stale /dashboard/* links to the real routes.
- Pricing tiers rendered as three equal-width columns; spec calls for the featured Accelerated Mastery tier to read as a hero card (larger, more padding) with the other two as smaller, quieter side cards. Fixed via column-width and padding, no markup changes to the checkout flow. - Sign-in/sign-up card heading and subtitle were left-aligned where spec shows them centered inside the card. - Removed two em-dashes in pricing copy (AGENTS.md bans them repo-wide) noticed while already in the file.
📝 WalkthroughWalkthroughThe PR adds route-aware marketing and authenticated dashboard shells, makes sidebar navigation configurable, updates dashboard and tools routes, removes page-level bottom navigation, changes legacy redirect handling, and refines sign-in and pricing presentation. ChangesApplication shell and presentation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant Middleware
participant DashboardLayout
participant NavSidebar
participant TopBar
participant DashboardPage
Browser->>Middleware: request dashboard route
Middleware->>DashboardLayout: continue protected request
DashboardLayout->>DashboardLayout: requireAuth()
DashboardLayout->>NavSidebar: render student navigation
DashboardLayout->>TopBar: render authenticated role
DashboardLayout->>DashboardPage: render routed content
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 4
🤖 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/app/`(dashboard)/tools/tools.module.css:
- Around line 5-7: Replace the unsupported :global(h3) selector in .heroCard
with a local class-based selector, and apply that class to the hero CardTitle
component so it retains the existing font-size styling while passing Stylelint.
In `@src/components/SiteHeader.tsx`:
- Around line 11-15: Add regression tests for SiteHeader covering a public route
that renders the header, an exact shell route such as /admin that returns null,
and a nested shell route such as /dashboard/tools that also returns null. Mock
or provide usePathname for each case and assert the rendered output preserves
the shell-route suppression behavior.
In `@src/middleware.ts`:
- Around line 52-56: Update the comment near the legacy /dashboard bookmark
handling to remove the em dash and replace it with a permitted punctuation mark,
such as a period, comma, or parentheses, without changing the comment’s meaning.
- Around line 57-59: Update the redirect handling in the pathname middleware
branch to clone request.nextUrl, assign the computed remainder to the clone’s
pathname, and redirect using that same-origin URL rather than constructing a URL
from the raw remainder. Preserve the original query parameters, and add
regression coverage for a double-slash path and a redirected URL containing a
query string.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: db26493e-aa82-4a3e-b83c-b77c6695c6f3
📒 Files selected for processing (14)
src/app/(dashboard)/courses/page.tsxsrc/app/(dashboard)/dashboard/page.tsxsrc/app/(dashboard)/layout.module.csssrc/app/(dashboard)/layout.tsxsrc/app/(dashboard)/tools/page.tsxsrc/app/(dashboard)/tools/tools.module.csssrc/app/(public)/auth/signin/auth.module.csssrc/app/(public)/pricing/page.tsxsrc/app/(public)/pricing/pricing.module.csssrc/app/layout.tsxsrc/components/SiteHeader.tsxsrc/components/ui/NavSidebar.tsxsrc/components/ui/TopBar.tsxsrc/middleware.ts
💤 Files with no reviewable changes (1)
- src/app/(dashboard)/courses/page.tsx
Comment on lines
+5
to
+7
| .heroCard :global(h3) { | ||
| font-size: var(--text-2xl); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Replace the unsupported :global selector.
Stylelint reports Line 5 as selector-pseudo-class-no-unknown, so this stylesheet fails linting. Apply a local class to the hero CardTitle instead.
Proposed fix
-.heroCard :global(h3) {
+.heroTitle {
font-size: var(--text-2xl);
}-<CardTitle>{heroTool.name}</CardTitle>
+<CardTitle className={styles.heroTitle}>{heroTool.name}</CardTitle>🧰 Tools
🪛 Stylelint (17.14.0)
[error] 5-5: Unknown pseudo-class selector ":global" (selector-pseudo-class-no-unknown)
(selector-pseudo-class-no-unknown)
🤖 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/app/`(dashboard)/tools/tools.module.css around lines 5 - 7, Replace the
unsupported :global(h3) selector in .heroCard with a local class-based selector,
and apply that class to the hero CardTitle component so it retains the existing
font-size styling while passing Stylelint.
Source: Linters/SAST tools
Comment on lines
+11
to
+15
| export function SiteHeader() { | ||
| const pathname = usePathname(); | ||
| if (SHELL_PREFIXES.some((prefix) => pathname === prefix || pathname.startsWith(`${prefix}/`))) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add regression tests for shell-route header suppression.
Cover a public route, an exact shell route such as /admin, and a nested shell route such as /dashboard/tools. This behavior fixes the double-header regression and should stay protected.
As per coding guidelines, “New features must include tests; admin and business-layer features require tests.”
🤖 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/SiteHeader.tsx` around lines 11 - 15, Add regression tests for
SiteHeader covering a public route that renders the header, an exact shell route
such as /admin that returns null, and a nested shell route such as
/dashboard/tools that also returns null. Mock or provide usePathname for each
case and assert the rendered output preserves the shell-route suppression
behavior.
Source: Coding guidelines
Comment on lines
+52
to
+56
| // Legacy /dashboard/<feature> bookmarks (real URLs for those features live | ||
| // at /courses, /payments, /tools, /live-classes, /certificates — the | ||
| // (dashboard) route group does not contribute to the URL). Strip the | ||
| // prefix and continue. Bare /dashboard is its own real page (the student | ||
| // home) and falls through to the auth check below instead of redirecting. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the em dash from this comment.
Line 53 violates the repository text policy. Use a period, comma, or parentheses instead.
As per coding guidelines, “Do not use emojis in code or commit messages, and do not use em-dashes. Use periods, commas, or parentheses instead.”
🤖 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/middleware.ts` around lines 52 - 56, Update the comment near the legacy
/dashboard bookmark handling to remove the em dash and replace it with a
permitted punctuation mark, such as a period, comma, or parentheses, without
changing the comment’s meaning.
Source: Coding guidelines
Comment on lines
+57
to
+59
| if (pathname.startsWith('/dashboard/')) { | ||
| const remainder = pathname.slice('/dashboard'.length); | ||
| return NextResponse.redirect(new URL(remainder, request.url)); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Keep legacy redirects on the current origin.
Line 59 treats remainder as a URL reference. /dashboard//attacker.example produces //attacker.example, which redirects off-site. This also drops the original query string. Clone request.nextUrl and assign its pathname instead.
Proposed fix
if (pathname.startsWith('/dashboard/')) {
const remainder = pathname.slice('/dashboard'.length);
- return NextResponse.redirect(new URL(remainder, request.url));
+ const redirectUrl = request.nextUrl.clone();
+ redirectUrl.pathname = remainder;
+ return NextResponse.redirect(redirectUrl);
}Add regression coverage for a double-slash path and a redirected URL with query parameters.
📝 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.
Suggested change
| if (pathname.startsWith('/dashboard/')) { | |
| const remainder = pathname.slice('/dashboard'.length); | |
| return NextResponse.redirect(new URL(remainder, request.url)); | |
| if (pathname.startsWith('/dashboard/')) { | |
| const remainder = pathname.slice('/dashboard'.length); | |
| const redirectUrl = request.nextUrl.clone(); | |
| redirectUrl.pathname = remainder; | |
| return NextResponse.redirect(redirectUrl); |
🤖 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/middleware.ts` around lines 57 - 59, Update the redirect handling in the
pathname middleware branch to clone request.nextUrl, assign the computed
remainder to the clone’s pathname, and redirect using that same-origin URL
rather than constructing a URL from the raw remainder. Preserve the original
query parameters, and add regression coverage for a double-slash path and a
redirected URL containing a query string.
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
2 participants
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
Follow-up to #38 (landing page design-drift fix). Audited the rest of the app — pricing, sign-in/sign-up, tools index, and the authenticated area generally — against the Field Manual spec in
docs/stitch-prompts.md, and found two tiers of drift.Structural (the bigger finding):
/adminalready has, just never extended to the student side./adminhas been rendering the public header above its own sidebar+TopBar this whole time, so a logged-in admin sees a redundant "Sign in / Get started" bar right next to their own name and a working "Sign out" button. Root cause: the root layout always rendered that header unconditionally, with nothing to hide it for shell routes./dashboardwas dead code: middleware redirected it (and only it, unconditionally) to/, apparently as over-broad legacy-redirect logic that was meant to catch/dashboard/<feature>bookmarks but also swallowed the bare path, making the actual dashboard page permanently unreachable.Fixes:
(dashboard)/layout.tsxgives every page in that route group a sidebar (Dashboard/Courses/Tools/Live classes/Certificates/Payments) + top bar, reusing and generalizing the adminNavSidebar/TopBarcomponents instead of duplicating them.SiteHeadercomponent hides the public header on shell routes (/adminand the (dashboard) group), fixing the double-header bug on both areas./dashboard/<feature>sub-paths only, restoring the real/dashboardpage.BottomNavfrom the pages that had it, fixed stale/dashboard/*links to their real routes.AGENTS.mdbans them repo-wide).Test plan
/dashboard,/courses,/tools,/admin— no double header anywhere, sidebar nav renders with correct active states on all authenticated pages,TopBarshows the real user role (not hardcoded "Admin")/dashboardcorrectly redirects to/auth/signin?redirect=%2FdashboardafterwardScope notes
/courses/[slug], lesson pages, quiz pages, payments, certificates, or live-classes content/layout in detail — only confirmed they now sit correctly inside the new sidebar shell. Those may have their own drift against the spec's screens 6-8, 13-17; flagging as a possible follow-up, not covered here./dashboardfix — noticed its matcher (/admin/:path*,/dashboard/:path*) doesn't cover/courses,/tools,/payments,/certificates, or/live-classes, so those routes rely solely on page-levelrequireAuth()with no edge-level defense-in-depth. That's a security-relevant change, not a design one, so left it as-is rather than bundling it in here.Co-Authored-By: Claude Sonnet 5
Claude-Session: https://claude.ai/code/session_01Q2obMCR2P3RHTZMqph1QL1
Generated by Claude Code
Summary by CodeRabbit
New Features
Improvements
Bug Fixes