Feature/grid filter continued#269
Open
santipalenque wants to merge 10 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds new GridFilter value types, parsing and saved-filter APIs, introduces BulkEditTable with row selection and inline editing, replaces table pagination with a shared wrapper, and exposes the new modules through package entries and the components barrel. ChangesGridFilter new value types and filter state API
BulkEditTable row selection and inline editing
Shared table pagination
Exports and bundle wiring
Sequence Diagram(s)sequenceDiagram
participant HostApp
participant useGridFilter as useGridFilter(id)
participant ReduxStore
participant GridFilter
participant customParser as criteria.customParser
HostApp->>useGridFilter: setFilters(filters, joinOperator)
useGridFilter->>ReduxStore: saveFilters(id, filters, validJoinOperator)
GridFilter->>customParser: parse async option objects
GridFilter->>GridFilter: warn when async criteria has no customParser
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
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
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
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.
smarcet
reviewed
Jun 24, 2026
smarcet
left a comment
smarcet
left a comment
Collaborator
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.
smarcet
reviewed
Jun 24, 2026
smarcet
reviewed
Jun 24, 2026
smarcet
reviewed
Jun 24, 2026
smarcet
requested changes
Jun 24, 2026
smarcet
left a comment
smarcet
left a comment
Collaborator
There was a problem hiding this comment.
@santipalenque please review comments
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/Dropdown/index.jsx`:
- Line 99: Normalize the incoming options in Dropdown so it always works with an
array, not just the undefined case. The current default in Dropdown and the
fallback path from ValueInput can still pass null, which breaks renderValue when
it calls options.map and options.find. Update Dropdown’s props handling to
coerce null/undefined to an empty array and ensure the renderValue logic always
uses that normalized value.
🪄 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: 67cb62b5-e756-4a31-91b1-b59fdda49df3
📒 Files selected for processing (14)
src/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/Dropdown/index.jsxsrc/components/mui/GridFilter/components/ValueInput/AsyncSelectInput.jsxsrc/components/mui/GridFilter/hooks/useGridFilter.jsxsrc/components/mui/table/CustomTablePagination.jssrc/i18n/en.json
✅ Files skipped from review due to trivial changes (3)
- src/components/mui/BulkEditTable/index.js
- src/components/mui/BulkEditTable/BulkEditTable.module.less
- src/i18n/en.json
🚧 Files skipped from review as they are similar to previous changes (10)
- src/components/mui/BulkEditTable/components/Toolbar.js
- src/components/mui/GridFilter/hooks/useGridFilter.jsx
- src/components/mui/BulkEditTable/hooks/useRowSelection.js
- src/components/mui/BulkEditTable/tests/BulkEditTable.test.js
- src/components/mui/BulkEditTable/components/Row.js
- src/components/mui/BulkEditTable/BulkEditTable.js
- src/components/mui/GridFilter/components/ValueInput/AsyncSelectInput.jsx
- src/components/mui/BulkEditTable/components/Heading.js
- src/components/mui/table/CustomTablePagination.js
- src/components/mui/BulkEditTable/components/Cell.js
santipalenque
force-pushed
the
feature/grid-filter-continued
branch
from
85497fe to
63c61a1
Compare
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 `@src/components/mui/BulkEditTable/components/Row.js`:
- Around line 98-114: The Row component in BulkEditTable uses hardcoded “event”
text in the IconButton aria-labels, which makes the generic row UI announce the
wrong entity. Update the aria-labels in Row so they use neutral action text or
derive the entity name from props instead of assuming events, while keeping the
existing actions.edit and actions.delete click behavior unchanged.
In `@src/components/mui/NumberInput.jsx`:
- Around line 46-52: The NumberInput handler is clamping both `min` and `max` on
every keystroke, which rewrites intermediate user input and blocks valid values
from being typed. Update the input flow in the NumberInput change logic so
mid-typing does not eagerly enforce `min` (and avoid rewriting partial values),
while still preserving `max` behavior only when appropriate or deferring
clamping until blur/commit. Keep the existing parsing and `onChange` path in
place, but adjust the normalization inside the change handler so the user can
type through prefixes like `1` toward `15` without the value being forced early.
🪄 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: a75deb03-bac3-4f1b-8371-7c33de2b9332
📒 Files selected for processing (36)
package.jsonsrc/components/index.jssrc/components/mui/AsyncSelectInput.jsxsrc/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/CompanySelectInput.jsxsrc/components/mui/DateTimeInput.jsxsrc/components/mui/Dropdown/index.jsxsrc/components/mui/GridFilter/GridFilter.jsxsrc/components/mui/GridFilter/components/Filter.jsxsrc/components/mui/GridFilter/components/ValueInput/index.jsxsrc/components/mui/GridFilter/hooks/useGridFilter.jsxsrc/components/mui/GridFilter/readme.mdsrc/components/mui/GridFilter/utils.jssrc/components/mui/NumberInput.jsxsrc/components/mui/SpeakerSelectInput.jsxsrc/components/mui/TextInput.jsxsrc/components/mui/__tests__/GridFilter.test.jsxsrc/components/mui/__tests__/mui-formik-dropdown-radio.test.jssrc/components/mui/__tests__/useGridFilter.test.jsxsrc/components/mui/editable-table/mui-table-editable.jssrc/components/mui/formik-inputs/mui-formik-dropdown-checkbox.jssrc/components/mui/formik-inputs/mui-formik-dropdown-radio.jssrc/components/mui/formik-inputs/mui-formik-select-v2.jssrc/components/mui/sortable-table/mui-table-sortable.jssrc/components/mui/table/CustomTablePagination.jssrc/components/mui/table/mui-table.jssrc/i18n/en.jsonwebpack.common.js
✅ Files skipped from review due to trivial changes (5)
- src/components/mui/formik-inputs/mui-formik-dropdown-checkbox.js
- src/components/mui/BulkEditTable/index.js
- src/components/mui/BulkEditTable/BulkEditTable.module.less
- src/components/mui/BulkEditTable/components/Toolbar.js
- src/components/mui/GridFilter/readme.md
🚧 Files skipped from review as they are similar to previous changes (19)
- src/components/index.js
- webpack.common.js
- src/components/mui/BulkEditTable/hooks/useRowSelection.js
- src/components/mui/tests/useGridFilter.test.jsx
- src/components/mui/BulkEditTable/tests/BulkEditTable.test.js
- src/components/mui/table/CustomTablePagination.js
- src/components/mui/GridFilter/components/ValueInput/index.jsx
- src/components/mui/Dropdown/index.jsx
- src/components/mui/GridFilter/hooks/useGridFilter.jsx
- src/components/mui/BulkEditTable/components/Heading.js
- src/components/mui/GridFilter/components/Filter.jsx
- src/components/mui/table/mui-table.js
- src/components/mui/editable-table/mui-table-editable.js
- src/components/mui/BulkEditTable/BulkEditTable.js
- src/components/mui/GridFilter/utils.js
- src/components/mui/BulkEditTable/components/Cell.js
- src/components/mui/sortable-table/mui-table-sortable.js
- src/components/mui/tests/GridFilter.test.jsx
- src/i18n/en.json
Comment on lines
+98
to
+114
| {actions.edit && ( | ||
| <IconButton | ||
| size="medium" | ||
| onClick={() => actions.edit.onClick(row)} | ||
| sx={{ padding: 0 }} | ||
| aria-label={`Edit event ${row[idKey]}`} | ||
| > | ||
| <EditIcon fontSize="large" /> | ||
| </IconButton> | ||
| )} | ||
| {actions.delete && ( | ||
| <IconButton | ||
| size="medium" | ||
| onClick={() => actions.delete.onClick(row)} | ||
| sx={{ padding: 0 }} | ||
| aria-label={`Delete event ${row[idKey]}`} | ||
| > |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use generic action labels instead of hardcoded “event”.
These aria-labels announce “Edit event” / “Delete event”, but this row component is generic and can render non-event data. That gives screen readers incorrect context whenever BulkEditTable is reused elsewhere. Prefer row-neutral text here, or derive the entity name from props.
Suggested fix
{actions.edit && (
<IconButton
size="medium"
onClick={() => actions.edit.onClick(row)}
sx={{ padding: 0 }}
- aria-label={`Edit event ${row[idKey]}`}
+ aria-label={`Edit row ${row[idKey]}`}
>
<EditIcon fontSize="large" />
</IconButton>
)}
{actions.delete && (
<IconButton
size="medium"
onClick={() => actions.delete.onClick(row)}
sx={{ padding: 0 }}
- aria-label={`Delete event ${row[idKey]}`}
+ aria-label={`Delete row ${row[idKey]}`}
>
<DeleteIcon fontSize="large" />
</IconButton>
)}📝 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
| {actions.edit && ( | |
| <IconButton | |
| size="medium" | |
| onClick={() => actions.edit.onClick(row)} | |
| sx={{ padding: 0 }} | |
| aria-label={`Edit event ${row[idKey]}`} | |
| > | |
| <EditIcon fontSize="large" /> | |
| </IconButton> | |
| )} | |
| {actions.delete && ( | |
| <IconButton | |
| size="medium" | |
| onClick={() => actions.delete.onClick(row)} | |
| sx={{ padding: 0 }} | |
| aria-label={`Delete event ${row[idKey]}`} | |
| > | |
| {actions.edit && ( | |
| <IconButton | |
| size="medium" | |
| onClick={() => actions.edit.onClick(row)} | |
| sx={{ padding: 0 }} | |
| aria-label={`Edit row ${row[idKey]}`} | |
| > | |
| <EditIcon fontSize="large" /> | |
| </IconButton> | |
| )} | |
| {actions.delete && ( | |
| <IconButton | |
| size="medium" | |
| onClick={() => actions.delete.onClick(row)} | |
| sx={{ padding: 0 }} | |
| aria-label={`Delete row ${row[idKey]}`} | |
| > |
🤖 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/Row.js` around lines 98 - 114,
The Row component in BulkEditTable uses hardcoded “event” text in the IconButton
aria-labels, which makes the generic row UI announce the wrong entity. Update
the aria-labels in Row so they use neutral action text or derive the entity name
from props instead of assuming events, while keeping the existing actions.edit
and actions.delete click behavior unchanged.
Comment on lines
+46
to
+52
| let clamped = parsed; | ||
| if (min != null) clamped = Math.max(clamped, min); | ||
| if (max != null) clamped = Math.min(clamped, max); | ||
| // only force-normalize the DOM when clamping changed the typed value; | ||
| // otherwise leave it alone so e.g. a trailing "." isn't stripped mid-typing | ||
| if (clamped !== parsed) e.target.value = clamped; | ||
| onChange({ target: { value: clamped } }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Eager min clamping mid-typing blocks valid entries.
Clamping on every keystroke makes it impossible to type a value whose prefix is below min. With min=10, a user typing 15 has the intermediate 1 immediately rewritten to 10, after which they can no longer reach 15. The max side has the analogous problem for values longer than the bound. Consider clamping on blur (or only enforcing max mid-typing) so intermediate keystrokes aren't rewritten.
🤖 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/NumberInput.jsx` around lines 46 - 52, The NumberInput
handler is clamping both `min` and `max` on every keystroke, which rewrites
intermediate user input and blocks valid values from being typed. Update the
input flow in the NumberInput change logic so mid-typing does not eagerly
enforce `min` (and avoid rewriting partial values), while still preserving `max`
behavior only when appropriate or deferring clamping until blur/commit. Keep the
existing parsing and `onChange` path in place, but adjust the normalization
inside the change handler so the user can type through prefixes like `1` toward
`15` without the value being forced early.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://app.clickup.com/t/86baf0qnm
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores