-
Notifications
You must be signed in to change notification settings - Fork 229
Fix open source templates filtering #1725
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Greptile OverviewGreptile SummaryThis PR attempts to fix template-related warnings by making two main changes:
Issues FoundCritical: The change in Architectural Concern: The The PR aligns with custom instruction f4c2a2d5 (using Confidence Score: 2/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant User
participant SearchUI as Search UI
participant AskAILink as Ask AI Link
participant ExternalURL as External URL
participant Sidebar as Templates Sidebar
participant State as TemplatesState
participant ComputedVar as filtered_templates
Note over SearchUI,AskAILink: Changed rx.link to rx.el.a
User->>SearchUI: Opens search dialog
SearchUI->>AskAILink: Renders button with rx.el.a
AskAILink--xExternalURL: Uses to prop BROKEN should be href
Note over AskAILink,ExternalURL: External links need href not to
User->>Sidebar: Filters templates by tag or query
Sidebar->>State: Updates checked_tags or query
State->>ComputedVar: Requests filtered_templates
Note over ComputedVar: auto_deps False may prevent recalc
ComputedVar->>State: Calls get all filtered templates
ComputedVar->>State: Side effect Sets total_pages
ComputedVar-->>Sidebar: Returns paginated templates
Note over ComputedVar: Should be pure no side effects
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 files reviewed, 2 comments
Additional Comments (1)
Additionally, this computed var has a side effect - it sets Consider refactoring to:
This approach would be more maintainable and avoid potential issues with stale computed values. Prompt To Fix With AIThis is a comment left during a code review.
Path: pcweb/pages/gallery/sidebar.py
Line: 65:75
Comment:
Adding `auto_deps=False` to this computed var disables automatic dependency tracking, which means the var may not recalculate when `self.query` or `self.checked_tags` change (these are accessed via `_get_all_filtered_templates()`).
Additionally, this computed var has a side effect - it sets `self.total_pages` (line 68-72), which is generally discouraged in computed vars. Computed vars should be pure functions without side effects.
Consider refactoring to:
1. Move the `total_pages` calculation to a separate computed var
2. Let Reflex automatically track dependencies by removing `auto_deps=False`
3. Or explicitly declare dependencies if needed
This approach would be more maintainable and avoid potential issues with stale computed values.
How can I resolve this? If you propose a fix, please make it concise. |
No description provided.