refactor: simplify filters with URL-based state and multi-select support#42
refactor: simplify filters with URL-based state and multi-select support#42PascalRepond merged 1 commit intomainfrom
Conversation
PascalRepond
commented
Jan 5, 2026
PascalRepond
commented
- Replace HTMX filter system with URL parameter-based filtering
- Add multi-select support for type, status, and score filters
- Consolidate search and index views into unified flow
- Add new templatetags: query_string, query_string_exclude, toggle_sort_direction
- Replace JS-based filter badges with server-rendered badges
- Simplify JS: remove badge manipulation, use URL cleanup on page load
- Refactor view logic into composable helper functions
- Add OR filters for type/status/score (previously AND only)
- Add has_review and has_cover filter options
- Update templates to use anchor links instead of HTMX requests
- Improve mobile responsive design for controls
📝 WalkthroughWalkthroughConsolidates media listing, search, filtering, sorting, and pagination into a URL-parameter-driven flow: adds template-tag utilities for query-string manipulation, centralizes server-side filter/sort/pagination in _build_media_context and apply* helpers, removes the separate search route, and converts many HTMX interactions to standard navigation with a filters drawer UI. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant QueryTag as TemplateTag
participant Server as DjangoView
participant DB
rect rgb(240,248,255)
Note over User,Browser: URL-driven filter interactions
User->>Browser: Clicks filter / removes badge
Browser->>Browser: Update query params (history.replaceState or navigate)
Browser->>Server: GET /?<query_params>
end
rect rgb(245,255,235)
Server->>Server: _build_media_context(request) parses request.GET (getlist for multi-values)
Server->>Server: _apply_filters() and sorting
Server->>DB: Query filtered, sorted, paginated results
DB-->>Server: Rows
Server->>QueryTag: render query_string for links
QueryTag-->>Server: query fragments
Server->>Browser: Rendered HTML with filter drawer and links
Browser->>User: Display updated page
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
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
Fix all issues with AI Agents 🤖
In @src/core/views.py:
- Around line 127-137: In _apply_score_filter, guard the int conversion of each
score string (the score loop that does score_q |= Q(score=int(score))) with a
try/except ValueError so malformed score values from the URL don't crash the
view; on ValueError simply skip that score (or optionally collect and raise a
validation error upstream) and continue building score_q (keep handling "none"
with Q(score__isnull=True) unchanged), then return queryset.filter(score_q) as
before.
- Around line 59-73: The hardcoded French string "Non noté" in the score display
construction breaks i18n; import Django's translation function (e.g., from
django.utils.translation import gettext as _) in src/core/views.py and replace
the literal "Non noté" in the filters["score_display"] loop with a call to _(
... ) so the label is translatable (e.g.,
filters["score_display"].append(("none", _("Non noté")))); ensure the import is
added near other imports and no other logic changes are made.
In @src/theme/static_src/src/styles.css:
- Around line 36-39: The comment and implementation disagree: the comment says
"Force checkboxes to be square" but .checkbox sets border-radius: 0.25rem
(rounded). Either update the comment to reflect the intent ("Make checkboxes
rounded") or change the CSS in .checkbox to use border-radius: 0 to actually
force square corners; pick the correct intent and make the comment and .checkbox
rule consistent.
🧹 Nitpick comments (6)
src/templates/partials/media-contributors.html (1)
1-1: Remove unused template tag library or use the helper.The
media_tagslibrary is loaded but none of its helpers (query_string,query_string_exclude,toggle_sort_direction) are used in this template. The URL is manually constructed on Line 6.🔎 Two options to address this
Option 1: Remove the unused load
-{% load media_tags %} {# Renders the list of contributors with links #} {# Parameters: media, view_mode, sort #}Option 2 (recommended): Use the query_string helper to preserve all current URL parameters
{# Renders the list of contributors with links #} {# Parameters: media, view_mode, sort #} {% if media.contributors.all %} {% for contributor in media.contributors.all %} - <a href="{% url 'home' %}?view_mode={{ view_mode|default:'grid' }}&sort={{ sort|default:'-review_date' }}&contributor={{ contributor.id }}" + <a href="{% url 'home' %}?{% query_string request view_mode=view_mode|default:'grid' sort=sort|default:'-review_date' contributor=contributor.id %}" class="link link-hover contributor-link">{{ contributor.name }}</a>{% if not forloop.last %};{% endif %} {% endfor %} {% endif %}Option 2 is recommended as it would preserve any additional query parameters that might exist (e.g., other active filters).
src/core/templatetags/media_tags.py (2)
78-107: Consider adding defensive checks for the request object.The function assumes
requesthas aGETattribute but doesn't validate this. If called with an invalid object, it would raise anAttributeError.🔎 Add defensive validation
@register.simple_tag def query_string(request, **kwargs): """ Build a query string from current GET parameters, with updates from kwargs. Args: request: The current request object **kwargs: Parameters to add/update/remove (None to remove) Returns: Query string with all parameters (including multi-value params) Example usage: <a href="?{% query_string request view_mode='grid' %}">Grid</a> <a href="?{% query_string request sort=None %}">Clear sort</a> """ + if not hasattr(request, 'GET'): + return "" + # Start with a copy of current GET parameters (handles multi-value) params = request.GET.copy() # Update with provided kwargs for key, value in kwargs.items(): if value is None: # Remove parameter params.pop(key, None) else: # Set parameter (replaces all values) params[key] = value # Build query string return params.urlencode() if params else ""
110-130: Consider adding defensive checks for the request object.Similar to
query_string, this function should validate thatrequesthas aGETattribute to avoidAttributeError.🔎 Add defensive validation
@register.simple_tag def query_string_exclude(request, *exclude_keys): """ Build a query string from current GET parameters, excluding specified keys. Args: request: The current request object *exclude_keys: Parameter names to exclude Returns: Query string with all parameters except excluded ones Example usage: <a href="?{% query_string_exclude request 'page' %}">Without page</a> """ + if not hasattr(request, 'GET'): + return "" + params = request.GET.copy() for key in exclude_keys: params.pop(key, None) return params.urlencode() if params else ""src/templates/media.html (1)
51-71: Consider simplifying the sort field dropdown logic.The inline
onchangehandler at line 64 mixes template logic and JavaScript, making it harder to maintain and debug. The logic attempts to preserve sort direction when changing fields.💡 Consider moving to a separate JS function
In your JavaScript file:
function handleSortFieldChange(selectElement, currentSort) { const baseUrl = '?' + document.querySelector('[data-query-params]').dataset.queryParams; const isDescending = currentSort.startsWith('-'); const newField = selectElement.value; const sortParam = isDescending ? `-${newField}` : newField; window.location.href = `${baseUrl}&sort=${sortParam}`; }Then in the template:
<select class="select join-item select-sm sm:select-md" id="sort-field-select" data-query-params="{% query_string_exclude request 'sort' 'sort_field' %}" onchange="handleSortFieldChange(this, '{{ sort }}')">src/core/views.py (2)
20-32: Consider simplifying redundant return values.The function returns both
normalized_sortandordering, which currently hold the same value (line 31). While this doesn't cause issues, it adds unnecessary complexity.If
normalized_sortandorderingare always identical, consider:
- Removing one of them and updating call sites, or
- Adding a comment explaining why both are needed if there's a semantic difference
140-154: Consider adding date validation for robustness.Lines 142-145 apply date filters without validation. While HTML5 date inputs provide client-side validation, invalid date strings in manipulated URLs could cause database errors.
💡 Add defensive date parsing
+from datetime import datetime + def _apply_date_and_content_filters(queryset, filters): """Apply review date, review content, and cover filters.""" if filters["review_from"]: - queryset = queryset.filter(review_date__gte=filters["review_from"]) + try: + datetime.fromisoformat(filters["review_from"]) + queryset = queryset.filter(review_date__gte=filters["review_from"]) + except (ValueError, TypeError): + pass # Skip invalid date if filters["review_to"]: - queryset = queryset.filter(review_date__lte=filters["review_to"]) + try: + datetime.fromisoformat(filters["review_to"]) + queryset = queryset.filter(review_date__lte=filters["review_to"]) + except (ValueError, TypeError): + pass # Skip invalid date # ... rest of function
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/core/templatetags/media_tags.pysrc/core/urls.pysrc/core/views.pysrc/static/js/base.jssrc/templates/media.htmlsrc/templates/partials/filters-drawer.htmlsrc/templates/partials/filters.htmlsrc/templates/partials/load-more-trigger.htmlsrc/templates/partials/media-contributors.htmlsrc/templates/partials/view-mode-toggle.htmlsrc/tests/core/test_views.pysrc/theme/static_src/src/styles.css
💤 Files with no reviewable changes (2)
- src/templates/partials/filters.html
- src/core/urls.py
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-12-26T15:18:46.932Z
Learnt from: PascalRepond
Repo: PascalRepond/datakult PR: 21
File: src/templates/accounts/profile_edit.html:23-58
Timestamp: 2025-12-26T15:18:46.932Z
Learning: In Django projects, attributes added to a form field's widget via field.widget.attrs.update(...) in the form's __init__ are rendered when using {{ form.field }} in templates. No explicit attribute definitions are needed in the template. This applies to templates under src/templates in Django apps; ensure you update attrs in __init__ for consistent HTMX behavior.
Applied to files:
src/templates/partials/filters-drawer.htmlsrc/templates/partials/view-mode-toggle.htmlsrc/templates/partials/media-contributors.htmlsrc/templates/media.htmlsrc/templates/partials/load-more-trigger.html
📚 Learning: 2026-01-03T21:16:52.649Z
Learnt from: PascalRepond
Repo: PascalRepond/datakult PR: 36
File: src/templates/partials/media-items.html:67-67
Timestamp: 2026-01-03T21:16:52.649Z
Learning: In HTML templates (e.g., src/templates/partials/media-items.html), Tailwind CSS v4.1.11 supports arbitrary numeric utilities (max-w-{n}, w-{n}, h-{n}) that map to calc(var(--spacing) * n). These should be considered valid Tailwind classes and not flagged as invalid. Ensure review tooling and linters treat such numeric utilities as allowed, and apply this guidance to other HTML/template files in the repository.
Applied to files:
src/templates/partials/filters-drawer.htmlsrc/templates/partials/view-mode-toggle.htmlsrc/templates/partials/media-contributors.htmlsrc/templates/media.htmlsrc/templates/partials/load-more-trigger.html
📚 Learning: 2026-01-04T08:55:43.091Z
Learnt from: PascalRepond
Repo: PascalRepond/datakult PR: 36
File: src/templates/partials/media-score-badge.html:1-9
Timestamp: 2026-01-04T08:55:43.091Z
Learning: In Django projects, configuring heroicons in TEMPLATES builtins makes heroicons.templatetags.heroicons available in all templates without needing {% load heroicons %}. For template reviews in this codebase, assume heroicon tags work globally in HTML templates under templates/, so avoid requiring explicit loads. If a template still uses {% load heroicons %}, assess whether the load is unnecessary and can be removed; ensure no conflicts arise from global tags.
Applied to files:
src/templates/partials/filters-drawer.htmlsrc/templates/partials/view-mode-toggle.htmlsrc/templates/partials/media-contributors.htmlsrc/templates/media.htmlsrc/templates/partials/load-more-trigger.html
🧬 Code graph analysis (4)
src/templates/partials/filters-drawer.html (1)
src/core/migrations/0003_media_review_media_review_date_media_score_and_more.py (1)
Migration(9-41)
src/tests/core/test_views.py (1)
src/tests/conftest.py (3)
logged_in_client(65-68)media(20-31)agent(12-16)
src/core/templatetags/media_tags.py (1)
src/static/js/base.js (2)
params(45-45)params(85-85)
src/core/views.py (1)
src/core/models.py (2)
Media(108-226)Agent(94-105)
🔇 Additional comments (23)
src/static/js/base.js (2)
42-73: LGTM: URL cleanup implementation is correct.The URL cleanup logic correctly:
- Collects keys to delete before modifying (avoiding iteration issues)
- Removes empty/null/undefined values
- Removes default parameter values
- Updates the browser's URL bar via
replaceStatewithout triggering a page reloadThis is the appropriate approach for cleaning URLs on initial page load.
Note: The AI summary states "If deletions happened, navigates by setting window.location to the updated URL," but the implementation only uses
history.replaceState(Line 71) without navigation, which is actually the correct behavior for this use case.
75-98: LGTM: Badge removal logic correctly handles both multi-value and single-value filters.The implementation:
- Uses event delegation efficiently
- Correctly removes specific values from multi-value filters (type, status, score) by filtering and re-appending
- Correctly removes single-value filters entirely
- Appropriately reloads the page to reflect the URL-driven state
src/templates/partials/load-more-trigger.html (2)
2-2: LGTM: Correct template tag loading.The
media_tagstemplate library is properly loaded to enable thequery_stringhelper used in this template.
7-7: LGTM: Pagination URLs correctly preserve query parameters.The
query_stringhelper correctly:
- Preserves all current URL parameters (filters, sorting, view mode)
- Updates only the
pageparameter for pagination- Maintains consistent state across infinite scroll loads
This is the correct approach for URL-driven state management.
Also applies to: 15-15
src/templates/partials/view-mode-toggle.html (2)
2-2: LGTM: Correct template tag loading.The
media_tagslibrary is properly loaded and used by thequery_stringhelper on Lines 5 and 8.
5-10: LGTM: View mode toggle correctly implemented with URL-driven state.The anchor links correctly:
- Use the
query_stringhelper to preserve all current URL parameters (filters, sorting, pagination)- Update only the
view_modeparameter when toggling- Apply conditional
btn-activestyling based on current mode- Maintain proper accessibility with aria-labels
This is the correct approach for URL-driven state management, replacing the previous HTMX implementation.
src/theme/static_src/src/styles.css (1)
33-33: Thewrap-break-wordutility class is valid in Tailwind CSS v4.1.11 (the version used in this project). It corresponds to the CSS propertyoverflow-wrap: break-word;and is part of Tailwind's standard overflow-wrap utilities.Likely an incorrect or invalid review comment.
src/tests/core/test_views.py (2)
165-191: LGTM! Test updates align with architectural changes.The tests have been correctly updated to reflect the integration of search functionality into the index/home view. The assertions remain appropriate and test coverage is maintained.
566-577: LGTM! Pagination test correctly updated.The test appropriately reflects the new routing while maintaining verification of search pagination behavior.
src/core/templatetags/media_tags.py (1)
133-152: LGTM! Clean implementation of sort direction toggle.The logic correctly handles empty values and direction toggling. The default to "review_date" is consistent with the view layer defaults.
src/templates/partials/filters-drawer.html (5)
1-28: LGTM! Drawer structure and state preservation implemented correctly.The form properly preserves existing query parameters via hidden inputs, and the drawer follows accessibility best practices with appropriate ARIA labels.
30-60: LGTM! Multi-select type filter implemented correctly.The checkbox-based multi-select pattern with dynamic label is well-implemented and provides good UX.
61-130: LGTM! Status and score filters correctly handle multi-select and null values.The score filter appropriately uses
stringformat:"d"for comparison and includes special handling for unrated media that aligns with the backend logic.
131-180: LGTM! Date range and content filters properly implemented.The date inputs and binary filter selects correctly maintain state and use values consistent with the backend filtering logic.
184-198: LGTM! Clear action buttons with appropriate filter clearing behavior.The "Clear all filters" link correctly preserves view mode and sort while removing all filter parameters, providing good user experience.
src/templates/media.html (4)
1-14: LGTM! Header with item count provides clear feedback.The template loads are appropriate, and the item count display with proper pluralization enhances user experience.
16-45: LGTM! Search form correctly preserves filter state.The form properly maintains all active filters and parameters during search submission through appropriate hidden inputs and multi-value loops.
173-174: LGTM! Filters drawer properly included.The inclusion is straightforward and integrates the reviewed filters drawer partial into the main template.
84-171: LGTM! Active filter badges correctly render with removal controls.The badges appropriately use display lists from the context and include data attributes for JavaScript-based removal. The structure is consistent across all filter types.
The JavaScript implementation in
src/static/js/base.jscorrectly handles both multi-value filters (type, status, score withdata-value) and single-value filters (review dates, has_review, has_cover, contributor withoutdata-value), removing specific filter values or entire filters as needed via query string manipulation.src/core/views.py (4)
97-104: LGTM! Contributor filter safely handles invalid IDs.Using
.first()instead of.get()prevents exceptions when an invalid contributor ID is provided, which is appropriate for a filter parameter.
157-164: LGTM! Filter orchestration cleanly composes all filter functions.The function appropriately sequences all filter applications and maintains the contributor object for context rendering.
167-203: LGTM! Centralized context builder consolidates view logic effectively.The function successfully unifies queryset construction, filtering, sorting, and pagination logic, eliminating duplication between
indexandload_more_mediaviews.
206-210: LGTM! View refactoring eliminates duplication and improves maintainability.Both
indexandload_more_mediaviews now share the centralized context building logic, making the codebase easier to maintain and modify.Also applies to: 273-278
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @src/templates/backup_manage.html:
- Around line 148-153: In the backup_manage.html template fix the button
element's type attribute by removing the trailing space: change type="button "
to type="button" on the <button> that contains the onclick history/back logic
and the heroicon_outline "arrow-left" to ensure valid HTML and avoid attribute
parsing issues.
In @src/theme/static_src/src/styles.css:
- Line 33: The CSS @apply line uses an invalid Tailwind class `wrap-break-word`;
replace that token with the correct Tailwind v4 class `break-words` in the rule
that contains `@apply inline-block max-w-full wrap-break-word
whitespace-normal;` so it becomes `@apply inline-block max-w-full break-words
whitespace-normal;` ensuring Tailwind will recognize and emit the intended
overflow-wrap style.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/tests/core/test_views.py (1)
1-1: Fix the formatting issue before merging.The Ruff formatting check failed for this file. Please run the formatter to fix the issue:
ruff format ./src --quiet
🧹 Nitpick comments (3)
src/templates/media.html (2)
69-77: Inline JavaScript inonchangeis functional but complex.The URL construction logic works correctly by preserving the current sort direction when changing fields. Consider extracting this to a named function in
base.jsfor better maintainability and testability, though this is a minor improvement.
104-129: Inconsistentdata-filteranddata-valueattribute placement.The status and score badges have
data-filteranddata-valueon both the outer<div>(lines 107-108, 120-121) and the inner<button>(lines 112-113, 125-126), while type badges only have these attributes on the button. This inconsistency could cause issues if JavaScript expects a uniform structure.Consider removing the duplicate attributes from the outer divs to match the type badge pattern.
🔎 Proposed fix
{% for status_val, status_label in filters.status_display %} - <div class="badge badge-secondary gap-1" - data-filter="status" - data-value="{{ status_val }}"> + <div class="badge badge-secondary gap-1"> <span>{{ status_label }}</span> <button type="button" class="btn btn-ghost btn-xs btn-circle remove-filter-badge" data-filter="status" data-value="{{ status_val }}">{% heroicon_mini "x-mark" %}</button> </div> {% endfor %}Apply the same pattern to score badges (lines 119-121).
src/core/views.py (1)
112-129: Consider using__inlookup for simpler OR filtering.The type and status filters manually build OR queries. Django's
__inlookup provides the same semantics more concisely.🔎 Proposed simplification
def _apply_type_filter(queryset, media_types): """Apply OR filter for media types.""" if not media_types: return queryset - type_q = Q() - for media_type in media_types: - type_q |= Q(media_type=media_type) - return queryset.filter(type_q) + return queryset.filter(media_type__in=media_types) def _apply_status_filter(queryset, statuses): """Apply OR filter for statuses.""" if not statuses: return queryset - status_q = Q() - for status in statuses: - status_q |= Q(status=status) - return queryset.filter(status_q) + return queryset.filter(status__in=statuses)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
src/core/templatetags/media_tags.pysrc/core/urls.pysrc/core/views.pysrc/static/js/base.jssrc/templates/backup_manage.htmlsrc/templates/base.htmlsrc/templates/media.htmlsrc/templates/media_detail.htmlsrc/templates/media_edit.htmlsrc/templates/partials/filters-drawer.htmlsrc/templates/partials/filters.htmlsrc/templates/partials/load-more-trigger.htmlsrc/templates/partials/media-contributors.htmlsrc/templates/partials/media-edit-button.htmlsrc/templates/partials/media-score-badge.htmlsrc/templates/partials/sidebar-nav.htmlsrc/templates/partials/view-mode-toggle.htmlsrc/tests/core/test_views.pysrc/theme/static_src/src/styles.css
💤 Files with no reviewable changes (2)
- src/core/urls.py
- src/templates/partials/filters.html
🚧 Files skipped from review as they are similar to previous changes (4)
- src/templates/partials/view-mode-toggle.html
- src/templates/partials/load-more-trigger.html
- src/templates/partials/media-contributors.html
- src/templates/partials/filters-drawer.html
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-12-26T15:18:46.932Z
Learnt from: PascalRepond
Repo: PascalRepond/datakult PR: 21
File: src/templates/accounts/profile_edit.html:23-58
Timestamp: 2025-12-26T15:18:46.932Z
Learning: In Django projects, attributes added to a form field's widget via field.widget.attrs.update(...) in the form's __init__ are rendered when using {{ form.field }} in templates. No explicit attribute definitions are needed in the template. This applies to templates under src/templates in Django apps; ensure you update attrs in __init__ for consistent HTMX behavior.
Applied to files:
src/templates/media_edit.htmlsrc/templates/partials/media-score-badge.htmlsrc/templates/partials/media-edit-button.htmlsrc/templates/partials/sidebar-nav.htmlsrc/templates/media.htmlsrc/templates/base.htmlsrc/templates/media_detail.htmlsrc/templates/backup_manage.html
📚 Learning: 2026-01-03T21:16:52.649Z
Learnt from: PascalRepond
Repo: PascalRepond/datakult PR: 36
File: src/templates/partials/media-items.html:67-67
Timestamp: 2026-01-03T21:16:52.649Z
Learning: In HTML templates (e.g., src/templates/partials/media-items.html), Tailwind CSS v4.1.11 supports arbitrary numeric utilities (max-w-{n}, w-{n}, h-{n}) that map to calc(var(--spacing) * n). These should be considered valid Tailwind classes and not flagged as invalid. Ensure review tooling and linters treat such numeric utilities as allowed, and apply this guidance to other HTML/template files in the repository.
Applied to files:
src/templates/media_edit.htmlsrc/templates/partials/media-score-badge.htmlsrc/templates/partials/media-edit-button.htmlsrc/templates/partials/sidebar-nav.htmlsrc/templates/media.htmlsrc/templates/base.htmlsrc/templates/media_detail.htmlsrc/templates/backup_manage.html
📚 Learning: 2026-01-04T08:55:43.091Z
Learnt from: PascalRepond
Repo: PascalRepond/datakult PR: 36
File: src/templates/partials/media-score-badge.html:1-9
Timestamp: 2026-01-04T08:55:43.091Z
Learning: In Django projects, configuring heroicons in TEMPLATES builtins makes heroicons.templatetags.heroicons available in all templates without needing {% load heroicons %}. For template reviews in this codebase, assume heroicon tags work globally in HTML templates under templates/, so avoid requiring explicit loads. If a template still uses {% load heroicons %}, assess whether the load is unnecessary and can be removed; ensure no conflicts arise from global tags.
Applied to files:
src/templates/media_edit.htmlsrc/templates/partials/media-score-badge.htmlsrc/templates/partials/media-edit-button.htmlsrc/templates/partials/sidebar-nav.htmlsrc/templates/media.htmlsrc/templates/base.htmlsrc/templates/media_detail.htmlsrc/templates/backup_manage.html
📚 Learning: 2025-12-27T18:00:03.595Z
Learnt from: PascalRepond
Repo: PascalRepond/datakult PR: 23
File: src/templates/partials/score-readonly.html:2-33
Timestamp: 2025-12-27T18:00:03.595Z
Learning: In src/templates/partials/score-readonly.html, the DaisyUI read-only rating pattern correctly uses `aria-current="true"` on the selected star and applies the same color class (bg-orange-400) to all star elements. DaisyUI's CSS automatically handles which stars are visually filled based on the aria-current position.
Applied to files:
src/templates/partials/media-score-badge.html
🧬 Code graph analysis (2)
src/tests/core/test_views.py (1)
src/tests/conftest.py (4)
logged_in_client(65-68)media(20-31)media_factory(35-49)agent(12-16)
src/core/views.py (1)
src/core/models.py (2)
Media(108-226)Agent(94-105)
🪛 GitHub Actions: CI
src/tests/core/test_views.py
[error] 1-1: Ruff formatting check failed. Would reformat: src/tests/core/test_views.py. Run 'ruff format ./src --quiet' or commit formatted changes.
🔇 Additional comments (26)
src/theme/static_src/src/styles.css (1)
36-39: Past review comment already addressed.This code segment was previously reviewed and discussed. The user clarified that the intent is to override DaisyUI's circular checkboxes with square-shaped checkboxes that have rounded corners, which the current implementation correctly achieves.
src/templates/base.html (1)
41-41: LGTM! Icon sizing standardization applied correctly.Removing explicit size classes from the hamburger icon aligns with the broader icon sizing refactor across templates. This change relies on default icon sizing for consistency.
src/templates/partials/media-score-badge.html (1)
6-6: LGTM! Explicit width added for consistency.Adding the
w-4class ensures the star icon has explicit width to match its height, preventing potential aspect ratio issues and improving consistency.src/templates/media_edit.html (1)
169-169: LGTM! Icon sizing standardization applied consistently.Both icon changes (calendar and trash) remove explicit size classes to rely on default icon sizing, which aligns with the broader icon standardization refactor across the codebase.
Also applies to: 202-202
src/templates/partials/media-edit-button.html (1)
7-7: LGTM! Appropriate icon variant selection.Switching from
heroicon_outlinetoheroicon_miniis appropriate for this button context. The mini variant is designed for smaller UI elements, which aligns with the button's compact size.src/templates/partials/sidebar-nav.html (1)
19-132: LGTM! Icon standardization looks good.The migration to
heroicon_minivariants and removal of explicit size classes aligns with the broader icon standardization across the codebase. The changes are purely cosmetic with no impact on navigation functionality.src/templates/media_detail.html (1)
21-85: LGTM! Icon standardization is consistent.The migration to
heroicon_miniand removal of explicit size constraints is consistent with the broader icon standardization effort across templates.src/static/js/base.js (2)
42-73: LGTM! URL cleanup logic is sound.The approach of building a list of keys to delete before actually deleting them correctly avoids modifying the collection during iteration. The cleanup only updates the URL when necessary, which is efficient.
75-98: LGTM! Badge removal logic correctly handles multi-value filters.The logic appropriately distinguishes between multi-value filters (type, status, score) and single-value filters, ensuring that removing one value from a multi-value filter preserves the other values.
src/core/templatetags/media_tags.py (3)
78-110: LGTM! Query string builder is well-implemented.The template tag correctly handles multi-value parameters and provides a clean API for updating query strings. The
Nonevalue convention for parameter removal is intuitive.
113-136: LGTM! Query string exclusion is straightforward.The template tag provides a clean way to preserve query parameters while excluding specific keys, which is useful for pagination and filter reset scenarios.
139-158: LGTM! Sort direction toggle is clean.The filter provides a simple and effective way to toggle sort direction in templates. The default fallback to "review_date" is sensible.
src/tests/core/test_views.py (3)
165-190: LGTM! Search integration updates are correct.The tests have been correctly updated to reflect that search functionality is now part of the index/home view. The test logic remains sound and properly validates search behavior.
248-342: LGTM! Sorting and filter validation tests are solid.The context key changes from
order_bytosortalign with the PR's refactoring. The new test for invalid date filters demonstrates good defensive programming by ensuring the application handles malformed input gracefully without crashing.
740-755: LGTM! Contributor link assertions updated correctly.The updated assertions properly validate the shift from HTMX-driven to URL-parameter-driven contributor filtering. The test confirms that contributor links now use standard navigation with query parameters instead of HTMX interactions.
src/templates/media.html (3)
9-19: LGTM! Clean pluralization implementation.The header correctly displays item count with proper pluralization using Django's
{% translate %}tags. The conditional logic for singular/plural is straightforward and readable.
24-52: Search form state preservation looks correct.The hidden inputs properly preserve multi-value filters using inline loops, and single-value filters with conditionals. The
query_string_excludeusage for the clear button correctly removes only the search parameter while preserving other state.
180-182: LGTM!The filters drawer and media list partials are correctly included.
src/core/views.py (7)
67-78: Previous issues addressed correctly.The hardcoded French text has been replaced with
_("Not rated")for proper i18n, and error handling has been added for malformed score values. Both past review comments have been properly resolved.
22-33: LGTM!The sorting resolution is well-implemented with proper validation, fallback to default, and preservation of sort direction.
36-80: LGTM!The filter extraction properly handles multi-value parameters, builds display mappings with error handling for malformed values, and includes the new
has_reviewandhas_coverfilters.
132-146: LGTM!The score filter correctly handles the special "none" case for null scores using OR queries, and includes proper error handling for malformed values as previously requested.
149-167: LGTM!The date and content filters use
contextlib.suppressappropriately to handle malformed URL values gracefully, and the empty/filled logic correctly handles both null and empty string cases.
180-215: LGTM!The
_build_media_contexthelper effectively consolidates the common logic for listing, filtering, searching, sorting, and pagination. The tuple return pattern allows views to use the page_obj if needed while getting the full context.
218-222: LGTM!The
indexandload_more_mediaviews are now clean delegations to the centralized_build_media_contexthelper, reducing code duplication and improving maintainability.Also applies to: 285-290
src/templates/backup_manage.html (1)
26-27: LGTM! Consistent icon standardization.The heroicon updates from
heroicon_outlinetoheroicon_miniare consistent with the broader PR goal to standardize icon rendering. Based on learnings, heroicon tags are globally available in this codebase.Also applies to: 47-48, 57-58, 96-97
- Replace HTMX filter system with URL parameter-based filtering - Add multi-select support for type, status, and score filters - Consolidate search and index views into unified flow - Add new templatetags: query_string, query_string_exclude, toggle_sort_direction - Replace JS-based filter badges with server-rendered badges - Simplify JS: remove badge manipulation, use URL cleanup on page load - Refactor view logic into composable helper functions - Add OR filters for type/status/score (previously AND only) - Add has_review and has_cover filter options - Update templates to use anchor links instead of HTMX requests - Improve mobile responsive design for controls
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/templates/media.html (1)
58-78: Consider extracting inline JavaScript for better maintainability.The
onchangehandler at line 71 contains complex inline JavaScript that constructs URLs with sort parameters. While functional, this logic could be extracted into a separate JavaScript function for improved readability and testability.🔎 Example refactoring approach
Create a function in a script block or external JS file:
function updateSort(selectElement, currentSort, baseUrl) { const field = selectElement.value; const isDesc = currentSort.startsWith('-'); const sortValue = field.startsWith('-') ? field : (isDesc ? '-' : '') + field; window.location.href = baseUrl + '&sort=' + sortValue; }Then update the select:
-<select class="select join-item select-sm sm:select-md" - id="sort-field-select" - onchange="window.location.href = '?{% query_string_exclude request 'sort' 'sort_field' %}&sort=' + (this.value.startsWith('-') ? this.value : '{% if sort|slice:":1" == "-" %}-{% endif %}' + this.value)"> +<select class="select join-item select-sm sm:select-md" + id="sort-field-select" + onchange="updateSort(this, '{{ sort }}', '?{% query_string_exclude request 'sort' 'sort_field' %}')">src/templates/backup_manage.html (1)
148-153: Past issue resolved; minor JavaScript style nitpick.The trailing space in the
typeattribute has been fixed. However, line 149 has a minor spacing inconsistency:history.length> 1should behistory.length > 1(space before>).🔎 Proposed fix
- <button type="button" - onclick="history.length> 1 ? history.back() : window.location.href='{% url 'home' %}'" - class="btn btn-ghost"> + <button type="button" + onclick="history.length > 1 ? history.back() : window.location.href='{% url 'home' %}'" + class="btn btn-ghost">
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
src/core/templatetags/media_tags.pysrc/core/urls.pysrc/core/views.pysrc/static/js/base.jssrc/templates/backup_manage.htmlsrc/templates/base.htmlsrc/templates/media.htmlsrc/templates/media_detail.htmlsrc/templates/media_edit.htmlsrc/templates/partials/filters-drawer.htmlsrc/templates/partials/filters.htmlsrc/templates/partials/load-more-trigger.htmlsrc/templates/partials/media-contributors.htmlsrc/templates/partials/media-edit-button.htmlsrc/templates/partials/media-score-badge.htmlsrc/templates/partials/sidebar-nav.htmlsrc/templates/partials/view-mode-toggle.htmlsrc/tests/core/test_views.pysrc/theme/static_src/src/styles.css
💤 Files with no reviewable changes (2)
- src/templates/partials/filters.html
- src/core/urls.py
🚧 Files skipped from review as they are similar to previous changes (7)
- src/templates/partials/view-mode-toggle.html
- src/templates/partials/media-contributors.html
- src/templates/partials/media-edit-button.html
- src/templates/partials/sidebar-nav.html
- src/templates/media_edit.html
- src/templates/media_detail.html
- src/templates/partials/media-score-badge.html
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-12-26T15:18:46.932Z
Learnt from: PascalRepond
Repo: PascalRepond/datakult PR: 21
File: src/templates/accounts/profile_edit.html:23-58
Timestamp: 2025-12-26T15:18:46.932Z
Learning: In Django projects, attributes added to a form field's widget via field.widget.attrs.update(...) in the form's __init__ are rendered when using {{ form.field }} in templates. No explicit attribute definitions are needed in the template. This applies to templates under src/templates in Django apps; ensure you update attrs in __init__ for consistent HTMX behavior.
Applied to files:
src/templates/backup_manage.htmlsrc/templates/partials/filters-drawer.htmlsrc/templates/media.htmlsrc/templates/base.htmlsrc/templates/partials/load-more-trigger.html
📚 Learning: 2026-01-03T21:16:52.649Z
Learnt from: PascalRepond
Repo: PascalRepond/datakult PR: 36
File: src/templates/partials/media-items.html:67-67
Timestamp: 2026-01-03T21:16:52.649Z
Learning: In HTML templates (e.g., src/templates/partials/media-items.html), Tailwind CSS v4.1.11 supports arbitrary numeric utilities (max-w-{n}, w-{n}, h-{n}) that map to calc(var(--spacing) * n). These should be considered valid Tailwind classes and not flagged as invalid. Ensure review tooling and linters treat such numeric utilities as allowed, and apply this guidance to other HTML/template files in the repository.
Applied to files:
src/templates/backup_manage.htmlsrc/templates/partials/filters-drawer.htmlsrc/templates/media.htmlsrc/templates/base.htmlsrc/templates/partials/load-more-trigger.html
📚 Learning: 2026-01-04T08:55:43.091Z
Learnt from: PascalRepond
Repo: PascalRepond/datakult PR: 36
File: src/templates/partials/media-score-badge.html:1-9
Timestamp: 2026-01-04T08:55:43.091Z
Learning: In Django projects, configuring heroicons in TEMPLATES builtins makes heroicons.templatetags.heroicons available in all templates without needing {% load heroicons %}. For template reviews in this codebase, assume heroicon tags work globally in HTML templates under templates/, so avoid requiring explicit loads. If a template still uses {% load heroicons %}, assess whether the load is unnecessary and can be removed; ensure no conflicts arise from global tags.
Applied to files:
src/templates/backup_manage.htmlsrc/templates/partials/filters-drawer.htmlsrc/templates/media.htmlsrc/templates/base.htmlsrc/templates/partials/load-more-trigger.html
📚 Learning: 2026-01-05T21:15:38.089Z
Learnt from: PascalRepond
Repo: PascalRepond/datakult PR: 42
File: src/theme/static_src/src/styles.css:33-33
Timestamp: 2026-01-05T21:15:38.089Z
Learning: In Tailwind CSS v4.1+, `wrap-break-word` is a valid utility class from the overflow-wrap utilities set. It should not be flagged as invalid.
Applied to files:
src/theme/static_src/src/styles.css
📚 Learning: 2026-01-03T21:16:52.649Z
Learnt from: PascalRepond
Repo: PascalRepond/datakult PR: 36
File: src/templates/partials/media-items.html:67-67
Timestamp: 2026-01-03T21:16:52.649Z
Learning: The datakult project uses Tailwind CSS v4.1.11, which supports arbitrary numeric utility classes like max-w-{number}, w-{number}, h-{number}, etc., that map to calc(var(--spacing) * <number>). These numeric utilities are valid and should not be flagged as invalid Tailwind classes.
Applied to files:
src/theme/static_src/src/styles.css
🧬 Code graph analysis (3)
src/tests/core/test_views.py (1)
src/tests/conftest.py (4)
logged_in_client(65-68)media(20-31)media_factory(35-49)agent(12-16)
src/core/views.py (1)
src/core/models.py (2)
Media(108-226)Agent(94-105)
src/core/templatetags/media_tags.py (1)
src/static/js/base.js (2)
params(45-45)params(85-85)
🔇 Additional comments (25)
src/theme/static_src/src/styles.css (2)
32-34: LGTM!The
.label-text-altstyling is appropriate for form validation error labels, allowing long error messages to wrap mid-word when necessary. Thewrap-break-wordutility is valid in Tailwind CSS v4.1+ as confirmed in previous discussions.Based on learnings,
wrap-break-wordis a valid Tailwind CSS v4.1+ utility.
36-39: LGTM!The
.checkboxstyling correctly overrides DaisyUI's default circular checkboxes, applying square shapes with rounded corners. This provides consistent styling for the new multi-select filters introduced in this PR.src/static/js/base.js (2)
42-73: LGTM! Clean URL cleanup implementation.The URL cleanup logic correctly:
- Builds a deletion list before modifying params (avoiding iteration issues)
- Removes empty and default values
- Uses
history.replaceStateto avoid creating history entries
75-98: LGTM! Badge removal correctly handles multi-value parameters.The badge removal handler properly:
- Uses event delegation for dynamically added badges
- Correctly handles multi-value filters by filtering and re-appending values
- Uses full page navigation for simplicity and consistency with URL-based state management
src/templates/partials/load-more-trigger.html (2)
2-2: LGTM! Proper template tag loading.The addition of
{% load media_tags %}is necessary to use thequery_stringhelper tag.
7-7: LGTM! Query string helper correctly preserves filter state.The use of
{% query_string request page=page_obj.next_page_number %}properly preserves all current URL parameters (filters, sorting, search) while updating only the page number. This eliminates the need forhx-includeand aligns with the URL-based state management approach.Also applies to: 15-15
src/core/templatetags/media_tags.py (3)
78-110: LGTM! Query string builder correctly handles parameter updates.The
query_stringtag properly:
- Creates a mutable copy of GET parameters
- Handles parameter removal via
Nonevalue- Replaces parameter values (line 107 uses assignment, which is correct for the single-value update use case)
- Returns properly encoded query string
Note: Line 107
params[key] = valuereplaces all values for that key. For multi-value parameters where you want to append rather than replace, this behavior should be documented or the calling code should handle it (which appears to be the case in the badge removal JavaScript).
113-136: LGTM! Query string exclusion is straightforward and correct.The
query_string_excludetag properly removes specified keys and returns the encoded result.
139-158: LGTM! Sort direction toggle handles edge cases well.The
toggle_sort_directionfilter correctly:
- Defaults to "review_date" for empty values
- Toggles the leading minus sign to invert direction
- Handles both ascending and descending cases
src/tests/core/test_views.py (4)
165-190: LGTM! Test updates correctly reflect search integration.The tests now properly target the
homeroute instead of a separatesearchroute, aligning with the consolidated view architecture. The docstring update clearly documents this integration.
256-270: LGTM! Consistent rename from order_by to sort.The context key rename from
order_bytosortis applied consistently across sorting tests, matching the new URL parameter naming convention.
328-340: LGTM! Good defensive test for invalid date handling.This test ensures that invalid date filter values are gracefully ignored rather than causing errors. This is important for user experience and prevents malicious or malformed URLs from crashing the application.
750-752: LGTM! Contributor link tests updated for URL-based navigation.The assertions correctly verify:
- Contributor filter parameter in URL (line 751)
- CSS class for styling (line 752)
- Absence of HTMX attributes (line 754)
This aligns with the shift from HTMX-driven to URL-based navigation.
src/templates/base.html (1)
41-41: Icon sizing is handled by heroicons package defaults.The
heroiconsDjango package (v2.13.0) provides default sizing for all icons. The removal ofclass="w-6 h-6"is correct and consistent with codebase conventions—13+ otherheroicon_outlineusages rely on the package's default sizing without explicit size classes. Thebtn btn-square btn-ghostclasses on the label element properly maintain button dimensions for the hamburger icon.src/templates/partials/filters-drawer.html (1)
1-200: LGTM! Well-structured filters drawer implementation.The filters drawer UI is cleanly implemented with proper state preservation, correct handling of multi-select filters, and appropriate use of Django template filters for type conversion (e.g.,
stringformat:"d"at line 116 for score comparisons).src/core/views.py (6)
1-33: LGTM! Sorting logic simplified correctly.The refactored
_resolve_sortingnow returns a 2-tuple(sort_field, normalized_sort)as documented, which simplifies the interface. The implementation correctly validates sort fields and normalizes the sort direction.
36-80: Excellent! Past i18n issue resolved.The hardcoded French string has been properly replaced with
_("Not rated")at line 72, and the score conversion includes appropriate error handling (lines 74-78) to skip malformed URL values gracefully.
102-172: Excellent! Past error handling issue resolved.The
_apply_score_filternow includes proper error handling at lines 135-139 with try/except to gracefully skip malformed score values from URLs, addressing the previously flagged issue. The date validation (lines 147-148, 151-152) appropriately usescontextlib.suppressto handle malformed date inputs without crashing.
174-209: LGTM! Excellent consolidation of context building logic.The new
_build_media_contextfunction successfully centralizes the media listing, filtering, sorting, and pagination logic into a single, reusable helper. This reduces code duplication and makes the flow easier to understand and maintain.
212-216: LGTM! View logic simplified correctly.The
indexview now cleanly delegates to_build_media_context, making it concise and maintainable while leveraging the centralized context building logic.
279-284: LGTM! Consistent use of centralized context builder.The
load_more_mediaHTMX view correctly leverages the same_build_media_contexthelper, ensuring consistent behavior between the main view and infinite scroll pagination.src/templates/media.html (3)
1-19: LGTM! Clean header with proper i18n.The item count display correctly uses
page_obj.paginator.countwith appropriate singular/plural translations, and the required template tags are properly loaded.
92-174: LGTM! Consistent filter badge rendering.The active filter badges are cleanly rendered using the server-side display tuples (
type_display,status_display,score_display) from the context, with consistent removal button structure usingremove-filter-badgeclass anddata-filter/data-valueattributes for JavaScript handling.
176-177: LGTM! Clean integration of filters drawer.The filters drawer is properly included as a separate partial, maintaining good separation of concerns in the template structure.
src/templates/backup_manage.html (1)
26-26: LGTM! Icon updates align with PR-wide standardization.The heroicon variant updates (using
heroicon_minifor action icons and removing explicit size attributes) are consistent with the broader refactoring across templates in this PR.Also applies to: 47-47, 57-57, 96-96, 108-108, 151-151
1 participant
