Refactor navigation to horizontal top header and improve loading states#61
Refactor navigation to horizontal top header and improve loading states#61
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request refactors the navigation system from a left sidebar to a horizontal top header layout, reorganizes the application routes using Next.js route groups for better navigation UX, and replaces full-page loading screens with component-level skeletons for improved user experience.
Changes:
- Replaced left sidebar navigation with horizontal top tabs on desktop and hamburger menu on mobile
- Created a (dashboard) route group to consolidate all dashboard pages under a single layout
- Replaced full-page LoadingScreen with TransactionDetailSkeleton for more granular loading states
Reviewed changes
Copilot reviewed 18 out of 21 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/layouts/dashboard/layout.tsx | Complete rewrite to use horizontal AppBar with centered navigation tabs and fixed-width left/right sections |
| src/layouts/components/nav-tabs.tsx | New component providing horizontal navigation tabs for desktop and collapsible menu for mobile |
| src/layouts/components/filter-popover.tsx | New component consolidating time period and token filters into a popover accessible from the header |
| src/layouts/dashboard/main.tsx | Removed Main component, kept only DashboardContent with simplified padding logic |
| src/components/logo/logo.tsx | Simplified Logo component with new 'single' prop replacing 'isLarge' and 'isSingle' props |
| src/components/skeletons/transaction-detail-skeleton.tsx | New skeleton component for transaction detail page loading state |
| src/components/loading-screen/loading-screen.tsx | Removed full-page loading screen component |
| src/app/(dashboard)/layout.tsx | New layout file for the dashboard route group |
| src/app/(dashboard)/page.tsx | Moved from src/app/page.tsx to dashboard route group |
| src/app/(dashboard)/transactions/page.tsx | New transactions page within dashboard route group |
| src/app/(dashboard)/transactions/[tx]/page.tsx | New transaction detail page within dashboard route group |
| src/app/(dashboard)/profile/page.tsx | New profile page within dashboard route group |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 23 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/app/(dashboard)/layout.tsx:10
- The profile page previously had
disableTimelines={true}in its dedicated layout, which would hide the FilterPopover (time/token filters) on the profile page. After this refactor, all pages in the (dashboard) route group share the same layout without this configuration. This means the FilterPopover will now appear on the profile page, which may not be the intended behavior since profile pages typically don't need time period or token filters.
Consider either:
- Adding a mechanism to conditionally hide the FilterPopover on specific routes (e.g., checking pathname in DashboardLayout)
- Moving the profile page outside the (dashboard) route group with its own layout
- Accepting this change if filters on the profile page are acceptable
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| xl: 0, | ||
| }, | ||
| }), | ||
| ...(disablePadding && { p: 0 }), |
There was a problem hiding this comment.
The DashboardContent component has significantly simplified its padding logic. The old implementation had responsive padding that varied by breakpoint and included variables for content padding top/bottom/horizontal. The new implementation only handles the disablePadding case with a simple p: 0.
This means content padding is now controlled entirely by the parent Box in DashboardLayout (lines 121-122 in layout.tsx), which applies py: { xs: 2, md: 3 }, px: { xs: 1.5, sm: 2, lg: 5 }. Verify that this simplified padding approach works correctly across all pages and doesn't break any existing layouts that relied on the previous padding behavior.
| children, | ||
| disablePadding, | ||
| maxWidth = 'lg', | ||
| maxWidth = 'xl', |
There was a problem hiding this comment.
The default maxWidth for DashboardContent has been changed from 'lg' to 'xl'. This will make content containers wider by default (from 1280px to 1536px). While this change is consistent with how it's being used throughout the codebase (all pages explicitly set maxWidth="xl"), this default change could affect any code that relies on DashboardContent without explicitly setting maxWidth. Verify this change is intentional and doesn't negatively impact any layouts.
| maxWidth = 'xl', | |
| maxWidth = 'lg', |
Summary
Changes
Navigation Refactor
Route Structure
Loading States