fix: match archive row color in form items table#814
Open
priscila-moneo wants to merge 1 commit intomasterfrom
Open
fix: match archive row color in form items table#814priscila-moneo wants to merge 1 commit intomasterfrom
priscila-moneo wants to merge 1 commit intomasterfrom
Conversation
📝 WalkthroughWalkthroughAdds per-cell editing to the editable table, centralizes archived-row cell styling via ARCHIVED_CELL_SX and helpers, introduces a disableProp option to mark rows as archived/disabled, updates pages to opt into disableProp, and extends tests with SX inspection and archived-style assertions. Changes
Sequence DiagramsequenceDiagram
actor User
participant TableCell
participant MuiTableEditable
participant onCellChange
User->>TableCell: Click cell to edit
TableCell->>MuiTableEditable: handleCellClick(rowId, colKey)
MuiTableEditable->>MuiTableEditable: set editingCell state
MuiTableEditable->>TableCell: render input (edit mode)
User->>TableCell: Change value and blur
TableCell->>MuiTableEditable: handleCellBlur(rowId, colKey, newValue)
MuiTableEditable->>MuiTableEditable: validate newValue
alt Validation passes
MuiTableEditable->>onCellChange: call with (rowId, colKey, newValue)
onCellChange->>MuiTableEditable: update data
MuiTableEditable->>TableCell: clear editing state, re-render cell
else Validation fails
MuiTableEditable->>TableCell: keep edit mode, show error
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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.
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/editable-table/mui-table-editable.js (1)
303-334:⚠️ Potential issue | 🟡 MinorMissing
widthin edit and delete cellsxprops.In
mui-table.js, the edit and delete cells pass{ width: 40 }togetCellSx(lines 198 and 235), but here they don't include width. This may cause inconsistent cell sizing between the two table components.🔧 Proposed fix
{onEdit && ( - <TableCell sx={getCellSx(row)}> + <TableCell sx={getCellSx(row, { width: 40 })}> <IconButton ... {onDelete && ( - <TableCell sx={getCellSx(row)}> + <TableCell sx={getCellSx(row, { width: 40 })}> <IconButton🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/mui/editable-table/mui-table-editable.js` around lines 303 - 334, The edit and delete action TableCell usages in mui-table-editable.js omit the width override passed elsewhere; update the TableCell that wraps the Edit IconButton (onEdit) and the TableCell that wraps the delete control (onDelete) to include the same width styling by passing a width value (e.g., { width: 40 }) into getCellSx — locate the TableCell elements around the onEdit and onDelete handlers and call getCellSx(row, { width: 40 }) or otherwise merge { width: 40 } into the sx prop so their cell sizing matches the mui-table counterparts.
🧹 Nitpick comments (2)
src/components/mui/table/mui-table.js (1)
31-87: Code duplication:ARCHIVED_CELL_SXand helpers exist in both table components.The
ARCHIVED_CELL_SXconstant andgetArchivedCellSx/getCellSxhelpers are duplicated insrc/components/mui/editable-table/mui-table-editable.js(lines 26-30 and 171-177). Consider extracting these into a shared utility module to ensure consistency and reduce maintenance burden if styling needs to change.♻️ Example extraction
Create a shared utility file:
// src/components/mui/table/archived-cell-utils.js export const ARCHIVED_CELL_SX = { backgroundColor: "background.light", color: "text.disabled" }; export const getArchivedCellSx = (row, disableProp) => disableProp && row[disableProp] ? ARCHIVED_CELL_SX : null; export const getCellSx = (row, disableProp, baseSx = {}) => ({ ...baseSx, ...(getArchivedCellSx(row, disableProp) || {}) });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/mui/table/mui-table.js` around lines 31 - 87, ARCHIVED_CELL_SX and the helpers getArchivedCellSx/getCellSx are duplicated; extract them into a shared utility module (e.g., archived-cell-utils) and update both MuiTable (symbols: ARCHIVED_CELL_SX, getArchivedCellSx, getCellSx) and the editable table (mui-table-editable.js where the same symbols appear) to import and use the shared functions; ensure the refactored helpers accept the disableProp (or row/disableProp) so existing calls in handle cell rendering still work and remove the duplicated definitions from both components.src/components/mui/__tests__/mui-table-editable.test.js (1)
329-348: Consider documenting the cell index mapping.The
archivedCellIndexes = [0, 1, 2, 3, 5]relies on implicit knowledge of the column order (3 data columns + edit + archive + delete). A brief comment explaining which cells these indexes correspond to would improve maintainability.📝 Suggested comment
const cells = within(aliceRow).getAllByTestId("mui-table-cell"); - const archivedCellIndexes = [0, 1, 2, 3, 5]; + // Indexes: 0=name, 1=role, 2=age (content cols), 3=edit, 4=archive (excluded), 5=delete + const archivedCellIndexes = [0, 1, 2, 3, 5]; archivedCellIndexes.forEach((index) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/mui/__tests__/mui-table-editable.test.js` around lines 329 - 348, Add a brief inline comment above the archivedCellIndexes array in the test "applies archived styles to content, edit and delete cells when disableProp matches" to document the mapping between indexes and columns (e.g., which indexes correspond to data columns, edit, archive, delete columns); update the archivedCellIndexes declaration near where getCellSx(cells[index]) is used so future readers can understand why [0,1,2,3,5] was chosen without inspecting the table column order set up by setup(...) and screen/within calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/components/mui/editable-table/mui-table-editable.js`:
- Around line 303-334: The edit and delete action TableCell usages in
mui-table-editable.js omit the width override passed elsewhere; update the
TableCell that wraps the Edit IconButton (onEdit) and the TableCell that wraps
the delete control (onDelete) to include the same width styling by passing a
width value (e.g., { width: 40 }) into getCellSx — locate the TableCell elements
around the onEdit and onDelete handlers and call getCellSx(row, { width: 40 })
or otherwise merge { width: 40 } into the sx prop so their cell sizing matches
the mui-table counterparts.
---
Nitpick comments:
In `@src/components/mui/__tests__/mui-table-editable.test.js`:
- Around line 329-348: Add a brief inline comment above the archivedCellIndexes
array in the test "applies archived styles to content, edit and delete cells
when disableProp matches" to document the mapping between indexes and columns
(e.g., which indexes correspond to data columns, edit, archive, delete columns);
update the archivedCellIndexes declaration near where getCellSx(cells[index]) is
used so future readers can understand why [0,1,2,3,5] was chosen without
inspecting the table column order set up by setup(...) and screen/within calls.
In `@src/components/mui/table/mui-table.js`:
- Around line 31-87: ARCHIVED_CELL_SX and the helpers
getArchivedCellSx/getCellSx are duplicated; extract them into a shared utility
module (e.g., archived-cell-utils) and update both MuiTable (symbols:
ARCHIVED_CELL_SX, getArchivedCellSx, getCellSx) and the editable table
(mui-table-editable.js where the same symbols appear) to import and use the
shared functions; ensure the refactored helpers accept the disableProp (or
row/disableProp) so existing calls in handle cell rendering still work and
remove the duplicated definitions from both components.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6b56a695-6b5d-4504-8744-7d15a4765370
📒 Files selected for processing (5)
src/components/mui/__tests__/mui-table-editable.test.jssrc/components/mui/editable-table/mui-table-editable.jssrc/components/mui/table/mui-table.jssrc/pages/sponsors/sponsor-form-item-list-page/index.jssrc/pages/sponsors/sponsor-forms-tab/components/manage-items/sponsor-forms-manage-items.js
martinquiroga-exo
requested changes
Mar 6, 2026
martinquiroga-exo
left a comment
martinquiroga-exo
left a comment
There was a problem hiding this comment.
@priscila-moneo please see comments. Thanks.
| {data.map((row, idx) => ( | ||
| // eslint-disable-next-line react/no-array-index-key | ||
| <TableRow key={`row-${idx}`} hover> | ||
| <TableRow key={`row-${idx}`}> |
There was a problem hiding this comment.
<TableRow key={row.id}>
This table clearly has row.id, so the index should not be used.
Use a stable identifier. Using array index as a key breaks reconciliation if rows reorder, update, or paginate.
|
|
||
| test("applies archived styles to content, edit and delete cells when disableProp matches", () => { | ||
| setup({ | ||
| options: { sortCol: "name", sortDir: "-1", disableProp: "is_archived" }, |
There was a problem hiding this comment.
sortDir should not be an string, it is expecting a number. This relies on implicit coercion.
options: { sortCol: "name", sortDir: -1, disableProp: "is_archived" },
priscila-moneo
force-pushed
the
fix/match-archive-row-color
branch
from
ef24cfa to
806909c
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/components/mui/table/mui-table.js (1)
31-34: Consider extracting shared styling utilities to a common module.The
ARCHIVED_CELL_SXconstant andgetArchivedCellSx/getCellSxhelpers are duplicated verbatim insrc/components/mui/editable-table/mui-table-editable.js(lines 26-29 and 171-177). Extracting these to a shared module (e.g.,src/components/mui/table/table-utils.js) would reduce maintenance burden and ensure consistent styling across both table components.Also applies to: 81-87
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/mui/table/mui-table.js` around lines 31 - 34, ARCHIVED_CELL_SX and the helpers getArchivedCellSx/getCellSx are duplicated across mui-table.js and mui-table-editable.js; extract them into a shared module (e.g., table-utils.js) and import them from both components: create a small module that exports ARCHIVED_CELL_SX, getArchivedCellSx, and getCellSx, update mui-table.js to import those symbols instead of defining them, and update mui-table-editable.js to remove the duplicated definitions and import the same symbols so both components reuse the single implementation.src/components/mui/__tests__/mui-table-editable.test.js (1)
342-347: Consider using semantic selectors instead of hardcoded cell indexes.The hardcoded indexes
[0, 1, 2, 3, 5]assume a specific cell order (3 data columns + edit + archive + delete). If columns are reordered or optional action columns change, this test could silently pass incorrectly or fail unexpectedly. Consider querying cells by role or data attribute to make the test more resilient.💡 Example approach
// Instead of hardcoded indexes, identify cells by context const nameCellSx = getCellSx(screen.getByText("Alice").closest("td")); const editCellSx = getCellSx(within(aliceRow).getByLabelText("general.edit").closest("td")); const deleteCellSx = getCellSx(within(aliceRow).getByLabelText("general.delete").closest("td")); [nameCellSx, editCellSx, deleteCellSx].forEach((sx) => { expect(sx.backgroundColor).toBe("background.light"); expect(sx.color).toBe("text.disabled"); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/mui/__tests__/mui-table-editable.test.js` around lines 342 - 347, The test currently relies on hardcoded indexes in archivedCellIndexes and getCellSx(cells[index]), which is brittle; update the test to locate cells semantically instead (e.g., use screen.getByText/closest('td') for data cells, within(row).getByLabelText('general.edit') or 'general.delete' for action cells) and pass those td elements into getCellSx instead of indexing into cells; keep the same assertions on sx.backgroundColor and sx.color but build the array of cell elements by querying content/aria-labels (referencing getCellSx, archivedCellIndexes, and cells to find and replace the indexing logic).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/components/mui/__tests__/mui-table-editable.test.js`:
- Around line 342-347: The test currently relies on hardcoded indexes in
archivedCellIndexes and getCellSx(cells[index]), which is brittle; update the
test to locate cells semantically instead (e.g., use
screen.getByText/closest('td') for data cells,
within(row).getByLabelText('general.edit') or 'general.delete' for action cells)
and pass those td elements into getCellSx instead of indexing into cells; keep
the same assertions on sx.backgroundColor and sx.color but build the array of
cell elements by querying content/aria-labels (referencing getCellSx,
archivedCellIndexes, and cells to find and replace the indexing logic).
In `@src/components/mui/table/mui-table.js`:
- Around line 31-34: ARCHIVED_CELL_SX and the helpers
getArchivedCellSx/getCellSx are duplicated across mui-table.js and
mui-table-editable.js; extract them into a shared module (e.g., table-utils.js)
and import them from both components: create a small module that exports
ARCHIVED_CELL_SX, getArchivedCellSx, and getCellSx, update mui-table.js to
import those symbols instead of defining them, and update mui-table-editable.js
to remove the duplicated definitions and import the same symbols so both
components reuse the single implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a3eb2e0d-1b15-48a1-a3b7-c6d2eadd47b1
📒 Files selected for processing (5)
src/components/mui/__tests__/mui-table-editable.test.jssrc/components/mui/editable-table/mui-table-editable.jssrc/components/mui/table/mui-table.jssrc/pages/sponsors/sponsor-form-item-list-page/index.jssrc/pages/sponsors/sponsor-forms-tab/components/manage-items/sponsor-forms-manage-items.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pages/sponsors/sponsor-form-item-list-page/index.js
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
2 participants
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.
ref: https://app.clickup.com/t/86b859xzw
Summary by CodeRabbit
New Features
Improvements