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 12 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 21628546856Details
💛 - Coveralls |
paanSinghCoder
force-pushed
the
refactor/admin-ui-child
branch
from
e887b1e to
ec4b710
Compare
…ent and related files
web/apps/admin/src/routes.tsx
Outdated
| <Route path=":roleId" element={<RoleDetails />} /> | ||
| </Route> | ||
| <Route path="roles" element={<RolesPageWithRouter />} /> | ||
| <Route path="roles/:roleId" element={<RolesPageWithRouter />} /> |
|
|
||
| .breadcrumb a { | ||
| text-decoration: none; | ||
| color: #007bff; |
There was a problem hiding this comment.
Out of scope for this PR. Will be picked up in another ticket.
| padding: 0; | ||
| margin: 0; | ||
| display: flex; | ||
| font-size: 16px; |
There was a problem hiding this comment.
Out of scope for this PR. Will be picked up in another ticket.
|
|
||
| .breadcrumb a::before { | ||
| content: ">"; | ||
| color: #666; |
There was a problem hiding this comment.
Out of scope for this PR. Will be picked up in another ticket.
|
|
||
| .breadcrumbItem::before { | ||
| content: ">"; | ||
| color: #666; |
There was a problem hiding this comment.
Out of scope for this PR. Will be picked up in another ticket.
| align="center" | ||
| justify="between" | ||
| className={className} | ||
| style={{ padding: "16px 24px", ...style }} |
There was a problem hiding this comment.
Out of scope for this PR. Will be picked up in another ticket.
| > | ||
| <Flex align="center" gap="medium"> | ||
| <Flex align="center" gap="small" className={styles.breadcrumb}> | ||
| <Text style={{ fontSize: "14px", fontWeight: "500" }}>{title}</Text> |
There was a problem hiding this comment.
fontWeight and fontSize can be props
There was a problem hiding this comment.
Out of scope for this PR. Will be picked up in another ticket.
| export function SheetHeader({ title, onClick, "data-testid": dataTestId }: SheetHeaderProps) { | ||
| return ( | ||
| <Flex justify="between" style={styles.header}> | ||
| <Text size={4} style={{ fontWeight: "500" }}> |
There was a problem hiding this comment.
Out of scope for this PR. Will be picked up in another ticket.
|
@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
| export function SheetHeader({ title, onClick, "data-testid": dataTestId }: SheetHeaderProps) { | ||
| return ( | ||
| <Flex justify="between" style={styles.header}> | ||
| <Text size={4} style={{ fontWeight: "500" }}> | ||
| {title} | ||
| </Text> | ||
| <Cross1Icon | ||
| onClick={onClick} | ||
| style={{ cursor: "pointer" }} | ||
| data-testid={dataTestId ?? "admin-close-btn"} | ||
| /> | ||
| </Flex> | ||
| ); | ||
| } | ||
|
|
||
| const styles: { header: CSSProperties } = { | ||
| header: { | ||
| padding: "18px 32px", | ||
| borderBottom: "1px solid var(--rs-color-border-base-primary)", | ||
| }, | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the SheetHeader.tsx file
find . -name "SheetHeader.tsx" -type f | head -20Repository: raystack/frontier
Length of output: 103
🏁 Script executed:
# Search for Cross1Icon component definition
find . -type f -name "*.tsx" -o -name "*.ts" | xargs grep -l "Cross1Icon" | head -10Repository: raystack/frontier
Length of output: 408
🏁 Script executed:
# Look for any accessibility-related attributes or patterns in the file
rg "role|aria-|keyboard" web/lib/admin/components/SheetHeader.tsx -A 3 -B 3Repository: raystack/frontier
Length of output: 43
🏁 Script executed:
# Read the SheetHeader.tsx file
cat -n web/lib/admin/components/SheetHeader.tsxRepository: raystack/frontier
Length of output: 1078
🏁 Script executed:
# Search for Cross1Icon import and definition
rg "import.*Cross1Icon|from.*@radix" web/lib/admin/components/SheetHeader.tsx -A 2 -B 2Repository: raystack/frontier
Length of output: 204
🏁 Script executed:
# Look for Cross1Icon in the entire codebase to understand its source
rg "Cross1Icon" --type tsx --type ts -B 2 | head -50Repository: raystack/frontier
Length of output: 88
🏁 Script executed:
# Search for other uses of Cross1Icon or similar icon patterns in the codebase
rg "Cross1Icon|Icon.*onClick" -A 3 -B 1 | head -60Repository: raystack/frontier
Length of output: 4304
🏁 Script executed:
# Look for button patterns with icons in the codebase
rg "<button.*aria-label|type=\"button\".*Icon" -A 2 -B 2 | head -80Repository: raystack/frontier
Length of output: 43
🏁 Script executed:
# Check if there are accessibility patterns or components for icon buttons
rg "IconButton|close.*button|aria-label" web/lib/admin/components -A 2 -B 2Repository: raystack/frontier
Length of output: 43
🏁 Script executed:
# Search for IconButton component definition and usage
rg "IconButton" web/ -A 5 -B 2 | head -100Repository: raystack/frontier
Length of output: 7568
🏁 Script executed:
# Check if `@raystack/apsara` has IconButton documentation or types
find . -name "*.tsx" -o -name "*.ts" | xargs grep -l "export.*IconButton\|from.*apsara.*IconButton" | head -10Repository: raystack/frontier
Length of output: 43
🏁 Script executed:
# Look for tests related to SheetHeader
find . -name "*SheetHeader*test*" -o -name "*SheetHeader*.spec.*"Repository: raystack/frontier
Length of output: 43
🏁 Script executed:
# Search for any related accessibility tests or patterns
rg "admin-close-btn|testid.*close" -A 3 -B 3 | head -80Repository: raystack/frontier
Length of output: 988
🏁 Script executed:
# Check the imports available in SheetHeader.tsx and if IconButton can be imported
rg "from.*@raystack/apsara" web/lib/admin/components/SheetHeader.tsxRepository: raystack/frontier
Length of output: 107
🏁 Script executed:
# Look at how IconButton is imported in other files to understand the pattern
rg "import.*IconButton.*@raystack/apsara" web/apps/admin/src/ -B 2 | head -30Repository: raystack/frontier
Length of output: 816
🏁 Script executed:
# Check if SheetHeader component is used in tests and what accessibility expectations exist
find . -path "*/test*" -o -path "*/spec*" | xargs grep -l "SheetHeader\|admin-close-btn" 2>/dev/nullRepository: raystack/frontier
Length of output: 45
🏁 Script executed:
# Check if IconButton can be imported and used in SheetHeader
grep -r "export.*IconButton" web/lib/admin/ 2>/dev/null | head -20Repository: raystack/frontier
Length of output: 43
🏁 Script executed:
# Look at an actual IconButton usage to understand required props
rg "IconButton" web/apps/admin/src/pages/invoices/list/navbar.tsx -A 5 -B 2 | head -40Repository: raystack/frontier
Length of output: 638
🏁 Script executed:
# Check if there's a pattern of using native button vs IconButton in headers/close actions
rg "<button.*close|<IconButton.*close" -A 3 -B 3 | head -60Repository: raystack/frontier
Length of output: 43
🏁 Script executed:
# Verify `@raystack/apsara` exports IconButton from web/lib
find web/lib -name "*.ts" -o -name "*.tsx" | xargs grep "export.*IconButton" 2>/dev/nullRepository: raystack/frontier
Length of output: 45
Use IconButton for the close action to meet accessibility requirements.
The Cross1Icon lacks button semantics and an accessible name, making it inaccessible to keyboard users. Use the IconButton component from @raystack/apsara (already available in this file's dependencies and used throughout the codebase) with an aria-label instead of attaching onClick directly to the icon.
♿ Proposed fix
export function SheetHeader({ title, onClick, "data-testid": dataTestId }: SheetHeaderProps) {
return (
<Flex justify="between" style={styles.header}>
<Text size={4} style={{ fontWeight: "500" }}>
{title}
</Text>
- <Cross1Icon
+ <IconButton
+ aria-label="Close"
onClick={onClick}
- style={{ cursor: "pointer" }}
data-testid={dataTestId ?? "admin-close-btn"}
- />
+ >
+ <Cross1Icon />
+ </IconButton>
</Flex>
);
}Also update the import to include IconButton:
-import { Flex, Text } from "@raystack/apsara";
+import { Flex, Text, IconButton } from "@raystack/apsara";🧰 Tools
🪛 GitHub Check: JS SDK Lint
[warning] 17-17:
Elements with an onClick handler must have a data-test-id attribute
🤖 Prompt for AI Agents
In `@web/lib/admin/components/SheetHeader.tsx` around lines 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.
| const handleRowClick = useCallback( | ||
| (role: Role) => | ||
| onSelectRole ? onSelectRole(role.id ?? "") : setInternalRoleId(role.id ?? undefined), | ||
| [onSelectRole] | ||
| ); |
There was a problem hiding this comment.
Inconsistent handling of undefined role ID.
Line 38 converts role.id to empty string (role.id ?? ""), while line 38 in the else branch uses role.id ?? undefined. This inconsistency could cause subtle bugs if consumers compare selectedRoleId values.
🐛 Proposed fix for consistency
const handleRowClick = useCallback(
(role: Role) =>
- onSelectRole ? onSelectRole(role.id ?? "") : setInternalRoleId(role.id ?? undefined),
+ onSelectRole ? onSelectRole(role.id ?? "") : setInternalRoleId(role.id),
[onSelectRole]
);Note: role.id is already string | undefined, so ?? undefined is redundant.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleRowClick = useCallback( | |
| (role: Role) => | |
| onSelectRole ? onSelectRole(role.id ?? "") : setInternalRoleId(role.id ?? undefined), | |
| [onSelectRole] | |
| ); | |
| const handleRowClick = useCallback( | |
| (role: Role) => | |
| onSelectRole ? onSelectRole(role.id ?? "") : setInternalRoleId(role.id), | |
| [onSelectRole] | |
| ); |
🤖 Prompt for AI Agents
In `@web/lib/admin/views/roles/index.tsx` around lines 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.
| const extendsList = [ | ||
| 'next', | ||
| 'prettier', | ||
| "eslint:recommended", | ||
| 'plugin:test-selectors/recommended' | ||
| ]; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if turbo ESLint config is still referenced elsewhere or if there are turbo-specific rules in use.
# Search for any references to eslint-config-turbo or turbo eslint rules
rg -n "turbo" --type js --type json -g "!node_modules" -g "!*.lock"Repository: raystack/frontier
Length of output: 843
🏁 Script executed:
cat -n web/tools/eslint-config/index.jsRepository: raystack/frontier
Length of output: 660
🏁 Script executed:
cat -n web/apps/client-demo/.eslintrc.jsRepository: raystack/frontier
Length of output: 242
Remove the unused eslint-config-turbo dependency or restore 'turbo' to the extends list.
The removal of 'turbo' from the extends list is problematic: web/tools/eslint-config/package.json still lists eslint-config-turbo as a dependency, and web/apps/client-demo/.eslintrc.js actively configures the turbo/no-undeclared-env-vars rule. Either restore 'turbo' to the extends list or remove the unused dependency from package.json.
🤖 Prompt for AI Agents
In `@web/tools/eslint-config/index.js` around lines 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.
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