refactor: split layout into (app) and (funnels) route groups#1598
Open
sidneyswift wants to merge 1 commit intotestfrom
Open
refactor: split layout into (app) and (funnels) route groups#1598sidneyswift wants to merge 1 commit intotestfrom
sidneyswift wants to merge 1 commit intotestfrom
Conversation
Move all existing SaaS routes into (app)/ route group with its own layout containing Providers, Sidebar, Header, and all shell UI. Root layout now only has the HTML shell (fonts, metadata, analytics). Create (funnels)/ route group with a bare layout for standalone conversion funnels. Add song-drop/ as the first funnel placeholder at /song-drop. URLs are unchanged — route groups are invisible to the browser. Made-with: Cursor
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughThe application's layout architecture is restructured to support separate layout branches. App-specific UI components (sidebar, header, modals, notification containers, providers) are moved from the root layout into a dedicated Changes
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ❌ 1❌ Failed checks (1 warning)
✏️ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/(funnels)/song-drop/page.tsx (1)
3-12: Give the standalone funnel a real content landmark.This page owns the whole flow but exposes it through generic
divs only. Making the outer wrapper<main>gives keyboard and screen-reader users a stable entry point without extra ARIA.As per coding guidelines, "Use semantic HTML elements appropriate to the component's role" and "Start with semantic HTML first, then augment with ARIA if needed".♻️ Suggested tweak
- <div className="flex items-center justify-center min-h-dvh"> + <main className="flex items-center justify-center min-h-dvh"> <div className="text-center space-y-4"> <h1 className="text-4xl font-bold tracking-tight"> Drop your song. </h1> <p className="text-lg text-muted-foreground"> We'll do the rest. </p> </div> - </div> + </main>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/`(funnels)/song-drop/page.tsx around lines 3 - 12, The outer wrapper currently uses a plain div which provides no semantic landmark for the funnel; replace the outermost <div className="flex items-center justify-center min-h-dvh"> with a semantic <main> element (keeping the same className and internal structure) so keyboard and screen-reader users get a proper content landmark—ensure the inner structure (<div className="text-center space-y-4">, <h1>, <p>) remains unchanged and do not add ARIA unless needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/layout.tsx`:
- Around line 70-72: The Providers wrapper (which composes ThemeProvider,
PrivyProvider, QueryClientProvider, FunnelReportProvider, PaymentProvider) was
moved inside the (app) route and so non-(app) branches (e.g., (funnels)) render
without those contexts; move the Providers component up in app/layout.tsx so it
wraps the whole route-group split (wrap children and DeferredAnalytics) and then
remove the nested Providers wrapper from app/(app)/layout.tsx so the provider
tree is initialized once for all routes.
---
Nitpick comments:
In `@app/`(funnels)/song-drop/page.tsx:
- Around line 3-12: The outer wrapper currently uses a plain div which provides
no semantic landmark for the funnel; replace the outermost <div className="flex
items-center justify-center min-h-dvh"> with a semantic <main> element (keeping
the same className and internal structure) so keyboard and screen-reader users
get a proper content landmark—ensure the inner structure (<div
className="text-center space-y-4">, <h1>, <p>) remains unchanged and do not add
ARIA unless needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6d8bd8a4-f0d5-48ed-bbdc-7c840c084d9f
📒 Files selected for processing (38)
app/(app)/access/page.tsxapp/(app)/agents/page.tsxapp/(app)/artists/page.tsxapp/(app)/catalogs/[catalogId]/page.tsxapp/(app)/catalogs/page.tsxapp/(app)/chat/[roomId]/page.tsxapp/(app)/chat/page.tsxapp/(app)/docs/account/constants.tsapp/(app)/docs/account/page.tsxapp/(app)/docs/base/constants.tsapp/(app)/docs/base/page.tsxapp/(app)/docs/fans/constants.tsapp/(app)/docs/fans/page.tsxapp/(app)/docs/posts/constants.tsapp/(app)/docs/posts/page.tsxapp/(app)/docs/segment_report/constants.tsapp/(app)/docs/segment_report/page.tsxapp/(app)/fans/page.tsxapp/(app)/files/page.tsxapp/(app)/keys/page.tsxapp/(app)/layout.tsxapp/(app)/offline/page.tsxapp/(app)/page.tsxapp/(app)/posts/page.tsxapp/(app)/privacy/page.tsxapp/(app)/sandboxes/layout.tsxapp/(app)/sandboxes/page.tsxapp/(app)/segment/[segmentId]/page.tsxapp/(app)/segments/page.tsxapp/(app)/settings/connectors/page.tsxapp/(app)/signin/page.tsxapp/(app)/sms/page.tsxapp/(app)/tasks/[runId]/page.tsxapp/(app)/tasks/page.tsxapp/(app)/terms/page.tsxapp/(funnels)/layout.tsxapp/(funnels)/song-drop/page.tsxapp/layout.tsx
Comment on lines
70
to
72
| <body className={`${geist.variable} antialiased`}> | ||
| <Suspense> | ||
| <Providers> | ||
| <div className="flex flex-col md:flex-row"> | ||
| <Sidebar /> | ||
| <Header /> | ||
| <ArtistSettingModal /> | ||
| <div className="grow flex h-[100dvh] pt-16 md:pt-0 md:h-screen overflow-hidden bg-sidebar"> | ||
| <div className="size-full md:py-4 md:pl-4"> | ||
| <div className="size-full bg-card overflow-y-auto md:rounded-xl flex flex-col md:shadow-md md:border md:border-border"> | ||
| {children} | ||
| </div> | ||
| </div> | ||
| <ArtistsSidebar /> | ||
| </div> | ||
| <MobileDownloadModal /> | ||
| </div> | ||
| <ToastContainer /> | ||
| <Toaster /> | ||
| </Providers> | ||
| </Suspense> | ||
| {children} | ||
| <DeferredAnalytics /> |
Contributor
There was a problem hiding this comment.
Keep shared providers above the route-group split.
Providers now only wraps the (app) branch (app/(app)/layout.tsx:17-35), but that tree owns ThemeProvider, PrivyProvider, QueryClientProvider, FunnelReportProvider, and PaymentProvider (providers/Providers.tsx:1-52). Every non-(app) route — including the new (funnels) branch — now renders without those contexts, which is a bigger behavior change than just removing the SaaS chrome.
♻️ Suggested direction
+import Providers from "@/providers/Providers";
...
- <body className={`${geist.variable} antialiased`}>
- {children}
+ <body className={`${geist.variable} antialiased`}>
+ <Providers>{children}</Providers>
<DeferredAnalytics />
</body>📝 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
| <body className={`${geist.variable} antialiased`}> | |
| <Suspense> | |
| <Providers> | |
| <div className="flex flex-col md:flex-row"> | |
| <Sidebar /> | |
| <Header /> | |
| <ArtistSettingModal /> | |
| <div className="grow flex h-[100dvh] pt-16 md:pt-0 md:h-screen overflow-hidden bg-sidebar"> | |
| <div className="size-full md:py-4 md:pl-4"> | |
| <div className="size-full bg-card overflow-y-auto md:rounded-xl flex flex-col md:shadow-md md:border md:border-border"> | |
| {children} | |
| </div> | |
| </div> | |
| <ArtistsSidebar /> | |
| </div> | |
| <MobileDownloadModal /> | |
| </div> | |
| <ToastContainer /> | |
| <Toaster /> | |
| </Providers> | |
| </Suspense> | |
| {children} | |
| <DeferredAnalytics /> | |
| import Providers from "@/providers/Providers"; | |
| <body className={`${geist.variable} antialiased`}> | |
| <Providers>{children}</Providers> | |
| <DeferredAnalytics /> | |
| </body> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/layout.tsx` around lines 70 - 72, The Providers wrapper (which composes
ThemeProvider, PrivyProvider, QueryClientProvider, FunnelReportProvider,
PaymentProvider) was moved inside the (app) route and so non-(app) branches
(e.g., (funnels)) render without those contexts; move the Providers component up
in app/layout.tsx so it wraps the whole route-group split (wrap children and
DeferredAnalytics) and then remove the nested Providers wrapper from
app/(app)/layout.tsx so the provider tree is initialized once for all routes.
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
layout.tsxinto a thin HTML shell (fonts, metadata, analytics) and moved the SaaS UI (Providers, Sidebar, Header, modals, toasts) into(app)/layout.tsx(app)/route group — URLs are unchanged since route groups are invisible to the browser(funnels)/route group with a bare layout for standalone conversion funnels, completely isolated from the SaaS shellsong-drop/as the first funnel placeholder at/song-dropTest plan
/song-droploads as a bare page with no sidebar/headerpnpm buildpasses (confirmed locally — 46 routes, 0 errors)Made with Cursor
Summary by CodeRabbit
New Features
Refactor