Feature: new grid filter#242
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a dialog-driven GridFilter with Redux persistence, supporting utilities and subcomponents (Dropdown, RoundButton, ToggleButtons), tests, i18n strings, webpack entries, and upgrades react-redux/redux in package.json. ChangesGrid Filter Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
src/components/mui/GridFilter/utils.js (1)
38-43: 💤 Low valueConsider using a symbol or generated ID for EMPTY_FILTER.
The string literal
"new"as theidcould conflict with actual filter IDs if the backend ever generates an ID with the value"new".🔄 Proposed alternatives
Option 1: Use a Symbol
export const EMPTY_FILTER = { criteria: null, operator: null, value: null, - id: "new" + id: Symbol("new") };Option 2: Use a unique prefix
export const EMPTY_FILTER = { criteria: null, operator: null, value: null, - id: "new" + id: "__new__" };🤖 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/mui/GridFilter/utils.js` around lines 38 - 43, EMPTY_FILTER currently uses the literal id "new" which may conflict with real IDs; update EMPTY_FILTER (and any code that compares or serializes its id) to use a non-colliding identifier instead—either replace the id with a Symbol (e.g., use Symbol('EMPTY_FILTER') and adjust any equality checks to use strict identity) or generate a namespaced/unique id (e.g., prefix with "__empty__" or use a UUID) and ensure functions that create, compare, or send filters handle the new form (refer to EMPTY_FILTER and any usages that read .id).package.json (1)
85-85: 🏗️ Heavy liftreact-redux v7.1.0 usage is compatible with the codebase.
The codebase actively uses react-redux v7.1.0 with three components using the
connect()HOC pattern and one using hooks (useDispatch,useSelector). All patterns found are fully compatible with react-redux v7—no breaking changes were detected, and the dependency combination with React 17 and Redux 3.7.2 is valid and stable.However, consider upgrading to
^7.2.9instead of^7.1.0for bug fixes and improvements.Also applies to: 152-152
🤖 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 `@package.json` at line 85, Update the react-redux dependency in package.json from "^7.1.0" to "^7.2.9" (the "react-redux" entry), run your package manager to update the lockfile (npm install or yarn install), and run the test suite and the app to ensure existing usages of connect(), useDispatch, and useSelector continue to work; if any type or runtime regressions appear, adjust imports/usages accordingly and commit the updated package.json and lockfile.src/components/mui/RoundButton/index.jsx (1)
18-29: ⚡ Quick winSupport MUI array-based
sxcomposition for full compatibility.The current object-spread pattern on line 18 (
sx={{ ...defaults, ...sx }}) prevents users from passing array or function values, which MUI v6 explicitly supports for theme-based and conditional styling. Converting to MUI's array format preserves defaults while accepting all three supported types.♻️ Proposed refactor
-const RoundButton = ({ children, sx = {}, ...props }) => ( +const RoundButton = ({ children, sx, ...props }) => ( <Button // eslint-disable-next-line react/jsx-props-no-spreading {...props} - sx={{ - width: 40, - height: 40, - minWidth: "auto", - borderRadius: "50%", - padding: 0, - ...sx - }} + sx={[ + { + width: 40, + height: 40, + minWidth: "auto", + borderRadius: "50%", + padding: 0 + }, + sx + ]} > {children} </Button> ); @@ RoundButton.propTypes = { children: PropTypes.node.isRequired, - sx: PropTypes.object + sx: PropTypes.oneOfType([ + PropTypes.object, + PropTypes.array, + PropTypes.func + ]) }; RoundButton.defaultProps = { - sx: {} + sx: undefined };Also applies to: lines 35-42
🤖 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/mui/RoundButton/index.jsx` around lines 18 - 29, The RoundButton component currently merges default sx properties by object-spreading which rejects array or function sx values; update the sx prop on the Button inside RoundButton (and the analogous block at the other occurrence) to compose defaults using MUI's array form: provide the defaults as the first array element and then concatenate the incoming sx (if it's already an array spread it, otherwise append it as a single element) so functions, arrays and objects are all supported by MUI; target the RoundButton component and the other sx merge site mentioned and replace the object spread with this array-composition pattern.
🤖 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/index.js`:
- Line 123: Change the incorrect default re-export of MuiGridFilter to use the
named export from the GridFilter barrel: import the named export GridFilter and
re-export it aliased as MuiGridFilter, and keep the other named exports
(OPERATORS as FILTER_OPERATORS, JOIN_OPERATORS as FILTER_JOIN_OPERATORS,
EMPTY_FILTER as FILTER_EMPTY_FILTER, useGridFilter, allFiltersReducer,
saveFilters as saveGridFilters, SAVE_FILTERS) as named re-exports; update the
export statement that currently references MuiGridFilter to reference GridFilter
(aliased) and the listed named symbols so it matches the named exports provided
by GridFilter.
In `@src/components/mui/GridFilter/components/ValueInput/index.jsx`:
- Around line 21-30: ValueInput currently accepts a missing type at runtime (it
falls back to Dropdown via INPUT_TYPE_MAP lookup) but its propTypes mark type as
required; update the prop contract to match runtime behavior by making the type
prop optional (remove PropTypes.string.isRequired and use PropTypes.string) or
add a default prop for type that maps to the Dropdown key; locate the ValueInput
component and adjust the propTypes (and/or defaultProps) for the type prop so
PropTypes no longer warns when type is omitted.
In `@src/components/mui/GridFilter/hooks/useGridFilter.jsx`:
- Around line 7-10: The hook assumes state.allGridFiltersState exists and
crashes if missing; update the useSelector call so it safely accesses the slice
and returns a default array when absent (e.g., use optional chaining and nullish
coalescing to get allFilters), then compute filter = allFilters.find(f => f.id
=== id) || {}; specifically change the selector used in useSelector and keep the
rest of the logic (referencing useSelector, allGridFiltersState, allFilters, id,
and filter) so the hook never throws during render when the slice is undefined.
In `@src/components/mui/GridFilter/utils.js`:
- Around line 11-15: The HAS and HAS_NOT operators (symbols HAS and HAS_NOT in
GridFilter utils) are marked "not available on API" and should not be sent to
the backend; either remove them from the exported operator list if they are
unusable, or clearly mark them as client-only and prevent submission by adding
runtime validation that filters out operator values ">>" and "!>>" before
building API requests (e.g., in the function that serializes filter rules), or
add UI-level documentation/tooltip indicating they perform client-side filtering
only—pick one approach and implement it consistently so the UI never sends
HAS/HAS_NOT to the API.
- Around line 3-36: The operator labels are being translated eagerly at module
load via T.translate in OPERATORS and JOIN_OPERATORS, causing untranslated keys
if i18n isn't initialized; update OPERATORS and JOIN_OPERATORS so label values
are computed lazily (e.g., replace static label strings with getter functions or
accessor functions that call T.translate at use time) and update any code that
reads operator.label to call or access the getter (keep the operator value
fields like value unchanged); specifically modify the OPERATORS constant and
JOIN_OPERATORS to store functions/getters that invoke T.translate rather than
calling T.translate during module initialization.
---
Nitpick comments:
In `@package.json`:
- Line 85: Update the react-redux dependency in package.json from "^7.1.0" to
"^7.2.9" (the "react-redux" entry), run your package manager to update the
lockfile (npm install or yarn install), and run the test suite and the app to
ensure existing usages of connect(), useDispatch, and useSelector continue to
work; if any type or runtime regressions appear, adjust imports/usages
accordingly and commit the updated package.json and lockfile.
In `@src/components/mui/GridFilter/utils.js`:
- Around line 38-43: EMPTY_FILTER currently uses the literal id "new" which may
conflict with real IDs; update EMPTY_FILTER (and any code that compares or
serializes its id) to use a non-colliding identifier instead—either replace the
id with a Symbol (e.g., use Symbol('EMPTY_FILTER') and adjust any equality
checks to use strict identity) or generate a namespaced/unique id (e.g., prefix
with "__empty__" or use a UUID) and ensure functions that create, compare, or
send filters handle the new form (refer to EMPTY_FILTER and any usages that read
.id).
In `@src/components/mui/RoundButton/index.jsx`:
- Around line 18-29: The RoundButton component currently merges default sx
properties by object-spreading which rejects array or function sx values; update
the sx prop on the Button inside RoundButton (and the analogous block at the
other occurrence) to compose defaults using MUI's array form: provide the
defaults as the first array element and then concatenate the incoming sx (if
it's already an array spread it, otherwise append it as a single element) so
functions, arrays and objects are all supported by MUI; target the RoundButton
component and the other sx merge site mentioned and replace the object spread
with this array-composition pattern.
🪄 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: 0145fadd-9bf7-4e7e-bd74-36021910a900
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (21)
package.jsonsrc/components/index.jssrc/components/mui/Dropdown/index.jsxsrc/components/mui/GridFilter/GridFilter.jsxsrc/components/mui/GridFilter/actions/filter-actions.jssrc/components/mui/GridFilter/components/Filter.jsxsrc/components/mui/GridFilter/components/FilterButton.jsxsrc/components/mui/GridFilter/components/ValueInput/index.jsxsrc/components/mui/GridFilter/hooks/useGridFilter.jsxsrc/components/mui/GridFilter/index.jssrc/components/mui/GridFilter/readme.mdsrc/components/mui/GridFilter/reducers/all-filters-reducer.jssrc/components/mui/GridFilter/reducers/filter-reducer.jssrc/components/mui/GridFilter/utils.jssrc/components/mui/RoundButton/index.jsxsrc/components/mui/ToggleButtons/index.jsxsrc/components/mui/__tests__/Dropdown.test.jsxsrc/components/mui/__tests__/GridFilter.test.jsxsrc/components/mui/__tests__/ToggleButtons.test.jsxsrc/i18n/en.jsonwebpack.common.js
| const ValueInput = ({ type, ...rest }) => { | ||
| const Component = type ? INPUT_TYPE_MAP[type] : Dropdown; // use dropdown as a placeholder | ||
| // eslint-disable-next-line react/jsx-props-no-spreading | ||
| return Component ? <Component {...rest} /> : null; | ||
| }; | ||
|
|
||
| ValueInput.propTypes = { | ||
| id: PropTypes.string.isRequired, | ||
| type: PropTypes.string.isRequired, | ||
| value: PropTypes.oneOfType([ |
There was a problem hiding this comment.
Align type prop contract with runtime fallback behavior.
Line 22 supports missing type (falls back to Dropdown), but Line 29 requires type. This mismatch creates noisy PropTypes warnings for a supported path.
Suggested fix
- type: PropTypes.string.isRequired,
+ type: PropTypes.oneOf(["text", "select"]),📝 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.
| const ValueInput = ({ type, ...rest }) => { | |
| const Component = type ? INPUT_TYPE_MAP[type] : Dropdown; // use dropdown as a placeholder | |
| // eslint-disable-next-line react/jsx-props-no-spreading | |
| return Component ? <Component {...rest} /> : null; | |
| }; | |
| ValueInput.propTypes = { | |
| id: PropTypes.string.isRequired, | |
| type: PropTypes.string.isRequired, | |
| value: PropTypes.oneOfType([ | |
| const ValueInput = ({ type, ...rest }) => { | |
| const Component = type ? INPUT_TYPE_MAP[type] : Dropdown; // use dropdown as a placeholder | |
| // eslint-disable-next-line react/jsx-props-no-spreading | |
| return Component ? <Component {...rest} /> : null; | |
| }; | |
| ValueInput.propTypes = { | |
| id: PropTypes.string.isRequired, | |
| type: PropTypes.oneOf(["text", "select"]), | |
| value: PropTypes.oneOfType([ |
🤖 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/mui/GridFilter/components/ValueInput/index.jsx` around lines
21 - 30, ValueInput currently accepts a missing type at runtime (it falls back
to Dropdown via INPUT_TYPE_MAP lookup) but its propTypes mark type as required;
update the prop contract to match runtime behavior by making the type prop
optional (remove PropTypes.string.isRequired and use PropTypes.string) or add a
default prop for type that maps to the Dropdown key; locate the ValueInput
component and adjust the propTypes (and/or defaultProps) for the type prop so
PropTypes no longer warns when type is omitted.
ca1a369 to
baf678e
Compare
baf678e to
f9c57c2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/components/mui/GridFilter/components/ValueInput/index.jsx (1)
22-30:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake
typeprop contract consistent with runtime fallback.Line 22 supports missing
type(fallback toDropdown), but Line 29 markstypeas required, producing avoidable PropTypes warnings.Suggested fix
- type: PropTypes.string.isRequired, + type: PropTypes.string,🤖 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/mui/GridFilter/components/ValueInput/index.jsx` around lines 22 - 30, The propTypes for ValueInput currently mark the type prop as required but the render path falls back to Dropdown when type is missing (Component = type ? INPUT_TYPE_MAP[type] : Dropdown), so update ValueInput.propTypes to make type optional (change PropTypes.string.isRequired to PropTypes.string) or alternatively provide a default via ValueInput.defaultProps (e.g., type: undefined or a default key) so the runtime fallback to Dropdown is consistent with the prop contract; refer to ValueInput, INPUT_TYPE_MAP, Dropdown, and ValueInput.propTypes when making the change.
🧹 Nitpick comments (3)
src/components/mui/__tests__/GridFilter.test.jsx (1)
73-73: ⚡ Quick winBrittle
getByRole("button")queries.
screen.getByRole("button")will throw as soon asGridFilter/FilterButtonrenders more than one button (e.g., a badge clear icon, a count chip), which is likely as this feature evolves. Consider scoping by accessible name (e.g.,getByRole("button", { name: "grid_filter.open_filters" })) to make these tests resilient.🤖 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/mui/__tests__/GridFilter.test.jsx` at line 73, The test is using a brittle screen.getByRole("button") which will break if GridFilter / FilterButton renders additional buttons; update the assertion in GridFilter.test.jsx to scope the role lookup by accessible name (e.g., use screen.getByRole("button", { name: "grid_filter.open_filters" }) or the actual i18n label used by FilterButton) so the test targets the intended FilterButton element in the GridFilter component; ensure you reference the same accessible name used in FilterButton/GridFilter to keep the test stable as UI adds other buttons.src/i18n/en.json (1)
143-144: 💤 Low valueConfirm trailing/leading spaces are intentional.
"filter_by": "Filter by "and"following": " one of the following: "carry padding spaces, presumably to concatenate fragments at render time. Concatenating translated fragments is fragile for i18n (word order varies by locale). If these are stitched together with a criteria/operator label in JSX, prefer a single template like"filter_by_following": "Filter by {criteria} one of the following: "so future translations can reorder freely.🤖 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/i18n/en.json` around lines 143 - 144, The two i18n keys "filter_by" and "following" include trailing/leading spaces and are being concatenated; replace them with a single templated key (e.g., "filter_by_following": "Filter by {criteria} one of the following:") and remove the old "filter_by" and "following" keys, then update any call sites that currently concatenate "filter_by" + criteria + "following" (look for uses of keys "filter_by" and "following") to instead call the translation for "filter_by_following" and pass the criteria as the {criteria} interpolation parameter so translators can reorder words safely.webpack.common.js (1)
90-92: 💤 Low valueImplicit index resolution is safe for the added MUI entry directories
components/mui/dropdown,components/mui/toggle-buttons, andcomponents/mui/round-buttoneach point to a directory containing anindex.jsx(andcomponents/mui/grid-filtercontainsindex.js).webpack.common.jssetsresolve.extensionsto['.js', '.jsx', '.json']and does not overrideresolve.mainFiles, so webpack’s defaultindexmain-file lookup will resolve these correctly; using explicit.../index.jsxpaths would be optional consistency/robustness rather than a fix.🤖 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 `@webpack.common.js` around lines 90 - 92, The three webpack entries 'components/mui/dropdown', 'components/mui/toggle-buttons', and 'components/mui/round-button' are safe to keep as-is because resolve.extensions includes '.jsx' and webpack will use the default 'index' main-file lookup; no change is required. If you prefer explicit consistency, update those entry values to point to the actual files (e.g. './src/components/mui/Dropdown/index.jsx', './src/components/mui/ToggleButtons/index.jsx', './src/components/mui/RoundButton/index.jsx') and note that 'components/mui/grid-filter' should point to './src/components/mui/GridFilter/index.js' to reflect its .js index.
🤖 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/mui/GridFilter/components/Filter.jsx`:
- Around line 49-53: The useEffect is incorrectly treating valid falsy values
(0, false) as unset by using the falsy check !value?.value; update the condition
in the effect that sets the sole option so it only triggers when value.value is
null or undefined (e.g. replace !value?.value with value?.value == null or
(value?.value === undefined || value?.value === null)), keeping the rest of the
logic (options = valueSettings.props?.options; if (options?.length === 1 && ...
) handleChange("value", options[0].value);) unchanged.
In `@src/components/mui/GridFilter/reducers/filter-reducer.js`:
- Around line 11-16: The FIL_${SAVE_FILTERS} branch in filterReducer
destructures payload without a guard which will throw if action.payload is
undefined; update the reducer to default payload to an empty object (e.g., treat
payload = {} when extracting { id, filters, joinOperator }) and give
joinOperator a sensible default (or fall back to existing default constant) so
that calling saveFilters(id) or useGridFilter.resetFilters (which may send no
payload) won't crash the reducer.
---
Duplicate comments:
In `@src/components/mui/GridFilter/components/ValueInput/index.jsx`:
- Around line 22-30: The propTypes for ValueInput currently mark the type prop
as required but the render path falls back to Dropdown when type is missing
(Component = type ? INPUT_TYPE_MAP[type] : Dropdown), so update
ValueInput.propTypes to make type optional (change PropTypes.string.isRequired
to PropTypes.string) or alternatively provide a default via
ValueInput.defaultProps (e.g., type: undefined or a default key) so the runtime
fallback to Dropdown is consistent with the prop contract; refer to ValueInput,
INPUT_TYPE_MAP, Dropdown, and ValueInput.propTypes when making the change.
---
Nitpick comments:
In `@src/components/mui/__tests__/GridFilter.test.jsx`:
- Line 73: The test is using a brittle screen.getByRole("button") which will
break if GridFilter / FilterButton renders additional buttons; update the
assertion in GridFilter.test.jsx to scope the role lookup by accessible name
(e.g., use screen.getByRole("button", { name: "grid_filter.open_filters" }) or
the actual i18n label used by FilterButton) so the test targets the intended
FilterButton element in the GridFilter component; ensure you reference the same
accessible name used in FilterButton/GridFilter to keep the test stable as UI
adds other buttons.
In `@src/i18n/en.json`:
- Around line 143-144: The two i18n keys "filter_by" and "following" include
trailing/leading spaces and are being concatenated; replace them with a single
templated key (e.g., "filter_by_following": "Filter by {criteria} one of the
following:") and remove the old "filter_by" and "following" keys, then update
any call sites that currently concatenate "filter_by" + criteria + "following"
(look for uses of keys "filter_by" and "following") to instead call the
translation for "filter_by_following" and pass the criteria as the {criteria}
interpolation parameter so translators can reorder words safely.
In `@webpack.common.js`:
- Around line 90-92: The three webpack entries 'components/mui/dropdown',
'components/mui/toggle-buttons', and 'components/mui/round-button' are safe to
keep as-is because resolve.extensions includes '.jsx' and webpack will use the
default 'index' main-file lookup; no change is required. If you prefer explicit
consistency, update those entry values to point to the actual files (e.g.
'./src/components/mui/Dropdown/index.jsx',
'./src/components/mui/ToggleButtons/index.jsx',
'./src/components/mui/RoundButton/index.jsx') and note that
'components/mui/grid-filter' should point to
'./src/components/mui/GridFilter/index.js' to reflect its .js index.
🪄 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: fa84d986-2e78-44b1-8f7d-7464ee122ea7
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (21)
package.jsonsrc/components/index.jssrc/components/mui/Dropdown/index.jsxsrc/components/mui/GridFilter/GridFilter.jsxsrc/components/mui/GridFilter/actions/filter-actions.jssrc/components/mui/GridFilter/components/Filter.jsxsrc/components/mui/GridFilter/components/FilterButton.jsxsrc/components/mui/GridFilter/components/ValueInput/index.jsxsrc/components/mui/GridFilter/hooks/useGridFilter.jsxsrc/components/mui/GridFilter/index.jssrc/components/mui/GridFilter/readme.mdsrc/components/mui/GridFilter/reducers/all-filters-reducer.jssrc/components/mui/GridFilter/reducers/filter-reducer.jssrc/components/mui/GridFilter/utils.jssrc/components/mui/RoundButton/index.jsxsrc/components/mui/ToggleButtons/index.jsxsrc/components/mui/__tests__/Dropdown.test.jsxsrc/components/mui/__tests__/GridFilter.test.jsxsrc/components/mui/__tests__/ToggleButtons.test.jsxsrc/i18n/en.jsonwebpack.common.js
✅ Files skipped from review due to trivial changes (2)
- src/components/mui/GridFilter/index.js
- src/components/mui/GridFilter/utils.js
|
done @smarcet |
https://app.clickup.com/t/86b9ruudz
Summary by CodeRabbit
New Features
Documentation
Tests
Chores