Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "openstack-uicore-foundation",
"version": "5.0.34",
"version": "5.0.36-beta.2",
"description": "ui reactjs components for openstack marketing site",
"main": "lib/openstack-uicore-foundation.js",
"scripts": {
Expand Down
2 changes: 2 additions & 0 deletions src/components/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,8 @@ export {useSnackbarMessage} from './mui/SnackbarNotification/Context'
export {default as MuiInfiniteTable} from './mui/infinite-table'
export {default as MuiEditableTable} from './mui/editable-table/mui-table-editable'
export {default as MuiTable} from './mui/table/mui-table'
export {default as MuiCustomTablePagination} from './mui/table/CustomTablePagination'
export {default as MuiBulkEditTable} from './mui/BulkEditTable'
export {default as MuiSponsorOrderGrid} from './mui/SponsorOrderGrid'
export {TotalRow as MuiTotalRow, NotesRow as MuiNotesRow, FeeRow as MuiFeeRow, PaymentRow as MuiPaymentRow, RefundRow as MuiRefundRow, DiscountRow as MuiDiscountRow} from './mui/table/extra-rows'
export {default as MuiFormikAsyncSelect} from './mui/formik-inputs/mui-formik-async-select'
Expand Down
160 changes: 160 additions & 0 deletions src/components/mui/BulkEditTable/BulkEditTable.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,160 @@
import React, { useEffect } from "react";
import PropTypes from "prop-types";
import Box from "@mui/material/Box";
Comment on lines +1 to +3

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

import Paper from "@mui/material/Paper";
import Table from "@mui/material/Table";
import TableBody from "@mui/material/TableBody";
import TableCell from "@mui/material/TableCell";
import TableContainer from "@mui/material/TableContainer";
import TableHead from "@mui/material/TableHead";
import TableRow from "@mui/material/TableRow";
import Checkbox from "@mui/material/Checkbox";
import Toolbar from "./components/Toolbar";
import Heading from "./components/Heading";
import Row from "./components/Row";
import useRowSelection from "./hooks/useRowSelection";
import styles from "./BulkEditTable.module.less";
import CustomTablePagination from "../table/CustomTablePagination";

const BulkEditTable = ({ options, columns, data, onSort, onUpdate, totalRows, perPage, currentPage, onPageChange, onPerPageChange }) => {
const {
selectedRows,
isSelected,
toggleRow,
isAllSelected,
toggleAll,
editField,
editEnabled,
enterEditMode,
cancel,
reset
} = useRowSelection();

const dataIds = data.map((row) => row.id).join(",");

// reset selection/edit state whenever the set of rows shown changes
// (pagination, filtering, sorting, search, etc.)
useEffect(() => {
reset();
}, [dataIds]);

const getSortDir = (columnKey) =>
columnKey === options.sortCol ? options.sortDir : null;

const handleUpdateEvents = (evt) => {
evt.stopPropagation();
evt.preventDefault();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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); });

onUpdate(selectedRows)
.then(() => reset())
.catch((error) => {
console.error("Error updating events:", error);
});
};
Comment on lines +44 to +52

@coderabbitai coderabbitai Bot Jun 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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.

Suggested change
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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!


return (
<Box sx={{ width: "100%" }}>
<Toolbar
editEnabled={editEnabled}
hasSelection={selectedRows.length > 0}
onEdit={enterEditMode}
onApply={handleUpdateEvents}
onCancel={cancel}
/>
<Paper elevation={0} sx={{ width: "100%", mb: 2 }}>
<TableContainer
component={Paper}
className={styles.tableWrapper}
sx={{ borderRadius: 0, boxShadow: "none" }}
>
<Table>
<TableHead sx={{ backgroundColor: "#EAEDF4" }}>
<TableRow>
<TableCell
align="center"
className={styles.checkColumn}
sx={{ backgroundColor: "#EAEDF4" }}
>
<Checkbox
checked={isAllSelected(data)}
onChange={() => toggleAll(data)}
slotProps={{ input: { "aria-label": "select all" } }}
/>
</TableCell>
{columns.map((col, i) => {
const sortable = !!col.sortable;
const colWidth = col.width ?? "";

return (
<Heading
editEnabled={editEnabled}
onSort={onSort}
sortDir={getSortDir(col.columnKey)}
sortable={sortable}
columnIndex={i}
columnKey={col.columnKey}
width={colWidth}
key={`heading_${col.columnKey}`}
>
{col.label}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 && (

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

<TableCell
align="center"
className={styles.actionColumn}
sx={{ backgroundColor: "#EAEDF4" }}
>
{options.actionsHeader || " "}
</TableCell>
)}
</TableRow>
</TableHead>
<TableBody>
{columns.length > 0 &&
data.map((row) => (
<Row
key={`row_${row.id}`}
row={row}
editEnabled={editEnabled}
isSelected={isSelected(row.id)}
editRow={selectedRows.find((r) => r.id === row.id) || row}
onToggle={() => toggleRow(row)}
onFieldChange={(key, value) =>
editField(row.id, key, value)
}
columns={columns}
actions={options.actions}
/>
))}
</TableBody>
</Table>
</TableContainer>
{perPage && currentPage && onPageChange && (
<CustomTablePagination
totalRows={totalRows}
perPage={perPage}
currentPage={currentPage}
onPageChange={onPageChange}
onPerPageChange={onPerPageChange}
/>
)}
</Paper>
</Box>
);
};

BulkEditTable.propTypes = {
options: PropTypes.object.isRequired,
columns: PropTypes.array.isRequired,
data: PropTypes.array.isRequired,
onSort: PropTypes.func.isRequired,
onUpdate: PropTypes.func.isRequired,
totalRows: PropTypes.number,
perPage: PropTypes.number,
currentPage: PropTypes.number,
onPageChange: PropTypes.func,
onPerPageChange: PropTypes.func
};

export default BulkEditTable;
58 changes: 58 additions & 0 deletions src/components/mui/BulkEditTable/BulkEditTable.module.less
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
.tableWrapper {
width: 100%;
overflow-x: auto;
position: relative;

td {
max-width: 150px;
text-overflow: ellipsis;
overflow-wrap: break-word;
vertical-align: middle;

&.dataColumn {
min-width: 150px;
}
}

// shared by header (th) and body (td) cells so the checkbox/action columns
// stay pinned and aligned in both rows while the data columns scroll
// horizontally. Background color is intentionally NOT set here: header
// cells need the header's grey, body cells need white, set via sx where
// each is rendered (an explicit color is required, `inherit` resolves to
// transparent here and lets the scrolling columns show through).
.checkColumn {
text-align: center;
position: sticky;
z-index: 5;
left: 0;
}

.actionColumn {
text-align: center;
position: sticky;
z-index: 5;
right: 0;
width: 60px;
min-width: 60px;
max-width: 60px;
}

.bulkEditCol {
min-width: 250px;
}
}

.dottedBorderLeft {
position: relative;
border-left: none;
&::before {
content: "";
position: absolute;
top: 0;
bottom: 0;
left: 0;
border-left: 1px dashed #e0e0e0;
height: 60%;
align-self: center;
}
}
48 changes: 48 additions & 0 deletions src/components/mui/BulkEditTable/__tests__/BulkEditTable.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
import React from "react";
import userEvent from "@testing-library/user-event";
import { act, render, screen, waitFor } from "@testing-library/react";
import BulkEditTable from "../BulkEditTable";

Comment on lines +1 to +5

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

describe("BulkEditTable", () => {
const baseProps = {
options: {
className: "test-table",
actions: {}
},
columns: [
{ columnKey: "id", value: "id", sortable: true },
{ columnKey: "title", value: "title", sortable: true }
],
onSort: jest.fn(),
data: [
{ id: 1, title: "Event 1" },
{ id: 2, title: "Event 2" }
]
};

beforeEach(() => {
jest.clearAllMocks();
});

test("applies bulk updates to selected rows", async () => {
const user = userEvent.setup();
const onUpdate = jest.fn(() => Promise.resolve());

render(<BulkEditTable {...baseProps} onUpdate={onUpdate} />);

const checkboxes = screen.getAllByRole("checkbox");

await user.click(checkboxes[1]);
await user.click(screen.getByText("event_list.edit_selected"));
await act(async () => {
await user.click(screen.getByText("bulk_actions_page.btn_apply_changes"));
});

await waitFor(() => {
expect(onUpdate).toHaveBeenCalledTimes(1);
expect(onUpdate).toHaveBeenCalledWith(
expect.arrayContaining([expect.objectContaining({ id: 1 })])
);
});
});
});
56 changes: 56 additions & 0 deletions src/components/mui/BulkEditTable/components/Cell.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
import React from "react";
import PropTypes from "prop-types";
import TextField from "@mui/material/TextField";

const DEFAULT_PLACEHOLDER = "Enter text...";
Comment thread
smarcet marked this conversation as resolved.

const Cell = ({ col, row, editRow, isEditingRow, onChange }) => {
if (isEditingRow && col.editableField === true) {
return (
<TextField
id={col.columnKey}
placeholder={col.placeholder || DEFAULT_PLACEHOLDER}
multiline
minRows={2}
fullWidth
size="small"
onChange={onChange}
value={editRow[col.columnKey] || ""}

@coderabbitai coderabbitai Bot Jun 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ 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.

Suggested change
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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

/>
);
}

if (isEditingRow && col.editableField) {
// editableField functions may short-circuit (e.g. `cond && <Input />`) and
// return `undefined` rather than `false`, which React rejects as a component return value.
return (
col.editableField({
value:
editRow[col.columnKey]?.id ??
editRow[col.columnKey]?.value ??
editRow[col.columnKey],

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

onChange,
row: editRow,
rowData: editRow[col.columnKey]
}) ?? null
);
}

if (col.render) {
return col.render(row[col.columnKey], row) ?? null;
}

return (
<span style={{ fontWeight: "normal" }}>{row[col.columnKey] ?? null}</span>
);
};

Cell.propTypes = {
col: PropTypes.object.isRequired,
row: PropTypes.object.isRequired,
editRow: PropTypes.object.isRequired,
isEditingRow: PropTypes.bool,
onChange: PropTypes.func
};

export default Cell;
Loading
Loading