Refactor index_layout: Extract nested components into modular utility functions#1007
Refactor index_layout: Extract nested components into modular utility functions#1007
Conversation
EngCaioFonseca
moved this from Backlog
to Testing | Waiting on PR response | Blocked
in Aspen Project Board
EngCaioFonseca
force-pushed
the
removing_nesting
branch
from
9f84273 to
7a0983e
Compare
MoralCode
left a comment
MoralCode
left a comment
There was a problem hiding this comment.
Skimmed through and left some feedback.
Given this PR is +600 - 300 in terms of diff (essentially taking the number of lines of code that get touched and doubling it), I'm a little unsure how this overall helps the codebase. It feels like we just replaced storing things in variables with wrapping them in parameter-less functions. Sure they are slightly more separated now, but i dont see how that helps actually refactor things to be more reusable or general-purpose
| import dash_bootstrap_components as dbc | ||
|
|
||
|
|
||
| def create_login_disabled_alert(): |
There was a problem hiding this comment.
rather than functions called "create ", i think we should consider using dash all in one components: dash.plotly.com/all-in-one-components like i did for the visualization refactor to create truly generic components.
For example for this function, i would make an Alert component that allowed the title, message, and colors to be customized, so we can reuse it for many types of alerts across the whole site.
The core point of functions is to make code reusable by allowing you to encapsulate a small unit of functionality and expose certain parameters to change its operation. If we are simply moving things into a function with no args without making use of these arguments to allow this function to be reused, then we are just creating a situation where future-us is going to just create a function called create_repository_error_alert() or my_custom_alert() and we are going to be back to having a problem of duplicate or otherwise hard to maintain code.
All in one components are, i think, a better option because they are in their own separate folder and thus almost have to (well not technically but i guess in as close as you can get to "socially" with code) be made generically because they are farther away in the code from where they are used, and thus could be imported by anything else in the codebase.
There was a problem hiding this comment.
Some of the functions I've already made into more generic component, like the icon function, which works for three of the icons in the sidebar.
I'll work on making the functions more generic for other components as well.
The main idea of this refactoring was to make the code cleaner and remove the excessive nesting as it was before and therefore more readable and maintainable as well. I agree that the functions should be generic and reusable, that was the point, but the main one was to turn that whole nested chunk into a more "functional" one.
But it's fine with me working on making them more generic for each component as well.
Like looking into the all-in-one components.
There was a problem hiding this comment.
Alerts seem like another thing that are likely to be useful to implement in a reusable fashion. It seems lik this function hard-codes things like the alert title and body text that should be passed in as function args (or even better, refactored as an All In One Component Class)
There was a problem hiding this comment.
ok it looks like we do have a generic create alert function
https://github.com/oss-aspen/8Knot/pull/1007/files#diff-72ed40f2e8458b3ed27e11b3c9c0f731f8e0aa5d07bc58d4820ad77d255a4fc1R250
This specific one should probably be removed in favor of passing the parameters into that one
8Knot/pages/index/login_utils.py
Outdated
| style={ | ||
| "backgroundColor": "#DFF0FB", | ||
| "borderColor": "#0F5880", | ||
| "border": "1px solid #0F5880", | ||
| "borderLeft": "5px solid #0F5880", | ||
| "boxShadow": "0 2px 8px rgba(0, 0, 0, 0.15)", | ||
| "maxWidth": "400px", | ||
| "padding": "15px", | ||
| "zIndex": "1000", |
There was a problem hiding this comment.
why are these styles inline?
There was a problem hiding this comment.
I didn't modify any of the styling, just brought to the functions, but I'll change those as well.
8Knot/pages/index/login_utils.py
Outdated
|
|
||
| def create_manage_groups_nav_item(augur_manager): | ||
| """ | ||
| Create the Manage Groups navigation item. |
There was a problem hiding this comment.
this documentation doesnt tell me anything new compared to the function name. comments should explain WHY we are doing something a certain way, or what something is for. The code already explains what it does
I disagree with this pov, because the code overall is not only a function of lines of code added and subtracted. |
EngCaioFonseca
force-pushed
the
removing_nesting
branch
from
30140bb to
4666ace
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| is_login_enabled: Check login status from environment | ||
| """ | ||
| import os | ||
| from typing import Optional, Dict, Any, Union, List, TYPE_CHECKING |
There was a problem hiding this comment.
[pylint] reported by reviewdog 🐶
W0611: Unused Dict imported from typing (unused-import)
| is_login_enabled: Check login status from environment | ||
| """ | ||
| import os | ||
| from typing import Optional, Dict, Any, Union, List, TYPE_CHECKING |
There was a problem hiding this comment.
[pylint] reported by reviewdog 🐶
W0611: Unused Any imported from typing (unused-import)
| is_login_enabled: Check login status from environment | ||
| """ | ||
| import os | ||
| from typing import Optional, Dict, Any, Union, List, TYPE_CHECKING |
There was a problem hiding this comment.
[pylint] reported by reviewdog 🐶
W0611: Unused Union imported from typing (unused-import)
| is_login_enabled: Check login status from environment | ||
| """ | ||
| import os | ||
| from typing import Optional, Dict, Any, Union, List, TYPE_CHECKING |
There was a problem hiding this comment.
[pylint] reported by reviewdog 🐶
W0611: Unused List imported from typing (unused-import)
8Knot/pages/index/search_utils.py
Outdated
| html.Div( | ||
| [ | ||
| create_search_multiselect(initial_option), | ||
| create_search_icon_button(), |
There was a problem hiding this comment.
[pylint] reported by reviewdog 🐶
E0602: Undefined variable 'create_search_icon_button' (undefined-variable)
8Knot/pages/index/search_utils.py
Outdated
| ], | ||
| className="search-input-wrapper", | ||
| ), | ||
| create_help_alert(), |
There was a problem hiding this comment.
[pylint] reported by reviewdog 🐶
E0602: Undefined variable 'create_help_alert' (undefined-variable)
8Knot/pages/index/search_utils.py
Outdated
| className="search-input-wrapper", | ||
| ), | ||
| create_help_alert(), | ||
| create_repo_list_alert(), |
There was a problem hiding this comment.
[pylint] reported by reviewdog 🐶
E0602: Undefined variable 'create_repo_list_alert' (undefined-variable)
cdolfi
left a comment
cdolfi
left a comment
There was a problem hiding this comment.
overall looks good to me going to let @MoralCode take a look before merging
MoralCode
left a comment
MoralCode
left a comment
There was a problem hiding this comment.
I see there has been some improvement with things like making a generic alert function with parameters so it can be reused.
If I were writing this PR, I'd still want to move more stuff into actual component classes. In addition to the benefits of reusability, AIO classes can also help move code to other parts of the codebase, decluttering these files further and making them more readable.
I also think proper AIO classes for major, but one-off components such as the searchbar, could also be beneficial for the above reason (encapsulation), and also because the searchbar is so complex, that giving it its own class (and own file) will allow us to keep all the various bits (data stores, toggle switches, etc) together, while also having more space to more thoroughly document it - especially since its a core component of the codebase.
| target: Optional[str] = None, | ||
| ) -> dbc.NavItem: | ||
| """ | ||
| Generic navigation item builder following Dash All-in-One Components pattern. |
There was a problem hiding this comment.
if this is meant to be an all in one component, it should probably be implemented as a class and be placed in its own file alongside our existing VisualizationAIO component.
| import dash_bootstrap_components as dbc | ||
|
|
||
|
|
||
| def create_login_disabled_alert(): |
There was a problem hiding this comment.
Alerts seem like another thing that are likely to be useful to implement in a reusable fashion. It seems lik this function hard-codes things like the alert title and body text that should be passed in as function args (or even better, refactored as an All In One Component Class)
| def create_login_disabled_banner(): | ||
| """ | ||
| Create the banner displayed when login is disabled. | ||
| Uses CSS class .login-banner-container for positioning. | ||
|
|
||
| Returns: | ||
| html.Div containing the login disabled banner, or None if login is enabled | ||
| """ | ||
| if not is_login_enabled(): | ||
| return html.Div( | ||
| create_login_disabled_alert(), | ||
| className="login-banner-container", | ||
| ) | ||
| return None |
There was a problem hiding this comment.
this seems like a really small function that encapsulates a very specific piece of functionality that - as far as i can tell - we likely only need in one place. Do we need a function for this?
| def create_login_popover(): | ||
| """ | ||
| Create the login failed popover. | ||
|
|
||
| Returns: | ||
| dbc.Popover component for login failure notifications | ||
| """ | ||
| return dbc.Popover( | ||
| children="Login Failed", | ||
| body=True, | ||
| id="login-popover", | ||
| is_open=False, | ||
| placement="bottom-end", | ||
| target="nav-dropdown", | ||
| ) |
There was a problem hiding this comment.
How is a popover different from the Alert we are using a few functions above this? Could this be another place where it may be helpful to reuse alerts?
| import dash_bootstrap_components as dbc | ||
|
|
||
|
|
||
| def create_login_disabled_alert(): |
There was a problem hiding this comment.
ok it looks like we do have a generic create alert function
https://github.com/oss-aspen/8Knot/pull/1007/files#diff-72ed40f2e8458b3ed27e11b3c9c0f731f8e0aa5d07bc58d4820ad77d255a4fc1R250
This specific one should probably be removed in favor of passing the parameters into that one
| Note: DMC MultiSelect requires inline styles via the 'styles' prop. | ||
| This is a component-specific API, not regular inline styling. |
There was a problem hiding this comment.
thanks for documenting the reasons why these are inline and not in CSS!
| def create_bot_filter_switch(): | ||
| """ | ||
| Create the GitHub bot filter switch component. | ||
|
|
||
| Returns: | ||
| dbc.Switch component for bot filtering | ||
| """ | ||
| return dbc.Switch( | ||
| id="bot-switch", | ||
| label="GitHub Bot Filter", | ||
| value=True, | ||
| input_class_name="botlist-filter-switch", | ||
| className="bot-filter-switch", | ||
| ) |
There was a problem hiding this comment.
If this is a function thats only used in create_search_controls(), it may be worth just merging it into that function with a comment
| # ===== LAYOUT COMPONENT FUNCTIONS ===== | ||
|
|
||
|
|
||
| def create_search_storage_components(): |
There was a problem hiding this comment.
if this function is only used by create_search_bar, maybe it should be merged in with that function - unless we plan to reuse it.
| ) | ||
|
|
||
|
|
||
| def create_search_bar(initial_option): |
There was a problem hiding this comment.
Should we make our entire search bar into an All in One component? it seems like theres a lot of complexity there with all the data storage, bot filter switches, etc that only matter for the search bar. If we can encapsulate all this into one AIO component class, that will likely simplify this code a lot and allow people to just treat "the search bar" as one encapsulated thing (sorta blackbox-y) and not have to worry about whats inside it unless their change necessitates it
|
@EngCaioFonseca luk when this is ready for another round of reviews |
EngCaioFonseca
force-pushed
the
removing_nesting
branch
from
df5d969 to
88e0341
Compare
|
Note
|
| Cohort / File(s) | Change Summary |
|---|---|
CSS Styling 8Knot/assets/color.css, 8Knot/assets/main_layout.css |
Added CSS custom properties for multiselect sizing (\-\-multiselect-min-height, \-\-multiselect-padding, \-\-multiselect-item-margin) and pill styling (\-\-pill-default-bg, \-\-pill-default-color). Removed legacy .search-icon styling block. Introduced utility classes for icon buttons, login banner positioning, and search component layout. |
Layout Refactoring 8Knot/pages/index/index_layout.py |
Replaced hardcoded login banner and navbar logic with function-based creation. Added imports for search_utils and login_utils. Changed login gating from environment variable to is_login_enabled() check. Components now created via utility functions: create_login_disabled_banner(), create_login_navbar(), create_search_bar(), create_bottom_navbar(). |
Login Utility Module 8Knot/pages/index/login_utils.py |
New module introducing modular login UI component builders including create_nav_item(), create_login_disabled_alert(), create_login_disabled_banner(), create_login_container(), create_login_popover(), create_login_nav(), create_login_navbar(), and is_login_enabled() environment reader. |
Search Utility Module 8Knot/pages/index/search_utils.py |
New module introducing comprehensive search bar component builders including create_alert(), create_store(), create_button(), create_search_storage_components(), create_multiselect_styles(), create_search_multiselect(), create_search_input_section(), create_bot_filter_switch(), create_search_controls(), create_search_bar(), and create_bottom_navbar(). |
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
Areas requiring extra attention:
- New modules
login_utils.pyandsearch_utils.pycontain multiple builder functions that should be verified for completeness and consistency with existing component structure - Verify that
is_login_enabled()correctly handles environment variable fallback and error cases - Check that CSS custom properties are correctly applied across multiselect and pill components
- Ensure
initialize_components(search_bar)is called with the correct argument and dependencies - Review the conditional rendering logic of
login_bannerin the main layout
Possibly related PRs
- UI fixes #995: Modifies multiselect/pill styling in color.css and main_layout.css with overlapping CSS variable definitions
- removal of duplicates/similar CSS. Prioritizing global variables. #980: Centralizes and renames CSS token surface in color.css and main_layout.css
Suggested reviewers
- cdolfi
Poem
🐰✨ The components now dance in modular grace,
With utility builders in their own special place!
Pills sport their new colors, searches find flow,
Login banners shine bright—what a refactor show! 🎨
Pre-merge checks and finishing touches
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Description check | The description is incomplete. It lacks the required Generative AI disclosure section, which is mandatory according to the repository template. | Complete the Generative AI disclosure section by checking the checkbox and providing details about any AI tools used, how they were used, and whether outputs were reviewed. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title clearly and accurately summarizes the main refactoring objective: extracting nested components from index_layout into modular utility functions. |
| Docstring Coverage | ✅ Passed | Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. |
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
- Commit unit tests in branch
removing_nesting
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 help to get the list of available commands and usage tips.
![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
♻️ Duplicate comments (3)
8Knot/pages/index/login_utils.py (3)
63-96: Consider using the genericcreate_alertfromsearch_utils.This function creates a specific alert with hardcoded content. The
search_utils.pymodule provides a genericcreate_alertbuilder that could be reused here to maintain consistency and reduce duplication.
14-14: Remove unused imports.
Dict,Any,Union, andListare imported but not used in this module. This was flagged by pylint.-from typing import Optional, Dict, Any, Union, List, TYPE_CHECKING +from typing import Optional, TYPE_CHECKING
198-206: Remove unnecessaryelseafterreturn.The
elseblock is unnecessary since theifbranch returns. This was flagged by pylint.def create_login_navbar(augur_manager: "AugurManager"): if is_login_enabled(): return [ dbc.Row( [dbc.Col(create_login_nav(augur_manager))], align="center", ), ] - else: - return [html.Div()] + return [html.Div()]
🧹 Nitpick comments (1)
8Knot/assets/color.css (1)
62-64: Consider reusing existing color variables.The new pill color variables duplicate existing values:
--pill-default-bg: #555555matches--gray-mediumand--pill-default-color: #ffffffmatches--color-white. Consider usingvar(--gray-medium)andvar(--color-white)as values to maintain a single source of truth./* MultiSelect pill colors */ - --pill-default-bg: #555555; - --pill-default-color: #ffffff; + --pill-default-bg: var(--gray-medium); + --pill-default-color: var(--color-white);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
8Knot/assets/color.css(1 hunks)8Knot/assets/main_layout.css(2 hunks)8Knot/pages/index/index_layout.py(2 hunks)8Knot/pages/index/login_utils.py(1 hunks)8Knot/pages/index/search_utils.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
8Knot/pages/index/login_utils.py (1)
8Knot/db_manager/augur_manager.py (1)
AugurManager(15-362)
8Knot/pages/index/index_layout.py (3)
8Knot/pages/index/search_utils.py (2)
create_search_bar(567-594)create_bottom_navbar(597-641)8Knot/pages/index/login_utils.py (3)
create_login_disabled_banner(99-112)create_login_navbar(188-206)is_login_enabled(209-216)8Knot/db_manager/augur_manager.py (1)
initial_multiselect_option(262-298)
🪛 Biome (2.1.2)
8Knot/assets/main_layout.css
[error] 414-414: Unexpected shorthand property border after border-color
(lint/suspicious/noShorthandPropertyOverrides)
⏰ 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: End-to-end test (Docker)
- GitHub Check: End-to-end test (Podman)
🔇 Additional comments (16)
8Knot/assets/main_layout.css (3)
35-38: LGTM!The new multiselect sizing tokens are well-organized and follow the existing naming conventions.
375-400: LGTM!The generic icon button utility classes are well-structured with proper state handling for hover, focus, and active states.
440-466: LGTM!The search component CSS classes are well-organized and use CSS variables consistently.
8Knot/pages/index/index_layout.py (3)
20-31: LGTM!Clean import organization separating search and login utilities. The modular structure improves maintainability.
113-139: LGTM!The layout structure is clean and well-organized. The conditional rendering of
login_bannerand the use of spread operators for stores/scripts is idiomatic.
94-104: Module-level initialization is properly sequenced.The concern about
augur.initial_multiselect_option()being called before database initialization is mitigated by the import order inapp.py:augur.multiselect_startup()is called at line 48 (which populatesmultiselect_optionsvia a database query), andindex_layout.pyis not imported until line 108, well after the database initialization completes. Additionally,initial_multiselect_option()includes a try-except block that returns a fallback option if any error occurs, providing defensive safety against early calls.8Knot/pages/index/login_utils.py (2)
26-57: LGTM!The generic
create_nav_itembuilder is well-documented and follows the All-in-One pattern with appropriate parameters for flexibility.
149-185: LGTM!The
create_login_navfunction cleanly composes the login navigation using the generic builders.8Knot/pages/index/search_utils.py (8)
1-27: LGTM!The module docstring clearly documents the All-in-One pattern and distinguishes between generic builders and search-specific functions. Good organization.
250-295: LGTM!The
create_alertbuilder is well-designed with comprehensive parameters for customization. Good handling of icon prepending with proper type checking.
298-314: LGTM!Simple and effective store builder following the generic pattern.
317-372: LGTM!The
create_buttonbuilder is flexible, supporting bothdbc.Buttonanddbc.NavLinkbased on whetherhrefis provided. Good pattern for reusability.
393-438: LGTM!The multiselect styles are well-documented, explaining why inline styles are required for DMC components. Good use of CSS variables for theming.
482-499: Inline styles are acceptable here for absolute positioning.The absolute positioning of the search icon requires precise pixel values that are specific to this component's layout. This is an appropriate use of inline styles.
567-594: LGTM!The
create_search_barfunction cleanly composes all sub-components. The storage components, alert, input section, and controls are well-organized.
597-641: LGTM!Clean data-driven approach for the bottom navbar. The nav items configuration makes it easy to add or modify links.
| .login-banner-alert { | ||
| background-color: var(--baby-blue-100); | ||
| border-color: var(--baby-blue-700); | ||
| border: 1px solid var(--baby-blue-700); | ||
| border-left: 5px solid var(--baby-blue-700); | ||
| box-shadow: 0 2px 8px var(--shadow-color-md); | ||
| max-width: 400px; | ||
| padding: 15px; | ||
| z-index: 1000; | ||
| } |
There was a problem hiding this comment.
Fix CSS shorthand property override.
The border shorthand on line 414 overrides the border-color set on line 413, making the border-color declaration ineffective. This was flagged by static analysis.
.login-banner-alert {
background-color: var(--baby-blue-100);
- border-color: var(--baby-blue-700);
border: 1px solid var(--baby-blue-700);
border-left: 5px solid var(--baby-blue-700);
box-shadow: 0 2px 8px var(--shadow-color-md);
max-width: 400px;
padding: 15px;
z-index: 1000;
}📝 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.
| .login-banner-alert { | |
| background-color: var(--baby-blue-100); | |
| border-color: var(--baby-blue-700); | |
| border: 1px solid var(--baby-blue-700); | |
| border-left: 5px solid var(--baby-blue-700); | |
| box-shadow: 0 2px 8px var(--shadow-color-md); | |
| max-width: 400px; | |
| padding: 15px; | |
| z-index: 1000; | |
| } | |
| .login-banner-alert { | |
| background-color: var(--baby-blue-100); | |
| border: 1px solid var(--baby-blue-700); | |
| border-left: 5px solid var(--baby-blue-700); | |
| box-shadow: 0 2px 8px var(--shadow-color-md); | |
| max-width: 400px; | |
| padding: 15px; | |
| z-index: 1000; | |
| } |
🧰 Tools
🪛 Biome (2.1.2)
[error] 414-414: Unexpected shorthand property border after border-color
(lint/suspicious/noShorthandPropertyOverrides)
🤖 Prompt for AI Agents
In 8Knot/assets/main_layout.css around lines 411 to 420, the `border: 1px solid
var(--baby-blue-700)` shorthand on line 414 overrides the earlier `border-color`
on line 413; remove the conflicting shorthand or replace it with explicit
properties (e.g., set `border-width: 1px; border-style: solid;` and keep
`border-color: var(--baby-blue-700);`) or move the `border-color` declaration
after the shorthand so the intended color is preserved.
In line with what we discussed, would you be able to merge it and I'll keep working on the improvements from the issues we open outside of this PR? |
|
@EngCaioFonseca Squash commits and rebase and good to merge |
EngCaioFonseca
force-pushed
the
removing_nesting
branch
from
88e0341 to
01c3646
Compare
|
@cdolfi good to go. |
github-project-automation
bot
moved this from Testing | Waiting on PR response | Blocked
to Done
in Aspen Project Board
3 participants
Cleaning/refactoring code. Removing nesting, creating functions into their own utils files.
Summary by CodeRabbit
Style
New Features
✏️ Tip: You can customize this high-level summary in your review settings.