feat: export Roles page as a component and consume it in apps/admin #1349
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
paanSinghCoder
changed the title
Pull Request Test Coverage Report for Build 21816871256Details
💛 - Coveralls |
paanSinghCoder
force-pushed
the
refactor/admin-ui-child
branch
from
e887b1e to
ec4b710
Compare
…ent and related files
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis PR refactors the roles management UI to use controlled component patterns instead of router-driven hooks, introduces new reusable header components (PageHeader, PageTitle, SheetHeader), updates dependency resolution to workspace reference, and exposes the admin library as a public export. Changes
Sequence DiagramsequenceDiagram
participant User
participant RolesPage
participant RolesView
participant RoleDetails
User->>RolesPage: Navigate to /roles or /roles/:id
RolesPage->>RolesPage: Extract roleId from URL params
RolesPage->>RolesView: Pass selectedRoleId & onSelectRole callback
User->>RolesView: Click on role in list
RolesView->>RolesView: handleRowClick(roleId)
RolesView->>RolesPage: onSelectRole(roleId)
RolesPage->>RolesPage: navigate(/roles/:id)
RolesPage->>RolesView: Update selectedRoleId prop
RolesView->>RolesView: Lookup role from data map
RolesView->>RoleDetails: Pass resolved role object
RoleDetails->>RoleDetails: Render role details
User->>RolesView: Click close on detail sheet
RolesView->>RolesPage: onCloseDetail()
RolesPage->>RolesPage: navigate(/roles)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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
🤖 Fix all issues with AI agents
In `@web/lib/admin/components/SheetHeader.tsx`:
- Around line 11-31: Replace the clickable Cross1Icon in SheetHeader with the
IconButton component from `@raystack/apsara` so the close control has proper
button semantics and an accessible name; update the import to include
IconButton, render IconButton (passing the Cross1Icon as its icon prop or
children), move the onClick handler from Cross1Icon to IconButton, add
aria-label (e.g., "Close sheet") and keep data-testid usage (use dataTestId or
default "admin-close-btn") to preserve tests.
In `@web/lib/admin/views/roles/index.tsx`:
- Around line 36-40: handleRowClick inconsistently maps Role.id to "" in the
onSelectRole branch but to undefined in the setInternalRoleId branch; make both
branches pass the same type (prefer leaving id as-is or using undefined
consistently). Update the handleRowClick callback to call onSelectRole(role.id)
(or onSelectRole(role.id ?? undefined) if callback expects string|undefined) and
setInternalRoleId(role.id) so both branches use role.id (string | undefined)
consistently; reference the handleRowClick function, the onSelectRole prop,
setInternalRoleId, and the Role.id field when making the change.
In `@web/tools/eslint-config/index.js`:
- Around line 2-7: The extendsList array currently omits 'turbo' while
package.json still lists eslint-config-turbo and client-demo/.eslintrc.js uses
the turbo/no-undeclared-env-vars rule; either re-add 'turbo' into the
extendsList (add 'turbo' to the extendsList constant so the turbo rules are
applied) or remove the eslint-config-turbo dependency from
web/tools/eslint-config/package.json and update client-demo/.eslintrc.js to stop
referencing turbo rules; locate and edit the extendsList constant and
package.json dependency to keep them consistent with the .eslintrc.js usage.
🧹 Nitpick comments (9)
web/tools/eslint-config/index.js (1)
2-7: Nitpick: Inconsistent quote style.Line 5 uses double quotes (
"eslint:recommended") while the other entries use single quotes. Consider using consistent quoting for readability.🔧 Suggested fix
const extendsList = [ 'next', 'prettier', - "eslint:recommended", + 'eslint:recommended', 'plugin:test-selectors/recommended' ];web/lib/admin/utils/helper.ts (1)
1-11: Avoid repeated object spreads inreduceByKey.
Current approach re-allocates the accumulator each iteration; mutating the accumulator reduces allocations on large arrays.♻️ Proposed refactor
export function reduceByKey<T extends Record<string, unknown>>( data: T[], key: keyof T ): Record<string, T> { - return data.reduce( - (acc, value) => ({ - ...acc, - [String(value[key])]: value, - }), - {} as Record<string, T> - ); + return data.reduce((acc, value) => { + acc[String(value[key])] = value; + return acc; + }, {} as Record<string, T>); }web/lib/admin/views/roles/details.tsx (1)
4-12: Consider a null guard to avoid empty detail rendering.
Ifroleis null, the component currently renders empty fields; an early return (or explicit empty state) keeps the UI cleaner and removes optional chaining.♻️ Suggested guard
export default function RoleDetails({ role }: { role: Role | null }) { + if (!role) return null; return ( <Flex direction="column" gap={9}> - <Text size={4}>{role?.name}</Text> + <Text size={4}>{role.name}</Text> <Flex direction="column" gap={9}> <Grid columns={2} gap="small"> <Text size={1}>Name</Text> - <Text size={1}>{role?.name}</Text> + <Text size={1}>{role.name}</Text> </Grid> </Flex> </Flex> ); }web/lib/admin/components/PageTitle.tsx (1)
8-16: Preserve the previous document title on unmount.
The cleanup currently resets toappName/empty, which can overwrite a prior title set elsewhere.♻️ Suggested adjustment
-import { useEffect } from "react"; +import { useEffect, useRef } from "react"; @@ export function PageTitle({ title, appName }: PageTitleProps) { const fullTitle = title && appName ? `${title} | ${appName}` : title ?? appName ?? ""; + const prevTitleRef = useRef<string | null>(null); useEffect(() => { + if (prevTitleRef.current === null) { + prevTitleRef.current = document.title; + } if (fullTitle) document.title = fullTitle; return () => { - document.title = appName ?? ""; + if (prevTitleRef.current !== null) { + document.title = prevTitleRef.current; + } }; - }, [fullTitle, appName]); + }, [fullTitle]); @@ return null; }web/lib/admin/views/roles/columns.tsx (1)
31-36: Prefer stable keys for permissions list items.
Line 33-34 uses the array index as the key; if permissions reorder, React can reuse DOM incorrectly. Consider keying by permission value (or a composite key if duplicates are possible).♻️ Suggested tweak
- {((info.getValue() as string[]) || []).map((p, i) => ( - <span key={i}>{p}</span> - ))} + {((info.getValue() as string[]) || []).map(p => ( + <span key={p}>{p}</span> + ))}web/lib/admin/components/PageHeader.tsx (2)
5-10: Breadcrumbhrefproperty is defined but never used.The
PageHeaderTypesdefineshref?: stringfor breadcrumb items, but the component renders plain<span>elements (lines 32-34) instead of anchor tags. The CSS also has styles for.breadcrumb athat won't apply.Either remove the
hrefproperty from the type if links aren't needed, or render anchors whenhrefis provided:♻️ Proposed fix to support href
{breadcrumb.map((item) => ( - <span key={item.name} className={styles.breadcrumbItem}> - {item.name} - </span> + item.href ? ( + <a key={item.name} href={item.href}> + {item.name} + </a> + ) : ( + <span key={item.name} className={styles.breadcrumbItem}> + {item.name} + </span> + ) ))}
31-35: Consider a more robust key for breadcrumb items.Using
item.nameas the key could cause issues if breadcrumb items have duplicate names. Consider using the index as a fallback:♻️ Proposed fix
- {breadcrumb.map((item) => ( - <span key={item.name} className={styles.breadcrumbItem}> + {breadcrumb.map((item, index) => ( + <span key={`${item.name}-${index}`} className={styles.breadcrumbItem}>web/lib/admin/components/page-header.module.css (2)
9-27: Anchor styles are currently unused.The
.breadcrumb aselectors (lines 9-27) define styles for anchor tags, but thePageHeadercomponent currently renders<span>elements with the.breadcrumbItemclass. These styles won't apply until the component is updated to render anchors whenhrefis provided.
11-11: Consider using design tokens instead of hardcoded colors.Hardcoded color values like
#007bffand#666may not align with the design system. If@raystack/apsaraprovides CSS variables or tokens, consider using them for consistency.Also applies to: 18-18, 35-35
3 participants
@raystack/frontier/adminapps/admin/src/containers/roles.list/intoweb/lib/admin/views/roles/as a router-agnostic page.@raystack/frontier/adminwith optional props: selectedRoleId, onSelectRole, onCloseDetail, appName.lib/adminshared pieces: utils (helper, connect-timestamp), components (PageTitle, SheetHeader, PageHeader) and CSS modules.RolesPageWithRouter(uses useParams/useNavigate) and routes roles + roles/:roleId.containers/roles.listfrom the app; roles are now implemented in the lib.Note: To test this in local, change frontier version to
"@raystack/frontier": "workspace:^",infrontier/web/apps/admin/package.jsonTechnical Details
Test Plan
Summary by CodeRabbit
New Features
Refactor
Chores