Feature/grid filter continued#269
Conversation
📝 WalkthroughWalkthroughExtends GridFilter with five new value input types (datetime, number, asyncSelect, speaker, company) backed by async option loading with debounce and customParser validation. Introduces useGridFilter.setFilters() for external filter state management. Creates new BulkEditTable component with row selection, inline editing, and toolbar actions. Extracts pagination logic into CustomTablePagination and integrates across all table variants. Adds comprehensive tests and documentation. ChangesGridFilter new value types, async select, and setFilters API
BulkEditTable: row selection, inline editing, and pagination
Table pagination refactoring: extract and integrate CustomTablePagination
Sequence Diagram(s)sequenceDiagram
participant Filter
participant ValueInput
participant AsyncSelectInput
participant queryFunction
participant GridFilter_parseFilter
Filter->>ValueInput: render(type="company", value, options)
ValueInput->>AsyncSelectInput: delegate via INPUT_TYPE_MAP
AsyncSelectInput->>queryFunction: queryFunction("", callback) on mount
queryFunction-->>AsyncSelectInput: callback(companies)
AsyncSelectInput->>Filter: onChange({target:{value:{value,label,raw}}})
Filter->>GridFilter_parseFilter: handleSubmit(filters)
GridFilter_parseFilter->>GridFilter_parseFilter: criteria=criterias[filter.criteria]
GridFilter_parseFilter->>GridFilter_parseFilter: check ASYNC_VALUE_TYPES.includes(criteria.type)
alt customParser missing
GridFilter_parseFilter->>GridFilter_parseFilter: console.error
else customParser provided
GridFilter_parseFilter->>GridFilter_parseFilter: return customParser(filter)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~90 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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. 🔧 OpenGrep (1.23.0)src/components/mui/BulkEditTable/components/Toolbar.js┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.15][ERROR]: unable to find a config; path src/components/mui/BulkEditTable/hooks/useRowSelection.js┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.15][ERROR]: unable to find a config; path src/components/mui/editable-table/mui-table-editable.js┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.15][ERROR]: unable to find a config; path
🔧 Stylelint (17.13.0)src/components/mui/BulkEditTable/BulkEditTable.module.lessError: ENOENT: no such file or directory, open '/.stylelintrc.json' 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/mui/Dropdown/index.jsx (1)
42-63:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAdd optional chaining to prevent crash when options is null.
Now that
optionscan benull(per the newdefaultPropsat line 99), line 52 and line 59 are missing the optional chaining guard that line 65 already has.Crash scenario for line 52:
- Dropdown receives
multiple={true}via the{...rest}spread (line 41)valueis an array (e.g.,[1, 2])optionsisnullrenderValuereaches line 50'sArray.isArray(selected)check → true- Line 52 executes
options.map(...)→ runtime crash: "Cannot read property 'map' of null"Crash scenario for line 59:
- Single-select mode (default)
valueis a non-empty scalaroptionsisnull- Line 59 executes
options.find(...)→ runtime crash🛡️ Proposed fix
if (Array.isArray(selected)) { const lookup = Object.fromEntries( - options.map((o) => [o.value, o.label]) + options?.map((o) => [o.value, o.label]) || [] ); return selected .map((v) => lookup[v]) .filter(Boolean) .join(", "); } - const selectedOption = options.find( + const selectedOption = options?.find( ({ value }) => value === selected );🤖 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/Dropdown/index.jsx` around lines 42 - 63, The renderValue function in the Dropdown component is missing optional chaining guards on the options parameter. At line 52 where options.map is called to create the lookup object for array values, and at line 59 where options.find is called to find the selected option label, add optional chaining (the ?. operator) before the .map() and .find() method calls respectively. This will prevent runtime crashes when options is null by ensuring these methods are only called if options exists.
🧹 Nitpick comments (1)
src/components/mui/GridFilter/utils.js (1)
31-32: Operator value collision: affects semantic preservation on deserialization.
BEFOREandAFTERreuse the same operator values ("<="and">=") asLESS_OR_EQUALandGREATER_OR_EQUAL. TheparseFilterfunction serializes filters using only the operator value string (e.g.,"fieldName<=value"), with no way to reconstruct which semantic intent was originally selected. On filter deserialization, a filter created withBEFOREbecomes indistinguishable from one created withLESS_OR_EQUAL, losing temporal context.If this aliasing is intentional (semantic labels for date-specific contexts), document the design. Otherwise, consider using distinct values (e.g.,
"<="for numeric,"<@"for temporal) to preserve semantic information.🤖 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 31 - 32, The BEFORE and AFTER operators in the grid filter operators object are using the same operator values as LESS_OR_EQUAL and GREATER_OR_EQUAL respectively, causing operator value collision. When filters are serialized and later deserialized through the parseFilter function, there is no way to distinguish whether a filter was originally created with BEFORE or LESS_OR_EQUAL (both use "<="), losing the semantic temporal context. Change the operator values for BEFORE and AFTER to use distinct values that do not collide with numeric comparison operators (for example, use "<@" for BEFORE and ">@" for AFTER instead of reusing "<=" and ">=") to preserve semantic information during serialization and deserialization.
🤖 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/ValueInput/AsyncSelectInput.jsx`:
- Around line 56-74: The fetchOptions function has a race condition where slower
earlier requests can overwrite newer results when the user types quickly, and
callbacks can also update state after unmount. Add a ref to track the mounted
status and another ref to track the current request ID. In the useEffect, set
mounted to true and clear it in the cleanup function. Modify fetchOptions to
generate a unique request ID for each call and pass it to the queryFunction
callback. In the callback, before calling setOptions and setLoading, check that
the request ID matches the current request ID stored in the ref and that the
component is still mounted. This ensures only the latest active request mutates
state and prevents updates after unmount.
---
Outside diff comments:
In `@src/components/mui/Dropdown/index.jsx`:
- Around line 42-63: The renderValue function in the Dropdown component is
missing optional chaining guards on the options parameter. At line 52 where
options.map is called to create the lookup object for array values, and at line
59 where options.find is called to find the selected option label, add optional
chaining (the ?. operator) before the .map() and .find() method calls
respectively. This will prevent runtime crashes when options is null by ensuring
these methods are only called if options exists.
---
Nitpick comments:
In `@src/components/mui/GridFilter/utils.js`:
- Around line 31-32: The BEFORE and AFTER operators in the grid filter operators
object are using the same operator values as LESS_OR_EQUAL and GREATER_OR_EQUAL
respectively, causing operator value collision. When filters are serialized and
later deserialized through the parseFilter function, there is no way to
distinguish whether a filter was originally created with BEFORE or LESS_OR_EQUAL
(both use "<="), losing the semantic temporal context. Change the operator
values for BEFORE and AFTER to use distinct values that do not collide with
numeric comparison operators (for example, use "<@" for BEFORE and ">@" for
AFTER instead of reusing "<=" and ">=") to preserve semantic information during
serialization and deserialization.
🪄 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: 87ce6b08-1d5e-4fb1-b5ad-f9aefa59e15b
📒 Files selected for processing (13)
src/components/mui/Dropdown/index.jsxsrc/components/mui/GridFilter/GridFilter.jsxsrc/components/mui/GridFilter/components/Filter.jsxsrc/components/mui/GridFilter/components/ValueInput/AsyncSelectInput.jsxsrc/components/mui/GridFilter/components/ValueInput/CompanySelectInput.jsxsrc/components/mui/GridFilter/components/ValueInput/DateTimeInput.jsxsrc/components/mui/GridFilter/components/ValueInput/NumberInput.jsxsrc/components/mui/GridFilter/components/ValueInput/SpeakerSelectInput.jsxsrc/components/mui/GridFilter/components/ValueInput/index.jsxsrc/components/mui/GridFilter/readme.mdsrc/components/mui/GridFilter/utils.jssrc/components/mui/__tests__/GridFilter.test.jsxsrc/i18n/en.json
| const fetchOptions = (searchTerm) => { | ||
| if (searchTerm && searchTerm.length < minSearchLength) { | ||
| setOptions([]); | ||
| return; | ||
| } | ||
| setLoading(true); | ||
| queryFunction(searchTerm, (rawResults) => { | ||
| setOptions((rawResults || []).map((item) => ({ ...formatOption(item), raw: item }))); | ||
| setLoading(false); | ||
| }); | ||
| }; | ||
|
|
||
| useEffect(() => { | ||
| fetchOptions(""); | ||
| return () => { | ||
| if (debounceRef.current) clearTimeout(debounceRef.current); | ||
| }; | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, []); |
There was a problem hiding this comment.
Guard against stale/out-of-order async responses when typing quickly.
fetchOptions accepts every callback result, so slower earlier requests can overwrite newer results, and callbacks can still update state after unmount. Track a request id + mounted flag so only the latest active request mutates state.
💡 Suggested fix
@@
const [options, setOptions] = useState([]);
const [loading, setLoading] = useState(false);
const debounceRef = useRef(null);
+ const lastRequestIdRef = useRef(0);
+ const isMountedRef = useRef(true);
@@
const fetchOptions = (searchTerm) => {
+ const requestId = ++lastRequestIdRef.current;
if (searchTerm && searchTerm.length < minSearchLength) {
setOptions([]);
+ setLoading(false);
return;
}
setLoading(true);
queryFunction(searchTerm, (rawResults) => {
+ if (!isMountedRef.current || requestId !== lastRequestIdRef.current) return;
setOptions((rawResults || []).map((item) => ({ ...formatOption(item), raw: item })));
setLoading(false);
});
};
@@
useEffect(() => {
fetchOptions("");
return () => {
+ isMountedRef.current = false;
if (debounceRef.current) clearTimeout(debounceRef.current);
};Also applies to: 76-80
🤖 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/AsyncSelectInput.jsx`
around lines 56 - 74, The fetchOptions function has a race condition where
slower earlier requests can overwrite newer results when the user types quickly,
and callbacks can also update state after unmount. Add a ref to track the
mounted status and another ref to track the current request ID. In the
useEffect, set mounted to true and clear it in the cleanup function. Modify
fetchOptions to generate a unique request ID for each call and pass it to the
queryFunction callback. In the callback, before calling setOptions and
setLoading, check that the request ID matches the current request ID stored in
the ref and that the component is still mounted. This ensures only the latest
active request mutates state and prevents updates after unmount.
There was a problem hiding this comment.
Confirmed valid — and more complete than what our review caught. This covers two compounding issues: (1) callbacks firing on an unmounted component when the user switches criterion while a query is in-flight, and (2) out-of-order responses overwriting newer results when the user types quickly (slow earlier request arrives after a faster later one). The requestId + isMountedRef pattern here addresses both. @santipalenque please address.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@package.json`:
- Line 3: The `@mui/x-date-pickers` package is currently listed only in
devDependencies but needs to be added to peerDependencies as well, since the new
DateTimeInput component (which uses DateTimePicker from this package) is
exported as part of the GridFilter component library. Add `@mui/x-date-pickers` to
the peerDependencies section in package.json with the same version constraint as
what is currently specified in devDependencies, so that consuming applications
receive clear guidance about this required dependency when using GridFilter with
the datetime value type.
In `@src/components/mui/GridFilter/hooks/useGridFilter.jsx`:
- Around line 29-30: The setFilters function on line 29-30 accepts any
joinOperator value without validation before dispatching to saveFilters, which
allows invalid values to be persisted in state and cause silent semantic drift
in the filter-reducer. Add validation logic inside setFilters to ensure the
joinOperator parameter is clamped to only valid values from JOIN_OPERATORS
(specifically ALL or ANY) before passing it to the dispatch call, using a safe
default value if an invalid value is provided.
🪄 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: 2cbf0bec-fe2e-4eb4-9eb6-95aa3c0796d9
📒 Files selected for processing (4)
package.jsonsrc/components/mui/GridFilter/hooks/useGridFilter.jsxsrc/components/mui/GridFilter/readme.mdsrc/components/mui/__tests__/useGridFilter.test.jsx
| const setFilters = (filters = [], joinOperator = JOIN_OPERATORS.ALL) => | ||
| dispatch(saveFilters(id, filters, joinOperator)); |
There was a problem hiding this comment.
Validate joinOperator before dispatching.
Line 29 forwards any joinOperator value into state; an invalid value is persisted and skips the JOIN_OPERATORS.ANY path in filter-reducer, so parsed semantics can silently drift. Clamp to ALL | ANY at this API boundary.
Suggested patch
- const setFilters = (filters = [], joinOperator = JOIN_OPERATORS.ALL) =>
- dispatch(saveFilters(id, filters, joinOperator));
+ const setFilters = (filters = [], joinOperator = JOIN_OPERATORS.ALL) => {
+ const safeJoinOperator =
+ joinOperator === JOIN_OPERATORS.ANY
+ ? JOIN_OPERATORS.ANY
+ : JOIN_OPERATORS.ALL;
+ dispatch(saveFilters(id, filters, safeJoinOperator));
+ };🤖 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 29 - 30,
The setFilters function on line 29-30 accepts any joinOperator value without
validation before dispatching to saveFilters, which allows invalid values to be
persisted in state and cause silent semantic drift in the filter-reducer. Add
validation logic inside setFilters to ensure the joinOperator parameter is
clamped to only valid values from JOIN_OPERATORS (specifically ALL or ANY)
before passing it to the dispatch call, using a safe default value if an invalid
value is provided.
There was a problem hiding this comment.
Confirmed valid. An invalid joinOperator value passed to setFilters is written directly into Redux state without validation, silently affecting filter semantics in the reducer. The clamping guard is a 3-line fix at a clear API boundary. @santipalenque please address.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/components/mui/BulkEditTable/__tests__/BulkEditTable.test.js (1)
41-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTighten payload assertion to catch accidental extra-row updates.
arrayContainingonly verifies that rowid: 1exists, so this test still passes ifonUpdateincorrectly receives additional unselected rows.Suggested test assertion update
await waitFor(() => { expect(onUpdate).toHaveBeenCalledTimes(1); - expect(onUpdate).toHaveBeenCalledWith( - expect.arrayContaining([expect.objectContaining({ id: 1 })]) - ); + expect(onUpdate).toHaveBeenCalledWith([ + expect.objectContaining({ id: 1 }) + ]); });🤖 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/BulkEditTable/__tests__/BulkEditTable.test.js` around lines 41 - 45, The assertion in the test for onUpdate uses arrayContaining which only verifies that the row with id: 1 is present in the payload but does not check that no additional unselected rows are included. Replace the arrayContaining assertion in the onUpdate expectation with a stricter assertion that verifies the exact contents of the array, ensuring both that id: 1 is present and that no extra or unselected rows are accidentally included in the update payload. This will catch cases where onUpdate is incorrectly called with additional rows beyond what was selected.
🤖 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/BulkEditTable/BulkEditTable.js`:
- Around line 44-52: The handleUpdateEvents function assumes onUpdate always
returns a Promise, but it could be a synchronous handler which would crash when
calling .then(). Wrap the result of calling onUpdate(selectedRows) with
Promise.resolve() to normalize both synchronous and asynchronous returns into a
Promise before chaining the .then() and .catch() methods. This ensures the code
handles both Promise-returning and synchronous onUpdate handlers gracefully.
In `@src/components/mui/BulkEditTable/components/Cell.js`:
- Line 18: In the Cell component's input value binding, the logical OR operator
(`||`) is coercing valid falsy values like 0 and false to empty strings,
corrupting the edited data. Replace the `||` operator with the nullish
coalescing operator (`??`) in the value assignment for editRow[col.columnKey] so
that only null and undefined values default to empty string, while preserving
legitimate falsy values like 0 and false.
In `@src/components/mui/BulkEditTable/hooks/useRowSelection.js`:
- Around line 9-22: The selection check functions isSelected and isAllSelected
are reading from the captured selectedRows state in the closure, which can
become stale during batched updates. Refactor isSelected to accept the current
selectedRows array as a parameter instead of relying on the closure's
selectedRows state. Then update both toggleRow and toggleAll to pass the current
parameter from their updater functions (or the computed selection state) to
these check functions, ensuring selection comparisons always use the most
current state rather than stale closure values.
In `@src/components/mui/table/CustomTablePagination.js`:
- Around line 60-62: In the handleRowsPerPageChange function, the
ev.target.value is being passed directly to onPerPageChange, but select element
values are always strings and the callback expects a number. Parse the select
value to an integer by wrapping ev.target.value with parseInt() or Number()
before passing it to the onPerPageChange callback to ensure type consistency
with the perPage prop and prevent potential bugs in parent components.
- Around line 46-54: The initialPerPage useRef captures the perPage value only
at component mount and never updates, causing the perPageOptions array to become
stale when the parent component changes the perPage prop after mount, creating a
mismatch between the displayed value and available options. Replace the useRef
pattern with React.useMemo for the perPageOptions variable, removing the
initialPerPage reference and using the perPage prop directly in the calculation,
then include both perPage and onPerPageChange in the useMemo dependency array to
ensure the options rebuild whenever either dependency changes.
---
Nitpick comments:
In `@src/components/mui/BulkEditTable/__tests__/BulkEditTable.test.js`:
- Around line 41-45: The assertion in the test for onUpdate uses arrayContaining
which only verifies that the row with id: 1 is present in the payload but does
not check that no additional unselected rows are included. Replace the
arrayContaining assertion in the onUpdate expectation with a stricter assertion
that verifies the exact contents of the array, ensuring both that id: 1 is
present and that no extra or unselected rows are accidentally included in the
update payload. This will catch cases where onUpdate is incorrectly called with
additional rows beyond what was selected.
🪄 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: bcdb4596-04ac-49e5-b270-e66551523da7
📒 Files selected for processing (16)
package.jsonsrc/components/index.jssrc/components/mui/BulkEditTable/BulkEditTable.jssrc/components/mui/BulkEditTable/BulkEditTable.module.lesssrc/components/mui/BulkEditTable/__tests__/BulkEditTable.test.jssrc/components/mui/BulkEditTable/components/Cell.jssrc/components/mui/BulkEditTable/components/Heading.jssrc/components/mui/BulkEditTable/components/Row.jssrc/components/mui/BulkEditTable/components/Toolbar.jssrc/components/mui/BulkEditTable/hooks/useRowSelection.jssrc/components/mui/BulkEditTable/index.jssrc/components/mui/editable-table/mui-table-editable.jssrc/components/mui/sortable-table/mui-table-sortable.jssrc/components/mui/table/CustomTablePagination.jssrc/components/mui/table/mui-table.jswebpack.common.js
✅ Files skipped from review due to trivial changes (4)
- src/components/mui/BulkEditTable/BulkEditTable.module.less
- src/components/mui/BulkEditTable/components/Toolbar.js
- package.json
- src/components/mui/BulkEditTable/index.js
| const handleUpdateEvents = (evt) => { | ||
| evt.stopPropagation(); | ||
| evt.preventDefault(); | ||
| onUpdate(selectedRows) | ||
| .then(() => reset()) | ||
| .catch((error) => { | ||
| console.error("Error updating events:", error); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Normalize onUpdate result before chaining .then().
This assumes onUpdate always returns a Promise. A synchronous handler will crash on .then.
Suggested fix
const handleUpdateEvents = (evt) => {
evt.stopPropagation();
evt.preventDefault();
- onUpdate(selectedRows)
+ Promise.resolve(onUpdate(selectedRows))
.then(() => reset())
.catch((error) => {
console.error("Error updating events:", error);
});
};📝 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 handleUpdateEvents = (evt) => { | |
| evt.stopPropagation(); | |
| evt.preventDefault(); | |
| onUpdate(selectedRows) | |
| .then(() => reset()) | |
| .catch((error) => { | |
| console.error("Error updating events:", error); | |
| }); | |
| }; | |
| const handleUpdateEvents = (evt) => { | |
| evt.stopPropagation(); | |
| evt.preventDefault(); | |
| Promise.resolve(onUpdate(selectedRows)) | |
| .then(() => reset()) | |
| .catch((error) => { | |
| console.error("Error updating events:", error); | |
| }); | |
| }; |
🤖 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/BulkEditTable/BulkEditTable.js` around lines 44 - 52, The
handleUpdateEvents function assumes onUpdate always returns a Promise, but it
could be a synchronous handler which would crash when calling .then(). Wrap the
result of calling onUpdate(selectedRows) with Promise.resolve() to normalize
both synchronous and asynchronous returns into a Promise before chaining the
.then() and .catch() methods. This ensures the code handles both
Promise-returning and synchronous onUpdate handlers gracefully.
There was a problem hiding this comment.
Confirmed valid — also flagged independently in our review. A synchronous onUpdate handler will throw TypeError: Cannot read properties of undefined (reading 'then') immediately. One-line fix: Promise.resolve(onUpdate(selectedRows)).then(...).catch(...). @santipalenque please address.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| fullWidth | ||
| size="small" | ||
| onChange={onChange} | ||
| value={editRow[col.columnKey] || ""} |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not coerce valid falsy cell values to empty string in edit mode.
Using || "" drops 0 and false. That changes displayed value and can corrupt edited payloads.
Suggested fix
- value={editRow[col.columnKey] || ""}
+ value={editRow[col.columnKey] ?? ""}📝 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.
| value={editRow[col.columnKey] || ""} | |
| value={editRow[col.columnKey] ?? ""} |
🤖 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/BulkEditTable/components/Cell.js` at line 18, In the Cell
component's input value binding, the logical OR operator (`||`) is coercing
valid falsy values like 0 and false to empty strings, corrupting the edited
data. Replace the `||` operator with the nullish coalescing operator (`??`) in
the value assignment for editRow[col.columnKey] so that only null and undefined
values default to empty string, while preserving legitimate falsy values like 0
and false.
There was a problem hiding this comment.
Confirmed valid — this one was missed in our review. || "" coerces 0 and false to empty string in edit mode, so any field with a legitimate numeric value of 0 (count, priority, score) displays as empty in the TextField and submits as "" instead of 0. Change to ?? to default only on null/undefined. @santipalenque please address.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| const toggleRow = (row) => { | ||
| setSelectedRows((current) => | ||
| isSelected(row.id) | ||
| ? current.filter((r) => r.id !== row.id) | ||
| : [...current, row] | ||
| ); | ||
| }; | ||
|
|
||
| const isAllSelected = (rows) => | ||
| rows.length > 0 && rows.every((row) => isSelected(row.id)); | ||
|
|
||
| const toggleAll = (rows) => { | ||
| setSelectedRows(isAllSelected(rows) ? [] : rows); | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use updater-local state for selection checks to avoid stale toggles.
toggleRow/toggleAll compute selection from selectedRows captured in closure instead of the updater’s current value. With batched updates, this can produce incorrect select/deselect results.
Suggested fix
- const isSelected = (rowId) => selectedRows.some((row) => row.id === rowId);
+ const isSelected = (rowId) => selectedRows.some((row) => row.id === rowId);
const toggleRow = (row) => {
setSelectedRows((current) =>
- isSelected(row.id)
+ current.some((r) => r.id === row.id)
? current.filter((r) => r.id !== row.id)
: [...current, row]
);
};
- const isAllSelected = (rows) =>
- rows.length > 0 && rows.every((row) => isSelected(row.id));
+ const isAllSelected = (rows) =>
+ rows.length > 0 && rows.every((row) => isSelected(row.id));
const toggleAll = (rows) => {
- setSelectedRows(isAllSelected(rows) ? [] : rows);
+ setSelectedRows((current) => {
+ const allSelected =
+ rows.length > 0 &&
+ rows.every((row) => current.some((r) => r.id === row.id));
+ return allSelected ? [] : rows;
+ });
};🤖 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/BulkEditTable/hooks/useRowSelection.js` around lines 9 -
22, The selection check functions isSelected and isAllSelected are reading from
the captured selectedRows state in the closure, which can become stale during
batched updates. Refactor isSelected to accept the current selectedRows array as
a parameter instead of relying on the closure's selectedRows state. Then update
both toggleRow and toggleAll to pass the current parameter from their updater
functions (or the computed selection state) to these check functions, ensuring
selection comparisons always use the most current state rather than stale
closure values.
There was a problem hiding this comment.
Confirmed valid edge case. toggleAll calls isAllSelected(rows) outside the setSelectedRows updater, reading closure state that could be stale under batched updates. toggleRow has the same pattern inside the updater. Moving both checks to use the current parameter from the updater is the safe fix, as shown in the suggested patch. @santipalenque please address.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| const initialPerPage = React.useRef(perPage); | ||
|
|
||
| let perPageOptions = BASE_PER_PAGE_OPTIONS.includes(initialPerPage.current) | ||
| ? BASE_PER_PAGE_OPTIONS | ||
| : [...BASE_PER_PAGE_OPTIONS, initialPerPage.current].sort((a, b) => a - b); | ||
|
|
||
| if (!onPerPageChange) { | ||
| perPageOptions = [initialPerPage.current]; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for dynamic perPage changes in table component usage
# Search for patterns where perPage state is updated
rg -nP --type=js --type=jsx -C3 'setPerPage|perPage.*=' -g '!node_modules' -g '!dist' -g '!build'Repository: OpenStackweb/openstack-uicore-foundation
Length of output: 206
🏁 Script executed:
# Read the full CustomTablePagination.js file to understand the complete context
cat -n src/components/mui/table/CustomTablePagination.jsRepository: OpenStackweb/openstack-uicore-foundation
Length of output: 3489
🏁 Script executed:
# Search for perPage usage patterns with corrected file type
rg -n 'setPerPage|perPage' -g '!node_modules' -g '!dist' -g '!build' --type=jsRepository: OpenStackweb/openstack-uicore-foundation
Length of output: 178
🏁 Script executed:
# Find parent components that use CustomTablePagination
rg -n 'CustomTablePagination|import.*CustomTablePagination' -g '!node_modules' -g '!dist' -g '!build' --type=jsRepository: OpenStackweb/openstack-uicore-foundation
Length of output: 178
🏁 Script executed:
# Search for any imports or usage of CustomTablePagination across the entire repo
find . -name "*.js" -o -name "*.jsx" | xargs grep -l "CustomTablePagination" 2>/dev/null | grep -v node_modules | head -20Repository: OpenStackweb/openstack-uicore-foundation
Length of output: 484
🏁 Script executed:
# Also search for any parent state that manages perPage and might change it
rg "useState.*perPage|perPage.*useState" --type=js --multiline 2>/dev/null | head -30Repository: OpenStackweb/openstack-uicore-foundation
Length of output: 178
🏁 Script executed:
# Examine the main mui-table.js to see how perPage is passed to CustomTablePagination
cat -n src/components/mui/table/mui-table.js | head -100Repository: OpenStackweb/openstack-uicore-foundation
Length of output: 3988
🏁 Script executed:
# Check for perPage state management in parent components
rg -n "perPage" src/components/mui/table/mui-table.js -A 2 -B 2Repository: OpenStackweb/openstack-uicore-foundation
Length of output: 817
🏁 Script executed:
# Check BulkEditTable.js usage
rg -n "perPage\|CustomTablePagination" src/components/mui/BulkEditTable/BulkEditTable.js -A 3 -B 3Repository: OpenStackweb/openstack-uicore-foundation
Length of output: 178
Use useMemo instead of useRef to rebuild options when perPage changes.
The useRef captures the initial perPage value at mount and never updates. If the parent component changes the perPage prop after mount, the dropdown options array becomes stale while the displayed value (rowsPerPage={perPage} on line 69) stays current. This creates a mismatch where the options don't reflect the actual current value.
Replace the useRef with useMemo and include perPage in the dependency array to ensure perPageOptions updates whenever perPage changes:
Suggested fix
const perPageOptions = React.useMemo(() => {
let options = BASE_PER_PAGE_OPTIONS.includes(perPage)
? BASE_PER_PAGE_OPTIONS
: [...BASE_PER_PAGE_OPTIONS, perPage].sort((a, b) => a - b);
if (!onPerPageChange) {
options = [perPage];
}
return options;
}, [perPage, onPerPageChange]);🤖 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/table/CustomTablePagination.js` around lines 46 - 54, The
initialPerPage useRef captures the perPage value only at component mount and
never updates, causing the perPageOptions array to become stale when the parent
component changes the perPage prop after mount, creating a mismatch between the
displayed value and available options. Replace the useRef pattern with
React.useMemo for the perPageOptions variable, removing the initialPerPage
reference and using the perPage prop directly in the calculation, then include
both perPage and onPerPageChange in the useMemo dependency array to ensure the
options rebuild whenever either dependency changes.
There was a problem hiding this comment.
Confirmed valid. useRef captures perPage only at mount — if the parent changes it later (e.g. restoring from URL state or local storage), the options dropdown goes stale while rowsPerPage shows the correct value, creating a visible mismatch. This behavior was carried over from the previous inline implementation; the extraction into CustomTablePagination is a good time to fix it with useMemo([perPage, onPerPageChange]). @santipalenque please address.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| const handleRowsPerPageChange = (ev) => { | ||
| onPerPageChange(ev.target.value); | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Parse the select value to a number.
The ev.target.value from an HTML select element is always a string (e.g., "10", "20"), but the onPerPageChange callback likely expects a number to match the perPage prop type. This type inconsistency could cause subtle bugs in parent components.
🔢 Proposed fix to parse value as integer
const handleRowsPerPageChange = (ev) => {
- onPerPageChange(ev.target.value);
+ onPerPageChange(parseInt(ev.target.value, 10));
};📝 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 handleRowsPerPageChange = (ev) => { | |
| onPerPageChange(ev.target.value); | |
| }; | |
| const handleRowsPerPageChange = (ev) => { | |
| onPerPageChange(parseInt(ev.target.value, 10)); | |
| }; |
🤖 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/table/CustomTablePagination.js` around lines 60 - 62, In
the handleRowsPerPageChange function, the ev.target.value is being passed
directly to onPerPageChange, but select element values are always strings and
the callback expects a number. Parse the select value to an integer by wrapping
ev.target.value with parseInt() or Number() before passing it to the
onPerPageChange callback to ensure type consistency with the perPage prop and
prevent potential bugs in parent components.
There was a problem hiding this comment.
Confirmed valid. ev.target.value from an HTML select is always a string, so onPerPageChange receives "10" instead of 10, causing a type mismatch against the perPage: PropTypes.number contract. This was pre-existing behavior in all three table components before the refactor; the extraction into a shared component makes it the right moment to fix it with parseInt(ev.target.value, 10). @santipalenque please address.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Pull request overview
This PR continues the GridFilter feature set by adding new operators and input types, improves table pagination reuse by extracting a shared pagination component, and introduces a new BulkEditTable component (with tests) for multi-row editing workflows.
Changes:
- GridFilter: add
before/afteroperators, new value types (datetime,number, async selects), and asetFilters(filters, joinOperator?)hook API for loading saved filter state. - Tables: extract shared
CustomTablePaginationand refactor existing MUI tables to use it. - Add
BulkEditTablecomponent (selection + edit mode) with module exports and initial test coverage.
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| webpack.common.js | Adds new build entry points for BulkEditTable and CustomTablePagination. |
| src/i18n/en.json | Adds GridFilter operator labels and placeholder strings for new value types. |
| src/components/mui/table/mui-table.js | Switches pagination rendering to shared CustomTablePagination. |
| src/components/mui/table/CustomTablePagination.js | Introduces shared pagination wrapper used across tables. |
| src/components/mui/sortable-table/mui-table-sortable.js | Refactors to use CustomTablePagination. |
| src/components/mui/GridFilter/utils.js | Adds BEFORE/AFTER operators and ASYNC_VALUE_TYPES helper. |
| src/components/mui/GridFilter/readme.md | Documents setFilters and new value types/behavior. |
| src/components/mui/GridFilter/hooks/useGridFilter.jsx | Adds setFilters(filters, joinOperator?) API. |
| src/components/mui/GridFilter/GridFilter.jsx | Adds async-type customParser warning and supports new operators. |
| src/components/mui/GridFilter/components/ValueInput/SpeakerSelectInput.jsx | Adds speaker async-select value input. |
| src/components/mui/GridFilter/components/ValueInput/NumberInput.jsx | Adds numeric value input with min/max/integer handling. |
| src/components/mui/GridFilter/components/ValueInput/index.jsx | Extends ValueInput type map for datetime/number/async entity types. |
| src/components/mui/GridFilter/components/ValueInput/DateTimeInput.jsx | Adds datetime picker input storing unix timestamps (seconds). |
| src/components/mui/GridFilter/components/ValueInput/CompanySelectInput.jsx | Adds company async-select value input. |
| src/components/mui/GridFilter/components/ValueInput/AsyncSelectInput.jsx | Adds generic async-select input for remote lookups. |
| src/components/mui/GridFilter/components/Filter.jsx | Adjusts layout sizing for criteria/operator/value inputs. |
| src/components/mui/editable-table/mui-table-editable.js | Refactors to use CustomTablePagination. |
| src/components/mui/Dropdown/index.jsx | Makes options optional and updates defaultProps/propTypes. |
| src/components/mui/BulkEditTable/index.js | Adds BulkEditTable module entry export. |
| src/components/mui/BulkEditTable/hooks/useRowSelection.js | Adds selection/edit-state hook for BulkEditTable. |
| src/components/mui/BulkEditTable/components/Toolbar.js | Adds Bulk edit toolbar actions (edit/apply/cancel). |
| src/components/mui/BulkEditTable/components/Row.js | Adds row rendering with selection + per-row actions. |
| src/components/mui/BulkEditTable/components/Heading.js | Adds sortable header cell behavior (disabled during edit mode). |
| src/components/mui/BulkEditTable/components/Cell.js | Adds editable vs read-only cell rendering for bulk edit mode. |
| src/components/mui/BulkEditTable/BulkEditTable.module.less | Adds sticky checkbox/action column + bulk-edit column styling. |
| src/components/mui/BulkEditTable/BulkEditTable.js | Adds new BulkEditTable component with pagination integration. |
| src/components/mui/BulkEditTable/tests/BulkEditTable.test.js | Adds initial BulkEditTable interaction test. |
| src/components/mui/tests/useGridFilter.test.jsx | Adds tests for new setFilters hook API. |
| src/components/mui/tests/GridFilter.test.jsx | Adds tests for datetime/number/async select value types and async parser warnings. |
| src/components/index.js | Exports BulkEditTable and CustomTablePagination from the package entry point. |
| package.json | Bumps package version to 5.0.36-beta.2. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Dropdown.defaultProps = { | ||
| value: null, | ||
| label: "", | ||
| placeholder: "" | ||
| placeholder: "", | ||
| options: null | ||
| }; |
There was a problem hiding this comment.
Confirmed valid — real crash risk missed in both our review and CodeRabbit's pass. renderValue at lines 52 and 59 calls options.map() and options.find() without a null-guard. Line 65 uses options?.map() (safe), but the same protection is missing inside renderValue. If options is null and selected holds a non-empty value, the component throws TypeError: Cannot read properties of null (reading 'map') at runtime. Fix: default options to [] instead of null in defaultProps. @santipalenque please address.
| import React from "react"; | ||
| import userEvent from "@testing-library/user-event"; | ||
| import { act, render, screen, waitFor } from "@testing-library/react"; | ||
| import BulkEditTable from "../BulkEditTable"; | ||
|
|
There was a problem hiding this comment.
Confirmed valid. The test clicks by raw translation keys ("event_list.edit_selected", "bulk_actions_page.btn_apply_changes") without mocking i18n-react. This works today only because T.translate returns the key itself when no dictionary is loaded in the test environment. Every other MUI component test in this repo (e.g. GridFilter.test.jsx) mocks it explicitly: jest.mock('i18n-react/dist/i18n-react', () => ({ default: { translate: (key) => key } })). Without the mock, if translations are ever pre-loaded in test setup the test breaks. @santipalenque please add the mock.
| import React, { useEffect } from "react"; | ||
| import PropTypes from "prop-types"; | ||
| import Box from "@mui/material/Box"; |
There was a problem hiding this comment.
Confirmed valid — consistency issue. The new GridFilter value input files (AsyncSelectInput.jsx, DateTimeInput.jsx, NumberInput.jsx, etc.) all include the Apache 2.0 header, but none of the new BulkEditTable source files do. As a published library this matters for downstream licensing clarity. @santipalenque please add the standard header to all new BulkEditTable modules.
| <Button variant="contained" onClick={onApply}> | ||
| {T.translate("bulk_actions_page.btn_apply_changes")} | ||
| </Button> | ||
| <Button variant="outlined" onClick={onCancel}> | ||
| {T.translate("general.cancel")} | ||
| </Button> | ||
| </> | ||
| ) : ( | ||
| <Button variant="contained" onClick={onEdit} disabled={!hasSelection}> | ||
| {T.translate("event_list.edit_selected")} | ||
| </Button> |
There was a problem hiding this comment.
Confirmed valid. Toolbar uses T.translate("bulk_actions_page.btn_apply_changes") and T.translate("event_list.edit_selected"), but neither key exists in the library's src/i18n/en.json (nor es.json / zh.json). general.cancel does exist. Consumers who rely on the bundled dictionary will see raw keys instead of labels. Two options: (1) add the keys to the library's i18n files, or (2) accept the labels as props so the consumer can pass translated strings directly — the latter is preferable since these are domain-specific labels that different consuming apps may word differently. @santipalenque please address.
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
Post-adversarial review — 6 findings (2 dropped from original pass: no-failure-UI is correct library behavior, setFilters stale-parsed is explicitly documented). One new finding added: hidden onChange contract in Cell for custom editableField components.
| const [selectedRows, setSelectedRows] = useState([]); | ||
| const [editEnabled, setEditEnabled] = useState(false); | ||
|
|
||
| const isSelected = (rowId) => selectedRows.some((row) => row.id === rowId); |
There was a problem hiding this comment.
Hardcoded row.id — no idKey escape hatch
isSelected, toggleRow, editField, and isAllSelected all assume the row identifier is always id. BulkEditTable.js also derives dataIds with row.id and Row.js renders row.id as a hardcoded cell before the column map.
Consumers whose rows use eventId, pk, or any key other than id get permanently empty selections and misaligned columns with no error.
The existing MuiTableSortable in the same repo accepts idKey = "id" as a defaulted prop. Same pattern here would fix it:
const useRowSelection = (idKey = 'id') => {
const isSelected = (rowId) => selectedRows.some((row) => row[idKey] === rowId);
...
}|
|
||
| const handleUpdateEvents = (evt) => { | ||
| evt.stopPropagation(); | ||
| evt.preventDefault(); |
There was a problem hiding this comment.
onUpdate must return a Promise — not enforced, crashes if it doesn't
onUpdate(selectedRows).then(...).catch(...) calls .then() on whatever onUpdate returns. If the consumer passes a synchronous callback (e.g. during testing or a simple state update), this throws TypeError: Cannot read properties of undefined (reading 'then') immediately.
The PropTypes declaration (PropTypes.func.isRequired) gives no hint of the Promise requirement.
One-line fix that preserves the current behavior for async consumers and doesn't crash for sync ones:
Promise.resolve(onUpdate(selectedRows))
.then(() => reset())
.catch((error) => { console.error('Error updating events:', error); });| value: | ||
| editRow[col.columnKey]?.id ?? | ||
| editRow[col.columnKey]?.value ?? | ||
| editRow[col.columnKey], |
There was a problem hiding this comment.
Hidden onChange contract for custom editableField components
onChange here is Row's onRowChange:
const onRowChange = (ev) => {
const { value, id } = ev.target; // expects ev.target.id === col.columnKey
onFieldChange(id, value);
};The built-in TextField sets id={col.columnKey} so ev.target.id is correct. But custom editableField renderers that fire onChange({ target: { value: newVal } }) without setting id cause onFieldChange(undefined, newVal) — silently writing to key undefined in the row state.
This contract ("your synthetic event must carry id equal to the column key") is nowhere documented. A better API adapts the event in Cell before passing it down:
onChange: (ev) => onChange({ target: { value: ev.target.value, id: col.columnKey } })or simplify to onChange(value) and let Cell supply the key.
| // for the sync `select` type); this type fetches its own, so it's stripped | ||
| // out here rather than spread onto the Autocomplete below. | ||
| const { options: _staleOptions, ...autocompleteProps } = rest; | ||
|
|
There was a problem hiding this comment.
minSearchLength is bypassed for the initial mount fetch
The guard:
if (searchTerm && searchTerm.length < minSearchLength) { ... }uses searchTerm && — an empty string is falsy, so the condition is never entered when fetchOptions("") is called on mount. This means the component fires a backend query on every mount regardless of minSearchLength.
A consumer who sets minSearchLength: 3 to avoid expensive searches until the user has typed something still gets that search on open.
Fix:
if (minSearchLength > 0 && (!searchTerm || searchTerm.length < minSearchLength)) {
setOptions([]);
return;
}| }); | ||
| }; | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
In-flight queryFunction callbacks execute on unmounted component (React 17)
The cleanup only cancels the debounce timer:
return () => { if (debounceRef.current) clearTimeout(debounceRef.current); };If the user changes criterion while a query is already in-flight (timer fired, waiting on the callback), the unmounted component still receives setOptions/setLoading calls, producing React 17 "Can't perform a React state update on an unmounted component" warnings.
Standard fix using a mounted ref:
const mountedRef = useRef(true);
useEffect(() => {
fetchOptions("");
return () => {
mountedRef.current = false;
if (debounceRef.current) clearTimeout(debounceRef.current);
};
}, []);
// inside fetchOptions callback:
if (mountedRef.current) { setOptions(...); setLoading(false); }| /> | ||
| </Box> | ||
| <Box sx={{ flex: "1 1 auto", minWidth: 0 }}> | ||
| <ValueInput |
There was a problem hiding this comment.
Breaking: default placeholder removed from ValueInput for text and select types
Before this PR, Filter passed placeholder={T.translate("grid_filter.select_values")} to ValueInput before spreading valueSettings.props (so the consumer could override it). That prop is now gone.
For criteria using type: "text" or type: "select" that don't explicitly include placeholder in their values.props, the value input now shows an empty placeholder where it previously showed "select values". Silent UX regression for existing consumers.
Restoring the fallback before the spread preserves backward compatibility:
<ValueInput
id={`${id}-value`}
value={value?.value ?? defaultValue}
type={valueSettings.type}
disabled={!value?.criteria}
placeholder={T.translate("grid_filter.select_values")} {/* restore */}
{...valueSettings.props}
options={valueOptions}
onChange={handleChangeValue}
/>
| width={colWidth} | ||
| key={`heading_${col.columnKey}`} | ||
| > | ||
| {col.label} |
There was a problem hiding this comment.
@santipalenque BulkEditTable renders headers from col.label only. Existing table column configs in this package use header (and the new test uses value), so consumers reusing those configs will get blank headers. Please add a compatible fallback such as col.header ?? col.label ?? col.value.
| </Heading> | ||
| ); | ||
| })} | ||
| {options.actions && ( |
There was a problem hiding this comment.
@santipalenque This condition checks only options.actions, but Row renders the matching actions cell only when actions.edit or actions.delete exists. Passing actions: {} (which the new test does) creates an extra header cell with no body cell, so columns are misaligned. Please gate the header on the same predicate as the row, e.g. options.actions?.edit || options.actions?.delete, or always render a placeholder body cell when actions are present.
| const criteria = criterias.find(({ key }) => key === filter.criteria); | ||
| const parser = criteria?.customParser; | ||
|
|
||
| if (!parser && ASYNC_VALUE_TYPES.includes(criteria?.values?.type)) { |
There was a problem hiding this comment.
@santipalenque This currently logs when an async value type has no customParser, but it still falls through to default serialization and applies the filter. Since async values are option objects, the default path produces strings like created_by_company==[object Object], so the UI appears to apply a filter that the API cannot interpret. Please make this a hard guard: block or omit the filter, or serialize async option values explicitly, instead of only logging.
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
@santipalenque please review comments
https://app.clickup.com/t/86baf0qnm
Summary by CodeRabbit
Release Notes
New Features
before/afteroperators.datetime,number(with min/max and integer input handling), and async selects forspeakerandcompany.useGridFilternow providessetFilters(filters, joinOperator)to programmatically load filter state.Bug Fixes
Documentation
Tests