[AGILE-432] Keep an added filter row visible by myabc · Pull Request #25519 · opf/openproject

myabc · 2026-09-22T13:24:08Z

Ticket

https://community.openproject.org/wp/AGILE-432 (regression)

Alternative to #25512, same symptom.

What are you trying to accomplish?

modules/meeting/spec/features/meetings_index_spec.rb:203 ("Meeting status" filter) fails on dev and on every PR based on it. Adding a list filter unhides its row on the client and then asks the server for a fresh render; the server renders a row hidden unless the query holds that filter (base_filter_form.rb:84). Since fd8536fb2de (AGILE-432) an autocomplete row without a selection is left out of the request, so the response hides the row again. The Meetings index is the one consumer whose response morphs the filter form (meetings_controller.rb:69-72); Projects and Portfolios only replace the filter button, Backlogs keeps the form outside its frame.

The meetings status filter (d53cc9b, OP-17199) landed on dev after the AGILE-432 branch was cut from release/17.9, so the two only met in the release merge, which raises no PR checks.

What approach did you choose and why?

An added row without a value is marked data-filter-pending. The controller cancels turbo:before-morph-element for marked rows, so a re-render leaves them alone, operator, draft value and active picker included, and cancels turbo:before-morph-attribute for disabled on their add-filter option. The marker goes once the row is submitted with a value or removed. On connect() a marker already in the DOM is stale (Turbo snapshots the page before the old controller disconnects): the row is hidden again and its controls reset to their server defaults, since the snapshot also keeps whatever the user had typed. So Back/Forward neither shows nor later submits an unsent draft. The request and the URL are untouched.

#25512 serializes the row as status_id = "" instead. The Meetings query rejects the blank value and returns nothing until a value is picked, an unsent draft lands in URL and history (which the AGILE-375 design rules out), and it adds a second filter memory next to sentFilters that OP-20194 and AGILE-375 both rewrite.

Empty date ranges ("-" as rendered by the picker, or "") now parse as no value as well, so a fresh "Dates interval" row is kept the same way. One-sided ranges are submitted as before.

AI involvement

Collaborative – AI generated a substantial part of the code; I reviewed and understand every line.

Merge checklist

Added/updated tests
Added/updated documentation in Lookbook (patterns, previews, etc)
Tested major browsers (Chrome, Firefox, Edge, ...)

github-actions · 2026-09-22T13:24:33Z

Caution

The provided work package version does not match the core version

Details:

Work package URL: https://community.openproject.org/wp/AGILE-432
Work package version: 17.9.0
Core version: 18.0.0

Please make sure that:

The work package version OR your pull request target branch is correct

Copilot

Copilot review overview

🔵 Needs a closer look

One or more issues must be addressed before approval.

Review effort: Lite
Findings: None

What changed in this PR

Keeps newly added, empty filter rows visible during Turbo morphs without adding draft values to requests or URLs.

Changes:

Tracks pending filter rows and preserves their visibility/disabled state.
Clears pending state when filters receive values or are removed.
Adds coverage for morphing, values, removal, and unrelated rows.

File	Description
frontend/src/stimulus/controllers/dynamic/filter/filters-form.controller.ts	Updated as part of this pull request.
frontend/src/stimulus/controllers/dynamic/filter/filters-form-pending-rows.controller.spec.ts	Updated as part of this pull request.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot

Copilot review overview

🟡 Changes recommended

Empty date-range filters bypass the new pending-row protection.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1

Open (1)

Parse empty date ranges as null and cover the date-row case · New

Copilot

Copilot review overview

🟡 Changes recommended

Cached pending rows retain draft control values, allowing discarded input to be reapplied after restoration.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 · 1

Open (2)

Reset pending row controls before Turbo caches the form · New
Add regression tests for one-sided date ranges · New

Resolved since last review (1)

Parse empty date ranges as null and cover the date-row case

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

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.

github-actions · 2026-09-22T16:54:19Z

Warning

Flaky specs

rspec ./modules/gantt/spec/features/timeline/timeline_dates_spec.rb[1:2:1]
rspec ./spec/features/notifications/navigation_spec.rb[1:1:1]
rspec ./spec/features/work_packages/new/attributes_from_filter_spec.rb[1:3:1]

🤖 Ask Copilot to investigate

Copy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer.

@copilot The following spec(s) are flaky in CI (first seen on PR #25519, linked for reference only):

- `rspec ./modules/gantt/spec/features/timeline/timeline_dates_spec.rb[1:2:1]`
- `rspec ./spec/features/notifications/navigation_spec.rb[1:1:1]`
- `rspec ./spec/features/work_packages/new/attributes_from_filter_spec.rb[1:3:1]`

Treat this as a standalone task, unrelated to PR #25519. Create a new branch from origin/dev and open a new pull request targeting dev — do not stack it on PR #25519 or reuse that branch.

Follow the playbook in docs/development/testing/handling-flaky-tests/README.md to find the root cause and fix the underlying race — do not skip, delete, or weaken the spec to make it pass; disabling is a last resort per the playbook, and only with a bug ticket. Verify the fix by running the spec(s) repeatedly (e.g. `script/bulk_run_rspec --run-count 10`).

If you cannot reproduce the flake or are not confident in a fix after reasonable investigation, do not fabricate a change or skip the spec to force CI green. Instead, leave the pull request in draft and document what you tried, the suspected cause, and any leads in its description, then assign @myabc to take over.

Once the fix is verified, title the PR after the spec(s) it fixes, and use the PR description to explain the root cause, how the change resolves it, and the before/after results. Label the PR `flaky-spec`, assign @myabc, and request a review from @myabc.
On every commit, set @myabc as the sole co-author with a `Co-authored-by:` trailer (use their GitHub no-reply email so it links to their account), so it is traceable who dispatched the fix.

lwassermann

This feels quite well tested.

The controller itself feels quite complex and not well factored. I fell like very many things are happening all in the same module, and that also makes the tests harder to set up and follow. But since this was the case before, and seems to happen often with stimulus, changing it here would be far out of scope.

I left some comments but they are smaller things or more open ended questions.

Since you asked for considered opinions, I do like using factory functions instead of the shared variables that get set up in before. I feel that factory functions make dependencies explicit. Unfortunately they can also make the test arrangement more verbose. Still, I like the explicitness and the natural magnetism such a function has for collecting the variants of a test suite. (An early version, pre-ts, can be found on the bitcrowd dev blog 🙂 )

tangopium

Nice fix 👍

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.

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.

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.

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.

myabc · 2026-09-24T08:12:39Z

The controller itself feels quite complex and not well factored.

@lwassermann I agree. There may be an opportunity for refactoring as part of AGILE-375 or an eventual follow-up.

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.

myabc · 2026-09-24T09:21:07Z

Is that something that could replace data-filter-pending? Or not because we need to protect both the added filter as well as the disabled menu entry? So it's not suitable here?

@lwassermann Good find. It would deal with half of the problem. Since 94f141b the morph skips a pending row entirely, which is exactly what data-turbo-permanent does under-the-hood, so the row part could use the attribute instead of a listener.

However it wouldn't cover the rest. data-filter-pending also tells the controller which rows to release on submit and which stale rows to clean up after a page is restored from cache. And the disabled entry in the "Add filter" menu still needs its own guard. So we would end up with two attributes on the row that always change together, and keep the listener anyway. As such, I've left it as-is for now.

github-actions · 2026-09-24T09:27:48Z

Warning

Flaky specs

rspec ./spec/features/notifications/navigation_spec.rb[1:1:1]
rspec ./spec/features/users/invite_user_modal/invite_user_modal_spec.rb[1:3:1:1:3:1:1]

🤖 Ask Copilot to investigate

Copy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer.

@copilot The following spec(s) are flaky in CI (first seen on PR #25519, linked for reference only):

- `rspec ./spec/features/notifications/navigation_spec.rb[1:1:1]`
- `rspec ./spec/features/users/invite_user_modal/invite_user_modal_spec.rb[1:3:1:1:3:1:1]`

Treat this as a standalone task, unrelated to PR #25519. Create a new branch from origin/dev and open a new pull request targeting dev — do not stack it on PR #25519 or reuse that branch.

Follow the playbook in docs/development/testing/handling-flaky-tests/README.md to find the root cause and fix the underlying race — do not skip, delete, or weaken the spec to make it pass; disabling is a last resort per the playbook, and only with a bug ticket. Verify the fix by running the spec(s) repeatedly (e.g. `script/bulk_run_rspec --run-count 10`).

If you cannot reproduce the flake or are not confident in a fix after reasonable investigation, do not fabricate a change or skip the spec to force CI green. Instead, leave the pull request in draft and document what you tried, the suspected cause, and any leads in its description, then assign @myabc to take over.

Once the fix is verified, title the PR after the spec(s) it fixes, and use the PR description to explain the root cause, how the change resolves it, and the before/after results. Label the PR `flaky-spec`, assign @myabc, and request a review from @myabc.
On every commit, set @myabc as the sole co-author with a `Co-authored-by:` trailer (use their GitHub no-reply email so it links to their account), so it is traceable who dispatched the fix.

lwassermann · 2026-09-24T09:34:02Z

That was my thoughts as well. But I thought I'd at least raise it in case I didn't have the full picture 🙂 Thank you for checking 🙇

Copilot AI lite review requested due to automatic review settings September 22, 2026 13:24

Copilot started reviewing on behalf of myabc September 22, 2026 13:24 View session

Copilot AI reviewed Sep 22, 2026

View reviewed changes

myabc mentioned this pull request Sep 22, 2026

Keep an added filter row visible until it gets a value #25512

Closed

1 of 3 tasks

myabc added this to the 18.0.x milestone Sep 22, 2026

myabc added javascript Pull requests that update Javascript code needs review labels Sep 22, 2026

myabc requested a balanced review from Copilot September 22, 2026 13:36

Copilot started reviewing on behalf of myabc September 22, 2026 13:37 View session

github-actions Bot added the ai: Collaborative 💻 AI generated a substantial part of the code; A human reviewed and understands every line. label Sep 22, 2026

Copilot AI reviewed Sep 22, 2026

View reviewed changes

Comment thread frontend/src/stimulus/controllers/dynamic/filter/filters-form.controller.ts Outdated

akabiru mentioned this pull request Sep 22, 2026

Feature/COMMS-1025: Labels administration #25394

Merged

myabc force-pushed the bug/agile-432-keep-added-filter-row-visible branch from 617f371 to 3813e15 Compare September 22, 2026 14:22

myabc added the ruby Pull requests that update Ruby code label Sep 22, 2026

myabc requested a balanced review from Copilot September 22, 2026 14:24

Copilot started reviewing on behalf of myabc September 22, 2026 14:25 View session

myabc requested review from oliverguenther and tangopium September 22, 2026 14:27

Copilot AI reviewed Sep 22, 2026

View reviewed changes

Comment thread frontend/src/stimulus/controllers/dynamic/filter/filters-form.controller.ts

Comment thread frontend/src/stimulus/controllers/dynamic/filter/filters-form-pending-rows.controller.spec.ts

myabc force-pushed the bug/agile-432-keep-added-filter-row-visible branch from 3813e15 to bf6dc0f Compare September 22, 2026 14:41

myabc commented Sep 22, 2026

View reviewed changes

Comment thread frontend/src/stimulus/controllers/dynamic/filter/filters-form.controller.ts Outdated

lwassermann reviewed Sep 22, 2026

View reviewed changes

Comment thread frontend/src/stimulus/controllers/dynamic/filter/filters-form.controller.ts Outdated

Detach filter form listeners via AbortController

dbb3e8e

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.

lwassermann approved these changes Sep 22, 2026

View reviewed changes

This comment was marked as resolved.

Sign in to view

tangopium approved these changes Sep 23, 2026

View reviewed changes

Comment thread frontend/src/stimulus/controllers/dynamic/filter/filters-form.controller.ts Outdated

Comment thread spec/features/projects/lists/filters_spec.rb

Move resetControls out of the filter controller

508168d

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.

myabc added 3 commits September 24, 2026 08:56

Name the pending-row release in sendForm

49f0bc6

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.

Wait out the filter debounce in the created-on spec

f4b99b9

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.

tangopium mentioned this pull request Sep 24, 2026

[AUTOWORK-311] Update copy service spec to named workflows #25564

Closed

1 of 3 tasks

Merge branch 'dev' into bug/agile-432-keep-added-filter-row-visible

d2c1827

myabc merged commit a5be9bf into dev Sep 24, 2026
21 checks passed

myabc deleted the bug/agile-432-keep-added-filter-row-visible branch September 24, 2026 09:35

github-actions Bot locked and limited conversation to collaborators Sep 24, 2026

Conversation

myabc commented Sep 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Ticket

What are you trying to accomplish?

What approach did you choose and why?

AI involvement

Merge checklist

Uh oh!

github-actions Bot commented Sep 22, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🔵 Needs a closer look

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Sep 22, 2026

Uh oh!

lwassermann left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as resolved.

tangopium left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

myabc commented Sep 24, 2026

Uh oh!

myabc commented Sep 24, 2026

Uh oh!

github-actions Bot commented Sep 24, 2026

Uh oh!

lwassermann commented Sep 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

myabc commented Sep 22, 2026 •

edited

Loading

lwassermann left a comment •

edited

Loading

lwassermann commented Sep 24, 2026 •

edited

Loading