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 merged 13 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 22052630203Details
💛 - Coveralls |
paanSinghCoder
force-pushed
the
refactor/admin-ui-child
branch
from
e887b1e to
ec4b710
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
…ent and related files
paanSinghCoder
force-pushed
the
refactor/admin-ui-child
branch
from
4df8c9d to
0ed835d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
web/lib/admin/views/roles/index.tsx (1)
56-68: Consider moving error logging to avoid repeated logs on re-render.The
console.erroron line 57 executes on every render whileisErroris true. For a cleaner approach, consider using auseEffectto log the error only once when it occurs.♻️ Optional: Log error once using useEffect
+import { useCallback, useState, useEffect } from "react"; ... + useEffect(() => { + if (isError && error) { + console.error("ConnectRPC Error:", error); + } + }, [isError, error]); + if (isError) { - console.error("ConnectRPC Error:", error); return ( <EmptyStateweb/Makefile (1)
5-7: Reusedeptarget for consistency.Keeps install logic centralized and avoids duplicating the install command.
Proposed tweak
-build-admin: - `@pnpm` install && pnpm exec turbo run build --filter=admin +build-admin: dep + `@pnpm` exec turbo run build --filter=admin
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/lib/admin/views/invoices/navbar.tsx (1)
7-7:⚠️ Potential issue | 🟡 MinorTypo: Component name should be
InvoicesNavbar.The component is named
InvoicesNavabar(extra 'a' in Navbar). This needs to be renamed in 3 locations:
- Export declaration in
navbar.tsxline 7- Import in
index.tsxline 10- Usage in
index.tsxline 132
🤖 Fix all issues with AI agents
In `@web/apps/admin/src/pages/audit-logs/AuditLogsPage.tsx`:
- Around line 1-18: The onExportCsv handler in AuditLogsPage currently uses an
unsafe double-cast (query as unknown as RQLExportRequest); replace this with an
explicit type alignment or transformation: either change the onExportCsv
signature to accept RQLExportRequest directly, or map the incoming RQLRequest
into a properly shaped RQLExportRequest (e.g., build a new object or use your
existing create(SchemaDefinition, {...}) helper) before calling
exportCsvFromStream with adminClient.exportAuditRecords. Ensure you reference
AuditLogsPage, onExportCsv, exportCsvFromStream, and
adminClient.exportAuditRecords and remove the double-cast so the request shape
matches RQLExportRequest at compile time.
In `@web/lib/admin/utils/connect-timestamp.ts`:
- Around line 11-14: The isNullTimestamp function currently treats any timestamp
with seconds <= 0 as null and converts BigInt to Number; change its logic to
detect the null sentinel explicitly by comparing timestamp.seconds to the
sentinel value -62135596800n using BigInt comparison (i.e., timestamp.seconds
=== -62135596800n) and avoid Number(...) conversion; update the
isNullTimestamp(timestamp?: Timestamp) implementation to return true only when
timestamp is undefined/null or when timestamp.seconds === -62135596800n (using
the Timestamp.seconds BigInt directly) so Unix epoch (0) is not treated as null.
In `@web/lib/admin/utils/transform-query.ts`:
- Around line 102-108: The rqlRequest builder is using || which will treat valid
0 values for offset/limit as falsy and overwrite them with defaults; update the
object construction in the rqlRequest creation (where RQLRequestSchema is used
to build rqlRequest) to use nullish coalescing (??) for offset and limit (e.g.,
use query.offset ?? 0 and query.limit ?? defaultLimit) so legitimate zero values
are preserved; keep transformSort(query.sort || [], fieldNameMapping) as-is
unless you also want to preserve empty-array semantics with ??.
In `@web/lib/admin/views/audit-logs/navbar.tsx`:
- Around line 31-44: onDownloadClick currently casts the result of
queryClient.getQueryData(AUDIT_LOG_QUERY_KEY) to RQLRequest and calls
onExportCsv without guarding for undefined; update onDownloadClick to check that
the cached query is present before casting/using it (e.g., const query =
queryClient.getQueryData(AUDIT_LOG_QUERY_KEY); if (!query) {
setIsDownloading(false); return; }) and only call onExportCsv(query as
RQLRequest) when query is defined; ensure setIsDownloading(true) /
setIsDownloading(false) semantics remain correct and keep error handling in the
catch block.
In `@web/lib/admin/views/products/index.tsx`:
- Around line 81-86: The ProductDetails instance is being passed no-op handlers
which produces inert UI; update the render guard to only mount ProductDetails
when the real callbacks are present (e.g., check that onClose and
onNavigateToPrices are not null/undefined) or make those props required on the
ProductDetails component and adjust its callers accordingly; locate the JSX that
renders ProductDetails and replace the current fallback (() => {}) usage with a
conditional that ensures both onClose and onNavigateToPrices are provided (or
change ProductDetails prop types to require onClose/onNavigateToPrices and
remove the optional fallbacks).
🧹 Nitpick comments (7)
web/lib/admin/assets/icons/InvoicesIcon.tsx (1)
13-13: Minor: SVG group id doesn't match component purpose.The
<g id="compass">suggests this SVG was originally a compass icon. Consider updating the id to"invoices"or similar for clarity, though this doesn't affect functionality.🔧 Suggested fix
- <g id="compass"> + <g id="invoices">web/lib/admin/views/invoices/navbar.tsx (1)
8-8: Simplify boolean initialization.
searchQuery ? true : falsecan be simplified to!!searchQueryorBoolean(searchQuery).🔧 Suggested fix
- const [showSearch, setShowSearch] = useState(searchQuery ? true : false); + const [showSearch, setShowSearch] = useState(!!searchQuery);web/lib/admin/components/PageHeader.tsx (1)
34-58: Consider using a more stable key for breadcrumb items.Using
item.nameas the key could cause React key collisions if breadcrumb items have duplicate names (e.g., navigating through nested entities with the same title). Consider using the index or a combination ofnameandhref:♻️ Suggested fix
- {breadcrumb.map((item) => + {breadcrumb.map((item, index) => item.href && onBreadcrumbClick ? ( <span - key={item.name} + key={`${index}-${item.name}`} role="button" ... </span> ) : ( - <span key={item.name} className={styles.breadcrumbItem}> + <span key={`${index}-${item.name}`} className={styles.breadcrumbItem}> {item.name} </span> ) )}web/lib/admin/utils/connect-pagination.ts (1)
16-20: Consider a more generic default foritemsKeyor make it required.The default value
"organizations"is specific to one use case. Since callers typically need to specify their own key (e.g.,"users","orgProjects","organizationInvoices"), consider either:
- Making the parameter required to prevent accidental misuse.
- Using a more generic default like
"items".♻️ Option: Make itemsKey required
export function getConnectNextPageParam<T extends ConnectRPCPaginatedResponse>( lastPage: T, queryParams: { query: RQLRequest }, - itemsKey: string = "organizations", + itemsKey: string, ) {web/lib/admin/views/products/columns.tsx (1)
34-64: Prefer the CSS module for the link-style button.There’s now a shared
.linkButtonstyle; using it avoids inline styles and keeps theming consistent.♻️ Suggested refactor
import { Flex, Image, Amount, type DataTableColumnDef } from "@raystack/apsara"; import type { Product } from "@raystack/proton/frontier"; import { timestampToDate, TimeStamp } from "../../utils/connect-timestamp"; +import styles from "./products.module.css"; @@ - <button + <button data-test-id="products-table-prices-link" type="button" onClick={(e) => { e.stopPropagation(); onNavigateToPrices(productId); }} - style={{ - background: "none", - border: "none", - padding: 0, - cursor: "pointer", - textDecoration: "underline", - font: "inherit", - color: "inherit", - }} + className={styles.linkButton} >web/lib/admin/views/products/header.tsx (1)
5-11: Make breadcrumbhrefoptional to avoid empty-string placeholders.
PageHeaderalready handles missinghref; aligning the type improves clarity for callers (e.g., current-page crumbs).♻️ Suggested tweak
const defaultPageHeader = { title: "Products", breadcrumb: [] as { - href: string; + href?: string; name: string; }[], };web/apps/admin/src/routes.tsx (1)
91-99: Consider flattening to sibling routes for clarity, but current nested structure is functionally correct.Both
RolesPageandProductsPageproperly extract params viauseParams()and don't render<Outlet>, so the nested route structure works as intended. However, sibling routes are clearer and align with React Router conventions:♻️ Suggested route flattening
- <Route path="roles" element={<RolesPage />}> - <Route path=":roleId" element={<RolesPage />} /> - </Route> + <Route path="roles" element={<RolesPage />} /> + <Route path="roles/:roleId" element={<RolesPage />} /> ... - <Route path="products" element={<ProductsPage />}> - <Route path=":productId" element={<ProductsPage />} /> - </Route> + <Route path="products" element={<ProductsPage />} /> + <Route path="products/:productId" element={<ProductsPage />} />
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