feat: move show directory to mui#896
Conversation
📝 WalkthroughWalkthroughRefactors SummitDirectoryPage to a functional React component with MuiTable, adds defensive member handling and render-time error fallback; extends loadSummits with term and hidePastEvents filters; updates i18n directory labels and adds error/loading keys. ChangesInternationalization updates
Summit Directory Page Refactor
Summit actions: filtering and escapes
Reducer: directory pagination handling
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant SummitPage as SummitDirectoryPage (UI)
participant Actions as loadSummits (Redux Action)
participant API as Backend API
participant Store as Redux Store
User->>SummitPage: open page / interact (search/toggle/paginate)
SummitPage->>Actions: dispatch loadSummits(term, hidePastEvents, page)
Actions->>API: HTTP GET /api/v1/summits/all?filter[]=... (filters built, term escaped)
API-->>Actions: 200 OK (summits list)
Actions->>Store: dispatch success (summits)
Store-->>SummitPage: updated props/state (safeSummits)
SummitPage->>User: render MuiTable with data (or show error if render fails)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
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
priscila-moneo
force-pushed
the
feature/move-show-directory-to-mui
branch
3 times, most recently
from
fcd09b1 to
40761d0
Compare
priscila-moneo
force-pushed
the
feature/move-show-directory-to-mui
branch
from
40761d0 to
7e6425f
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/pages/summits/summit-directory-page.js (2)
54-57: 💤 Low value
useEffectis missing dependencies.The effect references
loadSummits,clearCurrentSummit,perPage,searchTerm, andhidePastEventsbut the dependency array is[]. Mount-only execution is presumably intentional here (initial fetch with default state), butreact-hooks/exhaustive-depswill flag this and any future change that genuinely needs to react to a prop/state change won't trigger a re-fetch. Consider one of:
- Add an eslint-disable comment with a note explaining the mount-only intent, or
- Use a
mountedRefpattern, or- Add the deps and gate the call appropriately.
🤖 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/pages/summits/summit-directory-page.js` around lines 54 - 57, The useEffect in summit-directory-page currently calls clearCurrentSummit() and loadSummits(DEFAULT_CURRENT_PAGE, perPage, searchTerm, hidePastEvents) but has an empty dependency array which will be flagged; update the effect to explicitly document the mount-only intent by adding an eslint-disable-next-line react-hooks/exhaustive-deps above the useEffect and a brief comment like "run only on mount to perform initial fetch" OR alternatively add the referenced dependencies (loadSummits, clearCurrentSummit, perPage, searchTerm, hidePastEvents) to the dependency array and guard the call (e.g., with a mountedRef or a conditional flag) so loadSummits is only invoked on initial mount; refer to the useEffect, loadSummits, and clearCurrentSummit symbols when applying the change.
82-248: ⚖️ Poor tradeoffTry/catch wrapping JSX in a functional component does not provide real React error boundary semantics.
This only catches synchronous errors during this component's initial render—not errors thrown by child components like
MuiTable, during the commit phase, or in event handlers. Since the codebase already usesSentry.ErrorBoundaryat the app level (src/app.js, lines 176–215), consider removing this manual try/catch and relying on the existing app-level boundary, or wrap this page in a dedicated class-component error boundary if page-specific error handling is needed.The
<pre>{err.message}</pre>fallback also risks exposing technical details to end users—a generic error message would be safer.🤖 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/pages/summits/summit-directory-page.js` around lines 82 - 248, The component currently wraps its JSX return in a try/catch (surrounding handlers like handlePageChange, handleSearch, handleHidePastEventsChange and the returned JSX including MuiTable) which is not a real React error boundary; remove the outer try/catch and its fallback UI, and either rely on the existing app-level Sentry.ErrorBoundary or wrap this page in a proper ErrorBoundary class component (or higher-order boundary) if page-specific handling is required; if you must keep a local fallback, replace the sensitive err.message display with a generic user-friendly message instead of exposing technical details.
🤖 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/actions/summit-actions.js`:
- Around line 162-165: The filter for hidePastEvents is inverted: currently you
push `end_date<${now}` which selects past events; change the condition in the
hidePastEvents block so it pushes a filter that keeps events whose end_date is
at or after now (e.g. use `end_date>=${now}`) — update the code section in
src/actions/summit-actions.js where hidePastEvents is checked and the filters
array is appended.
In `@src/pages/summits/summit-directory-page.js`:
- Around line 122-191: Several UI strings are hardcoded; replace the literals
with i18n lookups: change the column headers for columnKey "sponsor_qty",
"sponsor_forms_qty", and "sponsor_attachments_qty" to use
T.translate("directory.sponsors"), T.translate("directory.forms"), and
T.translate("directory.attachments") respectively; replace the
FormControlLabel.label and Checkbox inputProps["aria-label"] literals with
T.translate("directory.hide_past_events"); and replace the MuiSearchInput
placeholder "Search..." with T.translate("directory.placeholders.search") (or
directory.select) and add the corresponding keys to the directory block in your
i18n JSON.
- Around line 120-134: The sponsor columns (columnKey: "sponsor_qty",
"sponsor_forms_qty", "sponsor_attachments_qty") are rendering 0 because
loadSummits is only requesting fields
"id,name,start_date,end_date,invite_only_registration"; update the loadSummits
call to include the sponsor count fields (or related expansions) so the API
returns sponsor_qty, sponsor_forms_qty and sponsor_attachments_qty (or the
appropriate relation paths) before keeping these columns, e.g., add those field
names or expand the sponsor relation in the fields/expand/relations argument
used by loadSummits; alternatively remove these column definitions from the
table until the backend provides the counts.
- Around line 220-221: MuiTable callbacks consistently receive full row objects
but onDelete currently wraps its arg as (id) => deleteSummit(id), which will
pass the whole row into deleteSummit (which expects an ID). Fix by aligning
shapes: either change the onDelete prop to extract the id from the row before
calling deleteSummit (e.g. call deleteSummit(row.id) from the wrapper) or change
deleteSummit to accept a summit row and use summit.id internally; update the
onDelete assignment (and any related deleteDialogBody or delete handlers) to use
the chosen shape consistently with handleEditSummit, handleSelectSummit, getName
and MuiTable's callback contract.
---
Nitpick comments:
In `@src/pages/summits/summit-directory-page.js`:
- Around line 54-57: The useEffect in summit-directory-page currently calls
clearCurrentSummit() and loadSummits(DEFAULT_CURRENT_PAGE, perPage, searchTerm,
hidePastEvents) but has an empty dependency array which will be flagged; update
the effect to explicitly document the mount-only intent by adding an
eslint-disable-next-line react-hooks/exhaustive-deps above the useEffect and a
brief comment like "run only on mount to perform initial fetch" OR alternatively
add the referenced dependencies (loadSummits, clearCurrentSummit, perPage,
searchTerm, hidePastEvents) to the dependency array and guard the call (e.g.,
with a mountedRef or a conditional flag) so loadSummits is only invoked on
initial mount; refer to the useEffect, loadSummits, and clearCurrentSummit
symbols when applying the change.
- Around line 82-248: The component currently wraps its JSX return in a
try/catch (surrounding handlers like handlePageChange, handleSearch,
handleHidePastEventsChange and the returned JSX including MuiTable) which is not
a real React error boundary; remove the outer try/catch and its fallback UI, and
either rely on the existing app-level Sentry.ErrorBoundary or wrap this page in
a proper ErrorBoundary class component (or higher-order boundary) if
page-specific handling is required; if you must keep a local fallback, replace
the sensitive err.message display with a generic user-friendly message instead
of exposing technical details.
🪄 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: 6895a491-7988-49b9-83fd-5ceb8d440bf6
📒 Files selected for processing (3)
src/actions/summit-actions.jssrc/i18n/en.jsonsrc/pages/summits/summit-directory-page.js
| { | ||
| columnKey: "sponsor_qty", | ||
| header: "Sponsors", | ||
| render: (row) => row.sponsor_qty ?? 0 | ||
| }, | ||
| { | ||
| columnKey: "sponsor_forms_qty", | ||
| header: "Forms", | ||
| render: (row) => row.sponsor_forms_qty ?? 0 | ||
| }, | ||
| { | ||
| columnKey: "sponsor_attachments_qty", | ||
| header: "Attachments", | ||
| render: (row) => row.sponsor_attachments_qty ?? 0 | ||
| }, |
There was a problem hiding this comment.
Sponsor columns will always show 0 — fields aren't requested from the API.
The columns sponsor_qty, sponsor_forms_qty, and sponsor_attachments_qty rely on properties that aren't included in the fields param of loadSummits (which currently requests only "id,name,start_date,end_date,invite_only_registration"). As a result, row.sponsor_qty ?? 0 will always render 0, making these columns misleading.
Either expand the fields (and expand/relations if needed) in loadSummits so the API returns these counts, or remove the columns until the backend supplies them.
🤖 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/pages/summits/summit-directory-page.js` around lines 120 - 134, The
sponsor columns (columnKey: "sponsor_qty", "sponsor_forms_qty",
"sponsor_attachments_qty") are rendering 0 because loadSummits is only
requesting fields "id,name,start_date,end_date,invite_only_registration"; update
the loadSummits call to include the sponsor count fields (or related expansions)
so the API returns sponsor_qty, sponsor_forms_qty and sponsor_attachments_qty
(or the appropriate relation paths) before keeping these columns, e.g., add
those field names or expand the sponsor relation in the fields/expand/relations
argument used by loadSummits; alternatively remove these column definitions from
the table until the backend provides the counts.
priscila-moneo
force-pushed
the
feature/move-show-directory-to-mui
branch
from
9c5543c to
f8aab1b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pages/summits/summit-dashboard-page.js`:
- Around line 84-85: Guard against member being undefined before calling Member
or canEditSummit: check the optional member prop and only construct Member when
present (e.g., set memberObj to null when member is falsy) and compute
canEditSummit using a safe fallback (call memberObj.canEditSummit() only when
memberObj exists and default to false otherwise); update the code around the
Member instantiation and the canEditSummit usage so Member and canEditSummit are
not invoked when member is undefined.
In `@src/reducers/summits/directory-reducer.js`:
- Around line 26-31: In the REQUEST_SUMMITS case in directory-reducer.js do not
clear summits (remove the summits: [] assignment); instead preserve
state.summits and only update pagination fields like currentPage and perPage
(and optionally set a loading flag). Locate the REQUEST_SUMMITS branch that
returns { ...state, summits: [], currentPage: page, perPage } and change it to
keep existing summits (e.g., return { ...state, currentPage: page, perPage } )
so UI isn’t emptied on failed fetches.
🪄 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: d6d97af8-f958-4fd1-a4dd-ef63f8401ae7
📒 Files selected for processing (5)
src/actions/summit-actions.jssrc/i18n/en.jsonsrc/pages/summits/summit-dashboard-page.jssrc/pages/summits/summit-directory-page.jssrc/reducers/summits/directory-reducer.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/i18n/en.json
- src/pages/summits/summit-directory-page.js
| const memberObj = new Member(member); | ||
| const canEditSummit = memberObj.canEditSummit(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how the Member constructor handles undefined/null
ast-grep --pattern 'class Member {
constructor($$$) {
$$$
}
$$$
}'Repository: fntechgit/summit-admin
Length of output: 6183
🏁 Script executed:
#!/bin/bash
# Check the PropTypes declaration and the context around lines 84-85
cat -n src/pages/summits/summit-dashboard-page.js | sed -n '1,100p'Repository: fntechgit/summit-admin
Length of output: 3783
🏁 Script executed:
#!/bin/bash
# Search for PropTypes definition in the component
cat -n src/pages/summits/summit-dashboard-page.js | tail -n +100Repository: fntechgit/summit-admin
Length of output: 17420
Guard against undefined member before calling canEditSummit().
The member prop is optional (no .isRequired in PropTypes at line 464), so it can arrive as undefined. The Member constructor stores it without validation, and canEditSummit() immediately accesses this._member.groups without a null check, causing a runtime crash. The early-return guard on line 87 only checks currentSummit, not member.
Add a null check before instantiating the Member or before calling canEditSummit():
const memberObj = member ? new Member(member) : null;
const canEditSummit = memberObj?.canEditSummit() ?? false;🤖 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/pages/summits/summit-dashboard-page.js` around lines 84 - 85, Guard
against member being undefined before calling Member or canEditSummit: check the
optional member prop and only construct Member when present (e.g., set memberObj
to null when member is falsy) and compute canEditSummit using a safe fallback
(call memberObj.canEditSummit() only when memberObj exists and default to false
otherwise); update the code around the Member instantiation and the
canEditSummit usage so Member and canEditSummit are not invoked when member is
undefined.
| return { | ||
| ...state, | ||
| summits: [], | ||
| currentPage: page, | ||
| perPage | ||
| }; |
There was a problem hiding this comment.
Avoid clearing summits during REQUEST_SUMMITS to prevent empty state on fetch errors.
On Line 28, resetting to summits: [] immediately drops visible data before the request succeeds. If the request fails, the reducer never restores the previous list, leaving the directory blank until another successful load.
💡 Proposed fix
return {
...state,
- summits: [],
currentPage: page,
perPage
};📝 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.
| return { | |
| ...state, | |
| summits: [], | |
| currentPage: page, | |
| perPage | |
| }; | |
| return { | |
| ...state, | |
| currentPage: page, | |
| perPage | |
| }; |
🤖 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/reducers/summits/directory-reducer.js` around lines 26 - 31, In the
REQUEST_SUMMITS case in directory-reducer.js do not clear summits (remove the
summits: [] assignment); instead preserve state.summits and only update
pagination fields like currentPage and perPage (and optionally set a loading
flag). Locate the REQUEST_SUMMITS branch that returns { ...state, summits: [],
currentPage: page, perPage } and change it to keep existing summits (e.g.,
return { ...state, currentPage: page, perPage } ) so UI isn’t emptied on failed
fetches.
3 participants
ref: https://app.clickup.com/t/86b9j5nuu
Edit:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation