From bf6dc0f058b73bd445223d6adf032f04ae8f310c Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Tue, 22 Sep 2026 14:23:19 +0100 Subject: [PATCH 1/7] [AGILE-432] Keep an added filter row visible Since the AGILE-432 fix a row added without a value is left out of the request, so a form that the response morphs (Meetings) renders it hidden again. The controller now marks such rows and stops the morph from hiding them, instead of sending an empty value the server rejects as an invalid filter. A restored page drops the marked rows again, as Turbo snapshots them before the controller disconnects. An empty date range counts as no value, so a fresh "Dates interval" row is kept the same way. Restores the Meeting status example in meetings_index_spec. https://community.openproject.org/wp/AGILE-432 --- ...lters-form-pending-rows.controller.spec.ts | 380 ++++++++++++++++++ .../dynamic/filter/filters-form.controller.ts | 89 +++- spec/features/projects/lists/filters_spec.rb | 14 +- 3 files changed, 471 insertions(+), 12 deletions(-) create mode 100644 frontend/src/stimulus/controllers/dynamic/filter/filters-form-pending-rows.controller.spec.ts diff --git a/frontend/src/stimulus/controllers/dynamic/filter/filters-form-pending-rows.controller.spec.ts b/frontend/src/stimulus/controllers/dynamic/filter/filters-form-pending-rows.controller.spec.ts new file mode 100644 index 000000000000..339acf7c2add --- /dev/null +++ b/frontend/src/stimulus/controllers/dynamic/filter/filters-form-pending-rows.controller.spec.ts @@ -0,0 +1,380 @@ +//-- copyright +// OpenProject is an open source project management software. +// Copyright (C) the OpenProject GmbH +// +// This program is free software; you can redistribute it and/or +// modify it under the terms of the GNU General Public License version 3. +// +// OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +// Copyright (C) 2006-2013 Jean-Philippe Lang +// Copyright (C) 2010-2013 the ChiliProject Team +// +// This program is free software; you can redistribute it and/or +// modify it under the terms of the GNU General Public License +// as published by the Free Software Foundation; either version 2 +// of the License, or (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with this program; if not, write to the Free Software +// Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +// +// See COPYRIGHT and LICENSE files for more details. +//++ + +import * as Turbo from '@hotwired/turbo'; +import type { MockInstance } from 'vitest'; +import { setupStimulusTest, type StimulusTestContext } from 'core-stimulus/test-helpers'; +import type FiltersFormControllerType from './filters-form.controller'; + +interface VisitingSession { + visit:(location:string, options:object) => void; +} + +interface StatusRow { + hidden:boolean; + disabled:boolean; + value:string; +} + +const UNKNOWN_TO_SERVER:StatusRow = { hidden: true, disabled: false, value: '' }; +const APPLIED_ON_SERVER:StatusRow = { hidden: false, disabled: true, value: '2' }; + +describe('Filters form pending rows', () => { + let ctx:StimulusTestContext; + let Controller:typeof FiltersFormControllerType; + let originalUrl:string; + let visit:MockInstance; + + beforeAll(async () => { + ({ default: Controller } = await import('./filters-form.controller')); + }); + + beforeEach(() => { + originalUrl = window.location.href; + visit = vi.spyOn(Turbo.session as unknown as VisitingSession, 'visit').mockImplementation(() => undefined); + }); + + afterEach(() => { + vi.restoreAllMocks(); + ctx.dispose(); + window.history.replaceState(window.history.state, '', originalUrl); + }); + + function form(statusRow:StatusRow) { + return `
+ +
+ +
+ +
+
+ +
`; + } + + async function mount(html = form(UNKNOWN_TO_SERVER)) { + ctx = await setupStimulusTest({ controllers: { 'filter--filters-form': Controller } }); + await ctx.mount(html); + return ctx.getController('filter--filters-form'); + } + + function host():HTMLElement { + return ctx.container.querySelector('[data-controller]')!; + } + + // getByRole returns HTMLElement; the generic narrows it where `.value` is read or written. + const addFilterSelect = () => ctx.screen.getByRole('combobox', { name: 'Add filter' }); + const statusOperator = () => ctx.screen.getByRole('combobox', { name: 'Status operator', hidden: true }); + const subjectOperator = () => ctx.screen.getByRole('combobox', { name: 'Subject operator', hidden: true }); + const subjectRow = () => subjectOperator().closest('[data-filter--filters-form-target="filter"]')!; + const statusOption = () => ctx.screen.getByRole('option', { name: 'Status', hidden: true }); + const subjectOption = () => ctx.screen.getByRole('option', { name: 'Subject', hidden: true }); + const statusValue = () => ctx.container.querySelector('[data-filter-name="status_id"] input[name="value"]')!; + const subjectValue = () => ctx.screen.getByRole('textbox', { name: 'Subject value', hidden: true }); + + function addStatusFilter(controller:FiltersFormControllerType) { + addFilterSelect().value = 'status_id'; + controller.addFilterByName('status_id'); + } + + function selectStatus(id:string) { + statusValue().value = id; + } + + function rerenderFromServer(statusRow:StatusRow) { + const template = document.createElement('template'); + template.innerHTML = form(statusRow); + Turbo.morphElements(host(), template.content.firstElementChild!); + } + + it('keeps an added row visible through a re-render that does not know it yet', async () => { + const controller = await mount(); + + addStatusFilter(controller); + expect(statusOperator()).toBeVisible(); + expect(statusOption()).toBeDisabled(); + + // Two morphs: Idiomorph syncs the incoming attributes (hidden) before it removes the ones + // only the current element has (the marker), so a single morph passes even when the + // marker itself is not protected. + rerenderFromServer(UNKNOWN_TO_SERVER); + rerenderFromServer(UNKNOWN_TO_SERVER); + + expect(statusOperator()).toBeVisible(); + expect(statusOption()).toBeDisabled(); + }); + + it('hands the row to the server once it has been submitted with a value', async () => { + const controller = await mount(); + addStatusFilter(controller); + + selectStatus('2'); + controller.autocompleteSendForm(); + rerenderFromServer(APPLIED_ON_SERVER); + + expect(statusOperator()).toBeVisible(); + expect(statusOption()).toBeDisabled(); + expect(statusValue()).toHaveValue('2'); + }); + + it('no longer protects a submitted row from a response that drops it', async () => { + const controller = await mount(); + addStatusFilter(controller); + + selectStatus('2'); + controller.autocompleteSendForm(); + rerenderFromServer(UNKNOWN_TO_SERVER); + + expect(statusOperator()).not.toBeVisible(); + expect(statusOption()).toBeEnabled(); + }); + + it('lets the server own a row that was removed again', async () => { + const controller = await mount(); + addStatusFilter(controller); + + controller.removeFilter({ params: { filterName: 'status_id' } }); + rerenderFromServer(UNKNOWN_TO_SERVER); + + expect(statusOperator()).not.toBeVisible(); + expect(statusOption()).toBeEnabled(); + }); + + it('does not keep other rows from being hidden by the server', async () => { + const controller = await mount(); + addStatusFilter(controller); + + // The operator select carries data-filter-name too, so the row is reached via its target attribute. + subjectRow().hidden = false; + expect(subjectOperator()).toBeVisible(); + + rerenderFromServer(UNKNOWN_TO_SERVER); + + expect(subjectOperator()).not.toBeVisible(); + expect(statusOperator()).toBeVisible(); + }); + + it('takes the add-filter option without the caller selecting it first', async () => { + const controller = await mount(); + + controller.addFilterByName('status_id'); + + expect(statusOption()).toBeDisabled(); + expect(addFilterSelect()).toHaveValue(''); + expect(statusOperator()).toBeVisible(); + }); + + it('drops an unsent row when a cached page is restored, keeping applied rows', async () => { + // The server applied "subject ~ x": row visible, option taken, value present. + const controller = await mount(form(UNKNOWN_TO_SERVER) + .replace('data-filter-name="subject" data-filter-type="string" hidden', 'data-filter-name="subject" data-filter-type="string"') + .replace('', '') + .replace(' { + const controller = await mount(); + addFilterSelect().value = 'subject'; + controller.addFilterByName('subject'); + subjectValue().value = 'unsent draft'; + + // Turbo's snapshot clone carries live control values, not just markup. `element` is not + // part of the published typings. + const { element: cached } = Turbo.PageSnapshot.fromElement(host()).clone() as unknown as { element:HTMLElement }; + ctx.dispose(); + ctx = await setupStimulusTest({ controllers: { 'filter--filters-form': Controller } }); + ctx.container.append(cached); + await ctx.nextFrame(); + const restored = ctx.getController('filter--filters-form'); + + expect(subjectOperator()).not.toBeVisible(); + expect(subjectValue()).toHaveValue(''); + + addFilterSelect().value = 'subject'; + restored.addFilterByName('subject'); + + expect(subjectOperator()).toBeVisible(); + expect(visit).not.toHaveBeenCalled(); + }); + + describe('with a date range row', () => { + function dateRow(rangeValue:string) { + return `
+ + +
`; + } + + const rangeOperator = () => ctx.screen.getByRole('combobox', { name: 'Dates interval operator', hidden: true }); + const rangeInput = () => ctx.screen.getByRole('textbox', { name: 'Dates interval range', hidden: true }); + + function rerenderDateRow() { + const template = document.createElement('template'); + template.innerHTML = dateRow('-'); + Turbo.morphElements(host(), template.content.firstElementChild!); + } + + async function addDatesInterval(rangeValue:string) { + const controller = await mount(dateRow(rangeValue)); + addFilterSelect().value = 'dates_interval'; + controller.addFilterByName('dates_interval'); + return controller; + } + + // The picker renders an empty range as "-"; a cleared picker leaves "". + it.each(['-', ''])('keeps a range row pending while its input holds %j', async (rangeValue) => { + const controller = await addDatesInterval(rangeValue); + + expect(controller.serializedFiltersWith()).toBe(''); + + rerenderDateRow(); + + expect(rangeOperator()).toBeVisible(); + expect(controller.serializedFiltersWith()).toBe(''); + }); + + it.each([ + ['2026-09-01 - ', '["2026-09-01",""]'], + [' - 2026-09-30', '["","2026-09-30"]'], + ])('still submits the one-sided range %j', async (rangeValue, serialized) => { + const controller = await addDatesInterval('-'); + + rangeInput().value = rangeValue; + controller.sendForm(); + + expect(controller.serializedFiltersWith()).toBe(`dates_interval <>d ${serialized}`); + }); + + it('releases the range row once both dates are set', async () => { + const controller = await addDatesInterval('-'); + + rangeInput().value = '2026-09-01 - 2026-09-30'; + controller.sendForm(); + expect(controller.serializedFiltersWith()).toBe('dates_interval <>d ["2026-09-01","2026-09-30"]'); + + rerenderDateRow(); + + expect(rangeOperator()).not.toBeVisible(); + expect(controller.serializedFiltersWith()).toBe(''); + }); + }); + + describe('on the Meetings transport (JSON filters, stream morph of the subheader)', () => { + function subheader(statusRow:StatusRow) { + return form(statusRow) + .replace('data-filter--filters-form-turbo-frame-request-value="backlogs_container"', + 'data-filter--filters-form-turbo-stream-request-value="true" data-filter--filters-form-output-format-value="json"'); + } + + function streamResponse(statusRow:StatusRow) { + return ` + + `; + } + + let fetchSpy:ReturnType; + + beforeEach(() => { + document.body.insertAdjacentHTML('beforeend', ''); + fetchSpy = vi.fn((_url:string) => Promise.resolve(new Response(streamResponse(UNKNOWN_TO_SERVER)))); + vi.stubGlobal('fetch', fetchSpy); + }); + + afterEach(() => { + vi.unstubAllGlobals(); + document.getElementById('global-loading-indicator')?.remove(); + }); + + it('keeps the added row through the morphed response and never sends it', async () => { + ctx = await setupStimulusTest({ controllers: { 'filter--filters-form': Controller } }); + await ctx.mount(`
${subheader(UNKNOWN_TO_SERVER)}
`); + const controller = ctx.getController('filter--filters-form'); + window.history.replaceState(window.history.state, '', `${window.location.pathname}?filters=[]`); + + addStatusFilter(controller); + // Adding the empty row alone changes nothing the server would receive, so sendForm() + // returns early; an unrelated (unowned) filter change forces the request. + controller.currentFiltersValue = [{ title: { operator: '~', values: ['x'] } }]; + controller.sendForm(); + + await vi.waitFor(() => expect(ctx.container.querySelector('[data-rendered]')).not.toBeNull()); + + expect(fetchSpy).toHaveBeenCalledTimes(1); + const filtersParam = new URL(String(fetchSpy.mock.calls[0][0]), window.location.origin).searchParams.get('filters'); + expect(filtersParam).toBe('[{"title":{"operator":"~","values":["x"]}}]'); + expect(filtersParam).not.toContain('status_id'); + expect(statusOperator()).toBeVisible(); + expect(statusOption()).toBeDisabled(); + }); + }); +}); 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..bd1dead08bdd 100644 --- a/frontend/src/stimulus/controllers/dynamic/filter/filters-form.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/filter/filters-form.controller.ts @@ -27,7 +27,7 @@ //++ import { Controller } from '@hotwired/stimulus'; -import { renderStreamMessage, visit } from '@hotwired/turbo'; +import { renderStreamMessage, visit, type TurboBeforeMorphAttributeEvent } from '@hotwired/turbo'; import { debounce } from 'lodash-es'; import { hideElement, @@ -50,6 +50,10 @@ type SerializedFilter = Record; type FilterFunc = (_value:T) => boolean; +// Marks a row the user added that holds no value yet. It is not part of any request, so a +// re-render of the form would hide it again; the morph guard keeps marked rows as they are. +const PENDING_ATTRIBUTE = 'data-filter-pending'; + export default class FiltersFormController extends Controller { static targets = [ 'filterFormToggle', @@ -115,6 +119,7 @@ export default class FiltersFormController extends Controller { private boundListener:() => void; private boundClearListener:(event:MouseEvent) => void; + private boundKeepPendingRows:(event:TurboBeforeMorphAttributeEvent) => void; private sentFilters:string|null = null; initialize() { @@ -122,16 +127,23 @@ export default class FiltersFormController extends Controller { this.boundListener = debounce(this.sendFormLive.bind(this), 300); this.boundClearListener = (event:MouseEvent) => this.clearInputWithButton(event); + this.boundKeepPendingRows = (event:TurboBeforeMorphAttributeEvent) => this.keepPendingRows(event); } connect() { const clearButton = document.getElementById(this.clearButtonIdValue); clearButton?.addEventListener('click', this.boundClearListener); + this.element.addEventListener('turbo:before-morph-attribute', this.boundKeepPendingRows); + + // A restored page brings its markup back but not this controller's model, so a marker + // already in the DOM here belongs to whoever was cached. + this.pendingRows().forEach((row) => this.releasePendingRow(row, { hide: true })); } disconnect() { const clearButton = document.getElementById(this.clearButtonIdValue); clearButton?.removeEventListener('click', this.boundClearListener); + this.element.removeEventListener('turbo:before-morph-attribute', this.boundKeepPendingRows); } addFilterSelectTargetConnected() { @@ -298,8 +310,9 @@ export default class FiltersFormController extends Controller { const selectedFilter = this.findTargetByName(filterName, this.filterTargets); if (selectedFilter) { selectedFilter.removeAttribute('hidden'); + selectedFilter.setAttribute(PENDING_ATTRIBUTE, ''); } - this.addFilterSelectTarget.selectedOptions[0].disabled = true; + this.setFilterOptionTaken(filterName, true); this.addFilterSelectTarget.selectedIndex = 0; this.focusFilterValueIfPossible(selectedFilter); @@ -342,15 +355,65 @@ export default class FiltersFormController extends Controller { removeFilter({ params: { filterName } }:{ params:{ filterName:string } }) { const filterToRemove = this.findTargetByName(filterName, this.filterTargets); - filterToRemove?.setAttribute('hidden', ''); - - const selectOptions = Array.from(this.addFilterSelectTarget.options); - const removedFilterOption = selectOptions.find((option) => option.value === filterName); - removedFilterOption?.removeAttribute('disabled'); + if (filterToRemove) { + filterToRemove.setAttribute('hidden', ''); + filterToRemove.removeAttribute(PENDING_ATTRIBUTE); + } + this.setFilterOptionTaken(filterName, false); this.sendFormLive(); } + private setFilterOptionTaken(filterName:string, taken:boolean) { + const option = Array.from(this.addFilterSelectTarget.options).find((candidate) => candidate.value === filterName); + if (option) { + option.disabled = taken; + } + } + + private pendingRows():HTMLElement[] { + return this.filterTargets.filter((row) => row.hasAttribute(PENDING_ATTRIBUTE)); + } + + private releasePendingRow(row:HTMLElement, { hide }:{ hide:boolean }) { + row.removeAttribute(PENDING_ATTRIBUTE); + if (hide) { + row.setAttribute('hidden', ''); + this.setFilterOptionTaken(row.dataset.filterName!, false); + this.resetControls(row); + } + } + + // Turbo's page snapshot keeps live control values, so a hidden row would otherwise still + // carry the draft the user typed and submit it the moment the filter is added again. + private resetControls(row:HTMLElement) { + row.querySelectorAll('input, select, textarea').forEach((control) => { + if (control instanceof HTMLSelectElement) { + Array.from(control.options).forEach((option) => { option.selected = option.defaultSelected; }); + } else if (control instanceof HTMLInputElement && (control.type === 'checkbox' || control.type === 'radio')) { + control.checked = control.defaultChecked; + } else { + control.value = control.defaultValue; + } + }); + } + + private keepPendingRows(event:TurboBeforeMorphAttributeEvent) { + const { attributeName } = event.detail; + const target = event.target as HTMLElement; + + const isPendingRow = this.filterTargets.includes(target) && target.hasAttribute(PENDING_ATTRIBUTE); + const keepsRow = isPendingRow && (attributeName === 'hidden' || attributeName === PENDING_ATTRIBUTE); + const keepsOptionTaken = attributeName === 'disabled' + && target instanceof HTMLOptionElement + && target.closest('select') === this.addFilterSelectTarget + && this.pendingRows().some((row) => row.dataset.filterName === target.value); + + if (keepsRow || keepsOptionTaken) { + event.preventDefault(); + } + } + clearInputWithButton(event:MouseEvent) { // Primer does not trigger an input event when clearing the value of the input field unless // it is focused. This handler will find the sibling input of the clear button inside the @@ -451,6 +514,11 @@ export default class FiltersFormController extends Controller { } sendForm() { + const submitted = new Set(this.parseFilters().map((filter) => filter.name)); + this.pendingRows() + .filter((row) => submitted.has(row.dataset.filterName!)) + .forEach((row) => this.releasePendingRow(row, { hide: false })); + // When we want the filter content to be written to a hidden input, do this. // When we do not also want the turbo requests, we can exit early here. Otherwise the automatic redirect // would also be triggered. We do not want this in the case where we use the filter input in another form @@ -644,10 +712,11 @@ export default class FiltersFormController extends Controller { value = [dateValue].filter((v) => v !== ''); } else if (operator === this.betweenDatesOperator) { - const rangeValue = this.findTargetById(filterName, this.dateRangeTargets)?.value; - const [fromValue, toValue] = rangeValue?.split(' - ') ?? []; + // The range picker renders an empty range as "-" (see Filters::Inputs::DateForm#between_dates_div). + const rangeValue = this.findTargetById(filterName, this.dateRangeTargets)?.value ?? ''; + const [fromValue = '', toValue = ''] = rangeValue === '-' ? [] : rangeValue.split(' - '); - value = [fromValue, toValue]; + value = fromValue === '' && toValue === '' ? [] : [fromValue, toValue]; } if (value && value.length > 0) { return value; diff --git a/spec/features/projects/lists/filters_spec.rb b/spec/features/projects/lists/filters_spec.rb index 8325603f6469..7c60e3500500 100644 --- a/spec/features/projects/lists/filters_spec.rb +++ b/spec/features/projects/lists/filters_spec.rb @@ -444,10 +444,20 @@ def load_and_open_filters(user) project_created_on_this_week, project_created_on_fixed_date) + # a range row without dates yet is not a filter: the list and the URL stay as they are projects_page.set_filter("created_at", "Created on", - "between", - ["2017-11-10", "2017-11-12"]) + "between") + wait_for_network_idle + + projects_page.expect_projects_listed(project_created_on_today, + project_created_on_this_week, + project_created_on_fixed_date) + expect(page).to have_no_current_path(/created_at/) + + projects_page.within_filter("created_at") do + projects_page.set_datetime_filter("created_at", "between", ["2017-11-10", "2017-11-12"]) + end projects_page.expect_projects_not_listed(project_created_on_today) projects_page.expect_projects_listed(project_created_on_fixed_date) From dbb3e8efc74ee01027618cec469a029aa3066d84 Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Tue, 22 Sep 2026 17:13:06 +0100 Subject: [PATCH 2/7] Detach filter form listeners via AbortController Replaces the stored bound handlers for the clear button and the morph guard with one AbortController that disconnect aborts. Drops the second getElementById lookup and the initialize-time bindings those needed. --- .../dynamic/filter/filters-form.controller.ts | 21 ++++++++----------- 1 file changed, 9 insertions(+), 12 deletions(-) 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 bd1dead08bdd..474d82607091 100644 --- a/frontend/src/stimulus/controllers/dynamic/filter/filters-form.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/filter/filters-form.controller.ts @@ -118,22 +118,21 @@ export default class FiltersFormController extends Controller { }); private boundListener:() => void; - private boundClearListener:(event:MouseEvent) => void; - private boundKeepPendingRows:(event:TurboBeforeMorphAttributeEvent) => void; + private abortController?:AbortController; private sentFilters:string|null = null; 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); - - this.boundClearListener = (event:MouseEvent) => this.clearInputWithButton(event); - this.boundKeepPendingRows = (event:TurboBeforeMorphAttributeEvent) => this.keepPendingRows(event); } connect() { + this.abortController = new AbortController(); + const { signal } = this.abortController; + const clearButton = document.getElementById(this.clearButtonIdValue); - clearButton?.addEventListener('click', this.boundClearListener); - this.element.addEventListener('turbo:before-morph-attribute', this.boundKeepPendingRows); + clearButton?.addEventListener('click', (event) => this.clearInputWithButton(event), { signal }); + this.element.addEventListener('turbo:before-morph-attribute', this.keepPendingRows, { signal }); // A restored page brings its markup back but not this controller's model, so a marker // already in the DOM here belongs to whoever was cached. @@ -141,9 +140,7 @@ export default class FiltersFormController extends Controller { } disconnect() { - const clearButton = document.getElementById(this.clearButtonIdValue); - clearButton?.removeEventListener('click', this.boundClearListener); - this.element.removeEventListener('turbo:before-morph-attribute', this.boundKeepPendingRows); + this.abortController?.abort(); } addFilterSelectTargetConnected() { @@ -398,7 +395,7 @@ export default class FiltersFormController extends Controller { }); } - private keepPendingRows(event:TurboBeforeMorphAttributeEvent) { + private readonly keepPendingRows = (event:TurboBeforeMorphAttributeEvent) => { const { attributeName } = event.detail; const target = event.target as HTMLElement; @@ -412,7 +409,7 @@ export default class FiltersFormController extends Controller { if (keepsRow || keepsOptionTaken) { event.preventDefault(); } - } + }; clearInputWithButton(event:MouseEvent) { // Primer does not trigger an input event when clearing the value of the input field unless From 508168d917c48c6eb8b31327d5a52ac94b37445c Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Thu, 24 Sep 2026 08:55:59 +0100 Subject: [PATCH 3/7] Move resetControls out of the filter controller The helper only touches the row it is given and never reads controller state, so it lives as a module function next to the pending marker it serves rather than as a private method. --- .../dynamic/filter/filters-form.controller.ts | 30 +++++++++---------- 1 file changed, 15 insertions(+), 15 deletions(-) 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 474d82607091..04377747e235 100644 --- a/frontend/src/stimulus/controllers/dynamic/filter/filters-form.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/filter/filters-form.controller.ts @@ -54,6 +54,20 @@ type FilterFunc = (_value:T) => boolean; // re-render of the form would hide it again; the morph guard keeps marked rows as they are. const PENDING_ATTRIBUTE = 'data-filter-pending'; +// Turbo's page snapshot keeps live control values, so a hidden row would otherwise still +// carry the draft the user typed and submit it the moment the filter is added again. +function resetControls(row:HTMLElement) { + row.querySelectorAll('input, select, textarea').forEach((control) => { + if (control instanceof HTMLSelectElement) { + Array.from(control.options).forEach((option) => { option.selected = option.defaultSelected; }); + } else if (control instanceof HTMLInputElement && (control.type === 'checkbox' || control.type === 'radio')) { + control.checked = control.defaultChecked; + } else { + control.value = control.defaultValue; + } + }); +} + export default class FiltersFormController extends Controller { static targets = [ 'filterFormToggle', @@ -377,24 +391,10 @@ export default class FiltersFormController extends Controller { if (hide) { row.setAttribute('hidden', ''); this.setFilterOptionTaken(row.dataset.filterName!, false); - this.resetControls(row); + resetControls(row); } } - // Turbo's page snapshot keeps live control values, so a hidden row would otherwise still - // carry the draft the user typed and submit it the moment the filter is added again. - private resetControls(row:HTMLElement) { - row.querySelectorAll('input, select, textarea').forEach((control) => { - if (control instanceof HTMLSelectElement) { - Array.from(control.options).forEach((option) => { option.selected = option.defaultSelected; }); - } else if (control instanceof HTMLInputElement && (control.type === 'checkbox' || control.type === 'radio')) { - control.checked = control.defaultChecked; - } else { - control.value = control.defaultValue; - } - }); - } - private readonly keepPendingRows = (event:TurboBeforeMorphAttributeEvent) => { const { attributeName } = event.detail; const target = event.target as HTMLElement; From 49f0bc64a2f1bbe028ba45116164e07d8b1becc9 Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Thu, 24 Sep 2026 08:56:14 +0100 Subject: [PATCH 4/7] Name the pending-row release in sendForm A row that now carries a value is the server's to render from here on, so its marker is dropped before the request goes out. The method name says so; the inline block in sendForm did not. --- .../dynamic/filter/filters-form.controller.ts | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) 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 04377747e235..445351bba6e7 100644 --- a/frontend/src/stimulus/controllers/dynamic/filter/filters-form.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/filter/filters-form.controller.ts @@ -386,6 +386,13 @@ export default class FiltersFormController extends Controller { return this.filterTargets.filter((row) => row.hasAttribute(PENDING_ATTRIBUTE)); } + private releaseSubmittedPendingRows() { + const submitted = new Set(this.parseFilters().map((filter) => filter.name)); + this.pendingRows() + .filter((row) => submitted.has(row.dataset.filterName!)) + .forEach((row) => this.releasePendingRow(row, { hide: false })); + } + private releasePendingRow(row:HTMLElement, { hide }:{ hide:boolean }) { row.removeAttribute(PENDING_ATTRIBUTE); if (hide) { @@ -511,10 +518,7 @@ export default class FiltersFormController extends Controller { } sendForm() { - const submitted = new Set(this.parseFilters().map((filter) => filter.name)); - this.pendingRows() - .filter((row) => submitted.has(row.dataset.filterName!)) - .forEach((row) => this.releasePendingRow(row, { hide: false })); + this.releaseSubmittedPendingRows(); // When we want the filter content to be written to a hidden input, do this. // When we do not also want the turbo requests, we can exit early here. Otherwise the automatic redirect From a600b7747684d69640441f4b9704c3a5f0b94831 Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Thu, 24 Sep 2026 08:59:14 +0100 Subject: [PATCH 5/7] Restore a date row's picker for its default operator Dropping an unsent row on a restored page resets its control values, but primer-multi-input keeps the active picker in hidden and disabled attributes that a value reset does not touch. Adding the filter again then showed the range picker under the default operator. The row now re-applies the operator's visibility after the reset. --- ...lters-form-pending-rows.controller.spec.ts | 92 +++++++++++++++++-- .../dynamic/filter/filters-form.controller.ts | 10 ++ 2 files changed, 94 insertions(+), 8 deletions(-) diff --git a/frontend/src/stimulus/controllers/dynamic/filter/filters-form-pending-rows.controller.spec.ts b/frontend/src/stimulus/controllers/dynamic/filter/filters-form-pending-rows.controller.spec.ts index 339acf7c2add..8905870ca07f 100644 --- a/frontend/src/stimulus/controllers/dynamic/filter/filters-form-pending-rows.controller.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/filter/filters-form-pending-rows.controller.spec.ts @@ -124,6 +124,17 @@ describe('Filters form pending rows', () => { statusValue().value = id; } + // Turbo's snapshot clone carries live control values, not just markup. `element` is not + // part of the published typings. + async function restoreCachedPage() { + const { element: cached } = Turbo.PageSnapshot.fromElement(host()).clone() as unknown as { element:HTMLElement }; + ctx.dispose(); + ctx = await setupStimulusTest({ controllers: { 'filter--filters-form': Controller } }); + ctx.container.append(cached); + await ctx.nextFrame(); + return ctx.getController('filter--filters-form'); + } + function rerenderFromServer(statusRow:StatusRow) { const template = document.createElement('template'); template.innerHTML = form(statusRow); @@ -236,14 +247,7 @@ describe('Filters form pending rows', () => { controller.addFilterByName('subject'); subjectValue().value = 'unsent draft'; - // Turbo's snapshot clone carries live control values, not just markup. `element` is not - // part of the published typings. - const { element: cached } = Turbo.PageSnapshot.fromElement(host()).clone() as unknown as { element:HTMLElement }; - ctx.dispose(); - ctx = await setupStimulusTest({ controllers: { 'filter--filters-form': Controller } }); - ctx.container.append(cached); - await ctx.nextFrame(); - const restored = ctx.getController('filter--filters-form'); + const restored = await restoreCachedPage(); expect(subjectOperator()).not.toBeVisible(); expect(subjectValue()).toHaveValue(''); @@ -329,6 +333,78 @@ describe('Filters form pending rows', () => { }); }); + describe('with a date row that swaps its picker per operator', () => { + const FILTER_NAME = 'created_at'; + + beforeAll(async () => { + await import('@primer/view-components/app/lib/primer/forms/primer_multi_input'); + }); + + // Mirrors Filters::Inputs::DateForm: one picker per operator inside a primer-multi-input, + // with the inactive one hidden and disabled by activateField(). + function dateRow() { + return `
+ + +
`; + } + + const dateOperator = () => ctx.screen.getByRole('combobox', { name: 'Created on operator', hidden: true }); + // An input carrying `hidden` drops out of the role tree, so the pickers are found by name. + const dayInput = () => ctx.container.querySelector('[data-name="singleDay"]')!; + const rangeInput = () => ctx.container.querySelector('[data-name="dateRange"]')!; + + function addCreatedOn(controller:FiltersFormControllerType) { + addFilterSelect().value = FILTER_NAME; + controller.addFilterByName(FILTER_NAME); + } + + function switchOperatorTo(controller:FiltersFormControllerType, operator:string) { + dateOperator().value = operator; + controller.setValueVisibility({ target: dateOperator(), params: { filterName: FILTER_NAME } }); + } + + it('shows the picker of the default operator again after a cached page is restored', async () => { + const controller = await mount(dateRow()); + addCreatedOn(controller); + switchOperatorTo(controller, '<>d'); + + expect(rangeInput()).toBeVisible(); + expect(dayInput()).not.toBeVisible(); + + const restored = await restoreCachedPage(); + addCreatedOn(restored); + + expect(dateOperator()).toHaveValue('=d'); + expect(dayInput()).toBeVisible(); + expect(dayInput()).toBeEnabled(); + expect(rangeInput()).not.toBeVisible(); + }); + }); + describe('on the Meetings transport (JSON filters, stream morph of the subheader)', () => { function subheader(statusRow:StatusRow) { return form(statusRow) 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 445351bba6e7..f10734d1bddf 100644 --- a/frontend/src/stimulus/controllers/dynamic/filter/filters-form.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/filter/filters-form.controller.ts @@ -399,6 +399,16 @@ export default class FiltersFormController extends Controller { row.setAttribute('hidden', ''); this.setFilterOptionTaken(row.dataset.filterName!, false); resetControls(row); + this.showValueForOperator(row.dataset.filterName!); + } + } + + // Which picker is shown, and whether the value container is shown at all, follows the + // operator through attributes that a reset of control values leaves untouched. + private showValueForOperator(filterName:string) { + const operator = this.findTargetByName(filterName, this.operatorTargets); + if (operator) { + this.setValueVisibility({ target: operator, params: { filterName } }); } } From f4b99b9a1ee06f50b3522341426a78a6d5776718 Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Thu, 24 Sep 2026 08:59:57 +0100 Subject: [PATCH 6/7] Wait out the filter debounce in the created-on spec The operator change submits behind a 300 ms debounce while the idle wait returned after 50 ms of quiet, so the list and URL assertions ran before any request could have gone out and passed either way. --- spec/features/projects/lists/filters_spec.rb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/spec/features/projects/lists/filters_spec.rb b/spec/features/projects/lists/filters_spec.rb index 7c60e3500500..fb05b292b0f3 100644 --- a/spec/features/projects/lists/filters_spec.rb +++ b/spec/features/projects/lists/filters_spec.rb @@ -444,11 +444,13 @@ def load_and_open_filters(user) project_created_on_this_week, project_created_on_fixed_date) - # a range row without dates yet is not a filter: the list and the URL stay as they are + # a range row without dates yet is not a filter: the list and the URL stay as they are. + # The operator change submits behind a 300 ms debounce, so the quiet period must outlast it + # for the assertions below to see a request that should not have happened. projects_page.set_filter("created_at", "Created on", "between") - wait_for_network_idle + wait_for_network_idle(duration: 0.5) projects_page.expect_projects_listed(project_created_on_today, project_created_on_this_week, From 94f141b719a3e06a9d3aaebf6df6e2098280eab3 Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Thu, 24 Sep 2026 09:26:47 +0100 Subject: [PATCH 7/7] Keep a pending row's controls through a morph Guarding only the row's hidden attribute and marker let a response that does not know the row put its operator, draft value and active picker back to server defaults. A user who had picked "does not contain" would then submit "contains". A pending row is now skipped by the morph as a whole; the add-filter option keeps its own guard. --- ...lters-form-pending-rows.controller.spec.ts | 35 ++++++++++++++++++- .../dynamic/filter/filters-form.controller.ts | 26 ++++++++++---- 2 files changed, 54 insertions(+), 7 deletions(-) diff --git a/frontend/src/stimulus/controllers/dynamic/filter/filters-form-pending-rows.controller.spec.ts b/frontend/src/stimulus/controllers/dynamic/filter/filters-form-pending-rows.controller.spec.ts index 8905870ca07f..f612cc165252 100644 --- a/frontend/src/stimulus/controllers/dynamic/filter/filters-form-pending-rows.controller.spec.ts +++ b/frontend/src/stimulus/controllers/dynamic/filter/filters-form-pending-rows.controller.spec.ts @@ -86,6 +86,7 @@ describe('Filters form pending rows', () => {