Conversation
Adds navigation, RTK Query slice, API types, table component and tests, and the /api-clients index page so users can view all OAuth clients. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overall this is a clean, well-scoped addition. The RTK Query slice is consistent with existing patterns, the types are properly generated/exported, the nav wiring is correct, and the test coverage is solid for a list component. A few things to address:
Suggestion (worth fixing before merge)
navigator.clipboard.writeText()inClipboardButton.tsxreturns a Promise whose rejection is silently swallowed. The tooltip will show "Copied!" even when the write failed. The fix is straightforward — movesetTooltipTextinto.then()and add a.catch().- The
errorvalue fromuseOAuthClientsListis never rendered. A failed API call looks identical to an empty list from the user's perspective.
Nice to have
- Empty state copy references a "Create API client" button that doesn't exist in this PR yet.
rotateOAuthClientSecretis the only mutation withoutinvalidatesTags, which is inconsistent and could cause stale cache issues when a detail page is added.
| <div> | ||
| <List | ||
| loading={isLoading} | ||
| itemLayout="horizontal" |
There was a problem hiding this comment.
Suggestion: surface API errors to the user
error is returned from useOAuthClientsList but never destructured or rendered in OAuthClientsList. If the API call fails, the component shows an empty list with no feedback — the user has no way to distinguish a genuine "no clients" state from a failed request.
Consider adding a simple error banner or inline message, e.g.:
const { data, total, isLoading, error, page, pageSize, setPage, setPageSize } =
useOAuthClientsList();
if (error) {
return <Alert type="error" message="Failed to load API clients." />;
}
Greptile SummaryThis PR adds the OAuth API clients list page ( Key findings:
Confidence Score: 3/5
Important Files Changed
Reviews (1): Last reviewed commit: "Remove create flow from list page (added..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const useOAuthClientsList = () => { | ||
| const [page, setPage] = useState(1); | ||
| const [pageSize, setPageSize] = useState(DEFAULT_PAGE_SIZE); | ||
|
|
||
| const { data, isLoading, error } = useListOAuthClientsQuery({ | ||
| page, | ||
| size: pageSize, | ||
| }); | ||
|
|
||
| return { | ||
| data: data?.items ?? [], | ||
| total: data?.total ?? 0, | ||
| isLoading, | ||
| error, | ||
| page, | ||
| pageSize, | ||
| setPage, | ||
| setPageSize, | ||
| }; | ||
| }; |
There was a problem hiding this comment.
The useOAuthClientsList hook manages page and pageSize with plain useState, but the project provides a dedicated usePagination (or its Ant Design wrapper useAntPagination) hook in src/features/common/pagination/ that synchronises pagination state with URL query parameters. Using local state means the current page is lost on refresh or navigation, and back/forward browser history won't work correctly.
Consider using useAntPagination from ~/features/common/pagination/useAntPagination:
import { useAntPagination } from "~/features/common/pagination/useAntPagination";
const useOAuthClientsList = () => {
const { pageIndex, pageSize, paginationProps } = useAntPagination();
const { data, isLoading, error } = useListOAuthClientsQuery({
page: pageIndex,
size: pageSize,
});
return {
data: data?.items ?? [],
total: data?.total ?? 0,
isLoading,
error,
paginationProps,
};
};Rule Used: Use the existing usePagination hook for paginati... (source)
Learnt From
ethyca/fides#6594
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| const ClientListItem = ({ client }: { client: ClientResponse }) => { | ||
| const canUpdate = useHasPermission([ScopeRegistryEnum.CLIENT_UPDATE]); |
There was a problem hiding this comment.
useHasPermission called per list item instead of once
useHasPermission is invoked inside ClientListItem, which is rendered once for every client in the list. The permission result is the same for all items in the list (it doesn't depend on the client prop), so calling the hook once at the parent level (OAuthClientsList) and passing the result as a prop would be more efficient and cleaner.
// In OAuthClientsList
const canUpdate = useHasPermission([ScopeRegistryEnum.CLIENT_UPDATE]);
// ...
renderItem={(client) => <ClientListItem client={client} canUpdate={canUpdate} />}// ClientListItem receives it as a prop
const ClientListItem = ({ client, canUpdate }: { client: ClientResponse; canUpdate: boolean }) => {| useOAuthClientsList(); | ||
|
|
||
| return ( | ||
| <div> |
There was a problem hiding this comment.
Flex over bare div wrapper
The outer <div> on line 94 is the layout wrapper for the list and pagination. Per project style, prefer Ant Design's Flex (vertical) component over plain div elements when possible.
| <div> | |
| <Flex vertical> |
Rule Used: Avoid using div elements when possible. Use sema... (source)
Learnt From
ethyca/fides#6763
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| rotateOAuthClientSecret: build.mutation<ClientSecretRotateResponse, string>( | ||
| { | ||
| query: (clientId) => ({ | ||
| url: `oauth/client/${clientId}/secret`, | ||
| method: "POST", | ||
| }), | ||
| }, | ||
| ), |
There was a problem hiding this comment.
invalidateTags on the rotate mutation
Every other mutation in this slice (createOAuthClient, updateOAuthClient, deleteOAuthClient) includes invalidateTags so related cache entries refresh automatically. The rotation mutation has no invalidateTags, meaning that if a future detail page shows metadata that changes after rotation (e.g. a last-rotated timestamp), the view will stay stale until a manual refetch.
Add invalidateTags targeting the specific client's cache entry:
invalidatesTags: (_result, _error, clientId) => [
{ type: "OAuth Client", id: clientId },
],Rule Used: When adding new RTK Query mutation endpoints, incl... (source)
Learnt From
ethyca/fides#6711
1 participant
Ticket ENG-3001
Description Of Changes
Adds navigation, RTK Query slice, API types, table component and tests,
and the /api-clients index page so users can view all OAuth clients.
Code Changes
Steps to Confirm
Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works