Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Caution The provided work package version does not match the core version Details:
Please make sure that:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9456d8b50
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // A row the user has added but not given a value to yet is still part of the form. It is | ||
| // sent with an empty value so the server keeps rendering it after the re-render, while | ||
| // `hasEffectiveValue` keeps it from counting as a change of the result set. | ||
| return values.length > 0 ? values : ['']; |
There was a problem hiding this comment.
When an autocomplete row is added without a selection, returning [''] submits an active filter such as {"state":{"operator":"=","values":[""]}}. In the meetings flow, ParamsToQueryService installs that filter on the query, Queries::Filters::Base rejects the blank value for a value-requiring operator, and Queries::BaseQuery#results consequently returns empty_scope; therefore adding the status filter immediately clears all meeting results even though hasEffectiveValue treats the row as result-neutral. The placeholder row needs to be preserved without making the server execute an invalid active filter.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, and it changed the approach. [""].compact_blank is empty, so validate_presence_of_values marks the query invalid and BaseQuery#results returns empty_scope: the meetings list would blank out between adding the filter and picking a value. That is what dev did before AGILE-432 too, which makes it a pre-existing defect rather than a new one, but not something to reinstate.
The row is no longer sent at all. The controller keeps the names of the rows it added and restores them after each stream render, so the query never sees a blank filter.
|
@tangopium thanks for this! would you mind updating the PR description to use the standard PR template ( |
|
Warning Flaky specs
🤖 Ask Copilot to investigateCopy 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. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved findings affect filter counting, request rollback, and parameter-reset behavior.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (2)
What changed in this PR
This PR keeps newly added, unvalued filter rows visible after server re-rendering while separating effective filters from placeholders.
Changes:
- Serializes empty autocomplete rows.
- Resets pagination and expansion only when effective filters change.
- Adds navigation regression coverage.
| File | Summary |
|---|---|
frontend/src/stimulus/controllers/dynamic/filter/filters-form.controller.ts |
Updates filter serialization and effective-filter tracking. Findings: critical (2 votes) counter inconsistency; moderate (3 votes) failed-request rollback; moderate (1 vote) fresh-load baseline comparison. |
frontend/src/stimulus/controllers/dynamic/filter/filters-form-navigation.controller.spec.ts |
Adds coverage for empty-row persistence and preserving existing-filter parameters. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // A row the user has added but not given a value to yet is still part of the form. It is | ||
| // sent with an empty value so the server keeps rendering it after the re-render, while | ||
| // `hasEffectiveValue` keeps it from counting as a change of the result set. | ||
| return values.length > 0 ? values : ['']; |
There was a problem hiding this comment.
Correct, and untested: the counter specs use a text row, which returns null when empty, so the inconsistency you describe would not have shown up in CI.
The parser change is reverted entirely in the new version, so parseFilters() and the counter behave exactly as they do on dev.
| }) | ||
| .catch((error:Error) => { | ||
| this.sentFilters = previousFilters; | ||
| this.sentFilters = rollbackFilters; |
There was a problem hiding this comment.
Correct. sentEffectiveFilters was set before the fetch and only sentFilters was restored in the failure path, so a retry would have skipped the reset of page and all.
That field is gone in the new version: nothing is sent when a row is added, so there is no second piece of state to roll back.
myabc
left a comment
myabc
left a comment
There was a problem hiding this comment.
@tangopium thanks for taking a look at this - and sorry that AGILE-432 ended up breaking on dev.
I'm not sure about serialising the empty row as status_id = "" however, since this has three effects I'd like to avoid:
Queries::Filters::Base#validate_presence_of_valuesrejects[""], and the Meetings query is nevervalid_subset!-ed, soQueries::BaseQuery#resultsreturnsempty_scope: adding "Meeting status" empties the list until a value is picked (see Codex's finding: #25512 (comment)).- An unsent draft reaches URL and history via
visit/replaceState. Back/Forward and reload then reconstruct a row the user never applied. This will cause an issue in an upcoming work package I'm currently specifying (AGILE-375). - It needs a second "effective filters" memory next to sentFilters, which both OP-20194 (#25280) and AGILE-375 rewrite.
Normally I'd prefer to provide feedback and give you the opportunity to revise. However in this instance - since it's causing failures on CI - I've gone ahead and proposed an alternate solution #25519. Sorry if this isn't good PR etiquette!
Tab-switching caused me to approve the wrong PR
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
f9456d8 to
e6492e4
Compare
github-actions
Bot
added
the
ai: Directed 🪄
|
Description now follows The approach changed with the force-push as well: the first version sent the added row with an empty value, which would have installed a filter the query rejects and emptied the result list until a value was picked. The controller now restores the rows it added after each stream render instead, so nothing is sent and no behaviour from AGILE-432 is altered. Details are in the description, and the review threads have the specifics. |
|
Closing in favour of #25519, which fixes the same regression and does it better. Both fixes are client-side and neither sends a filter the query would reject, so the direction is the same. The differences that matter:
The diagnosis in this PR stays valid and is reproduced in #25519: the server renders a filter row hidden unless the query holds it ( |
Ticket
https://community.openproject.org/wp/AGILE-432
What are you trying to accomplish?
modules/meeting/spec/features/meetings_index_spec.rb:203fails ondev, and so does every PR that merges currentdev:Behind the spec is a real defect: on the meetings index, adding the "Meeting status" filter no longer works. The row appears and is gone again by the next render, so the filter cannot be set at all. Every list filter added before it is given a value behaves this way.
Adding a 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:
Until fd8536f an autocomplete row with no selection was still serialised, with an empty value, so the query held it and the server kept rendering it. That commit changed the parser to report no value at all:
parseAdvancedFiltersskips null values, so the filter stops reaching the server and the next render hides the row. The DOM snapshot CI uploads on failure shows exactly that: the row is present and carrieshidden, and its entry inadd_filter_selectis selectable again, so the whole client-side addition had been undone by a server render.It was not caught because AGILE-432 targeted
release/17.9, where the meetings status filter does not exist (d53cc9bd440is not an ancestor of that branch), and it reacheddevthrough a plainMerge branch 'release/17.9' into dev, which raises no PR and no checks.What approach did you choose and why?
The controller remembers the rows it added and restores them after each stream render, so they survive a re-render without the server having to know about them.
The obvious alternative is to send the row with an empty value again, which is what
devdid before AGILE-432. That gets the row rendered, but it installs a filter the query rejects:So the meetings list would blank out between adding the filter and picking a value. That is a real defect on its own, it simply predates AGILE-432, and reinstating it to fix this one is not a trade worth making.
Restoring on the client keeps both properties: nothing invalid reaches the query, and the result set and the parameters AGILE-432 protects (
page, and the Backlogs inboxall) are untouched, since no request is made when a row is merely added. The change is additive, and no behaviour introduced by AGILE-432 is altered.A row stops being tracked as soon as the server renders it visible, which means the query holds it by then and the server owns it again.
AI involvement
Directed – I specified the requirements and AI implemented most of it; I validated via testing rather than a full line-by-line review.
Merge checklist