From e6492e49953c1cb73a57bc1dc2448e14bff85887 Mon Sep 17 00:00:00 2001 From: Andreas Eiselt Date: Tue, 22 Sep 2026 18:23:21 +0200 Subject: [PATCH] Keep a filter row the user added visible across re-renders Adding a list filter unhides its row on the client. Every later render of the form comes from the server, which renders a row hidden unless the query holds that filter, so the row the user just added is hidden again. Selecting "Meeting status" on the meetings index is impossible this way, and meetings_index_spec.rb:203 has been failing on dev for it. Sending the row with an empty value would get it rendered, but it would also install a filter the query rejects: Queries::Filters::Base requires a value for an operator that needs one, so BaseQuery#results falls back to an empty scope and the list blanks out until a value is picked. The controller keeps the names of the rows it added and restores them after each stream render instead. Nothing invalid reaches the query, the result set stays as it was, and a row stops being tracked as soon as the server renders it visible, which means the query holds it by then. https://community.openproject.org/wp/AGILE-432 --- .../filter/filters-form.controller.spec.ts | 77 +++++++++++++++++++ .../dynamic/filter/filters-form.controller.ts | 42 ++++++++++ 2 files changed, 119 insertions(+) diff --git a/frontend/src/stimulus/controllers/dynamic/filter/filters-form.controller.spec.ts b/frontend/src/stimulus/controllers/dynamic/filter/filters-form.controller.spec.ts index 62b63c385d4b..d51db287b763 100644 --- a/frontend/src/stimulus/controllers/dynamic/filter/filters-form.controller.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/filter/filters-form.controller.spec.ts @@ -154,3 +154,80 @@ describe('Filters form controller - filter count badge', () => { }); }); }); + +describe('Filters form controller - rows added without a value', () => { + let ctx:StimulusTestContext; + let FiltersFormController:typeof FiltersFormControllerType; + + beforeAll(async () => { + ({ default: FiltersFormController } = await import('./filters-form.controller')); + }); + + afterEach(() => { + ctx.dispose(); + }); + + async function mountForm() { + ctx = await setupStimulusTest({ + controllers: { 'filter--filters-form': FiltersFormController }, + }); + + await ctx.mount(` +
+ + ${ASSIGNEE_FILTER_ROW} +
+ `); + + return ctx.getController('filter--filters-form'); + } + + function row() { + return ctx.container.querySelector('[data-filter--filters-form-target="filter"]')!; + } + + function addFilterOption() { + return ctx.container.querySelector('option[value="assignee"]')!; + } + + it('shows the row again after a re-render hid it', async () => { + const controller = await mountForm(); + controller.addFilterByName('assignee'); + + // What the server sends back: the query does not hold the filter, so the row is hidden + // and its entry in the add-filter select is selectable again. + row().setAttribute('hidden', ''); + addFilterOption().removeAttribute('disabled'); + + controller.restorePendingFilters(); + + expect(row().hasAttribute('hidden')).toBe(false); + expect(addFilterOption().hasAttribute('disabled')).toBe(true); + }); + + it('leaves a row the user removed hidden', async () => { + const controller = await mountForm(); + controller.addFilterByName('assignee'); + controller.removeFilter({ params: { filterName: 'assignee' } }); + + controller.restorePendingFilters(); + + expect(row().hasAttribute('hidden')).toBe(true); + }); + + it('stops tracking a row once the server renders it itself', async () => { + const controller = await mountForm(); + controller.addFilterByName('assignee'); + + // A re-render that leaves the row visible means the query holds the filter now, so the + // row stops being this controller's business and a later re-render decides on its own. + controller.restorePendingFilters(); + row().setAttribute('hidden', ''); + controller.restorePendingFilters(); + + expect(row().hasAttribute('hidden')).toBe(true); + }); +}); diff --git a/frontend/src/stimulus/controllers/dynamic/filter/filters-form.controller.ts b/frontend/src/stimulus/controllers/dynamic/filter/filters-form.controller.ts index 81aaf1511bd4..9cc7bb42362f 100644 --- a/frontend/src/stimulus/controllers/dynamic/filter/filters-form.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/filter/filters-form.controller.ts @@ -84,6 +84,7 @@ export default class FiltersFormController extends Controller { declare readonly hasFilterFormToggleTarget:boolean; declare readonly hasFiltersInputTarget:boolean; + declare readonly hasAddFilterSelectTarget:boolean; static values = { displayFilters: { type: Boolean, default: false }, @@ -117,6 +118,11 @@ export default class FiltersFormController extends Controller { private boundClearListener:(event:MouseEvent) => void; private sentFilters:string|null = null; + // Rows the user added that have no value yet. They are kept visible here instead of being + // sent: the server hides a row its query does not hold, and a blank value would make that + // query invalid, which empties the result set. + private readonly pendingFilters = new Set(); + initialize() { // Initialize runs anytime an element with a controller connected to the DOM for the first time this.boundListener = debounce(this.sendFormLive.bind(this), 300); @@ -298,6 +304,7 @@ export default class FiltersFormController extends Controller { const selectedFilter = this.findTargetByName(filterName, this.filterTargets); if (selectedFilter) { selectedFilter.removeAttribute('hidden'); + this.pendingFilters.add(filterName); } this.addFilterSelectTarget.selectedOptions[0].disabled = true; this.addFilterSelectTarget.selectedIndex = 0; @@ -307,6 +314,39 @@ export default class FiltersFormController extends Controller { this.sendFormLive(); } + // A re-render renders each row from what the query holds, so it hides the pending ones again. + // Run after every stream render, and for rows that come back as new nodes rather than morphed. + restorePendingFilters() { + this.pendingFilters.forEach((filterName) => { + const row = this.findTargetByName(filterName, this.filterTargets); + + if (!row?.hasAttribute('hidden')) { + // The row is gone, or the query holds it now and the server renders it visible itself. + this.pendingFilters.delete(filterName); + return; + } + + this.showPendingFilter(row, filterName); + }); + } + + filterTargetConnected(target:HTMLElement) { + const filterName = target.getAttribute('data-filter-name'); + + if (filterName && this.pendingFilters.has(filterName) && target.hasAttribute('hidden')) { + this.showPendingFilter(target, filterName); + } + } + + private showPendingFilter(row:HTMLElement, filterName:string) { + row.removeAttribute('hidden'); + + if (!this.hasAddFilterSelectTarget) return; + + const option = Array.from(this.addFilterSelectTarget.options).find((candidate) => candidate.value === filterName); + option?.setAttribute('disabled', 'disabled'); + } + focusFilterValueIfPossible(element:undefined|HTMLElement) { const filterName = element?.getAttribute('data-filter-name'); if (!filterName) return; @@ -343,6 +383,7 @@ export default class FiltersFormController extends Controller { removeFilter({ params: { filterName } }:{ params:{ filterName:string } }) { const filterToRemove = this.findTargetByName(filterName, this.filterTargets); filterToRemove?.setAttribute('hidden', ''); + this.pendingFilters.delete(filterName); const selectOptions = Array.from(this.addFilterSelectTarget.options); const removedFilterOption = selectOptions.find((option) => option.value === filterName); @@ -506,6 +547,7 @@ export default class FiltersFormController extends Controller { .then((response:Response) => response.text()) .then((html:string) => { renderStreamMessage(html); + this.restorePendingFilters(); if (this.sentFilters === newFilters) { window.history.replaceState(window.history.state, '', browserUrl); }