Skip to content

feat(ui-rewrite): add rest of advanced settings in MCP server form#4597

Open
marekdano wants to merge 1 commit intoepic/ui-rewritefrom
4530-add-rest-advanced-settings
Open

feat(ui-rewrite): add rest of advanced settings in MCP server form#4597
marekdano wants to merge 1 commit intoepic/ui-rewritefrom
4530-add-rest-advanced-settings

Conversation

@marekdano
Copy link
Copy Markdown
Collaborator

🔗 Related Issue

Related: #4530


📝 Summary

Add the advanced settings for

  • Bearer token
  • Custom headers
  • OAuth 2.0
  • Query parameter
advanced_settings_mcp_server_form.mov

🏷️ Type of Change

  • Bug fix
  • Feature / Enhancement
  • Documentation
  • Refactor
  • Chore (deps, CI, tooling)
  • Other (describe below)

🧪 Verification

Check Command Status
Lint suite make lint
Unit tests make test
Coverage ≥ 80% make coverage

✅ Checklist

  • Code formatted (make black isort pre-commit)
  • Tests added/updated for changes
  • Documentation updated (if applicable)
  • No secrets or credentials committed

📓 Notes (optional)

Screenshots, design decisions, or additional context.

Signed-off-by: Marek Dano <Marek.Dano@ibm.com>
@marekdano marekdano requested review from a-effort and vishu-bh and removed request for brian-hussey and crivetimihai May 5, 2026 10:03
@marekdano marekdano added ui User Interface ui-rewrite Tasks for the isolated ui rewrite feature branch labels May 5, 2026
@marekdano marekdano self-assigned this May 5, 2026
@a-effort
Copy link
Copy Markdown
Collaborator

a-effort commented May 6, 2026

Good work getting all of this in... it's a BIG form, especially OAuth options! In the Figma, I may need to make adjustments to which fields are optional/not for this particular auth type depending on which options are entered. Will follow up with a proposal on that.

We can fine tune small style things later ofc, but there are a couple of proposed changes to update now:

  • One-time authentication: when toggled on, update text color from muted-foreground to foreground and show inline alert below. Default state should be toggled off (user can toggle on)
  • Token management checkboxes should default to checked (user can uncheck)

-->
🤖 Sonnet feedback:

The auth type switching, accessibility wiring, and the security warning on query params all look good.

A few things worth addressing before this moves toward production:

OAuth2Auth.tsx - mixed controlled/uncontrolled state will silently drop updates
The component initializes local state from props (useState(grantType), useState(issuerUrl), etc.) and then tries to "switch" between local and parent-controlled mode based on whether a callback is present. The problem: useState only reads its initializer once, so if a parent re-renders with different prop values after mount, the component ignores them.

Since MCPServerForm doesn't pass the optional handlers (onGrantTypeChange, onIssuerUrlChange, etc.), those fields are effectively uncontrolled. The fix is to lift all OAuth state up into MCPServerForm like the other auth fields (bearerToken, oauthClientId, etc.) and remove the internal state from OAuth2Auth entirely.

OAuth optional fields not reset on cancel/submit
resetForm() resets oauthClientId, oauthClientSecret, oauthTokenUrl but not grantType, issuerUrl, redirectUri,
authorizationUrl, scopes, storeTokens, autoRefresh since those live inside the component. Closing and reopening the form will show stale values. This is directly caused by the mixed controlled/uncontrolled state will silently drop updates issue.

Worth fixing before production
key={index} in CustomHeadersAuth when mapping headers -- removing a row from the middle will cause React to reuse the wrong DOM nodes. Adding a stable id to each header entry (e.g. assigned in addHeader) fixes this.

CustomHeader is defined both locally in CustomHeadersAuth.tsx and exported from AdvancedSettings.tsx. The local one should import from AdvancedSettings to avoid drift.

customHeaders is initialised with one blank row in MCPServerForm regardless of which auth type is selected. Initialising to [] is cleaner.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ui User Interface ui-rewrite Tasks for the isolated ui rewrite feature branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants