refactor(admin): flatten to single routed tab bar (#3974)#3975
refactor(admin): flatten to single routed tab bar (#3974)#3975PierreBrisorgueil merged 2 commits intomasterfrom
Conversation
Split `admin.content.vue` into four routed views (Users, Organizations, Readiness, Activity) and mount them as children of the `/admin` parent route. The admin layout now renders a single flat tab bar combining the built-ins with the `config.admin.tabs` extras, so downstream modules contribute siblings instead of being stacked under a separate "General" bar. Global concerns (error banner + mailer warning) moved to the layout to stay visible across every admin tab. - Split `admin.content.vue` → `admin.users.view.vue`, `admin.organizations.view.vue`, `admin.readiness.view.vue`, `admin.activity.view.vue` - Update `admin.router.js`: `''` redirects to `Admin Users`; each section gets a dedicated child route (`users`, `organizations`, `readiness`, `activity`) keeping CASL meta propagation - Update `admin.layout.vue`: merge built-in + extra tabs in a single `<v-tabs>` row, longest-prefix active-tab resolution keeps detail routes (`/admin/users/:id`) highlighting the parent tab - Readiness and Activity fetch on `mounted()` instead of via a nested tab-model watcher - Redispatch unit tests across the four new views + refresh router and layout specs for the new structure - Document the change in `MIGRATIONS.md` (URL-level breaking change; downstream config unchanged)
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 51 minutes and 50 seconds. ⌛ 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: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
WalkthroughRestructures admin navigation from a nested tab UI (General tab containing Users/Organizations/Readiness/Activity in a v-window) to a flat routed architecture with dedicated child routes under Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Router
participant AdminLayout
participant ViewComponent
participant AdminStore
User->>Router: Navigate to /admin/users
Router->>AdminLayout: Render with matched child route
AdminLayout->>AdminLayout: Resolve active tab, render flat tab bar
AdminLayout->>ViewComponent: Mount AdminUsers component via router-view
ViewComponent->>ViewComponent: On mounted() hook
ViewComponent->>AdminStore: Call getUsers()
AdminStore-->>ViewComponent: Return users data
ViewComponent->>ViewComponent: Update computed users from store
ViewComponent-->>User: Render users table
User->>Router: Click Organizations tab
Router->>AdminLayout: Navigate to /admin/organizations
AdminLayout->>ViewComponent: Unmount AdminUsers, mount AdminOrganizations
ViewComponent->>ViewComponent: On mounted() hook
ViewComponent->>AdminStore: Call getOrganizations()
AdminStore-->>ViewComponent: Return organizations data
ViewComponent-->>User: Render organizations table
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
🚥 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3975 +/- ##
=======================================
Coverage 99.48% 99.48%
=======================================
Files 30 30
Lines 968 968
Branches 267 267
=======================================
Hits 963 963
Misses 5 5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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/modules/admin/views/admin.activity.view.vue`:
- Around line 193-202: The fetchActivityLogs method sets activityLoading true
then awaits useAdminStore().getAuditLogs but does not guarantee activityLoading
is reset if getAuditLogs throws; wrap the await call in a try/finally (inside
fetchActivityLogs) so activityLoading is set to true before the try and set to
false in the finally block, leaving the rest of the parameter passing (action,
userId, page, perPage) unchanged; update the fetchActivityLogs function to use
try/finally to ensure the progress state is always cleared.
In `@src/modules/admin/views/admin.readiness.view.vue`:
- Around line 82-86: The fetchReadiness method sets readinessLoading true then
awaits useAdminStore().getReadiness(), but if getReadiness() rejects
readinessLoading stays true; wrap the await in a try/finally so readinessLoading
is always set to false in the finally block. Update the fetchReadiness function
to use try { await useAdminStore().getReadiness(); } finally {
this.readinessLoading = false; } while keeping this.readinessLoading = true at
the start and preserving any existing error propagation or handling.
In `@src/modules/admin/views/admin.users.view.vue`:
- Around line 20-22: The click handler on the v-chip uses m.organizationId?._id
|| m.organizationId?.id and can push "undefined" when m.organizationId is
null/undefined; update the handler in admin.users.view.vue to guard first: check
for a valid id (e.g., const orgId = m.organizationId?._id ||
m.organizationId?.id) and only call $router.push('/admin/organizations/' +
orgId) when orgId is truthy, otherwise either no-op or navigate to a safe
fallback (e.g., organizations list); ensure the guard references the same
m.organizationId lookup used in the template display.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f59a23e7-8ef6-4e93-9c16-3bec0fac0d6a
📒 Files selected for processing (16)
MIGRATIONS.mdsrc/lib/helpers/tests/router.unit.tests.jssrc/modules/admin/router/admin.router.jssrc/modules/admin/tests/admin.activity.view.unit.tests.jssrc/modules/admin/tests/admin.content.unit.tests.jssrc/modules/admin/tests/admin.layout.unit.tests.jssrc/modules/admin/tests/admin.organizations.view.unit.tests.jssrc/modules/admin/tests/admin.readiness.view.unit.tests.jssrc/modules/admin/tests/admin.router.unit.tests.jssrc/modules/admin/tests/admin.users.view.unit.tests.jssrc/modules/admin/views/admin.activity.view.vuesrc/modules/admin/views/admin.content.vuesrc/modules/admin/views/admin.layout.vuesrc/modules/admin/views/admin.organizations.view.vuesrc/modules/admin/views/admin.readiness.view.vuesrc/modules/admin/views/admin.users.view.vue
💤 Files with no reviewable changes (2)
- src/modules/admin/tests/admin.content.unit.tests.js
- src/modules/admin/views/admin.content.vue
There was a problem hiding this comment.
Pull request overview
This PR refactors the Admin module navigation to use a single routed tab bar where built-in admin sections (Users / Organizations / Readiness / Activity) are routed children and render as siblings alongside downstream config.admin.tabs extras.
Changes:
- Split the previous nested
admin.content.vueinto four dedicated routed views and removed the old nested tab content file. - Updated
/adminrouting to redirect to/admin/usersand added explicit child routes for each built-in section. - Updated
admin.layout.vueto render one flat<v-tabs>row (built-ins + validated extras) and moved global alerts (error + mailer warning) into the layout; redistributed unit tests accordingly.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/modules/admin/views/admin.users.view.vue | New routed Users view extracted from old nested admin content. |
| src/modules/admin/views/admin.organizations.view.vue | New routed Organizations view extracted from old nested admin content. |
| src/modules/admin/views/admin.readiness.view.vue | New routed Readiness view; now fetches on mounted(). |
| src/modules/admin/views/admin.activity.view.vue | New routed Activity view; now fetches on mounted() and supports filters/pagination. |
| src/modules/admin/views/admin.layout.vue | Renders a single flat tab row (built-ins + extras) and shows global alerts across all admin tabs. |
| src/modules/admin/views/admin.content.vue | Removed; old nested tab/window implementation deleted. |
| src/modules/admin/router/admin.router.js | Adds built-in child routes + redirect from empty child path to Users. |
| src/modules/admin/tests/admin.users.view.unit.tests.js | New unit tests for Users routed view. |
| src/modules/admin/tests/admin.organizations.view.unit.tests.js | New unit tests for Organizations routed view. |
| src/modules/admin/tests/admin.readiness.view.unit.tests.js | Updated to test the new Readiness routed view. |
| src/modules/admin/tests/admin.activity.view.unit.tests.js | Updated to test the new Activity routed view. |
| src/modules/admin/tests/admin.layout.unit.tests.js | Updated to validate the flattened single-row tab bar behavior and active-tab resolution. |
| src/modules/admin/tests/admin.router.unit.tests.js | Updated route structure + redirect expectations for the new child routes. |
| src/modules/admin/tests/admin.content.unit.tests.js | Removed; tests redistributed to per-view specs. |
| src/lib/helpers/tests/router.unit.tests.js | Updates the admin route fixture used by injectAdminChildren tests to match the new structure. |
| MIGRATIONS.md | Documents the URL-level breaking change and downstream actions. |
| await adminStore.updateUser({ id: item.id || item._id }, { roles: newRoles }); | ||
| await this.fetchUsers(); |
There was a problem hiding this comment.
useAdminStore().updateUser() can throw (the store rethrows on error). As written, an exception here will bubble up from the click handler and can result in an unhandled promise rejection + skipped refresh. Wrap the update/refresh in a try/catch (or handle the rejected Promise) so failures are surfaced via the existing adminStore.error without breaking the UI flow.
| await adminStore.updateUser({ id: item.id || item._id }, { roles: newRoles }); | |
| await this.fetchUsers(); | |
| try { | |
| await adminStore.updateUser({ id: item.id || item._id }, { roles: newRoles }); | |
| await this.fetchUsers(); | |
| } catch (error) { | |
| // The admin store already records the failure state; prevent an unhandled rejection in the UI. | |
| } |
| await useAdminStore().deleteUser({ id: this.deleteDialog.userId }); | ||
| this.deleteDialog.show = false; | ||
| await this.fetchUsers(); |
There was a problem hiding this comment.
useAdminStore().deleteUser() can throw (the store rethrows on error). If deletion fails, this method will throw before closing the dialog and may trigger an unhandled promise rejection. Handle errors here (try/catch) so the dialog/UI state remains consistent and the store error banner can be shown.
| await useAdminStore().deleteUser({ id: this.deleteDialog.userId }); | |
| this.deleteDialog.show = false; | |
| await this.fetchUsers(); | |
| try { | |
| await useAdminStore().deleteUser({ id: this.deleteDialog.userId }); | |
| this.deleteDialog.show = false; | |
| await this.fetchUsers(); | |
| } catch (error) { | |
| // The store handles error state/banner display; keep the dialog open on failure. | |
| } |
| * @param {object} [params] - Optional query params forwarded to the store. | ||
| * @returns {Promise<void>} | ||
| */ | ||
| async fetchUsers(params) { | ||
| await useAdminStore().getUsers(params); |
There was a problem hiding this comment.
The params argument here is documented as an object, but coreDataTableComponent calls fetchAction with a pagination query-string (see tools.pageRequest), and adminStore.getUsers() interpolates params into the URL. Update the JSDoc (and ideally the parameter name/type) to reflect the actual string format to avoid accidental /page/[object Object] calls.
| * @param {object} [params] - Optional query params forwarded to the store. | |
| * @returns {Promise<void>} | |
| */ | |
| async fetchUsers(params) { | |
| await useAdminStore().getUsers(params); | |
| * @param {string} [pageRequest] - Optional pagination/query-string segment forwarded to the store URL builder. | |
| * @returns {Promise<void>} | |
| */ | |
| async fetchUsers(pageRequest) { | |
| await useAdminStore().getUsers(pageRequest); |
| * @param {object} [params] - Optional query params forwarded to the store. | ||
| * @returns {Promise<void>} | ||
| */ | ||
| async fetchOrganizations(params) { | ||
| await useAdminStore().getOrganizations(params); |
There was a problem hiding this comment.
Same as Users: coreDataTableComponent calls fetchAction with a pagination query-string, and adminStore.getOrganizations() interpolates params into the URL. The JSDoc currently says {object}; update it to the correct string type/format (and consider renaming the arg) to prevent misuse.
| * @param {object} [params] - Optional query params forwarded to the store. | |
| * @returns {Promise<void>} | |
| */ | |
| async fetchOrganizations(params) { | |
| await useAdminStore().getOrganizations(params); | |
| * @param {string} [queryString] - Optional pagination query-string fragment forwarded to the store URL. | |
| * @returns {Promise<void>} | |
| */ | |
| async fetchOrganizations(queryString) { | |
| await useAdminStore().getOrganizations(queryString); |
| await wrapper.vm.fetchUsers({ page: 1 }); | ||
| expect(adminStore.getUsers).toHaveBeenCalledWith({ page: 1 }); |
There was a problem hiding this comment.
This test exercises fetchUsers with an object { page: 1 }, but in production coreDataTableComponent passes a pagination query-string (e.g. "0&5&search"), and the store action interpolates that string into the URL. Update the test to pass a representative string so it matches the real contract and doesn’t normalize incorrect usage.
| await wrapper.vm.fetchUsers({ page: 1 }); | |
| expect(adminStore.getUsers).toHaveBeenCalledWith({ page: 1 }); | |
| await wrapper.vm.fetchUsers('0&5&search'); | |
| expect(adminStore.getUsers).toHaveBeenCalledWith('0&5&search'); |
| await wrapper.vm.fetchOrganizations({ page: 2 }); | ||
| expect(adminStore.getOrganizations).toHaveBeenCalledWith({ page: 2 }); |
There was a problem hiding this comment.
Same issue as the Users view test: fetchOrganizations is called with an object in this test, but coreDataTableComponent passes a pagination query-string that is interpolated into the request URL. Update the test to use a representative string value to reflect the real integration contract.
| await wrapper.vm.fetchOrganizations({ page: 2 }); | |
| expect(adminStore.getOrganizations).toHaveBeenCalledWith({ page: 2 }); | |
| await wrapper.vm.fetchOrganizations('?page=2'); | |
| expect(adminStore.getOrganizations).toHaveBeenCalledWith('?page=2'); |
|
|
||
| 1. Run `/update-stack` to pull the changes. | ||
| 2. Verify hard-coded links in your project. The old URL `/admin` used to | ||
| land on the nested "General" tab; it now 301-redirects to |
There was a problem hiding this comment.
This describes the /admin → /admin/users change as a "301" redirect, but the implementation shown in this repo is a client-side vue-router redirect (not an HTTP 301). Unless there is also a server-level 301 configured elsewhere, consider rewording to "redirects" / "client-side redirects" to avoid misleading downstream maintainers.
| land on the nested "General" tab; it now 301-redirects to | |
| land on the nested "General" tab; it now redirects to |
| ><v-chip | ||
| v-for="m in item.memberships || []" | ||
| :key="m._id || m.id" | ||
| size="small" | ||
| :variant="isUserActiveOrg(item, m) ? 'flat' : 'tonal'" | ||
| :color="orgColor(m.organizationId)" | ||
| class="mr-1 text-capitalize" | ||
| style="cursor: pointer" | ||
| @click="$router.push('/admin/organizations/' + (m.organizationId?._id || m.organizationId?.id))" | ||
| >{{ (m.organizationId && m.organizationId.name) || '—' }} ({{ m.role || '—' }})</v-chip |
There was a problem hiding this comment.
The organization membership chip is always clickable, but when m.organizationId is missing/null the click handler will navigate to /admin/organizations/undefined. Consider deriving an orgId first and only enabling navigation (and the pointer cursor) when it exists; otherwise render a non-clickable chip/state.
| ><v-chip | |
| v-for="m in item.memberships || []" | |
| :key="m._id || m.id" | |
| size="small" | |
| :variant="isUserActiveOrg(item, m) ? 'flat' : 'tonal'" | |
| :color="orgColor(m.organizationId)" | |
| class="mr-1 text-capitalize" | |
| style="cursor: pointer" | |
| @click="$router.push('/admin/organizations/' + (m.organizationId?._id || m.organizationId?.id))" | |
| >{{ (m.organizationId && m.organizationId.name) || '—' }} ({{ m.role || '—' }})</v-chip | |
| ><template v-for="m in item.memberships || []" :key="m._id || m.id" | |
| ><v-chip | |
| v-if="m.organizationId?._id || m.organizationId?.id" | |
| size="small" | |
| :variant="isUserActiveOrg(item, m) ? 'flat' : 'tonal'" | |
| :color="orgColor(m.organizationId)" | |
| class="mr-1 text-capitalize" | |
| style="cursor: pointer" | |
| @click="$router.push('/admin/organizations/' + (m.organizationId?._id || m.organizationId?.id))" | |
| >{{ (m.organizationId && m.organizationId.name) || '—' }} ({{ m.role || '—' }})</v-chip | |
| ><v-chip | |
| v-else | |
| size="small" | |
| :variant="isUserActiveOrg(item, m) ? 'flat' : 'tonal'" | |
| :color="orgColor(m.organizationId)" | |
| class="mr-1 text-capitalize" | |
| >{{ (m.organizationId && m.organizationId.name) || '—' }} ({{ m.role || '—' }})</v-chip | |
| ></template |
- Wrap readiness/activity fetch loading flags in try/finally so the progress bars never stay pinned on store rejection. - Wrap admin.users store mutations in try/catch and guard membership chip navigation against undefined organization ids. - Clarify JSDoc for fetchUsers / fetchOrganizations to reflect the pagination query-string contract used by coreDataTableComponent. - Update unit tests to pass the documented string param to the fetch wrappers instead of an ad-hoc object. - Reword MIGRATIONS.md to describe the /admin redirect as client-side via vue-router (it is not an HTTP 301).
PierreBrisorgueil
dismissed
coderabbitai[bot]’s stale review
All 3 inline findings already addressed in commit 1884b09 (loading-flag try/finally + undefined-orgId guard). CodeRabbit acknowledged the fixes via subsequent COMMENTED reviews — dismissing stale CHANGES_REQUESTED to unblock merge.
Summary
Resolves #3974. Flattens the admin section to a single routed tab bar so the built-in Users / Organizations / Readiness / Activity tabs become siblings of the downstream extras from
config.admin.tabs, instead of being stacked under a separate "General" row.admin.content.vue(488 lines) into four dedicated routed views:admin.users.view.vue,admin.organizations.view.vue,admin.readiness.view.vue,admin.activity.view.vue.admin.router.jsnow mounts each section as a child of the/adminparent route (
users,organizations,readiness,activity). Theempty child
''redirects to{ name: 'Admin Users' }so existing/adminlinks keep working. CASL meta propagates as before.admin.layout.vuerenders a single<v-tabs>row combining built-intabs with the validated
config.admin.tabsextras. Longest-prefixactive-tab resolution keeps detail routes (
/admin/users/:id)highlighting the parent tab.
admin.content.vueinto the layout so they stay visible across everyadmin tab, including downstream extras (bonus UX).
mounted()instead of via anested tab-model
watch.admin.content.unit.tests.jsacross fourper-view spec files and updated
admin.router.unit.tests.jsandadmin.layout.unit.tests.jsfor the new structure.Breaking change (URL-level)
Documented in
MIGRATIONS.md. The old URL/adminused to land on thenested General tab and now 301-redirects to
/admin/users. New stableURLs:
/admin/users,/admin/organizations,/admin/readiness,/admin/activity.No config change required for downstream projects contributing
extras via
config.admin.tabs+injectAdminChildren. Projects thatoverride
admin.content.vuemust delete that override (file removed);replace with per-view overrides or a custom tab instead.
Test plan
npm run test:unit— 1023 passednpm run test:coverage— 98.99% statements / 93.68% branches, no thresholds lowerednpm run lint— cleannpm run build— successtrawl.me/adminafter downstream deploys/update-stackon downstream Vue projects that useconfig.admin.tabs(trawl_vue)Summary by CodeRabbit
New Features
/admin/users), enabling deep-linking and bookmarking.Improvements