fix(site/src): reposition org pickers in AI settings models and MCP pages to be consistent - #28564
Conversation
…able/disable UI and align org picker
…bmit until the edit form is dirty
…er chevron behavior
…, error states, and a11y
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb2789d1b5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ibetitsmike
left a comment
ibetitsmike
left a comment
There was a problem hiding this comment.
Disclosure: This review was researched and written by Xum, an AI coding agent, acting for @ibetitsmike.
Ran the repo frontend-review skill (FE1 to FE10 from .claude/docs/FRONTEND_PATTERNS.md) over the full merge-base diff (29 files under site/src/).
FE1 FAIL ModelsPage/components/ModelFormFields.tsx:253 interactive add-mode organization select is never rendered in any story (every form story pins a single accessible org)
FE2 PASS no any/casts/non-null assertions; generated types used
FE3 PASS note inline: shared OrganizationAutocomplete restyle reaches 5 consumers outside these pages
FE4 PASS added comments carry rationale and stay factually correct
FE5 PASS search empty state, error-state pickers, and org-keyed search reset all handled
FE6 FAIL MCPServersPage/components/MCPServerFormHeader.tsx:89 permission-disabled switch is keyboard-unreachable and its reason undiscoverable; two smaller ARIA notes inline
FE7 PASS query keys via chatModels/organizationsPermissions constants; no direct API calls in components
FE8 PASS no new effects; filtering derived via useMemo
FE9 PASS shared MockGitHubMCPServer reused; per-story inline query wiring
FE10 PASS role/label/text queries throughout
Validation on the PR head (eb2789d): biome check --error-on-warnings clean across site/, tsc -p . clean, and PR CI is green including the Storybook interaction suite.
Out-of-title scope (status column removal, kebab menu to inline Delete, dirty-gated submit) is documented in the description and covered by updated stories, so no FE3 action needed; just noting it spans more than picker placement.
Nice touches: the Unavailable badge story asserts keyboard focus opens the tooltip without activating the row, and the dirty-gated update flow is exercised end to end with the submitted payload.
| const [searchParams] = useSearchParams(); | ||
| const id = useId(); | ||
|
|
||
| if (!readOnly && accessibleOrganizations.length <= 1) { |
There was a problem hiding this comment.
Question: with a single accessible org the add form renders no organization field at all, while the MCP add form shows a read-only value (showSingleOrganization) and the models edit form always shows the static value via readOnly. That preserves the old layout-level behavior, but it leaves add and edit inconsistent on single-org deployments. Intentional?
There was a problem hiding this comment.
Coder Agents: Kept the current behavior intentionally: add/list switchers hide for a single accessible org to match the previous layout-level behavior, while edit pages still show the static org because it explains the immutable scope. MCP add differs because its create flow already had showSingleOrganization semantics; I left models unchanged rather than expanding scope.
Documentation CheckUpdates Needed
Documentation to Verify
Notes:
Automated review via Coder Agents |
Docs previewCheck off each page once it's been reviewed. If a page changes in a later push, its checkbox clears automatically so it gets a fresh look. Pages not yet wired into the docs navigation aren't listed here. |
ibetitsmike
changed the title
ibetitsmike
left a comment
ibetitsmike
left a comment
There was a problem hiding this comment.
Disclosure: This review was researched and written by Xum, an AI coding agent, acting for @ibetitsmike.
Follow-up frontend-review audit at the current head 51c7e4d9 (supersedes the earlier FE table posted on eb2789d): all FE1-FE10 now PASS.
Adjudication of the open threads/findings, verified in code:
- FE1 add-mode org picker story gap -> addressed in
f337a74(AddModelPageView.stories.tsxWithOrganizationPickermounts two orgs and drives the picker;ProviderNotFound/LoadErrorkeep it available). - FE6 keyboard-unreachable disabled switch (
MCPServerFormHeader) -> addressed inf337a74(aria-disabled path, switch stays reachable, tooltip restored). - Codex P2 (focusable Unavailable badge on a generic div) + aria-label-on-div nit -> fixed in
3103e9f8:Badge asChildwrapping a real<button type="button">, focus opens the tooltip without activating the row, story-verified. - Codex P1 (org switch into a no-permission org unmounts the page) -> fixed in
3103e9f8: add/duplicate selectors filter to orgs withcreateChatModelConfigs(story asserts unauthorized orgs are excluded); edit/list keep full selection. - coderagents docs check -> fixed in
50c7d4f2:models.mdnow documents the Unavailable/Disabled badges instead of the removed Status column. <output>live-region concern -> verified fixed in51c7e4d9:div role="group"keeps the accessible name (contains the visible text;getByLabelTextqueries intact) and drops the implicitrole="status".useMemoand shared chevron restyle threads -> verified removed/reverted inf337a74.
Transparency: commits 3103e9f8 + 50c7d4f2 were written and pushed by Xum acting for @ibetitsmike, and the PR title scope was dropped to feat: (the docs commit made the previous site scope fail contrib/title).
Status: FE-clean at current head. CI note: a GitHub Actions incident (~15:19Z) caused startup_failures and a dropped ci trigger on the previous head; contrib + docs-preview were re-run and ci is in progress on 51c7e4d9 at time of writing.
|
Coder Agents: Follow-up on the org selector pattern: I checked workspaces/agents/templates/users. The shared pattern is to render |
tracyjohnsonux
changed the title
tracyjohnsonux
changed the title
2 participants
Repositions the organization picker on the AI settings models and MCP servers pages so it lives with the content it scopes instead of floating above the page header.
Models (
/ai/settings/models)OrganizationModelsLayoutinto a sharedModelOrganizationSelectcomponent that preserves the current path and auxiliary query params when switching orgs.MCP servers (
/ai/settings/mcp-servers)OrganizationPickernow renders a static read-only value instead of a disabled button when the org cannot be changed (no handler or single org). This fixes the mutedcontent-disabledtext on the update page. The read-only rendering is shared with the models edit page via a newOrganizationValuecomponent besideOrganizationAutocomplete.Status columns (both list tables)
content-disabled.Decision log
ModelOrganizationSelectreusesOrganizationAutocompleteand reads accessible organizations from the models context (accessibleOrganizationsadded toOrganizationModelsContext), rather than duplicating the layout's navigation logic per page.orgsearch param and preserves the path and remaining params, exactly as the old layout-level picker did.OrganizationPickerread-only state uses an<output>element (labelable, keeps theLabelassociation) rather than a disabled button, so non-interactive values do not render with disabled styling or sit in the tab order.ModelsPageViewstory duplicating the shared picker's select-and-navigate flow was deliberately dropped; list-page placement is covered by theOrganizationModelsLayoutstories that mount the realModelsPage. TheModelFormfallback-branch picker has no dedicated story since its sibling branch and the shared flow are covered.Verification:
pnpm lint:types,pnpm check, and all affected Storybook tests pass (ModelsPage and MCPServersPage: 140), plus theorganizationModelsandmcpServerFormLogicunit tests.🤖 This PR was generated by Coder Agents on behalf of @tracyjohnsonux.