Feature: new filter component#927
Open
santipalenque wants to merge 8 commits into
Open
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
santipalenque
force-pushed
the
feature/new-filter-component
branch
from
2f21c4a to
0fb436b
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/components/GridFilter/__tests__/Dropdown.test.jsx (1)
94-107: ⚡ Quick winAssert exact callback invocation count.
toHaveBeenCalled()can still pass if one selection triggers duplicate calls. Tighten this to a single expected invocation for better regression detection.Proposed test hardening
fireEvent.mouseDown(screen.getByRole("combobox")); fireEvent.click(screen.getByRole("option", { name: "Option A" })); + expect(onChange).toHaveBeenCalledTimes(1); expect(onChange).toHaveBeenCalled();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/GridFilter/__tests__/Dropdown.test.jsx` around lines 94 - 107, The test "calls onChange when an option is selected" uses a mock onChange but only asserts toHaveBeenCalled(), which allows duplicate invocations; change the assertion to expect(onChange).toHaveBeenCalledTimes(1) to ensure exactly one invocation when fireEvent.mouseDown(...) and fireEvent.click(...) are used; update the assertion in Dropdown.test.jsx for the onChange mock to assert a single call.src/components/GridFilter/GridFilter.jsx (1)
75-84: 💤 Low valueConsider using a more robust ID generation strategy.
When
f.criteriais undefined (e.g., forEMPTY_FILTER), the ID becomes"undefined-0". While this works because IDs remain unique via the index, it creates non-semantic keys. Consider using a counter or generating UUIDs for cleaner filter IDs.Alternative approach
+let filterId = 0; + const handleAdd = () => { setFilters((prevFilters) => { - // replacing "new" id and adding new empty filter - const currentFilters = prevFilters.map((f, i) => ({ - ...f, - id: `${f.criteria}-${i}` - })); - return [...currentFilters, EMPTY_FILTER]; + return [...prevFilters, { ...EMPTY_FILTER, id: `filter-${filterId++}` }]; }); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/GridFilter/GridFilter.jsx` around lines 75 - 84, handleAdd creates IDs using `${f.criteria}-${i}` which yields non-semantic "undefined-0" when f.criteria is missing; change ID generation to a robust strategy by introducing a monotonic counter state or using a UUID generator inside handleAdd and when remapping prevFilters so every filter gets a deterministic unique id; update the mapping in handleAdd (and creation of the appended EMPTY_FILTER) to assign the new id (e.g., use filterIdCounter++ or uuid() to produce ids) and ensure EMPTY_FILTER is cloned/assigned an id rather than relying on criteria for the key.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/GridFilter/GridFilter.jsx`:
- Line 48: In GridFilter.jsx (inside the GridFilter component), correct the typo
in the inline comment: replace "rest to applied filters when closing modal
(Cancel)" with "reset to applied filters when closing modal (Cancel)" so the
comment accurately describes restoring applied filters on Cancel.
---
Nitpick comments:
In `@src/components/GridFilter/__tests__/Dropdown.test.jsx`:
- Around line 94-107: The test "calls onChange when an option is selected" uses
a mock onChange but only asserts toHaveBeenCalled(), which allows duplicate
invocations; change the assertion to expect(onChange).toHaveBeenCalledTimes(1)
to ensure exactly one invocation when fireEvent.mouseDown(...) and
fireEvent.click(...) are used; update the assertion in Dropdown.test.jsx for the
onChange mock to assert a single call.
In `@src/components/GridFilter/GridFilter.jsx`:
- Around line 75-84: handleAdd creates IDs using `${f.criteria}-${i}` which
yields non-semantic "undefined-0" when f.criteria is missing; change ID
generation to a robust strategy by introducing a monotonic counter state or
using a UUID generator inside handleAdd and when remapping prevFilters so every
filter gets a deterministic unique id; update the mapping in handleAdd (and
creation of the appended EMPTY_FILTER) to assign the new id (e.g., use
filterIdCounter++ or uuid() to produce ids) and ensure EMPTY_FILTER is
cloned/assigned an id rather than relying on criteria for the key.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2df76316-6443-42d9-8701-bae2af3803a2
📒 Files selected for processing (9)
src/components/GridFilter/GridFilter.jsxsrc/components/GridFilter/__tests__/Dropdown.test.jsxsrc/components/GridFilter/components/Dropdown.jsxsrc/components/GridFilter/components/Filter.jsxsrc/components/GridFilter/components/FilterButton.jsxsrc/components/GridFilter/reducers/filter-reducer.jssrc/i18n/en.jsonsrc/pages/summit_speakers/summit-speakers-list-page/components/send-email-modal.jssrc/pages/summit_speakers/summit-speakers-list-page/index.js
🚧 Files skipped from review as they are similar to previous changes (7)
- src/components/GridFilter/components/Dropdown.jsx
- src/components/GridFilter/reducers/filter-reducer.js
- src/components/GridFilter/components/FilterButton.jsx
- src/pages/summit_speakers/summit-speakers-list-page/components/send-email-modal.js
- src/components/GridFilter/components/Filter.jsx
- src/i18n/en.json
- src/pages/summit_speakers/summit-speakers-list-page/index.js
|
|
||
| useEffect(() => { | ||
| if (openModal) { | ||
| // we want to rest to applied filters when closing modal (Cancel) |
There was a problem hiding this comment.
Fix typo in comment.
Change "rest to applied filters" to "reset to applied filters".
📝 Proposed fix
- // we want to rest to applied filters when closing modal (Cancel)
+ // we want to reset to applied filters when closing modal (Cancel)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // we want to rest to applied filters when closing modal (Cancel) | |
| // we want to reset to applied filters when closing modal (Cancel) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/GridFilter/GridFilter.jsx` at line 48, In GridFilter.jsx
(inside the GridFilter component), correct the typo in the inline comment:
replace "rest to applied filters when closing modal (Cancel)" with "reset to
applied filters when closing modal (Cancel)" so the comment accurately describes
restoring applied filters on Cancel.
There was a problem hiding this comment.
Pull request overview
This PR introduces a new MUI-based grid filter UI for the Summit Speakers/Submitters list and refactors the list page to use the new filter mechanism, along with dependency updates needed to support it.
Changes:
- Replace the legacy Summit Speakers list page with a new implementation that supports a modal grid filter and integrates filtering into list fetch/export/email flows.
- Refactor speaker/submitter filtering from object-based UI filters to array-based “parsed filter” strings passed to the server.
- Update dependencies (notably
openstack-uicore-foundationandreact-redux) and wire the grid-filter reducer into the Redux store.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| yarn.lock | Updates lockfile for openstack-uicore-foundation and react-redux upgrade. |
| package.json | Bumps openstack-uicore-foundation and react-redux versions. |
| src/store.js | Adds allFiltersReducer state for grid filter persistence/usage. |
| src/reducers/summit_submitters/summit-submitters-list-reducer.js | Removes legacy filter fields; normalizes term default. |
| src/reducers/summit_speakers/summit-speakers-list-reducer.js | Removes legacy filter fields; normalizes term default. |
| src/pages/summit_speakers/summit-speakers-list-page/index.js | New speakers/submitters list page using GridFilter + server-side parsed filters. |
| src/pages/summit_speakers/summit-speakers-list-page/components/send-email-modal.js | New Send Email modal integrated with parsed filters. |
| src/pages/summit_speakers/summit-speakers-list-page.js | Removes the legacy class-based list page implementation. |
| src/actions/submitter-actions.js | Switches to array-based filters and removes legacy parseFilters helper. |
| src/actions/speaker-actions.js | Switches to array-based filters and removes legacy parseFilters helper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }; | ||
|
|
||
| const filter = parseFilters(filters); | ||
| const filter = [...filters]; |
Comment on lines
209
to
213
| if (!selectedAll && selectedItems.length > 0) { | ||
| // we don't need the filter criteria, we have the ids | ||
| filter.push(`id==${selectedItems.join("||")}`); | ||
| const originalFilters = parseFilters(filters); | ||
| const originalFilters = [...filters]; | ||
|
|
Comment on lines
223
to
226
| payload.original_filter = originalFilters; | ||
| } else { | ||
| filter = parseFilters(filters); | ||
| filter = [...filters]; | ||
|
|
Comment on lines
+126
to
+170
| if (f.value) { | ||
| switch (f.value) { | ||
| case "only_rejected": | ||
| filter.push("has_rejected_presentations==true"); | ||
| filter.push("has_accepted_presentations==false"); | ||
| filter.push("has_alternate_presentations==false"); | ||
| break; | ||
| case "only_accepted": | ||
| filter.push("has_rejected_presentations==false"); | ||
| filter.push("has_accepted_presentations==true"); | ||
| filter.push("has_alternate_presentations==false"); | ||
| break; | ||
| case "only_alternate": | ||
| filter.push("has_rejected_presentations==false"); | ||
| filter.push("has_accepted_presentations==false"); | ||
| filter.push("has_alternate_presentations==true"); | ||
| break; | ||
| case "accepted_alternate": | ||
| filter.push("has_rejected_presentations==false"); | ||
| filter.push("has_accepted_presentations==true"); | ||
| filter.push("has_alternate_presentations==true"); | ||
| break; | ||
| case "accepted_rejected": | ||
| filter.push("has_rejected_presentations==true"); | ||
| filter.push("has_accepted_presentations==true"); | ||
| filter.push("has_alternate_presentations==false"); | ||
| break; | ||
| case "alternate_rejected": | ||
| filter.push("has_rejected_presentations==true"); | ||
| filter.push("has_accepted_presentations==false"); | ||
| filter.push("has_alternate_presentations==true"); | ||
| break; | ||
| case "accepted": | ||
| filter.push("has_accepted_presentations==true"); | ||
| break; | ||
| case "rejected": | ||
| filter.push("has_rejected_presentations==true"); | ||
| break; | ||
| case "alternate": | ||
| filter.push("has_alternate_presentations==true"); | ||
| break; | ||
| default: | ||
| break; | ||
| } | ||
| } |
Comment on lines
+284
to
+286
| const { term, page, perPage, order, orderDir } = subjectProps; | ||
|
|
||
| const mergedParams = { term, page, perPage, order, orderDir, ...params }; |
| <br /> | ||
| <input | ||
| id="ingest_email" | ||
| className="form-control" |
| <input | ||
| id="should_send_copy_2_submitter" | ||
| className="form-check-input" | ||
| type="checkbox" |
Comment on lines
114
to
118
| "react-dropzone": "^4.2.13", | ||
| "react-final-form": "^6.5.9", | ||
| "react-google-maps": "^9.4.5", | ||
| "react-redux": "^5.0.7", | ||
| "react-redux": "^7.1.0", | ||
| "react-router": "^4.3.1", |
There was a problem hiding this comment.
@santipalenque do we need to update redux too ?
Comment on lines
787
to
790
| const { currentSummit } = currentSummitState; | ||
| const filter = parseFilters(filters); | ||
| const filter = [...filters]; | ||
|
|
||
| dispatch(startLoading()); |
Comment on lines
62
to
66
| const accessToken = await getAccessTokenSafely(); | ||
| const { currentSummit } = currentSummitState; | ||
|
|
||
| const filter = parseFilters(filters); | ||
| const filter = [...filters]; | ||
|
|
|
@santipalenque 
clicked at [APPLY FILTER] but does not invoke the API
tested against dev and should retrieve 619 entries 
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://app.clickup.com/t/86b9ruudz
Summary by CodeRabbit
New Features
Documentation
Tests
Chores