Sidebar layout #745
Sidebar layout #745
Conversation
📝 WalkthroughWalkthroughAdds a sidebar system and integrates it across the UI. New module widgets/sidebar.py introduces SidebarRef, use_sidebar, sidebar rendering helpers, a mobile header, and layout primitives. Settings gain GOOEY_LOGO_FACE, SIDEBAR_LINKS, SIDEBAR_ICON_SIZE and HEADER_ICONS updated. routers/root.page_wrapper signature changed to accept a page and now uses a sidebar-first two-pane layout; explore/account pages and BasePage call sidebar_mobile_header and BasePage exposes render_sidebar. workspaces/widgets.global_workspace_selector gained hide_name and layout tweaks. New icons added in daras_ai_v2/icons.py. Removed an html scrollbar CSS rule and adjusted google one-tap template markup. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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 |
anish-work
force-pushed
the
sidebar_layout
branch
from
b344a91 to
b3e3ebe
Compare
![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
🧹 Nitpick comments (1)
widgets/sidebar.py (1)
21-38: Good implementation with room for layout consistency improvement.The conditional rendering based on sidebar state works well, and the icon/non-icon handling is functional. However, based on the retrieved learning about maintaining consistent alignment, consider improving the layout structure.
The current approach uses different div structures for icon vs non-icon cases. Consider using a more consistent two-column approach like
gui.columns([2, 10])to maintain better visual alignment:- with gui.tag("a", href=url, className="text-decoration-none d-flex"): - if icon: - with gui.div( - className="d-inline-block me-3", - style={"height": "24px"}, - ): - sidebar_list_item(icon, label) - else: - with gui.div( - className="d-inline-block me-3 small", - style={"width": "24px"}, - ): - gui.html(" ") - gui.html(label) + with gui.tag("a", href=url, className="text-decoration-none"): + col1, col2 = gui.columns([2, 10]) + with col1: + if icon: + gui.html(icon, className="me-2") + # else: leave empty for consistent alignment + with col2: + gui.html(label)This approach prioritizes consistent alignment over space optimization, as indicated in the retrieved learning.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
daras_ai_v2/base.py(4 hunks)daras_ai_v2/settings.py(2 hunks)routers/root.py(3 hunks)widgets/sidebar.py(1 hunks)workspaces/widgets.py(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: nikochiko
PR: GooeyAI/gooey-server#703
File: workspaces/widgets.py:161-166
Timestamp: 2025-06-16T11:43:04.972Z
Learning: In workspaces/widgets.py header links rendering, the gui.columns([2, 10]) layout should be maintained consistently even when no icon exists, leaving empty space in the first column for visual alignment purposes. This prioritizes consistent alignment over space optimization.
Learnt from: nikochiko
PR: GooeyAI/gooey-server#703
File: daras_ai_v2/settings.py:301-308
Timestamp: 2025-06-16T11:42:40.993Z
Learning: In daras_ai_v2/settings.py, when using a localhost frontend with api.gooey.ai backend deployment, hard-coded "/explore/" paths work correctly as relative URLs, but EXPLORE_URL computed from APP_BASE_URL may point to the wrong domain (localhost instead of api.gooey.ai).
daras_ai_v2/settings.py (1)
Learnt from: nikochiko
PR: #703
File: daras_ai_v2/settings.py:301-308
Timestamp: 2025-06-16T11:42:40.993Z
Learning: In daras_ai_v2/settings.py, when using a localhost frontend with api.gooey.ai backend deployment, hard-coded "/explore/" paths work correctly as relative URLs, but EXPLORE_URL computed from APP_BASE_URL may point to the wrong domain (localhost instead of api.gooey.ai).
daras_ai_v2/base.py (1)
Learnt from: nikochiko
PR: #703
File: workspaces/widgets.py:161-166
Timestamp: 2025-06-16T11:43:04.972Z
Learning: In workspaces/widgets.py header links rendering, the gui.columns([2, 10]) layout should be maintained consistently even when no icon exists, leaving empty space in the first column for visual alignment purposes. This prioritizes consistent alignment over space optimization.
widgets/sidebar.py (1)
Learnt from: nikochiko
PR: #703
File: workspaces/widgets.py:161-166
Timestamp: 2025-06-16T11:43:04.972Z
Learning: In workspaces/widgets.py header links rendering, the gui.columns([2, 10]) layout should be maintained consistently even when no icon exists, leaving empty space in the first column for visual alignment purposes. This prioritizes consistent alignment over space optimization.
🧬 Code Graph Analysis (2)
workspaces/widgets.py (3)
app_users/models.py (1)
AppUser(90-262)workspaces/admin.py (1)
display_name(146-147)workspaces/models.py (1)
display_name(392-402)
daras_ai_v2/base.py (1)
widgets/sidebar.py (1)
render_base_sidebar(41-52)
🔇 Additional comments (18)
daras_ai_v2/settings.py (2)
260-260: LGTM! Logo constant follows existing pattern.The new
GOOEY_LOGO_FACEconstant follows the established naming convention and pattern of other logo constants in the file.
311-318: LGTM! Sidebar links structure is well-designed.The
SIDEBAR_LINKSconstant appropriately mirrors the existingHEADER_LINKSstructure while adding the necessary icon HTML for sidebar rendering. The use of FontAwesome icons is consistent, and the paths correctly leverage existing URL constants for external links.workspaces/widgets.py (2)
20-20: LGTM! Optional parameter maintains backward compatibility.The addition of the
hide_nameparameter with a default value ofFalsemaintains backward compatibility while enabling the new sidebar functionality.
58-62: LGTM! Conditional rendering logic is correct.The conditional logic properly hides both the display name and dropdown icon when
hide_nameisTrue, while maintaining the existing HTML structure. This implementation aligns well with the sidebar layout requirements.daras_ai_v2/base.py (3)
94-94: LGTM: Import correctly added for sidebar functionality.The import statement follows the existing import structure and is necessary for the new sidebar integration.
197-198: LGTM: Sidebar method correctly implemented.The method properly delegates to
render_base_sidebarwith the sidebar state. The unusedrequestparameter suggests good design for future extensibility where subclasses might implement page-specific sidebars.
442-442: LGTM: Header alignment improved for sidebar integration.Changing from
align-items-starttoalign-items-centerprovides better visual balance with the new sidebar toggle button and logo elements.widgets/sidebar.py (2)
5-18: LGTM: Sidebar item styling is well-implemented.The function provides consistent styling for sidebar items with appropriate icon sizing constraints and spacing. The CSS approach ensures icons maintain uniform appearance regardless of their actual dimensions.
41-52: LGTM: Solid sidebar container implementation.The function provides a well-structured sidebar container with appropriate flex layout, consistent spacing, and fixed minimum width for stable appearance. The separation of the core "Saved" link from the configurable sidebar items is a good design choice.
routers/root.py (9)
49-49: LGTM: Import addition for sidebar functionality.The import of
render_base_sidebarfromwidgets.sidebaris correctly added and aligns with the new sidebar functionality being implemented.
690-690: LGTM: Function call updated to pass page instance.The
render_recipe_pagefunction correctly passes thepageinstance topage_wrapper, enabling page-specific sidebar rendering.
707-707: LGTM: Function signature enhanced for page-specific sidebars.The addition of the optional
pageparameter allows the wrapper to conditionally render page-specific sidebars while maintaining backward compatibility.
712-715: LGTM: Sidebar layout initialization.The sidebar layout setup with toggle functionality is well-implemented. The session state key
"main-sidebar-layout"is appropriately scoped and the default open state (True) provides good UX.
717-745: Review the sidebar header implementation.The sidebar header implementation has several aspects to consider:
- Logo sizing: The logo is correctly sized at 44px x 44px for the sidebar
- Conditional workspace selector: The logic appropriately shows workspace selector only when sidebar is open and user is authenticated
- Toggle button: Uses Font Awesome icon with appropriate styling
747-750: LGTM: Conditional sidebar content rendering.The logic correctly renders either a page-specific sidebar (when
pageis provided) or the base sidebar. This provides the flexibility needed for different page types while maintaining consistency.
752-768: LGTM: Bottom workspace selector for collapsed sidebar.The implementation correctly handles the collapsed sidebar state by:
- Positioning the workspace selector at the bottom with absolute positioning
- Using appropriate z-index (1000) to ensure visibility
- Calling
global_workspace_selectorwithhide_name=Trueparameter for compact display- Maintaining the same authentication logic as the main sidebar
769-780: LGTM: Main content pane structure.The main content pane properly:
- Uses the pane container from the sidebar layout
- Maintains the full viewport height with
min-vh-100- Includes all necessary scripts (Google Analytics, clipboard, footer, login)
- Preserves the original content structure with appropriate padding
- Yields the current workspace for downstream usage
763-763: Verifiedhide_namesupport inglobal_workspace_selector. The function in workspaces/widgets.py (line 20) declareshide_name: bool = False, so passinghide_name=Truein routers/root.py (lines 733, 762) is valid. No further action required.
anish-work
force-pushed
the
sidebar_layout
branch
from
b3e3ebe to
3d032f6
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
widgets/sidebar.py (1)
127-199: Fix typo and remove debug print statement.The sidebar layout implementation is comprehensive with good responsive design. However:
- Fix the typo in variable name
sidebar_funtion_classes→sidebar_function_classes- Remove the debug print statement at line 183
- sidebar_funtion_classes = ( + sidebar_function_classes = ( "gooey-sidebar-open" if sidebar_ref.is_open else "gooey-sidebar-closed" )- print(side_bar_styles, ">>>") with (sidebar_content_placeholder = gui.div( - className=f"d-flex flex-column flex-grow-1 gooey-sidebar {sidebar_funtion_classes}", + className=f"d-flex flex-column flex-grow-1 gooey-sidebar {sidebar_function_classes}", style={"height": "100dvh"}, )
🧹 Nitpick comments (1)
widgets/sidebar.py (1)
65-83: Sidebar item list implementation aligns with established patterns.The function correctly handles icon/non-icon cases and maintains visual alignment. The approach of preserving spacing for non-icon items is consistent with the codebase patterns (as seen in workspaces/widgets.py).
Consider extracting the hard-coded style values (e.g., "24px") to constants for better maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
daras_ai_v2/base.py(4 hunks)daras_ai_v2/icons.py(1 hunks)daras_ai_v2/settings.py(2 hunks)routers/root.py(3 hunks)widgets/explore.py(2 hunks)widgets/sidebar.py(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- daras_ai_v2/settings.py
- daras_ai_v2/icons.py
🚧 Files skipped from review as they are similar to previous changes (2)
- routers/root.py
- daras_ai_v2/base.py
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: nikochiko
PR: GooeyAI/gooey-server#703
File: workspaces/widgets.py:161-166
Timestamp: 2025-06-16T11:43:04.972Z
Learning: In workspaces/widgets.py header links rendering, the gui.columns([2, 10]) layout should be maintained consistently even when no icon exists, leaving empty space in the first column for visual alignment purposes. This prioritizes consistent alignment over space optimization.
Learnt from: nikochiko
PR: GooeyAI/gooey-server#703
File: daras_ai_v2/settings.py:301-308
Timestamp: 2025-06-16T11:42:40.993Z
Learning: In daras_ai_v2/settings.py, when using a localhost frontend with api.gooey.ai backend deployment, hard-coded "/explore/" paths work correctly as relative URLs, but EXPLORE_URL computed from APP_BASE_URL may point to the wrong domain (localhost instead of api.gooey.ai).
🧬 Code Graph Analysis (1)
widgets/explore.py (1)
widgets/sidebar.py (1)
sidebar_logo_header(103-124)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (python)
- GitHub Check: test (3.10.12, 1.8.3)
🔇 Additional comments (4)
widgets/explore.py (1)
16-16: Import looks good.The import follows the existing pattern and correctly brings in the required sidebar header functionality.
widgets/sidebar.py (3)
7-35: Well-structured state management class.The
SidebarRefclass provides a clean abstraction for sidebar state management with appropriate methods and properties for handling both desktop and mobile states.
37-46: Clean factory function for sidebar references.The function properly manages state persistence and includes type safety through boolean conversion.
48-63: Appropriate sidebar item rendering.The function correctly handles conditional title display based on sidebar state and applies consistent icon styling.
widgets/sidebar.py
Outdated
| def render_default_sidebar(): | ||
| is_sidebar_open = gui.session_state.get("main-sidebar", True) | ||
| with gui.div( | ||
| className="d-flex flex-column flex-grow-1 gap-3 px-3 my-3 text-nowrap", | ||
| style={"marginLeft": "4px"}, | ||
| ): | ||
| with gui.tag( | ||
| "a", | ||
| href="/saved/", | ||
| className="pe-2 text-decoration-none d-flex", | ||
| ): | ||
| sidebar_list_item( | ||
| "<i class='fa-regular fa-floppy-disk'></i>", "Saved", is_sidebar_open | ||
| ) | ||
|
|
||
| sidebar_item_list(is_sidebar_open) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making the saved URL configurable.
The implementation is clean, but the hard-coded "/saved/" URL should be moved to settings for consistency with other URL configurations in the codebase.
with gui.tag(
"a",
- href="/saved/",
+ href=settings.SAVED_URL if hasattr(settings, 'SAVED_URL') else "/saved/",
className="pe-2 text-decoration-none d-flex",
):Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In widgets/sidebar.py around lines 85 to 101, the URL "/saved/" is hard-coded in
the render_default_sidebar function. To fix this, move the "/saved/" URL to a
configuration setting or constant in the settings module or a central config
file. Then, import and use that setting here instead of the hard-coded string to
maintain consistency with other URL configurations in the codebase.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
widgets/sidebar.py (2)
85-101: Hard-coded URL should be moved to settings for consistency.The "/saved/" URL is hard-coded, which is inconsistent with other URL configurations in the codebase.
Apply this diff to use a settings-based URL:
with gui.tag( "a", - href="/saved/", + href=getattr(settings, 'SAVED_URL', "/saved/"), className="pe-2 text-decoration-none d-flex", ):
103-125: Remove debug print statement before merging.The debug print statement at line 122 should be removed from production code.
Apply this diff to remove the debug statement:
if open_mobile_sidebar: - print(sidebar_ref.set_mobile_open, ">>>") sidebar_ref.set_mobile_open(True) raise gui.RerunException()
🧹 Nitpick comments (1)
widgets/sidebar.py (1)
65-83: Consider maintaining consistent column layout for visual alignment.The current implementation switches between different div structures when icons are present vs absent. Based on retrieved learnings, consistent layout should be maintained even when no icon exists.
Apply this diff to maintain consistent alignment:
def sidebar_item_list(is_sidebar_open): for i, (url, label, icon) in enumerate(settings.SIDEBAR_LINKS): if not is_sidebar_open and i >= 1: break with gui.tag("a", href=url, className="text-decoration-none d-flex"): - if icon: - with gui.div( - className="d-inline-block me-3", - style={"height": "24px"}, - ): - sidebar_list_item(icon, label, is_sidebar_open) - else: - with gui.div( - className="d-inline-block me-3 small", - style={"width": "24px"}, - ): - gui.html(" ") - gui.html(label) + with gui.div( + className="d-inline-block me-3", + style={"height": "24px"}, + ): + if icon: + sidebar_list_item(icon, label, is_sidebar_open) + else: + gui.html(" ") + if is_sidebar_open: + gui.html(label)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
routers/root.py(3 hunks)widgets/sidebar.py(1 hunks)workspaces/widgets.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- workspaces/widgets.py
- routers/root.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: nikochiko
PR: GooeyAI/gooey-server#703
File: workspaces/widgets.py:161-166
Timestamp: 2025-06-16T11:43:04.972Z
Learning: In workspaces/widgets.py header links rendering, the gui.columns([2, 10]) layout should be maintained consistently even when no icon exists, leaving empty space in the first column for visual alignment purposes. This prioritizes consistent alignment over space optimization.
Learnt from: nikochiko
PR: GooeyAI/gooey-server#703
File: daras_ai_v2/settings.py:301-308
Timestamp: 2025-06-16T11:42:40.993Z
Learning: In daras_ai_v2/settings.py, when using a localhost frontend with api.gooey.ai backend deployment, hard-coded "/explore/" paths work correctly as relative URLs, but EXPLORE_URL computed from APP_BASE_URL may point to the wrong domain (localhost instead of api.gooey.ai).
widgets/sidebar.py (2)
Learnt from: nikochiko
PR: #703
File: daras_ai_v2/settings.py:301-308
Timestamp: 2025-06-16T11:42:40.993Z
Learning: In daras_ai_v2/settings.py, when using a localhost frontend with api.gooey.ai backend deployment, hard-coded "/explore/" paths work correctly as relative URLs, but EXPLORE_URL computed from APP_BASE_URL may point to the wrong domain (localhost instead of api.gooey.ai).
Learnt from: nikochiko
PR: #703
File: workspaces/widgets.py:161-166
Timestamp: 2025-06-16T11:43:04.972Z
Learning: In workspaces/widgets.py header links rendering, the gui.columns([2, 10]) layout should be maintained consistently even when no icon exists, leaving empty space in the first column for visual alignment purposes. This prioritizes consistent alignment over space optimization.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test (3.10.12, 1.8.3)
- GitHub Check: Analyze (python)
- GitHub Check: test (3.10.12, 1.8.3)
🔇 Additional comments (3)
widgets/sidebar.py (3)
7-35: LGTM! Well-structured state management class.The
SidebarRefclass provides clean encapsulation of sidebar state with proper session state integration. The property methods for generating keys are efficient and the state management methods correctly update both instance and session state.
37-46: LGTM! Clean factory function implementation.The
use_sidebarfunction provides a clean interface for creating sidebar references with proper default handling and session state initialization.
48-63: LGTM! Conditional rendering handles responsive sidebar properly.The function correctly renders sidebar items with appropriate styling and conditionally shows titles based on sidebar state.
anish-work
force-pushed
the
sidebar_layout
branch
from
f906825 to
fd7bdc4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
widgets/sidebar.py (2)
101-116: Consider making the saved URL configurable.The hard-coded "/saved/" URL should be moved to settings for consistency with other URL configurations in the codebase, as mentioned in previous reviews.
144-144: Fix typo in variable name.The variable name
sidebar_funtion_classeshas a typo and should besidebar_function_classes.- sidebar_funtion_classes = ( + sidebar_function_classes = (
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
daras_ai_v2/base.py(3 hunks)daras_ai_v2/icons.py(1 hunks)daras_ai_v2/settings.py(2 hunks)routers/account.py(2 hunks)routers/root.py(4 hunks)static/css/app.css(0 hunks)widgets/explore.py(2 hunks)widgets/sidebar.py(1 hunks)workspaces/widgets.py(4 hunks)
💤 Files with no reviewable changes (1)
- static/css/app.css
✅ Files skipped from review due to trivial changes (2)
- widgets/explore.py
- routers/account.py
🚧 Files skipped from review as they are similar to previous changes (5)
- daras_ai_v2/icons.py
- workspaces/widgets.py
- daras_ai_v2/settings.py
- routers/root.py
- daras_ai_v2/base.py
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: nikochiko
PR: GooeyAI/gooey-server#703
File: workspaces/widgets.py:161-166
Timestamp: 2025-06-16T11:43:04.972Z
Learning: In workspaces/widgets.py header links rendering, the gui.columns([2, 10]) layout should be maintained consistently even when no icon exists, leaving empty space in the first column for visual alignment purposes. This prioritizes consistent alignment over space optimization.
Learnt from: nikochiko
PR: GooeyAI/gooey-server#703
File: daras_ai_v2/settings.py:301-308
Timestamp: 2025-06-16T11:42:40.993Z
Learning: In daras_ai_v2/settings.py, when using a localhost frontend with api.gooey.ai backend deployment, hard-coded "/explore/" paths work correctly as relative URLs, but EXPLORE_URL computed from APP_BASE_URL may point to the wrong domain (localhost instead of api.gooey.ai).
📚 Learning: in daras_ai_v2/settings.py, when using a localhost frontend with api.gooey.ai backend deployment, ha...
Learnt from: nikochiko
PR: GooeyAI/gooey-server#703
File: daras_ai_v2/settings.py:301-308
Timestamp: 2025-06-16T11:42:40.993Z
Learning: In daras_ai_v2/settings.py, when using a localhost frontend with api.gooey.ai backend deployment, hard-coded "/explore/" paths work correctly as relative URLs, but EXPLORE_URL computed from APP_BASE_URL may point to the wrong domain (localhost instead of api.gooey.ai).
Applied to files:
widgets/sidebar.py
📚 Learning: in workspaces/widgets.py header links rendering, the gui.columns([2, 10]) layout should be maintaine...
Learnt from: nikochiko
PR: GooeyAI/gooey-server#703
File: workspaces/widgets.py:161-166
Timestamp: 2025-06-16T11:43:04.972Z
Learning: In workspaces/widgets.py header links rendering, the gui.columns([2, 10]) layout should be maintained consistently even when no icon exists, leaving empty space in the first column for visual alignment purposes. This prioritizes consistent alignment over space optimization.
Applied to files:
widgets/sidebar.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (python)
- GitHub Check: test (3.10.12, 1.8.3)
🔇 Additional comments (7)
widgets/sidebar.py (7)
1-5: LGTM!Clean and well-organized imports with all dependencies properly referenced.
7-30: LGTM!Well-designed state management class with clean separation of concerns. The mobile state synchronization in
set_mobile_openis a good design choice.
32-42: LGTM!Clean factory function with proper state management and good documentation.
44-84: LGTM!Well-structured sidebar item renderer with good CSS styling and proper conditional rendering logic.
86-99: LGTM!Good implementation that maintains layout consistency and properly limits visible items when collapsed.
118-140: LGTM!Clean mobile header implementation with proper state management and responsive design.
147-207: LGTM!Excellent CSS implementation with smooth transitions, proper responsive design, and good accessibility considerations.
| raise gui.RerunException() | ||
|
|
||
| sidebar_content_placeholder = gui.div( | ||
| className=f"d-flex flex-column flex-grow-1 gooey-sidebar {sidebar_funtion_classes}", |
There was a problem hiding this comment.
Fix typo in variable usage.
The variable name sidebar_funtion_classes should be sidebar_function_classes to match the corrected variable name.
- className=f"d-flex flex-column flex-grow-1 gooey-sidebar {sidebar_funtion_classes}",
+ className=f"d-flex flex-column flex-grow-1 gooey-sidebar {sidebar_function_classes}",📝 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.
| className=f"d-flex flex-column flex-grow-1 gooey-sidebar {sidebar_funtion_classes}", | |
| className=f"d-flex flex-column flex-grow-1 gooey-sidebar {sidebar_function_classes}", |
🤖 Prompt for AI Agents
In widgets/sidebar.py at line 235, there is a typo in the variable name used in
the className string. Replace `sidebar_funtion_classes` with the correctly
spelled `sidebar_function_classes` to match the intended variable name.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
daras_ai_v2/base.py (1)
397-399: Mobile header call is good; add a11y to the underlying componentCalling
sidebar_logo_header(self.request.session)is the right integration point for mobile. However, the underlying widget currently renders:
- a logo
<img>withoutalt- an icon-only toggle button without accessible name/state
Please add
alt="Gooey AI Logo"to the image, andariaLabel+ariaExpandedto the toggle inwidgets/sidebar.py:sidebar_logo_header. This mirrors an earlier suggestion and keeps things WCAG-friendly.Example (in
widgets/sidebar.py):gui.tag( "img", src=settings.GOOEY_LOGO_FACE, width=settings.SIDEBAR_ICON_SIZE, height=settings.SIDEBAR_ICON_SIZE, className=" logo-face", + alt="Gooey AI Logo", ) open_mobile_sidebar = gui.button( label=icons.sidebar_flip, className="m-0", unsafe_allow_html=True, type="tertiary", + ariaLabel="Open sidebar navigation", + ariaExpanded=sidebar_ref.is_mobile_open, )Also consider assigning an id (e.g.,
id="main-sidebar") to the sidebar container and addingariaControls="main-sidebar"to the toggle.
🧹 Nitpick comments (6)
daras_ai_v2/base.py (1)
197-199: Default hook is fine; optionally underscore the unused param
render_sidebar(self, request, sidebar_ref)provides a good override point. Since the default implementation doesn’t usesidebar_ref, consider underscoring it to avoid “unused” lints.- def render_sidebar(self, request, sidebar_ref): + def render_sidebar(self, request, _sidebar_ref): render_default_sidebar(request.session)routers/root.py (5)
710-714: Add light typing forpageto improve clarity (without import cycles)The new
pageparam is untyped; consider a structural Protocol to document the expected interface without importingBasePage.from contextlib import contextmanager from enum import Enum from time import time +from typing import Protocol, Optional +class _HasRenderSidebar(Protocol): + def render_sidebar(self, request, sidebar_ref) -> None: ... + def render(self) -> None: ... @contextmanager def page_wrapper( request: Request, className="", - page=None, + page: Optional[_HasRenderSidebar] = None, ):
720-724: Minor: simplifycontainerassignment
container = page if page else Nonecan be justcontainer = page.- container = page if page else None + container = page
740-759: Add alt text to logo and ARIA to the open buttonImprove screen reader support for the sidebar header elements.
# sidebar header gui.tag( "img", src=settings.GOOEY_LOGO_FACE, width=settings.SIDEBAR_ICON_SIZE, height=settings.SIDEBAR_ICON_SIZE, - className=" logo-face d-block", + className=" logo-face d-block", + alt="Gooey AI Logo", ) open_sidebar_btn = gui.button( label=icons.sidebar_flip, className="m-0 d-none hover-btn", unsafe_allow_html=True, type="tertiary", + ariaLabel="Open sidebar navigation", + ariaExpanded=sidebar_ref.is_open, + ariaControls="main-sidebar", )Note: To fully wire
ariaControls, setid="main-sidebar"on the sidebar container inwidgets/sidebar.sidebar_layoutwheresidebar_content_placeholderis created.
789-808: Add ARIA to close buttons (mobile and desktop)Expose accessible names and state for the close actions.
- close_mobile_sidebar = gui.button( + close_mobile_sidebar = gui.button( label=icons.cancel, className="m-0 d-md-none p-2", unsafe_allow_html=True, type="tertiary", + ariaLabel="Close sidebar", + ariaExpanded=sidebar_ref.is_mobile_open, + ariaControls="main-sidebar", ) ... - if sidebar_ref.is_open: + if sidebar_ref.is_open: close_sidebar = gui.button( label=icons.sidebar_flip, className="m-0 d-none d-md-block", unsafe_allow_html=True, type="tertiary", + ariaLabel="Close sidebar", + ariaExpanded=sidebar_ref.is_open, + ariaControls="main-sidebar", )Optional: also close on Escape for keyboard users.
with pane_container: with gui.div(className="d-flex flex-column min-vh-100 w-100 pt-md-2"): + gui.html(""" + <script> + document.addEventListener('keydown', (e) => { + if (e.key === 'Escape') { + const btn = document.getElementById('sidebar-hidden-btn'); + if (btn) btn.click(); + } + }); + </script> + """)
847-877: Remove large commented-out mobile search blockThis block is fully commented and superseded by the new sidebar UX. Removing it will reduce noise and avoid future confusion.
-# def _render_mobile_search_button(request: Request, search_filters: SearchFilters): -# ... -# gui.button( -# "Cancel", -# type="tertiary", -# className="d-md-none fs-6 m-0 ms-1 p-1", -# onClick=JS_HIDE_MOBILE_SEARCH, -# )If the associated
JS_SHOW_MOBILE_SEARCH/JS_HIDE_MOBILE_SEARCHconstants are now unused, consider removing them too.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
daras_ai_v2/base.py(3 hunks)daras_ai_v2/settings.py(2 hunks)routers/root.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- daras_ai_v2/settings.py
🧰 Additional context used
🧬 Code graph analysis (2)
daras_ai_v2/base.py (1)
widgets/sidebar.py (2)
render_default_sidebar(101-115)sidebar_logo_header(118-139)
routers/root.py (4)
widgets/sidebar.py (5)
render_default_sidebar(101-115)sidebar_layout(142-242)use_sidebar(32-41)set_open(20-21)set_mobile_open(23-25)daras_ai_v2/base.py (3)
current_workspace(1441-1446)render_sidebar(197-198)render(370-418)workspaces/widgets.py (1)
global_workspace_selector(21-170)widgets/explore.py (1)
render(55-94)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test (3.10.12, 1.8.3)
🔇 Additional comments (7)
daras_ai_v2/base.py (1)
94-94: LGTM: import new sidebar helpersImporting
render_default_sidebarandsidebar_logo_headerhere looks appropriate and keeps BasePage lean.routers/root.py (6)
50-50: LGTM: import of sidebar primitivesBringing in
use_sidebar,sidebar_layout, andrender_default_sidebaris consistent with the new layout approach.
254-256: Explore route wrapper usage looks correctWrapping
explore.render(...)withpage_wrapper(request)aligns this route with the new two-pane layout. No issues spotted.
693-701: Page object wiring into wrapper is correctPassing
page=pageenables page-specific sidebars viapage.render_sidebar(...). The meta generation below still reads from the correct state.
809-813: Conditional sidebar rendering: looks goodDefer to page-specific sidebars when available; fallback to
render_default_sidebarotherwise.
815-833: Bottom workspace selector: good UX; ensure no overlap on small screensThis desktop-only bottom bar is helpful when collapsed. Just confirm it stays hidden on mobile overlays to avoid stacking with the fixed mobile sidebar.
You can sanity-check by toggling collapse on ≤767px widths and ensuring the
.d-md-block d-noneclasses prevent overlap.
833-845: Main content pane wiring is cleanMetrics/scripts injected once, content isolated within
#main-content, and footer/login scripts follow. Looks good.
anish-work
force-pushed
the
sidebar_layout
branch
from
122aee0 to
31e7286
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
routers/root.py (3)
744-750: Accessibility: add alt text and titles for icon-only elementsAdd accessible text for the logo image and icon-only buttons.
Apply:
@@ - gui.tag( + gui.tag( "img", src=settings.GOOEY_LOGO_FACE, width=settings.SIDEBAR_ICON_SIZE, height=settings.SIDEBAR_ICON_SIZE, className=" logo-face d-block", + alt="Gooey logo", ) @@ - open_sidebar_btn = gui.button( + open_sidebar_btn = gui.button( label=icons.sidebar_flip, className="m-0 d-none hover-btn gooey-btn", unsafe_allow_html=True, type="tertiary", + title="Open sidebar", ) @@ - close_mobile_sidebar = gui.button( + close_mobile_sidebar = gui.button( label=icons.cancel, className="m-0 d-md-none gooey-btn", unsafe_allow_html=True, type="tertiary", + title="Close sidebar", ) @@ - close_sidebar = gui.button( + close_sidebar = gui.button( label=icons.sidebar_flip, className="m-0 d-none d-md-block gooey-btn", unsafe_allow_html=True, type="tertiary", + title="Close sidebar", )Also applies to: 751-756, 788-797, 798-808
736-760: Click-to-open coupling is fragile (id-specific); prefer container-based detectionThe open-on-click path in
widgets.sidebar.sidebar_layoutchecksevent.target.id === "sidebar-click-container", which won’t fire when clicking child elements. Since the handler is bound on the container, useevent.currentTargetinstead.Follow-up (change in widgets/sidebar.py, not this file):
# in sidebar_layout(...), replace the onClick condition onClick=dedent( """ if (event.currentTarget && event.currentTarget.classList.contains("sidebar-click-container")) { document.getElementById("sidebar-hidden-btn").click(); } """ if not sidebar_ref.is_open else "" ),Alternatively, remove the condition entirely and rely on the binding context of the container.
847-877: Remove commented-out mobile search UI and unused JS constants
The_render_mobile_search_buttonfunction and itsJS_SHOW_MOBILE_SEARCH/JS_HIDE_MOBILE_SEARCHconstants aren’t referenced elsewhere; remove them from routers/root.py to clean up dead code.-# def _render_mobile_search_button(request: Request, search_filters: SearchFilters): -# with gui.div( -# className="d-flex d-md-none flex-grow-1 justify-content-end", -# ): -# gui.button( -# icons.search, -# type="tertiary", -# unsafe_allow_html=True, -# className="m-0", -# onClick=JS_SHOW_MOBILE_SEARCH, -# ) -# -# with ( -# gui.styled("@media (min-width: 768px) { & { position: static !important; } }"), -# gui.div( -# className="d-md-flex flex-grow-1 justify-content-center align-items-center bg-white top-0 left-0", -# style={"display": "none", "zIndex": "10"}, -# id="mobile_search_container", -# ), -# ): -# render_search_bar_with_redirect( -# request=request, -# search_filters=search_filters or SearchFilters(), -# ) -# gui.button( -# "Cancel", -# type="tertiary", -# className="d-md-none fs-6 m-0 ms-1 p-1", -# onClick=JS_HIDE_MOBILE_SEARCH, -# )Also remove the
JS_SHOW_MOBILE_SEARCHandJS_HIDE_MOBILE_SEARCHdefinitions at lines 879–888.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
daras_ai_v2/icons.py(1 hunks)daras_ai_v2/settings.py(2 hunks)routers/root.py(4 hunks)widgets/sidebar.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- daras_ai_v2/icons.py
- widgets/sidebar.py
- daras_ai_v2/settings.py
🧰 Additional context used
🧬 Code graph analysis (1)
routers/root.py (3)
widgets/sidebar.py (5)
render_default_sidebar(115-135)sidebar_layout(169-278)use_sidebar(32-41)set_open(20-21)set_mobile_open(23-25)daras_ai_v2/base.py (3)
current_workspace(1441-1446)render_sidebar(197-198)render(370-418)workspaces/widgets.py (1)
global_workspace_selector(21-170)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test (3.10.12, 1.8.3)
🔇 Additional comments (7)
routers/root.py (7)
50-50: Importing sidebar utilities — LGTMThe new import aligns with the sidebar-first layout. No issues.
254-256: Explore routed through the new wrapper — LGTMDelegating layout to
page_wrapper(request)keeps page logic focused.
693-695: Per-page sidebar viapage_wrapper(request, page=page)— LGTMThis enables page-specific sidebars while preserving default behavior.
720-722: Sidebar state initialization — LGTMUsing a stable key (
"main-sidebar") and default open state is reasonable.
834-845: Script re-injection on rerenders
gtag.htmlandlogin_scripts.htmlare injected on each render; ensure templates deduplicate by id to avoid duplicate event bindings afterRerunExceptions.If not deduped, wrap with guards or inject once at app root.
809-813: render_sidebar fallback confirmed in base
The base class in daras_ai_v2/base.py defines render_sidebar (calling render_default_sidebar), so every page will correctly fall back when not overridden. No further action required.
709-714: Allpage_wrappercall sites updated. Verified across the repo—onlyrequest,className, and (where intended)pageare passed; no callers still usesearch_filtersorshow_search_bar.
anish-work
force-pushed
the
sidebar_layout
branch
from
31e7286 to
72a6f4d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
daras_ai_v2/base.py(3 hunks)daras_ai_v2/icons.py(1 hunks)daras_ai_v2/settings.py(2 hunks)routers/account.py(2 hunks)routers/root.py(4 hunks)static/css/app.css(0 hunks)widgets/explore.py(2 hunks)widgets/sidebar.py(1 hunks)workspaces/widgets.py(4 hunks)
💤 Files with no reviewable changes (1)
- static/css/app.css
🚧 Files skipped from review as they are similar to previous changes (2)
- routers/account.py
- daras_ai_v2/icons.py
🧰 Additional context used
🧬 Code graph analysis (4)
workspaces/widgets.py (2)
app_users/models.py (1)
AppUser(90-262)workspaces/models.py (2)
display_name(397-407)html_icon(409-416)
routers/root.py (3)
widgets/sidebar.py (5)
render_default_sidebar(115-135)sidebar_layout(169-278)use_sidebar(32-41)set_open(20-21)set_mobile_open(23-25)daras_ai_v2/base.py (3)
current_workspace(1441-1446)render_sidebar(197-198)render(370-418)workspaces/widgets.py (1)
global_workspace_selector(21-167)
widgets/explore.py (1)
widgets/sidebar.py (1)
sidebar_logo_header(138-160)
daras_ai_v2/base.py (1)
widgets/sidebar.py (2)
render_default_sidebar(115-135)sidebar_logo_header(138-160)
🪛 Ruff (0.13.1)
widgets/sidebar.py
101-101: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
276-276: Found useless expression. Either assign it to a variable or remove it.
(B018)
277-277: Found useless expression. Either assign it to a variable or remove it.
(B018)
daras_ai_v2/base.py
197-197: Unused method argument: sidebar_ref
(ARG002)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test (3.10.12, 1.8.3)
🔇 Additional comments (4)
widgets/sidebar.py (1)
145-156: Restore the accessibility affordances for the logo & toggleFollowing up on the earlier review: the mobile-only logo still lacks
alttext and the toggle button is missing ARIA metadata, so screen-reader users get no contextual cues. Please reinstate those attributes.gui.tag( "img", src=settings.GOOEY_LOGO_FACE, width=settings.SIDEBAR_ICON_SIZE, height=settings.SIDEBAR_ICON_SIZE, className=" logo-face", + alt="Gooey AI logo", ) open_mobile_sidebar = gui.button( label=icons.sidebar_flip, className="m-0", unsafe_allow_html=True, type="tertiary", style={"padding": "6px 10px"}, + ariaLabel="Toggle sidebar navigation", + ariaExpanded=str(sidebar_ref.is_mobile_open).lower(), )widgets/explore.py (1)
19-19: Explore header hook looks good.Confirming the new
sidebar_logo_headerimport and early call so mobile users get a toggle before the search UI—no issues from my side.Also applies to: 56-57
workspaces/widgets.py (1)
21-69: Popover tweaks andhide_nametoggle read clean.The optional
hide_nameflag plus escaping the display name keeps us safe without breaking the existing selector UX—looks solid.routers/root.py (1)
809-812: No missingsidebar_refinrender_sidebaroverrides
Allrender_sidebar(self, request, sidebar_ref)definitions are present; no instances of the old two-argument signature were found.
widgets/sidebar.py
Outdated
| open_mobile_sidebar = gui.button( | ||
| label=icons.sidebar_flip, | ||
| className="m-0", | ||
| unsafe_allow_html=True, | ||
| type="tertiary", | ||
| style={"padding": "6px 10px"}, | ||
| ) | ||
| if open_mobile_sidebar: | ||
| sidebar_ref.set_mobile_open(True) | ||
| raise gui.RerunException() |
There was a problem hiding this comment.
Allow the mobile toggle to close the sidebar
The mobile header button always calls set_mobile_open(True) so once opened, the sidebar can never be closed again. Please toggle the current state instead so users can dismiss the mobile sidebar.
if open_mobile_sidebar:
- sidebar_ref.set_mobile_open(True)
+ sidebar_ref.set_mobile_open(not sidebar_ref.is_mobile_open)
raise gui.RerunException()📝 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.
| open_mobile_sidebar = gui.button( | |
| label=icons.sidebar_flip, | |
| className="m-0", | |
| unsafe_allow_html=True, | |
| type="tertiary", | |
| style={"padding": "6px 10px"}, | |
| ) | |
| if open_mobile_sidebar: | |
| sidebar_ref.set_mobile_open(True) | |
| raise gui.RerunException() | |
| open_mobile_sidebar = gui.button( | |
| label=icons.sidebar_flip, | |
| className="m-0", | |
| unsafe_allow_html=True, | |
| type="tertiary", | |
| style={"padding": "6px 10px"}, | |
| ) | |
| if open_mobile_sidebar: | |
| sidebar_ref.set_mobile_open(not sidebar_ref.is_mobile_open) | |
| raise gui.RerunException() |
🤖 Prompt for AI Agents
In widgets/sidebar.py around lines 151 to 160, the mobile header button always
sets the sidebar open with sidebar_ref.set_mobile_open(True) so it can never be
closed; change this to read the current mobile open state from the sidebar_ref
(e.g., via a getter or attribute) and call set_mobile_open with the negated
value to toggle the state, then raise gui.RerunException() as before to
re-render.
| with ( | ||
| gui.styled(side_bar_styles), | ||
| gui.div( | ||
| className="d-flex w-100 h-100 position-relative sidebar-click-container", | ||
| style={"height": "100dvh"}, | ||
| onClick=dedent( | ||
| """ | ||
| if (event.target.id === "sidebar-click-container") { | ||
| document.getElementById("sidebar-hidden-btn").click(); | ||
| } | ||
| """ | ||
| if not sidebar_ref.is_open | ||
| else "" | ||
| ), |
There was a problem hiding this comment.
Fix the sidebar overlay click handler
The onClick script checks for event.target.id === "sidebar-click-container", but the wrapper div never sets that id, so the hidden button never fires and a collapsed sidebar can’t reopen. Add the missing id (or adjust the selector) so the handler runs.
gui.div(
className="d-flex w-100 h-100 position-relative sidebar-click-container",
+ id="sidebar-click-container",
style={"height": "100dvh"},
onClick=dedent(📝 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.
| with ( | |
| gui.styled(side_bar_styles), | |
| gui.div( | |
| className="d-flex w-100 h-100 position-relative sidebar-click-container", | |
| style={"height": "100dvh"}, | |
| onClick=dedent( | |
| """ | |
| if (event.target.id === "sidebar-click-container") { | |
| document.getElementById("sidebar-hidden-btn").click(); | |
| } | |
| """ | |
| if not sidebar_ref.is_open | |
| else "" | |
| ), | |
| with ( | |
| gui.styled(side_bar_styles), | |
| gui.div( | |
| className="d-flex w-100 h-100 position-relative sidebar-click-container", | |
| id="sidebar-click-container", | |
| style={"height": "100dvh"}, | |
| onClick=dedent( | |
| """ | |
| if (event.target.id === "sidebar-click-container") { | |
| document.getElementById("sidebar-hidden-btn").click(); | |
| } | |
| """ | |
| if not sidebar_ref.is_open | |
| else "" | |
| ), |
🤖 Prompt for AI Agents
In widgets/sidebar.py around lines 245 to 258, the onClick handler compares
event.target.id === "sidebar-click-container" but the wrapper div never sets
that id; update the wrapper div to include id="sidebar-click-container" (or
change the selector logic to check for the existing className
"sidebar-click-container") so the condition matches and the hidden button click
fires to reopen a collapsed sidebar.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
widgets/sidebar.py (2)
179-188: Allow the mobile toggle button to close the sidebar too.Currently it always calls
set_mobile_open(True), so users cannot toggle it off via the same control.- if open_mobile_sidebar: - sidebar_ref.set_mobile_open(True) + if open_mobile_sidebar: + sidebar_ref.set_mobile_open(not sidebar_ref.is_mobile_open) raise gui.RerunException()
273-287: Fix sidebar overlay click handler: missing id prevents reopening.The onClick script checks
event.target.id === "sidebar-click-container", but this wrapper div doesn’t set that id, so the hidden button never fires.- gui.div( - className="d-flex w-100 h-100 position-relative sidebar-click-container", + gui.div( + className="d-flex w-100 h-100 position-relative sidebar-click-container", + id="sidebar-click-container", style={"height": "100dvh"},
🧹 Nitpick comments (6)
widgets/sidebar.py (6)
199-201: Renamesidebar_funtion_classestypo for clarity.Spelling error can cause confusion and future bugs.
- sidebar_funtion_classes = ( + sidebar_function_classes = ( "gooey-sidebar-open" if sidebar_ref.is_open else "gooey-sidebar-closed" ) ... - sidebar_content_placeholder = gui.div( - className=f"d-flex flex-column flex-grow-1 gooey-sidebar {sidebar_funtion_classes}", + sidebar_content_placeholder = gui.div( + className=f"d-flex flex-column flex-grow-1 gooey-sidebar {sidebar_function_classes}", style={"height": "100dvh"}, )Also applies to: 298-301
40-46: Comment/code mismatch on “fresh page load” threshold.Comment says “1 second” but code uses
0.5. Align the comment (or the constant).- # If more than 1 second has passed since last load, consider it a fresh page load - if current_time - last_load_time > 0.5: + # If more than 0.5 seconds have passed since last load, consider it a fresh page load + if current_time - last_load_time > 0.5:
44-46: Use dict.pop for concise state cleanup.Simplifies deletion without a prior
incheck.- mobile_key = key + ":mobile" - if mobile_key in session: - del session[mobile_key] + mobile_key = key + ":mobile" + session.pop(mobile_key, None)
125-131: Remove unused enumerate index.Index
iisn’t used; avoid enumerate or rename to_.-def sidebar_item_list(is_sidebar_open, current_url=None): - for i, (url, label, icon) in enumerate(settings.SIDEBAR_LINKS): +def sidebar_item_list(is_sidebar_open, current_url=None): + for url, label, icon in settings.SIDEBAR_LINKS:
147-160: Avoid hard-coded URLs in the default sidebar.Use route helpers or a settings constant for
/account/saved/and/explore/to prevent drift with router changes.Would you like a follow-up patch to wire these via
get_route_path(...)or asettingsconstant?
101-118: Add accessible labels when the sidebar is collapsed (icon-only links).When closed, anchors render only an icon. Add
aria-label(and optionallytitle) withtitletext for screen readers and tooltips.- with gui.tag( + with gui.tag( "a", href=url, - className=link_classes, + className=link_classes, + ariaLabel=title, + title=title, ):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
daras_ai_v2/base.py(3 hunks)routers/account.py(2 hunks)routers/root.py(5 hunks)templates/google_one_tap_button.html(1 hunks)widgets/explore.py(2 hunks)widgets/sidebar.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- routers/account.py
- daras_ai_v2/base.py
- widgets/explore.py
🧰 Additional context used
🧬 Code graph analysis (1)
routers/root.py (4)
widgets/sidebar.py (5)
render_default_sidebar(139-163)sidebar_layout(197-303)use_sidebar(32-57)set_open(20-21)set_mobile_open(23-25)daras_ai_v2/base.py (3)
current_workspace(1441-1446)render_sidebar(197-198)render(370-418)workspaces/widgets.py (1)
global_workspace_selector(21-167)widgets/explore.py (1)
render(55-94)
🪛 Ruff (0.13.2)
widgets/sidebar.py
45-45: Use pop instead of key in dict followed by del dict[key]
Replace if statement with .pop(..., None)
(RUF051)
125-125: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test (3.10.12, 1.8.3)
- GitHub Check: test (3.10.12, 1.8.3)
🔇 Additional comments (3)
templates/google_one_tap_button.html (1)
9-12: Confirm responsive intent for the mobile icon on desktop.Removing
d-md-nonewill show the 40px icon on desktop too. Is that desired, or should desktop render the full-width button?routers/root.py (2)
720-724: Good integration of sidebar state and header controls.Sidebar ref + layout wiring looks correct; header adds the hover open control and mobile close. Ensure the overlay id fix lands in
widgets/sidebar.pyso the click-to-open behavior works end-to-end.Also applies to: 735-760
905-912: One Tap template inclusion looks good; depends on the template fix.Including
google_one_tap_button.htmlhere is fine. Please apply the template null‑guard so desktop rendering doesn’t throw when the desktop container is absent.
| <!-- <span id="g_id_signin_desktop" class="d-none d-md-inline"></span> --> | ||
| <span id="g_id_signin_mobile"></span> |
There was a problem hiding this comment.
Avoid calling renderButton on a missing element (desktop container removed).
You commented out #g_id_signin_desktop but still invoke renderButton for it, which will error when the container is null. Guard the call or remove it.
async function oneTapSignin() {
google.accounts.id.prompt();
- google.accounts.id.renderButton(document.getElementById("g_id_signin_desktop"), {
- shape: "rectangular",
- width: 200,
- text: "continue_with",
- size: "large",
- });
+ const desktopEl = document.getElementById("g_id_signin_desktop");
+ if (desktopEl) {
+ google.accounts.id.renderButton(desktopEl, {
+ shape: "rectangular",
+ width: 200,
+ text: "continue_with",
+ size: "large",
+ });
+ }
google.accounts.id.renderButton(document.getElementById("g_id_signin_mobile"), {
shape: "rectangular",
width: 40,
type: "icon",
size: "large",
});
}Also applies to: 25-33
🤖 Prompt for AI Agents
templates/google_one_tap_button.html around lines 10-11 (also check 25-33): the
desktop container span was commented out but the code still calls renderButton
for the desktop id, which will throw when document.getElementById returns null;
update the logic to check for the element before calling renderButton (e.g.,
getElementById(...) and if the result is truthy then call renderButton) or
remove the desktop render call entirely, and apply the same guard to the other
occurrences around lines 25-33.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
widgets/sidebar.py (2)
167-198: Critical: Mobile sidebar toggle button only opens, never closes.Line 197 always calls
set_mobile_open(True), preventing users from closing the mobile sidebar once opened. This was flagged in a previous review but remains unaddressed.Apply this diff to toggle the state:
if open_mobile_sidebar: - sidebar_ref.set_mobile_open(True) + sidebar_ref.set_mobile_open(not sidebar_ref.is_mobile_open) raise gui.RerunException()
207-319: Fix typo and add missing ID attribute for click handler.Two issues persist from previous reviews:
- Line 209: Variable name typo
sidebar_funtion_classes(should besidebar_function_classes)- Lines 292-296: The onClick handler checks
event.target.id === "sidebar-click-container", but the div at line 292 usesclassNameinstead of setting anidattribute, causing the click handler to never fireApply this diff:
is_mobile_open = sidebar_ref.is_mobile_open - sidebar_funtion_classes = ( + sidebar_function_classes = ( "gooey-sidebar-open" if sidebar_ref.is_open else "gooey-sidebar-closed" ) # ... (style definitions remain the same) ... with ( gui.styled(side_bar_styles), gui.div( className="d-flex w-100 h-100 position-relative sidebar-click-container", + id="sidebar-click-container", style={"height": "100dvh"}, onClick=dedent( """ if (event.target.id === "sidebar-click-container") { document.getElementById("sidebar-hidden-btn").click(); } """ if not sidebar_ref.is_open else "" ), ), ): # ... sidebar_content_placeholder = gui.div( - className=f"d-flex flex-column flex-grow-1 gooey-sidebar {sidebar_funtion_classes}", + className=f"d-flex flex-column flex-grow-1 gooey-sidebar {sidebar_function_classes}", style={"height": "100dvh"}, )
🧹 Nitpick comments (3)
widgets/sidebar.py (2)
33-58: Reconsider the fresh page load detection mechanism.The timestamp-based approach (0.5 second threshold) for detecting fresh page loads may be unreliable in scenarios with slow networks or rapid navigation. Consider using a more robust mechanism such as a navigation counter or explicit page load markers.
Additionally, apply this diff to use the more efficient
popoperation:- # If more than 1 second has passed since last load, consider it a fresh page load if current_time - last_load_time > 0.5: # Fresh page load - clear mobile state mobile_key = key + ":mobile" - if mobile_key in session: - del session[mobile_key] + session.pop(mobile_key, None)
125-137: Remove unused loop variable.The loop variable
iis not used within the loop body.Apply this diff:
- for i, (url, label, icon) in enumerate(settings.SIDEBAR_LINKS): + for url, label, icon in settings.SIDEBAR_LINKS: if icon:routers/root.py (1)
710-844: Ensure workspace selector is accessible on mobile when sidebar is closed.
The bottom section (lines 820–822) is hidden below md breakpoints (d-md-block d-none), preventing mobile users from selecting a workspace. Provide a mobile-specific placement or toggle for workspace selection when the sidebar is closed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
daras_ai_v2/base.py(3 hunks)routers/account.py(2 hunks)routers/root.py(6 hunks)widgets/explore.py(2 hunks)widgets/sidebar.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- widgets/explore.py
- daras_ai_v2/base.py
🧰 Additional context used
🧬 Code graph analysis (3)
routers/root.py (3)
widgets/sidebar.py (5)
render_default_sidebar(140-164)sidebar_layout(207-319)use_sidebar(33-58)set_open(21-22)set_mobile_open(24-26)daras_ai_v2/base.py (3)
current_workspace(1442-1447)render_sidebar(198-199)render(371-419)workspaces/widgets.py (1)
global_workspace_selector(21-167)
routers/account.py (1)
widgets/sidebar.py (1)
sidebar_mobile_header(167-198)
widgets/sidebar.py (1)
routers/root.py (1)
anonymous_login_container(897-928)
🪛 Ruff (0.14.0)
widgets/sidebar.py
46-46: Use pop instead of key in dict followed by del dict[key]
Replace if statement with .pop(..., None)
(RUF051)
126-126: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test (3.10.12, 1.8.3)
- GitHub Check: test (3.10.12, 1.8.3)
🔇 Additional comments (9)
routers/account.py (2)
30-30: LGTM!The import is correctly added and aligns with the sidebar-first layout introduced in this PR.
385-385: LGTM!The mobile header is appropriately placed before the navigation tabs, ensuring consistent mobile UX across account pages.
routers/root.py (4)
50-50: LGTM!The imports correctly bring in the new sidebar utilities required for the refactored page wrapper.
254-255: LGTM!The simplified page_wrapper call delegates search UI rendering to the explore module, aligning with the sidebar-first layout approach.
693-694: LGTM!Passing the page instance enables custom sidebar rendering per recipe, which is a good extensibility pattern.
897-899: LGTM!The
hide_sign_inparameter enables flexible login UI composition for different sidebar states (e.g., collapsed sidebar showing only the icon).widgets/sidebar.py (3)
8-31: LGTM!The
SidebarRefclass provides clean state encapsulation for sidebar open/closed state with proper session persistence.
61-123: LGTM!The sidebar list item rendering correctly handles open/closed states, selection highlighting, and hover interactions.
140-164: LGTM!The default sidebar rendering provides sensible defaults with the core navigation items (Saved, Explore) and integrates the configurable sidebar links from settings.
anish-work
force-pushed
the
sidebar_layout
branch
from
cd3afaf to
4213b8f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
routers/root.py (1)
290-305: Closed-sidebar can’t reopen reliably (overlay click handler condition is brittle)onClick checks event.target.id, but clicks on children won’t match; use event.currentTarget to detect background clicks when collapsed.
- onClick=dedent( - """ - if (event.target.id === "sidebar-click-container") { - document.getElementById("sidebar-hidden-btn").click(); - } - """ - if not sidebar_ref.is_open - else "" - ), + onClick=dedent( + """ + // Reopen only when the background wrapper itself is clicked + if (event.currentTarget === event.target) { + document.getElementById("sidebar-hidden-btn").click(); + } + """ + if not sidebar_ref.is_open + else "" + ),
♻️ Duplicate comments (4)
routers/root.py (1)
744-756: Add alt text and ARIA to improve accessibility of sidebar controls and logoLogo lacks alt; toggle buttons lack accessible labels/state.
gui.tag( "img", src=settings.GOOEY_LOGO_FACE, width=settings.SIDEBAR_ICON_SIZE, height=settings.SIDEBAR_ICON_SIZE, - className=" logo-face d-none d-md-block", + className=" logo-face d-none d-md-block", + alt="Gooey.AI logo", ) open_sidebar_btn = gui.button( label=icons.sidebar_flip, className="m-0 d-none hover-btn gooey-btn", unsafe_allow_html=True, type="tertiary", + ariaLabel="Open sidebar", + ariaExpanded=str(sidebar_ref.is_open).lower(), ) @@ close_mobile_sidebar = gui.button( label=icons.cancel, className="m-0 d-md-none gooey-btn", unsafe_allow_html=True, type="tertiary", + ariaLabel="Close sidebar", + ariaExpanded=str(sidebar_ref.is_mobile_open).lower(), ) @@ if sidebar_ref.is_open: close_sidebar = gui.button( label=icons.sidebar_flip, className="m-0 d-none d-md-block gooey-btn", unsafe_allow_html=True, type="tertiary", + ariaLabel="Close sidebar", + ariaExpanded=str(sidebar_ref.is_open).lower(), )Also applies to: 789-793, 799-807
widgets/sidebar.py (3)
171-181: Add alt text and ARIA to mobile header for accessibilityProvide alt on logo and labels/states on the toggle button.
gui.tag( "img", src=settings.GOOEY_LOGO_RECT, width="120px", height="auto", - className=" logo-face", + className=" logo-face", + alt="Gooey.AI logo", ) @@ open_mobile_sidebar = gui.button( label=icons.sidebar_flip, className="m-0", unsafe_allow_html=True, type="tertiary", style={"padding": "6px 10px"}, + ariaLabel="Toggle sidebar", + ariaExpanded=str(use_sidebar("main-sidebar", request.session).is_mobile_open).lower(), )Also applies to: 190-197
148-156: Avoid hard-coded “Saved” URLMove “/account/saved/” (and “/explore/”) to a settings constant or generate via get_route_path to keep URLs consistent with router changes.
190-199: Mobile toggle is one-way; cannot close once openedButton always sets set_mobile_open(True). Toggle instead so users can close on mobile.
- if open_mobile_sidebar: - sidebar_ref.set_mobile_open(True) - raise gui.RerunException() + if open_mobile_sidebar: + sidebar_ref.set_mobile_open(not sidebar_ref.is_mobile_open) + raise gui.RerunException()
🧹 Nitpick comments (4)
routers/root.py (1)
50-51: Note on import-time: sidebar helpers at module importLikely fine, but if import-time regresses, consider moving sidebar imports into page_wrapper to defer cost until first render.
You can reuse the import-time script shared in base.py to compare before/after.
widgets/sidebar.py (3)
41-47: Use dict.pop and align comment with thresholdCleaner and avoids KeyError branch; comment says “1 second” but code uses 0.5s.
- # If more than 1 second has passed since last load, consider it a fresh page load + # If more than 0.5 seconds have passed since last load, consider it a fresh page load if current_time - last_load_time > 0.5: # Fresh page load - clear mobile state - mobile_key = key + ":mobile" - if mobile_key in session: - del session[mobile_key] + mobile_key = key + ":mobile" + session.pop(mobile_key, None)Also applies to: 48-50
126-131: Remove unused enumerate variablei is unused; simplify loop.
-def sidebar_item_list(is_sidebar_open, current_url=None): - for i, (url, label, icon) in enumerate(settings.SIDEBAR_LINKS): +def sidebar_item_list(is_sidebar_open, current_url=None): + for url, label, icon in settings.SIDEBAR_LINKS: if icon: with gui.div(): sidebar_list_item( icon, label, is_sidebar_open, url, icons.arrow_up_right, current_url )
210-213: Fix typo: sidebar_funtion_classes → sidebar_function_classesImproves readability and avoids future confusion.
- sidebar_funtion_classes = ( + sidebar_function_classes = ( "gooey-sidebar-open" if sidebar_ref.is_open else "gooey-sidebar-closed" ) @@ - sidebar_content_placeholder = gui.div( - className=f"d-flex flex-column flex-grow-1 gooey-sidebar {sidebar_funtion_classes}", + sidebar_content_placeholder = gui.div( + className=f"d-flex flex-column flex-grow-1 gooey-sidebar {sidebar_function_classes}", style={"height": "100dvh"}, )Also applies to: 316-319
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
daras_ai_v2/base.py(3 hunks)routers/account.py(2 hunks)routers/root.py(6 hunks)widgets/explore.py(2 hunks)widgets/sidebar.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- routers/account.py
- widgets/explore.py
🧰 Additional context used
🧬 Code graph analysis (3)
routers/root.py (4)
widgets/sidebar.py (5)
render_default_sidebar(140-164)sidebar_layout(208-320)use_sidebar(33-58)set_open(21-22)set_mobile_open(24-26)daras_ai_v2/base.py (3)
current_workspace(1442-1447)render_sidebar(198-199)render(371-419)workspaces/widgets.py (1)
global_workspace_selector(21-167)widgets/explore.py (1)
render(55-94)
daras_ai_v2/base.py (1)
widgets/sidebar.py (2)
render_default_sidebar(140-164)sidebar_mobile_header(167-199)
widgets/sidebar.py (1)
routers/root.py (1)
anonymous_login_container(897-928)
🪛 Ruff (0.14.0)
widgets/sidebar.py
46-46: Use pop instead of key in dict followed by del dict[key]
Replace if statement with .pop(..., None)
(RUF051)
126-126: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test (3.10.12, 1.8.3)
🔇 Additional comments (3)
daras_ai_v2/base.py (2)
398-400: Mobile header insertion looks correctGood placement; mobile-only container and reduced header spacing improve layout.
94-94: Lazy-load render_default_sidebar to minimize import timeThe refactoring is accurate and properly scoped. Import-time measurement shows widgets.sidebar module can be deferred:
render_default_sidebaris only invoked inside therender_sidebar()method (line 199), not during module initialization. Moving the import into the method will eliminate the baseline cost of loading widgets.sidebar when this module is imported butrender_sidebar()is never called.Note:
sidebar_mobile_headerfollows the same pattern (called only from within therender()method at line 398) and could also be lazy-loaded for consistency, though this review addresses onlyrender_default_sidebar.routers/root.py (1)
254-256: Wrapper adoption LGTMSwitching to page_wrapper for Explore and recipe pages integrates the new sidebar layout cleanly.
Also applies to: 693-695
1 participant
Q/A checklist
How to check import time?
You can visualize this using tuna:
To measure import time for a specific library:
To reduce import times, import libraries that take a long time inside the functions that use them instead of at the top of the file:
Legal Boilerplate
Look, I get it. The entity doing business as “Gooey.AI” and/or “Dara.network” was incorporated in the State of Delaware in 2020 as Dara Network Inc. and is gonna need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Dara Network Inc can use, modify, copy, and redistribute my contributions, under its choice of terms.