feat(ui): persist admin table filters across HTMX pagination and partial refresh#3647
feat(ui): persist admin table filters across HTMX pagination and partial refresh#3647
Conversation
marekdano
left a comment
There was a problem hiding this comment.
Thanks for the PR, @omorros!
This PR correctly fixes the filter persistence issue across pagination with a clean, well-architected solution.
🎯 Key Strengths
-
Root Cause Correctly Identified: The issue was that loadPage() in pagination_controls.html used template-baked query_params (static Jinja2 values rendered at page load), which became stale after user interactions.
-
Elegant Solution: The fix establishes a clear data flow:
- Source of Truth: Browser URL (written by
updatePanelSearchStateInUrl()) - Pagination: Reads live state from URL via
currentUrlParams.get(livePrefix + 'q') - Rehydration: htmx:afterSettle listener restores input values after HTMX swaps
- Defense-in-Depth: The fix includes multiple layers:
- Primary: Read live URL params in loadPage() (lines 139-151 in pagination_controls.html)
- Fallback: Still reads from input elements (lines 115-137) for backward compatibility
- Rehydration: Restores inputs after HTMX swaps (lines 18743-18760 in admin.js)
- User Feedback: Added updateFilterStatus() with aria-live regions for accessibility
📋 Technical Review
pagination_controls.html (+13 lines):
✅ Correctly reads namespaced params (e.g., servers_q, tools_tags)
✅ Overrides stale template values
✅ Preserves team_id for multi-tenancy
admin.js (+65 lines):
updateFilterStatus()(lines 18703-18738):
- ✅ Reads URL params to detect active filters
- ✅ Displays filtered count from pagination controls
- ✅ Falls back to "Filters active" if count unavailable
- ✅ Clears status when no filters active
htmx:afterSettlelistener (lines 18743-18760):
- ✅ Triggers on table/pagination swaps
- ✅ Calls resetSearchInputsState() + initializeSearchInputsMemoized()
- ✅ Updates filter status after rehydration
admin.html (+6 lines):
- ✅ Added to all 6 tables
- ✅ Proper aria-live="polite" and role="status" for accessibility
🔍 Minor Observations
-
Redundant Input Reading: Lines 115-137 in pagination_controls.html still read from input elements before the URL override. This is harmless (URL wins) but could be simplified in a future refactor.
-
Event Listener Scope: The htmx:afterSettle listener triggers on any element ending with -table, -table-body, -list-container, or -pagination-controls. This is broad but safe since
initializeSearchInputsMemoized()is memoized. -
Filter Status Timing: updateFilterStatus() reads pagination totals from Alpine-rendered elements. If Alpine hasn't rendered yet, it falls back to "Filters active" (acceptable).
✅ Verification Checklist
- ✅ Fixes root cause (stale template-baked params)
- ✅ Maintains backward compatibility (still reads inputs as fallback)
- ✅ Handles all 6 admin tables (servers, tools, resources, prompts, gateways, a2a-agents)
- ✅ Preserves team_id for multi-tenancy
- ✅ Accessibility (aria-live regions)
- ✅ Clean, minimal changes (84 additions, 0 deletions)
- ✅ No breaking changes
This PR correctly fixes the filter persistence bug with a well-architected solution. The redundant input reading is harmless and can be cleaned up in a future refactor if desired.
LGTM 🚀
…ial refresh Closes IBM#2992 Signed-off-by: Oriol Morros Vilaseca <OM368@student.aru.ac.uk>
…agination (IBM#3647) Add 34 tests covering the new filter persistence code: - updateFilterStatus: filter-status span updates, per-table independence, pagination text integration, no-op safety - htmx:afterSettle rehydration listener: target ID matching for table, table-body, list-container, pagination-controls suffixes; null/empty guards - pagination_controls URL-param filter reading: namespaced q/tags reading, extraParams override precedence, team_id preservation, namespace isolation Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
dccbdc0 to
d949262
Compare
Maintainer ReviewRebased onto latest Conflict ResolutionMain recently added DOM-based search input reading in
Test Coverage AddedAdded 34 unit tests in
Full test suite: 899/899 JS tests pass, 0 regressions. Review Notes
|
…ate (IBM#3647) Add flushPendingSearchState() to close the 250ms debounce race window: when a user types in a search input and clicks pagination before the debounced URL update fires, loadPage() now flushes pending timers first so the URL reflects the live input values. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
crivetimihai
left a comment
There was a problem hiding this comment.
LGTM — rebased, reviewed, and hardened.
Changes made on top of the original commit:
- Conflict resolution — kept URL-based filter reading (correct), dropped main's DOM-based approach (had catalog/a2a-agents input ID mismatch bug)
- Debounce race fix — added
flushPendingSearchState()so paginating within the 250ms debounce window still picks up live input values viaPANEL_SEARCH_CONFIG(correct IDs for all 6 tables) - 40 new tests —
updateFilterStatus,htmx:afterSettlerehydration, URL-param filter reading, and flush function coverage
905/905 JS tests pass, all linters clean.
… contexts (IBM#3647) Add getLiveSearchState() that reads DOM inputs via PANEL_SEARCH_CONFIG (correct IDs for all 6 tables including catalog and a2a-agents). Used as a fallback by loadPage() when URL params are empty — covers sandboxed iframe contexts where safeReplaceState is a no-op and the URL is never updated. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
Additional fix: DOM fallback for restricted iframe contextsOn review, the URL-only approach would regress filter persistence in embedded/iframe deployments where Fix: Added
14 additional tests covering 919/919 JS tests pass (1 pre-existing flaky gantt-chart test occasionally fails in parallel runs, passes in isolation). |
…back (IBM#3647) Invert the precedence in loadPage(): read DOM inputs first (always live, works in restricted iframe contexts where safeReplaceState is a no-op and the URL stays stale), fall back to URL params only when DOM inputs have no values (fresh page load from bookmarked URL before inputs are populated). This fixes the case where the URL has stale tools_q=alpha but the user has typed beta — previously URL took precedence and the DOM fallback was skipped because liveQuery was truthy. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
Fix: DOM-first precedence for filter readsThe previous layering was wrong — URL params took precedence over DOM inputs, so in restricted iframe contexts where Inverted the precedence in
Tests updated: the old test "URL params take precedence over DOM fallback" is now correctly "DOM takes precedence over stale URL params (restricted context fix)". Added test for "DOM wins over both stale URL and stale extraParams". 920/920 JS tests pass. |
🔗 Related Issue
Closes #2992 Replaces #3204 (could not push conflict resolution to upstream branch). Original work from #3017 and #3100.
📝 Summary
Persist admin table filters (search text, tag filters, "show inactive" toggles) across HTMX pagination clicks and
partial refreshes for all 6 admin tables.
Root cause:
pagination_controls.htmlloadPage()built HTMX request URLs using template-bakedquery_params(static Jinja2 values). When pagination controls were rendered before a search, clicking page 2 dropped the activefilters.
Changes:
pagination_controls.html—loadPage()now reads live namespacedq/tagsfrom the browser URL (source of truthwritten by
updatePanelSearchStateInUrl), overriding stale template-baked values.admin.js— Addedhtmx:afterSettlelistener that rehydrates search inputs from URL state after content swaps.Added
updateFilterStatus()for visible filtered row count text. -admin.html— Addedaria-live="polite"status spans next to all 6 table search bars (servers, tools, resources,prompts, gateways, a2a-agents).
🏷️ Type of Change
🧪 Verification
npx vitest runmake testmake lintmake coverage✅ Checklist
make black isort pre-commit)📓 Notes (optional)
Rebased on latest
mainto resolve merge conflicts from #3204. The code and functionality are identical to thepreviously approved PR.