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 introduces the Confidence Score: 4/5PR is safe to merge; all prior blocking concerns have been resolved and remaining items are low-risk style suggestions. Prior review concerns (pagination URL sync, per-item permission hook, div wrapper, unhandled clipboard promise) are all addressed. The two remaining P2 suggestions — extracting the repeated URL constant and fixing the clipboard mock return type — are non-blocking. The unrelated Transfer export in fidesui is minor. Core functionality is clean, well-tested, and follows project patterns. No files require special attention; oauth-clients.slice.ts and OAuthClientsList.test.tsx have minor style improvements worth addressing before follow-up PRs build on top of them. Important Files Changed
Reviews (2): Last reviewed commit: "Mock useAntPagination in OAuthClientsLis..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@greptile rereview |
… error Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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