feat: hide sidebar on login page#413
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary of ChangesHello, 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 enhances the user experience by preventing the sidebar from being displayed on the login page. By introducing specific logic to detect the authentication route, the layout is streamlined, providing a cleaner and more focused interface for users during the login process. 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. Changelog
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
|
📝 WalkthroughWalkthroughThe layout shell now derives Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can enable review details to help with troubleshooting, context usage and more.Enable the |
![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 logic to hide the sidebar on the login page. The implementation is straightforward and correct. However, it could be improved to also handle other authentication-related pages, such as /register and /forgot-password, to ensure a consistent user experience across all authentication flows. I have included a specific suggestion to make this logic more comprehensive and maintainable.
| const [collapsed, setCollapsed] = useState(true) | ||
| const pathname = usePathname() | ||
| const isAgentBoardFocusPage = pathname.startsWith("/studio/agent-board/") | ||
| const isAuthPage = pathname === "/login" |
There was a problem hiding this comment.
The current implementation only hides the sidebar on the /login page. It's likely that other authentication-related pages like /register and /forgot-password (which are linked from the login page) should also not display the sidebar. To cover these cases and improve maintainability, you could check if the current path is present in a list of authentication-related paths.
| const isAuthPage = pathname === "/login" | |
| const isAuthPage = ["/login", "/register", "/forgot-password"].includes(pathname) |
There was a problem hiding this comment.
Pull request overview
Updates the main layout shell to suppress the left sidebar on the login route, keeping the login experience focused and uncluttered while preserving existing behavior for the agent board focus view.
Changes:
- Adds a
hideSidebarflag based on route detection (/studio/agent-board/*and/login). - Uses
hideSidebarto conditionally render the<aside>and to remove the main content left padding when the sidebar is hidden.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const isAuthPage = pathname === "/login" | ||
| const hideSidebar = isAgentBoardFocusPage || isAuthPage |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/components/layout/LayoutShell.tsx (1)
12-30:⚠️ Potential issue | 🟠 MajorPlease add/update tests for the new route-based sidebar behavior.
This PR changes visible layout behavior, but no corresponding tests were included. Please add coverage for at least: auth route hides sidebar, non-auth route shows sidebar, and agent-board route still hides sidebar.
As per coding guidelines, "{src,web/src}/**/*.{py,ts,tsx}: If behavior changes, add or update tests".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/layout/LayoutShell.tsx` around lines 12 - 30, Add unit tests covering the route-based sidebar visibility introduced in LayoutShell.tsx: create tests that render LayoutShell with different pathname contexts (use MemoryRouter/next-router-mock or app router test helpers) and assert the Sidebar component presence/absence based on isAuthPage and isAgentBoardFocusPage. Specifically add three tests: when pathname = "/login" assert Sidebar is not rendered, when pathname = "/" (or another non-auth route) assert Sidebar is rendered, and when simulating the agent-board focus route (the condition that makes isAgentBoardFocusPage true) assert Sidebar is not rendered; use react-testing-library queries to find the Sidebar (by component role, text, or test-id) and ensure collapsed state tests remain unaffected. Ensure tests live alongside other LayoutShell tests (or create web/src/components/layout/LayoutShell.test.tsx) and mock any router or context hooks used by LayoutShell.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/src/components/layout/LayoutShell.tsx`:
- Around line 12-13: isAuthPage currently only checks pathname === "/login",
causing other auth routes to still show sidebar/padding; update the isAuthPage
computation (used by hideSidebar) to detect all auth routes (e.g., "/login",
"/register", "/forgot-password", "/reset-password") by matching pathname against
a set/array or a prefix/regex so any of those paths return true and hideSidebar
behaves consistently across auth pages.
---
Outside diff comments:
In `@web/src/components/layout/LayoutShell.tsx`:
- Around line 12-30: Add unit tests covering the route-based sidebar visibility
introduced in LayoutShell.tsx: create tests that render LayoutShell with
different pathname contexts (use MemoryRouter/next-router-mock or app router
test helpers) and assert the Sidebar component presence/absence based on
isAuthPage and isAgentBoardFocusPage. Specifically add three tests: when
pathname = "/login" assert Sidebar is not rendered, when pathname = "/" (or
another non-auth route) assert Sidebar is rendered, and when simulating the
agent-board focus route (the condition that makes isAgentBoardFocusPage true)
assert Sidebar is not rendered; use react-testing-library queries to find the
Sidebar (by component role, text, or test-id) and ensure collapsed state tests
remain unaffected. Ensure tests live alongside other LayoutShell tests (or
create web/src/components/layout/LayoutShell.test.tsx) and mock any router or
context hooks used by LayoutShell.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4be5ce9d-5d7b-43d5-bcbb-a728dc43d66f
📒 Files selected for processing (1)
web/src/components/layout/LayoutShell.tsx
| const isAuthPage = pathname === "/login" | ||
| const hideSidebar = isAgentBoardFocusPage || isAuthPage |
There was a problem hiding this comment.
Expand auth-route detection beyond /login.
isAuthPage only matches /login, so sidebar/padding still appear on other auth pages (e.g. /register, /forgot-password, /reset-password), creating inconsistent auth UX.
Suggested fix
- const isAuthPage = pathname === "/login"
+ const AUTH_ROUTES = ["/login", "/register", "/forgot-password", "/reset-password"] as const
+ const isAuthPage = AUTH_ROUTES.includes(pathname as (typeof AUTH_ROUTES)[number])
const hideSidebar = isAgentBoardFocusPage || isAuthPageAs per coding guidelines, "web/src/**/*.{ts,tsx}: Follow established patterns in web/src/ for TypeScript/React code".
📝 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.
| const isAuthPage = pathname === "/login" | |
| const hideSidebar = isAgentBoardFocusPage || isAuthPage | |
| const AUTH_ROUTES = ["/login", "/register", "/forgot-password", "/reset-password"] as const | |
| const isAuthPage = AUTH_ROUTES.includes(pathname as (typeof AUTH_ROUTES)[number]) | |
| const hideSidebar = isAgentBoardFocusPage || isAuthPage |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/layout/LayoutShell.tsx` around lines 12 - 13, isAuthPage
currently only checks pathname === "/login", causing other auth routes to still
show sidebar/padding; update the isAuthPage computation (used by hideSidebar) to
detect all auth routes (e.g., "/login", "/register", "/forgot-password",
"/reset-password") by matching pathname against a set/array or a prefix/regex so
any of those paths return true and hideSidebar behaves consistently across auth
pages.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/components/layout/LayoutShell.tsx (1)
12-30:⚠️ Potential issue | 🟠 MajorAdd/Update tests for the new route-based layout behavior.
This PR changes visible behavior (sidebar + padding by pathname), but no test updates are included in the provided changes. Please add coverage for auth routes and at least one non-auth route to prevent regressions.
As per coding guidelines, "{src,web/src}/**/*.{py,ts,tsx}: If behavior changes, add or update tests".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/layout/LayoutShell.tsx` around lines 12 - 30, The LayoutShell change introduces route-based behavior via isAuthPage and hideSidebar (driven by pathname) that affects sidebar visibility and main padding but no tests were added; add/update tests for LayoutShell to cover at least: (1) an auth route (e.g., "/login" or "/register") asserting the sidebar is hidden and main does not have md:pl-14/md:pl-56 padding, and (2) a non-auth route asserting the sidebar is rendered and main receives the correct padding when collapsed and when expanded. Locate tests referencing LayoutShell or Sidebar and add component tests (React Testing Library/Jest) that render LayoutShell with mocked pathname values and check for presence/absence of the <aside> (Sidebar) and the computed class names on <main> to validate both behaviors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@web/src/components/layout/LayoutShell.tsx`:
- Around line 12-30: The LayoutShell change introduces route-based behavior via
isAuthPage and hideSidebar (driven by pathname) that affects sidebar visibility
and main padding but no tests were added; add/update tests for LayoutShell to
cover at least: (1) an auth route (e.g., "/login" or "/register") asserting the
sidebar is hidden and main does not have md:pl-14/md:pl-56 padding, and (2) a
non-auth route asserting the sidebar is rendered and main receives the correct
padding when collapsed and when expanded. Locate tests referencing LayoutShell
or Sidebar and add component tests (React Testing Library/Jest) that render
LayoutShell with mocked pathname values and check for presence/absence of the
<aside> (Sidebar) and the computed class names on <main> to validate both
behaviors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f33325a8-c853-41e3-829e-a2f11c310ba5
📒 Files selected for processing (1)
web/src/components/layout/LayoutShell.tsx
Summary by CodeRabbit