feat: add enhanced navigation system with active route indicators #102
Conversation
Summary of ChangesHello @Gooichand, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the application's user experience by revamping the navigation system. It introduces four new core application pages (Chat, Documents, Settings, API) and integrates a dynamic Navbar that adapts its links based on the user's current location. Additionally, a new breadcrumb component provides clear pathing, making it easier for users to understand their position within the application and navigate efficiently. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant enhancement to the application's navigation system by adding four new pages (Chat, Documents, Settings, API), a breadcrumb component for better orientation, and an improved navbar with active route indicators. The changes are well-structured. My review includes suggestions to improve code maintainability by creating a reusable page layout component, enhance performance in the new Breadcrumb component, and address some inconsistencies in the navigation links to improve user experience.
| <div className="min-h-screen bg-[#1E1E1E] text-white pt-20 px-6"> | ||
| <div className="max-w-6xl mx-auto"> | ||
| <Breadcrumb /> | ||
| <header className="mb-8"> | ||
| <h1 className="text-3xl font-bold mb-2">API Information</h1> | ||
| <p className="text-gray-400">Manage API keys and view documentation</p> | ||
| </header> | ||
|
|
||
| <div className="space-y-6"> | ||
| <div className="bg-zinc-900/50 rounded-lg p-6 border border-zinc-700"> | ||
| <h3 className="text-xl font-semibold mb-4">API Keys</h3> | ||
| <div className="space-y-4"> | ||
| <div className="flex items-center justify-between p-4 bg-zinc-800 rounded-lg"> | ||
| <div> | ||
| <h4 className="font-medium">Production Key</h4> | ||
| <p className="text-sm text-gray-400 font-mono">lm_****************************</p> | ||
| </div> | ||
| <button className="px-4 py-2 bg-red-600 text-sm rounded-lg hover:bg-red-700"> | ||
| Revoke | ||
| </button> | ||
| </div> | ||
| </div> | ||
| <button className="mt-4 px-4 py-2 bg-blue-600 text-sm rounded-lg hover:bg-blue-700"> | ||
| Generate New Key | ||
| </button> | ||
| </div> | ||
|
|
||
| <div className="bg-zinc-900/50 rounded-lg p-6 border border-zinc-700"> | ||
| <h3 className="text-xl font-semibold mb-4">API Endpoints</h3> | ||
| <div className="space-y-3"> | ||
| <div className="p-3 bg-zinc-800 rounded font-mono text-sm"> | ||
| <span className="text-green-400">POST</span> /api/v1/chat/send-message | ||
| </div> | ||
| <div className="p-3 bg-zinc-800 rounded font-mono text-sm"> | ||
| <span className="text-blue-400">GET</span> /api/v1/upload/files | ||
| </div> | ||
| <div className="p-3 bg-zinc-800 rounded font-mono text-sm"> | ||
| <span className="text-yellow-400">PUT</span> /api/v1/user/ai-config | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
There's a lot of repeated layout code across the new page components (APIPage, ChatPage, DocumentsPage, SettingsPage). Specifically, the div with background color, padding, and max-width container is duplicated. To improve maintainability and reduce code duplication, consider creating a reusable PageLayout component that wraps the content of each page.
For example, you could create a PageLayout component:
// src/shared/component/v1/PageLayout.tsx
import React from 'react';
const PageLayout: React.FC<{ children: React.ReactNode }> = ({ children }) => (
<div className="min-h-screen bg-[#1E1E1E] text-white pt-20 px-6">
<div className="max-w-6xl mx-auto">{children}</div>
</div>
);
export default PageLayout;And then use it in your page components like this:
// APIPage.tsx
import PageLayout from '...';
// ...
const APIPage: React.FC = () => (
<PageLayout>
<Breadcrumb />
{/* ... rest of the page content */}
</PageLayout>
);| const getBreadcrumbs = (): BreadcrumbItem[] => { | ||
| const pathMap: Record<string, string> = { | ||
| '/': 'Home', | ||
| '/chat': 'Chat Interface', | ||
| '/documents': 'Documents & RAG', | ||
| '/settings': 'Model Settings', | ||
| '/api': 'API Information' | ||
| } | ||
|
|
||
| const currentPath = location.pathname | ||
| const breadcrumbs: BreadcrumbItem[] = [] | ||
|
|
||
| if (currentPath !== '/') { | ||
| breadcrumbs.push({ label: 'Home', path: '/' }) | ||
| } | ||
|
|
||
| if (pathMap[currentPath]) { | ||
| breadcrumbs.push({ label: pathMap[currentPath], path: currentPath }) | ||
| } | ||
|
|
||
| return breadcrumbs | ||
| } | ||
|
|
||
| const breadcrumbs = getBreadcrumbs() |
There was a problem hiding this comment.
The getBreadcrumbs function is redefined on every render, which is inefficient. For better performance and cleaner code, you can compute the breadcrumbs array using React.useMemo. This ensures it's only recalculated when the location.pathname changes. Additionally, the pathMap object can be moved outside the component body to be a constant, preventing its redeclaration on each render.
const breadcrumbs = React.useMemo((): BreadcrumbItem[] => {
const pathMap: Record<string, string> = {
'/': 'Home',
'/chat': 'Chat Interface',
'/documents': 'Documents & RAG',
'/settings': 'Model Settings',
'/api': 'API Information'
}
const currentPath = location.pathname
const crumbs: BreadcrumbItem[] = []
if (currentPath !== '/') {
crumbs.push({ label: 'Home', path: '/' })
}
if (pathMap[currentPath]) {
crumbs.push({ label: pathMap[currentPath], path: currentPath })
}
return crumbs
}, [location.pathname])
| {isAppRoute ? ( | ||
| // App Navigation (when user is in main app sections) | ||
| <> | ||
| <NavLink to="/chat" className={navLinkClass}> | ||
| 💬 Chat | ||
| </NavLink> | ||
| <NavLink to="/documents" className={navLinkClass}> | ||
| 📚 Documents | ||
| </NavLink> | ||
| <NavLink to="/settings" className={navLinkClass}> | ||
| ⚙️ Settings | ||
| </NavLink> | ||
| <NavLink to="/api" className={navLinkClass}> | ||
| 🔌 API | ||
| </NavLink> | ||
| </> | ||
| ) : ( | ||
| // Public Navigation (for homepage, etc.) | ||
| <> | ||
| <NavLink to="/" className={publicNavLinkClass}> | ||
| Home | ||
| </NavLink> | ||
| <NavLink to="/chat" className={publicNavLinkClass}> | ||
| Chat | ||
| </NavLink> | ||
| <NavLink to="/documents" className={publicNavLinkClass}> | ||
| Documents | ||
| </NavLink> | ||
| <NavLink to="/settings" className={publicNavLinkClass}> | ||
| Settings | ||
| </NavLink> | ||
| </> | ||
| )} |
There was a problem hiding this comment.
The conditional rendering for navigation links introduces some UX inconsistencies:
- Missing API Link: The public navigation (for the homepage) is missing a link to the
/apipage, which is one of the main application sections. - Missing Home Link: The app navigation is missing a link back to the Home page. When a user navigates from the homepage to an app page, the 'Home' link disappears. While the logo links to home, an explicit navigation link provides better and more consistent usability.
Consider unifying the navigation links or ensuring both sets of links provide a consistent way to access all main pages.
1 participant
🎯 Problem Solved
Fixed UX navigation challenges where users couldn't easily tell which section was active or navigate between Chat, Documents, Settings, and API sections.
✨ Features Added
🧪 Testing
📸 Screenshots
Resolves navigation and routing indicator issues for improved user experience.