Conversation
Co-authored-by: Copilot <copilot@github.com>
📝 WalkthroughWalkthroughThis PR introduces a new Providers management feature to the UI, enabling creation, editing, and deletion of AI model providers with provider-specific configurations. Updates include new provider forms and listing pages, API client integration, type definitions for multiple provider types (OpenAI, Anthropic, Bedrock, etc.), React Query hooks for provider data management, and refactoring of the model form to use dynamic provider lookup instead of hardcoded provider lists. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant ModelForm
participant useProviders
participant API
participant Database
User->>ModelForm: Open model form
ModelForm->>useProviders: Fetch provider list
useProviders->>API: GET /providers
API->>Database: Query providers
Database-->>API: Return providers
API-->>useProviders: Provider list
useProviders-->>ModelForm: Providers loaded
ModelForm->>User: Display Combobox with providers
User->>ModelForm: Select provider & enter model name
User->>ModelForm: Click Submit
ModelForm->>API: POST /models {provider_id, model}
API->>Database: Create model with provider_id
Database-->>API: Model created
API-->>ModelForm: Success
ModelForm->>User: Navigate to models list
sequenceDiagram
actor User
participant ProvidersPage
participant useProviders
participant useDeleteProvider
participant API
participant Database
User->>ProvidersPage: Visit /providers
ProvidersPage->>useProviders: Fetch provider list
useProviders->>API: GET /providers
API->>Database: Query providers
Database-->>API: Return providers
API-->>useProviders: Provider list
useProviders-->>ProvidersPage: Display table
ProvidersPage->>User: Show providers table
User->>ProvidersPage: Click delete on row
ProvidersPage->>User: Show confirmation dialog
User->>ProvidersPage: Confirm delete
ProvidersPage->>useDeleteProvider: Delete provider
useDeleteProvider->>API: DELETE /providers/{id}
API->>Database: Delete provider
Database-->>API: Success
API-->>useDeleteProvider: Invalidate cache
useDeleteProvider-->>ProvidersPage: Refresh list
sequenceDiagram
actor User
participant ProviderForm
participant useCreateProvider
participant API
participant Database
User->>ProviderForm: Open create provider form
ProviderForm->>User: Display name, type, config fields
User->>ProviderForm: Select provider type (e.g., Bedrock)
ProviderForm->>User: Show type-specific config fields
User->>ProviderForm: Fill name, region, access key, secret
User->>ProviderForm: Click Submit
ProviderForm->>ProviderForm: Validate required fields
ProviderForm->>useCreateProvider: Create provider
useCreateProvider->>API: POST /providers {name, type, config}
API->>Database: Create provider with config
Database-->>API: Provider created
API-->>useCreateProvider: Success & cache invalidate
useCreateProvider-->>ProviderForm: Done
ProviderForm->>User: Navigate to /providers
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 1
🧹 Nitpick comments (5)
ui/src/routes/_layout/providers/create.tsx (1)
20-23: Consider wrapping mutation in try-catch for explicit error handling.If
createProvider.mutateAsync(data)throws, the navigation on line 22 won't execute (which is correct), but the unhandled rejection may cause issues. While React Query surfaces the error viacreateProvider.error, wrapping in try-catch makes the intent clearer and allows for additional error handling if needed.✨ Optional: explicit error handling
async function handleSubmit(data: Provider) { - await createProvider.mutateAsync(data); - navigate({ to: '/providers' }); + try { + await createProvider.mutateAsync(data); + navigate({ to: '/providers' }); + } catch { + // Error is surfaced via createProvider.error + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/routes/_layout/providers/create.tsx` around lines 20 - 23, The handleSubmit function should explicitly catch errors from createProvider.mutateAsync to avoid unhandled rejections and allow custom handling; wrap the await createProvider.mutateAsync(data) call in a try-catch inside handleSubmit, on success call navigate({ to: '/providers' }) and in the catch block surface or log the error (e.g., use createProvider.error, console.error or set local error state) and optionally rethrow or return to stop further processing; update the handleSubmit function to reference createProvider.mutateAsync and navigate accordingly.ui/src/routes/_layout/providers/index.tsx (2)
119-125: Per-row delete lacks user feedback on failure.The delete button calls
deleteProvider.mutate()without a callback. If the deletion fails, React Query will set an error state, but there's no toast or inline feedback to notify the user. Consider addingonErrorcallback or a toast notification.✨ Add error feedback
<Button variant="destructive" size="icon-lg" - onClick={() => deleteProvider.mutate(row.original.id)} + onClick={() => + deleteProvider.mutate(row.original.id, { + onError: (err) => { + // Consider toast notification here + console.error('Delete failed:', err.message); + }, + }) + } >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/routes/_layout/providers/index.tsx` around lines 119 - 125, The per-row delete Button calls deleteProvider.mutate(row.original.id) without handling failure feedback; update the deleteProvider call to provide an onError (and optionally onSuccess/onSettled) callback so the UI shows a toast or inline message when deletion fails (use the same toast utility used elsewhere), e.g., wire onError to display a descriptive error including row.original.id and the error message, and ensure any optimistic updates are reverted in onError or reconciled in onSettled; locate the Button invoking deleteProvider.mutate and the deleteProvider (React Query mutation) to add these callbacks.
131-136: Bulk delete stops on first failure and clears selection regardless.The sequential
awaitloop will stop if any deletion throws, leaving remaining items undeleted. Additionally,setRowSelection({})executes even after a failure, which could confuse users about what was actually deleted.✨ Use Promise.allSettled for resilient bulk delete
async function handleDeleteSelected() { - for (const id of selectedKeys) { - await deleteProvider.mutateAsync(id); - } - setRowSelection({}); + const results = await Promise.allSettled( + selectedKeys.map((id) => deleteProvider.mutateAsync(id)) + ); + const failed = results.filter((r) => r.status === 'rejected'); + if (failed.length > 0) { + // Consider toast: `${failed.length} deletion(s) failed` + console.error(`${failed.length} deletion(s) failed`); + } + setRowSelection({}); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/routes/_layout/providers/index.tsx` around lines 131 - 136, The bulk delete in handleDeleteSelected uses a sequential await loop over selectedKeys and will abort on the first thrown error and still call setRowSelection({}), which can leave some items undeleted while clearing UI selection; change to fire all deletions concurrently with Promise.allSettled on selectedKeys mapped to deleteProvider.mutateAsync(id), collect successes and failures, only clear selection (setRowSelection({})) for items that succeeded (or if all succeeded), and surface or aggregate failures (e.g., return or log failing ids/errors) so the UI can inform the user which deletes failed instead of silently clearing selection.ui/src/routes/_layout/providers/$id.tsx (1)
27-31: Consider replacing nativeconfirm()with a styled dialog.The native
confirm()works but is visually inconsistent with the rest of the UI and blocks the main thread. Consider using a styled confirmation dialog component for better UX consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/routes/_layout/providers/`$id.tsx around lines 27 - 31, Replace the blocking native confirm() in handleDelete with your app’s styled confirmation dialog component: add local state (e.g., isDeleteDialogOpen) and an onDeleteConfirm handler that calls deleteProvider.mutateAsync(id), closes the dialog, and then navigate({ to: '/providers' }); update the button that currently triggers handleDelete to instead set isDeleteDialogOpen = true; wire the dialog’s confirm button to onDeleteConfirm and its cancel to close the dialog; ensure you use the existing function names (handleDelete -> open dialog flow, deleteProvider.mutateAsync, navigate) and preserve accessibility/focus management for the dialog.ui/src/lib/queries/providers.ts (1)
45-73: Mutation hooks lack 401 error handling unlike query hooks.The query hooks (
useProviders,useProvider) handle 401 errors by opening the admin modal, but the mutation hooks don't. If a mutation fails with 401 (e.g., expired/invalid key), the user won't see the modal and won't know why the operation failed.Consider adding consistent
onErrorhandling to mutations:♻️ Suggested improvement
export function useCreateProvider() { const qc = useQueryClient(); - const { key } = useAdminKey(); + const { key, openModal } = useAdminKey(); return useMutation({ mutationFn: (data: Provider) => providersApi.create(key!, data), onSuccess: () => qc.invalidateQueries({ queryKey: providerKeys.list() }), + onError: (err) => { + if (err instanceof ApiClientError && err.status === 401) { + openModal(); + } + }, }); }Apply similar pattern to
useUpdateProvideranduseDeleteProvider.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/lib/queries/providers.ts` around lines 45 - 73, Add consistent 401 error handling to the mutation hooks so the admin modal opens on unauthorized errors like the query hooks do: update useCreateProvider, useUpdateProvider, and useDeleteProvider to include an onError handler that checks for a 401 response (or error.status === 401) and calls the same function used by the query hooks to open the admin modal (match the pattern used in useProviders/useProvider), while still letting other errors propagate/handle normally and keeping existing onSuccess invalidation logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/src/lib/api/client.ts`:
- Around line 78-88: The get, update, and delete methods in the client (get,
update, delete) interpolate raw provider id into the path which can break for
IDs with reserved characters; change the URL construction to encode the path
segment (e.g. use encodeURIComponent on the id) when composing
`/providers/${id}` so the endpoint always receives a safe path segment while
leaving create unchanged.
---
Nitpick comments:
In `@ui/src/lib/queries/providers.ts`:
- Around line 45-73: Add consistent 401 error handling to the mutation hooks so
the admin modal opens on unauthorized errors like the query hooks do: update
useCreateProvider, useUpdateProvider, and useDeleteProvider to include an
onError handler that checks for a 401 response (or error.status === 401) and
calls the same function used by the query hooks to open the admin modal (match
the pattern used in useProviders/useProvider), while still letting other errors
propagate/handle normally and keeping existing onSuccess invalidation logic.
In `@ui/src/routes/_layout/providers/`$id.tsx:
- Around line 27-31: Replace the blocking native confirm() in handleDelete with
your app’s styled confirmation dialog component: add local state (e.g.,
isDeleteDialogOpen) and an onDeleteConfirm handler that calls
deleteProvider.mutateAsync(id), closes the dialog, and then navigate({ to:
'/providers' }); update the button that currently triggers handleDelete to
instead set isDeleteDialogOpen = true; wire the dialog’s confirm button to
onDeleteConfirm and its cancel to close the dialog; ensure you use the existing
function names (handleDelete -> open dialog flow, deleteProvider.mutateAsync,
navigate) and preserve accessibility/focus management for the dialog.
In `@ui/src/routes/_layout/providers/create.tsx`:
- Around line 20-23: The handleSubmit function should explicitly catch errors
from createProvider.mutateAsync to avoid unhandled rejections and allow custom
handling; wrap the await createProvider.mutateAsync(data) call in a try-catch
inside handleSubmit, on success call navigate({ to: '/providers' }) and in the
catch block surface or log the error (e.g., use createProvider.error,
console.error or set local error state) and optionally rethrow or return to stop
further processing; update the handleSubmit function to reference
createProvider.mutateAsync and navigate accordingly.
In `@ui/src/routes/_layout/providers/index.tsx`:
- Around line 119-125: The per-row delete Button calls
deleteProvider.mutate(row.original.id) without handling failure feedback; update
the deleteProvider call to provide an onError (and optionally
onSuccess/onSettled) callback so the UI shows a toast or inline message when
deletion fails (use the same toast utility used elsewhere), e.g., wire onError
to display a descriptive error including row.original.id and the error message,
and ensure any optimistic updates are reverted in onError or reconciled in
onSettled; locate the Button invoking deleteProvider.mutate and the
deleteProvider (React Query mutation) to add these callbacks.
- Around line 131-136: The bulk delete in handleDeleteSelected uses a sequential
await loop over selectedKeys and will abort on the first thrown error and still
call setRowSelection({}), which can leave some items undeleted while clearing UI
selection; change to fire all deletions concurrently with Promise.allSettled on
selectedKeys mapped to deleteProvider.mutateAsync(id), collect successes and
failures, only clear selection (setRowSelection({})) for items that succeeded
(or if all succeeded), and surface or aggregate failures (e.g., return or log
failing ids/errors) so the UI can inform the user which deletes failed instead
of silently clearing selection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f02598f8-3272-450f-8949-a9764617ea05
⛔ Files ignored due to path filters (1)
ui/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (20)
ui/package.jsonui/src/components/layout/sidebar.tsxui/src/components/models/model-form.tsxui/src/components/providers/provider-form.tsxui/src/components/ui/button.tsxui/src/components/ui/combobox.tsxui/src/components/ui/input-group.tsxui/src/components/ui/input.tsxui/src/components/ui/textarea.tsxui/src/i18n/locales/en.jsonui/src/i18n/locales/zh-CN.jsonui/src/lib/api/client.tsui/src/lib/api/types.tsui/src/lib/queries/providers.tsui/src/routeTree.gen.tsui/src/routes/_layout/providers/$id.tsxui/src/routes/_layout/providers/create.tsxui/src/routes/_layout/providers/index.tsxui/tsconfig.jsonui/vite.config.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Improvements