Fix: responsive grids - #311
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (38)
💤 Files with no reviewable changes (5)
📝 WalkthroughWalkthroughThe PR rebuilds MUI table components under ChangesMUI table restructuring
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MuiTable
participant TableShell
participant SortableHeaderContent
participant TableCellContent
participant RowActionsMenu
participant createDeleteHandler
MuiTable->>TableShell: render table content and pagination
MuiTable->>SortableHeaderContent: provide sort state and callback
MuiTable->>TableCellContent: render row cell values
MuiTable->>RowActionsMenu: provide responsive row actions
RowActionsMenu->>createDeleteHandler: invoke delete action
createDeleteHandler-->>MuiTable: confirm item deletion
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 12
🧹 Nitpick comments (1)
src/components/mui/tables/mui-table/index.js (1)
203-203: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCoerce
disabledto a boolean.
options.disableProp && row[options.disableProp]can evaluate tonull,undefined, a string, or a number.IconButtonexpects a boolean fordisabled.getRowActionsalready applies!!(...)for the same conditions, so the menu path and the icon path can disagree in typing. Apply!!at each icon button for consistency.♻️ Proposed change
- disabled={options.disableProp && row[options.disableProp]} + disabled={!!(options.disableProp && row[options.disableProp])}Also applies to: 234-238, 268-268, 293-293
🤖 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/tables/mui-table/index.js` at line 203, Update each IconButton disabled prop in the table row-action rendering, including the instances near the current expression and the additional highlighted locations, to coerce the disable condition with !!. Match the boolean coercion already used by getRowActions so every icon path passes a boolean to IconButton.
🤖 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/tables/components/create-delete-handler.js`:
- Around line 19-20: Update the dialog options in create-delete-handler to use
the showConfirmDialog API field iconType instead of type, preserving the
"warning" value so the delete confirmation displays its warning state.
- Around line 4-10: Update createDeleteHandler to accept a getId callback and
use it when invoking onDelete instead of always reading item.id. In
sortable-table, pass a callback that returns item[idKey], preserving the
configured row identifier for deletion while retaining existing behavior for the
default idKey.
In `@src/components/mui/tables/components/row-actions-menu.js`:
- Around line 28-34: Add a localized aria-label to the icon-only IconButton that
opens the row actions menu, using the component’s existing localization
mechanism and an existing translation key where available. Preserve the current
click behavior and data-testid.
In `@src/components/mui/tables/components/sortable-header-content.js`:
- Around line 15-17: Update the onClick handler in the sortable header content
to pass ascending order when col.columnKey differs from sortCol, and only negate
sortDir when the clicked column is already active. Preserve the existing column
key passed to onSort.
In `@src/components/mui/tables/components/styles.module.less`:
- Around line 7-12: Center the absolutely positioned border pseudo-element by
updating its positioning declarations: replace the top-zero alignment with top:
50% and apply a vertical translate matching half its 60% height. Remove the
ineffective align-self declaration while preserving the existing bottom, left,
border, and height styling.
In `@src/components/mui/tables/components/use-scroll-fade.js`:
- Around line 15-23: Update the React.useEffect around updateFades to observe
container and table dimension changes with a ResizeObserver, invoking
updateFades when either resizes. Observe the relevant container and table
elements, and disconnect the observer during cleanup alongside removing the
scroll listener.
In `@src/components/mui/tables/editable-table/index.js`:
- Around line 251-263: Update the editable-cell TableCell rendering around
isEditable, adding tabIndex only for editable cells and handling Enter or Space
keyboard activation by invoking handleCellClick with the row and column key.
Ignore keyboard events originating from the nested TextField, while preserving
non-editable cell behavior.
In `@src/components/mui/tables/mui-table/index.js`:
- Around line 115-119: Update the select action in the table component to use
the existing T.translate mechanism for the "View" label, including both the menu
entry and its tooltip. Add or reuse the appropriate translation key so both
visible labels follow the current locale while preserving the existing onSelect
behavior.
- Line 180: Update the className construction in the table cell rendering to
omit falsy or undefined values instead of interpolating them as strings.
Preserve both the conditional dotted-border class from col.dottedBorder and
col.className when present, while ensuring the rendered class list contains only
valid class names.
In `@src/components/mui/tables/sortable-table/index.js`:
- Around line 77-80: Use the configured idKey consistently for item identity in
the sortable table. Update the reorder logic around movedItemId and the
createDeleteHandler helper to derive IDs through a getId callback, then pass
(item) => item[idKey] when creating the delete handler so onReorder and onDelete
receive the configured item ID.
- Around line 182-217: Add localized aria-label values to the edit, delete, and
reorder IconButton elements in the table row actions, using the existing
localization mechanism and clear action-specific labels while preserving their
current click and drag behavior.
- Around line 66-75: Update the reorder logic around reordered and
updateOrderKey so assigning the new order does not mutate item objects supplied
through data. Create new item objects when applying updateOrderKey, while
preserving unchanged item references or values as appropriate, then pass the
resulting reordered collection to onReorder.
---
Nitpick comments:
In `@src/components/mui/tables/mui-table/index.js`:
- Line 203: Update each IconButton disabled prop in the table row-action
rendering, including the instances near the current expression and the
additional highlighted locations, to coerce the disable condition with !!. Match
the boolean coercion already used by getRowActions so every icon path passes a
boolean to IconButton.
🪄 Autofix
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 Plus
Run ID: e76a6e75-5930-4cee-8a49-64a1205c7f32
📒 Files selected for processing (38)
src/components/index.jssrc/components/mui/BulkEditTable/BulkEditTable.jssrc/components/mui/MuiBaseCustomTheme.jssrc/components/mui/SponsorOrderGrid/index.jssrc/components/mui/__tests__/fee-row.test.jssrc/components/mui/__tests__/mui-table-editable.test.jssrc/components/mui/__tests__/mui-table-sortable.test.jssrc/components/mui/__tests__/mui-table.test.jssrc/components/mui/__tests__/notes-row.test.jssrc/components/mui/__tests__/payment-row.test.jssrc/components/mui/__tests__/refund-row.test.jssrc/components/mui/__tests__/total-row.test.jssrc/components/mui/editable-table/mui-table-editable.jssrc/components/mui/editable-table/mui-table-editable.module.lesssrc/components/mui/sortable-table/mui-table-sortable.jssrc/components/mui/sortable-table/styles.module.lesssrc/components/mui/table/mui-table.jssrc/components/mui/tables/components/CustomTablePagination.jssrc/components/mui/tables/components/create-delete-handler.jssrc/components/mui/tables/components/row-actions-menu.jssrc/components/mui/tables/components/scroll-fade-overlay.jssrc/components/mui/tables/components/sortable-header-content.jssrc/components/mui/tables/components/styles.module.lesssrc/components/mui/tables/components/table-cell-content.jssrc/components/mui/tables/components/table-shell.jssrc/components/mui/tables/components/table-styles.jssrc/components/mui/tables/components/use-scroll-fade.jssrc/components/mui/tables/editable-table/index.jssrc/components/mui/tables/extra-rows/DiscountRow.jsxsrc/components/mui/tables/extra-rows/FeeRow.jsxsrc/components/mui/tables/extra-rows/NotesRow.jsxsrc/components/mui/tables/extra-rows/PaymentRow.jsxsrc/components/mui/tables/extra-rows/RefundRow.jsxsrc/components/mui/tables/extra-rows/TotalRow.jsxsrc/components/mui/tables/extra-rows/index.jssrc/components/mui/tables/mui-table/index.jssrc/components/mui/tables/sortable-table/index.jswebpack.common.js
💤 Files with no reviewable changes (5)
- src/components/mui/sortable-table/styles.module.less
- src/components/mui/editable-table/mui-table-editable.module.less
- src/components/mui/table/mui-table.js
- src/components/mui/sortable-table/mui-table-sortable.js
- src/components/mui/editable-table/mui-table-editable.js
| const createDeleteHandler = ({ | ||
| onDelete, | ||
| getName = (item) => item.name, | ||
| deleteDialogTitle, | ||
| deleteDialogBody, | ||
| deleteDialogConfirmText, | ||
| confirmButtonColor |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Use the configured row identifier for deletion.
Line 27 always passes item.id to onDelete.
src/components/mui/tables/sortable-table/index.js supports idKey. If a row uses a non-id key, deletion receives undefined.
Add a getId callback to this factory. Pass item[idKey] from the sortable table.
Proposed fix
const createDeleteHandler = ({
onDelete,
+ getId = (item) => item.id,
getName = (item) => item.name,
@@
if (isConfirmed) {
- onDelete(item.id);
+ onDelete(getId(item));
}
};Also applies to: 26-28
🤖 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/tables/components/create-delete-handler.js` around lines 4
- 10, Update createDeleteHandler to accept a getId callback and use it when
invoking onDelete instead of always reading item.id. In sortable-table, pass a
callback that returns item[idKey], preserving the configured row identifier for
deletion while retaining existing behavior for the default idKey.
| type: "warning", | ||
| showCancelButton: true, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Pass the warning icon with the dialog API field name.
showConfirmDialog reads iconType. It ignores type.
Replace type: "warning" with iconType: "warning" so the delete confirmation shows its warning state.
🤖 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/tables/components/create-delete-handler.js` around lines
19 - 20, Update the dialog options in create-delete-handler to use the
showConfirmDialog API field iconType instead of type, preserving the "warning"
value so the delete confirmation displays its warning state.
| <IconButton | ||
| size="small" | ||
| onClick={(e) => setAnchorEl(e.currentTarget)} | ||
| data-testid="row-actions-menu-trigger" | ||
| > | ||
| <MoreVertIcon fontSize="small" /> | ||
| </IconButton> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add an accessible name to the actions trigger.
The icon-only IconButton has no aria-label.
Add a localized aria-label that identifies the row actions menu.
🤖 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/tables/components/row-actions-menu.js` around lines 28 -
34, Add a localized aria-label to the icon-only IconButton that opens the row
actions menu, using the component’s existing localization mechanism and an
existing translation key where available. Preserve the current click behavior
and data-testid.
| active={sortCol === col.columnKey} | ||
| direction={sortCol === col.columnKey && sortDir === -1 ? "desc" : "asc"} | ||
| onClick={() => onSort(col.columnKey, sortDir * -1)} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Start a newly selected column in ascending order.
Line 17 toggles sortDir even when the user selects a different column.
If the active column is ascending, the next column starts descending. Use ascending order for a new column. Only toggle when sortCol === col.columnKey.
Proposed fix
- onClick={() => onSort(col.columnKey, sortDir * -1)}
+ onClick={() =>
+ onSort(
+ col.columnKey,
+ sortCol === col.columnKey && sortDir === 1 ? -1 : 1
+ )
+ }📝 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.
| active={sortCol === col.columnKey} | |
| direction={sortCol === col.columnKey && sortDir === -1 ? "desc" : "asc"} | |
| onClick={() => onSort(col.columnKey, sortDir * -1)} | |
| active={sortCol === col.columnKey} | |
| direction={sortCol === col.columnKey && sortDir === -1 ? "desc" : "asc"} | |
| onClick={() => | |
| onSort( | |
| col.columnKey, | |
| sortCol === col.columnKey && sortDir === 1 ? -1 : 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/tables/components/sortable-header-content.js` around lines
15 - 17, Update the onClick handler in the sortable header content to pass
ascending order when col.columnKey differs from sortCol, and only negate sortDir
when the clicked column is already active. Preserve the existing column key
passed to onSort.
| React.useEffect(() => { | ||
| const el = containerRef.current; | ||
| if (!el) return undefined; | ||
|
|
||
| updateFades(); | ||
| el.addEventListener("scroll", updateFades); | ||
|
|
||
| return () => el.removeEventListener("scroll", updateFades); | ||
| }, [updateFades]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Update fade state when layout dimensions change.
The hook updates fades only on mount and scroll.
If the viewport, container, or table width changes, showLeftFade and showRightFade can remain stale until the user scrolls.
Observe the container and table size changes, then call updateFades. Disconnect the observer during cleanup.
🤖 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/tables/components/use-scroll-fade.js` around lines 15 -
23, Update the React.useEffect around updateFades to observe container and table
dimension changes with a ResizeObserver, invoking updateFades when either
resizes. Observe the relevant container and table elements, and disconnect the
observer during cleanup alongside removing the scroll listener.
| onSelect && { | ||
| label: "View", | ||
| onClick: () => onSelect(row), | ||
| disabled: !!(options.disableProp && row[options.disableProp]) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Translate the "View" action label.
Every other action label uses T.translate. The select action uses the hardcoded string "View" in both the menu entry and the tooltip. Add a translation key so the label follows the locale.
🌐 Proposed fix
onSelect && {
- label: "View",
+ label: T.translate("general.view"),- <Tooltip title="View">
+ <Tooltip title={T.translate("general.view")}>Also applies to: 286-286
🤖 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/tables/mui-table/index.js` around lines 115 - 119, Update
the select action in the table component to use the existing T.translate
mechanism for the "View" label, including both the menu entry and its tooltip.
Add or reuse the appropriate translation key so both visible labels follow the
current locale while preserving the existing onSelect behavior.
| <TableCell | ||
| key={col.columnKey} | ||
| align={col.align ?? "left"} | ||
| className={`${col.dottedBorder && styles.dottedBorderLeft} ${col.className}`} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Remove falsy values from the generated class name.
If col.dottedBorder is falsy, the template literal inserts the string false. If col.className is undefined, it inserts undefined. The rendered class attribute then contains "false undefined". Build the class list from defined values only.
🔧 Proposed fix
- className={`${col.dottedBorder && styles.dottedBorderLeft} ${col.className}`}
+ className={[
+ col.dottedBorder && styles.dottedBorderLeft,
+ col.className
+ ]
+ .filter(Boolean)
+ .join(" ")}📝 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.
| className={`${col.dottedBorder && styles.dottedBorderLeft} ${col.className}`} | |
| className={[ | |
| col.dottedBorder && styles.dottedBorderLeft, | |
| col.className | |
| ] | |
| .filter(Boolean) | |
| .join(" ")} |
🤖 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/tables/mui-table/index.js` at line 180, Update the
className construction in the table cell rendering to omit falsy or undefined
values instead of interpolating them as strings. Preserve both the conditional
dotted-border class from col.dottedBorder and col.className when present, while
ensuring the rendered class list contains only valid class names.
| const reordered = [...data]; | ||
| const [movedItem] = reordered.splice(result.source.index, 1); | ||
| reordered.splice(result.destination.index, 0, movedItem); | ||
|
|
||
| // change value based on updateOrderKey | ||
| if (updateOrderKey) { | ||
| reordered.forEach((item, idx) => { | ||
| item[updateOrderKey] = idx + 1; | ||
| }); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not mutate items received through data.
reordered copies the array but retains each item reference. Lines 72-74 then mutate caller-owned items before onReorder accepts or persists the change. Create new item objects before assigning updateOrderKey.
Proposed fix
- const reordered = [...data];
+ const reordered = data.map((item) => ({ ...item }));📝 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 reordered = [...data]; | |
| const [movedItem] = reordered.splice(result.source.index, 1); | |
| reordered.splice(result.destination.index, 0, movedItem); | |
| // change value based on updateOrderKey | |
| if (updateOrderKey) { | |
| reordered.forEach((item, idx) => { | |
| item[updateOrderKey] = idx + 1; | |
| }); | |
| } | |
| const reordered = data.map((item) => ({ ...item })); | |
| const [movedItem] = reordered.splice(result.source.index, 1); | |
| reordered.splice(result.destination.index, 0, movedItem); | |
| // change value based on updateOrderKey | |
| if (updateOrderKey) { | |
| reordered.forEach((item, idx) => { | |
| item[updateOrderKey] = idx + 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/tables/sortable-table/index.js` around lines 66 - 75,
Update the reorder logic around reordered and updateOrderKey so assigning the
new order does not mutate item objects supplied through data. Create new item
objects when applying updateOrderKey, while preserving unchanged item references
or values as appropriate, then pass the resulting reordered collection to
onReorder.
| const movedItemId = movedItem.id; | ||
| const newOrder = reordered.find( | ||
| (item) => item[idKey || "id"] === movedItemId | ||
| )?.[updateOrderKey]; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use idKey for all item identity callbacks.
Line 77 always reads movedItem.id, while line 79 compares with idKey. If callers configure idKey, onReorder receives an incorrect item ID and usually an undefined newOrder.
Deletion has the same defect. createDeleteHandler calls onDelete(item.id). Add a getId callback to that helper, then pass (item) => item[idKey] from this table.
Also applies to: 85-91
🤖 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/tables/sortable-table/index.js` around lines 77 - 80, Use
the configured idKey consistently for item identity in the sortable table.
Update the reorder logic around movedItemId and the createDeleteHandler helper
to derive IDs through a getId callback, then pass (item) => item[idKey] when
creating the delete handler so onReorder and onDelete receive the configured
item ID.
| <IconButton | ||
| size="large" | ||
| onClick={() => onEdit(row)} | ||
| sx={{ padding: 0 }} | ||
| > | ||
| <EditIcon fontSize="large" /> | ||
| </IconButton> | ||
| </TableCell> | ||
| )} | ||
| {/* Delete column */} | ||
| {onDelete && ( | ||
| <TableCell | ||
| align="center" | ||
| sx={getActionCellSx(row)} | ||
| className={styles.dottedBorderLeft} | ||
| > | ||
| <IconButton | ||
| size="large" | ||
| onClick={() => handleDelete(row)} | ||
| sx={{ padding: 0 }} | ||
| > | ||
| <DeleteIcon fontSize="large" /> | ||
| </IconButton> | ||
| </TableCell> | ||
| )} | ||
| {/* Re order column */} | ||
| {onReorder && ( | ||
| <TableCell | ||
| align="center" | ||
| sx={getActionCellSx(row)} | ||
| className={styles.dottedBorderLeft} | ||
| {...provided.dragHandleProps} | ||
| > | ||
| <IconButton size="large"> | ||
| <UnfoldMoreIcon fontSize="large" /> | ||
| </IconButton> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add accessible names to the action buttons.
The edit, delete, and reorder buttons contain only icons. Screen reader users cannot identify their actions. Add localized aria-label values to all three IconButton elements.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 186-186: A list component should have a key to prevent re-rendering
Context:
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(list-component-needs-key)
[warning] 192-204: A list component should have a key to prevent re-rendering
Context:
<IconButton
size="large"
onClick={() => handleDelete(row)}
sx={{ padding: 0 }}
>
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(list-component-needs-key)
[warning] 197-203: A list component should have a key to prevent re-rendering
Context: <IconButton
size="large"
onClick={() => handleDelete(row)}
sx={{ padding: 0 }}
>
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(list-component-needs-key)
[warning] 202-202: A list component should have a key to prevent re-rendering
Context:
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(list-component-needs-key)
[warning] 208-217: A list component should have a key to prevent re-rendering
Context: <TableCell
align="center"
sx={getActionCellSx(row)}
className={styles.dottedBorderLeft}
{...provided.dragHandleProps}
>
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(list-component-needs-key)
[warning] 214-216: A list component should have a key to prevent re-rendering
Context:
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(list-component-needs-key)
[warning] 215-215: A list component should have a key to prevent re-rendering
Context:
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(list-component-needs-key)
🤖 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/tables/sortable-table/index.js` around lines 182 - 217,
Add localized aria-label values to the edit, delete, and reorder IconButton
elements in the table row actions, using the existing localization mechanism and
clear action-specific labels while preserving their current click and drag
behavior.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 12
🧹 Nitpick comments (1)
src/components/mui/tables/mui-table/index.js (1)
203-203: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCoerce
disabledto a boolean.
options.disableProp && row[options.disableProp]can evaluate tonull,undefined, a string, or a number.IconButtonexpects a boolean fordisabled.getRowActionsalready applies!!(...)for the same conditions, so the menu path and the icon path can disagree in typing. Apply!!at each icon button for consistency.♻️ Proposed change
- disabled={options.disableProp && row[options.disableProp]} + disabled={!!(options.disableProp && row[options.disableProp])}Also applies to: 234-238, 268-268, 293-293
🤖 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/tables/mui-table/index.js` at line 203, Update each IconButton disabled prop in the table row-action rendering, including the instances near the current expression and the additional highlighted locations, to coerce the disable condition with !!. Match the boolean coercion already used by getRowActions so every icon path passes a boolean to IconButton.
🤖 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/tables/components/create-delete-handler.js`:
- Around line 19-20: Update the dialog options in create-delete-handler to use
the showConfirmDialog API field iconType instead of type, preserving the
"warning" value so the delete confirmation displays its warning state.
- Around line 4-10: Update createDeleteHandler to accept a getId callback and
use it when invoking onDelete instead of always reading item.id. In
sortable-table, pass a callback that returns item[idKey], preserving the
configured row identifier for deletion while retaining existing behavior for the
default idKey.
In `@src/components/mui/tables/components/row-actions-menu.js`:
- Around line 28-34: Add a localized aria-label to the icon-only IconButton that
opens the row actions menu, using the component’s existing localization
mechanism and an existing translation key where available. Preserve the current
click behavior and data-testid.
In `@src/components/mui/tables/components/sortable-header-content.js`:
- Around line 15-17: Update the onClick handler in the sortable header content
to pass ascending order when col.columnKey differs from sortCol, and only negate
sortDir when the clicked column is already active. Preserve the existing column
key passed to onSort.
In `@src/components/mui/tables/components/styles.module.less`:
- Around line 7-12: Center the absolutely positioned border pseudo-element by
updating its positioning declarations: replace the top-zero alignment with top:
50% and apply a vertical translate matching half its 60% height. Remove the
ineffective align-self declaration while preserving the existing bottom, left,
border, and height styling.
In `@src/components/mui/tables/components/use-scroll-fade.js`:
- Around line 15-23: Update the React.useEffect around updateFades to observe
container and table dimension changes with a ResizeObserver, invoking
updateFades when either resizes. Observe the relevant container and table
elements, and disconnect the observer during cleanup alongside removing the
scroll listener.
In `@src/components/mui/tables/editable-table/index.js`:
- Around line 251-263: Update the editable-cell TableCell rendering around
isEditable, adding tabIndex only for editable cells and handling Enter or Space
keyboard activation by invoking handleCellClick with the row and column key.
Ignore keyboard events originating from the nested TextField, while preserving
non-editable cell behavior.
In `@src/components/mui/tables/mui-table/index.js`:
- Around line 115-119: Update the select action in the table component to use
the existing T.translate mechanism for the "View" label, including both the menu
entry and its tooltip. Add or reuse the appropriate translation key so both
visible labels follow the current locale while preserving the existing onSelect
behavior.
- Line 180: Update the className construction in the table cell rendering to
omit falsy or undefined values instead of interpolating them as strings.
Preserve both the conditional dotted-border class from col.dottedBorder and
col.className when present, while ensuring the rendered class list contains only
valid class names.
In `@src/components/mui/tables/sortable-table/index.js`:
- Around line 77-80: Use the configured idKey consistently for item identity in
the sortable table. Update the reorder logic around movedItemId and the
createDeleteHandler helper to derive IDs through a getId callback, then pass
(item) => item[idKey] when creating the delete handler so onReorder and onDelete
receive the configured item ID.
- Around line 182-217: Add localized aria-label values to the edit, delete, and
reorder IconButton elements in the table row actions, using the existing
localization mechanism and clear action-specific labels while preserving their
current click and drag behavior.
- Around line 66-75: Update the reorder logic around reordered and
updateOrderKey so assigning the new order does not mutate item objects supplied
through data. Create new item objects when applying updateOrderKey, while
preserving unchanged item references or values as appropriate, then pass the
resulting reordered collection to onReorder.
---
Nitpick comments:
In `@src/components/mui/tables/mui-table/index.js`:
- Line 203: Update each IconButton disabled prop in the table row-action
rendering, including the instances near the current expression and the
additional highlighted locations, to coerce the disable condition with !!. Match
the boolean coercion already used by getRowActions so every icon path passes a
boolean to IconButton.
🪄 Autofix
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 Plus
Run ID: e76a6e75-5930-4cee-8a49-64a1205c7f32
📒 Files selected for processing (38)
src/components/index.jssrc/components/mui/BulkEditTable/BulkEditTable.jssrc/components/mui/MuiBaseCustomTheme.jssrc/components/mui/SponsorOrderGrid/index.jssrc/components/mui/__tests__/fee-row.test.jssrc/components/mui/__tests__/mui-table-editable.test.jssrc/components/mui/__tests__/mui-table-sortable.test.jssrc/components/mui/__tests__/mui-table.test.jssrc/components/mui/__tests__/notes-row.test.jssrc/components/mui/__tests__/payment-row.test.jssrc/components/mui/__tests__/refund-row.test.jssrc/components/mui/__tests__/total-row.test.jssrc/components/mui/editable-table/mui-table-editable.jssrc/components/mui/editable-table/mui-table-editable.module.lesssrc/components/mui/sortable-table/mui-table-sortable.jssrc/components/mui/sortable-table/styles.module.lesssrc/components/mui/table/mui-table.jssrc/components/mui/tables/components/CustomTablePagination.jssrc/components/mui/tables/components/create-delete-handler.jssrc/components/mui/tables/components/row-actions-menu.jssrc/components/mui/tables/components/scroll-fade-overlay.jssrc/components/mui/tables/components/sortable-header-content.jssrc/components/mui/tables/components/styles.module.lesssrc/components/mui/tables/components/table-cell-content.jssrc/components/mui/tables/components/table-shell.jssrc/components/mui/tables/components/table-styles.jssrc/components/mui/tables/components/use-scroll-fade.jssrc/components/mui/tables/editable-table/index.jssrc/components/mui/tables/extra-rows/DiscountRow.jsxsrc/components/mui/tables/extra-rows/FeeRow.jsxsrc/components/mui/tables/extra-rows/NotesRow.jsxsrc/components/mui/tables/extra-rows/PaymentRow.jsxsrc/components/mui/tables/extra-rows/RefundRow.jsxsrc/components/mui/tables/extra-rows/TotalRow.jsxsrc/components/mui/tables/extra-rows/index.jssrc/components/mui/tables/mui-table/index.jssrc/components/mui/tables/sortable-table/index.jswebpack.common.js
💤 Files with no reviewable changes (5)
- src/components/mui/sortable-table/styles.module.less
- src/components/mui/editable-table/mui-table-editable.module.less
- src/components/mui/table/mui-table.js
- src/components/mui/sortable-table/mui-table-sortable.js
- src/components/mui/editable-table/mui-table-editable.js
🛑 Comments failed to post (1)
src/components/mui/tables/components/styles.module.less (1)
7-12: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Center the dotted border pseudo-element.
align-selfhas no effect on an absolutely positioned pseudo-element.The current border starts at the top. Use
top: 50%andtransformto center the 60% height border.Proposed fix
- top: 0; - bottom: 0; + top: 50%; left: 0; border-left: 1px dashed `#e0e0e0`; height: 60%; - align-self: center; + transform: translateY(-50%);📝 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.top: 50%; left: 0; border-left: 1px dashed `#e0e0e0`; height: 60%; transform: translateY(-50%);🤖 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/tables/components/styles.module.less` around lines 7 - 12, Center the absolutely positioned border pseudo-element by updating its positioning declarations: replace the top-zero alignment with top: 50% and apply a vertical translate matching half its 60% height. Remove the ineffective align-self declaration while preserving the existing bottom, left, border, and height styling.
1 participant
https://app.clickup.com/t/9014802374/86bb7tvh1
Summary by CodeRabbit