Open
Conversation
📝 WalkthroughWalkthroughRefactors SummitDirectoryPage from a React class to a functional component, replaces SweetAlert2/react-bootstrap table with openstack-uicore-foundation's MuiTable, adds defensive error handling and navigation updates, and extends i18n Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
priscila-moneo
requested review from
martinquiroga-exo,
santipalenque and
smarcet
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/pages/summits/summit-directory-page.js (3)
44-47: Missing dependencies inuseEffect.The dependency array is empty while the effect reads
clearCurrentSummitandloadSummitsfrom props. In practice these are stableconnect-bound action creators so the effect will behave correctly, but this will triggerreact-hooks/exhaustive-deps. Consider either listing them explicitly or adding an eslint disable comment with a brief justification.♻️ Suggested tweak
- useEffect(() => { - clearCurrentSummit(); - loadSummits(); - }, []); + useEffect(() => { + clearCurrentSummit(); + loadSummits(); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/summits/summit-directory-page.js` around lines 44 - 47, The useEffect currently calls clearCurrentSummit and loadSummits but has an empty dependency array (in component SummitDirectoryPage), which triggers react-hooks/exhaustive-deps; update the effect to either include clearCurrentSummit and loadSummits in the dependency array (useEffect(() => { clearCurrentSummit(); loadSummits(); }, [clearCurrentSummit, loadSummits])) or, if you confirm these are stable connect-bound action creators, add an eslint-disable-next-line comment above the useEffect with a short justification (e.g., "action creators from connect are stable") so the linter is satisfied and intent is documented.
204-213: Prop shadowing of imported action creators — worth a sanity check.The destructured props
loadSummits,clearCurrentSummit, anddeleteSummiton lines 34–36 shadow the module-level imports used inconnect'smapDispatchToPropsobject on lines 209–213. This is correct (the shorthand object-form ofmapDispatchToPropsdispatches the actions and exposes them as props with the same names), so the prop versions are the dispatch-bound ones. Just flagging the shadowing since it can be confusing for future readers; renaming either the imports or the props would make the wiring more obvious.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/summits/summit-directory-page.js` around lines 204 - 213, The component props loadSummits, clearCurrentSummit, and deleteSummit shadow the same-named imported action creators used in the connect(...) shorthand, which is confusing; fix by renaming either the imports or the prop variables to make the dispatch-bound props explicit (e.g., rename imported action creators to loadSummitsAction/clearCurrentSummitAction/deleteSummitAction and keep props as loadSummits/clearCurrentSummit/deleteSummit, or keep imports and alias the props when destructuring such as loadSummits: loadSummitsDispatch), updating references in SummitDirectoryPage and the connect call accordingly (symbols: loadSummits, clearCurrentSummit, deleteSummit, connect, mapStateToProps, SummitDirectoryPage).
72-201: Use an Error Boundary instead of try/catch for rendering errors.The try/catch wrapping this JSX only catches synchronous errors in the immediate render expression, not errors thrown inside child components like
<MuiTable>or its columnrenderfunctions. Those child errors occur during React's reconciliation phase and bypass the parent's try/catch entirely. Error boundaries (via a class component or thereact-error-boundarylibrary) are the idiomatic pattern for displaying thedirectory.error_loadingfallback and will reliably catch errors fromMuiTableand its descendants.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/summits/summit-directory-page.js` around lines 72 - 201, The try/catch around the JSX won't catch errors thrown inside child components like MuiTable or its column renderers; remove the surrounding try/catch and instead wrap the rendering (at least the <MuiTable> and related UI) in a proper Error Boundary (either a class component implementing componentDidCatch/getDerivedStateFromError or use react-error-boundary's <ErrorBoundary>) that renders the same fallback UI (T.translate("directory.error_loading") and the error message) on error; keep existing handlers (handlePageChange, handleNewSummit, handleEditSummit, handleSelectSummit) and the columns definition intact and replace the catch fallback with the ErrorBoundary's fallbackRender or fallbackComponent so child errors are reliably caught.
🤖 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/pages/summits/summit-directory-page.js`:
- Around line 44-47: The useEffect currently calls clearCurrentSummit and
loadSummits but has an empty dependency array (in component
SummitDirectoryPage), which triggers react-hooks/exhaustive-deps; update the
effect to either include clearCurrentSummit and loadSummits in the dependency
array (useEffect(() => { clearCurrentSummit(); loadSummits(); },
[clearCurrentSummit, loadSummits])) or, if you confirm these are stable
connect-bound action creators, add an eslint-disable-next-line comment above the
useEffect with a short justification (e.g., "action creators from connect are
stable") so the linter is satisfied and intent is documented.
- Around line 204-213: The component props loadSummits, clearCurrentSummit, and
deleteSummit shadow the same-named imported action creators used in the
connect(...) shorthand, which is confusing; fix by renaming either the imports
or the prop variables to make the dispatch-bound props explicit (e.g., rename
imported action creators to
loadSummitsAction/clearCurrentSummitAction/deleteSummitAction and keep props as
loadSummits/clearCurrentSummit/deleteSummit, or keep imports and alias the props
when destructuring such as loadSummits: loadSummitsDispatch), updating
references in SummitDirectoryPage and the connect call accordingly (symbols:
loadSummits, clearCurrentSummit, deleteSummit, connect, mapStateToProps,
SummitDirectoryPage).
- Around line 72-201: The try/catch around the JSX won't catch errors thrown
inside child components like MuiTable or its column renderers; remove the
surrounding try/catch and instead wrap the rendering (at least the <MuiTable>
and related UI) in a proper Error Boundary (either a class component
implementing componentDidCatch/getDerivedStateFromError or use
react-error-boundary's <ErrorBoundary>) that renders the same fallback UI
(T.translate("directory.error_loading") and the error message) on error; keep
existing handlers (handlePageChange, handleNewSummit, handleEditSummit,
handleSelectSummit) and the columns definition intact and replace the catch
fallback with the ErrorBoundary's fallbackRender or fallbackComponent so child
errors are reliably caught.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 21a65486-d602-4f8e-8b8f-d2654cc934a5
📒 Files selected for processing (2)
src/i18n/en.jsonsrc/pages/summits/summit-directory-page.js
priscila-moneo
force-pushed
the
feature/move-show-directory-to-mui
branch
from
0e593b0 to
06890a3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/pages/summits/summit-directory-page.js (3)
44-47: Potentially missing dependencies inuseEffect.The effect depends on
clearCurrentSummitandloadSummitsbut the dependency array is empty. While these come frommapDispatchToPropsand are stable across renders (so the current behavior is correct), this will triggerreact-hooks/exhaustive-depswarnings and is fragile if the bindings ever change. Either include them in the dependency array or suppress the rule with a comment explaining why.♻️ Suggested change
useEffect(() => { clearCurrentSummit(); loadSummits(); + // eslint-disable-next-line react-hooks/exhaustive-deps }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/summits/summit-directory-page.js` around lines 44 - 47, The useEffect that calls clearCurrentSummit and loadSummits has an empty dependency array which will trigger react-hooks/exhaustive-deps warnings and is fragile if those bindings change; update the effect by either adding clearCurrentSummit and loadSummits to the dependency array (useEffect(() => { clearCurrentSummit(); loadSummits(); }, [clearCurrentSummit, loadSummits])) or, if these props are known to be stable (e.g., from mapDispatchToProps), keep the empty array but add a clear explanatory ESLint suppression comment (// eslint-disable-next-line react-hooks/exhaustive-deps — clearCurrentSummit and loadSummits are stable) immediately above the useEffect to silence the rule.
72-201:try/catcharound JSX won't catch child render errors—use anErrorBoundaryinstead.This
try/catchonly captures synchronous errors thrown directly in this function (e.g., accessing undefined props while buildingcolumns). Errors thrown during render of child components likeMuiTableare not caught here and require a ReactErrorBoundary(class component withcomponentDidCatch/getDerivedStateFromError). Event handler errors also escape this wrapper.If the goal is to handle data normalization issues, the safeguards at lines 39–43 are already sufficient; if the goal is to catch render errors, use an
ErrorBoundaryinstead.Also avoid exposing
err.messagedirectly to users; log it and show a generic message instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/summits/summit-directory-page.js` around lines 72 - 201, The current try/catch wrapping the JSX (around the render that returns the Grid2/MuiTable) won't catch render-time or child component errors (e.g., inside MuiTable) — replace this pattern by removing the try/catch and wrapping the parts that can throw (at least the <MuiTable> and its surrounding UI) with a React ErrorBoundary (class component implementing getDerivedStateFromError/componentDidCatch) and use that ErrorBoundary in place of the try/catch; in the ErrorBoundary component log the actual error (console or a logger) and render a generic user-facing message instead of directly exposing err.message; keep existing data normalization safeguards (safeSummits) and ensure event handlers (handleNewSummit, handleEditSummit, handleSelectSummit, deleteSummit) have their own try/catch where appropriate.
173-173: Simplify theonDeletecallback by removing the unnecessary wrapper lambda.The
MuiTablecomponent passes the rowiddirectly toonDelete. The wrapper(id) => deleteSummit(id)is redundant;deleteSummitcan be passed directly, matching the pattern used elsewhere in the codebase (e.g.,sponsor-list-page.js).♻️ Simplification
- onDelete={canDeleteSummits ? (id) => deleteSummit(id) : undefined} + onDelete={canDeleteSummits ? deleteSummit : undefined}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/summits/summit-directory-page.js` at line 173, The onDelete prop is passed a redundant wrapper lambda in the MuiTable usage; replace onDelete={canDeleteSummits ? (id) => deleteSummit(id) : undefined} with a direct reference so it becomes onDelete={canDeleteSummits ? deleteSummit : undefined}, keeping the conditional via canDeleteSummits and using the existing deleteSummit function (this mirrors the pattern used elsewhere like sponsor-list-page and avoids the unnecessary (id) => deleteSummit(id) wrapper).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pages/summits/summit-directory-page.js`:
- Around line 89-175: Columns are marked sortable but sorting is only in-memory
for the current page; either remove sortable: true from the column definitions
or implement server-side sorting by extending loadSummits(order) in
src/actions/summit-actions.js to accept an order string (e.g. "-start_date") and
applying it to the API call, then wire an onSort callback on the component (pass
onSort to MuiTable) that receives (sortCol, sortDir) and calls
loadSummits(currentPage, perPage, computedOrder) and updates options
(sortCol/sortDir) so header clicks trigger a server fetch instead of client-only
reordering; update any initial options to reflect the chosen server-side order.
---
Nitpick comments:
In `@src/pages/summits/summit-directory-page.js`:
- Around line 44-47: The useEffect that calls clearCurrentSummit and loadSummits
has an empty dependency array which will trigger react-hooks/exhaustive-deps
warnings and is fragile if those bindings change; update the effect by either
adding clearCurrentSummit and loadSummits to the dependency array (useEffect(()
=> { clearCurrentSummit(); loadSummits(); }, [clearCurrentSummit, loadSummits]))
or, if these props are known to be stable (e.g., from mapDispatchToProps), keep
the empty array but add a clear explanatory ESLint suppression comment (//
eslint-disable-next-line react-hooks/exhaustive-deps — clearCurrentSummit and
loadSummits are stable) immediately above the useEffect to silence the rule.
- Around line 72-201: The current try/catch wrapping the JSX (around the render
that returns the Grid2/MuiTable) won't catch render-time or child component
errors (e.g., inside MuiTable) — replace this pattern by removing the try/catch
and wrapping the parts that can throw (at least the <MuiTable> and its
surrounding UI) with a React ErrorBoundary (class component implementing
getDerivedStateFromError/componentDidCatch) and use that ErrorBoundary in place
of the try/catch; in the ErrorBoundary component log the actual error (console
or a logger) and render a generic user-facing message instead of directly
exposing err.message; keep existing data normalization safeguards (safeSummits)
and ensure event handlers (handleNewSummit, handleEditSummit,
handleSelectSummit, deleteSummit) have their own try/catch where appropriate.
- Line 173: The onDelete prop is passed a redundant wrapper lambda in the
MuiTable usage; replace onDelete={canDeleteSummits ? (id) => deleteSummit(id) :
undefined} with a direct reference so it becomes onDelete={canDeleteSummits ?
deleteSummit : undefined}, keeping the conditional via canDeleteSummits and
using the existing deleteSummit function (this mirrors the pattern used
elsewhere like sponsor-list-page and avoids the unnecessary (id) =>
deleteSummit(id) wrapper).
🪄 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: 541965e4-7908-4851-8879-2d6c50c08026
📒 Files selected for processing (2)
src/i18n/en.jsonsrc/pages/summits/summit-directory-page.js
✅ Files skipped from review due to trivial changes (1)
- src/i18n/en.json
Comment on lines
+89
to
+175
| const columns = [ | ||
| { | ||
| columnKey: "id", | ||
| header: T.translate("directory.id"), | ||
| width: 80, | ||
| sortable: true | ||
| }, | ||
| { | ||
| columnKey: "name", | ||
| header: T.translate("directory.summit_name"), | ||
| sortable: true | ||
| }, | ||
| { | ||
| columnKey: "start_date", | ||
| header: T.translate("directory.start_date"), | ||
| sortable: true, | ||
| render: (row) => formatEpoch(row.start_date, "MMMM Do YYYY") | ||
| }, | ||
| { | ||
| columnKey: "end_date", | ||
| header: T.translate("directory.end_date"), | ||
| sortable: true, | ||
| render: (row) => formatEpoch(row.end_date, "MMMM Do YYYY") | ||
| }, | ||
| { | ||
| columnKey: "invite_only_registration", | ||
| header: T.translate("directory.invitation_only"), | ||
| width: 120, | ||
| render: (row) => | ||
| row.invite_only_registration ? ( | ||
| <span style={{ color: "#b26a00", fontWeight: 500 }}> | ||
| {T.translate("directory.invitation_only")} | ||
| </span> | ||
| ) : null | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| render() { | ||
| const { summits, member, lastPage, currentPage, totalSummits } = this.props; | ||
| const memberObj = new Member(member); | ||
|
|
||
| const canEditSummit = memberObj.canEditSummit(); | ||
| const canAddSummits = memberObj.canAddSummits(); | ||
| const canDeleteSummits = memberObj.canDeleteSummits(); | ||
| ]; | ||
|
|
||
| return ( | ||
| <div className="container"> | ||
| <h3> | ||
| {" "} | ||
| {T.translate("directory.summits")} ({totalSummits}) | ||
| </h3> | ||
| {canAddSummits && ( | ||
| <div className="row"> | ||
| <div className="col-md-6 col-md-offset-6 text-right"> | ||
| <button | ||
| className="btn btn-primary right-space" | ||
| onClick={this.onNewSummit.bind(this)} | ||
| <div | ||
| className="container" | ||
| style={{ background: "#fff", borderRadius: 4 }} | ||
| > | ||
| <h3>{T.translate("directory.summits")}</h3> | ||
| <Grid2 | ||
| container | ||
| sx={{ | ||
| mb: 2, | ||
| width: "100%", | ||
| alignItems: "center", | ||
| justifyContent: "space-between", | ||
| flexWrap: "wrap" | ||
| }} | ||
| > | ||
| <Box component="span" sx={{ minWidth: 120 }}> | ||
| {totalSummits} {T.translate("general.items")} | ||
| </Box> | ||
| <Box sx={{ display: "flex", alignItems: "center", gap: 2 }}> | ||
| {canAddSummits && ( | ||
| <Button | ||
| variant="contained" | ||
| color="primary" | ||
| startIcon={<AddIcon />} | ||
| onClick={handleNewSummit} | ||
| sx={{ | ||
| height: "36px", | ||
| padding: "6px 16px", | ||
| fontSize: "1.4rem", | ||
| lineHeight: "2.4rem", | ||
| letterSpacing: "0.4px" | ||
| }} | ||
| > | ||
| {T.translate("directory.add_summit")} | ||
| </button> | ||
| </div> | ||
| </div> | ||
| )} | ||
| <div> | ||
| <table className="table" id="summit_table"> | ||
| <tbody> | ||
| {summits && | ||
| summits.map((summit) => ( | ||
| <tr key={`summit_${summit.id}`}> | ||
| <td className="summit_id"> {summit.id} </td> | ||
| <td className="summit_name"> {summit.name} </td> | ||
| <td> {formatEpoch(summit.start_date, "MMMM Do YYYY")} </td> | ||
| <td> {formatEpoch(summit.end_date, "MMMM Do YYYY")} </td> | ||
| <td> | ||
| {summit.invite_only_registration && ( | ||
| <span className="badge badge-warning"> | ||
| {" "} | ||
| {T.translate("directory.invitation_only")} | ||
| </span> | ||
| )} | ||
| </td> | ||
| <td className="center_text actions"> | ||
| <a | ||
| href="" | ||
| onClick={(e) => this.onSelectedSummit(e, summit)} | ||
| className="btn btn-default btn-sm" | ||
| > | ||
| {T.translate("directory.select")} | ||
| </a> | ||
| {canEditSummit && ( | ||
| <a | ||
| href="" | ||
| onClick={this.onEditSummit.bind(this, summit)} | ||
| className="btn btn-default btn-sm" | ||
| > | ||
| {T.translate("general.edit")} | ||
| </a> | ||
| )} | ||
| {canDeleteSummits && ( | ||
| <a | ||
| href="" | ||
| onClick={this.onDeleteSummit.bind(this, summit)} | ||
| className="btn btn-danger btn-sm" | ||
| > | ||
| {T.translate("general.delete")} | ||
| </a> | ||
| )} | ||
| </td> | ||
| </tr> | ||
| ))} | ||
| </tbody> | ||
| </table> | ||
| <Pagination | ||
| bsSize="medium" | ||
| prev | ||
| next | ||
| first | ||
| last | ||
| ellipsis | ||
| boundaryLinks | ||
| maxButtons={10} | ||
| items={lastPage} | ||
| activePage={currentPage} | ||
| onSelect={this.handlePageChange} | ||
| /> | ||
| </div> | ||
| </Button> | ||
| )} | ||
| </Box> | ||
| </Grid2> | ||
| <MuiTable | ||
| columns={columns} | ||
| data={safeSummits} | ||
| totalRows={totalSummits} | ||
| perPage={perPage} | ||
| currentPage={currentPage} | ||
| onPageChange={handlePageChange} | ||
| onEdit={canEditSummit ? handleEditSummit : undefined} | ||
| onDelete={canDeleteSummits ? (id) => deleteSummit(id) : undefined} | ||
| onSelect={handleSelectSummit} | ||
| options={{ sortCol: "id", sortDir: 1 }} |
There was a problem hiding this comment.
Sortable columns may not actually sort the full dataset.
Columns are marked sortable: true and options={{ sortCol: "id", sortDir: 1 }} sets an initial sort, but:
- No
onSort(or equivalent) callback is passed toMuiTable, so sort changes cannot trigger a server-side re-fetch. loadSummitsinsrc/actions/summit-actions.jshardcodesorder: "-start_date"and does not accept a sort parameter.
With server-side pagination (totalRows=67, perPage=10), any in-memory sort performed by MuiTable only reorders the 10 rows currently on screen — not the full dataset — which is misleading to users clicking the header arrows.
Options:
- Drop
sortable: truefrom the columns until server-side sorting is implemented. - Or extend
loadSummitsto accept anorderparameter and wire anonSortcallback that callsloadSummits(currentPage, perPage, order).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/summits/summit-directory-page.js` around lines 89 - 175, Columns
are marked sortable but sorting is only in-memory for the current page; either
remove sortable: true from the column definitions or implement server-side
sorting by extending loadSummits(order) in src/actions/summit-actions.js to
accept an order string (e.g. "-start_date") and applying it to the API call,
then wire an onSort callback on the component (pass onSort to MuiTable) that
receives (sortCol, sortDir) and calls loadSummits(currentPage, perPage,
computedOrder) and updates options (sortCol/sortDir) so header clicks trigger a
server fetch instead of client-only reordering; update any initial options to
reflect the chosen server-side order.
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/86b9j5nuu
Summary by CodeRabbit
New Features
Bug Fixes
Documentation