Feature: new grid filter#242
Open
santipalenque wants to merge 4 commits into
Open
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 ignored due to path filters (1)
📒 Files selected for processing (21)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (17)
📝 WalkthroughWalkthroughAdds a dialog-driven GridFilter with Redux persistence, supporting utilities and subcomponents (Dropdown, RoundButton, ToggleButtons), tests, i18n strings, webpack entries, and upgrades react-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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
| export {default as MuiDropdown} from './mui/Dropdown' | ||
| export {default as MuiRoundButton} from './mui/RoundButton' | ||
| export {default as MuiToggleButtons} from './mui/ToggleButtons' | ||
| export {default as MuiGridFilter, OPERATORS as FILTER_OPERATORS, JOIN_OPERATORS as FILTER_JOIN_OPERATORS, EMPTY_FILTER as FILTER_EMPTY_FILTER, useGridFilter as useGridFilter, allFiltersReducer as allFiltersReducer, saveFilters as saveGridFilters, SAVE_FILTERS as SAVE_FILTERS } from './mui/GridFilter' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Verify GridFilter barrel has NO default export:"
rg -nP '^\s*export\s+default\b' src/components/mui/GridFilter/index.js || true
echo
echo "2) Show GridFilter barrel exports:"
nl -ba src/components/mui/GridFilter/index.js | sed -n '1,20p'
echo
echo "3) Show top-level re-export line:"
nl -ba src/components/index.js | sed -n '120,124p'Repository: OpenStackweb/openstack-uicore-foundation
Length of output: 232
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Looking for GridFilter module ==="
find src -name "*GridFilter*" -type f 2>/dev/null | head -20
echo
echo "=== Checking if src/components/mui/GridFilter/index.js exists ==="
if [ -f "src/components/mui/GridFilter/index.js" ]; then
echo "File exists. Reading it:"
cat -n src/components/mui/GridFilter/index.js | head -50
else
echo "File not found at that path. Let's search:"
find . -path "*/GridFilter/index.js" 2>/dev/null | head -5
fi
echo
echo "=== Reading src/components/index.js around line 123 ==="
if [ -f "src/components/index.js" ]; then
cat -n src/components/index.js | sed -n '115,130p'
else
echo "File not found at that path"
fiRepository: OpenStackweb/openstack-uicore-foundation
Length of output: 2044
Fix mismatched export type for MuiGridFilter on Line 123.
Line 123 attempts to import a default export from ./mui/GridFilter, but the GridFilter barrel file (src/components/mui/GridFilter/index.js) exports GridFilter and all other symbols as named exports only. There is no default export. This will cause a build/runtime error.
Change line 123 to import these symbols as named exports:
Proposed fix
-export {default as MuiGridFilter, OPERATORS as FILTER_OPERATORS, JOIN_OPERATORS as FILTER_JOIN_OPERATORS, EMPTY_FILTER as FILTER_EMPTY_FILTER, useGridFilter as useGridFilter, allFiltersReducer as allFiltersReducer, saveFilters as saveGridFilters, SAVE_FILTERS as SAVE_FILTERS } from './mui/GridFilter'
+export {
+ GridFilter as MuiGridFilter,
+ OPERATORS as FILTER_OPERATORS,
+ JOIN_OPERATORS as FILTER_JOIN_OPERATORS,
+ EMPTY_FILTER as FILTER_EMPTY_FILTER,
+ useGridFilter,
+ allFiltersReducer,
+ saveFilters as saveGridFilters,
+ SAVE_FILTERS
+} from './mui/GridFilter'📝 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
| export {default as MuiGridFilter, OPERATORS as FILTER_OPERATORS, JOIN_OPERATORS as FILTER_JOIN_OPERATORS, EMPTY_FILTER as FILTER_EMPTY_FILTER, useGridFilter as useGridFilter, allFiltersReducer as allFiltersReducer, saveFilters as saveGridFilters, SAVE_FILTERS as SAVE_FILTERS } from './mui/GridFilter' | |
| export { | |
| GridFilter as MuiGridFilter, | |
| OPERATORS as FILTER_OPERATORS, | |
| JOIN_OPERATORS as FILTER_JOIN_OPERATORS, | |
| EMPTY_FILTER as FILTER_EMPTY_FILTER, | |
| useGridFilter, | |
| allFiltersReducer, | |
| saveFilters as saveGridFilters, | |
| SAVE_FILTERS | |
| } from './mui/GridFilter' |
🤖 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/index.js` at 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.
Comment on lines
+21
to
+30
| 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.
Suggested change
| 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.
Comment on lines
+7
to
+10
| const allFilters = useSelector( | ||
| (state) => state.allGridFiltersState.allFilters | ||
| ); | ||
| const filter = allFilters.find((f) => f.id === id) || {}; |
There was a problem hiding this comment.
Guard Redux slice access to avoid render-time crashes.
Line 8 assumes allGridFiltersState always exists; if it is missing, this hook throws before returning defaults.
🐛 Proposed fix
- const allFilters = useSelector(
- (state) => state.allGridFiltersState.allFilters
- );
+ const allFilters = useSelector(
+ (state) => state.allGridFiltersState?.allFilters ?? []
+ );📝 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
| const allFilters = useSelector( | |
| (state) => state.allGridFiltersState.allFilters | |
| ); | |
| const filter = allFilters.find((f) => f.id === id) || {}; | |
| const allFilters = useSelector( | |
| (state) => state.allGridFiltersState?.allFilters ?? [] | |
| ); | |
| const filter = allFilters.find((f) => f.id === id) || {}; |
🤖 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/hooks/useGridFilter.jsx` around lines 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.
Comment on lines
+3
to
+36
| export const OPERATORS = { | ||
| IS: { value: "==", label: T.translate("grid_filter.operators.is") }, | ||
| IS_NOT: { value: "<>", label: T.translate("grid_filter.operators.is_not") }, | ||
| LIKE: { value: "=@", label: T.translate("grid_filter.operators.like") }, | ||
| LIKE_START: { | ||
| value: "@@", | ||
| label: T.translate("grid_filter.operators.like_start") | ||
| }, | ||
| HAS: { value: ">>", label: T.translate("grid_filter.operators.has") }, // not available on API | ||
| HAS_NOT: { | ||
| value: "!>>", | ||
| label: T.translate("grid_filter.operators.has_not") | ||
| }, // not available on API | ||
| LESS: { value: "<", label: T.translate("grid_filter.operators.less") }, | ||
| LESS_OR_EQUAL: { | ||
| value: "<=", | ||
| label: T.translate("grid_filter.operators.less_or_equal") | ||
| }, | ||
| GREATER: { value: ">", label: T.translate("grid_filter.operators.greater") }, | ||
| GREATER_OR_EQUAL: { | ||
| value: ">=", | ||
| label: T.translate("grid_filter.operators.greater_or_equal") | ||
| }, | ||
| BETWEEN: { value: "[]", label: T.translate("grid_filter.operators.between") }, | ||
| BETWEEN_STRICT: { | ||
| value: "()", | ||
| label: T.translate("grid_filter.operators.between_strict") | ||
| } | ||
| }; | ||
|
|
||
| export const JOIN_OPERATORS = { | ||
| ALL: T.translate("grid_filter.operators.all"), | ||
| ANY: T.translate("grid_filter.operators.any") | ||
| }; |
There was a problem hiding this comment.
Translations are evaluated eagerly at module initialization.
The T.translate() calls execute when this module loads, before i18n may be initialized. If the i18n system hasn't loaded translations yet, this will return translation keys instead of localized strings, causing all operator labels to display as "grid_filter.operators.is" etc.
🔧 Proposed fix using getter functions
-export const OPERATORS = {
- IS: { value: "==", label: T.translate("grid_filter.operators.is") },
- IS_NOT: { value: "<>", label: T.translate("grid_filter.operators.is_not") },
- LIKE: { value: "=@", label: T.translate("grid_filter.operators.like") },
- LIKE_START: {
- value: "@@",
- label: T.translate("grid_filter.operators.like_start")
- },
- HAS: { value: ">>", label: T.translate("grid_filter.operators.has") },
- HAS_NOT: {
- value: "!>>",
- label: T.translate("grid_filter.operators.has_not")
- },
- LESS: { value: "<", label: T.translate("grid_filter.operators.less") },
- LESS_OR_EQUAL: {
- value: "<=",
- label: T.translate("grid_filter.operators.less_or_equal")
- },
- GREATER: { value: ">", label: T.translate("grid_filter.operators.greater") },
- GREATER_OR_EQUAL: {
- value: ">=",
- label: T.translate("grid_filter.operators.greater_or_equal")
- },
- BETWEEN: { value: "[]", label: T.translate("grid_filter.operators.between") },
- BETWEEN_STRICT: {
- value: "()",
- label: T.translate("grid_filter.operators.between_strict")
- }
-};
+const getOperatorLabel = (key) => T.translate(`grid_filter.operators.${key}`);
+
+export const OPERATORS = {
+ IS: { value: "==", get label() { return getOperatorLabel("is"); } },
+ IS_NOT: { value: "<>", get label() { return getOperatorLabel("is_not"); } },
+ LIKE: { value: "=@", get label() { return getOperatorLabel("like"); } },
+ LIKE_START: { value: "@@", get label() { return getOperatorLabel("like_start"); } },
+ HAS: { value: ">>", get label() { return getOperatorLabel("has"); } },
+ HAS_NOT: { value: "!>>", get label() { return getOperatorLabel("has_not"); } },
+ LESS: { value: "<", get label() { return getOperatorLabel("less"); } },
+ LESS_OR_EQUAL: { value: "<=", get label() { return getOperatorLabel("less_or_equal"); } },
+ GREATER: { value: ">", get label() { return getOperatorLabel("greater"); } },
+ GREATER_OR_EQUAL: { value: ">=", get label() { return getOperatorLabel("greater_or_equal"); } },
+ BETWEEN: { value: "[]", get label() { return getOperatorLabel("between"); } },
+ BETWEEN_STRICT: { value: "()", get label() { return getOperatorLabel("between_strict"); } }
+};
-export const JOIN_OPERATORS = {
- ALL: T.translate("grid_filter.operators.all"),
- ANY: T.translate("grid_filter.operators.any")
-};
+export const JOIN_OPERATORS = {
+ get ALL() { return T.translate("grid_filter.operators.all"); },
+ get ANY() { return T.translate("grid_filter.operators.any"); }
+};🤖 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 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.
Comment on lines
+11
to
+15
| HAS: { value: ">>", label: T.translate("grid_filter.operators.has") }, // not available on API | ||
| HAS_NOT: { | ||
| value: "!>>", | ||
| label: T.translate("grid_filter.operators.has_not") | ||
| }, // not available on API |
There was a problem hiding this comment.
Clarify operators marked as "not available on API".
The HAS and HAS_NOT operators are marked with // not available on API comments. If these operators are not supported by the backend API, including them in the UI could lead to runtime errors or unexpected behavior when filters are applied.
Consider either:
- Removing these operators if they cannot be used
- Documenting how/when they should be used (e.g., client-side filtering only)
- Adding runtime validation to prevent their submission to the API
🤖 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 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.
santipalenque
force-pushed
the
feature/new-grid-filter
branch
from
ca1a369 to
baf678e
Compare
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
1 participant
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