feat: move tax types grid to mui#931
Conversation
📝 WalkthroughWalkthroughThis PR moves tax-type management to an in-place modal: actions gain term/pagination/sort, the reducer stores pagination state, the form is rebuilt with Formik+Yup and MUI, a TaxTypePopup dialog wraps the form, the list page uses hooks + MuiTable and opens the modal for add/edit, and routing/layout files are updated. ChangesTax Type Management Modal Refactor
Sequence DiagramsequenceDiagram
actor User
participant List as TaxTypeListPage
participant Actions as ReduxActions
participant Server as API
participant Popup as TaxTypePopup
participant Form as TaxTypeForm
User->>List: load page / interact (search/sort/paginate)
List->>Actions: getTaxTypes(term, page, perPage, order, orderDir)
Actions->>Server: fetch tax types (filters, pagination)
Server-->>Actions: paginated response
Actions-->>List: update reducer (taxTypes, total, current_page, last_page)
User->>List: click Add
List->>Actions: resetTaxTypeForm()
List->>Popup: open modal (empty entity)
User->>Form: enter data
Form->>Actions: saveTaxType(entity)
Actions->>Server: create/update tax type
Server-->>Actions: saved entity
Actions-->>List: refresh via getTaxTypes(...)
List->>Popup: close modal
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 |
santipalenque
left a comment
santipalenque
left a comment
There was a problem hiding this comment.
overall looks great, just a couple of requests
| `${window.API_BASE_URL}/api/v1/summits/${currentSummit.id}/tax-types`, | ||
| authErrorHandler, | ||
| { order, orderDir } | ||
| { order: normalizedOrder, orderDir, page, perPage, term } |
There was a problem hiding this comment.
the order stored in reducer should be order not normalizedOrder, the latter is just to send to API
| }) => { | ||
| const [entity, setEntity] = useState({ ...entityProp }); | ||
| const [errors, setErrors] = useState(errorsProp); | ||
| const prevEntityRef = useRef(entityProp); |
There was a problem hiding this comment.
no need for this, you can accomplish the same with just an useEffect and putting the entity in dependencies
| } | ||
| } | ||
| <Switch> | ||
| <Route strict exact path={match.url} component={TaxTypeListPage} /> |
There was a problem hiding this comment.
if we are only having one route then there is no need for a tax-type-layout, we can just move this route to the parent layout and get rid of it
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/pages/taxes/tax-type-list-page.js (1)
57-61: 💤 Low valueConsider keying the effect on
currentSummit?.id.Depending on
currentSummitre-runs the fetch whenever any field of the summit object changes reference. Since the only thing this effect cares about is the active summit identity, using[currentSummit?.id]is cheaper and matches the guard on line 58.🤖 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/taxes/tax-type-list-page.js` around lines 57 - 61, The useEffect currently depends on the entire currentSummit object causing unnecessary re-runs; change its dependency array to depend only on currentSummit?.id and keep the existing guard so the effect only triggers when the summit identity changes—update the useEffect dependency from [currentSummit] to [currentSummit?.id] for the effect that calls getTaxTypes(term, currentPage, perPage, order, orderDir).src/actions/tax-actions.js (1)
82-85: 💤 Low valueDead
nullchecks on guaranteed non-null values.
normalizedOrderis always assigned to eitherorderor"name"via the ternary on lines 59–61, andorderDirhas a default value (DEFAULT_ORDER_DIR). Both will never benull, so theif (normalizedOrder != null && orderDir != null)guard is dead code. You can drop the conditional and always setparams.order.🤖 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/tax-actions.js` around lines 82 - 85, The null-check guard around setting params.order is dead because normalizedOrder is always assigned (either order or "name") and orderDir defaults to DEFAULT_ORDER_DIR; remove the surrounding if and always assign params.order by computing orderDirSign (using orderDir === 1 ? "" : "-") and setting params.order = `${orderDirSign}${normalizedOrder}` within the same block (refer to normalizedOrder, orderDir, DEFAULT_ORDER_DIR, orderDirSign, and params.order).
🤖 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/forms/tax-type-form.js`:
- Around line 23-46: The useEffect in TaxTypeForm currently depends on local
state (entity, errors) causing cleared local errors to be clobbered by
errorsProp; split into two effects: 1) a useEffect that runs when errorsProp
changes to call scrollToError(errorsProp) and, if needed, setErrors(errorsProp)
only when shallowEqual(errors, errorsProp) is false (depend only on errorsProp
and errorsProp reference change), and 2) a useEffect that syncs entity from
entityProp (compare entityProp.id to entity.id and call
setEntity({...entityProp}) when entityProp.id changes) that depends only on
entityProp; remove entity and errors from the dependency array of the original
combined effect so local handleChange updates to errors are not overwritten.
In `@src/pages/taxes/tax-type-list-page.js`:
- Around line 92-96: The modal is being closed in the finally block of
handleSave which hides validation errors on failure; instead, remove
setShowTaxTypeModal(false) from the finally and call it only on successful save
inside the promise resolution (the .then chain after saveTaxType), e.g., after
getTaxTypes completes, call setShowTaxTypeModal(false); leave rejections
untouched so currentTaxTypeErrors (and any error UI) remain visible when
saveTaxType rejects.
---
Nitpick comments:
In `@src/actions/tax-actions.js`:
- Around line 82-85: The null-check guard around setting params.order is dead
because normalizedOrder is always assigned (either order or "name") and orderDir
defaults to DEFAULT_ORDER_DIR; remove the surrounding if and always assign
params.order by computing orderDirSign (using orderDir === 1 ? "" : "-") and
setting params.order = `${orderDirSign}${normalizedOrder}` within the same block
(refer to normalizedOrder, orderDir, DEFAULT_ORDER_DIR, orderDirSign, and
params.order).
In `@src/pages/taxes/tax-type-list-page.js`:
- Around line 57-61: The useEffect currently depends on the entire currentSummit
object causing unnecessary re-runs; change its dependency array to depend only
on currentSummit?.id and keep the existing guard so the effect only triggers
when the summit identity changes—update the useEffect dependency from
[currentSummit] to [currentSummit?.id] for the effect that calls
getTaxTypes(term, currentPage, perPage, order, orderDir).
🪄 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: 9640f65d-8b34-4dd1-bd06-09cb030783d6
📒 Files selected for processing (8)
src/actions/tax-actions.jssrc/components/forms/tax-type-form.jssrc/layouts/summit-id-layout.jssrc/layouts/tax-type-layout.jssrc/pages/taxes/edit-tax-type-page.jssrc/pages/taxes/popup/tax-type-popup.jssrc/pages/taxes/tax-type-list-page.jssrc/reducers/taxes/tax-type-list-reducer.js
💤 Files with no reviewable changes (2)
- src/pages/taxes/edit-tax-type-page.js
- src/layouts/tax-type-layout.js
| path={`${match.url}/tax-types`} | ||
| render={(routeProps) => ( | ||
| <> | ||
| <Breadcrumb |
There was a problem hiding this comment.
this is the onlyu route, the breadcrumb can go in the Page component
| const RegFeedMetadataLayout = React.lazy(() => | ||
| import("./reg-feed-metadata-layout") | ||
| ); | ||
| const RestrictedTaxTypeListPage = Restrict(TaxTypeListPage, "taxes"); |
There was a problem hiding this comment.
you can add the Restrict in the page component export: example
| newEntity[id] = value; | ||
| this.setState({ entity: newEntity, errors: newErrors }); | ||
| } | ||
| if (ev.target.type === "checkbox") { |
There was a problem hiding this comment.
doesn't look like we have any checkboxes or datetime inputs in this form, do we need this ?
priscila-moneo
force-pushed
the
feature/move-tax-types-grid-mui
branch
from
552e588 to
e8c64c4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/forms/tax-type-form.js`:
- Around line 128-184: The form is a real <form> but Enter triggers native
submit because handleSubmit is only wired to the Button onClick; add an onSubmit
prop to the form element (the Box with component="form") that calls handleSubmit
and ensures it does event.preventDefault(), and then remove (or keep but no-op)
the Button's onClick to avoid duplicate handling; update the form wiring so
handleSubmit handles the submit event from the form (signature event => {
event.preventDefault(); ... }) and keep the Button as type="submit".
🪄 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: e5631419-7127-47c9-957b-cee70da10552
📒 Files selected for processing (8)
src/actions/tax-actions.jssrc/components/forms/tax-type-form.jssrc/layouts/summit-id-layout.jssrc/layouts/tax-type-layout.jssrc/pages/taxes/edit-tax-type-page.jssrc/pages/taxes/popup/tax-type-popup.jssrc/pages/taxes/tax-type-list-page.jssrc/reducers/taxes/tax-type-list-reducer.js
💤 Files with no reviewable changes (2)
- src/pages/taxes/edit-tax-type-page.js
- src/layouts/tax-type-layout.js
✅ Files skipped from review due to trivial changes (1)
- src/reducers/taxes/tax-type-list-reducer.js
🚧 Files skipped from review as they are similar to previous changes (3)
- src/pages/taxes/popup/tax-type-popup.js
- src/actions/tax-actions.js
- src/pages/taxes/tax-type-list-page.js
priscila-moneo
force-pushed
the
feature/move-tax-types-grid-mui
branch
from
e8c64c4 to
9e52a0a
Compare
santipalenque
left a comment
santipalenque
left a comment
There was a problem hiding this comment.
Very clean, good job!
Just need confirmation from @smarcet on the forms migration
|
|
||
| return ( | ||
| <Box component="form" onSubmit={handleSubmit}> | ||
| <input type="hidden" id="id" value={entity.id} /> |
| onSubmit | ||
| }) => { | ||
| const [entity, setEntity] = useState({ ...entityProp }); | ||
| const [errors, setErrors] = useState(errorsProp); |
There was a problem hiding this comment.
@smarcet this legacy approach relies on the API for input validation, should we migrate to formik/yup while migrating to MUI ?
There was a problem hiding this comment.
yes we need to use formik / yup as we are doing with the rest of the MUI forms
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
@priscila-moneo please review comments about formik/yup
Signed-off-by: Priscila Moneo <priscila_moneo@hotmail.com.ar>
priscila-moneo
force-pushed
the
feature/move-tax-types-grid-mui
branch
from
9e52a0a to
dd302b2
Compare
@smarcet Ready! |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/reducers/taxes/tax-type-list-reducer.js (1)
64-70:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
totalTaxTypesnot decremented on delete.When a tax type is deleted,
taxTypesis filtered buttotalTaxTypesremains unchanged. This causes a mismatch between the displayed count and actual items. The list page should refresh after delete (as noted in that file's review) to get the correct count from the server, or the reducer could decrementtotalTaxTypeshere.🔧 Option: Decrement count in reducer
case TAX_TYPE_DELETED: { const { taxTypeId } = payload; return { ...state, - taxTypes: state.taxTypes.filter((t) => t.id !== taxTypeId) + taxTypes: state.taxTypes.filter((t) => t.id !== taxTypeId), + totalTaxTypes: state.totalTaxTypes - 1 }; }Alternatively, ensure the list page refreshes after delete to fetch the updated count from the server.
🤖 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/taxes/tax-type-list-reducer.js` around lines 64 - 70, In the TAX_TYPE_DELETED case of the reducer update both the taxTypes array and totalTaxTypes: after filtering out the deleted item (taxTypes.filter(...)) decrement totalTaxTypes (e.g., totalTaxTypes - 1 or set to Math.max(0, state.totalTaxTypes - 1)) so the count stays in sync with the removed item; ensure you return a new state object and guard against going below zero.
🧹 Nitpick comments (1)
src/pages/taxes/popup/tax-type-popup.js (1)
39-46: 💤 Low valueRemove unused
errorsprop from TaxTypeForm.
TaxTypeFormwas migrated to Formik/Yup validation and no longer accepts anerrorsprop. The prop is being passed here (line 42) but will be ignored.🧹 Suggested cleanup
<TaxTypeForm entity={entity} currentSummit={currentSummit} - errors={errors} onTicketLink={onTicketLink} onTicketUnLink={onTicketUnLink} onSubmit={onSubmit} />Also remove the
errorsprop fromTaxTypePopuppropTypes/defaultProps and the component signature since it's no longer needed.🤖 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/taxes/popup/tax-type-popup.js` around lines 39 - 46, Remove the unused errors prop passed to TaxTypeForm: delete the errors={errors} prop in the TaxTypeForm JSX and then remove the now-unused errors prop from the TaxTypePopup component's props/signature and from its propTypes and defaultProps declarations; ensure no remaining references to errors exist in TaxTypePopup so ESLint won't complain and Formik/Yup validation remains authoritative in TaxTypeForm.
🤖 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/taxes/tax-type-list-page.js`:
- Around line 65-67: handleDelete currently just calls deleteTaxType(taxTypeId)
and doesn't update pagination info; after deletion you must refresh the list and
adjust the current page when needed. Change handleDelete to await the
deleteTaxType call (or use its returned promise) and then either dispatch the
action that reloads the list (e.g., fetchTaxTypes or loadTaxTypes) for the
current page, and if totalTaxTypes % pageSize === 1 and currentPage > 1
decrement currentPage before reloading so users on the last-item page are moved
back; reference handleDelete, deleteTaxType, totalTaxTypes, pageSize/currentPage
(or your pagination state), and the reducer action TAX_TYPE_DELETED to implement
this flow.
---
Outside diff comments:
In `@src/reducers/taxes/tax-type-list-reducer.js`:
- Around line 64-70: In the TAX_TYPE_DELETED case of the reducer update both the
taxTypes array and totalTaxTypes: after filtering out the deleted item
(taxTypes.filter(...)) decrement totalTaxTypes (e.g., totalTaxTypes - 1 or set
to Math.max(0, state.totalTaxTypes - 1)) so the count stays in sync with the
removed item; ensure you return a new state object and guard against going below
zero.
---
Nitpick comments:
In `@src/pages/taxes/popup/tax-type-popup.js`:
- Around line 39-46: Remove the unused errors prop passed to TaxTypeForm: delete
the errors={errors} prop in the TaxTypeForm JSX and then remove the now-unused
errors prop from the TaxTypePopup component's props/signature and from its
propTypes and defaultProps declarations; ensure no remaining references to
errors exist in TaxTypePopup so ESLint won't complain and Formik/Yup validation
remains authoritative in TaxTypeForm.
🪄 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: eede6858-fedc-425f-96ff-43d327e0c689
📒 Files selected for processing (8)
src/actions/tax-actions.jssrc/components/forms/tax-type-form.jssrc/layouts/summit-id-layout.jssrc/layouts/tax-type-layout.jssrc/pages/taxes/edit-tax-type-page.jssrc/pages/taxes/popup/tax-type-popup.jssrc/pages/taxes/tax-type-list-page.jssrc/reducers/taxes/tax-type-list-reducer.js
💤 Files with no reviewable changes (2)
- src/pages/taxes/edit-tax-type-page.js
- src/layouts/tax-type-layout.js
| const handleDelete = (taxTypeId) => { | ||
| deleteTaxType(taxTypeId); | ||
| }; |
There was a problem hiding this comment.
List not refreshed after delete.
handleDelete calls deleteTaxType(taxTypeId) but doesn't refresh the list afterward. While the reducer removes the item from local state via TAX_TYPE_DELETED, the totalTaxTypes count won't be updated, and if the user is on the last page with one item, they'll see an empty page instead of being moved to the previous page.
🔧 Suggested fix
const handleDelete = (taxTypeId) => {
- deleteTaxType(taxTypeId);
+ deleteTaxType(taxTypeId).then(() => {
+ getTaxTypes(term, currentPage, perPage, order, orderDir);
+ });
};🤖 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/taxes/tax-type-list-page.js` around lines 65 - 67, handleDelete
currently just calls deleteTaxType(taxTypeId) and doesn't update pagination
info; after deletion you must refresh the list and adjust the current page when
needed. Change handleDelete to await the deleteTaxType call (or use its returned
promise) and then either dispatch the action that reloads the list (e.g.,
fetchTaxTypes or loadTaxTypes) for the current page, and if totalTaxTypes %
pageSize === 1 and currentPage > 1 decrement currentPage before reloading so
users on the last-item page are moved back; reference handleDelete,
deleteTaxType, totalTaxTypes, pageSize/currentPage (or your pagination state),
and the reducer action TAX_TYPE_DELETED to implement this flow.
3 participants
ref: https://app.clickup.com/t/86b9tggfp
Summary by CodeRabbit
New Features
Refactor