Bot builder in sidebar #830
Conversation
📝 WalkthroughWalkthroughAdds a sidebar-driven inline Gooey Builder flow. New module widgets/sidebar.py provides SidebarRef, use_sidebar, and sidebar_layout for session-backed responsive sidebar state and layout. daras_ai_v2/gooey_builder.py renames and adapts the builder entrypoint to render_gooey_builder_inline with inline mode, JS wiring, and onClose handling. daras_ai_v2/base.py adds BasePage.render_sidebar and rewires _render_gooey_builder to use sidebar state, merge builder state into gui.session_state, and expose open/close mobile handling. routers/root.py updates page_wrapper to render a two-pane layout using sidebar_layout/use_sidebar. daras_ai_v2/settings.py adds GOOEY_BUILDER_ICON and concurrency constants. 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 |
![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 (6)
daras_ai_v2/gooey_builder.py (2)
3-3: Defer heavy Django model import to reduce server import timeImporting BotIntegration at module import time can slow startup. Lazily import inside the functions instead.
Apply this diff:
-from bots.models import BotIntegration +# Defer heavy Django model import to function scope to reduce import timeAnd inside both functions:
def render_gooey_builder_inline(page_slug: str, builder_state: dict): + from bots.models import BotIntegration if not settings.GOOEY_BUILDER_INTEGRATION_ID: return @@ def render_gooey_builder(page_slug: str, builder_state: dict): + from bots.models import BotIntegration if not settings.GOOEY_BUILDER_INTEGRATION_ID: returnAlso applies to: 11-15, 66-74
12-14: Avoid hard-coding hostname; derive at runtime or accept a parameterUsing hostname="gooey.ai" may break staging/custom hosts and Google Maps API hostname checks. Consider deriving from settings.APP_BASE_URL or passing hostname from the caller.
Example:
from furl import furl hostname = furl(settings.APP_BASE_URL).host or "gooey.ai" config = bi.get_web_widget_config(hostname=hostname, target="#gooey-builder-embed")Also applies to: 16-24, 30-33
daras_ai_v2/base.py (1)
417-446: Unify default sidebar open state with routers.root to avoid first-load flickerrender_sidebar uses use_sidebar("builder-sidebar") with default default_open=True, but routers.root initializes the same key with default_open=False. Consider aligning defaults in both places (False) to prevent state desync on first load.
Would you like a small patch to centralize this default in a constant?
widgets/sidebar.py (3)
41-47: Use dict.pop instead of membership test + del (RUF051)Simplify and avoid KeyError race.
Apply this diff:
- 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] + if current_time - last_load_time > 0.5: + # Fresh page load - clear mobile state + mobile_key = key + ":mobile" + session.pop(mobile_key, None)
127-134: Remove unused enumerate index (B007)The loop counter i is unused.
Apply this diff:
-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:
41-43: Fix misleading comment about time thresholdComment says “1 second” but code uses 0.5s.
Update to: “If more than 0.5 seconds has passed since last load…”
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
daras_ai_v2/base.py(4 hunks)daras_ai_v2/gooey_builder.py(1 hunks)routers/root.py(4 hunks)widgets/sidebar.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
daras_ai_v2/gooey_builder.py (1)
bots/models/bot_integration.py (1)
get_web_widget_config(565-592)
routers/root.py (4)
widgets/sidebar.py (2)
sidebar_layout(210-322)use_sidebar(33-60)daras_ai_v2/base.py (3)
render_sidebar(417-445)render(368-415)current_workspace(1467-1472)routers/account.py (1)
explore_in_current_workspace(140-156)workspaces/widgets.py (1)
global_workspace_selector(21-161)
widgets/sidebar.py (1)
routers/root.py (1)
anonymous_login_container(853-882)
daras_ai_v2/base.py (2)
daras_ai_v2/gooey_builder.py (2)
render_gooey_builder(66-121)render_gooey_builder_inline(7-63)widgets/sidebar.py (3)
sidebar_layout(210-322)use_sidebar(33-60)set_open(21-22)
🪛 Ruff (0.14.2)
widgets/sidebar.py
46-46: Use pop instead of key in dict followed by del dict[key]
Replace if statement with .pop(..., None)
(RUF051)
128-128: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
| 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.
Click-to-open handler never fires (id mismatch and target misuse)
The onClick handler checks event.target.id === "sidebar-click-container", but the wrapper div has no id and target is often a child. Add the id and compare against event.currentTarget.
Apply this diff:
- 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 ""
- ),
- ),
+ gui.div(
+ id="sidebar-click-container",
+ className="d-flex w-100 h-100 position-relative sidebar-click-container",
+ style={"height": "100dvh"},
+ onClick=dedent(
+ """
+ if (event.currentTarget.id === "sidebar-click-container") {
+ document.getElementById("sidebar-hidden-btn").click();
+ }
+ """
+ if not sidebar_ref.is_open
+ else ""
+ ),
+ ),Also applies to: 308-316
🤖 Prompt for AI Agents
In widgets/sidebar.py around lines 292-306 (and similarly 308-316) the
click-to-open handler checks event.target.id === "sidebar-click-container" but
the wrapper div has no id and event.target may be a child element; add
id="sidebar-click-container" to the wrapper div and change the JS check to
compare against event.currentTarget (e.g., event.currentTarget.id ===
"sidebar-click-container") so the handler fires correctly; apply the same id
addition and event.currentTarget comparison to the other occurrence.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
daras_ai_v2/base.py (1)
1252-1274: Do not pass entire session_state to the widget; use the filtered approach in the commented code.This change still passes
gui.session_statewholesale to the client widget (line 1254), which can expose unrelated or sensitive session data. The previous review flagged this exact issue, and the commented code (lines 1255-1274) demonstrates the safer, filtered approach.Apply this diff to restore the filtered payload:
- render_gooey_builder_inline(self.slug_versions[-1], gui.session_state) - # render_gooey_builder( - # page_slug=self.slug_versions[-1], - # builder_state=dict( - # status=dict( - # error_msg=gui.session_state.get(StateKeys.error_msg), - # run_status=gui.session_state.get(StateKeys.run_status), - # run_time=gui.session_state.get(StateKeys.run_time), - # ), - # request=extract_model_fields( - # model=self.RequestModel, state=gui.session_state - # ), - # response=extract_model_fields( - # model=self.ResponseModel, state=gui.session_state - # ), - # metadata=dict( - # title=self.current_pr.title, - # description=self.current_pr.notes, - # ), - # ), - # ) + builder_state = dict( + status=dict( + error_msg=gui.session_state.get(StateKeys.error_msg), + run_status=gui.session_state.get(StateKeys.run_status), + run_time=gui.session_state.get(StateKeys.run_time), + ), + request=extract_model_fields( + model=self.RequestModel, state=gui.session_state + ), + response=extract_model_fields( + model=self.ResponseModel, state=gui.session_state + ), + metadata=dict( + title=self.current_pr.title, + description=self.current_pr.notes, + ), + ) + render_gooey_builder_inline(self.slug_versions[-1], builder_state)
🧹 Nitpick comments (1)
daras_ai_v2/gooey_builder.py (1)
7-24: Consider documenting the expected structure and security implications ofbuilder_state.The
builder_stateparameter accepts any dict and passes it to the client widget via variables. Adding a docstring or type hint clarifying the expected structure (e.g., status/request/response/metadata) and noting that sensitive data should be filtered out would help prevent misuse.Example:
def render_gooey_builder_inline(page_slug: str, builder_state: dict): + """ + Render the Gooey builder widget in inline mode. + + Args: + page_slug: The workflow page slug + builder_state: Filtered state dict containing only non-sensitive data + (e.g., status, request, response, metadata). Do not pass raw session_state. + """ if not settings.GOOEY_BUILDER_INTEGRATION_ID: return
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
daras_ai_v2/base.py(4 hunks)daras_ai_v2/gooey_builder.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
daras_ai_v2/base.py (2)
daras_ai_v2/gooey_builder.py (2)
render_gooey_builder(70-125)render_gooey_builder_inline(7-67)widgets/sidebar.py (3)
sidebar_layout(210-322)use_sidebar(33-60)set_open(21-22)
daras_ai_v2/gooey_builder.py (1)
bots/models/bot_integration.py (1)
get_web_widget_config(565-592)
⏰ 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)
anish-work
force-pushed
the
bot_builder_sidebar
branch
from
79e776a to
df49769
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
daras_ai_v2/base.py (1)
1282-1284: Do not pass entire session_state to the widget; filter to required fields only.Passing
gui.session_statewholesale torender_gooey_builder_inlinecan inadvertently expose unrelated or sensitive data to the client widget. The commented-out code below (lines 1286-1305) shows the safer, filtered approach.Apply the diff from the previous review to restore a filtered payload:
if not self.is_current_user_admin(): return -render_gooey_builder_inline(self.slug_versions[-1], gui.session_state) +builder_state = dict( + status=dict( + error_msg=gui.session_state.get(StateKeys.error_msg), + run_status=gui.session_state.get(StateKeys.run_status), + run_time=gui.session_state.get(StateKeys.run_time), + ), + request=extract_model_fields( + model=self.RequestModel, state=gui.session_state + ), + response=extract_model_fields( + model=self.ResponseModel, state=gui.session_state + ), + metadata=dict( + title=self.current_pr.title, + description=self.current_pr.notes, + ), +) +render_gooey_builder_inline(self.slug_versions[-1], builder_state)This preserves the inline experience while avoiding broad session exposure.
Based on past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
daras_ai_v2/base.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
daras_ai_v2/base.py (2)
daras_ai_v2/gooey_builder.py (2)
render_gooey_builder(70-125)render_gooey_builder_inline(7-67)widgets/sidebar.py (4)
sidebar_layout(210-322)use_sidebar(33-60)set_open(21-22)set_mobile_open(24-26)
⏰ 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 (2)
daras_ai_v2/base.py (2)
48-48: LGTM: Imports support the new sidebar functionality.The new imports are necessary for the sidebar integration. Note that
sidebar_layoutis imported at line 85 but not used in this file—it's likely consumed inrouters/root.pyas mentioned in the AI summary.Also applies to: 85-85
419-479: render_sidebar() is properly integrated and invoked.Verification confirms the method is called from
routers/root.pyline 740 ascontainer.render_sidebar(). The implementation correctly restricts the builder to the run tab, manages state transitions, and handles both desktop and mobile views. No issues found.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
daras_ai_v2/base.py (1)
1291-1310: Remove commented code.As noted in the previous review, this commented block should be removed to improve code cleanliness. The inline rendering approach is now established, and keeping the old implementation as a comment adds unnecessary clutter.
Apply this diff to remove the commented code:
- - # render_gooey_builder( - # page_slug=self.slug_versions[-1], - # builder_state=dict( - # status=dict( - # error_msg=gui.session_state.get(StateKeys.error_msg), - # run_status=gui.session_state.get(StateKeys.run_status), - # run_time=gui.session_state.get(StateKeys.run_time), - # ), - # request=extract_model_fields( - # model=self.RequestModel, state=gui.session_state - # ), - # response=extract_model_fields( - # model=self.ResponseModel, state=gui.session_state - # ), - # metadata=dict( - # title=self.current_pr.title, - # description=self.current_pr.notes, - # ), - # ), - # )Based on learnings
🧹 Nitpick comments (1)
daras_ai_v2/base.py (1)
419-466: Consider adding a docstring to document sidebar integration.The
render_sidebarmethod is a new public method but lacks documentation explaining when it's called and its integration with the page wrapper/layout system.Apply this diff to add documentation:
def render_sidebar(self): + """ + Render an admin-only sidebar containing the Gooey builder interface. + + This method is called by the page wrapper to render sidebar content. + The sidebar is only displayed on the run tab for admin users and provides + an inline builder interface for workflow configuration. + """ if not self.is_current_user_admin(): return
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
daras_ai_v2/base.py(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
daras_ai_v2/base.py (2)
daras_ai_v2/gooey_builder.py (2)
render_gooey_builder(70-125)render_gooey_builder_inline(7-67)widgets/sidebar.py (4)
sidebar_layout(210-325)use_sidebar(33-60)set_open(21-22)set_mobile_open(24-26)
⏰ 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 (1)
daras_ai_v2/base.py (1)
1270-1289: Security concern from past review has been addressed.The current implementation correctly filters
builder_stateto only include necessary fields (status, request, response, metadata) rather than passing the entiregui.session_state. The use ofextract_model_fieldsensures only model-defined fields are sent to the client widget.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
routers/root.py (1)
823-828: Fix className concatenation bug (regression from past review).Despite the past review comment indicating this was addressed, the
classNameparameter concatenation issue remains:
- When the sidebar is closed (
not is_builder_sidebar_open), the customclassNameparameter is completely dropped- When the sidebar is open, there's no space between
"mx-2 w-100"and the customclassNameApply this diff to properly concatenate the className in both cases:
with gui.div( id="main-content", - className="container-xxl " - if not is_builder_sidebar_open - else "mx-2 w-100" + className, + className=( + "container-xxl " + className + if not is_builder_sidebar_open + else "mx-2 w-100 " + className + ), ):
🧹 Nitpick comments (1)
routers/root.py (1)
733-733: Consider removing redundant assignment.The line
container = page if page else Noneis redundant since it simply assignspagetocontainer. You can usepagedirectly in the subsequent code.Apply this diff to simplify:
- container = page if page else None sidebar_ref = use_sidebar("builder-sidebar", request.session, default_open=False) sidebar_content, pane_content = sidebar_layout(sidebar_ref) is_builder_sidebar_open = sidebar_ref.is_open with sidebar_content: - if container: - container.render_sidebar() + if page: + page.render_sidebar()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
routers/root.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
routers/root.py (6)
widgets/sidebar.py (3)
sidebar_layout(210-325)use_sidebar(33-60)set_mobile_open(24-26)daras_ai_v2/base.py (3)
render_sidebar(419-465)render(370-417)current_workspace(1520-1525)app_users/models.py (2)
is_admin(261-262)save(359-365)daras_ai_v2/fastapi_tricks.py (1)
get_route_path(77-80)routers/account.py (1)
explore_in_current_workspace(140-156)workspaces/widgets.py (1)
global_workspace_selector(21-161)
⏰ 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)
routers/root.py (3)
48-48: LGTM!The sidebar imports are correctly added to support the new two-pane layout functionality.
705-705: LGTM!Correctly passes the page instance to enable page-specific sidebar rendering.
835-864: LGTM!The mobile search button implementation correctly uses responsive classes (
d-md-none) to show only on mobile devices, and the toggle logic properly manages visibility.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
daras_ai_v2/base.py (1)
1291-1310: Remove the commented legacyrender_gooey_buildercallThe commented block with the old non‑inline
render_gooey_builder(...)call is now redundant and increases noise. Since the inline path is the active implementation, it’s safe to delete this block.- # render_gooey_builder( - # page_slug=self.slug_versions[-1], - # builder_state=dict( - # status=dict( - # error_msg=gui.session_state.get(StateKeys.error_msg), - # run_status=gui.session_state.get(StateKeys.run_status), - # run_time=gui.session_state.get(StateKeys.run_time), - # ), - # request=extract_model_fields( - # model=self.RequestModel, state=gui.session_state - # ), - # response=extract_model_fields( - # model=self.ResponseModel, state=gui.session_state - # ), - # metadata=dict( - # title=self.current_pr.title, - # description=self.current_pr.notes, - # ), - # ), - # )
🧹 Nitpick comments (1)
daras_ai_v2/gooey_builder.py (1)
7-68: Guardconfig.onCloseagainst missing#onCloseelementThe inline builder wires
config.onClosetodocument.getElementById("onClose").click(), which will throw if the element is absent (e.g., layout changes, multiple builders, or timing edge cases). A small null‑check keeps the widget from crashing when the close target isn’t present.- config.onClose = function() { - document.getElementById("onClose").click(); - }; + config.onClose = function() { + const btn = document.getElementById("onClose"); + if (btn) btn.click(); + };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
daras_ai_v2/base.py(5 hunks)daras_ai_v2/gooey_builder.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
daras_ai_v2/gooey_builder.py (1)
bots/models/bot_integration.py (2)
BotIntegration(163-626)get_web_widget_config(565-592)
daras_ai_v2/base.py (2)
daras_ai_v2/gooey_builder.py (2)
render_gooey_builder(71-126)render_gooey_builder_inline(7-68)widgets/sidebar.py (3)
use_sidebar(33-60)set_open(21-22)set_mobile_open(24-26)
⏰ 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 (1)
daras_ai_v2/base.py (1)
1251-1289: Inline builder wiring and filtered state look goodThe updated
_render_gooey_buildernow callsrender_gooey_builder_inlinewith a filteredbuilder_state(status, request, response, metadata viaextract_model_fields). This avoids sending fullsession_stateto the widget while preserving necessary context for the builder. No changes needed here.
| def render_sidebar(self): | ||
| if not self.is_current_user_admin(): | ||
| return | ||
|
|
||
| sidebar_ref = use_sidebar("builder-sidebar", self.request.session) | ||
| if self.tab != RecipeTabs.run and self.tab != RecipeTabs.preview: | ||
| if sidebar_ref.is_open or sidebar_ref.is_mobile_open: | ||
| sidebar_ref.set_open(False) | ||
| sidebar_ref.set_mobile_open(False) | ||
| raise gui.RerunException() | ||
| return | ||
|
|
||
| if sidebar_ref.is_open or sidebar_ref.is_mobile_open: | ||
| gui.tag( | ||
| "button", | ||
| type="submit", | ||
| name="onCloseGooeyBuilder", | ||
| value="yes", | ||
| hidden=True, | ||
| id="onClose", | ||
| ) # hidden button to trigger the onClose event passed in the config | ||
|
|
||
| if gui.session_state.pop("onCloseGooeyBuilder", None): | ||
| sidebar_ref.set_open(False) | ||
| raise gui.RerunException() | ||
|
|
||
| with gui.div(className="w-100 h-100"): | ||
| self._render_gooey_builder() | ||
| else: | ||
| with gui.styled("& .gooey-builder-open-button:hover { scale: 1.2; }"): | ||
| with gui.div( | ||
| className="w-100 position-absolute", | ||
| style={"bottom": "24px", "left": "16px", "zIndex": "1000"}, | ||
| ): | ||
| gooey_builder_open_button = gui.button( | ||
| label=f"<img src='{settings.GOOEY_BUILDER_ICON}' style='width: 56px; height: 56px; border-radius: 50%;' />", | ||
| className="btn btn-secondary border-0 d-none d-md-block p-0 gooey-builder-open-button", | ||
| style={ | ||
| "width": "56px", | ||
| "height": "56px", | ||
| "borderRadius": "50%", | ||
| "boxShadow": "#0000001a 0 1px 4px, #0003 0 2px 12px", | ||
| }, | ||
| ) | ||
| if gooey_builder_open_button: | ||
| sidebar_ref.set_open(True) | ||
| raise gui.RerunException() | ||
|
|
There was a problem hiding this comment.
Ensure sidebar close works for both desktop and mobile states
In render_sidebar, when handling the hidden "onCloseGooeyBuilder" button, only sidebar_ref.set_open(False) is called. If the sidebar was opened via the mobile state (is_mobile_open), this leaves is_mobile_open True in the session, so the sidebar may immediately re‑open or get out of sync on mobile.
Recommend closing both flags when the builder’s onClose fires:
- if sidebar_ref.is_open or sidebar_ref.is_mobile_open:
+ if sidebar_ref.is_open or sidebar_ref.is_mobile_open:
gui.tag(
"button",
@@
- if gui.session_state.pop("onCloseGooeyBuilder", None):
- sidebar_ref.set_open(False)
- raise gui.RerunException()
+ if gui.session_state.pop("onCloseGooeyBuilder", None):
+ sidebar_ref.set_open(False)
+ sidebar_ref.set_mobile_open(False)
+ raise gui.RerunException()🤖 Prompt for AI Agents
daras_ai_v2/base.py around lines 419-466: the hidden "onCloseGooeyBuilder"
handler only calls sidebar_ref.set_open(False), leaving
sidebar_ref.is_mobile_open possibly true and causing mobile state to remain
out-of-sync; update the handler (the gui.session_state.pop branch) to also call
sidebar_ref.set_mobile_open(False) before raising gui.RerunException, and ensure
any other close-only branches in this range that call set_open(False) also clear
set_mobile_open(False) so both flags are closed when the builder onClose fires.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
routers/root.py (2)
721-729: Correctpageparameter type annotation (currently only allowsNone)
page: None = Noneannotates the parameter as literallyNone, which hides that callers can/should pass a page instance (e.g.,BasePage) and degrades type-checker/IDE help.Consider something like:
-from contextlib import contextmanager +from contextlib import contextmanager +from typing import Optional +from daras_ai_v2.base import BasePage @@ def page_wrapper( request: Request, - page: None = None, + page: Optional[BasePage] = None,(or your preferred union/forward-reference style if circular imports are a concern).
829-835: Fixmain-contentclassName concatenation (drops custom classes when sidebar is closed)Right now:
className="container-xxl " if not is_builder_sidebar_open else "mx-2 w-100" + className,
- Ignores the passed
classNamewhen the sidebar is closed.- Misses a separating space when the sidebar is open.
A more robust pattern:
- with gui.div( - id="main-content", - className="container-xxl " - if not is_builder_sidebar_open - else "mx-2 w-100" + className, - ): + base_class = ( + "container-xxl" + if not is_builder_sidebar_open + else "mx-2 w-100" + ) + extra_class = f" {className}" if className else "" + with gui.div( + id="main-content", + className=base_class + extra_class, + ):so any caller-provided classes are honored in both sidebar states with correct spacing.
🧹 Nitpick comments (1)
routers/root.py (1)
783-799: Hide Gooey builder mobile button on non-recipe pagesThe admin-only mobile builder button is rendered for every page using
page_wrapper, even whenpageisNone, so on non-recipe pages tapping it can show a blank sidebar.To avoid dangling UI, you can additionally gate on recipe pages:
- if request.user and request.user.is_admin: + if ( + is_recipe_page + and request.user + and request.user.is_admin + ):so the button only appears where the builder actually renders.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
routers/root.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{py,js,ts,tsx,java,cs,cpp,c,go,rb,php}
📄 CodeRabbit inference engine (.cursor/rules/devs-rules.mdc)
Format code in reverse topological order: place the main() function at the top and dependencies below it
Files:
routers/root.py
🧬 Code graph analysis (1)
routers/root.py (2)
widgets/sidebar.py (4)
sidebar_layout(210-325)use_sidebar(33-60)set_open(21-22)set_mobile_open(24-26)daras_ai_v2/base.py (2)
render_sidebar(419-465)render(370-417)
⏰ 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 (2)
routers/root.py (2)
48-48: Sidebar wiring for recipe pages looks correctImporting
sidebar_layout/use_sidebarand callingpage_wrapper(request, page=page, is_recipe_page=True)inrender_recipe_pagecleanly ties the sidebar state to the concretepageinstance; routing and meta-generation remain unchanged and look sound.Also applies to: 705-712
841-847: Mobile search button container change looks goodUsing
className="d-flex d-md-none justify-content-end"for the mobile search button container keeps it mobile-only while ensuring proper flex alignment; no functional issues here.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
daras_ai_v2/base.py (2)
1291-1310: Remove legacy commentedrender_gooey_builderblockThe fully commented
render_gooey_builder(...)block is now redundant given the inline embed path above and adds noise. It’s also the only place in this file that referencesrender_gooey_builder, so keeping it around can confuse future readers.Consider deleting the entire commented block:
- # render_gooey_builder( - # page_slug=self.slug_versions[-1], - # builder_state=dict( - # status=dict( - # error_msg=gui.session_state.get(StateKeys.error_msg), - # run_status=gui.session_state.get(StateKeys.run_status), - # run_time=gui.session_state.get(StateKeys.run_time), - # ), - # request=extract_model_fields( - # model=self.RequestModel, state=gui.session_state - # ), - # response=extract_model_fields( - # model=self.ResponseModel, state=gui.session_state - # ), - # metadata=dict( - # title=self.current_pr.title, - # description=self.current_pr.notes, - # ), - # ), - # )(You can then drop the
render_gooey_builderimport at Line 48 if it’s unused elsewhere.)
419-466: Close handler should also clear mobile sidebar stateIn
render_sidebar, the"onCloseGooeyBuilder"path (Line 441) only callssidebar_ref.set_open(False). If the sidebar was opened viais_mobile_open, the mobile flag remainsTruein the session, so the sidebar can get out of sync or immediately re-open on mobile. Thetab != run/previewbranch correctly clears both flags; the onClose handler should do the same.Recommend updating the onClose branch:
- if gui.session_state.pop("onCloseGooeyBuilder", None): - sidebar_ref.set_open(False) - raise gui.RerunException() + if gui.session_state.pop("onCloseGooeyBuilder", None): + sidebar_ref.set_open(False) + sidebar_ref.set_mobile_open(False) + raise gui.RerunException()Also double-check any other future close-only branches to ensure both flags are reset.
🧹 Nitpick comments (1)
workspaces/models.py (1)
177-177: Consider addinghelp_textfor admin clarity.The
featuresfield is a generic JSON bag. Adding ahelp_textwould help administrators understand its intended use and expected structure.- features = models.JSONField(default=dict) + features = models.JSONField(default=dict, help_text="Workspace feature flags and configuration")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
daras_ai_v2/base.py(5 hunks)usage_costs/migrations/0038_alter_modelpricing_model_name.py(1 hunks)workspaces/admin.py(3 hunks)workspaces/migrations/0013_workspace_features.py(1 hunks)workspaces/models.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{py,js,ts,tsx,java,cs,cpp,c,go,rb,php}
📄 CodeRabbit inference engine (.cursor/rules/devs-rules.mdc)
Format code in reverse topological order: place the main() function at the top and dependencies below it
Files:
daras_ai_v2/base.pyworkspaces/models.pyworkspaces/admin.pyusage_costs/migrations/0038_alter_modelpricing_model_name.pyworkspaces/migrations/0013_workspace_features.py
🧬 Code graph analysis (2)
daras_ai_v2/base.py (2)
daras_ai_v2/gooey_builder.py (2)
render_gooey_builder(71-126)render_gooey_builder_inline(7-68)widgets/sidebar.py (4)
sidebar_layout(210-325)use_sidebar(33-60)set_open(21-22)set_mobile_open(24-26)
workspaces/admin.py (1)
gooeysite/custom_widgets.py (1)
JSONEditorWidget(4-26)
🪛 GitHub Actions: Python tests
workspaces/models.py
[error] 175-177: ruff format would reformat 1 file; run 'ruff format' to fix code style issues.
🪛 Ruff (0.14.6)
workspaces/admin.py
135-137: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
usage_costs/migrations/0038_alter_modelpricing_model_name.py
8-10: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
12-298: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
workspaces/migrations/0013_workspace_features.py
8-10: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
12-18: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
🔇 Additional comments (6)
workspaces/admin.py (2)
135-138: LGTM!The
formfield_overridesconfiguration correctly mapsJSONFieldtoJSONEditorWidget, providing a better editing experience for JSON data in the admin. The static analysis warning (RUF012) about mutable class attributes is a false positive here—this is the standard Django admin pattern and the dict is not mutated at runtime.
86-108: LGTM!The
featuresfield is appropriately placed in the admin fields list and is editable, which is correct for administrator-managed feature configuration.workspaces/migrations/0013_workspace_features.py (1)
1-18: LGTM!The migration is correctly generated and aligns with the model change. It properly uses
default=dict(callable) to avoid mutable default issues. The static analysis warnings (RUF012) are false positives—Django migrations use class attributes fordependenciesandoperationsby design.daras_ai_v2/base.py (3)
48-48: Inline builder import matches new usageImporting
render_gooey_builder_inlinehere aligns with_render_gooey_builder’s new inline usage; the change is consistent and correct.
85-85: Sidebar hooks wired into BasePageBringing in
use_sidebarforrender_sidebaris appropriate and keeps sidebar state tied to the Django session; no issues from this change in this file.
1251-1289: Filtered builder_state with inline embed avoids leaking full session
_render_gooey_buildernow:
- Pops and applies
update_gui_statein a constrained way viafields_to_save().- Gates access to admins only (
is_current_user_admin()).- Calls
render_gooey_builder_inlinewith a filteredbuilder_statecomposed of:
status(error_msg,run_status,run_time)request/responsefromextract_model_fields- Minimal
metadata(title, description)This addresses the earlier concern about passing the entire
gui.session_stateto the client widget and keeps the payload scoped to what the builder needs.
| ("protogen_2_2", "Protogen V2.2 (darkstorm2150)"), | ||
| ("epicdream", "epiCDream [Deprecated] (epinikion)"), | ||
| ("flux_1_dev", "FLUX.1 [dev]"), | ||
| ("dream_shaper", "DreamShaper (Lykon)"), | ||
| ("dreamlike_2", "Dreamlike Photoreal 2.0 (dreamlike.art)"), | ||
| ("sd_2", "Stable Diffusion v2.1 (stability.ai)"), | ||
| ("sd_1_5", "Stable Diffusion v1.5 (RunwayML)"), | ||
| ("dall_e", "DALL·E 2 (OpenAI)"), | ||
| ("dall_e_3", "DALL·E 3 (OpenAI)"), | ||
| ("gpt_image_1", "GPT Image 1 (OpenAI)"), | ||
| ("nano_banana", "Nano Banana (Google)"), | ||
| ("openjourney_2", "Open Journey v2 beta [Deprecated] (PromptHero)"), | ||
| ("openjourney", "Open Journey [Deprecated] (PromptHero)"), | ||
| ("analog_diffusion", "Analog Diffusion [Deprecated] (wavymulder)"), | ||
| ("protogen_5_3", "Protogen v5.3 [Deprecated] (darkstorm2150)"), | ||
| ("jack_qiao", "Stable Diffusion v1.4 [Deprecated] (Jack Qiao)"), | ||
| ( | ||
| "rodent_diffusion_1_5", | ||
| "Rodent Diffusion 1.5 [Deprecated] (NerdyRodent)", | ||
| ), | ||
| ("deepfloyd_if", "DeepFloyd IF [Deprecated] (stability.ai)"), | ||
| ("flux_pro_kontext", "FLUX.1 Pro Kontext (fal.ai)"), | ||
| ("dream_shaper", "DreamShaper (Lykon)"), | ||
| ("dreamlike_2", "Dreamlike Photoreal 2.0 (dreamlike.art)"), | ||
| ("sd_2", "Stable Diffusion v2.1 (stability.ai)"), | ||
| ("sd_1_5", "Stable Diffusion v1.5 (RunwayML)"), | ||
| ("dall_e", "Dall-E (OpenAI)"), | ||
| ("gpt_image_1", "GPT Image 1 (OpenAI)"), | ||
| ("nano_banana", "Nano Banana (Google)"), | ||
| ("instruct_pix2pix", "✨ InstructPix2Pix (Tim Brooks)"), | ||
| ("openjourney_2", "Open Journey v2 beta [Deprecated] (PromptHero)"), | ||
| ("openjourney", "Open Journey [Deprecated] (PromptHero)"), | ||
| ("analog_diffusion", "Analog Diffusion [Deprecated] (wavymulder)"), | ||
| ("protogen_5_3", "Protogen v5.3 [Deprecated] (darkstorm2150)"), | ||
| ("jack_qiao", "Stable Diffusion v1.4 [Deprecated] (Jack Qiao)"), | ||
| ( | ||
| "rodent_diffusion_1_5", | ||
| "Rodent Diffusion 1.5 [Deprecated] (NerdyRodent)", | ||
| ), | ||
| ("sd_2", "Stable Diffusion v2.1 (stability.ai)"), | ||
| ("runway_ml", "Stable Diffusion v1.5 (RunwayML)"), | ||
| ("dall_e", "Dall-E (OpenAI)"), | ||
| ("jack_qiao", "Stable Diffusion v1.4 [Deprecated] (Jack Qiao)"), | ||
| ("wav2lip", "LipSync (wav2lip)"), | ||
| ("sadtalker", "LipSync (sadtalker)"), | ||
| ("twilio_voice", "Twilio Voice"), | ||
| ("twilio_sms", "Twilio SMS"), |
There was a problem hiding this comment.
Duplicate choices entries for several models in tail of list
The choices list for model_name has multiple duplicate keys and labels in the image-model section (e.g., dream_shaper, dreamlike_2, sd_2, sd_1_5, dall_e, gpt_image_1, nano_banana, openjourney_2, openjourney, analog_diffusion, protogen_5_3, jack_qiao, rodent_diffusion_1_5 appear more than once).
This won’t break the migration, but it will:
- Produce duplicate options in admin/forms.
- Make
get_model_name_display()effectively depend on the first occurrence only, which is confusing and brittle.
I’d recommend deduping these choices at the source definition (likely in usage_costs.models.ModelPricing.model_name or a shared choices constant), then regenerating the migration so that each model key appears exactly once with a single canonical label.
workspaces/models.py
Outdated
| features = models.JSONField(default=dict) | ||
| class Meta: |
There was a problem hiding this comment.
Add a blank line before class Meta.
The pipeline failure indicates a formatting issue. PEP 8 and ruff expect a blank line before nested class definitions within a class body.
Apply this diff to fix the formatting:
objects = WorkspaceQuerySet.as_manager()
features = models.JSONField(default=dict)
+
class Meta:📝 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.
| features = models.JSONField(default=dict) | |
| class Meta: | |
| objects = WorkspaceQuerySet.as_manager() | |
| features = models.JSONField(default=dict) | |
| class Meta: |
🤖 Prompt for AI Agents
In workspaces/models.py around lines 177-178, there is no blank line before the
nested class Meta; add a single blank line between the fields (features =
models.JSONField(...)) and the line declaring "class Meta:" so the nested class
is separated per PEP8/ruff formatting rules; run the formatter/ruff to confirm
the change.
95bb0b4 to
05b09e5
Compare
anish-work
force-pushed
the
bot_builder_sidebar
branch
from
05b09e5 to
4e23759
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
routers/root.py (3)
734-734: Type annotation still needs correction despite being marked as addressed.The past review comment flagged
page: None = Noneas incorrect because it only allowsNoneas a value. While marked as addressed in earlier commits, the provided code still shows this issue. The parameter should accept eitherNoneor a page instance.Apply the suggested fix from the previous review:
def page_wrapper( request: Request, - page: None = None, + page: typing.Optional["BasePage"] = None, className="", search_filters: typing.Optional[SearchFilters] = None, show_search_bar: bool = True, is_recipe_page: bool = False, ):Note: You may need to add
from __future__ import annotationsat the top of the file or importBasePagedirectly if not already present.
748-752: Mobile sidebar state not considered (duplicate of previous review).The logic on line 748 sets
is_builder_sidebar_open = sidebar_ref.is_openbut ignoressidebar_ref.is_mobile_open. This was flagged in a previous review as causing mobile-open sidebars to remain visually open on non-recipe pages.Apply the suggested fix from the previous review:
- is_builder_sidebar_open = sidebar_ref.is_open - if not is_recipe_page and (is_builder_sidebar_open): + is_builder_sidebar_open = sidebar_ref.is_open or sidebar_ref.is_mobile_open + if not is_recipe_page and is_builder_sidebar_open: sidebar_ref.set_open(False) sidebar_ref.set_mobile_open(False) raise gui.RerunException()
839-845: className concatenation issue still present (duplicate of previous review).The past review flagged that when the sidebar is closed, the
classNameparameter is dropped, and when open, spacing before custom classes is missing. While marked as addressed, the provided code still shows this issue.Apply the suggested fix from the previous review:
with gui.div( id="main-content", - className="container-xxl " - if not is_builder_sidebar_open - else "mx-2 w-100" + className, + className=( + "container-xxl " + className + if not is_builder_sidebar_open + else "mx-2 w-100 " + className + ), ):widgets/sidebar.py (1)
296-310: Click-to-open handler issue still present (duplicate of previous review).The past review flagged that the onClick handler checks
event.target.id === "sidebar-click-container"but:
- The wrapper div has no
idattributeevent.targetrefers to the clicked element (which may be a child), not the containerThis prevents the handler from firing correctly.
Apply the suggested fix from the previous review:
gui.div( + id="sidebar-click-container", className="d-flex w-100 h-100 position-relative sidebar-click-container", style={"height": "100dvh"}, onClick=dedent( """ - if (event.target.id === "sidebar-click-container") { + if (event.currentTarget.id === "sidebar-click-container") { document.getElementById("sidebar-hidden-btn").click(); } """ if not sidebar_ref.is_open else "" ), ),
🧹 Nitpick comments (5)
daras_ai_v2/gooey_builder.py (1)
7-126: Consider refactoring to eliminate code duplication.The
render_gooey_builder_inlineandrender_gooey_builderfunctions share approximately 90% identical code. Only themode,showRunLink, div style, andonClosehandler differ between them.🔎 Consider this refactoring approach:
Extract common logic into a helper function and parameterize the differences:
def _render_gooey_builder_common( page_slug: str, builder_state: dict, mode: str = "default", show_run_link: bool = False, div_style: str = "", on_close_handler: str = "" ): """Common logic for rendering Gooey builder.""" if not settings.GOOEY_BUILDER_INTEGRATION_ID: return bi = BotIntegration.objects.get(id=settings.GOOEY_BUILDER_INTEGRATION_ID) config = bi.get_web_widget_config( hostname="gooey.ai", target="#gooey-builder-embed" ) config["mode"] = mode if show_run_link: config["showRunLink"] = True branding = config.setdefault("branding", {}) branding["showPoweredByGooey"] = False config.setdefault("payload", {}).setdefault("variables", {}) variables = dict( update_gui_state_params=dict(state=builder_state, page_slug=page_slug), ) gui.html( f""" <div id="gooey-builder-embed" {div_style}></div> <script id="gooey-builder-embed-script" src="{settings.WEB_WIDGET_LIB}"></script> """ ) mount_call = "GooeyEmbed.mount(config);" if not on_close_handler else f""" config.onClose = function() {{ document.getElementById("onClose").click(); }}; GooeyEmbed.mount(config); """ gui.js( f""" async function onload() {{ await window.waitUntilHydrated; if (typeof GooeyEmbed === "undefined" || document.getElementById("gooey-builder-embed")?.children.length) return; GooeyEmbed.setGooeyBuilderVariables = (value) => {{ config.payload.variables = value; }}; GooeyEmbed.setGooeyBuilderVariables(variables); {mount_call} }} const script = document.getElementById("gooey-builder-embed-script"); if (script) script.onload = onload; onload(); window.addEventListener("hydrated", onload); if (typeof GooeyEmbed !== "undefined" && GooeyEmbed.setGooeyBuilderVariables) {{ GooeyEmbed.setGooeyBuilderVariables(variables); }} """, config=config, variables=variables, ) def render_gooey_builder_inline(page_slug: str, builder_state: dict): _render_gooey_builder_common( page_slug, builder_state, mode="inline", show_run_link=True, div_style='style="height: 100%"', on_close_handler="onClose" ) def render_gooey_builder(page_slug: str, builder_state: dict): _render_gooey_builder_common(page_slug, builder_state)routers/root.py (1)
744-744: Unused variable assignment.The variable
containeris assigned on line 744 but is never used in the function. It appears this was meant to be used somewhere but was replaced by direct access topage.🔎 Apply this diff to remove the unused variable:
- container = page if page else None sidebar_ref = use_sidebar("builder-sidebar", request.session, default_open=False)widgets/sidebar.py (3)
24-26: Remove or document commented code.Line 26 contains commented code
# self.set_open(value). This suggests uncertainty about whether setting mobile open should also set the regular open state. Either remove this if it's not needed, or add a comment explaining why this relationship is intentionally decoupled.
128-128: Remove unused loop variable.The loop variable
iis declared but never used in the loop body. This is flagged by static analysis.🔎 Apply this diff:
- for i, (url, label, icon) in enumerate(settings.SIDEBAR_LINKS): + for url, label, icon in settings.SIDEBAR_LINKS:Or if you anticipate using the index in the future:
- for i, (url, label, icon) in enumerate(settings.SIDEBAR_LINKS): + for _i, (url, label, icon) in enumerate(settings.SIDEBAR_LINKS):Based on learnings, static analysis hints should be addressed when valid.
44-46: Usepop()instead of checking existence then deleting.Static analysis correctly identifies that the pattern of checking if a key exists and then deleting it can be simplified using
pop().🔎 Apply this diff:
- mobile_key = key + ":mobile" - if mobile_key in session: - del session[mobile_key] + session.pop(key + ":mobile", None)This is more concise and thread-safe.
Based on learnings, static analysis hints should be addressed when valid.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
daras_ai_v2/base.py(5 hunks)daras_ai_v2/gooey_builder.py(1 hunks)daras_ai_v2/settings.py(1 hunks)routers/root.py(3 hunks)widgets/sidebar.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- daras_ai_v2/settings.py
- daras_ai_v2/base.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{py,js,ts,tsx,java,cs,cpp,c,go,rb,php}
📄 CodeRabbit inference engine (.cursor/rules/devs-rules.mdc)
Format code in reverse topological order: place the main() function at the top and dependencies below it
Files:
daras_ai_v2/gooey_builder.pyrouters/root.pywidgets/sidebar.py
🧬 Code graph analysis (1)
daras_ai_v2/gooey_builder.py (1)
bots/models/bot_integration.py (2)
BotIntegration(163-626)get_web_widget_config(565-592)
🪛 Ruff (0.14.8)
widgets/sidebar.py
46-46: Use pop instead of key in dict followed by del dict[key]
Replace if statement with .pop(..., None)
(RUF051)
128-128: 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 (4)
routers/root.py (2)
754-757: LGTM!The conditional sidebar rendering logic correctly checks for a page instance before calling
render_sidebar().
796-808: LGTM!The Gooey builder mobile button is properly gated for admins, correctly updates the sidebar state, and triggers a rerun to reflect the changes.
widgets/sidebar.py (1)
169-202: LGTM!The mobile header correctly handles the button click to open the sidebar and triggers a rerun to reflect the state change.
daras_ai_v2/gooey_builder.py (1)
50-52: No action needed. The element with id="onClose" is properly defined in the calling context. Inbase.py(lines 445-449), a hidden button withid="onClose"is explicitly rendered when the sidebar is open, before the gooey_builder code executes. The callback will successfully locate and trigger this element.
| # Check if this is a fresh page load by comparing timestamps | ||
| last_load_time = session.get(f"{key}:last_load_time", 0) | ||
| current_time = time.time() | ||
|
|
||
| # 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] | ||
|
|
||
| # Update the last load time | ||
| session[f"{key}:last_load_time"] = current_time |
There was a problem hiding this comment.
Time-based page load detection is fragile and unreliable.
The logic uses a 0.5-second threshold to determine if a page load is "fresh" and should clear mobile state. This approach has several issues:
- Fast navigations (< 0.5s) won't clear mobile state, causing stale state to persist
- Multiple rapid requests could interfere with each other
- The threshold is arbitrary and may not work reliably across different network conditions
🔎 Consider a more robust approach:
Use a navigation counter or request ID instead of time-based detection:
def use_sidebar(key: str, session: dict, default_open: bool = True) -> SidebarRef:
"""Create or get a sidebar reference with state management."""
# Use a navigation counter instead of time
nav_key = f"{key}:nav_count"
current_nav = session.get(nav_key, 0)
# Increment on each call - fresh page loads will have new session
# or can be explicitly reset by caller
if f"{key}:mobile" in session and current_nav == 0:
# First call in this session - clear mobile state
del session[f"{key}:mobile"]
session[nav_key] = current_nav + 1
# set the default open state in session here
session[key] = bool(session.get(key, default_open))
ref = SidebarRef(
key=key,
session=session,
is_open=bool(session.get(key, default_open)),
is_mobile_open=bool(session.get(key + ":mobile", False)),
)
return refAlternatively, use a page load marker that the caller explicitly resets, or leverage existing session management patterns in your framework.
🧰 Tools
🪛 Ruff (0.14.8)
46-46: Use pop instead of key in dict followed by del dict[key]
Replace if statement with .pop(..., None)
(RUF051)
🤖 Prompt for AI Agents
In widgets/sidebar.py around lines 37 to 49, replace the fragile time-based
fresh-load detection with a deterministic navigation counter or request
identifier stored in session; add a nav_key like f"{key}:nav_count", read
current_nav = session.get(nav_key, 0), clear the mobile state only when
current_nav == 0 (or when a new request id differs), then increment and store
session[nav_key] = current_nav + 1, and finally initialize session[key] and the
SidebarRef is_open/is_mobile_open from session; ensure no reliance on time
thresholds and document that callers can reset the nav counter if they need to
force a fresh load.
|
@devxpy cleaned up, you can review now.. |
| ## HUGE HACK HERE | ||
| # Check if this is a fresh page load by comparing timestamps | ||
| last_load_time = session.get(f"{key}:last_load_time", 0) | ||
| current_time = time.time() |
There was a problem hiding this comment.
@devxpy Having trouble to manage mobile open and close state as:
- We don't want to remember the open state in mobile while in large devices we want to keep it open
Hence used this workaround works fine but not sure if its reliable though
anish-work
force-pushed
the
bot_builder_sidebar
branch
from
1384057 to
477aa34
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
widgets/sidebar.py (1)
34-47: Time-based fresh-load detection remains fragile.This logic uses a 0.5-second threshold to detect fresh page loads, which can be unreliable under varying network conditions or fast navigations.
routers/root.py (2)
751-755: Include mobile-open state in sidebar open check.
is_builder_sidebar_openonly checkssidebar_ref.is_open, ignoringsidebar_ref.is_mobile_open. This can leave a visually open but empty sidebar on mobile when navigating to non-recipe pages.🔎 Apply this diff:
- is_builder_sidebar_open = sidebar_ref.is_open - if not is_recipe_page and is_builder_sidebar_open: + is_builder_sidebar_open = sidebar_ref.is_open or sidebar_ref.is_mobile_open + if not is_recipe_page and is_builder_sidebar_open: sidebar_ref.set_open(False) sidebar_ref.set_mobile_open(False) raise gui.RerunException()
842-847: Fix className concatenation (parameter ignored when sidebar is closed).When the sidebar is closed, the
classNameparameter is dropped. When open, a space is missing before the custom classes.🔎 Apply this diff:
with gui.div( id="main-content", - className="container-xxl " - if not is_builder_sidebar_open - else "mx-2 w-100" + className, + className=( + "container-xxl " + className + if not is_builder_sidebar_open + else "mx-2 w-100 " + className + ), ):daras_ai_v2/base.py (1)
1257-1259: Close both desktop and mobile sidebar states on builder close.When the builder's
onClosefires, onlyset_open(False)is called. If the sidebar was opened via mobile,is_mobile_openremainsTrue, causing the sidebar to stay open or get out of sync.🔎 Apply this diff:
if gui.session_state.pop("onCloseGooeyBuilder", None): sidebar_ref.set_open(False) + sidebar_ref.set_mobile_open(False) raise gui.RerunException()
🧹 Nitpick comments (1)
widgets/sidebar.py (1)
42-44: Usepop()instead ofif key in dictfollowed bydel.As suggested by static analysis, this can be simplified.
🔎 Apply this diff:
- mobile_key = key + ":mobile" - if mobile_key in session: - del session[mobile_key] + session.pop(key + ":mobile", None)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
daras_ai_v2/base.py(4 hunks)daras_ai_v2/gooey_builder.py(5 hunks)routers/root.py(3 hunks)widgets/sidebar.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{py,js,ts,tsx,java,cs,cpp,c,go,rb,php}
📄 CodeRabbit inference engine (.cursor/rules/devs-rules.mdc)
Format code in reverse topological order: place the main() function at the top and dependencies below it
Files:
daras_ai_v2/base.pywidgets/sidebar.pydaras_ai_v2/gooey_builder.pyrouters/root.py
🪛 GitHub Actions: Python tests
widgets/sidebar.py
[error] 57-57: Ruff formatting would reformat this file. Please run 'ruff format --diff .' to fix formatting issues.
daras_ai_v2/gooey_builder.py
[error] 65-65: Ruff formatting would reformat this file. Please run 'ruff format --diff .' to fix formatting issues.
🪛 Ruff (0.14.8)
widgets/sidebar.py
44-44: Use pop instead of key in dict followed by del dict[key]
Replace if statement with .pop(..., None)
(RUF051)
🔇 Additional comments (8)
daras_ai_v2/gooey_builder.py (1)
7-68: Implementation looks good overall.The inline builder wiring correctly:
- Sets mode to "inline" with showRunLink enabled
- Defines a reusable
setGooeyBuilderVariablesfunction for state updates- Wires the
onClosehandler to trigger the hidden button click in the parent pagewidgets/sidebar.py (1)
66-163: Sidebar layout implementation looks good.The CSS styling properly handles responsive behavior with media queries for mobile viewports. The layout returns appropriate placeholders for sidebar and main content.
routers/root.py (2)
53-55: Good use of TYPE_CHECKING for forward reference.The
BasePageimport underTYPE_CHECKINGavoids circular import issues while providing proper type hints.
799-811: Admin-only mobile builder button is well implemented.The mobile open button is correctly restricted to admin users and triggers a rerun after setting the mobile open state.
daras_ai_v2/base.py (4)
429-431: New render_sidebar method properly delegates to builder rendering.The sidebar rendering is cleanly delegated to
_render_gooey_builder().
1280-1299: Good: Builder state is now properly filtered.The
builder_stateis correctly constructed with only the necessary fields (status, request, response, metadata) rather than exposing the entiresession_state. This addresses the previous security concern.
1262-1278: Verify state merging doesn't lose important session data.When
update_gui_stateis received, the code clearsgui.session_stateand repopulates it with onlyfields_to_save()plus the update. Ensure no critical session data (outside of fields_to_save) is inadvertently lost during this operation.
1300-1319: Floating builder button implementation is well done.The floating button is properly:
- Hidden on mobile (
d-none d-md-block) since mobile uses a header button- Positioned fixed at bottom-left with appropriate z-index
- Styled with hover scale effect for better UX
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
daras_ai_v2/gooey_builder.py (1)
65-68: Fix Ruff formatting issue (already flagged).The Ruff formatting issue at the end of this function has already been flagged in previous reviews. Please run
ruff format .to fix.widgets/sidebar.py (2)
34-47: Time-based page load detection is fragile (already flagged).The time-based hack using a 0.5-second threshold to detect fresh page loads has already been flagged as a major issue in previous reviews. This approach is unreliable across different network conditions and navigation patterns.
56-58: Fix Ruff formatting issue (already flagged).The Ruff formatting issue at line 57 has already been flagged in previous reviews and pipeline failures. Please run
poetry run ruff format .to fix.
🧹 Nitpick comments (1)
widgets/sidebar.py (1)
43-44: Consider usingpop()for more idiomatic dict key removal.The pattern
if key in dictfollowed bydel dict[key]can be replaced withsession.pop(mobile_key, None)for more concise and idiomatic code.🔎 Apply this diff:
- if mobile_key in session: - del session[mobile_key] + session.pop(mobile_key, None)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
daras_ai_v2/gooey_builder.py(4 hunks)widgets/sidebar.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{py,js,ts,tsx,java,cs,cpp,c,go,rb,php}
📄 CodeRabbit inference engine (.cursor/rules/devs-rules.mdc)
Format code in reverse topological order: place the main() function at the top and dependencies below it
Files:
daras_ai_v2/gooey_builder.pywidgets/sidebar.py
🪛 GitHub Actions: Python tests
widgets/sidebar.py
[error] 57-57: Ruff formatting check failed. 1 file would be reformatted by 'ruff format --diff .'. Please run 'poetry run ruff format .' to fix.
🪛 Ruff (0.14.8)
widgets/sidebar.py
44-44: Use pop instead of key in dict followed by del dict[key]
Replace if statement with .pop(..., None)
(RUF051)
🔇 Additional comments (5)
daras_ai_v2/gooey_builder.py (3)
7-19: LGTM! Inline mode configuration is correct.The function rename to
render_gooey_builder_inlineclarifies the inline usage pattern, and the configuration correctly enables inline mode withshowRunLinkand appropriate branding settings.
30-30: LGTM! Height styling ensures proper container fill.The
height: 100%style ensures the embed widget fills its container, which is essential for the sidebar inline rendering.
50-52: The "onClose" element is correctly implemented. It is rendered in daras_ai_v2/base.py (line 1254) as a hidden button intentionally designed to trigger the onClose event, confirming the handler in gooey_builder.py properly references an existing element.widgets/sidebar.py (2)
5-28: LGTM! SidebarRef class is well-structured.The
SidebarRefclass correctly manages sidebar state by keeping instance attributes synchronized with session storage through its setter methods. The design is clean and appropriate for this use case.
60-163: LGTM! Sidebar layout implementation is comprehensive.The
sidebar_layoutfunction correctly implements responsive sidebar behavior with:
- Well-defined width constants
- Comprehensive CSS with proper media queries for mobile (max-width: 990px and 767px)
- Appropriate sticky positioning for sidebar
- Correct placeholder elements for composing the two-pane layout
The implementation handles both desktop and mobile states cleanly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
widgets/sidebar.py (4)
23-23: Remove or clarify the commented code.The commented line
# self.set_open(value)suggests uncertainty about whether mobile state should sync desktop state. Either uncomment with a clear reason, or remove if no longer needed.
43-44: Apply Ruff suggestion to simplify dictionary cleanup.Use
.pop(mobile_key, None)instead of checking key existence followed by deletion.🔎 Proposed fix
- if mobile_key in session: - del session[mobile_key] + session.pop(mobile_key, None)Based on static analysis hints.
50-55: Simplify redundant default_open evaluation.Line 50 sets
session[key]to the resolved boolean value, then lines 54-55 re-evaluate the same logic. Store the result once and reuse it.🔎 Proposed refactor
# set the default open state in session here - session[key] = bool(session.get(key, default_open)) + is_open_value = bool(session.get(key, default_open)) + session[key] = is_open_value ref = SidebarRef( key=key, session=session, - is_open=bool(session.get(key, default_open)), + is_open=is_open_value, is_mobile_open=bool(session.get(key + ":mobile", False)), )
69-69: Fix typo in variable name.
sidebar_funtion_classesshould besidebar_function_classes(missing 'c' in "function").🔎 Proposed fix
- sidebar_funtion_classes = ( + sidebar_function_classes = ( "gooey-sidebar-open" if sidebar_ref.is_open or sidebar_ref.is_mobile_open else "gooey-sidebar-closed" )And update the reference on line 161:
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}", )
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
widgets/sidebar.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{py,js,ts,tsx,java,cs,cpp,c,go,rb,php}
📄 CodeRabbit inference engine (.cursor/rules/devs-rules.mdc)
Format code in reverse topological order: place the main() function at the top and dependencies below it
Files:
widgets/sidebar.py
🪛 Ruff (0.14.8)
widgets/sidebar.py
44-44: Use pop instead of key in dict followed by del dict[key]
Replace if statement with .pop(..., None)
(RUF051)
⏰ 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)
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.