Explore page fix #715
Explore page fix #715
Conversation
WalkthroughThe changes introduce several UI and logic adjustments across three modules. In Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
✨ Finishing Touches
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: 0
🧹 Nitpick comments (3)
widgets/saved_workflow.py (1)
198-207: Consider improving the version notes structure.The version notes section has been moved and wrapped in a styled div. The implementation looks good, but there's a minor improvement opportunity.
- with gui.div( - className="text-truncate text-muted", - style=dict(maxWidth="200px"), - ): + with gui.div( + className="text-truncate text-muted", + style={"maxWidth": "200px"}, + ):This follows the static analysis suggestion to use dictionary literal instead of
dict()call.widgets/workflow_search.py (1)
185-224: Consider refactoring the complex conditional logic for better readability.The
_render_runfunction has grown quite complex with multiple conditional branches. While the logic appears correct, it would benefit from some refactoring to improve maintainability.Consider extracting helper functions:
def _render_run(pr: PublishedRun): workflow = Workflow(pr.workflow) # Extract UI display flags display_flags = _determine_display_flags(pr, user, search_filters) render_saved_workflow_preview( workflow.page_cls, pr, workflow_pill=f"{workflow.get_or_create_metadata().emoji} {workflow.short_title}", hide_visibility_pill=True, **display_flags ) def _determine_display_flags(pr: PublishedRun, user: AppUser | None, search_filters: SearchFilters) -> dict: """Determine which UI elements should be shown/hidden based on user context.""" show_workspace_author = not bool(search_filters and search_filters.workspace) is_member = bool(getattr(pr, "is_member", False)) hide_last_editor = bool(pr.workspace_id and not is_member) hide_updated_at = hide_last_editor show_all_run_counts = _should_show_all_run_counts( is_member, user, search_filters ) return { "show_workspace_author": show_workspace_author, "hide_last_editor": hide_last_editor, "hide_updated_at": hide_updated_at, "show_all_run_counts": show_all_run_counts, } def _should_show_all_run_counts(is_member: bool, user: AppUser | None, search_filters: SearchFilters) -> bool: """Check if all run counts should be shown (for workspace owners).""" if not (is_member and search_filters and search_filters.workspace and user and not user.is_anonymous): return False user_workspace_ids = {w.id for w in user.cached_workspaces} user_workspace_handles = {w.handle.name for w in user.cached_workspaces if w.handle} try: workspace_id = int(search_filters.workspace) return workspace_id in user_workspace_ids except ValueError: return search_filters.workspace in user_workspace_handleswidgets/explore.py (1)
139-155: Consider extracting the workspace ownership logic to a shared utility.The workspace ownership check logic is duplicated between this file and
widgets/workflow_search.py. Consider extracting this to a shared utility function to maintain DRY principles.Create a utility function in a shared module:
# In a shared utility module, e.g., utils/workspace.py def is_user_workspace_owner(user: AppUser | None, workspace_filter: str | None) -> bool: """Check if the current user owns the filtered workspace.""" if not (user and workspace_filter and not user.is_anonymous): return False user_workspace_ids = {w.id for w in user.cached_workspaces} user_workspace_handles = {w.handle.name for w in user.cached_workspaces if w.handle} try: workspace_id = int(workspace_filter) return workspace_id in user_workspace_ids except ValueError: return workspace_filter in user_workspace_handlesThen use it in both files:
show_all_counts = is_user_workspace_owner(user, search_filters.workspace)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
widgets/author.py(8 hunks)widgets/explore.py(3 hunks)widgets/saved_workflow.py(4 hunks)widgets/workflow_search.py(1 hunks)
🧰 Additional context used
🪛 Pylint (3.3.7)
widgets/saved_workflow.py
[refactor] 204-204: Consider using '{"maxWidth": '200px'}' instead of a call to 'dict'.
(R1735)
🔇 Additional comments (13)
widgets/saved_workflow.py (5)
32-34: LGTM: Clean parameter additions for UI control.The new parameters provide clear control over UI element visibility with sensible defaults.
87-89: LGTM: Consistent parameter passing.The parameters are correctly passed through to the footer rendering function.
173-175: LGTM: Function signature updated correctly.The new parameters are properly added with appropriate defaults.
210-219: LGTM: Proper conditional rendering for updated timestamp.The logic correctly checks for the
hide_updated_atflag and applies consistent styling.
221-221: LGTM: Enhanced run count display logic.The condition properly respects both the existing threshold (50+ runs) and the new
show_all_run_countsflag for more granular control.widgets/workflow_search.py (1)
204-212: LGTM: Robust workspace ownership check.The implementation correctly handles both numeric workspace IDs and string handles, with appropriate error handling for the type conversion.
widgets/explore.py (3)
18-18: LGTM: Minor text improvements.Good catch fixing the typographic apostrophes to standard quotes for consistency.
Also applies to: 21-21
47-52: LGTM: Proper parameter threading through grid layout.The lambda functions correctly pass the user and search_filters parameters to the rendering functions.
Also applies to: 54-59
100-104: LGTM: Clean function signature updates.The parameters are consistently added across all rendering functions with appropriate defaults.
Also applies to: 112-116, 127-131
widgets/author.py (4)
26-26: LGTM: Appropriate special case for breadcrumbs.Correctly preserves underlines for breadcrumb navigation by explicitly setting
remove_underline=False.
45-49: LGTM: Clean conditional link styling.The link creation logic properly applies the text decoration class based on the parameter, with appropriate fallback to dummy element.
60-60: LGTM: Consistent parameter threading.The
remove_underlineparameter is properly added to all functions with sensible defaults and correctly passed through the call hierarchy.Also applies to: 79-79, 89-89, 105-105, 116-116
126-131: LGTM: Improved link creation logic.The explicit if-else structure is more readable than the previous conditional expression and properly handles the class name application.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
utils/workspace.py (2)
4-18: Consider performance optimization for frequently called function.The function recreates sets from
user.cached_workspaceson every call. Since this utility is used across multiple UI components (as mentioned in the AI summary), consider caching these sets to improve performance.def is_user_workspace_owner(user: AppUser | None, workspace_filter: str | None) -> bool: """Check if the current user owns the filtered workspace.""" if not (user and workspace_filter and not user.is_anonymous): return False - user_workspace_ids = {w.id for w in user.cached_workspaces} - user_workspace_handles = {w.handle.name for w in user.cached_workspaces if w.handle} + # Cache workspace sets to avoid recreating on every call + if not hasattr(user, '_workspace_ids_cache'): + user._workspace_ids_cache = {w.id for w in user.cached_workspaces} + user._workspace_handles_cache = {w.handle.name for w in user.cached_workspaces if w.handle} + + user_workspace_ids = user._workspace_ids_cache + user_workspace_handles = user._workspace_handles_cache try: # Check if workspace filter is numeric (workspace ID) workspace_id = int(workspace_filter) return workspace_id in user_workspace_ids except ValueError: # Workspace filter is a handle name return workspace_filter in user_workspace_handles
4-5: Enhance function documentation.The docstring could be more detailed about the supported workspace filter formats and the function's behavior.
- """Check if the current user owns the filtered workspace.""" + """ + Check if the current user owns the filtered workspace. + + Args: + user: The user to check ownership for. Returns False if None or anonymous. + workspace_filter: Either a workspace ID (numeric string) or handle name (string). + Returns False if None or empty. + + Returns: + True if the user owns the workspace identified by the filter, False otherwise. + """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
utils/__init__.py(1 hunks)utils/workspace.py(1 hunks)widgets/explore.py(3 hunks)widgets/saved_workflow.py(5 hunks)widgets/workflow_search.py(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- utils/init.py
🚧 Files skipped from review as they are similar to previous changes (3)
- widgets/saved_workflow.py
- widgets/workflow_search.py
- widgets/explore.py
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test (3.10.12, 1.8.3)
utils/workspace.py
Outdated
| try: | ||
| # Check if workspace filter is numeric (workspace ID) | ||
| workspace_id = int(workspace_filter) | ||
| return workspace_id in user_workspace_ids | ||
| except ValueError: | ||
| # Workspace filter is a handle name | ||
| return workspace_filter in user_workspace_handles |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and add input validation.
The current implementation only handles ValueError for int conversion. Consider adding more comprehensive error handling and input validation.
try:
# Check if workspace filter is numeric (workspace ID)
workspace_id = int(workspace_filter)
+ if workspace_id <= 0:
+ return False
return workspace_id in user_workspace_ids
- except ValueError:
+ except (ValueError, TypeError):
# Workspace filter is a handle name
+ if not isinstance(workspace_filter, str) or not workspace_filter.strip():
+ return False
return workspace_filter in user_workspace_handles📝 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.
| try: | |
| # Check if workspace filter is numeric (workspace ID) | |
| workspace_id = int(workspace_filter) | |
| return workspace_id in user_workspace_ids | |
| except ValueError: | |
| # Workspace filter is a handle name | |
| return workspace_filter in user_workspace_handles | |
| try: | |
| # Check if workspace filter is numeric (workspace ID) | |
| workspace_id = int(workspace_filter) | |
| if workspace_id <= 0: | |
| return False | |
| return workspace_id in user_workspace_ids | |
| except (ValueError, TypeError): | |
| # Workspace filter is a handle name | |
| if not isinstance(workspace_filter, str) or not workspace_filter.strip(): | |
| return False | |
| return workspace_filter in user_workspace_handles |
🤖 Prompt for AI Agents
In utils/workspace.py around lines 12 to 18, improve the error handling by
adding input validation before attempting to convert workspace_filter to int,
and catch additional potential exceptions beyond ValueError. Validate that
workspace_filter is a string or appropriate type, and handle cases where it
might be None or an unexpected type, returning False or raising a clear error as
needed. Expand the except block to handle other exceptions that could arise
during conversion or membership checks to make the function more robust.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
utils/workspace.py (1)
39-87: Consider refactoring to reduce return statements, but current implementation is acceptable.The static analysis tool flags 9 return statements exceeding the recommended limit of 6. While this could be refactored to use fewer returns, the current defensive programming approach with early returns actually improves readability and follows good practices for input validation and error handling.
If you prefer to address the static analysis warning, consider consolidating some return statements:
- if not (user and workspace_filter and not user.is_anonymous): - return False - - # Additional input validation - if not isinstance(workspace_filter, str): - return False - - workspace_filter = workspace_filter.strip() - if not workspace_filter: - return False + if not (user and workspace_filter and not user.is_anonymous and + isinstance(workspace_filter, str) and workspace_filter.strip()): + return False + + workspace_filter = workspace_filter.strip()However, the current implementation is perfectly acceptable and prioritizes clarity over strict adherence to the return statement limit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
utils/workspace.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
utils/workspace.py (1)
app_users/models.py (2)
AppUser(90-262)cached_workspaces(242-249)
🪛 Pylint (3.3.7)
utils/workspace.py
[refactor] 4-4: Too many return statements (9/6)
(R0911)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test (3.10.12, 1.8.3)
- GitHub Check: test (3.10.12, 1.8.3)
🔇 Additional comments (2)
utils/workspace.py (2)
54-58: Past review comments have been properly addressed.The previous concerns about safety checks for workspace handle attribute access have been resolved with the comprehensive validation on line 57:
if w.handle and hasattr(w.handle, "name") and w.handle.name.
4-88: Excellent implementation with comprehensive error handling and performance optimization.This utility function is well-designed with:
- Thorough input validation and documentation
- Smart caching mechanism for performance optimization
- Comprehensive exception handling covering edge cases
- Support for both workspace ID and handle name formats
The function effectively centralizes workspace ownership logic and will enable consistent UI behavior across the widgets that use it.
… and reverted hyperlink changes
avi-007
force-pushed
the
explore-page-fix
branch
2 times, most recently
from
1381890 to
e0edc9a
Compare
# Conflicts: # widgets/author.py
…n by removing redundant validation and exception handling
mobile layout has weird spacing |
… display in explore and saved workflow widgets
feat: default column mapping if the field name matches the column name fix: show variables in bulk run columns
Q/A checklist
I have tested my UI changes on mobile and they look acceptable
I have tested changes to the workflows in both the API and the UI
I have done a code review of my changes and looked at each line of the diff + the references of each function I have changed
My changes have not increased the import time of the server
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.