feat: move admin access group grid to mui#911
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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 (8)
📝 WalkthroughWalkthroughThis PR refactors the admin access module from a multi-page CRUD flow to a dialog-based single-page pattern. The list page now handles both viewing and editing via a modal dialog, with supporting changes to action creators (pagination params, conditional redirect), form UI (Bootstrap to MUI), layout routing (simplified single route), and reducer state tracking (pagination totals, deletion count updates). The dedicated edit page is removed as its functionality is consolidated into the list page dialog. ChangesAdmin Access Module Dialog-Based Refactor
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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Review rate limit: 0/1 reviews remaining, refill in 38 minutes and 3 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/layouts/admin-access-layout.js`:
- Around line 30-36: The current Route path (`path={`${match.url}/:access_id?`}`
in the Switch) is too permissive and treats any single-segment suffix as an edit
route; replace it with explicit routes: add a Route for the "new" page using
`${match.url}/new` (rendering AdminAccessListPage or AdminAccessForm as
appropriate) and add a separate Route for numeric IDs using a constrained param
like `${match.url}/:access_id(\\d+)` that renders AdminAccessListPage for
editing; keep the Redirect to `match.url` after those Routes and ensure the
order is: new route, numeric-id route, then Redirect so invalid segments still
fall through to the redirect.
In `@src/pages/admin_access/admin-access-list-page.js`:
- Around line 63-79: The getAdminAccess(accessId).then(() => setOpen(true)) call
can reopen the modal with stale data; modify the effect around
useEffect/getAdminAccess to guard against late responses by sequencing or
cancellation: capture the current accessId/isNew (from match.params.access_id
and a flag from /new) or create a request token/AbortController before calling
getAdminAccess, and when the promise resolves verify the token matches the
latest accessId (and still not isNew) before calling setOpen(true) and applying
fetched data (or abort the fetch). Also ensure resetAdminAccessForm is only
applied for the intended /new route by checking the same guard.
- Around line 106-112: handleDeleteAdminAccess currently performs an optimistic
delete via deleteAdminAccess but never refetches the paginated data, leaving
currentPage pointing at an empty page after deleting the last item; change the
flow so that after deleteAdminAccess resolves (or in its success
callback/promise then), call the pagination refetch function (e.g.,
fetchAdminAccessPage or refetchAdminAccess) for the currentPage, and if the
returned page is empty and currentPage > 1, decrement currentPage and refetch
the previous page; update the state that holds currentPage and the page data
accordingly instead of relying only on the reducer’s local filter.
🪄 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: f9c84e8b-8dbe-47a6-856f-4041b0d8ca43
📒 Files selected for processing (8)
src/actions/admin-access-actions.jssrc/components/forms/admin-access-form.jssrc/layouts/admin-access-layout.jssrc/pages/admin_access/__tests__/admin-access-list-page.test.jssrc/pages/admin_access/admin-access-list-page.jssrc/pages/admin_access/edit-admin-access-page.jssrc/reducers/admin_access/__tests__/admin-access-list-reducer.test.jssrc/reducers/admin_access/admin-access-list-reducer.js
💤 Files with no reviewable changes (1)
- src/pages/admin_access/edit-admin-access-page.js
| useEffect(() => { | ||
| const { access_id: accessId } = match.params; | ||
| const isNew = /\/new$/.test(history.location.pathname); | ||
|
|
||
| this.state = {}; | ||
| } | ||
| if (isNew) { | ||
| resetAdminAccessForm(); | ||
| setOpen(true); | ||
| return; | ||
| } | ||
|
|
||
| componentDidMount() { | ||
| this.props.getAdminAccesses(); | ||
| } | ||
| if (accessId) { | ||
| getAdminAccess(accessId).then(() => setOpen(true)); | ||
| return; | ||
| } | ||
|
|
||
| handleEdit(admin_access_id) { | ||
| const { history } = this.props; | ||
| history.push(`/app/admin-access/${admin_access_id}`); | ||
| } | ||
| setOpen(false); | ||
| }, [match.params.access_id, history.location.pathname]); |
There was a problem hiding this comment.
Guard against stale edit loads.
getAdminAccess(accessId).then(() => setOpen(true)) has no stale-request check. If the user closes the dialog or switches from /app/admin-access/1 to /app/admin-access/new before the fetch finishes, the late response can reopen the modal and repopulate the form with the old record. This needs request sequencing/cancellation, or at least a route/id guard before applying the result.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/admin_access/admin-access-list-page.js` around lines 63 - 79, The
getAdminAccess(accessId).then(() => setOpen(true)) call can reopen the modal
with stale data; modify the effect around useEffect/getAdminAccess to guard
against late responses by sequencing or cancellation: capture the current
accessId/isNew (from match.params.access_id and a flag from /new) or create a
request token/AbortController before calling getAdminAccess, and when the
promise resolves verify the token matches the latest accessId (and still not
isNew) before calling setOpen(true) and applying fetched data (or abort the
fetch). Also ensure resetAdminAccessForm is only applied for the intended /new
route by checking the same guard.
|
|
||
| dispatch( | ||
| showMessage(successMessage, () => { | ||
| history.push(`/app/admin-access/${payload.response.id}`); |
There was a problem hiding this comment.
redirectOnCreate if should go here, if not the noAlert flag is redundant
| ? `${member.first_name} ${member.last_name} (${member.email})` | ||
| : `${member.first_name} ${member.last_name} (${member.id})`; | ||
| }} | ||
| : `${member.first_name} ${member.last_name} (${member.id})`} |
There was a problem hiding this comment.
if the only thing that changes is the parenthesis, then the ternary condition should go there
| path={`${match.url}/:access_id?`} | ||
| component={AdminAccessListPage} | ||
| /> | ||
| <Redirect to={match.url} /> |
There was a problem hiding this comment.
before the routes would only match on / or digit, or new. Now it matches everything , for example app/admin-access/santi. it is not equivalent to what we had
| this.handleDeleteAdminAccess = this.handleDeleteAdminAccess.bind(this); | ||
| useEffect(() => { | ||
| const { access_id: accessId } = match.params; | ||
| const isNew = /\/new$/.test(history.location.pathname); |
There was a problem hiding this comment.
so what if the url is app/admin-access/santi/new ?
| const totalItems = | ||
| typeof totalAdminAccesses === "number" | ||
| ? totalAdminAccesses | ||
| : admin_accesses.length; |
There was a problem hiding this comment.
admin_accesses is paginated, the length of this will rarely be the total
priscila-moneo
force-pushed
the
feature/move-admin-access-group-grid-mui
branch
from
2c47f00 to
e24b38c
Compare
2 participants
ref: https://app.clickup.com/t/86b9n7qe1
Summary by CodeRabbit
New Features
Style
Bug Fixes