feat(config): add showMcpUI setting to enable/disable MCP UI widgets#494
Open
edgarsskore wants to merge 4 commits into
Open
feat(config): add showMcpUI setting to enable/disable MCP UI widgets#494edgarsskore wants to merge 4 commits into
edgarsskore wants to merge 4 commits into
Conversation
Add a user-editable showMcpUI config field that explicitly controls whether tools advertise interactive UI widgets. Decision priority: explicit override > A/B experiment assignment > default ON. - The decision is resolved once per server process so a session renders consistently; changing the setting takes effect after the client restarts the MCP server (mid-session tools-list flips confuse hosts that bind UI to the cached list). - set_config_value response and the field description note the restart requirement. - Config editor shows the effective decision when the value is unset, so the toggle reflects reality instead of rendering unset as off. - Config editor now shows a quiet inline "Saved" confirmation next to the setting that changed (covers toggles, inputs, shell select, and list modals); errors keep the floating tooltip. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThis PR adds a new boolean ChangesMCP UI Override with Inline Save Feedback
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
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 docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 2
🤖 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 `@src/ui/config-editor/src/app.ts`:
- Line 593: The inline save-status chip (the <span class="setting-save-status"
data-save-status-key="${escapeHtml(entry.key)}" ...>) lacks live-region
semantics; update the span rendered in app.ts (the element that uses class
"setting-save-status" and data-save-status-key) to include role="status",
aria-live="polite", and aria-atomic="true" so assistive tech announces save
confirmations, and apply the same attributes to the other occurrences of the
same span (lines around where controlHtml is injected).
- Around line 642-643: The selector built with
querySelector(`[data-save-status-key="${key}"]`) can break for keys containing
CSS-special characters; update the lookup to escape the key using CSS.escape
before embedding it in the selector so the element is reliably found (i.e.,
replace the plain `${key}` usage when constructing the selector for
data-save-status-key used to set const chip = container.querySelector(...)).
Ensure you reference CSS.escape(key) so the selection logic always returns the
correct HTMLElement or null.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4c5429b6-1fa3-4dd9-b532-09f72ca58612
📒 Files selected for processing (8)
src/config-field-definitions.tssrc/config-manager.tssrc/server.tssrc/tools/config.tssrc/ui/config-editor/src/app.tssrc/ui/styles/apps/config-editor.csssrc/utils/mcp-ui-ab-test.tstest/ab-test.test.js
The inline save-status chip replaced the floating live-region tooltip for success messages, which dropped screen-reader announcements. Mark the chip as a polite atomic status region so saves are announced again. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Keys currently come from CONFIG_FIELD_KEYS and are selector-safe, but the editor accepts arbitrary entry keys from structuredContent, so escape before embedding in querySelector. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
set_config_value already tagged server_call_tool with the key name, so we knew the setting was used but not which way it was set. Tag the boolean value for showMcpUI specifically (no path/PII concern); other config keys stay key-name only.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/server.ts`:
- Around line 1231-1233: The telemetry capture for the showMcpUI configuration
is not validating the input type before stringifying it. Add a type guard to
verify that the value is actually a boolean using typeof check before setting
telemetryData.set_config_value_bool. Only when the value passes the boolean type
check should you then stringify it and assign it to the telemetry field. This
prevents malformed client payloads from leaking arbitrary text into the
telemetry metric.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+1231
to
+1233
| if ((args as any).key === 'showMcpUI') { | ||
| telemetryData.set_config_value_bool = String((args as any).value); | ||
| } |
Contributor
There was a problem hiding this comment.
Guard telemetry capture to booleans only before stringifying.
On Line 1232, String((args as any).value) records any input type for showMcpUI before validation runs. A malformed client payload could leak arbitrary text into telemetry and corrupt the intended boolean metric. Only emit this field when typeof value === 'boolean'.
Suggested patch
- if ((args as any).key === 'showMcpUI') {
- telemetryData.set_config_value_bool = String((args as any).value);
- }
+ if ((args as any).key === 'showMcpUI' && typeof (args as any).value === 'boolean') {
+ telemetryData.set_config_value_bool = String((args as any).value);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if ((args as any).key === 'showMcpUI') { | |
| telemetryData.set_config_value_bool = String((args as any).value); | |
| } | |
| if ((args as any).key === 'showMcpUI' && typeof (args as any).value === 'boolean') { | |
| telemetryData.set_config_value_bool = String((args as any).value); | |
| } |
🤖 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 `@src/server.ts` around lines 1231 - 1233, The telemetry capture for the
showMcpUI configuration is not validating the input type before stringifying it.
Add a type guard to verify that the value is actually a boolean using typeof
check before setting telemetryData.set_config_value_bool. Only when the value
passes the boolean type check should you then stringify it and assign it to the
telemetry field. This prevents malformed client payloads from leaking arbitrary
text into the telemetry metric.
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.
Add a user-editable showMcpUI config field that explicitly controls whether tools advertise interactive UI widgets. Decision priority: explicit override > A/B experiment assignment > default ON.
Summary by CodeRabbit