Feature/companies list mui#910
Open
tomrndom wants to merge 12 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCompany state, actions, and UI are updated for a dialog-driven company management flow. The async autocomplete gains default-options support, company strings are extended, and a new ChangesCompany Management MUI Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
tomrndom
force-pushed
the
feature/companies-list-mui
branch
from
c27c41f to
ecef716
Compare
tomrndom
force-pushed
the
feature/companies-list-mui
branch
from
c32be01 to
4616df3
Compare
![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
smarcet
previously requested changes
Jun 18, 2026
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.
smarcet
previously requested changes
Jun 18, 2026
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
requested changes
Jun 18, 2026
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.
smarcet
requested changes
Jun 18, 2026
smarcet
reviewed
Jun 23, 2026
smarcet
reviewed
Jun 23, 2026
smarcet
reviewed
Jun 23, 2026
smarcet
reviewed
Jun 23, 2026
smarcet
requested changes
Jun 23, 2026
…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>
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
00063d2 to
b82a7fb
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/actions/company-actions.js (1)
208-220: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReturn the upload promise from
attachLogoand cover create-failure loading cleanup (Line 208, Line 218).
attachLogoresolves before the upload finishes becausedispatch(uploadFile(...))is not returned in either branch. Also, in the create path, a failedPOST /companiesleavesstartLoading()unbalanced.Suggested fix
const uploadFile = picAttr === "logo" ? uploadLogo : uploadBigLogo; if (entity.id) { - dispatch(uploadFile(entity, file)); + return dispatch(uploadFile(entity, file)); } else { 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) => { + return 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 208 - 220, The attachLogo function does not properly return the upload promise because dispatch(uploadFile(...)) is not returned in either branch, causing the function to resolve before the upload completes. In the first branch (when entity.id exists), return the result of dispatch(uploadFile(entity, file)). In the second branch (POST request path), return the result of dispatch(uploadFile(payload.response, file)) from within the .then() callback. Additionally, add a .catch() handler to the promise chain to ensure proper cleanup of the loading state when the POST request fails, maintaining the balance of startLoading() call.
🤖 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/company-actions.js`:
- Around line 262-263: Add a guard clause before calling startsWith on
normalizedEntity.color to prevent runtime errors when color is null, undefined,
or not a string. Verify that normalizedEntity.color exists and is a string type
before attempting to call the startsWith method, and only execute the substr
operation when the guard condition is satisfied.
In `@src/components/mui/formik-inputs/mui-formik-async-select.js`:
- Around line 21-24: The component has inconsistent handling of multi-select
mode, using both isMulti prop (in the function parameter destructuring and at
Line 96 with Autocomplete) and multiple prop (at Line 30 for value shape and
Line 73 for selection mapping, and Line 140). Standardize on a single prop name
throughout the component by replacing all occurrences of the multiple prop with
isMulti to ensure consistent behavior when determining whether the component
should operate in multi-select or single-select mode across value shape
handling, selection mapping, and the Autocomplete component configuration.
---
Outside diff comments:
In `@src/actions/company-actions.js`:
- Around line 208-220: The attachLogo function does not properly return the
upload promise because dispatch(uploadFile(...)) is not returned in either
branch, causing the function to resolve before the upload completes. In the
first branch (when entity.id exists), return the result of
dispatch(uploadFile(entity, file)). In the second branch (POST request path),
return the result of dispatch(uploadFile(payload.response, file)) from within
the .then() callback. Additionally, add a .catch() handler to the promise chain
to ensure proper cleanup of the loading state when the POST request fails,
maintaining the balance of startLoading() call.
🪄 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: 27ef094a-3b4d-481f-93f7-ae55d6229799
⛔ 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/i18n/en.jsonsrc/pages/companies/company-list-page.jssrc/pages/companies/components/__tests__/company-dialog.test.jssrc/pages/companies/components/company-dialog.jssrc/reducers/companies/company-list-reducer.jssrc/reducers/companies/company-reducer.js
✅ Files skipped from review due to trivial changes (2)
- package.json
- src/i18n/en.json
🚧 Files skipped from review as they are similar to previous changes (4)
- src/pages/companies/components/tests/company-dialog.test.js
- src/reducers/companies/company-list-reducer.js
- src/pages/companies/company-list-page.js
- src/reducers/companies/company-reducer.js
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
|
@tomrndom there's an argument order mismatch in The dialog calls: onAddSponsorship(
formik.values.id, // company id
selectedSponsoredProject, // project id
selectedSponsorShipType // sponsorship type id (a number)
)But saveSupportingCompany(
selectedSponsoredProject,
selectedSponsorShipType,
{ id: 0, company: { id: companyId } }
)With the current order, onAddSponsorship(
selectedSponsoredProject,
selectedSponsorShipType,
{ id: 0, company: { id: formik.values.id } }
) |
|
@tomrndom When export const attachLogo = (entity, file, picAttr) => async (dispatch) => {
// ...
if (entity.id) {
dispatch(uploadFile(entity, file)); // no return
}
};Because it's an setIsSaving(true);
onAttach(initialEntity, uploadLogo, field)
.finally(() => setIsSaving(false)); // fires in the same tick
Fix is a one-liner: if (entity.id) {
return dispatch(uploadFile(entity, file)); // propagates the Promise
} |
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
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/86b9n7p5w
Signed-off-by: Tomás Castillo tcastilloboireau@gmail.com
Summary by CodeRabbit