feat: export Roles page as a component and consume it in apps/admin#1349
feat: export Roles page as a component and consume it in apps/admin#1349paanSinghCoder wants to merge 15 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
paanSinghCoder
changed the title
Pull Request Test Coverage Report for Build 21900536870Details
💛 - Coveralls |
paanSinghCoder
force-pushed
the
refactor/admin-ui-child
branch
from
e887b1e to
ec4b710
Compare
…ent and related files
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughCentralizes roles list/detail routing via a new RolesPage wired to a controlled RolesView, adds reusable header/title/sheet components, introduces admin build/export artifacts and tsup config, adds utilities (timestamp, reduceByKey), UI tweaks (permissions layout), minor ESLint/package.json adjustments, and removes local admin .eslintrc. Changes
Sequence DiagramsequenceDiagram
participant User
participant RolesPage
participant RolesView
participant RoleDetails
User->>RolesPage: Navigate to /roles or /roles/:roleId
RolesPage->>RolesPage: read roleId from URL params
RolesPage->>RolesView: render with selectedRoleId & onSelectRole/onCloseDetail
User->>RolesView: click role row
RolesView->>RolesView: handleRowClick -> determine roleId
RolesView->>RolesPage: call onSelectRole(roleId)
RolesPage->>RolesPage: navigate to /roles/:roleId
RolesPage->>RolesView: selectedRoleId updated
RolesView->>RoleDetails: pass resolved role prop
RoleDetails->>RoleDetails: render details
User->>RoleDetails: click close
RoleDetails->>RolesView: trigger onCloseDetail
RolesView->>RolesPage: onCloseDetail()
RolesPage->>RolesPage: navigate to /roles
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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