Improve side panel drawer UX - toggleable drawer, label #911
Conversation
- Make session sidebar click-to-toggle and off-canvas on narrow widths\n- Add drawer top bar with logo + close button\n- Improve header responsiveness to prevent overflow\n- Keep IndependentPanel full-height even with few messages
- Auto-open drawer by default on wide standalone IndependentPanel tabs\n- Keep side panel closed by default via tabs.getCurrent() detection\n- Add ChatGPTBox label in drawer header
WalkthroughThis pull request introduces a collapsible sidebar feature for the IndependentPanel with new toggle controls, updated component props, and comprehensive layout restructuring. The implementation includes responsive styling for mobile views, state management for sidebar toggling, and UI enhancements to the ConversationCard component. Changes
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 |
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||||||||
Summary of ChangesHello @azidancorp, 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 introduces significant user experience enhancements to the side panel drawer, transitioning from an intrusive hover-based activation to a more intuitive click-to-toggle mechanism. These changes provide a more controlled and responsive interface, adapting seamlessly to different screen sizes and panel contexts, ultimately offering a cleaner design and more efficient use of screen real estate. 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
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||||||||||
![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 significantly improves the side panel drawer's user experience by replacing the hover-to-expand behavior with a more intentional click-to-toggle mechanism. The introduction of an off-canvas drawer for narrow screens and auto-opening on wider standalone tabs are excellent responsive design choices. The code is well-structured, and the state management for the sidebar visibility is handled effectively. My review focuses on a few areas to enhance code quality and align with React best practices, such as using style objects over strings and ensuring stable keys for list rendering. I've also pointed out a small opportunity for cleanup in the SCSS. Overall, this is a solid enhancement.
| ☰ | ||
| </button> | ||
| ) : ( | ||
| <img src={logo} style="user-select:none;width:20px;height:20px;" /> |
There was a problem hiding this comment.
In React/JSX, it's a best practice to provide styles as a camelCased object rather than a string. This improves code consistency, type safety, and prevents potential rendering issues.
| <img src={logo} style="user-select:none;width:20px;height:20px;" /> | |
| <img src={logo} style={{ userSelect: 'none', width: '20px', height: '20px' }} /> |
| index, // TODO editable session name | ||
| ) => ( | ||
| <button | ||
| key={index} |
There was a problem hiding this comment.
Using the array index as a key for list items is an anti-pattern in React, especially for lists where items can be added, removed, or reordered. This can lead to unpredictable UI behavior and state management issues. It's recommended to use a unique and stable identifier from your data, such as session.sessionId, to ensure React can correctly track each item.
| key={index} | |
| key={session.sessionId} |
| <button | ||
| key={index} | ||
| className={`normal-button ${sessionId === session.sessionId ? 'active' : ''}`} | ||
| style="display: flex; align-items: center; justify-content: space-between;" |
There was a problem hiding this comment.
It's a best practice in React/JSX to provide styles as a camelCased object instead of a string. This improves consistency, maintainability, and leverages React's synthetic event system more effectively.
| style="display: flex; align-items: center; justify-content: space-between;" | |
| style={{ display: 'flex', alignItems: 'center', justifyContent: 'space-between' }} |
| width: min(280px, 85vw); | ||
| min-width: 0; | ||
| transform: translateX(-105%); | ||
| padding: 10px; |
There was a problem hiding this comment.
This padding: 10px rule for the collapsed sidebar on small screens appears to be unnecessary. When collapsed, the sidebar is translated off-screen and has visibility: hidden, so this padding has no visual effect. On wider screens, the collapsed sidebar correctly has padding: 0. Removing this line would improve consistency and reduce confusion for future maintenance.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/components/ConversationCard/index.jsx (1)
379-393: Consider using a proper icon component instead of Unicode character.The hamburger menu (☰) Unicode character works but may render inconsistently across platforms/fonts. Given that the codebase already uses
@primer/octicons-reactandreact-bootstrap-icons, using a dedicated icon component (e.g.,ThreeBarsfrom Octicons orListfrom react-bootstrap-icons) would provide more consistent rendering.The accessibility attributes (
aria-label,aria-expanded) are correctly implemented.🔎 Optional: Use icon component for consistency
+import { ThreeBarsIcon } from '@primer/octicons-react' ... ) : props.onToggleSidebar ? ( <button type="button" className="gpt-util-icon gpt-menu-toggle" title="Toggle sidebar" aria-label="Toggle sidebar" aria-expanded={Boolean(props.sidebarOpen)} onClick={(e) => { e.preventDefault() e.stopPropagation() props.onToggleSidebar() }} > - ☰ + <ThreeBarsIcon size={16} /> </button>src/pages/IndependentPanel/styles.scss (1)
99-103: Consider adding transition for padding.The collapsed state sets
padding: 0but padding isn't included in the transition property (line 81), which may cause a jarring visual jump.🔎 Optional: Include padding in transition
.chat-sidebar { ... - transition: width 0.3s, min-width 0.3s; + transition: width 0.3s, min-width 0.3s, padding 0.3s; padding: 10px;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/ConversationCard/index.jsx(4 hunks)src/pages/IndependentPanel/App.jsx(5 hunks)src/pages/IndependentPanel/styles.scss(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/components/ConversationCard/index.jsx (2)
src/utils/is-safari.mjs (1)
isSafari(1-3)src/pages/IndependentPanel/App.jsx (1)
logo(20-20)
src/pages/IndependentPanel/App.jsx (3)
src/services/local-session.mjs (4)
Browser(71-71)index(37-37)deleteSession(35-44)deleteSession(35-44)src/background/index.mjs (1)
tab(113-113)src/utils/open-url.mjs (1)
openUrl(3-11)
🔇 Additional comments (13)
src/components/ConversationCard/index.jsx (4)
350-356: LGTM! Proper flex layout for pageMode.The conditional styling correctly applies
flexGrow: 1andminWidth: 0when inpageMode, which prevents overflow issues in constrained sidebar widths. The Safari max-width constraint is preserved.
398-400: LGTM! Flex-based select sizing.The conditional styling ensures the select element shrinks properly in constrained layouts by applying
width: 0,flexGrow: 1, andminWidth: 0when appropriate.
449-450: LGTM! Header action area stabilization.Adding
flexShrink: 0prevents the action buttons from being compressed when space is limited, maintaining UI consistency.
647-648: LGTM! PropTypes properly documented.The new props
onToggleSidebarandsidebarOpenare correctly typed asPropTypes.funcandPropTypes.boolrespectively.src/pages/IndependentPanel/App.jsx (4)
103-117: LGTM! Clean sidebar toggle/close helpers.The functions correctly set
hasUserToggledRef.current = trueto prevent auto-open from overriding user choice. ThecloseSidebarIfOverlayfunction appropriately usesmatchMediafor responsive behavior.
140-142: Backdrop click handler correctly closes sidebar.The backdrop with
role="presentation"and click-to-close behavior is a good UX pattern for modal-like overlays.
222-223: LGTM! Props correctly wired to ConversationCard.The
sidebarOpen={!collapsed}andonToggleSidebar={toggleSidebar}props are correctly passed, enabling the toggle button in ConversationCard's header.
70-84: No issues with Browser.tabs.getCurrent() behavior.The API correctly returns undefined when called from a non-tab context like a side panel. The code's truthiness check properly handles this case. The double-check pattern defensively prevents race conditions after mount. Minor note: the inline comment says "returns null" but the documented behavior is "undefined"—both are falsy, so the code works correctly either way.
src/pages/IndependentPanel/styles.scss (5)
39-58: LGTM! Root container and toggle button styling.The
100vhheight with flex column layout ensures full viewport coverage. The.gpt-menu-togglebutton styling is clean with proper theming via CSS custom properties.
60-73: LGTM! Flex container with proper overflow handling.The combination of
min-height: 0,min-width: 0, andoverflow: hiddencorrectly prevents flex children from overflowing. Theposition: relativeenables absolute positioning for the backdrop.
114-124: Negative margin technique works but is fragile.The
margin: -10px -10px 0on.chat-sidebar-topbarcompensates for parent padding. This works but couples the topbar styling tightly to the parent's padding value. If parent padding changes, this will break.This is acceptable for now but worth noting for future maintenance.
209-245: LGTM! Well-implemented mobile overlay pattern.The responsive styles correctly implement:
- Semi-transparent backdrop with proper z-index layering
- Slide-in animation using
transform: translateX()- Visibility + pointer-events handling for collapsed state
- Delayed visibility transition to allow slide-out animation to complete
The
min(280px, 85vw)width constraint is a good responsive approach.
232-240: Good use of delayed visibility transition.The
transition: transform 0.3s, visibility 0s linear 0.3sensures the sidebar remains visible during the slide-out animation before becoming hidden. This is the correct pattern for animated hide/show.
| <button | ||
| type="button" | ||
| className="gpt-util-icon gpt-menu-toggle" | ||
| title="Toggle sidebar" |
There was a problem hiding this comment.
Hardcoded English string should use i18n.
The title attribute uses a hardcoded string "Toggle sidebar" while the rest of the component uses the t() function for translations.
🔎 Proposed fix
<button
type="button"
className="gpt-util-icon gpt-menu-toggle"
- title="Toggle sidebar"
- aria-label="Toggle sidebar"
+ title={t('Toggle sidebar')}
+ aria-label={t('Toggle sidebar')}
aria-expanded={Boolean(props.sidebarOpen)}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/components/ConversationCard/index.jsx around line 383, the title
attribute is hardcoded as "Toggle sidebar"; replace it with the i18n call used
elsewhere (e.g. title={t('Toggle sidebar')} or, preferably, a semantic key like
title={t('conversation.toggle_sidebar')}) and ensure the t function is in scope
(import/useTranslation or prop) and add the new key/value to the translation
files so the string is localized.
| <button | ||
| type="button" | ||
| className="gpt-util-icon gpt-menu-toggle chat-sidebar-close" | ||
| aria-label="Close sidebar" | ||
| onClick={closeSidebar} | ||
| > | ||
| ✕ | ||
| </button> |
There was a problem hiding this comment.
Same i18n concern: hardcoded "Close sidebar" string.
The close button uses a hardcoded English aria-label. Consider using t() for consistency with the rest of the component.
🔎 Proposed fix
<button
type="button"
className="gpt-util-icon gpt-menu-toggle chat-sidebar-close"
- aria-label="Close sidebar"
+ aria-label={t('Close sidebar')}
onClick={closeSidebar}
>
✕
</button>📝 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.
| <button | |
| type="button" | |
| className="gpt-util-icon gpt-menu-toggle chat-sidebar-close" | |
| aria-label="Close sidebar" | |
| onClick={closeSidebar} | |
| > | |
| ✕ | |
| </button> | |
| <button | |
| type="button" | |
| className="gpt-util-icon gpt-menu-toggle chat-sidebar-close" | |
| aria-label={t('Close sidebar')} | |
| onClick={closeSidebar} | |
| > | |
| ✕ | |
| </button> |
🤖 Prompt for AI Agents
In src/pages/IndependentPanel/App.jsx around lines 150 to 157, the close
button's aria-label is hardcoded as "Close sidebar"; replace this with the i18n
translator (t) so the label is localized (e.g. aria-label={t('closeSidebar')}),
ensure the t function is imported/available in this component (or pulled from
useTranslation hook) and add/update the 'closeSidebar' key in your translation
files as needed; keep the button behavior unchanged.
| {sessions.map( | ||
| ( | ||
| session, | ||
| index, // TODO editable session name | ||
| ) => ( | ||
| <button | ||
| key={index} | ||
| className={`normal-button ${sessionId === session.sessionId ? 'active' : ''}`} | ||
| style="display: flex; align-items: center; justify-content: space-between;" | ||
| onClick={() => { | ||
| setSessionIdSafe(session.sessionId) | ||
| closeSidebarIfOverlay() | ||
| }} | ||
| > | ||
| {session.sessionName} | ||
| <span className="gpt-util-group"> | ||
| <DeleteButton | ||
| size={14} | ||
| text={t('Delete Conversation')} | ||
| onConfirm={() => | ||
| deleteSession(session.sessionId).then((sessions) => { | ||
| setSessions(sessions) | ||
| setSessionIdSafe(sessions[0].sessionId) | ||
| }) | ||
| } | ||
| /> | ||
| </span> | ||
| </button> | ||
| ), | ||
| )} |
There was a problem hiding this comment.
Using array index as React key for session list.
Using index as a key for session buttons can cause reconciliation issues when sessions are reordered or deleted. Since each session has a unique sessionId, use that instead.
🔎 Proposed fix
{sessions.map(
(
session,
- index, // TODO editable session name
+ // TODO editable session name
) => (
<button
- key={index}
+ key={session.sessionId}
className={`normal-button ${sessionId === session.sessionId ? 'active' : ''}`}📝 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.
| {sessions.map( | |
| ( | |
| session, | |
| index, // TODO editable session name | |
| ) => ( | |
| <button | |
| key={index} | |
| className={`normal-button ${sessionId === session.sessionId ? 'active' : ''}`} | |
| style="display: flex; align-items: center; justify-content: space-between;" | |
| onClick={() => { | |
| setSessionIdSafe(session.sessionId) | |
| closeSidebarIfOverlay() | |
| }} | |
| > | |
| {session.sessionName} | |
| <span className="gpt-util-group"> | |
| <DeleteButton | |
| size={14} | |
| text={t('Delete Conversation')} | |
| onConfirm={() => | |
| deleteSession(session.sessionId).then((sessions) => { | |
| setSessions(sessions) | |
| setSessionIdSafe(sessions[0].sessionId) | |
| }) | |
| } | |
| /> | |
| </span> | |
| </button> | |
| ), | |
| )} | |
| {sessions.map( | |
| ( | |
| session, | |
| // TODO editable session name | |
| ) => ( | |
| <button | |
| key={session.sessionId} | |
| className={`normal-button ${sessionId === session.sessionId ? 'active' : ''}`} | |
| style="display: flex; align-items: center; justify-content: space-between;" | |
| onClick={() => { | |
| setSessionIdSafe(session.sessionId) | |
| closeSidebarIfOverlay() | |
| }} | |
| > | |
| {session.sessionName} | |
| <span className="gpt-util-group"> | |
| <DeleteButton | |
| size={14} | |
| text={t('Delete Conversation')} | |
| onConfirm={() => | |
| deleteSession(session.sessionId).then((sessions) => { | |
| setSessions(sessions) | |
| setSessionIdSafe(sessions[0].sessionId) | |
| }) | |
| } | |
| /> | |
| </span> | |
| </button> | |
| ), | |
| )} |
🤖 Prompt for AI Agents
In src/pages/IndependentPanel/App.jsx around lines 169 to 198, the list buttons
use the array index as the React key which can break reconciliation when
sessions are reordered or removed; change the key to use the stable unique
identifier session.sessionId instead (i.e., key={session.sessionId}) so React
can correctly track items during updates.
1 participant
User description
The hover to expand drawer is quite intrusive at small widths (eg the Side Panel), and especially there as the mouse crosses the threshold frequently. Changed it to be manually activated upon click. This also has the advantage of more horizontal space in the sidepanel view too. Added ChatGPTBox label next to logo
stays closed by default.
Conversation Page view:
Sidepanel view:
Sidepanel drawer opened view:
PR Type
Enhancement
Description
Implement click-to-toggle sidebar with off-canvas drawer on narrow screens
Add drawer header with logo, brand label, and close button
Auto-open drawer on wide IndependentPanel tabs, keep side panel closed by default
Improve responsive layout with proper flex constraints and overflow handling
Add sidebar toggle button to conversation header for page mode
Diagram Walkthrough
File Walkthrough
styles.scss
Responsive drawer layout with off-canvas positioningsrc/pages/IndependentPanel/styles.scss
constraints
≤600px width
constraints
App.jsx
Add auto-open logic and sidebar state managementsrc/pages/IndependentPanel/App.jsx
tabs.getCurrent()
responsive behavior
auto-open override
topbar header
ConversationCard component
screens
index.jsx
Add sidebar toggle button and pageMode layout supportsrc/components/ConversationCard/index.jsx
flex-grow behavior
button
provided
stability
Summary by CodeRabbit
New Features
UI/UX Improvements
✏️ Tip: You can customize this high-level summary in your review settings.