-
Notifications
You must be signed in to change notification settings - Fork 25
feat: config access tab #2810
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: config access tab #2810
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThis pull request introduces a new configuration access details feature. A new route and page component display access information for a configuration, including API endpoints to fetch access summaries, a React Query hook to manage the data, new type definitions, and UI updates to add an Access tab to the config details view. Changes
Sequence DiagramsequenceDiagram
actor User
participant Page as ConfigDetailsAccessPage
participant Hook as useConfigAccessSummaryQuery
participant API as getConfigAccessSummary
participant DB as Database
User->>Page: Navigate to config/:id/access
activate Page
Page->>Hook: useConfigAccessSummaryQuery(configId)
activate Hook
Hook->>API: getConfigAccessSummary(configId)
activate API
API->>DB: Query access summary records
activate DB
DB-->>API: Return access records with pagination
deactivate DB
API-->>Hook: Return resolved data
deactivate API
Hook-->>Page: Return { data, isLoading }
deactivate Hook
Page->>Page: Render access table with formatters
Page-->>User: Display access summary table
deactivate Page
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
ffa2e94 to
bc6d4ab
Compare
bc6d4ab to
460c8e1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/api/services/configs.ts (1)
166-180: Simplify the order parameter encoding.The implementation correctly encodes the
configIdparameter, but theencodeURIComponentcall on line 171-172 for the static string"user.asc"is unnecessary. Static sort parameters don't require encoding and other similar functions in this file (e.g.,getConfigChanges,getAllConfigsMatchingQuery) don't encode their order parameters.🔎 Proposed simplification
export const getConfigAccessSummary = (configId: string) => resolvePostGrestRequestWithPagination<ConfigAccessSummary[]>( ConfigDB.get( `/config_access_summary?config_id=eq.${encodeURIComponent( configId - )}&select=user,email,role,external_group_id,last_signed_in_at,last_reviewed_at,created_at&order=${encodeURIComponent( - "user.asc" - )}`, + )}&select=user,email,role,external_group_id,last_signed_in_at,last_reviewed_at,created_at&order=user.asc`, { headers: { Prefer: "count=exact" } } ) );src/pages/config/details/ConfigDetailsAccessPage.tsx (1)
46-124: Consider adding error state handling.The component correctly uses the React Query hook and renders loading states, but it doesn't handle error scenarios. If the API call fails, users may see an indefinite loading state or an empty table without explanation.
🔎 Proposed error handling
const { data: accessSummary, isLoading, + isError, + error, refetch } = useConfigAccessSummaryQuery(id); const columns = useMemo<MRT_ColumnDef<ConfigAccessSummary>[]>( // ... column definitions ); const rows = accessSummary?.data ?? []; + if (isError) { + return ( + <ConfigDetailsTabs + pageTitlePrefix={"Catalog Access"} + isLoading={false} + refetch={refetch} + activeTabName="Access" + > + <div className="flex h-full items-center justify-center"> + <p className="text-red-600">Failed to load access data. Please try again.</p> + </div> + </ConfigDetailsTabs> + ); + } + return ( <ConfigDetailsTabs // ... rest of component
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/App.tsxsrc/api/query-hooks/useConfigAccessSummaryQuery.tsxsrc/api/services/configs.tssrc/api/types/configs.tssrc/components/Configs/ConfigDetailsTabs.tsxsrc/components/Configs/ConfigTabsLinks.tsxsrc/pages/config/details/ConfigDetailsAccessPage.tsx
🧰 Additional context used
🧬 Code graph analysis (5)
src/api/query-hooks/useConfigAccessSummaryQuery.tsx (1)
src/api/services/configs.ts (1)
getConfigAccessSummary(166-180)
src/api/services/configs.ts (3)
src/api/resolve.ts (1)
resolvePostGrestRequestWithPagination(24-33)src/api/types/configs.ts (1)
ConfigAccessSummary(100-108)src/api/axios.ts (1)
ConfigDB(27-34)
src/components/Configs/ConfigTabsLinks.tsx (2)
src/api/query-hooks/useConfigAccessSummaryQuery.tsx (1)
useConfigAccessSummaryQuery(4-13)src/ui/Badge/Badge.tsx (1)
Badge(15-59)
src/pages/config/details/ConfigDetailsAccessPage.tsx (6)
src/ui/MRTDataTable/MRTCellProps.ts (1)
MRTCellProps(9-16)src/api/types/configs.ts (1)
ConfigAccessSummary(100-108)src/ui/Age/Age.tsx (1)
Age(24-105)src/api/query-hooks/useConfigAccessSummaryQuery.tsx (1)
useConfigAccessSummaryQuery(4-13)src/components/Configs/ConfigDetailsTabs.tsx (1)
ConfigDetailsTabs(33-95)src/ui/MRTDataTable/MRTDataTable.tsx (1)
MRTDataTable(58-203)
src/App.tsx (3)
src/pages/config/details/ConfigDetailsAccessPage.tsx (1)
ConfigDetailsAccessPage(46-125)src/components/Permissions/AuthorizationAccessCheck.tsx (1)
withAuthorizationAccessCheck(58-73)src/context/UserAccessContext/permissions.ts (1)
tables(1-27)
🔇 Additional comments (8)
src/api/types/configs.ts (1)
100-108: LGTM! Clean type definition.The
ConfigAccessSummaryinterface is well-structured with appropriate optional fields and consistent naming conventions.src/components/Configs/ConfigDetailsTabs.tsx (1)
27-27: LGTM! Consistent with existing tab pattern.The addition of "Access" to the
activeTabNameunion type properly enables the new Access tab functionality.src/App.tsx (1)
230-234: LGTM! Follows the established dynamic import pattern.The dynamic import for
ConfigDetailsAccessPageis consistent with other page imports in the file.src/api/query-hooks/useConfigAccessSummaryQuery.tsx (1)
4-12: LGTM! Solid React Query hook implementation.The hook correctly uses
enabled: !!configIdto prevent queries with undefined IDs, andkeepPreviousData: trueprovides smooth UX during navigation. The non-null assertion in thequeryFnis safe given theenabledguard.src/components/Configs/ConfigTabsLinks.tsx (3)
8-8: LGTM! Clean import.
35-37: Good fallback chain for computing access count.The implementation correctly handles different response formats by checking both
totalEntriesanddata.lengthwith appropriate fallbacks.
93-104: LGTM! Conditional tab rendering provides clean UX.The Access tab only appears when there's access data (
accessCount > 0), which prevents showing empty tabs. The Badge component effectively displays the count, consistent with other tabs like Changes and Insights.Note: The current implementation silently hides the tab if the query fails. This is acceptable for a "Chill" review, but consider whether error states should be surfaced to users in future iterations.
src/pages/config/details/ConfigDetailsAccessPage.tsx (1)
12-44: LGTM! Well-structured cell renderers.The cell renderer implementations handle null values appropriately and provide clear visual feedback. The AccessTypeCell correctly distinguishes between group-based and direct access with appropriate badges and tooltips.
Dummy data example:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.