Feature: new filter component#927
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
2f21c4a to
0fb436b
Compare
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.
| // 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.
https://app.clickup.com/t/86b9ruudz
Summary by CodeRabbit
New Features
Documentation
Tests
Chores