feat: implement AI settings page with providers management UI#25328
feat: implement AI settings page with providers management UI#25328jakehwll wants to merge 98 commits into
Conversation
| args: { | ||
| provider: "bedrock", | ||
| }, | ||
| }; |
There was a problem hiding this comment.
maybe a test for the fallback name? idk, too low value?
8895cc0 to
e2cecfa
Compare
jeremyruppel
left a comment
There was a problem hiding this comment.
🤖 Generated by Coder Agents on behalf of @jeremyruppel
Clean structure: container/view separation, React Query key hierarchy, cache invalidation, and Storybook coverage for every new component are all solid. The credential-masking UX is thoughtfully designed.
A few things to look at: 1 P1, 3 P2, 2 P3 across 7 inline comments. This review contains findings that may need attention before merge.
| <div className="flex flex-col gap-2"> | ||
| <Label htmlFor={id}>{label}</Label> | ||
| {description && ( | ||
| <div className="text-xs text-content-secondary">{description}</div> |
There was a problem hiding this comment.
P2 description not linked to aria-describedby; screen readers won't announce it (Structural Analyst P2, Frontend Reviewer P2)
Frontend Reviewer: "The new
descriptionprop renders a div between<Label>and<Input>. However, the existingaria-describedbyon the<Input>only references the error or helper text IDs. The description div has noidand is not included inaria-describedby. Screen readers will not announce the description when the input receives focus."
This is a cross-cutting change in a shared component, so worth getting right. Fix: give the div id={\${id}-description`}and include it in the input'saria-describedby` chain.
| key={provider.name} | ||
| hover | ||
| className="cursor-pointer" | ||
| onClick={() => onClick?.()} |
There was a problem hiding this comment.
P2 Clickable table row is not keyboard-accessible (Frontend Reviewer P2)
Frontend Reviewer: "
<TableRow>hasonClickandcursor-pointerbut notabIndex,role, oronKeyDownhandler. Keyboard-only users cannot navigate to or activate these rows. Persite/AGENTS.md, interactive elements need a semantic role and keyboard access."
Fix: add tabIndex={0}, role="link" (or wrap with <Link>), and handle Enter/Space.
There was a problem hiding this comment.
| const newAccessKey = values.accessKey.trim(); | ||
| const newAccessKeySecret = values.accessKeySecret.trim(); | ||
| const hasNewCredentials = newAccessKey !== "" && newAccessKeySecret !== ""; | ||
|
|
There was a problem hiding this comment.
P2 Partial Bedrock credential update is silently discarded (Test Auditor P2, Edge Case Analyst P2, Contract Auditor P2)
Contract Auditor: "
hasNewCredentialsrequires bothnewAccessKey !== ""ANDnewAccessKeySecret !== "". If the user clears both fields, then types a new access key but leaves the secret empty,hasNewCredentialsisfalse. The code falls through to theexistingProvider.settingsbranch and silently preserves the old credentials. The user believes they updated the access key; the API receives the old one."
The Yup schema for edit mode makes both fields optional, so form-level validation won't catch this either. Fix: add a Yup .test() that requires both fields when either is non-empty.
There was a problem hiding this comment.
🤖 Generated by Coder Agents on behalf of @jakehwll
Fixed in 338be75d. The Bedrock Yup schema now has paired .test() checks on accessKey and accessKeySecret: when one is non-empty and not the saved-credential mask, the other is required. Submitting a partial Bedrock rotation now surfaces an inline validation error instead of being silently dropped by the mapper. The mapper itself still gates the credential fields on both values being filled (and mask-sanitized) so a bug bypassing the schema would still keep the existing values rather than write a half-update.
| !canViewHealth | ||
| ) | ||
| return null; | ||
| } |
There was a problem hiding this comment.
P3 Braceless if after multi-line condition (Edge Case Analyst P2, Contract Auditor P2, Structural Analyst P2; downgraded to P3)
Looks like an accidental side-effect of adding canViewAISettings to the condition list. The closing ) lost its matching { for the body. This is the only braceless multi-line if in this file. Restore the braces.
| <PageHeaderTitle>Providers</PageHeaderTitle> | ||
| <PageHeaderSubtitle> | ||
| Connect third-party LLM services like OpenAI, Anthropic, or Google. | ||
| Each provider supplies models that users can select for their |
There was a problem hiding this comment.
P3 Subtitle mentions "Google" but the type selector only offers OpenAI, Anthropic, and Bedrock (Product Reviewer P2; downgraded to P3)
Both ProvidersPageView and AddProviderPageView say "Connect third-party LLM services like OpenAI, Anthropic, or Google." No Google option exists in the form. Quick copy fix.
22dabfa to
62b664c
Compare
e2cecfa to
8e17245
Compare
8e17245 to
aa9f084
Compare
35194e7 to
893ed76
Compare
d602abb to
76cc762
Compare
ba81223 to
1bf6275
Compare
08b59d3 to
31d4359
Compare
…addableProviderTypes
…erType for ProviderFormValues.type
…pe from a host lookup table
…ore states and a focus-clear play function
… CredentialField in ProviderForm and drop the unused CredentialFieldProps export
Rebases the frontend changes from #25328 onto current main. All three dependency PRs (#24892, #24893, #24894) are already merged, so this lands the UI layer cleanly. - AISettingsLayout with sidebar, mounted under /ai/settings - ProvidersPage list view + table of configured providers - AddProviderPage and UpdateProviderPage with shared ProviderForm - ProviderIcon and ProviderRow components with Storybook stories - aiProviders query + CRUD mutation hooks - viewAnyAIProvider permission against ai_provider RBAC resource - Navbar deployment dropdown entry for AI settings
Adds an unsaved-changes prompt to the AI provider add/update forms. The prompt pairs a beforeunload listener (for tab close, refresh) with useBlocker (for in-app navigation), reusing the ConfirmDialog primitive. The form treats values as saved once the parent mutation resolves without an error, so a successful submit does not re-trigger the prompt. Parents now await mutateAsync so the form's submitting state stays true through the success redirect, suppressing the in-flight prompt.
- Create AISettingsSidebarView with AI Governance, Providers, and Manage Coder Agents entries - Move AI routes to /ai/settings with dedicated AISettingsLayout - Add AI entry to admin settings dropdown in navbar - Remove AI items from deployment sidebar - Switch provider pages to SettingsHeader for consistent alignment - Streamline ProviderForm layout with side-by-side fields - Move enable toggle from form to update page header - Add model count display and disabled badge to update page
Add -ml-3 to offset the subtle button's px-3 padding so the arrow icon sits flush with the text content below it.
Add validateOnMount so isValid reflects required-field state from the start. Button stays disabled until the user has both changed something and all required fields pass validation.
Remove provider-specific model counting logic and link to /agents/settings/models instead.
Point the Providers entry in the agents settings sidebar to the new AI settings page and add an ArrowUpRightIcon to indicate the external navigation.
…dirty form after a successful save React Query reports a settled mutation's error as null, so the post-success effect's submitError === undefined check never matched. The dirty baseline was never cleared and the unsaved-changes prompt fired after a successful update. Switch to a truthy check that covers both null and undefined, and add a storybook regression that drives the full submit-then-navigate path.
Port from PR #25328: warn before leaving a dirty provider form via useBlocker (in-app) and beforeunload (tab close). Parents switch to mutateAsync so the submitting state stays true through navigation. Also fixes the dirty-reset after save (null vs undefined check).
Brings Tracey's UI improvements from #25551 onto this branch: - New top-level AI section at /ai/settings with its own layout and sidebar (AISettingsLayout, AISettingsSidebarView): AI Governance, Providers, and Manage Coder Agents entries. - Navbar: 'AI' entry under the admin settings dropdown, gated on viewAnyAIProvider. - Provider pages use SettingsHeader for consistent alignment with the AI Governance page; provider form fields use a side-by-side layout and the enable toggle moved from the form into the update page header alongside a disabled badge and model count. - Deployment sidebar: AI Governance and Manage Coder Agents items removed (moved to the new AI sidebar). - Submit button disabled until the form is dirty and valid. - Status column with an Enabled badge. Also restores getChatACL / updateChatACL on api.ts which were dropped in Tracey's rebase, and removes a brittle storybook timing test that no longer survives the new validateOnMount config.
Split into a 5-PR Graphite stack to keep reviews focused. Each layer is reviewable on its own and the stack ends with the same UX as this PR:
This PR is being kept open as a reference until the stack lands; Jake will close it manually once the stack merges. |
|
Closing as this has been stacked in multiple PRs 🙂 |
Scaffolds a new
/ai/settingsadmin page for managing AI providers (formerly "AI Bridge providers"). Stacked on top of #24892 / #24893 / #24894: the page is now wired against the real/api/v2/ai/providersHTTP CRUD routes from the providers-handlers stack and the generatedcodersdktypes.AISettingsLayoutwith breadcrumb + sidebar, mounted under/ai/settingsProvidersPagelist view + table of configured providersAddProviderPageandUpdateProviderPageflows backed by a sharedProviderFormProviderIconandProviderRowcomponents, each with Storybook storiesaiProvidersquery + create/update/delete mutation hooks againstAPI.{create,update,delete,get}AIProvider(s)createAIProviderMutationposts the provider then, when an API key was supplied, chainsPOST /providers/{id}/keysand rolls the provider back viaDELETEif the key failstype:"anthropic"provider whosesettingscarries the discriminated{_type:"bedrock",_version:1,...}shape;isBedrockProviderdetects this on load^[a-z0-9]+(-[a-z0-9]+)*$viewAnyAIProviderpermission against theai_providerRBAC resource (instead of reusingviewAnyAIBridgeInterception, which belongs to the AI Bridge logs feature)Recent review-pass cleanup
baseURL/baseUrlinto a singlebaseUrl; OpenAI/Anthropic relabelled as "Custom endpoint" with matching validationproviderFormValuesToCreateRequest→providerFormValuesToRequest(helper is used for both create and update)isCredentialPlaceholderheuristic is gonegetProviderIcon/getProviderNamenow fall back toCircleQuestionMarkIcon+ a sensible label for unknown provider types (matching the AI Bridge sessions page convention)providerFormKeyremount; rely on formik'senableReinitializeafter the query refetches the provider/api/v2/ai/providers(and/keyssub-resource); delete the in-page mock module, moveMockAIProvider*totestHelpers/entities.tsviewAnyAIProviderpermission entry topermissions.jsonand switch the four call sites that previously reused the AI Bridge permissiondk/aibridge-providers-handlersand adaptproviderFormApiMap.ts+testHelpers/entities.tsto the new discriminatedAIProviderSettings({_type, _version, region, model, small_fast_model, access_key, access_key_secret})