feat: refactor MCP client form auth UX with split kind/scope dropdowns and accordion OAuth settings - #3799
Merged
Merged
Conversation
|
|
This was referenced May 27, 2026
Collaborator
Author
Contributor
|
Warning Review limit reached
More reviews will be available in 53 minutes and 59 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThree MCP registry view components restructure authentication form controls, table display columns, and details sheet presentation. Form authentication splits into Kind/Scope selection with OAuth settings in an accordion. Table adds Edit action and Auth Type/Scope columns. Details sheet includes a connection summary block and enhanced Auto-execute header documentation. ChangesMCP Registry Client UI Restructure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Confidence Score: 4/5Safe to merge after fixing the dropdown focus-restoration regression; the auth split/recombine logic and table changes are otherwise correct. The keyboard accessibility regression in the actions dropdown — where Escape and outside-click no longer return focus to the trigger — is a real, reproducible behaviour change introduced by the unconditional e.preventDefault() in onCloseAutoFocus. All other changes are UI-only refactors with no backend contract changes. ui/app/workspace/mcp-registry/views/mcpClientsTable.tsx — the onCloseAutoFocus handler needs to be scoped to the Edit action only. Important Files Changed
Reviews (3): Last reviewed commit: "fix: mcp ui fixes" | Re-trigger Greptile |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/app/workspace/mcp-registry/views/mcpClientsTable.tsx (1)
364-386:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake the new row-level test ids row-qualified.
mcp-client-connection-type,mcp-client-auth-scope, andmcp-client-vk-accessare rendered once per row, so they repeat across the table. If these are meant for E2E, include a stable qualifier such asclient_id; otherwise tests will need brittle nth-match selectors.Example adjustment
- <TableCell data-testid="mcp-client-connection-type"> + <TableCell data-testid={`mcp-client-connection-type-${c.config.client_id}`}> @@ - <TableCell data-testid="mcp-client-auth-type">{getAuthTypeDisplay(c.config.auth_type)}</TableCell> - <TableCell data-testid="mcp-client-auth-scope">{getAuthScopeDisplay(c.config.auth_type)}</TableCell> + <TableCell data-testid={`mcp-client-auth-type-${c.config.client_id}`}>{getAuthTypeDisplay(c.config.auth_type)}</TableCell> + <TableCell data-testid={`mcp-client-auth-scope-${c.config.client_id}`}>{getAuthScopeDisplay(c.config.auth_type)}</TableCell> @@ - <TableCell data-testid="mcp-client-vk-access"> + <TableCell data-testid={`mcp-client-vk-access-${c.config.client_id}`}>Based on learnings, "Enforce the 3-part data-testid convention: -- for UI testids in Bifrost UI TSX components."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/app/workspace/mcp-registry/views/mcpClientsTable.tsx` around lines 364 - 386, The row-level testids used in mcpClientsTable.tsx are not qualified per-row; update the data-testid attributes for the TableCell elements that render per-row (currently "mcp-client-connection-type", "mcp-client-auth-scope", and "mcp-client-vk-access") to follow the 3-part convention entity-element-qualifier by appending a stable row qualifier (e.g., the client's ID from the row object `c.config.client_id` or another unique field) — e.g. `mcp-client-connection-type-{c.config.client_id}`, `mcp-client-auth-scope-{c.config.client_id}`, `mcp-client-vk-access-{c.config.client_id}`; make the same change pattern where you render connection/auth display via getConnectionTypeDisplay/getAuthScopeDisplay and the vk configs count so E2E tests can target a unique cell per client.
🧹 Nitpick comments (1)
ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx (1)
910-918: ⚡ Quick winConsider adding data-testid for consistency with other help links.
For consistency with the existing code-mode help link (line 455:
data-testid="code-mode-link-help"), consider adding a data-testid to this Auto-execute documentation link.📝 Suggested addition
<a href="https://docs.getbifrost.ai/mcp/agent-mode" target="_blank" rel="noopener noreferrer" + data-testid="auto-execute-link-help" aria-label="Learn more about Auto-execute and Agent Mode" className="text-muted-foreground hover:text-foreground focus-visible:ring-ring inline-flex rounded focus-visible:ring-2 focus-visible:outline-none" >🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx` around lines 910 - 918, The Auto-execute/Agent Mode documentation anchor (<a href="https://docs.getbifrost.ai/mcp/agent-mode" ...> with the Info icon) is missing a data-testid; add a data-testid attribute (e.g., data-testid="auto-execute-link-help" or similar) to that <a> element in mcpClientSheet.tsx so it matches the pattern used for the code-mode help link (data-testid="code-mode-link-help"), keeping the existing attributes (target, rel, aria-label, className, and nested <Info />) intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ui/app/workspace/mcp-registry/views/mcpClientForm.tsx`:
- Around line 447-489: The form currently branches on authKind/authScope in
component state and in onSubmit (authKind, authScope, applyAuthKind,
applyAuthScope) which scatters validation logic; replace that imperative
validation with a Zod-backed form schema and resolver: define a Zod schema that
captures authentication rules (authKind: "none" | "headers" | "oauth",
authScope: "shared" | "per_user", and any OAuth-specific fields) with clear
messages and conditional refinements (e.g., require authScope and OAuth fields
only when authKind !== "none"), wire that schema into the form via the form
resolver used by this component (react-hook-form or equivalent), remove the
ad-hoc checks in applyAuthKind/applyAuthScope and onSubmit, and surface schema
errors to the Select/FormLabel UI so validation is centralized and consistent
(also apply the same refactor to the other block referenced at lines ~569-715).
- Around line 661-710: Add stable data-testid attributes to the new OAuth
advanced inputs so E2E can target them: add a data-testid on the Token URL input
for the FormField name "oauth_config.token_url" (e.g.,
mcp-oauth-token-url-input), on the Registration URL input for
"oauth_config.registration_url" (e.g., mcp-oauth-registration-url-input), and on
the Scopes Input bound to the scopesText state (e.g., mcp-oauth-scopes-input),
following the 3-part convention <entity>-<element>-<qualifier>; place the
attributes on the respective <Input> elements in mcpClientForm.tsx.
In `@ui/app/workspace/mcp-registry/views/mcpClientsTable.tsx`:
- Around line 63-75: The Edit menu item currently lacks a stable test selector;
update the DropdownMenuItem rendering (the hasUpdateAccess block that calls
onSelect/onEdit and uses PencilIcon) to include a data-testid prop following the
3-part convention (e.g., "client-edit-menuitem" or "mcpClient-edit-menuitem") so
E2E tests can target this interactive element directly; keep existing onSelect
behavior (preventDefault, onEdit(client), setIsOpen(false)) and only add the
data-testid to DropdownMenuItem.
---
Outside diff comments:
In `@ui/app/workspace/mcp-registry/views/mcpClientsTable.tsx`:
- Around line 364-386: The row-level testids used in mcpClientsTable.tsx are not
qualified per-row; update the data-testid attributes for the TableCell elements
that render per-row (currently "mcp-client-connection-type",
"mcp-client-auth-scope", and "mcp-client-vk-access") to follow the 3-part
convention entity-element-qualifier by appending a stable row qualifier (e.g.,
the client's ID from the row object `c.config.client_id` or another unique
field) — e.g. `mcp-client-connection-type-{c.config.client_id}`,
`mcp-client-auth-scope-{c.config.client_id}`,
`mcp-client-vk-access-{c.config.client_id}`; make the same change pattern where
you render connection/auth display via
getConnectionTypeDisplay/getAuthScopeDisplay and the vk configs count so E2E
tests can target a unique cell per client.
---
Nitpick comments:
In `@ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx`:
- Around line 910-918: The Auto-execute/Agent Mode documentation anchor (<a
href="https://docs.getbifrost.ai/mcp/agent-mode" ...> with the Info icon) is
missing a data-testid; add a data-testid attribute (e.g.,
data-testid="auto-execute-link-help" or similar) to that <a> element in
mcpClientSheet.tsx so it matches the pattern used for the code-mode help link
(data-testid="code-mode-link-help"), keeping the existing attributes (target,
rel, aria-label, className, and nested <Info />) intact.
🪄 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: 217e67ab-1ff7-4d64-99ce-5aa63ec51f90
📒 Files selected for processing (3)
ui/app/workspace/mcp-registry/views/mcpClientForm.tsxui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/app/workspace/mcp-registry/views/mcpClientsTable.tsx
Pratham-Mishra04
force-pushed
the
05-27-fix_mcp_ui_fixes
branch
from
May 27, 2026 08:53
7e3ce46 to
d2d0ca8
Compare
Pratham-Mishra04
force-pushed
the
05-27-docs_mcp_codemode_docs_update
branch
from
May 27, 2026 08:53
f434076 to
9cdca18
Compare
Collaborator
Author
Merge activity
|
Pratham-Mishra04
changed the base branch from
05-27-docs_mcp_codemode_docs_update
to
graphite-base/3799
May 27, 2026 10:59
Pratham-Mishra04
force-pushed
the
05-27-fix_mcp_ui_fixes
branch
from
May 27, 2026 11:02
d2d0ca8 to
816e878
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+64
to
+70
| onCloseAutoFocus={(e) => { | ||
| // Edit opens a Sheet; letting the dropdown restore focus to its | ||
| // trigger fights the Sheet's autofocus and leaves focus outside | ||
| // the dialog — which breaks ESC-to-close. Hand focus off to the | ||
| // Sheet by skipping the dropdown's auto-restore. | ||
| e.preventDefault(); | ||
| }} |
Contributor
There was a problem hiding this comment.
onCloseAutoFocus unconditionally blocks Radix from restoring focus to the trigger button on every dropdown close — including Escape-to-dismiss and outside-click. After this change, a keyboard user who presses Escape to dismiss the menu loses their focus position entirely, breaking sequential keyboard navigation. The intent (letting the Sheet steal focus when Edit is selected) only needs to fire when the Edit item was the one chosen. Track that with a ref (e.g. editSelectedRef) and only call e.preventDefault() when it is set, so normal close paths continue to restore focus.
akshaydeo
pushed a commit
that referenced
this pull request
May 29, 2026
…s and accordion OAuth settings (#3799) ## Summary Improves the MCP client creation and management UI by splitting the single `auth_type` dropdown into separate "Authentication Type" and "Auth Scope" selectors, making the distinction between shared and per-user auth more intuitive. Also improves the MCP clients table with better column organization and replaces the clickable row pattern with an explicit Edit action in the actions menu. ## Changes - The `auth_type` field (which encodes both kind and scope, e.g. `per_user_oauth`) is now represented in the form as two independent dropdowns: **Authentication Type** (`none` / `headers` / `oauth`) and **Auth Scope** (`shared` / `per-user`). These recombine into the existing wire format so the backend contract is unchanged. - OAuth advanced settings (client ID, client secret, authorize URL, token URL, registration URL, scopes) are collapsed into an `Accordion` component to reduce visual noise. - The Connection URL field's tooltip explaining `env.<VAR>` syntax was removed from the label area. - A read-only connection summary block is shown in the edit sheet, since connection type and target cannot be changed after creation. A helper note to that effect is also added to the creation form. - The MCP clients table replaces the "Connection Info" column with a **VK Access** column showing whether the server is available to all virtual keys or a specific count. Auth is now split across two columns: **Auth Type** and **Auth Scope**. - Connection type badges are now rendered with a monospace `Badge` component. - Clicking a table row no longer opens the edit sheet; an explicit **Edit** item (with pencil icon) is added to the row actions dropdown menu. - A tooltip with a link to Agent Mode docs is added to the "Auto-execute" column header in the tools table, clarifying that the setting only applies in Agent Mode. - `getConnectionDisplay` helper removed from the table since connection info is no longer shown as a column. - `authScope` state is reset to `"shared"` when the form is closed/reset. ## Type of change - [ ] Bug fix - [x] Feature - [x] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [x] UI (React) - [ ] Docs ## How to test 1. Open the MCP Registry and click **Add Server**. 2. Select connection type **SSE** or **HTTP**. 3. Verify the **Authentication Type** dropdown shows `None`, `Headers`, and `OAuth 2.0` (no per-user variants). 4. Select `Headers` or `OAuth 2.0` and confirm an **Auth Scope** dropdown appears with `Shared` and `Per-User` options. 5. Confirm that toggling Auth Scope correctly maps to the underlying `auth_type` value (e.g. `per_user_oauth` when OAuth + Per-User). 6. Select `OAuth 2.0` and verify the advanced settings are hidden behind an accordion. 7. Save a client and confirm the table shows separate **Auth Type** and **Auth Scope** columns with correct values. 8. Confirm the **VK Access** column correctly shows `All`, `N VKs`, or `None`. 9. Confirm clicking a table row no longer opens the sheet; use the **⋯** menu → **Edit** instead. 10. Open an existing client's edit sheet and verify the read-only connection summary is displayed. ```sh cd ui pnpm i pnpm build ``` ## Screenshots/Recordings _Before/after screenshots recommended for the form auth dropdowns, the accordion, and the updated table columns._ ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations The `per_user_*` auth types store credentials per user rather than in the shared server config. The refactored UI preserves this behavior — the split dropdowns recombine into the same wire values, so no change to how credentials are stored or transmitted. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
17 tasks
18 tasks
This was referenced Jul 2, 2026
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
2 participants
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
Improves the MCP client creation and management UI by splitting the single
auth_typedropdown into separate "Authentication Type" and "Auth Scope" selectors, making the distinction between shared and per-user auth more intuitive. Also improves the MCP clients table with better column organization and replaces the clickable row pattern with an explicit Edit action in the actions menu.Changes
auth_typefield (which encodes both kind and scope, e.g.per_user_oauth) is now represented in the form as two independent dropdowns: Authentication Type (none/headers/oauth) and Auth Scope (shared/per-user). These recombine into the existing wire format so the backend contract is unchanged.Accordioncomponent to reduce visual noise.env.<VAR>syntax was removed from the label area.Badgecomponent.getConnectionDisplayhelper removed from the table since connection info is no longer shown as a column.authScopestate is reset to"shared"when the form is closed/reset.Type of change
Affected areas
How to test
None,Headers, andOAuth 2.0(no per-user variants).HeadersorOAuth 2.0and confirm an Auth Scope dropdown appears withSharedandPer-Useroptions.auth_typevalue (e.g.per_user_oauthwhen OAuth + Per-User).OAuth 2.0and verify the advanced settings are hidden behind an accordion.All,N VKs, orNone.cd ui pnpm i pnpm buildScreenshots/Recordings
Before/after screenshots recommended for the form auth dropdowns, the accordion, and the updated table columns.
Breaking changes
Related issues
Security considerations
The
per_user_*auth types store credentials per user rather than in the shared server config. The refactored UI preserves this behavior — the split dropdowns recombine into the same wire values, so no change to how credentials are stored or transmitted.Checklist
docs/contributing/README.mdand followed the guidelines