Feature/companies list mui#910
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughRefactors the company list page from a class component to a functional MUI-based page with a dialog-driven create/edit flow via a new ChangesCompany Management MUI Refactor
Sequence Diagram(s)sequenceDiagram
actor User
participant CompanyListPage
participant CompanyDialog
participant company-actions
participant API as REST API
participant Snackbar as snackbarHandler
rect rgba(33, 150, 243, 0.5)
Note over User,CompanyDialog: Edit flow
User->>CompanyListPage: click Edit on row
CompanyListPage->>company-actions: getCompany(id)
company-actions->>API: GET /companies/:id
API-->>company-actions: company entity
company-actions-->>CompanyListPage: currentCompany updated in Redux
CompanyListPage->>CompanyDialog: open with currentCompany
end
rect rgba(76, 175, 80, 0.5)
Note over User,Snackbar: Save flow
User->>CompanyDialog: submit form
CompanyDialog->>CompanyListPage: onSave(entity)
CompanyListPage->>company-actions: saveCompany(entity)
company-actions->>API: PUT or POST /companies
API-->>company-actions: success
company-actions->>Snackbar: snackbarSuccessHandler
company-actions->>company-actions: stopLoading() via .finally()
CompanyListPage->>CompanyDialog: close dialog
CompanyListPage->>company-actions: getCompanies() refresh
end
rect rgba(244, 67, 54, 0.5)
Note over User,Snackbar: Delete flow
User->>CompanyListPage: confirm delete via MuiTable dialog
CompanyListPage->>company-actions: deleteCompany(id)
company-actions->>API: DELETE /companies/:id
API-->>company-actions: success
company-actions->>Snackbar: snackbarSuccessHandler
company-actions->>company-actions: stopLoading() via .finally()
CompanyListPage->>company-actions: getCompanies() refresh
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
tomrndom
force-pushed
the
feature/companies-list-mui
branch
from
c27c41f to
ecef716
Compare
…er and actions Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
tomrndom
force-pushed
the
feature/companies-list-mui
branch
from
c32be01 to
4616df3
Compare
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/actions/company-actions.js (2)
210-219:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnsure
attachLogoclears loading state when company creation fails.Line 197 starts loading, but in the create-before-upload branch (Line 210-219), a failure in
postRequestnever reachesuploadFile(wherestopLoadinglives), leaving loading stuck.💡 Suggested fix
} else { - return postRequest( + return postRequest( createAction(UPDATE_COMPANY), createAction(COMPANY_ADDED), `${window.API_BASE_URL}/api/v1/companies`, normalizedEntity, snackbarErrorHandler, entity - )(params)(dispatch).then((payload) => { - dispatch(uploadFile(payload.response, file)); - }); + )(params)(dispatch) + .then((payload) => dispatch(uploadFile(payload.response, file))) + .catch((err) => { + dispatch(stopLoading()); + throw err; + }); } };🤖 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/actions/company-actions.js` around lines 210 - 219, The postRequest call in the attachLogo function (lines 210-219) only handles the success case via the .then() handler which calls uploadFile and its associated stopLoading action, but does not handle errors from postRequest. Add a .catch() handler to the promise chain that dispatches the stopLoading action when postRequest fails, ensuring the loading state is cleared regardless of whether the request succeeds or fails.
155-191:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn the request promise from
saveCompany.Line 156 and Line 174 start async requests but do not return them. The thunk resolves early, so callers cannot await real completion (which can break dialog
isSaving/post-save sequencing).💡 Suggested fix
if (entity.id) { - putRequest( + return putRequest( createAction(UPDATE_COMPANY), createAction(COMPANY_UPDATED), `${window.API_BASE_URL}/api/v1/companies/${entity.id}`, normalizedEntity, snackbarErrorHandler, entity )(params)(dispatch) .then(() => { dispatch( snackbarSuccessHandler({ title: T.translate("general.success"), html: T.translate("edit_company.company_saved") }) ); }) .finally(() => dispatch(stopLoading())); } else { - postRequest( + return postRequest( createAction(UPDATE_COMPANY), createAction(COMPANY_ADDED), `${window.API_BASE_URL}/api/v1/companies`, normalizedEntity, snackbarErrorHandler, entity )(params)(dispatch) .then(() => { dispatch( snackbarSuccessHandler({ title: T.translate("general.success"), html: T.translate("edit_company.company_created") }) ); }) .finally(() => dispatch(stopLoading())); }🤖 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/actions/company-actions.js` around lines 155 - 191, The saveCompany function does not return the promises from the async requests, causing the thunk to resolve before the actual requests complete. Add return statements before both the putRequest call (in the if block when entity.id exists) and the postRequest call (in the else block) to return the promise chains, allowing callers to properly await the completion of the save operations and maintain correct sequencing for dialog state management.
🤖 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/formik-inputs/mui-formik-color-input.js`:
- Line 8: The localValue state in the component is only initialized once from
field.value using useState, so it does not update when the Formik field value
changes externally (such as during form resets). Add a useEffect hook with
field.value as a dependency that calls setLocalValue whenever field.value
changes. This will ensure the color picker always displays the current Formik
field value even when it is updated externally.
In `@src/pages/companies/company-list-page.js`:
- Around line 78-80: The useState hook at the initial companies fetch is being
misused with a callback and dependency array, but useState ignores the second
argument entirely and is not intended for side effects. Replace the useState
call with useEffect to properly handle the side effect of calling getCompanies
on component mount, similar to how getSponsoredProjects is correctly implemented
elsewhere in the file. Move the getCompanies invocation inside the useEffect
function body and pass an empty dependency array to ensure it executes only on
initial component mount.
In `@src/pages/companies/components/company-dialog.js`:
- Around line 96-114: The `country` field is missing from the Formik
`initialValues` object in the company-dialog.js file, even though a country
autocomplete input exists in the form. Add the missing `country` field to the
initialValues object alongside the other fields like `city` and `state`, using
the pattern `country: initialEntity?.country ?? ""` to ensure the country value
is properly initialized and preserved when the form is submitted via
formik.values.
- Around line 174-184: The handleDeleteSponsorship function is not awaiting the
Promise returned by showConfirmDialog, which causes the confirm variable to hold
a Promise object instead of the boolean result. Since a Promise object is always
truthy, the if (confirm) check will always be true and onDeleteSponsorship will
execute immediately without waiting for user confirmation. Make the function
async by adding the async keyword to its declaration, then add await before the
showConfirmDialog call when assigning to confirm, so the code waits for the
user's response before executing onDeleteSponsorship.
- Around line 155-161: The handleAddSponsorshipType function calls
onAddSponsorship with incorrect argument order. The current order passes
(companyId, projectId, sponsorshipTypeId), but the function expects (projectId,
sponsorshipTypeId, entity) where the company ID must be included in the entity
object as a property. Reorder the arguments so selectedSponsoredProject is
passed first, selectedSponsorShipType second, and create an entity object as the
third argument that contains the company ID from formik.values.id.
In `@src/reducers/companies/company-reducer.js`:
- Around line 104-111: The code assumes that the find() call searching through
project_sponsorships will always return a match, but when no
supporting_companies matches the given supportingCompanyId, the variable f will
be undefined. When f is undefined, the subsequent filter operation trying to
access f.id will throw an error. Add a guard condition to check that f is
defined and truthy before attempting to filter project_sponsorships by f.id. If
f is undefined, return the current state unchanged or handle the case
appropriately instead of proceeding with the filter.
- Around line 63-69: The reducer's switch statement for the LOGOUT_USER case is
missing null safety for the payload object. In the destructuring assignment
where type and payload are extracted from the action parameter, add a default
value of an empty object to the payload variable to handle cases where
action.payload is undefined. Then, replace the unsafe hasOwnProperty call on
payload with Object.prototype.hasOwnProperty.call(payload, "persistStore") to
safely check for the persistStore property without throwing a TypeError when
payload is undefined.
---
Outside diff comments:
In `@src/actions/company-actions.js`:
- Around line 210-219: The postRequest call in the attachLogo function (lines
210-219) only handles the success case via the .then() handler which calls
uploadFile and its associated stopLoading action, but does not handle errors
from postRequest. Add a .catch() handler to the promise chain that dispatches
the stopLoading action when postRequest fails, ensuring the loading state is
cleared regardless of whether the request succeeds or fails.
- Around line 155-191: The saveCompany function does not return the promises
from the async requests, causing the thunk to resolve before the actual requests
complete. Add return statements before both the putRequest call (in the if block
when entity.id exists) and the postRequest call (in the else block) to return
the promise chains, allowing callers to properly await the completion of the
save operations and maintain correct sequencing for dialog state management.
🪄 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: 0f161bb2-ad2c-464d-98fc-783e2d30d6b7
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
package.jsonsrc/actions/company-actions.jssrc/components/mui/formik-inputs/mui-formik-async-select.jssrc/components/mui/formik-inputs/mui-formik-color-input.jssrc/i18n/en.jsonsrc/pages/companies/company-list-page.jssrc/pages/companies/components/company-dialog.jssrc/reducers/companies/company-list-reducer.jssrc/reducers/companies/company-reducer.js
|
|
||
| const MuiFormikColorInput = ({ name, ...rest }) => { | ||
| const [field, meta, helpers] = useField(name); | ||
| const [localValue, setLocalValue] = useState(field.value || "#000000"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "mui-formik-color-input.js" -type fRepository: fntechgit/summit-admin
Length of output: 127
🏁 Script executed:
cat -n ./src/components/mui/formik-inputs/mui-formik-color-input.jsRepository: fntechgit/summit-admin
Length of output: 1653
🏁 Script executed:
# Check if there are other similar color input components or formik input components
find ./src/components/mui/formik-inputs -type f -name "*.js" | head -20Repository: fntechgit/summit-admin
Length of output: 1284
🏁 Script executed:
# Check how this component is used in the codebase to understand reset/reinitialize patterns
rg "MuiFormikColorInput" --type js -A 2 -B 2 | head -50Repository: fntechgit/summit-admin
Length of output: 1103
🏁 Script executed:
# Search more broadly for usage of this component (case-insensitive)
rg "formik-color-input" --type js -i | head -20Repository: fntechgit/summit-admin
Length of output: 48
🏁 Script executed:
# Check if there's a pattern of how other components handle Formik field changes
rg "useField" ./src/components/mui/formik-inputs -A 5 | head -100Repository: fntechgit/summit-admin
Length of output: 7868
🏁 Script executed:
# Check if Formik's useField updates when field.value changes externally
# Also check for form reset patterns in the codebase
rg "formik|resetForm|reset" ./src/components/mui/formik-inputs -i | grep -E "(reset|Form)" | head -20Repository: fntechgit/summit-admin
Length of output: 2021
🏁 Script executed:
# Search for resetForm usage in the codebase
rg "resetForm|\.reset\(" --type js -B 3 -A 3 | head -60Repository: fntechgit/summit-admin
Length of output: 4766
🏁 Script executed:
# Check if there are examples of useEffect with field.value dependency in other formik input components
rg "useEffect.*field\." ./src/components/mui/formik-inputs --type js -A 5 -B 2Repository: fntechgit/summit-admin
Length of output: 48
🏁 Script executed:
# Check if there are any useEffect patterns in formik inputs at all
rg "useEffect" ./src/components/mui/formik-inputs --type js -A 3 -B 2Repository: fntechgit/summit-admin
Length of output: 6376
🏁 Script executed:
# Let me verify the exact issue: check if useState initializer runs only once
# and if field.value changes are reflected
rg "useState.*field\.value" ./src/components/mui/formik-inputs --type js -B 2 -A 5Repository: fntechgit/summit-admin
Length of output: 865
Sync local color state when Formik value changes externally.
The localValue state is initialized only once from field.value, so form resets or external Formik value updates leave the picker displaying stale data. Add a useEffect to synchronize the local state whenever the Formik field value changes.
Proposed fix
-import React, { useRef, useState } from "react";
+import React, { useEffect, useRef, useState } from "react";
@@
const [localValue, setLocalValue] = useState(field.value || "`#000000`");
const isDirtyRef = useRef(false);
+
+ useEffect(() => {
+ setLocalValue(field.value || "`#000000`");
+ }, [field.value]);🤖 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/formik-inputs/mui-formik-color-input.js` at line 8, The
localValue state in the component is only initialized once from field.value
using useState, so it does not update when the Formik field value changes
externally (such as during form resets). Add a useEffect hook with field.value
as a dependency that calls setLocalValue whenever field.value changes. This will
ensure the color picker always displays the current Formik field value even when
it is updated externally.
| initialValues: { | ||
| id: initialEntity?.id ?? 0, | ||
| name: initialEntity?.name ?? "", | ||
| url: initialEntity?.url ?? "", | ||
| contact_email: initialEntity?.contact_email ?? "", | ||
| member_level: initialEntity?.member_level ?? "", | ||
| color: initialEntity?.color ?? "", | ||
| admin_email: initialEntity?.admin_email ?? "", | ||
| city: initialEntity?.city ?? "", | ||
| state: initialEntity?.state ?? "", | ||
| industry: initialEntity?.industry ?? "", | ||
| products: initialEntity?.products ?? "", | ||
| contributions: initialEntity?.contributions ?? "", | ||
| description: initialEntity?.description ?? "", | ||
| overview: initialEntity?.overview ?? "", | ||
| commitment: initialEntity?.commitment ?? "", | ||
| logo: initialEntity?.logo ?? "", | ||
| big_logo: initialEntity?.big_logo ?? "" | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/pages/companies/components/company-dialog.js | sed -n '90,120p'Repository: fntechgit/summit-admin
Length of output: 1570
🏁 Script executed:
cat -n src/pages/companies/components/company-dialog.js | sed -n '300,325p'Repository: fntechgit/summit-admin
Length of output: 1309
country field is missing from Formik initialValues and will be lost on submit.
The form renders an autocomplete for name="country" (line 308), but country is not initialized in the Formik initialValues object (lines 96-114). When the form is submitted, valuesToSave = { ...formik.values } will exclude the country field, causing any edits to be discarded.
Proposed fix
initialValues: {
id: initialEntity?.id ?? 0,
name: initialEntity?.name ?? "",
url: initialEntity?.url ?? "",
contact_email: initialEntity?.contact_email ?? "",
member_level: initialEntity?.member_level ?? "",
color: initialEntity?.color ?? "",
admin_email: initialEntity?.admin_email ?? "",
city: initialEntity?.city ?? "",
state: initialEntity?.state ?? "",
+ country: initialEntity?.country ?? "",
industry: initialEntity?.industry ?? "",
products: initialEntity?.products ?? "",
contributions: initialEntity?.contributions ?? "",
description: initialEntity?.description ?? "",
overview: initialEntity?.overview ?? "",
commitment: initialEntity?.commitment ?? "",
logo: initialEntity?.logo ?? "",
big_logo: initialEntity?.big_logo ?? ""
},🤖 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/companies/components/company-dialog.js` around lines 96 - 114, The
`country` field is missing from the Formik `initialValues` object in the
company-dialog.js file, even though a country autocomplete input exists in the
form. Add the missing `country` field to the initialValues object alongside the
other fields like `city` and `state`, using the pattern `country:
initialEntity?.country ?? ""` to ensure the country value is properly
initialized and preserved when the form is submitted via formik.values.
| const handleAddSponsorshipType = () => { | ||
| if (!selectedSponsoredProject || !selectedSponsorShipType) return; | ||
| onAddSponsorship( | ||
| formik.values.id, | ||
| selectedSponsoredProject, | ||
| selectedSponsorShipType | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and read the file in question
find . -name "company-dialog.js" -type fRepository: fntechgit/summit-admin
Length of output: 117
🏁 Script executed:
# Search for onAddSponsorship definition and usage
rg "onAddSponsorship" --type js -B 2 -A 10Repository: fntechgit/summit-admin
Length of output: 6134
🏁 Script executed:
# Search for sponsorship-related action definitions or API calls
rg "sponsorship" --type js -i | head -50Repository: fntechgit/summit-admin
Length of output: 4765
🏁 Script executed:
# Find the definition of saveSupportingCompany and addSponsoreProjectSponsorship
rg "saveSupportingCompany|addSponsoreProjectSponsorship" --type js -B 5 -A 15Repository: fntechgit/summit-admin
Length of output: 17877
🏁 Script executed:
# Also search for the action creator definition
fd -type f -name "*action*" | grep -E "(company|sponsor)" | head -20Repository: fntechgit/summit-admin
Length of output: 237
🏁 Script executed:
# Look for reducer or saga that handles sponsorship
rg "addSponsoreProjectSponsorship|saveSupportingCompany" --type js | head -20Repository: fntechgit/summit-admin
Length of output: 1301
🏁 Script executed:
cat -n src/pages/companies/components/company-dialog.js | sed -n '150,165p'Repository: fntechgit/summit-admin
Length of output: 599
🏁 Script executed:
# Also check what the entire file looks like to understand the full context
wc -l src/pages/companies/components/company-dialog.jsRepository: fntechgit/summit-admin
Length of output: 119
🏁 Script executed:
# Let me check the exact saveSupportingCompany implementation to see the full parameters
sed -n '1,50p' src/actions/sponsored-project-actions.js | grep -A 30 "export const saveSupportingCompany"Repository: fntechgit/summit-admin
Length of output: 48
🏁 Script executed:
# Find the exact line where saveSupportingCompany is exported
rg "export const saveSupportingCompany" src/actions/sponsored-project-actions.js -A 40Repository: fntechgit/summit-admin
Length of output: 1447
Fix argument order in sponsorship add call.
onAddSponsorship is called with (companyId, projectId, sponsorshipTypeId), but saveSupportingCompany expects (projectId, sponsorshipTypeId, entity). The API path requires projectId as the first parameter, and the company ID must be passed as part of the entity object.
Proposed fix
const handleAddSponsorshipType = () => {
if (!selectedSponsoredProject || !selectedSponsorShipType) return;
onAddSponsorship(
- formik.values.id,
selectedSponsoredProject,
- selectedSponsorShipType
+ selectedSponsorShipType,
+ { id: formik.values.id }
);
};📝 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 handleAddSponsorshipType = () => { | |
| if (!selectedSponsoredProject || !selectedSponsorShipType) return; | |
| onAddSponsorship( | |
| formik.values.id, | |
| selectedSponsoredProject, | |
| selectedSponsorShipType | |
| ); | |
| const handleAddSponsorshipType = () => { | |
| if (!selectedSponsoredProject || !selectedSponsorShipType) return; | |
| onAddSponsorship( | |
| selectedSponsoredProject, | |
| selectedSponsorShipType, | |
| { id: formik.values.id } | |
| ); | |
| }; |
🤖 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/companies/components/company-dialog.js` around lines 155 - 161, The
handleAddSponsorshipType function calls onAddSponsorship with incorrect argument
order. The current order passes (companyId, projectId, sponsorshipTypeId), but
the function expects (projectId, sponsorshipTypeId, entity) where the company ID
must be included in the entity object as a property. Reorder the arguments so
selectedSponsoredProject is passed first, selectedSponsorShipType second, and
create an entity object as the third argument that contains the company ID from
formik.values.id.
| const { type, payload } = action; | ||
| switch (type) { | ||
| case LOGOUT_USER: | ||
| { | ||
| // we need this in ce the token expired while editing the form | ||
| if (payload.hasOwnProperty("persistStore")) { | ||
| return state; | ||
| } else { | ||
| return { ...state, entity: { ...DEFAULT_ENTITY }, errors: {} }; | ||
| } | ||
| // we need this in ce the token expired while editing the form | ||
| if (payload.hasOwnProperty("persistStore")) { | ||
| return state; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether LOGOUT_USER is ever dispatched without payload in repository code.
rg -nP --type=js -C2 'type\s*:\s*LOGOUT_USER'
rg -nP --type=js -C2 'createAction\(\s*LOGOUT_USER\s*\)'Repository: fntechgit/summit-admin
Length of output: 2050
Guard LOGOUT_USER against missing payload.
Line 67 calls payload.hasOwnProperty(...) without first ensuring payload exists. Test cases dispatch LOGOUT_USER without a payload object (e.g., sponsor-form-items-list-reducer.test.js:60, badge-scans-list-reducer.test.js:71, show-pages-list-reducer.test.js:55), causing the reducer to throw TypeError and break state updates during logout.
Apply the suggested fix:
- Default
payloadto{}at destructuring. - Use
Object.prototype.hasOwnProperty.call(payload, "persistStore")for safe property checks.
💡 Suggested fix
-const companyReducer = (state = DEFAULT_STATE, action) => {
- const { type, payload } = action;
+const companyReducer = (state = DEFAULT_STATE, action) => {
+ const { type, payload = {} } = action;
switch (type) {
case LOGOUT_USER:
// we need this in case the token expired while editing the form
- if (payload.hasOwnProperty("persistStore")) {
+ if (Object.prototype.hasOwnProperty.call(payload, "persistStore")) {
return state;
}
return { ...state, entity: { ...DEFAULT_ENTITY }, errors: {} };📝 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 { type, payload } = action; | |
| switch (type) { | |
| case LOGOUT_USER: | |
| { | |
| // we need this in ce the token expired while editing the form | |
| if (payload.hasOwnProperty("persistStore")) { | |
| return state; | |
| } else { | |
| return { ...state, entity: { ...DEFAULT_ENTITY }, errors: {} }; | |
| } | |
| // we need this in ce the token expired while editing the form | |
| if (payload.hasOwnProperty("persistStore")) { | |
| return state; | |
| } | |
| const { type, payload = {} } = action; | |
| switch (type) { | |
| case LOGOUT_USER: | |
| // we need this in case the token expired while editing the form | |
| if (Object.prototype.hasOwnProperty.call(payload, "persistStore")) { | |
| return 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/reducers/companies/company-reducer.js` around lines 63 - 69, The
reducer's switch statement for the LOGOUT_USER case is missing null safety for
the payload object. In the destructuring assignment where type and payload are
extracted from the action parameter, add a default value of an empty object to
the payload variable to handle cases where action.payload is undefined. Then,
replace the unsafe hasOwnProperty call on payload with
Object.prototype.hasOwnProperty.call(payload, "persistStore") to safely check
for the persistStore property without throwing a TypeError when payload is
undefined.
| const f = project_sponsorships.find((ps) => { | ||
| const e = ps.supporting_companies.find( | ||
| (sp) => sp.id == payload.supportingCompanyId | ||
| ); | ||
| return e; | ||
| }); | ||
| project_sponsorships = project_sponsorships.filter((e) => e.id != f.id); | ||
| return { |
There was a problem hiding this comment.
Avoid dereferencing f.id when no sponsorship match exists.
Line 110 assumes find(...) always succeeds. A stale or already-deleted supportingCompanyId makes f undefined and throws in the reducer.
💡 Suggested fix
- const f = project_sponsorships.find((ps) => {
+ const f = project_sponsorships.find((ps) => {
const e = ps.supporting_companies.find(
(sp) => sp.id == payload.supportingCompanyId
);
return e;
});
+ if (!f) return state;
project_sponsorships = project_sponsorships.filter((e) => e.id != f.id);📝 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 f = project_sponsorships.find((ps) => { | |
| const e = ps.supporting_companies.find( | |
| (sp) => sp.id == payload.supportingCompanyId | |
| ); | |
| return e; | |
| }); | |
| project_sponsorships = project_sponsorships.filter((e) => e.id != f.id); | |
| return { | |
| const f = project_sponsorships.find((ps) => { | |
| const e = ps.supporting_companies.find( | |
| (sp) => sp.id == payload.supportingCompanyId | |
| ); | |
| return e; | |
| }); | |
| if (!f) return state; | |
| project_sponsorships = project_sponsorships.filter((e) => e.id != f.id); | |
| return { |
🤖 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/companies/company-reducer.js` around lines 104 - 111, The code
assumes that the find() call searching through project_sponsorships will always
return a match, but when no supporting_companies matches the given
supportingCompanyId, the variable f will be undefined. When f is undefined, the
subsequent filter operation trying to access f.id will throw an error. Add a
guard condition to check that f is defined and truthy before attempting to
filter project_sponsorships by f.id. If f is undefined, return the current state
unchanged or handle the case appropriately instead of proceeding with the
filter.
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
Review contra patrón popup/dialog (ticket #86bag2zk7)
El ticket #86bag2zk7 define el patrón canónico para popups en páginas de lista MUI. Esta PR introduce el mismo CompanyListPage + CompanyDialog pero desvía del patrón en 4 puntos críticos y tiene 2 bugs adicionales. Ver los comentarios inline para detalle específico.
| sponsoredProjects | ||
| }) => { | ||
| const [companyPopup, setCompanyPopup] = useState(false); | ||
| const [isSaving, setIsSaving] = useState(false); |
There was a problem hiding this comment.
[HIGH] Anti-patrón: isSaving vive en el padre
El patrón del ticket es explícito: "isSaving lives in the popup, never in the parent".
isSaving debe moverse a CompanyDialog y eliminarse de CompanyListPage. El padre nunca debe controlar el estado de guardado del popup.
| sortDir: orderDir | ||
| }; | ||
|
|
||
| useState(() => { |
There was a problem hiding this comment.
[MEDIUM] useState usado como useEffect
useState ignora su segundo argumento — el [] de dependencias no tiene efecto. La función se ejecuta como lazy initializer durante el render, lo cual es un efecto secundario en fase de render (prohibido en React y ejecutado dos veces en Strict Mode).
// ❌ actual
useState(() => { getCompanies(); }, []);
// ✅ correcto
useEffect(() => { getCompanies(); }, []);| setIsSaving(true); | ||
| saveCompany(entity) | ||
| .then(() => { | ||
| setCompanyPopup(false); |
There was a problem hiding this comment.
[HIGH] Anti-patrón: el padre cierra el dialog y no retorna la promesa
Dos deviaciones del patrón del ticket en este bloque:
- El padre cierra el dialog (
setCompanyPopup(false)) — el patrón requiere que sea el popup quien llameonClose()en su propio.then(). handleSaveno retorna la promesa — el ticket requiere: "Parent's handleSave must RETURN the promise so the popup chain works". Sinreturn, el popup no puede encadenar.then(() => onClose()).
// ✅ correcto según el patrón
const handleSave = (entity) =>
saveCompany(entity)
.then(() => getCompanies(term, DEFAULT_CURRENT_PAGE, perPage, order, orderDir));
// El popup recibe este handleSave como onSave y llama onClose() en su propio .then()| {companyPopup && ( | ||
| <CompanyDialog | ||
| entity={currentCompany} | ||
| isSaving={isSaving} |
There was a problem hiding this comment.
[HIGH] Prop drilling de isSaving al popup
El ticket lo lista como anti-patrón explícito: "Split onSaved + onClose props are the anti-pattern" y el equivalente aquí es pasar isSaving al popup en lugar de que el popup lo gestione internamente.
CompanyDialog debe tener su propio const [isSaving, setIsSaving] = useState(false) y no recibir isSaving como prop.
| )(params)(dispatch).then(() => { | ||
| dispatch(showSuccessMessage(T.translate("edit_company.company_saved"))); | ||
| }); | ||
| )(params)(dispatch) |
There was a problem hiding this comment.
[HIGH] Falta return — promesa no propagada al caller
La función async resuelve inmediatamente al llegar aquí porque putRequest(...)(params)(dispatch) no está retornado. El caller en handleSave ve una Promise<void> que ya está resuelta, por lo que .then() dispara antes de que la API responda.
Consecuencias: el dialog se cierra antes de confirmar éxito, la lista se recarga con datos desactualizados, y la protección de doble-submit queda sin efecto.
// ✅ agregar return en ambas ramas
return putRequest(...)(params)(dispatch)
.then(...)
.finally(...);| }) | ||
| ); | ||
| }); | ||
| )(params)(dispatch) |
There was a problem hiding this comment.
[HIGH] Falta return en rama else — mismo problema que la rama if
postRequest(...)(params)(dispatch) tampoco está retornado. Ver el comentario en la línea 163.
| ); | ||
| if (!supportingCompany) return; | ||
|
|
||
| const confirm = showConfirmDialog({ |
There was a problem hiding this comment.
[HIGH] showConfirmDialog usada de forma sincrónica — el delete siempre se ejecuta
showConfirmDialog retorna una Promise<boolean>. Un objeto Promise es siempre truthy, por lo que onDeleteSponsorship se llama siempre, sin importar si el usuario confirma o cancela. El dialog de confirmación aparece pero el delete ya fue despachado.
// ❌ actual — confirm es Promise (siempre truthy)
const confirm = showConfirmDialog({ ... });
if (confirm) onDeleteSponsorship(...);
// ✅ correcto
const handleDeleteSponsorship = async (sponsorshipId) => {
// ...
const confirmed = await showConfirmDialog({ ... });
if (confirmed) onDeleteSponsorship(...);
};
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
Review against the popup/dialog pattern defined in ticket #86bag2zk7. The new CompanyListPage + CompanyDialog deviates from the established pattern in 4 critical points and has 2 additional bugs. See inline comments for details.
smarcet
dismissed
their stale review
Superseded by updated review with only confirmed findings
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
Review against ticket #86bag2zk7. Four confirmed bugs and one question on an intentional behavior change. See inline comments.
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
2 participants
ref: https://app.clickup.com/t/86b9n7p5w
Signed-off-by: Tomás Castillo tcastilloboireau@gmail.com
Summary by CodeRabbit
New Features
Improvements