feat: move tax types grid to mui#931
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR converts tax-type management from page-based navigation to a modal workflow: list and form components are rewritten as hooks with MUI, a new TaxTypePopup dialog wraps the form, actions and reducer gain search/pagination/sort support, routing is updated to load the refactored list page, and legacy tax-type layout/edit page files are removed. 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 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
Signed-off-by: Priscila Moneo <priscila_moneo@hotmail.com.ar>
priscila-moneo
force-pushed
the
feature/move-tax-types-grid-mui
branch
from
e8c64c4 to
9e52a0a
Compare
2 participants
ref: https://app.clickup.com/t/86b9tggfp
Summary by CodeRabbit
New Features
Refactor