feat: add sorting for explore search #738
Conversation
📝 Walkthrough""" WalkthroughThis set of changes introduces enhancements to the search and navigation experience, primarily within the explore and header components. The explore page is refactored to use a unified Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧠 Learnings (1)routers/root.py (1)Learnt from: nikochiko ⏰ 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)
🔇 Additional comments (16)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (4)
templates/google_one_tap_button.html (1)
24-29: Consider increasing mobile button width for better touch target.The mobile sign-in button width of 60px might be smaller than optimal for touch interactions. Consider increasing it to at least 64px to meet accessibility guidelines.
- google.accounts.id.renderButton(document.getElementById("g_id_signin_mobile"), { - shape: "rectangular", - width: 60, - type: "icon", - size: "large", - }); + google.accounts.id.renderButton(document.getElementById("g_id_signin_mobile"), { + shape: "rectangular", + width: 64, + type: "icon", + size: "large", + });daras_ai_v2/base.py (2)
357-363: Potential indentation issue with dedent and f-string.The f-string inside
dedent()might produce unexpected indentation. Consider moving string formatting outside dedent.- gui.write( - dedent(f""" - You currently don't have access to this resource. Please request access from the - {owner_workspace.display_name(current_user=self.request.user)} admin or sign in with another account. - You are logged in as {self.request.user.email or self.request.user.phone_number}. - """) - ) + workspace_name = owner_workspace.display_name(current_user=self.request.user) + user_identifier = self.request.user.email or self.request.user.phone_number + gui.write( + dedent(f""" + You currently don't have access to this resource. Please request access from the + {workspace_name} admin or sign in with another account. + You are logged in as {user_identifier}. + """).strip() + )
2454-2481: Consider refactoring complex authorization logic for better maintainability.The authorization logic is comprehensive but complex. Consider breaking it down into smaller, more focused methods for better readability and testability.
def is_user_authorized(self, user: AppUser | None = None) -> bool: if self.is_user_admin(user): return True sr, pr = self.current_sr_pr if pr.saved_run_id != sr.id: - # not published run - return ( - # free workspace: allow anyone - not sr.workspace.subscription_id - or ( - PricingPlan.from_sub(sr.workspace.subscription) - == PricingPlan.STARTER - ) - # paid workspace: allow members - or bool(user and sr.workspace in user.cached_workspaces) - ) + return self._is_unpublished_run_authorized(user, sr) + + return self._is_published_run_authorized(user, pr) - # published run - return ( - pr.is_approved_example - or ( - WorkflowAccessLevel(pr.public_access) - in (WorkflowAccessLevel.FIND_AND_VIEW, WorkflowAccessLevel.EDIT) - ) - # current user is in the same workspace - or bool(user and pr.workspace in user.cached_workspaces) - ) +def _is_unpublished_run_authorized(self, user: AppUser | None, sr: SavedRun) -> bool: + """Check authorization for unpublished runs.""" + # Free/starter workspace: allow anyone + if not sr.workspace.subscription_id: + return True + if PricingPlan.from_sub(sr.workspace.subscription) == PricingPlan.STARTER: + return True + # Paid workspace: allow members only + return bool(user and sr.workspace in user.cached_workspaces) + +def _is_published_run_authorized(self, user: AppUser | None, pr: PublishedRun) -> bool: + """Check authorization for published runs.""" + if pr.is_approved_example: + return True + if WorkflowAccessLevel(pr.public_access) in (WorkflowAccessLevel.FIND_AND_VIEW, WorkflowAccessLevel.EDIT): + return True + # Allow workspace members + return bool(user and pr.workspace in user.cached_workspaces)routers/root.py (1)
715-734: Consider using CSS classes instead of inline styles for better maintainability.While the current implementation works, toggling CSS classes would be more maintainable than directly setting display properties.
Instead of manipulating inline styles directly, consider adding/removing CSS classes:
def get_js_hide_mobile_search(): return dedent(""" event.preventDefault(); const hide_on_mobile_search = document.querySelectorAll('.hide_on_mobile_search'); const show_on_mobile_search = document.querySelectorAll('.show_on_mobile_search'); - hide_on_mobile_search.forEach(el => el.style.setProperty('display', 'flex')); - show_on_mobile_search.forEach(el => el.style.setProperty('display', 'none')); + hide_on_mobile_search.forEach(el => el.classList.remove('d-none')); + show_on_mobile_search.forEach(el => el.classList.add('d-none')); """)This approach would be more consistent with the Bootstrap utility classes used elsewhere in the code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
daras_ai_v2/base.py(7 hunks)daras_ai_v2/icons.py(2 hunks)daras_ai_v2/meta_content.py(1 hunks)daras_ai_v2/paypal.py(2 hunks)daras_ai_v2/settings.py(2 hunks)recipes/VideoBots.py(0 hunks)routers/account.py(2 hunks)routers/root.py(9 hunks)templates/google_one_tap_button.html(2 hunks)widgets/explore.py(5 hunks)widgets/saved_workflow.py(5 hunks)widgets/workflow_search.py(9 hunks)workspaces/widgets.py(6 hunks)
💤 Files with no reviewable changes (1)
- recipes/VideoBots.py
🧰 Additional context used
🧠 Learnings (6)
daras_ai_v2/settings.py (1)
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/icons.py (1)
Learnt from: nikochiko
PR: GooeyAI/gooey-server#703
File: daras_ai_v2/settings.py:309-309
Timestamp: 2025-06-16T11:40:10.683Z
Learning: The FontAwesome icon `fa-magnifying-glass` is available in both `fa-solid` and `fa-regular` styles. The user nikochiko confirmed this with documentation link https://fontawesome.com/icons/magnifying-glass?f=classic&s=regular
routers/account.py (1)
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).
workspaces/widgets.py (1)
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.
routers/root.py (1)
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.
widgets/saved_workflow.py (1)
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.
🧬 Code Graph Analysis (1)
routers/account.py (5)
daras_ai_v2/fastapi_tricks.py (2)
get_app_route_url(68-74)get_route_path(77-80)widgets/workflow_search.py (2)
SearchFilters(60-67)get_filter_value_from_workspace(271-272)daras_ai_v2/base.py (1)
current_workspace(1420-1425)workspaces/widgets.py (1)
get_current_workspace(192-203)routers/root.py (1)
explore_page(250-262)
⏰ 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 (32)
daras_ai_v2/icons.py (1)
25-25: LGTM! Icon additions support new UI features.The new icons are properly formatted with correct FontAwesome classes and are positioned logically within the file structure. The
searchicon usesfa-solid fa-magnifying-glasswhich is appropriate for UI elements.Also applies to: 28-28, 50-53
daras_ai_v2/paypal.py (1)
6-6: LGTM! Type annotation improvements enhance code clarity.The consolidated imports and explicit
ClassVarannotations for class-level constants are excellent improvements that enhance type safety without changing functionality.Also applies to: 21-22, 27-27
daras_ai_v2/settings.py (1)
309-309: LGTM! Removal of static explore link supports dynamic navigation.Clearing the
HEADER_ICONSdictionary aligns with the refactoring of explore page navigation to be more dynamic and context-aware, as mentioned in the PR objectives.routers/account.py (2)
16-16: LGTM! Import addition supports URL generation with query parameters.The
get_app_route_urlimport is necessary for constructing URLs with query parameters in the new redirect logic.
139-156: LGTM! Well-implemented workspace-scoped explore redirect.The new route handler correctly:
- Performs authentication checks with appropriate redirect to login
- Retrieves the current workspace for the authenticated user
- Creates search filters scoped to that workspace
- Redirects to the explore page with properly encoded filters
This implementation aligns with the PR objective of redirecting from the saved tab to the explore page with workspace filtering.
daras_ai_v2/meta_content.py (1)
220-238: LGTM! Authorization check enhances SEO and security.The new authorization check correctly prevents search engines from following or indexing pages that require authentication, while preserving the original tab-based logic for publicly accessible content. This aligns with the broader authorization improvements introduced in the PR.
templates/google_one_tap_button.html (1)
1-3: Good responsive design implementation.The separation of desktop and mobile sign-in buttons with responsive classes (
d-none d-md-inlineandd-md-none) is a clean approach for device-specific rendering.workspaces/widgets.py (5)
2-2: Good security practice with HTML escaping.Adding the
htmlmodule import to properly escape user-generated content prevents XSS vulnerabilities.
22-27: Excellent security and code quality improvements.The changes improve the code in multiple ways:
- Proper HTML escaping prevents XSS vulnerabilities
- Structured div approach is more maintainable than string concatenation
- Responsive styling with text truncation improves UI on smaller screens
Also applies to: 38-39, 60-67, 118-119
143-143: Appropriate breakpoint adjustment for better UX.Changing from
d-lg-nonetod-xl-noneallows header links to display on large screens, improving navigation accessibility.
133-158: Excellent refactoring to reduce code duplication.The refactoring to use
render_link_in_dropdownhelper function improves code maintainability while preserving the consistent column layout pattern.
163-190: Well-designed helper function following DRY principle.The
render_link_in_dropdownfunction excellently encapsulates the repeated dropdown link pattern while maintaining the requiredgui.columns([2, 10])layout for visual consistency. The optional parameters provide good flexibility.widgets/explore.py (3)
26-51: Well-structured meta tag generation with good SEO practices.The
build_meta_tagsfunction uses clean pattern matching and generates contextual titles that improve SEO. The defensive programming with default SearchFilters instance prevents null reference errors.
54-68: Smart UI improvements and defensive programming.Good improvements:
- Responsive heading visibility based on search context
- Copying
search_filtersprevents mutation side effects- Cleaner redirect logic with default exclusion reduces URL clutter
88-88: Good style consistency and UI enhancements.The changes improve code consistency by using
gui.divinstead ofgui.tag("div")and enhance the run count display with better styling and layout.Also applies to: 129-129, 148-154
daras_ai_v2/base.py (2)
366-368: Proper authorization gate implementation.The authorization check is correctly placed at the beginning of the render method with an early return pattern, preventing any unauthorized access to sensitive operations.
1477-1477: Good improvements for type safety and performance.Adding type hints improves code clarity, and using
select_relatedfor the PublishedRun query reduces database queries by eagerly loading related objects.Also applies to: 1487-1488
routers/root.py (5)
8-8: LGTM!The imports are properly organized and all appear to be used in the code.
Also applies to: 13-13, 38-40, 50-51
250-262: Good refactoring to consolidate query parameters!The use of a Pydantic model with FastAPI's
Query()annotation improves parameter handling and validation while making the code more maintainable.
698-713: Well-implemented search redirect logic!The function properly handles search state changes and URL updates using the framework's redirect mechanism.
737-741: LGTM!The addition of the optional
search_filtersparameter maintains backward compatibility while enabling search functionality.
808-809: Good improvements to header navigation!The breakpoint adjustments and refactoring to use the
render_link_in_dropdownhelper improve consistency and user experience.Also applies to: 815-822, 852-852, 862-869
widgets/saved_workflow.py (3)
27-27: Good use of keyword-only arguments!The addition of
*enforces keyword-only arguments, improving API clarity and preventing positional argument mistakes.Also applies to: 33-33, 87-87
135-161: Good CSS improvements for better layout flexibility!The flexbox approach and increased max-width for author names improve the responsive behavior and accommodate longer content.
166-228: Well-structured footer rendering improvements!The conditional display based on membership status and consistent muted text styling improve the UI hierarchy and information disclosure.
widgets/workflow_search.py (7)
60-68: LGTM!The SearchFilters model properly includes the new sort field and the boolean evaluation correctly considers all filter fields.
271-286: Well-designed workspace filter helpers!The functions provide a clean abstraction for converting between workspaces and filter values, properly handling both handle names and IDs.
76-133: Excellent responsive UI implementation!The conditional rendering based on login status and dynamic sort option display provide a clean, context-aware interface.
217-247: Smart placeholder text generation!The dynamic placeholder provides helpful context about the current search scope, improving user understanding.
363-382: Well-implemented sorting logic!The sort filter provides comprehensive ordering strategies for each sort option, with appropriate fallback ordering.
31-33: Fix incorrect decorator usage - cannot use @classmethod and @Property together.Using both decorators will cause a runtime error. Choose one approach:
Option 1 - Use @classmethod:
- @classmethod - @property - def default(cls) -> "SortOptions": + @classmethod + def default(cls) -> "SortOptions": return cls.FEATUREDOption 2 - Use class attribute:
- @classmethod - @property - def default(cls) -> "SortOptions": - return cls.FEATURED + DEFAULT = "featured"Then update usage from
SortOptions.defaulttoSortOptions.default()orSortOptions.DEFAULT.Likely an incorrect or invalid review comment.
329-344: Verification Complete:is_memberIs Properly AnnotatedI’ve confirmed that in
build_workflow_access_filterwe annotate
is_member=Q(membership__role__isnull=False)for authenticated users- default
is_member=Falsefor anonymous usersNo further changes are required here.
nikochiko
force-pushed
the
search-sort
branch
2 times, most recently
from
a7888fa to
ea18713
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
widgets/workflow_search.py (4)
30-33: Fix the decorator syntax issue on thedefaultproperty.The code combines
@classmethodand@propertydecorators incorrectly. A property cannot be a classmethod - these decorators are mutually exclusive.- @classmethod - @property - def default(cls) -> "SortOptions": - return cls.FEATURED + @classmethod + def default(cls) -> "SortOptions": + return cls.FEATURED
289-291: Fix the SortOptions.default reference after correcting the decorator.This code references
SortOptions.defaultwhich has the decorator issue identified earlier.sort_options = dict( - (opt.value if opt != SortOptions.default else "", opt) for opt in SortOptions + (opt.value if opt != SortOptions.default() else "", opt) for opt in SortOptions )
294-294: Fix the SortOptions.default reference in format_func.This line also references the problematic
SortOptions.defaultproperty.def format_func(v: str | None) -> str: - opt = sort_options.get(v, SortOptions.default) + opt = sort_options.get(v, SortOptions.default())
371-374: Fix the SortOptions.default reference in query building.Another instance of the decorator issue that needs to be addressed.
qs = build_sort_filter( qs, - search_filters.sort and SortOptions(search_filters.sort) or SortOptions.default, + search_filters.sort and SortOptions(search_filters.sort) or SortOptions.default(), )
🧹 Nitpick comments (2)
widgets/workflow_search.py (2)
207-228: Consider caching workspace lookups in placeholder generation.The function calls
get_workspace_from_filter_valueand performs workspace display name generation on every invocation, which could be expensive if called frequently.Consider memoizing or caching the workspace lookup result:
def get_placeholder_by_search_filters( search_filters: SearchFilters, current_user: AppUser | None, fallback_workspace_text: str = "Gooey.AI", ) -> str: from daras_ai_v2.all_pages import page_slug_map, normalize_slug text = "Search" - workspace = current_user and get_workspace_from_filter_value( - current_user, search_filters.workspace - ) + workspace = None + if current_user and search_filters.workspace: + # Consider caching this lookup if called frequently + workspace = get_workspace_from_filter_value( + current_user, search_filters.workspace + )
253-267: Optimize workspace lookup with early return and better iteration.The function iterates through all cached workspaces on every call. Consider using a dictionary lookup for better performance.
def get_workspace_from_filter_value( user: AppUser, value: str ) -> typing.Optional["Workspace"]: if not value: return None + # Try direct ID lookup first if value is numeric + if value.isdigit(): + workspace_id = int(value) + for w in user.cached_workspaces: + if w.id == workspace_id: + return w + + # Then try handle name lookup for w in user.cached_workspaces: - if str(w.id) == value or (w.handle_id and w.handle.name == value): + if w.handle_id and w.handle.name == value: return w return None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
daras_ai_v2/icons.py(2 hunks)routers/root.py(9 hunks)widgets/explore.py(4 hunks)widgets/workflow_search.py(9 hunks)workspaces/widgets.py(6 hunks)
🧬 Code Graph Analysis (1)
widgets/workflow_search.py (7)
app_users/models.py (2)
AppUser(90-262)cached_workspaces(242-249)bots/models/published_run.py (1)
PublishedRun(118-346)bots/models/workflow.py (5)
Workflow(189-257)short_title(230-232)page_cls(240-243)get_or_create_metadata(245-257)emoji(235-237)daras_ai_v2/grid_layout_widget.py (1)
grid_layout(25-38)widgets/saved_workflow.py (1)
render_saved_workflow_preview(24-91)workspaces/models.py (4)
Workspace(105-449)display_name(392-402)display_html(47-57)display_html(387-390)daras_ai_v2/all_pages.py (1)
normalize_slug(109-110)
✅ Files skipped from review due to trivial changes (1)
- daras_ai_v2/icons.py
🚧 Files skipped from review as they are similar to previous changes (3)
- widgets/explore.py
- workspaces/widgets.py
- routers/root.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
widgets/workflow_search.py (7)
app_users/models.py (2)
AppUser(90-262)cached_workspaces(242-249)bots/models/published_run.py (1)
PublishedRun(118-346)bots/models/workflow.py (5)
Workflow(189-257)short_title(230-232)page_cls(240-243)get_or_create_metadata(245-257)emoji(235-237)daras_ai_v2/grid_layout_widget.py (1)
grid_layout(25-38)widgets/saved_workflow.py (1)
render_saved_workflow_preview(24-91)workspaces/models.py (4)
Workspace(105-449)display_name(392-402)display_html(47-57)display_html(387-390)daras_ai_v2/all_pages.py (1)
normalize_slug(109-110)
⏰ 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 (6)
widgets/workflow_search.py (6)
76-85: Good refactoring of the conditional UI layout logic.The dynamic column class assignment based on filter visibility states improves the responsive layout handling.
369-369: Good refactoring to pass user context to search filter builder.Adding the
userparameter tobuild_search_filterenables proper workspace filtering based on user permissions.
379-398: Well-structured sorting implementation with comprehensive options.The sort filtering function provides good coverage of different sorting criteria with appropriate fallback ordering for each case.
442-444: Good integration of workspace filtering with user context.The workspace filter now properly uses the helper function to resolve workspace objects from filter values.
375-375: Confirm Clearing of Default Ordering on PublishedRunThe
PublishedRunmodel defines a default ordering (Meta.ordering = ["-updated_at"]) inbots/models/published_run.py(around line 181). The call atwidgets/workflow_search.py:375:qs = qs.order_by()will strip out that default ordering before any subsequent sorts. Please verify this is intentional. If you need to preserve the model’s default sort, either remove the
order_by()call or explicitly apply the desired fields after clearing.Locations to review:
bots/models/published_run.py(class Meta →ordering = ["-updated_at"])widgets/workflow_search.pyline 375 (qs = qs.order_by())
162-164: No performance issues detected in placeholder generationI’ve reviewed
get_placeholder_by_search_filtersand its helperget_workspace_from_filter_value. Both perform only:
- A cached import (Python memoizes modules after the first load)
- A simple O(n) loop over
current_user.cached_workspaces(n is typically small)- A dictionary lookup and string concatenations
These operations are millisecond-scale even for dozens of workspaces, so calling this on each render won’t introduce noticeable lag. No changes required.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
routers/root.py (1)
742-743: Import placement should be optimized.The import is placed inside the function, which can impact performance if called frequently. Consider moving this to the module level.
+from routers.account import explore_in_current_workspace + @contextmanager def page_wrapper( request: Request, className="", search_filters: typing.Optional[SearchFilters] = None, show_search_bar: bool = True, ): - from routers.account import explore_in_current_workspace - context = {"request": request, "block_incognito": True}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
routers/root.py(9 hunks)widgets/explore.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- widgets/explore.py
🧰 Additional context used
🧠 Learnings (1)
routers/root.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.
⏰ 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 (18)
routers/root.py (18)
8-8: Import addition looks good.The
textwrap.dedentimport is used appropriately in the new JavaScript helper functions below.
13-13: Import addition looks good.The
Queryimport from FastAPI is correctly used in the updatedexplore_pagefunction signature.
38-38: Import addition looks good.The
get_app_route_urlimport is used appropriately in the new search redirect functionality.
50-51: Import additions look good.Both imports are used correctly -
render_link_in_dropdownfor consistent dropdown link rendering andSearchFilters, render_search_barfor the new unified search functionality.
698-712: Search bar redirect helper function is well implemented.The function correctly handles search query changes and redirects using the FastAPI route URL generation. The logic is sound and follows the expected pattern.
714-733: JavaScript helper functions look good.Both mobile search show/hide functions correctly manipulate DOM classes and handle focus management. The use of
dedentimproves readability and the logic is sound.
736-741: Function signature enhancement looks good.The addition of
search_filtersandshow_search_barparameters provides the necessary flexibility for the new search functionality while maintaining backward compatibility with defaults.
753-756: Header container styling looks good.The position-relative and flexbox classes provide proper layout structure for the responsive header design.
757-775: Logo container structure is well organized.The responsive logo display logic correctly shows different logos for different screen sizes using Bootstrap classes.
776-806: Mobile search implementation is comprehensive.The conditional rendering, responsive classes, and JavaScript integration provide a solid mobile search experience. The toggle functionality and focus management are properly implemented.
807-812: Header container styling looks good.The max-width constraint and flexbox layout provide proper structure for header elements while preventing overflow from long workspace names.
813-819: Conditional explore link rendering looks good.The logic correctly shows the explore link when the search bar is hidden, maintaining consistent navigation options.
821-824: Header links refactoring looks good.The use of the new
render_header_linkhelper function provides consistent rendering and includes icon support from settings.
826-831: Saved link addition looks good.The new "Saved" link for authenticated users provides direct access to workspace-scoped saved runs, enhancing user experience.
860-861: Responsive class change looks good.The change from
d-lg-nonetod-xl-nonealigns with the consistent breakpoint adjustments mentioned in the AI summary.
870-883: Dropdown link refactoring is well implemented.The use of
render_link_in_dropdownhelper function provides consistent styling and behavior across all dropdown links, improving maintainability.
885-890: New header link helper function is well designed.The function provides consistent link rendering with optional icon support and proper responsive classes. The structure and implementation are sound.
250-262: Explore_page signature change is safe
I verified there are no direct calls toexplore_page—it’s only referenced by route helpers in:
routers/root.pyworkspaces/widgets.pyrouters/account.pySince these use
get_route_path/get_app_route_urland rely on the router’s dependency injection to supplySearchFilters, no further updates are needed.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
widgets/workflow_search.py (2)
286-317: Update usage after fixing the SortOptions.default decorator issue.The responsive sort menu implementation is well-designed, but lines 290 and 294 reference
SortOptions.defaultwhich has the decorator issue. After fixing the decorator problem, update these references accordingly.The responsive design with icons on mobile and full labels on desktop is excellent for UX.
372-375: Update usage after fixing the SortOptions.default decorator issue.Line 374 references
SortOptions.defaultwhich has the decorator issue. After fixing the decorator problem, update this reference accordingly.
🧹 Nitpick comments (1)
widgets/workflow_search.py (1)
70-116: Consider refactoring for better readability.The function works correctly but has complex conditional logic for determining column classes and rendering different UI components. The logic is sound but could benefit from extraction into smaller helper functions.
Consider extracting the column class determination logic:
def _get_filter_column_classes(show_workspace_filter: bool, show_sort_option: bool) -> tuple[str, str]: if not show_sort_option: return "col-12 col-md-7", "" elif show_workspace_filter: return "col-9 col-md-7", "col-3 col-md-5" else: return "col-7", "col-5"This would make the main function logic clearer and easier to test.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
widgets/saved_workflow.py(5 hunks)widgets/workflow_search.py(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- widgets/saved_workflow.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). (1)
- GitHub Check: test (3.10.12, 1.8.3)
🔇 Additional comments (7)
widgets/workflow_search.py (7)
60-67: LGTM!The
SearchFiltersmodel is well-structured with appropriate field types and the__bool__method correctly includes the newsortfield for comprehensive filter state checking.
119-196: LGTM!The updated function signature and integration with
SearchFiltersimproves the API design. The dynamic placeholder generation enhances user experience by providing contextual search hints.
199-228: Excellent UX enhancement!This function provides contextual search placeholders that help users understand their current search scope. The error handling for invalid workflow slugs and special handling for personal workspaces improves the user experience.
253-267: LGTM!These helper functions provide clean abstraction for workspace filtering, handling both workspace handles and IDs appropriately. The bidirectional conversion ensures consistency across the filtering system.
341-363: LGTM!The conditional display logic appropriately handles privacy concerns and provides relevant information based on user membership and current filters. The inline
_render_runfunction keeps the logic contained and readable.
379-398: Excellent sorting implementation with comprehensive fallback criteria.The sorting logic is well-designed with appropriate fallback criteria for each sort option. The FEATURED sort particularly shows thoughtful prioritization of approved examples, root workflows, and user-created content.
437-485: LGTM!The search filter implementation is robust with proper text search tokenization and sanitization. The integration with workspace helpers and full-text search using SearchVector provides comprehensive search capabilities.
nikochiko
force-pushed
the
search-sort
branch
2 times, most recently
from
e3b88eb to
77e2272
Compare
refactor: SearchFilters type now has defaults as "" instead of None
use position-absolute instead of setting display: none
to be consistent with `render_search_bar()`
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
widgets/workflow_search.py (3)
116-116: Update usage of SortOptions.default after fixing the decorator issue.This code references
SortOptions.defaultwhich has the decorator issue. Update based on the chosen fix approach.
379-379: Update usage of SortOptions.default after fixing the decorator issue.This code references
SortOptions.defaultwhich has the decorator issue. Update based on the chosen fix approach.
30-33: Fix the incompatible decorator combination.The
@classmethodand@propertydecorators cannot be combined. This will cause anAttributeErrorwhen trying to accessSortOptions.default.Choose one of these solutions:
Solution 1 (Recommended): Use a simple class method
- @classmethod - @property - def default(cls) -> "SortOptions": + @classmethod + def default(cls) -> "SortOptions": return cls.FEATUREDSolution 2: Use a class attribute
- @classmethod - @property - def default(cls) -> "SortOptions": - return cls.FEATURED + default = FEATURED # Define after the enum valuesThen update the usage in lines 116 and 379 to call
SortOptions.default()(for solution 1) or accessSortOptions.default(for solution 2).
🧹 Nitpick comments (1)
widgets/workflow_search.py (1)
292-306: Good abstraction for workspace filtering.The helper functions provide clean separation of concerns for workspace filter value handling. The logic correctly supports both handle names and workspace IDs.
Consider adding a null check for safety:
def get_filter_value_from_workspace(workspace: "Workspace") -> str: - return (workspace.handle_id and workspace.handle.name) or str(workspace.id) + return (workspace.handle_id and workspace.handle and workspace.handle.name) or str(workspace.id)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
widgets/workflow_search.py(9 hunks)
⏰ 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 (6)
widgets/workflow_search.py (6)
60-67: LGTM! Clean model extension.The addition of the
sortfield toSearchFiltersand its inclusion in the__bool__method is well-implemented and maintains consistency with the existing pattern.
157-235: Excellent enhancement to search bar functionality.The addition of dynamic placeholder text via
get_placeholder_by_search_filterssignificantly improves user experience by providing contextual search hints. The parameter handling and props management are well-implemented.
238-267: Well-designed helper function with clear logic.The dynamic placeholder generation enhances UX by providing contextual search hints. The fallback logic, workspace handling, and local import to avoid circular dependencies are all well-implemented.
346-368: Smart conditional display logic for run counts.The updated logic to show run counts when sorting by "Most Runs" even for non-members makes perfect sense from a UX perspective - users should see the data they're sorting by.
384-403: Comprehensive and well-designed sorting implementation.The sorting logic is thoughtfully implemented with appropriate fallback criteria for each option. The FEATURED sorting's multi-criteria approach prioritizing approved examples and root workflows is particularly well-designed.
442-490: Good integration of workspace filtering with existing search logic.The updated function signature and use of workspace helper functions integrates well with the existing comprehensive search implementation. The search logic remains robust with proper token sanitization and multi-field SearchVector coverage.
…_search, use GooeyEnum for enums, tweak workspace popper button padding
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.